diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 670eb5e2..1bcd78b9 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4681,7 +4681,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre ## 1015. OIDC relying party keys federated accounts on a reassignable username claim while the non-reassignable `sub` is discarded (ASVS 10.5.2) -> ๐Ÿ”ข **Filed 2026-08-04 โ€” not started.** Value **7/10** ยท Difficulty **4/10** ยท _quick win_. The relying party keys federated identity on `oidc_username_claim` (default `preferred_username`), which an IdP is free to **reassign**, while the non-reassignable `sub` is verified and then dropped into an audit field. On first deployment a new holder of a retired username would be handed the prior holder's account. +> โœ… **Closed 2026-08-06 โ€” Option A shipped (subject-continuity guard); ADR 0142 Amendment A owner-ratified.** Value **7/10** ยท Difficulty **4/10** ยท _quick win_. The relying party keyed federated identity on a reassignable username claim while the non-reassignable `sub` was verified then dropped, so on first deployment a new holder of a retired username would have been handed the prior holder's account (ASVS 10.5.2). Fixed by pinning the federated identity to `(issuer, sub)` โ€” two nullable store columns with idempotent three-backend migrations โ€” and refusing a login whose username resolves to an account bound to a different `sub` (`federated_subject_conflict`); the account is still resolved by AD username and roles still come from LDAP. Residual: a legitimately reassigned username is refused with no rebind path, so an operator rebind action is the recommended follow-on. **Cluster:** Security / authentication. **Priority:** P1. **Verdict:** build. **Severity:** high on first deployment โ€” account takeover without any credential compromise. diff --git a/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md b/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md index 7972d0ca..48fec4da 100644 --- a/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md +++ b/docs/adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md @@ -64,7 +64,8 @@ the **same password-free lookup the Kerberos path already uses** โ€” to `LOCAL`, which would route `reauth()` to a password check against a NULL hash and permanently 403 every step-up route. Adding no member leaves that landmine unarmed. - **Zero store work.** No column, no migration, no three-backend parity, no group-map namespace - decision. + decision. **[SUPERSEDED by Amendment A (2026-08-06, BACKLOG #1015): a subject-continuity guard adds two + nullable `oidc_issuer` / `oidc_subject` columns with idempotent three-backend migrations; see Amendment A.]** - **A principal with no on-prem AD object is refused** (`not_in_directory`). Hybrid-only, by design. - **The username claim's UPN suffix is checked against an operator-pinned allow-list** (`[auth].oidc_allowed_username_domains`, defaulting to `ad_domain`) **before** the local part is used @@ -305,3 +306,56 @@ open item 5 โ€” **stays open**, and this ADR's non-sticky OIDC availability flag UPN suffix in `oidc_allowed_username_domains` still succeeds. This cell exists because the control was added late, after a review found the omission was exploitable. - [ ] Confirm whether `truststore` is a base dependency before relying on it for the IdP TLS-trust knob. + +--- + +## Amendment A (2026-08-06) โ€” subject-continuity guard: pin the federated identity to `(issuer, sub)` (BACKLOG #1015) + +> **Status: ACCEPTED โ€” owner-ratified 2026-08-06.** Supersedes the **"Zero store work"** Decision bullet and +> reclassifies the residual that bullet framed as an acceptable *wrong-user login* to a **closed P1 account +> takeover** (ASVS 10.5.2). Built as BACKLOG #1015 (branch `fix-1015-oidc-sub`). *In force* means the store +> carries two nullable columns and the federated path enforces subject continuity โ€” **not** that the lab cells +> are discharged. + +### A.1 The gap this closes +The Decision bounds a claims-parsing bug to a *wrong-user login* ("roles come from LDAP"), and the UPN-suffix +allow-list closes the *attacker-chosen* username-collision path by checking the token's **domain**. Neither +closes a **username reassignment**: an IdP that reassigns an already-allowed username to a **different** +principal (same allowed domain, a **different, non-reassignable `sub`**) resolves โ€” by username โ€” to the +*prior* person's local account and mints a session on it. No credential is compromised, the domain check +passes, every ladder rung is green: an account **takeover**, not a benign wrong-user login. The +non-reassignable `sub` was discarded, so nothing detected the change. + +### A.2 The mechanism (Option A โ€” the AD-backed model is preserved) +The account is **still resolved by its AD username** and roles **still come from LDAP** โ€” every property in the +Decision is unchanged. Added: the account's federated identity is **pinned to `(issuer, sub)`**. On an OIDC +login, after `resolve_principal` succeeds, the resolved account's bound `(oidc_issuer, oidc_subject)` is read; +if it differs from the presented token's, the login is **refused** (`LoginOutcome.reason = +"federated_subject_conflict"`, audited) **before a session is minted**; a first federated login records the +binding. AD-password and Kerberos callers pass `None` and stay byte-identical. + +### A.3 What this overturns, precisely +- **"Zero store work" is superseded** by the minimum a continuity guard requires: two **nullable** columns + `oidc_issuer` / `oidc_subject` on the users table with **idempotent** migrations in all three backends + (SQLite PRAGMA-guarded, SQL Server `COL_LENGTH`-guarded, Postgres `information_schema`-guarded) plus an + `AuthStore` setter. No group-map namespace, no per-message store work; the *Out of scope* "any store + migration" is narrowed to this one additive, nullable, no-backfill pair. +- **AC-1 (byte-identical when `oidc_enabled=false`) is reconciled, not broken.** The idempotent `ALTER` runs + regardless of `oidc_enabled`, so the *schema* carries two nullable columns even with federation off; they are + unread and unwritten on every non-federated path, so **runtime behaviour** with `oidc_enabled=false` is + byte-identical (a no-op migration, not a behavioural change). Read AC-1 as a runtime-behaviour guarantee; + this footnote keeps the ADR and the shipped DDL from disagreeing. + +### A.4 Residual (stated, not hidden) +A **legitimately** reassigned username โ€” a *new* person taking over an old username and presenting a *new* +`sub` โ€” is now permanently refused (`federated_subject_conflict`) with **no rebind path**. That is the *safe* +failure direction (refuse rather than take over) and is narrow, but a real availability edge. **Recommended +follow-on:** an operator **rebind** action that clears/re-binds an account's `(oidc_issuer, oidc_subject)` +after an out-of-band identity check, so a genuine reassignment is an admin operation rather than a lockout. + +### A.5 Acceptance criterion added +- **AC-12 (subject continuity)** โ€” WHEN an OIDC login's username resolves to an account already bound to an + `(issuer, sub)` other than the token's, THE SYSTEM SHALL refuse with an audited `federated_subject_conflict` + and mint no session; WHEN the account is unbound, it SHALL record the binding on that login; WHEN the bound + tuple matches, the login proceeds unchanged -> the federated-path regression tests (changed-sub / same-username + refused; same-sub / changed-username resolves to the same account). diff --git a/messagefoundry/auth/oidc/claims.py b/messagefoundry/auth/oidc/claims.py index a9b31180..e573c446 100644 --- a/messagefoundry/auth/oidc/claims.py +++ b/messagefoundry/auth/oidc/claims.py @@ -95,6 +95,15 @@ class FederatedPrincipal: username: str subject: str + # The pinned issuer this assertion was verified against (``policy.issuer``, which ``_check_core_claims`` + # already proved the token's ``iss`` equals). Carried alongside ``subject`` so the relying party can + # PIN the local federated account's identity to the non-reassignable ``(issuer, sub)`` tuple (BACKLOG + # #1015): the AD-backed account is still resolved BY its username (ADR 0142 keeps roles LDAP-sourced), + # but a login whose reassignable username resolves to an account already bound to a DIFFERENT + # ``(issuer, sub)`` is refused โ€” so a reassigned username cannot take over the prior holder's account. + # A ``sub`` is only guaranteed stable WITHIN one issuer, so the binding needs both halves even though + # a single issuer is pinned today. + issuer: str amr: tuple[str, ...] acr: str | None # The signature-verified ``exp`` (epoch seconds). ADR 0142 AC-6 caps the engine session at it, so @@ -338,6 +347,7 @@ def validate_id_token( return FederatedPrincipal( username=username, subject=subject, + issuer=policy.issuer, amr=amr, acr=acr, expires_at=expires_at, diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 4fd0bf00..54a6c2b1 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -1007,6 +1007,25 @@ async def authenticate_oidc( ok=False, error="user not found in directory", reason="not_in_directory" ) + # BACKLOG #1015 (ADR 0142): subject-continuity guard. The AD-backed account is still RESOLVED by + # its username (roles stay LDAP-sourced), but its federated identity is PINNED to the non- + # reassignable OIDC (issuer, sub). If a local account for this resolved username is already bound + # to a DIFFERENT verified subject, an IdP has reassigned the username to a new person โ€” refuse + # rather than hand the new subject the prior holder's account (the account-takeover-without- + # credential-compromise this item closes). An unbound account (never federated-logged-in) binds + # on first login below, in _complete_ad_login. + bound = await self._store.get_user_by_username(principal.username) + if ( + bound is not None + and bound.oidc_subject is not None + and (bound.oidc_issuer, bound.oidc_subject) + != (principal_claims.issuer, principal_claims.subject) + ): + await self._directory_reject_audit(username, "oidc", "federated_subject_conflict") + return LoginOutcome( + ok=False, error="federated sign-in failed", reason="federated_subject_conflict" + ) + max_expires_at = principal_claims.expires_at if self._settings.oidc_session_max_hours: max_expires_at = min( @@ -1041,6 +1060,7 @@ async def authenticate_oidc( "mfa_verified": mfa_verified, }, max_expires_at=max_expires_at, + federated_subject=(principal_claims.issuer, principal_claims.subject), ) async def _directory_reject_audit(self, actor: str, mech: str, reason: str) -> None: @@ -1062,7 +1082,12 @@ async def _complete_ad_login( mech: str | None = None, evidence: Mapping[str, object] | None = None, max_expires_at: float | None = None, + federated_subject: tuple[str, str] | None = None, ) -> LoginOutcome: + # ``federated_subject`` is the verified OIDC ``(issuer, sub)`` and is passed ONLY by the + # federated path (BACKLOG #1015). It defaults to None, so the AD-simple-bind and Kerberos + # callers stay byte-identical โ€” no extra store write, no changed audit row. The federated + # caller has already enforced the subject-continuity guard before reaching here. existing = await self._store.get_user_by_username(principal.username) if existing is not None and existing.auth_provider != AuthProvider.AD.value: # Never let an AD login adopt/overwrite a like-named LOCAL account (provider confusion). @@ -1074,6 +1099,20 @@ async def _complete_ad_login( ) return LoginOutcome(ok=False, error="account conflict") user = await self._upsert_ad_user(principal) + if ( + federated_subject is not None + and ( + user.oidc_issuer, + user.oidc_subject, + ) + != federated_subject + ): + # First federated login for this account (or an unbound AD account's first): record the + # (issuer, sub) binding so a later reassigned-username login carrying a different subject is + # refused by the guard above. A matching binding is left untouched (no updated_at churn). + await self._store.set_user_federated_subject( + user.id, federated_subject[0], federated_subject[1] + ) role_ids = sorted(await self._store.roles_for_ad_groups(principal.groups)) previous = set(await self._store.get_user_role_ids(user.id)) await self._store.set_user_roles(user.id, role_ids, assigned_by="ad-sync") diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index 27cf2cd1..deb3a07a 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -1685,6 +1685,14 @@ async def set_user_channel_scope( self, user_id: str, scope_json: str | None, *, now: float | None = None ) -> None: ... + async def set_user_federated_subject( + self, user_id: str, issuer: str, subject: str, *, now: float | None = None + ) -> None: + """Bind a user's verified federated ``(issuer, sub)`` identity (BACKLOG #1015). Recorded on the + first federated login so a later login whose reassignable username resolves to this account but + carries a different subject is refused, not handed the account.""" + ... + async def roles_for_ad_groups(self, groups: Iterable[str]) -> set[str]: ... async def list_ad_group_role_map(self) -> Sequence[Row]: ... diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 2a03b502..eac02da1 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -521,7 +521,9 @@ def __init__(self, method: str, outbox_ids: tuple[str, ...]) -> None: totp_enabled BOOLEAN NOT NULL DEFAULT FALSE, totp_enrolled_at DOUBLE PRECISION, totp_recovery_codes TEXT, - last_totp_step INTEGER + last_totp_step INTEGER, + oidc_issuer TEXT, + oidc_subject TEXT )""", """CREATE TABLE IF NOT EXISTS roles ( id TEXT PRIMARY KEY, @@ -1074,12 +1076,16 @@ async def _migrate_lease_columns(self, conn: Any) -> None: "SELECT column_name FROM information_schema.columns WHERE table_name='users'" ) } + # Federated (issuer, sub) identity keying (BACKLOG #1015): NULL on existing rows = "not yet + # federated" (username stays the sole key), byte-identical to before. Idempotent. for column, decl in ( ("totp_secret", "TEXT"), ("totp_enabled", "BOOLEAN NOT NULL DEFAULT FALSE"), ("totp_enrolled_at", "DOUBLE PRECISION"), ("totp_recovery_codes", "TEXT"), ("last_totp_step", "INTEGER"), + ("oidc_issuer", "TEXT"), + ("oidc_subject", "TEXT"), ): if column not in users_cols: await conn.execute(f"ALTER TABLE users ADD COLUMN {column} {decl}") @@ -6463,6 +6469,19 @@ async def set_user_channel_scope( "UPDATE users SET channel_scope=$1, updated_at=$2 WHERE id=$3", scope_json, now, user_id ) + async def set_user_federated_subject( + self, user_id: str, issuer: str, subject: str, *, now: float | None = None + ) -> None: + """Bind a user's federated ``(issuer, sub)`` identity (BACKLOG #1015).""" + now = time.time() if now is None else now + await self._execute( + "UPDATE users SET oidc_issuer=$1, oidc_subject=$2, updated_at=$3 WHERE id=$4", + issuer, + subject, + now, + user_id, + ) + async def roles_for_ad_groups(self, groups: Iterable[str]) -> set[str]: normalized = sorted({g.strip().lower() for g in groups if g.strip()}) if not normalized: diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 181431f3..c3b1ecfa 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -1352,7 +1352,8 @@ def __init__(self, conn: Any, cur: Any) -> None: failed_attempts INT NOT NULL DEFAULT 0, locked_until FLOAT NULL, channel_scope NVARCHAR(MAX) NULL, totp_secret NVARCHAR(MAX) NULL, totp_enabled BIT NOT NULL DEFAULT 0, totp_enrolled_at FLOAT NULL, - totp_recovery_codes NVARCHAR(MAX) NULL, last_totp_step INT NULL)""", + totp_recovery_codes NVARCHAR(MAX) NULL, last_totp_step INT NULL, + oidc_issuer NVARCHAR(MAX) NULL, oidc_subject NVARCHAR(MAX) NULL)""", """IF COL_LENGTH('users','channel_scope') IS NULL ALTER TABLE users ADD channel_scope NVARCHAR(MAX) NULL""", # MFA (WP-14): TOTP columns ALTER-ed in for a pre-existing users table (idempotent). @@ -1367,6 +1368,12 @@ def __init__(self, conn: Any, cur: Any) -> None: # Single-use TOTP within the step window (ASVS 6.5.1): highest consumed time-step. """IF COL_LENGTH('users','last_totp_step') IS NULL ALTER TABLE users ADD last_totp_step INT NULL""", + # Federated (issuer, sub) identity keying (BACKLOG #1015): COL_LENGTH-gated ADD on a pre-existing + # users table. NULL on existing rows = "not yet federated" (username stays the sole key). Idempotent. + """IF COL_LENGTH('users','oidc_issuer') IS NULL + ALTER TABLE users ADD oidc_issuer NVARCHAR(MAX) NULL""", + """IF COL_LENGTH('users','oidc_subject') IS NULL + ALTER TABLE users ADD oidc_subject NVARCHAR(MAX) NULL""", """IF OBJECT_ID('roles','U') IS NULL CREATE TABLE roles ( id NVARCHAR(64) NOT NULL PRIMARY KEY, display_name NVARCHAR(128) NOT NULL, description NVARCHAR(512) NULL, builtin BIT NOT NULL DEFAULT 1, @@ -9443,6 +9450,16 @@ async def set_user_channel_scope( (scope_json, now, user_id), ) + async def set_user_federated_subject( + self, user_id: str, issuer: str, subject: str, *, now: float | None = None + ) -> None: + """Bind a user's federated ``(issuer, sub)`` identity (BACKLOG #1015).""" + now = time.time() if now is None else now + await self._execute( + "UPDATE users SET oidc_issuer=?, oidc_subject=?, updated_at=? WHERE id=?", + (issuer, subject, now, user_id), + ) + async def roles_for_ad_groups(self, groups: Iterable[str]) -> set[str]: normalized = sorted({g.strip().lower() for g in groups if g.strip()}) if not normalized: diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 0ed1888e..a0f33455 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -772,6 +772,14 @@ class UserRecord: # via the store's get_totp_secret / get_recovery_code_hashes accessors. totp_enabled: bool = False totp_enrolled_at: float | None = None + # Federated-account identity (BACKLOG #1015, ADR 0142): the verified OIDC ``(issuer, sub)`` this + # AD-backed account's federated identity is PINNED to. Non-reassignable, unlike the display username. + # The account is still resolved by its username; this binding only refuses a login whose username + # resolves here but carries a different subject. NULL on a local account and on an AD account that + # has never completed a federated login. Set on the first federated login and enforced on every + # subsequent one, so a reassigned username cannot hand the account to a new subject. + oidc_issuer: str | None = None + oidc_subject: str | None = None @classmethod def from_mapping(cls, d: Mapping[str, Any]) -> UserRecord: @@ -793,6 +801,8 @@ def from_mapping(cls, d: Mapping[str, Any]) -> UserRecord: channel_scope=d.get("channel_scope"), totp_enabled=bool(d.get("totp_enabled", 0)), totp_enrolled_at=_opt_float(d.get("totp_enrolled_at")), + oidc_issuer=d.get("oidc_issuer"), + oidc_subject=d.get("oidc_subject"), ) @@ -1595,7 +1605,9 @@ def _append_channel_scope( totp_enabled INTEGER NOT NULL DEFAULT 0, -- TOTP enrolled + confirmed active totp_enrolled_at REAL, totp_recovery_codes TEXT, -- JSON list of argon2id hashes of single-use recovery codes - last_totp_step INTEGER -- highest TOTP time-step already consumed (single-use within window, ASVS 6.5.1); NULL = none yet + last_totp_step INTEGER, -- highest TOTP time-step already consumed (single-use within window, ASVS 6.5.1); NULL = none yet + oidc_issuer TEXT, -- federated identity (BACKLOG #1015): verified OIDC issuer; NULL = not federated / never federated-logged-in + oidc_subject TEXT -- federated identity (BACKLOG #1015): verified OIDC sub; the account's federated login is pinned to (issuer, sub), refusing a reassigned username ); CREATE TABLE IF NOT EXISTS roles ( @@ -3042,12 +3054,17 @@ async def _migrate(db: aiosqlite.Connection) -> None: # --- end custom RBAC roles (ADR 0045) -------------------------------- # MFA (WP-14): a pre-existing DB's users predate the TOTP columns โ€” ALTER them in (NULL/0 on # existing rows = "not enrolled", correct). Idempotent: skipped once present. + # Federated (issuer, sub) identity keying (BACKLOG #1015): a pre-existing DB's users predate the + # columns โ€” ALTER them in (NULL on existing rows = "not yet federated", byte-identical to before, + # since the username stayed the sole key). Idempotent: skipped once present. for column, decl in ( ("totp_secret", "TEXT"), ("totp_enabled", "INTEGER NOT NULL DEFAULT 0"), ("totp_enrolled_at", "REAL"), ("totp_recovery_codes", "TEXT"), ("last_totp_step", "INTEGER"), + ("oidc_issuer", "TEXT"), + ("oidc_subject", "TEXT"), ): if column not in user_cols: await db.execute(f"ALTER TABLE users ADD COLUMN {column} {decl}") @@ -8164,6 +8181,20 @@ async def set_user_channel_scope( ) await self._commit() + async def set_user_federated_subject( + self, user_id: str, issuer: str, subject: str, *, now: float | None = None + ) -> None: + """Bind a user's federated ``(issuer, sub)`` identity (BACKLOG #1015). Recorded on the first + federated login so a later login carrying a different ``sub`` for a reassigned username is + refused rather than handed the prior subject's account.""" + now = time.time() if now is None else now + async with self._lock: + await self._db.execute( + "UPDATE users SET oidc_issuer=?, oidc_subject=?, updated_at=? WHERE id=?", + (issuer, subject, now, user_id), + ) + await self._commit() + async def roles_for_ad_groups(self, groups: Iterable[str]) -> set[str]: normalized = sorted({g.strip().lower() for g in groups if g.strip()}) if not normalized: diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 4447d313..e5c12a79 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -256,6 +256,16 @@ def test_verified_exp_is_carried_not_reparsed(rsa_key: rsa.RSAPrivateKey) -> Non assert principal.expires_at == 1_002_500 +def test_federated_principal_carries_issuer(rsa_key: rsa.RSAPrivateKey) -> None: + """BACKLOG #1015: the principal carries the pinned ``issuer`` so the relying party can key the local + account on the non-reassignable ``(issuer, sub)`` tuple, not on the reassignable username. It is the + policy issuer (already proven equal to the token's ``iss`` by the core-claim rung), not sub-only.""" + jws = _mint(rsa_key, "k1", _good_claims()) + principal = oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert principal.issuer == "https://idp.example" + assert principal.subject == "S-1-5-21-abc" + + @pytest.mark.parametrize( ("mutate", "reason"), [ diff --git a/tests/test_auth_oidc_service.py b/tests/test_auth_oidc_service.py index a3f1a3c1..bee45cb3 100644 --- a/tests/test_auth_oidc_service.py +++ b/tests/test_auth_oidc_service.py @@ -28,7 +28,7 @@ from messagefoundry.auth import oidc from messagefoundry.auth.identity import AuthProvider from messagefoundry.auth.ldap import AdPrincipal -from messagefoundry.auth.service import AuthService +from messagefoundry.auth.service import AuthService, LoginOutcome from messagefoundry.auth.tokens import hash_token from messagefoundry.config.models import SignatureAlgorithm from messagefoundry.config.settings import AuthSettings @@ -125,8 +125,18 @@ def _settings(**over: Any) -> AuthSettings: class _FakeLdap: - def __init__(self, principal: AdPrincipal | None = PRINCIPAL) -> None: + def __init__( + self, + principal: AdPrincipal | None = PRINCIPAL, + *, + by_username: dict[str, AdPrincipal | None] | None = None, + ) -> None: + # ``by_username`` maps a (domain-stripped) resolve key to the AD object it resolves to, so a test + # can express a CHANGED OIDC display-username that still resolves to the same AD object (BACKLOG + # #1015). When it is None (the default), the fixed ``principal`` is returned for any username, so + # every pre-existing caller is unchanged. self._principal = principal + self._by_username = by_username self.resolved: list[str] = [] def authenticate(self, username: str, password: str) -> AdPrincipal | None: @@ -134,6 +144,8 @@ def authenticate(self, username: str, password: str) -> AdPrincipal | None: def resolve_principal(self, username: str) -> AdPrincipal | None: self.resolved.append(username) + if self._by_username is not None: + return self._by_username.get(username, self._principal) return self._principal @@ -180,6 +192,20 @@ async def _audit_rows(store: MessageStore, action: str) -> list[Mapping[str, Any return [a for a in await store.list_audit() if a["action"] == action] +async def _oidc_login( + service: AuthService, + monkeypatch: pytest.MonkeyPatch, + rsa_key: rsa.RSAPrivateKey, + **claim_over: Any, +) -> LoginOutcome: + """Run one federated login, stubbing the token exchange to return a freshly-minted id_token whose + claims are ``_claims(**claim_over)`` (so a test can vary ``sub`` / ``preferred_username``).""" + _stub_exchange(monkeypatch, _mint(rsa_key, _claims(**claim_over))) + return await service.authenticate_oidc( + AUTH_CODE, _flow(), redirect_uri="https://ops.example/ui/oidc/callback" + ) + + # --- #285 (ASVS 6.7.1): the enforcement dial reaches the OIDC anchor's construction seam ------------ @@ -304,6 +330,107 @@ async def test_an_alternate_upn_suffix_is_accepted_when_allow_listed( await store.close() +# --- #1015: the account is keyed on (issuer, sub), never the reassignable username ----------------- + + +async def test_changed_subject_same_username_does_not_take_over( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """BACKLOG #1015, the P1 regression. A first federated login binds the local account to its verified + subject. When the IdP later REASSIGNS the username to a different person (a new ``sub``, a normal IdP + lifecycle operation), that new subject must NOT be handed the prior holder's account โ€” that is account + takeover with no credential compromise. The login is refused with a closed-set audit reason and the + bound account is left untouched.""" + store = await MessageStore.open(":memory:") + try: + service = await _service(store, rsa_key) + first = await _oidc_login(service, monkeypatch, rsa_key, sub="S-1-alice") + assert first.ok and first.identity is not None + account = await store.get_user_by_username("jdoe") + assert account is not None + assert account.oidc_subject == "S-1-alice" # bound on the first federated login + + # The username is reassigned to a new person (new sub); they complete a federated login. + second = await _oidc_login(service, monkeypatch, rsa_key, sub="S-1-bob") + assert not second.ok and second.token is None + assert second.reason == "federated_subject_conflict" + + after = await store.get_user_by_username("jdoe") + assert after is not None + assert after.id == account.id # the same account, not taken over + assert after.oidc_subject == "S-1-alice" # still bound to the ORIGINAL subject + rows = await _audit_rows(store, "auth.login_failed") + assert any('"reason": "federated_subject_conflict"' in (r["detail"] or "") for r in rows) + finally: + await store.close() + + +async def test_same_subject_changed_username_is_same_account( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """The converse of the takeover case: the SAME person (same ``sub``) whose display username changed + but still resolves to the same on-prem AD object must land on the SAME local account, with the + display refreshed โ€” not be refused and not fork a second row.""" + store = await MessageStore.open(":memory:") + try: + renamed = AdPrincipal( + username="jdoe", # same AD object (stable sAMAccountName), new display name + display_name="Jane Doe-Smith", + email="jane@corp.example", + dn="CN=jdoe,DC=corp,DC=example", + groups=PRINCIPAL.groups, + ) + ldap = _FakeLdap(by_username={"jdoe": PRINCIPAL, "jsmith": renamed}) + service = await _service(store, rsa_key, ldap=ldap) + + first = await _oidc_login( + service, monkeypatch, rsa_key, sub="S-1-alice", preferred_username="jdoe@corp.example" + ) + assert first.ok + account = await store.get_user_by_username("jdoe") + assert account is not None + assert account.oidc_subject == "S-1-alice" + assert account.display_name == "J Doe" + + # Same subject, a changed preferred_username that AD resolves to the same object. + second = await _oidc_login( + service, monkeypatch, rsa_key, sub="S-1-alice", preferred_username="jsmith@corp.example" + ) + assert second.ok and second.identity is not None + after = await store.get_user_by_username("jdoe") + assert after is not None + assert after.id == account.id # same local account + assert after.oidc_subject == "S-1-alice" # binding stable + assert after.display_name == "Jane Doe-Smith" # refreshed from the resolved AD object + assert await store.get_user_by_username("jsmith") is None # no forked row + finally: + await store.close() + + +async def test_username_reused_across_two_subjects_does_not_collide( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """Two distinct subjects present the same display username at different times. The second must be + refused cleanly (no unhandled exception, a closed-set audit reason) and must not share the first + subject's account or receive a session.""" + store = await MessageStore.open(":memory:") + try: + service = await _service(store, rsa_key) + first = await _oidc_login(service, monkeypatch, rsa_key, sub="S-1-alice") + assert first.ok + + second = await _oidc_login(service, monkeypatch, rsa_key, sub="S-1-carol") + assert not second.ok and second.token is None + assert second.reason == "federated_subject_conflict" + + account = await store.get_user_by_username("jdoe") + assert account is not None + assert account.oidc_subject == "S-1-alice" # still the first subject's account + assert len([u for u in await store.list_users() if u.username == "jdoe"]) == 1 + finally: + await store.close() + + # --- AC-6: the session is capped at the verified id_token exp ---------------------------------------