From 2049bacd2711bb8d11eb2a3fce004adabbc5fc7b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 9 Aug 2026 17:16:03 -0500 Subject: [PATCH] docs(adr-0142): disclose the trust-on-first-use window A.4 left out A.4 is titled "Residual (stated, not hidden)" and states only the availability edge -- a legitimately reassigned username is permanently refused with no rebind path. Read on its own it implies the takeover half is closed. It is not, and the undisclosed half is the more consequential one. THE GUARD IS TRUST-ON-FIRST-USE. Its precondition is that the resolved account ALREADY carries a binding. AC-12 says so in terms -- "WHEN the account is unbound, it SHALL record the binding on that login" -- so on an account that has never federated-logged-in there is nothing to compare, the check short-circuits, and the binding is recorded for whoever arrives first. That is EXACTLY the scenario A.1 names as the gap this amendment closes: an IdP reassigns an already-allowed username to a different principal, the domain check passes, every ladder rung is green, and a session is minted on the prior person's account. So A.1's own attack still succeeds against any unbound account, and the guard then protects the NEW holder's binding against everyone else. Reproduced end to end against shipped code (in-memory SQLite, RS256 id_token, stubbed token exchange, fake LDAP): an account created by AD-password login (oidc_subject NULL) was taken over by a different verified subject presenting the same reassigned preferred_username -- same user row, session minted, the incumbent's LDAP-derived roles inherited -- and a third subject was then refused. The window is not narrow, because unbound is the DEFAULT state: every account predating federation (the entire existing population on day one), plus every account created afterwards by AD-password or Kerberos login, since those callers pass None and write no binding. NO DECISION IS MADE HERE. The two follow-ons -- refuse unbound accounts that predate federation, or an operator pre-binding step -- both carry a rollout cost (lockout vs a new admin surface) and that is an owner call, not a documentation one. The ADR now records the trade instead of half of it. Worth noting (b) is the same admin surface A.4's existing availability follow-on already recommends, so one control can discharge both. Documentation only; no code, no behaviour change. Cross-checked that the insert does not disturb the downstream scorecard's three anchors into this file -- all sit above the insertion point and still resolve. Co-Authored-By: Claude Opus 5 --- ...ode-pkce-relying-party-hybrid-ad-backed.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) 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 48fec4da..c064c702 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 @@ -353,6 +353,45 @@ failure direction (refuse rather than take over) and is narrow, but a real avail 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. +**⚠️ SECOND RESIDUAL, added 2026-08-09 — the guard is trust-on-first-use, so A.1's attack still succeeds +against an UNBOUND account.** This section previously stated only the availability edge above, which read as +though the takeover half were fully closed. It is not, and the omission is the more consequential half. + +The guard's precondition is that the resolved account *already carries* a binding. AC-12 below says so in +terms — "WHEN the account is unbound, it SHALL record the binding on that login" — so on an account that has +never federated-logged-in there is nothing to compare against, the check short-circuits, and the binding is +recorded for whoever arrives first. That is **exactly the scenario A.1 names**: an IdP reassigns an +already-allowed username to a different principal, the domain check passes, every ladder rung is green, and a +session is minted on the prior person's account. The guard then protects the *new* holder's binding against +everyone else. + +Reproduced end to end against the shipped code (in-memory SQLite, RS256 `id_token`, stubbed token exchange, +fake LDAP): an account created by AD-password login (`oidc_subject` NULL) was taken over by a *different* +verified subject presenting the same reassigned `preferred_username` — same user row, session minted, the +incumbent's **LDAP-derived roles inherited** — and a third subject was then refused. + +The window is not narrow, because *unbound* is the default state: + +- every account that predates enabling federation — i.e. the entire existing population on day one; and +- every account created afterwards by AD-password or Kerberos login, since those callers pass `None` and + write no binding (A.2). + +What bounds it: the attacker must be a principal the pinned IdP will mint a token for, within an allow-listed +UPN suffix, and the target AD object must exist (hybrid-only). The suffix allow-list constrains the *suffix*, +never the local part, and defaults to the configured `ad_domain` — the very suffix the organisation's own IdP +mints. So the residual is conditional on IdP tenancy, not on engine configuration. + +**Follow-ons (a decision, not a recommendation — both carry a rollout cost, and this ADR does not pick one):** +either *(a)* refuse an unbound account that predates federation, closing the window at the price of locking out +every existing user until an operator binds them; or *(b)* an operator **pre-binding** step, which avoids the +lockout but needs a new admin surface and a migration path. Note *(b)* is the same admin surface the +availability residual above already recommends, so one control can discharge both. + +Assessed downstream as ASVS **10.5.2** (`partial` — a refusing control exists and ships on, but covers only +bound accounts) and **6.8.1** (`partial` — cross-IdP identity is still keyed on the reassignable username, with +`(issuer, sub)` able only to veto the row that claim already selected). Neither can reach `pass` while this +window is open. + ### 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`