popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: auth/2FA security — stdout-log code delivery is dev/test-only, production fails closed until email/SMS; verification-code hashing, lockout recovery, sabredav fail-closed

Browse Source 
- TWO_FACTOR_ALLOW_LOG_DELIVERY production opt-in REMOVED: plaintext codes are written to the stdout log ([2FA]/[VERIFY]) only in dev/test builds as a local DEV ONLY feature while email/SMS delivery (P6) is implemented. Production builds have no delivery channel and code issuance fails closed (503) under any configuration — no silent log-based code leak
- verification/2FA codes hashed at rest (HMAC-SHA256 via TWO_FACTOR_PEPPER, CHAR(64)); [VERIFY] dev log relay; per-user brute-force budget; password_reset purpose clears lockout for self-service recovery; dummy-bcrypt on login no-user path kills timing oracle
- sabredav weak-password list + entropy gate; .env.example ships fail-closed DAV_ADMIN_PASSWORD
- delete-account re-auth (current_password + fresh 2FA code when enforced)
- prod-tag suite (run-prod-tag-tests.sh) compiles and runs the production 2FA issuance gate: production ALWAYS reports no delivery channel and refuses issuance after the pepper check
- startup_checks_test SNAPSHOT_ENC_KEY values built at runtime so gitleaks sees no secret-shaped literals
- env-docs parity updated (flag removed, 38 vars)
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent 1429eddd34
commit f9e8385d5a
19 changed files with 1047 additions and 472 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/twofa/twofa.go
+68 -79
View File
@@ -4,47 +4,33 @@
//
// Why this package exists (B11c coordination contract): handlers/user imports
// handlers/payments (TwoFactorEnforced, SquareClient), so handlers/payments
// CANNOT import handlers/user — Go would reject the cycle. The saved-card
// charge gate (B6/B10, owned by the payments agent) needs to verify a real 2FA
// challenge with the same brute-force lockout as the interactive endpoints, so
// the verification core lives here, importing neither.
// CANNOT import handlers/user — Go would reject the cycle. The verification
// core therefore lives here, importing neither, so both sides of the import
// boundary can reach it.
//
// Contract for the payments gate:
// The verification consumers are the INTERACTIVE ACCOUNT FLOWS ONLY. The
// saved-card payments gates are now exclusively PSD2 SCA (Square buyer
// verification) and no longer call Check/VerifyForUser — the "charge gate"
// contract described in earlier revisions is obsolete. Today the callers are:
//
// err := twofa.VerifyForUser(ctx, userID, code, twofa.ConsumeOnVerify)
// if err != nil {
// switch {
// case errors.Is(err, twofa.ErrIncorrect):
// // 400
// case errors.Is(err, twofa.ErrLockedOut):
// // 429
// case errors.Is(err, twofa.ErrMissingOrExpired):
// // 400 — user must request a fresh code
// default:
// // 500 (DB failure)
// }
// }
// - handlers/user: 2FA setup verify (VerifyTwoFAHandler) and 2FA disable
// re-verification (DisableTwoFAHandler), both via checkTwoFACode with
// DeferredConsume (they clear the pending fields themselves on success);
// - handlers/user/account.go: delete-account re-authentication
// (DeleteAccountHandler) via twofa.VerifyForUser with ConsumeOnVerify, so
// one code authorizes exactly one account erasure.
//
// Consume mode (MEDIUM-2 remediation, finding 1): a successful verify with
// consume=true NULLs the pending code ATOMICALLY in the same critical section
// as the check, so one code authorizes exactly ONE operation — two concurrent
// charges can never both pass the gate with the same code (the per-user mutex
// serializes Check, and the second verify reads a NULLed digest and returns
// ErrMissingOrExpired). The payments saved-card CHARGE gates should therefore
// pass twofa.ConsumeOnVerify for FRESH charges: the code is burned at the gate,
// and a failed/ambiguous Square charge re-mints a fresh code (via the user
// package's exported EnsurePendingTwoFACode — reached through the HTTP mint
// endpoints, since handlers/payments cannot import handlers/user) instead of
// re-verifying the same code. This replaces the earlier MEDIUM-2 deferred
// consume (verify-with-consume=false at the gate + ConsumePendingCode at
// terminal success), which under concurrency let two gates both verify the same
// code before either charge consumed it.
// The payments package still touches this package on the TERMINAL-SUCCESS path
// only: ConsumePendingCode (after an SCA-approved saved-card charge or gift
// card issuance, where the pending code left over from the interactive mint
// must be retired) and StateFor/Hash for its code re-issue bookkeeping.
//
// The interactive setup/disable flows pass DeferredConsume (false) — they clear
// the pending fields themselves on success (enableTwoFA / disableTwoFA), so the
// code must stay valid through their whole handshake. The save-card SAVE gate
// (handlers/payments) passes ConsumeOnVerify (true), since saving a card is a
// terminal operation with no downstream charge to attach consumption to.
// Consume mode: a successful verify with consume=true NULLs the pending code
// ATOMICALLY in the same critical section as the check, so one code authorizes
// exactly ONE operation — two concurrent consumers can never both pass the gate
// with the same code (the per-user mutex serializes Check, and the second
// verify reads a NULLed digest and returns ErrMissingOrExpired). DeleteAccount
// is the only current ConsumeOnVerify caller.
//
// The failed-attempt counter is keyed per user and resets ONLY on a successful
// verify (or after the 10-minute attempt window elapses) — never on a fresh
@@ -78,9 +64,10 @@ const MaxAttempts = 5
const (
// ConsumeOnVerify makes a successful verify SINGLE-USE immediately: the
// pending-code digest and expiry are NULLed in the same critical section as
// the successful check (see Check). Use this for FRESH terminal operations —
// the saved-card CHARGE gates (finding 1) and the SAVE gate — where one
// code must authorize exactly one operation.
// the successful check (see Check). Use this for a TERMINAL operation that
// must be authorized by exactly one code — today that is the delete-account
// re-authentication flow (DeleteAccountHandler); the saved-card charge and
// SAVE gates no longer call Check (SCA-only since the PSD2 rework).
ConsumeOnVerify = true
// DeferredConsume verifies WITHOUT consuming; the caller NULLs the code
// itself when its operation reaches terminal success (ConsumePendingCode) or
@@ -194,7 +181,11 @@ func newSaturatedLockedState() *AttemptState {
st.Count.Store(MaxAttempts)
// Pinned so far in the future that now.Sub(LastActive) is always
// <= AttemptWindow (LockedOut true) and never > AttemptWindow (no reset).
st.SetLastActive(time.Now().Add(24 * 365 * 24 * time.Hour))
// clock.Now(), not time.Now(): the rest of the package reads time through
// crussell/clock (UTC-normalised, test-controllable) so the pinned stamp
// must be expressed in the same clock or a frozen test clock would leave
// now.Sub(LastActive) inconsistent with the saturation invariant.
st.SetLastActive(clock.Now().Add(24 * 365 * 24 * time.Hour))
return st
}
@@ -373,11 +364,12 @@ const (
// the pending code (lockout). A missing or expired pending code returns
// MissingOrExpired. consume makes a correct code single-use IMMEDIATELY: the
// stored digest and its expiry are NULLed right here, so one code cannot
// authorize a second operation within its lifetime. The interactive
// setup/disable flows pass DeferredConsume (false) and clear the pending fields
// themselves on success. The payments saved-card charge gates pass
// ConsumeOnVerify (true) for FRESH charges (finding 1): the code is burned at
// the gate, and a failed Square charge re-mints a fresh one. The returned
// authorize a second operation within its lifetime. The interactive account
// flows are the only consumers: the 2FA setup/disable handshakes pass
// DeferredConsume (false) and clear the pending fields themselves on success,
// while delete-account re-authentication passes ConsumeOnVerify (true) so one
// code authorizes exactly one erasure. The payments gates are SCA-only and no
// longer call Check. The returned
// error is non-nil only for DB failures (callers return 500); a lockout's
// pending-code invalidation failure is logged here and still reported as a
// lockout.
@@ -443,16 +435,16 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
}
// Success: clear the attempt counter before the caller performs its
// action. The mint-cooldown stamp (LastMintAt) is deliberately NOT cleared
// here (Round 2 Loop A finding 2): a code verified at the saved-card gate
// may still be followed by a FAILED Square charge that re-issues a fresh
// code (payments.reissueTwoFACodeAfterFailedCharge), and that re-issue path
// here (Round 2 Loop A finding 2): a code verified at an interactive gate
// may still be followed by a FAILED money action whose retry mints a fresh
// code, and that fresh-code mint path (twoFAMintThrottled in handlers/user)
// enforces the per-user mint cooldown against this stamp. Clearing it on a
// gate-verify let a charge-failure loop mint a fresh code on every
// iteration with no 60s cooldown (code churn + dev log flooding). The stamp
// is cleared only at a TERMINAL SUCCESS — the completed-charge consumption
// path (ConsumePendingCode, called by the money agent inside the
// transaction that records the completed charge) — so a customer who just
// completed a charge can immediately request a fresh code.
// gate-verify let a failure loop mint a fresh code on every iteration with
// no 60s cooldown (code churn + dev log flooding). The stamp is cleared
// only at a TERMINAL SUCCESS — the completed-operation consumption path
// (ConsumePendingCode, called inside the transaction that records the
// completed operation) — so a user who just completed a flow can
// immediately request a fresh code.
st.Count.Store(0)
st.SetLastActive(clock.Now())
ResetAttempts(userID)
@@ -508,23 +500,23 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry, and
// clears the per-user mint-cooldown stamp (AttemptState.LastMintAt).
// Since finding 1 the saved-card CHARGE gates consume a FRESH charge's code at
// verify time (consume=true — single-use), so this is no longer the gate's
// consumption path: it is used by the PENDING-REUSE retry path, whose gate
// verified WITHOUT consuming (consume=false) so a retry that fails again keeps
// its code for one more attempt — the handlers call this when the retry reaches
// a TERMINAL SUCCESS state, inside the transaction that records the completed
// charge. Idempotent: consuming an already-NULL pending code is a no-op, so a
// code still authorizes exactly one completed charge and can never authorize a
// second after success. Accepts a db.Querier so the write can ride the caller's
// transaction (pgx.Tx) or the pool proxy.
// The saved-card charge gates are SCA-only and no longer consume codes at a
// gate verify (there is no homegrown gate verify to consume at); this is used
// on the TERMINAL-SUCCESS paths of the payments package — after an
// SCA-approved saved-card charge, a gift card issuance, or a till sale that
// used the customer's pending code — inside the transaction that records the
// completed operation, so a pending code minted for a flow can never authorize
// a second one. Idempotent: consuming an already-NULL pending code is a no-op,
// so a code still authorizes exactly one completed charge and can never
// authorize a second after success. Accepts a db.Querier so the write can ride
// the caller's transaction (pgx.Tx) or the pool proxy.
//
// Round 2 Loop A finding 2: this is the ONLY place the mint-cooldown stamp is
// cleared on the charge path. A successful gate VERIFY (twofa.Check) must NOT
// clear it — the charge may still fail and the re-issue path
// (payments.reissueTwoFACodeAfterFailedCharge) enforces its cooldown against
// the stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting,
// so consumption (which runs only at that terminal state) clears it.
// clear it — the flow may still fail and the retry's fresh-code mint
// (twoFAMintThrottled in handlers/user) enforces its cooldown against the
// stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting, so
// consumption (which runs only at that terminal state) clears it.
func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error {
if userID == "" {
return nil
@@ -586,16 +578,13 @@ var (
// VerifyForUser verifies a 2FA code for a user outside the HTTP handler layer,
// under the same per-user brute-force lockout as the interactive endpoints.
// It returns nil on a correct code, or one of ErrIncorrect / ErrLockedOut /
// ErrMissingOrExpired (or a DB error, wrapped). This is the entry point for
// the payments card-access gate (B6/B10): a saved-card charge must present a
// real, freshly-verified challenge. consume makes a correct code single-use
// IMMEDIATELY (the pending-code digest and expiry are NULLed in the same
// critical section as the successful check — see Check). The payments saved-
// card CHARGE gate passes ConsumeOnVerify for FRESH charges (finding 1: a code
// authorizes exactly one charge, and a failed charge re-mints); the save-card
// SAVE gate passes ConsumeOnVerify too; the interactive setup/disable flows
// pass DeferredConsume and clear the pending fields themselves on success
// (enableTwoFA / disableTwoFA).
// ErrMissingOrExpired (or a DB error, wrapped). This is the non-HTTTP entry
// point for the interactive account flows — today only delete-account
// re-authentication (handlers/user/account.go), which passes ConsumeOnVerify so
// a code authorizes exactly one erasure. The payments saved-card gates no
// longer verify codes (SCA-only); the interactive setup/disable flows reach
// Check through handlers/user's checkTwoFACode with DeferredConsume and clear
// the pending fields themselves on success.
func VerifyForUser(ctx context.Context, userID, code string, consume bool) error {
st := StateFor(userID)
st.Mu.Lock()