From c70cc4a5d6948720eb337a218c2281ace166b30d Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Sat, 8 Jun 2024 13:35:17 +0700 Subject: [PATCH 1/4] Apply ExcludeForHash annotation to Filter. By that the newly added fields will not alter the byte array used for the hash and the signature, thus it will not cause issues with removing an old filter which did not inculde those new fields. Signed-off-by: HenrikJannsen --- .../main/java/bisq/core/filter/Filter.java | 28 ++++++++- .../java/bisq/core/filter/FilterManager.java | 3 +- .../FilterWithExcludeForHashFieldTests.java | 63 +++++++++++++++++++ .../java/bisq/core/filter/TestFilter.java | 4 +- 4 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/bisq/core/filter/FilterWithExcludeForHashFieldTests.java diff --git a/core/src/main/java/bisq/core/filter/Filter.java b/core/src/main/java/bisq/core/filter/Filter.java index 069cc91dbf4..5decd1ba25d 100644 --- a/core/src/main/java/bisq/core/filter/Filter.java +++ b/core/src/main/java/bisq/core/filter/Filter.java @@ -20,6 +20,8 @@ import bisq.network.p2p.storage.payload.ExpirablePayload; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; +import bisq.common.ExcludeForHash; +import bisq.common.ExcludeForHashAwareProto; import bisq.common.crypto.Sig; import bisq.common.proto.ProtoUtil; import bisq.common.proto.network.GetDataResponsePriority; @@ -27,6 +29,8 @@ import bisq.common.util.ExtraDataMapValidator; import bisq.common.util.Utilities; +import protobuf.StoragePayload; + import com.google.protobuf.ByteString; import com.google.common.annotations.VisibleForTesting; @@ -49,7 +53,7 @@ @Slf4j @Getter @EqualsAndHashCode -public final class Filter implements ProtectedStoragePayload, ExpirablePayload { +public final class Filter implements ProtectedStoragePayload, ExpirablePayload, ExcludeForHashAwareProto { public static final long TTL = TimeUnit.DAYS.toMillis(180); private final List bannedOfferIds; @@ -124,7 +128,9 @@ public final class Filter implements ProtectedStoragePayload, ExpirablePayload { private final List delayedPayoutPaymentAccounts; // Added at v 1.9.16 + @ExcludeForHash private final List addedBtcNodes; + @ExcludeForHash private final List addedSeedNodes; // After we have created the signature from the filter data we clone it and apply the signature @@ -379,6 +385,24 @@ public Filter(List bannedOfferIds, @Override public protobuf.StoragePayload toProtoMessage() { + return toProto(false); + } + + @Override + public protobuf.StoragePayload toProto(boolean serializeForHash) { + return resolveProto(serializeForHash); + } + + @Override + public protobuf.StoragePayload.Builder getBuilder(boolean serializeForHash) { + return StoragePayload.newBuilder().setFilter(toFilterProto(serializeForHash)); + } + + private protobuf.Filter toFilterProto(boolean serializeForHash) { + return resolveBuilder(getFilterBuilder(serializeForHash), serializeForHash).build(); + } + + private protobuf.Filter.Builder getFilterBuilder(boolean serializeForHash) { List paymentAccountFilterList = bannedPaymentAccounts.stream() .map(PaymentAccountFilter::toProtoMessage) .collect(Collectors.toList()); @@ -426,7 +450,7 @@ public protobuf.StoragePayload toProtoMessage() { Optional.ofNullable(signatureAsBase64).ifPresent(builder::setSignatureAsBase64); Optional.ofNullable(extraDataMap).ifPresent(builder::putAllExtraData); - return protobuf.StoragePayload.newBuilder().setFilter(builder).build(); + return builder; } public static Filter fromProto(protobuf.Filter proto) { diff --git a/core/src/main/java/bisq/core/filter/FilterManager.java b/core/src/main/java/bisq/core/filter/FilterManager.java index ad0c29f3264..c0fb5bae852 100644 --- a/core/src/main/java/bisq/core/filter/FilterManager.java +++ b/core/src/main/java/bisq/core/filter/FilterManager.java @@ -701,8 +701,7 @@ private ECKey toECKey(String privKeyString) { } private Sha256Hash getSha256Hash(Filter filter) { - byte[] filterData = filter.serializeForHash(); - return Sha256Hash.of(filterData); + return Sha256Hash.of(filter.serializeForHash()); } private String getPubKeyAsHex(ECKey ecKey) { diff --git a/core/src/test/java/bisq/core/filter/FilterWithExcludeForHashFieldTests.java b/core/src/test/java/bisq/core/filter/FilterWithExcludeForHashFieldTests.java new file mode 100644 index 00000000000..91d10185a83 --- /dev/null +++ b/core/src/test/java/bisq/core/filter/FilterWithExcludeForHashFieldTests.java @@ -0,0 +1,63 @@ +/* + * 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.core.filter; + +import bisq.common.crypto.Sig; + +import org.bouncycastle.jce.provider.BouncyCastleProvider; + +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.NoSuchAlgorithmException; +import java.security.NoSuchProviderException; +import java.security.PublicKey; +import java.security.Security; + +import java.util.Arrays; + +import org.mockito.junit.jupiter.MockitoExtension; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@ExtendWith(MockitoExtension.class) +public class FilterWithExcludeForHashFieldTests { + static { + Security.addProvider(new BouncyCastleProvider()); + } + + private final PublicKey ownerPublicKey; + + public FilterWithExcludeForHashFieldTests() throws NoSuchAlgorithmException, NoSuchProviderException { + KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance(Sig.KEY_ALGO, "BC"); + KeyPair ownerKeyPair = keyPairGenerator.generateKeyPair(); + ownerPublicKey = ownerKeyPair.getPublic(); + } + + @Test + void testSerializeForHashAnnotation() { + Filter filterWithoutSig = TestFilter.createFilter(ownerPublicKey, "invalidPubKeyAsHex"); + byte[] serializeForHashBytes = filterWithoutSig.serializeForHash(); + byte[] serializeBytes = filterWithoutSig.serialize(); + assertFalse(Arrays.equals(serializeForHashBytes, serializeBytes)); + assertTrue(serializeBytes.length > serializeForHashBytes.length); + } +} diff --git a/core/src/test/java/bisq/core/filter/TestFilter.java b/core/src/test/java/bisq/core/filter/TestFilter.java index ef232fa8827..047850f4854 100644 --- a/core/src/test/java/bisq/core/filter/TestFilter.java +++ b/core/src/test/java/bisq/core/filter/TestFilter.java @@ -103,8 +103,8 @@ public static Filter createFilter(PublicKey ownerPublicKey, String signerPubKeyA 1, 1, Collections.emptyList(), - Collections.emptyList(), - Collections.emptyList() + List.of("test1.onion:1221"), + List.of("test2.onion:1221") ); } From b3288e3839a444004745ac78dac128e9c18ffe56 Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Sat, 8 Jun 2024 14:51:11 +0700 Subject: [PATCH 2/4] Improve logs Signed-off-by: HenrikJannsen --- .../src/main/java/bisq/common/ExcludeForHashAwareProto.java | 2 +- .../network/p2p/storage/payload/ProtectedStorageEntry.java | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/common/src/main/java/bisq/common/ExcludeForHashAwareProto.java b/common/src/main/java/bisq/common/ExcludeForHashAwareProto.java index 18977fb3b9f..284e223c5c6 100644 --- a/common/src/main/java/bisq/common/ExcludeForHashAwareProto.java +++ b/common/src/main/java/bisq/common/ExcludeForHashAwareProto.java @@ -103,7 +103,7 @@ default Set getExcludedFields() { default B clearAnnotatedFields(B builder) { Set excludedFields = getExcludedFields(); if (!excludedFields.isEmpty()) { - getLogger().error("Clear fields in builder annotated with @ExcludeForHash: {}", excludedFields); + getLogger().debug("Clear fields in builder annotated with @ExcludeForHash: {}", excludedFields); } for (Descriptors.FieldDescriptor fieldDesc : builder.getAllFields().keySet()) { if (excludedFields.contains(fieldDesc.getName())) { 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 9f4f61c2853..1f1c4e1f696 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 @@ -32,6 +32,8 @@ import com.google.common.base.Preconditions; +import org.bouncycastle.util.encoders.Hex; + import java.security.PublicKey; import java.time.Clock; @@ -215,7 +217,9 @@ boolean isSignatureValid() { boolean result = Sig.verify(this.ownerPubKey, hashOfDataAndSeqNr, this.signature); if (!result) - log.warn("ProtectedStorageEntry::isSignatureValid() failed.\n{}}", this); + log.warn("Invalid signature for {}.\nSerialized data as hex={}}", + protectedStoragePayload.getClass().getSimpleName(), + Hex.toHexString(protectedStoragePayload.toProtoMessage().toByteArray())); return result; } catch (CryptoException e) { From cb565bdbb29296cbd42578517602ca2343f251b7 Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Sat, 8 Jun 2024 16:51:43 +0700 Subject: [PATCH 3/4] Add ExcludeForHashAwareProto interface to DataAndSeqNrPair Signed-off-by: HenrikJannsen --- .../network/p2p/storage/P2PDataStorage.java | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) 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 c80f6c2720c..d8ac6dddc18 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -55,6 +55,7 @@ import bisq.network.p2p.storage.persistence.ResourceDataStoreService; import bisq.network.p2p.storage.persistence.SequenceNumberMap; +import bisq.common.ExcludeForHashAwareProto; import bisq.common.Timer; import bisq.common.UserThread; import bisq.common.app.Capabilities; @@ -71,6 +72,8 @@ import bisq.common.util.Tuple2; import bisq.common.util.Utilities; +import protobuf.StoragePayload; + import com.google.protobuf.ByteString; import com.google.inject.name.Named; @@ -1253,8 +1256,8 @@ public static byte[] get32ByteHash(NetworkPayload data) { */ @EqualsAndHashCode @ToString - public static final class DataAndSeqNrPair implements NetworkPayload { - // data are only used for calculating cryptographic hash from both values so they are kept private + public static final class DataAndSeqNrPair implements NetworkPayload, ExcludeForHashAwareProto { + // data are only used for calculating cryptographic hash from both values, so they are kept private private final ProtectedStoragePayload protectedStoragePayload; private final int sequenceNumber; @@ -1263,13 +1266,31 @@ public DataAndSeqNrPair(ProtectedStoragePayload protectedStoragePayload, int seq this.sequenceNumber = sequenceNumber; } - // Used only for calculating hash of byte array from PB object @Override - public com.google.protobuf.Message toProtoMessage() { + public protobuf.DataAndSeqNrPair toProtoMessage() { + return toProto(false); + } + + @Override + public protobuf.DataAndSeqNrPair toProto(boolean serializeForHash) { + return resolveProto(serializeForHash); + } + + @Override + public protobuf.DataAndSeqNrPair.Builder getBuilder(boolean serializeForHash) { return protobuf.DataAndSeqNrPair.newBuilder() - .setPayload((protobuf.StoragePayload) protectedStoragePayload.toProtoMessage()) - .setSequenceNumber(sequenceNumber) - .build(); + .setPayload(toStoragePayloadProto(serializeForHash)) + .setSequenceNumber(sequenceNumber); + } + + private protobuf.StoragePayload toStoragePayloadProto(boolean serializeForHash) { + if (protectedStoragePayload instanceof ExcludeForHashAwareProto) { + ExcludeForHashAwareProto proto = (ExcludeForHashAwareProto) protectedStoragePayload; + StoragePayload.Builder builder = (StoragePayload.Builder) proto.getBuilder(serializeForHash); + return resolveBuilder(builder, serializeForHash).build(); + } else { + return (StoragePayload) protectedStoragePayload.toProtoMessage(); + } } } From 73a1281771e8f3db8961d1b962d80094c8418757 Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Sat, 8 Jun 2024 17:07:08 +0700 Subject: [PATCH 4/4] Add uid to Filter Signed-off-by: HenrikJannsen --- .../main/java/bisq/core/filter/Filter.java | 27 ++++++++++++++----- .../java/bisq/core/filter/TestFilter.java | 4 ++- .../core/user/UserPayloadModelVOTest.java | 4 ++- .../main/overlays/windows/FilterWindow.java | 9 ++++++- proto/src/main/proto/pb.proto | 1 + 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/bisq/core/filter/Filter.java b/core/src/main/java/bisq/core/filter/Filter.java index 5decd1ba25d..2461dd0179f 100644 --- a/core/src/main/java/bisq/core/filter/Filter.java +++ b/core/src/main/java/bisq/core/filter/Filter.java @@ -132,6 +132,10 @@ public final class Filter implements ProtectedStoragePayload, ExpirablePayload, private final List addedBtcNodes; @ExcludeForHash private final List addedSeedNodes; + // As we might add more ExcludeForHash we want to ensure to have a unique identifier. + // The hash of the data is not unique anymore if the only change have been at + // the ExcludeForHash annotated fields. + private final String uid; // After we have created the signature from the filter data we clone it and apply the signature static Filter cloneWithSig(Filter filter, String signatureAsBase64) { @@ -172,7 +176,8 @@ static Filter cloneWithSig(Filter filter, String signatureAsBase64) { filter.getTakerFeeBsq(), filter.getDelayedPayoutPaymentAccounts(), filter.getAddedBtcNodes(), - filter.getAddedSeedNodes()); + filter.getAddedSeedNodes(), + filter.getUid()); } // Used for signature verification as we created the sig without the signatureAsBase64 field we set it to null again @@ -214,7 +219,8 @@ static Filter cloneWithoutSig(Filter filter) { filter.getTakerFeeBsq(), filter.getDelayedPayoutPaymentAccounts(), filter.getAddedBtcNodes(), - filter.getAddedSeedNodes()); + filter.getAddedSeedNodes(), + filter.getUid()); } public Filter(List bannedOfferIds, @@ -251,7 +257,8 @@ public Filter(List bannedOfferIds, long takerFeeBsq, List delayedPayoutPaymentAccounts, List addedBtcNodes, - List addedSeedNodes) { + List addedSeedNodes, + String uid) { this(bannedOfferIds, nodeAddressesBannedFromTrading, bannedPaymentAccounts, @@ -289,7 +296,8 @@ public Filter(List bannedOfferIds, takerFeeBsq, delayedPayoutPaymentAccounts, addedBtcNodes, - addedSeedNodes); + addedSeedNodes, + uid); } @@ -335,7 +343,8 @@ public Filter(List bannedOfferIds, long takerFeeBsq, List delayedPayoutPaymentAccounts, List addedBtcNodes, - List addedSeedNodes) { + List addedSeedNodes, + String uid) { this.bannedOfferIds = bannedOfferIds; this.nodeAddressesBannedFromTrading = nodeAddressesBannedFromTrading; this.bannedPaymentAccounts = bannedPaymentAccounts; @@ -374,6 +383,7 @@ public Filter(List bannedOfferIds, this.delayedPayoutPaymentAccounts = delayedPayoutPaymentAccounts; this.addedBtcNodes = addedBtcNodes; this.addedSeedNodes = addedSeedNodes; + this.uid = uid; // ownerPubKeyBytes can be null when called from tests if (ownerPubKeyBytes != null) { @@ -445,7 +455,8 @@ private protobuf.Filter.Builder getFilterBuilder(boolean serializeForHash) { .setTakerFeeBsq(takerFeeBsq) .addAllDelayedPayoutPaymentAccounts(delayedPayoutPaymentAccountList) .addAllAddedBtcNodes(addedBtcNodes) - .addAllAddedSeedNodes(addedSeedNodes); + .addAllAddedSeedNodes(addedSeedNodes) + .setUid(uid); Optional.ofNullable(signatureAsBase64).ifPresent(builder::setSignatureAsBase64); Optional.ofNullable(extraDataMap).ifPresent(builder::putAllExtraData); @@ -498,7 +509,8 @@ public static Filter fromProto(protobuf.Filter proto) { proto.getTakerFeeBsq(), delayedPayoutPaymentAccounts, ProtoUtil.protocolStringListToList(proto.getAddedBtcNodesList()), - ProtoUtil.protocolStringListToList(proto.getAddedSeedNodesList()) + ProtoUtil.protocolStringListToList(proto.getAddedSeedNodesList()), + proto.getUid() ); } @@ -558,6 +570,7 @@ public String toString() { ",\n takerFeeBsq=" + takerFeeBsq + ",\n addedBtcNodes=" + addedBtcNodes + ",\n addedSeedNodes=" + addedSeedNodes + + ",\n uid=" + uid + "\n}"; } } diff --git a/core/src/test/java/bisq/core/filter/TestFilter.java b/core/src/test/java/bisq/core/filter/TestFilter.java index 047850f4854..ff2c041f0c3 100644 --- a/core/src/test/java/bisq/core/filter/TestFilter.java +++ b/core/src/test/java/bisq/core/filter/TestFilter.java @@ -32,6 +32,7 @@ import java.util.Collections; import java.util.List; +import java.util.UUID; import static org.bitcoinj.core.Utils.HEX; @@ -104,7 +105,8 @@ public static Filter createFilter(PublicKey ownerPublicKey, String signerPubKeyA 1, Collections.emptyList(), List.of("test1.onion:1221"), - List.of("test2.onion:1221") + List.of("test2.onion:1221"), + UUID.randomUUID().toString() ); } diff --git a/core/src/test/java/bisq/core/user/UserPayloadModelVOTest.java b/core/src/test/java/bisq/core/user/UserPayloadModelVOTest.java index 930bedadb72..52152c3118f 100644 --- a/core/src/test/java/bisq/core/user/UserPayloadModelVOTest.java +++ b/core/src/test/java/bisq/core/user/UserPayloadModelVOTest.java @@ -26,6 +26,7 @@ import com.google.common.collect.Lists; import java.util.HashSet; +import java.util.UUID; import org.junit.jupiter.api.Disabled; @@ -79,7 +80,8 @@ public void testRoundtripFull() { 0, Lists.newArrayList(), Lists.newArrayList(), - Lists.newArrayList())); + Lists.newArrayList(), + UUID.randomUUID().toString())); vo.setRegisteredArbitrator(ArbitratorTest.getArbitratorMock()); vo.setRegisteredMediator(MediatorTest.getMediatorMock()); diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/FilterWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/FilterWindow.java index c997271d66f..b736f73fee0 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/FilterWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/FilterWindow.java @@ -60,6 +60,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.UUID; import java.util.stream.Collectors; import static bisq.desktop.util.FormBuilder.addInputTextField; @@ -129,6 +130,8 @@ private void addContent() { InputTextField keyTF = addInputTextField(gridPane, ++rowIndex, Res.get("shared.unlock"), 10); + InputTextField uidTF = addInputTextField(gridPane, ++rowIndex, + "UID", 10); if (useDevPrivilegeKeys) { keyTF.setText(DevEnv.getDEV_PRIVILEGE_PRIV_KEY()); } @@ -212,6 +215,7 @@ private void addContent() { Filter filter = filterManager.getDevFilter(); if (filter != null) { + uidTF.setText(filter.getUid()); setupFieldFromList(offerIdsTF, filter.getBannedOfferIds()); setupFieldFromList(bannedFromTradingTF, filter.getNodeAddressesBannedFromTrading()); setupFieldFromList(bannedFromNetworkTF, filter.getNodeAddressesBannedFromNetwork()); @@ -247,6 +251,8 @@ private void addContent() { makerFeeBsqTF.setText(bsqFormatter.formatBSQSatoshis(filter.getMakerFeeBsq())); takerFeeBsqTF.setText(bsqFormatter.formatBSQSatoshis(filter.getTakerFeeBsq())); setupFieldFromPaymentAccountFiltersList(delayedPayoutTF, filter.getDelayedPayoutPaymentAccounts()); + } else { + uidTF.setText(UUID.randomUUID().toString()); } Button removeFilterMessageButton = new AutoTooltipButton(Res.get("filterWindow.remove")); @@ -292,7 +298,8 @@ private void addContent() { ParsingUtils.parseToCoin(takerFeeBsqTF.getText(), bsqFormatter).value, readAsPaymentAccountFiltersList(delayedPayoutTF), readAsList(addedBtcNodesTF), - readAsList(addedSeedNodesTF) + readAsList(addedSeedNodesTF), + uidTF.getText() ); // We remove first the old filter diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto index f5be6a1f66b..034343f1996 100644 --- a/proto/src/main/proto/pb.proto +++ b/proto/src/main/proto/pb.proto @@ -769,6 +769,7 @@ message Filter { repeated PaymentAccountFilter delayedPayoutPaymentAccounts = 36; repeated string addedBtcNodes = 37; repeated string addedSeedNodes = 38; + string uid = 39; } /* Deprecated */