Skip to content

sec(auth): hold every failed challenge to a fixed deadline, on both seams (BACKLOG #1140) - #930

Open
wshallwshall wants to merge 3 commits into
mainfrom
claude/mgr-1140
Open

sec(auth): hold every failed challenge to a fixed deadline, on both seams (BACKLOG #1140)#930
wshallwshall wants to merge 3 commits into
mainfrom
claude/mgr-1140

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

Closes nothing. BACKLOG #1140 is Verdict: research / Closing-act: scorecard-rescore, so this PR does not close it, does not flip its banner and writes nothing to the vault. It builds the named residual and corrects the row. The item stays open.

What this builds

The item's "DELIBERATELY NOT BUILT" list, items (a) and (c) -- one change sited at two seams.

A fixed-deadline failure equaliser: _failure_deadline + _sleep_until + AuthService._equalize_failure in messagefoundry/auth/service.py, wrapping login and authenticate_kerberos. Every failed outcome is held to an instant fixed before dispatch, so its latency is a function of the start time and nothing else -- not of which branch ran, and so not of anything about the username. Successes return unpadded: a valid credential has already disclosed that the account exists, and enumeration is about telling two failures apart.

(c) is covered without editing messagefoundry_webconsole/routes/sso.py. The pad sits on the service method, so GET /ui/sso and the JSON Kerberos leg inherit it from one site; a route-local pad would have equalized one caller and left the other to be found later. A test drives the real route and asserts the pad fired, rather than asserting it from the call graph.

What the pad buys at /ui/sso, and what it does not. The route already collapses every reject to one 303 to /ui/login?e=sso_failed, so latency was the last channel separating an unresolvable principal (a directory search) from a like-named local account (that search plus a store lookup) from a directory outage (the full ad_connect_timeout) from "SSO is not configured" (no work at all). It does not buy username-enumeration protection in 6.3.8's sense, because the attacker does not choose the username there -- SPNEGO supplies it from a KDC-issued ticket. What it removes is a caller learning which reject branch its one presentable principal landed in. It also does not cover kerberos_available == False, which the route answers with a different error code before reaching the service; that is a server-wide configuration fact, identical for every principal and already disclosed in the redirect. The two rejects the route answers itself (malformed base64, non-navigation fetch) are deliberately not padded and are pinned as such.

Re-measurement first, and it changed the picture

Re-measured by execution before building, in-process against a real SQLite store, warmed and interleaved, 25 samples per branch.

Three claims in the row are stale at HEAD.

  1. The re-score banner still reads "the asymmetry stands verbatim at HEAD" citing auth/ldap.py:243-244. The 2026-09-03 entry already recorded that as false; it is still false.
  2. The Severity line says a first deployment would inherit a directory-namespace enumeration oracle through the AD leg. BACKLOG #1137 retired directory password sign-in on 2026-08-22. login(provider=AD) refuses before any lookup, and LdapAuthenticator.authenticate -- the _equalizing_bind path -- has exactly one caller left, _reauth_ad, which is post-session and per-actor rate-limited. That oracle is no longer reachable from an unauthenticated seam.
  3. The item names "an extra awaited store write only on the valid-user-wrong-password branch" as a live residual. It is real in the code and not measurable on this store: 45.99 ms vs 45.68 ms, a 0.31 ms gap inside a 13 ms p10-p90 band. Both are dominated by the one argon2id verify (~40 ms).

The loudest channel is one the item never names, because #1137 created it two days after the research was written: the retired-AD refusal answered in 0.61 ms against 45.7-49.6 ms for every local branch -- a 75x spread, selectable through the request's own provider field. It is not a username oracle (it refuses identically for every username), so it does not restore the severity the row claims, but it is squarely what the verb grades. Second finding: the bootstrap spelling is the one local branch measurably slower than its siblings, 49.58 ms vs 45.7-46.2 ms, from the extra lookup plus #1268's supersession check.

branch before after
unknown username 45.99 ms 511.04 ms
valid user, wrong password 45.68 ms 511.60 ms
locked account 46.16 ms 511.33 ms
bootstrap username 49.58 ms 512.04 ms
AD pathway retired 0.61 ms 511.83 ms
spread 48.97 ms (75x) 1.00 ms

The ~11 ms above the 500 ms budget is asyncio.sleep wake-up jitter (Windows timer granularity ~15.6 ms), identical across branches including the one that does no work -- so it is scheduler noise, not branch signal. At n=8 an apparent 9 ms gap on one branch looked real; at n=40 it collapsed to 1.00 ms across all five.

The item's "hard-bound the directory call at the same budget" is declined, with a reason

Bounding the LDAP call at 0.5 s would refuse legitimate logins against any directory slower than that, and ad_connect_timeout / ad_receive_timeout default to 10 s each precisely because a real controller can be slow. The fail-open the item is right to fear is closed differently: _failure_deadline quantizes an overrun up to the next whole multiple of the budget instead of giving up, so it can never return a deadline at or behind now. An overrun then discloses a slot index rather than the raw elapsed -- and on the directory leg an overrun discloses an availability fact identical for every principal. A per-seam, per-process latched warning reports that the budget is wrong for the hardware; it is latched because the login surface is unauthenticated and a warning per overrun would be the same unbounded log amplifier the rate-limited audit paths exist to avoid.

The budget is a module constant, not an operator setting: an operator who could lower it could silently disable the control. 0.5 s is about 9x the measured p90.

Verified by mutation. All six were invisible to the pre-existing suite

tests/test_asvs_login_deadline.py -- 31 tests, offline, no directory, on every CI leg, following the tests/test_asvs_audit_constant_time.py pattern. Six mutations of the shipped code, each run against the new file and against the 121 pre-existing tests in test_auth_hardening + test_api_auth + test_auth_core + test_auth_session_lifecycle:

mutation new file 121 pre-existing
pad removed from the login seam RED (2) GREEN
pad removed from the Kerberos seam RED (3) GREEN
deadline steered by the attempted username RED (1) GREEN
overrun made to fail open RED (12) GREEN
successes padded too RED (2) GREEN
warning un-latched RED (1) GREEN

The third is the one the row's own warning is about. The tests assert invariance across branches, never equality against a shared symbol -- so a deadline computed from caller input reds them where a tautological equality assertion would pass.

Two existing guards interacted, and neither was loosened

tests/test_docs_security_pathways.py treats every _login* coroutine returning a LoginOutcome as a per-provider pathway owing a docs/SECURITY.md row (ASVS 6.1.3), so the inner method is named _dispatch_login, not _login -- a pure refactoring must not force that guard to be loosened to fit. Its sibling assertion AST-reads mfa_verified=True out of authenticate_kerberos; the grant moved one frame down into _authenticate_kerberos, so the anchor follows the fact, exactly as that test's own #1137 note did for _login_ad. Both still fail in the direction they were built to catch. This is the only test file I changed that I did not create, and it is a one-line anchor change plus a docstring note.

Contention -- PRs I read before editing messagefoundry/auth/service.py

Read 832, 842, 845, 851, and their service.py hunk ranges specifically. The change is purely additive to that file (146 insertions, 0 deletions at the point I checked; the only structural change is two method signatures being renamed) and is sited away from every one of their hunks: mine are at the constants block (about line 108), login (about 833) and authenticate_kerberos (about 1043); theirs are at 26/233/1384/1402/3117 (832), 153/947/1355/1377/1753+ (842), 57/96/289 (845) and 275 (851).

One likely conflict, named rather than worked around: PR 842. It rewrites the comment block immediately above _complete_ad_login inside authenticate_kerberos (changing mfa_verified=True to False for BACKLOG #1144) and it touches tests/test_docs_security_pathways.py. My split moved that call into _authenticate_kerberos and re-anchored that test's AST read. Whichever lands second should keep 842's mfa_verified=False and my _authenticate_kerberos anchor -- they are compatible, but the merge will need a human. Item 1136 (_ensure_bootstrap_admin, first-run path) and 1139 (directory-repoint notification address) do not touch either seam; no collision seen.

I did not edit api/auth_routes.py, config/settings.py, messagefoundry_webconsole/routes/, or anything under messagefoundry/auth/ other than service.py.

Interpreter and versions

All readings from this worktree's own venv, printed with sys.executable beside them:

sys.executable = ...\worktrees\manager-277cbb-b1140\.venv\Scripts\python.exe   (3.14.6)
argon2-cffi 25.1.0   argon2-cffi-bindings 25.1.0   ldap3 2.9.1   cryptography 50.0.1

All four match requirements.lock exactly, so the argon2 timing that sizes the budget is evidence about shipped behaviour. I did not use the system python or the primary checkout's venv.

Checks I ran

  • ruff check . -- pass. ruff format --check . -- pass (1257 files).
  • mypy messagefoundry (strict) -- pass, 268 source files.
  • pytest tests/test_asvs_login_deadline.py -- 31 passed.
  • pytest on test_docs_security_pathways + test_auth_hardening + test_api_auth + test_auth_core + test_auth_session_lifecycle + test_seam_discovery + test_security_static -- 245 passed.
  • pytest packaging/messagefoundry-webconsole/tests -- 422 passed, 3 skipped. This is the suite carrying the 85 /ui/login posts and the /ui/sso legs.
  • pytest tests/test_seam_discovery.py tests/test_security_doc_drift.py tests/test_security_doc_rate_limits.py tests/test_step_up.py tests/test_mfa.py tests/test_mfa_access_gate.py -- pass.
  • Pre-commit ran on the commit: ruff format, ruff check, ledger gate, backlog parse, leak guard, SPDX, control chars, secrets, bandit -- all passed.
  • Glyph scan of every line I added, with a positive control that detected U+2705 / U+26A0 / U+1F600 -- clean. The glyphs remaining in docs/BACKLOG.md are the pre-existing banner alphabet and the #1265 population, none introduced here.
  • CI-time cost of the pad measured, since it adds latency to failed logins: 63.16 s vs 58.89 s across the 121 auth tests, about 4 s. No conftest budget override is needed, so the shipped default is exercised end to end.

Checks I could NOT run

  • The full tests/ suite did not finish. Started in the background and reached 8% in about 30 minutes under fleet contention, clean so far. This is the known local-starvation mode, not a symptom of this change. Somebody must read the hosted test legs -- all four, including py3.11 -- after I exit.
  • The sql-server and postgres store legs skip silently without a live container and were not run. This change touches no store code, so I expect no interaction, but that is an expectation and not a measurement.
  • windows-service-smoke and the other hosted-runner-only legs -- never visible to a builder.

What I deliberately left

  • (b) the conflict branch and (d) the LDAPException asymmetry -- both recorded as deliberate in the row, and the brief said to leave them. Note that (c)'s pad now equalizes (b)'s timing at the Kerberos seam, which is the part a branch-parity test inside the directory module could never see; the disclosure itself is untouched.
  • The shared bind-then-release helper and the module-level decoy CN, still declined for the reasons the row gives.
  • authenticate_oidc was considered and left unpadded, which is a judgment I am flagging rather than burying: its cost is dominated by an unbounded round trip to the IdP token endpoint, its branch set is configuration and flow state rather than anything the caller names, and padding it would be theatre over external latency. If a reviewer disagrees, it is a three-line change.

Mode detected: not ultracode -- no system-reminder in my context said so.

…eams (BACKLOG #1140)

ASVS 6.3.8 asks that valid users not be deducible from failed authentication
challenges, including by different response times. Messages and status codes
were already uniform on both challenge seams; latency was not.

Adds a fixed-deadline failure equaliser at the service seams: `_failure_deadline`
+ `_sleep_until` + `AuthService._equalize_failure`, wrapping `login` and
`authenticate_kerberos`. A failed outcome is held to an instant fixed before
dispatch, so its latency is a function of the start time and nothing else.
Successes return unpadded on purpose -- a valid credential has already disclosed
that the account exists, and enumeration is about telling two failures apart.

Sited on the service methods rather than in `routes/sso.py`, so `GET /ui/sso`
and the JSON Kerberos leg inherit it from one site. Driven through the real
route in the tests rather than asserted from the call graph.

An overrun quantizes up to the next whole multiple of the budget instead of
giving up, so the pad cannot fail open under the load that makes it matter; a
latched per-seam warning reports that the budget is wrong for the hardware.
Hard-bounding the directory call at the same budget was declined: it would
refuse legitimate logins against any directory slower than 0.5s, and the
ldap3 timeouts default to 10s each for that reason.

Measured before, in-process on a real SQLite store, warmed and interleaved,
25 samples per branch: the five sign-in failure branches spread 48.97ms
(0.61ms for the retired-AD refusal against 45.7-49.6ms for the local branches).
After: 1.00ms, the residual being asyncio.sleep wake-up jitter uncorrelated
with the branch.

The inner method is `_dispatch_login`, not `_login`, because
test_docs_security_pathways treats every `_login*` coroutine returning a
LoginOutcome as a per-provider pathway owing a SECURITY.md row -- a pure
refactoring must not force that guard to be loosened. Its sibling assertion
AST-reads `mfa_verified=True`, which moved one frame down, so the anchor
follows the fact exactly as that test's own BACKLOG #1137 note did.

Six mutations of the shipped code each red their intended guard in the new
test file, and all six are green across the 121 pre-existing auth tests --
including a deadline steered by the attempted username, which is the tautology
hazard the backlog row warns about. The tests assert invariance across
branches, never equality against a shared symbol.

Research item: no re-score, no banner flip, no vault write. The row's
re-score banner, its Severity line and one named residual are corrected in
the same change from re-measurement at HEAD.
@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 6, 2026
wshallwshall and others added 2 commits September 6, 2026 13:41
Neither side of this conflict was wholly right, so taking either one whole
would have been wrong. Each carried one correct half.

  main  polarity False  correct (#1144)   anchor authenticate_kerberos  stale
  930   polarity True   stale             anchor _authenticate_kerberos correct

The merged code settles it rather than my preference: after the merge the
Kerberos grant reads `mfa_verified=False` inside `_authenticate_kerberos`, so
the assertion follows main's polarity at this branch's anchor.

Main's #1144 retired the owner-signed delegated-directory relaxation that minted
a hard True, on the ground that an application receiving no assertion must
assume the minimum mechanism. This branch was cut before that and still asserted
True. Note that the three-way merge already resolved the SOURCE correctly on its
own; only the test's assertion needed a decision.

This branch's local `_mfa_grant` helper is dropped for main's shared
`mfa_grant_values`, a second definition beside a shared one being the defect the
ledger rules name. It also called `textwrap.dedent`, and main removed that
import when it extracted the helper, so keeping this side would have raised
NameError on top of asserting the wrong polarity.

Two positive controls, because a green here otherwise proves nothing: flipping
the polarity to True fails the test, and moving the anchor back to the public
wrapper fails it too. Restored, 17 passed. Ruff clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant