fix: support IPv6 with IPv4 (libp2p#2864)

Upgrades to latest nat-port-mapper for better IPv6 support.
acul71 · Dec 1, 2024 · 146c696 · 146c696
1 parent 143669c
commit 146c696
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 61 deletions.
diff --git a/packages/upnp-nat/package.json b/packages/upnp-nat/package.json
@@ -50,7 +50,7 @@
     "test:electron-main": "aegir test -t electron-main"
   },
   "dependencies": {
-    "@achingbrain/nat-port-mapper": "^3.0.2",
+    "@achingbrain/nat-port-mapper": "^4.0.0",
     "@chainsafe/is-ip": "^2.0.2",
     "@libp2p/interface": "^2.2.1",
     "@libp2p/interface-internal": "^2.1.1",

diff --git a/packages/upnp-nat/src/constants.ts b/packages/upnp-nat/src/constants.ts
@@ -1,3 +1,2 @@
-export const DEFAULT_PORT_MAPPING_TTL = 720_000
 export const DEFAULT_GATEWAY_SEARCH_TIMEOUT = 60_000
 export const DEFAULT_GATEWAY_SEARCH_INTERVAL = 300_000
diff --git a/packages/upnp-nat/src/gateway-finder.ts b/packages/upnp-nat/src/gateway-finder.ts
@@ -34,8 +34,13 @@ export class GatewayFinder extends TypedEventEmitter<GatewayFinderEvents> {
 
     // every five minutes, search for network gateways for one minute
     this.findGateways = repeatingTask(async (options) => {
-      for await (const gateway of this.portMappingClient.findGateways(options)) {
-        if (this.gateways.some(g => g.id === gateway.id)) {
+      for await (const gateway of this.portMappingClient.findGateways({
+        ...options,
+        searchInterval: 10000
+      })) {
+        if (this.gateways.some(g => {
+          return g.id === gateway.id && g.family === gateway.family
+        })) {
           // already seen this gateway
           continue
         }

diff --git a/packages/upnp-nat/src/index.ts b/packages/upnp-nat/src/index.ts
@@ -86,6 +86,13 @@ export interface UPnPNATInit {
    */
   portMappingAutoRefresh?: boolean
 
+  /**
+   * How long before a port mapping expires to refresh it in ms
+   *
+   * @default 60_000
+   */
+  portMappingRefreshThreshold?: number
+
   /**
    * A preconfigured instance of a NatAPI client can be passed as an option,
    * otherwise one will be created

diff --git a/packages/upnp-nat/src/upnp-nat.ts b/packages/upnp-nat/src/upnp-nat.ts
@@ -1,7 +1,6 @@
 import { upnpNat } from '@achingbrain/nat-port-mapper'
 import { serviceCapabilities, setMaxListeners, start, stop } from '@libp2p/interface'
 import { debounce } from '@libp2p/utils/debounce'
-import { DEFAULT_PORT_MAPPING_TTL } from './constants.js'
 import { GatewayFinder } from './gateway-finder.js'
 import { UPnPPortMapper } from './upnp-port-mapper.js'
 import type { UPnPNATComponents, UPnPNATInit, UPnPNAT as UPnPNATInterface } from './index.js'
@@ -29,8 +28,9 @@ export class UPnPNAT implements Startable, UPnPNATInterface {
 
     this.portMappingClient = init.portMappingClient ?? upnpNat({
       description: init.portMappingDescription ?? `${components.nodeInfo.name}@${components.nodeInfo.version} ${components.peerId.toString()}`,
-      ttl: init.portMappingTTL ?? DEFAULT_PORT_MAPPING_TTL,
-      autoRefresh: init.portMappingAutoRefresh ?? true
+      ttl: init.portMappingTTL,
+      autoRefresh: init.portMappingAutoRefresh,
+      refreshThreshold: init.portMappingRefreshThreshold
     })
 
     // trigger update when our addresses change

diff --git a/packages/upnp-nat/src/upnp-port-mapper.ts b/packages/upnp-nat/src/upnp-port-mapper.ts
@@ -1,11 +1,12 @@
-import { isIPv4, isIPv6 } from '@chainsafe/is-ip'
+import { isIPv4 } from '@chainsafe/is-ip'
 import { InvalidParametersError, start, stop } from '@libp2p/interface'
+import { isLinkLocal } from '@libp2p/utils/multiaddr/is-link-local'
 import { isLoopback } from '@libp2p/utils/multiaddr/is-loopback'
 import { isPrivate } from '@libp2p/utils/multiaddr/is-private'
 import { isPrivateIp } from '@libp2p/utils/private-ip'
 import { QUICV1, TCP, WebSockets, WebSocketsSecure, WebTransport } from '@multiformats/multiaddr-matcher'
 import { dynamicExternalAddress } from './check-external-address.js'
-import { DoubleNATError, InvalidIPAddressError } from './errors.js'
+import { DoubleNATError } from './errors.js'
 import type { ExternalAddress } from './check-external-address.js'
 import type { Gateway } from '@achingbrain/nat-port-mapper'
 import type { ComponentLogger, Logger } from '@libp2p/interface'
@@ -98,12 +99,20 @@ export class UPnPPortMapper {
   /**
    * Return any eligible multiaddrs that are not mapped on the detected gateway
    */
-  private getUnmappedAddresses (multiaddrs: Multiaddr[], ipType: 4 | 6): Multiaddr[] {
+  private getUnmappedAddresses (multiaddrs: Multiaddr[], publicAddresses: string[]): Multiaddr[] {
     const output: Multiaddr[] = []
 
     for (const ma of multiaddrs) {
-      // ignore public addresses
-      if (!isPrivate(ma)) {
+      const stringTuples = ma.stringTuples()
+      const address = `${stringTuples[0][1]}`
+
+      // ignore public IPv4 addresses
+      if (isIPv4(address) && !isPrivate(ma)) {
+        continue
+      }
+
+      // ignore any addresses that match the interface on the network gateway
+      if (publicAddresses.includes(address)) {
         continue
       }
 
@@ -112,6 +121,11 @@ export class UPnPPortMapper {
         continue
       }
 
+      // ignore link-local addresses
+      if (isLinkLocal(ma)) {
+        continue
+      }
+
       // only IP based addresses
       if (!(
         TCP.exactMatch(ma) ||
@@ -123,13 +137,9 @@ export class UPnPPortMapper {
         continue
       }
 
-      const { port, host, family, transport } = ma.toOptions()
+      const { port, transport } = ma.toOptions()
 
-      if (family !== ipType) {
-        continue
-      }
-
-      if (this.mappedPorts.has(`${host}-${port}-${transport}`)) {
+      if (this.mappedPorts.has(`${port}-${transport}`)) {
         continue
       }
 
@@ -143,62 +153,49 @@ export class UPnPPortMapper {
     try {
       const externalHost = await this.externalAddress.getPublicIp()
 
-      let ipType: 4 | 6 = 4
-
-      if (isIPv4(externalHost)) {
-        ipType = 4
-      } else if (isIPv6(externalHost)) {
-        ipType = 6
-      } else {
-        throw new InvalidIPAddressError(`Public address ${externalHost} was not an IPv4 address`)
-      }
-
       // filter addresses to get private, non-relay, IP based addresses that we
       // haven't mapped yet
-      const addresses = this.getUnmappedAddresses(this.addressManager.getAddresses(), ipType)
+      const addresses = this.getUnmappedAddresses(this.addressManager.getAddresses(), [externalHost])
 
       if (addresses.length === 0) {
         this.log('no private, non-relay, unmapped, IP based addresses found')
         return
       }
 
-      this.log('%s public IP %s', this.externalAddress != null ? 'using configured' : 'discovered', externalHost)
+      this.log('discovered public IP %s', externalHost)
 
       this.assertNotBehindDoubleNAT(externalHost)
 
       for (const addr of addresses) {
         // try to open uPnP ports for each thin waist address
-        const { family, host, port, transport } = addr.toOptions()
+        const { port, host, transport, family } = addr.toOptions()
 
-        if (family === 6) {
-          // only support IPv4 addresses
+        // don't try to open port on IPv6 host via IPv4 gateway
+        if (family === 4 && this.gateway.family !== 'IPv4') {
           continue
         }
 
-        if (this.mappedPorts.has(`${host}-${port}-${transport}`)) {
-          // already mapped this port
+        // don't try to open port on IPv4 host via IPv6 gateway
+        if (family === 6 && this.gateway.family !== 'IPv6') {
           continue
         }
 
-        try {
-          const key = `${host}-${port}-${transport}`
-          this.log('creating mapping of key %s', key)
+        const key = `${host}-${port}-${transport}`
 
-          const externalPort = await this.gateway.map(port, {
-            localAddress: host,
-            protocol: transport === 'tcp' ? 'tcp' : 'udp'
-          })
+        if (this.mappedPorts.has(key)) {
+          // already mapped this port
+          continue
+        }
 
-          this.mappedPorts.set(key, {
-            externalHost,
-            externalPort
+        try {
+          const mapping = await this.gateway.map(port, host, {
+            protocol: transport === 'tcp' ? 'TCP' : 'UDP'
           })
-
-          this.log('created mapping of %s:%s to %s:%s', externalHost, externalPort, host, port)
-
-          this.addressManager.addPublicAddressMapping(host, port, externalHost, externalPort, transport === 'tcp' ? 'tcp' : 'udp')
+          this.mappedPorts.set(key, mapping)
+          this.addressManager.addPublicAddressMapping(mapping.internalHost, mapping.internalPort, mapping.externalHost, mapping.externalPort, transport === 'tcp' ? 'tcp' : 'udp')
+          this.log('created mapping of %s:%s to %s:%s for protocol %s', mapping.internalHost, mapping.internalPort, mapping.externalHost, mapping.externalPort, transport)
         } catch (err) {
-          this.log.error('failed to create mapping of %s:%s - %e', host, port, err)
+          this.log.error('failed to create mapping for %s:%d for protocol - %e', host, port, transport, err)
         }
       }
     } catch (err: any) {

diff --git a/packages/upnp-nat/test/index.spec.ts b/packages/upnp-nat/test/index.spec.ts
@@ -35,7 +35,9 @@ describe('UPnP NAT (TCP)', () => {
       events: new TypedEventEmitter()
     }
 
-    gateway = stubInterface<Gateway>()
+    gateway = stubInterface<Gateway>({
+      family: 'IPv4'
+    })
     client = stubInterface<UPnPNATClient>({
       findGateways: async function * (options) {
         yield gateway
@@ -71,20 +73,35 @@ describe('UPnP NAT (TCP)', () => {
       components
     } = await createNatManager()
 
-    gateway.externalIp.resolves('82.3.1.5')
+    const internalHost = '192.168.1.12'
+    const internalPort = 4002
+
+    const externalHost = '82.3.1.5'
+    const externalPort = 4003
+
+    gateway.externalIp.resolves(externalHost)
 
     components.addressManager.getAddresses.returns([
       multiaddr('/ip4/127.0.0.1/tcp/4002'),
-      multiaddr('/ip4/192.168.1.12/tcp/4002')
+      multiaddr(`/ip4/${internalHost}/tcp/${internalPort}`)
     ])
 
+    gateway.map.withArgs(internalPort, internalHost).resolves({
+      internalHost,
+      internalPort,
+      externalHost,
+      externalPort,
+      protocol: 'TCP'
+    })
+
     await start(natManager)
     await natManager.mapIpAddresses()
 
     expect(gateway.map.called).to.be.true()
-    expect(gateway.map.getCall(0).args[0]).to.equal(4002)
-    expect(gateway.map.getCall(0).args[1]).to.include({
-      protocol: 'tcp'
+    expect(gateway.map.getCall(0).args[0]).to.equal(internalPort)
+    expect(gateway.map.getCall(0).args[1]).to.equal(internalHost)
+    expect(gateway.map.getCall(0).args[2]).to.include({
+      protocol: 'TCP'
     })
     expect(components.addressManager.addPublicAddressMapping.called).to.be.true()
   })
@@ -95,20 +112,35 @@ describe('UPnP NAT (TCP)', () => {
       components
     } = await createNatManager()
 
-    gateway.externalIp.resolves('82.3.1.5')
+    const internalHost = '192.168.1.12'
+    const internalPort = 4002
+
+    const externalHost = '82.3.1.5'
+    const externalPort = 4003
+
+    gateway.externalIp.resolves(externalHost)
 
     components.addressManager.getAddresses.returns([
       multiaddr('/ip4/127.0.0.1/tcp/4002'),
-      multiaddr('/ip4/192.168.1.12/tcp/4002')
+      multiaddr(`/ip4/${internalHost}/tcp/${internalPort}`)
     ])
 
+    gateway.map.withArgs(internalPort, internalHost).resolves({
+      internalHost,
+      internalPort,
+      externalHost,
+      externalPort,
+      protocol: 'TCP'
+    })
+
     await start(natManager)
     await natManager.mapIpAddresses()
 
     expect(gateway.map.called).to.be.true()
-    expect(gateway.map.getCall(0).args[0]).to.equal(4002)
-    expect(gateway.map.getCall(0).args[1]).to.include({
-      protocol: 'tcp'
+    expect(gateway.map.getCall(0).args[0]).to.equal(internalPort)
+    expect(gateway.map.getCall(0).args[1]).to.equal(internalHost)
+    expect(gateway.map.getCall(0).args[2]).to.include({
+      protocol: 'TCP'
     })
     expect(components.addressManager.addPublicAddressMapping.called).to.be.true()
   })