From 142ba4fcf72fd109331eb253a4e5359eaebfed7a Mon Sep 17 00:00:00 2001 From: Alex Potsides Date: Mon, 5 Jun 2023 17:51:28 +0100 Subject: [PATCH] fix: store unsigned identify data when signed peer record is missing (#1790) --- src/identify/identify.ts | 117 +++++++++++++++++++++--------------- test/identify/index.spec.ts | 103 +++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 49 deletions(-) diff --git a/src/identify/identify.ts b/src/identify/identify.ts index ad53d5f275..ea8e70a890 100644 --- a/src/identify/identify.ts +++ b/src/identify/identify.ts @@ -429,79 +429,98 @@ export class DefaultIdentifyService implements Startable { } async #consumeIdentifyMessage (remotePeer: PeerId, message: Identify): Promise { + log('received identify from %p', remotePeer) + if (message == null) { throw new Error('Message was null or undefined') } - log('received identify from %p', remotePeer) - - if (message.signedPeerRecord == null) { - return + const peer = { + addresses: message.listenAddrs.map(buf => ({ + isCertified: false, + multiaddr: multiaddr(buf) + })), + protocols: message.protocols, + metadata: new Map(), + peerRecordEnvelope: message.signedPeerRecord } - let peerRecordEnvelope = message.signedPeerRecord - const envelope = await RecordEnvelope.openAndCertify(peerRecordEnvelope, PeerRecord.DOMAIN) - let peerRecord = PeerRecord.createFromProtobuf(envelope.payload) + let output: SignedPeerRecord | undefined - // Verify peerId - if (!peerRecord.peerId.equals(envelope.peerId)) { - throw new Error('signing key does not match PeerId in the PeerRecord') - } + // if the peer record has been sent, prefer the addresses in the record as they are signed by the remote peer + if (message.signedPeerRecord != null) { + log('received signedPeerRecord in push from %p', remotePeer) - // Make sure remote peer is the one sending the record - if (!remotePeer.equals(peerRecord.peerId)) { - throw new Error('signing key does not match remote PeerId') - } + let peerRecordEnvelope = message.signedPeerRecord + const envelope = await RecordEnvelope.openAndCertify(peerRecordEnvelope, PeerRecord.DOMAIN) + let peerRecord = PeerRecord.createFromProtobuf(envelope.payload) - let peer: Peer | undefined + // Verify peerId + if (!peerRecord.peerId.equals(envelope.peerId)) { + throw new Error('signing key does not match PeerId in the PeerRecord') + } - try { - peer = await this.peerStore.get(peerRecord.peerId) - } catch (err: any) { - if (err.code !== 'ERR_NOT_FOUND') { - throw err + // Make sure remote peer is the one sending the record + if (!remotePeer.equals(peerRecord.peerId)) { + throw new Error('signing key does not match remote PeerId') + } + + let existingPeer: Peer | undefined + + try { + existingPeer = await this.peerStore.get(peerRecord.peerId) + } catch (err: any) { + if (err.code !== 'ERR_NOT_FOUND') { + throw err + } } - } - log('received signedPeerRecord in push from %p', remotePeer) - const metadata = peer?.metadata ?? new Map() + if (existingPeer != null) { + // don't lose any existing metadata + peer.metadata = existingPeer.metadata - if (peer?.peerRecordEnvelope != null) { - const storedEnvelope = await RecordEnvelope.createFromProtobuf(peer.peerRecordEnvelope) - const storedRecord = PeerRecord.createFromProtobuf(storedEnvelope.payload) + // if we have previously received a signed record for this peer, compare it to the incoming one + if (existingPeer.peerRecordEnvelope != null) { + const storedEnvelope = await RecordEnvelope.createFromProtobuf(existingPeer.peerRecordEnvelope) + const storedRecord = PeerRecord.createFromProtobuf(storedEnvelope.payload) - // ensure seq is greater than, or equal to, the last received - if (storedRecord.seqNumber >= peerRecord.seqNumber) { - log('sequence number was lower or equal to existing sequence number - stored: %d received: %d', storedRecord.seqNumber, peerRecord.seqNumber) - peerRecord = storedRecord - peerRecordEnvelope = peer.peerRecordEnvelope + // ensure seq is greater than, or equal to, the last received + if (storedRecord.seqNumber >= peerRecord.seqNumber) { + log('sequence number was lower or equal to existing sequence number - stored: %d received: %d', storedRecord.seqNumber, peerRecord.seqNumber) + peerRecord = storedRecord + peerRecordEnvelope = existingPeer.peerRecordEnvelope + } + } } + + // store the signed record for next time + peer.peerRecordEnvelope = peerRecordEnvelope + + // override the stored addresses with the signed multiaddrs + peer.addresses = peerRecord.multiaddrs.map(multiaddr => ({ + isCertified: true, + multiaddr + })) + + output = { + seq: peerRecord.seqNumber, + addresses: peerRecord.multiaddrs + } + } else { + log('%p did not send a signed peer record', remotePeer) } if (message.agentVersion != null) { - metadata.set('AgentVersion', uint8ArrayFromString(message.agentVersion)) + peer.metadata.set('AgentVersion', uint8ArrayFromString(message.agentVersion)) } if (message.protocolVersion != null) { - metadata.set('ProtocolVersion', uint8ArrayFromString(message.protocolVersion)) + peer.metadata.set('ProtocolVersion', uint8ArrayFromString(message.protocolVersion)) } - await this.peerStore.patch(peerRecord.peerId, { - peerRecordEnvelope, - protocols: message.protocols, - addresses: peerRecord.multiaddrs.map(multiaddr => ({ - isCertified: true, - multiaddr - })), - metadata - }) - - log('consumed signedPeerRecord sent in push from %p', remotePeer) + await this.peerStore.patch(remotePeer, peer) - return { - seq: peerRecord.seqNumber, - addresses: peerRecord.multiaddrs - } + return output } } diff --git a/test/identify/index.spec.ts b/test/identify/index.spec.ts index 5971247333..67616070df 100644 --- a/test/identify/index.spec.ts +++ b/test/identify/index.spec.ts @@ -13,6 +13,7 @@ import { MemoryDatastore } from 'datastore-core/memory' import delay from 'delay' import drain from 'it-drain' import * as lp from 'it-length-prefixed' +import { pbStream } from 'it-pb-stream' import { pipe } from 'it-pipe' import pDefer from 'p-defer' import sinon from 'sinon' @@ -28,8 +29,10 @@ import { } from '../../src/identify/consts.js' 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 Peers from '../fixtures/peers.js' +import type { IncomingStreamData } from '@libp2p/interface-registrar' import type { TransportManager } from '@libp2p/interface-transport' const listenMaddrs = [multiaddr('/ip4/127.0.0.1/tcp/15002/ws')] @@ -397,4 +400,104 @@ describe('identify', () => { ]) expect(peer).to.have.property('peerRecordEnvelope').that.equalBytes(peerRecordEnvelope) }) + + it('should store data without signed peer record', async () => { + const localIdentify = new DefaultIdentifyService(localComponents, defaultInit) + const remoteIdentify = new DefaultIdentifyService(remoteComponents, defaultInit) + + await start(localIdentify) + await start(remoteIdentify) + + const [localToRemote] = connectionPair(localComponents, remoteComponents) + + const localPeerStorePatchSpy = sinon.spy(localComponents.peerStore, 'patch') + + // should know know remote peer + await expect(localComponents.peerStore.has(remoteComponents.peerId)).to.eventually.be.false() + + const message = { + protocolVersion: 'protocol version', + agentVersion: 'agent version', + listenAddrs: [multiaddr('/ip4/127.0.0.1/tcp/1234').bytes], + protocols: ['protocols'], + publicKey: remoteComponents.peerId.publicKey + } + + remoteIdentify._handleIdentify = async (data: IncomingStreamData): Promise => { + const { stream } = data + const pb = pbStream(stream) + pb.writePB(message, Identify) + } + + // Run identify + await localIdentify.identify(localToRemote) + + expect(localPeerStorePatchSpy.callCount).to.equal(1) + + // should have stored all passed data + const peer = await localComponents.peerStore.get(remoteComponents.peerId) + expect(peer.metadata.get('AgentVersion')).to.equalBytes(uint8ArrayFromString(message.agentVersion)) + expect(peer.metadata.get('ProtocolVersion')).to.equalBytes(uint8ArrayFromString(message.protocolVersion)) + expect(peer.protocols).to.deep.equal(message.protocols) + expect(peer.addresses).to.deep.equal([{ + isCertified: false, + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/1234') + }]) + expect(peer.id.publicKey).to.equalBytes(remoteComponents.peerId.publicKey) + }) + + it('should prefer addresses from signed peer record', async () => { + const localIdentify = new DefaultIdentifyService(localComponents, defaultInit) + const remoteIdentify = new DefaultIdentifyService(remoteComponents, defaultInit) + + await start(localIdentify) + await start(remoteIdentify) + + const [localToRemote] = connectionPair(localComponents, remoteComponents) + + const localPeerStorePatchSpy = sinon.spy(localComponents.peerStore, 'patch') + + // should know know remote peer + await expect(localComponents.peerStore.has(remoteComponents.peerId)).to.eventually.be.false() + + const signedPeerRecord = await RecordEnvelope.seal(new PeerRecord({ + peerId: remoteComponents.peerId, + multiaddrs: [ + multiaddr('/ip4/127.0.0.1/tcp/5678') + ], + seqNumber: BigInt(Date.now() * 2) + }), remoteComponents.peerId) + const peerRecordEnvelope = signedPeerRecord.marshal() + + const message = { + protocolVersion: 'protocol version', + agentVersion: 'agent version', + listenAddrs: [multiaddr('/ip4/127.0.0.1/tcp/1234').bytes], + protocols: ['protocols'], + publicKey: remoteComponents.peerId.publicKey, + signedPeerRecord: peerRecordEnvelope + } + + remoteIdentify._handleIdentify = async (data: IncomingStreamData): Promise => { + const { stream } = data + const pb = pbStream(stream) + pb.writePB(message, Identify) + } + + // Run identify + await localIdentify.identify(localToRemote) + + expect(localPeerStorePatchSpy.callCount).to.equal(1) + + // should have stored all passed data + const peer = await localComponents.peerStore.get(remoteComponents.peerId) + expect(peer.metadata.get('AgentVersion')).to.equalBytes(uint8ArrayFromString(message.agentVersion)) + expect(peer.metadata.get('ProtocolVersion')).to.equalBytes(uint8ArrayFromString(message.protocolVersion)) + expect(peer.protocols).to.deep.equal(message.protocols) + expect(peer.addresses).to.deep.equal([{ + isCertified: true, + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/5678') + }]) + expect(peer.id.publicKey).to.equalBytes(remoteComponents.peerId.publicKey) + }) })