fix: reject stale signed peer records using RFC 0003 sequence numbers - #280
Merged
Conversation
A valid envelope signature proves only that the peer authored the record at some point, not that it is current. Identify accepted any record that verified, so a correctly signed but older envelope replayed at a node rolled its certified addresses back to stale state — exactly what signing those addresses was meant to prevent. This was unfinished scope from #230. Add a certified-record store to the Switch keyed by PeerId, holding the greatest accepted seq alongside the envelope it arrived in, and gate incoming records on it: a first record is accepted, and after that only a strictly greater seq is. Note that go-libp2p is more permissive, rejecting only lastState.Seq > rec.Seq so that an equal seq refreshes its address TTL; this implementation has no address TTL for such a refresh to renew, so it follows RFC 0003's "greater than" wording instead. The check needs the peer store, so it runs in storeIdentify rather than in the pure validation chain, which keeps requestIdentify's contract intact. Because validateSignedPeerRecord has already applied the record's addresses by then, a refused record has that undone: the envelope and address list are cleared from the update, leaving mergeIdentify holding the certified values already known. The envelope is opened outside the transaction so an STM retry cannot re-run signature verification. Both intake paths — Identify on connect and Identify Push — go through storeIdentify, so both are covered. The reusable entry points live in LibP2P.Switch.CertifiedRecords so GossipSub peer exchange can apply the same rule (#247). Closes #248
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #248. First of the #248 → #247 → #259 chain.
Problem
openPeerRecordEnvelopeverifies an envelope's signature, payload type and signer/subject binding, andvalidateSignedPeerRecordthen replacesidListenAddrswith the record's addresses. Nothing readPeerRecord.seq.A signature proves the peer authored the record at some point, not that it is current. Replaying an older, correctly signed envelope therefore rolled a peer's certified addresses back to stale state — the thing signing them was supposed to prevent. Unfinished scope from #230.
Fix
New
LibP2P.Switch.CertifiedRecords:CertifiedRecord { crSeq, crEnvelope, crAddresses }— the accepted record plus the envelope it arrived in, kept verbatim since a record is only self-certifying while its signature travels with it.verifyPeerRecord :: PeerId -> ByteString -> Either String CertifiedRecord— signature, payload type and signer/subject binding. Now the single source of truth for that check;validateSignedPeerRecordcalls it.consumeCertifiedRecord :: TVar (Map PeerId CertifiedRecord) -> PeerId -> CertifiedRecord -> STM Bool— the RFC 0003 rule.lookupCertifiedRecord.SwitchgainsswCertifiedRecords :: TVar (Map PeerId CertifiedRecord).The check needs the peer store, so it runs in
storeIdentifyrather than in the pure validation chain — that keepsrequestIdentify's existing contract (it still prefers verified record addresses in its return value).validateSignedPeerRecordhas already applied the record's addresses by then, so a refused record has that undone: the envelope and address list are cleared from the update, which leavesmergeIdentifyholding the certified values already known. A replay therefore changes nothing.storeIdentifymoves fromSTM ()toIO ()so the envelope is opened once outside the transaction — an STM retry must not re-run Ed25519 verification.Both intake paths (
identifyPeeron connect,handleIdentifyPush) go throughstoreIdentify, so both are covered.Equal sequence numbers: spec over go-libp2p
RFC 0003 says a receiver "MUST ... reject incoming records unless they contain a greater
seqvalue than the last received".go-libp2p's
pstorememis more permissive — it rejects onlylastState.Seq > rec.Seq, accepting an equalseqas an address-TTL refresh. This implementation has no address TTL for such a refresh to renew, so accepting an equalseqwould only widen the replay window for no benefit. This PR follows the RFC's wording; the divergence is documented onconsumeCertifiedRecord.Also checked while there: go's addr book keeps unsigned addresses even when a signed record exists ("Unsigned addrs ... are not touched"), so this PR deliberately leaves the unsigned
idListenAddrspath alone.Tests
13 new examples, verified to fail without the fix. With
consumeCertifiedRecordstubbed to always accept, 4 of the 5IdentifySpeccases and 4 of theCertifiedRecordsSpecsequence cases fail; the two that still pass are "first record accepted" and "greater seq accepted", which should pass either way.test/LibP2P/Switch/CertifiedRecordsSpec.hs—verifyPeerRecordaccept/reject, and first / greater / equal / lower / per-peer sequence handling.test/LibP2P/Protocol/Identify/IdentifySpec.hs— the push path end to end: greater seq accepted, equal and lower ignored, the newer envelope retained across a stale push, and a stale record not erasing what was learned in between.cabal test: 1222 examples, 0 failures (was 1209).Next in the chain
#247 wires accepted PX records to bounded peer dialing and will call
verifyPeerRecord/consumeCertifiedRecordfor the same freshness guarantee on the GossipSub path. Per the repo's no-stacked-PRs rule, that branch will be cut frommainafter this merges.