fix(@libp2p/mdns): do not send TXT records that are too long (#2014)

The data field of mDNS TXT records has a [hard limit of 255 characters](https://agnat.github.io/node_mdns/user_guide.html#txt_records) - some multiaddrs can be longer than this so filter them out before sending, otherwise remote peers can fail to parse our mDNS response.

We should also only be sending [link-local addresses](https://github.com/libp2p/specs/blob/master/discovery/mdns.md#issues) in mDNS responses so filter any non-link-local addresses out too, though go-libp2p includes loopback addresses even though it probably shouldn't (according to the mDNS spec which conflicts with our mDNS Peer Discovery spec) so we continue to do so as well.

Fixes #2012

Author: achingbrain

packages/peer-discovery-mdns/package.json

@@ -47,6 +47,7 @@
     "@libp2p/interface": "^0.1.2",
     "@libp2p/logger": "^3.0.2",
     "@libp2p/peer-id": "^3.0.2",
+    "@libp2p/utils": "^4.0.2",
     "@multiformats/multiaddr": "^12.1.5",
     "@types/multicast-dns": "^7.2.1",
     "dns-packet": "^5.4.0",

packages/peer-discovery-mdns/src/index.ts

@@ -6,6 +6,7 @@ import * as query from './query.js'
 import { stringGen } from './utils.js'
 import type { PeerDiscovery, PeerDiscoveryEvents } from '@libp2p/interface/peer-discovery'
 import type { PeerInfo } from '@libp2p/interface/peer-info'
+import type { Startable } from '@libp2p/interface/src/startable.js'
 import type { AddressManager } from '@libp2p/interface-internal/address-manager'
 
 const log = logger('libp2p:mdns')
@@ -23,7 +24,7 @@ export interface MulticastDNSComponents {
   addressManager: AddressManager
 }
 
-class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDiscovery {
+class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDiscovery, Startable {
   public mdns?: multicastDNS.MulticastDNS
 
   private readonly broadcast: boolean
@@ -50,9 +51,10 @@ class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDisc
     this.port = init.port ?? 5353
     this.components = components
     this._queryInterval = null
-    this._onPeer = this._onPeer.bind(this)
     this._onMdnsQuery = this._onMdnsQuery.bind(this)
     this._onMdnsResponse = this._onMdnsResponse.bind(this)
+    this._onMdnsWarning = this._onMdnsWarning.bind(this)
+    this._onMdnsError = this._onMdnsError.bind(this)
   }
 
   readonly [peerDiscovery] = this
@@ -76,6 +78,8 @@ class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDisc
     this.mdns = multicastDNS({ port: this.port, ip: this.ip })
     this.mdns.on('query', this._onMdnsQuery)
     this.mdns.on('response', this._onMdnsResponse)
+    this.mdns.on('warning', this._onMdnsWarning)
+    this.mdns.on('error', this._onMdnsError)
 
     this._queryInterval = query.queryLAN(this.mdns, this.serviceTag, this.interval)
   }
@@ -113,14 +117,12 @@ class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDisc
     }
   }
 
-  _onPeer (evt: CustomEvent<PeerInfo>): void {
-    if (this.mdns == null) {
-      return
-    }
+  _onMdnsWarning (err: Error): void {
+    log.error('mdns warning', err)
+  }
 
-    this.dispatchEvent(new CustomEvent<PeerInfo>('peer', {
-      detail: evt.detail
-    }))
+  _onMdnsError (err: Error): void {
+    log.error('mdns error', err)
   }
 
   /**
@@ -135,6 +137,8 @@ class MulticastDNS extends EventEmitter<PeerDiscoveryEvents> implements PeerDisc
 
     this.mdns.removeListener('query', this._onMdnsQuery)
     this.mdns.removeListener('response', this._onMdnsResponse)
+    this.mdns.removeListener('warning', this._onMdnsWarning)
+    this.mdns.removeListener('error', this._onMdnsError)
 
     if (this._queryInterval != null) {
       clearInterval(this._queryInterval)

packages/peer-discovery-mdns/src/query.ts

@@ -1,6 +1,7 @@
 import { logger } from '@libp2p/logger'
 import { peerIdFromString } from '@libp2p/peer-id'
-import { multiaddr, type Multiaddr } from '@multiformats/multiaddr'
+import { isPrivate } from '@libp2p/utils/multiaddr/is-private'
+import { multiaddr, type Multiaddr, protocols } from '@multiformats/multiaddr'
 import type { PeerInfo } from '@libp2p/interface/peer-info'
 import type { Answer, StringAnswer, TxtAnswer } from 'dns-packet'
 import type { MulticastDNS, QueryPacket, ResponsePacket } from 'multicast-dns'
@@ -9,7 +10,7 @@ const log = logger('libp2p:mdns:query')
 
 export function queryLAN (mdns: MulticastDNS, serviceTag: string, interval: number): ReturnType<typeof setInterval> {
   const query = (): void => {
-    log('query', serviceTag)
+    log.trace('query', serviceTag)
 
     mdns.query({
       questions: [{
@@ -40,6 +41,16 @@ export function gotResponse (rsp: ResponsePacket, localPeerName: string, service
     }
   })
 
+  // according to the spec, peer details should be in the additional records,
+  // not the answers though it seems go-libp2p at least ignores this?
+  // https://github.com/libp2p/specs/blob/master/discovery/mdns.md#response
+  rsp.additionals.forEach((answer) => {
+    switch (answer.type) {
+      case 'TXT': txtAnswers.push(answer); break
+      default: break
+    }
+  })
+
   if (answerPTR == null ||
     answerPTR?.name !== serviceTag ||
     txtAnswers.length === 0 ||
@@ -63,7 +74,7 @@ export function gotResponse (rsp: ResponsePacket, localPeerName: string, service
 
     return {
       id: peerIdFromString(peerId),
-      multiaddrs,
+      multiaddrs: multiaddrs.map(addr => addr.decapsulateCode(protocols('p2p').code)),
       protocols: []
     }
   } catch (e) {
@@ -92,20 +103,45 @@ export function gotQuery (qry: QueryPacket, mdns: MulticastDNS, peerName: string
       data: peerName + '.' + serviceTag
     })
 
-    multiaddrs.forEach((addr) => {
-      // spec mandates multiaddr contains peer id
-      if (addr.getPeerId() != null) {
+    multiaddrs
+      // mDNS requires link-local addresses only
+      // https://github.com/libp2p/specs/blob/master/discovery/mdns.md#issues
+      .filter(isLinkLocal)
+      .forEach((addr) => {
+        const data = 'dnsaddr=' + addr.toString()
+
+        // TXT record fields have a max data length of 255 bytes
+        // see 6.1 - https://www.ietf.org/rfc/rfc6763.txt
+        if (data.length > 255) {
+          log('multiaddr %a is too long to use in mDNS query response', addr)
+          return
+        }
+
+        // spec mandates multiaddr contains peer id
+        if (addr.getPeerId() == null) {
+          log('multiaddr %a did not have a peer ID so cannot be used in mDNS query response', addr)
+          return
+        }
+
         answers.push({
           name: peerName + '.' + serviceTag,
           type: 'TXT',
           class: 'IN',
           ttl: 120,
-          data: 'dnsaddr=' + addr.toString()
+          data
         })
-      }
-    })
+      })
 
-    log('responding to query')
+    log.trace('responding to query')
     mdns.respond(answers)
   }
 }
+
+function isLinkLocal (ma: Multiaddr): boolean {
+  // match private ip4/ip6 & loopback addresses
+  if (isPrivate(ma)) {
+    return true
+  }
+
+  return false
+}

packages/peer-discovery-mdns/test/compliance.spec.ts

@@ -1,6 +1,7 @@
 /* eslint-env mocha */
 
 import { CustomEvent } from '@libp2p/interface/events'
+import { isStartable } from '@libp2p/interface/startable'
 import tests from '@libp2p/interface-compliance-tests/peer-discovery'
 import { createEd25519PeerId } from '@libp2p/peer-id-factory'
 import { multiaddr } from '@multiformats/multiaddr'
@@ -32,16 +33,21 @@ describe('compliance tests', () => {
       })
 
       // Trigger discovery
-      const maStr = '/ip4/127.0.0.1/tcp/15555/ws/p2p-webrtc-star/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSooo2d'
-
-      // @ts-expect-error not a PeerDiscovery field
-      intervalId = setInterval(() => discovery._onPeer(new CustomEvent('peer', {
-        detail: {
-          id: peerId2,
-          multiaddrs: [multiaddr(maStr)],
-          protocols: []
+      const maStr = '/ip4/127.0.0.1/tcp/15555/ws/p2p-webrtc-star'
+
+      intervalId = setInterval(() => {
+        if (isStartable(discovery) && !discovery.isStarted()) {
+          return
         }
-      })), 1000)
+
+        discovery.dispatchEvent(new CustomEvent('peer', {
+          detail: {
+            id: peerId2,
+            multiaddrs: [multiaddr(maStr)],
+            protocols: []
+          }
+        }))
+      }, 1000)
 
       return discovery
     },

packages/peer-discovery-mdns/test/multicast-dns.spec.ts

@@ -38,25 +38,25 @@ describe('MulticastDNS', () => {
     ])
 
     aMultiaddrs = [
-      multiaddr('/ip4/127.0.0.1/tcp/20001'),
+      multiaddr('/ip4/192.168.1.142/tcp/20001'),
       multiaddr('/dns4/webrtc-star.discovery.libp2p.io/tcp/443/wss/p2p-webrtc-star'),
       multiaddr('/dns4/discovery.libp2p.io/tcp/8443')
     ]
 
     bMultiaddrs = [
-      multiaddr('/ip4/127.0.0.1/tcp/20002'),
-      multiaddr('/ip6/::1/tcp/20002'),
+      multiaddr('/ip4/192.168.1.143/tcp/20002'),
+      multiaddr('/ip6/2604:1380:4602:5c00::3/tcp/20002'),
       multiaddr('/dnsaddr/discovery.libp2p.io')
     ]
 
     cMultiaddrs = [
-      multiaddr('/ip4/127.0.0.1/tcp/20003'),
-      multiaddr('/ip4/127.0.0.1/tcp/30003/ws'),
+      multiaddr('/ip4/192.168.1.144/tcp/20003'),
+      multiaddr('/ip4/192.168.1.144/tcp/30003/ws'),
       multiaddr('/dns4/discovery.libp2p.io')
     ]
 
     dMultiaddrs = [
-      multiaddr('/ip4/127.0.0.1/tcp/30003/ws')
+      multiaddr('/ip4/192.168.1.145/tcp/30003/ws')
     ]
   })
 
