diff --git a/crates/auth-core/src/lib.rs b/crates/auth-core/src/lib.rs index 71e19ba..322afb9 100644 --- a/crates/auth-core/src/lib.rs +++ b/crates/auth-core/src/lib.rs @@ -19,6 +19,12 @@ //! 3. **A signing key's private half never reaches an output.** //! `tokens::SigningKeys` prints key ids only, and the published JWKS //! is built from the re-derived public point (issue #9). +//! 4. **Changing how somebody signs in needs a recent login, not just a +//! live session** (issue #31). [`require_recent_authentication`] is the +//! one rule, and the window is one key — +//! `AUTH_CORE_STEP_UP_WINDOW_SECS`, default 900 — because a deployment +//! that hardened one login method and forgot another would have +//! hardened nothing. #![forbid(unsafe_code)] @@ -46,9 +52,11 @@ pub use secrets::{ verify_password, verify_secret, }; pub use sessions::{ - ABSOLUTE_CAP_DAYS, COOKIE_NAME, IssuedSession, Login, SESSION_INVALID, SESSION_VALUE_BYTES, - SLIDE_AFTER_SECS, SLIDE_WINDOW_DAYS, Session, SessionError, ValidSession, clear_cookie, - cookie_value, issue, revoke_all, set_cookie, ua_family, validate, + ABSOLUTE_CAP_DAYS, COOKIE_NAME, DEFAULT_STEP_UP_WINDOW_SECS, IssuedSession, Login, + MAX_STEP_UP_WINDOW_SECS, MIN_STEP_UP_WINDOW_SECS, REAUTHENTICATION_REQUIRED, SESSION_INVALID, + SESSION_VALUE_BYTES, SLIDE_AFTER_SECS, SLIDE_WINDOW_DAYS, Session, SessionError, ValidSession, + clear_cookie, cookie_value, issue, require_recent_authentication, revoke_all, set_cookie, + step_up_window_secs, ua_family, validate, }; pub use store::{ Bytes, CLIENT_CONFIDENTIAL, CLIENT_PUBLIC, CREDENTIAL_PASSKEY, CREDENTIAL_PASSWORD, diff --git a/crates/auth-core/src/sessions.rs b/crates/auth-core/src/sessions.rs index 9167483..953096e 100644 --- a/crates/auth-core/src/sessions.rs +++ b/crates/auth-core/src/sessions.rs @@ -53,6 +53,32 @@ pub const SESSION_INVALID: cratefield_core::ProblemDef = cratefield_core::Proble description: "Missing, unknown, revoked or expired session — not distinguished", }; +/// Changing how somebody signs in needs an authentication newer than this, +/// not merely a live session (issue #31). Fifteen minutes is the common +/// default: long enough to add a passkey without signing in twice, short +/// enough that a session left open on a shared machine cannot be turned +/// into permanent access. +pub const DEFAULT_STEP_UP_WINDOW_SECS: i64 = 900; +/// Below a minute the window is unusable: a person cannot finish a WebAuthn +/// ceremony inside it. +pub const MIN_STEP_UP_WINDOW_SECS: i64 = 60; +/// Above a day it is not a step-up, it is a session lifetime. +pub const MAX_STEP_UP_WINDOW_SECS: i64 = 86_400; + +/// The session is real, but the login behind it is too old to change how +/// this account signs in. +/// +/// `403`, not `401`, and that distinction is the point: a client that got a +/// `401` should send the person through a full login, while this one should +/// re-authenticate them and retry. Answering both with +/// [`SESSION_INVALID`] would make a step-up look like a logout. +pub const REAUTHENTICATION_REQUIRED: cratefield_core::ProblemDef = cratefield_core::ProblemDef { + slug: "auth/reauthentication-required", + status: StatusCode::FORBIDDEN, + title: "Sign in again to change how you sign in", + description: "The session is valid, but the authentication behind it is older than the step-up window", +}; + /// Failure while issuing a session: entropy or database. The cookie /// value never leaves the failing call. #[derive(Debug, thiserror::Error)] @@ -75,6 +101,74 @@ pub enum SessionError { pub struct ValidSession { pub id: String, pub user_id: String, + /// When the login behind this session happened — the session's own + /// `created_at`, which sliding never moves. + pub authenticated_at: OffsetDateTime, + /// RFC 8176 method references recorded at that login, empty when the + /// login method wired none. Read rather than ignored so a later rule + /// can ask *how* somebody authenticated, not only how long ago. + pub amr: Vec, +} + +impl ValidSession { + /// Whether the login behind this session is newer than `window_secs`. + /// + /// A session whose `created_at` is in the future counts as recent: that + /// is clock skew between the writer and the reader, and the safe + /// failure for skew is not to lock somebody out of their own account + /// page. The window itself is validated on the way in, so it cannot be + /// negative here. + #[must_use] + pub fn authenticated_within(&self, now: OffsetDateTime, window_secs: i64) -> bool { + now - self.authenticated_at <= time::Duration::seconds(window_secs) + } +} + +/// The step-up guard: a live session is not enough to change how somebody +/// signs in (issue #31). +/// +/// # Errors +/// +/// [`REAUTHENTICATION_REQUIRED`] when the login behind the session is older +/// than `window_secs`. The caller adds its own `instance`. +pub fn require_recent_authentication( + session: &ValidSession, + now: OffsetDateTime, + window_secs: i64, +) -> Result<(), Problem> { + if session.authenticated_within(now, window_secs) { + Ok(()) + } else { + Err(Problem::new(&REAUTHENTICATION_REQUIRED)) + } +} + +/// Resolves the step-up window from configuration. +/// +/// Owned by `auth-core` and keyed `AUTH_CORE_STEP_UP_WINDOW_SECS` rather +/// than per module, because every login method has to apply the same rule: +/// a deployment that hardened passkeys and forgot passwords would have +/// hardened nothing. +/// +/// # Errors +/// +/// A message naming the key when the value is not a whole number of seconds +/// inside [`MIN_STEP_UP_WINDOW_SECS`]..=[`MAX_STEP_UP_WINDOW_SECS`]. +pub fn step_up_window_secs(cfg: &dyn cratefield_core::Config) -> Result { + let module = cratefield_core::ModuleConfig::new("auth-core", cfg); + match module.get_opt("STEP_UP_WINDOW_SECS") { + None => Ok(DEFAULT_STEP_UP_WINDOW_SECS), + Some(raw) => match raw.trim().parse::() { + Ok(secs) if (MIN_STEP_UP_WINDOW_SECS..=MAX_STEP_UP_WINDOW_SECS).contains(&secs) => { + Ok(secs) + } + _ => Err(format!( + "{} must be between {MIN_STEP_UP_WINDOW_SECS} and \ + {MAX_STEP_UP_WINDOW_SECS} seconds, got {raw:?}", + module.key("STEP_UP_WINDOW_SECS") + )), + }, + } } pub(crate) fn iso(t: OffsetDateTime) -> String { @@ -357,9 +451,23 @@ pub async fn validate( } } + let authenticated_at = OffsetDateTime::parse(&row.created_at, &Rfc3339) + .map_err(|_| DbError::Query("session created_at is malformed".to_owned()))?; + // A malformed `amr` is not worth refusing a session over: it is + // provenance, not authority. Log it and carry on with none. + let amr = match row.amr.as_deref() { + None => Vec::new(), + Some(raw) => serde_json::from_str::>(raw).unwrap_or_else(|_| { + tracing::warn!(session = %row.id, "session amr is not a JSON array of strings"); + Vec::new() + }), + }; + Ok(Some(ValidSession { id: row.id, user_id: row.user_id, + authenticated_at, + amr, })) } diff --git a/crates/auth-core/tests/sessions.rs b/crates/auth-core/tests/sessions.rs index e5be2ec..176906d 100644 --- a/crates/auth-core/tests/sessions.rs +++ b/crates/auth-core/tests/sessions.rs @@ -198,12 +198,20 @@ async fn issue_stores_only_the_hash_and_builds_the_host_cookie() { let ok = validate(&*kit.db, &at(EPOCH + 60), &issued.value) .await .expect("validate"); + let ok = ok.expect("the session validates"); + assert_eq!(ok.id, issued.session_id); + assert_eq!(ok.user_id, "u1"); + // The login behind the session, not the moment it was last used: this + // is what the step-up rule reads, and sliding must never move it. assert_eq!( - ok, - Some(factory0_auth_core::ValidSession { - id: issued.session_id.clone(), - user_id: "u1".to_owned() - }) + ok.authenticated_at.unix_timestamp(), + EPOCH, + "authenticated_at is not the login time" + ); + assert_eq!( + ok.amr, + vec!["passkey".to_owned()], + "the amr recorded at login was lost" ); assert!( validate( diff --git a/crates/auth-passkeys/src/lib.rs b/crates/auth-passkeys/src/lib.rs index 51d1bb4..c6a6444 100644 --- a/crates/auth-passkeys/src/lib.rs +++ b/crates/auth-passkeys/src/lib.rs @@ -228,6 +228,9 @@ impl RelyingParty { pub(crate) struct ModuleState { pub ctx: Arc, pub rp: Option, + /// How recent a login has to be to add or remove a passkey (issue #31). + /// Resolved by `auth-core` so every login method reads one key. + pub step_up_window_secs: i64, } impl ModuleState { @@ -295,16 +298,23 @@ impl Module for Passkeys { } fn validate_config(&self, cfg: &dyn Config) -> Result<(), ConfigError> { - match RelyingParty::from_config(cfg) { - Ok(_) => Ok(()), - Err(problems) => { - let mut errors = ConfigError::default(); - for problem in problems { - errors.push(format!("auth-passkeys: {problem}")); - } - Err(errors) + let mut errors = ConfigError::default(); + if let Err(problems) = RelyingParty::from_config(cfg) { + for problem in problems { + errors.push(format!("auth-passkeys: {problem}")); } } + // The step-up window is auth-core's key, but a deployment that + // mistypes it should hear about it from whichever module enforces + // it rather than silently getting the default. + if let Err(problem) = factory0_auth_core::step_up_window_secs(cfg) { + errors.push(format!("auth-passkeys: {problem}")); + } + if errors.is_empty() { + Ok(()) + } else { + Err(errors) + } } fn router(&self, ctx: ModuleContext) -> axum::Router { @@ -321,9 +331,18 @@ impl Module for Passkeys { None } }; + // A bad value is reported by `validate_config`; here it degrades to + // the default rather than taking the module down, and the default + // is the stricter of the two outcomes anyway. + let step_up_window_secs = factory0_auth_core::step_up_window_secs(&*ctx.config) + .unwrap_or_else(|problem| { + tracing::error!(problem, "the step-up window is unusable; using the default"); + factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS + }); let state = Arc::new(ModuleState { ctx: Arc::new(ctx), rp, + step_up_window_secs, }); register::router().merge(login::router()).with_state(state) } diff --git a/crates/auth-passkeys/src/register.rs b/crates/auth-passkeys/src/register.rs index e136bf5..e9ee9fe 100644 --- a/crates/auth-passkeys/src/register.rs +++ b/crates/auth-passkeys/src/register.rs @@ -24,7 +24,9 @@ use webauthn_rs_proto::{ }; use crate::challenge::{self, PURPOSE_REGISTER}; -use crate::request::{b64u, ceremony_failed, hex, internal, ok, ports, require_session}; +use crate::request::{ + b64u, ceremony_failed, hex, internal, ok, ports, require_recent_session, require_session, +}; use crate::webauthn::{UserVerification, verify_registration}; use crate::{COSE_EDDSA, COSE_ES256, COSE_RS256, ModuleState}; @@ -52,7 +54,7 @@ async fn options( scope: Scope, headers: HeaderMap, ) -> Result { - let session = require_session(&state, &headers, &scope).await?; + let session = require_recent_session(&state, &headers, &scope).await?; let rp = state.rp()?; let (db, clock, id_gen) = ports(&state)?; @@ -162,7 +164,7 @@ async fn verify( headers: HeaderMap, Json(body): Json, ) -> Result { - let session = require_session(&state, &headers, &scope).await?; + let session = require_recent_session(&state, &headers, &scope).await?; let rp = state.rp()?; let (db, clock, id_gen) = ports(&state)?; @@ -344,7 +346,7 @@ async fn remove( headers: HeaderMap, Path(id): Path, ) -> Result { - let session = require_session(&state, &headers, &scope).await?; + let session = require_recent_session(&state, &headers, &scope).await?; let (db, _, _) = ports(&state)?; let credentials = credentials_by_user(db, &session.user_id) diff --git a/crates/auth-passkeys/src/request.rs b/crates/auth-passkeys/src/request.rs index ef25625..b949ae4 100644 --- a/crates/auth-passkeys/src/request.rs +++ b/crates/auth-passkeys/src/request.rs @@ -50,6 +50,33 @@ pub(crate) async fn require_session( } } +/// The signed-in user, for an endpoint that **changes how they sign in**. +/// +/// A live session is not enough here (issue #31). Adding a passkey to a +/// hijacked session is the classic persistence step: the passkey outlives +/// the password change and the session revocation that follow, and nothing +/// about it looks unusual on an account page. So the login behind the +/// session has to be recent, not merely valid. +/// +/// The refusal is `403 auth/reauthentication-required`, distinct from the +/// `401` a signed-out caller gets, so a client re-authenticates and retries +/// instead of dropping the person into a full login. +pub(crate) async fn require_recent_session( + state: &ModuleState, + headers: &HeaderMap, + scope: &Scope, +) -> Result { + let session = require_session(state, headers, scope).await?; + let (_, clock, _) = ports(state)?; + factory0_auth_core::require_recent_authentication( + &session, + clock.now(), + state.step_up_window_secs, + ) + .map_err(|problem| problem.instance(&scope.request_id))?; + Ok(session) +} + /// The rate limit on the two endpoints anyone can call. Keyed on the client /// address **only**: keying on the email as well would let anyone lock a /// named account out of its own logins, which trades one denial of service diff --git a/crates/auth-passkeys/tests/registration.rs b/crates/auth-passkeys/tests/registration.rs index 4470770..70f112b 100644 --- a/crates/auth-passkeys/tests/registration.rs +++ b/crates/auth-passkeys/tests/registration.rs @@ -464,3 +464,243 @@ fn one_person_cannot_delete_another_persons_passkey() { assert_eq!(response.status, StatusCode::NOT_FOUND); }); } + +// --- Step-up: changing how you sign in needs a recent login (issue #31) --- + +const REAUTH: &str = "https://factory0.ventures/problems/auth/reauthentication-required"; + +/// Registers one passkey and returns (session cookie, credential row id), +/// with the clock still at the moment of the login. +async fn a_registered_passkey(kit: &support::Kit) -> (String, String) { + let user = kit.user("nick@example.com").await; + let cookie = kit.sign_in(&user).await; + let options = post(kit, OPTIONS, "{}", Some(&cookie)).await; + assert_eq!(options.status, StatusCode::OK, "{}", options.text()); + let authenticator = SoftAuthenticator::new(Algorithm::Es256); + let credential = authenticator.register(RP_ID, ORIGIN, &challenge_of(&options.json())); + let verified = post( + kit, + VERIFY, + &verify_body(&credential, "laptop"), + Some(&cookie), + ) + .await; + assert_eq!(verified.status, StatusCode::OK, "{}", verified.text()); + let listed = send(kit, Method::GET, CREDENTIALS, None, Some(&cookie)) + .await + .json(); + let id = listed["passkeys"][0]["id"] + .as_str() + .expect("a credential id") + .to_owned(); + (cookie, id) +} + +#[test] +fn a_session_older_than_the_window_cannot_add_a_passkey() { + // The attack: a session cookie that was stolen, or left open on a + // shared machine, adds a passkey. That passkey then outlives the + // password change and the session revocation the person does when they + // notice, and nothing about it looks unusual on the account page. + pollster::block_on(async { + let kit = support::kit(); + let user = kit.user("nick@example.com").await; + let cookie = kit.sign_in(&user).await; + + kit.clock + .advance_secs(factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS + 1); + + let response = post(&kit, OPTIONS, "{}", Some(&cookie)).await; + assert_eq!( + response.status, + StatusCode::FORBIDDEN, + "a stale session was allowed to start a registration: {}", + response.text() + ); + // 403 and its own type, not the 401 a signed-out caller gets: the + // client should re-authenticate and retry, not start a full login. + assert_eq!(response.json()["type"], REAUTH); + }); +} + +#[test] +fn a_session_older_than_the_window_cannot_finish_a_registration() { + // The second half of the same door. Guarding only `options` would let a + // caller collect a challenge while fresh and spend it later. + pollster::block_on(async { + let kit = support::kit(); + let user = kit.user("nick@example.com").await; + let cookie = kit.sign_in(&user).await; + + let options = post(&kit, OPTIONS, "{}", Some(&cookie)).await; + assert_eq!(options.status, StatusCode::OK, "{}", options.text()); + let authenticator = SoftAuthenticator::new(Algorithm::Es256); + let credential = authenticator.register(RP_ID, ORIGIN, &challenge_of(&options.json())); + + kit.clock + .advance_secs(factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS + 1); + + let response = post( + &kit, + VERIFY, + &verify_body(&credential, "laptop"), + Some(&cookie), + ) + .await; + assert_eq!( + response.status, + StatusCode::FORBIDDEN, + "a challenge taken while fresh was spent after the window: {}", + response.text() + ); + assert_eq!(response.json()["type"], REAUTH); + }); +} + +#[test] +fn a_session_older_than_the_window_cannot_remove_a_passkey() { + // Removal matters as much as addition: stripping somebody's only + // passkey is how you force them onto a weaker method you control. + pollster::block_on(async { + let kit = support::kit(); + let (cookie, id) = a_registered_passkey(&kit).await; + + kit.clock + .advance_secs(factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS + 1); + + let response = send( + &kit, + Method::DELETE, + &format!("{CREDENTIALS}/{id}"), + None, + Some(&cookie), + ) + .await; + assert_eq!( + response.status, + StatusCode::FORBIDDEN, + "a stale session removed a passkey: {}", + response.text() + ); + assert_eq!(response.json()["type"], REAUTH); + }); +} + +#[test] +fn a_session_inside_the_window_still_works() { + // The other half, and the one that would make this feature useless if + // it were wrong: the rule must not refuse a person who signed in a + // moment ago. + pollster::block_on(async { + let kit = support::kit(); + let user = kit.user("nick@example.com").await; + let cookie = kit.sign_in(&user).await; + + // One second short of the window: still allowed. + kit.clock + .advance_secs(factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS - 1); + + let response = post(&kit, OPTIONS, "{}", Some(&cookie)).await; + assert_eq!( + response.status, + StatusCode::OK, + "a session inside the window was refused: {}", + response.text() + ); + }); +} + +#[test] +fn listing_passkeys_does_not_need_a_recent_login() { + // Deliberate asymmetry: reading which passkeys exist changes nothing, + // and making the account page demand a re-authentication to render + // would train people to re-authenticate for no reason. + pollster::block_on(async { + let kit = support::kit(); + let (cookie, _) = a_registered_passkey(&kit).await; + + kit.clock + .advance_secs(factory0_auth_core::DEFAULT_STEP_UP_WINDOW_SECS + 1); + + let response = send(&kit, Method::GET, CREDENTIALS, None, Some(&cookie)).await; + assert_eq!( + response.status, + StatusCode::OK, + "listing was refused: {}", + response.text() + ); + assert_eq!( + response.json()["passkeys"] + .as_array() + .expect("passkeys") + .len(), + 1 + ); + }); +} + +#[test] +fn the_window_is_configurable_and_validated() { + // A deployment can widen or narrow it, and a typo is a build failure + // rather than a silent default. + use cratefield_core::Module as _; + + let mut pairs = support::config_pairs(); + pairs.push(( + "AUTH_CORE_STEP_UP_WINDOW_SECS".to_owned(), + "not-a-number".to_owned(), + )); + let config = cratefield_core::MapConfig::from_pairs(pairs); + let refused = factory0_auth_passkeys::Passkeys::new() + .validate_config(&config) + .expect_err("a non-numeric window was accepted"); + assert!( + format!("{refused:?}").contains("AUTH_CORE_STEP_UP_WINDOW_SECS"), + "the error does not name the key: {refused:?}" + ); + + for bad in ["0", "59", "86401", "-900"] { + let mut pairs = support::config_pairs(); + pairs.push(("AUTH_CORE_STEP_UP_WINDOW_SECS".to_owned(), bad.to_owned())); + assert!( + factory0_auth_passkeys::Passkeys::new() + .validate_config(&cratefield_core::MapConfig::from_pairs(pairs)) + .is_err(), + "{bad} was accepted as a step-up window" + ); + } + + // And a sane value is taken. + let mut pairs = support::config_pairs(); + pairs.push(("AUTH_CORE_STEP_UP_WINDOW_SECS".to_owned(), "300".to_owned())); + assert!( + factory0_auth_passkeys::Passkeys::new() + .validate_config(&cratefield_core::MapConfig::from_pairs(pairs)) + .is_ok() + ); +} + +#[test] +fn a_narrower_window_is_enforced_not_merely_accepted() { + // Configuration that parses but does nothing is the usual way a + // security setting fails, so assert the number reaches the guard. + pollster::block_on(async { + let mut pairs = support::config_pairs(); + pairs.push(("AUTH_CORE_STEP_UP_WINDOW_SECS".to_owned(), "60".to_owned())); + let kit = support::kit_with(pairs); + let user = kit.user("nick@example.com").await; + let cookie = kit.sign_in(&user).await; + + // Well inside the default window, well outside the configured one. + kit.clock.advance_secs(61); + + let response = post(&kit, OPTIONS, "{}", Some(&cookie)).await; + assert_eq!( + response.status, + StatusCode::FORBIDDEN, + "the configured 60s window was not applied: {}", + response.text() + ); + assert_eq!(response.json()["type"], REAUTH); + }); +}