feat: TOTP enrollment, recovery codes, admin reset (#1005 slice 2) - #1008
Merged
Conversation
The user side of the step-up MFA epic. The factor belongs to the user, enrolled once at /admin/account/mfa; apps opt in via slice 1's policy fields. Still no challenge/grants/guard (slices 3-4). - Migration 0029 (dual-dialect): user_mfa (AES-256-GCM secret via the RUSCKER_MASTER_KEY, pending→confirmed states, last_used_step reserved for slice-3 replay prevention) + user_mfa_recovery (salted SHA-256 hashes — codes are high-entropy random, argon2 deliberately not needed; one-time transactional consumption). - Enrollment flow: password re-auth → QR (inline SVG, no CDN) + manual base32 key → first valid code confirms (SHA-1/6/30s, ±1 step, per authenticator-app compat) → 10 recovery codes shown exactly once. Pending secret lives encrypted in the DB (HA-safe, survives restarts); a wrong code keeps the same secret; a confirmed factor can never be overwritten without reset. Break-glass sessions cannot enroll (no password to re-auth). Missing master key fails closed (503 + hint). Confirm attempts rate-limited (5/min, 429 + Retry-After). Audit: mfa.enroll / mfa.reset, never the material. - Admin: user edit page shows factor state + audited Reset 2FA; paginated list untouched. - Deps (crates.io-verified): totp-rs 5.7.2 (gen_secret+otpauth, no qr feature) + qrcode 0.14.1 (svg only, no image stack). - i18n x4; integration suite (happy path with a code computed from the decrypted secret, wrong password/code, rate limit, 503 without key, reset, break-glass, must-change pinning) + pg lifecycle round-trip. Implemented by a Codex (gpt-5.6, high) agent. Verified: full gate green; postgres-it green vs a real postgres:16; live smoke end-to-end with the confirmation code computed INDEPENDENTLY in python (RFC 6238 by hand) — proving real authenticator interop; secret verified absent-in-cleartext in the DB blob; 5x401→429; reset audited. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two codex-review findings, one mechanism: - P1: while an enrollment was pending, ANY session for the same user — e.g. a stolen cookie that never passed the password re-auth — could POST a wrong code and receive the setup re-render WITH the decrypted secret + QR, then finish enrollment. The pending row now carries a random ceremony token, mirrored in a short-lived HttpOnly SameSite=Strict cookie scoped to the MFA pages and set only by the password-reauthenticated /start. /confirm requires the matching cookie (constant-time compare) BEFORE any decrypt/re-render; without it: generic 409, zero secret material. - P2: the confirm UPDATE matched username+pending only, so a racing re-start could get its REPLACEMENT secret confirmed by a code that proved the old one. The UPDATE is now conditional on the ceremony — a stale confirm affects 0 rows and 409s. Cookie removal sets the issuing Path (the #923 lesson). Tests: the existing suite now threads the ceremony cookie like a real browser, plus two adversarial cases (same-user session without the cookie never sees the secret and cannot confirm even with a correct code; a restarted enrollment invalidates the old ceremony). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iew r2) Codex review: /start ran an unthrottled argon2 verify per guess — a stolen session cookie became an unlimited password oracle (and a CPU sink). A dedicated per-username limiter (same 5/min shape as the code limiter) now gates the re-auth: failures count, success clears, and a tripped window refuses even the correct password with 429 + Retry-After. Localized via the existing rate-limit message. Test: five wrong guesses, then the CORRECT password is refused and no pending row exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lice 2 review r3) Codex review: allow() was a read-only check and the failure was only recorded AFTER the awaited argon2/TOTP verification — N concurrent requests could all pass the check first, unbounding both password guesses and CPU-heavy verifies (TOCTOU). The limiter now exposes try_reserve(): prune + check + count in ONE mutex critical section, called before any await; success still clears the entry so legitimate flows never accumulate. Applied to both the re-auth and the code limiters; record_failure removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
milkway
force-pushed
the
feat/1005-mfa-enroll
branch
from
July 18, 2026 03:50
555f3d3 to
07d3fb0
Compare
milkway
added a commit
that referenced
this pull request
Jul 18, 2026
Two codex-review findings: - P1 (pg MVCC): under READ COMMITTED the conditional INSERT…SELECT can still read the pre-revocation epoch while the revoker's transaction is uncommitted, and the revoker's DELETE snapshot then misses the new grant. Each grant now RECORDS the epoch it was issued under and evaluate() validates it against the live factor row on every read — issuance-time races become harmless because an old-epoch grant is simply never accepted. Test simulates the slipped-through case. - P1 (migration immutability): the epoch column was added by editing 0029 — but 0029 belongs to slice 2 (PR #1008) and may be applied before this slice lands; editing it would trip sqlx's applied- migration checksum. 0029 restored byte-for-byte (from the slice-2 tip — first restore came from the wrong commit and dropped the ceremony column; caught by the unit suite); security_epoch moved to 0030 as ALTER TABLE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
milkway
added a commit
that referenced
this pull request
Jul 18, 2026
* feat: MFA challenge + trusted-device grants (#1005 slice 3) The proof machinery the slice-4 proxy guard will consume. No proxy changes yet. - Migration 0030 (dual-dialect): user_mfa_grants — opaque 32-byte token stored only as salt:SHA-256, bound to user + mfa_verified_at + expiry (30-day cap) + the factor it proved (confirmed_at binding: a reset/re-enrollment invalidates old grants) + a session-id hash for the 0-day semantics. - Challenge at /admin/account/mfa/challenge: TOTP or recovery code → device grant + cookie (__ruscker_mfa_device={id}.{token}, HttpOnly, SameSite=Strict, base-path root so slice 4 sees it on /app/*). Audit mfa.verify / mfa.recovery_used. Atomic try_reserve limiter. - Decision API mfa::evaluate → Satisfied | ChallengeRequired | EnrollmentRequired: policy → enrollment → cookie → grant → constant- time token compare → expiry → factor binding → 0-day session binding or proof-age vs the app's window. One proof serves all apps, each comparing the age against its own effective_mfa_validity_days(). - TOTP replay prevention: the accepted step (current/±1, derived explicitly) is recorded via a conditional last_used_step update — covers challenge AND enrollment confirm. - Revocations: password set/change and MFA reset delete grants inside their existing transactions; user deletion cascades; explicit "forget this device" / "sign out all devices" buttons audit mfa.trusted_device.revoke. Normal logout keeps the device. Implemented by a Codex (gpt-5.6, high) agent. Verified: full gate green; postgres-it green vs real postgres:16 (grants round-trip); live smoke — challenge with a python-computed step+1 code (the enrollment consumed the current step; replay guard forces strictly-greater), grant row hash-only, same-code replay 401, password change revoked the grant, audit trail correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: strip MFA cookies from proxied requests + honest revoke-all label (#1005 slice 3 review) Two codex-review findings: - P1: the trusted-device grant cookie is root-scoped (slice 4 needs it on /app/*), so browsers send it on every proxied request — and the #258 cookie filter didn't know it: the MFA BEARER would have reached app containers, letting a compromised app capture it and bypass MFA once it also had the password. is_ruscker_cookie now strips the whole __ruscker_mfa_ prefix (grant + ceremony) on both the HTTP path and the WS handshake. Proxy test: app cookies pass, MFA cookies never reach the upstream. - P2: "Sign out of all devices" only revokes trusted-device MFA grants, not login sessions — relabeled ×4 to "Forget all devices" with a confirm text that says exactly that (sessions stay active). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: security epoch closes the revocation/challenge race (#1005 slice 3 review r2) Codex review: a revocation (password set/change, forget-all) that committed while a challenge was in flight deleted the grants — and the challenge's independent INSERT then created a fresh grant AFTER, surviving the revocation. user_mfa now carries a security_epoch bumped inside every revocation transaction; grant issuance is an INSERT…SELECT conditional on the epoch read BEFORE the TOTP verification, so the revocation always wins (the in-flight insert hits 0 rows and the route asks the user to challenge again). MFA reset deletes the factor row, which refuses stale issuance the same way. Contract test: stale-epoch create is refused after a racing password change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: per-grant epoch + immutable 0029 (#1005 slice 3 review r3) Two codex-review findings: - P1 (pg MVCC): under READ COMMITTED the conditional INSERT…SELECT can still read the pre-revocation epoch while the revoker's transaction is uncommitted, and the revoker's DELETE snapshot then misses the new grant. Each grant now RECORDS the epoch it was issued under and evaluate() validates it against the live factor row on every read — issuance-time races become harmless because an old-epoch grant is simply never accepted. Test simulates the slipped-through case. - P1 (migration immutability): the epoch column was added by editing 0029 — but 0029 belongs to slice 2 (PR #1008) and may be applied before this slice lands; editing it would trip sqlx's applied- migration checksum. 0029 restored byte-for-byte (from the slice-2 tip — first restore came from the wrong commit and dropped the ceremony column; caught by the unit suite); security_epoch moved to 0030 as ALTER TABLE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: transactional grant issuance — recovery rollback + cookie rotation (#1005 slice 3 review r4) Two codex-review findings, one mechanism (db::mfa_grants::issue, one transaction per dialect: consume recovery → retire previous grant → epoch-conditional insert → audit): - P2: the recovery code was consumed BEFORE grant creation — a stale- epoch refusal (or any later failure) burned a finite code with nothing issued. The spend now lives inside the issuance transaction and rolls back with it; the route matches the code first (find_recovery_candidate, no consume) and passes the id in. Test: stale-epoch refusal leaves the code usable. - P2: every successful re-challenge left the browser's PREVIOUS grant alive while overwriting its cookie — a copied old cookie stayed a valid bearer until hard expiry. The issuance transaction now retires the cookie's old grant (scoped to the same username). Test: second challenge rotates the cookie and the grant count stays at one. The separate post-issuance audit call is gone — the audit row commits with the grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: read-order in evaluate + TOTP step inside the issue tx (#1005 slice 3 review r5) - P1: evaluate() read the factor BEFORE the grant — a stale-epoch grant committing between the two reads could match the cached (pre- revocation) epoch and return Satisfied. The authoritative factor read now happens AFTER the grant fetch, so its epoch is at least as fresh as the grant's; the no-cookie path fetches the factor directly (one SELECT either way — this is slice 4's hot path). - P2: record_used_step committed before issue() — a StaleEpoch refusal burned the current 30s code, locking the user out until the next step. The step consumption is now a conditional UPDATE inside the issuance transaction (new Replayed refusal), rolling back with everything else. Enrollment-confirm keeps its standalone step guard (no issuance to bundle with). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: recovery-code challenges work without the master key (#1005 slice 3 review r6) Codex review: the blanket master-key check at the top of the challenge handler also rejected kind=recovery with 503 — but recovery codes only compare salted hashes; they never decrypt the TOTP secret. A node restarted without RUSCKER_MASTER_KEY would lock out users holding valid recovery codes: exactly the emergency path that must survive misconfiguration. The check now lives inside the TOTP branch alone (which does decrypt). Test: keyless state — TOTP 503s with the hint, a valid recovery code still issues a grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: consistent lock order + exclusive grant rotation (#1005 slice 3 review r7) Two codex-review concurrency findings on Postgres: - P2 deadlock: a re-challenge locks user_mfa (the TOTP-step UPDATE) then the grant row (rotation DELETE), while every revocation locked grants then user_mfa (epoch bump) — opposite order, so an overlap could deadlock and abort the SECURITY action with a 500. All revocation paths (revoke_all + the silent delete_all helpers used by password change / MFA reset) now bump the epoch BEFORE deleting grants, matching issue()'s user_mfa-then-grants order. - P2 double grant: two challenges from the same browser carry the same previous grant id; the first retired it, the second's DELETE hit 0 rows but still inserted — leaving TWO live grants for one cookie (overwritten bearer valid 30 days). The rotation DELETE is now required to affect exactly one row; a 0-row retire returns the new Superseded refusal (route → 409 "verified in another tab"). Test: concurrent rotation of the same grant leaves exactly one live grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: one grant per browser-session via UPSERT (#1005 slice 3 review r8) The r7 exclusive-rotation fix introduced a P1 lockout: after a revocation from another browser, THIS browser keeps its device cookie but its grant is gone, so the next valid challenge passed the stale id to issue(), got Superseded, and — since the cookie was never cleared — every retry looped, locking the user out. Root cause: keying rotation off the cookie's grant id. Redesign, keyed off the session instead: - Migration 0030 gains UNIQUE(username, session_binding). - issue() UPSERTs the single row for the browser-session (ON CONFLICT DO UPDATE id/token/factor/epoch/verified/expires). A re-challenge replaces its row (old cookie dies), a stale cookie after revocation simply issues fresh (no lockout), and two concurrent challenges from one browser converge to ONE row (no orphan). replace_grant_id and the Superseded refusal are gone. - Lock order is now uniformly user_mfa -> grants -> recovery across issue()/revoke_all/delete_all/reset, closing the r8 P2 deadlock between a recovery challenge and an MFA reset on Postgres (recovery consumption moved AFTER the grant UPSERT). - Tests: the concurrent-rotation test becomes "one grant per session + stale-cookie-after-revocation recovers (no lockout)"; eval/revoke tests use distinct bindings to model separate browsers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat: enrollment establishes the initial device proof (#1005 slice 3, from slice-4 review) The proxy guard (slice 4) redirects an unenrolled user to enroll; but enrollment confirmation only recorded the TOTP step — it minted no grant. The recovery page's "Continue to app" link then hit the guard, got ChallengeRequired, and the challenge rejected the just-entered code as a replay, forcing the user to wait a TOTP interval (or spend a recovery code). Now confirm() issues the browser's initial device grant (the code IS a valid proof) and sets the device cookie, so enrollment flows straight into the app. Belongs in slice 3 (issue() lives here); the fix is what slice 4's guard makes necessary. - Audited mfa.verify (the device is verified, beside the mfa.enroll already written). Non-fatal on failure — recovery codes still render. - The step was already consumed by enrollment, so issuance passes no step/recovery to consume. - Tests: enrollment asserts the initial grant + cookie; the enroll-then-manual-grant tests use distinct bindings / updated counts to account for the initial grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: lock the factor row on every issuance + sweep expired grants (#1005 slice 3 review r9) Two codex-review findings: - P2 (pg recovery-path epoch race): the recovery-code path has no TOTP-step UPDATE, so under READ COMMITTED the epoch-conditional INSERT read a snapshot a racing revocation could pass — inserting an old-epoch (thus unusable) grant while committing the one-time recovery-code spend. issue() now reads+validates the factor row FIRST on EVERY path (Postgres: SELECT … FOR UPDATE; SQLite: a serialised read), turning a raced revocation into a clean StaleEpoch rollback that leaves the recovery code unspent. - P2 (grant retention): session-only MFA mints a row per login and expired rows were only filtered at read time. issue() now sweeps the user's expired grants (DELETE … WHERE expires_at < now) so the table can't grow unboundedly. Test covers the sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: write-lock the epoch check + parent-before-child lock order (#1005 slice 3 review r10) Two database-specific concurrency edges, both turning a rare same-user race into a transient 500/deadlock (never a security or integrity bug): - P2 SQLite WAL: pool.begin() is deferred, so reading the epoch took a read snapshot; a revocation committing before our later write would raise SQLITE_BUSY_SNAPSHOT (a 500) instead of a clean recheck. The epoch check is now a no-op conditional UPDATE that takes the WRITE lock up front — rows_affected==1 means the epoch held under the lock, 0 means a raced revocation → StaleEpoch. - P2 Postgres deadlock vs user deletion: issuance locked user_mfa first, but the grant INSERT's FK check needs a key-share lock on the parent users row, while user deletion locks users then cascades. The pg path now locks the parent `users` row (FOR KEY SHARE) BEFORE user_mfa, so both follow parent→child order. This completes the lock-ordering hardening (user→user_mfa→grants→ recovery, parent before child) across issue()/revoke/reset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
14 tasks
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.
Slice 2 of 4 of the step-up MFA epic (#1005). Stacked on #1007 (slice 1) — review/merge that first. The USER side: enrolling a TOTP factor, recovery codes, encrypted storage, admin visibility/reset. Still no challenge, trusted-device grants, or proxy guard (slices 3–4).
What changed
user_mfa(AES-256-GCM secret viaRUSCKER_MASTER_KEY; pending→confirmed; a per-enrollment ceremony token;last_used_stepreserved for slice-3 replay prevention) +user_mfa_recovery(salted SHA-256 hashes — codes are high-entropy random, argon2 deliberately not used; transactional one-time consumption)./admin/account/mfa(the account-password page pattern): password re-auth → QR (inline SVG, no CDN) + manual base32 key → first valid code confirms (SHA-1/6-digit/30s, ±1 step — authenticator-app compat) → 10 recovery codes shown exactly once. Pending secret lives encrypted in the DB (HA-safe, survives restarts); wrong code keeps the same secret; a confirmed factor is never overwritten without reset; break-glass sessions can't enroll; missing master key fails closed (503 + hint).totp-rs 5.7.2(gen_secret+otpauth, noqrfeature) andqrcode 0.14.1(svg only).mfa.enroll/mfa.reset— never the material. i18n ×4.Review hardening — 4 real findings by Codex (
gpt-5.6high), 4 rounds to a clean passHttpOnly/SameSite=Strictcookie set only by the password-re-authed/start;/confirmrequires it (constant-time compare) before any decrypt/re-render — otherwise a generic 409 with zero secret material.UPDATEmatched username+pending only, so a racing re-start could get its replacement secret confirmed by a code proving the old one. TheUPDATEis now conditional on the ceremony; a stale confirm hits 0 rows and 409s./startran an unthrottled argon2 verify per guess. A dedicated per-username limiter (5/min) now gates the re-auth.allow()was read-only and failures recorded only after the awaited verify — N concurrent requests could all pass first. Both limiters nowtry_reserve(): prune+check+count in one mutex critical section, before any await.Validation
postgres:16(incl. the new pg begin/confirm/reset round-trip with the ceremony column).mfa.enroll/mfa.resetrows).Implemented by a Codex (
gpt-5.6, high) agent; verified and hardened as above. Slice 3 (challenge + trusted-device grants) follows.🤖 Generated with Claude Code