@@ -110,7 +110,8 @@ describe('MulticastDNS', () => {
     await pWaitFor(() => peers.has(expectedPeer))
     mdnsA.removeEventListener('peer', foundPeer)
 
-    expect(peers.get(expectedPeer).multiaddrs.length).to.equal(3)
+    // everything except loopback
+    expect(peers.get(expectedPeer).multiaddrs.length).to.equal(2)
 
     await stop(mdnsA, mdnsB, mdnsD)
   })
@@ -141,15 +142,6 @@ describe('MulticastDNS', () => {
     await stop(mdnsC)
   })
 
-  it('should start and stop with go-libp2p-mdns compat', async () => {
-    const mdnsA = mdns({
-      port: 50004
-    })(getComponents(pA, aMultiaddrs))
-
-    await start(mdnsA)
-    await stop(mdnsA)
-  })
-
   it('should not emit undefined peer ids', async () => {
     const mdnsA = mdns({
       port: 50004
@@ -219,4 +211,80 @@ describe('MulticastDNS', () => {
 
     await stop(mdnsA, mdnsB)
   })
+
+  it('only includes link-local addresses', async function () {
+    this.timeout(40 * 1000)
+
+    // these are not link-local addresses
+    const publicAddress = '/ip4/48.52.76.32/tcp/1234'
+    const relayDnsAddress = `/dnsaddr/example.org/tcp/1234/p2p/${pD.toString()}/p2p-circuit`
+    const dnsAddress = '/dns4/example.org/tcp/1234'
+
+    // this address is too long to fit in a TXT record
+    const longAddress = `/ip4/192.168.1.142/udp/4001/quic-v1/webtransport/certhash/uEiDils3hWFJmsWOJIoMPxAcpzlyFNxTDZpklIoB8643ddw/certhash/uEiAM4BGr4OMK3O9cFGwfbNc4J7XYnsKE5wNPKKaTLa4fkw/p2p/${pD.toString()}/p2p-circuit`
+
+    // these are link-local addresses
+    const relayAddress = `/ip4/192.168.1.142/tcp/1234/p2p/${pD.toString()}/p2p-circuit`
+    const localAddress = '/ip4/192.168.1.123/tcp/1234'
+    const localWsAddress = '/ip4/192.168.1.123/tcp/1234/ws'
+
+    // these are not link-local but go-libp2p advertises loopback addresses even
+    // though you shouldn't for mDNS
+    const loopbackAddress = '/ip4/127.0.0.1/tcp/1234'
+    const loopbackAddress6 = '/ip6/::1/tcp/1234'
+
+    const mdnsA = mdns({
+      broadcast: false, // do not talk to ourself
+      port: 50005,
+      ip: '224.0.0.252'
+    })(getComponents(pA, aMultiaddrs))
+
+    const mdnsB = mdns({
+      port: 50005, // port must be the same
+      ip: '224.0.0.252' // ip must be the same
+    })(getComponents(pB, [
+      multiaddr(publicAddress),
+      multiaddr(relayAddress),
+      multiaddr(relayDnsAddress),
+      multiaddr(localAddress),
+      multiaddr(loopbackAddress),
+      multiaddr(loopbackAddress6),
+      multiaddr(dnsAddress),
+      multiaddr(longAddress),
+      multiaddr(localWsAddress)
+    ]))
+
+    await start(mdnsA, mdnsB)
+
+    const { detail: { id, multiaddrs } } = await new Promise<CustomEvent<PeerInfo>>((resolve) => {
+      mdnsA.addEventListener('peer', resolve, {
+        once: true
+      })
+    })
+
+    expect(pB.toString()).to.eql(id.toString())
+
+    ;[
+      publicAddress,
+      relayDnsAddress,
+      dnsAddress,
+      longAddress
+    ].forEach(addr => {
+      expect(multiaddrs.map(ma => ma.toString()))
+        .to.not.include(addr)
+    })
+
+    ;[
+      relayAddress,
+      localAddress,
+      localWsAddress,
+      loopbackAddress,
+      loopbackAddress6
+    ].forEach(addr => {
+      expect(multiaddrs.map(ma => ma.toString()))
+        .to.include(addr)
+    })
+
+    await stop(mdnsA, mdnsB)
+  })
 })