From 8c0093d01e47177aaad7b8d01a0e8edab1d8d86f Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 22 Nov 2024 17:01:48 +0000 Subject: [PATCH] fix: use ip/port mapping Switch to the new public address mapping api in the address manager to ensure all host/port pairs are advertised with external ip addresses --- .../upnp-nat/src/check-external-address.ts | 33 ++---- packages/upnp-nat/src/index.ts | 20 ---- packages/upnp-nat/src/upnp-nat.ts | 111 +++++++++--------- packages/upnp-nat/test/index.spec.ts | 20 ++-- 4 files changed, 71 insertions(+), 113 deletions(-) diff --git a/packages/upnp-nat/src/check-external-address.ts b/packages/upnp-nat/src/check-external-address.ts index bad9f14d3c..f6a9fdfe2d 100644 --- a/packages/upnp-nat/src/check-external-address.ts +++ b/packages/upnp-nat/src/check-external-address.ts @@ -1,6 +1,5 @@ import { NotStartedError, start, stop } from '@libp2p/interface' import { repeatingTask } from '@libp2p/utils/repeating-task' -import { multiaddr } from '@multiformats/multiaddr' import pDefer from 'p-defer' import { raceSignal } from 'race-signal' import type { NatAPI } from '@achingbrain/nat-port-mapper' @@ -18,7 +17,7 @@ export interface ExternalAddressCheckerComponents { export interface ExternalAddressCheckerInit { interval?: number timeout?: number - autoConfirmAddress?: boolean + onExternalAddressChange?(newExternalAddress: string): void } export interface ExternalAddress { @@ -36,13 +35,13 @@ class ExternalAddressChecker implements ExternalAddress, Startable { private lastPublicIp?: string private readonly lastPublicIpPromise: DeferredPromise private readonly check: RepeatingTask - private readonly autoConfirmAddress: boolean + private readonly onExternalAddressChange?: (newExternalAddress: string) => void - constructor (components: ExternalAddressCheckerComponents, init: ExternalAddressCheckerInit = {}) { + constructor (components: ExternalAddressCheckerComponents, init: ExternalAddressCheckerInit) { this.log = components.logger.forComponent('libp2p:upnp-nat:external-address-check') this.client = components.client this.addressManager = components.addressManager - this.autoConfirmAddress = init.autoConfirmAddress ?? false + this.onExternalAddressChange = init.onExternalAddressChange this.started = false this.checkExternalAddress = this.checkExternalAddress.bind(this) @@ -93,26 +92,8 @@ class ExternalAddressChecker implements ExternalAddress, Startable { if (this.lastPublicIp != null && externalAddress !== this.lastPublicIp) { this.log('external address changed from %s to %s', this.lastPublicIp, externalAddress) - for (const ma of this.addressManager.getAddresses()) { - const addrString = ma.toString() - - if (!addrString.includes(this.lastPublicIp)) { - continue - } - - // create a new version of the multiaddr with the new public IP - const newAddress = multiaddr(addrString.replace(this.lastPublicIp, externalAddress)) - - // remove the old address and add the new one - this.addressManager.removeObservedAddr(ma) - this.addressManager.confirmObservedAddr(newAddress) - - if (this.autoConfirmAddress) { - this.addressManager.confirmObservedAddr(newAddress) - } else { - this.addressManager.addObservedAddr(newAddress) - } - } + // notify listeners that the address has changed + this.onExternalAddressChange?.(externalAddress) } this.lastPublicIp = externalAddress @@ -128,7 +109,7 @@ class ExternalAddressChecker implements ExternalAddress, Startable { } } -export function dynamicExternalAddress (components: ExternalAddressCheckerComponents, init: ExternalAddressCheckerInit = {}): ExternalAddress { +export function dynamicExternalAddress (components: ExternalAddressCheckerComponents, init: ExternalAddressCheckerInit): ExternalAddress { return new ExternalAddressChecker(components, init) } diff --git a/packages/upnp-nat/src/index.ts b/packages/upnp-nat/src/index.ts index 5168a601cd..8f5152b396 100644 --- a/packages/upnp-nat/src/index.ts +++ b/packages/upnp-nat/src/index.ts @@ -96,15 +96,6 @@ export interface UPnPNATInit { */ gateway?: string - /** - * How long in ms to wait before giving up trying to auto-detect a - * `urn:schemas-upnp-org:device:InternetGatewayDevice:1` device on the local - * network - * - * @default 10000 - */ - gatewayDetectionTimeout?: number - /** * Ports are mapped when the `self:peer:update` event fires, which happens * when the node's addresses change. To avoid starting to map ports while @@ -120,17 +111,6 @@ export interface UPnPNATInit { * otherwise one will be created */ client?: NatAPI - - /** - * Any mapped addresses are added to the observed address list. These - * addresses require additional verification by the `@libp2p/autonat` protocol - * or similar before they are trusted. - * - * To skip this verification and trust them immediately pass `true` here - * - * @default false - */ - autoConfirmAddress?: boolean } export interface UPnPNATComponents { diff --git a/packages/upnp-nat/src/upnp-nat.ts b/packages/upnp-nat/src/upnp-nat.ts index 78e7b55dbf..60a2ba4b94 100644 --- a/packages/upnp-nat/src/upnp-nat.ts +++ b/packages/upnp-nat/src/upnp-nat.ts @@ -1,13 +1,11 @@ import { upnpNat } from '@achingbrain/nat-port-mapper' import { isIPv4, isIPv6 } from '@chainsafe/is-ip' -import { InvalidParametersError, serviceCapabilities, serviceDependencies, start, stop } from '@libp2p/interface' +import { InvalidParametersError, serviceCapabilities, setMaxListeners, start, stop } from '@libp2p/interface' import { debounce } from '@libp2p/utils/debounce' import { isLoopback } from '@libp2p/utils/multiaddr/is-loopback' import { isPrivate } from '@libp2p/utils/multiaddr/is-private' import { isPrivateIp } from '@libp2p/utils/private-ip' -import { multiaddr } from '@multiformats/multiaddr' import { QUICV1, TCP, WebSockets, WebSocketsSecure, WebTransport } from '@multiformats/multiaddr-matcher' -import { raceSignal } from 'race-signal' import { dynamicExternalAddress, staticExternalAddress } from './check-external-address.js' import { DoubleNATError, InvalidIPAddressError } from './errors.js' import type { ExternalAddress } from './check-external-address.js' @@ -22,35 +20,35 @@ const DEFAULT_TTL = 7200 export type { NatAPI, MapPortOptions } +interface PortMapping { + externalHost: string + externalPort: number +} + export class UPnPNAT implements Startable, UPnPNATInterface { public client: NatAPI private readonly addressManager: AddressManager private readonly events: TypedEventTarget private readonly externalAddress: ExternalAddress - private readonly localAddress?: string private readonly description: string private readonly ttl: number private readonly keepAlive: boolean private readonly gateway?: string private started: boolean private readonly log: Logger - private readonly gatewayDetectionTimeout: number - private readonly mappedPorts: Map + private readonly mappedPorts: Map private readonly onSelfPeerUpdate: DebouncedFunction - private readonly autoConfirmAddress: boolean + private shutdownController?: AbortController constructor (components: UPnPNATComponents, init: UPnPNATInit) { this.log = components.logger.forComponent('libp2p:upnp-nat') this.addressManager = components.addressManager this.events = components.events this.started = false - this.localAddress = init.localAddress this.description = init.description ?? `${components.nodeInfo.name}@${components.nodeInfo.version} ${components.peerId.toString()}` this.ttl = init.ttl ?? DEFAULT_TTL this.keepAlive = init.keepAlive ?? true this.gateway = init.gateway - this.gatewayDetectionTimeout = init.gatewayDetectionTimeout ?? 10000 - this.autoConfirmAddress = init.autoConfirmAddress ?? false this.mappedPorts = new Map() if (this.ttl < DEFAULT_TTL) { @@ -74,9 +72,9 @@ export class UPnPNAT implements Startable, UPnPNATInterface { addressManager: this.addressManager, logger: components.logger }, { - autoConfirmAddress: init.autoConfirmAddress, interval: init.externalAddressCheckInterval, - timeout: init.externalAddressCheckTimeout + timeout: init.externalAddressCheckTimeout, + onExternalAddressChange: this.remapPorts.bind(this) }) } } @@ -87,16 +85,6 @@ export class UPnPNAT implements Startable, UPnPNATInterface { '@libp2p/nat-traversal' ] - get [serviceDependencies] (): string[] { - if (!this.autoConfirmAddress) { - return [ - '@libp2p/autonat' - ] - } - - return [] - } - isStarted (): boolean { return this.started } @@ -107,6 +95,8 @@ export class UPnPNAT implements Startable, UPnPNATInterface { } this.started = true + this.shutdownController = new AbortController() + setMaxListeners(Infinity, this.shutdownController.signal) this.events.addEventListener('self:peer:update', this.onSelfPeerUpdate) await start(this.externalAddress, this.onSelfPeerUpdate) } @@ -121,6 +111,7 @@ export class UPnPNAT implements Startable, UPnPNATInterface { this.log.error(err) } + this.shutdownController?.abort() this.events.removeEventListener('self:peer:update', this.onSelfPeerUpdate) await stop(this.externalAddress, this.onSelfPeerUpdate) this.started = false @@ -158,13 +149,13 @@ export class UPnPNAT implements Startable, UPnPNATInterface { continue } - const { port, family } = ma.toOptions() + const { port, host, family, transport } = ma.toOptions() if (family !== ipType) { continue } - if (this.mappedPorts.has(port)) { + if (this.mappedPorts.has(`${host}-${port}-${transport}`)) { continue } @@ -175,26 +166,18 @@ export class UPnPNAT implements Startable, UPnPNATInterface { } async mapIpAddresses (): Promise { - if (this.externalAddress == null) { - this.log('discovering public address') - } - - const publicIp = await this.externalAddress.getPublicIp({ - signal: AbortSignal.timeout(this.gatewayDetectionTimeout) - }) - - this.externalAddress ?? await raceSignal(this.client.externalIp(), AbortSignal.timeout(this.gatewayDetectionTimeout), { - errorMessage: `Did not discover a "urn:schemas-upnp-org:device:InternetGatewayDevice:1" device on the local network after ${this.gatewayDetectionTimeout}ms - UPnP may not be configured on your router correctly` + const externalHost = await this.externalAddress.getPublicIp({ + signal: this.shutdownController?.signal }) let ipType: 4 | 6 = 4 - if (isIPv4(publicIp)) { + if (isIPv4(externalHost)) { ipType = 4 - } else if (isIPv6(publicIp)) { + } else if (isIPv6(externalHost)) { ipType = 6 } else { - throw new InvalidIPAddressError(`Public address ${publicIp} was not an IPv4 address`) + throw new InvalidIPAddressError(`Public address ${externalHost} was not an IPv4 address`) } // filter addresses to get private, non-relay, IP based addresses that we @@ -206,9 +189,9 @@ export class UPnPNAT implements Startable, UPnPNATInterface { return } - this.log('%s public IP %s', this.externalAddress != null ? 'using configured' : 'discovered', publicIp) + this.log('%s public IP %s', this.externalAddress != null ? 'using configured' : 'discovered', externalHost) - this.assertNotBehindDoubleNAT(publicIp) + this.assertNotBehindDoubleNAT(externalHost) for (const addr of addresses) { // try to open uPnP ports for each thin waist address @@ -219,30 +202,29 @@ export class UPnPNAT implements Startable, UPnPNATInterface { continue } - if (this.mappedPorts.has(port)) { + if (this.mappedPorts.has(`${host}-${port}-${transport}`)) { // already mapped this port continue } - const protocol = transport.toUpperCase() - - this.log(`creating mapping of ${host}:${port}`) + try { + this.log(`creating mapping of %s:%s key ${host}-${port}-${transport}`, host, port) - const mappedPort = await this.client.map(port, { - localAddress: host, - protocol: protocol === 'TCP' ? 'TCP' : 'UDP' - }) + const externalPort = await this.client.map(port, { + localAddress: host, + protocol: transport === 'tcp' ? 'TCP' : 'UDP' + }) - this.mappedPorts.set(port, mappedPort) + this.mappedPorts.set(`${host}-${port}-${transport}`, { + externalHost, + externalPort + }) - const ma = multiaddr(addr.toString().replace(`/ip${family}/${host}/${transport}/${port}`, `/ip${family}/${publicIp}/${transport}/${mappedPort}`)) + this.log('created mapping of %s:%s to %s:%s', externalHost, externalPort, host, port) - this.log(`created mapping of ${publicIp}:${mappedPort} to ${host}:${port} as %a`, ma) - - if (this.autoConfirmAddress) { - this.addressManager.confirmObservedAddr(ma) - } else { - this.addressManager.addObservedAddr(ma) + this.addressManager.addPublicAddressMapping(host, port, externalHost, externalPort, transport === 'tcp' ? 'tcp' : 'udp') + } catch (err) { + this.log.error('failed to create mapping of %s:%s - %e', host, port, err) } } } @@ -254,11 +236,28 @@ export class UPnPNAT implements Startable, UPnPNATInterface { const isPrivate = isPrivateIp(publicIp) if (isPrivate === true) { - throw new DoubleNATError(`${publicIp} is private - please set config.nat.externalIp to an externally routable IP or ensure you are not behind a double NAT`) + throw new DoubleNATError(`${publicIp} is private - please init uPnPNAT with 'externalAddress' set to an externally routable IP or ensure you are not behind a double NAT`) } if (isPrivate == null) { throw new InvalidParametersError(`${publicIp} is not an IP address`) } } + + /** + * Update the local address mappings when the gateway's external interface + * address changes + */ + private remapPorts (newExternalHost: string): void { + for (const [key, { externalHost, externalPort }] of this.mappedPorts.entries()) { + const [ + host, + port, + transport + ] = key.split('-') + + this.addressManager.removePublicAddressMapping(host, parseInt(port), externalHost, externalPort, transport === 'tcp' ? 'tcp' : 'udp') + this.addressManager.addPublicAddressMapping(host, parseInt(port), newExternalHost, externalPort, transport === 'tcp' ? 'tcp' : 'udp') + } + } } diff --git a/packages/upnp-nat/test/index.spec.ts b/packages/upnp-nat/test/index.spec.ts index eefb064e02..7e5a54a2f7 100644 --- a/packages/upnp-nat/test/index.spec.ts +++ b/packages/upnp-nat/test/index.spec.ts @@ -81,16 +81,14 @@ describe('UPnP NAT (TCP)', () => { expect(client.map.getCall(0).args[1]).to.include({ protocol: 'TCP' }) - expect(components.addressManager.addObservedAddr.called).to.be.true() + expect(components.addressManager.addPublicAddressMapping.called).to.be.true() }) it('should map TCP connections to external ports and trust them immediately', async () => { const { natManager, components - } = await createNatManager({ - autoConfirmAddress: true - }) + } = await createNatManager() client.externalIp.resolves('82.3.1.5') @@ -107,7 +105,7 @@ describe('UPnP NAT (TCP)', () => { expect(client.map.getCall(0).args[1]).to.include({ protocol: 'TCP' }) - expect(components.addressManager.confirmObservedAddr.called).to.be.true() + expect(components.addressManager.addPublicAddressMapping.called).to.be.true() }) it('should not map TCP connections when double-natted', async () => { @@ -128,7 +126,7 @@ describe('UPnP NAT (TCP)', () => { .with.property('name', 'DoubleNATError') expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should not map non-ipv4 connections to external ports', async () => { @@ -147,7 +145,7 @@ describe('UPnP NAT (TCP)', () => { await natManager.mapIpAddresses() expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should not map non-ipv6 loopback connections to external ports', async () => { @@ -166,7 +164,7 @@ describe('UPnP NAT (TCP)', () => { await natManager.mapIpAddresses() expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should not map non-TCP connections to external ports', async () => { @@ -185,7 +183,7 @@ describe('UPnP NAT (TCP)', () => { await natManager.mapIpAddresses() expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should not map loopback connections to external ports', async () => { @@ -204,7 +202,7 @@ describe('UPnP NAT (TCP)', () => { await natManager.mapIpAddresses() expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should not map non-thin-waist connections to external ports', async () => { @@ -223,7 +221,7 @@ describe('UPnP NAT (TCP)', () => { await natManager.mapIpAddresses() expect(client.map.called).to.be.false() - expect(components.addressManager.addObservedAddr.called).to.be.false() + expect(components.addressManager.addPublicAddressMapping.called).to.be.false() }) it('should specify large enough TTL', async () => {