diff --git a/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java b/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java index fcbe82760a2..8df36611d58 100644 --- a/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java +++ b/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java @@ -20,6 +20,8 @@ import bisq.common.proto.ProtoResolver; import bisq.common.proto.ProtobufferException; +import java.time.Clock; + public interface NetworkProtoResolver extends ProtoResolver { NetworkEnvelope fromProto(protobuf.NetworkEnvelope proto) throws ProtobufferException; @@ -27,4 +29,6 @@ public interface NetworkProtoResolver extends ProtoResolver { NetworkPayload fromProto(protobuf.StoragePayload proto); NetworkPayload fromProto(protobuf.StorageEntryWrapper proto); + + Clock getClock(); } diff --git a/core/src/main/java/bisq/core/proto/CoreProtoResolver.java b/core/src/main/java/bisq/core/proto/CoreProtoResolver.java index 4799786b7db..b0ce384192b 100644 --- a/core/src/main/java/bisq/core/proto/CoreProtoResolver.java +++ b/core/src/main/java/bisq/core/proto/CoreProtoResolver.java @@ -59,10 +59,16 @@ import bisq.common.proto.ProtobufferRuntimeException; import bisq.common.proto.persistable.PersistableEnvelope; +import java.time.Clock; + +import lombok.Getter; import lombok.extern.slf4j.Slf4j; @Slf4j public class CoreProtoResolver implements ProtoResolver { + @Getter + protected Clock clock; + @Override public PaymentAccountPayload fromProto(protobuf.PaymentAccountPayload proto) { if (proto != null) { diff --git a/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java b/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java index 91791a5ba39..c44ce44e80e 100644 --- a/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java +++ b/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java @@ -88,6 +88,8 @@ import javax.inject.Inject; import javax.inject.Singleton; +import java.time.Clock; + import lombok.extern.slf4j.Slf4j; // TODO Use ProtobufferException instead of ProtobufferRuntimeException @@ -95,7 +97,8 @@ @Singleton public class CoreNetworkProtoResolver extends CoreProtoResolver implements NetworkProtoResolver { @Inject - public CoreNetworkProtoResolver() { + public CoreNetworkProtoResolver(Clock clock) { + this.clock = clock; } @Override diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java b/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java index d72a68767c5..5ae4dc61ac1 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java @@ -49,6 +49,8 @@ import org.springframework.core.env.PropertySource; +import java.time.Clock; + import java.io.File; import java.util.Collections; @@ -118,7 +120,7 @@ protected void execute() { // start the network node networkNode = new TorNetworkNode(Integer.parseInt(configuration.getProperty(TOR_PROXY_PORT, "9053")), - new CoreNetworkProtoResolver(), false, + new CoreNetworkProtoResolver(Clock.systemDefaultZone()), false, new AvailableTor(Monitor.TOR_WORKING_DIR, torHiddenServiceDir.getName())); networkNode.start(this); @@ -139,7 +141,7 @@ public String getProperty(String name) { }); CorruptedDatabaseFilesHandler corruptedDatabaseFilesHandler = new CorruptedDatabaseFilesHandler(); int maxConnections = Integer.parseInt(configuration.getProperty(MAX_CONNECTIONS, "12")); - NetworkProtoResolver networkProtoResolver = new CoreNetworkProtoResolver(); + NetworkProtoResolver networkProtoResolver = new CoreNetworkProtoResolver(Clock.systemDefaultZone()); CorePersistenceProtoResolver persistenceProtoResolver = new CorePersistenceProtoResolver(null, networkProtoResolver, storageDir, corruptedDatabaseFilesHandler); DefaultSeedNodeRepository seedNodeRepository = new DefaultSeedNodeRepository(environment, null); diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java index 2b7fb9097b3..c47820d06e5 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java @@ -39,6 +39,8 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; +import java.time.Clock; + import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -90,7 +92,7 @@ public P2PSeedNodeSnapshotBase(Reporter reporter) { protected void execute() { // start the network node final NetworkNode networkNode = new TorNetworkNode(Integer.parseInt(configuration.getProperty(TOR_PROXY_PORT, "9054")), - new CoreNetworkProtoResolver(), false, + new CoreNetworkProtoResolver(Clock.systemDefaultZone()), false, new AvailableTor(Monitor.TOR_WORKING_DIR, "unused")); // we do not need to start the networkNode, as we do not need the HS //networkNode.start(this); diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index f0a1fcddb75..d90fdeb2565 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PService.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PService.java @@ -700,12 +700,12 @@ public void onBroadcastFailed(String errorMessage) { }; boolean result = p2PDataStorage.addProtectedStorageEntry(protectedMailboxStorageEntry, networkNode.getNodeAddress(), listener, true); if (!result) { - //TODO remove and add again with a delay to ensure the data will be broadcasted - // The p2PDataStorage.remove makes probably sense but need to be analysed more. - // Don't change that if it is not 100% clear. sendMailboxMessageListener.onFault("Data already exists in our local database"); - boolean removeResult = p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true); - log.debug("remove result=" + removeResult); + + // This should only fail if there are concurrent calls to addProtectedStorageEntry with the + // same ProtectedMailboxStorageEntry. This is an unexpected use case so if it happens we + // want to see it, but it is not worth throwing an exception. + log.error("Unexpected state: adding mailbox message that already exists."); } } catch (CryptoException e) { log.error("Signing at getDataWithSignedSeqNr failed. That should never happen."); @@ -759,7 +759,7 @@ private void delayedRemoveEntryFromMailbox(DecryptedMessageWithPubKey decryptedM expirableMailboxStoragePayload, keyRing.getSignatureKeyPair(), receiversPubKey); - p2PDataStorage.removeMailboxData(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true); + p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true); } catch (CryptoException e) { log.error("Signing at getDataWithSignedSeqNr failed. That should never happen."); } diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 1d911896025..98e7671b305 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -67,8 +67,6 @@ import com.google.common.annotations.VisibleForTesting; -import org.apache.commons.lang3.StringUtils; - import java.security.KeyPair; import java.security.PublicKey; @@ -93,12 +91,15 @@ import javax.annotation.Nullable; +import static com.google.common.base.Preconditions.checkArgument; + @Slf4j public class P2PDataStorage implements MessageListener, ConnectionListener, PersistedDataHost { /** * How many days to keep an entry before it is purged. */ - private static final int PURGE_AGE_DAYS = 10; + @VisibleForTesting + public static final int PURGE_AGE_DAYS = 10; @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; @@ -121,6 +122,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); private final Clock clock; + protected int maxSequenceNumberMapSizeBeforePurge; + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor /////////////////////////////////////////////////////////////////////////////////////////// @@ -145,6 +148,7 @@ public P2PDataStorage(NetworkNode networkNode, this.sequenceNumberMapStorage = sequenceNumberMapStorage; sequenceNumberMapStorage.setNumMaxBackupFiles(5); + this.maxSequenceNumberMapSizeBeforePurge = 1000; } @Override @@ -175,40 +179,43 @@ public void shutDown() { removeExpiredEntriesTimer.stop(); } - public void onBootstrapComplete() { - removeExpiredEntriesTimer = UserThread.runPeriodically(() -> { - log.trace("removeExpiredEntries"); - // The moment when an object becomes expired will not be synchronous in the network and we could - // get add network_messages after the object has expired. To avoid repeated additions of already expired - // object when we get it sent from new peers, we don’t remove the sequence number from the map. - // That way an ADD message for an already expired data will fail because the sequence number - // is equal and not larger as expected. - Map temp = new HashMap<>(map); - Set toRemoveSet = new HashSet<>(); - temp.entrySet().stream() - .filter(entry -> entry.getValue().isExpired()) - .forEach(entry -> { - ByteArray hashOfPayload = entry.getKey(); - ProtectedStorageEntry protectedStorageEntry = map.get(hashOfPayload); - if (!(protectedStorageEntry.getProtectedStoragePayload() instanceof PersistableNetworkPayload)) { - toRemoveSet.add(protectedStorageEntry); - log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry)); - map.remove(hashOfPayload); - } - }); + @VisibleForTesting + void removeExpiredEntries() { + log.trace("removeExpiredEntries"); + // The moment when an object becomes expired will not be synchronous in the network and we could + // get add network_messages after the object has expired. To avoid repeated additions of already expired + // object when we get it sent from new peers, we don’t remove the sequence number from the map. + // That way an ADD message for an already expired data will fail because the sequence number + // is equal and not larger as expected. + Map temp = new HashMap<>(map); + Set toRemoveSet = new HashSet<>(); + temp.entrySet().stream() + .filter(entry -> entry.getValue().isExpired(this.clock)) + .forEach(entry -> { + ByteArray hashOfPayload = entry.getKey(); + ProtectedStorageEntry protectedStorageEntry = map.get(hashOfPayload); + if (!(protectedStorageEntry.getProtectedStoragePayload() instanceof PersistableNetworkPayload)) { + toRemoveSet.add(protectedStorageEntry); + log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry)); + map.remove(hashOfPayload); + } + }); - // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying - // about start and end of iteration. - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); - toRemoveSet.forEach(protectedStorageEntry -> { - hashMapChangedListeners.forEach(l -> l.onRemoved(protectedStorageEntry)); - removeFromProtectedDataStore(protectedStorageEntry); - }); - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); + // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying + // about start and end of iteration. + hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); + toRemoveSet.forEach(protectedStorageEntry -> { + hashMapChangedListeners.forEach(l -> l.onRemoved(protectedStorageEntry)); + removeFromProtectedDataStore(protectedStorageEntry); + }); + hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); + + if (sequenceNumberMap.size() > this.maxSequenceNumberMapSizeBeforePurge) + sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); + } - if (sequenceNumberMap.size() > 1000) - sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); - }, CHECK_TTL_INTERVAL_SEC); + public void onBootstrapComplete() { + removeExpiredEntriesTimer = UserThread.runPeriodically(this::removeExpiredEntries, CHECK_TTL_INTERVAL_SEC); } public Map getAppendOnlyDataStoreMap() { @@ -233,7 +240,7 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { } else if (networkEnvelope instanceof RemoveDataMessage) { remove(((RemoveDataMessage) networkEnvelope).getProtectedStorageEntry(), peersNodeAddress, false); } else if (networkEnvelope instanceof RemoveMailboxDataMessage) { - removeMailboxData(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false); + remove(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false); } else if (networkEnvelope instanceof RefreshOfferMessage) { refreshTTL((RefreshOfferMessage) networkEnvelope, peersNodeAddress, false); } else if (networkEnvelope instanceof AddPersistableNetworkPayloadMessage) { @@ -283,7 +290,7 @@ public void onDisconnect(CloseConnectionReason closeConnectionReason, Connection // TODO investigate what causes the disconnections. // Usually the are: SOCKET_TIMEOUT ,TERMINATED (EOFException) protectedStorageEntry.backDate(); - if (protectedStorageEntry.isExpired()) { + if (protectedStorageEntry.isExpired(this.clock)) { log.info("We found an expired data entry which we have already back dated. " + "We remove the protectedStoragePayload:\n\t" + Utilities.toTruncatedString(protectedStorageEntry.getProtectedStoragePayload(), 100)); doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); @@ -314,33 +321,41 @@ public boolean addPersistableNetworkPayload(PersistableNetworkPayload payload, boolean reBroadcast, boolean checkDate) { log.trace("addPersistableNetworkPayload payload={}", payload); - byte[] hash = payload.getHash(); - if (payload.verifyHashSize()) { - ByteArray hashAsByteArray = new ByteArray(hash); - boolean containsKey = getAppendOnlyDataStoreMap().containsKey(hashAsByteArray); - if (!containsKey || reBroadcast) { - if (!(payload instanceof DateTolerantPayload) || !checkDate || ((DateTolerantPayload) payload).isDateInTolerance(clock)) { - if (!containsKey) { - appendOnlyDataStoreService.put(hashAsByteArray, payload); - appendOnlyDataStoreListeners.forEach(e -> e.onAdded(payload)); - } - if (allowBroadcast) - broadcaster.broadcast(new AddPersistableNetworkPayloadMessage(payload), sender, null, isDataOwner); - return true; - } else { - log.warn("Publish date of payload is not matching our current time and outside of our tolerance.\n" + - "Payload={}; now={}", payload.toString(), new Date()); - return false; - } - } else { - log.trace("We have that payload already in our map."); - return false; - } - } else { - log.warn("We got a hash exceeding our permitted size"); + // Payload hash size does not match expectation for that type of message. + if (!payload.verifyHashSize()) { + log.warn("addPersistableNetworkPayload failed due to unexpected hash size"); + return false; + } + + ByteArray hashAsByteArray = new ByteArray(payload.getHash()); + boolean payloadHashAlreadyInStore = getAppendOnlyDataStoreMap().containsKey(hashAsByteArray); + + // Store already knows about this payload. Ignore it unless the caller specifically requests a republish. + if (payloadHashAlreadyInStore && !reBroadcast) { + log.trace("addPersistableNetworkPayload failed due to duplicate payload"); + return false; + } + + // DateTolerantPayloads are only checked for tolerance from the onMessage handler (checkDate == true). If not in + // tolerance, ignore it. + if (checkDate && payload instanceof DateTolerantPayload && !((DateTolerantPayload) payload).isDateInTolerance((clock))) { + log.warn("addPersistableNetworkPayload failed due to payload time outside tolerance.\n" + + "Payload={}; now={}", payload.toString(), new Date()); return false; } + + // Add the payload and publish the state update to the appendOnlyDataStoreListeners + if (!payloadHashAlreadyInStore) { + appendOnlyDataStoreService.put(hashAsByteArray, payload); + appendOnlyDataStoreListeners.forEach(e -> e.onAdded(payload)); + } + + // Broadcast the payload if requested by caller + if (allowBroadcast) + broadcaster.broadcast(new AddPersistableNetworkPayloadMessage(payload), sender, null, isDataOwner); + + return true; } // When we receive initial data we skip several checks to improve performance. We requested only missing entries so we @@ -380,50 +395,41 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - boolean sequenceNrValid = isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - boolean result = sequenceNrValid && - checkPublicKeys(protectedStorageEntry, true) - && checkSignature(protectedStorageEntry); + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + return false; - boolean containsKey = map.containsKey(hashOfPayload); - if (containsKey) { - result = result && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload); - } + // Verify the ProtectedStorageEntry is well formed and valid for the add operation + if (!protectedStorageEntry.isValidForAddOperation()) + return false; - // printData("before add"); - if (result) { - boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); - if (!containsKey || hasSequenceNrIncreased) { - // At startup we don't have the item so we store it. At updates of the seq nr we store as well. - map.put(hashOfPayload, protectedStorageEntry); - hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - // printData("after add"); - } else { - log.trace("We got that version of the data already, so we don't store it."); - } + // If we have already seen an Entry with the same hash, verify the metadata is equal + if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry)) + return false; - if (hasSequenceNrIncreased) { - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); - // We set the delay higher as we might receive a batch of items - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); + // This is an updated entry. Record it and signal listeners. + map.put(hashOfPayload, protectedStorageEntry); + hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - if (allowBroadcast) - broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); - } else { - log.trace("We got that version of the data already, so we don't broadcast it."); - } + // Record the updated sequence number and persist it. Higher delay so we can batch more items. + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); - if (protectedStoragePayload instanceof PersistablePayload) { - ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload); - ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry); - if (previous == null) - protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - } - } else { - log.trace("add failed"); + // Optionally, broadcast the add/update depending on the calling environment + if (allowBroadcast) + broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); + + // Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes + if (protectedStoragePayload instanceof PersistablePayload) { + ByteArray compactHash = P2PDataStorage.getCompactHashAsByteArray(protectedStoragePayload); + ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry); + if (previous == null) + protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); } - return result; + + return true; } private void broadcastProtectedStorageEntry(ProtectedStorageEntry protectedStorageEntry, @@ -436,40 +442,44 @@ private void broadcastProtectedStorageEntry(ProtectedStorageEntry protectedStora public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, @Nullable NodeAddress sender, boolean isDataOwner) { - ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); - if (map.containsKey(hashOfPayload)) { - ProtectedStorageEntry storedData = map.get(hashOfPayload); - int sequenceNumber = refreshTTLMessage.getSequenceNumber(); - if (sequenceNumberMap.containsKey(hashOfPayload) && sequenceNumberMap.get(hashOfPayload).sequenceNr == sequenceNumber) { - log.trace("We got that message with that seq nr already from another peer. We ignore that message."); - return true; - } else { - PublicKey ownerPubKey = storedData.getProtectedStoragePayload().getOwnerPubKey(); - byte[] hashOfDataAndSeqNr = refreshTTLMessage.getHashOfDataAndSeqNr(); - byte[] signature = refreshTTLMessage.getSignature(); - // printData("before refreshTTL"); - if (hasSequenceNrIncreased(sequenceNumber, hashOfPayload) && - checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload) && - checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) { - log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); - storedData.refreshTTL(); - storedData.updateSequenceNumber(sequenceNumber); - storedData.updateSignature(signature); - printData("after refreshTTL"); - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); - - broadcast(refreshTTLMessage, sender, null, isDataOwner); - return true; - } + ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); + ProtectedStorageEntry storedData = map.get(hashOfPayload); - return false; - } - } else { + if (storedData == null) { log.debug("We don't have data for that refresh message in our map. That is expected if we missed the data publishing."); + return false; } + + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + ProtectedStorageEntry updatedEntry = new ProtectedStorageEntry( + storedEntry.getProtectedStoragePayload(), + storedEntry.getOwnerPubKey(), + refreshTTLMessage.getSequenceNumber(), + refreshTTLMessage.getSignature(), + this.clock); + + + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(updatedEntry.getSequenceNumber(), hashOfPayload)) + return false; + + // Verify the updated ProtectedStorageEntry is well formed and valid for update + if (!updatedEntry.isValidForAddOperation()) + return false; + + // Update the hash map with the updated entry + map.put(hashOfPayload, updatedEntry); + + // Record the latest sequence number and persist it + sequenceNumberMap.put(hashOfPayload, new MapValue(updatedEntry.getSequenceNumber(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); + + // Always broadcast refreshes + broadcast(refreshTTLMessage, sender, null, isDataOwner); + + return true; } public boolean remove(ProtectedStorageEntry protectedStorageEntry, @@ -477,32 +487,46 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, boolean isDataOwner) { ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - boolean containsKey = map.containsKey(hashOfPayload); - if (!containsKey) + + // If we don't know about the target of this remove, ignore it + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + if (storedEntry == null) { log.debug("Remove data ignored as we don't have an entry for that data."); - boolean result = containsKey - && checkPublicKeys(protectedStorageEntry, false) - && isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload) - && checkSignature(protectedStorageEntry) - && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload); + return false; + } - // printData("before remove"); - if (result) { - doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); - printData("after remove"); - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + // If we have seen a more recent operation for this payload, ignore this one + if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + return false; - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + // Verify the ProtectedStorageEntry is well formed and valid for the remove operation + if (!protectedStorageEntry.isValidForRemoveOperation()) + return false; - broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + // If we have already seen an Entry with the same hash, verify the metadata is the same + if (!protectedStorageEntry.matchesRelevantPubKey(storedEntry)) + return false; - removeFromProtectedDataStore(protectedStorageEntry); + // Valid remove entry, do the remove and signal listeners + doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); + printData("after remove"); + + // Record the latest sequence number and persist it + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + + maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) { + broadcast(new RemoveMailboxDataMessage((ProtectedMailboxStorageEntry) protectedStorageEntry), sender, null, isDataOwner); } else { - log.debug("remove failed"); + broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); } - return result; - } + + removeFromProtectedDataStore(protectedStorageEntry); + + return true; +} /** @@ -548,42 +572,7 @@ private void removeFromProtectedDataStore(ProtectedStorageEntry protectedStorage } } } - - @SuppressWarnings("UnusedReturnValue") - public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxStorageEntry, - @Nullable NodeAddress sender, - boolean isDataOwner) { - ProtectedStoragePayload protectedStoragePayload = protectedMailboxStorageEntry.getProtectedStoragePayload(); - ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - boolean containsKey = map.containsKey(hashOfPayload); - if (!containsKey) - log.debug("Remove data ignored as we don't have an entry for that data."); - - int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); - PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - boolean result = containsKey && - isSequenceNrValid(sequenceNumber, hashOfPayload) && - checkPublicKeys(protectedMailboxStorageEntry, false) && - protectedMailboxStorageEntry.getMailboxStoragePayload().getOwnerPubKey().equals(receiversPubKey) && // at remove both keys are the same (only receiver is able to remove data) - checkSignature(protectedMailboxStorageEntry) && - checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload); - - // printData("before removeMailboxData"); - if (result) { - doRemoveProtectedExpirableData(protectedMailboxStorageEntry, hashOfPayload); - printData("after removeMailboxData"); - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); - - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); - - broadcast(new RemoveMailboxDataMessage(protectedMailboxStorageEntry), sender, null, isDataOwner); - } else { - log.debug("removeMailboxData failed"); - } - return result; - } - + private void maybeAddToRemoveAddOncePayloads(ProtectedStoragePayload protectedStoragePayload, ByteArray hashOfData) { if (protectedStoragePayload instanceof AddOncePayload) { @@ -603,7 +592,7 @@ public ProtectedStorageEntry getProtectedStorageEntry(ProtectedStoragePayload pr byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(ownerStoragePubKey.getPrivate(), hashOfDataAndSeqNr); - return new ProtectedStorageEntry(protectedStoragePayload, ownerStoragePubKey.getPublic(), sequenceNumber, signature); + return new ProtectedStorageEntry(protectedStoragePayload, ownerStoragePubKey.getPublic(), sequenceNumber, signature, this.clock); } public RefreshOfferMessage getRefreshTTLMessage(ProtectedStoragePayload protectedStoragePayload, @@ -635,7 +624,7 @@ public ProtectedMailboxStorageEntry getMailboxDataWithSignedSeqNr(MailboxStorage byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(expirableMailboxStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(storageSignaturePubKey.getPrivate(), hashOfDataAndSeqNr); return new ProtectedMailboxStorageEntry(expirableMailboxStoragePayload, - storageSignaturePubKey.getPublic(), sequenceNumber, signature, receiversPublicKey); + storageSignaturePubKey.getPublic(), sequenceNumber, signature, receiversPublicKey, this.clock); } public void addHashMapChangedListener(HashMapChangedListener hashMapChangedListener) { @@ -676,24 +665,6 @@ private void doRemoveProtectedExpirableData(ProtectedStorageEntry protectedStora hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); } - private boolean isSequenceNrValid(int newSequenceNumber, ByteArray hashOfData) { - if (sequenceNumberMap.containsKey(hashOfData)) { - int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; - if (newSequenceNumber >= storedSequenceNumber) { - log.trace("Sequence number is valid (>=). sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber); - return true; - } else { - log.debug("Sequence number is invalid. sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber + "\n" + - "That can happen if the data owner gets an old delayed data storage message."); - return false; - } - } else { - return true; - } - } - private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) { if (sequenceNumberMap.containsKey(hashOfData)) { int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; @@ -737,72 +708,6 @@ private boolean checkSignature(PublicKey ownerPubKey, byte[] hashOfDataAndSeqNr, } } - private boolean checkSignature(ProtectedStorageEntry protectedStorageEntry) { - byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(protectedStorageEntry.getProtectedStoragePayload(), protectedStorageEntry.getSequenceNumber())); - return checkSignature(protectedStorageEntry.getOwnerPubKey(), hashOfDataAndSeqNr, protectedStorageEntry.getSignature()); - } - - // Check that the pubkey of the storage entry matches the allowed pubkey for the addition or removal operation - // in the contained mailbox message, or the pubKey of other kinds of network_messages. - private boolean checkPublicKeys(ProtectedStorageEntry protectedStorageEntry, boolean isAddOperation) { - boolean result; - ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof MailboxStoragePayload) { - MailboxStoragePayload payload = (MailboxStoragePayload) protectedStoragePayload; - if (isAddOperation) - result = payload.getSenderPubKeyForAddOperation() != null && - payload.getSenderPubKeyForAddOperation().equals(protectedStorageEntry.getOwnerPubKey()); - else - result = payload.getOwnerPubKey() != null && - payload.getOwnerPubKey().equals(protectedStorageEntry.getOwnerPubKey()); - } else { - result = protectedStorageEntry.getOwnerPubKey() != null && - protectedStoragePayload != null && - protectedStorageEntry.getOwnerPubKey().equals(protectedStoragePayload.getOwnerPubKey()); - } - - if (!result) { - String res1 = protectedStorageEntry.toString(); - String res2 = "null"; - if (protectedStoragePayload != null && - protectedStoragePayload.getOwnerPubKey() != null) - res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); - - log.warn("PublicKey of payload data and ProtectedStorageEntry are not matching. protectedStorageEntry=" + res1 + - "protectedStorageEntry.getStoragePayload().getOwnerPubKey()=" + res2); - } - return result; - } - - private boolean checkIfStoredDataPubKeyMatchesNewDataPubKey(PublicKey ownerPubKey, ByteArray hashOfData) { - ProtectedStorageEntry storedData = map.get(hashOfData); - boolean result = storedData.getOwnerPubKey() != null && storedData.getOwnerPubKey().equals(ownerPubKey); - if (!result) - log.warn("New data entry does not match our stored data. storedData.ownerPubKey=" + - (storedData.getOwnerPubKey() != null ? storedData.getOwnerPubKey().toString() : "null") + - ", ownerPubKey=" + ownerPubKey); - - return result; - } - - private boolean checkIfStoredMailboxDataMatchesNewMailboxData(PublicKey receiversPubKey, ByteArray hashOfData) { - ProtectedStorageEntry storedData = map.get(hashOfData); - if (storedData instanceof ProtectedMailboxStorageEntry) { - ProtectedMailboxStorageEntry entry = (ProtectedMailboxStorageEntry) storedData; - // publicKey is not the same (stored: sender, new: receiver) - boolean result = entry.getReceiversPubKey().equals(receiversPubKey) - && get32ByteHashAsByteArray(entry.getProtectedStoragePayload()).equals(hashOfData); - if (!result) - log.warn("New data entry does not match our stored data. entry.receiversPubKey=" + entry.getReceiversPubKey() - + ", receiversPubKey=" + receiversPubKey); - - return result; - } else { - log.error("We expected a MailboxData but got other type. That must never happen. storedData=" + storedData); - return false; - } - } - private void broadcast(BroadcastMessage message, @Nullable NodeAddress sender, @Nullable BroadcastHandler.Listener listener, boolean isDataOwner) { broadcaster.broadcast(message, sender, listener, isDataOwner); @@ -823,7 +728,7 @@ private static byte[] getCompactHash(ProtectedStoragePayload protectedStoragePay // Get a new map with entries older than PURGE_AGE_DAYS purged from the given map. private Map getPurgedSequenceNumberMap(Map persisted) { Map purged = new HashMap<>(); - long maxAgeTs = System.currentTimeMillis() - TimeUnit.DAYS.toMillis(PURGE_AGE_DAYS); + long maxAgeTs = this.clock.millis() - TimeUnit.DAYS.toMillis(PURGE_AGE_DAYS); persisted.forEach((key, value) -> { if (value.timeStamp > maxAgeTs) purged.put(key, value); diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java index 509f45645f3..f0d51ef6108 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java @@ -25,6 +25,8 @@ import java.security.PublicKey; +import java.time.Clock; + import lombok.EqualsAndHashCode; import lombok.Value; import lombok.extern.slf4j.Slf4j; @@ -40,38 +42,155 @@ public ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, PublicKey ownerPubKey, int sequenceNumber, byte[] signature, - PublicKey receiversPubKey) { - super(mailboxStoragePayload, ownerPubKey, sequenceNumber, signature); + PublicKey receiversPubKey, + Clock clock) { + this(mailboxStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + Sig.getPublicKeyBytes(receiversPubKey), + receiversPubKey, + clock.millis(), + clock); + } + + private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, + byte[] ownerPubKeyBytes, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature, + byte[] receiversPubKeyBytes, + PublicKey receiversPubKey, + long creationTimeStamp, + Clock clock) { + super(mailboxStoragePayload, + ownerPubKeyBytes, + ownerPubKey, + sequenceNumber, + signature, + creationTimeStamp, + clock); this.receiversPubKey = receiversPubKey; - receiversPubKeyBytes = Sig.getPublicKeyBytes(receiversPubKey); + this.receiversPubKeyBytes = receiversPubKeyBytes; } + /////////////////////////////////////////////////////////////////////////////////////////// + // API + /////////////////////////////////////////////////////////////////////////////////////////// + public MailboxStoragePayload getMailboxStoragePayload() { return (MailboxStoragePayload) getProtectedStoragePayload(); } + /* + * Returns true if this Entry is valid for an add operation. For mailbox Entrys, the entry owner must + * match the valid sender Public Key specified in the payload. (Only sender can add) + */ + @Override + public boolean isValidForAddOperation() { + if (!this.isSignatureValid()) + return false; + + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); + boolean result = mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && + mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(mailboxStoragePayload.getSenderPubKeyForAddOperation().getEncoded(),true); + + log.warn("ProtectedMailboxStorageEntry::isValidForAddOperation() failed. " + + "Entry owner does not match sender key in payload:\nProtectedStorageEntry=%{}\n" + + "SenderPubKeyForAddOperation=%{}", res1, res2); + } + + return result; + } + + /* + * Returns true if the Entry is valid for a remove operation. For mailbox Entrys, the entry owner must + * match the payload owner. (Only receiver can remove) + */ + @Override + public boolean isValidForRemoveOperation() { + if (!this.isSignatureValid()) + return false; + + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); + + // Verify the Entry has the correct receiversPubKey for removal + if (!mailboxStoragePayload.getOwnerPubKey().equals(this.receiversPubKey)) { + log.debug("Entry receiversPubKey does not match payload owner which is a requirement for removing MailboxStoragePayloads"); + return false; + } + + boolean result = mailboxStoragePayload.getOwnerPubKey() != null && + mailboxStoragePayload.getOwnerPubKey().equals(this.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(mailboxStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn("ProtectedMailboxStorageEntry::isValidForRemoveOperation() failed. " + + "Entry owner does not match Payload owner:\nProtectedStorageEntry={}\n" + + "PayloadOwner={}", res1, res2); + } + + return result; + } + + @Override + /* + * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object + * matches. For ProtectedMailboxStorageEntry, the receiversPubKey must stay the same. + */ + public boolean matchesRelevantPubKey(ProtectedStorageEntry protectedStorageEntry) { + if (!(protectedStorageEntry instanceof ProtectedMailboxStorageEntry)) { + log.error("ProtectedMailboxStorageEntry::isMetadataEquals() failed due to object type mismatch. " + + "ProtectedMailboxStorageEntry required, but got\n" + protectedStorageEntry); + + return false; + } + + ProtectedMailboxStorageEntry protectedMailboxStorageEntry = (ProtectedMailboxStorageEntry) protectedStorageEntry; + + boolean result = protectedMailboxStorageEntry.getReceiversPubKey().equals(this.receiversPubKey); + if (!result) { + log.warn("ProtectedMailboxStorageEntry::isMetadataEquals() failed due to metadata mismatch. " + + "new.receiversPubKey=" + Utilities.bytesAsHexString(protectedMailboxStorageEntry.getReceiversPubKeyBytes()) + + "stored.receiversPubKey=" + Utilities.bytesAsHexString(this.getReceiversPubKeyBytes())); + } + + return result; + } + /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER /////////////////////////////////////////////////////////////////////////////////////////// - private ProtectedMailboxStorageEntry(long creationTimeStamp, - MailboxStoragePayload mailboxStoragePayload, - byte[] ownerPubKey, + private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, + byte[] ownerPubKeyBytes, int sequenceNumber, byte[] signature, - byte[] receiversPubKeyBytes) { - super(creationTimeStamp, - mailboxStoragePayload, - ownerPubKey, + byte[] receiversPubKeyBytes, + long creationTimeStamp, + Clock clock) { + this(mailboxStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), sequenceNumber, - signature); - - this.receiversPubKeyBytes = receiversPubKeyBytes; - receiversPubKey = Sig.getPublicKeyFromBytes(receiversPubKeyBytes); - - maybeAdjustCreationTimeStamp(); + signature, + receiversPubKeyBytes, + Sig.getPublicKeyFromBytes(receiversPubKeyBytes), + creationTimeStamp, + clock); } public protobuf.ProtectedMailboxStorageEntry toProtoMessage() { @@ -85,20 +204,20 @@ public static ProtectedMailboxStorageEntry fromProto(protobuf.ProtectedMailboxSt NetworkProtoResolver resolver) { ProtectedStorageEntry entry = ProtectedStorageEntry.fromProto(proto.getEntry(), resolver); return new ProtectedMailboxStorageEntry( - entry.getCreationTimeStamp(), (MailboxStoragePayload) entry.getProtectedStoragePayload(), entry.getOwnerPubKey().getEncoded(), entry.getSequenceNumber(), entry.getSignature(), - proto.getReceiversPubKeyBytes().toByteArray()); + proto.getReceiversPubKeyBytes().toByteArray(), + entry.getCreationTimeStamp(), + resolver.getClock()); } @Override public String toString() { return "ProtectedMailboxStorageEntry{" + - "\n receiversPubKeyBytes=" + Utilities.bytesAsHexString(receiversPubKeyBytes) + - ",\n receiversPubKey=" + receiversPubKey + - "\n} " + super.toString(); + "\n\tReceivers Public Key: " + Utilities.bytesAsHexString(receiversPubKeyBytes) + + "\n" + super.toString(); } } diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java index b967caf594f..a055e101018 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java @@ -17,16 +17,22 @@ package bisq.network.p2p.storage.payload; +import bisq.network.p2p.storage.P2PDataStorage; + +import bisq.common.crypto.CryptoException; import bisq.common.crypto.Sig; import bisq.common.proto.network.NetworkPayload; import bisq.common.proto.network.NetworkProtoResolver; import bisq.common.proto.persistable.PersistablePayload; +import bisq.common.util.Utilities; import com.google.protobuf.ByteString; import com.google.protobuf.Message; import java.security.PublicKey; +import java.time.Clock; + import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -38,42 +44,60 @@ public class ProtectedStorageEntry implements NetworkPayload, PersistablePayload private final ProtectedStoragePayload protectedStoragePayload; private final byte[] ownerPubKeyBytes; transient private final PublicKey ownerPubKey; - private int sequenceNumber; + private final int sequenceNumber; private byte[] signature; private long creationTimeStamp; public ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature, + Clock clock) { + this(protectedStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + clock.millis(), + clock); + } + + protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + byte[] ownerPubKeyBytes, PublicKey ownerPubKey, int sequenceNumber, - byte[] signature) { + byte[] signature, + long creationTimeStamp, + Clock clock) { + this.protectedStoragePayload = protectedStoragePayload; - ownerPubKeyBytes = Sig.getPublicKeyBytes(ownerPubKey); + this.ownerPubKeyBytes = ownerPubKeyBytes; this.ownerPubKey = ownerPubKey; this.sequenceNumber = sequenceNumber; this.signature = signature; - this.creationTimeStamp = System.currentTimeMillis(); - } + this.creationTimeStamp = creationTimeStamp; + maybeAdjustCreationTimeStamp(clock); + } /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER /////////////////////////////////////////////////////////////////////////////////////////// - protected ProtectedStorageEntry(long creationTimeStamp, - ProtectedStoragePayload protectedStoragePayload, + private ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, byte[] ownerPubKeyBytes, int sequenceNumber, - byte[] signature) { - this.protectedStoragePayload = protectedStoragePayload; - this.ownerPubKeyBytes = ownerPubKeyBytes; - ownerPubKey = Sig.getPublicKeyFromBytes(ownerPubKeyBytes); - - this.sequenceNumber = sequenceNumber; - this.signature = signature; - this.creationTimeStamp = creationTimeStamp; - - maybeAdjustCreationTimeStamp(); + byte[] signature, + long creationTimeStamp, + Clock clock) { + this(protectedStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), + sequenceNumber, + signature, + creationTimeStamp, + clock); } public Message toProtoMessage() { @@ -93,11 +117,13 @@ public protobuf.ProtectedStorageEntry toProtectedStorageEntry() { public static ProtectedStorageEntry fromProto(protobuf.ProtectedStorageEntry proto, NetworkProtoResolver resolver) { - return new ProtectedStorageEntry(proto.getCreationTimeStamp(), + return new ProtectedStorageEntry( ProtectedStoragePayload.fromProto(proto.getStoragePayload(), resolver), proto.getOwnerPubKeyBytes().toByteArray(), proto.getSequenceNumber(), - proto.getSignature().toByteArray()); + proto.getSignature().toByteArray(), + proto.getCreationTimeStamp(), + resolver.getClock()); } @@ -105,14 +131,10 @@ public static ProtectedStorageEntry fromProto(protobuf.ProtectedStorageEntry pro // API /////////////////////////////////////////////////////////////////////////////////////////// - public void maybeAdjustCreationTimeStamp() { + public void maybeAdjustCreationTimeStamp(Clock clock) { // We don't allow creation date in the future, but we cannot be too strict as clocks are not synced - if (creationTimeStamp > System.currentTimeMillis()) - creationTimeStamp = System.currentTimeMillis(); - } - - public void refreshTTL() { - creationTimeStamp = System.currentTimeMillis(); + if (creationTimeStamp > clock.millis()) + creationTimeStamp = clock.millis(); } public void backDate() { @@ -120,16 +142,115 @@ public void backDate() { creationTimeStamp -= ((ExpirablePayload) protectedStoragePayload).getTTL() / 2; } - public void updateSequenceNumber(int sequenceNumber) { - this.sequenceNumber = sequenceNumber; - } - + // TODO: only used in tests so find a better way to test and delete public API public void updateSignature(byte[] signature) { this.signature = signature; } - public boolean isExpired() { + public boolean isExpired(Clock clock) { return protectedStoragePayload instanceof ExpirablePayload && - (System.currentTimeMillis() - creationTimeStamp) > ((ExpirablePayload) protectedStoragePayload).getTTL(); + (clock.millis() - creationTimeStamp) > ((ExpirablePayload) protectedStoragePayload).getTTL(); + } + + /* + * Returns true if the Entry is valid for an add operation. For non-mailbox Entrys, the entry owner must + * match the payload owner. + */ + public boolean isValidForAddOperation() { + if (!this.isSignatureValid()) + return false; + + // TODO: The code currently supports MailboxStoragePayload objects inside ProtectedStorageEntry. Fix this. + if (protectedStoragePayload instanceof MailboxStoragePayload) { + MailboxStoragePayload mailboxStoragePayload = (MailboxStoragePayload) this.getProtectedStoragePayload(); + return mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && + mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); + + } else { + boolean result = this.ownerPubKey != null && + this.protectedStoragePayload != null && + this.ownerPubKey.equals(protectedStoragePayload.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn("ProtectedStorageEntry::isValidForAddOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry={}\nPayloadOwner={}", res1, res2); + } + + return result; + } + } + + /* + * Returns true if the Entry is valid for a remove operation. For non-mailbox Entrys, the entry owner must + * match the payload owner. + */ + public boolean isValidForRemoveOperation() { + + // Same requirements as add() + boolean result = this.isValidForAddOperation(); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn("ProtectedStorageEntry::isValidForRemoveOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry={}\nPayloadOwner={}", res1, res2); + } + + return result; + } + + /* + * Returns true if the signature for the Entry is valid for the payload, sequence number, and ownerPubKey + */ + boolean isSignatureValid() { + try { + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash( + new P2PDataStorage.DataAndSeqNrPair(this.protectedStoragePayload, this.sequenceNumber)); + + boolean result = Sig.verify(this.ownerPubKey, hashOfDataAndSeqNr, this.signature); + + if (!result) + log.warn("ProtectedStorageEntry::isSignatureValid() failed.\n{}}", this); + + return result; + } catch (CryptoException e) { + log.error("ProtectedStorageEntry::isSignatureValid() exception {}", e.toString()); + return false; + } + } + + /* + * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object + * matches. + */ + public boolean matchesRelevantPubKey(ProtectedStorageEntry protectedStorageEntry) { + boolean result = protectedStorageEntry.getOwnerPubKey().equals(this.ownerPubKey); + + if (!result) { + log.warn("New data entry does not match our stored data. storedData.ownerPubKey=" + + (protectedStorageEntry.getOwnerPubKey() != null ? protectedStorageEntry.getOwnerPubKey().toString() : "null") + + ", ownerPubKey=" + this.ownerPubKey); + } + + return result; + } + + @Override + public String toString() { + return "ProtectedStorageEntry {" + + "\n\tPayload: " + protectedStoragePayload + + "\n\tOwner Public Key: " + Utilities.bytesAsHexString(this.ownerPubKeyBytes) + + "\n\tSequence Number: " + this.sequenceNumber + + "\n\tSignature: " + Utilities.bytesAsHexString(this.signature) + + "\n\tTimestamp: " + this.creationTimeStamp + + "\n} "; } } diff --git a/p2p/src/test/java/bisq/network/p2p/TestUtils.java b/p2p/src/test/java/bisq/network/p2p/TestUtils.java index b483fde4b3c..87d42b20cd2 100644 --- a/p2p/src/test/java/bisq/network/p2p/TestUtils.java +++ b/p2p/src/test/java/bisq/network/p2p/TestUtils.java @@ -27,6 +27,8 @@ import java.security.KeyPairGenerator; import java.security.NoSuchAlgorithmException; +import java.time.Clock; + import java.util.Set; import java.util.concurrent.CountDownLatch; @@ -184,6 +186,9 @@ public NetworkPayload fromProto(protobuf.StoragePayload proto) { public NetworkPayload fromProto(protobuf.StorageEntryWrapper proto) { return null; } + + @Override + public Clock getClock() { return null; } }; } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java index 41964e2715c..0586350296c 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -24,7 +24,9 @@ import bisq.network.p2p.storage.payload.ProtectedStoragePayload; import bisq.network.p2p.storage.payload.RequiresOwnerIsOnlinePayload; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; +import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService; import bisq.network.p2p.storage.persistence.ProtectedDataStoreListener; +import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; import bisq.network.p2p.storage.persistence.ResourceDataStoreService; import bisq.network.p2p.storage.persistence.SequenceNumberMap; @@ -67,13 +69,13 @@ public class P2PDataStorageTest { static class ExpirableProtectedStoragePayload extends ProtectedStoragePayloadStub implements ExpirablePayload, RequiresOwnerIsOnlinePayload { private long ttl; - ExpirableProtectedStoragePayload(KeyPair ownerKeys) { - super(ownerKeys.getPublic()); + ExpirableProtectedStoragePayload(PublicKey ownerPubKey) { + super(ownerPubKey); ttl = TimeUnit.DAYS.toMillis(90); } - ExpirableProtectedStoragePayload(KeyPair ownerKeys, long ttl) { - this(ownerKeys); + ExpirableProtectedStoragePayload(PublicKey ownerPubKey, long ttl) { + this(ownerPubKey); this.ttl = ttl; } @@ -88,6 +90,17 @@ public long getTTL() { } } + static class PersistableExpirableProtectedStoragePayload extends ExpirableProtectedStoragePayload implements PersistablePayload { + + PersistableExpirableProtectedStoragePayload(PublicKey ownerPubKey) { + super(ownerPubKey); + } + + PersistableExpirableProtectedStoragePayload(PublicKey ownerPubKey, long ttl) { + super(ownerPubKey, ttl); + } + } + // Common state for tests that initializes the P2PDataStore and mocks out the dependencies. Allows // shared state verification between all tests. static class TestState { @@ -98,16 +111,36 @@ static class TestState { final ProtectedDataStoreListener protectedDataStoreListener; final HashMapChangedListener hashMapChangedListener; final Storage mockSeqNrStorage; + final ClockFake clockFake; + + /** + * Subclass of P2PDataStorage that allows for easier testing, but keeps all functionality + */ + static class P2PDataStorageForTest extends P2PDataStorage { + + P2PDataStorageForTest(NetworkNode networkNode, + Broadcaster broadcaster, + AppendOnlyDataStoreService appendOnlyDataStoreService, + ProtectedDataStoreService protectedDataStoreService, + ResourceDataStoreService resourceDataStoreService, + Storage sequenceNumberMapStorage, + Clock clock) { + super(networkNode, broadcaster, appendOnlyDataStoreService, protectedDataStoreService, resourceDataStoreService, sequenceNumberMapStorage, clock); + + this.maxSequenceNumberMapSizeBeforePurge = 5; + } + } TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); + this.clockFake = new ClockFake(); - this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), + this.mockedStorage = new P2PDataStorageForTest(mock(NetworkNode.class), this.mockBroadcaster, new AppendOnlyDataStoreServiceFake(), new ProtectedDataStoreServiceFake(), mock(ResourceDataStoreService.class), - this.mockSeqNrStorage, Clock.systemUTC()); + this.mockSeqNrStorage, this.clockFake); this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); @@ -125,6 +158,10 @@ void resetState() { reset(this.hashMapChangedListener); reset(this.mockSeqNrStorage); } + + void incrementClock() { + this.clockFake.increment(TimeUnit.HOURS.toMillis(1)); + } } // Represents a snapshot of a TestState allowing easier verification of state before and after an operation. @@ -187,11 +224,12 @@ private static ProtectedStorageEntry buildProtectedStorageEntry( ProtectedStoragePayload protectedStoragePayload, KeyPair entryOwnerKeys, KeyPair entrySignerKeys, - int sequenceNumber) throws CryptoException { + int sequenceNumber, + Clock clock) throws CryptoException { byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(entrySignerKeys.getPrivate(), hashOfDataAndSeqNr); - return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature); + return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature, clock); } private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, @@ -213,14 +251,15 @@ private static ProtectedStorageEntry buildProtectedMailboxStorageEntry( PrivateKey entrySigner, PublicKey entryOwnerPubKey, PublicKey entryReceiversPubKey, - int sequenceNumber) throws CryptoException { + int sequenceNumber, + Clock clock) throws CryptoException { MailboxStoragePayload payload = buildMailboxStoragePayload(payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(payload, sequenceNumber)); byte[] signature = Sig.sign(entrySigner, hashOfDataAndSeqNr); return new ProtectedMailboxStorageEntry(payload, - entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey); + entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey, clock); } private static RefreshOfferMessage buildRefreshOfferMessage(ProtectedStoragePayload protectedStoragePayload, @@ -333,6 +372,8 @@ private static void verifyProtectedStorageRemove(TestState currentState, SavedTestState beforeState, ProtectedStorageEntry protectedStorageEntry, boolean expectedStateChange, + boolean expectedBroadcastOnStateChange, + boolean expectedSeqNrWriteOnStateChange, boolean expectedIsDataOwner) { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); @@ -348,9 +389,16 @@ private static void verifyProtectedStorageRemove(TestState currentState, verify(currentState.hashMapChangedListener).onRemoved(protectedStorageEntry); - verify(currentState.mockBroadcaster).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + if (expectedSeqNrWriteOnStateChange) + verifySequenceNumberMapWriteContains(currentState, P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + + if (expectedBroadcastOnStateChange) { + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) + verify(currentState.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + else + verify(currentState.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + } - verifySequenceNumberMapWriteContains(currentState, P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, currentState.mockedStorage.getMap().get(hashMapHash)); @@ -689,7 +737,7 @@ boolean doRefreshTTL(RefreshOfferMessage refreshOfferMessage) { ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws CryptoException { // Entry signed and owned by same owner as payload - return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber); + return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber, this.testState.clockFake); } // Return a ProtectedStorageEntry that is valid for remove. @@ -697,7 +745,7 @@ ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws ProtectedStorageEntry getProtectedStorageEntryForRemove(int sequenceNumber) throws CryptoException { // Entry signed and owned by same owner as payload - return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber); + return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber, this.testState.clockFake); } void doProtectedStorageAddAndVerify(ProtectedStorageEntry protectedStorageEntry, @@ -725,7 +773,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - verifyProtectedStorageRemove(this.testState, beforeState, entry, expectInternalStateChange, this.expectIsDataOwner()); + verifyProtectedStorageRemove(this.testState, beforeState, entry, expectInternalStateChange, true, true, this.expectIsDataOwner()); } // TESTCASE: Adding a well-formed entry is successful @@ -737,12 +785,11 @@ public void addProtectedStorageEntry() throws CryptoException { } // TESTCASE: Adding duplicate payload w/ same sequence number - // TODO: Should adds() of existing sequence #s return false since they don't update state? @Test public void addProtectedStorageEntry_duplicateSeqNrGt0() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageAddAndVerify(entryForAdd, true, false); + doProtectedStorageAddAndVerify(entryForAdd, false, false); } // TESTCASE: Adding duplicate payload w/ 0 sequence number (special branch in code for logging) @@ -750,7 +797,7 @@ public void addProtectedStorageEntry_duplicateSeqNrGt0() throws CryptoException public void addProtectedStorageEntry_duplicateSeqNrEq0() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(0); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageAddAndVerify(entryForAdd, true, false); + doProtectedStorageAddAndVerify(entryForAdd, false, false); } // TESTCASE: Adding duplicate payload for w/ lower sequence number @@ -772,21 +819,17 @@ public void addProtectedStorageEntry_greaterSeqNr() throws CryptoException { } // TESTCASE: Add w/ same sequence number after remove of sequence number - // XXXBUGXXX: Since removes aren't required to increase the sequence number, duplicate adds - // can occur that will cause listeners to be signaled. Any well-intentioned nodes will create remove messages - // that increment the seq #, but this may just fall into a larger effort to protect against malicious nodes. -/* @Test + // Regression test for old remove() behavior that succeeded if add.seq# == remove.seq# + @Test public void addProtectectedStorageEntry_afterRemoveSameSeqNr() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false); - // Should be false, false. Instead, the hashmap is updated and hashmap listeners are signaled. - // Broadcast isn't called doProtectedStorageAddAndVerify(entryForAdd, false, false); - }*/ + } // TESTCASE: Entry signature does not match entry owner @Test @@ -804,7 +847,7 @@ public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatible() throw // For standard ProtectedStorageEntrys the entry owner must match the payload owner for adds ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 1); + this.protectedStoragePayload, notOwner, notOwner, 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -819,8 +862,6 @@ public void addProtectedStorageEntry_PayloadHashCollision_Fails() { }*/ // TESTCASE: Removing an item after successfully added (remove seq # == add seq #) - // XXXBUGXXX A state change shouldn't occur. Any well-intentioned nodes will create remove messages - // that increment the seq #, but this may just fall into a larger effort to protect against malicious nodes. @Test public void remove_seqNrEqAddSeqNr() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); @@ -828,8 +869,7 @@ public void remove_seqNrEqAddSeqNr() throws CryptoException { doProtectedStorageAddAndVerify(entryForAdd, true, true); - // should be (false, false) - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } // TESTCASE: Removing an item after successfully added (remove seq # > add seq #) @@ -866,7 +906,7 @@ public void remove_invalidEntrySig() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); entryForRemove.updateSignature(new byte[] { 0 }); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -881,7 +921,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE // For standard ProtectedStorageEntrys the entry owner must match the payload owner for removes ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 1); + this.protectedStoragePayload, notOwner, notOwner, 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -977,7 +1017,7 @@ public void refreshTTL_existingEntry() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,1), true, false); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,1), false, false); } // TESTCASE: Duplicate refresh message (same seq #) @@ -986,8 +1026,13 @@ public void refreshTTL_duplicateRefreshSeqNrEqual() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, false); + + this.testState.incrementClock(); + + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), false, false); } // TESTCASE: Duplicate refresh message (greater seq #) @@ -996,7 +1041,12 @@ public void refreshTTL_duplicateRefreshSeqNrGreater() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,2), true, true); + + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), true, true); } @@ -1006,18 +1056,25 @@ public void refreshTTL_duplicateRefreshSeqNrLower() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), true, true); + + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,2), false, false); } // TESTCASE: Refresh previously removed entry @Test public void refreshTTL_refreshAfterRemove() throws CryptoException { - ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); - doProtectedStorageAddAndVerify(entry, true, true); - doProtectedStorageRemoveAndVerify(entry, true, true); + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForAdd(2); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), false, false); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + + doRefreshTTLAndVerify(buildRefreshOfferMessage(entryForAdd, this.payloadOwnerKeys,3), false, false); } // TESTCASE: Refresh an entry, but owner doesn't match PubKey of original add owner @@ -1032,17 +1089,10 @@ public void refreshTTL_refreshEntryOwnerOriginalOwnerMismatch() throws CryptoExc } // Runs the ProtectedStorageEntryTestBase tests against the PersistablePayload marker class - public static class PersistableProtectedStoragePayloadTest extends ProtectedStorageEntryTestBase { - private static class PersistableProtectedStoragePayload extends ProtectedStoragePayloadStub implements PersistablePayload { - - PersistableProtectedStoragePayload(PublicKey ownerPubKey) { - super(ownerPubKey); - } - } - + public static class PersistableExpirableProtectedStoragePayloadTest extends ProtectedStorageEntryTestBase { @Override protected ProtectedStoragePayload createInstance(KeyPair payloadOwnerKeys) { - return new PersistableProtectedStoragePayload(payloadOwnerKeys.getPublic()); + return new PersistableExpirableProtectedStoragePayload(payloadOwnerKeys.getPublic()); } } @@ -1080,18 +1130,18 @@ boolean doRemove(ProtectedStorageEntry entry) { return true; } else { // XXX: All external callers just pass in true, a future patch can remove the argument. - return testState.mockedStorage.removeMailboxData((ProtectedMailboxStorageEntry) entry, getTestNodeAddress(), true); + return testState.mockedStorage.remove(entry, getTestNodeAddress(), true); } } @Override ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws CryptoException { - return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber); + return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber, this.testState.clockFake); } @Override ProtectedStorageEntry getProtectedStorageEntryForRemove(int sequenceNumber) throws CryptoException { - return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber); + return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber, this.testState.clockFake); } @Override @@ -1099,12 +1149,81 @@ protected ProtectedStoragePayload createInstance(KeyPair payloadOwnerKeys) { return null; } + // TESTCASE: adding a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the receiver should fail + @Test + public void addProtectedStorageEntry_badWrappingReceiverEntry() throws CryptoException, NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, receiverKeys, 1, this.testState.clockFake); + + doProtectedStorageAddAndVerify(entryForAdd, false, false); + } + + // TESTCASE: adding a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the sender should fail + // XXXBUGXXX Miswrapped MailboxStoragePayload objects go through non-mailbox validation. This circumvents the + // Entry.ownerPubKey == Payload.senderPubKeyForAddOperation checks. + @Test + public void addProtectedStorageEntry_badWrappingSenderEntry() throws CryptoException, NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, senderKeys, 1, this.testState.clockFake); + + // should be (false, false) + doProtectedStorageAddAndVerify(entryForAdd, true, true); + } + + // TESTCASE: removing a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the receiver should fail + // XXX: The reason this fails correctly is extremely subtle. checkIfStoredDataPubKeyMatchesNewDataPubKey() + // in the non-mailbox path sees the the Entry owner changed from the initial add request and fails. If we see + // this invalid wrapper we should fail more explicitly. + @Test + public void remove_badWrappingReceiverEntry() throws CryptoException { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + + ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, receiverKeys, 2, this.testState.clockFake); + + SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove); + + boolean addResult = super.doRemove(entryForRemove); + + if (!this.useMessageHandler) + Assert.assertEquals(false, addResult); + + verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, false, false, false, this.expectIsDataOwner()); + } + + // TESTCASE: removing a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the sender should fail + @Test + public void remove_badWrappingSenderEntry() throws CryptoException { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + + ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, senderKeys, 2, this.testState.clockFake); + + SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove); + + boolean addResult = super.doRemove(entryForRemove); + + // should be (false, false) + if (!this.useMessageHandler) + Assert.assertEquals(true, addResult); + + verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, true, true, true, this.expectIsDataOwner()); + } + // TESTCASE: Adding fails when Entry owner is different from sender @Test public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatible() throws CryptoException, NoSuchAlgorithmException { KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -1116,7 +1235,7 @@ public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatibleNoSideEf doProtectedStorageAddAndVerify(this.getProtectedStorageEntryForAdd(1), true, true); - ProtectedStorageEntry invalidEntryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry invalidEntryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(invalidEntryForAdd, false, false); } @@ -1129,7 +1248,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE KeyPair notReceiver = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1142,7 +1261,7 @@ public void remove_payloadOwnerEntryReceiversPubKeyNotCompatible() throws NoSuch KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1155,60 +1274,12 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry KeyPair otherKeys = TestUtils.generateKeyPair(); // Add an entry that has an invalid Entry.receiversPubKey. Unfortunately, this succeeds right now. - ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), otherKeys.getPublic(), 1); + ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), otherKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, true, true); doProtectedStorageRemoveAndVerify(this.getProtectedStorageEntryForRemove(2), false, false); } - - // XXXBUGXXX: The P2PService calls remove() instead of removeFromMailbox() in the addMailboxData() path. - // This test shows it will always fail even with a valid remove entry. Future work should be able to - // combine the remove paths in the same way the add() paths are combined. This will require deprecating - // the receiversPubKey field which is a duplicate of the ownerPubKey in the MailboxStoragePayload. - // More investigation is needed. - @Test - public void remove_canCallWrongRemoveAndFail() throws CryptoException { - - ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); - ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); - - doProtectedStorageAddAndVerify(entryForAdd, true, true); - - SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove); - - // Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify - // it fails - boolean addResult = super.doRemove(entryForRemove); - - if (!this.useMessageHandler) - Assert.assertFalse(addResult); - - // should succeed with expectedStatechange==true when remove paths are combined - verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, false, this.expectIsDataOwner()); - } - - // TESTCASE: Verify misuse of the API (calling remove() instead of removeFromMailbox correctly errors with - // a payload that is valid for remove of a non-mailbox entry. - @Test - public void remove_canCallWrongRemoveAndFailInvalidPayload() throws CryptoException { - - ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); - - doProtectedStorageAddAndVerify(entryForAdd, true, true); - - SavedTestState beforeState = new SavedTestState(this.testState, entryForAdd); - - // Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify - // it fails with a payload that isn't signed by payload.ownerPubKey - boolean addResult = super.doRemove(entryForAdd); - - if (!this.useMessageHandler) - Assert.assertFalse(addResult); - - verifyProtectedStorageRemove(this.testState, beforeState, entryForAdd, false, this.expectIsDataOwner()); - } - // TESTCASE: Add after removed when add-once required (greater seq #) @Override @Test @@ -1241,7 +1312,7 @@ public void setUp() { public void getProtectedStorageEntry_NoExist() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); @@ -1255,7 +1326,7 @@ public void getProtectedStorageEntry_NoExist() throws NoSuchAlgorithmException, public void getProtectedStorageEntry() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); @@ -1272,7 +1343,7 @@ public void getProtectedStorageEntry() throws NoSuchAlgorithmException, CryptoEx public void getProtectedStorageEntry_FirstOnMessageSecondAPI() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); Connection mockedConnection = mock(Connection.class); @@ -1292,7 +1363,7 @@ public void getProtectedStorageEntry_FirstOnMessageSecondAPI() throws NoSuchAlgo public void getRefreshTTLMessage_NoExists() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); RefreshOfferMessage refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); @@ -1307,7 +1378,7 @@ public void getRefreshTTLMessage_NoExists() throws NoSuchAlgorithmException, Cry public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true); @@ -1316,6 +1387,8 @@ public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoExcept refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); + this.testState.incrementClock(); + SavedTestState beforeState = new SavedTestState(this.testState, refreshOfferMessage); Assert.assertTrue(this.testState.mockedStorage.refreshTTL(refreshOfferMessage, getTestNodeAddress(), true)); @@ -1327,7 +1400,7 @@ public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoExcept public void getRefreshTTLMessage_FirstOnMessageSecondAPI() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true); @@ -1338,6 +1411,8 @@ public void getRefreshTTLMessage_FirstOnMessageSecondAPI() throws NoSuchAlgorith RefreshOfferMessage refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); + this.testState.incrementClock(); + SavedTestState beforeState = new SavedTestState(this.testState, refreshOfferMessage); Assert.assertTrue(this.testState.mockedStorage.refreshTTL(refreshOfferMessage, getTestNodeAddress(), true)); @@ -1356,9 +1431,9 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertFalse(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true, true, true); } // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API @@ -1378,9 +1453,9 @@ public void getMailboxDataWithSignedSeqNr_AddThenRemove() throws NoSuchAlgorithm this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } // TESTCASE: Removing a mailbox message that was added from the onMessage handler @@ -1391,7 +1466,7 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), - senderKeys.getPublic(), receiverKeys.getPublic(), 1); + senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); Connection mockedConnection = mock(Connection.class); when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress())); @@ -1404,9 +1479,9 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } } @@ -1416,7 +1491,7 @@ public static class DisconnectTest { private static ProtectedStorageEntry populateTestState(TestState testState, long ttl) throws CryptoException, NoSuchAlgorithmException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys, ttl); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic(), ttl); ProtectedStorageEntry protectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, false); @@ -1518,9 +1593,152 @@ public void connectionClosedReduceTTLAndExpireItemsFromPeer() throws NoSuchAlgor SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + // Increment the time by 1 hour which will put the protectedStorageState outside TTL + this.testState.incrementClock(); + this.testState.mockedStorage.onDisconnect(CloseConnectionReason.SOCKET_CLOSED, mockedConnection); verifyStateAfterDisconnect(this.testState, beforeState, true, false); } } + + public static class RemoveExpiredTests { + TestState testState; + + @Before + public void setUp() { + this.testState = new TestState(); + + // Deep in the bowels of protobuf we grab the messageID from the version module. This is required to hash the + // full MailboxStoragePayload so make sure it is initialized. + Version.setBaseCryptoNetworkId(1); + } + + // TESTCASE: Correctly skips entries that are not expirable + @Test + public void removeExpiredEntries_SkipsNonExpirableEntries() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly skips non-persistable entries that are not expired + @Test + public void removeExpiredEntries_SkipNonExpiredExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly expires non-persistable entries that are expired + @Test + public void removeExpiredEntries_ExpiresExpiredExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + // Increment the clock by an hour which will cause the Payloads to be outside the TTL range + this.testState.incrementClock(); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, true, false, false, false); + } + + // TESTCASE: Correctly skips persistable entries that are not expired + @Test + public void removeExpiredEntries_SkipNonExpiredPersistableExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly expires persistable entries that are expired + @Test + public void removeExpiredEntries_ExpiresExpiredPersistableExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry protectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + // Increment the clock by an hour which will cause the Payloads to be outside the TTL range + this.testState.incrementClock(); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, true, false, false, false); + } + + // TESTCASE: Ensure we try to purge old entries sequence number map when size exceeds the maximum size + // and that entries less than PURGE_AGE_DAYS remain + @Test + public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchAlgorithmException { + final int initialClockIncrement = 5; + + // Add 4 entries to our sequence number map that will be purged + KeyPair purgedOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload purgedProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(purgedOwnerKeys.getPublic(), 0); + ProtectedStorageEntry purgedProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(purgedProtectedStoragePayload, purgedOwnerKeys); + + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, getTestNodeAddress(), null, true)); + + for (int i = 0; i < 4; ++i) { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(tmpEntry, getTestNodeAddress(), null, true)); + } + + // Increment the time by 5 days which is less than the purge requirement. This will allow the map to have + // some values that will be purged and others that will stay. + this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(initialClockIncrement)); + + // Add a final entry that will not be purged + KeyPair keepOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload keepProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(keepOwnerKeys.getPublic(), 0); + ProtectedStorageEntry keepProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(keepProtectedStoragePayload, keepOwnerKeys); + + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, getTestNodeAddress(), null, true)); + + // P2PDataStorage::PURGE_AGE_DAYS == 10 days + // Advance time past it so they will be valid purge targets + this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(P2PDataStorage.PURGE_AGE_DAYS + 1 - initialClockIncrement)); + + // The first entry (11 days old) should be purged + SavedTestState beforeState = new SavedTestState(this.testState, purgedProtectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + verifyProtectedStorageRemove(this.testState, beforeState, purgedProtectedStorageEntry, true, false, false, false); + + // Which means that an addition of a purged entry should succeed. + beforeState = new SavedTestState(this.testState, purgedProtectedStorageEntry); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, getTestNodeAddress(), null, false)); + verifyProtectedStorageAdd(this.testState, beforeState, purgedProtectedStorageEntry, true, false); + + // The second entry (5 days old) should still exist which means trying to add it again should fail. + beforeState = new SavedTestState(this.testState, keepProtectedStorageEntry); + Assert.assertFalse(this.testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, getTestNodeAddress(), null, false)); + verifyProtectedStorageAdd(this.testState, beforeState, keepProtectedStorageEntry, false, false); + } + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java index f04dd02bf76..458f17528b9 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java @@ -42,6 +42,8 @@ import java.security.SignatureException; import java.security.cert.CertificateException; +import java.time.Clock; + import java.nio.file.Path; import java.nio.file.Paths; @@ -142,7 +144,7 @@ public void testAddAndRemove() throws InterruptedException, NoSuchAlgorithmExcep int newSequenceNumber = data.getSequenceNumber() + 1; byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(data.getProtectedStoragePayload(), newSequenceNumber)); byte[] signature = Sig.sign(storageSignatureKeyPair1.getPrivate(), hashOfDataAndSeqNr); - ProtectedStorageEntry dataToRemove = new ProtectedStorageEntry(data.getProtectedStoragePayload(), data.getOwnerPubKey(), newSequenceNumber, signature); + ProtectedStorageEntry dataToRemove = new ProtectedStorageEntry(data.getProtectedStoragePayload(), data.getOwnerPubKey(), newSequenceNumber, signature, Clock.systemDefaultZone()); Assert.assertTrue(dataStorage1.remove(dataToRemove, null, true)); Assert.assertEquals(0, dataStorage1.getMap().size()); } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java index 43ac2b5655b..3c70ff6b7eb 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java @@ -36,6 +36,8 @@ import java.security.SignatureException; import java.security.cert.CertificateException; +import java.time.Clock; + import java.io.File; import java.io.IOException; @@ -72,7 +74,7 @@ public void toProtoBuf() throws Exception { MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload(prefixedSealedAndSignedMessage, keyRing1.getPubKeyRing().getSignaturePubKey(), keyRing1.getPubKeyRing().getSignaturePubKey()); ProtectedStorageEntry protectedStorageEntry = new ProtectedMailboxStorageEntry(mailboxStoragePayload, - keyRing1.getSignatureKeyPair().getPublic(), 1, RandomUtils.nextBytes(10), keyRing1.getPubKeyRing().getSignaturePubKey()); + keyRing1.getSignatureKeyPair().getPublic(), 1, RandomUtils.nextBytes(10), keyRing1.getPubKeyRing().getSignaturePubKey(), Clock.systemDefaultZone()); AddDataMessage dataMessage1 = new AddDataMessage(protectedStorageEntry); protobuf.NetworkEnvelope envelope = dataMessage1.toProtoNetworkEnvelope(); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java new file mode 100644 index 00000000000..6e852615c55 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java @@ -0,0 +1,49 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage.mocks; + +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneId; + +public class ClockFake extends Clock { + private Instant currentInstant; + + public ClockFake() { + this.currentInstant = Instant.now(); + } + + @Override + public ZoneId getZone() { + throw new UnsupportedOperationException("ClockFake does not support getZone"); + } + + @Override + public Clock withZone(ZoneId zoneId) { + throw new UnsupportedOperationException("ClockFake does not support withZone"); + } + + @Override + public Instant instant() { + return this.currentInstant; + } + + public void increment(long milliseconds) { + this.currentInstant = this.currentInstant.plusMillis(milliseconds); + } +} diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java new file mode 100644 index 00000000000..b97850fc381 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -0,0 +1,202 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage.payload; + +import bisq.network.p2p.PrefixedSealedAndSignedMessage; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.storage.P2PDataStorage; + +import bisq.common.app.Version; +import bisq.common.crypto.CryptoException; +import bisq.common.crypto.Sig; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; +import java.security.PublicKey; + +import java.time.Clock; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import static org.mockito.Mockito.*; + +public class ProtectedMailboxStorageEntryTest { + + private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, + PublicKey payloadOwnerPubKey) { + + // Mock out the PrefixedSealedAndSignedMessage with a version that just serializes to the DEFAULT_INSTANCE + // in protobuf. This object is never validated in the test, but needs to be hashed as part of the testing path. + PrefixedSealedAndSignedMessage prefixedSealedAndSignedMessageMock = mock(PrefixedSealedAndSignedMessage.class); + protobuf.NetworkEnvelope networkEnvelopeMock = mock(protobuf.NetworkEnvelope.class); + when(networkEnvelopeMock.getPrefixedSealedAndSignedMessage()).thenReturn( + protobuf.PrefixedSealedAndSignedMessage.getDefaultInstance()); + when(prefixedSealedAndSignedMessageMock.toProtoNetworkEnvelope()).thenReturn(networkEnvelopeMock); + + return new MailboxStoragePayload( + prefixedSealedAndSignedMessageMock, payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); + } + + private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry( + MailboxStoragePayload mailboxStoragePayload, KeyPair ownerKey, PublicKey receiverKey, int sequenceNumber) throws CryptoException { + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(mailboxStoragePayload, sequenceNumber)); + byte[] signature = Sig.sign(ownerKey.getPrivate(), hashOfDataAndSeqNr); + + return new ProtectedMailboxStorageEntry(mailboxStoragePayload, ownerKey.getPublic(), sequenceNumber, signature, receiverKey, Clock.systemDefaultZone()); + } + + @Before + public void SetUp() { + // Deep in the bowels of protobuf we grab the messageID from the version module. This is required to hash the + // full MailboxStoragePayload so make sure it is initialized. + Version.setBaseCryptoNetworkId(1); + } + + // TESTCASE: validForAddOperation() should return true if the Entry owner and sender key specified in payload match + @Test + public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should return false if the Entry owner and sender key specified in payload don't match + @Test + public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // 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()); + } + + // TESTCASE: validForAddOperation() should fail if the signature isn't valid + @Test + public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + protectedStorageEntry.updateSignature( new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match + @Test + public void validForRemove() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); + + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match + @Test + public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: isValidForRemoveOperation() should fail if the signature is bad + @Test + public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); + + protectedStorageEntry.updateSignature(new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: isValidForRemoveOperation() should fail if the receiversPubKey does not match the Entry owner + @Test + public void isValidForRemoveOperation_ReceiversPubKeyMismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, senderKeys.getPublic(), 1); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: isMetadataEquals() should succeed if the sequence number changes + @Test + public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry seqNrOne = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 2); + + Assert.assertTrue(seqNrOne.matchesRelevantPubKey(seqNrTwo)); + } + + // TESTCASE: isMetadataEquals() should fail if the receiversPubKey changes + @Test + public void isMetadataEquals_receiverPubKeyChanged() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry seqNrOne = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1); + + Assert.assertFalse(seqNrOne.matchesRelevantPubKey(seqNrTwo)); + } +} diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java new file mode 100644 index 00000000000..b32ee34c943 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -0,0 +1,216 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage.payload; + +import bisq.network.p2p.PrefixedSealedAndSignedMessage; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.storage.P2PDataStorage; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; + +import bisq.common.app.Version; +import bisq.common.crypto.CryptoException; +import bisq.common.crypto.Sig; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; +import java.security.PublicKey; + +import java.time.Clock; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class ProtectedStorageEntryTest { + private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner, int sequenceNumber) throws CryptoException { + return buildProtectedStorageEntry(new ProtectedStoragePayloadStub(payloadOwner.getPublic()), entryOwner, sequenceNumber); + } + + private static ProtectedStorageEntry buildProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + KeyPair entryOwner, int sequenceNumber) throws CryptoException { + + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); + byte[] signature = Sig.sign(entryOwner.getPrivate(), hashOfDataAndSeqNr); + + return new ProtectedStorageEntry(protectedStoragePayload, entryOwner.getPublic(), sequenceNumber, + signature, Clock.systemDefaultZone()); + } + + private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, + PublicKey payloadOwnerPubKey) { + + // Mock out the PrefixedSealedAndSignedMessage with a version that just serializes to the DEFAULT_INSTANCE + // in protobuf. This object is never validated in the test, but needs to be hashed as part of the testing path. + PrefixedSealedAndSignedMessage prefixedSealedAndSignedMessageMock = mock(PrefixedSealedAndSignedMessage.class); + protobuf.NetworkEnvelope networkEnvelopeMock = mock(protobuf.NetworkEnvelope.class); + when(networkEnvelopeMock.getPrefixedSealedAndSignedMessage()).thenReturn( + protobuf.PrefixedSealedAndSignedMessage.getDefaultInstance()); + when(prefixedSealedAndSignedMessageMock.toProtoNetworkEnvelope()).thenReturn(networkEnvelopeMock); + + return new MailboxStoragePayload( + prefixedSealedAndSignedMessageMock, payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); + } + + @Before + public void SetUp() { + // Deep in the bowels of protobuf we grab the messageID from the version module. This is required to hash the + // full MailboxStoragePayload so make sure it is initialized. + Version.setBaseCryptoNetworkId(1); + } + + // TESTCASE: validForAddOperation() should return true if the Entry owner and payload owner match + @Test + public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should return false if the Entry owner and payload owner don't match + @Test + public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys, 1); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the sender + // XXXBUGXXX: Currently, a mis-wrapped MailboxStorageEntry will circumvent the senderPubKeyForAddOperation checks + @Test + public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, 1); + + // should be assertFalse + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the receiver + @Test + public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, 1); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the signature isn't valid + @Test + public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + protectedStorageEntry.updateSignature( new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match + @Test + public void isValidForRemoveOperation() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match + @Test + public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys, 1); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the sender + // XXXBUGXXX: Currently, a mis-wrapped MailboxStoragePayload will succeed + @Test + public void isValidForRemoveOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, 1); + + // should be assertFalse + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + @Test + public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, 1); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: isValidForRemoveOperation() should fail if the signature is bad + @Test + public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + protectedStorageEntry.updateSignature(new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: isMetadataEquals() should succeed if the sequence number changes + @Test + public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry seqNrOne = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedStorageEntry(ownerKeys, ownerKeys, 2); + + Assert.assertTrue(seqNrOne.matchesRelevantPubKey(seqNrTwo)); + } + + // TESTCASE: isMetadataEquals() should fail if the OwnerPubKey changes + @Test + public void isMetadataEquals_OwnerPubKeyChanged() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwner = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntryOne = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + ProtectedStorageEntry protectedStorageEntryTwo = buildProtectedStorageEntry(ownerKeys, notOwner, 1); + + Assert.assertFalse(protectedStorageEntryOne.matchesRelevantPubKey(protectedStorageEntryTwo)); + } +}