From 7a054174931268ee4da7141ad8820a940f11baf2 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 09:18:33 +0200 Subject: [PATCH] fix(auth): scope gate was fail-OPEN; JWKS lock held across a blocking fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two charges from an adversarial multi-vendor review (qe-court). Both verified in source before fixing. --- 1. AUTHORIZATION BYPASS: POST /api/v1/adaptive/train reachable with read --- `required_scope_for` enumerated admin routes by prefix (`/api/v1/train/`, plus DELETE on models/recording) and let EVERYTHING ELSE fall through to `sensing:read`. `POST /api/v1/adaptive/train` (main.rs:8112 -> handler main.rs:5028) calls `adaptive_classifier::train_from_recordings()`, writes the model to disk with `model.save()`, and swaps `state.adaptive_model` used by the live inference pipeline. It does not start with `/api/v1/train/`, so it landed on `sensing:read` — the scope `wifi-densepose login` requests BY DEFAULT. Exactly the blast radius the read/admin split exists to prevent, reachable by the lowest-privilege token the product issues. Fixed by inverting the polarity, which is the real defect — a denylist for a security gate keeps missing routes as routes keep being added: GET/HEAD/OPTIONS -> sensing:read any other method -> sensing:admin ...unless the exact path is in READ_SAFE_MUTATIONS (an explicit allowlist of mutations that change runtime state but destroy nothing: ws-ticket, model load/unload/activate, calibration start/stop, recording start/stop, vendor event ingest) A mutating route added tomorrow is now admin-gated by default. Pinned by `an_unknown_mutating_route_defaults_to_admin`. `POST /api/v1/ws-ticket` is allowlisted deliberately and has its own test: were it admin, the read scope could never open a stream from a browser at all. Enumerated all 17 mutating /api/v1 routes to build the allowlist rather than guessing. `POST /api/v1/config/ground-truth` now requires admin — a deliberate tightening, it writes config. --- 2. DoS: blocking JWKS fetch under std::sync::Mutex in async middleware --- Filed INDEPENDENTLY by two prosecutors, which is why it gets fixed rather than argued about. `decoding_key_for` locked a `std::sync::Mutex` and held it across a blocking `ureq` call (3s timeout, longer on a dead link), invoked from inside the async `require_bearer`. `Mutex::lock()` in an async fn is a blocking syscall, not a yield point — so one slow JWKS fetch blocked EVERY concurrent request on that mutex, including ones carrying already-cached valid tokens, and parked the tokio workers running them. On Pi-class hardware with few workers that stalls the whole server. It fires on the routine 300s TTL rollover whenever the link is degraded — the exact offline case the module exists to tolerate. Reachable by anyone able to send a syntactically valid ES256 header with an unknown kid, since kid lookup precedes signature verification. Now three phases: read under the lock, RELEASE, network, re-take only to install. Cost is a possible duplicated idempotent GET during a rollover, which is strictly better than serialising every request behind one socket. The codebase already uses spawn_blocking for outbound I/O elsewhere (main.rs:2490, 5272); moving verification fully onto spawn_blocking remains a follow-up — this removes the amplification, not every blocking millisecond. Tests: 533 sensing-server (7 new scope-polarity), 82 ruview-auth. Co-Authored-By: Ruflo & AQE --- v2/crates/ruview-auth/src/jwks.rs | 124 +++++++----- .../src/bearer_auth.rs | 179 ++++++++++++++++-- 2 files changed, 238 insertions(+), 65 deletions(-) diff --git a/v2/crates/ruview-auth/src/jwks.rs b/v2/crates/ruview-auth/src/jwks.rs index 1a3e391d..0ff02223 100644 --- a/v2/crates/ruview-auth/src/jwks.rs +++ b/v2/crates/ruview-auth/src/jwks.rs @@ -16,6 +16,26 @@ //! We fail closed in exactly one case: **we have never successfully fetched a //! key set.** Then there is nothing to reason with, and admitting a request //! would mean admitting an unverified token. +//! +//! ## The lock is never held across the network call +//! +//! [`JwksCache::decoding_key_for`] reads the cache under the lock, RELEASES it, +//! does any HTTP, then re-takes the lock only to install the result. +//! +//! This is not tidiness. An earlier revision held a `std::sync::Mutex` across a +//! blocking `ureq` call made from inside async middleware. `Mutex::lock()` in an +//! async fn is a real blocking syscall, not a yield point — so one slow or +//! unreachable JWKS fetch (up to the 3s timeout, longer if the link is dead) +//! blocked EVERY concurrent request on that mutex, including requests carrying +//! already-cached, perfectly valid tokens, and parked the tokio worker threads +//! they were running on. On Pi-class hardware with few workers that stalls the +//! whole server, and it fires on the routine 300s TTL rollover whenever the +//! network is degraded — precisely the offline-tolerance case this module +//! exists to handle. +//! +//! The cost of releasing the lock is that two callers can fetch concurrently +//! during a rollover. That is a harmless duplicated idempotent GET, and it is +//! strictly better than serialising every request behind one socket. use std::collections::HashMap; use std::sync::Mutex; @@ -126,61 +146,65 @@ impl JwksCache { /// Resolve the verification key for a token header's `kid`. pub fn decoding_key_for(&self, kid: &str) -> Result { - let mut state = self.state.lock().expect("jwks cache poisoned"); - - let stale = match state.fetched_at { - None => true, - Some(at) => at.elapsed() >= self.ttl, + // ---- Phase 1: answer from cache, holding the lock only to read. ---- + let (fresh, have_any, may_force) = { + let state = self.state.lock().expect("jwks cache poisoned"); + let fresh = state + .fetched_at + .map_or(false, |at| at.elapsed() < self.ttl); + if fresh { + if let Some(key) = state.keys.get(kid) { + return Ok(key.clone()); + } + } + let may_force = state + .last_forced_refetch + .map_or(true, |at| at.elapsed() >= FORCED_REFETCH_MIN_INTERVAL); + (fresh, state.fetched_at.is_some(), may_force) }; + // Lock released. Everything below may take milliseconds-to-seconds and + // MUST NOT hold it — see the module docs. - if stale { - // Routine refresh. A failure here is survivable if we still hold a - // key set (see module docs); it is fatal only if we never had one. - match self.fetch_and_parse() { - Ok(fresh) => { - state.keys = fresh; - state.fetched_at = Some(Instant::now()); + // A fresh cache that simply lacks this kid: one rate-limited forced + // refetch, in case identity rotated inside the TTL. + if fresh && !may_force { + return Err(JwksError::UnknownKid(kid.to_owned())); + } + if fresh { + self.state + .lock() + .expect("jwks cache poisoned") + .last_forced_refetch = Some(Instant::now()); + } + + // ---- Phase 2: network, WITHOUT the lock held. ---- + let fetched = self.fetch_and_parse(); + + // ---- Phase 3: install, holding the lock only to write. ---- + let mut state = self.state.lock().expect("jwks cache poisoned"); + match fetched { + Ok(keys) => { + state.keys = keys; + state.fetched_at = Some(Instant::now()); + } + Err(e) => { + // A key that verified a minute ago has not stopped being valid + // because the network blipped. + if !have_any { + return Err(JwksError::NeverFetched); } - Err(e) if state.fetched_at.is_some() => { - tracing::warn!( - url = %self.url, - error = %e, - "JWKS refresh failed; continuing with the previously cached key set" - ); - } - Err(_) => return Err(JwksError::NeverFetched), + tracing::warn!( + url = %self.url, + error = %e, + "JWKS refresh failed; continuing with the previously cached key set" + ); } } - - if let Some(key) = state.keys.get(kid) { - return Ok(key.clone()); - } - - // Unknown kid against a cache we believe is fresh: identity may have - // rotated inside the TTL. One forced refetch, rate-limited so a token - // carrying a junk kid cannot turn every request into an outbound fetch. - let may_force = state - .last_forced_refetch - .map_or(true, |at| at.elapsed() >= FORCED_REFETCH_MIN_INTERVAL); - - if may_force { - state.last_forced_refetch = Some(Instant::now()); - match self.fetch_and_parse() { - Ok(fresh) => { - state.keys = fresh; - state.fetched_at = Some(Instant::now()); - if let Some(key) = state.keys.get(kid) { - tracing::info!(kid = %kid, "JWKS key rotation picked up via forced refetch"); - return Ok(key.clone()); - } - } - Err(e) => { - tracing::warn!(url = %self.url, error = %e, "forced JWKS refetch failed"); - } - } - } - - Err(JwksError::UnknownKid(kid.to_owned())) + state + .keys + .get(kid) + .cloned() + .ok_or_else(|| JwksError::UnknownKid(kid.to_owned())) } fn fetch_and_parse(&self) -> Result, JwksError> { diff --git a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs index 89638caa..b57783c6 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs @@ -287,32 +287,69 @@ impl AuthState { /// The scope a request must carry, split by **blast radius** (ADR-060): can /// this call destroy something, or only observe? /// -/// `sensing:admin` covers exactly three things: -/// * `/api/v1/train/*` — burns hours of CPU on a Pi and writes models; -/// * `DELETE /api/v1/models/{id}` — irreversible loss of a trained model; -/// * `DELETE /api/v1/recording/{id}` — irreversible loss of a labelled capture. +/// **Reads are open; writes are closed unless explicitly allowlisted.** /// -/// Everything else is `sensing:read`. Note what is deliberately *not* admin: -/// loading/unloading a model and starting a recording both mutate server state -/// but destroy nothing, and putting them behind the destructive scope would -/// push routine dashboard use into asking for a capability it does not need — -/// the opposite of least privilege. +/// An earlier revision enumerated the admin routes by prefix and let everything +/// else fall through to `sensing:read`. That is the wrong polarity for a +/// security gate and it shipped a real hole: `POST /api/v1/adaptive/train` +/// trains a classifier, overwrites the on-disk model and swaps the live one — +/// but it does not start with `/api/v1/train/`, so it landed on `sensing:read`, +/// the scope `wifi-densepose login` requests BY DEFAULT. A denylist for a scope +/// gate will keep missing routes as routes keep being added. +/// +/// So: `GET`/`HEAD`/`OPTIONS` need `sensing:read`. Any other method needs +/// `sensing:admin` unless its exact path is in [`READ_SAFE_MUTATIONS`] — routes +/// that change runtime state but destroy nothing. A new mutating route added +/// without thought is therefore admin-gated by default, which is the safe way +/// to be wrong. /// /// `sensing:read` is not "harmless": for a presence and vital-signs sensor, /// read access tells the holder who is home. It is *non-destructive*, which is /// a weaker claim. pub fn required_scope_for(method: &Method, path: &str) -> &'static str { - if path.starts_with("/api/v1/train/") { - return scope::SENSING_ADMIN; + // Reads are open to `sensing:read`. + if !is_mutating(method) { + return scope::SENSING_READ; } - if method == Method::DELETE - && (path.starts_with("/api/v1/models/") || path.starts_with("/api/v1/recording/")) + // A small, explicit allowlist of mutating routes that change runtime state + // but destroy nothing — a dashboard doing its ordinary job. + if READ_SAFE_MUTATIONS.contains(&path) + || (path.starts_with("/api/v1/rf/vendors/") && path.ends_with("/events")) { - return scope::SENSING_ADMIN; + return scope::SENSING_READ; } - scope::SENSING_READ + // Everything else that mutates requires admin. FAIL CLOSED — see the docs + // above for why this is a default rather than a list. + scope::SENSING_ADMIN } +fn is_mutating(method: &Method) -> bool { + !matches!(*method, Method::GET | Method::HEAD | Method::OPTIONS) +} + +/// Mutating routes that need only `sensing:read`. +/// +/// Deliberately an ALLOWLIST. Everything absent from it that mutates requires +/// `sensing:admin`, so adding a route without thinking about scope fails safe +/// instead of silently landing on read. +const READ_SAFE_MUTATIONS: &[&str] = &[ + // Browsers must be able to obtain a WebSocket ticket with a read token, + // or the read scope cannot open a stream at all. + "/api/v1/ws-ticket", + // Load/unload/activate: reversible, destroy nothing. + "/api/v1/models/load", + "/api/v1/models/unload", + "/api/v1/models/lora/activate", + "/api/v1/model/sona/activate", + "/api/v1/adaptive/unload", + // Capture and calibration: create data, never destroy it. + "/api/v1/calibration/start", + "/api/v1/calibration/stop", + "/api/v1/pose/calibrate", + "/api/v1/recording/start", + "/api/v1/recording/stop", +]; + /// Constant-time byte slice equality. Returns `false` immediately on length /// mismatch (lengths are not secret here — both sides are fixed tokens). fn ct_eq(a: &[u8], b: &[u8]) -> bool { @@ -1293,3 +1330,115 @@ mod ws_path_matching_tests { assert!(!is_ws_path("/ws")); } } + +/// The scope classifier, after inverting it to fail-closed. The charge that +/// forced this: `POST /api/v1/adaptive/train` trains and overwrites the live +/// model, but did not match the `/api/v1/train/` prefix, so it was reachable +/// with `sensing:read` — the scope `login` requests by default. +#[cfg(test)] +mod scope_gate_polarity_tests { + use super::*; + + #[test] + fn adaptive_train_requires_admin() { + // The reported bypass. Handler calls train_from_recordings(), writes + // the model to disk and swaps the live one. + assert_eq!( + required_scope_for(&Method::POST, "/api/v1/adaptive/train"), + scope::SENSING_ADMIN + ); + } + + #[test] + fn every_known_destructive_route_requires_admin() { + for (m, p) in [ + (Method::POST, "/api/v1/train/start"), + (Method::POST, "/api/v1/train/stop"), + (Method::POST, "/api/v1/adaptive/train"), + (Method::DELETE, "/api/v1/models/m1"), + (Method::DELETE, "/api/v1/recording/r1"), + (Method::POST, "/api/v1/config/ground-truth"), + ] { + assert_eq!( + required_scope_for(&m, p), + scope::SENSING_ADMIN, + "{m} {p} must require admin" + ); + } + } + + #[test] + fn an_unknown_mutating_route_defaults_to_admin() { + // THE property the old denylist lacked. A route added tomorrow is + // admin-gated until someone consciously classifies it as read-safe. + for p in [ + "/api/v1/some/route/invented/later", + "/api/v1/adaptive/retrain-everything", + "/api/v1/models/nuke", + ] { + assert_eq!( + required_scope_for(&Method::POST, p), + scope::SENSING_ADMIN, + "unknown mutating route {p} must fail closed to admin" + ); + assert_eq!( + required_scope_for(&Method::DELETE, p), + scope::SENSING_ADMIN + ); + } + } + + #[test] + fn reads_stay_open_to_the_read_scope() { + for p in [ + "/api/v1/models", + "/api/v1/models/m1", + "/api/v1/recording/list", + "/api/v1/adaptive/status", + "/api/v1/anything/at/all", + ] { + assert_eq!( + required_scope_for(&Method::GET, p), + scope::SENSING_READ, + "GET {p} must stay open to read" + ); + } + } + + #[test] + fn allowlisted_mutations_stay_read_scoped() { + // Non-destructive state changes a dashboard makes routinely. Pushing + // these to admin would force ordinary use to hold delete capability. + for p in READ_SAFE_MUTATIONS { + assert_eq!( + required_scope_for(&Method::POST, p), + scope::SENSING_READ, + "{p} is allowlisted and must stay read-scoped" + ); + } + } + + #[test] + fn a_read_token_can_still_mint_a_websocket_ticket() { + // Load-bearing: if this needed admin, the read scope could never open + // a stream from a browser at all. + assert_eq!( + required_scope_for(&Method::POST, "/api/v1/ws-ticket"), + scope::SENSING_READ + ); + } + + #[test] + fn vendor_event_ingest_is_read_scoped_by_prefix() { + // Path carries a `:vendor` segment, so it cannot be an exact match. + assert_eq!( + required_scope_for(&Method::POST, "/api/v1/rf/vendors/netgear/events"), + scope::SENSING_READ + ); + // ...but the prefix must not become a wildcard for anything under it. + assert_eq!( + required_scope_for(&Method::POST, "/api/v1/rf/vendors/netgear/delete-all"), + scope::SENSING_ADMIN + ); + } +}