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

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)
+  })
 })