From b9a8d935c11b3ccf74530256e9c0c6d99fb3b99e Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Wed, 27 Mar 2019 11:06:21 +1000 Subject: [PATCH 1/3] local list checks against self and sync status provider overrides other providers if we're not in sync --- ...odeLocalConfigPermissioningController.java | 33 ++++++++++--------- .../node/NodePermissioningController.java | 6 ++++ .../SyncStatusNodePermissioningProvider.java | 5 +-- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java index a736362ac2..07ec87bfdb 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java @@ -207,25 +207,28 @@ private Collection peerToEnodeURI(final Collection peers) { return peers.parallelStream().map(EnodeURL::toString).collect(Collectors.toList()); } + private boolean compareEnodes(final EnodeURL nodeA, final EnodeURL nodeB) { + boolean idsMatch = nodeA.getNodeId().equals(nodeB.getNodeId()); + boolean hostsMatch = nodeA.getIp().equals(nodeB.getIp()); + boolean listeningPortsMatch = nodeA.getListeningPort().equals(nodeB.getListeningPort()); + boolean discoveryPortsMatch = true; + if (nodeA.getDiscoveryPort().isPresent() && nodeB.getDiscoveryPort().isPresent()) { + discoveryPortsMatch = + nodeA.getDiscoveryPort().getAsInt() == nodeB.getDiscoveryPort().getAsInt(); + } + + return idsMatch && hostsMatch && listeningPortsMatch && discoveryPortsMatch; + } + public boolean isPermitted(final String enodeURL) { return isPermitted(new EnodeURL(enodeURL)); } public boolean isPermitted(final EnodeURL node) { - return nodesWhitelist.stream() - .anyMatch( - p -> { - boolean idsMatch = node.getNodeId().equals(p.getNodeId()); - boolean hostsMatch = node.getIp().equals(p.getIp()); - boolean listeningPortsMatch = node.getListeningPort().equals(p.getListeningPort()); - boolean discoveryPortsMatch = true; - if (node.getDiscoveryPort().isPresent() && p.getDiscoveryPort().isPresent()) { - discoveryPortsMatch = - node.getDiscoveryPort().getAsInt() == p.getDiscoveryPort().getAsInt(); - } - - return idsMatch && hostsMatch && listeningPortsMatch && discoveryPortsMatch; - }); + if (compareEnodes(selfEnode, node)) { + return true; + } + return nodesWhitelist.stream().anyMatch(p -> compareEnodes(p, node)); } public List getNodesWhitelist() { @@ -315,6 +318,6 @@ public Optional message() { @Override public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) { - return isPermitted(sourceEnode) || isPermitted(destinationEnode); + return isPermitted(sourceEnode) && isPermitted(destinationEnode); } } diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java index 07a60f7118..3783315401 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java @@ -39,6 +39,12 @@ public NodePermissioningController( public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) { LOG.trace("Checking node permission: {} -> {}", sourceEnode, destinationEnode); + if (syncStatusNodePermissioningProvider + .map(p -> !p.hasReachedSync() && p.isPermitted(sourceEnode, destinationEnode)) + .orElse(false)) { + return true; + } + if (syncStatusNodePermissioningProvider.isPresent() && !syncStatusNodePermissioningProvider.get().isPermitted(sourceEnode, destinationEnode)) { return false; diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java index 184d70a70a..6ca25926bc 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java @@ -24,8 +24,6 @@ import java.util.Optional; import java.util.OptionalLong; -import com.google.common.annotations.VisibleForTesting; - public class SyncStatusNodePermissioningProvider implements NodePermissioningProvider { private final Synchronizer synchronizer; @@ -95,8 +93,7 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio } } - @VisibleForTesting - boolean hasReachedSync() { + public boolean hasReachedSync() { return hasReachedSync; } } From a1ef34f5646826444e6504728e0119dfd74e2ea3 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Wed, 27 Mar 2019 12:28:41 +1000 Subject: [PATCH 2/3] Test fixes --- .../NodeLocalConfigPermissioningControllerTest.java | 2 -- .../permissioning/node/NodePermissioningControllerTest.java | 5 ++++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java index c34aa695b7..081a611a61 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java @@ -232,8 +232,6 @@ public void whenCheckingIfNodeIsPermittedOrderDoesNotMatter() { controller.addNodes(Arrays.asList(enode1)); assertThat(controller.isPermitted(new EnodeURL(enode1), new EnodeURL(selfEnode))).isTrue(); assertThat(controller.isPermitted(new EnodeURL(selfEnode), new EnodeURL(enode1))).isTrue(); - - assertThat(controller.isPermitted(new EnodeURL(selfEnode), new EnodeURL(selfEnode))).isFalse(); } @Test diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerTest.java index ac147ef012..36ec789e99 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerTest.java @@ -15,6 +15,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -64,7 +65,7 @@ public void before() { public void isPermittedShouldDelegateToSyncStatusProvider() { controller.isPermitted(enode1, enode2); - verify(syncStatusNodePermissioningProvider).isPermitted(eq(enode1), eq(enode2)); + verify(syncStatusNodePermissioningProvider, atLeast(1)).isPermitted(eq(enode1), eq(enode2)); } @Test @@ -103,6 +104,7 @@ public void whenNoSyncStatusProviderWeShouldDelegateToLocalConfigNodePermissioni new NodePermissioningController(syncStatusNodePermissioningProviderOptional, providers); when(syncStatusNodePermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(true); + when(syncStatusNodePermissioningProvider.hasReachedSync()).thenReturn(true); when(localConfigNodePermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(true); when(otherPermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(true); @@ -129,6 +131,7 @@ private List getNodePermissioningProviders() { new NodePermissioningController(syncStatusNodePermissioningProviderOptional, providers); when(syncStatusNodePermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(true); + when(syncStatusNodePermissioningProvider.hasReachedSync()).thenReturn(true); when(localConfigNodePermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(true); when(otherPermissioningProvider.isPermitted(eq(enode1), eq(enode2))).thenReturn(false); From 5de5a47df4ef71d518495f91e759aec2d0c09918 Mon Sep 17 00:00:00 2001 From: Chris Mckay Date: Wed, 27 Mar 2019 15:35:11 +1000 Subject: [PATCH 3/3] Fixed self enode detection in local file --- .../NodeLocalConfigPermissioningController.java | 6 +++++- .../java/tech/pegasys/pantheon/util/enode/EnodeURL.java | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java index 07ec87bfdb..53fbf70317 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java @@ -207,6 +207,10 @@ private Collection peerToEnodeURI(final Collection peers) { return peers.parallelStream().map(EnodeURL::toString).collect(Collectors.toList()); } + private boolean checkSelfEnode(final EnodeURL node) { + return selfEnode.getNodeId().equals(node.getNodeId()); + } + private boolean compareEnodes(final EnodeURL nodeA, final EnodeURL nodeB) { boolean idsMatch = nodeA.getNodeId().equals(nodeB.getNodeId()); boolean hostsMatch = nodeA.getIp().equals(nodeB.getIp()); @@ -225,7 +229,7 @@ public boolean isPermitted(final String enodeURL) { } public boolean isPermitted(final EnodeURL node) { - if (compareEnodes(selfEnode, node)) { + if (checkSelfEnode(node)) { return true; } return nodesWhitelist.stream().anyMatch(p -> compareEnodes(p, node)); diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java index bcf3e44949..70d15f4564 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java @@ -64,7 +64,11 @@ public EnodeURL( final InetAddress address, final Integer listeningPort, final OptionalInt discoveryPort) { - this.nodeId = nodeId; + if (nodeId.startsWith("0x")) { + this.nodeId = nodeId.substring(2); + } else { + this.nodeId = nodeId; + } this.ip = address; this.listeningPort = listeningPort; this.discoveryPort = discoveryPort;