From db6e51980e3e2edfe5566dc3258965818929d951 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Thu, 31 Oct 2019 12:27:56 -0600 Subject: [PATCH] [PAN-3143] Handle zero port better in NAT (#147) 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 Signed-off-by: edwardmack --- .../p2p/network/DefaultP2PNetwork.java | 31 +++-- .../p2p/network/DefaultP2PNetworkTest.java | 4 +- .../besu/nat/upnp/UpnpNatManager.java | 114 ++---------------- .../besu/nat/upnp/UpnpNatManagerTest.java | 24 +--- 4 files changed, 36 insertions(+), 137 deletions(-) diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java index dcd31b7e4f1..4efc1267520 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java @@ -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 = @@ -362,8 +372,9 @@ private void setLocalNode(final int listeningPort, final int discoveryPort) { localNode.setEnode(localEnode); } - private void configureNatEnvironment() { - CompletableFuture natQueryFuture = this.natManager.get().queryExternalIPAddress(); + private void configureNatEnvironment(final int listeningPort, final int discoveryPort) { + final CompletableFuture natQueryFuture = + this.natManager.orElseThrow().queryExternalIPAddress(); String externalAddress = null; try { final int timeoutSeconds = 60; @@ -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); diff --git a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java index 4dc6098dc38..92aed4f54e7 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -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())) diff --git a/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java b/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java index 4c6da4305c3..e7c8db8b062 100644 --- a/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java +++ b/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java @@ -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; @@ -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; /** @@ -178,23 +179,6 @@ synchronized CompletableFuture getWANIPConnectionService() { return getService(SERVICE_TYPE_WAN_IP_CONNECTION); } - /** - * Get the local address on which we discovered our external IP address. - * - *

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 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 @@ -225,8 +209,6 @@ public synchronized CompletableFuture queryExternalIPAddress() { * *

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) @@ -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 queryStatusInfo() { - if (!started) { - throw new IllegalStateException("Cannot call queryStatusInfo() when in stopped state"); - } - final CompletableFuture 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: @@ -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 requestPortForward( + public void requestPortForward( final int port, final Protocol protocol, final String description) { - return this.requestPortForward( + this.requestPortForward( new PortMapping( true, new UnsignedIntegerFourBytes(0), @@ -354,45 +295,6 @@ public CompletableFuture 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. - * - *

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 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. * @@ -400,9 +302,9 @@ public CompletableFuture requestPortForward( * @return A CompletableFuture that can be used to query the result (or error). */ private CompletableFuture 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 upnpQueryFuture = new CompletableFuture<>(); diff --git a/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java b/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java index 7843d2381cf..f39940c28ee 100644 --- a/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java +++ b/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java @@ -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