Skip to content

Commit

Permalink
[PAN-3143] Handle zero port better in NAT (hyperledger#147)
Browse files Browse the repository at this point in the history 
When setting the p2p port to zero and turning on UPNP nat an attempt is
made to map port zero.  This should actually map the opened port
instead.

The core logic is also now set up to throw an exception if a zero local
port is requested.

Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: edwardmack <[email protected]>
  • Loading branch information
@shemnon @edwardmack
shemnon authored and edwardmack committed Nov 4, 2019
1 parent 8db4a95 commit db6e519
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 137 deletions.
31 changes: 19 additions & 12 deletions ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,22 @@ public void start() {
return;
}

final int configuredDiscoveryPort = config.getDiscovery().getBindPort();
final int configuredRlpxPort = config.getRlpx().getBindPort();

final int listeningPort = rlpxAgent.start().join();
final int discoveryPort =
peerDiscoveryAgent
.start(
(configuredDiscoveryPort == 0 && configuredRlpxPort == 0)
? listeningPort
: configuredDiscoveryPort)
.join();

if (natManager.isPresent()) {
this.configureNatEnvironment();
this.configureNatEnvironment(listeningPort, discoveryPort);
}

final int listeningPort = rlpxAgent.start().join();
final int discoveryPort = peerDiscoveryAgent.start(listeningPort).join();
setLocalNode(listeningPort, discoveryPort);

peerBondedObserverId =
Expand Down Expand Up @@ -362,8 +372,9 @@ private void setLocalNode(final int listeningPort, final int discoveryPort) {
localNode.setEnode(localEnode);
}

private void configureNatEnvironment() {
CompletableFuture<String> natQueryFuture = this.natManager.get().queryExternalIPAddress();
private void configureNatEnvironment(final int listeningPort, final int discoveryPort) {
final CompletableFuture<String> natQueryFuture =
this.natManager.orElseThrow().queryExternalIPAddress();
String externalAddress = null;
try {
final int timeoutSeconds = 60;
Expand All @@ -379,19 +390,15 @@ private void configureNatEnvironment() {
LOG.info("External IP detected: " + externalAddress);
this.natManager
.get()
.requestPortForward(
this.config.getDiscovery().getBindPort(),
UpnpNatManager.Protocol.UDP,
"besu-discovery");
.requestPortForward(discoveryPort, UpnpNatManager.Protocol.UDP, "besu-discovery");
this.natManager
.get()
.requestPortForward(
this.config.getRlpx().getBindPort(), UpnpNatManager.Protocol.TCP, "besu-rlpx");
.requestPortForward(listeningPort, UpnpNatManager.Protocol.TCP, "besu-rlpx");
} else {
LOG.info("No external IP detected within timeout.");
}

} catch (Exception e) {
} catch (final Exception e) {
LOG.error("Error configuring NAT environment", e);
}
natExternalAddress = Optional.ofNullable(externalAddress);
Expand Down
4 changes: 3 additions & 1 deletion ...um/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ public void before() {
lenient().when(rlpxAgent.stop()).thenReturn(CompletableFuture.completedFuture(null));
lenient()
.when(discoveryAgent.start(anyInt()))
.thenReturn(CompletableFuture.completedFuture(30303));
.thenAnswer(
invocation ->
CompletableFuture.completedFuture(invocation.getArgument(0, Integer.class)));
lenient().when(discoveryAgent.stop()).thenReturn(CompletableFuture.completedFuture(null));
lenient()
.when(discoveryAgent.observePeerBondedEvents(discoverySubscriberCaptor.capture()))
Expand Down
114 changes: 8 additions & 106 deletions nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
*/
package org.hyperledger.besu.nat.upnp;

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

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand All @@ -38,10 +41,8 @@
import org.jupnp.registry.Registry;
import org.jupnp.registry.RegistryListener;
import org.jupnp.support.igd.callback.GetExternalIP;
import org.jupnp.support.igd.callback.GetStatusInfo;
import org.jupnp.support.igd.callback.PortMappingAdd;
import org.jupnp.support.igd.callback.PortMappingDelete;
import org.jupnp.support.model.Connection;
import org.jupnp.support.model.PortMapping;

/**
Expand Down Expand Up @@ -178,23 +179,6 @@ synchronized CompletableFuture<RemoteService> getWANIPConnectionService() {
return getService(SERVICE_TYPE_WAN_IP_CONNECTION);
}

/**
* Get the local address on which we discovered our external IP address.
*
* <p>This can be useful to distinguish which network interface the external address was
* discovered on.
*
* @return the local address on which our GetExternalIP was discovered on, or an empty value if no
* GetExternalIP query has been performed successfully.
*/
public Optional<String> getDiscoveredOnLocalAddress() {
if (!started) {
throw new IllegalStateException(
"Cannot call getDiscoveredOnLocalAddress() when in stopped state");
}
return this.discoveredOnLocalAddress;
}

/**
* Returns a CompletableFuture that will wait for the given service type to be discovered. No new
* query will be performed, and if the service has already been discovered, the future will
Expand Down Expand Up @@ -225,8 +209,6 @@ public synchronized CompletableFuture<String> queryExternalIPAddress() {
*
* <p>Note that this is not synchronized, as it is expected to be called within an
* already-synchronized context ({@link #start()}).
*
* @return A CompletableFuture that can be used to query the result (or error).
*/
private void initiateExternalIpQuery() {
discoverService(SERVICE_TYPE_WAN_IP_CONNECTION)
Expand Down Expand Up @@ -285,46 +267,6 @@ public void failure(
});
}

/**
* Sends a UPnP request to the discovered IGD to request status info.
*
* @return A CompletableFuture that can be used to query the result (or error).
*/
public CompletableFuture<Connection.StatusInfo> queryStatusInfo() {
if (!started) {
throw new IllegalStateException("Cannot call queryStatusInfo() when in stopped state");
}
final CompletableFuture<Connection.StatusInfo> upnpQueryFuture = new CompletableFuture<>();

return discoverService(SERVICE_TYPE_WAN_IP_CONNECTION)
.thenCompose(
service -> {
GetStatusInfo callback =
new GetStatusInfo(service) {
@Override
public void success(final Connection.StatusInfo statusInfo) {
upnpQueryFuture.complete(statusInfo);
}

/**
* Because the underlying jupnp library omits generics info in this method
* signature, we must too when we override it.
*/
@Override
@SuppressWarnings("rawtypes")
public void failure(
final ActionInvocation invocation,
final UpnpResponse operation,
final String msg) {
upnpQueryFuture.completeExceptionally(new Exception(msg));
}
};
upnpService.getControlPoint().execute(callback);

return upnpQueryFuture;
});
}

/**
* Convenience function to call {@link #requestPortForward(PortMapping)} with the following
* defaults:
Expand All @@ -337,12 +279,11 @@ public void failure(
* @param port is the port to be used for both internal and external port values
* @param protocol is either UDP or TCP
* @param description is a free-form description, often displayed in router UIs
* @return A CompletableFuture which will provide the results of the request
*/
public CompletableFuture<Void> requestPortForward(
public void requestPortForward(
final int port, final Protocol protocol, final String description) {

return this.requestPortForward(
this.requestPortForward(
new PortMapping(
true,
new UnsignedIntegerFourBytes(0),
Expand All @@ -354,55 +295,16 @@ public CompletableFuture<Void> requestPortForward(
description));
}

/**
* Convenience function to avoid use of PortMapping object. Takes the same arguments as are in a
* PortMapping object and constructs such an object for the caller.
*
* <p>This method chains to the {@link #requestPortForward(PortMapping)} method.
*
* @param enabled specifies whether or not the PortMapping is enabled
* @param leaseDurationSeconds is the duration of the PortMapping, in seconds
* @param remoteHost is a domain name or IP address used to filter which remote source this
* forwarding can apply to
* @param externalPort is the source port (the port visible to the Internet)
* @param internalPort is the destination port (the port to be forwarded to)
* @param internalClient is the destination host on the local LAN
* @param protocol is either UDP or TCP
* @param description is a free-form description, often displayed in router UIs
* @return A CompletableFuture which will provide the results of the request
*/
public CompletableFuture<Void> requestPortForward(
final boolean enabled,
final int leaseDurationSeconds,
final String remoteHost,
final int externalPort,
final int internalPort,
final String internalClient,
final Protocol protocol,
final String description) {

return this.requestPortForward(
new PortMapping(
enabled,
new UnsignedIntegerFourBytes(leaseDurationSeconds),
remoteHost,
new UnsignedIntegerTwoBytes(externalPort),
new UnsignedIntegerTwoBytes(internalPort),
internalClient,
toJupnpProtocol(protocol),
description));
}

/**
* Sends a UPnP request to the discovered IGD to request a port forward.
*
* @param portMapping is a portMapping object describing the desired port mapping parameters.
* @return A CompletableFuture that can be used to query the result (or error).
*/
private CompletableFuture<Void> requestPortForward(final PortMapping portMapping) {
if (!started) {
throw new IllegalStateException("Cannot call requestPortForward() when in stopped state");
}
checkArgument(
portMapping.getInternalPort().getValue() != 0, "Cannot map to internal port zero.");
checkState(started, "Cannot call requestPortForward() when in stopped state");

CompletableFuture<Void> upnpQueryFuture = new CompletableFuture<>();

Expand Down
24 changes: 6 additions & 18 deletions nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,33 +100,21 @@ public void startDoesNothingWhenAlreadyStarted() throws Exception {
}

@Test
public void getDiscoveredOnLocalAddressThrowsWhenCalledBeforeStart() throws Exception {

assertThatThrownBy(
() -> {
upnpManager.getDiscoveredOnLocalAddress();
})
.isInstanceOf(IllegalStateException.class);
}

@Test
public void queryStatusInfoThrowsWhenCalledBeforeStart() throws Exception {
public void requestPortForwardThrowsWhenCalledBeforeStart() throws Exception {

assertThatThrownBy(
() -> {
upnpManager.queryStatusInfo();
upnpManager.requestPortForward(80, UpnpNatManager.Protocol.TCP, "");
})
.isInstanceOf(IllegalStateException.class);
}

@Test
public void requestPortForwardThrowsWhenCalledBeforeStart() throws Exception {
public void requestPortForwardThrowsWhenPortIsZero() {
upnpManager.start();

assertThatThrownBy(
() -> {
upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, "");
})
.isInstanceOf(IllegalStateException.class);
assertThatThrownBy(() -> upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, ""))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
Expand Down

0 comments on commit db6e519

Please sign in to comment.