docs(adr-0142): disclose the trust-on-first-use window A.4 left out - #307
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
### A.4 Residual (stated, not hidden)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 isn't, 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:
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 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. 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, not reasoned
Against shipped code (in-memory SQLite, RS256
id_token, stubbed token exchange, fake LDAP): an account created by AD-password login (oidc_subjectNULL) was taken over by a different verified subject presenting the same reassignedpreferred_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
Unbound is the default state:
Noneand 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.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). That's an owner call, not a documentation one. This PR 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.
Downstream
Assessed 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 reachpasswhile this window is open.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.
Third of three from the same review: #293 (TOTP clamp docs), #295 (anchor drift vs invalidation).
🤖 Generated with Claude Code