From 763ee8202899b67002ab98adbad4f97f50497e5f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 9 Aug 2026 15:14:59 +0200 Subject: [PATCH] security(session): do not expose 'sid' from OIDC prefer exposing origin of the session: passwod, opaque, magic_link, oidc, unknown --- frontend/src/lib/api/types.ts | 1 - migrations/20261013000000_sessions_origin.sql | 36 ++++++++ src/application/dtos/session_dto.rs | 70 +++++++++++++-- .../services/auth_application_service.rs | 17 +++- .../services/device_auth_service.rs | 1 + src/domain/entities/session.rs | 87 +++++++++++++++++++ .../repositories/pg/session_pg_repository.rs | 22 +++-- .../api/handlers/opaque_auth_handler.rs | 8 +- 8 files changed, 225 insertions(+), 17 deletions(-) create mode 100644 migrations/20261013000000_sessions_origin.sql diff --git a/frontend/src/lib/api/types.ts b/frontend/src/lib/api/types.ts index eab34726..2e787e86 100644 --- a/frontend/src/lib/api/types.ts +++ b/frontend/src/lib/api/types.ts @@ -767,7 +767,6 @@ export interface SessionSummary { dpop_jkt_prefix: string | null; is_revoked: boolean; is_active: boolean; - oidc_sid: string | null; /** `true` when this row IS the admin's currently-active session — * compared server-side by `dpop_jkt`. Panel uses this to warn * before revoking ("this will log you out"). Always `false` when diff --git a/migrations/20261013000000_sessions_origin.sql b/migrations/20261013000000_sessions_origin.sql new file mode 100644 index 00000000..91f550b4 --- /dev/null +++ b/migrations/20261013000000_sessions_origin.sql @@ -0,0 +1,36 @@ +-- Session origin — how the row was minted. +-- +-- Populated at session-mint time by each login handler (legacy password, +-- OPAQUE aPAKE, magic-link redemption, OIDC callback, RFC 8628 device +-- authorization). Refresh copies the parent session's origin (a refresh +-- doesn't change how the user originally authenticated). Existing rows +-- predating this column default to `unknown`. +-- +-- Purpose: gives admins a first-class filter on the sessions panel +-- ("show me only the OIDC sessions", "spot the magic-link ones during +-- a suspected phishing wave") without them having to infer from +-- adjacent fields (`oidc_id_token IS NOT NULL` etc.). Also drives +-- correlation with audit lines that already carry the same enum. +-- +-- Stored as `text` rather than a PG ENUM: enums lock the schema (adding +-- a new variant needs a migration + release coordination), whereas a +-- checked text column can gain values by editing the constraint. The +-- Rust `SessionOrigin` enum uses `#[serde(rename_all = "snake_case")]` +-- so wire values match column values one-to-one. +-- +-- No index — origin is a display column read alongside the row by PK; +-- filtering happens client-side in the admin panel (page size caps at +-- 100, so scanning is fine). +ALTER TABLE auth.sessions + ADD COLUMN IF NOT EXISTS origin TEXT NOT NULL DEFAULT 'unknown'; + +-- Enforce the known values at the storage layer so a rogue INSERT +-- can't smuggle an arbitrary string that would then confuse the +-- serde-typed enum deserialize on read. Adding a new variant is a +-- one-line ALTER + Rust enum change. +ALTER TABLE auth.sessions + ADD CONSTRAINT sessions_origin_known + CHECK (origin IN ('password', 'opaque', 'magic_link', 'oidc', 'device', 'unknown')); + +COMMENT ON COLUMN auth.sessions.origin IS + 'How this session was minted: password | opaque | magic_link | oidc | device | unknown. Set at INSERT time by the login handler; carried over on refresh.'; diff --git a/src/application/dtos/session_dto.rs b/src/application/dtos/session_dto.rs index df95b0da..87fe23ba 100644 --- a/src/application/dtos/session_dto.rs +++ b/src/application/dtos/session_dto.rs @@ -19,7 +19,7 @@ use serde::Serialize; use utoipa::ToSchema; use uuid::Uuid; -use crate::domain::entities::session::Session; +use crate::domain::entities::session::{Session, SessionOrigin}; /// Authenticated-caller context — the caller's identity + session- /// bound signals a service method might key off. Constructed at the @@ -69,10 +69,17 @@ pub struct SessionSummaryDto { /// Whether this row is currently usable — `!revoked && expires_at > now()`. /// Kept server-side so the SPA doesn't drift if the browser clock is off. pub is_active: bool, - /// OIDC session identifier when the login came through OIDC and the - /// IdP emitted `sid` — otherwise `None`. Useful when an operator is - /// correlating with the upstream IdP's session log. - pub oidc_sid: Option, + // NOTE: no `oidc_sid` / `oidc_sid_prefix` field. The IdP-emitted + // sid identifies the row's upstream session and stays server-side + // (used by Back-Channel Logout matching). Exposing even a prefix + // earns no operator utility over what `id` / `created_at` / + // `ip_address` / `user_agent` already give. `origin` below answers + // "how did this session start?" cleanly. + /// How the session was minted — see [`SessionOrigin`]. Set at + /// INSERT time by the login handler and carried over on refresh + /// (rotation doesn't change how the user first authenticated). + /// Drives the admin panel's origin column + filter. + pub origin: SessionOrigin, /// `true` when this row IS the caller's currently-active session — /// set by the service layer by comparing the row's `dpop_jkt` with /// the caller's own bound thumbprint. Lets the admin panel flag @@ -114,7 +121,7 @@ impl SessionSummaryDto { dpop_jkt_prefix, is_revoked, is_active: !is_revoked && !is_expired, - oidc_sid: s.oidc_sid().map(str::to_owned), + origin: s.origin(), is_current, } } @@ -133,6 +140,7 @@ mod tests { Some("Mozilla/5.0".to_string()), 30, Uuid::new_v4(), + crate::domain::entities::session::SessionOrigin::Password, ); if revoked { s.revoke(); @@ -143,6 +151,55 @@ mod tests { s } + fn oidc_session(sid: &str) -> Session { + Session::from_raw( + Uuid::new_v4(), + Uuid::new_v4(), + "rt".to_string(), + Utc::now() + Duration::days(30), + None, + None, + Utc::now(), + false, + Uuid::new_v4(), + Some("dummy.id.token".to_string()), + Some(sid.to_string()), + None, + crate::domain::entities::session::SessionOrigin::Oidc, + ) + } + + #[test] + fn dto_never_leaks_oidc_sid() { + // The IdP-emitted `sid` uniquely correlates the row to a real + // user's live IdP session and MUST stay server-side (used by + // Back-Channel Logout matching, never useful to an admin + // viewing sessions). Not even a prefix — see the DTO comment. + let full_sid = "8aa711b3-7438-cb35-4089-71a202e12285"; + let dto = SessionSummaryDto::from(oidc_session(full_sid)); + let json = serde_json::to_string(&dto).unwrap(); + assert!( + !json.contains(full_sid), + "oidc_sid must never appear in the wire shape (not even a prefix): {json}" + ); + assert!( + !json.contains("oidc_sid"), + "the `oidc_sid` key MUST NOT appear in the DTO shape: {json}" + ); + } + + #[test] + fn dto_never_leaks_oidc_id_token() { + // The id_token itself must NEVER surface — it's a JWT carrying + // user claims + a valid `id_token_hint` for RP-initiated logout. + let dto = SessionSummaryDto::from(oidc_session("sid-1")); + let json = serde_json::to_string(&dto).unwrap(); + assert!( + !json.contains("dummy.id.token"), + "oidc_id_token must never appear in the wire shape: {json}" + ); + } + #[test] fn dto_never_leaks_refresh_token() { let s = base(false, None); @@ -200,6 +257,7 @@ mod tests { None, None, None, + crate::domain::entities::session::SessionOrigin::Unknown, ); let dto = SessionSummaryDto::from(s); assert!(!dto.is_active); diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index a1ac68d8..adbb9de0 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -1011,7 +1011,13 @@ impl AuthApplicationService { // handshake (Phase 1, `login/ke3`). Both paths converge here // so lifecycle + token + session-family semantics stay in // one place. - self.mint_session_for_authenticated_user(user, dto.dpop_jkt, client_ip, user_agent) + self.mint_session_for_authenticated_user( + user, + dto.dpop_jkt, + client_ip, + user_agent, + crate::domain::entities::session::SessionOrigin::Password, + ) .await } @@ -1041,6 +1047,7 @@ impl AuthApplicationService { dpop_jkt: Option, client_ip: Option, user_agent: Option, + origin: crate::domain::entities::session::SessionOrigin, ) -> Result { // Lifecycle: dispatch login BEFORE register_login() so hooks // observing `last_login_at().is_none()` see "first ever login" @@ -1103,6 +1110,7 @@ impl AuthApplicationService { user_agent, self.token_service.refresh_token_expiry_days(), Uuid::new_v4(), + origin, ); if let Some(jkt) = validated_jkt { session = session.with_dpop_jkt(jkt); @@ -1368,6 +1376,7 @@ impl AuthApplicationService { user_agent, self.token_service.refresh_token_expiry_days(), Uuid::new_v4(), + crate::domain::entities::session::SessionOrigin::MagicLink, ); self.session_storage.create_session(session).await?; @@ -1540,6 +1549,11 @@ impl AuthApplicationService { user_agent, self.token_service.refresh_token_expiry_days(), session.family_id(), + // A rotation doesn't change how the user first authenticated, + // so origin is inherited from the parent row. Also keeps the + // admin panel's origin column stable across the natural + // refresh cycle apiFetch triggers on every 401. + session.origin(), ); if let Some(jkt) = session.dpop_jkt() { new_session = new_session.with_dpop_jkt(jkt.to_string()); @@ -4385,6 +4399,7 @@ impl AuthApplicationService { user_agent, self.token_service.refresh_token_expiry_days(), Uuid::new_v4(), + crate::domain::entities::session::SessionOrigin::Oidc, ) .with_oidc_id_token(token_set.id_token.clone()); // Bind the IdP's session identifier so Back-Channel Logout can diff --git a/src/application/services/device_auth_service.rs b/src/application/services/device_auth_service.rs index 1fbdf5ce..8e0a455e 100644 --- a/src/application/services/device_auth_service.rs +++ b/src/application/services/device_auth_service.rs @@ -191,6 +191,7 @@ impl DeviceAuthService { Some(format!("device:{}", dc.client_name())), // user_agent self.token_service.refresh_token_expiry_days(), Uuid::new_v4(), + crate::domain::entities::session::SessionOrigin::Device, ); self.session_storage.create_session(session).await?; diff --git a/src/domain/entities/session.rs b/src/domain/entities/session.rs index be96c8fd..bf85674e 100644 --- a/src/domain/entities/session.rs +++ b/src/domain/entities/session.rs @@ -1,6 +1,62 @@ use chrono::{DateTime, Duration, Utc}; use uuid::Uuid; +/// How a session was originally minted. Set at INSERT by the login +/// handler; carried over on refresh (a rotation doesn't change how the +/// user first authenticated). Stored as `text` server-side with a CHECK +/// constraint — see `migrations/20261013000000_sessions_origin.sql`. +/// +/// `serde(rename_all = "snake_case")` so the wire values match the +/// column values one-to-one: `password | opaque | magic_link | oidc | +/// unknown`. `Unknown` is the fallback for pre-migration rows and any +/// future login path that hasn't been taught to stamp an origin yet. +#[derive( + Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize, utoipa::ToSchema, +)] +#[serde(rename_all = "snake_case")] +pub enum SessionOrigin { + Password, + Opaque, + MagicLink, + Oidc, + /// RFC 8628 device authorization grant — CLI, TV apps, headless + /// clients that can't run a WebCrypto keypair. Always unbound at + /// the DPoP middleware. Distinct origin because admin operators + /// want to know "this login came from a headless device flow", not + /// conflate it with browser password entry. + Device, + Unknown, +} + +impl SessionOrigin { + /// Wire / column string form. Kept out of `Display` to avoid + /// accidental use in log lines where the Debug form is fine. + pub fn as_str(self) -> &'static str { + match self { + Self::Password => "password", + Self::Opaque => "opaque", + Self::MagicLink => "magic_link", + Self::Oidc => "oidc", + Self::Device => "device", + Self::Unknown => "unknown", + } + } + + /// Parse from the column string. Any unrecognised value maps to + /// `Unknown` — matches the CHECK constraint's failure mode + /// (impossible on well-behaved writes, defensive on load). + pub fn from_str(s: &str) -> Self { + match s { + "password" => Self::Password, + "opaque" => Self::Opaque, + "magic_link" => Self::MagicLink, + "oidc" => Self::Oidc, + "device" => Self::Device, + _ => Self::Unknown, + } + } +} + #[derive(Debug, Clone)] pub struct Session { id: Uuid, @@ -33,9 +89,14 @@ pub struct Session { /// a stolen cookie replay without the private key — the whole point /// of the binding is to prevent that. dpop_jkt: Option, + /// How this row was minted — see [`SessionOrigin`]. Required at + /// construction so a callsite can't forget to record it (the + /// admin sessions panel filters on this). + origin: SessionOrigin, } impl Session { + #[allow(clippy::too_many_arguments)] pub fn new( user_id: Uuid, refresh_token: String, @@ -43,6 +104,7 @@ impl Session { user_agent: Option, expires_in_days: i64, family_id: Uuid, + origin: SessionOrigin, ) -> Self { if refresh_token.is_empty() { panic!("Session refresh_token cannot be empty"); @@ -62,6 +124,7 @@ impl Session { oidc_id_token: None, oidc_sid: None, dpop_jkt: None, + origin, } } @@ -112,6 +175,7 @@ impl Session { oidc_id_token: Option, oidc_sid: Option, dpop_jkt: Option, + origin: SessionOrigin, ) -> Self { Self { id, @@ -126,6 +190,7 @@ impl Session { oidc_id_token, oidc_sid, dpop_jkt, + origin, } } @@ -185,6 +250,10 @@ impl Session { pub fn dpop_jkt(&self) -> Option<&str> { self.dpop_jkt.as_deref() } + + pub fn origin(&self) -> SessionOrigin { + self.origin + } } #[cfg(test)] @@ -199,6 +268,7 @@ mod tests { None, 30, Uuid::new_v4(), + SessionOrigin::Unknown, ) } @@ -236,7 +306,24 @@ mod tests { None, None, Some("thumbprint-xyz".to_string()), + SessionOrigin::Unknown, ); assert_eq!(s.dpop_jkt(), Some("thumbprint-xyz")); } + + #[test] + fn session_origin_round_trip_snake_case_strings() { + for o in [ + SessionOrigin::Password, + SessionOrigin::Opaque, + SessionOrigin::MagicLink, + SessionOrigin::Oidc, + SessionOrigin::Device, + SessionOrigin::Unknown, + ] { + assert_eq!(SessionOrigin::from_str(o.as_str()), o); + } + // Unknown catches typos / drift-off-column-values. + assert_eq!(SessionOrigin::from_str("bogus"), SessionOrigin::Unknown); + } } diff --git a/src/infrastructure/repositories/pg/session_pg_repository.rs b/src/infrastructure/repositories/pg/session_pg_repository.rs index 6f6778d6..bd461f66 100644 --- a/src/infrastructure/repositories/pg/session_pg_repository.rs +++ b/src/infrastructure/repositories/pg/session_pg_repository.rs @@ -53,9 +53,9 @@ impl SessionRepository for SessionPgRepository { INSERT INTO auth.sessions ( id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin ) VALUES ( - $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12 + $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13 ) "#, ) @@ -71,6 +71,7 @@ impl SessionRepository for SessionPgRepository { .bind(session_clone.oidc_id_token()) .bind(session_clone.oidc_sid()) .bind(session_clone.dpop_jkt()) + .bind(session_clone.origin().as_str()) .execute(&mut **tx) .await .map_err(Self::map_sqlx_error)?; @@ -116,7 +117,7 @@ impl SessionRepository for SessionPgRepository { SELECT id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin FROM auth.sessions WHERE id = $1 "#, @@ -139,6 +140,7 @@ impl SessionRepository for SessionPgRepository { row.get("oidc_id_token"), row.get("oidc_sid"), row.get("dpop_jkt"), + crate::domain::entities::session::SessionOrigin::from_str(row.get("origin")), )) } @@ -153,7 +155,7 @@ impl SessionRepository for SessionPgRepository { SELECT id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin FROM auth.sessions WHERE refresh_token = $1 "#, @@ -176,6 +178,7 @@ impl SessionRepository for SessionPgRepository { row.get("oidc_id_token"), row.get("oidc_sid"), row.get("dpop_jkt"), + crate::domain::entities::session::SessionOrigin::from_str(row.get("origin")), )) } @@ -189,7 +192,7 @@ impl SessionRepository for SessionPgRepository { SELECT id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin FROM auth.sessions WHERE user_id = $1 ORDER BY created_at DESC @@ -216,6 +219,7 @@ impl SessionRepository for SessionPgRepository { row.get("oidc_id_token"), row.get("oidc_sid"), row.get("dpop_jkt"), + crate::domain::entities::session::SessionOrigin::from_str(row.get("origin")), ) }) .collect(); @@ -244,7 +248,7 @@ impl SessionRepository for SessionPgRepository { SELECT id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin FROM auth.sessions WHERE ($1::uuid IS NULL OR user_id = $1) AND ($2 OR (revoked = false AND expires_at > NOW())) @@ -276,6 +280,7 @@ impl SessionRepository for SessionPgRepository { row.get("oidc_id_token"), row.get("oidc_sid"), row.get("dpop_jkt"), + crate::domain::entities::session::SessionOrigin::from_str(row.get("origin")), ) }) .collect(); @@ -598,9 +603,9 @@ impl SessionStoragePort for SessionPgRepository { INSERT INTO auth.sessions ( id, user_id, refresh_token, expires_at, ip_address, user_agent, created_at, revoked, family_id, - oidc_id_token, oidc_sid, dpop_jkt + oidc_id_token, oidc_sid, dpop_jkt, origin ) VALUES ( - $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12 + $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13 ) "#, ) @@ -616,6 +621,7 @@ impl SessionStoragePort for SessionPgRepository { .bind(session_clone.oidc_id_token()) .bind(session_clone.oidc_sid()) .bind(session_clone.dpop_jkt()) + .bind(session_clone.origin().as_str()) .execute(&mut **tx) .await .map_err(Self::map_sqlx_error)?; diff --git a/src/interfaces/api/handlers/opaque_auth_handler.rs b/src/interfaces/api/handlers/opaque_auth_handler.rs index c3c2180d..00e90ffc 100644 --- a/src/interfaces/api/handlers/opaque_auth_handler.rs +++ b/src/interfaces/api/handlers/opaque_auth_handler.rs @@ -785,7 +785,13 @@ pub async fn login_ke3( // we don't want to have flipped the migration flag for a user // whose login didn't actually complete. let session = auth - .mint_session_for_authenticated_user(user, dto.dpop_jkt, Some(client_ip), user_agent) + .mint_session_for_authenticated_user( + user, + dto.dpop_jkt, + Some(client_ip), + user_agent, + crate::domain::entities::session::SessionOrigin::Opaque, + ) .await .map_err(AppError::from)?;