Skip to content

sec(auth): let a directory account hold an engine second factor, and mint Kerberos at the minimum (BACKLOG #1144) - #842

Open
wshallwshall wants to merge 5 commits into
mainfrom
worktree-agent-aaf8481e1400f92e7
Open

sec(auth): let a directory account hold an engine second factor, and mint Kerberos at the minimum (BACKLOG #1144)#842
wshallwshall wants to merge 5 commits into
mainfrom
worktree-agent-aaf8481e1400f92e7

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

BACKLOG #1144, steps 1 and 2 only, co-landed. Steps 3 through 6 stay open and the item stays open.

Why co-landing is not optional

Shipping step 2 alone is the measured confinement. A minimum-minted directory session is refused on
every route outside the six-entry exempt set at messagefoundry/api/security.py, and before this
change neither enrollment ceremony accepted a non-local account. So step 2 without step 1 locks
directory users out. With zero deployments the upgrade-lockout cost is vacuous (CLAUDE.md section 0),
which is exactly why the simple correct end state was available: there is no migration path or
compatibility shim here, and none was built.

Targets were located by SYMBOL, because every line anchor in the row had drifted

At 46ea10a78, :2060-2061, :2112, :2130, :2378, :2412, :884 and :911-914 all named
unrelated code. What was actually found, and what happened to it:

Named in the row Found at HEAD as Action
blanket strength exemption AuthService._mfa_required_for, auth_provider == AD -> False deleted
non-local refusal 1 begin_mfa_enrollment deleted
non-local refusal 2 confirm_mfa_enrollment deleted
non-local refusal 3 begin_webauthn_registration deleted
non-local refusal 4 finish_webauthn_registration deleted
directory mint leg 1 LDAP simple-bind login ALREADY GONE, retired by #1137 in 97d37bc9a
directory mint leg 2 authenticate_kerberos now passes mfa_verified=False

The #1245 exemption removal recorded in auth/service.py is a DIFFERENT thing and was not touched:
it is the bootstrap carve-out on the ASVS 6.4.1 temp-password expiry inside _login_local. The
strength exemption this item names was still present in _mfa_required_for.

Four sites the row's enumeration did not name, included and why

The row says "the four non-local refusals". A completeness claim written 2026-08-20 is a liability
(SDS-3.6), and each of these makes step 1 real rather than nominal:

  1. admin_reset_mfa refused a non-local account. Without it, enrollment is a ONE-WAY DOOR: a
    directory user who loses an authenticator has no recovery, because every route that could help
    stands behind the factor they lost.
  2. POST /me/mfa/enroll refused an externally-managed account at the API layer. It is the only way a
    directory account reaches the ceremony over HTTP.
  3. The console account page hid the MFA and passkey cards from a directory account. That would have
    stranded exactly the users the mint confines.
  4. The console admin user page hid the Reset MFA button for a directory account, making (1)
    unreachable from the operator UI. Three independent review agents found this one.

mfa_satisfied's directory branch is now a FLOOR rather than an exit: it refuses while require_mfa
is on, and otherwise falls through to the shared per-user rule, so an enrolled directory account must
satisfy the factor it enrolled. It decides exactly one case the shared rule would decide differently
(require_mfa_scope="administrators" plus a non-Administrator with no factor), and that case is now
named in the comment and pinned by a test.

One defect found by the review pass, not by a test

POST /auth/negotiate reported mfa_required=False while minting a session the gate would refuse.
Correct while a directory session was minted MFA-satisfied; a lie afterwards. The directory tail now
asks mfa_satisfied rather than negating the grant, because with require_mfa off an un-enrolled
directory session grants nothing yet owes nothing.

What is deliberately NOT here

  • No minimum-strength fallback statement. That is step 6 and would be false today. The prose
    changes describe mechanism only; none claims this requirement's fallback clause is satisfied.
  • oidc_require_mfa_claim's default is unchanged — the residual names it as a worse-direction
    re-score trigger.
  • No argument that the conditional verb is vacuously satisfied, and no not-applicable claim. Both
    are foreclosed by name, the second because the precondition is reachable by configuration.
  • Steps 3 through 6, unchanged and untouched.

Read this before re-scoring the cell

A LARGER BLOCKER exists than the item names. Every implementing control lives inside an
identity-provider leg that ships off and cannot ship on without operator configuration
(config/settings.py refuses ad_enabled = true without a server and search base, a secure scheme, a
bind account and a bind password, all defaulting unset; Kerberos requires AD; federated requires AD
plus four more). Whether the 2026-08-16 "an off-by-default control can never be graded pass" ruling
reaches a CONDITIONAL requirement whose ANTECEDENT is unreachable on a stock install is STILL
UNTAKEN. Under the strict reading this build moves the cell not at all. No re-score may assume an
answer. The scorecard lives in the vault and was not touched.

The residual is STRENGTHENED, not corrected. The withdrawn amendment stays withdrawn: the step-up
freshness stamp is seeded at login (_complete_ad_login defaults seed_reauth=True), so per-function
re-checks compare a timestamp the engine stamps itself for the whole step_up_max_age_seconds window.
That is step 5 and was not touched. The SSO leg still splits by caller.

WHAT NO BUILD REACHES: strength and method on the simple-bind and SSO legs, whose protocols return no
assertion. Any future record claiming they VERIFY strength is wrong on the protocol.

Known cost, measured and recorded rather than hidden

A directory session leaves the one-round-trip early return in mfa_satisfied for its whole life.
With require_mfa ON that is two round-trips, unchanged from before. With require_mfa OFF an
un-enrolled directory session pays three to four per authorized request instead of one, permanently,
because it is never stamped. The cheap fix -- minting on the require_mfa dial -- was declined: it
would stamp a factor-satisfied timestamp on policy rather than on what happened, and would put the
setting in two places. A separate, pre-existing dead round-trip also sits in that path: the role read
is unconditional while _mfa_required_for short-circuits before looking at it on the shipped scope.
Fixing it means duplicating the helper's condition at the call site, so it is named here as unfiled
work on the subject rather than smuggled in.

Checks

Run: ruff format --check, ruff check (All checks passed), mypy messagefoundry (Success: no
issues found in 267 source files), and /simplify (four review agents; findings applied or declined
with reasons above).

pytest, quoting its own summary lines:

  • 633 passed, 2 warnings in 118.53s -- the whole auth, MFA, WebAuthn, OIDC, AD, step-up, CLI, docs,
    settings and security-posture surface
  • 42 passed, 364 deselected in 86.52s -- web console account, MFA, passkey, SSO
  • 28 passed, 378 deselected in 39.58s -- web console admin and user detail
  • 328 passed, 2 warnings in 17.83s -- doc guards and claim checks
  • 57 passed in 7.63s -- security posture, posture defaults, gate parity (these pin the
    security_loosenings text this change rewords)

DESELECTED, and it is not mine: tests/test_ci_retry_native_crash.py fails on this box because the
test shells to C:\windows\system32\bash.EXE, which cannot resolve the Windows path to
scripts/ci/retry-native-crash.sh. This diff touches zero files under scripts/.

NOT RUN, and there is NO result to infer: the full tests/ sweep. Three attempts were made. Two were
invalidated because commits landed while they were running, and the third was killed by the harness
before it printed a summary, so it produced no line at all -- not a pass, not a fail, no data. Treat
the full sweep as outstanding. The targeted runs above cover every file this diff touches; what the
sweep would add is the areas the diff does not reach (pipeline, transports, store, parsing).

LEGS ONLY A HOSTED RUNNER SEES must be read after this lands, in particular the web console suite in
full (only subsets were run locally) and windows-service-smoke.

🤖 Generated with Claude Code

wshallwshall and others added 5 commits September 3, 2026 18:58
…LOG #1144)

Two halves that only work together, so they land together. The Kerberos leg
passed mfa_verified=True under the delegated-directory relaxation, granting
satisfaction on an assertion the engine never receives; it now passes False.
That alone confines every directory session to six exempt routes, so the same
commit removes the non-local refusals from both enrollment ceremonies, from
admin_reset_mfa, from POST /me/mfa/enroll and from the console account page,
and drops the blanket provider exemption in _mfa_required_for.

mfa_satisfied keeps a directory floor rather than an exit: it refuses while
require_mfa is on, and otherwise falls through to the shared per-user rule, so
an enrolled directory account must satisfy the factor it enrolled.

Every prose claim the change falsified is corrected here rather than left to
drift: SECURITY.md, CONFIGURATION.md, SECURITY-LOOSENING.md, the settings and
totp docstrings, and the serve exposure-gate message.

Steps 3 through 6 of the item stay open. No minimum-strength fallback statement
is written; doing so today would be false.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rns on

The item stays OPEN: steps 3 through 6 are untouched. The row now says which
symbols moved, because every line anchor it carried had drifted and one of the
two directory legs it names was retired by #1137 two days after the research
was written.

Three things a later re-score must not lose. The residual is STRENGTHENED, not
corrected: the step-up window is seeded at login, so per-function re-checks
compare a timestamp the engine stamps itself, and the withdrawn amendment stays
withdrawn. The deciding question is still untaken, so no re-score may assume an
answer to it. And the two cheap moves the 2026-08-16 ruling created stay
foreclosed by name.

#1137's row gets a dated note, not a disposition: the constraint it states as
fixed no longer holds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d (BACKLOG #1144)

Minting the Kerberos leg at the minimum left a JSON client misinformed. The
directory tail hardcoded mfa_required=False and POST /auth/negotiate defaulted
it, both correct while a directory session was minted MFA-satisfied. After the
mint moved, a client was told no second factor was needed and its very next
call was refused with X-MFA-Required and no earlier signal.

The tail now asks the gate rather than the grant. `not mfa_verified` would
over-report: with require_mfa off and no factor enrolled the shared rule still
admits the session, so the caller would be prompted for a factor it does not
owe. One extra read on a rare path keeps one source for the answer.

Found by the review pass over the parent commit, not by a failing test -- no
test asserted mfa_required on the directory path. One now does, in all three
states.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four statements the earlier commits missed, found by re-reading with a positive
control rather than by trusting the first grep. Two section banners in
auth/service.py still scoped the TOTP and passkey surfaces to local accounts;
the harness login comment still promised mfa_required is always False for a
directory login, which the outcome fix had just made false; and SECURITY.md's
MFA section still said every LOCAL account must satisfy the gate.

None of these changes behaviour. They are the statements a reader would act on,
and leaving them is how a corrected control acquires a false premise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…till made it

The sweep that found these ran with a positive control, which is why it found
them and the first pass did not. Each told an operator something now false:
that an all-AD deployment need not enable require_mfa; that AD users stay
directory-delegated; that making a service account a directory principal is a
way out of the MFA scope; that directory delegation is a compensating control
for single-factor admin at exposure; and that 6.3.3 is satisfied only for local
accounts.

Prose only. The two Secure_Development_Standards entries are corrected in place
rather than annotated, because they describe current engine state rather than
recording a past decision -- unlike the ADRs, which date their claims and are
left alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@wshallwshall wshallwshall added the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Reply to the Lander's ask, on the PR rather than by mail so no path from a message is involved.

1. Open PR numbers

  • 842 (this one). That is the only one.

2. Finished but not pushed

Nothing. Everything this seat produced is pushed.

For the record, the other three landed while this seat was holding, so they need nothing from you:

PR State Item
801 MERGED #332, sigstore held at 4.4.0, ignore entry, lock now audited
810 MERGED #1134, seven stale corpus-size claims retired
820 MERGED #1199, off-box audit tee gets a sink

3. Ready to land

842, with three things named rather than hidden.

  • Reads BEHIND. Per your own rules that is not a blocker and I have not run update-branch.
  • The ledger row is already isolated: commit a7e355265, docs/BACKLOG.md only, 19 insertions. It sits second of five rather than last. I did not reorder, because rewriting a builder's five-commit history unsupervised looked like the worse risk. If you want it last, say so and it can be redone deliberately.
  • messagefoundry/auth/service.py is contended: peers hold #1140 and #1148 and this diff edits it heavily. Worth sequencing.

Verification, and its one gap. 1088 passing tests across five targeted runs quoting pytest's own summary lines, covering every file the diff touches. Plus ruff check, ruff format --check, mypy strict on 267 files, and every pre-commit gate at all five commits. The full tests/ sweep produced no result -- three attempts, two invalidated by commits landing mid-run and the third killed. Recorded as no data rather than letting silence read as a pass. Unread locally: the web console suite in full, and windows-service-smoke.

Two defects that merged into main, so they are follow-ups now, not amendments.

  • PR 810 landed a #1134 row calling that row's premise "false". It is stale, not false: the corpus is two concatenated runs, the row describes the first 10,000 only, and 18 + 5,256 = 5,274 reconciles both numbers. One word.
  • PR 820 landed an ensure_logger_sink docstring saying "only two call sites install a root handler". There is a third, messagefoundry/tray/__main__.py, which installs a RotatingFileHandler with no PHI filter. The grep that produced "two" could not match root.addHandler. Should read "at least two" per CLAUDE.md section 11.

Noted on your side and not re-diagnosed here: test_worktree_gate_control_plane.py and the web console 405-passed-zero-failures red.

@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Disclosure, per the Manager playbook section 6.

I applied the reviewed label to this pull request myself, and I am the seat that commissioned the diff. That is not an independent read of the scope, and a later auditor cannot tell the difference unless it is written down. So: this label records my read of work I asked for, not a second party's.

The playbook permits a Manager to label its own worker's pull request; it does not permit doing so silently. I labelled at 16:20 Central and should have said this then. Recording it late rather than not at all.

What the label is actually standing on. The builder ran five targeted suites totalling 1088 passing tests, quoting pytest's own summary lines, covering every file this diff touches. Plus ruff check, ruff format --check, mypy strict across 267 files, /simplify, and every pre-commit gate at all five commits.

The gap, stated rather than hidden: the full tests/ sweep produced no result. Three attempts -- two invalidated because commits landed mid-run, the third killed. It is recorded as no data, not as a pass. It would have covered pipeline, transports, store and parsing, which this diff does not reach.

One red, for whoever reads this next: sql server (store + connector) 2022 is FAILURE. I have not diagnosed it and am not claiming it is unrelated. CI gate and gate liveness (advisory) are still queued. Overall: 32 success, 10 skipped, 2 queued, 1 failure.

Unchanged from my earlier comment: the ledger row is already isolated in commit a7e355265, touching docs/BACKLOG.md only, though it sits second of five rather than last. messagefoundry/auth/service.py is contended with peers holding #1140 and #1148.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewed A reviewer has read this. Removed automatically when new commits arrive.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant