diff --git a/packages/libp2p/src/upnp-nat/index.ts b/packages/libp2p/src/upnp-nat/index.ts index 8e422f6b1a..f5693b5c18 100644 --- a/packages/libp2p/src/upnp-nat/index.ts +++ b/packages/libp2p/src/upnp-nat/index.ts @@ -7,10 +7,12 @@ import isPrivateIp from 'private-ip' import { isBrowser } from 'wherearewe' import { codes } from '../errors.js' import * as pkg from '../version.js' -import type { PeerId } from '@libp2p/interface/peer-id' -import type { Startable } from '@libp2p/interface/startable' import type { AddressManager } from '@libp2p/interface-internal/address-manager' +import type { Libp2pEvents } from '@libp2p/interface' +import type { PeerId } from '@libp2p/interface-peer-id' import type { TransportManager } from '@libp2p/interface-internal/transport-manager' +import type { EventEmitter } from '@libp2p/interface/events' +import type { Startable } from '@libp2p/interface/startable' const log = logger('libp2p:upnp-nat') const DEFAULT_TTL = 7200 @@ -62,6 +64,7 @@ export interface UPnPNATComponents { peerId: PeerId transportManager: TransportManager addressManager: AddressManager + events: EventEmitter } class UPnPNAT implements Startable { @@ -89,6 +92,14 @@ class UPnPNAT implements Startable { if (this.ttl < DEFAULT_TTL) { throw new CodeError(`NatManager ttl should be at least ${DEFAULT_TTL} seconds`, codes.ERR_INVALID_PARAMETERS) } + + // try to map external ports when our address list changes + this.components.events.addEventListener('self:peer:update', () => { + this.mapAddresses() + .catch(err => { + log.error('error mapping external addresses', err) + }) + }) } isStarted (): boolean { @@ -96,29 +107,19 @@ class UPnPNAT implements Startable { } start (): void { - // #TODO: is there a way to remove this? Seems like a hack - } - - /** - * Attempt to use uPnP to configure port mapping using the current gateway. - * - * Run after start to ensure the transport manager has all addresses configured. - */ - afterStart (): void { if (isBrowser || this.started) { return } this.started = true - - // done async to not slow down startup - void this._start().catch((err) => { - // hole punching errors are non-fatal - log.error(err) - }) } - async _start (): Promise { + /** + * Attempt to use uPnP to configure port mapping using the current gateway. + * + * Run after start to ensure the transport manager has all addresses configured. + */ + async mapAddresses (): Promise { const addrs = this.components.transportManager.getAddrs() for (const addr of addrs) { @@ -128,21 +129,31 @@ class UPnPNAT implements Startable { if (!addr.isThinWaistAddress() || transport !== 'tcp') { // only bare tcp addresses // eslint-disable-next-line no-continue + log.trace('skipping %a as it is not a think waist address', addr) continue } if (isLoopback(addr)) { // eslint-disable-next-line no-continue + log.trace('skipping %a as it is a loopback address', addr) continue } if (family !== 4) { // ignore ipv6 // eslint-disable-next-line no-continue + log.trace('skipping %a as it is not an ip4 address', addr) continue } const client = this._getClient() + + if (client.openPorts.map(p => p.localPort).includes(port)) { + // skip ports we have already mapped + log.trace('skipping %a as it is already mapped', addr) + continue + } + const publicIp = this.externalAddress ?? await client.externalIp() const isPrivate = isPrivateIp(publicIp) @@ -154,6 +165,10 @@ class UPnPNAT implements Startable { throw new Error(`${publicIp} is not an IP address`) } + if (!this.isStarted()) { + return + } + const publicPort = highPort() log(`opening uPnP connection from ${publicIp}:${publicPort} to ${host}:${port}`) @@ -165,11 +180,15 @@ class UPnPNAT implements Startable { protocol: transport.toUpperCase() === 'TCP' ? 'TCP' : 'UDP' }) - this.components.addressManager.addObservedAddr(fromNodeAddress({ + const externalAddress = fromNodeAddress({ family: 4, address: publicIp, port: publicPort - }, transport)) + }, transport) + + log('mapped external uPnP address %a', externalAddress) + + this.components.addressManager.addObservedAddr(externalAddress) } } diff --git a/packages/libp2p/test/upnp-nat/upnp-nat.node.ts b/packages/libp2p/test/upnp-nat/upnp-nat.node.ts index 42d6cc6407..ee70040591 100644 --- a/packages/libp2p/test/upnp-nat/upnp-nat.node.ts +++ b/packages/libp2p/test/upnp-nat/upnp-nat.node.ts @@ -8,8 +8,9 @@ import { createEd25519PeerId } from '@libp2p/peer-id-factory' import { tcp } from '@libp2p/tcp' import { multiaddr } from '@multiformats/multiaddr' import { expect } from 'aegir/chai' -import delay from 'delay' import { pEvent } from 'p-event' +import pWaitFor from 'p-wait-for' +import sinon from 'sinon' import { type StubbedInstance, stubInterface } from 'sinon-ts' import { DefaultAddressManager } from '../../src/address-manager/index.js' import { defaultComponents, type Components } from '../../src/components.js' @@ -18,7 +19,7 @@ import { DefaultTransportManager } from '../../src/transport-manager.js' import { uPnPNATService } from '../../src/upnp-nat/index.js' import type { NatAPI } from '@achingbrain/nat-port-mapper' import type { PeerUpdate } from '@libp2p/interface' -import type { PeerId } from '@libp2p/interface/peer-id' +import type { PeerId } from '@libp2p/interface-peer-id' import type { PeerData, PeerStore } from '@libp2p/interface/peer-store' const DEFAULT_ADDRESSES = [ @@ -26,17 +27,18 @@ const DEFAULT_ADDRESSES = [ '/ip4/0.0.0.0/tcp/0' ] -describe('UPnP NAT (TCP)', () => { +describe.only('UPnP NAT (TCP)', () => { const teardown: Array<() => Promise> = [] let client: StubbedInstance async function createNatManager (addrs = DEFAULT_ADDRESSES, natManagerOptions = {}): Promise<{ natManager: any, components: Components }> { const events = new EventEmitter() + const peerStore: StubbedInstance = stubInterface() const components: any = defaultComponents({ peerId: await createEd25519PeerId(), upgrader: mockUpgrader({ events }), events, - peerStore: stubInterface() + peerStore }) components.peerStore.patch.callsFake(async (peerId: PeerId, details: PeerData) => { @@ -59,6 +61,7 @@ describe('UPnP NAT (TCP)', () => { })(components) client = stubInterface() + client.openPorts = [] natManager._getClient = () => { return client @@ -67,11 +70,11 @@ describe('UPnP NAT (TCP)', () => { components.transportManager.add(tcp()()) await start(components) + await components.transportManager.listen(components.addressManager.getListenAddrs()) teardown.push(async () => { await stop(natManager) await components.transportManager.removeAll() - await stop(components) }) return { @@ -95,7 +98,17 @@ describe('UPnP NAT (TCP)', () => { await start(natManager) - await delay(100) + const addObservedAddr = sinon.spy(components.addressManager, 'addObservedAddr') + + components.events.safeDispatchEvent('self:peer:update', { + detail: { + peer: {} + } + }) + + await pWaitFor(() => addObservedAddr.called, { + interval: 100 + }) observed = components.addressManager.getObservedAddrs().map(ma => ma.toString()) expect(observed).to.not.be.empty() @@ -108,6 +121,8 @@ describe('UPnP NAT (TCP)', () => { expect(client.map.called).to.be.true() + console.info('internalPorts', internalPorts) + internalPorts.forEach(port => { expect(client.map.getCall(0).args[0]).to.include({ localPort: port, @@ -135,7 +150,7 @@ describe('UPnP NAT (TCP)', () => { let observed = components.addressManager.getObservedAddrs().map(ma => ma.toString()) expect(observed).to.be.empty() - await expect(natManager._start()).to.eventually.be.rejectedWith(/double NAT/) + await expect(natManager.mapAddresses()).to.eventually.be.rejectedWith(/double NAT/) observed = components.addressManager.getObservedAddrs().map(ma => ma.toString()) expect(observed).to.be.empty()