From a964b04962424097b332bb394559f33abb145fba Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Fri, 14 Dec 2018 13:47:05 +1000 Subject: [PATCH] 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 --- .../p2p/config/DiscoveryConfiguration.java | 13 +- .../config/PermissioningConfiguration.java | 4 + .../PermissioningConfigurationTest.java | 20 ++- .../pegasys/pantheon/cli/PantheonCommand.java | 32 +++- .../pantheon/cli/PantheonCommandTest.java | 167 +++++++++++++----- 5 files changed, 185 insertions(+), 51 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/DiscoveryConfiguration.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/DiscoveryConfiguration.java index f68b888eb3..6473a47605 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/DiscoveryConfiguration.java +++ b/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 getBootstrapPeersFromGenericCollection( + final Collection bootstrapPeers) { + List 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() { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfiguration.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfiguration.java index 71a9bc0996..b5469e8866 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfiguration.java +++ b/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); + } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfigurationTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfigurationTest.java index 526771772f..a5fc177441 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/config/PermissioningConfigurationTest.java +++ b/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(); + } } diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 9146d0df6e..85e163b732 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/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 bootnodes = + DiscoveryConfiguration.getBootstrapPeersFromGenericCollection( + ethNetworkConfig.getBootNodes()); + if (permissioningConfiguration.isNodeWhitelistSet() && bootnodes != null) { + List 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) { diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java index cc150ff232..0af4f5b79d 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ 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 = { - "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)); + parseCommand("--bootnodes", String.join(",", validENodeStrings)); 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() { - parseCommand("--nodes-whitelist"); + public void callingWithInvalidNodesWhitelistMustDisplayErrorAndUsage() { + 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 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() { - parseCommand("--nodes-whitelist", "invalid_enode_url"); + public void nodesWhitelistOptionMustIncludeBootnodes() { + parseCommand( + "--bootnodes", + String.join(",", validENodeStrings[0], validENodeStrings[1]), + "--nodes-whitelist", + String.join(",", validENodeStrings)); + + verify(mockRunnerBuilder) + .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture()); + verify(mockRunnerBuilder).build(); + + final List enodeURLAsStringList = + permissioningConfigurationArgumentCaptor + .getValue() + .getNodeWhitelist() + .stream() + .map(URI::toString) + .collect(Collectors.toList()); + + assertThat(enodeURLAsStringList).containsExactlyInAnyOrder(validENodeStrings); + assertThat(permissioningConfigurationArgumentCaptor.getValue().isNodeWhitelistSet()).isTrue(); + 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); + assertThat(commandErrorOutput.toString()).isEmpty(); } @Test - public void nodesWhitelistOptionMustBeUsed() { - 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" - }; - parseCommand("--nodes-whitelist", String.join(",", nodes)); + public void nodesWhitelistOptionWhichDoesNotIncludeBootnodesMustDisplayError() { + final String bootNodeNotWhitelisted = "enode://" + VALID_NODE_ID + "@192.168.0.9:4567"; + parseCommand( + "--bootnodes", + String.join(",", bootNodeNotWhitelisted, validENodeStrings[2]), + "--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 ropstenWithNodesWhitelistOptionWhichDoesIncludeRopstenBootnodesMustNotDisplayError() { + parseCommand("--ropsten", "--nodes-whitelist", String.join(",", ropstenBootnodes)); verify(mockRunnerBuilder) .permissioningConfiguration(permissioningConfigurationArgumentCaptor.capture()); verify(mockRunnerBuilder).build(); - assertThat( - permissioningConfigurationArgumentCaptor - .getValue() - .getNodeWhitelist() - .stream() - .map(URI::toString) - .collect(Collectors.toList())) - .containsExactlyInAnyOrder(nodes); + final List enodeURLAsStringList = + permissioningConfigurationArgumentCaptor + .getValue() + .getNodeWhitelist() + .stream() + .map(URI::toString) + .collect(Collectors.toList()); + + 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); }