Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit max. nr. of PersistableNetworkPayload and ProtectedStorageEntries #3562

Merged
merged 6 commits into from
Nov 5, 2019
Merged

Limit max. nr. of PersistableNetworkPayload and ProtectedStorageEntries #3562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a3eaf18
Limit max. nr. of PersistableNetworkPayload and ProtectedStorageEntry…
chimp1984 Nov 5, 2019
3d35179
Add peers node address to logs
chimp1984 Nov 5, 2019
26bf309
Improve logs
chimp1984 Nov 5, 2019
612b131
Log connection UID if not peer address available
chimp1984 Nov 5, 2019
d48c105
Add cleanup code or invalid objects
chimp1984 Nov 5, 2019
0ba7196
Add log
chimp1984 Nov 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions core/src/main/java/bisq/core/filter/FilterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ public void onAllServicesInitialized() {
public void onAdded(ProtectedStorageEntry data) {
if (data.getProtectedStoragePayload() instanceof Filter) {
Filter filter = (Filter) data.getProtectedStoragePayload();
addFilter(filter);
boolean wasValid = addFilter(filter);
if (!wasValid) {
UserThread.runAfter(() -> p2PService.getP2PDataStorage().removeInvalidProtectedStorageEntry(data), 1);
}
}
}

Expand Down Expand Up @@ -203,7 +206,7 @@ private void resetFilters() {
filterProperty.set(null);
}

private void addFilter(Filter filter) {
private boolean addFilter(Filter filter) {
if (verifySignature(filter)) {
// Seed nodes are requested at startup before we get the filter so we only apply the banned
// nodes at the next startup and don't update the list in the P2P network domain.
Expand All @@ -223,6 +226,9 @@ private void addFilter(Filter filter) {
if (filter.isPreventPublicBtcNetwork() &&
preferences.getBitcoinNodesOptionOrdinal() == BtcNodes.BitcoinNodesOption.PUBLIC.ordinal())
preferences.setBitcoinNodesOptionOrdinal(BtcNodes.BitcoinNodesOption.PROVIDED.ordinal());
return true;
} else {
return false;
}
}

Expand Down
31 changes: 31 additions & 0 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,37 @@ && checkSignature(protectedStorageEntry)
return result;
}


/**
* This method must be called only from client code not from network messages! We omit the ownership checks
* so we must apply it only if it comes from our trusted application code. It is used from client code which detects
* that the domain object violates specific domain rules.
* We could make it more generic by adding an Interface with a generic validation method.
*
* @param protectedStorageEntry The entry to be removed
*/
public void removeInvalidProtectedStorageEntry(ProtectedStorageEntry protectedStorageEntry) {
Copy link
Contributor

@julianknutsen julianknutsen Nov 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leads to data structure inconsistency in the event that two consumers listen for the same Payload type and one consumer calls into this function to remove the data. It also breaks encapsulation having the consumers know about the internals of the P2PDataStore. This isn't perfect now, anyway, but moving towards a model where P2PDataStore is private to P2PService may be a good goal.

It seems like a cleaner way to do the same thing would be to have the P2PDataStore validate each stored entry prior to becoming "active" or signaling listeners. The ProtectedStorageEntry and/or payload could implement an interface that would do this check and remove the data before it was every available for consumers.

Is that what you were thinking with the "generic validation method"? Doing this would dovetail well with my existing refactoring and I can probably try it out and see how it looks. I think it would give a lot more flexibility for the future if there are certain Entrys or Payloads that we need to purge but made it past prior validation. Here is just one example I found while testing:

bisq/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java

Lines 96 to 109 in 0ab156f

// TESTCASE: validForAddOperation() should fail if Entry.receiversPubKey and Payload.ownerPubKey don't match
// XXXBUGXXX: The current code doesn't validate this mismatch, but it would create an added payload that could never
// be removed since the remove code requires Entry.receiversPubKey == Payload.ownerPubKey
@Test
public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws NoSuchAlgorithmException, CryptoException {
KeyPair senderKeys = TestUtils.generateKeyPair();
KeyPair receiverKeys = TestUtils.generateKeyPair();
MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic());
ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1);
// should be assertFalse
Assert.assertTrue(protectedStorageEntry.isValidForAddOperation());
}

ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload);

if (!map.containsKey(hashOfPayload)) {
return;
}

doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload);
removeFromProtectedDataStore(protectedStorageEntry);

// We do not update the sequence number as that method is only called if we have received an invalid
// protectedStorageEntry from a previous add operation.

// We do not call maybeAddToRemoveAddOncePayloads to avoid that an invalid object might block a valid object
// which we might receive in future (could be potential attack).

// We do not broadcast as this is a local operation only to avoid our maps get polluted with invalid objects
// and as we do not check for ownership a node would not accept such a procedure if it would come from untrusted
// source (network).
}

private void removeFromProtectedDataStore(ProtectedStorageEntry protectedStorageEntry) {
ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof PersistablePayload) {
Expand Down