diff --git a/crates/buzz-auth/Cargo.toml b/crates/buzz-auth/Cargo.toml index 158d282cd6..6cbe491e2c 100644 --- a/crates/buzz-auth/Cargo.toml +++ b/crates/buzz-auth/Cargo.toml @@ -14,6 +14,7 @@ dev = [] [dev-dependencies] # `use_pem` enables EncodingKey::from_ec_pem for minting ES256 test assertions. jsonwebtoken = { version = "10.4.0", default-features = false, features = ["aws_lc_rs", "use_pem"] } +tokio = { workspace = true, features = ["test-util"] } [dependencies] buzz-core = { workspace = true } diff --git a/crates/buzz-auth/src/nip_fi/config.rs b/crates/buzz-auth/src/nip_fi/config.rs index 227e9e0dfd..5c264b00ee 100644 --- a/crates/buzz-auth/src/nip_fi/config.rs +++ b/crates/buzz-auth/src/nip_fi/config.rs @@ -561,8 +561,8 @@ impl IssuerRegistry { self.policies.is_empty() } - /// All registered issuer policies, in unspecified order. Useful for - /// iterating over every configured issuer during startup validation. + /// Iteration order is deliberately unspecified; callers must not depend on + /// registration order. pub fn all_policies(&self) -> impl Iterator { self.policies.values() } diff --git a/crates/buzz-auth/src/nip_fi/jwks/mod.rs b/crates/buzz-auth/src/nip_fi/jwks/mod.rs index e90dd37867..ef8b8297a7 100644 --- a/crates/buzz-auth/src/nip_fi/jwks/mod.rs +++ b/crates/buzz-auth/src/nip_fi/jwks/mod.rs @@ -47,8 +47,9 @@ pub const MAX_JWKS_RESPONSE_BYTES: usize = 512 * 1024; // 512 KiB /// range panics when computing snapshot deadlines. pub const MAX_JWKS_TIMING_SECONDS: u64 = 365 * 24 * 3600; // 1 year -/// Per-request deadline for the complete JWKS fetch (connect + headers + body), -/// enforced inside `fetch_jwks` independently of any client-level timeout. +/// Hard deadline for the complete JWKS fetch: hostname resolution, connect, +/// headers, and body streaming combined. Applied via `tokio::time::timeout` +/// so a stalled resolver cannot keep `fetch_jwks` pending indefinitely. pub const JWKS_REQUEST_TIMEOUT_SECS: u64 = 10; /// Validate that a JWKS URI is safe to fetch: HTTPS scheme, no credentials, @@ -89,14 +90,31 @@ pub fn validate_jwks_uri(uri: &str) -> Result<(), JwksFetchError> { /// Returns the first safe address for DNS pinning. Blocks on the OS resolver /// via `spawn_blocking` to avoid blocking the async runtime. /// +/// Uses the `(host, port)` tuple form of `ToSocketAddrs` — not +/// `format!("{host}:{port}")` — so IPv6 literal hosts (returned without +/// brackets by `Url::host_str()`) are handled correctly without socket-address +/// ambiguity. +/// /// Rejecting *any* resolved address (not just the first) closes split-horizon /// DNS attacks: if an attacker can cause one DNS record to resolve to a private /// address, the entire request is blocked even when other records are public. -async fn resolve_and_check_ssrf(host: &str, port: u16) -> Result { - let addr_str = format!("{host}:{port}"); +pub(crate) async fn resolve_and_check_ssrf( + host: &str, + port: u16, +) -> Result { + // Fast path: if the host is already a parsed IP literal, skip the resolver. + if let Ok(ip) = host.parse::() { + if is_private_ip(&ip) { + return Err(JwksFetchError::InvalidUri); + } + return Ok(ip); + } + + // Hostname path: use the tuple form to avoid IPv6-bracket ambiguity. + let host_owned = host.to_owned(); let addrs: Vec = tokio::task::spawn_blocking(move || { use std::net::ToSocketAddrs; - addr_str + (host_owned.as_str(), port) .to_socket_addrs() .map(|iter| iter.map(|sa| sa.ip()).collect::>()) }) @@ -191,9 +209,12 @@ pub enum JwksFetchError { /// may implement it — external types cannot name the private supertrait. /// /// Implementations MUST: +/// - validate the URI (scheme, credentials, fragment, bare private-IP host) +/// before any I/O; /// - resolve hostname targets and reject any private/reserved resolved address; /// - deny redirects (3xx responses rejected as `NetworkError`); -/// - enforce a finite per-fetch deadline ([`JWKS_REQUEST_TIMEOUT_SECS`]); +/// - enforce a finite per-fetch deadline covering resolution, connect, headers, +/// and body streaming — the entire operation must be bounded; /// - enforce [`MAX_JWKS_RESPONSE_BYTES`] via incremental streaming; /// - reject non-2xx responses. pub trait JwksFetcher: super::verifier::sealed::Sealed + Send + Sync + 'static { @@ -212,8 +233,8 @@ pub trait JwksFetcher: super::verifier::sealed::Sealed + Send + Sync + 'static { /// `buzz_core::network::is_private_ip` before the request is sent; /// - the request is pinned to the validated address to prevent DNS rebinding /// TOCTOU (the OS resolver is called once per fetch, not once per URL); -/// - a per-request timeout of [`JWKS_REQUEST_TIMEOUT_SECS`] is applied via -/// `RequestBuilder::timeout`; +/// - the complete operation (resolution, connect, headers, body streaming) is +/// bounded by [`JWKS_REQUEST_TIMEOUT_SECS`] via `tokio::time::timeout`; /// - 3xx responses are rejected as `NetworkError` — redirects are never followed; /// - the body is streamed incrementally and stopped at /// [`MAX_JWKS_RESPONSE_BYTES`] + 1. @@ -238,64 +259,88 @@ impl super::verifier::sealed::Sealed for HttpJwksFetcher {} impl JwksFetcher for HttpJwksFetcher { async fn fetch_jwks<'a>(&'a self, uri: &'a str) -> Result { - let parsed = Url::parse(uri).map_err(|_| JwksFetchError::InvalidUri)?; - let host = parsed.host_str().ok_or(JwksFetchError::InvalidUri)?; - let port = parsed.port_or_known_default().unwrap_or(443); - - // Resolve and check every IP before sending. Pins DNS to the validated - // address to prevent rebinding TOCTOU between check and connect. - let safe_ip = resolve_and_check_ssrf(host, port).await?; - - // Build a per-request client that: - // - denies redirects (a 3xx to an internal host bypasses the URI check); - // - has no system proxy (proxy would resolve the original hostname itself); - // - pins this request to the validated IP. - // The connection pool from self.client is not reused here by design — - // DNS pinning requires a fresh client for each pinned address. - let pinned_client = reqwest::Client::builder() - .redirect(reqwest::redirect::Policy::none()) - .no_proxy() - .resolve(host, std::net::SocketAddr::new(safe_ip, port)) - .build() - .map_err(|_| JwksFetchError::NetworkError)?; - - let response = pinned_client - .get(uri) - .timeout(std::time::Duration::from_secs(JWKS_REQUEST_TIMEOUT_SECS)) - .send() - .await - .map_err(|_| JwksFetchError::NetworkError)?; - - // Reject non-2xx. A 3xx here means our no-redirect policy was somehow - // bypassed — treat as a network error. - if !response.status().is_success() { - return Err(JwksFetchError::NetworkError); - } - - // Early-exit on Content-Length before streaming. A lying or absent - // Content-Length is caught by the incremental counter below. - if let Some(content_length) = response.content_length() { - if content_length as usize > MAX_JWKS_RESPONSE_BYTES { - return Err(JwksFetchError::ResponseTooLarge); - } - } - - // Stream incrementally; stop at MAX_JWKS_RESPONSE_BYTES + 1 so we - // never buffer more than the limit before rejecting. - let mut body = Vec::with_capacity(MAX_JWKS_RESPONSE_BYTES.min(64 * 1024)); - let mut stream = response.bytes_stream(); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|_| JwksFetchError::NetworkError)?; - if body.len().saturating_add(chunk.len()) > MAX_JWKS_RESPONSE_BYTES { - return Err(JwksFetchError::ResponseTooLarge); - } - body.extend_from_slice(&chunk); - } - - String::from_utf8(body).map_err(|_| JwksFetchError::ParseError) + with_deadline( + fetch_jwks_inner(uri), + std::time::Duration::from_secs(JWKS_REQUEST_TIMEOUT_SECS), + ) + .await } } +/// Bound `fut` with a hard `tokio::time::timeout`. Elapsed maps to +/// `NetworkError`. Production passes `fetch_jwks_inner(uri)`; tests pass +/// `std::future::pending()` to verify the seam deterministically. +async fn with_deadline(fut: F, timeout: std::time::Duration) -> Result +where + F: std::future::Future>, +{ + tokio::time::timeout(timeout, fut) + .await + .map_err(|_| JwksFetchError::NetworkError)? +} + +/// Inner fetch logic. Called only by `HttpJwksFetcher::fetch_jwks` via `with_deadline`. +async fn fetch_jwks_inner(uri: &str) -> Result { + // Full URI validation first — scheme, credentials, fragment, bare + // private-IP host. This enforces the JwksFetcher contract for direct + // callers of HttpJwksFetcher regardless of whether ProductionJwksSource + // pre-validated the URI. + validate_jwks_uri(uri)?; + + let parsed = Url::parse(uri).map_err(|_| JwksFetchError::InvalidUri)?; + let host = parsed.host_str().ok_or(JwksFetchError::InvalidUri)?; + let port = parsed.port_or_known_default().unwrap_or(443); + + // Resolve and check every IP before sending. Pins DNS to the validated + // address to prevent rebinding TOCTOU between check and connect. + let safe_ip = resolve_and_check_ssrf(host, port).await?; + + // Build a per-request client that: + // - denies redirects (a 3xx to an internal host bypasses the URI check); + // - has no system proxy (proxy would resolve the original hostname itself); + // - pins this request to the validated IP. + let pinned_client = reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .no_proxy() + .resolve(host, std::net::SocketAddr::new(safe_ip, port)) + .build() + .map_err(|_| JwksFetchError::NetworkError)?; + + let response = pinned_client + .get(uri) + .send() + .await + .map_err(|_| JwksFetchError::NetworkError)?; + + // Reject non-2xx. A 3xx here means our no-redirect policy was somehow + // bypassed — treat as a network error. + if !response.status().is_success() { + return Err(JwksFetchError::NetworkError); + } + + // Early-exit on Content-Length before streaming. A lying or absent + // Content-Length is caught by the incremental counter below. + if let Some(content_length) = response.content_length() { + if content_length as usize > MAX_JWKS_RESPONSE_BYTES { + return Err(JwksFetchError::ResponseTooLarge); + } + } + + // Stream incrementally; stop at MAX_JWKS_RESPONSE_BYTES + 1 so we + // never buffer more than the limit before rejecting. + let mut body = Vec::with_capacity(MAX_JWKS_RESPONSE_BYTES.min(64 * 1024)); + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|_| JwksFetchError::NetworkError)?; + if body.len().saturating_add(chunk.len()) > MAX_JWKS_RESPONSE_BYTES { + return Err(JwksFetchError::ResponseTooLarge); + } + body.extend_from_slice(&chunk); + } + + String::from_utf8(body).map_err(|_| JwksFetchError::ParseError) +} + fn parse_and_bound_jwks(body: &str) -> Result { let key_set: JwkSet = serde_json::from_str(body).map_err(|_| JwksFetchError::ParseError)?; if key_set.keys.is_empty() || key_set.keys.len() > MAX_JWKS_KEYS { diff --git a/crates/buzz-auth/src/nip_fi/jwks/tests.rs b/crates/buzz-auth/src/nip_fi/jwks/tests.rs index 0542d46ae1..90bc401436 100644 --- a/crates/buzz-auth/src/nip_fi/jwks/tests.rs +++ b/crates/buzz-auth/src/nip_fi/jwks/tests.rs @@ -394,6 +394,11 @@ fn validate_uri_accepts_valid_https() { assert!(validate_jwks_uri("https://id.example/.well-known/jwks.json").is_ok()); } +#[test] +fn validate_uri_accepts_public_ipv6() { + assert!(validate_jwks_uri("https://[2606:4700::1]/.well-known/jwks.json").is_ok()); +} + #[test] fn validate_uri_rejects_http() { assert_eq!( @@ -449,3 +454,76 @@ fn validate_uri_rejects_unparseable() { JwksFetchError::InvalidUri ); } + +#[tokio::test] +async fn http_fetcher_rejects_http_uri_before_connection() { + let fetcher = HttpJwksFetcher::new(); + // Would resolve DNS and return NetworkError if validation ran after I/O. + let err = fetcher + .fetch_jwks("http://id.example/.well-known/jwks.json") + .await + .unwrap_err(); + assert_eq!(err, JwksFetchError::InvalidUri); +} + +#[tokio::test] +async fn http_fetcher_rejects_credentials_uri_before_connection() { + let fetcher = HttpJwksFetcher::new(); + let err = fetcher + .fetch_jwks("https://user:pass@id.example/.well-known/jwks.json") + .await + .unwrap_err(); + assert_eq!(err, JwksFetchError::InvalidUri); +} + +#[tokio::test] +async fn http_fetcher_rejects_fragment_uri_before_connection() { + let fetcher = HttpJwksFetcher::new(); + let err = fetcher + .fetch_jwks("https://id.example/.well-known/jwks.json#section") + .await + .unwrap_err(); + assert_eq!(err, JwksFetchError::InvalidUri); +} + +#[tokio::test] +async fn http_fetcher_rejects_private_ip_uri_before_connection() { + let fetcher = HttpJwksFetcher::new(); + let err = fetcher + .fetch_jwks("https://10.0.0.1/.well-known/jwks.json") + .await + .unwrap_err(); + assert_eq!(err, JwksFetchError::InvalidUri); +} + +#[tokio::test] +async fn resolve_ssrf_rejects_ipv6_loopback_fast_path() { + let err = super::resolve_and_check_ssrf("::1", 443).await.unwrap_err(); + assert_eq!(err, JwksFetchError::InvalidUri); +} + +#[tokio::test] +async fn resolve_ssrf_accepts_public_ipv6_fast_path() { + let ip = super::resolve_and_check_ssrf("2606:4700::1", 443) + .await + .unwrap(); + assert_eq!(ip, "2606:4700::1".parse::().unwrap()); +} + +/// `with_deadline` must fire before the outer guard when the inner future +/// never resolves. Uses `std::future::pending()` so no DNS or I/O occurs. +/// Removing the `tokio::time::timeout` inside `with_deadline` leaves the +/// future permanently pending — the outer guard fires and the test fails. +#[tokio::test(start_paused = true)] +async fn with_deadline_fires_before_outer_guard() { + let inner = super::with_deadline( + std::future::pending::>(), + std::time::Duration::ZERO, + ); + // Independent 1-second outer guard. Must not be the one that fires. + let result = tokio::time::timeout(std::time::Duration::from_secs(1), inner).await; + assert_eq!( + result.expect("outer guard fired — with_deadline timeout seam missing"), + Err(JwksFetchError::NetworkError), + ); +}