fix(node): bound replay of authenticated gossip ref-update events - #334
Open
beardthelion wants to merge 11 commits into
Open
fix(node): bound replay of authenticated gossip ref-update events#334beardthelion wants to merge 11 commits into
beardthelion wants to merge 11 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
beardthelion
added
crate:core
beardthelion
force-pushed
the
fix/p2p-gossip-ingest-auth
branch
2 times, most recently
from
August 17, 2026 13:14
7ef43fc to
c003f65
Compare
beardthelion
force-pushed
the
fix/gossip-replay-bound
branch
from
August 17, 2026 13:14
aee00b3 to
2349a89
Compare
jatmn
requested changes
Aug 21, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P1] Sanitize every rejected timestamp before logging it
crates/gitlawb-node/src/p2p/mod.rs:1250
DateTime::parse_from_rfc3339accepts RFC 3339 fractional seconds with arbitrarily many digits (discarding precision after nanoseconds). A signed timestamp such as an old instant followed by megabytes of fractional digits therefore parses successfully, reaches theTooOldorTooFarFuturearm, and is copied intoStaleTimestamp; the swarm loop then logs that reason verbatim. Only the parser-error branch callssanitize_for_log.The root cause is treating successful parsing as proof that the original wire representation is safe to expose. The parsed
DateTimeis bounded, but the source string is not. A registered peer can consequently turn bounded gossip deliveries into unbounded log, disk, and log-pipeline traffic despite the per-source event limiter. Apply the same bounded/control-character-safe rendering to all three freshness failure branches, preferably by logging a bounded canonical rendering of the parsed timestamp forTooOldandTooFarFuturerather than the original wire string. Please add a regression test using an overlong but parseable fractional timestamp that exercises both directions and asserts that the emitted detail is bounded and contains no control characters.
jatmn
requested changes
Aug 31, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P1] Rebase onto the current target branch before merge
The PR is currentlyCONFLICTING/DIRTY. Its target (fix/p2p-gossip-ingest-auth) and head each mergedmainindependently and have diverged acrossp2p/mod.rs,metrics.rs, anddb/mod.rs, so this cannot merge as reviewed. Rebase or reconstruct the head on the current target, resolve the conflicts, and request review of the resulting diff.
Builds the two layers that bound replay of an authenticated ref-update, with nothing calling them yet; the ingest path is wired in the next commit. The freshness check is deliberately two-directional rather than an absolute delta: an abs() window admits an event stamped up to the window ahead and pins its seen-set slot until the clock catches up. Producers were enumerated before settling the unparseable arm; the sole production publish site emits RFC-3339, so an unparseable timestamp is refused rather than admitted. The replay guard keys on SHA-256 of the canonical signing bytes, not the raw wire bytes, because one signature verifies against many encodings and only the canonical form collapses them to a single key. A golden digest is frozen for the pre-version artifact, and the same constant is asserted for its v-injected twin, so the collapse is pinned rather than described. Reservations settle through a drop guard so only a confirmed entry outlives the ingest call, which keeps a transient write failure from permanently burning an event's slot. Replayed and StaleTimestamp are separate outcomes because they diagnose different conditions, a mesh replay against a broken clock or a healing partition, and folding them would be the same observability lie the unsigned shed variant already exists to avoid.
Wires the freshness check and the replay guard into ingest, immediately after signature verification and above the author debit. The lower placement, just above the writes, stops the duplicate row and the duplicate sync but still lets every replay drain the victim author's budget, which is the harm the defect names: a captured signature replayed 500 times empties the victim's window and their next genuine push is refused. The guard runs on the verified path only. Unsigned event bytes are predictable, so applying it there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay, which is a censorship primitive rather than a defense. A replay flood still costs a parse and one Ed25519 verify, because the guard has to sit below verification for the reason above. What it removes is the peer_exists round trip, the victim's author debit, the ref-update row and the sync enqueue. The existing author-budget test signed one event and ingested the same bytes five hundred times, so it had to re-sign per iteration to keep exercising the budget under a shared guard. Giving it a fresh guard per call would have kept it green while gutting the property it exists to prove. Its over-budget probe needed the same treatment, since the replay gate sits above the author gate and would have refused the burst's last bytes before the budget assertion ran. Both saturation tests now take a shared lock so each keeps an exact assertion on a process-wide counter; a lower bound would stay green if the fail-open branch ever double-counted.
At capacity the inline sweep ran on every event, so a full O(capacity) retain happened under the lock once per message and reclaimed nothing when nothing had expired. The guard that bounds replay became a CPU amplifier in exactly the state an attacker drives toward. The sweep is now rate limited to once a second; the periodic sweep still reclaims on its own cadence, so only the redundant rescans go away. An unparseable timestamp was echoed into the refusal detail verbatim. That value is attacker-controlled and arbitrary length, so it reached a warn! as both a log-injection and an unbounded-size sink. Only that arm needs sanitizing; the other two ran through the parser first. The capacity rationale cited a count of registered DIDs, which this same file says elsewhere an attacker mints freely through the announce path, so it was not a bound at all. It now cites the bound that is real: reaching saturation costs a hundred thousand durable rows and as many sync enqueues inside one retention horizon, which the database makes loud. ingest_now was read twice per ingest while its own doc comment claimed the two layers share one reading, which is the invariant the retention derivation rests on. Now read once and passed to both. Also: the Unparseable outcome is driven through ingest rather than only as a pure function, the periodic sweep is observable without a live swarm, the restart exposure is written down where the rest of the tradeoffs already are, and the saturation-counter lock covers every test that can reach Saturated rather than the two I first found.
…e sibling window Seven tests for the six gaps a review found. The reservation's release path was reasoned rather than executed: the drop guard was only ever proven through an early refusal, while the case its own doc comment names, a transient write failure burning the event's slot, was never driven. Both write directions now are. An expired entry at capacity must be replaced in place rather than answer Saturated, confirm on an already-swept entry is pinned as a deliberate no-op, and the single-critical-section shape is now driven concurrently rather than asserted sequentially. Restart behavior was documented but untested; a fresh guard readmits a seen event and the freshness window still bounds it, which is the composition that makes the restart exposure finite. check_created in gitlawb-core used a symmetric abs() window, so a request stamped 299 seconds ahead was accepted and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path, 300s late and 60s early, with the error naming the direction so a fast peer and a slow one need different operator action. There was no future-direction test at all; there are now five covering both. Every caller was checked and none depended on the symmetry. Adding a freshness window obliges auditing the siblings, which is how this one surfaced: the repo argued both ways in two files for a week.
…ng the window check created is parsed as an unrestricted i64 straight from the Signature-Input header, so the sender picks it. At i64::MIN the past-side subtraction overflows, which panics a debug build and wraps a release one into a value that can read as inside the window. Confirmed by execution before the fix: 'attempt to subtract with overflow'. Saturating subtraction answers correctly at both ends, since a timestamp that far out is refused by whichever side it saturates toward. A test drives all four extremes of the type and asserts each is refused by a named direction; reverting to plain subtraction reddens it on the overflow. The symmetric abs() form this replaced had the same hazard, so splitting the window into two comparisons did not introduce it, but it did not remove it either. Found by a cross-family review pass after six same-family reviewers and I had all read the line.
…plit Integration only, from rebasing this branch onto #325's current head. #325 now returns IngestOutcome::UnsignedAdmitted where it used to return Accepted for an unsigned rolling-upgrade admission, and this branch predates that split. Three sites. The seen-set bypass test asserted Accepted on both unsigned deliveries and on the stale-timestamp case; both now expect UnsignedAdmitted, and the properties under test are unchanged, that unsigned bytes are admitted twice rather than deduplicated and that the freshness window does not reach them. The warn-only-on-admission test's budget-spent case gained the ReplayGuard argument the signature now takes, with a fresh guard because that case drives an unsigned event the replay block skips. The ingest match also grows an arm rather than changing one: `None if unsigned` returns UnsignedAdmitted without settling a reservation, since the replay block is gated on `verified` and an unsigned admission never holds one.
DateTime::parse_from_rfc3339 accepts arbitrarily many fractional-second digits, so a signed timestamp can parse successfully and still carry megabytes of attacker-chosen wire bytes into the TooOld / TooFarFuture refusal details, which the swarm loop logs verbatim. Only the parser-error branch sanitized. The root cause was treating successful parsing as proof the wire representation is safe to expose. The TooOld and TooFarFuture variants now carry the PARSED instant and the detail renders its canonical to_rfc3339() form - bounded by this build's formatter and control-free by construction - while the unparseable arm keeps sanitize_for_log on the wire string. All three arms go through one freshness_refusal_detail helper so the invariant has a single home, plus a regression test driving an overlong but parseable fractional stamp through both directions.
The two ingest tests added on the target branch after this PR forked (an_over_budget_author_is_shed_without_a_peer_lookup and the_early_author_check_does_not_track_an_unseen_did) still call ingest_ref_update with the pre-guard signature. Thread a fresh ReplayGuard through each call, matching every other test site.
Two real swarms on QUIC loopback with real signatures and real Postgres: a signed ref-update from one node is admitted and stored by the other, a re-delivery of the same signed content under a malleated wire encoding is refused by the replay guard, and an unsigned event is refused under require_signed. Both refusals are asserted in the warn stream, and a control event landing after them proves the mesh still delivers. This closes the seams ref_update_publish_args names as needing a live swarm: the PublishRefUpdate select! arm dispatching to sign+publish, and require_signed reaching the receive path. It needs a test-only command variant, PublishRawRefUpdate, since the production publish path signs and serializes itself and can never put a malleated or unsigned event on the wire. Still uncovered and named in the test's doc comment: the run_ingest_sweep select! arm on its 300-second interval, and the main.rs half of the flag wiring above p2p::start.
…ytes Two findings on the pre-auth reject path: - An unsigned event under require_signed was resolved and slug-validated before the flag refused it, so a hostile flood bought DID resolution and a rejection reason built from attacker-chosen fields under only the loose pre-parse brake. The unsigned refusal now runs immediately after the version gate, before any field is read. - The swarm loop logged IngestOutcome::Rejected and WriteFailed reasons verbatim, and several producers embed wire fields (the claimed node_did in resolve failures, for one). A signed event carrying a control-bearing node_did reached the operator log with escape sequences intact and no length bound. Both warn sites now render through sanitize_for_log, so every present and future Rejected producer is bounded and control-free at the one sink. The live-mesh test now also publishes a signed event with a 4096-char control-bearing node_did and asserts the warn stream shows the refusal with no escape byte and no run past the sanitizer ceiling. A unit test pins the new ordering by asserting the exact fixed refusal for an unsigned event with a hostile node_did.
…om id The reservation releases on a failed write so a republish can repair the half that never landed. But each ingest minted a fresh row id, so a retry after a partial failure (row stored, queue write failed) stored the same signed content a second time. A replay in a degraded state could repeat a landed side effect until a budget refused it. The row id for a verified event is now derived from the replay key, so the retry's insert dedupes on the existing ON CONFLICT instead of duplicating. The queue entry still retries, which is the repair the release exists for. Unsigned events keep a random id: they carry no replay key by design, and deduplicating unauthenticated content would let an attacker pre-send a victim's expected event and have the genuine one dropped. Two existing tests pinned the duplicate write and now assert the deduped shape: the partial-failure republish repairs the queue entry while the row stays at one, and the restart readmission's residual effect is a duplicate queue entry (a refetch), not a second record. The new test asserts both the retry reaches the writes (WriteFailed, not Replayed) and the row count stays at one.
beardthelion
force-pushed
the
fix/gossip-replay-bound
branch
from
September 10, 2026 23:25
314173c to
77b4ebc
Compare
Collaborator
Author
|
Rebased onto the current target. The branch now carries the seven commits replayed on top of Verified locally:
Three further commits on the same surface, each with a regression test:
|
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
3 participants
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.
Bounds replay of authenticated gossip ref-update events. Stacked on #325, which is what makes these events authenticated in the first place.
Why this is separate from #325 rather than folded into it
With #325 merged and this not yet merged, an attacker's capability is strictly lower than it was before #325: forging arbitrary ref-updates for any peer required no key at all, and afterwards they are reduced to replaying events a legitimate node actually published, with the same repo, refs, and shas. The intermediate state is better than the pre-state, not worse, so the split does not leave a window where the system is more exposed than it was.
That is the test I applied, and it is the one the #173/#321 split failed: that resolver hardening withheld the provider CID for unrepaired rows, so deploying it alone actively broke existing pins, and the ordering argument only held as long as nothing shipped in the gap. Writing this out here rather than leaving it in a comment is the other half of that lesson.
What it does
Two layers, following the shape production gossip systems use, where a dedup-layer fix and an authentication-layer fix are paired rather than either being treated as sufficient.
A freshness window on the already-signature-covered
timestamp: 600 seconds into the past, 60 into the future. Two comparisons, never a distance. Anabs()window accepts a future-dated event as readily as a late one, and a future-dated event also pins a seen-set slot while sitting outside the past-window check until the clock catches up.A bounded seen-set keyed on SHA-256 of the canonical signing bytes, not the raw wire bytes. That distinction is the whole point: one signature verifies against a family of wire encodings, so a raw-bytes key deduplicates nothing. The frozen pre-version artifact and the same artifact with
"v":0injected are different lengths, both verify against one signature, and produce identical signing bytes.The guard runs immediately after signature verification and above the per-author debit. Below the debit it would still stop the duplicate row and the duplicate sync while letting every replay drain the victim author's budget, which is the harm this is about. It runs on the verified path only: unsigned bytes are predictable, so applying dedup there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay.
A key is recorded only on acceptance, through a reservation whose drop releases it, so a transient write failure does not permanently burn an event's slot. At capacity the guard fails open and counts the degradation, because a saturated set that dropped all fresh gossip would convert a loud resource attack into quiet mesh-wide censorship.
Also included:
check_createdin gitlawb-core used a symmetricabs()window, so it tolerated 300 seconds of future-dating and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path. Adding a freshness window obliges auditing the siblings, which is how that surfaced.What is proven rather than argued
Every guard here is backed by a mutation that turns a named test red, and they are present-but-wrong re-implementations rather than deletions, because each of these compiles and reads sensibly: keying on raw wire bytes, keying fresh per ingest, keying on
(repo, ref, sha), anabs()freshness window, recording before accept, failing closed at saturation, placing the guard below the author debit, a sweep that stops evicting, confirming on a write failure, and treating an expired entry as absent at capacity.Two are worth calling out because the obvious version of each test cannot catch what it names. The revert case needs three events, since two never collide under a
(repo, ref, sha)key. The layer-composition case has to run at the future-skew edge, since a present-stamped probe is rejected by freshness anyway and stays green under a shortened retention.An attacker-supplied
createdcould overflow the window subtraction and panic a debug build; that is fixed with saturating arithmetic and a test driving all four extremes of the type.Known open, not closed here
A compromised or malicious registered signer can still mint fresh signed events with fresh timestamps, each of which gets a distinct key and passes freshness. This bounds third-party replay of a captured event; it is not an aggregate write-volume defense and should not be described as one.
The seen-set is in-process, so a restart readmits any event still inside its freshness window, once per restart. Bounded by the window rather than unbounded, and documented on the guard.
POST /api/v1/sync/notifyreaches the same two sinks with no dedup. Bounded differently there, since that handler still accepts unsigned notifications naming any known peer DID, so forgery already beats replay and the per-IP limiter is the real bound. Worth closing separately.The freshness window is an availability bound with no resume: a peer returning from a partition longer than the window drains a backlog stamped at push time and every one of those events is dropped, with no republish and no repair path. That is a product decision about what federation should do with stale-but-genuine updates, not something to patch quietly.
Full suite, clippy and fmt clean locally; CI has the authoritative run for this head.