From 95f658ce888c1da1596fb70aefc044da8b403f0e Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 23 Sep 2026 13:49:32 -0400 Subject: [PATCH] feat(oauth)!: check the DPoP proof on a pushed authorization request The atproto profile has a client begin DPoP at PAR. A push with no proof is refused as invalid_dpop_proof, and one whose proof carries no current nonce gets the use_dpop_nonce challenge, both before the client's document is fetched. Nothing is stored from the proof, and its jti is not spent. Co-Authored-By: Claude Opus 5.5 (1M context) Change-Id: I69648641752ac73e407fd5de1e2ad6f318e4de23 --- crates/didbot-serve/src/routes/oauth.rs | 8 +- crates/didbot-serve/src/tests/oauth.rs | 18 +-- .../didbot-serve/tests/oauth_account_flow.rs | 4 +- .../tests/oauth_standard_client.rs | 111 ++++++++++++++++-- crates/didbot-swarm/tests/decision_bounds.rs | 64 ++++++---- crates/didbot/tests/conformance/bot_did.rs | 67 +++++++---- 6 files changed, 211 insertions(+), 61 deletions(-) diff --git a/crates/didbot-serve/src/routes/oauth.rs b/crates/didbot-serve/src/routes/oauth.rs index 81e67b98..88344543 100644 --- a/crates/didbot-serve/src/routes/oauth.rs +++ b/crates/didbot-serve/src/routes/oauth.rs @@ -52,7 +52,7 @@ pub(crate) struct OAuthParForm { /// /// Unauthenticated, and bounded per caller address by /// [`crate::oauth::OAuthState::par_rate_limiter`] before any of that -/// happens. +/// happens. Its DPoP proof is checked next; see [`checked_proof`]. pub(crate) async fn oauth_par( State(state): State, peer: Peer, @@ -69,6 +69,12 @@ pub(crate) async fn oauth_par( return too_many_oauth_requests("par"); } + // Checked and not accepted: a push may carry no credential at all, and + // an entry in the replay window costs one. + if let Err(refused) = checked_proof(&state, &headers, "/oauth/par") { + return refused.into_response(); + } + let request = ParRequest { client_id: form.client_id, response_type: form.response_type, diff --git a/crates/didbot-serve/src/tests/oauth.rs b/crates/didbot-serve/src/tests/oauth.rs index bf0e4542..fb943cbd 100644 --- a/crates/didbot-serve/src/tests/oauth.rs +++ b/crates/didbot-serve/src/tests/oauth.rs @@ -203,9 +203,9 @@ fn get_from(uri: &str, caller: &str) -> Request { /// `POST /oauth/par`, carrying an `X-Forwarded-For` header; the `POST` /// counterpart of [`get_from`]. fn par_from(caller: &str) -> Request { - // `http:` rather than `https:`, so `resolve_client` refuses the scheme - // before it would go and fetch anything: this test is about the budget - // in front of that fetch, not about the fetch. + // No DPoP proof, so the push is refused before it would go and fetch + // anything: this test is about the budget in front of that fetch, not + // about the fetch. Request::builder() .method("POST") .uri("/oauth/par") @@ -245,15 +245,15 @@ async fn oauth_par_is_rate_limited_per_caller_address() { crate::health::HealthState::new(), ); - // Short of the budget every call fails on the client's own metadata -- - // 401 `invalid_client`, not 429 -- and only crossing it answers with - // `TOO_MANY_REQUESTS`. + // Short of the budget every call fails on its missing DPoP proof -- + // 400 `invalid_dpop_proof`, not 429 -- and only crossing it answers + // with `TOO_MANY_REQUESTS`. let (first, _) = send(&app, par_from("203.0.113.9")).await; let (second, _) = send(&app, par_from("203.0.113.9")).await; let (third, body) = send(&app, par_from("203.0.113.9")).await; - assert_eq!(first, StatusCode::UNAUTHORIZED); - assert_eq!(second, StatusCode::UNAUTHORIZED); + assert_eq!(first, StatusCode::BAD_REQUEST); + assert_eq!(second, StatusCode::BAD_REQUEST); assert_eq!(third, StatusCode::TOO_MANY_REQUESTS, "{body}"); assert_eq!( body["error"], "invalid_request", @@ -262,7 +262,7 @@ async fn oauth_par_is_rate_limited_per_caller_address() { // A different caller address is a separate budget. let (status, _) = send(&app, par_from("198.51.100.4")).await; - assert_eq!(status, StatusCode::UNAUTHORIZED); + assert_eq!(status, StatusCode::BAD_REQUEST); } /// `POST /oauth/token`, carrying an `X-Forwarded-For` header. The refresh diff --git a/crates/didbot-serve/tests/oauth_account_flow.rs b/crates/didbot-serve/tests/oauth_account_flow.rs index 7e926fe4..0cf3b628 100644 --- a/crates/didbot-serve/tests/oauth_account_flow.rs +++ b/crates/didbot-serve/tests/oauth_account_flow.rs @@ -363,7 +363,8 @@ async fn push_as( } /// One `POST /oauth/par` carrying exactly `form`, for the tests about a -/// malformed push. +/// malformed push. Its DPoP proof is a string the fixture's verifier takes +/// whatever it says. async fn post_par(fixture: &Fixture, form: &[(&str, &str)]) -> (StatusCode, serde_json::Value) { let response = fixture .app @@ -373,6 +374,7 @@ async fn post_par(fixture: &Fixture, form: &[(&str, &str)]) -> (StatusCode, serd .method("POST") .uri("/oauth/par") .header("content-type", "application/x-www-form-urlencoded") + .header("dpop", "any-proof-string") .body(Body::from(form_body(form))) .unwrap(), ) diff --git a/crates/didbot-serve/tests/oauth_standard_client.rs b/crates/didbot-serve/tests/oauth_standard_client.rs index a255a249..1fe0a6fe 100644 --- a/crates/didbot-serve/tests/oauth_standard_client.rs +++ b/crates/didbot-serve/tests/oauth_standard_client.rs @@ -27,13 +27,14 @@ //! A confidential client signs in with jacquard's own keyset, and every //! request it makes to `par` and `token` carries a client assertion signed //! with it. The assertions the refusals below are built from are signed by -//! that keyset too. Its callback is a loopback listener, the redirect this -//! server finishes a sign-in for. +//! that keyset too, and the pushes that carry them sign their DPoP proofs +//! with jacquard's own proof builder. Its callback is a loopback listener, +//! the redirect this server finishes a sign-in for. use std::sync::Arc; use axum::body::Body; -use axum::http::{header, Request, StatusCode}; +use axum::http::{header, HeaderMap, Request, StatusCode}; use jacquard_common::http_client::HttpClient; use jacquard_common::xrpc::{GenericError, XrpcClient, XrpcMethod, XrpcRequest, XrpcResp}; use jacquard_common::{AuthorizationToken, BosStr}; @@ -62,6 +63,7 @@ use didbot_serve::oauth::client_metadata::{ validate, CachedClientMetadata, ClientKey, ClientMetadataCache, ClientMetadataDocument, MemoryClientMetadataCache, }; +use didbot_serve::oauth::dpop::DPOP_NONCE; use didbot_serve::oauth::token::OAuthTokenStore; use didbot_serve::oauth::OAuthState; use didbot_serve::{app_with_auth, AuthState, HealthState}; @@ -296,6 +298,15 @@ impl Deployment { /// One request to the router, answered as JSON. async fn call(&self, request: Request) -> (StatusCode, serde_json::Value) { + let (status, _, body) = self.call_with_headers(request).await; + (status, body) + } + + /// [`Self::call`], keeping the answer's headers. + async fn call_with_headers( + &self, + request: Request, + ) -> (StatusCode, HeaderMap, serde_json::Value) { let response = self .network .app @@ -304,11 +315,13 @@ impl Deployment { .await .expect("the router answers every request"); let status = response.status(); + let headers = response.headers().clone(); let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) .await .expect("a body the router built is finite"); ( status, + headers, serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null), ) } @@ -337,8 +350,33 @@ impl Deployment { } /// `POST /oauth/par` for [`WEB_CLIENT`], signing in as this deployment's - /// account, carrying `assertion` when there is one. + /// account, carrying `assertion` when there is one. A nonce challenge is + /// answered once, as jacquard answers one: the same push, with a proof + /// carrying the nonce it names. async fn push(&self, assertion: Option<&str>) -> (StatusCode, serde_json::Value) { + let prove = par_prover(&self.network.zone); + let (status, headers, body) = self.push_proving(assertion, Some(&prove(None))).await; + match headers + .get(DPOP_NONCE) + .and_then(|value| value.to_str().ok()) + { + Some(nonce) if body["error"] == "use_dpop_nonce" => { + let (status, _, body) = self + .push_proving(assertion, Some(&prove(Some(nonce)))) + .await; + (status, body) + } + _ => (status, body), + } + } + + /// The same push, carrying `proof` as its DPoP proof when there is one, + /// answered with its headers. + async fn push_proving( + &self, + assertion: Option<&str>, + proof: Option<&str>, + ) -> (StatusCode, HeaderMap, serde_json::Value) { let mut form = url::form_urlencoded::Serializer::new(String::new()); form.append_pair("client_id", WEB_CLIENT) .append_pair("response_type", "code") @@ -359,11 +397,16 @@ impl Deployment { ) .append_pair("client_assertion", assertion); } - self.call( - Request::builder() - .method("POST") - .uri("/oauth/par") - .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + let request = Request::builder() + .method("POST") + .uri("/oauth/par") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded"); + let request = match proof { + Some(proof) => request.header("dpop", proof), + None => request, + }; + self.call_with_headers( + request .body(Body::from(form.finish())) .expect("request builds"), ) @@ -497,6 +540,18 @@ struct Loaded { page: String, } +/// Signs `POST /oauth/par` proofs for `zone` with one fresh key, each +/// carrying the nonce it is handed, as jacquard signs a push. +fn par_prover(zone: &str) -> impl Fn(Option<&str>) -> String { + let key = jacquard_oauth::utils::generate_key(&["ES256"]).expect("jacquard makes an ES256 key"); + let htu = format!("https://{zone}/oauth/par"); + move |nonce| { + jacquard_oauth::dpop::build_dpop_proof(&key, "POST", &htu, nonce, None) + .expect("jacquard signs a proof") + .to_string() + } +} + /// `com.atproto.repo.createRecord`, as jacquard sends any procedure. #[derive(serde::Serialize)] struct CreateRecord { @@ -1049,3 +1104,41 @@ async fn a_public_client_that_sends_an_assertion_is_refused() { let (status, body) = deployment.push(Some(&signed)).await; assert_invalid_client(status, &body, "names no client authentication"); } + +/// **The profile's first DPoP request.** A push whose proof carries no nonce +/// is RFC 9449 §8's challenge: a 400 naming `use_dpop_nonce`, with the nonce +/// in `DPoP-Nonce`. The same push, signed again with that nonce, lands, and +/// its answer hands out a nonce for the next request. +#[tokio::test] +async fn a_push_without_a_nonce_is_challenged_and_its_retry_lands() { + let deployment = Deployment::build("pds.test", Some("quillwort.pds.test")); + deployment.publish(&web_client_metadata(), None); + let prove = par_prover(&deployment.network.zone); + + let (status, headers, body) = deployment.push_proving(None, Some(&prove(None))).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + assert_eq!(body["error"], "use_dpop_nonce", "{body}"); + let nonce = headers + .get(DPOP_NONCE) + .and_then(|value| value.to_str().ok()) + .expect("the challenge names the nonce to retry with") + .to_owned(); + + let (status, headers, body) = deployment + .push_proving(None, Some(&prove(Some(&nonce)))) + .await; + assert_eq!(status, StatusCode::CREATED, "{body}"); + assert!(headers.contains_key(DPOP_NONCE), "{headers:?}"); +} + +/// The profile has a client begin DPoP at its push, so a push with no proof +/// is refused, as RFC 9449 §5 refuses a proof that does not check out. +#[tokio::test] +async fn a_push_without_a_proof_is_refused() { + let deployment = Deployment::build("pds.test", Some("quillwort.pds.test")); + deployment.publish(&web_client_metadata(), None); + + let (status, _, body) = deployment.push_proving(None, None).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + assert_eq!(body["error"], "invalid_dpop_proof", "{body}"); +} diff --git a/crates/didbot-swarm/tests/decision_bounds.rs b/crates/didbot-swarm/tests/decision_bounds.rs index def6c09d..40bcf9e7 100644 --- a/crates/didbot-swarm/tests/decision_bounds.rs +++ b/crates/didbot-swarm/tests/decision_bounds.rs @@ -128,6 +128,10 @@ const REDIRECT_URI: &str = "http://127.0.0.1:1/cb"; /// status and body -- a caller decides for itself whether a refusal is /// expected. /// +/// Its DPoP proof is signed with a key made for the push, and a nonce +/// challenge is answered once with the nonce it names, as a client answers +/// one. +/// /// The `code_challenge` is a fixed, well-formed placeholder: nothing in this /// file exchanges the codes these pushes could produce (bar the one signed-in /// agent, which mints its own through [`Pds::sign_in`]), so nothing needs it @@ -137,26 +141,46 @@ async fn push( base_url: &str, login_hint: &str, ) -> (reqwest::StatusCode, serde_json::Value) { - let response = http - .post(format!("{base_url}/oauth/par")) - .form(&[ - ("client_id", client_id().as_str()), - ("response_type", "code"), - ("redirect_uri", REDIRECT_URI), - ("scope", "atproto"), - ( - "code_challenge", - "a-fixed-placeholder-challenge-1234567890123", - ), - ("code_challenge_method", "S256"), - ("login_hint", login_hint), - ]) - .send() - .await - .expect("PAR is reachable"); - let status = response.status(); - let body = response.json().await.unwrap_or(serde_json::Value::Null); - (status, body) + let key = jacquard_oauth::utils::generate_key(&["ES256"]).expect("jacquard makes an ES256 key"); + // What the server's own metadata names, which is not the address dialled. + let htu = format!("http://{ZONE}/oauth/par"); + let mut nonce: Option = None; + loop { + let proof = + jacquard_oauth::dpop::build_dpop_proof(&key, "POST", &htu, nonce.as_deref(), None) + .expect("jacquard signs a proof"); + let response = http + .post(format!("{base_url}/oauth/par")) + .header("dpop", proof.as_str()) + .form(&[ + ("client_id", client_id().as_str()), + ("response_type", "code"), + ("redirect_uri", REDIRECT_URI), + ("scope", "atproto"), + ( + "code_challenge", + "a-fixed-placeholder-challenge-1234567890123", + ), + ("code_challenge_method", "S256"), + ("login_hint", login_hint), + ]) + .send() + .await + .expect("PAR is reachable"); + let status = response.status(); + let named = response + .headers() + .get("dpop-nonce") + .and_then(|value| value.to_str().ok()) + .map(str::to_owned); + let body = response.json().await.unwrap_or(serde_json::Value::Null); + match named { + Some(named) if nonce.is_none() && body["error"] == "use_dpop_nonce" => { + nonce = Some(named); + } + _ => return (status, body), + } + } } async fn get_authorization( diff --git a/crates/didbot/tests/conformance/bot_did.rs b/crates/didbot/tests/conformance/bot_did.rs index 0ad0d740..371798aa 100644 --- a/crates/didbot/tests/conformance/bot_did.rs +++ b/crates/didbot/tests/conformance/bot_did.rs @@ -692,7 +692,8 @@ async fn an_unknown_request_is_read_with_a_declared_error() { } /// One real `POST /oauth/par` for the account, answering with the pushed -/// request it minted. +/// request it minted. Its DPoP proof is signed with a key made for the push, +/// and the server's nonce challenge is answered once, as a client answers it. async fn push(server: &Deployment) -> String { let form = [ ("client_id", CLIENT_ID), @@ -711,26 +712,50 @@ async fn push(server: &Deployment) -> String { .map(|(name, value)| format!("{name}={}", encode(value))) .collect::>() .join("&"); - let answer = call( - &server.app, - Request::builder() - .method("POST") - .uri("/oauth/par") - .header("content-type", "application/x-www-form-urlencoded") - .body(Body::from(form)) - .expect("request builds"), - ) - .await; - assert_eq!( - answer.status, - StatusCode::CREATED, - "the push was refused: {}", - answer.body - ); - answer.body["request_uri"] - .as_str() - .expect("a pushed request has a uri") - .to_owned() + let key = jacquard_oauth::utils::generate_key(&["ES256"]).expect("jacquard makes an ES256 key"); + let htu = format!("http://{ZONE}/oauth/par"); + let mut nonce: Option = None; + loop { + let proof = + jacquard_oauth::dpop::build_dpop_proof(&key, "POST", &htu, nonce.as_deref(), None) + .expect("jacquard signs a proof"); + let response = server + .app + .clone() + .oneshot( + Request::builder() + .method("POST") + .uri("/oauth/par") + .header("content-type", "application/x-www-form-urlencoded") + .header("dpop", proof.as_str()) + .body(Body::from(form.clone())) + .expect("request builds"), + ) + .await + .expect("the router answers"); + let status = response.status(); + let named = response + .headers() + .get("dpop-nonce") + .and_then(|value| value.to_str().ok()) + .map(str::to_owned); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("body reads"); + let body: Value = serde_json::from_slice(&bytes).unwrap_or(Value::Null); + match named { + Some(named) if nonce.is_none() && body["error"] == "use_dpop_nonce" => { + nonce = Some(named); + } + _ => { + assert_eq!(status, StatusCode::CREATED, "the push was refused: {body}"); + return body["request_uri"] + .as_str() + .expect("a pushed request has a uri") + .to_owned(); + } + } + } } // --------------------------------------------------------------------------- -- 2.51.2