diff --git a/storage/nat.nim b/storage/nat.nim index 22f99ef9..a3bafd5b 100644 --- a/storage/nat.nim +++ b/storage/nat.nim @@ -34,6 +34,13 @@ type NatConfig* = object of true: extIp*: IpAddress of false: nat*: NatStrategy +type PortMapping* = object + tcpMappingId: cint + udpMappingId: cint + activeMappingProtocol*: MappingProtocol + activeTcpPort*: Port + activeUdpPort*: Port + type NatPortMapper* = ref object of RootObj natConfig*: NatConfig tcpPort*: Port @@ -41,11 +48,7 @@ type NatPortMapper* = ref object of RootObj discoverTimeout*: int mappingTimeout*: int recheckPeriod*: int - tcpMappingId: Option[cint] - udpMappingId: Option[cint] - activeMappingProtocol*: Option[MappingProtocol] - activeTcpPort*: Option[Port] - activeUdpPort*: Option[Port] + portMapping*: Option[PortMapping] plumInitialized: bool closed: bool @@ -71,21 +74,21 @@ method createMappingFor*( method destroyMappingFor*(m: NatPortMapper, id: cint) {.base, gcsafe.} = destroyMapping(id) -method hasLiveMapping*(m: NatPortMapper, id: cint): bool {.base, gcsafe.} = - hasMapping(id) +method hasLivePortMapping*(m: NatPortMapper): bool {.base, gcsafe.} = + ## True only when a mapping was created AND both the TCP and UDP mappings are + ## still live in the router. + if m.portMapping.isNone: + return false + + let pm = m.portMapping.get + hasMapping(pm.tcpMappingId) and hasMapping(pm.udpMappingId) proc resetMappings(m: NatPortMapper) = - if m.tcpMappingId.isSome: - m.destroyMappingFor(m.tcpMappingId.get) - m.tcpMappingId = none(cint) - - if m.udpMappingId.isSome: - m.destroyMappingFor(m.udpMappingId.get) - m.udpMappingId = none(cint) - - m.activeMappingProtocol = none(MappingProtocol) - m.activeTcpPort = none(Port) - m.activeUdpPort = none(Port) + if m.portMapping.isSome: + let pm = m.portMapping.get + m.destroyMappingFor(pm.tcpMappingId) + m.destroyMappingFor(pm.udpMappingId) + m.portMapping = none(PortMapping) method mapNatPorts*( m: NatPortMapper @@ -95,11 +98,10 @@ method mapNatPorts*( if m.closed or m.natConfig.hasExtIp: return none((Port, Port, MappingProtocol)) - # If both mappings are still active, return the stored ports without recreating. - if m.activeTcpPort.isSome and m.activeUdpPort.isSome and m.activeMappingProtocol.isSome and - m.tcpMappingId.isSome and m.hasLiveMapping(m.tcpMappingId.get) and - m.udpMappingId.isSome and m.hasLiveMapping(m.udpMappingId.get): - return some((m.activeTcpPort.get, m.activeUdpPort.get, m.activeMappingProtocol.get)) + # If both mappings are still live, return the stored ports without recreating. + if m.hasLivePortMapping(): + let pm = m.portMapping.get + return some((pm.activeTcpPort, pm.activeUdpPort, pm.activeMappingProtocol)) if not m.plumInitialized: let res = m.initPlum() @@ -123,13 +125,18 @@ method mapNatPorts*( m.destroyMappingFor(tcpRes.value.id) return none((Port, Port, MappingProtocol)) - m.tcpMappingId = some(tcpRes.value.id) - m.udpMappingId = some(udpRes.value.id) - m.activeMappingProtocol = some(tcpRes.value.mapping.mappingProtocol) - m.activeTcpPort = some(Port(tcpRes.value.mapping.externalPort)) - m.activeUdpPort = some(Port(udpRes.value.mapping.externalPort)) + m.portMapping = some( + PortMapping( + tcpMappingId: tcpRes.value.id, + udpMappingId: udpRes.value.id, + activeMappingProtocol: tcpRes.value.mapping.mappingProtocol, + activeTcpPort: Port(tcpRes.value.mapping.externalPort), + activeUdpPort: Port(udpRes.value.mapping.externalPort), + ) + ) - some((m.activeTcpPort.get, m.activeUdpPort.get, m.activeMappingProtocol.get)) + let pm = m.portMapping.get + some((pm.activeTcpPort, pm.activeUdpPort, pm.activeMappingProtocol)) proc close*(m: NatPortMapper) = m.resetMappings() @@ -143,14 +150,6 @@ proc stop*(m: NatPortMapper) = m.closed = true m.close() -proc isPortMapped*(m: NatPortMapper, port: Port): bool = - m.activeTcpPort.isSome and m.activeTcpPort.get == port - -method hasMappingIds*(m: NatPortMapper): bool {.base, gcsafe.} = - # Only checks that mappings were created, not that they are still live - # (use hasMapping() for liveness check). - m.tcpMappingId.isSome and m.udpMappingId.isSome - method handleNatStatus*( m: NatPortMapper, networkReachability: NetworkReachability, @@ -174,13 +173,14 @@ method handleNatStatus*( discovery.protocol.clientMode = false - discovery.announceDirectAddrs( - @[dialBackAddr.get], udpPort = m.activeUdpPort.get(discoveryPort) - ) + # Here we don't rely on the port mapping because we consider + # that port mapped is the same as the discovery port. + # This can be wrong for PCP but it is an accepted limitation + discovery.announceDirectAddrs(@[dialBackAddr.get], udpPort = discoveryPort) else: warn "Empty dialback address in AutoNat when node is Reachable" of NotReachable: - var hasPortMapping = false + var mappingCreated = false discovery.protocol.clientMode = true @@ -189,9 +189,10 @@ method handleNatStatus*( # If the relay is running, the addresses will be updated on reservation. discovery.announceDirectAddrs(@[], udpPort = discoveryPort) - if m.hasMappingIds(): - # The mapping was created but the node is still not reachable. - debug "Not Reachable with active port mapping, keeping it and starting relay if not started" + if m.hasLivePortMapping(): + # The mapping is still live but the node is not reachable: keep it and let + # the relay take over. A dead mapping falls through to be recreated. + debug "Not Reachable with live port mapping, keeping it and starting relay if not started" else: debug "Node is not reachable trying port mapping now" @@ -202,16 +203,15 @@ method handleNatStatus*( info "Port mapping created successfully", tcpPort, udpPort, protocol - # The address mapper uses the mapped port to build the candidate address - # for AutoNAT; the announce happens once AutoNAT confirms Reachable. + # The announce happens once AutoNAT confirms Reachable. - hasPortMapping = true + mappingCreated = true else: # In case of failure, close the port mapping in order to rerun discover # on the next iteration m.close() - if not hasPortMapping and not autoRelayService.isRunning: + if not mappingCreated and not autoRelayService.isRunning: debug "No port mapping found let's start autorelay" await autoRelayService.start(switch) @@ -224,9 +224,9 @@ proc reachabilityStr*(autonat: Option[AutonatV2Service]): string = "Unknown" proc portMappingStr*(natMapper: Option[NatPortMapper]): string = - if natMapper.isNone or natMapper.get.activeMappingProtocol.isNone: + if natMapper.isNone or natMapper.get.portMapping.isNone: return "none" - case natMapper.get.activeMappingProtocol.get + case natMapper.get.portMapping.get.activeMappingProtocol of MappingProtocol.UPnP: "upnp" of MappingProtocol.NatPmp: "pmp" of MappingProtocol.PCP: "pcp" diff --git a/tests/storage/natsimulation.nim b/tests/storage/natsimulation.nim index 2ae0b77d..19ec0a33 100644 --- a/tests/storage/natsimulation.nim +++ b/tests/storage/natsimulation.nim @@ -70,7 +70,8 @@ proc allowInbound(r: NatRouter, remote: TransportAddress, localPort: Port): bool else: discard - if r.natMapper.isSome and r.natMapper.get.isPortMapped(localPort): + if r.natMapper.isSome and r.natMapper.get.portMapping.isSome and + r.natMapper.get.portMapping.get.activeTcpPort == localPort: return true case r.filtering diff --git a/tests/storage/testnatdetection.nim b/tests/storage/testnatdetection.nim index c58b10d8..ddd19fdf 100644 --- a/tests/storage/testnatdetection.nim +++ b/tests/storage/testnatdetection.nim @@ -50,9 +50,13 @@ method mapNatPorts*( ): Future[Option[(Port, Port, MappingProtocol)]] {. async: (raises: [CancelledError]), gcsafe .} = - m.activeTcpPort = some(mockMappedTcpPort) - m.activeUdpPort = some(mockMappedUdpPort) - m.activeMappingProtocol = some(MappingProtocol.PCP) + m.portMapping = some( + PortMapping( + activeMappingProtocol: MappingProtocol.PCP, + activeTcpPort: mockMappedTcpPort, + activeUdpPort: mockMappedUdpPort, + ) + ) some((mockMappedTcpPort, mockMappedUdpPort, MappingProtocol.PCP)) # Captures the candidate addresses the service sends and answers Reachable, so diff --git a/tests/storage/testnatreaction.nim b/tests/storage/testnatreaction.nim index 76887506..5e5de7bc 100644 --- a/tests/storage/testnatreaction.nim +++ b/tests/storage/testnatreaction.nim @@ -4,7 +4,6 @@ import pkg/libp2p/[multiaddress, multihash, multicodec] import pkg/libp2p/protocols/connectivity/autonat/types import pkg/libp2p/protocols/connectivity/relay/client as relayClientModule import pkg/libp2p/services/autorelayservice except setup -import pkg/libp2p/observedaddrmanager import pkg/results import ./helpers @@ -38,8 +37,8 @@ type MockMapNatPortMapper = ref object of NatPortMapper method initPlum(m: MockMapNatPortMapper): Result[void, string] {.gcsafe.} = ok() -method hasLiveMapping(m: MockMapNatPortMapper, id: cint): bool {.gcsafe.} = - m.live +method hasLivePortMapping(m: MockMapNatPortMapper): bool {.gcsafe.} = + m.portMapping.isSome and m.live method createMappingFor( m: MockMapNatPortMapper, protocol: PlumProtocol, port: uint16 @@ -116,12 +115,19 @@ asyncchecksuite "NAT reaction - port mapping": check disc.announceAddrs == newSeq[MultiAddress]() check disc.protocol.clientMode - test "handleNatStatus starts relay when NotReachable with an active mapping": - privateAccess(NatPortMapper) + test "handleNatStatus keeps a live mapping and starts relay when NotReachable": + privateAccess(PortMapping) let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid") - let mapper = MockNatPortMapper() - mapper.tcpMappingId = some(cint(1)) - mapper.udpMappingId = some(cint(2)) + let mapper = MockMapNatPortMapper(live: true) + mapper.portMapping = some( + PortMapping( + tcpMappingId: cint(1), + udpMappingId: cint(2), + activeMappingProtocol: MappingProtocol.UPnP, + activeTcpPort: Port(9000), + activeUdpPort: Port(9001), + ) + ) autorelayservice.setup(autoRelay, sw) await mapper.handleNatStatus( @@ -131,7 +137,35 @@ asyncchecksuite "NAT reaction - port mapping": check autoRelay.isRunning check disc.announceAddrs == newSeq[MultiAddress]() check disc.protocol.clientMode - check mapper.hasMappingIds() # the active mapping is kept + check mapper.portMapping.isSome # the live mapping is kept + check mapper.destroyed.len == 0 # never torn down + + test "handleNatStatus recreates a dead mapping instead of pinning it": + privateAccess(PortMapping) + let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid") + let mapper = MockMapNatPortMapper( + live: false, + tcpResult: mappingOk(cint(10), 9000), + udpResult: mappingOk(cint(20), 9001), + ) + mapper.portMapping = some( + PortMapping( + tcpMappingId: cint(1), + udpMappingId: cint(2), + activeMappingProtocol: MappingProtocol.UPnP, + activeTcpPort: Port(9000), + activeUdpPort: Port(9001), + ) + ) + + autorelayservice.setup(autoRelay, sw) + await mapper.handleNatStatus( + NotReachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay + ) + + check mapper.destroyed == @[cint(1), cint(2)] # the dead mapping is torn down + check mapper.portMapping.isSome # replaced by a fresh one + check not autoRelay.isRunning # direct path kept, no relay test "handleNatStatus stops relay and exits client mode when mapping is created and node is Reachable": let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid") @@ -193,20 +227,6 @@ asyncchecksuite "NAT reaction - address announcing": check disc.announceAddrs == @[dialBack] - test "handleNatStatus announces the mapped external UDP port when a mapping is active": - let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid") - - let mapper = - NatPortMapper(discoveryPort: discoveryPort, activeUdpPort: some(Port(40001))) - await mapper.handleNatStatus( - Reachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay - ) - - let sprAddrs = disc.getSpr().data.addresses.mapIt(it.address) - check MultiAddress.init("/ip4/1.2.3.4/udp/40001").expect("valid") in sprAddrs - check MultiAddress.init("/ip4/1.2.3.4/udp/" & $discoveryPort).expect("valid") notin - sprAddrs - test "handleNatStatus does not announce when Reachable without a dial-back address": let mapper = NatPortMapper(discoveryPort: discoveryPort) await mapper.handleNatStatus( @@ -246,7 +266,7 @@ asyncchecksuite "NAT reaction - address announcing": check disc.announceAddrs.len == 0 proc mapperWith(protocol: MappingProtocol): Option[NatPortMapper] = - some(NatPortMapper(activeMappingProtocol: some(protocol))) + some(NatPortMapper(portMapping: some(PortMapping(activeMappingProtocol: protocol)))) asyncchecksuite "NAT - portMappingStr": test "no mapper is none": @@ -308,15 +328,17 @@ asyncchecksuite "NAT - mapNatPorts": check mapper.createAttempts.len == 0 # short-circuits before any mapping test "reuses the existing mapping when both are still live": - privateAccess(NatPortMapper) - let mapper = MockMapNatPortMapper( - live: true, - activeTcpPort: some(Port(9000)), - activeUdpPort: some(Port(9001)), - activeMappingProtocol: some(MappingProtocol.UPnP), + privateAccess(PortMapping) + let mapper = MockMapNatPortMapper(live: true) + mapper.portMapping = some( + PortMapping( + tcpMappingId: cint(1), + udpMappingId: cint(2), + activeMappingProtocol: MappingProtocol.UPnP, + activeTcpPort: Port(9000), + activeUdpPort: Port(9001), + ) ) - mapper.tcpMappingId = some(cint(1)) - mapper.udpMappingId = some(cint(2)) check (await mapper.mapNatPorts()) == some((Port(9000), Port(9001), MappingProtocol.UPnP)) diff --git a/tests/storage/testnatsimulation.nim b/tests/storage/testnatsimulation.nim index 6e046526..077f8d62 100644 --- a/tests/storage/testnatsimulation.nim +++ b/tests/storage/testnatsimulation.nim @@ -143,7 +143,7 @@ asyncchecksuite "Nat transport - Double NAT": test "bootstrap cannot connect to nat node regardless of port mapping": let actualPort = initTAddress(natNode.peerInfo.addrs[0]).get().port let natMapper = NatPortMapper() - natMapper.activeTcpPort = some(actualPort) + natMapper.portMapping = some(PortMapping(activeTcpPort: actualPort)) router.natMapper = some(natMapper) check await cannotConnect(bootstrap, natNode) @@ -166,7 +166,7 @@ asyncchecksuite "Nat transport - Port Mapping": test "bootstrap can connect to nat node when port mapping matches listen port": let actualPort = initTAddress(natNode.peerInfo.addrs[0]).get().port let natMapper = NatPortMapper() - natMapper.activeTcpPort = some(actualPort) + natMapper.portMapping = some(PortMapping(activeTcpPort: actualPort)) router.natMapper = some(natMapper) await bootstrap.connect(natNode.peerInfo.peerId, natNode.peerInfo.addrs) @@ -174,7 +174,7 @@ asyncchecksuite "Nat transport - Port Mapping": test "bootstrap cannot connect to nat node when port mapping does not match": let natMapper = NatPortMapper() - natMapper.activeTcpPort = some(Port(1)) + natMapper.portMapping = some(PortMapping(activeTcpPort: Port(1))) router.natMapper = some(natMapper) check await cannotConnect(bootstrap, natNode)