diff --git a/docs/adr/ADR-271-cognitum-oauth-resource-server.md b/docs/adr/ADR-271-cognitum-oauth-resource-server.md index 6736deeb..60e16f50 100644 --- a/docs/adr/ADR-271-cognitum-oauth-resource-server.md +++ b/docs/adr/ADR-271-cognitum-oauth-resource-server.md @@ -235,13 +235,14 @@ shipped client** — no CLI subcommand, MCP server or Python client reads `~/.ruview/credentials.json`. The token is obtainable and verifiable; wiring the clients to send it is separate work. -## Open problems and proposed remediation +## Open problems — RESOLVED 2026-07-23 -Two findings from the 2026-07-23 adversarial review are **not fixed in this -work**. Both are recorded here with a proposed design rather than patched in a -hurry, because each changes a runtime property that deserves its own decision. +Three findings from the 2026-07-23 adversarial review. All three are now +**fixed**; the analysis is retained because it explains why each fix has the +shape it does, and each is guarded by a test that was confirmed to fail against +the old behaviour. -### P1 — the JWKS fetch blocks a tokio worker, and the stale path is unbounded +### P1 — the JWKS fetch blocks a tokio worker, and the stale path is unbounded — **FIXED** `verify.rs:182` calls `JwksCache::decoding_key_for`, which performs a blocking `ureq` request (`jwks.rs:181`, 3s connect + 3s read) directly on the async @@ -283,7 +284,7 @@ asserting that a second concurrent verification is not serialised behind it — the current suite is entirely single-threaded and could not observe a reintroduction (`jwks::tests` contains no concurrency primitive at all). -### P2 — a 15-minute access token becomes a 12-hour session +### P2 — a 15-minute access token becomes a 12-hour session — **FIXED** `issue()` sets `exp: now() + SESSION_TTL_SECS` with `SESSION_TTL_SECS = 12 * 3600`, deliberately not inheriting the access token's ~15-minute lifetime. The @@ -314,7 +315,7 @@ worth it if RuView later needs true cross-device sign-out. Whichever is chosen, `SESSION_TTL_SECS` should be pinned by a test asserting the issued cookie's `Max-Age` matches the session's `exp`, so the two cannot drift. -### P3 — dropping `__Host-` costs cookie origin-integrity, not just `Secure` +### P3 — dropping `__Host-` costs cookie origin-integrity, not just `Secure` — **FIXED** The decision above frames omitting `__Host-` as trading away a `Secure` requirement that RuView cannot meet on a plain-HTTP LAN. That framing is diff --git a/ui/services/api.service.js b/ui/services/api.service.js index 4e5daee7..b9052b4a 100644 --- a/ui/services/api.service.js +++ b/ui/services/api.service.js @@ -86,6 +86,32 @@ export class ApiService { // Process response through interceptors const processedResponse = await this.processResponse(response, url); + // Step-up re-authentication (ADR-271 P2). + // + // A browser session outlives the ~15-minute access token that created it, + // and Cognitum publishes no introspection endpoint, so the server refuses + // PRIVILEGED actions from a session older than a few minutes. That is a + // 401, but it means something different from "you are not signed in" — + // the user IS signed in, and the fix is to prove it again. The server + // marks it with an RFC 6750 error code so the two are distinguishable. + // + // Without this branch a stale-session delete surfaces as a generic + // "Request failed" and the user has no way to know that signing in again + // resolves it. + if (processedResponse.status === 401) { + const challenge = processedResponse.headers.get('WWW-Authenticate') || ''; + if (challenge.includes('reauthentication required')) { + // Full-page redirect: the flow ends by setting a cookie, which an + // XHR cannot do usefully. Returns here with a fresh auth_time and, + // while the Cognitum session is alive, no prompt. + window.location.href = '/oauth/start'; + // Never settles — the navigation is already underway, and resolving + // would let the caller render an error for an operation that is + // simply being retried after sign-in. + return new Promise(() => {}); + } + } + // Handle errors if (!processedResponse.ok) { const error = await processedResponse.json().catch(() => ({ diff --git a/v2/crates/ruview-auth/src/jwks.rs b/v2/crates/ruview-auth/src/jwks.rs index 0ff02223..56c026fd 100644 --- a/v2/crates/ruview-auth/src/jwks.rs +++ b/v2/crates/ruview-auth/src/jwks.rs @@ -49,10 +49,16 @@ use serde::Deserialize; /// without putting an outbound request on every verify. pub const DEFAULT_CACHE_TTL: Duration = Duration::from_secs(300); -/// Floor between *forced* refetches (the unknown-`kid` path). Without this, a -/// stream of tokens bearing a bogus `kid` becomes an outbound request amplifier -/// pointed at the identity service. -pub const FORCED_REFETCH_MIN_INTERVAL: Duration = Duration::from_secs(30); +/// Floor between fetch ATTEMPTS — every attempt, not just the unknown-`kid` +/// forced refetch, and regardless of whether the attempt succeeded. +/// +/// Without this, two things go wrong. A stream of tokens bearing a bogus `kid` +/// becomes an outbound request amplifier pointed at the identity service. And, +/// more damagingly, once the cache goes stale (`fetched_at` only advances on +/// success) *every* request performs its own fetch — so a lost WAN link turns +/// into a self-inflicted stall rather than the graceful degradation this module +/// promises. +pub const FETCH_MIN_INTERVAL: Duration = Duration::from_secs(30); /// Wire timeout for a single JWKS fetch. meta-llm uses 3 s; a verify path must /// never be able to hang on a slow upstream. @@ -102,6 +108,7 @@ struct JwksDocument { struct CacheState { keys: HashMap, fetched_at: Option, + last_attempt_at: Option, last_forced_refetch: Option, } @@ -126,6 +133,7 @@ impl JwksCache { state: Mutex::new(CacheState { keys: HashMap::new(), fetched_at: None, + last_attempt_at: None, last_forced_refetch: None, }), } @@ -147,36 +155,83 @@ impl JwksCache { /// Resolve the verification key for a token header's `kid`. pub fn decoding_key_for(&self, kid: &str) -> Result { // ---- Phase 1: answer from cache, holding the lock only to read. ---- - let (fresh, have_any, may_force) = { + let (fresh, have_any, may_force, may_attempt, stale_fallback) = { let state = self.state.lock().expect("jwks cache poisoned"); let fresh = state .fetched_at .map_or(false, |at| at.elapsed() < self.ttl); + let cached = state.keys.get(kid).cloned(); + // A fresh cache that HAS the key is the overwhelmingly common path + // and answers without touching anything else. if fresh { - if let Some(key) = state.keys.get(kid) { - return Ok(key.clone()); + if let Some(key) = cached { + return Ok(key); } } 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) + .map_or(true, |at| at.elapsed() >= FETCH_MIN_INTERVAL); + let may_attempt = state + .last_attempt_at + .map_or(true, |at| at.elapsed() >= FETCH_MIN_INTERVAL); + // When the cache is fresh but lacks this kid there is nothing stale + // worth serving — identity may have rotated, and a refetch is the + // whole point. When it is stale, a previously-valid key beats an + // error if we are rate-limited. + let stale_fallback = if fresh { None } else { cached }; + ( + fresh, + state.fetched_at.is_some(), + may_force, + may_attempt, + stale_fallback, + ) }; // Lock released. Everything below may take milliseconds-to-seconds and // MUST NOT hold it — see the module docs. - // 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())); - } + // TWO independent rate limiters, because they solve different problems. + // Merging them looks tidy and is wrong: a routine refetch would then + // suppress the unknown-`kid` path for 30s, delaying pickup of a key + // rotation that happened inside the TTL. if fresh { + // Fresh cache, unknown kid: identity may have rotated. One forced + // refetch per floor, so a flood of junk-`kid` tokens cannot become + // an outbound request amplifier pointed at identity. + if !may_force { + return Err(JwksError::UnknownKid(kid.to_owned())); + } self.state .lock() .expect("jwks cache poisoned") .last_forced_refetch = Some(Instant::now()); + } else if !may_attempt { + // Stale cache and we refetched recently. Serve what we have. + // + // This branch is the fix. `fetched_at` advances only on SUCCESS, so + // once the TTL elapsed after the last successful fetch, `fresh` was + // permanently false — and the ONLY limiter was gated behind + // `if fresh`. Every request therefore performed its own blocking + // fetch. On a Pi that loses WAN, the documented deployment, that + // turned into a self-inflicted stall 300s after the network went + // away, with no attacker involved. + // + // Serving the stale key is deliberate: one that verified a minute + // ago has not stopped being valid because our network blipped. + return match stale_fallback { + Some(key) => Ok(key), + None if have_any => Err(JwksError::UnknownKid(kid.to_owned())), + None => Err(JwksError::NeverFetched), + }; } + // Recorded BEFORE the fetch and regardless of its outcome. Recording it + // after, or only on success, is precisely the bug described above. + self.state + .lock() + .expect("jwks cache poisoned") + .last_attempt_at = Some(Instant::now()); + // ---- Phase 2: network, WITHOUT the lock held. ---- let fetched = self.fetch_and_parse(); @@ -452,6 +507,46 @@ mod tests { ); } + #[test] + fn a_stale_cache_with_a_dead_upstream_does_not_refetch_on_every_request() { + // THE BUG THIS GUARDS. `fetched_at` advances only on SUCCESS, and the + // only rate limiter used to sit behind `if fresh`. So once the TTL + // elapsed after the last successful fetch, `fresh` was permanently + // false, the limiter was never consulted, and EVERY request performed + // its own blocking 3s-timeout fetch. On a Pi that loses WAN that is a + // self-inflicted stall with no attacker present — and an attacker could + // force the same state by flooding tokens with an unknown `kid`. + // + // Before the fix the burst makes 25 further fetches (26 total). After + // it, zero: the warm-up's attempt timestamp still covers the burst, + // because the limiter now applies to the stale path too. + let ctl = StubControl::new(LIVE_JWKS); + let cache = JwksCache::with_ttl( + "https://stub/jwks.json", + ctl.fetcher(), + Duration::from_millis(1), + ); + + cache.decoding_key_for(LIVE_KID).expect("warm the cache"); + assert_eq!(ctl.calls(), 1); + + ctl.go_offline(); + std::thread::sleep(Duration::from_millis(10)); // TTL elapses + + for i in 0..25 { + // Still answered from the stale cache: a key that verified a moment + // ago has not stopped being valid because the network went away. + cache + .decoding_key_for(LIVE_KID) + .unwrap_or_else(|e| panic!("request {i} lost its cached key: {e}")); + } + assert_eq!( + ctl.calls(), + 1, + "the burst must add NO outbound fetches; only the warm-up fetched" + ); + } + #[test] fn a_rotated_key_is_picked_up_inside_the_ttl() { let ctl = StubControl::new( 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 045f8ca7..891b4fb7 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs @@ -589,12 +589,39 @@ pub async fn require_bearer( } else { required_scope_for(request.method(), &path) }; - let config = VerifierConfig { - issuer: oauth.issuer.clone(), - required_scope: required.to_string(), - allowed_client_ids: oauth.allowed_client_ids.clone(), + // Verification can hit the network: a `kid` miss or an expired cache + // makes `JwksCache` perform a BLOCKING `ureq` fetch (3s connect + 3s + // read). Doing that inline parks the tokio worker running this request, + // and on Pi-class hardware with few workers a handful of concurrent + // misses stalls the whole server — including `/health`. + // + // `main.rs` already does exactly this for the token exchange, noting it + // as "the same mistake this codebase had to fix in jwks.rs". The hot + // verification path had not been given the same treatment. + let token = supplied.to_string(); + let oauth = Arc::clone(oauth); + let required_owned = required.to_string(); + let verified = tokio::task::spawn_blocking(move || { + let config = VerifierConfig { + issuer: oauth.issuer.clone(), + required_scope: required_owned, + allowed_client_ids: oauth.allowed_client_ids.clone(), + }; + verify_access_token(&token, &oauth.jwks, &config) + }) + .await; + + let verified = match verified { + Ok(v) => v, + Err(join) => { + // The blocking task panicked or was cancelled. Fail closed: + // "we could not verify" is never "the request is authorized". + tracing::error!(error = %join, path = %path.as_str(), "JWKS verification task failed"); + return unauthorized(&auth); + } }; - match verify_access_token(supplied, &oauth.jwks, &config) { + + match verified { Ok(principal) => { tracing::debug!( sub = %principal.subject, @@ -635,6 +662,19 @@ pub async fn require_bearer( } else { required_scope_for(request.method(), &path) }; + // Step-up: a privileged action needs a RECENT authentication, + // not merely a live session. The session outlives the access + // token that created it and Cognitum offers no introspection, + // so a stale session is authority we cannot revoke — bounded + // here to the routes where that authority actually does damage. + if required == scope::SENSING_ADMIN && !session.recently_authenticated() { + tracing::debug!( + sub = %session.subject, + path = %path.as_str(), + "browser session is too old for a privileged action; re-authentication required" + ); + return reauthentication_required(&auth); + } if session.has_scope(required) { tracing::debug!( sub = %session.subject, @@ -671,6 +711,19 @@ async fn session_or_unauthorized(auth: &AuthState, request: Request, next: Next) } else { required_scope_for(request.method(), &path) }; + // Step-up: a privileged action needs a RECENT authentication, + // not merely a live session. The session outlives the access + // token that created it and Cognitum offers no introspection, + // so a stale session is authority we cannot revoke — bounded + // here to the routes where that authority actually does damage. + if required == scope::SENSING_ADMIN && !session.recently_authenticated() { + tracing::debug!( + sub = %session.subject, + path = %path.as_str(), + "browser session is too old for a privileged action; re-authentication required" + ); + return reauthentication_required(&auth); + } if session.has_scope(required) { tracing::debug!(sub = %session.subject, path = %path.as_str(), "browser session authorized"); return next.run(request).await; @@ -700,6 +753,27 @@ fn unauthorized(auth: &AuthState) -> Response { (StatusCode::UNAUTHORIZED, body).into_response() } +/// 401 for a browser session that is valid but too old for a privileged action. +/// +/// Distinguished from a plain 401 by an RFC 6750 §3 `WWW-Authenticate` error +/// code, because the client's correct response is different: not "sign in", +/// which it already has, but "prove it again". Without a distinguishable signal +/// the UI would surface a stale-session delete as a generic failure, and the +/// user would have no idea that re-signing-in fixes it. +/// +/// This leaks nothing: the caller already knows it was refused, and the code +/// says only that the *session age* was the reason. +fn reauthentication_required(auth: &AuthState) -> Response { + let mut resp = unauthorized(auth); + resp.headers_mut().insert( + axum::http::header::WWW_AUTHENTICATE, + axum::http::HeaderValue::from_static( + r#"Bearer error="invalid_token", error_description="reauthentication required for a privileged action""#, + ), + ); + resp +} + /// Convenience re-export so handlers can name the type they pull out of /// request extensions without depending on `ruview-auth` directly. pub use ruview_auth::Principal as AuthenticatedPrincipal; @@ -1226,6 +1300,90 @@ mod oauth_tests { ); } + // ── step-up: privileged actions need a RECENT authentication ────── + + fn aged_admin_cookie(age: i64) -> String { + format!( + "ruview_session={}", + crate::browser_session::test_cookie_value_aged( + "sub-b", + "acct-b", + &format!("{} {}", scope::SENSING_READ, scope::SENSING_ADMIN), + 3600, + age, + ) + ) + } + + #[tokio::test] + async fn a_stale_but_live_session_can_still_read() { + // Step-up must not degrade into "re-authenticate every 5 minutes". The + // dashboard's primary use is watching a live stream; reads ride the + // full session lifetime. + let c = aged_admin_cookie(crate::browser_session::ADMIN_REVERIFY_SECS + 60); + assert_eq!( + call_with_session(oauth_only(), "GET", "/api/v1/models", &c).await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn a_stale_session_cannot_delete_even_holding_the_admin_scope() { + // The session outlives the ~15-minute access token that created it, and + // Cognitum publishes no introspection endpoint — so a stale session is + // authority nobody can revoke. Bounded to the routes where it does + // damage: holding `sensing:admin` is necessary but no longer sufficient. + let c = aged_admin_cookie(crate::browser_session::ADMIN_REVERIFY_SECS + 60); + for (method, path) in [ + ("DELETE", "/api/v1/models/m1"), + ("DELETE", "/api/v1/recording/r1"), + ("POST", "/api/v1/train/start"), + ] { + assert_eq!( + call_with_session(oauth_only(), method, path, &c).await, + StatusCode::UNAUTHORIZED, + "{method} {path} accepted a stale session for a privileged action" + ); + } + } + + #[tokio::test] + async fn a_freshly_authenticated_session_can_delete() { + // The negative above must not pass merely because admin never works. + let c = aged_admin_cookie(0); + assert_eq!( + call_with_session(oauth_only(), "DELETE", "/api/v1/models/m1", &c).await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn a_session_cookie_predating_auth_time_cannot_perform_admin_actions() { + // Cookies issued before `auth_time` existed deserialize with 0, which is + // infinitely stale. They must degrade to read-only rather than being + // treated as freshly authenticated — fail closed, and self-healing the + // next time the user signs in. + let legacy = serde_json::json!({ + "subject": "sub-old", + "account_id": "acct-old", + "scope": "sensing:read sensing:admin", + "exp": chrono::Utc::now().timestamp() + 3600, + }); + let raw = crate::browser_session::test_sign_for_tests(&serde_json::to_vec(&legacy).unwrap()); + let c = format!("ruview_session={raw}"); + + assert_eq!( + call_with_session(oauth_only(), "GET", "/api/v1/models", &c).await, + StatusCode::OK, + "an old cookie must keep working for reads" + ); + assert_eq!( + call_with_session(oauth_only(), "DELETE", "/api/v1/models/m1", &c).await, + StatusCode::UNAUTHORIZED, + "an old cookie must not carry privileged authority" + ); + } + #[tokio::test] async fn an_expired_browser_session_is_refused() { let c = session_cookie(scope::SENSING_READ, -1); diff --git a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs index ff86c559..b863bf52 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -54,7 +54,32 @@ const SESSION_COOKIE: &str = "ruview_session"; /// The OAuth round-trip is a page load or two. Ten minutes is generous. const TXN_TTL_SECS: i64 = 600; /// How long a browser stays signed in before repeating the redirect. -const SESSION_TTL_SECS: i64 = 12 * 3600; +/// +/// One hour, down from twelve. The session cookie is an assertion that this +/// server verified a Cognitum access token; that token lives ~15 minutes, and +/// Cognitum publishes no introspection endpoint, so there is no way to ask +/// whether the grant behind a session still stands. Every second of this TTL is +/// time a revoked or disabled account keeps working. Twelve hours made that +/// window a working day. +/// +/// An hour is short enough to bound the damage and long enough that the +/// re-auth redirect is rare; because the user's Cognitum session is normally +/// still alive, that redirect is usually silent. +pub const SESSION_TTL_SECS: i64 = 3600; + +/// How recently the user must have actually authenticated for this server to +/// honour a **privileged** (`sensing:admin`) request from a browser session. +/// +/// Reads ride the full [`SESSION_TTL_SECS`]; deleting models and recordings, and +/// starting training, do not. This is step-up-by-recency: the blast radius of a +/// stale session is the mutating routes, so those are what get re-verified, +/// rather than making every user re-authenticate hourly for a dashboard whose +/// primary use is watching a live stream. +/// +/// When a session is too old for an admin action the request is refused, and +/// the UI sends the user back through `/oauth/start` — which returns with a +/// fresh `auth_time` and, if their Cognitum session is live, no prompt. +pub const ADMIN_REVERIFY_SECS: i64 = 300; fn now() -> i64 { SystemTime::now() @@ -116,6 +141,15 @@ pub struct BrowserSession { pub account_id: String, pub scope: String, pub exp: i64, + /// When the user last actually authenticated against Cognitum, unix + /// seconds — the OIDC `auth_time` idea, used here for step-up. + /// + /// `#[serde(default)]` so a cookie issued before this field existed still + /// deserializes. It then reads as `0`, which is infinitely stale, so such a + /// session can still read but cannot perform a privileged action until the + /// user signs in again. Fail-closed, and self-healing on next sign-in. + #[serde(default)] + pub auth_time: i64, } impl BrowserSession { @@ -125,6 +159,14 @@ impl BrowserSession { pub fn has_scope(&self, want: &str) -> bool { self.scope.split_whitespace().any(|s| s == want) } + /// Has the user authenticated recently enough for a privileged action? + /// + /// See [`ADMIN_REVERIFY_SECS`]. Note this is deliberately NOT "is the + /// session live" — a session can be perfectly valid for reads and still too + /// old to delete a model. + pub fn recently_authenticated(&self) -> bool { + now() - self.auth_time < ADMIN_REVERIFY_SECS + } } fn cookie(name: &str, value: &str, max_age: i64, secure: bool) -> String { @@ -135,11 +177,75 @@ fn cookie(name: &str, value: &str, max_age: i64, secure: bool) -> String { } /// Read one cookie from a raw `Cookie:` header. +/// +/// Returns the FIRST match, which is only safe when the caller has already +/// established there is exactly one — see [`read_all_cookies`] and the +/// shadowing attack it exists to stop. Kept for callers that genuinely want +/// first-match semantics; the credential paths do not. pub fn read_cookie(header: &str, name: &str) -> Option { - header.split(';').find_map(|part| { - let (k, v) = part.split_once('=')?; - (k.trim() == name).then(|| v.trim().to_string()) - }) + read_all_cookies(header, name).into_iter().next() +} + +/// Every value sent under `name`, in header order. +/// +/// # Why this is not `read_cookie` +/// +/// A `Cookie:` header can legitimately carry the same name more than once — +/// cookies are keyed by (name, domain, path), and RFC 6265 §5.4 orders +/// longer-`Path` matches FIRST. Cookies are also not isolated by port or by +/// scheme, so *any* other service on the same host, or a plain-HTTP MITM +/// injecting a `Set-Cookie`, can add one. +/// +/// Taking the first match therefore let an attacker **shadow** a victim's +/// session: sign in normally, capture your own validly-signed +/// `ruview_session`, then get it set with `Path=/ui` on the victim's browser. +/// The victim then sends both, the attacker's first, and it verifies — +/// because it is genuinely signed. The victim silently operates inside the +/// attacker's session; `/oauth/status` reports the attacker's account and the +/// victim's recordings are attributed to them. +/// +/// Note the shape of this: the signature was doing its job the whole time. +/// Forgery is not the threat here, and "the signature protects the value" does +/// not answer it. The `__Host-` prefix would — it forbids `Domain` and pins +/// `Path=/` — but it also requires `Secure`, and RuView is routinely reached +/// over plain HTTP on a LAN, where such a cookie is never sent at all. +/// +/// So the callers resolve ambiguity themselves: accept only when exactly one +/// candidate verifies. An attacker can still cause a *refusal* by planting a +/// second valid cookie, which is a nuisance; they can no longer cause a +/// silent takeover, which is a compromise. +pub fn read_all_cookies(header: &str, name: &str) -> Vec { + header + .split(';') + .filter_map(|part| { + let (k, v) = part.split_once('=')?; + (k.trim() == name).then(|| v.trim().to_string()) + }) + .collect() +} + +/// Unwrap the one candidate that verifies, or `None` if zero or several do. +/// +/// Several verifying means the browser sent two genuinely-signed cookies of the +/// same name — which a legitimate client never does, and which is exactly the +/// shadowing attack described on [`read_all_cookies`]. Refusing is correct: we +/// cannot tell which one the user meant, and guessing is how the takeover works. +fn unsign_unambiguous(header: &str, name: &str, secret: &str) -> Option> { + let mut verified = read_all_cookies(header, name) + .into_iter() + .filter_map(|raw| unsign(&raw, secret)); + let first = verified.next()?; + match verified.next() { + None => Some(first), + Some(_) => { + tracing::warn!( + cookie = name, + "request carried more than one validly-signed {name}; refusing rather than \ + guessing which is the user's — see read_all_cookies" + ); + None + } + } } #[derive(Debug, thiserror::Error)] @@ -325,8 +431,10 @@ pub fn clear_session(secure: bool) -> String { /// the PKCE verifier needed for the exchange. pub fn verifier_for_callback(cookie_header: &str, state: &str) -> Result { let secret = secret()?; - let raw = read_cookie(cookie_header, TXN_COOKIE).ok_or(SessionError::InvalidTransaction)?; - let bytes = unsign(&raw, &secret).ok_or(SessionError::InvalidTransaction)?; + // Unambiguous: a second validly-signed txn cookie would let an attacker + // substitute their own PKCE verifier, defeating the binding entirely. + let bytes = unsign_unambiguous(cookie_header, TXN_COOKIE, &secret) + .ok_or(SessionError::InvalidTransaction)?; let txn: Transaction = serde_json::from_slice(&bytes).map_err(|_| SessionError::InvalidTransaction)?; if txn.exp < now() { @@ -350,6 +458,10 @@ pub fn issue(principal: &ruview_auth::Principal, secure: bool) -> Result Result Option { let secret = secret().ok()?; - let raw = read_cookie(cookie_header, SESSION_COOKIE)?; - let bytes = unsign(&raw, &secret)?; + let bytes = unsign_unambiguous(cookie_header, SESSION_COOKIE, &secret)?; let session: BrowserSession = serde_json::from_slice(&bytes).ok()?; session.is_live().then_some(session) } @@ -387,6 +498,29 @@ pub(crate) fn init_secret_for_tests() { /// test-only bypass. `ttl` may be negative to forge an already-expired session. #[cfg(test)] pub(crate) fn test_cookie_value(subject: &str, account_id: &str, scope: &str, ttl: i64) -> String { + // Freshly authenticated, so step-up does not interfere with tests about + // scope or expiry. Use `test_cookie_value_aged` to exercise step-up itself. + test_cookie_value_aged(subject, account_id, scope, ttl, 0) +} + +/// Sign an arbitrary payload with the test secret, so a test elsewhere in the +/// crate can construct a cookie whose SHAPE differs from the current struct — +/// e.g. one issued before a field existed. +#[cfg(test)] +pub(crate) fn test_sign_for_tests(payload: &[u8]) -> String { + init_secret_for_tests(); + sign(payload, &secret().expect("secret installed above")) +} + +/// As [`test_cookie_value`], but `age` seconds since the user authenticated. +#[cfg(test)] +pub(crate) fn test_cookie_value_aged( + subject: &str, + account_id: &str, + scope: &str, + ttl: i64, + age: i64, +) -> String { init_secret_for_tests(); let secret = secret().expect("secret installed above"); let session = BrowserSession { @@ -394,6 +528,7 @@ pub(crate) fn test_cookie_value(subject: &str, account_id: &str, scope: &str, tt account_id: account_id.to_string(), scope: scope.to_string(), exp: now() + ttl, + auth_time: now() - age, }; sign(&serde_json::to_vec(&session).expect("serializes"), &secret) } @@ -563,6 +698,7 @@ mod tests { account_id: "acct-attacker".into(), scope: "sensing:admin".into(), exp: now() + 3600, + auth_time: now(), }) .unwrap(), "a-different-secret", @@ -570,6 +706,85 @@ mod tests { assert!(from_cookie_header(&format!("{SESSION_COOKIE}={forged}")).is_none()); } + // ── cookie shadowing (P3) ───────────────────────────────────────── + + #[test] + fn every_value_sent_under_a_name_is_visible_not_just_the_first() { + let h = "ruview_session=attacker; other=x; ruview_session=victim"; + assert_eq!( + read_all_cookies(h, "ruview_session"), + vec!["attacker".to_string(), "victim".to_string()] + ); + } + + #[test] + fn a_shadowing_cookie_cannot_silently_take_over_the_session() { + // THE ATTACK. Cookies are keyed by (name, domain, path) and RFC 6265 + // §5.4 sends longer-`Path` matches FIRST. They are not isolated by port + // or scheme, so any other service on this host — or a plain-HTTP MITM + // injecting Set-Cookie — can plant one. + // + // The attacker signs in legitimately, captures their OWN validly-signed + // cookie, and gets it set with `Path=/ui` on the victim's browser. Under + // first-match the victim's browser sends the attacker's cookie first, it + // verifies (it IS genuinely signed), and the victim silently operates + // inside the attacker's session. + // + // The signature was never the problem, which is why "it's signed" does + // not answer this. + init_secret_for_tests(); + let attacker = test_cookie_value("attacker", "acct-attacker", "sensing:read", 3600); + let victim = test_cookie_value("victim", "acct-victim", "sensing:read", 3600); + + let header = format!("ruview_session={attacker}; ruview_session={victim}"); + assert!( + from_cookie_header(&header).is_none(), + "two validly-signed sessions must be refused, not resolved by order" + ); + + // Order must not matter — the victim's cookie arriving first is the same + // ambiguity, not a pass. + let reversed = format!("ruview_session={victim}; ruview_session={attacker}"); + assert!(from_cookie_header(&reversed).is_none()); + } + + #[test] + fn a_junk_shadow_cookie_does_not_lock_the_real_user_out() { + // Only ONE candidate verifies, so there is no ambiguity to refuse. This + // matters: if any duplicate name caused a refusal, planting garbage + // would be a trivial denial of service against every user. + init_secret_for_tests(); + let real = test_cookie_value("victim", "acct-victim", "sensing:read", 3600); + for header in [ + format!("ruview_session=not-even-signed; ruview_session={real}"), + format!("ruview_session={real}; ruview_session=bm9wZQ.deadbeef"), + ] { + let s = from_cookie_header(&header).expect("the genuine cookie must still work"); + assert_eq!(s.subject, "victim"); + } + } + + #[test] + fn a_shadowing_transaction_cookie_cannot_substitute_a_pkce_verifier() { + // Same attack against the sign-in transaction: a second validly-signed + // txn cookie would let an attacker supply their own verifier and state, + // which defeats the PKCE binding rather than merely confusing it. + init_secret_for_tests(); + let (url_a, cookie_a) = begin("https://a.example", "ruview", "sensing:read", false).unwrap(); + let (_url_b, cookie_b) = begin("https://a.example", "ruview", "sensing:read", false).unwrap(); + let state_a = query_param(&url_a, "state"); + + let header = format!( + "{TXN_COOKIE}={}; {TXN_COOKIE}={}", + value_of(&cookie_a), + value_of(&cookie_b) + ); + assert!(matches!( + verifier_for_callback(&header, &state_a), + Err(SessionError::InvalidTransaction) + )); + } + #[test] fn clearing_cookies_expires_them_immediately() { for c in [clear_session(false), clear_transaction(false)] { @@ -583,6 +798,7 @@ mod tests { account_id: "acct-1".into(), scope: "sensing:read".into(), exp, + auth_time: now(), } }