Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Commit

Permalink
[PAN-1683] Limit the fraction of wire connections initiated by peers (#…
Browse files Browse the repository at this point in the history
…1665)

* [PAN-1683] Limit the fraction of wire connections initiated by peers

To protect against eclipse attacks, we should not allow all of our wire connections to be initiated from network peers.  Some fraction of wire connections should be initiated by our node by connecting to peers in our discovery peer table. This PR ensures the fraction abides the limit.

* change fraction default value and add tests

* make fraction of remote wire connections configurable

- add a cli option to configure the fraction: `--fraction-remote-connections-allowed`
- introduce `Fraction` class to handle the conversion of the CLI option to a double and check if the value is between 0.0 and 1.0
- add tests
- fix broken tests

* remove unused local variable

* fix RunnerTest

* set fraction to 1.0 in PantheonFactoryConfigurationBuilder

* update test

* Introduce --limit-remote-wire-connections-enabled

* fix unused field

* fix conflict

* Update ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/rlpx/RlpxAgent.java

Co-Authored-By: Danno Ferrin <[email protected]>

* Update ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/rlpx/RlpxAgent.java

Co-Authored-By: Danno Ferrin <[email protected]>

* Update ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/rlpx/RlpxAgent.java

Co-Authored-By: Danno Ferrin <[email protected]>

* fix PR discussion and tests
  • Loading branch information
