From 3f5c41722ecff21d2e4fbef0fe5ae78089fbf16f Mon Sep 17 00:00:00 2001 From: Lucas Saldanha Date: Tue, 5 Feb 2019 15:11:14 +1300 Subject: [PATCH] [NC-2247] Fixing node whitelist isPermitted check (#766) * [NC-2247] Fixing node whitelist isPermitted check * Typos Signed-off-by: Adrian Sutton --- .../internal/PeerDiscoveryController.java | 5 + .../NodeWhitelistController.java | 19 +++- .../NodeWhitelistControllerTest.java | 105 ++++++++++++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 840ce490d1..f6debecb13 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -211,6 +211,7 @@ public class PeerDiscoveryController { } if (!whitelistIfPresentIsNodePermitted(sender)) { + LOG.trace("Dropping packet from peer not in the whitelist ({})", sender.getEnodeURI()); return; } @@ -222,6 +223,7 @@ public class PeerDiscoveryController { switch (packet.getType()) { case PING: + LOG.trace("Received PING packet from {}", sender.getEnodeURI()); if (!peerBlacklisted && addToPeerTable(peer)) { final PingPacketData ping = packet.getPacketData(PingPacketData.class).get(); respondToPing(ping, packet.getHash(), peer); @@ -230,6 +232,7 @@ public class PeerDiscoveryController { break; case PONG: { + LOG.trace("Received PONG packet from {}", sender.getEnodeURI()); matchInteraction(packet) .ifPresent( interaction -> { @@ -246,6 +249,7 @@ public class PeerDiscoveryController { break; } case NEIGHBORS: + LOG.trace("Received NEIGHBORS packet from {}", sender.getEnodeURI()); matchInteraction(packet) .ifPresent( interaction -> { @@ -271,6 +275,7 @@ public class PeerDiscoveryController { break; case FIND_NEIGHBORS: + LOG.trace("Received FIND_NEIGHBORS packet from {}", sender.getEnodeURI()); if (!peerKnown || peerBlacklisted) { break; } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java index 68c10dde40..b7f4346483 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistController.java @@ -99,7 +99,24 @@ public class NodeWhitelistController { } public boolean isPermitted(final Peer node) { - return (nodesWhitelist.contains(node)); + return nodesWhitelist + .stream() + .anyMatch( + p -> { + boolean idsMatch = node.getId().equals(p.getId()); + boolean hostsMatch = node.getEndpoint().getHost().equals(p.getEndpoint().getHost()); + boolean udpPortsMatch = + node.getEndpoint().getUdpPort() == p.getEndpoint().getUdpPort(); + boolean tcpPortsMatchIfPresent = true; + if (node.getEndpoint().getTcpPort().isPresent() + && p.getEndpoint().getTcpPort().isPresent()) { + tcpPortsMatchIfPresent = + node.getEndpoint().getTcpPort().getAsInt() + == p.getEndpoint().getTcpPort().getAsInt(); + } + + return idsMatch && hostsMatch && udpPortsMatch && tcpPortsMatchIfPresent; + }); } public List getNodesWhitelist() { diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java index fe9311842c..839ba67583 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissioning/NodeWhitelistControllerTest.java @@ -16,8 +16,10 @@ import static org.assertj.core.api.Assertions.assertThat; import static tech.pegasys.pantheon.ethereum.p2p.permissioning.NodeWhitelistController.NodesWhitelistResult; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.permissioning.PermissioningConfiguration; import tech.pegasys.pantheon.ethereum.permissioning.WhitelistOperationResult; +import tech.pegasys.pantheon.util.bytes.BytesValue; import java.util.ArrayList; import java.util.Arrays; @@ -125,4 +127,107 @@ public class NodeWhitelistControllerTest { assertThat(actualResult).isEqualToComparingOnlyGivenFields(expected, "result"); } + + @Test + public void whenNodeIdsAreDifferentItShouldNotBePermitted() { + Peer peer1 = + new DefaultPeer( + BytesValue.fromHexString( + "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303); + Peer peer2 = + new DefaultPeer( + BytesValue.fromHexString( + "0xbbba80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303); + + controller.addNode(peer1); + + assertThat(controller.isPermitted(peer2)).isFalse(); + } + + @Test + public void whenNodesHostsAreDifferentItShouldNotBePermitted() { + Peer peer1 = + new DefaultPeer( + BytesValue.fromHexString( + "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303); + Peer peer2 = + new DefaultPeer( + BytesValue.fromHexString( + "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.2", + 30303); + + controller.addNode(peer1); + + assertThat(controller.isPermitted(peer2)).isFalse(); + } + + @Test + public void whenNodesUdpPortsAreDifferentItShouldNotBePermitted() { + Peer peer1 = + new DefaultPeer( + BytesValue.fromHexString( + "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30301); + Peer peer2 = + new DefaultPeer( + BytesValue.fromHexString( + "0xaaaa80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30302); + + controller.addNode(peer1); + + assertThat(controller.isPermitted(peer2)).isFalse(); + } + + @Test + public void whenCheckingIfNodeIsPermittedTcpPortShouldNotBeConsideredIfAbsent() { + Peer peerWithTcpPortSet = + new DefaultPeer( + BytesValue.fromHexString( + "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303, + 10001); + Peer peerWithoutTcpPortSet = + new DefaultPeer( + BytesValue.fromHexString( + "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303); + + controller.addNode(peerWithoutTcpPortSet); + + assertThat(controller.isPermitted(peerWithTcpPortSet)).isTrue(); + } + + @Test + public void whenCheckingIfNodeIsPermittedTcpPortShouldBeConsideredIfPresent() { + Peer peerWithTcpPortSet = + new DefaultPeer( + BytesValue.fromHexString( + "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303, + 10001); + Peer peerWithDifferentTcpPortSet = + new DefaultPeer( + BytesValue.fromHexString( + "0x6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + "127.0.0.1", + 30303, + 10002); + + controller.addNode(peerWithDifferentTcpPortSet); + + assertThat(controller.isPermitted(peerWithTcpPortSet)).isFalse(); + } }