From 025c082a4d3d08904f1f5b0209ed6f40648fb78d Mon Sep 17 00:00:00 2001 From: Alex Potsides Date: Fri, 3 Nov 2023 12:01:15 +0000 Subject: [PATCH] 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. --- packages/libp2p/src/identify/identify.ts | 57 ++++++++++----- packages/libp2p/test/identify/index.spec.ts | 79 ++++++++++++++++++++- 2 files changed, 118 insertions(+), 18 deletions(-) diff --git a/packages/libp2p/src/identify/identify.ts b/packages/libp2p/src/identify/identify.ts index 906e5fddb8..ea5fbcf796 100644 --- a/packages/libp2p/src/identify/identify.ts +++ b/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 = {} - 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, diff --git a/packages/libp2p/test/identify/index.spec.ts b/packages/libp2p/test/identify/index.spec.ts index 63134a7d0c..ee64e175bd 100644 --- a/packages/libp2p/test/identify/index.spec.ts +++ b/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: 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({ + 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: 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({ + remotePeer: remoteComponents.peerId + }) + }) + + expect(localPeerStorePatchSpy.called).to.be.false('patch was called when public key was invalid') + }) })