AbdelStark committed Jul 11, 2019
1 parent 105304a commit 17a09af
Show file tree
Hide file tree
Showing 15 changed files with 270 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ public PantheonNode(
this.devMode = devMode;
this.p2pEnabled = p2pEnabled;
this.networkingConfiguration = networkingConfiguration;
this.getNetworkingConfiguration().getRlpx().setFractionRemoteWireConnectionsAllowed(1.0);
this.discoveryEnabled = discoveryEnabled;
plugins.forEach(
pluginName -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ public void startNode(final PantheonNode node) {
.p2pAdvertisedHost(node.getHostName())
.p2pListenPort(0)
.maxPeers(25)
.fractionRemoteConnectionsAllowed(1.0)
.networkingConfiguration(node.getNetworkingConfiguration())
.jsonRpcConfiguration(node.jsonRpcConfiguration())
.webSocketConfiguration(node.webSocketConfiguration())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ public PantheonFactoryConfigurationBuilder revertReasonEnabled() {
}

public PantheonFactoryConfiguration build() {
networkingConfiguration.getRlpx().setFractionRemoteWireConnectionsAllowed(1.0);
return new PantheonFactoryConfiguration(
name,
miningParameters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ public TestNode(
.setRlpx(
RlpxConfiguration.create()
.setBindPort(listenPort)
.setSupportedProtocols(EthProtocol.get()));
.setSupportedProtocols(EthProtocol.get())
.setFractionRemoteWireConnectionsAllowed(1.0));

final GenesisConfigFile genesisConfigFile = GenesisConfigFile.development();
final ProtocolSchedule<Void> protocolSchedule =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
*/
package tech.pegasys.pantheon.ethereum.p2p.config;

import static com.google.common.base.Preconditions.checkState;

import tech.pegasys.pantheon.ethereum.p2p.rlpx.wire.SubProtocol;

import java.util.Arrays;
Expand All @@ -20,10 +22,13 @@
import java.util.Objects;

public class RlpxConfiguration {
public static final Double DEFAULT_FRACTION_REMOTE_CONNECTIONS_ALLOWED = 0.5;
private String clientId = "TestClient/1.0.0";
private String bindHost = "0.0.0.0";
private int bindPort = 30303;
private int maxPeers = 25;
private boolean limitRemoteWireConnectionsEnabled = false;
private double fractionRemoteWireConnectionsAllowed = DEFAULT_FRACTION_REMOTE_CONNECTIONS_ALLOWED;
private List<SubProtocol> supportedProtocols = Collections.emptyList();

public static RlpxConfiguration create() {
Expand Down Expand Up @@ -80,6 +85,29 @@ public RlpxConfiguration setClientId(final String clientId) {
return this;
}

public boolean isLimitRemoteWireConnectionsEnabled() {
return limitRemoteWireConnectionsEnabled;
}

public RlpxConfiguration setLimitRemoteWireConnectionsEnabled(
final boolean limitRemoteWireConnectionsEnabled) {
this.limitRemoteWireConnectionsEnabled = limitRemoteWireConnectionsEnabled;
return this;
}

public double getFractionRemoteWireConnectionsAllowed() {
return fractionRemoteWireConnectionsAllowed;
}

public RlpxConfiguration setFractionRemoteWireConnectionsAllowed(
final double fractionRemoteWireConnectionsAllowed) {
checkState(
fractionRemoteWireConnectionsAllowed > 0.0,
"Fraction of remote connections allowed must be positive.");
this.fractionRemoteWireConnectionsAllowed = fractionRemoteWireConnectionsAllowed;
return this;
}

@Override
public boolean equals(final Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ public class RlpxAgent {
private final PeerConnectionEvents connectionEvents;
private final Subscribers<ConnectCallback> connectSubscribers = Subscribers.create();
private final ConnectionInitializer connectionInitializer;
private final boolean limitRemoteWireConnectionsEnabled;
private final double fractionRemoteConnectionsAllowed;

private final int maxPeers;

Expand All @@ -83,13 +85,17 @@ private RlpxAgent(
final ConnectionInitializer connectionInitializer,
final int maxPeers,
final PeerProperties peerProperties,
final MetricsSystem metricsSystem) {
final MetricsSystem metricsSystem,
final boolean limitRemoteWireConnectionsEnabled,
final double fractionRemoteConnectionsAllowed) {
this.maxPeers = maxPeers;
this.peerProperties = peerProperties;
this.localNode = localNode;
this.peerPermissions = peerPermissions;
this.connectionEvents = connectionEvents;
this.connectionInitializer = connectionInitializer;
this.limitRemoteWireConnectionsEnabled = limitRemoteWireConnectionsEnabled;
this.fractionRemoteConnectionsAllowed = fractionRemoteConnectionsAllowed;

// Setup metrics
connectedPeersCounter =
Expand Down Expand Up @@ -329,6 +335,16 @@ private void handleIncomingConnection(final PeerConnection peerConnection) {
return;
}

// Disconnect if the fraction of wire connections initiated by peers is too high to protect
// against eclipse attacks
if (limitRemoteWireConnectionsEnabled && remoteConnectionExceedsLimit()) {
LOG.warn(
"Fraction of remotely initiated connection is too high, rejecting incoming connection. (max ratio allowed: {})",
fractionRemoteConnectionsAllowed);
peerConnection.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
return;
}

// Track this new connection, deduplicating existing connection if necessary
final AtomicBoolean newConnectionAccepted = new AtomicBoolean(false);
final RlpxConnection inboundConnection = RlpxConnection.inboundConnection(peerConnection);
Expand Down Expand Up @@ -374,6 +390,21 @@ private void handleIncomingConnection(final PeerConnection peerConnection) {
enforceConnectionLimits();
}

private boolean remoteConnectionExceedsLimit() {
final int remotelyInitiatedConnectionsCount =
Math.toIntExact(
connectionsById.values().stream()
.filter(RlpxConnection::isActive)
.filter(RlpxConnection::initiatedRemotely)
.count());
if (remotelyInitiatedConnectionsCount == 0) {
return false;
}
final double fractionRemoteConnections =
(double) remotelyInitiatedConnectionsCount + 1 / (double) maxPeers;
return fractionRemoteConnections > fractionRemoteConnectionsAllowed;
}

private void enforceConnectionLimits() {
connectionsById.values().stream()
.filter(RlpxConnection::isActive)
Expand Down Expand Up @@ -477,7 +508,9 @@ public RlpxAgent build() {
connectionInitializer,
config.getMaxPeers(),
peerProperties,
metricsSystem);
metricsSystem,
config.isLimitRemoteWireConnectionsEnabled(),
config.getFractionRemoteWireConnectionsAllowed());
}

private void validate() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,22 @@ public void incomingConnection_maxPeersExceeded()
}
}

@Test
public void connect_failsWhenFractionOfRemoteConnectionsTooHigh() {
startAgentWithMaxPeersAndEnableLimitRemoteConnections(3);
// Connect 1 peer (locally initiated)
agent.connect(createPeer());
// Connect 1 peer (remotely initiated)
connectionInitializer.simulateIncomingConnection(connection(createPeer()));
// Add remotely initiated connection, this one must be rejected because the fraction of remote
// connections is 0.5
final Peer remotelyInitiatedPeer = createPeer();
final MockPeerConnection incomingConnection = connection(remotelyInitiatedPeer);
connectionInitializer.simulateIncomingConnection(incomingConnection);
assertThat(incomingConnection.getDisconnectReason())
.contains(DisconnectReason.SUBPROTOCOL_TRIGGERED);
}

@Test
public void connect_succeedsForExemptPeerWhenMaxPeersConnected()
throws ExecutionException, InterruptedException {
Expand Down Expand Up @@ -802,12 +818,18 @@ private void startAgentWithMaxPeers(final int maxPeers) {
startAgent();
}

private void startAgentWithMaxPeersAndEnableLimitRemoteConnections(final int maxPeers) {
config.setLimitRemoteWireConnectionsEnabled(true);
startAgentWithMaxPeers(maxPeers);
}

private void startAgent(final BytesValue nodeId) {
agent.start();
localNode.setEnode(enodeBuilder().nodeId(nodeId).build());
}

private RlpxAgent agent() {
config.setLimitRemoteWireConnectionsEnabled(true);
return RlpxAgent.builder()
.keyPair(KEY_PAIR)
.config(config)
Expand Down
18 changes: 17 additions & 1 deletion pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ public class RunnerBuilder {
private int p2pListenPort;
private NatMethod natMethod = NatMethod.NONE;
private int maxPeers;
private boolean limitRemoteWireConnectionsEnabled = false;
private double fractionRemoteConnectionsAllowed;
private EthNetworkConfig ethNetworkConfig;

private JsonRpcConfiguration jsonRpcConfiguration;
Expand Down Expand Up @@ -174,6 +176,18 @@ public RunnerBuilder maxPeers(final int maxPeers) {
return this;
}

public RunnerBuilder limitRemoteWireConnectionsEnabled(
final boolean limitRemoteWireConnectionsEnabled) {
this.limitRemoteWireConnectionsEnabled = limitRemoteWireConnectionsEnabled;
return this;
}

public RunnerBuilder fractionRemoteConnectionsAllowed(
final double fractionRemoteConnectionsAllowed) {
this.fractionRemoteConnectionsAllowed = fractionRemoteConnectionsAllowed;
return this;
}

public RunnerBuilder jsonRpcConfiguration(final JsonRpcConfiguration jsonRpcConfiguration) {
this.jsonRpcConfiguration = jsonRpcConfiguration;
return this;
Expand Down Expand Up @@ -261,7 +275,9 @@ public Runner build() {
.setBindPort(p2pListenPort)
.setMaxPeers(maxPeers)
.setSupportedProtocols(subProtocols)
.setClientId(PantheonInfo.version());
.setClientId(PantheonInfo.version())
.setLimitRemoteWireConnectionsEnabled(limitRemoteWireConnectionsEnabled)
.setFractionRemoteWireConnectionsAllowed(fractionRemoteConnectionsAllowed);
networkingConfiguration.setRlpx(rlpxConfiguration).setDiscovery(discoveryConfiguration);

final PeerPermissionsBlacklist bannedNodes = PeerPermissionsBlacklist.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import tech.pegasys.pantheon.ethereum.core.Wei;
import tech.pegasys.pantheon.ethereum.eth.sync.SyncMode;
import tech.pegasys.pantheon.ethereum.p2p.config.RlpxConfiguration;
import tech.pegasys.pantheon.nat.NatMethod;
import tech.pegasys.pantheon.util.bytes.BytesValue;

Expand All @@ -36,6 +37,7 @@ public interface DefaultCommandValues {
String PANTHEON_HOME_PROPERTY_NAME = "pantheon.home";
String DEFAULT_DATA_DIR_PATH = "./build/data";
String MANDATORY_INTEGER_FORMAT_HELP = "<INTEGER>";
String MANDATORY_DOUBLE_FORMAT_HELP = "<DOUBLE>";
String MANDATORY_LONG_FORMAT_HELP = "<LONG>";
String MANDATORY_MODE_FORMAT_HELP = "<MODE>";
String MANDATORY_NETWORK_FORMAT_HELP = "<NETWORK>";
Expand Down Expand Up @@ -63,6 +65,8 @@ public interface DefaultCommandValues {
int FAST_SYNC_MAX_WAIT_TIME = 0;
int FAST_SYNC_MIN_PEER_COUNT = 5;
int DEFAULT_MAX_PEERS = 25;
double DEFAULT_FRACTION_REMOTE_WIRE_CONNECTIONS_ALLOWED =
RlpxConfiguration.DEFAULT_FRACTION_REMOTE_CONNECTIONS_ALLOWED;

static Path getDefaultPantheonDataPath(final Object command) {
// this property is retrieved from Gradle tasks or Pantheon running shell script.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
import tech.pegasys.pantheon.util.InvalidConfigurationException;
import tech.pegasys.pantheon.util.PermissioningConfigurationValidator;
import tech.pegasys.pantheon.util.bytes.BytesValue;
import tech.pegasys.pantheon.util.number.Fraction;
import tech.pegasys.pantheon.util.number.PositiveNumber;
import tech.pegasys.pantheon.util.uint.UInt256;

Expand Down Expand Up @@ -236,6 +237,21 @@ void setBootnodes(final List<String> values) {
"Maximum P2P peer connections that can be established (default: ${DEFAULT-VALUE})")
private final Integer maxPeers = DEFAULT_MAX_PEERS;

@Option(
names = {"--limit-remote-wire-connections-enabled"},
description =
"Set to limit the fraction of wire connections initiated by peers. (default: ${DEFAULT-VALUE})")
private final Boolean isLimitRemoteWireConnectionsEnabled = false;

@Option(
names = {"--fraction-remote-connections-allowed"},
paramLabel = MANDATORY_DOUBLE_FORMAT_HELP,
description =
"Maximum fraction of remotely initiated wire connections that can be established. Must be between 0.0 and 1.0. (default: ${DEFAULT-VALUE})",
arity = "1")
private final Fraction fractionRemoteConnectionsAllowed =
Fraction.fromDouble(DEFAULT_FRACTION_REMOTE_WIRE_CONNECTIONS_ALLOWED);

@Option(
names = {"--banned-node-ids", "--banned-node-id"},
paramLabel = MANDATORY_NODE_ID_FORMAT_HELP,
Expand Down Expand Up @@ -685,6 +701,7 @@ private PantheonCommand registerConverters() {
commandLine.registerConverter(UInt256.class, (arg) -> UInt256.of(new BigInteger(arg)));
commandLine.registerConverter(Wei.class, (arg) -> Wei.of(Long.parseUnsignedLong(arg)));
commandLine.registerConverter(PositiveNumber.class, PositiveNumber::fromString);
commandLine.registerConverter(Fraction.class, Fraction::fromString);
final MetricCategoryConverter metricCategoryConverter = new MetricCategoryConverter();
metricCategoryConverter.addCategories(PantheonMetricCategory.class);
metricCategoryConverter.addCategories(StandardMetricCategory.class);
Expand Down Expand Up @@ -776,8 +793,8 @@ private PantheonCommand checkOptions() {
"--discovery-enabled",
"--max-peers",
"--banned-node-id",
"--banned-node-ids"));

"--banned-node-ids",
"--fraction-remote-connections-allowed"));
// Check that mining options are able to work or send an error
checkOptionDependencies(
logger,
Expand Down Expand Up @@ -1168,6 +1185,8 @@ private void synchronize(
.p2pAdvertisedHost(p2pAdvertisedHost)
.p2pListenPort(p2pListenPort)
.maxPeers(maxPeers)
.limitRemoteWireConnectionsEnabled(isLimitRemoteWireConnectionsEnabled)
.fractionRemoteConnectionsAllowed(fractionRemoteConnectionsAllowed.getValue())
.networkingConfiguration(networkingOptions.toDomainObject())
.graphQLConfiguration(graphQLConfiguration)
.jsonRpcConfiguration(jsonRpcConfiguration)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.webSocketConfiguration(aheadWebSocketConfiguration)
.metricsConfiguration(aheadMetricsConfiguration)
.dataDir(dbAhead)
.fractionRemoteConnectionsAllowed(1.0)
.build();
try {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyDouble;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
Expand Down Expand Up @@ -108,6 +109,7 @@ public abstract class CommandTestAbstract {
@Captor protected ArgumentCaptor<File> fileArgumentCaptor;
@Captor protected ArgumentCaptor<String> stringArgumentCaptor;
@Captor protected ArgumentCaptor<Integer> intArgumentCaptor;
@Captor protected ArgumentCaptor<Double> doubleArgumentCaptor;
@Captor protected ArgumentCaptor<EthNetworkConfig> ethNetworkConfigArgumentCaptor;
@Captor protected ArgumentCaptor<SynchronizerConfiguration> syncConfigurationCaptor;
@Captor protected ArgumentCaptor<JsonRpcConfiguration> jsonRpcConfigArgumentCaptor;
Expand Down Expand Up @@ -162,6 +164,10 @@ public void initMocks() throws Exception {
when(mockRunnerBuilder.p2pAdvertisedHost(anyString())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.p2pListenPort(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.maxPeers(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.limitRemoteWireConnectionsEnabled(anyBoolean()))
.thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.fractionRemoteConnectionsAllowed(anyDouble()))
.thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.p2pEnabled(anyBoolean())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.natMethod(any())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.jsonRpcConfiguration(any())).thenReturn(mockRunnerBuilder);
Expand Down
Loading

0 comments on commit 17a09af

Please sign in to comment.