From b05bae4fdcf32b1ad1f6840a2eb31a1e25d4bcd4 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 00:21:24 -0500 Subject: [PATCH 1/4] fix(auth): pin OIDC federated identity to the non-reassignable (issuer, sub) (BACKLOG #1015) The OIDC relying party resolved the AD-backed account by the reassignable username claim (oidc_username_claim, default preferred_username) and dropped the verified sub into an audit field. On first deployment a reassigned username (a normal IdP lifecycle operation) would let a new subject be handed the prior holder's account -- account takeover with no credential compromise (ASVS 10.5.2). This is wrong in the shipped code; NOT-DEPLOYED beta, so it is stated in the conditional and there is no stored account to migrate. Fix -- subject-continuity guard. FederatedPrincipal now carries the pinned issuer alongside sub. The account is STILL resolved by its AD username (ADR 0142 keeps roles LDAP-sourced and the principal must exist on-prem), but its federated login is now PINNED to the verified (issuer, sub): the binding is recorded on the first federated login and enforced on every later one, so a login whose username resolves to an account already bound to a DIFFERENT subject is refused (federated_subject_conflict, a closed-set audit reason that collapses to the generic oidc_failed on the login page) rather than adopted. Store: adds nullable oidc_issuer/oidc_subject columns and set_user_federated_subject across SQLite, SQL Server, and Postgres (idempotent ALTERs, byte-parallel to the shipped totp migration; SELECT * + from_mapping reads them on all three, NULL = never federated). Persisting the non-reassignable subject is unavoidable for any (issuer, sub) keying, so this is store work ADR 0142 recorded as out of scope ("zero store work"). This lane does not allocate or amend an ADR by design -- ADR 0142 must be amended by the owner in the same PR (see PR body). Tests: three regressions (changed-subject-same-username refused; same-subject- changed-username stays the same account with the display refreshed; a username reused across two subjects refused without collision) plus a claims test that the principal carries the issuer. Each was falsified -- guard neutralized -> the two takeover tests go red (audit shows login_success for actor jdoe carrying a different sub); issuer broken -> the claims test goes red; binding write suppressed -> all three service tests go red. Owner decision pending (see PR body): Option A (this guard, which fits the AD-backed model) vs Option B (drop users.username UNIQUE and key the row on (issuer, sub), username display-only). Option A is fail-closed: a legitimately reassigned username is refused with no automated rebind path. --- messagefoundry/auth/oidc/claims.py | 10 +++ messagefoundry/auth/service.py | 39 +++++++++ messagefoundry/store/base.py | 8 ++ messagefoundry/store/postgres.py | 21 ++++- messagefoundry/store/sqlserver.py | 19 ++++- messagefoundry/store/store.py | 33 +++++++- tests/test_auth_oidc.py | 10 +++ tests/test_auth_oidc_service.py | 131 ++++++++++++++++++++++++++++- 8 files changed, 266 insertions(+), 5 deletions(-) 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 --------------------------------------- From 36c2a62472d601166a9307b79562fec89d1c9097 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 00:21:51 -0500 Subject: [PATCH 2/4] backlog: mark #1015 in progress -- subject-continuity guard built, awaiting owner review (BACKLOG #1015) Flip only the #1015 banner from not-started to in-progress: Option A (an OIDC subject-continuity guard) is built and committed, but #1015 is NOT closed -- it awaits two owner decisions: (1) ratify Option A vs Option B (full re-key), and (2) authorize an ADR 0142 amendment (this change adds oidc_issuer/oidc_subject store columns + three-backend migrations that ADR 0142's "zero store work" decision forecloses). Only the banner line under the 1015 heading changed; the census was NOT recomputed. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e3142125..c5472bba 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4679,7 +4679,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. +> 🚧 **In progress 2026-08-06 — subject-continuity guard built (Option A); awaiting owner review of the ADR-0142 store-column amendment and the Option-A-vs-B account-model choice.** 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. **Cluster:** Security / authentication. **Priority:** P1. **Verdict:** build. **Severity:** high on first deployment — account takeover without any credential compromise. From 8ffc36fe27b5ff8d145ae71b9309b5cfdf9e32e7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 04:11:39 -0500 Subject: [PATCH 3/4] docs(adr): ADR 0142 Amendment A -- subject-continuity guard supersedes zero-store-work (BACKLOG #1015) Owner-ratified 2026-08-06. #1015's fix (Option A, committed earlier on this branch) adds oidc_issuer/oidc_subject store columns + a federated-path subject-continuity guard, which overturns ADR 0142's "Zero store work" Decision and reclassifies the username-reassignment residual it framed as an acceptable wrong-user login to a closed P1 account takeover. Amendment A records the actual mechanism (account still resolved by AD username, federated identity pinned to (issuer,sub)), narrows the store-migration out-of-scope to the two nullable columns, reconciles AC-1 (runtime byte-identical when oidc_enabled is false -- the idempotent ALTER runs regardless, a no-op migration), states the reassigned-username availability residual + the recommended operator rebind follow-on, and adds AC-12 (subject continuity). A superseding pointer is added at the Zero-store-work bullet. Docs only. --- ...ode-pkce-relying-party-hybrid-ad-backed.md | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) 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). From fb288c0a3c20afde6be14d36353efea328fd79c5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 04:13:05 -0500 Subject: [PATCH 4/4] backlog: close #1015 -- Option A subject-continuity guard + ADR 0142 Amendment A (BACKLOG #1015) Flip #1015 from in-progress to closed: Option A (the subject-continuity guard) is built and ADR 0142 Amendment A is owner-ratified, so the account-takeover is fixed and the code and its governing ADR are now self-consistent. Only #1015's banner line under its heading changed; the census was NOT recomputed. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index c5472bba..ffea73d6 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4679,7 +4679,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) -> 🚧 **In progress 2026-08-06 — subject-continuity guard built (Option A); awaiting owner review of the ADR-0142 store-column amendment and the Option-A-vs-B account-model choice.** 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.