From 0700ec770f348017d452e16298f70a37b3caf97b Mon Sep 17 00:00:00 2001 From: Kim De Mey Date: Tue, 2 Mar 2021 17:13:29 +0100 Subject: [PATCH] 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 | 120 +++++++++++++++++++++++++++++-- eth/net/utils.nim | 16 ++++- eth/p2p/discoveryv5/dcli.nim | 52 +++----------- eth/p2p/discoveryv5/protocol.nim | 23 +++--- tests/p2p/discv5_test_helper.nim | 3 +- tests/p2p/test_discoveryv5.nim | 16 +++-- 6 files changed, 164 insertions(+), 66 deletions(-) diff --git a/eth/net/nat.nim b/eth/net/nat.nim index c77dd15..f8645b8 100644 --- a/eth/net/nat.nim +++ b/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, - eth/common/utils + chronicles, json_serialization/std/net, chronos, + 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 diff --git a/eth/net/utils.nim b/eth/net/utils.nim index 5b6748a..390f9ed 100644 --- a/eth/net/utils.nim +++ b/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 diff --git a/eth/p2p/discoveryv5/dcli.nim b/eth/p2p/discoveryv5/dcli.nim index 58ac99f..aee106b 100644 --- a/eth/p2p/discoveryv5/dcli.nim +++ b/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,10 +137,17 @@ proc discover(d: protocol.Protocol) {.async.} = proc run(config: DiscoveryConf) = let - (ip, tcpPort, udpPort) = setupNat(config) - d = newProtocol(config.nodeKey, ip, tcpPort, udpPort, - bootstrapRecords = config.bootnodes, bindIp = config.listenAddress, - enrAutoUpdate = config.enrAutoUpdate) + bindIp = config.listenAddress + udpPort = Port(config.udpPort) + # 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() diff --git a/eth/p2p/discoveryv5/protocol.nim b/eth/p2p/discoveryv5/protocol.nim index e69c488..1071a6b 100644 --- a/eth/p2p/discoveryv5/protocol.nim +++ b/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], - tcpPort, udpPort: Port, + enrIp: Option[ValidIpAddress], + 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), - some(udpPort), extraFields).expect("Record within size limits") + record = enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort, + 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 diff --git a/tests/p2p/discv5_test_helper.nim b/tests/p2p/discv5_test_helper.nim index 8cdcf93..716b71a 100644 --- a/tests/p2p/discv5_test_helper.nim +++ b/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, diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index 98bbcdd..1e9bfc7 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/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) - noUpdatesNode = newProtocol(privKey, ip, port, port, rng = rng, - previousRecord = some(node.getRecord())) - updatesNode = newProtocol(privKey, ip, port, Port(20302), rng = rng, + node = newProtocol(privKey, ip, some(port), some(port), bindPort = port, + rng = rng) + noUpdatesNode = newProtocol(privKey, ip, some(port), some(port), + 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, - localEnrFields = {"addfield": @[byte 0]}, + moreUpdatesNode = newProtocol(privKey, ip, some(port), some(port), + 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":