NC-1930 ensure bootnodes are a subset of node whitelist (#414)

* ensure bootnodes are in nodes-whitelist

* added test for network config eg ropsten with --node-whitelist

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/DiscoveryConfiguration.java

@@ -137,15 +137,22 @@ public class DiscoveryConfiguration {
   }
 
   public DiscoveryConfiguration setBootstrapPeers(final Collection<?> bootstrapPeers) {
+    this.bootstrapPeers = getBootstrapPeersFromGenericCollection(bootstrapPeers);
+    return this;
+  }
+
+  public static List<Peer> getBootstrapPeersFromGenericCollection(
+      final Collection<?> bootstrapPeers) {
+    List<Peer> bootnodes;
     if (bootstrapPeers.stream().allMatch(URI.class::isInstance)) {
-      this.bootstrapPeers =
+      bootnodes =
           bootstrapPeers.stream().map(URI.class::cast).map(DefaultPeer::fromURI).collect(toList());
     } else if (bootstrapPeers.stream().allMatch(Peer.class::isInstance)) {
-      this.bootstrapPeers = bootstrapPeers.stream().map(Peer.class::cast).collect(toList());
+      bootnodes = bootstrapPeers.stream().map(Peer.class::cast).collect(toList());
     } else {
       throw new IllegalArgumentException("Expected a list of Peers or a list of enode URIs");
     }
-    return this;
+    return bootnodes;
   }
 
   public String getAdvertisedHost() {

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfiguration.java

@@ -41,4 +41,8 @@ public class PermissioningConfiguration {
   public boolean isNodeWhitelistSet() {
     return nodeWhitelistSet;
   }
+
+  public boolean contains(final URI node) {
+    return !isNodeWhitelistSet() || nodeWhitelist.contains(node);
+  }
 }

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfigurationTest.java

@@ -21,6 +21,12 @@ import org.junit.Test;
 
 public class PermissioningConfigurationTest {
 
+  final URI[] nodes = {
+    URI.create("enode://001@123:4567"),
+    URI.create("enode://002@123:4567"),
+    URI.create("enode://003@123:4567")
+  };
+
   @Test
   public void defaultConfiguration() {
     final PermissioningConfiguration configuration = PermissioningConfiguration.createDefault();
@@ -30,11 +36,6 @@ public class PermissioningConfigurationTest {
 
   @Test
   public void setNodeWhitelist() {
-    final URI[] nodes = {
-      URI.create("enode://001@123:4567"),
-      URI.create("enode://002@123:4567"),
-      URI.create("enode://003@123:4567")
-    };
     final PermissioningConfiguration configuration = PermissioningConfiguration.createDefault();
     configuration.setNodeWhitelist(Arrays.asList(nodes));
     assertThat(configuration.getNodeWhitelist()).containsExactlyInAnyOrder(nodes);
@@ -42,10 +43,17 @@ public class PermissioningConfigurationTest {
   }
 
   @Test
-  public void setNodeWhiteListPassingNull() {
+  public void setNodeWhiteListPassingNullShouldNotChangeWhitelistSetFlag() {
     final PermissioningConfiguration configuration = PermissioningConfiguration.createDefault();
     configuration.setNodeWhitelist(null);
     assertThat(configuration.getNodeWhitelist()).isEmpty();
     assertThat(configuration.isNodeWhitelistSet()).isFalse();
   }
+
+  @Test
+  public void contains() {
+    final PermissioningConfiguration configuration = PermissioningConfiguration.createDefault();
+    configuration.setNodeWhitelist(Arrays.asList(nodes));
+    assertThat(configuration.contains(URI.create("enode://001@123:4567"))).isTrue();
+  }
 }

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -34,8 +34,10 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcConfiguration;
 import tech.pegasys.pantheon.ethereum.jsonrpc.RpcApi;
 import tech.pegasys.pantheon.ethereum.jsonrpc.RpcApis;
 import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.WebSocketConfiguration;
+import tech.pegasys.pantheon.ethereum.p2p.config.DiscoveryConfiguration;
 import tech.pegasys.pantheon.ethereum.p2p.config.PermissioningConfiguration;
 import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer;
+import tech.pegasys.pantheon.ethereum.p2p.peers.Peer;
 import tech.pegasys.pantheon.ethereum.util.InvalidConfigurationException;
 import tech.pegasys.pantheon.metrics.MetricsSystem;
 import tech.pegasys.pantheon.metrics.prometheus.PrometheusMetricsSystem;
@@ -53,6 +55,7 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import com.google.common.io.Resources;
@@ -473,7 +476,11 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
           new CommandLine(this),
           "Unable to connect to multiple networks simultaneously. Specify one of --ropsten, --rinkeby or --goerli");
     }
+
     final EthNetworkConfig ethNetworkConfig = ethNetworkConfig();
+    PermissioningConfiguration permissioningConfiguration = permissioningConfiguration();
+    ensureAllBootnodesAreInWhitelist(ethNetworkConfig, permissioningConfiguration);
+
     synchronize(
         buildController(),
         noPeerDiscovery,
@@ -482,7 +489,30 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
         p2pHostAndPort,
         jsonRpcConfiguration(),
         webSocketConfiguration(),
-        permissioningConfiguration());
+        permissioningConfiguration);
+  }
+
+  private void ensureAllBootnodesAreInWhitelist(
+      final EthNetworkConfig ethNetworkConfig,
+      final PermissioningConfiguration permissioningConfiguration) {
+    List<Peer> bootnodes =
+        DiscoveryConfiguration.getBootstrapPeersFromGenericCollection(
+            ethNetworkConfig.getBootNodes());
+    if (permissioningConfiguration.isNodeWhitelistSet() && bootnodes != null) {
+      List<Peer> whitelist =
+          permissioningConfiguration
+              .getNodeWhitelist()
+              .stream()
+              .map(DefaultPeer::fromURI)
+              .collect(Collectors.toList());
+      for (Peer bootnode : bootnodes) {
+        if (!whitelist.contains(bootnode)) {
+          throw new ParameterException(
+              new CommandLine(this),
+              "Cannot start node with bootnode(s) that are not in nodes-whitelist " + bootnode);
+        }
+      }
+    }
   }
 
   private static int trueCount(final Boolean... b) {

pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java

@@ -49,6 +49,7 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -73,6 +74,17 @@ public class PantheonCommandTest extends CommandTestAbstract {
   private static final WebSocketConfiguration defaultWebSocketConfiguration;
   private static final String GENESIS_CONFIG_TESTDATA = "genesis_config";
 
+  final String[] validENodeStrings = {
+    "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
+    "enode://" + VALID_NODE_ID + "@192.168.0.2:4567",
+    "enode://" + VALID_NODE_ID + "@192.168.0.3:4567"
+  };
+
+  final String[] ropstenBootnodes = {
+    "enode://6332792c4a00e3e4ee0926ed89e0d27ef985424d97b6a45bf0f23e51f0dcb5e66b875777506458aea7af6f9e4ffb69f43f3778ee73c81ed9d34c51c4b16b0b0f@52.232.243.152:30303",
+    "enode://94c15d1b9e2fe7ce56e458b9a3b672ef11894ddedd0c6f247e0f1d3487f52b66208fb4aeb8179fce6e3a749ea93ed147c37976d67af557508d199d9594c35f09@192.81.208.223:30303"
+  };
+
   static {
     final JsonRpcConfiguration rpcConf = JsonRpcConfiguration.createDefault();
     rpcConf.addRpcApi(CliqueRpcApis.CLIQUE);
@@ -447,18 +459,13 @@ public class PantheonCommandTest extends CommandTestAbstract {
 
   @Test
   public void bootnodesOptionMustBeUsed() {
-    final String[] nodes = {
+    parseCommand("--bootnodes", String.join(",", validENodeStrings));
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567"
-    };
-    parseCommand("--bootnodes", String.join(",", nodes));
 
     verify(mockRunnerBuilder).bootstrapPeers(uriListArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
     assertThat(uriListArgumentCaptor.getValue())
-        .isEqualTo(Stream.of(nodes).map(URI::create).collect(Collectors.toList()));
+        .isEqualTo(Stream.of(validENodeStrings).map(URI::create).collect(Collectors.toList()));
 
     assertThat(commandOutput.toString()).isEmpty();
     assertThat(commandErrorOutput.toString()).isEmpty();
@@ -487,14 +494,39 @@ public class PantheonCommandTest extends CommandTestAbstract {
   }
 
   @Test
-  public void callingWithNodesWhitelistOptionButNoValueMustNotError() {
+  public void callingWithInvalidNodesWhitelistMustDisplayErrorAndUsage() {
-    parseCommand("--nodes-whitelist");
+    parseCommand("--nodes-whitelist", "invalid_enode_url");
+    assertThat(commandOutput.toString()).isEmpty();
+    /*
+     Because of the way Picocli handles errors parsing errors for lists with arity 0..*, we don't
+     get the nice error msg with that was wrong. It only shows to the user the values that weren't
+     parsed correctly.
+    */
+    final String expectedErrorOutputStart = "Unmatched argument: invalid_enode_url";
+    assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart);
+  }
+
+  @Test
+  public void nodesWhitelistOptionMustBeUsed() {
+    parseCommand(
+        "--nodes-whitelist",
+        String.join(",", validENodeStrings),
+        "--bootnodes",
+        validENodeStrings[0]);
 
     verify(mockRunnerBuilder)
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    assertThat(permissioningConfigurationArgumentCaptor.getValue().getNodeWhitelist()).isEmpty();
+    final List<String> enodeURLAsStringList =
+        permissioningConfigurationArgumentCaptor
+            .getValue()
+            .getNodeWhitelist()
+            .stream()
+            .map(URI::toString)
+            .collect(Collectors.toList());
+
+    assertThat(enodeURLAsStringList).containsExactlyInAnyOrder(validENodeStrings);
     assertThat(permissioningConfigurationArgumentCaptor.getValue().isNodeWhitelistSet()).isTrue();
 
     assertThat(commandOutput.toString()).isEmpty();
@@ -502,45 +534,93 @@ public class PantheonCommandTest extends CommandTestAbstract {
   }
 
   @Test
-  public void callingWithInvalidNodesWhitelistMustDisplayErrorAndUsage() {
+  public void nodesWhitelistOptionMustIncludeBootnodes() {
-    parseCommand("--nodes-whitelist", "invalid_enode_url");
+    parseCommand(
+        "--bootnodes",
+        String.join(",", validENodeStrings[0], validENodeStrings[1]),
+        "--nodes-whitelist",
+        String.join(",", validENodeStrings));
+
+    verify(mockRunnerBuilder)
+        .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
+    verify(mockRunnerBuilder).build();
+
+    final List<String> enodeURLAsStringList =
+        permissioningConfigurationArgumentCaptor
+            .getValue()
+            .getNodeWhitelist()
+            .stream()
+            .map(URI::toString)
+            .collect(Collectors.toList());
+
+    assertThat(enodeURLAsStringList).containsExactlyInAnyOrder(validENodeStrings);
+    assertThat(permissioningConfigurationArgumentCaptor.getValue().isNodeWhitelistSet()).isTrue();
+
     assertThat(commandOutput.toString()).isEmpty();
-    /*
+    assertThat(commandErrorOutput.toString()).isEmpty();
-     Because of the way Picocli handles errors parsing errors for lists with arity 0..*, we don't
-     get the nice error msg with that was wrong. It only shows to the user the values that weren't
-     parsed correctly.
-    */
-    final String expectedErrorOutputStart = "Unmatched argument: invalid_enode_url";
-    assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart);
   }
 
   @Test
-  public void nodesWhitelistOptionMustBeUsed() {
+  public void nodesWhitelistOptionWhichDoesNotIncludeBootnodesMustDisplayError() {
-    final String[] nodes = {
+    final String bootNodeNotWhitelisted = "enode://" + VALID_NODE_ID + "@192.168.0.9:4567";
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
+    parseCommand(
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
+        "--bootnodes",
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567"
+        String.join(",", bootNodeNotWhitelisted, validENodeStrings[2]),
-    };
+        "--nodes-whitelist",
-    parseCommand("--nodes-whitelist", String.join(",", nodes));
+        String.join(",", validENodeStrings));
+
+    verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains("Cannot start node with bootnode(s) that are not in nodes-whitelist");
+  }
+
+  @Test
+  public void ropstenWithNodesWhitelistOptionWhichDoesIncludeRopstenBootnodesMustNotDisplayError() {
+    parseCommand("--ropsten", "--nodes-whitelist", String.join(",", ropstenBootnodes));
 
     verify(mockRunnerBuilder)
         .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture());
     verify(mockRunnerBuilder).build();
 
-    assertThat(
+    final List<String> enodeURLAsStringList =
         permissioningConfigurationArgumentCaptor
             .getValue()
             .getNodeWhitelist()
             .stream()
             .map(URI::toString)
-                .collect(Collectors.toList()))
+            .collect(Collectors.toList());
-        .containsExactlyInAnyOrder(nodes);
+
+    assertThat(enodeURLAsStringList).containsExactlyInAnyOrder(ropstenBootnodes);
     assertThat(permissioningConfigurationArgumentCaptor.getValue().isNodeWhitelistSet()).isTrue();
 
     assertThat(commandOutput.toString()).isEmpty();
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void ropstenWithNodesWhitelistOptionWhichDoesNotIncludeRopstenBootnodesMustDisplayError() {
+    parseCommand("--ropsten", "--nodes-whitelist", String.join(",", validENodeStrings));
+
+    verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains("Cannot start node with bootnode(s) that are not in nodes-whitelist");
+  }
+
+  @Test
+  public void nodesWhitelistWithEmptyListAndNonEmptyBootnodesMustDisplayError() {
+    parseCommand("--bootnodes", String.join(",", validENodeStrings[0]), "--nodes-whitelist");
+
+    verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains("Cannot start node with bootnode(s) that are not in nodes-whitelist");
+  }
+
   @Test
   public void bannedNodeIdsOptionMustBeUsed() {
     final String[] nodes = {"0001", "0002", "0003"};
@@ -651,6 +731,16 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void rpcApisPropertyWithInvalidEntryMustDisplayError() {
+    parseCommand("--rpc-api", "BOB");
+
+    verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).contains("Invalid value for option '--rpc-api'");
+  }
+
   @Test
   public void jsonRpcHostAndPortOptionMustBeUsed() {
 
@@ -1013,18 +1103,13 @@ public class PantheonCommandTest extends CommandTestAbstract {
   }
 
   private void networkValuesCanBeOverridden(final String network) throws Exception {
-    final String[] nodes = {
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567",
-      "enode://" + VALID_NODE_ID + "@192.168.0.1:4567"
-    };
     final Path genesisFile = createFakeGenesisFile();
     parseCommand(
         "--" + network,
         "--network-id",
         "1",
         "--bootnodes",
-        String.join(",", nodes),
+        String.join(",", validENodeStrings),
         "--genesis",
         genesisFile.toString());
 
@@ -1038,7 +1123,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).isEmpty();
     assertThat(networkArg.getValue().getGenesisConfig()).isEqualTo("genesis_config");
     assertThat(networkArg.getValue().getBootNodes())
-        .isEqualTo(Stream.of(nodes).map(URI::create).collect(Collectors.toList()));
+        .isEqualTo(Stream.of(validENodeStrings).map(URI::create).collect(Collectors.toList()));
     assertThat(networkArg.getValue().getNetworkId()).isEqualTo(1);
   }