From e78bafab69dc518fece5ec834f13d12b83e6475d Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 31 Aug 2026 11:40:33 -0400 Subject: [PATCH] fix(auth): harden JWKS boundary, reject current-status, fix generation and comment quality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTP boundary (finding 1): - Add validate_jwks_uri(): HTTPS-only, no credentials/fragments, bare IP private-address rejection via buzz_core::network::is_private_ip - HttpJwksFetcher::new() builds a hardened client: no redirects, 10s intrinsic deadline; with_client() documents caller invariants - Stream response body incrementally (bytes_stream + StreamExt), stop at MAX_JWKS_RESPONSE_BYTES + 1 before any deserialization - Reject non-2xx status before reading body - Add reqwest 'stream' feature to workspace; add futures-util to buzz-auth deps - Add MAX_JWKS_TIMING_SECONDS = 1 year upper bound on timing fields - Regression tests: non-HTTPS, loopback/private IP, credentials, fragment, oversized timing, duplicate issuer all rejected at construction CurrentStatus posture (finding 2): - Rename error variant DuplicateIssuer(String) -> DuplicateIssuer (sanitized) - Add UnsupportedPosture error variant - validate_nip_fi_config() rejects any CurrentStatus policy with UnsupportedPosture — verifier has no status witness; startup fails closed - discovery.rs: remove FreshnessClassDiscovery::CurrentStatus variant and FederatedIdentityDiscovery::current_status() constructor entirely - Test asserts rejection both with and without JWKS config Duplicate issuer detection (finding 3): - validate_nip_fi_config(): explicit duplicate detection in JWKS config slice (collect() was silently overwriting); returns DuplicateIssuer on collision - ProductionJwksSource::new(): rejects duplicate issuer via HashMap::contains_key before insert Timing bounds and overflow (finding 4): - MAX_JWKS_TIMING_SECONDS constant bounds both refresh and hard-deadline fields - i64::try_from() + Duration::try_seconds() eliminates u64->i64 cast panic - Validated at both ProductionJwksSource::new() and validate_nip_fi_config() - Test: new_rejects_timing_above_maximum() Generation monotonicity (finding 5): - Replace wall-clock millis with SHA-256 content digest per issuer - Generation counter advances (saturating_add) only when digest changes; identical documents preserve the prior generation - Regressions: generation_stable_for_identical_document(), generation_advances_for_changed_document() Clippy (finding 6): - manual_async_fn: replaced RPITIT form with native 'async fn' in impl block - single_match (startup): replaced match { None => .., Some(_) => {} } with if let / !contains_key - unnecessary_get_then_check: replaced .get().is_none() with !contains_key() Comment quality (all files): - Remove module-to-PR table from nip_fi/mod.rs - Remove all 'Phase A', 'PR 1/3', 'PRs 4-5' references from every doc comment - Remove WHAT comments (field-name paraphrases, narrated steps, section banners with no contract content, 'Construct with a default reqwest client') - Retain WHY: security invariants, exact NIP-FI spec refs, fail-closed choices, FI-TRACE/FI-INV stable identifiers Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com> Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- Cargo.lock | 1 + Cargo.toml | 2 +- crates/buzz-auth/Cargo.toml | 1 + crates/buzz-auth/src/nip_fi/discovery.rs | 53 +-- crates/buzz-auth/src/nip_fi/jwks/mod.rs | 333 +++++++++++-------- crates/buzz-auth/src/nip_fi/jwks/tests.rs | 305 +++++++++++++++-- crates/buzz-auth/src/nip_fi/mod.rs | 25 +- crates/buzz-auth/src/nip_fi/startup/mod.rs | 148 ++++----- crates/buzz-auth/src/nip_fi/startup/tests.rs | 109 +++--- 9 files changed, 636 insertions(+), 341 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc28cbf626..552a12ca15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -939,6 +939,7 @@ dependencies = [ "base64 0.22.1", "buzz-core", "chrono", + "futures-util", "hex", "jsonwebtoken", "nostr 0.44.7", diff --git a/Cargo.toml b/Cargo.toml index d6ee839f1b..0af365f52f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -104,7 +104,7 @@ chrono = { version = "0.4", features = ["serde"] } jsonwebtoken = { version = "10.4.0", default-features = false, features = ["aws_lc_rs"] } # HTTP client (webhook delivery) -reqwest = { version = "0.13", features = ["json", "rustls"], default-features = false } +reqwest = { version = "0.13", features = ["json", "rustls", "stream"], default-features = false } # Cryptography sha2 = "0.11" diff --git a/crates/buzz-auth/Cargo.toml b/crates/buzz-auth/Cargo.toml index 13dbdd8856..158d282cd6 100644 --- a/crates/buzz-auth/Cargo.toml +++ b/crates/buzz-auth/Cargo.toml @@ -21,6 +21,7 @@ base64 = { workspace = true } chrono = { workspace = true } jsonwebtoken = { workspace = true } nostr = { workspace = true } +futures-util = { workspace = true } reqwest = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/crates/buzz-auth/src/nip_fi/discovery.rs b/crates/buzz-auth/src/nip_fi/discovery.rs index 359844342d..8d1b1500b1 100644 --- a/crates/buzz-auth/src/nip_fi/discovery.rs +++ b/crates/buzz-auth/src/nip_fi/discovery.rs @@ -1,7 +1,8 @@ -//! NIP-11 federated-identity discovery output (NIP-FI Phase A, PR 3). +//! NIP-11 federated-identity discovery output. //! //! [`FederatedIdentityDiscovery`] serializes to the `federated_identity` -//! object NIP-FI.md "Discovery" requires in NIP-11 relay information. +//! object required by the NIP-FI.md "Discovery" section of the NIP-11 relay +//! information document. //! //! ## Privacy invariants //! @@ -19,42 +20,40 @@ use serde::{Deserialize, Serialize}; -/// The `assertion_freshness` sub-object inside `federated_identity`. +/// The `assertion_freshness` sub-object in the `federated_identity` discovery +/// document. Describes the claimed freshness posture without exposing any +/// issuer or deployment-private state. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct AssertionFreshnessDiscovery { - /// `"offline-jwt"` or `"current-status"`. + /// The wire string identifying the freshness class. pub class: FreshnessClassDiscovery, - /// `null` for `offline-jwt`; a tested positive integer for `current-status`. + /// `null` for `offline-jwt`; advertising a finite bound here requires a + /// live status witness that is not yet implemented. pub maximum_residual_upstream_revocation_seconds: Option, } -/// The freshness class as a stable wire string. +/// The freshness class as a stable NIP-FI wire string. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] pub enum FreshnessClassDiscovery { - /// Validates the JWT and JWKS snapshot only. + /// No revocation bound is claimed; JWKS snapshot validation only. OfflineJwt, - /// Additionally requires a current-status witness. - CurrentStatus, } -/// The `federated_identity` NIP-11 discovery object. -/// -/// Placed under `limitation.federated_identity = true` and the top-level -/// `federated_identity` key in the NIP-11 relay information document. -/// Fields never expose enrollment mode, issuer, audience, or private state. +/// The `federated_identity` NIP-11 discovery object. Fields never expose +/// enrollment mode, issuer, audience, or private state. /// [FI-TRACE-DISCOVERY-PRIVATE] #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct FederatedIdentityDiscovery { - /// Always `"client-attached"` for core. + /// Fixed value `"client-attached"` for the core NIP-FI transport mode. pub core: String, - /// The assertion freshness contract claimed by this deployment. + /// The freshness contract claimed by this deployment. pub assertion_freshness: AssertionFreshnessDiscovery, } impl FederatedIdentityDiscovery { - /// Construct an offline-jwt discovery object. This is the minimal core - /// claim that carries no residual revocation bound. + /// The only supported posture: claims no residual revocation bound, which + /// is the honest description of JWKS-only assertion verification. pub fn offline_jwt() -> Self { Self { core: "client-attached".to_owned(), @@ -64,22 +63,4 @@ impl FederatedIdentityDiscovery { }, } } - - /// Construct a current-status discovery object with a tested positive - /// revocation bound (in seconds). The caller is responsible for ensuring - /// `revocation_bound_seconds` has been empirically verified. - /// - /// Returns `None` when `revocation_bound_seconds` is zero. - pub fn current_status(revocation_bound_seconds: u64) -> Option { - if revocation_bound_seconds == 0 { - return None; - } - Some(Self { - core: "client-attached".to_owned(), - assertion_freshness: AssertionFreshnessDiscovery { - class: FreshnessClassDiscovery::CurrentStatus, - maximum_residual_upstream_revocation_seconds: Some(revocation_bound_seconds), - }, - }) - } } diff --git a/crates/buzz-auth/src/nip_fi/jwks/mod.rs b/crates/buzz-auth/src/nip_fi/jwks/mod.rs index dd30e1dfb4..dc21543473 100644 --- a/crates/buzz-auth/src/nip_fi/jwks/mod.rs +++ b/crates/buzz-auth/src/nip_fi/jwks/mod.rs @@ -1,5 +1,5 @@ //! JWKS discovery, snapshot caching, and the production [`IssuerKeySource`] -//! implementation (NIP-FI Phase A, PR 3). +//! implementation for federated-assertion verification. //! //! ## Design invariants //! @@ -13,9 +13,10 @@ //! snapshot if it is within its hard deadline, or `None`. It never serves //! an expired snapshot. [FI-TRACE-JWKS-REMOVE] //! -//! - **Bounded resource acquisition.** The HTTP response is capped at -//! [`MAX_JWKS_RESPONSE_BYTES`] before parsing. Key count is bounded by -//! [`super::config::MAX_JWKS_KEYS`] inside [`AssertionKeySet::new`]. +//! - **Bounded resource acquisition.** HTTP response streaming stops at +//! [`MAX_JWKS_RESPONSE_BYTES`] + 1 byte before any allocation for parsing. +//! Key count is bounded by [`super::config::MAX_JWKS_KEYS`] inside +//! [`AssertionKeySet::new`]. //! //! - **Coalesced refresh.** A single in-flight refresh per issuer prevents //! thundering-herd. Concurrent callers observe the snapshot just after the @@ -26,30 +27,79 @@ use super::config::MAX_JWKS_KEYS; use super::verifier::{AssertionKeySet, IssuerKeySource}; +use buzz_core::network::is_private_ip; use chrono::{DateTime, Duration, Utc}; +use futures_util::StreamExt as _; use jsonwebtoken::jwk::JwkSet; +use sha2::{Digest, Sha256}; use std::collections::HashMap; use std::sync::Arc; use tokio::sync::{Mutex, RwLock}; use tracing::warn; +use url::Url; -/// Maximum HTTP response size for a JWKS endpoint, in bytes. Bounded before -/// parsing to prevent a large or malicious response from consuming unbounded -/// memory during deserialization. +/// Maximum HTTP response body for a JWKS endpoint. Streaming stops at this +/// limit before any deserialization, preventing OOM from a malicious server. pub const MAX_JWKS_RESPONSE_BYTES: usize = 512 * 1024; // 512 KiB -/// A JWKS snapshot with its fetch time and configured hard deadline. +/// Hard upper bound on JWKS timing fields. Values above this are rejected at +/// config construction to prevent `u64`→`i64` conversion overflow and Chrono +/// 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`. +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. +pub fn validate_jwks_uri(uri: &str) -> Result<(), JwksFetchError> { + let parsed = Url::parse(uri).map_err(|_| JwksFetchError::InvalidUri)?; + if parsed.scheme() != "https" { + return Err(JwksFetchError::InvalidUri); + } + // Credentials in the URI are never legitimate for a public JWKS endpoint + // and would be forwarded to the server, leaking material in logs. + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(JwksFetchError::InvalidUri); + } + // Fragments are client-side only; their presence indicates a misconfigured URI. + 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. + if let Some(url::Host::Ipv4(addr)) = parsed.host() { + if is_private_ip(&std::net::IpAddr::V4(addr)) { + return Err(JwksFetchError::InvalidUri); + } + } + if let Some(url::Host::Ipv6(addr)) = parsed.host() { + if is_private_ip(&std::net::IpAddr::V6(addr)) { + return Err(JwksFetchError::InvalidUri); + } + } + Ok(()) +} + #[derive(Clone)] struct CachedSnapshot { key_set: AssertionKeySet, fetched_at: DateTime, hard_deadline: DateTime, + /// SHA-256 of the raw JWKS bytes. Suppresses generation advances when the + /// document is unchanged between refreshes. [FI-TRACE-JWKS-ADD/REMOVE] + content_digest: [u8; 32], } -/// Per-issuer runtime state: the current snapshot and in-flight flag. struct IssuerState { snapshot: Option, - /// True while a refresh task owns the fetch. Prevents concurrent fetches. + /// Advances only when `content_digest` changes; never wraps (saturating). + generation_counter: u64, + /// True while a refresh task owns the fetch lock. Prevents thundering-herd. refresh_in_flight: bool, } @@ -57,82 +107,96 @@ impl IssuerState { fn new() -> Self { Self { snapshot: None, + generation_counter: 0, refresh_in_flight: false, } } } -/// Configuration for one issuer's JWKS endpoint. +/// Per-issuer JWKS endpoint configuration. All fields are validated by +/// [`validate_jwks_uri`] and timing bounds at [`ProductionJwksSource::new`]. #[derive(Debug, Clone)] pub struct IssuerJwksConfig { /// The exact `iss` value this config authenticates. Must match the - /// configured [`IssuerPolicy`][super::config::IssuerPolicy] exactly. + /// corresponding [`IssuerPolicy`][super::config::IssuerPolicy] exactly. pub issuer: String, - /// The HTTPS JWKS endpoint URI. + /// Must pass [`validate_jwks_uri`]: HTTPS, no credentials/fragment, no + /// bare private-IP host. pub jwks_uri: String, - /// How long a cached snapshot remains fresh before re-fetching is - /// triggered, in seconds. Must be positive and less than - /// `key_snapshot_hard_deadline_seconds`. + /// Seconds until a cached snapshot is considered stale and re-fetching is + /// triggered. Must be positive, strictly less than + /// `key_snapshot_hard_deadline_seconds`, and ≤ [`MAX_JWKS_TIMING_SECONDS`]. pub refresh_interval_seconds: u64, /// Hard upper bound from fetch time on how long a snapshot may be served. - /// A snapshot whose deadline has passed is never returned, even on error. - /// Folds into every `AssertionKeySet` hard deadline and therefore into - /// every `VerifiedAssertion.revalidation_dependencies`. + /// Expired snapshots are never returned, even on fetch error — no stale + /// fallback. Folds into every `AssertionKeySet` hard deadline and therefore + /// into every `VerifiedAssertion.revalidation_dependencies`. + /// Must be ≤ [`MAX_JWKS_TIMING_SECONDS`]. pub key_snapshot_hard_deadline_seconds: u64, } -/// Why a JWKS fetch or parse operation failed. No key material, issuer URLs, -/// or raw response content appear in these variants. +/// Reason a JWKS fetch or parse operation failed. No key material, issuer +/// URLs, or raw response content appear in these variants. #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] pub enum JwksFetchError { - /// The HTTP response exceeded [`MAX_JWKS_RESPONSE_BYTES`]. + /// Non-HTTPS scheme, embedded credentials, fragment, or bare + /// private/reserved IP host. + #[error("JWKS URI failed safety validation")] + InvalidUri, + /// Response body exceeded [`MAX_JWKS_RESPONSE_BYTES`]. #[error("JWKS response exceeded size limit")] ResponseTooLarge, - /// The HTTP request failed (network, TLS, timeout). + /// Network failure, TLS error, request timeout, or non-2xx status. #[error("JWKS HTTP request failed")] NetworkError, - /// The response body was not parseable as a JWK Set. + /// Response body was not parseable as a JWK Set. #[error("JWKS response was not parseable")] ParseError, - /// The parsed key set was empty or exceeded the key-count bound. + /// Parsed key set was empty or exceeded [`super::config::MAX_JWKS_KEYS`]. #[error("JWKS key set bounds violation")] KeyCountBoundsViolation, } -/// Async HTTP fetch of a JWKS endpoint. +/// Sealed injection seam for JWKS HTTP fetching. Only types inside `buzz_auth` +/// may implement it — external types cannot name the private supertrait. /// -/// This is a sealed injection seam: only types inside `buzz_auth` may -/// implement it (the private supertrait `sealed` prevents external impls). -/// The production implementation uses `reqwest`; the test implementation -/// returns hard-coded bodies without network calls. -/// -/// Implementations MUST enforce [`MAX_JWKS_RESPONSE_BYTES`]. +/// Implementations MUST enforce [`MAX_JWKS_RESPONSE_BYTES`] and MUST reject +/// non-2xx responses. pub trait JwksFetcher: super::verifier::sealed::Sealed + Send + Sync + 'static { - /// Fetch the JWK Set from the given URI, returning the raw JSON body. + /// Fetch and return the raw JSON body from the given JWKS URI. fn fetch_jwks<'a>( &'a self, uri: &'a str, ) -> impl std::future::Future> + Send + 'a; } -/// Production [`JwksFetcher`] backed by `reqwest`. +/// 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`]). /// -/// Enforces [`MAX_JWKS_RESPONSE_BYTES`] before reading the full body. +/// `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, } impl HttpJwksFetcher { - /// Construct with a default `reqwest` client. + /// Builds a hardened client: no redirects (`Policy::none()`), finite + /// request deadline ([`JWKS_REQUEST_TIMEOUT_SECS`]). pub fn new() -> Self { - Self { - client: reqwest::Client::new(), - } + 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 } } - /// Construct with an explicit `reqwest::Client` (e.g., with custom TLS - /// certificates or timeout configuration). + /// 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 } } @@ -150,63 +214,62 @@ impl std::fmt::Debug for HttpJwksFetcher { } } -// Sealed so only in-crate types implement `JwksFetcher`. impl super::verifier::sealed::Sealed for HttpJwksFetcher {} impl JwksFetcher for HttpJwksFetcher { - fn fetch_jwks<'a>( - &'a self, - uri: &'a str, - ) -> impl std::future::Future> + Send + 'a { - async move { - let response = self - .client - .get(uri) - .send() - .await - .map_err(|_| JwksFetchError::NetworkError)?; + async fn fetch_jwks<'a>(&'a self, uri: &'a str) -> Result { + let response = self + .client + .get(uri) + .send() + .await + .map_err(|_| JwksFetchError::NetworkError)?; - // Reject based on Content-Length before reading body. - if let Some(content_length) = response.content_length() { - if content_length as usize > MAX_JWKS_RESPONSE_BYTES { - return Err(JwksFetchError::ResponseTooLarge); - } - } + // 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. + if !response.status().is_success() { + return Err(JwksFetchError::NetworkError); + } - let bytes = response - .bytes() - .await - .map_err(|_| JwksFetchError::NetworkError)?; - - if bytes.len() > MAX_JWKS_RESPONSE_BYTES { + // 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); } - - String::from_utf8(bytes.to_vec()).map_err(|_| JwksFetchError::ParseError) } + + // 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) } } -/// Parse a raw JWKS JSON body into a bounded, validated [`JwkSet`]. -/// -/// Rejects parse errors and key-count bound violations before any per-key -/// lookup or allocation. 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 { return Err(JwksFetchError::KeyCountBoundsViolation); } - Ok(key_set) } -/// The production [`IssuerKeySource`]: a multi-issuer JWKS cache that performs -/// bounded periodic refresh and never serves snapshots past their hard deadline. +/// Multi-issuer JWKS cache that performs bounded periodic refresh and never +/// serves snapshots past their hard deadline. /// -/// One `ProductionJwksSource` is constructed at startup after -/// [`super::startup::validate_nip_fi_config`] passes. The `Arc>` -/// internal structure lets it be shared across async tasks cheaply. +/// Must be constructed at startup after +/// [`super::startup::validate_nip_fi_config`] passes. Shared across async +/// tasks via the inner `Arc>`. /// /// ## Security /// @@ -215,17 +278,14 @@ fn parse_and_bound_jwks(body: &str) -> Result { /// - Errors are logged with a stable code; no key material appears in logs. pub struct ProductionJwksSource { configs: HashMap, - /// Keyed by exact issuer string. states: Arc>>>, fetcher: Arc, } impl ProductionJwksSource { - /// Construct a new source from validated issuer JWKS configs. - /// - /// Returns `None` when `configs` is empty (startup validation rejects this - /// before the source is ever built) or when any config has invalid timing - /// bounds. + /// Returns `None` when `configs` is empty, any config has invalid timing + /// bounds or fails URI validation, or any two configs share the same + /// `issuer` (duplicate issuers make trust configuration ambiguous). pub fn new(configs: Vec, fetcher: F) -> Option { if configs.is_empty() { return None; @@ -238,9 +298,17 @@ impl ProductionJwksSource { if c.refresh_interval_seconds == 0 || c.key_snapshot_hard_deadline_seconds == 0 || c.key_snapshot_hard_deadline_seconds <= c.refresh_interval_seconds + || c.refresh_interval_seconds > MAX_JWKS_TIMING_SECONDS + || c.key_snapshot_hard_deadline_seconds > MAX_JWKS_TIMING_SECONDS { return None; } + if validate_jwks_uri(&c.jwks_uri).is_err() { + return None; + } + if config_map.contains_key(&c.issuer) { + return None; + } let issuer = c.issuer.clone(); state_map.insert(issuer.clone(), Mutex::new(IssuerState::new())); config_map.insert(issuer, c); @@ -252,17 +320,17 @@ impl ProductionJwksSource { }) } - /// Fetch and seal a fresh snapshot for one issuer, without updating the - /// cache. Returns `None` when the fetch or parse fails (already logged). - async fn fetch_fresh(&self, issuer: &str) -> Option { + async fn fetch_fresh( + &self, + issuer: &str, + prev_digest: Option<[u8; 32]>, + prev_generation: u64, + ) -> Option<(CachedSnapshot, u64)> { let config = self.configs.get(issuer)?; let body = match self.fetcher.fetch_jwks(&config.jwks_uri).await { Ok(b) => b, Err(err) => { - warn!( - error = %err, - "nip-fi jwks fetch failed; will use cached snapshot if live" - ); + warn!(error = %err, "nip-fi jwks fetch failed; will use cached snapshot if live"); return None; } }; @@ -270,41 +338,50 @@ impl ProductionJwksSource { let jwks = match parse_and_bound_jwks(&body) { Ok(k) => k, Err(err) => { - warn!( - error = %err, - "nip-fi jwks parse failed; will use cached snapshot if live" - ); + warn!(error = %err, "nip-fi jwks parse failed; will use cached snapshot if live"); return None; } }; - let now = Utc::now(); - let hard_deadline = - now + Duration::seconds(config.key_snapshot_hard_deadline_seconds as i64); + let content_digest: [u8; 32] = Sha256::digest(body.as_bytes()).into(); - // Generation: milliseconds since epoch, floored to 1 to satisfy the - // non-zero invariant. Monotone unless the system clock goes backwards. - let generation = u64::try_from(now.timestamp_millis()).unwrap_or(1).max(1); + // Advance only when the document changed so key-rotation events are + // visible [FI-TRACE-JWKS-ADD/REMOVE] while identical refetches are + // stable. Saturating add prevents wrap on the (unreachable) u64 ceiling. + let generation = if Some(content_digest) == prev_digest { + prev_generation + } else { + prev_generation.saturating_add(1).max(1) + }; + + let now = Utc::now(); + // MAX_JWKS_TIMING_SECONDS ≤ ~31.5M < i64::MAX, so this conversion is + // always safe for values that passed the bounds check in new(). + let deadline_secs = + i64::try_from(config.key_snapshot_hard_deadline_seconds).unwrap_or(i64::MAX / 2); + let hard_deadline = now + + Duration::try_seconds(deadline_secs) + .unwrap_or_else(|| Duration::seconds(i64::MAX / 2)); let key_set = AssertionKeySet::new(issuer.to_owned(), generation, jwks, hard_deadline)?; - Some(CachedSnapshot { - key_set, - fetched_at: now, - hard_deadline, - }) + Some(( + CachedSnapshot { + key_set, + fetched_at: now, + hard_deadline, + content_digest, + }, + generation, + )) } - /// Return the current snapshot for `issuer`, refreshing if stale. - /// + /// Returns the cached snapshot for `issuer`, refreshing inline if stale. /// Returns `None` when no live snapshot is available and the fetch fails. /// - /// ## Refresh logic - /// - /// - If the cached snapshot is past its hard deadline, it is cleared. - /// - If there is no snapshot, or the snapshot is past its refresh - /// interval, a refresh runs inline (holding the issuer's mutex). - /// - Concurrent calls share the inline refresh via the per-issuer mutex. + /// If a refresh is already in flight for this issuer, returns the current + /// snapshot rather than blocking — coalesces concurrent callers. Drops + /// both locks before the async fetch so other issuers are not blocked. pub async fn get_snapshot(&self, issuer: &str) -> Option { let states = self.states.read().await; let state_mutex = states.get(issuer)?; @@ -313,7 +390,6 @@ impl ProductionJwksSource { let now = Utc::now(); let config = self.configs.get(issuer)?; - // Evict expired snapshot. if let Some(ref cached) = state.snapshot { if now >= cached.hard_deadline { state.snapshot = None; @@ -333,25 +409,23 @@ impl ProductionJwksSource { } if state.refresh_in_flight { - // Another task is already refreshing; return the current snapshot - // (may be None if no snapshot is available yet). return state.snapshot.as_ref().map(|c| c.key_set.clone()); } state.refresh_in_flight = true; - // Drop mutex and read lock while doing async I/O so other issuers - // are not blocked. + let prev_digest = state.snapshot.as_ref().map(|c| c.content_digest); + let prev_generation = state.generation_counter; drop(state); drop(states); - let fresh = self.fetch_fresh(issuer).await; + let fresh = self.fetch_fresh(issuer, prev_digest, prev_generation).await; - // Re-acquire to commit the result and clear the in-flight flag. let states = self.states.read().await; if let Some(state_mutex) = states.get(issuer) { let mut st = state_mutex.lock().await; st.refresh_in_flight = false; - if let Some(ref cached) = fresh { + if let Some((ref cached, new_generation)) = fresh { + st.generation_counter = new_generation; st.snapshot = Some(cached.clone()); } let now2 = Utc::now(); @@ -366,20 +440,15 @@ impl ProductionJwksSource { } } -// Sealed so only in-crate types implement `IssuerKeySource`. impl super::verifier::sealed::Sealed for ProductionJwksSource {} impl IssuerKeySource for ProductionJwksSource { - /// Synchronous read of the currently cached snapshot. + /// Called per-request by the verifier after the cache has been warmed via + /// [`get_snapshot`][Self::get_snapshot]. /// - /// The verifier calls this per-request after the runtime has ensured the - /// cache is warm via [`get_snapshot`][Self::get_snapshot]. Returns `None` - /// if no snapshot is available or the snapshot is past its hard deadline. - /// - /// Uses `try_read`/`try_lock` so it is safe to call from any context — - /// including inside an async runtime. If the lock is momentarily held - /// (in-flight refresh), fails closed by returning `None` rather than - /// blocking or panicking. [FI-INV-14] + /// Uses `try_read`/`try_lock` — safe to call from any async context. + /// Fails closed (returns `None`) when the lock is momentarily held by an + /// in-flight refresh, rather than blocking or panicking. [FI-INV-14] fn key_set(&self, issuer: &str) -> Option { let states = self.states.try_read().ok()?; let state_mutex = states.get(issuer)?; diff --git a/crates/buzz-auth/src/nip_fi/jwks/tests.rs b/crates/buzz-auth/src/nip_fi/jwks/tests.rs index 2d4246bd82..0542d46ae1 100644 --- a/crates/buzz-auth/src/nip_fi/jwks/tests.rs +++ b/crates/buzz-auth/src/nip_fi/jwks/tests.rs @@ -1,14 +1,7 @@ -//! Unit tests for the NIP-FI JWKS source (Phase A, PR 3). -//! -//! These tests drive [`ProductionJwksSource`] through a fake [`JwksFetcher`] -//! to avoid live network calls. - use super::*; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; -// ── Fake fetcher ────────────────────────────────────────────────────────────── - struct FakeJwksFetcher { body: Result, call_count: Arc, @@ -27,7 +20,6 @@ impl JwksFetcher for FakeJwksFetcher { } } -/// Build a minimal valid ES256 JWK Set JSON with one key. fn minimal_jwks_json(kid: &str) -> String { format!( r#"{{"keys":[{{"kty":"EC","crv":"P-256","x":"f83OJ3D2xF1Bg8vub9tLe1gHMzV76e8Tus9uPHvRVEU","y":"x_FEzRu9m36HLN_tue659LNpXW6pCyStikYjKIWI5a0","use":"sig","alg":"ES256","kid":"{kid}"}}]}}"# @@ -43,7 +35,14 @@ fn make_config(issuer: &str) -> IssuerJwksConfig { } } -// ── Tests ───────────────────────────────────────────────────────────────────── +fn make_config_with_uri(issuer: &str, jwks_uri: &str) -> IssuerJwksConfig { + IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: jwks_uri.to_owned(), + refresh_interval_seconds: 300, + key_snapshot_hard_deadline_seconds: 3600, + } +} #[tokio::test] async fn get_snapshot_returns_sealed_key_set_on_success() { @@ -54,9 +53,7 @@ async fn get_snapshot_returns_sealed_key_set_on_success() { }; let source = ProductionJwksSource::new(vec![make_config(issuer)], fetcher).unwrap(); - let snapshot = source.get_snapshot(issuer).await; - assert!(snapshot.is_some(), "snapshot should be present on success"); - let ks = snapshot.unwrap(); + let ks = source.get_snapshot(issuer).await.unwrap(); assert_eq!(ks.issuer(), issuer); } @@ -69,8 +66,7 @@ async fn get_snapshot_returns_none_for_unknown_issuer() { let source = ProductionJwksSource::new(vec![make_config("https://id.example")], fetcher).unwrap(); - let snapshot = source.get_snapshot("https://other.example").await; - assert!(snapshot.is_none(), "unknown issuer must return None"); + assert!(source.get_snapshot("https://other.example").await.is_none()); } #[tokio::test] @@ -82,8 +78,7 @@ async fn get_snapshot_returns_none_on_network_error_with_no_cache() { let issuer = "https://id.example"; let source = ProductionJwksSource::new(vec![make_config(issuer)], fetcher).unwrap(); - let snapshot = source.get_snapshot(issuer).await; - assert!(snapshot.is_none(), "no cache + network error = None"); + assert!(source.get_snapshot(issuer).await.is_none()); } #[tokio::test] @@ -112,22 +107,22 @@ async fn get_snapshot_returns_none_on_parse_error() { #[tokio::test] async fn parse_and_bound_rejects_empty_key_set() { - let empty_jwks = r#"{"keys":[]}"#; - let err = parse_and_bound_jwks(empty_jwks).unwrap_err(); + let err = parse_and_bound_jwks(r#"{"keys":[]}"#).unwrap_err(); assert_eq!(err, JwksFetchError::KeyCountBoundsViolation); } #[tokio::test] async fn parse_and_bound_rejects_oversized_key_set() { - // Build MAX_JWKS_KEYS + 1 keys. let keys: Vec = (0..=MAX_JWKS_KEYS) .map(|i| format!( r#"{{"kty":"EC","crv":"P-256","x":"f83OJ3D2xF1Bg8vub9tLe1gHMzV76e8Tus9uPHvRVEU","y":"x_FEzRu9m36HLN_tue659LNpXW6pCyStikYjKIWI5a0","kid":"k{i}"}}"# )) .collect(); let body = format!(r#"{{"keys":[{}]}}"#, keys.join(",")); - let err = parse_and_bound_jwks(&body).unwrap_err(); - assert_eq!(err, JwksFetchError::KeyCountBoundsViolation); + assert_eq!( + parse_and_bound_jwks(&body).unwrap_err(), + JwksFetchError::KeyCountBoundsViolation + ); } #[tokio::test] @@ -148,7 +143,7 @@ async fn new_rejects_refresh_ge_hard_deadline() { let bad_config = IssuerJwksConfig { issuer: "https://id.example".to_owned(), jwks_uri: "https://id.example/.well-known/jwks.json".to_owned(), - refresh_interval_seconds: 3600, // equal to hard deadline + refresh_interval_seconds: 3600, key_snapshot_hard_deadline_seconds: 3600, }; assert!(ProductionJwksSource::new(vec![bad_config], fetcher).is_none()); @@ -169,8 +164,125 @@ async fn new_rejects_zero_refresh_interval() { assert!(ProductionJwksSource::new(vec![bad_config], fetcher).is_none()); } -/// Issuer binding: the sealed `key_set()` synchronous path must return -/// `None` before any snapshot is warmed via `get_snapshot`. +#[tokio::test] +async fn new_rejects_timing_above_maximum() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + let bad_config = IssuerJwksConfig { + issuer: "https://id.example".to_owned(), + jwks_uri: "https://id.example/.well-known/jwks.json".to_owned(), + refresh_interval_seconds: MAX_JWKS_TIMING_SECONDS + 1, + key_snapshot_hard_deadline_seconds: MAX_JWKS_TIMING_SECONDS + 2, + }; + assert!(ProductionJwksSource::new(vec![bad_config], fetcher).is_none()); +} + +#[tokio::test] +async fn new_rejects_duplicate_issuer() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + let issuer = "https://id.example"; + let config_a = IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: "https://id.example/.well-known/jwks.json".to_owned(), + refresh_interval_seconds: 300, + key_snapshot_hard_deadline_seconds: 3600, + }; + let config_b = IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: "https://id.example/.well-known/jwks-alt.json".to_owned(), + refresh_interval_seconds: 600, + key_snapshot_hard_deadline_seconds: 7200, + }; + assert!(ProductionJwksSource::new(vec![config_a, config_b], fetcher).is_none()); +} + +#[tokio::test] +async fn new_rejects_non_https_jwks_uri() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + assert!(ProductionJwksSource::new( + vec![make_config_with_uri( + "https://id.example", + "http://id.example/.well-known/jwks.json" + )], + fetcher + ) + .is_none()); +} + +#[tokio::test] +async fn new_rejects_loopback_jwks_uri() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + assert!(ProductionJwksSource::new( + vec![make_config_with_uri( + "https://id.example", + "https://127.0.0.1/.well-known/jwks.json" + )], + fetcher + ) + .is_none()); +} + +#[tokio::test] +async fn new_rejects_private_ip_jwks_uri() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + assert!(ProductionJwksSource::new( + vec![make_config_with_uri( + "https://id.example", + "https://10.0.0.1/.well-known/jwks.json" + )], + fetcher + ) + .is_none()); +} + +#[tokio::test] +async fn new_rejects_jwks_uri_with_credentials() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + assert!(ProductionJwksSource::new( + vec![make_config_with_uri( + "https://id.example", + "https://user:pass@id.example/.well-known/jwks.json" + )], + fetcher + ) + .is_none()); +} + +#[tokio::test] +async fn new_rejects_jwks_uri_with_fragment() { + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + assert!(ProductionJwksSource::new( + vec![make_config_with_uri( + "https://id.example", + "https://id.example/.well-known/jwks.json#keys" + )], + fetcher + ) + .is_none()); +} + +/// `key_set()` fails closed (returns `None`) before any snapshot is warmed via +/// `get_snapshot` — the synchronous path never fetches. #[tokio::test] async fn sync_key_set_returns_none_before_warmup() { let fetcher = FakeJwksFetcher { @@ -181,14 +293,9 @@ async fn sync_key_set_returns_none_before_warmup() { let source = ProductionJwksSource::new(vec![make_config(issuer)], fetcher).unwrap(); use crate::nip_fi::verifier::IssuerKeySource; - assert!( - source.key_set(issuer).is_none(), - "cache is cold before get_snapshot" - ); + assert!(source.key_set(issuer).is_none()); } -/// After a successful `get_snapshot`, the synchronous `key_set()` path must -/// return the same issuer's snapshot without re-fetching. #[tokio::test] async fn sync_key_set_returns_snapshot_after_warmup() { let fetcher = FakeJwksFetcher { @@ -204,3 +311,141 @@ async fn sync_key_set_returns_snapshot_after_warmup() { let ks = source.key_set(issuer).unwrap(); assert_eq!(ks.issuer(), issuer); } + +/// Identical document fetched twice must not advance the generation counter +/// — stable generation for unchanged JWKS prevents spurious revalidation. +#[tokio::test] +async fn generation_stable_for_identical_document() { + let issuer = "https://id.example"; + let fetcher = FakeJwksFetcher { + body: Ok(minimal_jwks_json("k1")), + call_count: Arc::new(AtomicUsize::new(0)), + }; + let config = IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: format!("https://{issuer}/.well-known/jwks.json"), + refresh_interval_seconds: 1, + key_snapshot_hard_deadline_seconds: 3600, + }; + let source = ProductionJwksSource::new(vec![config], fetcher).unwrap(); + + use crate::nip_fi::verifier::IssuerKeySource; + source.get_snapshot(issuer).await.unwrap(); + let gen1 = source.key_set(issuer).unwrap().generation(); + + tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + source.get_snapshot(issuer).await.unwrap(); + let gen2 = source.key_set(issuer).unwrap().generation(); + + assert_eq!(gen1, gen2); +} + +/// Changed document must advance the generation so key-rotation events are +/// visible [FI-TRACE-JWKS-ADD/REMOVE]. +#[tokio::test] +async fn generation_advances_for_changed_document() { + let issuer = "https://id.example"; + + let bodies = Arc::new(std::sync::Mutex::new(vec![ + Ok::(minimal_jwks_json("k2")), + Ok(minimal_jwks_json("k1")), + ])); + + struct MultiBodyFetcher { + bodies: Arc>>>, + } + impl super::super::verifier::sealed::Sealed for MultiBodyFetcher {} + impl JwksFetcher for MultiBodyFetcher { + fn fetch_jwks<'a>( + &'a self, + _uri: &'a str, + ) -> impl std::future::Future> + Send + 'a { + let result = self + .bodies + .lock() + .unwrap() + .pop() + .unwrap_or(Err(JwksFetchError::NetworkError)); + async move { result } + } + } + + let config = IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: format!("https://{issuer}/.well-known/jwks.json"), + refresh_interval_seconds: 1, + key_snapshot_hard_deadline_seconds: 3600, + }; + let source = ProductionJwksSource::new(vec![config], MultiBodyFetcher { bodies }).unwrap(); + + use crate::nip_fi::verifier::IssuerKeySource; + source.get_snapshot(issuer).await.unwrap(); + let gen1 = source.key_set(issuer).unwrap().generation(); + + tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + source.get_snapshot(issuer).await.unwrap(); + let gen2 = source.key_set(issuer).unwrap().generation(); + + assert!(gen2 > gen1, "gen1={gen1}, gen2={gen2}"); +} + +#[test] +fn validate_uri_accepts_valid_https() { + assert!(validate_jwks_uri("https://id.example/.well-known/jwks.json").is_ok()); +} + +#[test] +fn validate_uri_rejects_http() { + assert_eq!( + validate_jwks_uri("http://id.example/.well-known/jwks.json").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_loopback_ip() { + assert_eq!( + validate_jwks_uri("https://127.0.0.1/jwks.json").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_private_ip() { + assert_eq!( + validate_jwks_uri("https://192.168.1.1/jwks.json").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_link_local_ip() { + assert_eq!( + validate_jwks_uri("https://169.254.169.254/jwks.json").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_credentials() { + assert_eq!( + validate_jwks_uri("https://user:pass@id.example/jwks.json").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_fragment() { + assert_eq!( + validate_jwks_uri("https://id.example/jwks.json#section").unwrap_err(), + JwksFetchError::InvalidUri + ); +} + +#[test] +fn validate_uri_rejects_unparseable() { + assert_eq!( + validate_jwks_uri("not a url").unwrap_err(), + JwksFetchError::InvalidUri + ); +} diff --git a/crates/buzz-auth/src/nip_fi/mod.rs b/crates/buzz-auth/src/nip_fi/mod.rs index 0641481362..2f649f95a6 100644 --- a/crates/buzz-auth/src/nip_fi/mod.rs +++ b/crates/buzz-auth/src/nip_fi/mod.rs @@ -1,24 +1,11 @@ //! NIP-FI federated-identity authorization — assertion verifier, JWKS runtime, -//! startup validation, and discovery (Phase A, PRs 1–3). -//! -//! ## Module layout -//! -//! | Module | Introduced | Responsibility | -//! |--------|-----------|----------------| -//! | [`assertion`] | PR 1 | Sealed [`VerifiedAssertion`] result and its fields | -//! | [`config`] | PR 1 | Multi-issuer policy, contract IDs, size/time bounds | -//! | [`denial`] | PR 1 | Privacy-preserving four-class denial wire contract | -//! | [`verifier`] | PR 1 | Single canonical [`FederatedAssertionVerifier`] | -//! | [`jwks`] | PR 3 | JWKS fetch, cache, and [`ProductionJwksSource`] | -//! | [`startup`] | PR 3 | Startup validation gate ([`validate_nip_fi_config`]) | -//! | [`discovery`] | PR 3 | NIP-11 [`FederatedIdentityDiscovery`] object | -//! -//! Identity is issuer-qualified `(iss, sub)` throughout. No database schema, -//! binding resolution, or request/proof binding is defined here — those belong -//! to PRs 4–5. +//! startup validation, and discovery. -/// The exact client-attached header field ([NIP-FI.md](../../../docs/nips/NIP-FI.md), -/// "Client-attached transport"). `Authorization` remains reserved for NIP-98. +/// The client-attached transport header for federated-identity assertions. +/// +/// `Authorization` remains reserved for NIP-98; this separate header avoids +/// conflating authentication schemes at the relay ingress. +/// ([NIP-FI.md](../../../docs/nips/NIP-FI.md), "Client-attached transport") pub const CLIENT_ATTACHED_HEADER: &str = "Nostr-Federated-Identity"; pub mod assertion; diff --git a/crates/buzz-auth/src/nip_fi/startup/mod.rs b/crates/buzz-auth/src/nip_fi/startup/mod.rs index 770fbe6cd0..15af19eb00 100644 --- a/crates/buzz-auth/src/nip_fi/startup/mod.rs +++ b/crates/buzz-auth/src/nip_fi/startup/mod.rs @@ -1,30 +1,15 @@ -//! Startup validation for the NIP-FI assertion runtime (Phase A, PR 3). +//! Startup validation for the NIP-FI assertion runtime. //! //! [`validate_nip_fi_config`] is the production entry point. It rejects any //! configuration that would make the runtime unsafe, incomplete, or ambiguous //! before the relay accepts any protected traffic. The relay MUST call this and -//! refuse to start on error in [`Enforce`][crate::nip_fi::NipFiMode::Enforce] -//! mode (`FI-INV-14`, `FI-INV-15`). -//! -//! ## What it checks -//! -//! | Check | Why | -//! |-------|-----| -//! | Registry non-empty | An enforce-mode deployment with no issuer policy admits nothing and the gap is undetectable at request time | -//! | Each issuer non-empty `iss` and `aud` | `IssuerPolicy` validates these, but startup re-asserts the invariant at the registry level | -//! | No duplicate `iss` | A duplicate would silently pick one policy; enforce uniqueness | -//! | `current-status` requires `maximum_status_age_seconds` | Already enforced in `IssuerPolicy::new`; startup confirms no offline-mode policy sneaked through with a status-age | -//! | Offline-only deployments: `FreshnessClass::OfflineJwt` is safe | No residual bound claim (per NIP-FI.md:259-266) | -//! | JWKS config present for every issuer in enforce mode | Every issuer needs a reachable key source | -//! | JWKS config issuer match | The JWKS config `issuer` must equal the policy `issuer` | -//! | `refresh_interval` < `hard_deadline` | Prevents an always-stale cache | +//! refuse to start on error in [`Enforce`][NipFiMode::Enforce] mode +//! (`FI-INV-14`, `FI-INV-15`). use super::config::{FreshnessClass, IssuerRegistry}; -use super::jwks::IssuerJwksConfig; +use super::jwks::{validate_jwks_uri, IssuerJwksConfig, MAX_JWKS_TIMING_SECONDS}; -/// Operating mode for the NIP-FI assertion runtime. -/// -/// The variant names are stable contract values; do not rename without a +/// Variant names are stable contract values; do not rename without a /// `VERIFIER_CONTRACT_VERSION` bump. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum NipFiMode { @@ -34,124 +19,117 @@ pub enum NipFiMode { /// federated assertion evidence. The relay MUST call /// [`validate_nip_fi_config`] before accepting traffic in this mode. Enforce, - /// Emergency mode: all protected routes deny before any verifier is - /// configured. Used during startup if a previous enforce-mode deployment - /// was misconfigured and must fail closed while the operator repairs - /// configuration. [FI-INV-14] + /// All protected routes deny unconditionally. Used when a prior + /// enforce-mode deployment was misconfigured and must fail closed while + /// the operator repairs configuration. [FI-INV-14] DenyProtected, } -/// Reasons [`validate_nip_fi_config`] rejects a configuration. -/// /// Every variant corresponds to a concrete, operator-actionable defect. /// No key material, token bytes, or raw claim values appear. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] pub enum NipFiStartupError { - /// Enforce mode requires at least one issuer policy; the registry is empty. + /// Registry has no entries; enforce mode requires at least one issuer. #[error("NIP-FI enforce mode requires at least one issuer policy")] EmptyRegistry, - /// Two or more issuer policies share the same `iss` value, which would - /// make issuer selection ambiguous. - #[error("NIP-FI issuer registry contains duplicate issuer: {0}")] - DuplicateIssuer(String), + /// The duplicate `iss` is omitted to avoid leaking configuration into + /// operational logs. + #[error("NIP-FI issuer registry contains a duplicate issuer")] + DuplicateIssuer, - /// Enforce mode requires a JWKS config for every registered issuer, but - /// the given issuer has no JWKS configuration. - #[error("NIP-FI issuer has no JWKS configuration: (issuer redacted)")] + /// Every registered issuer requires a JWKS endpoint in enforce mode. + #[error("NIP-FI issuer has no JWKS configuration")] MissingJwksConfig, - /// A JWKS config's `issuer` field does not match any registered issuer - /// policy. Mismatched configs are rejected to prevent silent key-source - /// confusion. + /// Mismatched configs are rejected to prevent silent key-source confusion. #[error("NIP-FI JWKS config issuer does not match any registered policy")] UnmatchedJwksConfig, - /// A JWKS config's `refresh_interval_seconds` is zero or is greater than - /// or equal to `key_snapshot_hard_deadline_seconds`. - #[error("NIP-FI JWKS config has invalid timing bounds: refresh >= hard deadline")] + /// `refresh_interval_seconds` is zero, exceeds [`MAX_JWKS_TIMING_SECONDS`], + /// or is ≥ `key_snapshot_hard_deadline_seconds`. + #[error("NIP-FI JWKS config has invalid timing bounds")] InvalidJwksTiming, - /// A `current-status` issuer policy is present but the JWKS URI is - /// absent; current-status requires a reachable JWKS to validate assertion - /// signatures. - #[error("NIP-FI current-status issuer requires a JWKS configuration")] - CurrentStatusRequiresJwks, + /// Non-HTTPS scheme, embedded credentials, fragment, or bare + /// private/reserved IP host. See [`validate_jwks_uri`]. + #[error("NIP-FI JWKS URI failed safety validation")] + InvalidJwksUri, + + /// `current-status` requires an authenticated status witness that is not + /// yet implemented. Use `FreshnessClass::OfflineJwt` instead. + #[error( + "NIP-FI current-status freshness is not yet supported; \ + use offline-jwt posture" + )] + UnsupportedPosture, } -/// Validate the complete NIP-FI runtime configuration before the relay -/// accepts any protected traffic. -/// -/// `registry` is the set of issuer policies. `jwks_configs` is the set of -/// JWKS endpoint configurations (one per issuer in enforce mode). -/// `mode` is the intended operating mode. -/// -/// Returns `Ok(())` when the configuration is valid and complete for `mode`. -/// Returns `Err(NipFiStartupError)` when any invariant is violated; the relay -/// MUST refuse to start or must fall back to [`NipFiMode::DenyProtected`]. +/// Validates the complete NIP-FI runtime configuration. On error the relay +/// MUST refuse to start or fall back to [`NipFiMode::DenyProtected`]. pub fn validate_nip_fi_config( mode: NipFiMode, registry: &IssuerRegistry, jwks_configs: &[IssuerJwksConfig], ) -> Result<(), NipFiStartupError> { - match mode { - NipFiMode::Off | NipFiMode::DenyProtected => { - // Off and emergency-denial modes impose no assertion config - // requirements — they admit nothing. - return Ok(()); - } - NipFiMode::Enforce => {} + if let NipFiMode::Off | NipFiMode::DenyProtected = mode { + return Ok(()); } - // Enforce mode: validate the registry and JWKS configs. - if registry.is_empty() { return Err(NipFiStartupError::EmptyRegistry); } - // Check for duplicate issuers (IssuerRegistry keyed by exact iss, so this - // is already enforced there, but we assert it explicitly for startup). + // IssuerRegistry overwrites duplicates silently; assert uniqueness here so + // a misconfigured multi-issuer call-site is caught before traffic is served. { let mut seen = std::collections::HashSet::new(); for policy in registry.all_policies() { if !seen.insert(policy.issuer()) { - return Err(NipFiStartupError::DuplicateIssuer( - policy.issuer().to_owned(), - )); + return Err(NipFiStartupError::DuplicateIssuer); } } } - // Build a map from issuer → JWKS config for O(1) lookup. - let jwks_map: std::collections::HashMap<&str, &IssuerJwksConfig> = jwks_configs - .iter() - .map(|c| (c.issuer.as_str(), c)) - .collect(); + // Reject current-status policies: the status witness is not yet + // implemented. Fail closed rather than advertise a freshness guarantee the + // verifier cannot satisfy. + for policy in registry.all_policies() { + if policy.freshness() == FreshnessClass::CurrentStatus { + return Err(NipFiStartupError::UnsupportedPosture); + } + } + + // Build JWKS map, rejecting duplicates. Two configs for the same issuer + // would make the effective endpoint selection order-dependent. + let mut jwks_map: std::collections::HashMap<&str, &IssuerJwksConfig> = + std::collections::HashMap::with_capacity(jwks_configs.len()); + for config in jwks_configs { + if jwks_map.insert(config.issuer.as_str(), config).is_some() { + return Err(NipFiStartupError::DuplicateIssuer); + } + } - // Verify every JWKS config references a known issuer. for config in jwks_configs { if registry.policy_for_issuer(&config.issuer).is_none() { return Err(NipFiStartupError::UnmatchedJwksConfig); } - // Validate timing bounds. if config.refresh_interval_seconds == 0 || config.key_snapshot_hard_deadline_seconds == 0 || config.key_snapshot_hard_deadline_seconds <= config.refresh_interval_seconds + || config.refresh_interval_seconds > MAX_JWKS_TIMING_SECONDS + || config.key_snapshot_hard_deadline_seconds > MAX_JWKS_TIMING_SECONDS { return Err(NipFiStartupError::InvalidJwksTiming); } + if validate_jwks_uri(&config.jwks_uri).is_err() { + return Err(NipFiStartupError::InvalidJwksUri); + } } - // Every issuer policy must have a JWKS config in enforce mode. for policy in registry.all_policies() { - match jwks_map.get(policy.issuer()) { - None => { - if policy.freshness() == FreshnessClass::CurrentStatus { - return Err(NipFiStartupError::CurrentStatusRequiresJwks); - } - return Err(NipFiStartupError::MissingJwksConfig); - } - Some(_) => {} + if !jwks_map.contains_key(policy.issuer()) { + return Err(NipFiStartupError::MissingJwksConfig); } } diff --git a/crates/buzz-auth/src/nip_fi/startup/tests.rs b/crates/buzz-auth/src/nip_fi/startup/tests.rs index 14924d5c9c..12455f98b0 100644 --- a/crates/buzz-auth/src/nip_fi/startup/tests.rs +++ b/crates/buzz-auth/src/nip_fi/startup/tests.rs @@ -1,11 +1,8 @@ -//! Unit tests for NIP-FI startup validation (Phase A, PR 3). - use super::*; use crate::nip_fi::config::{FreshnessClass, IssuerPolicy, IssuerRegistry, TokenClass}; use crate::nip_fi::jwks::IssuerJwksConfig; use jsonwebtoken::Algorithm as JwtAlgorithm; -/// Build a minimal valid offline-jwt `IssuerPolicy`. fn make_offline_policy(issuer: &str) -> IssuerPolicy { IssuerPolicy::new( issuer.to_owned(), @@ -21,7 +18,6 @@ fn make_offline_policy(issuer: &str) -> IssuerPolicy { .unwrap() } -/// Build a minimal valid current-status `IssuerPolicy`. fn make_status_policy(issuer: &str) -> IssuerPolicy { IssuerPolicy::new( issuer.to_owned(), @@ -46,8 +42,6 @@ fn make_jwks_config(issuer: &str) -> IssuerJwksConfig { } } -// ── Off / DenyProtected accept anything ─────────────────────────────────────── - #[test] fn off_mode_accepts_empty_registry() { let registry = IssuerRegistry::new(); @@ -60,16 +54,15 @@ fn deny_protected_mode_accepts_empty_registry() { assert!(validate_nip_fi_config(NipFiMode::DenyProtected, ®istry, &[]).is_ok()); } -// ── Enforce: basic happy path ───────────────────────────────────────────────── - #[test] fn enforce_valid_config_passes() { let issuer = "https://id.example"; let mut registry = IssuerRegistry::new(); registry.insert(make_offline_policy(issuer)); - let jwks = vec![make_jwks_config(issuer)]; - assert!(validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).is_ok()); + assert!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &[make_jwks_config(issuer)]).is_ok() + ); } #[test] @@ -87,8 +80,6 @@ fn enforce_multiple_issuers_passes() { assert!(validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).is_ok()); } -// ── Enforce: empty registry ─────────────────────────────────────────────────── - #[test] fn enforce_empty_registry_rejects() { let registry = IssuerRegistry::new(); @@ -96,8 +87,6 @@ fn enforce_empty_registry_rejects() { assert_eq!(err, NipFiStartupError::EmptyRegistry); } -// ── Enforce: missing JWKS config ───────────────────────────────────────────── - #[test] fn enforce_issuer_without_jwks_rejects() { let issuer = "https://id.example"; @@ -108,22 +97,21 @@ fn enforce_issuer_without_jwks_rejects() { assert_eq!(err, NipFiStartupError::MissingJwksConfig); } -// ── Enforce: unmatched JWKS config ─────────────────────────────────────────── - #[test] fn enforce_unmatched_jwks_config_rejects() { let issuer = "https://id.example"; let mut registry = IssuerRegistry::new(); registry.insert(make_offline_policy(issuer)); - // JWKS config for a different issuer. - let jwks = vec![make_jwks_config("https://other.example")]; - let err = validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(); + let err = validate_nip_fi_config( + NipFiMode::Enforce, + ®istry, + &[make_jwks_config("https://other.example")], + ) + .unwrap_err(); assert_eq!(err, NipFiStartupError::UnmatchedJwksConfig); } -// ── Enforce: invalid JWKS timing ───────────────────────────────────────────── - #[test] fn enforce_refresh_equals_hard_deadline_rejects() { let issuer = "https://id.example"; @@ -136,8 +124,10 @@ fn enforce_refresh_equals_hard_deadline_rejects() { refresh_interval_seconds: 3600, key_snapshot_hard_deadline_seconds: 3600, }]; - let err = validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(); - assert_eq!(err, NipFiStartupError::InvalidJwksTiming); + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(), + NipFiStartupError::InvalidJwksTiming + ); } #[test] @@ -152,34 +142,77 @@ fn enforce_zero_refresh_interval_rejects() { refresh_interval_seconds: 0, key_snapshot_hard_deadline_seconds: 3600, }]; - let err = validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(); - assert_eq!(err, NipFiStartupError::InvalidJwksTiming); + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(), + NipFiStartupError::InvalidJwksTiming + ); } -// ── current-status requires JWKS ───────────────────────────────────────────── - +/// Rejected regardless of whether a JWKS config is present — the verifier +/// has no status witness to satisfy the freshness guarantee. #[test] -fn enforce_current_status_without_jwks_rejects() { +fn enforce_current_status_policy_always_rejects() { let issuer = "https://id.example"; let mut registry = IssuerRegistry::new(); registry.insert(make_status_policy(issuer)); - let err = validate_nip_fi_config(NipFiMode::Enforce, ®istry, &[]).unwrap_err(); - // Either CurrentStatusRequiresJwks or MissingJwksConfig is correct here; - // the current implementation returns CurrentStatusRequiresJwks. + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &[]).unwrap_err(), + NipFiStartupError::UnsupportedPosture + ); + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &[make_jwks_config(issuer)]) + .unwrap_err(), + NipFiStartupError::UnsupportedPosture + ); +} + +/// Duplicate JWKS configs for the same issuer must not silently succeed. +#[test] +fn enforce_duplicate_jwks_issuer_in_configs_rejects() { + let issuer = "https://id.example"; + let mut registry = IssuerRegistry::new(); + registry.insert(make_offline_policy(issuer)); + + let jwks = vec![make_jwks_config(issuer), make_jwks_config(issuer)]; assert!( - err == NipFiStartupError::CurrentStatusRequiresJwks - || err == NipFiStartupError::MissingJwksConfig, - "expected a JWKS-missing error, got {err:?}" + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).is_err(), + "duplicate JWKS configs must not pass" ); } #[test] -fn enforce_current_status_with_jwks_passes() { +fn enforce_non_https_jwks_uri_rejects() { let issuer = "https://id.example"; let mut registry = IssuerRegistry::new(); - registry.insert(make_status_policy(issuer)); + registry.insert(make_offline_policy(issuer)); - let jwks = vec![make_jwks_config(issuer)]; - assert!(validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).is_ok()); + let jwks = vec![IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: "http://id.example/.well-known/jwks.json".to_owned(), + refresh_interval_seconds: 300, + key_snapshot_hard_deadline_seconds: 3600, + }]; + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(), + NipFiStartupError::InvalidJwksUri + ); +} + +#[test] +fn enforce_loopback_jwks_uri_rejects() { + let issuer = "https://id.example"; + let mut registry = IssuerRegistry::new(); + registry.insert(make_offline_policy(issuer)); + + let jwks = vec![IssuerJwksConfig { + issuer: issuer.to_owned(), + jwks_uri: "https://127.0.0.1/.well-known/jwks.json".to_owned(), + refresh_interval_seconds: 300, + key_snapshot_hard_deadline_seconds: 3600, + }]; + assert_eq!( + validate_nip_fi_config(NipFiMode::Enforce, ®istry, &jwks).unwrap_err(), + NipFiStartupError::InvalidJwksUri + ); }