fix(node): require Content-Digest on a signed request - #306
Open
beardthelion wants to merge 2 commits into
Open
Conversation
require_signature rebuilds the RFC 9421 signing string from the request
itself, and it read the Content-Digest header with a .unwrap_or("")
fallback. When the header was absent, the covered content-digest component
came out as the empty string on both the signing side and the verifying
side, so the Ed25519 check passed over a string that committed to nothing
about the body. The comparison that would have caught it was guarded on the
presence of that same absent header, so it was skipped. The two defects
compose: a signed request could carry any body at all, and the handler
behind the middleware saw it as authenticated.
The header is now required. An absent Content-Digest is a 400 with
missing_content_digest, and the digest-versus-body comparison is
unconditional, since presence is established before it runs. There is no
longer a path through the middleware on which the body goes uncompared.
Refusing is safe for every client that already works. sign_request computes
the digest with no branch and always returns it in SignedHeaders, and all
eight production call sites attach it as a Content-Digest header: the three
in gl's HTTP client, the fetch and push requests in git-remote-gitlawb, the
node's peer announce, the sync push in sync.rs, and the sync notify in
api/repos.rs. So no conforming client regresses. What the refusal removes is
the case where a caller signed an empty digest and sent a body no signature
covered.
HttpSignature::parse now also refuses a covered component that appears more
than once, per RFC 9421 section 2.1. A repeated identifier says nothing the
single one did not, and it lengthens the signing string a verifier has to
build from a list the caller chooses.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe change tightens HTTP signature validation. Invalid covered components and duplicates are rejected during parsing. Signed requests without a valid ChangesHTTP signature validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change rejects signed requests without a Content-Digest header and duplicate signature components; no actionable merge-blocking risk remains based on the supplied evidence. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant require_signature
participant RequestBody
participant Handler
Client->>require_signature: Send signed request
require_signature->>RequestBody: Verify Content-Digest
require_signature->>Handler: Forward validated request
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is complete and follows the repository template. It explains the security issue, lists both changes, identifies issue Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes remain focused on HTTP signature validation and the linked security objectives. The additional rejection of empty or parameterized components is related parser validation and does not introduce unrelated functionality. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
11 tasks
![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
🤖 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-core/src/http_sig.rs`:
- Around line 76-86: The HttpSignature::parse component validation must handle
parameterized component identifiers consistently instead of comparing raw text
while later methods only support bare names. Either reject any component
parameters during parsing, or normalize and preserve them through
missing_components() and build_signing_string(); also add coverage proving
reordered equivalent parameters cannot bypass duplicate detection.
🪄 Autofix
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: 617ccfde-796b-48d3-be02-77351de052b9
📒 Files selected for processing (2)
crates/gitlawb-core/src/http_sig.rscrates/gitlawb-node/src/auth/mod.rs
beardthelion
added
crate:core
jatmn
requested changes
Aug 9, 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.
Findings
- [P1] Make duplicate-component validation linear
crates/gitlawb-core/src/http_sig.rs:79
This scans the remaining component list for every component. An unauthenticated caller can send aSignature-Inputwith many distinct quoted components, so the new check performs quadratic string comparisons before timestamp, key, required-component, or signature validation rejects it. This server uses Axum/Hyper's default HTTP/1 configuration and does not set a tighter header buffer limit (Hyper permits roughly 417 KiB by default), enough for tens of thousands of short component tokens and billions of comparisons.optional_signatureroutes invoke this path whenever either signature header is present, including public read routes without an IP limiter, allowing repeated header-only requests to monopolize worker CPU. Track seen components in aHashSet(or enforce a strict component count) and add coverage for a long distinct-component list.
Collaborator
Author
|
Heads-up while this is open, since it touches
This PR's diff does not touch that indexing (it adds |
This was referenced Aug 15, 2026
3 tasks
Use a HashSet for covered-component duplicate detection so hostile Signature-Input headers cannot force quadratic work before rejection. Reject RFC 9421 component parameters during parse since only bare names are supported end to end.
Collaborator
Author
|
Addressed both review items on 2572169. Duplicate-component validation now tracks seen bare names in a HashSet instead of scanning the tail slice per entry. Revert-check: restoring the old loop made Parameterized component identifiers are rejected at parse (
|
Gravirei
added a commit
to Gravirei/node
that referenced
this pull request
Aug 28, 2026
split 1/4) This is the handler-level half of Split PR 1. The previous commit added the migration and the DB methods; this one threads them through crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack), the cert issuer, and the startup drain. CHANGES IN THE HANDLER ====================== In git_receive_pack, AT THE LAST POSSIBLE MOMENT before the smart_http::receive_pack call, the handler now: 1. Generates a per-handler request_id (UUID). 2. Captures the raw Signature, Signature-Input, and Content-Digest headers from the request. 3. Calls db.insert_pending_ref_transitions(request_id, ...) which writes one row per ref update in state 'prepared'. The receive_pack call runs as before. After it returns: 4. On Ok: db.mark_pending_ref_transitions_applied(request_id) — the row is the ONLY thing that promotes a 'prepared' row to 'applied', and the drain reads only 'applied' rows. A process crash before this call leaves the row in 'prepared', which the drain never promotes. 5. On Err: db.mark_pending_ref_transitions_cancelled(request_id) — a failed receive_pack leaves the row in 'cancelled', which the drain never promotes. This is what closes the reviewer's two proofs: Proof 1 (crash window): if the process dies after mark_pending_ref_transitions_applied but before the bookkeeping writes, the row is in 'applied' and the next startup drain re-derives the push event, the per-ref certificate (carrying the ORIGINAL pusher DID, not a placeholder), and the anchor handoff. The drain uses the persisted authentic pusher DID and signature header, not a recovered placeholder. Proof 2 (failed receive-pack): the row is only ever flipped to 'applied' in the explicit Ok branch above. A 'prepared' or 'cancelled' row is invisible to the drain, so a failed or dropped receive_pack cannot turn a prepared intent into completed accounting or anchoring. BOOKKEEPING IS NOW DETERMINISTIC-ID =================================== The post-Ok bookkeeping at api/repos.rs:2448 now uses: - record_push_with_id with push_event_id_for(request_id, first_ref) — ON CONFLICT (id) DO NOTHING, so a recovery re-pass is a no-op. - issue_ref_certificate_idempotent with ref_cert_id_for(request_id, ref_name) — ON CONFLICT (repo_id, ref_name) DO NOTHING, returns None if a live-path cert already exists. - insert_anchor_job_idempotent with anchor_job_id_for(repo_id, ref_name, old_sha, new_sha) — the per-transition tuple key, so two pushes to the same ref produce one anchor upload per landed state. The legacy entry points (record_push, issue_ref_certificate, insert_ref_certificate) remain for callers that prefer a fresh UUID per cert; they are #[allow(dead_code)] for the PR 3 cert/CLI compat pass to decide whether to keep or remove. STARTUP DRAIN ============= crates/gitlawb-node/src/main.rs calls durable_outbox::drain_pending_ref_transitions(state, 1000) ONCE before serving, after migrations and after the existing peer / quarantine prunes. Non-fatal: a transient drain failure logs and leaves the rows for the next startup. durable_outbox::drain_pending_ref_transitions reads every 'applied' row, calls derive_one (which re-derives the three artifacts using the persisted authentic pusher DID and signature header), then deletes the row. A second drain pass is a no-op for both the artifacts (idempotent inserts) and the row (gone after the first pass). NEW END-TO-END TESTS ==================== crates/gitlawb-node/src/durable_outbox.rs adds three end-to-end tests in drain_tests, complementing the eight DB-layer tests in db::pending_ref_transition_tests: - drain_re_derives_all_three_artifacts_for_an_applied_row: the reviewer's first proof. Inserts a row in 'applied' state (the crash window), drains, asserts exactly one push event row, exactly one cert row carrying the original pusher DID (not a placeholder), and exactly one anchor job row. Asserts the deterministic cert id matches. Asserts a second drain pass is a no-op. - cancelled_row_produces_no_artifacts: the reviewer's second proof for the cancelled state. A row in 'cancelled' (receive_pack returned Err) is invisible to the drain. - prepared_row_produces_no_artifacts: the reviewer's second proof for the prepared state. A row in 'prepared' (handler crashed between insert_prepared and the post-Ok branch) is invisible to the drain. Each test names the invariant it pins and the production line it covers. Reverting that line turns the named assertion red. Compiles clean, 1099 tests pass with 0 regressions, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - Gitlawb#134 (anchors auth): composes. The /arweave/anchors route already requires auth; this PR does not change the route. - Gitlawb#285 (advisory-lock session affinity): composes. The durable intent is written inside the same handler that holds the lock from Gitlawb#285; no changes to the lock layer. - Gitlawb#306 (Content-Digest on signed requests): composes. PR 1 persists the Content-Digest header that Gitlawb#306 makes mandatory. - Gitlawb#314 (small-order Ed25519): independent. PR 1's tests use strong keys. - Gitlawb#324 (libp2p keypair persistence): independent. PR 1 does not touch p2p identity. - Gitlawb#325 (gossip ref-update auth): independent. PR 1's signed envelope is the HTTP-side equivalent, not the gossip-side. - Gitlawb#382 (replication withheld-subtree trees): independent. PR 1 does not touch replication or pin selection.
kevincodex1
added a commit
that referenced
this pull request
Aug 31, 2026
A claim is provenance, so the signature stored beside it has to reach the bytes stored beside it. It did not. The signing string covers the body only through a content-digest line, and require_signature rebuilds the covered values from the request: with no Content-Digest header the covered digest is the empty string on the signing side and the verifying side alike, the Ed25519 check passes over method, path and nothing else, and the digest-versus-body comparison was skipped for want of a header. An owner could sign that once and post any body under it. The row that produced carried a valid signature, a signing string that verifies under the owner's key, and a request_body the signing string says nothing about — history nobody can re-verify, which is the one thing the row is for. The write path now recomputes the digest of the bytes it is about to persist and requires the signing string to carry exactly that line, which is the write-time form of the check re_verify already ran against a stored row. Line-exact rather than a substring search: build_signing_string emits one component per line, and a contains would also be satisfied by the digest appearing inside another component's value. This is deliberately not the only line of defence, and deliberately not the middleware's. PR #306 requires the header in require_signature and closes the same hole one layer earlier for every signed route; that belongs there, because no other route's correctness should depend on this handler. Neither substitutes for the other. Without #306 the empty digest is still accepted everywhere else; without this, a later relaxation of the middleware resumes writing unverifiable history in silence. Nothing here touches require_signature, so the two do not collide. The route's advertised 8 KiB body bound was also a bound on what gets stored rather than on what gets buffered: require_signature collects and hashes the whole body before any handler runs, so by the time the handler measured it the allocation had happened, and concurrent requests multiplied exactly the cost the number was written to prevent. A RequestBodyLimitLayer now carries the same figure at the transport, outside the auth layers and inside the rate limiters, matching the shape the git routes already use. The handler keeps its own check: it is the one that holds for a handler reached by any other route table. The hand-injected test material had to become coherent with the request it rides on, because a stand-in built over other bytes now takes every bare-router test down the refusal branch. It is built from the body it accompanies and carries a real content-digest line.
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.
Summary
Refuses a signed request that carries no
Content-Digestheader, and refuses a duplicated covered component inSignature-Input.Motivation & context
Closes #305
The header was read with a fallback to the empty string. With the header absent, the covered
content-digestis empty on both the signing and verifying sides, so verification passes and the body comparison is then skipped for want of a header. A request could carry a body its signature never covered while presenting as validly signed. Every write to a node authenticates this way, so the guarantee a signature is meant to give did not hold for a caller who simply left a header off.The parser change is smaller: RFC 9421 section 2.1 says a component identifier appears at most once, and a repeat conveys nothing while repeating a line in the signing string a verifier has to build.
Kind of change
What changed
gitlawb-node:require_signaturereturns 400 with amissing_content_digestcode when the header is absent, and the body comparison is now unconditional.gitlawb-core:HttpSignature::parserejects a repeated covered component.How a reviewer can verify
The first test signs
content-digestas the empty string and then sends no header, which is the exact shape that slipped through, and asserts the request is refused before any handler runs. Both tests were checked against an injected revert of the production line they cover, so a regression turns them red rather than leaving them green.Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare clean.env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNo conforming client regresses.
sign_requestcomputes the digest unconditionally and returns it inSignedHeaders, and all eight production call sites attach the header:gl/src/http.rs(three),git-remote-gitlawb/src/main.rs(two),gitlawb-node/src/main.rs,sync.rs, andapi/repos.rs. The only paths that omit it are git-remote's unsigned ones, which carry no signature headers at all and are refused earlier. Both changes refuse things a conforming client never sends, so previously signed history is unaffected.Notes for reviewers
This started life inside #304, the commit status surface, because I found it while making stored status claims re-verifiable. It does not belong there: that feature does not need it, this is a protocol change, and the two want different scrutiny. #304 has been rebased to drop it and now depends on nothing here.
Summary by CodeRabbit
Content-Digestheader.