[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 <danno.ferrin@gmail.com>

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

@@ -189,12 +189,22 @@ public class DefaultP2PNetwork implements P2PNetwork {
       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 @@ public class DefaultP2PNetwork implements P2PNetwork {
     localNode.setEnode(localEnode);
   }
 
-  private void configureNatEnvironment() {
+  private void configureNatEnvironment(final int listeningPort, final int discoveryPort) {
-    CompletableFuture<String> natQueryFuture = this.natManager.get().queryExternalIPAddress();
+    final CompletableFuture<String> natQueryFuture =
+        this.natManager.orElseThrow().queryExternalIPAddress();
     String externalAddress = null;
     try {
       final int timeoutSeconds = 60;
@@ -379,19 +390,15 @@ public class DefaultP2PNetwork implements P2PNetwork {
         LOG.info("External IP detected: " + externalAddress);
         this.natManager
             .get()
-            .requestPortForward(
+            .requestPortForward(discoveryPort, UpnpNatManager.Protocol.UDP, "besu-discovery");
-                this.config.getDiscovery().getBindPort(),
-                UpnpNatManager.Protocol.UDP,
-                "besu-discovery");
         this.natManager
             .get()
-            .requestPortForward(
+            .requestPortForward(listeningPort, UpnpNatManager.Protocol.TCP, "besu-rlpx");
-                this.config.getRlpx().getBindPort(), 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);

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java

@@ -90,7 +90,9 @@ public final class DefaultP2PNetworkTest {
     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()))

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.model.types.UnsignedIntegerTwoBytes;
 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 @@ public class UpnpNatManager {
     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
@@ -225,8 +209,6 @@ public class UpnpNatManager {
    *
    * <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)
@@ -285,46 +267,6 @@ public class UpnpNatManager {
             });
   }
 
-  /**
-   * 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:
@@ -337,12 +279,11 @@ public class UpnpNatManager {
    * @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),
@@ -354,45 +295,6 @@ public class UpnpNatManager {
             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.
    *
@@ -400,9 +302,9 @@ public class UpnpNatManager {
    * @return A CompletableFuture that can be used to query the result (or error).
    */
   private CompletableFuture<Void> requestPortForward(final PortMapping portMapping) {
-    if (!started) {
+    checkArgument(
-      throw new IllegalStateException("Cannot call requestPortForward() when in stopped state");
+        portMapping.getInternalPort().getValue() != 0, "Cannot map to internal port zero.");
-    }
+    checkState(started, "Cannot call requestPortForward() when in stopped state");
 
     CompletableFuture<Void> upnpQueryFuture = new CompletableFuture<>();
 

nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java

@@ -100,33 +100,21 @@ public final class UpnpNatManagerTest {
   }
 
   @Test
-  public void getDiscoveredOnLocalAddressThrowsWhenCalledBeforeStart() throws Exception {
+  public void requestPortForwardThrowsWhenCalledBeforeStart() throws Exception {
-
-    assertThatThrownBy(
-            () -> {
-              upnpManager.getDiscoveredOnLocalAddress();
-            })
-        .isInstanceOf(IllegalStateException.class);
-  }
-
-  @Test
-  public void queryStatusInfoThrowsWhenCalledBeforeStart() 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(
+    assertThatThrownBy(() -> upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, ""))
-            () -> {
+        .isInstanceOf(IllegalArgumentException.class);
-              upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, "");
-            })
-        .isInstanceOf(IllegalStateException.class);
   }
 
   @Test