From 3c0ec83a0e08dad8c07f3a7b60e3700d48fc7461 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 31 Aug 2026 12:13:03 -0400 Subject: [PATCH] fix(buzz-auth): close SSRF/redirect bypass in HttpJwksFetcher; trim internal markers - Remove with_client() bypass: HttpJwksFetcher is now a unit struct; each fetch_jwks call builds a dedicated per-request pinned client. - Add resolve_and_check_ssrf: DNS-resolves host:port via spawn_blocking, rejects any resolved private/reserved IP (closes DNS-rebinding TOCTOU). - Per-request client enforces: redirect(Policy::none()), no_proxy(), .resolve(host, pinned_ip), and timeout(JWKS_REQUEST_TIMEOUT_SECS). - Drop unused client field (dead_code warning) now that no shared pool is needed. - Remove pure-paraphrase doc on IssuerRegistry::all_policies(); replace with doc stating constraint (unspecified order, startup use). - Remove all 'PR N' internal markers from doc comments; replace with production-stable references to the jwks runtime. Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com> Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-auth/src/nip_fi/config.rs | 3 +- crates/buzz-auth/src/nip_fi/jwks/mod.rs | 136 +++++++++++++++--------- crates/buzz-auth/src/nip_fi/verifier.rs | 14 +-- 3 files changed, 97 insertions(+), 56 deletions(-) diff --git a/crates/buzz-auth/src/nip_fi/config.rs b/crates/buzz-auth/src/nip_fi/config.rs index 83866df247..227e9e0dfd 100644 --- a/crates/buzz-auth/src/nip_fi/config.rs +++ b/crates/buzz-auth/src/nip_fi/config.rs @@ -561,7 +561,8 @@ impl IssuerRegistry { self.policies.is_empty() } - /// Iterate over all registered policies. + /// All registered issuer policies, in unspecified order. Useful for + /// iterating over every configured issuer during startup validation. 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 dc21543473..e90dd37867 100644 --- a/crates/buzz-auth/src/nip_fi/jwks/mod.rs +++ b/crates/buzz-auth/src/nip_fi/jwks/mod.rs @@ -47,15 +47,15 @@ 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). -/// This constant documents the timeout set on the default `HttpJwksFetcher::new()` -/// client; it cannot be removed via `with_client`. +/// Per-request deadline for the complete JWKS fetch (connect + headers + body), +/// enforced inside `fetch_jwks` independently of any client-level timeout. pub const JWKS_REQUEST_TIMEOUT_SECS: u64 = 10; /// Validate that a JWKS URI is safe to fetch: HTTPS scheme, no credentials, -/// no fragment, and the host (if a bare IP) is not private/reserved. Hostnames -/// are not resolved here — runtime SSRF for hostname targets is limited by -/// redirect denial and the intrinsic request deadline. +/// no fragment, and the host (if a bare IP) is not private/reserved. +/// Hostname targets are resolved and checked at every fetch in `fetch_jwks` +/// to prevent DNS rebinding — this check catches the most common +/// misconfiguration at construction time. pub fn validate_jwks_uri(uri: &str) -> Result<(), JwksFetchError> { let parsed = Url::parse(uri).map_err(|_| JwksFetchError::InvalidUri)?; if parsed.scheme() != "https" { @@ -70,8 +70,7 @@ pub fn validate_jwks_uri(uri: &str) -> Result<(), JwksFetchError> { if parsed.fragment().is_some() { return Err(JwksFetchError::InvalidUri); } - // Reject bare private/reserved IP targets at construction time. Hostname - // targets are additionally constrained at runtime by redirect denial. + // Reject bare private/reserved IP targets at construction time. if let Some(url::Host::Ipv4(addr)) = parsed.host() { if is_private_ip(&std::net::IpAddr::V4(addr)) { return Err(JwksFetchError::InvalidUri); @@ -85,6 +84,37 @@ pub fn validate_jwks_uri(uri: &str) -> Result<(), JwksFetchError> { Ok(()) } +/// Resolve `host:port` to IP addresses and reject if any are private/reserved. +/// +/// Returns the first safe address for DNS pinning. Blocks on the OS resolver +/// via `spawn_blocking` to avoid blocking the async runtime. +/// +/// 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}"); + let addrs: Vec = tokio::task::spawn_blocking(move || { + use std::net::ToSocketAddrs; + addr_str + .to_socket_addrs() + .map(|iter| iter.map(|sa| sa.ip()).collect::>()) + }) + .await + .map_err(|_| JwksFetchError::NetworkError)? + .map_err(|_| JwksFetchError::NetworkError)?; + + if addrs.is_empty() { + return Err(JwksFetchError::NetworkError); + } + for ip in &addrs { + if is_private_ip(ip) { + return Err(JwksFetchError::InvalidUri); + } + } + Ok(addrs[0]) +} + #[derive(Clone)] struct CachedSnapshot { key_set: AssertionKeySet, @@ -139,8 +169,8 @@ pub struct IssuerJwksConfig { /// URLs, or raw response content appear in these variants. #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] pub enum JwksFetchError { - /// Non-HTTPS scheme, embedded credentials, fragment, or bare - /// private/reserved IP host. + /// Non-HTTPS scheme, embedded credentials, fragment, bare + /// private/reserved IP host, or DNS resolved to a private/reserved address. #[error("JWKS URI failed safety validation")] InvalidUri, /// Response body exceeded [`MAX_JWKS_RESPONSE_BYTES`]. @@ -160,8 +190,12 @@ pub enum JwksFetchError { /// Sealed injection seam for JWKS HTTP fetching. Only types inside `buzz_auth` /// may implement it — external types cannot name the private supertrait. /// -/// Implementations MUST enforce [`MAX_JWKS_RESPONSE_BYTES`] and MUST reject -/// non-2xx responses. +/// Implementations MUST: +/// - 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 [`MAX_JWKS_RESPONSE_BYTES`] via incremental streaming; +/// - reject non-2xx responses. pub trait JwksFetcher: super::verifier::sealed::Sealed + Send + Sync + 'static { /// Fetch and return the raw JSON body from the given JWKS URI. fn fetch_jwks<'a>( @@ -170,35 +204,27 @@ pub trait JwksFetcher: super::verifier::sealed::Sealed + Send + Sync + 'static { ) -> impl std::future::Future> + Send + 'a; } -/// Production [`JwksFetcher`] backed by `reqwest`. The default client enforces: -/// - no redirects (`Policy::none()`) — a redirect to an internal host would -/// bypass the URI safety check performed at startup; -/// - a finite per-request deadline ([`JWKS_REQUEST_TIMEOUT_SECS`]). +/// Production [`JwksFetcher`] backed by `reqwest`. Each call to `fetch_jwks` +/// builds a dedicated pinned client — no shared connection state between fetches. /// -/// `with_client` accepts a caller-supplied client; the caller must preserve -/// the no-redirect and finite-timeout invariants. The JWKS URI safety check -/// is still enforced by [`ProductionJwksSource::new`] regardless. -#[derive(Clone)] -pub struct HttpJwksFetcher { - client: reqwest::Client, -} +/// Per-fetch boundary enforcement: +/// - hostname DNS is resolved and every address checked against +/// `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`; +/// - 3xx responses are rejected as `NetworkError` — redirects are never followed; +/// - the body is streamed incrementally and stopped at +/// [`MAX_JWKS_RESPONSE_BYTES`] + 1. +#[derive(Clone, Debug)] +pub struct HttpJwksFetcher; impl HttpJwksFetcher { - /// Builds a hardened client: no redirects (`Policy::none()`), finite - /// request deadline ([`JWKS_REQUEST_TIMEOUT_SECS`]). + /// Builds a new fetcher. Security invariants are enforced per-request in + /// `fetch_jwks` — each call constructs a dedicated pinned client. pub fn new() -> Self { - let client = reqwest::Client::builder() - .redirect(reqwest::redirect::Policy::none()) - .timeout(std::time::Duration::from_secs(JWKS_REQUEST_TIMEOUT_SECS)) - .build() - .expect("HttpJwksFetcher default client build failed"); - Self { client } - } - - /// The caller is responsible for preserving the no-redirect and - /// finite-timeout invariants documented on this type. - pub fn with_client(client: reqwest::Client) -> Self { - Self { client } + Self } } @@ -208,26 +234,40 @@ impl Default for HttpJwksFetcher { } } -impl std::fmt::Debug for HttpJwksFetcher { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str("HttpJwksFetcher") - } -} - impl super::verifier::sealed::Sealed for HttpJwksFetcher {} impl JwksFetcher for HttpJwksFetcher { async fn fetch_jwks<'a>(&'a self, uri: &'a str) -> Result { - let response = self - .client + 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)?; - // Non-2xx rejected before reading the body. A 3xx here means the - // client followed a redirect (default client disallows this); 4xx/5xx - // means the endpoint is not serving JWKS. + // 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); } diff --git a/crates/buzz-auth/src/nip_fi/verifier.rs b/crates/buzz-auth/src/nip_fi/verifier.rs index 19a4824377..167dd4f7d8 100644 --- a/crates/buzz-auth/src/nip_fi/verifier.rs +++ b/crates/buzz-auth/src/nip_fi/verifier.rs @@ -64,7 +64,7 @@ pub(crate) mod sealed { /// construction seam: [`verify`] takes no snapshot argument, and this type has /// no public constructor, so an external consumer cannot build a snapshot that /// labels issuer B's JWKS as issuer A. Building a snapshot (and the source that -/// serves it) is the trusted configuration act PR 3's JWKS runtime performs at +/// serves it) is the trusted configuration act the `jwks` runtime performs at /// startup, not a per-request or external input. /// /// The crate-private constructor is a live regression: an external crate that @@ -90,7 +90,7 @@ impl AssertionKeySet { /// generation and a required key-snapshot hard deadline. Rejects a zero /// generation, an empty issuer, an empty or oversized key set /// ([`MAX_JWKS_KEYS`]), or a non-positive deadline. Crate-private: only the - /// trusted in-crate configuration path (PR 3's JWKS runtime) may bind key + /// trusted in-crate configuration path (the `jwks` runtime) may bind key /// material to an issuer. /// /// Bounding the key count here is the pre-lookup control (NIP-FI.md:166-171): @@ -146,7 +146,7 @@ impl fmt::Debug for AssertionKeySet { /// instead asks this source for the snapshot bound to the token's /// signature-authenticated `iss`. A request-path caller therefore cannot /// relabel one issuer's JWKS as another's — the cross-issuer bypass at the old -/// `verify(token, key_set)` seam. Configuring the source (PR 3's JWKS runtime) +/// `verify(token, key_set)` seam. Configuring the source (the `jwks` runtime) /// is a trusted startup act, not per-request input. /// /// This trait is sealed via a private supertrait, so it cannot be implemented @@ -174,7 +174,7 @@ pub trait IssuerKeySource: sealed::Sealed { } /// A fixed issuer→snapshot key source for the in-crate verifier tests, -/// standing in for PR 3's JWKS runtime. It is `cfg(test)`-only — not behind a +/// standing in for the `jwks` runtime. It is `cfg(test)`-only — not behind a /// downstream-selectable Cargo feature — so no dependent crate can enable it to /// reconstruct the authority. An honest source returns only the snapshot bound /// to the exact issuer requested, the invariant the real runtime source @@ -345,7 +345,7 @@ impl FederatedAssertionVerifier { // is `evidence_rejected` (403), and this defers a valid one as // `authorization_unavailable` (503) so a missing witness never // masquerades as rejected evidence, nor invalid input as unavailable - // (NIP-FI.md:459-476). PR 3 adds the witness path additively. + // (NIP-FI.md:459-476). if policy.freshness() == FreshnessClass::CurrentStatus { return Err(VerifierError::StatusWitnessUnavailable); } @@ -719,8 +719,8 @@ fn parse_nostr_pubkey_claim( } } -/// Capture only the claim names the policy reads into a canonical set. For PR 1 -/// the closed set is the `scope` claim, split on ASCII space; unchecked claims +/// Capture only the claim names the policy reads into a canonical set. The +/// closed set is the `scope` claim, split on ASCII space; unchecked claims /// never enter the result. fn capture_capabilities( _policy: &IssuerPolicy,