diff --git a/crates/didbot-serve/src/auth.rs b/crates/didbot-serve/src/auth.rs index bebd453f..06491738 100644 --- a/crates/didbot-serve/src/auth.rs +++ b/crates/didbot-serve/src/auth.rs @@ -411,6 +411,99 @@ pub fn require_agent_token( } } +/// The `DPoP` credential scheme's raw pieces, straight off the +/// `Authorization` and `DPoP` headers — deliberately not routed through +/// [`present`]/[`Presented`], which still folds `DPoP` into +/// [`Presented::Disabled`] for every other route in this module that has +/// not opted into accepting it (see [`DISABLED_SCHEMES`]'s own doc). Kept to +/// exactly the two headers a caller presenting this scheme sends, mirroring +/// [`present`]'s own header-splitting rather than growing that function a +/// third scheme every existing call site would need a new match arm for. +fn dpop_bearer(headers: &HeaderMap) -> Option<(String, &str)> { + let raw = headers + .get(header::AUTHORIZATION) + .and_then(|value| value.to_str().ok())?; + let (scheme, rest) = raw.split_once(' ')?; + if !scheme.eq_ignore_ascii_case("DPoP") { + return None; + } + let proof = headers.get("dpop").and_then(|value| value.to_str().ok())?; + Some((rest.trim().to_owned(), proof)) +} + +/// Resolves either an agent's own [`Credential::AgentToken`] or a DPoP-bound +/// OAuth access token minted by `crate::oauth::token`, for +/// `com.atproto.repo.*`'s write routes and `uploadBlob` — the resource path +/// an app authorized over OAuth actually writes through. +/// +/// `http_method` and `path` are this request's own method and route path +/// (e.g. `"/xrpc/com.atproto.repo.createRecord"`); a DPoP proof presented +/// here must bind to *this* request, not merely to some request the caller +/// once made. The absolute `htu` a proof is checked against is derived from +/// `path` the same way [`crate::oauth::discovery::request_htu`] builds it +/// for `/oauth/token` — out of this deployment's own zone service document, +/// never a header — and, deliberately, only when a `DPoP`-scheme credential +/// is actually presented: an agent-token route with no zone document seeded +/// yet (a bare `FakeRegistry`, or a deployment mid-bootstrap) must still be +/// reachable with a plain agent token, which needs no `htu` at all. +/// +/// A `DPoP ` credential is checked in two steps, both required: the +/// `DPoP` header's proof must verify against this request +/// ([`crate::oauth::dpop_seam::DpopVerifier::verify`]), and the resulting +/// thumbprint must match the one `access_token` was minted bound to +/// ([`crate::oauth::token::OAuthTokenStore::validate_access`]). A bare +/// stolen token with no proof, or a well-formed proof signed by the wrong +/// key, fails at one of those two steps — see `plan/adversarial.md`'s +/// DPoP-binding findings, which this function exists to close. +pub fn require_agent_token_or_dpop( + registry: &dyn Registry, + oauth: &crate::oauth::OAuthState, + headers: &HeaderMap, + http_method: &str, + path: &str, +) -> Result { + if let Some((token, proof)) = dpop_bearer(headers) { + let htu = crate::oauth::discovery::request_htu(registry, path).map_err(|err| { + ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "ServerError", + err.to_string(), + ) + })?; + let thumbprint = oauth + .dpop + .verify(proof, http_method, &htu) + .map_err(|err| invalid_token(&err.to_string()))?; + return oauth + .tokens + .validate_access(&token, &thumbprint) + .ok_or_else(|| { + invalid_token( + "this access token is not one this server issued, has expired, or does not \ + match the key the presented DPoP proof proves possession of", + ) + }); + } + match present(headers) { + Presented::None => Err(authentication_required("no Authorization header")), + Presented::Unrecognized(scheme) => Err(authentication_required(&format!( + "`{scheme}` is not a credential scheme this server defines" + ))), + Presented::Disabled(scheme) => Err(scheme_disabled(scheme)), + Presented::Operator(_) => Err(invalid_token( + "this route takes an agent token or a DPoP-bound OAuth access token, not an \ + operator credential", + )), + Presented::Bearer(token) => match registry.verify_agent_token(&token) { + Ok(did) => Ok(did), + Err(TokenError::Expired) => Err(expired_token()), + Err(TokenError::Unknown) => Err(invalid_token( + "this token is not an agent token this server issued", + )), + }, + } +} + /// Resolves either an agent's own credential or an operator credential, for /// `deleteAgent` and `setAgentPinned`. /// diff --git a/crates/didbot-serve/src/routes.rs b/crates/didbot-serve/src/routes.rs index 0d8ec01b..4c9c558a 100644 --- a/crates/didbot-serve/src/routes.rs +++ b/crates/didbot-serve/src/routes.rs @@ -521,6 +521,23 @@ fn parse_body(body: &Bytes) -> Result { serde_json::from_slice(body).map_err(|err| ApiError::bad_request(err.to_string())) } +/// Resolves a repo write route's credential — an agent token or a DPoP-bound +/// OAuth access token. See `auth::require_agent_token_or_dpop`. +fn resource_credential( + state: &AppState, + headers: &HeaderMap, + http_method: &str, + path: &str, +) -> Result { + auth::require_agent_token_or_dpop( + state.registry.as_ref(), + &state.oauth, + headers, + http_method, + path, + ) +} + /// `GET /health` /// /// `status` is liveness: it is `"ok"` whenever this process can schedule a @@ -1303,7 +1320,12 @@ async fn list_ledgers(State(state): State, headers: HeaderMap) -> Resp /// client to re-read and retry, and this retry can never succeed. A caller /// that meant to replace the record wants `putRecord`. async fn create_record(State(state): State, headers: HeaderMap, body: Bytes) -> Response { - let authenticated = match auth::require_agent_token(state.registry.as_ref(), &headers) { + let authenticated = match resource_credential( + &state, + &headers, + "POST", + "/xrpc/com.atproto.repo.createRecord", + ) { Ok(did) => did, Err(err) => return err.into_response(), }; @@ -1342,10 +1364,11 @@ async fn create_record(State(state): State, headers: HeaderMap, body: /// and [`create_record`]. Both `swapRecord` and `swapCommit` are honoured, /// and `swapRecord: null` asserts the record does not exist yet. async fn put_record(State(state): State, headers: HeaderMap, body: Bytes) -> Response { - let authenticated = match auth::require_agent_token(state.registry.as_ref(), &headers) { - Ok(did) => did, - Err(err) => return err.into_response(), - }; + let authenticated = + match resource_credential(&state, &headers, "POST", "/xrpc/com.atproto.repo.putRecord") { + Ok(did) => did, + Err(err) => return err.into_response(), + }; let request: PutRecordRequest = match parse_body(&body) { Ok(request) => request, Err(err) => return err.into_response(), @@ -1485,7 +1508,12 @@ fn check_write_not_halted(state: &AppState) -> Result<(), ApiError> { /// The response omits `commit`, which the lexicon marks optional. There is no /// commit here to report. async fn delete_record(State(state): State, headers: HeaderMap, body: Bytes) -> Response { - let authenticated = match auth::require_agent_token(state.registry.as_ref(), &headers) { + let authenticated = match resource_credential( + &state, + &headers, + "POST", + "/xrpc/com.atproto.repo.deleteRecord", + ) { Ok(did) => did, Err(err) => return err.into_response(), }; @@ -1536,7 +1564,12 @@ async fn delete_record(State(state): State, headers: HeaderMap, body: /// look before it: it is checked under the same lock the batch's one commit /// is built under. async fn apply_writes(State(state): State, headers: HeaderMap, body: Bytes) -> Response { - let authenticated = match auth::require_agent_token(state.registry.as_ref(), &headers) { + let authenticated = match resource_credential( + &state, + &headers, + "POST", + "/xrpc/com.atproto.repo.applyWrites", + ) { Ok(did) => did, Err(err) => return err.into_response(), }; diff --git a/crates/didbot-serve/src/tests.rs b/crates/didbot-serve/src/tests.rs index 2566601e..9a2b4285 100644 --- a/crates/didbot-serve/src/tests.rs +++ b/crates/didbot-serve/src/tests.rs @@ -6665,6 +6665,94 @@ mod dpop_binding { authenticate" ); } + + /// Full round trip, through the real routes: an app mints a DPoP-bound + /// access token over `/oauth/token`, an attacker holding the bare token + /// string but not the key tries to write a record with it and is + /// refused by `com.atproto.repo.createRecord` itself, and the + /// legitimate client's own key succeeds — the resource-path binding + /// `auth::require_agent_token_or_dpop` and `crate::routes::create_record` + /// wire together, exercised end to end rather than at a single + /// function's own unit tests. `plan/adversarial.md`'s "the seam is + /// whether the resource-serving code path actually extracts `cnf.jkt` + /// from the token and passes it through to the check" is exactly what + /// this test drives. + #[tokio::test] + async fn a_forged_key_cannot_write_with_a_stolen_access_token_and_the_real_key_can() { + let fake = FakeRegistry::seeded("kestrel"); + seed_zone_document(&fake, PDS_ENDPOINT); + let registry: Arc = Arc::new(fake); + + let code_store = Arc::new(MemoryAuthorizationCodeStore::new()); + let client_id = "https://client.example/metadata.json"; + let (code, code_verifier) = seed_code(&code_store, client_id, SEEDED_DID); + + let app = app_with_auth( + registry, + BroadcastSink::default(), + Firehose::default(), + AuthState { + oauth: crate::oauth::OAuthState { + code_store: code_store.clone(), + ..crate::oauth::OAuthState::default() + }, + ..AuthState::default() + }, + HealthState::new(), + ); + + // Mint, bound to the legitimate client's key. + let legit_key = P256SigningKey::random(&mut rand_core::OsRng); + let token_htu = format!("{PDS_ENDPOINT}/oauth/token"); + let mint_proof = build_proof(&legit_key, "POST", &token_htu, "mint-jti"); + let (status, _headers, body) = call_on( + app.clone(), + token_request(&code, &code_verifier, client_id, &mint_proof), + ) + .await; + assert_eq!(status, StatusCode::OK, "exchange must succeed: {body:?}"); + let access_token = body["access_token"] + .as_str() + .expect("access_token present") + .to_owned(); + + let write_htu = format!("{PDS_ENDPOINT}/xrpc/com.atproto.repo.createRecord"); + let write_request = |proof: String| { + Request::builder() + .method("POST") + .uri("/xrpc/com.atproto.repo.createRecord") + .header(header::CONTENT_TYPE, "application/json") + .header(header::AUTHORIZATION, format!("DPoP {access_token}")) + .header("dpop", proof) + .body(Body::from(create_body( + SCROBBLE, + scrobble("dpop end to end"), + ))) + .expect("request builds") + }; + + // The attacker: the bare access_token string, a forged key of their + // own, an otherwise well-formed proof. + let attacker_key = P256SigningKey::random(&mut rand_core::OsRng); + let forged_proof = build_proof(&attacker_key, "POST", &write_htu, "forged-write-jti"); + let (status, _headers, body) = call_on(app.clone(), write_request(forged_proof)).await; + assert_eq!( + status, + StatusCode::UNAUTHORIZED, + "a forged key's proof must not write with a stolen access token, got {body:?}" + ); + assert_eq!(body["error"], "InvalidToken"); + + // The legitimate client, using its own key, succeeds. + let legit_proof = build_proof(&legit_key, "POST", &write_htu, "legit-write-jti"); + let (status, _headers, body) = call_on(app, write_request(legit_proof)).await; + assert_eq!( + status, + StatusCode::OK, + "the legitimate client's own key must be able to write, got {body:?}" + ); + } + /// Positive control: `oauth::token::OAuthTokenStore::refresh` *does* /// enforce the binding this module's other tests find missing /// elsewhere, and does so against the specific adversarial shape the