Improve network address discovery / NAT setup (#323)

* Add search for best route and refactor setupNat to setupAddress

* Update setupAddress and make enr ports in discovery optional

* Add specific error log when no route is found
* Use bindIP if it is public
* Adjust some log levels

eth/net/nat.nim

@@ -7,10 +7,10 @@
 # those terms.
 
 import
-  options, os, strutils, times,
+  std/[options, os, strutils, times],
   stew/results, nat_traversal/[miniupnpc, natpmp],
-  chronicles, json_serialization/std/net,
+  chronicles, json_serialization/std/net, chronos,
-  eth/common/utils
+  eth/common/utils, ./utils as netutils
 
 type
   NatStrategy* = enum
@@ -114,7 +114,7 @@ proc doPortMapping(tcpPort, udpPort: Port, description: string): Option[(Port, P
                                     desc = description,
                                     leaseDuration = 0)
       if pmres.isErr:
-        error "UPnP port mapping", msg = pmres.error
+        error "UPnP port mapping", msg = pmres.error, port
         return
       else:
         # let's check it
@@ -123,7 +123,7 @@ proc doPortMapping(tcpPort, udpPort: Port, description: string): Option[(Port, P
         if cres.isErr:
           warn "UPnP port mapping check failed. Assuming the check itself is broken and the port mapping was done.", msg = cres.error
 
-        debug "UPnP: added port mapping", externalPort = port, internalPort = port, protocol = protocol
+        info "UPnP: added port mapping", externalPort = port, internalPort = port, protocol = protocol
         case protocol:
           of UPNPProtocol.TCP:
             extTcpPort = port
@@ -138,11 +138,11 @@ proc doPortMapping(tcpPort, udpPort: Port, description: string): Option[(Port, P
                                     protocol = protocol,
                                     lifetime = NATPMP_LIFETIME)
       if pmres.isErr:
-        error "NAT-PMP port mapping", msg = pmres.error
+        error "NAT-PMP port mapping", msg = pmres.error, port
         return
       else:
         let extPort = Port(pmres.value)
-        debug "NAT-PMP: added port mapping", externalPort = extPort, internalPort = port, protocol = protocol
+        info "NAT-PMP: added port mapping", externalPort = extPort, internalPort = port, protocol = protocol
         case protocol:
           of NatPmpProtocol.TCP:
             extTcpPort = extPort
@@ -237,3 +237,109 @@ proc redirectPorts*(tcpPort, udpPort: Port, description: string): Option[(Port,
     # atexit() in disguise
     addQuitProc(stopNatThread)
 
+proc setupNat*(natStrategy: NatStrategy, tcpPort, udpPort: Port,
+    clientId: string):
+    tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]] =
+  # TODO: check and forward actual errors?
+
+  let extIp = getExternalIP(natStrategy)
+  if extIP.isSome:
+    let ip = ValidIpAddress.init(extIp.get)
+    let extPorts = ({.gcsafe.}:
+      redirectPorts(tcpPort = tcpPort,
+                    udpPort = udpPort,
+                    description = clientId))
+    if extPorts.isSome:
+      let (extTcpPort, extUdpPort) = extPorts.get()
+      (some(ip), some(extTcpPort), some(extUdpPort))
+    else:
+      error "UPnP/NAT-PMP available but port forwarding failed"
+      (none(ValidIpAddress), none(Port), none(Port))
+  else:
+    warn "UPnP/NAT-PMP not available"
+    (none(ValidIpAddress), none(Port), none(Port))
+
+proc getRouteIpv4*(): Result[ValidIpAddress, cstring] {.raises: [Defect].} =
+  # Avoiding Exception with initTAddress and can't make it work with static.
+  # Note: `publicAddress` is only used an "example" IP to find the best route,
+  # no data is send over the network to this IP!
+  let
+    publicAddress = TransportAddress(family: AddressFamily.IPv4,
+      address_v4: [1'u8, 1, 1, 1], port: Port(0))
+    route = getBestRoute(publicAddress)
+
+  if route.source.isUnspecified():
+    err("No best ipv4 route found")
+  else:
+    let ip = try: route.source.address()
+             except ValueError as e:
+               # This should not occur really.
+               error "Address convertion error", exception = e.name, msg = e.msg
+               return err("Invalid IP address")
+    ok(ValidIpAddress.init(ip))
+
+proc setupAddress*(nat: string, bindIp: ValidIpAddress, tcpPort, udpPort: Port,
+    clientId: string):
+    tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]]
+    {.gcsafe.} =
+  case nat.toLowerAscii:
+    of "any":
+      let bindAddress = initTAddress(bindIP, Port(0))
+      if bindAddress.isAnyLocal():
+        let ip = getRouteIpv4()
+        if ip.isErr():
+          # No route was found, log error and continue without IP.
+          # Could also `quit QuitFailure` here.
+          error "No routable IP address found, check your network connection",
+            error = ip.error
+          return (none(ValidIpAddress), none(Port), none(Port))
+        elif ip.get().isPublic():
+          return (some(ip.get()), some(tcpPort), some(udpPort))
+        else:
+          # Best route IP is not public, might be an internal network and the
+          # node is either behind a gateway with NAT or for example a container
+          # or VM bridge (or both). Lets try UPnP and NAT-PMP for the case where
+          # the node is behind a gateway with UPnP or NAT-PMP support.
+          return setupNat(NatAny, tcpPort, udpPort, clientId)
+      elif bindAddress.isPublic():
+        # When a specific public interface is provided, use that one.
+        return (some(ValidIpAddress.init(bindIP)), some(tcpPort), some(udpPort))
+      else:
+        return setupNat(NatAny, tcpPort, udpPort, clientId)
+    of "none":
+      let bindAddress = initTAddress(bindIP, Port(0))
+      if bindAddress.isAnyLocal():
+        let ip = getRouteIpv4()
+        if ip.isErr():
+          # No route was found, log error and continue without IP.
+          # Could also `quit QuitFailure` here.
+          error "No routable IP address found, check your network connection",
+            error = ip.error
+          return (none(ValidIpAddress), none(Port), none(Port))
+        elif ip.get().isPublic():
+          return (some(ip.get()), some(tcpPort), some(udpPort))
+        else:
+          error "No public IP address found. Should not use --nat:none option"
+          return (none(ValidIpAddress), none(Port), none(Port))
+      elif bindAddress.isPublic():
+        # When a specific public interface is provided, use that one.
+        return (some(ValidIpAddress.init(bindIP)), some(tcpPort), some(udpPort))
+      else:
+        error "Bind IP is not a public IP address. Should not use --nat:none option"
+        return (none(ValidIpAddress), none(Port), none(Port))
+    of "upnp":
+      return setupNat(NatUpnp, tcpPort, udpPort, clientId)
+    of "pmp":
+      return setupNat(NatPmp, tcpPort, udpPort, clientId)
+    else:
+      if nat.startsWith("extip:"):
+        try:
+          # any required port redirection must be done by hand
+          let ip = ValidIpAddress.init(nat[6..^1])
+          return (some(ip), some(tcpPort), some(udpPort))
+        except ValueError:
+          error "Not a valid IP address", address = nat[6..^1]
+          quit QuitFailure
+      else:
+        error "Not a valid NAT option", value = nat
+        quit QuitFailure

eth/net/utils.nim

@@ -1,6 +1,6 @@
 import
   std/[tables, hashes],
-  stew/shims/net as stewNet
+  stew/shims/net as stewNet, chronos
 
 {.push raises: [Defect].}
 
@@ -25,3 +25,17 @@ proc dec*(ipLimits: var IpLimits, ip: ValidIpAddress) =
     ipLimits.ips.del(ip)
   elif val > 1:
     ipLimits.ips[ip] = val - 1
+
+proc isPublic*(address: TransportAddress): bool {.raises: [Defect].} =
+  # TODO: Some are still missing, for special reserved addresses see:
+  # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
+  # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml
+  if address.isLoopback() or address.isSiteLocal() or
+      address.isMulticast() or address.isLinkLocal():
+    false
+  else:
+    true
+
+proc isPublic*(address: IpAddress): bool {.raises: [Defect].} =
+  let a = initTAddress(address, Port(0))
+  a.isPublic

eth/p2p/discoveryv5/dcli.nim

@@ -129,43 +129,6 @@ proc parseCmdArg*(T: type PrivateKey, p: TaintedString): T =
 proc completeCmdArg*(T: type PrivateKey, val: TaintedString): seq[string] =
   return @[]
 
-proc setupNat(conf: DiscoveryConf): tuple[ip: Option[ValidIpAddress],
-                                          tcpPort: Port,
-                                          udpPort: Port] {.gcsafe.} =
-  # defaults
-  result.tcpPort = Port(conf.udpPort)
-  result.udpPort = Port(conf.udpPort)
-
-  var nat: NatStrategy
-  case conf.nat.toLowerAscii:
-    of "any":
-      nat = NatAny
-    of "none":
-      nat = NatNone
-    of "upnp":
-      nat = NatUpnp
-    of "pmp":
-      nat = NatPmp
-    else:
-      if conf.nat.startsWith("extip:") and isIpAddress(conf.nat[6..^1]):
-        # any required port redirection is assumed to be done by hand
-        result.ip = some(ValidIpAddress.init(conf.nat[6..^1]))
-        nat = NatNone
-      else:
-        error "not a valid NAT mechanism, nor a valid IP address", value = conf.nat
-        quit(QuitFailure)
-
-  if nat != NatNone:
-    let extIp = getExternalIP(nat)
-    if extIP.isSome:
-      result.ip = some(ValidIpAddress.init extIp.get)
-      let extPorts = ({.gcsafe.}:
-        redirectPorts(tcpPort = result.tcpPort,
-                      udpPort = result.udpPort,
-                      description = "Discovery v5 ports"))
-      if extPorts.isSome:
-        (result.tcpPort, result.udpPort) = extPorts.get()
-
 proc discover(d: protocol.Protocol) {.async.} =
   while true:
     let discovered = await d.queryRandom()
@@ -174,9 +137,16 @@ proc discover(d: protocol.Protocol) {.async.} =
 
 proc run(config: DiscoveryConf) =
   let
-    (ip, tcpPort, udpPort) = setupNat(config)
+    bindIp = config.listenAddress
-    d = newProtocol(config.nodeKey, ip, tcpPort, udpPort,
+    udpPort = Port(config.udpPort)
-      bootstrapRecords = config.bootnodes, bindIp = config.listenAddress,
+    # TODO: allow for no TCP port mapping!
+    (extIp, _, extUdpPort) = setupAddress(config.nat,
+      config.listenAddress, udpPort, udpPort, "dcli")
+
+  let d = newProtocol(config.nodeKey,
+          extIp, none(Port), extUdpPort,
+          bootstrapRecords = config.bootnodes,
+          bindIp = bindIp, bindPort = udpPort,
           enrAutoUpdate = config.enrAutoUpdate)
 
   d.open()

eth/p2p/discoveryv5/protocol.nim

@@ -948,11 +948,12 @@ proc ipMajorityLoop(d: Protocol) {.async, raises: [Exception, Defect].} =
     trace "ipMajorityLoop canceled"
 
 proc newProtocol*(privKey: PrivateKey,
-                  externalIp: Option[ValidIpAddress],
+                  enrIp: Option[ValidIpAddress],
-                  tcpPort, udpPort: Port,
+                  enrTcpPort, enrUdpPort: Option[Port],
                   localEnrFields: openarray[(string, seq[byte])] = [],
                   bootstrapRecords: openarray[Record] = [],
                   previousRecord = none[enr.Record](),
+                  bindPort: Port,
                   bindIp = IPv4_any(),
                   enrAutoUpdate = false,
                   tableIpLimits = DefaultTableIpLimits,
@@ -970,11 +971,17 @@ proc newProtocol*(privKey: PrivateKey,
   var record: Record
   if previousRecord.isSome():
     record = previousRecord.get()
-    record.update(privKey, externalIp, some(tcpPort), some(udpPort),
+    record.update(privKey, enrIp, enrTcpPort, enrUdpPort,
       extraFields).expect("Record within size limits and correct key")
   else:
-    record = enr.Record.init(1, privKey, externalIp, some(tcpPort),
+    record = enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort,
-      some(udpPort), extraFields).expect("Record within size limits")
+      extraFields).expect("Record within size limits")
+
+  info "ENR initialized", ip = enrIp, tcp = enrTcpPort, udp = enrUdpPort,
+    seqNum = record.seqNum, uri = toURI(record)
+  if enrIp.isNone():
+    warn "No external IP provided for the ENR, this node will not be discoverable"
+
   let node = newNode(record).expect("Properly initialized record")
 
   # TODO Consider whether this should be a Defect
@@ -983,7 +990,7 @@ proc newProtocol*(privKey: PrivateKey,
   result = Protocol(
     privateKey: privKey,
     localNode: node,
-    bindAddress: Address(ip: ValidIpAddress.init(bindIp), port: udpPort),
+    bindAddress: Address(ip: ValidIpAddress.init(bindIp), port: bindPort),
     codec: Codec(localNode: node, privKey: privKey,
       sessions: Sessions.init(256)),
     bootstrapRecords: @bootstrapRecords,
@@ -995,10 +1002,8 @@ proc newProtocol*(privKey: PrivateKey,
 
 proc open*(d: Protocol) {.raises: [Exception, Defect].} =
   info "Starting discovery node", node = d.localNode,
-    bindAddress = d.bindAddress, uri = toURI(d.localNode.record)
+    bindAddress = d.bindAddress
 
-  if d.localNode.address.isNone():
-    info "No external IP provided, this node will not be discoverable"
   # TODO allow binding to specific IP / IPv6 / etc
   let ta = initTAddress(d.bindAddress.ip, d.bindAddress.port)
   # TODO: raises `OSError` and `IOSelectorsException`, the latter which is

tests/p2p/discv5_test_helper.nim

@@ -20,7 +20,8 @@ proc initDiscoveryNode*(rng: ref BrHmacDrbgContext, privKey: PrivateKey,
 
   result = newProtocol(privKey,
                        some(address.ip),
-                       address.port, address.port,
+                       some(address.port), some(address.port),
+                       bindPort = address.port,
                        bootstrapRecords = bootstrapRecords,
                        localEnrFields = localEnrFields,
                        previousRecord = previousRecord,

tests/p2p/test_discoveryv5.nim

@@ -371,13 +371,15 @@ procSuite "Discovery v5 Tests":
       privKey = PrivateKey.random(rng[])
       ip = some(ValidIpAddress.init("127.0.0.1"))
       port = Port(20301)
-      node = newProtocol(privKey, ip, port, port, rng = rng)
+      node = newProtocol(privKey, ip, some(port), some(port), bindPort = port,
-      noUpdatesNode = newProtocol(privKey, ip, port, port, rng = rng,
+        rng = rng)
-        previousRecord = some(node.getRecord()))
+      noUpdatesNode = newProtocol(privKey, ip, some(port), some(port),
-      updatesNode = newProtocol(privKey, ip, port, Port(20302), rng = rng,
+        bindPort = port, rng = rng, previousRecord = some(node.getRecord()))
+      updatesNode = newProtocol(privKey, ip, some(port), some(Port(20302)),
+        bindPort = port, rng = rng,
         previousRecord = some(noUpdatesNode.getRecord()))
-      moreUpdatesNode = newProtocol(privKey, ip, port, port, rng = rng,
+      moreUpdatesNode = newProtocol(privKey, ip, some(port), some(port),
-        localEnrFields = {"addfield": @[byte 0]},
+        bindPort = port, rng = rng, localEnrFields = {"addfield": @[byte 0]},
         previousRecord = some(updatesNode.getRecord()))
     check:
       node.getRecord().seqNum == 1
@@ -388,7 +390,7 @@ procSuite "Discovery v5 Tests":
     # Defect (for now?) on incorrect key use
     expect ResultDefect:
       let incorrectKeyUpdates = newProtocol(PrivateKey.random(rng[]),
-        ip, port, port, rng = rng,
+        ip, some(port), some(port), bindPort = port, rng = rng,
         previousRecord = some(updatesNode.getRecord()))
 
   asyncTest "Update node record with revalidate":