fix(node): bind a peer row to its DID so only its keyholder can repoint it (#273) - #290
Open
beardthelion wants to merge 4 commits into
Open
fix(node): bind a peer row to its DID so only its keyholder can repoint it (#273)#290beardthelion wants to merge 4 commits into
beardthelion wants to merge 4 commits into
Conversation
added 4 commits
July 30, 2026 11:17
upsert_peer ended in an unconditional ON CONFLICT DO UPDATE SET http_url,
so any unauthenticated caller could repoint any peer. A repointed row
steers the sync worker's git remote, the post-receive notify fan-out,
trigger_sync, and the public GET /api/v1/resolve/{did}, none of which
consult the reachability flag that #270's stopgap resets.
An unsigned write may now insert an unseen DID but may never change an
existing row's http_url; changing one requires a verified signature from
that row's own DID. The gate is at the db boundary rather than in the
announce handler because there is a second writer: the bootstrap
announce-back in main.rs reads a peer's own JSON response body with no
proof of anything, and now declares itself unproven, which makes it
insert-only by construction.
The proven variant carries the DID it proves and upsert_peer compares it
against the row being written. A bare proven/unproven flag would say that
something was proven without saying which DID, leaving the real check in
the handler and reproducing RUSTSEC-2022-0009, where a valid signature was
accepted without checking it derived the claimed identity.
An unproven insert is restricted to did:key. Only did:key can ever
authenticate here, so a did:web or did:gitlawb row created unsigned would
become unwritable by anyone once this rule lands.
The unproven path derives its refusal from rows_affected, because an
ON CONFLICT DO UPDATE with a guarding WHERE reports zero rows rather than
raising, and a silent no-op is the fail-open this gate exists to prevent.
The refusal is a typed error the handler maps to 403, downcast out of the
anyhow chain the way sqlx errors already are, so it does not render as a
500.
Observed at c83cbc5 before the fix: an unsigned announce naming an
existing peer returned 200 OK and left the attacker's URL stored. That
test is the RED this change turns green.
Two of #270's reachability tests now drive the proven path. Under this
rule an unproven repoint is refused, so they can no longer express a URL
change through it; they assert the same behavior as before.
Drives the must-not set through the production router with real RFC 9421 signatures rather than injected extensions, so the signed path is exercised as a caller would reach it, in both require_signed_peer_writes modes. Covers the allowed cases and every rejection: unsigned insert of an unseen DID, unsigned repoint refused with the row byte-identical after, a signature from the row's own DID accepted, a signature from a different DID refused, an identical-URL re-announce refreshing only last_seen, and a did:web announce refused with no row created. The signature-from-a-different-DID case is the RUSTSEC-2022-0009 shape, where libp2p-core accepted a valid signature without checking it derived the claimed identity. It asserts the key is bound to the row rather than to anything the request carries. The announce handler's keyid-mismatch branch had no test at all and returned 400. It now returns 403 and is tested: a caller denied for want of proof of control over the targeted row gets the same response class whether or not it presented a signature. The completeness ledger derives its set from every writer of the peers table rather than from upsert_peer's callers, because a caller scan cannot see a future writer that issues its own SQL. A second table records the call-site authority for main.rs's bootstrap announce-back, which issues no SQL and so can never appear in the writer ledger; its authority choice has no runtime test in this change and is recorded as such. The type system carries the guard: the authority parameter has no default, so a new caller cannot omit it. The scan is the backstop for a raw statement that bypasses upsert_peer entirely, and stays minimal rather than copying the handler-directory apparatus, which does not apply to a scan over SQL literals.
… decode The gate restricting unproven inserts to did:key checked the method label only. Did::from_str validates that the method is one of key, web, or gitlawb and never inspects the key material, so did:key:notarealkey passed and created exactly the row nobody can ever correct that the gate exists to prevent. It now resolves the key the way the signature path does. Six test fixtures using invented did:key strings stopped inserting once the gate became real. That failure is the evidence it now reads key material rather than a prefix, and it means those fixtures were exercising a path no real DID could take. They carry derivable values now. Resolving the key put a quadratic base58 decode on a route that accepts anonymous callers by default. Measured through the router, a 64,000 character did:key cost 8.05s against 4.6ms for the previous label check, on a tokio worker with no spawn_blocking, and a 1.5 MB body reaches the handler. The bound sits in to_verifying_key so it also covers the HTTP-signature keyid, which reads an untrusted string through the same function. An ed25519 method-id is a fixed 48 characters, so 64 is slack. Its test asserts the error names the length rather than merely being an error, because an oversized id fails either way and a check for is_err() would pass with the guard removed, having already paid the cost. Three last_ping_ok assertions were vacuous: the seed helper never granted reachability, so they compared false against false and would have held against code that never cleared the flag. The helper grants it now and asserts the seed took. Nothing was hiding behind them.
…hat did not happen The announce route now answers 403 when an unproven caller tries to repoint an existing peer. gl peer add discarded the local node's reply with let _ and printed the success line unconditionally, so a refused add reported as a completed one. The refusal is split into a small function so the path is assertable, and it is tested for a 403 with a message, a refusal with no body, and the accepted case. The local add stays best effort: a transport failure warns rather than failing the command, which is what the surrounding code already assumed. The README described the staged rollout as accepting unsigned peers on these routes, which is no longer the whole rule. With signed peer writes off, an unsigned announce may still register an unseen peer and refresh a row whose URL is unchanged, but changing an existing peer's http_url needs a signature from that peer's own DID, and only a did:key can be registered unsigned.
Contributor
📝 WalkthroughWalkthroughChangesPeer write authority
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Peer as Peer client
participant Announce as announce
participant DB as Db::upsert_peer
participant Peers as peers table
Peer->>Announce: POST peer announcement
Announce->>Announce: derive proven or unproven authority
Announce->>DB: upsert_peer(did, http_url, authority)
DB->>Peers: apply authority-specific write
Peers-->>DB: success or denial
DB-->>Announce: result
Announce-->>Peer: HTTP success or mapped error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
6613-6650: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe ledger scan keys on line-level text, so formatting changes can flip it.
foundcounts needle hits per source line, andLEDGERpins exact counts. Splitting a SQL literal across lines, or a comment/doc string that happens to containUPDATE peers, changes the count without changing behavior. Given the doc comment already frames this as a deliberate backstop, consider matching on the statement rather than the line (e.g. strip comments and normalize whitespace before scanning) so the failure mode is "a new writer appeared", not "someone reflowed a query".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/db/mod.rs` around lines 6613 - 6650, Update every_peers_table_write_is_dispositioned to scan normalized SQL statements rather than raw source lines: strip comments and normalize whitespace before matching the needles, so formatting or incidental comment text does not alter counts. Preserve the existing LEDGER comparison and anti-vacuity checks while ensuring real peers-table writers remain dispositioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/peers.rs`:
- Around line 216-236: The auth keyid check in the proven_did flow must compare
normalized DID values rather than raw strings. Reuse the shared DID
normalization/comparison approach established for the boundary check in the
database layer, and update the auth.0 versus announced_did comparison while
preserving the existing Forbidden response for mismatches.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 6613-6650: Update every_peers_table_write_is_dispositioned to scan
normalized SQL statements rather than raw source lines: strip comments and
normalize whitespace before matching the needles, so formatting or incidental
comment text does not alter counts. Preserve the existing LEDGER comparison and
anti-vacuity checks while ensuring real peers-table writers remain
dispositioned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3028af9-ea8e-4e28-9c3d-40f79e6aa237
📒 Files selected for processing (6)
README.mdcrates/gitlawb-core/src/did.rscrates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gl/src/peer.rs
Comment on lines
+216
to
+236
| let proven_did: Option<String> = if let Some(Extension(auth)) = auth { | ||
| // 403, not 400: this caller proved control of some DID and spent that | ||
| // proof on a different DID's row, which is a refusal of authority over | ||
| // the targeted row: the same denial as an unsigned repoint, and two | ||
| // callers denied for the same reason must not get different response | ||
| // classes. The 400 class stays for input-form failures: a malformed | ||
| // DID, a non-public URL, a self-announce, and a DID method that can | ||
| // never authenticate. | ||
| if auth.0 != announced_did.to_string() { | ||
| return Err(AppError::BadRequest( | ||
| return Err(AppError::Forbidden( | ||
| "Signature keyid must match announced DID".into(), | ||
| )); | ||
| } | ||
| Some(auth.0) | ||
| } else { | ||
| tracing::warn!( | ||
| did = %announced_did, | ||
| "accepted unsigned peer announce; set GITLAWB_REQUIRE_SIGNED_PEER_WRITES=true after all peers upgrade" | ||
| ); | ||
| } | ||
| None | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keyid-vs-body DID comparison is raw string equality.
auth.0 != announced_did.to_string() is the same unnormalized DID comparison as the boundary check in crates/gitlawb-node/src/db/mod.rs Line 2214; see the consolidated note for the shared fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gitlawb-node/src/api/peers.rs` around lines 216 - 236, The auth keyid
check in the proven_did flow must compare normalized DID values rather than raw
strings. Reuse the shared DID normalization/comparison approach established for
the boundary check in the database layer, and update the auth.0 versus
announced_did comparison while preserving the existing Forbidden response for
mismatches.
Collaborator
Author
|
On the red check: #292 bumps it. I have verified that audit exits 0 on that branch; I have not re-run this PR's CI against a main containing the bump, so treat the green here as expected rather than demonstrated. Every other check passes. |
jatmn
requested changes
Jul 30, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
The peer-authority gate looks correct for #273. Unsigned repoint is blocked, proven writes are bound to the row DID, and the CLI no longer reports success over a refused local add. I only have two small follow-ups.
Findings
-
[P3]
gl peer addrefusal test does not match production error text
crates/gl/src/peer.rs:255-266
a_refused_local_add_warns_with_the_node_reasonfeeds a fictional message ("peer http_url change requires a signature from that peer") intolocal_add_refusal, but the node returnsunproven announce cannot change an existing peer's http_url: {did}fromPeerWriteDenied::UnprovenRepoint. The test only exercises the helper with mock JSON, so it would not catch a regression in the real refusal body. Please drive it with the production message (or an integration fixture from the handler). -
[P3] Oversized
did:keyon the unproven path is reported asmethodNotSupported
crates/gitlawb-node/src/db/mod.rs:2235-2244,crates/gitlawb-core/src/did.rs:89-93
Whento_verifying_key()rejects an oversize method-id ("did:key method-specific id too long"), the unproven gate collapses the failure intoPeerWriteDenied::UnsupportedDidMethod, whichpeer_write_errormaps to HTTP 400 with amethodNotSupportedmessage. The request is still rejected, but clients cannot distinguish an oversize or invaliddid:keyfrom an unsupported DID method, unlike the signed path which surfacesunresolvable_didwith the underlying reason. Please propagate the length/invalid-key error distinctly on the unproven announce path.
Notes (not blocking this PR)
cargo auditfails on RUSTSEC-2026-0220 inruint, inherited frommain; this diff does not touch manifests or the lockfile. Merge is blocked until that advisory is cleared on the integration branch (e.g. #292).- Bootstrap gossip announce-back is insert-only by design, so existing bootstrap rows no longer pick up URL changes through that path. That is the documented tradeoff of the unproven writer, not a gate bug.
- U1 (
unsigned_announce_cannot_repoint_an_existing_peer) omits a 403 assertion on purpose; U4 and U5 already cover the response class.
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
2 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.
upsert_peerended in an unconditionalON CONFLICT DO UPDATE SET http_url, so any unauthenticated caller could repoint any peer. A repointed row steers the sync worker's git remote, the post-receive notify fan-out,trigger_sync, and the publicGET /api/v1/resolve/{did}, none of which consult the reachability flag that #270's stopgap resets.An unsigned write may now insert an unseen DID but may never change an existing row's
http_url. Changing one requires a verified signature from that row's own DID.The gate sits at the db boundary, not in the announce handler, because there is a second writer. The bootstrap announce-back in
main.rsreads a peer's own JSON response with no proof of anything. It now declares itself unproven, which makes it insert-only by construction rather than by remembering.The proven variant carries the DID it proves, and
upsert_peercompares it against the row being written. A bare proven/unproven flag would say that something was proven without saying which DID, leaving the real check in the handler. That is the RUSTSEC-2022-0009 shape, wherelibp2p-coreaccepted a valid signature without checking it derived the claimed identity.#273 proposed recording the announcing key in a new column, which turns out to be unnecessary. Only
did:keycan authenticate here (auth/mod.rsresolves the verifying key from the keyid), and adid:keyis the ed25519 public key, sopeers.didalready is the first-seen key. No migration.An unproven insert is therefore restricted to keys that actually resolve. Checking the method label was not enough:
did:key:notarealkeypasses a label check and creates a row nobody can ever correct. Resolving the key put a quadratic base58 decode on an anonymous route, so there is a length bound ahead of it, which also covers the signature keyid path since both reach the same function.gl peer adddiscarded the local node's reply and printed success unconditionally, so the new refusal would have been reported as a completed add. It now surfaces the node's message.What this does not close
Squatting. An attacker who announces an unseen DID before its owner is indistinguishable from that owner, which is the standard trust-on-first-use residual (RFC 4251 section 4.1, RFC 7435 section 3). The rule stops them holding the row, since only the real keyholder can produce the signature, but the window is real: peer DIDs are enumerable through the unauthenticated peer list, and the announce is one-shot at boot. A repair path is a follow-up rather than part of this change.
Verification
The defect was re-observed at the merge base before any fix was written: an unsigned announce naming an existing peer returned 200 OK and left the attacker's URL stored.
Eight guard mutations were run, each turning a specific named test red on its expected message. Three of those runs are worth noting because they were not clean the first time: one scored INCONCLUSIVE because the injected mutation did not compile, and one scored RED-WRONG-REASON because the test reddened on a different assertion than expected. Both would have read as passing without checking the failure text.
One call site has no runtime test here. The bootstrap announce-back is held by the compile-enforced parameter and the writer ledger, and is recorded as reasoned-not-run rather than counted as covered.
Known gaps
seqreplay guard. Deferred to the HTTP-signature nonce ledger, which is off by default, so this gap is open today.did:keycannot rotate, so building one would bypass the gate: a peer that loses its key announces under a new DID.require_signed_peer_writesis not flipped.Closes #273.
Summary by CodeRabbit
Security
did:keyidentifiers.Bug Fixes
User Experience
peer addnow reports local peer-list update refusals with the status and reason, and confirms successful updates.Documentation