From 6fd046417afbdfbe6cf49d1b425e84970ba242df Mon Sep 17 00:00:00 2001 From: tomdebone Date: Thu, 10 Sep 2026 06:25:51 +0200 Subject: [PATCH] =?UTF-8?q?feat(auth):=20Audience=20der=20Access-Tokens=20?= =?UTF-8?q?pr=C3=BCfen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verify_jwt` setzt `validate_aud = false` — es kann den Aufrufer nicht kennen. Also blieb `aud` bisher ungeprüft, obwohl die PDS es setzt. Was die Prüfung bringt: die PDS signiert Tokens für *ihre* AppView. Ohne Audience-Check wäre ein Token, das an einen anderen Dienst mit derselben PDS-Vertrauensbeziehung geht, hier wiederverwendbar — und umgekehrt. Es ist der Unterschied zwischen "die PDS bürgt für diesen Nutzer" und "die PDS bürgt für diesen Nutzer *im Gespräch mit uns*". Dafür musste der Wert erst einmal etwas sein, das beide Seiten berechnen können: die PDS setzte ihn hart auf did:web:appview.maarcadetweet.local. Jetzt leiten ihn beide über AppConfig::appview_did() aus APPVIEW_PUBLIC_URL ab — dieselbe did:web-Regel wie schon für pds_did(). Ein Mismatch ist TokenInvalid, nicht Forbidden: das ist der Code, auf den der Client seine Token-Erneuerung stützt. Eine Instanz, die ihre APPVIEW_PUBLIC_URL ändert, heilt sich damit beim nächsten Refresh selbst, statt jeden angemeldeten Nutzer auszusperren. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013HC9HLrUU1LNwkzp8nkDLX --- crates/appview/src/auth.rs | 116 +++++++++++++++++++++++----- crates/appview/tests/common/mod.rs | 21 +++-- crates/at-shared/src/config.rs | 13 ++++ crates/pds-server/src/jwt_issuer.rs | 5 +- 4 files changed, 128 insertions(+), 27 deletions(-) diff --git a/crates/appview/src/auth.rs b/crates/appview/src/auth.rs index c7b70ea..a4eec36 100644 --- a/crates/appview/src/auth.rs +++ b/crates/appview/src/auth.rs @@ -157,12 +157,16 @@ pub struct PdsKeys { http: reqwest::Client, /// Fully-qualified URL of the PDS's DID document. did_doc_url: String, + /// The `aud` every access token must carry: this AppView's own + /// service DID. See [`verify_with_key`] for why it's checked. + expected_aud: String, inner: RwLock, } impl PdsKeys { - /// Build a cache pointed at `base_url` (no trailing slash required). - pub fn new(base_url: &str) -> Self { + /// Build a cache pointed at `base_url` (no trailing slash required), + /// accepting only tokens addressed to `expected_aud`. + pub fn new(base_url: &str, expected_aud: impl Into) -> Self { let http = reqwest::Client::builder() .timeout(DID_DOC_TIMEOUT) .build() @@ -173,6 +177,7 @@ impl PdsKeys { "{}/.well-known/did.json", base_url.trim_end_matches('/') ), + expected_aud: expected_aud.into(), inner: RwLock::new(CachedKey::default()), } } @@ -180,7 +185,7 @@ impl PdsKeys { /// Same PDS the handle-sync worker talks to: `PDS_INTERNAL_URL` /// when set, else `PDS_PUBLIC_URL`. pub fn from_config(cfg: &at_shared::config::AppConfig) -> Self { - Self::new(&cfg.pds_base_url()) + Self::new(&cfg.pds_base_url(), cfg.appview_did()) } pub fn did_doc_url(&self) -> &str { @@ -267,13 +272,13 @@ impl PdsKeys { /// restart. pub async fn verify_access_token(&self, token: &str) -> Result { let key = self.key_or_fetch().await?; - match verify_with_key(token, &key) { + match verify_with_key(token, &key, &self.expected_aud) { Ok(claims) => Ok(claims), Err(first) => { let Some(fresh) = self.refetch_if_stale(&key).await else { return Err(first); }; - verify_with_key(token, &fresh).map_err(|_| first) + verify_with_key(token, &fresh, &self.expected_aud).map_err(|_| first) } } } @@ -309,16 +314,44 @@ fn extract_public_key_multibase(doc: &Value) -> anyhow::Result { /// leeway for clock skew); the scope check is ours, and it is the line /// that keeps a 90-day refresh token from working as a session /// credential. -fn verify_with_key(token: &str, pubkey_multibase: &str) -> Result { +fn verify_with_key( + token: &str, + pubkey_multibase: &str, + expected_aud: &str, +) -> Result { let claims = at_crypto::jwt::verify_jwt(token, pubkey_multibase) .map_err(|e| AuthError::Invalid(format!("invalid token: {e}")))?; match claims.scope.as_deref() { - Some(ACCESS_SCOPE) => Ok(claims), - other => Err(AuthError::Invalid(format!( - "token scope {:?} is not {ACCESS_SCOPE}", - other.unwrap_or("") - ))), + Some(ACCESS_SCOPE) => {} + other => { + return Err(AuthError::Invalid(format!( + "token scope {:?} is not {ACCESS_SCOPE}", + other.unwrap_or("") + ))) + } } + // Audience. `at_crypto::jwt::verify_jwt` sets `validate_aud = false` + // because it has no way of knowing who the caller is, so the check + // belongs here. + // + // What it buys: the PDS signs tokens for *its* AppView. Without an + // audience check, a token handed to any other service that trusts + // the same PDS key would be replayable here — and, the other way + // round, a token this AppView issued trust in could be replayed + // there. It is the difference between "the PDS vouches for this + // user" and "the PDS vouches for this user *talking to us*". + // + // A mismatch is `TokenInvalid` rather than `Forbidden` on purpose: + // that is the code the desktop client refreshes on, so a + // deployment that changes `APPVIEW_PUBLIC_URL` heals itself on the + // next refresh instead of stranding every signed-in user. + if claims.aud != expected_aud { + return Err(AuthError::Invalid(format!( + "token audience {:?} is not {expected_aud:?}", + claims.aud + ))); + } + Ok(claims) } /// Extract the bearer token from an `Authorization` header. @@ -466,14 +499,29 @@ mod tests { (kp, multibase) } + /// The audience the tests' AppView identifies as — what + /// `AppConfig::appview_did()` would return for + /// `APPVIEW_PUBLIC_URL=http://127.0.0.1:2584`. + const TEST_AUD: &str = "did:web:127.0.0.1%3A2584"; + fn mint(kp: &P256Keypair, did: &str, scope: &str, ttl_secs: i64) -> String { + mint_for(kp, did, scope, ttl_secs, TEST_AUD) + } + + fn mint_for( + kp: &P256Keypair, + did: &str, + scope: &str, + ttl_secs: i64, + aud: &str, + ) -> String { let now = chrono::Utc::now().timestamp(); issue_jwt( kp, &JwtClaims { iss: "did:web:127.0.0.1%3A2583".into(), sub: did.into(), - aud: "did:web:appview.maarcadetweet.local".into(), + aud: aud.into(), iat: now - 1, exp: now + ttl_secs, jti: None, @@ -511,6 +559,34 @@ mod tests { assert_eq!(bearer_token(&header_map("Bearer tok")).unwrap(), "tok"); } + /// A token minted for a different AppView must not work here, and + /// must fail as `TokenInvalid` so the client refreshes rather than + /// treating it as a permanent rejection. + #[test] + fn token_for_another_audience_is_rejected() { + let (kp, mb) = test_key(); + let token = mint_for( + &kp, + "did:plc:alice", + ACCESS_SCOPE, + 3600, + "did:web:someone-elses-appview.example", + ); + let err = verify_with_key(&token, &mb, TEST_AUD).unwrap_err(); + assert!( + matches!(err, AuthError::Invalid(ref m) if m.contains("audience")), + "expected an audience rejection, got {err:?}" + ); + let (status, body) = err.into_response_parts(); + assert_eq!(status, StatusCode::UNAUTHORIZED); + assert_eq!(body.0["error"], "TokenInvalid"); + + // The same token *is* fine for the AppView it was minted for. + assert!( + verify_with_key(&token, &mb, "did:web:someone-elses-appview.example").is_ok() + ); + } + #[test] fn error_bodies_carry_the_documented_codes() { // These strings are a contract: the desktop client keys its @@ -535,14 +611,14 @@ mod tests { fn valid_access_token_verifies() { let (kp, mb) = test_key(); let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, 3600); - let claims = verify_with_key(&token, &mb).unwrap(); + let claims = verify_with_key(&token, &mb, TEST_AUD).unwrap(); assert_eq!(claims.sub, "did:plc:alice"); } #[test] fn garbage_token_is_invalid() { let (_, mb) = test_key(); - let err = verify_with_key("not-a-jwt", &mb).unwrap_err(); + let err = verify_with_key("not-a-jwt", &mb, TEST_AUD).unwrap_err(); assert!(matches!(err, AuthError::Invalid(_))); } @@ -552,7 +628,7 @@ mod tests { let (_, other_mb) = test_key(); let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, 3600); assert!(matches!( - verify_with_key(&token, &other_mb).unwrap_err(), + verify_with_key(&token, &other_mb, TEST_AUD).unwrap_err(), AuthError::Invalid(_) )); } @@ -563,7 +639,7 @@ mod tests { // days. Without the scope check it would be a session token. let (kp, mb) = test_key(); let token = mint(&kp, "did:plc:alice", "com.atproto.refresh", 3600); - let err = verify_with_key(&token, &mb).unwrap_err(); + let err = verify_with_key(&token, &mb, TEST_AUD).unwrap_err(); match err { AuthError::Invalid(msg) => assert!(msg.contains("com.atproto.refresh")), other => panic!("expected Invalid, got {other:?}"), @@ -576,7 +652,7 @@ mod tests { let (kp, mb) = test_key(); let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, -120); assert!(matches!( - verify_with_key(&token, &mb).unwrap_err(), + verify_with_key(&token, &mb, TEST_AUD).unwrap_err(), AuthError::Invalid(_) )); } @@ -631,13 +707,13 @@ mod tests { #[test] fn did_doc_url_is_built_from_the_base_url() { assert_eq!( - PdsKeys::new("http://127.0.0.1:2583").did_doc_url(), + PdsKeys::new("http://127.0.0.1:2583", TEST_AUD).did_doc_url(), "http://127.0.0.1:2583/.well-known/did.json" ); // A trailing slash must not produce a double slash — some // servers 404 on it. assert_eq!( - PdsKeys::new("http://pds:3000/").did_doc_url(), + PdsKeys::new("http://pds:3000/", TEST_AUD).did_doc_url(), "http://pds:3000/.well-known/did.json" ); } @@ -646,7 +722,7 @@ mod tests { async fn verification_fails_closed_when_the_pds_is_unreachable() { // Port 1 on loopback: nothing listens there, so the fetch fails // fast. The result must be a 503, never a pass-through. - let keys = PdsKeys::new("http://127.0.0.1:1"); + let keys = PdsKeys::new("http://127.0.0.1:1", TEST_AUD); let err = keys.verify_access_token("whatever").await.unwrap_err(); assert!( matches!(err, AuthError::Unavailable(_)), diff --git a/crates/appview/tests/common/mod.rs b/crates/appview/tests/common/mod.rs index 1d71881..fb19246 100644 --- a/crates/appview/tests/common/mod.rs +++ b/crates/appview/tests/common/mod.rs @@ -29,11 +29,20 @@ use at_crypto::ecdsa::P256Keypair; use at_crypto::jwt::{issue_jwt, JwtClaims}; -/// Audience the PDS stamps into access tokens. Not validated by -/// `verify_jwt` today (`validate_aud = false`), but minting a token -/// that differs from the real thing would make this helper a poor -/// stand-in for the client. -const APPVIEW_AUD: &str = "did:web:appview.maarcadetweet.local"; +/// Audience the PDS stamps into access tokens — and, since the +/// audience check landed, the value the AppView insists on: its own +/// service DID, derived from `APPVIEW_PUBLIC_URL`. A token minted with +/// anything else is rejected as `TokenInvalid`, which is exactly what +/// we want a wrong value here to look like. +/// +/// Derived the same way `AppConfig::appview_did()` does it, from the +/// same environment variable, so this helper can't drift from the +/// service it's impersonating the PDS for. +fn appview_aud() -> String { + let url = std::env::var("APPVIEW_PUBLIC_URL") + .unwrap_or_else(|_| "http://127.0.0.1:2584".to_string()); + at_shared::config::did_web_from_url(&url) +} /// The scope the AppView insists on. A token with any other scope — /// `com.atproto.refresh`, say — is rejected with `TokenInvalid`. @@ -156,7 +165,7 @@ pub fn mint_access_jwt( &JwtClaims { iss: "did:web:test".into(), sub: did.to_string(), - aud: APPVIEW_AUD.into(), + aud: appview_aud(), iat: now - 1, exp: now + ttl_secs, jti: None, diff --git a/crates/at-shared/src/config.rs b/crates/at-shared/src/config.rs index b2390a9..4d0c1f2 100644 --- a/crates/at-shared/src/config.rs +++ b/crates/at-shared/src/config.rs @@ -138,6 +138,19 @@ impl AppConfig { did_web_from_url(&self.pds_public_url) } + /// The AppView's own service DID, derived from `APPVIEW_PUBLIC_URL`. + /// + /// Also one derivation, two consumers: the PDS stamps it into the + /// `aud` of every access token it issues, and the AppView checks + /// incoming tokens against it. A token minted for a *different* + /// AppView must not be usable here — that's the whole point of an + /// audience — so both sides have to agree on the spelling, and the + /// only way to guarantee that is to compute it the same way from + /// the same configuration. + pub fn appview_did(&self) -> String { + did_web_from_url(&self.appview_public_url) + } + /// Base URL the AppView uses to reach the PDS. /// /// `PDS_INTERNAL_URL` when set (the cluster-internal hostname), diff --git a/crates/pds-server/src/jwt_issuer.rs b/crates/pds-server/src/jwt_issuer.rs index c613583..948659a 100644 --- a/crates/pds-server/src/jwt_issuer.rs +++ b/crates/pds-server/src/jwt_issuer.rs @@ -47,7 +47,10 @@ pub fn issue_access_jwt( // did:web resolver could follow. iss: cfg.pds_did(), sub: did.to_string(), - aud: "did:web:appview.maarcadetweet.local".into(), + // The AppView this token is meant for. Derived from + // `APPVIEW_PUBLIC_URL` rather than hardcoded, so the AppView can + // check it against its own identity (`AppConfig::appview_did`). + aud: cfg.appview_did(), iat: now, exp, jti: Some(uuid::Uuid::new_v4().to_string()),