From c8baa9ae76580bc79461a46b980b4db304cbaf1d Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Tue, 13 Nov 2018 07:41:38 -0700 Subject: [PATCH] Ban Peers via CLI (#254) * Ban Peers via CLI As part of working on #251 I needed to be able to ban certain nodes from my connection pool and let others connect. This is a general solution to add a --banned-nodeids CLI flag where the nodeIds of banned nodes are listed. --- .../dsl/node/ThreadPantheonNodeRunner.java | 4 +- .../ethereum/p2p/peers/PeerBlacklist.java | 33 ++++++- .../ethereum/p2p/peers/PeerBlacklistTest.java | 85 +++++++++++++++++-- .../tech/pegasys/pantheon/RunnerBuilder.java | 10 ++- .../pegasys/pantheon/cli/PantheonCommand.java | 12 ++- .../tech/pegasys/pantheon/RunnerTest.java | 6 +- .../pantheon/cli/PantheonCommandTest.java | 65 +++++++++++++- 7 files changed, 198 insertions(+), 17 deletions(-) diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java index 08bb5a8280..9da28affd0 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java @@ -22,6 +22,7 @@ import tech.pegasys.pantheon.controller.PantheonController; import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration.Builder; import java.io.IOException; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutorService; @@ -76,7 +77,8 @@ public class ThreadPantheonNodeRunner implements PantheonNodeRunner { 25, node.jsonRpcConfiguration(), node.webSocketConfiguration(), - node.homeDirectory()); + node.homeDirectory(), + Collections.emptySet()); nodeExecutor.submit(runner::execute); diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java index 4f797d30d2..a0535cdde3 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java @@ -25,6 +25,19 @@ import java.util.Set; import com.google.common.collect.ImmutableSet; +/** + * A list of nodes that the running client will not communicate with. This can be because of network + * issues, protocol issues, or by being explicitly set on the command line. + * + *

Peers are stored and identified strictly by their nodeId, the convenience methods taking + * {@link Peer}s and {@link PeerConnection}s redirect to the methods that take {@link BytesValue} + * object that represent the node ID of the banned nodes. + * + *

The storage list is not infinite. A default cap of 500 is applied and nodes are removed on a + * first added first removed basis. Adding a new copy of the same node will not affect the priority + * for removal. The exception to this is a list of banned nodes passed in by reference to the + * constructor. This list neither adds nor removes from that list passed in. + */ public class PeerBlacklist implements DisconnectCallback { private static final int DEFAULT_BLACKLIST_CAP = 500; @@ -46,16 +59,28 @@ public class PeerBlacklist implements DisconnectCallback { } })); - public PeerBlacklist(final int blacklistCap) { + /** These nodes are always banned for the life of this list. They are not subject to rollover. */ + private final Set bannedNodeIds; + + public PeerBlacklist(final int blacklistCap, final Set bannedNodeIds) { this.blacklistCap = blacklistCap; + this.bannedNodeIds = bannedNodeIds; + } + + public PeerBlacklist(final int blacklistCap) { + this(blacklistCap, Collections.emptySet()); + } + + public PeerBlacklist(final Set bannedNodeIds) { + this(DEFAULT_BLACKLIST_CAP, bannedNodeIds); } public PeerBlacklist() { - this(DEFAULT_BLACKLIST_CAP); + this(DEFAULT_BLACKLIST_CAP, Collections.emptySet()); } private boolean contains(final BytesValue nodeId) { - return blacklistedNodeIds.contains(nodeId); + return blacklistedNodeIds.contains(nodeId) || bannedNodeIds.contains(nodeId); } public boolean contains(final PeerConnection peer) { @@ -70,7 +95,7 @@ public class PeerBlacklist implements DisconnectCallback { add(peer.getId()); } - private void add(final BytesValue peerId) { + public void add(final BytesValue peerId) { blacklistedNodeIds.add(peerId); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java index b31244606f..40af78fd87 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java @@ -21,13 +21,80 @@ import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; import tech.pegasys.pantheon.util.bytes.BytesValue; +import java.util.Collections; + import org.junit.Test; public class PeerBlacklistTest { private int nodeIdValue = 1; @Test - public void doesNotBlacklistPeerForNormalDisconnect() throws Exception { + public void directlyAddingPeerWorks() { + final PeerBlacklist blacklist = new PeerBlacklist(); + final Peer peer = generatePeer(); + + assertThat(blacklist.contains(peer)).isFalse(); + + blacklist.add(peer); + + assertThat(blacklist.contains(peer)).isTrue(); + } + + @Test + public void directlyAddingPeerByPeerIdWorks() { + final PeerBlacklist blacklist = new PeerBlacklist(); + final Peer peer = generatePeer(); + + assertThat(blacklist.contains(peer)).isFalse(); + + blacklist.add(peer.getId()); + + assertThat(blacklist.contains(peer)).isTrue(); + } + + @Test + public void banningPeerByPeerIdWorks() { + final Peer peer = generatePeer(); + final PeerBlacklist blacklist = new PeerBlacklist(Collections.singleton(peer.getId())); + + assertThat(blacklist.contains(peer)).isTrue(); + + blacklist.add(peer.getId()); + + assertThat(blacklist.contains(peer)).isTrue(); + } + + @Test + public void bannedNodesDoNotRollover() { + final Peer bannedPeer = generatePeer(); + final Peer peer1 = generatePeer(); + final Peer peer2 = generatePeer(); + final Peer peer3 = generatePeer(); + final PeerBlacklist blacklist = new PeerBlacklist(2, Collections.singleton(bannedPeer.getId())); + + assertThat(blacklist.contains(bannedPeer)).isTrue(); + assertThat(blacklist.contains(peer1)).isFalse(); + assertThat(blacklist.contains(peer2)).isFalse(); + assertThat(blacklist.contains(peer3)).isFalse(); + + // fill to the limit + blacklist.add(peer1.getId()); + blacklist.add(peer2.getId()); + assertThat(blacklist.contains(bannedPeer)).isTrue(); + assertThat(blacklist.contains(peer1)).isTrue(); + assertThat(blacklist.contains(peer2)).isTrue(); + assertThat(blacklist.contains(peer3)).isFalse(); + + // trigger rollover + blacklist.add(peer3.getId()); + assertThat(blacklist.contains(bannedPeer)).isTrue(); + assertThat(blacklist.contains(peer1)).isFalse(); + assertThat(blacklist.contains(peer2)).isTrue(); + assertThat(blacklist.contains(peer3)).isTrue(); + } + + @Test + public void doesNotBlacklistPeerForNormalDisconnect() { final PeerBlacklist blacklist = new PeerBlacklist(); final PeerConnection peer = generatePeerConnection(); @@ -39,7 +106,7 @@ public class PeerBlacklistTest { } @Test - public void blacklistPeerForBadBehavior() throws Exception { + public void blacklistPeerForBadBehavior() { final PeerBlacklist blacklist = new PeerBlacklist(); final PeerConnection peer = generatePeerConnection(); @@ -52,7 +119,7 @@ public class PeerBlacklistTest { } @Test - public void doesNotBlacklistPeerForOurBadBehavior() throws Exception { + public void doesNotBlacklistPeerForOurBadBehavior() { final PeerBlacklist blacklist = new PeerBlacklist(); final PeerConnection peer = generatePeerConnection(); @@ -64,7 +131,7 @@ public class PeerBlacklistTest { } @Test - public void blacklistIncompatiblePeer() throws Exception { + public void blacklistIncompatiblePeer() { final PeerBlacklist blacklist = new PeerBlacklist(); final PeerConnection peer = generatePeerConnection(); @@ -76,7 +143,7 @@ public class PeerBlacklistTest { } @Test - public void blacklistIncompatiblePeerWhoIssuesDisconnect() throws Exception { + public void blacklistIncompatiblePeerWhoIssuesDisconnect() { final PeerBlacklist blacklist = new PeerBlacklist(); final PeerConnection peer = generatePeerConnection(); @@ -88,7 +155,7 @@ public class PeerBlacklistTest { } @Test - public void capsSizeOfList() throws Exception { + public void capsSizeOfList() { final PeerBlacklist blacklist = new PeerBlacklist(2); final PeerConnection peer1 = generatePeerConnection(); @@ -136,4 +203,10 @@ public class PeerBlacklistTest { return peer; } + + private Peer generatePeer() { + final byte[] id = new byte[64]; + id[0] = (byte) nodeIdValue++; + return new DefaultPeer(BytesValue.wrap(id), "10.9.8.7", 65535, 65534); + } } diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index b01d73c3fe..0ff1c0f472 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -54,6 +54,7 @@ import tech.pegasys.pantheon.ethereum.p2p.netty.NettyP2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; +import tech.pegasys.pantheon.util.bytes.BytesValue; import java.nio.file.Path; import java.util.Collection; @@ -78,7 +79,8 @@ public class RunnerBuilder { final int maxPeers, final JsonRpcConfiguration jsonRpcConfiguration, final WebSocketConfiguration webSocketConfiguration, - final Path dataDir) { + final Path dataDir, + final Collection bannedNodeIds) { Preconditions.checkNotNull(pantheonController); @@ -122,6 +124,10 @@ public class RunnerBuilder { .setClientId(PantheonInfo.version()) .setSupportedProtocols(subProtocols); + final PeerBlacklist peerBlacklist = + new PeerBlacklist( + bannedNodeIds.stream().map(BytesValue::fromHexString).collect(Collectors.toSet())); + final NetworkRunner networkRunner = NetworkRunner.builder() .protocolManagers(protocolManagers) @@ -134,7 +140,7 @@ public class RunnerBuilder { networkConfig, caps, PeerRequirement.aggregateOf(protocolManagers), - new PeerBlacklist())) + peerBlacklist)) .build(); final Synchronizer synchronizer = pantheonController.getSynchronizer(); 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 a8793a891f..4cf475d4e3 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -44,6 +44,7 @@ import java.nio.file.InvalidPathException; 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.function.Function; @@ -199,6 +200,14 @@ public class PantheonCommand implements Runnable { ) private final Integer maxTrailingPeers = Integer.MAX_VALUE; + @Option( + names = {"--banned-nodeids"}, + description = "A list of node IDs to ban from the p2p network.", + split = ",", + arity = "1..*" + ) + private final Collection bannedNodeIds = null; + // TODO: Re-enable as per NC-1057/NC-1681 // @Option( // names = {"--sync-mode"}, @@ -491,7 +500,8 @@ public class PantheonCommand implements Runnable { maxPeers, jsonRpcConfiguration, webSocketConfiguration, - dataDir); + dataDir, + bannedNodeIds == null ? Collections.emptySet() : bannedNodeIds); addShutdownHook(runner); runner.execute(); diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java index 3984adfb81..b5268761cf 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java @@ -127,7 +127,8 @@ public final class RunnerTest { 3, aheadJsonRpcConfiguration, aheadWebSocketConfiguration, - dbAhead); + dbAhead, + Collections.emptySet()); try { executorService.submit(runnerAhead::execute); @@ -161,7 +162,8 @@ public final class RunnerTest { 3, behindJsonRpcConfiguration, behindWebSocketConfiguration, - dbBehind); + dbBehind, + Collections.emptySet()); executorService.submit(runnerBehind::execute); final Call.Factory client = new OkHttpClient(); 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 9e0f982646..d15ad9f454 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -93,6 +93,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), any(), + any(), any())) .thenReturn(mockRunner); } @@ -135,6 +136,7 @@ public class PantheonCommandTest extends CommandTestAbstract { eq(25), eq(defaultJsonRpcConfiguration), eq(defaultWebSocketConfiguration), + any(), any()); final ArgumentCaptor miningArg = @@ -262,6 +264,7 @@ public class PantheonCommandTest extends CommandTestAbstract { eq(42), eq(jsonRpcConfiguration), eq(webSocketConfiguration), + any(), any()); final Collection nodes = @@ -315,6 +318,7 @@ public class PantheonCommandTest extends CommandTestAbstract { eq(25), eq(jsonRpcConfiguration), eq(webSocketConfiguration), + any(), any()); verify(mockControllerBuilder).build(any(), any(), any(), eq(false), any(), eq(false)); @@ -368,7 +372,17 @@ public class PantheonCommandTest extends CommandTestAbstract { verify(mockRunnerBuilder) .build( - any(), any(), eq(false), any(), anyString(), anyInt(), anyInt(), any(), any(), any()); + any(), + any(), + eq(false), + any(), + anyString(), + anyInt(), + anyInt(), + any(), + any(), + any(), + any()); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -383,6 +397,15 @@ public class PantheonCommandTest extends CommandTestAbstract { assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); } + @Test + public void callingWithBannedNodeidsOptionButNoValueMustDisplayErrorAndUsage() { + parseCommand("--banned-nodeids"); + assertThat(commandOutput.toString()).isEmpty(); + final String expectedErrorOutputStart = + "Missing required parameter for option '--banned-nodeids' at index 0 ()"; + assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); + } + @Test public void bootnodesOptionMustBeUsed() { final String[] nodes = {"enode://001@123:4567", "enode://002@123:4567", "enode://003@123:4567"}; @@ -399,6 +422,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), any(), + any(), any()); assertThat(stringListArgumentCaptor.getValue().toArray()).isEqualTo(nodes); @@ -407,6 +431,31 @@ public class PantheonCommandTest extends CommandTestAbstract { assertThat(commandErrorOutput.toString()).isEmpty(); } + @Test + public void banNodeIdsOptionMustBeUsed() { + final String[] nodes = {"0001", "0002", "0003"}; + parseCommand("--banned-nodeids", String.join(",", nodes)); + + verify(mockRunnerBuilder) + .build( + any(), + any(), + anyBoolean(), + any(), + anyString(), + anyInt(), + anyInt(), + any(), + any(), + any(), + stringListArgumentCaptor.capture()); + + assertThat(stringListArgumentCaptor.getValue().toArray()).isEqualTo(nodes); + + assertThat(commandOutput.toString()).isEmpty(); + assertThat(commandErrorOutput.toString()).isEmpty(); + } + @Test public void p2pHostAndPortOptionMustBeUsed() { @@ -425,6 +474,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), any(), + any(), any()); assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); @@ -451,6 +501,7 @@ public class PantheonCommandTest extends CommandTestAbstract { intArgumentCaptor.capture(), any(), any(), + any(), any()); assertThat(intArgumentCaptor.getValue()).isEqualTo(maxPeers); @@ -497,6 +548,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().isEnabled()).isFalse(); @@ -520,6 +572,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().isEnabled()).isTrue(); @@ -543,6 +596,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getRpcApis()) @@ -570,6 +624,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(host); @@ -595,6 +650,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains().toArray()) @@ -620,6 +676,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains().toArray()) @@ -645,6 +702,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains()) @@ -670,6 +728,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), jsonRpcConfigArgumentCaptor.capture(), any(), + any(), any()); assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains()).isEmpty(); @@ -741,6 +800,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), wsRpcConfigArgumentCaptor.capture(), + any(), any()); assertThat(wsRpcConfigArgumentCaptor.getValue().isEnabled()).isFalse(); @@ -764,6 +824,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), wsRpcConfigArgumentCaptor.capture(), + any(), any()); assertThat(wsRpcConfigArgumentCaptor.getValue().isEnabled()).isTrue(); @@ -787,6 +848,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), wsRpcConfigArgumentCaptor.capture(), + any(), any()); assertThat(wsRpcConfigArgumentCaptor.getValue().getRpcApis()) @@ -813,6 +875,7 @@ public class PantheonCommandTest extends CommandTestAbstract { anyInt(), any(), wsRpcConfigArgumentCaptor.capture(), + any(), any()); assertThat(wsRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(host);