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 74d9fc78e0..a626be0ea1 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 @@ -41,11 +41,15 @@ public class NodePermissioningController { } public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) { - LOG.trace("Checking node permission: {} -> {}", sourceEnode, destinationEnode); + LOG.trace("Node permissioning: Checking {} -> {}", sourceEnode, destinationEnode); if (syncStatusNodePermissioningProvider .map(p -> !p.hasReachedSync() && p.isPermitted(sourceEnode, destinationEnode)) .orElse(false)) { + + LOG.trace( + "Node permissioning - Sync Status: Permitted {} -> {}", sourceEnode, destinationEnode); + return true; } @@ -54,19 +58,40 @@ public class NodePermissioningController { p -> p.isPermitted(sourceEnode, destinationEnode)); if (insufficientPeerPermission.isPresent()) { - return insufficientPeerPermission.get(); + final Boolean permitted = insufficientPeerPermission.get(); + + LOG.trace( + "Node permissioning - Insufficient Peers: {} {} -> {}", + permitted ? "Permitted" : "Rejected", + sourceEnode, + destinationEnode); + + return permitted; } if (syncStatusNodePermissioningProvider.isPresent() && !syncStatusNodePermissioningProvider.get().isPermitted(sourceEnode, destinationEnode)) { + + LOG.trace( + "Node permissioning - Sync Status: Rejected {} -> {}", sourceEnode, destinationEnode); + return false; } else { for (final NodePermissioningProvider provider : providers) { if (!provider.isPermitted(sourceEnode, destinationEnode)) { + LOG.trace( + "Node permissioning - {}: Rejected {} -> {}", + provider.getClass().getSimpleName(), + sourceEnode, + destinationEnode); + return false; } } } + + LOG.trace("Node permissioning: Permitted {} -> {}", sourceEnode, destinationEnode); + return true; } 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 f4e56f4616..170d57af01 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 @@ -103,7 +103,7 @@ public class SyncStatusNodePermissioningProvider implements NodePermissioningPro return true; } else { checkCounter.inc(); - if (fixedNodes.contains(destinationEnode)) { + if (fixedNodes.stream().anyMatch(p -> EnodeURL.sameListeningEndpoint(p, destinationEnode))) { checkCounterPermitted.inc(); return true; } else { diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java index 6de0adee51..11fb3c56f6 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java @@ -31,6 +31,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.function.IntSupplier; +import com.google.common.collect.Lists; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -187,4 +188,25 @@ public class SyncStatusNodePermissioningProviderTest { verify(checkPermittedCounter, times(0)).inc(); verify(checkUnpermittedCounter, times(0)).inc(); } + + @Test + public void syncStatusPermissioningCheckShouldIgnoreEnodeURLDiscoveryPort() { + syncStatusListener.onSyncStatus(new SyncStatus(0, 1, 2)); + assertThat(provider.hasReachedSync()).isFalse(); + + final EnodeURL bootnode = + EnodeURL.fromString( + "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.3:5678"); + final EnodeURL enodeWithDiscoveryPort = + EnodeURL.fromString( + "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.3:5678?discport=30303"); + + final SyncStatusNodePermissioningProvider provider = + new SyncStatusNodePermissioningProvider( + synchronizer, Lists.newArrayList(bootnode), metricsSystem); + + boolean isPermitted = provider.isPermitted(enode1, enodeWithDiscoveryPort); + + assertThat(isPermitted).isTrue(); + } } diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/util/PermissioningConfigurationValidator.java b/pantheon/src/main/java/tech/pegasys/pantheon/util/PermissioningConfigurationValidator.java index 41265f6031..d13a991385 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/util/PermissioningConfigurationValidator.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/util/PermissioningConfigurationValidator.java @@ -15,7 +15,7 @@ package tech.pegasys.pantheon.util; import tech.pegasys.pantheon.ethereum.permissioning.LocalPermissioningConfiguration; import java.net.URI; -import java.util.ArrayList; +import java.net.URISyntaxException; import java.util.Collection; import java.util.List; import java.util.stream.Collectors; @@ -26,16 +26,38 @@ public class PermissioningConfigurationValidator { final Collection nodeURIs, final LocalPermissioningConfiguration permissioningConfiguration) throws Exception { - List nodesNotInWhitelist = new ArrayList<>(); + if (permissioningConfiguration.isNodeWhitelistEnabled() && nodeURIs != null) { - nodesNotInWhitelist = + final List whitelistNodesWithoutQueryParam = + permissioningConfiguration.getNodeWhitelist().stream() + .map(PermissioningConfigurationValidator::removeQueryFromURI) + .collect(Collectors.toList()); + + final List nodeURIsNotInWhitelist = nodeURIs.stream() - .filter(enode -> !permissioningConfiguration.getNodeWhitelist().contains(enode)) + .map(PermissioningConfigurationValidator::removeQueryFromURI) + .filter(uri -> !whitelistNodesWithoutQueryParam.contains(uri)) .collect(Collectors.toList()); + + if (!nodeURIsNotInWhitelist.isEmpty()) { + throw new Exception( + "Specified node(s) not in nodes-whitelist " + enodesAsStrings(nodeURIsNotInWhitelist)); + } } - if (!nodesNotInWhitelist.isEmpty()) { - throw new Exception( - "Specified node(s) not in nodes-whitelist " + enodesAsStrings(nodesNotInWhitelist)); + } + + private static URI removeQueryFromURI(final URI uri) { + try { + return new URI( + uri.getScheme(), + uri.getUserInfo(), + uri.getHost(), + uri.getPort(), + uri.getPath(), + null, + uri.getFragment()); + } catch (URISyntaxException e) { + throw new IllegalArgumentException(e); } } diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java index b04b9d7e17..a361ee4b7a 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java @@ -28,6 +28,7 @@ import java.nio.file.Path; import java.util.List; import java.util.stream.Collectors; +import com.google.common.collect.Lists; import com.google.common.io.Resources; import org.junit.Test; @@ -89,4 +90,35 @@ public class LocalPermissioningConfigurationValidatorTest { "enode://94c15d1b9e2fe7ce56e458b9a3b672ef11894ddedd0c6f247e0f1d3487f52b66208fb4aeb8179fce6e3a749ea93ed147c37976d67af557508d199d9594c35f09@192.81.208.223:30303"); } } + + @Test + public void nodeWhitelistCheckShouldIgnoreDiscoveryPortParam() throws Exception { + final URL configFile = this.getClass().getResource(PERMISSIONING_CONFIG); + final Path toml = Files.createTempFile("toml", ""); + toml.toFile().deleteOnExit(); + Files.write(toml, Resources.toByteArray(configFile)); + + final LocalPermissioningConfiguration permissioningConfiguration = + PermissioningConfigurationBuilder.permissioningConfiguration( + true, toml.toAbsolutePath().toString(), true, toml.toAbsolutePath().toString()); + + // This node is defined in the PERMISSIONING_CONFIG file without the discovery port + final URI enodeURL = + URI.create( + "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.9:4567?discport=30303"); + + // In an URI comparison the URLs should not match + boolean isInWhitelist = permissioningConfiguration.getNodeWhitelist().contains(enodeURL); + assertThat(isInWhitelist).isFalse(); + + // However, for the whitelist validation, we should ignore the discovery port and don't throw an + // error + try { + PermissioningConfigurationValidator.areAllNodesAreInWhitelist( + Lists.newArrayList(enodeURL), permissioningConfiguration); + } catch (Exception e) { + fail( + "Exception not expected. Validation of nodes in whitelist should ignore the optional discovery port param."); + } + } }