From 9a2eac3b8c37b19393d104e5f29c84aa64900b54 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:20:44 -0500 Subject: [PATCH] fix(node): require Content-Digest on a signed request 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. --- crates/gitlawb-core/src/http_sig.rs | 38 +++++++++ crates/gitlawb-node/src/auth/mod.rs | 125 +++++++++++++++++++++++----- 2 files changed, 142 insertions(+), 21 deletions(-) diff --git a/crates/gitlawb-core/src/http_sig.rs b/crates/gitlawb-core/src/http_sig.rs index 1089e34d..e2a91c16 100644 --- a/crates/gitlawb-core/src/http_sig.rs +++ b/crates/gitlawb-core/src/http_sig.rs @@ -73,6 +73,17 @@ impl HttpSignature { .map(|s| s.trim_matches('"').to_string()) .collect(); + // RFC 9421 §2.1: a component identifier appears at most once. A repeat + // says nothing extra and only lengthens the signing string a verifier + // has to build, so it is refused rather than folded. + for (i, c) in components.iter().enumerate() { + if components[i + 1..].contains(c) { + return Err(Error::HttpSignature(format!( + "duplicate covered component '{c}' in Signature-Input" + ))); + } + } + let params = parse_params(params_str)?; let key_id: Did = params @@ -272,6 +283,33 @@ mod tests { assert!(missing.contains(&"content-digest")); } + /// RFC 9421 §2.1: a component identifier must not appear twice in the + /// covered list. Repeating one is not a way to say anything, but it does + /// repeat a line in the signing string, so the size of what a verifier + /// builds (and what a node persists alongside a claim) is set by how many + /// times the caller chose to write the same name. + #[test] + fn parse_rejects_duplicate_components() { + let kp = Keypair::generate(); + let did = kp.did(); + let sig_input = format!( + r#"sig1=("@method" "@path" "@path" "content-digest");keyid="{did}";alg="ed25519";created=1000"# + ); + let err = HttpSignature::parse(&sig_input, "sig1=:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA:") + .expect_err("a repeated component must be refused"); + assert!( + err.to_string().contains("duplicate"), + "the error must name the duplication, got: {err}" + ); + + // The control: the same list without the repeat still parses. + let ok = format!( + r#"sig1=("@method" "@path" "content-digest");keyid="{did}";alg="ed25519";created=1000"# + ); + HttpSignature::parse(&ok, "sig1=:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA:") + .expect("a distinct component list must still parse"); + } + #[test] fn verify_signature_end_to_end() { use crate::identity::verify; diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 720fb3ae..f4abfd61 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -162,17 +162,34 @@ pub async fn require_signature(request: Request, next: Next) -> Response { .unwrap_or("/") .to_string(); - let content_digest = parts + // Required, never defaulted. The signing string is rebuilt from the request, + // so an absent header would make the covered `content-digest` empty on both + // sides: verification would pass and the digest-versus-body comparison below + // would have nothing to compare, leaving a signed request free to carry a + // body its signature never covered. `sign_request` always emits the header, + // so refusing here costs no conforming client anything. + let content_digest = match parts .headers .get("content-digest") .and_then(|v| v.to_str().ok()) - .unwrap_or("") - .to_string(); + { + Some(v) => v.to_string(), + None => { + return ( + StatusCode::BAD_REQUEST, + Json(json!({ + "error": "missing_content_digest", + "message": "Content-Digest header is required on a signed request", + })), + ) + .into_response() + } + }; let mut request_values: HashMap = HashMap::new(); request_values.insert("@method".to_string(), method); request_values.insert("@path".to_string(), path_and_query); - request_values.insert("content-digest".to_string(), content_digest); + request_values.insert("content-digest".to_string(), content_digest.clone()); // The @signature-params value is the part of Signature-Input after "sig1=" let sig_params_value = sig_input.strip_prefix("sig1=").unwrap_or(&sig_input); @@ -217,23 +234,18 @@ pub async fn require_signature(request: Request, next: Next) -> Response { .into_response(); } - // Verify Content-Digest matches the actual request body - if let Some(claimed) = parts - .headers - .get("content-digest") - .and_then(|v| v.to_str().ok()) - { - let actual = compute_content_digest(&body_bytes); - if claimed != actual { - return ( - StatusCode::BAD_REQUEST, - Json(json!({ - "error": "content_digest_mismatch", - "message": "Content-Digest does not match request body", - })), - ) - .into_response(); - } + // Verify Content-Digest matches the actual request body. Unconditional: the + // header's presence was established above, so there is no branch here that + // skips the comparison. + if content_digest != compute_content_digest(&body_bytes) { + return ( + StatusCode::BAD_REQUEST, + Json(json!({ + "error": "content_digest_mismatch", + "message": "Content-Digest does not match request body", + })), + ) + .into_response(); } tracing::info!(did = %sig.key_id, "✓ authenticated request"); @@ -610,4 +622,75 @@ mod tests { let body_json: serde_json::Value = serde_json::from_slice(&body_bytes).unwrap(); assert_eq!(body_json["error"], "invalid_ucan"); } + + /// A request that signs `content-digest` as the EMPTY string and then sends + /// no Content-Digest header at all must be refused before any handler runs. + /// + /// This is the whole point of requiring the header rather than defaulting it: + /// the middleware rebuilds the signing string from the request, so an absent + /// header makes the covered digest empty on both sides. The Ed25519 check + /// passes, the body-versus-digest comparison is skipped for want of a header, + /// and a signed request carries a body the signature never covered. + #[tokio::test] + async fn signed_request_without_content_digest_is_rejected() { + use base64::{engine::general_purpose::STANDARD, Engine}; + use std::sync::atomic::{AtomicBool, Ordering}; + + static REACHED: AtomicBool = AtomicBool::new(false); + + let kp = Keypair::generate(); + let did = kp.did(); + let created = chrono::Utc::now().timestamp(); + let signature_input = format!( + r#"sig1=("@method" "@path" "content-digest");keyid="{did}";alg="ed25519";created={created}"# + ); + let sig_params_value = &signature_input["sig1=".len()..]; + + let mut values: HashMap = HashMap::new(); + values.insert("@method".to_string(), "POST".to_string()); + values.insert("@path".to_string(), "/x".to_string()); + // Exactly what the middleware derives when the header is absent. + values.insert("content-digest".to_string(), String::new()); + let signing_string = + build_signing_string(COVERED_COMPONENTS, sig_params_value, &values).unwrap(); + let signature = format!( + "sig1=:{}:", + STANDARD.encode(kp.sign(signing_string.as_bytes()).to_bytes()) + ); + + let app = Router::new() + .route( + "/x", + axum::routing::post(|| async { + REACHED.store(true, Ordering::SeqCst); + StatusCode::OK + }), + ) + .layer(middleware::from_fn(require_signature)); + + let req = Request::builder() + .method("POST") + .uri("/x") + .header("signature-input", signature_input) + .header("signature", signature) + .body(Body::from("a body no signature covers")) + .unwrap(); + + let resp = app.oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "a signed request with no Content-Digest header must be refused" + ); + let bytes = axum::body::to_bytes(resp.into_body(), 2048).await.unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!( + body["error"], "missing_content_digest", + "the refusal must name the absent Content-Digest header" + ); + assert!( + !REACHED.load(Ordering::SeqCst), + "the handler must never see a request whose body no signature covers" + ); + } }