berkus
/
wifi-densepose
mirror of https://github.com/ruvnet/wifi-densepose.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(auth): scope gate was fail-OPEN; JWKS lock held across a blocking fetch 

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
This commit is contained in:
Dragan Spiridonov 2026-07-23 09:18:33 +02:00
parent 3347e258e6
commit 7a05417493
2 changed files with 238 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

124
v2/crates/ruview-auth/src/jwks.rs
View File

@ -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<DecodingKey, JwksError> {
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<HashMap<String, DecodingKey>, JwksError> {

179
v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs
View File

@ -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
);
}
}