auth: retire the bootstrap account (#1020) and guard MFA last-factor removal (#1022) - #316
Closed
wshallwshall wants to merge 4 commits into
Closed
auth: retire the bootstrap account (#1020) and guard MFA last-factor removal (#1022)#316wshallwshall wants to merge 4 commits into
wshallwshall wants to merge 4 commits into
Conversation
…key-delete (BACKLOG #1022)
delete_webauthn_credential refused to remove the last enrolled second factor while MFA was
required; disable_mfa consulted neither has_webauthn_credentials nor _mfa_required_for. A user
holding TOTP plus one passkey could therefore delete the passkey (permitted while TOTP remained)
and then disable TOTP unguarded, reaching zero enrolled factors -- the state ADR 0068 AC-10 says
the system shall refuse. Only the ORDER of the two removals decided whether it was allowed.
disable_mfa now carries the same guard, keyed the same way (has_webauthn_credentials, then
_mfa_required_for(..., second_factor_enrolled=False) on identity.roles). It is deliberately not a
TOTP-only check -- a user who keeps a passkey stays free to drop TOTP -- and is gated on
totp_enabled so a disable that removes nothing stays idempotent rather than refusing a no-op.
The ValueError is mapped at both call sites, which would otherwise have 500'd:
DELETE /me/mfa returns 400 with the refusal text, and the console's /ui/account/mfa/disable
renders it on the account page exactly as ui_webauthn_delete renders the passkey twin.
ADR 0068 AC-10 is widened from the passkey ROUTE to the zero-factor STATE, since as written it
forbade one path to that state rather than the state itself; the ADR's parity-follow-up note and
the two docs/SECURITY.md claims (the DELETE /me/mfa route-table row, which recorded the absence,
and the TOTP paragraph) move with it.
Not a bypass and not an MFA-enforcement change: login already issues mfa_verified=not
mfa_required and require() applies the second factor as an ASVS 6.3.3 access gate, so the
pre-guard outcome was a forced re-enrollment, not single-factor access. Nothing is deployed, so
there is no migration cost here.
Red-first, all three directions measured on the pre-fix code: the passkey-then-TOTP ordering
reached zero factors (DID NOT RAISE); dropping the route try/except gives "unhandled error on
DELETE /me/mfa: ValueError"; dropping the console except-arm loses the HTML error page.
test_disable_and_admin_reset_clear_mfa now pins require_mfa=False -- the documented opt-out --
because under the secure default the new guard correctly refuses to strip the account's only
factor. The new guard tests are deliberately extra-free (they stage a passkey through the
AuthStore surface, the tests/_webauthn_store_contract.py precedent): an importorskip("webauthn")
on this path would skip the guard on every leg that installs without the optional extra.
…dd `messagefoundry admin-create` (BACKLOG #1020) #1020 asked for the first-run bootstrap Administrator to carry a deliverable email so the PHI notification gate could see it. ASVS 6.3.2 wants that account NOT TO EXIST, and both could not land. Under the owner ruling of 2026-08-10 the stricter end state wins: the account is retired, so #1020 closes as SUPERSEDED rather than as built -- an account that does not exist needs no mailbox. THE REMOVAL WAS GATED ON A REPLACEMENT PATH, AND NONE SHIPPED. Searched by construct, not assumed: no `users`/`admin` CLI subcommand exists in `_DISPATCH`; `create_local_user` is reachable only through `POST /users` / the console under `users:manage`, which needs an account already; and the directory paths cannot bootstrap either, because `roles_for_ad_groups` reads an AD group map that lives ONLY in the store and is written by `set_ad_group_map` -- itself an authenticated admin action. A fresh store had exactly one route to a first administrator, and it was the implicit one. So the smallest explicit route is built in the same change. `messagefoundry admin-create --username <name>` opens the engine's own resolved store and creates a local Administrator: password from a no-echo confirmed prompt or `--password-stdin`, NEVER an argv flag (argv is readable by other accounts on the box); held to the deployment's own `[auth]` policy via `password_violations`, so the CLI is not a laxer second path into the same account store; audited `user.created`; `must_change_password` clear, because the operator standing at the box chose their own password and there is no second party a forced rotation would protect. It prints the store it actually wrote, so a `--db` / `MEFOR_STORE_*` typo surfaces now rather than as a login failure later. Removed with the account, because each existed only to manage its lifetime: `bootstrap-admin.txt` and `_emit_bootstrap_admin` (with it, the cleartext-credential-at-rest risk ADR 0034 accepted -- that row is now marked withdrawn rather than deleted, so the register can still be reconciled against a re-scan); `_ensure_bootstrap_admin`, `_retire_superseded_bootstrap`, `bootstrap_expiry_warning` and the `BOOTSTRAP_USERNAME` login carve-outs; `[auth]` bootstrap_expiry_hours / bootstrap_warn_hours; and the `bootstrap_admin_expiring` alert event (rule-targetable event types 18 -> 17). `initialize()` now seeds roles and returns None. The ASVS 6.4.1 initial-password gate loses its bootstrap exemption, which is a strengthening: the carve-out is no longer a username special case, and every unclaimed admin-issued temp is held to `initial_password_expiry_hours`. The first administrator is exempt for the right reason -- its `must_change_password` is clear because the operator set the password. The IDE moves with it. `statusBar.ts`'s store-less fork confirm, the setup-page copy and the two tests pinning them said "creates a NEW database and a bootstrap admin"; that is now false, and a shipped modal promising an account the engine will not create is worse than the fork it guards. They now say "a NEW empty database with no accounts". ADRs 0110 and 0112 carry dated amendments rather than edits -- their decisions stand, two factual premises under them did not. Nothing is deployed, so there is no migration to stage and no compatibility shim to write; a deploying site would simply run one command it does not run today. Red-first, both directions: tests/test_admin_create_cli.py (10) drives a FRESH store to an authenticated session that reaches the `users:manage`-gated GET /users, and separately asserts that two `initialize()` calls leave zero users and no `admin` account -- the half that is easy to lose, since a reintroduced bootstrap would leave every other test in the suite green. Its env is pinned: measured 2026-08-10, an ambient MEFOR_STORE_PATH plus MEFOR_AUTH_PASSWORD_MIN_LENGTH failed 7 of those 10, and test_the_env_pin_is_load_bearing_and_the_hazard_is_real proves both that the pin clears the prefix and that the redirect it pins against is real. tests/test_bootstrap_admin_perms.py is deleted: it asserted the 0600/O_EXCL/symlink-refusal properties of a credential file that is no longer written anywhere. `_CONTEXT_TABLE_A_ROWS` moves 35 -> 34 in the same commit as the SECURITY.md row it counts -- that gate exists to catch an undeclared row deletion, and it caught this one.
…rst run can still write (BACKLOG #1020) Both live-backend fixtures reproduce "a fresh, engine-started, NEVER-restored database" by writing two audit rows directly, and one of them was `auth.bootstrap_admin_created` -- an action the engine no longer emits now that the implicit first-run Administrator is retired. The gate under test keys on "audit_log non-empty AND no dr_backup row", so the assertions were unaffected either way; what was wrong is the fixture's own claim to be "the exact real-path state", which it no longer was. They now write `user.created` (what `messagefoundry admin-create` audits) plus `auth.login_success`, which is the signature a first run actually leaves. These two legs SKIP locally -- there is no live SQL Server or Postgres here -- so this is verified by construct and by ruff/mypy only; the Windows CI `store-sqlserver` and `store-postgres` legs are what execute them.
The store-locator rationale was written twice when the print was made backend-aware. SDS-3.5: state a load-bearing fact once. No behaviour change.
Collaborator
Author
|
Closing this PR, not abandoning the work. The branch Why it cannot go green. It cites Why it should not merge yet. Adversarial verification found Where the record now lives. #321 has landed and amends the item(s) in the ledger -- partial, not closed, with the residuals named. The ledger tells the truth about this work whether or not the code lands, which is what makes closing this PR safe. To resume: fix the finding on |
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
1 participant
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.
BACKLOG #1020 #1022
Handed over by the authoring session at a usage cutoff. Reported COMPLETE and verified there: full
suite 0 failed, with the node-id diff accounting for every one of 24 removals and 19 additions.
NOT INDEPENDENTLY VERIFIED BY THE COORDINATOR -- pushed and routed to preserve the work off a local
ref, not merged. Whoever picks this up should run the standard battery before landing it: merge-tree
against a known-conflicting control, ledger figures re-derived from this branch, and a leak scan with
the detector set confirmed non-empty.
Leak-scanned before pushing: zero private paths, zero bare slugs. One customer/IP pattern hit was
triaged and cleared --
client="10.1.1.1"in tests/test_admin_new_ip.py, a synthetic fixture IP.Deliberately NOT armed for auto-merge.