PIE-1663: Ignore discport during startup whitelist validation (#1625)

* Ignore discport during startup whitelist validation * Fixing SyncStatusNodePermissioningProvider
PegaSysEng · Jul 2, 2019 · aa40d54 · aa40d54
1 parent e203a9c
commit aa40d54
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 10 deletions.
diff --git a/...n/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/...n/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java
@@ -41,11 +41,15 @@ public 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 boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio
             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/...ys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java b/...ys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java
@@ -103,7 +103,7 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio
       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/...antheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java b/...antheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProviderTest.java
@@ -31,6 +31,7 @@
 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 void whenHasSyncedIsPermittedShouldReturnTrue() {
     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
@@ -15,7 +15,7 @@
 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 static void areAllNodesAreInWhitelist(
       final Collection<URI> nodeURIs,
       final LocalPermissioningConfiguration permissioningConfiguration)
       throws Exception {
-    List<URI> nodesNotInWhitelist = new ArrayList<>();
+
     if (permissioningConfiguration.isNodeWhitelistEnabled() && nodeURIs != null) {
-      nodesNotInWhitelist =
+      final List<URI> whitelistNodesWithoutQueryParam =
+          permissioningConfiguration.getNodeWhitelist().stream()
+              .map(PermissioningConfigurationValidator::removeQueryFromURI)
+              .collect(Collectors.toList());
+
+      final List<URI> 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/...rc/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java b/...rc/test/java/tech/pegasys/pantheon/util/LocalPermissioningConfigurationValidatorTest.java
@@ -28,6 +28,7 @@
 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 void nodesWhitelistOptionWhichDoesNotIncludeBootnodesMustError() throws E
               "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.");
+    }
+  }
 }