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" + ); + } }