From 78f1e2672cf2daee9c5b4eab563c8d8f1b778acd Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Fri, 11 Sep 2026 19:38:50 -0400 Subject: [PATCH] fix(oauth)!: ask the gate before fetching a client_id, and bound the metadata cache `POST /oauth/par` fetched a document from a host the unauthenticated caller named before the policy gate was asked, cached every distinct `client_id` in a map nothing evicted, and reached loopback on any deployment. The gate now judges first, the cache holds at most 512 entries, and only a `.localhost` zone fetches a loopback client. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I6e45f03737f032ac3f9e4890b36eaf71ae4c2ea7 --- crates/didbot-serve/src/bin/didbot-pds.rs | 3 + .../didbot-serve/src/oauth/client_metadata.rs | 193 +++++++++++++++++- crates/didbot-serve/src/oauth/par.rs | 96 +++++++-- crates/didbot-serve/src/routes.rs | 1 + crates/didbot-serve/src/tests.rs | 1 + 5 files changed, 269 insertions(+), 25 deletions(-) diff --git a/crates/didbot-serve/src/bin/didbot-pds.rs b/crates/didbot-serve/src/bin/didbot-pds.rs index 24303a5b..278cc77d 100644 --- a/crates/didbot-serve/src/bin/didbot-pds.rs +++ b/crates/didbot-serve/src/bin/didbot-pds.rs @@ -2706,6 +2706,9 @@ mod config_tests { evaluation_log: oauth.evaluation_log.as_ref(), pds_did: "did:web:pds.example", pds_hostname: "pds.example", + loopback_clients: didbot_serve::oauth::client_metadata::LoopbackClients::for_zone( + "pds.example", + ), }; let err = push_authorization_request( &oauth.http, diff --git a/crates/didbot-serve/src/oauth/client_metadata.rs b/crates/didbot-serve/src/oauth/client_metadata.rs index 2d3bcd9f..f8493017 100644 --- a/crates/didbot-serve/src/oauth/client_metadata.rs +++ b/crates/didbot-serve/src/oauth/client_metadata.rs @@ -38,6 +38,17 @@ use time::{Duration, OffsetDateTime}; /// deployment's whole uptime. pub const CACHE_TTL: Duration = Duration::minutes(15); +/// How many distinct `client_id`s [`MemoryClientMetadataCache`] holds at +/// once. +/// +/// `POST /oauth/par` is unauthenticated and the `client_id` it names is the +/// caller's to choose, so every distinct string a caller invents is a +/// cacheable entry. Without a bound that map only ever grows, which is a way +/// to spend this server's memory while holding no credential. Past it the +/// cache drops what it has held longest, so a spree of invented ids costs a +/// re-fetch for the real clients rather than the process. +pub const CACHE_CAPACITY: usize = 512; + /// How long the whole client metadata fetch may take — connect, headers and /// body together, not just the handshake. /// @@ -176,6 +187,38 @@ fn is_loopback_host(host: &str) -> bool { .is_ok_and(|ip| ip.is_loopback()) } +/// Whether this deployment will fetch a client metadata document over +/// loopback `http://`. +/// +/// The atproto profile's "Localhost Client Development" carve-out lets a +/// `client_id` be `http://localhost:/...`, which this server would +/// then fetch. On a real deployment that is an outbound GET to a port of the +/// caller's choosing on the machine running the PDS, with the outcome +/// readable from the error it answers: a port scanner reachable from an +/// unauthenticated endpoint. A deployment under `.localhost` is a +/// development one and needs the carve-out; every other deployment refuses +/// it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LoopbackClients { + /// Fetch them — a development deployment. + Fetched, + /// Refuse them. + Refused, +} + +impl LoopbackClients { + /// What a deployment serving `zone` does. `.localhost` and `localhost` + /// itself are development zones; nothing else is. + pub fn for_zone(zone: &str) -> Self { + let zone = zone.trim_end_matches('.').to_ascii_lowercase(); + if zone == "localhost" || zone.ends_with(".localhost") { + Self::Fetched + } else { + Self::Refused + } + } +} + /// Checks a URL is `https://` (or loopback `http://`) with no fragment. fn validate_url_scheme(raw: &str) -> Result { let url = reqwest::Url::parse(raw).map_err(|_| ())?; @@ -375,11 +418,30 @@ impl ClientMetadataCache for MemoryClientMetadataCache { Some(entry.clone()) } + /// Stores the entry, dropping expired ones first and then the entry + /// held longest if the cache is still at [`CACHE_CAPACITY`]. + /// + /// Swept on write, the way `oauth::par`'s and `oauth::consent`'s own + /// stores sweep theirs: no background task, and a caller's cost does not + /// grow with how many other callers there are. fn put(&self, client_id: &str, entry: CachedClientMetadata) { - self.entries + let now = OffsetDateTime::now_utc(); + let mut entries = self + .entries .write() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .insert(client_id.to_owned(), entry); + .unwrap_or_else(|poisoned| poisoned.into_inner()); + entries.retain(|_, held| now - held.fetched_at <= CACHE_TTL); + while entries.len() >= CACHE_CAPACITY && !entries.contains_key(client_id) { + let Some(oldest) = entries + .iter() + .min_by_key(|(_, held)| held.fetched_at) + .map(|(id, _)| id.clone()) + else { + break; + }; + entries.remove(&oldest); + } + entries.insert(client_id.to_owned(), entry); } } @@ -456,8 +518,12 @@ pub async fn resolve_client( http: &reqwest::Client, cache: &dyn ClientMetadataCache, client_id: &str, + loopback: LoopbackClients, ) -> Result<(ClientMetadataDocument, ClientKey), ClientMetadataError> { if let Some(built) = parse_loopback_client_id(client_id) { + if loopback == LoopbackClients::Refused { + return Err(ClientMetadataError::NotHttps(client_id.to_owned())); + } let document = validate(client_id, built?)?; let key = ClientKey::compute(&document); return Ok((document, key)); @@ -467,6 +533,11 @@ pub async fn resolve_client( } let url = validate_url_scheme(client_id) .map_err(|()| ClientMetadataError::NotHttps(client_id.to_owned()))?; + // Before the fetch, and answering exactly what a non-HTTPS `client_id` + // answers: this deployment does not reach loopback on a caller's say-so. + if url.scheme() == "http" && loopback == LoopbackClients::Refused { + return Err(ClientMetadataError::NotHttps(client_id.to_owned())); + } if url.fragment().is_some() { return Err(ClientMetadataError::HasFragment(client_id.to_owned())); } @@ -846,7 +917,7 @@ mod tests { let client = crate::oauth::client_metadata_http_client(); let client_id = format!("http://{addr}/client-metadata.json"); let started = tokio::time::Instant::now(); - let resolving = resolve_client(&client, &cache, &client_id); + let resolving = resolve_client(&client, &cache, &client_id, LoopbackClients::Fetched); let err = tokio::time::timeout(FETCH_TIMEOUT * 2, resolving) .await .expect("the fetch's own timeout fires first") @@ -865,6 +936,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, "http://localhost/?redirect_uri=http%3A%2F%2F127.0.0.1%3A9%2Fcb&scope=atproto", + LoopbackClients::Fetched, ) .await .expect("the loopback form resolves without a fetch"); @@ -900,6 +972,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &client_id, + LoopbackClients::Fetched, ) .await .expect("the fetchable localhost: development form still fetches"); @@ -915,6 +988,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &client_id, + LoopbackClients::Fetched, ) .await .expect("an ordinary document resolves"); @@ -931,6 +1005,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &format!("{base}/moved.json"), + LoopbackClients::Fetched, ) .await .expect_err("a redirect is not a client metadata document"); @@ -952,6 +1027,7 @@ mod tests { &didbot_http::client(), &cache, &format!("{base}/moved.json"), + LoopbackClients::Fetched, ) .await .expect_err("the document did not come from the client_id"); @@ -971,6 +1047,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &format!("{base}/huge.json"), + LoopbackClients::Fetched, ) .await .expect_err("a document over the cap is not read"); @@ -988,6 +1065,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &format!("{base}/id.json"), + LoopbackClients::Fetched, ) .await .expect_err("a body that outgrows the cap is abandoned"); @@ -997,6 +1075,112 @@ mod tests { ); } + /// **The port oracle this closes.** A `client_id` naming a loopback + /// host makes this server fetch a port of the caller's choosing on the + /// machine running it, and answer differently depending on what it + /// found. `POST /oauth/par` is unauthenticated, so on a deployment that + /// is not itself a development one that is a port scanner anyone can + /// drive. Refused before the fetch, with the same answer a plain + /// `http://` `client_id` gets, so nothing is learned from the wording + /// either. + #[tokio::test] + async fn a_loopback_client_id_is_refused_before_the_fetch_off_a_localhost_zone() { + let base = client_host().await; + let cache = MemoryClientMetadataCache::new(); + let client_id = format!("{base}/id.json"); + let err = resolve_client( + &crate::oauth::client_metadata_http_client(), + &cache, + &client_id, + LoopbackClients::Refused, + ) + .await + .expect_err("a real deployment does not fetch loopback on a caller's say-so"); + assert!(matches!(err, ClientMetadataError::NotHttps(_)), "{err:?}"); + assert!(cache.get(&client_id).is_none()); + + // The development form that needs no fetch at all is refused the + // same way, so neither shape is a way in. + let built = resolve_client( + &crate::oauth::client_metadata_http_client(), + &cache, + "http://localhost/?redirect_uri=http%3A%2F%2F127.0.0.1%3A9%2Fcb&scope=atproto", + LoopbackClients::Refused, + ) + .await; + assert!(matches!(built, Err(ClientMetadataError::NotHttps(_)))); + } + + #[test] + fn only_a_localhost_zone_fetches_loopback_clients() { + for zone in [ + "localhost", + "agents.localhost", + "AGENTS.LOCALHOST", + "a.b.localhost.", + ] { + assert_eq!( + LoopbackClients::for_zone(zone), + LoopbackClients::Fetched, + "{zone}" + ); + } + for zone in ["pds.did.bot", "localhost.example", "example.com", ""] { + assert_eq!( + LoopbackClients::for_zone(zone), + LoopbackClients::Refused, + "{zone}" + ); + } + } + + /// **The unbounded map this closes.** Every distinct `client_id` an + /// unauthenticated caller invents was its own entry, in a map nothing + /// ever removed from. Past [`CACHE_CAPACITY`] the entry held longest + /// goes, so the footprint is the bound rather than the caller's + /// imagination. + #[test] + fn the_cache_never_holds_more_than_its_capacity() { + let cache = MemoryClientMetadataCache::new(); + let document = ClientMetadataDocument { + client_id: "https://client.example/id.json".to_owned(), + client_name: None, + client_uri: None, + logo_uri: None, + redirect_uris: vec!["https://client.example/cb".to_owned()], + grant_types: vec!["authorization_code".to_owned()], + response_types: vec!["code".to_owned()], + token_endpoint_auth_method: "none".to_owned(), + scope: Some("atproto".to_owned()), + dpop_bound_access_tokens: true, + }; + let key = ClientKey::compute(&document); + let start = OffsetDateTime::now_utc(); + for n in 0..(CACHE_CAPACITY * 2) { + cache.put( + &format!("https://invented-{n}.example/id.json"), + CachedClientMetadata { + document: document.clone(), + key: key.clone(), + // Distinct, increasing, so "held longest" is well defined. + fetched_at: start + Duration::seconds(n as i64), + }, + ); + } + assert_eq!( + cache.entries.read().unwrap().len(), + CACHE_CAPACITY, + "the cache grew past its bound" + ); + assert!(cache.get("https://invented-0.example/id.json").is_none()); + assert!(cache + .get(&format!( + "https://invented-{}.example/id.json", + CACHE_CAPACITY * 2 - 1 + )) + .is_some()); + } + /// Nothing is cached from a fetch that was refused — a refusal must not /// become a poisoned entry a later, legitimate fetch reads back. #[tokio::test] @@ -1008,6 +1192,7 @@ mod tests { &crate::oauth::client_metadata_http_client(), &cache, &client_id, + LoopbackClients::Fetched, ) .await; assert!(cache.get(&client_id).is_none()); diff --git a/crates/didbot-serve/src/oauth/par.rs b/crates/didbot-serve/src/oauth/par.rs index 1d377728..4482f451 100644 --- a/crates/didbot-serve/src/oauth/par.rs +++ b/crates/didbot-serve/src/oauth/par.rs @@ -299,6 +299,9 @@ pub struct DecisionSeams<'a> { pub pds_did: &'a str, /// This deployment's own hostname, for the same subject. pub pds_hostname: &'a str, + /// Whether this deployment fetches a loopback `http://` client metadata + /// document; see [`crate::oauth::client_metadata::LoopbackClients`]. + pub loopback_clients: crate::oauth::client_metadata::LoopbackClients, } /// Handles one pushed authorization request end to end: resolves and @@ -367,27 +370,6 @@ pub async fn push_authorization_request( Some(_) => return Err(ParError::UnsupportedResponseType), } let client_id = didbot_pds::policy::ClientId::parse(&request.client_id)?; - let (document, client_key) = - crate::oauth::client_metadata::resolve_client(http, cache, client_id.as_str()).await?; - - let redirect_uri = request - .redirect_uri - .ok_or(ParError::MissingParameter("redirect_uri"))?; - if !document.redirect_uris.iter().any(|u| u == &redirect_uri) { - return Err(ParError::RedirectUriNotRegistered(redirect_uri)); - } - - let code_challenge = request - .code_challenge - .ok_or(ParError::MissingParameter("code_challenge"))?; - let code_challenge_method = request - .code_challenge_method - .ok_or(ParError::MissingParameter("code_challenge_method"))?; - if code_challenge_method != "S256" { - return Err(ParError::UnsupportedCodeChallengeMethod( - code_challenge_method, - )); - } let scope_str = request.scope.ok_or(ParError::MissingParameter("scope"))?; let scope = ScopeSet::parse(&scope_str)?; @@ -402,6 +384,11 @@ pub async fn push_authorization_request( // reused by [`decide`] for its own row in the evaluation log, rather // than rebuilt there, so a caller of this function is not the only // thing this judgment is asked on behalf of exactly once. + // + // **Asked before the document is fetched.** Everything it judges — + // `client_id` and the requested scope — is in the request itself, and + // the fetch is an outbound request to a host this unauthenticated caller + // named. A client the gate refuses should not cost this deployment one. let atoms: Vec = scope.0.iter().map(ToString::to_string).collect(); let borrowed: Vec<&str> = atoms.iter().map(String::as_str).collect(); let subject = @@ -415,6 +402,33 @@ pub async fn push_authorization_request( return Err(ParError::ClientRefused); } + let (document, client_key) = crate::oauth::client_metadata::resolve_client( + http, + cache, + client_id.as_str(), + seams.loopback_clients, + ) + .await?; + + let redirect_uri = request + .redirect_uri + .ok_or(ParError::MissingParameter("redirect_uri"))?; + if !document.redirect_uris.iter().any(|u| u == &redirect_uri) { + return Err(ParError::RedirectUriNotRegistered(redirect_uri)); + } + + let code_challenge = request + .code_challenge + .ok_or(ParError::MissingParameter("code_challenge"))?; + let code_challenge_method = request + .code_challenge_method + .ok_or(ParError::MissingParameter("code_challenge_method"))?; + if code_challenge_method != "S256" { + return Err(ParError::UnsupportedCodeChallengeMethod( + code_challenge_method, + )); + } + let now = OffsetDateTime::now_utc(); let expires_at = now + REQUEST_URI_TTL; @@ -809,6 +823,9 @@ mod tests { evaluation_log: self.log.as_ref(), pds_did: "did:web:pds.example", pds_hostname: "pds.example", + loopback_clients: crate::oauth::client_metadata::LoopbackClients::for_zone( + "pds.example", + ), } } } @@ -857,6 +874,43 @@ mod tests { ); } + /// **The outbound request this closes.** `POST /oauth/par` is + /// unauthenticated and `client_id` is a URL the caller chose, so a fetch + /// before the gate is asked lets anybody make this server reach a host + /// of their choosing — even one the operator has denied outright. The + /// gate judges `client_id` and the requested scope, both of which are in + /// the request, so it is asked first and a refused client costs no + /// outbound request at all. + /// + /// Measured on the clock: the `client_id` names a host that accepts and + /// never answers, so a fetch would cost `FETCH_TIMEOUT`. + #[tokio::test(start_paused = true)] + async fn a_denied_client_is_refused_before_its_metadata_is_fetched() { + let addr = didbot_http::test_support::hung_server(); + let fixture = Fixture::denying_client("^http://.*$"); + let store = MemoryParStore::new(); + let mut hung = request(); + hung.client_id = format!("http://{addr}/id.json"); + + let started = tokio::time::Instant::now(); + let err = push_authorization_request( + &crate::oauth::client_metadata_http_client(), + &MemoryClientMetadataCache::new(), + &store, + hung, + &resolves, + &fixture.seams(&GrantAnyScope), + ) + .await + .unwrap_err(); + assert_eq!(err, ParError::ClientRefused); + assert_eq!( + started.elapsed(), + std::time::Duration::ZERO, + "the document was fetched before the gate was asked" + ); + } + #[tokio::test] async fn an_unregistered_redirect_uri_is_refused_even_when_admitted() { let fixture = Fixture::new(); diff --git a/crates/didbot-serve/src/routes.rs b/crates/didbot-serve/src/routes.rs index e4da9a7b..e1fde4e5 100644 --- a/crates/didbot-serve/src/routes.rs +++ b/crates/didbot-serve/src/routes.rs @@ -1277,6 +1277,7 @@ async fn oauth_par( evaluation_log: state.oauth.evaluation_log.as_ref(), pds_did: &pds_did, pds_hostname: &pds_hostname, + loopback_clients: crate::oauth::client_metadata::LoopbackClients::for_zone(&pds_hostname), }; match push_authorization_request( &state.oauth.http, diff --git a/crates/didbot-serve/src/tests.rs b/crates/didbot-serve/src/tests.rs index 5d253c1b..7b3114f2 100644 --- a/crates/didbot-serve/src/tests.rs +++ b/crates/didbot-serve/src/tests.rs @@ -6308,6 +6308,7 @@ fn par_from(caller: &str) -> Request { "client_id=http%3A%2F%2Fclient.example%2Fid.json\ &response_type=code\ &redirect_uri=http%3A%2F%2Fclient.example%2Fcb\ + &login_hint=did%3Aweb%3Aagent.example\ &scope=atproto&code_challenge=x&code_challenge_method=S256", )) .expect("request builds") -- 2.51.2