fix: do not overwrite addresses on identify push when none are sent (#2192)

During identify-push in response to a protocol update go-libp2p does not send a signed peer record or the current list of listen addresses.

Protobuf does not let us differentiate between an empty list and no list items at all because the field is just repeated - an absence of repeated fields doesn't mean the list was omitted.

When the listen address list is empty, preserve any existing addresses.

Author: achingbrain

packages/libp2p/src/identify/identify.ts

@@ -23,7 +23,7 @@ import type { Libp2pEvents, IdentifyResult, SignedPeerRecord, AbortOptions } fro
 import type { Connection, Stream } from '@libp2p/interface/connection'
 import type { TypedEventTarget } from '@libp2p/interface/events'
 import type { PeerId } from '@libp2p/interface/peer-id'
-import type { Peer, PeerStore } from '@libp2p/interface/peer-store'
+import type { Peer, PeerData, PeerStore } from '@libp2p/interface/peer-store'
 import type { Startable } from '@libp2p/interface/startable'
 import type { AddressManager } from '@libp2p/interface-internal/address-manager'
 import type { ConnectionManager } from '@libp2p/interface-internal/connection-manager'
@@ -404,17 +404,30 @@ export class DefaultIdentifyService implements Startable, IdentifyService {
     log('received identify from %p', connection.remotePeer)
 
     if (message == null) {
-      throw new Error('Message was null or undefined')
+      throw new CodeError('message was null or undefined', 'ERR_INVALID_MESSAGE')
     }
 
-    const peer = {
-      addresses: message.listenAddrs.map(buf => ({
+    const peer: PeerData = {}
+
+    if (message.listenAddrs.length > 0) {
+      peer.addresses = message.listenAddrs.map(buf => ({
         isCertified: false,
         multiaddr: multiaddr(buf)
-      })),
-      protocols: message.protocols,
-      metadata: new Map(),
-      peerRecordEnvelope: message.signedPeerRecord
+      }))
+    }
+
+    if (message.protocols.length > 0) {
+      peer.protocols = message.protocols
+    }
+
+    if (message.publicKey != null) {
+      peer.publicKey = message.publicKey
+
+      const peerId = await peerIdFromKeys(message.publicKey)
+
+      if (!peerId.equals(connection.remotePeer)) {
+        throw new CodeError('public key did not match remote PeerId', 'ERR_INVALID_PUBLIC_KEY')
+      }
     }
 
     let output: SignedPeerRecord | undefined
@@ -429,12 +442,12 @@ export class DefaultIdentifyService implements Startable, IdentifyService {
 
       // Verify peerId
       if (!peerRecord.peerId.equals(envelope.peerId)) {
-        throw new Error('signing key does not match PeerId in the PeerRecord')
+        throw new CodeError('signing key does not match PeerId in the PeerRecord', 'ERR_INVALID_SIGNING_KEY')
       }
 
       // Make sure remote peer is the one sending the record
       if (!connection.remotePeer.equals(peerRecord.peerId)) {
-        throw new Error('signing key does not match remote PeerId')
+        throw new CodeError('signing key does not match remote PeerId', 'ERR_INVALID_PEER_RECORD_KEY')
       }
 
       let existingPeer: Peer | undefined
@@ -482,15 +495,25 @@ export class DefaultIdentifyService implements Startable, IdentifyService {
       log('%p did not send a signed peer record', connection.remotePeer)
     }
 
-    if (message.agentVersion != null) {
-      peer.metadata.set('AgentVersion', uint8ArrayFromString(message.agentVersion))
-    }
+    log('patching %p with', peer)
+    await this.peerStore.patch(connection.remotePeer, peer)
 
-    if (message.protocolVersion != null) {
-      peer.metadata.set('ProtocolVersion', uint8ArrayFromString(message.protocolVersion))
-    }
+    if (message.agentVersion != null || message.protocolVersion != null) {
+      const metadata: Record<string, Uint8Array> = {}
 
-    await this.peerStore.patch(connection.remotePeer, peer)
+      if (message.agentVersion != null) {
+        metadata.AgentVersion = uint8ArrayFromString(message.agentVersion)
+      }
+
+      if (message.protocolVersion != null) {
+        metadata.ProtocolVersion = uint8ArrayFromString(message.protocolVersion)
+      }
+
+      log('updating %p metadata', peer)
+      await this.peerStore.merge(connection.remotePeer, {
+        metadata
+      })
+    }
 
     const result: IdentifyResult = {
       peerId: connection.remotePeer,

packages/libp2p/test/identify/index.spec.ts

@@ -3,7 +3,7 @@
 
 import { TypedEventEmitter } from '@libp2p/interface/events'
 import { start, stop } from '@libp2p/interface/startable'
-import { mockConnectionGater, mockRegistrar, mockUpgrader, connectionPair } from '@libp2p/interface-compliance-tests/mocks'
+import { mockConnectionGater, mockRegistrar, mockUpgrader, connectionPair, mockStream } from '@libp2p/interface-compliance-tests/mocks'
 import { createEd25519PeerId } from '@libp2p/peer-id-factory'
 import { PeerRecord, RecordEnvelope } from '@libp2p/peer-record'
 import { PersistentPeerStore } from '@libp2p/peer-store'
@@ -15,6 +15,7 @@ import drain from 'it-drain'
 import * as lp from 'it-length-prefixed'
 import { pipe } from 'it-pipe'
 import { pbStream } from 'it-protobuf-stream'
+import { pushable } from 'it-pushable'
 import pDefer from 'p-defer'
 import sinon from 'sinon'
 import { stubInterface } from 'sinon-ts'
@@ -31,8 +32,11 @@ import { DefaultIdentifyService } from '../../src/identify/identify.js'
 import { identifyService, type IdentifyServiceInit, Message } from '../../src/identify/index.js'
 import { Identify } from '../../src/identify/pb/message.js'
 import { DefaultTransportManager } from '../../src/transport-manager.js'
+import type { Connection } from '@libp2p/interface/connection'
+import type { Peer } from '@libp2p/interface/peer-store'
 import type { IncomingStreamData } from '@libp2p/interface-internal/registrar'
 import type { TransportManager } from '@libp2p/interface-internal/transport-manager'
+import type { Duplex, Source } from 'it-stream-types'
 
 const listenMaddrs = [multiaddr('/ip4/127.0.0.1/tcp/15002/ws')]
 
@@ -504,4 +508,77 @@ describe('identify', () => {
     }])
     expect(peer.id.publicKey).to.equalBytes(remoteComponents.peerId.publicKey)
   })
+
+  it('should not overwrite peer data when fields are omitted', async () => {
+    const localIdentify = new DefaultIdentifyService(localComponents, defaultInit)
+    await start(localIdentify)
+
+    const peer: Peer = {
+      id: remoteComponents.peerId,
+      addresses: [{
+        multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4001'),
+        isCertified: true
+      }],
+      protocols: [
+        '/proto/1'
+      ],
+      metadata: new Map([['key', uint8ArrayFromString('value')]]),
+      tags: new Map([['key', { value: 1 }]])
+    }
+
+    await localComponents.peerStore.save(remoteComponents.peerId, peer)
+
+    const duplex: Duplex<any, Source<any>, any> = {
+      source: pushable(),
+      sink: async (source) => {
+        await drain(source)
+      }
+    }
+
+    duplex.source.push(lp.encode.single(Identify.encode({
+      protocols: [
+        '/proto/2'
+      ]
+    })))
+
+    await localIdentify._handlePush({
+      stream: mockStream(duplex),
+      connection: stubInterface<Connection>({
+        remotePeer: remoteComponents.peerId
+      })
+    })
+
+    const updatedPeerData = await localComponents.peerStore.get(remoteComponents.peerId)
+    expect(updatedPeerData.addresses[0].multiaddr.toString()).to.equal('/ip4/127.0.0.1/tcp/4001')
+    expect(updatedPeerData.protocols).to.deep.equal(['/proto/2'])
+    expect(updatedPeerData.metadata.get('key')).to.equalBytes(uint8ArrayFromString('value'))
+    expect(updatedPeerData.tags.get('key')).to.deep.equal({ value: 1 })
+  })
+
+  it('should reject incorrect public key', async () => {
+    const localIdentify = new DefaultIdentifyService(localComponents, defaultInit)
+    await start(localIdentify)
+
+    const duplex: Duplex<any, Source<any>, any> = {
+      source: pushable(),
+      sink: async (source) => {
+        await drain(source)
+      }
+    }
+
+    duplex.source.push(lp.encode.single(Identify.encode({
+      publicKey: Uint8Array.from([0, 1, 2, 3, 4])
+    })))
+
+    const localPeerStorePatchSpy = sinon.spy(localComponents.peerStore, 'patch')
+
+    await localIdentify._handlePush({
+      stream: mockStream(duplex),
+      connection: stubInterface<Connection>({
+        remotePeer: remoteComponents.peerId
+      })
+    })
+
+    expect(localPeerStorePatchSpy.called).to.be.false('patch was called when public key was invalid')
+  })
 })