feat(auth): Audience der Access-Tokens prüfen
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HC9HLrUU1LNwkzp8nkDLX
This commit is contained in:
co-authored by
Claude Opus 5
parent
f7b78fd5db
commit
6fd046417a
+94
-18
@@ -157,12 +157,16 @@ pub struct PdsKeys {
|
|||||||
http: reqwest::Client,
|
http: reqwest::Client,
|
||||||
/// Fully-qualified URL of the PDS's DID document.
|
/// Fully-qualified URL of the PDS's DID document.
|
||||||
did_doc_url: String,
|
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<CachedKey>,
|
inner: RwLock<CachedKey>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl PdsKeys {
|
impl PdsKeys {
|
||||||
/// Build a cache pointed at `base_url` (no trailing slash required).
|
/// Build a cache pointed at `base_url` (no trailing slash required),
|
||||||
pub fn new(base_url: &str) -> Self {
|
/// accepting only tokens addressed to `expected_aud`.
|
||||||
|
pub fn new(base_url: &str, expected_aud: impl Into<String>) -> Self {
|
||||||
let http = reqwest::Client::builder()
|
let http = reqwest::Client::builder()
|
||||||
.timeout(DID_DOC_TIMEOUT)
|
.timeout(DID_DOC_TIMEOUT)
|
||||||
.build()
|
.build()
|
||||||
@@ -173,6 +177,7 @@ impl PdsKeys {
|
|||||||
"{}/.well-known/did.json",
|
"{}/.well-known/did.json",
|
||||||
base_url.trim_end_matches('/')
|
base_url.trim_end_matches('/')
|
||||||
),
|
),
|
||||||
|
expected_aud: expected_aud.into(),
|
||||||
inner: RwLock::new(CachedKey::default()),
|
inner: RwLock::new(CachedKey::default()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -180,7 +185,7 @@ impl PdsKeys {
|
|||||||
/// Same PDS the handle-sync worker talks to: `PDS_INTERNAL_URL`
|
/// Same PDS the handle-sync worker talks to: `PDS_INTERNAL_URL`
|
||||||
/// when set, else `PDS_PUBLIC_URL`.
|
/// when set, else `PDS_PUBLIC_URL`.
|
||||||
pub fn from_config(cfg: &at_shared::config::AppConfig) -> Self {
|
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 {
|
pub fn did_doc_url(&self) -> &str {
|
||||||
@@ -267,13 +272,13 @@ impl PdsKeys {
|
|||||||
/// restart.
|
/// restart.
|
||||||
pub async fn verify_access_token(&self, token: &str) -> Result<JwtClaims, AuthError> {
|
pub async fn verify_access_token(&self, token: &str) -> Result<JwtClaims, AuthError> {
|
||||||
let key = self.key_or_fetch().await?;
|
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),
|
Ok(claims) => Ok(claims),
|
||||||
Err(first) => {
|
Err(first) => {
|
||||||
let Some(fresh) = self.refetch_if_stale(&key).await else {
|
let Some(fresh) = self.refetch_if_stale(&key).await else {
|
||||||
return Err(first);
|
return Err(first);
|
||||||
};
|
};
|
||||||
verify_with_key(token, &fresh).map_err(|_| first)
|
verify_with_key(token, &fresh, &self.expected_aud).map_err(|_| first)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -309,17 +314,45 @@ fn extract_public_key_multibase(doc: &Value) -> anyhow::Result<String> {
|
|||||||
/// leeway for clock skew); the scope check is ours, and it is the line
|
/// 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
|
/// that keeps a 90-day refresh token from working as a session
|
||||||
/// credential.
|
/// credential.
|
||||||
fn verify_with_key(token: &str, pubkey_multibase: &str) -> Result<JwtClaims, AuthError> {
|
fn verify_with_key(
|
||||||
|
token: &str,
|
||||||
|
pubkey_multibase: &str,
|
||||||
|
expected_aud: &str,
|
||||||
|
) -> Result<JwtClaims, AuthError> {
|
||||||
let claims = at_crypto::jwt::verify_jwt(token, pubkey_multibase)
|
let claims = at_crypto::jwt::verify_jwt(token, pubkey_multibase)
|
||||||
.map_err(|e| AuthError::Invalid(format!("invalid token: {e}")))?;
|
.map_err(|e| AuthError::Invalid(format!("invalid token: {e}")))?;
|
||||||
match claims.scope.as_deref() {
|
match claims.scope.as_deref() {
|
||||||
Some(ACCESS_SCOPE) => Ok(claims),
|
Some(ACCESS_SCOPE) => {}
|
||||||
other => Err(AuthError::Invalid(format!(
|
other => {
|
||||||
|
return Err(AuthError::Invalid(format!(
|
||||||
"token scope {:?} is not {ACCESS_SCOPE}",
|
"token scope {:?} is not {ACCESS_SCOPE}",
|
||||||
other.unwrap_or("<none>")
|
other.unwrap_or("<none>")
|
||||||
))),
|
)))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// 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.
|
/// Extract the bearer token from an `Authorization` header.
|
||||||
///
|
///
|
||||||
@@ -466,14 +499,29 @@ mod tests {
|
|||||||
(kp, multibase)
|
(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 {
|
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();
|
let now = chrono::Utc::now().timestamp();
|
||||||
issue_jwt(
|
issue_jwt(
|
||||||
kp,
|
kp,
|
||||||
&JwtClaims {
|
&JwtClaims {
|
||||||
iss: "did:web:127.0.0.1%3A2583".into(),
|
iss: "did:web:127.0.0.1%3A2583".into(),
|
||||||
sub: did.into(),
|
sub: did.into(),
|
||||||
aud: "did:web:appview.maarcadetweet.local".into(),
|
aud: aud.into(),
|
||||||
iat: now - 1,
|
iat: now - 1,
|
||||||
exp: now + ttl_secs,
|
exp: now + ttl_secs,
|
||||||
jti: None,
|
jti: None,
|
||||||
@@ -511,6 +559,34 @@ mod tests {
|
|||||||
assert_eq!(bearer_token(&header_map("Bearer tok")).unwrap(), "tok");
|
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]
|
#[test]
|
||||||
fn error_bodies_carry_the_documented_codes() {
|
fn error_bodies_carry_the_documented_codes() {
|
||||||
// These strings are a contract: the desktop client keys its
|
// These strings are a contract: the desktop client keys its
|
||||||
@@ -535,14 +611,14 @@ mod tests {
|
|||||||
fn valid_access_token_verifies() {
|
fn valid_access_token_verifies() {
|
||||||
let (kp, mb) = test_key();
|
let (kp, mb) = test_key();
|
||||||
let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, 3600);
|
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");
|
assert_eq!(claims.sub, "did:plc:alice");
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn garbage_token_is_invalid() {
|
fn garbage_token_is_invalid() {
|
||||||
let (_, mb) = test_key();
|
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(_)));
|
assert!(matches!(err, AuthError::Invalid(_)));
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -552,7 +628,7 @@ mod tests {
|
|||||||
let (_, other_mb) = test_key();
|
let (_, other_mb) = test_key();
|
||||||
let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, 3600);
|
let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, 3600);
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
verify_with_key(&token, &other_mb).unwrap_err(),
|
verify_with_key(&token, &other_mb, TEST_AUD).unwrap_err(),
|
||||||
AuthError::Invalid(_)
|
AuthError::Invalid(_)
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
@@ -563,7 +639,7 @@ mod tests {
|
|||||||
// days. Without the scope check it would be a session token.
|
// days. Without the scope check it would be a session token.
|
||||||
let (kp, mb) = test_key();
|
let (kp, mb) = test_key();
|
||||||
let token = mint(&kp, "did:plc:alice", "com.atproto.refresh", 3600);
|
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 {
|
match err {
|
||||||
AuthError::Invalid(msg) => assert!(msg.contains("com.atproto.refresh")),
|
AuthError::Invalid(msg) => assert!(msg.contains("com.atproto.refresh")),
|
||||||
other => panic!("expected Invalid, got {other:?}"),
|
other => panic!("expected Invalid, got {other:?}"),
|
||||||
@@ -576,7 +652,7 @@ mod tests {
|
|||||||
let (kp, mb) = test_key();
|
let (kp, mb) = test_key();
|
||||||
let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, -120);
|
let token = mint(&kp, "did:plc:alice", ACCESS_SCOPE, -120);
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
verify_with_key(&token, &mb).unwrap_err(),
|
verify_with_key(&token, &mb, TEST_AUD).unwrap_err(),
|
||||||
AuthError::Invalid(_)
|
AuthError::Invalid(_)
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
@@ -631,13 +707,13 @@ mod tests {
|
|||||||
#[test]
|
#[test]
|
||||||
fn did_doc_url_is_built_from_the_base_url() {
|
fn did_doc_url_is_built_from_the_base_url() {
|
||||||
assert_eq!(
|
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"
|
"http://127.0.0.1:2583/.well-known/did.json"
|
||||||
);
|
);
|
||||||
// A trailing slash must not produce a double slash — some
|
// A trailing slash must not produce a double slash — some
|
||||||
// servers 404 on it.
|
// servers 404 on it.
|
||||||
assert_eq!(
|
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"
|
"http://pds:3000/.well-known/did.json"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
@@ -646,7 +722,7 @@ mod tests {
|
|||||||
async fn verification_fails_closed_when_the_pds_is_unreachable() {
|
async fn verification_fails_closed_when_the_pds_is_unreachable() {
|
||||||
// Port 1 on loopback: nothing listens there, so the fetch fails
|
// Port 1 on loopback: nothing listens there, so the fetch fails
|
||||||
// fast. The result must be a 503, never a pass-through.
|
// 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();
|
let err = keys.verify_access_token("whatever").await.unwrap_err();
|
||||||
assert!(
|
assert!(
|
||||||
matches!(err, AuthError::Unavailable(_)),
|
matches!(err, AuthError::Unavailable(_)),
|
||||||
|
|||||||
@@ -29,11 +29,20 @@
|
|||||||
use at_crypto::ecdsa::P256Keypair;
|
use at_crypto::ecdsa::P256Keypair;
|
||||||
use at_crypto::jwt::{issue_jwt, JwtClaims};
|
use at_crypto::jwt::{issue_jwt, JwtClaims};
|
||||||
|
|
||||||
/// Audience the PDS stamps into access tokens. Not validated by
|
/// Audience the PDS stamps into access tokens — and, since the
|
||||||
/// `verify_jwt` today (`validate_aud = false`), but minting a token
|
/// audience check landed, the value the AppView insists on: its own
|
||||||
/// that differs from the real thing would make this helper a poor
|
/// service DID, derived from `APPVIEW_PUBLIC_URL`. A token minted with
|
||||||
/// stand-in for the client.
|
/// anything else is rejected as `TokenInvalid`, which is exactly what
|
||||||
const APPVIEW_AUD: &str = "did:web:appview.maarcadetweet.local";
|
/// 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 —
|
/// The scope the AppView insists on. A token with any other scope —
|
||||||
/// `com.atproto.refresh`, say — is rejected with `TokenInvalid`.
|
/// `com.atproto.refresh`, say — is rejected with `TokenInvalid`.
|
||||||
@@ -156,7 +165,7 @@ pub fn mint_access_jwt(
|
|||||||
&JwtClaims {
|
&JwtClaims {
|
||||||
iss: "did:web:test".into(),
|
iss: "did:web:test".into(),
|
||||||
sub: did.to_string(),
|
sub: did.to_string(),
|
||||||
aud: APPVIEW_AUD.into(),
|
aud: appview_aud(),
|
||||||
iat: now - 1,
|
iat: now - 1,
|
||||||
exp: now + ttl_secs,
|
exp: now + ttl_secs,
|
||||||
jti: None,
|
jti: None,
|
||||||
|
|||||||
@@ -138,6 +138,19 @@ impl AppConfig {
|
|||||||
did_web_from_url(&self.pds_public_url)
|
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.
|
/// Base URL the AppView uses to reach the PDS.
|
||||||
///
|
///
|
||||||
/// `PDS_INTERNAL_URL` when set (the cluster-internal hostname),
|
/// `PDS_INTERNAL_URL` when set (the cluster-internal hostname),
|
||||||
|
|||||||
@@ -47,7 +47,10 @@ pub fn issue_access_jwt(
|
|||||||
// did:web resolver could follow.
|
// did:web resolver could follow.
|
||||||
iss: cfg.pds_did(),
|
iss: cfg.pds_did(),
|
||||||
sub: did.to_string(),
|
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,
|
iat: now,
|
||||||
exp,
|
exp,
|
||||||
jti: Some(uuid::Uuid::new_v4().to_string()),
|
jti: Some(uuid::Uuid::new_v4().to_string()),
|
||||||
|
|||||||
Reference in New Issue
Block a user