[IAmJSD]

Summary

Issue #263 lists auth failure paths that had no negative test. I checked each item against current main (2d822f3). Most are now covered by tests that came in with the fixes merged since the audit. Two still reproduced. This PR adds their regression tests and fixes both, fail-closed:

#263 item	State on main	This PR
Wildcard-resource carve-out variants (cli#1901)	Covered: carveout_wildcard_test.go (from #2069)	Mutation probe only (M1)
Recovery-code persist failure (cli#1916)	Covered: TestLogin_WithMFA_RecoveryCode_PersistenceFailure and TestIntegration_LoginRecoveryPersistenceFailure (#462)	Mutation probe only (M2)
Deactivation asserts DeleteUserSessions and a rejected session (#89)	Covered: TestService_UpdateUser/update_active_status_successfully, TestService_ValidateSession/deactivated_user_session_rejected	Mutation probes only (M3a, M3b)
MFA re-enrol while enabled refused (cli#1915)	Covered: TestMFASetupAndEnable_RefusedWhileEnabled	Mutation probe only (M4)
Reset on a deactivated account refused (A03-006)	Covered: request and confirm subtests in service_password_test.go	Mutation probes only (M5a, M5b)
Invited user can use forgot-password (A03-009)	Bug still present, no test	Test and fix
Signer recovers after a failed first KMS call (A03-014)	Bug still present, no test	Test and fix
Turn TestResolveBastionProvider_LegacyFallback into an error assertion once the fallback is removed (A03-011)	The fallback has not been removed	Out of scope, see below

Root cause and fix

A03-009: forgot-password silently did nothing for invited users

resetRecentlyIssued worked out a token's age as PasswordResetExpiry - time.Until(expiry), which treats every stored token as a 1h reset token. Invite tokens are issued with PasswordSetupExpiry (7d), so an invite's age came out negative for about 6d23h. During that time RequestPasswordReset returned nil without issuing a token or sending mail. That is the recovery path CreateUserResult sends the admin to when the invite email fails.

Fix (internal/auth/service_password.go): a reset token never has more than PasswordResetExpiry left, so a longer remaining window can only belong to an invite. In that case the age is now derived from PasswordSetupExpiry. Reset tokens are handled exactly as before. A remaining window longer than even the invite lifetime still gives a negative age, so it stays rate-limited (fail-closed). No schema change.

Concurrency: an invited user's request now reaches the existing SetPasswordResetToken compare-and-swap on password_reset_expiry. The new integration test runs a second request between the first one's read and its write, on real Postgres. Only the request that commits first stores and mails a token. The losing request mails nothing.

A03-014: KMS signers cached the first lookup error forever

All three KMS-backed signers (AWS, Azure Key Vault, GCP) resolved the public key inside sync.Once and stored the error alongside it. One failed first GetPublicKey/GetKey (a throttle, a network blip, or a caller whose ctx was already canceled) made every later KeyID/PublicKey/Mint fail until the instance recycled. That takes down every federated Azure/GCP credential exchange.

Fix (internal/oidc/signer.go plus the three signers): a shared publicKeyCache memoizes only a successful resolution. A failed fetch goes back to its caller, and the next call fetches again. A mutex serializes first callers, so concurrent callers never fetch in parallel and a success is fetched exactly once. As before, waiters block while the first fetch is in flight, the same as with sync.Once. On error PublicKey now returns an untyped nil instead of a typed-nil *ecdsa.PublicKey. Validation errors (wrong curve, not ECDSA, incomplete JWK) still fail every call. They are just no longer memoized.

Verification

All of this is local unit, mock and fixture evidence, plus real local PostgreSQL 16 via testcontainers. Nothing calls real AWS KMS, Azure Key Vault or GCP KMS: the signer tests use in-process fakes that wrap the existing fake clients with a failure gate. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail-before (new tests against the unfixed production code):

go test ./internal/auth/ -run 'TestService_RequestPasswordReset_InvitedUser|TestResetRecentlyIssued' -count=1 -v
--- FAIL: TestService_RequestPasswordReset_InvitedUser/invite_older_than_the_rate_limit_gets_a_fresh_link
--- FAIL: TestResetRecentlyIssued/invite_token_past_the_limit
--- FAIL: TestResetRecentlyIssued/invite_token_a_day_old

go test -tags integration ./internal/auth/ -run TestIntegration_InvitedUserForgotPasswordReissuesLink -count=1
Error: "[]" should have 1 item(s), but has 0 -- the first committed request must issue and mail a link

go test ./internal/oidc/ -run TestKMSSigner -count=1 -race -v
PublicKey after a failed first lookup: oidc: kms:GetPublicKey: transient key lookup failure (the failure was cached)
  (same for context canceled, and for azure and gcp: 6/6 subtests fail)
callers that saw the transient failure = 16, want 1   (aws, azure, gcp)
key lookups = 1, want 2                               (aws, azure, gcp)

Pass-after (c0a4764f):

go build ./...
go vet ./internal/auth/ ./internal/oidc/ && go vet -tags integration ./internal/auth/
go test -race -count=1 ./internal/auth/ ./internal/oidc/ ./internal/mocks/        # ok, ok, ok
go test -tags integration -count=1 ./internal/auth/                                # ok (real Postgres, full auth integration suite)
go test -short -count=1 ./internal/{credentials,api,purchase,scheduler,server}/    # ok (packages importing internal/oidc)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/... ./internal/oidc/                      # 0 issues
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/auth/...              # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/oidc internal/auth/service_password.go                         # silent
bash scripts/gosec-hook.sh <changed files>                                                                   # exit 0
git-secrets --scan (fresh git init, isolated HOME, repo's setup-git-secrets.sh)                              # exit 0
bash scripts/test-git-secrets-allowlist.sh                                                                   # 42 PASS, 0 FAIL

Mutation probes. For each probe I applied one production mutation, ran the named tests, and restored the file. Every probe was killed:

ID	Production mutation	Killed by
M1	coversCarvedOut compares exact pairs instead of checkPermissionMatch (reintroduces cli#1901)	TestGrantCeiling_WildcardMoneyVerbNotGrantable/create_execute:*
M2	recovery-code login ignores a ConsumeMFARecoveryCode failure	TestLogin_WithMFA_RecoveryCode_PersistenceFailure
M3a	deactivation skips DeleteUserSessions	TestService_UpdateUser/update_active_status_successfully (+2 more)
M3b	ValidateSession drops the !user.Active check	TestService_ValidateSession/deactivated_user_session_rejected
M4	MFASetup drops the MFAEnabled refusal	TestMFASetupAndEnable_RefusedWhileEnabled
M5a	RequestPasswordReset drops the DeactivatedAt refusal	TestService_RequestPasswordReset/deactivated_account_gets_no_reset_link,...
M5b	ConfirmPasswordReset drops the DeactivatedAt refusal	TestService_ConfirmPasswordReset/admin-deactivated_account_cannot_self-reactivate_via_reset
M6a	invite age derived from the reset lifetime again (the A03-009 bug)	TestService_RequestPasswordReset_InvitedUser, TestResetRecentlyIssued
M6b	every token aged as an invite (breaks the reset-token rate limit)	TestResetRecentlyIssued/reset_token_just_issued, TestService_RequestPasswordReset_RateLimit
M6c	invites never rate-limited	TestService_RequestPasswordReset_InvitedUser/invite_inside_the_rate_limit_window_is_not_reissued
M7a	signer cache memoizes the error (the A03-014 bug)	TestKMSSignerRetriesFailedKeyLookup (all backends)
M7b	signer cache never memoizes success	TestKMSSignerRetriesFailedKeyLookup (lookup count)
M7c	signer cache mutex removed	TestKMSSignerConcurrentFirstResolution under -race (all backends)
M8	SetPasswordResetToken CAS on password_reset_expiry neutralized	TestIntegration_InvitedUserForgotPasswordReissuesLink (real Postgres)

Out of scope

• A03-011, bastion legacy fallback. The last checklist item is conditional on the fallback being removed, and it has not been. No production caller sets AWSResolveOptions.AccountLookup/STSClientFactory (scheduler, org discovery, purchase, marketplace, AWS probe, server). Making resolveBastionProvider fail closed on its own would therefore break every bastion account on every path. The real fix needs those callers wired, and that belongs in its own change. I left TestResolveBastionProvider_LegacyFallback as it is rather than write a test that pins a fallback nobody has removed. I found no separate issue tracking A03-011. This PR only refers to test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263 (it does not close it), so test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263 stays open to track that item.
• chore(auth): medium/low findings from the 2026-07-28 full review #104, signer key rotation. A successful resolution is still memoized for the process lifetime, so the key-rotation and JWKS-staleness item in chore(auth): medium/low findings from the 2026-07-28 full review #104 is unchanged. This PR only stops failures from being memoized.

Refs #263 (the bastion fallback test, A03-011, stays open until the fallback is removed)

Maintainers: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, impact/internal, type/chore, effort/m, urgency/this-quarter, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE c0a4764

Reviewed head c0a4764 against origin/main ca1e8a8. git merge-tree is clean (merged tree 52116640). I ran all tests below on the merged tree. The PR uses "Refs #263" with no closing keyword (closingIssuesReferences is []), which is correct because the A03-011 item stays open.

Findings

1. nit (process): the single commit has 662 changed lines (528+, 134-), which is over the 400-line cap. Non-blocking.
2. nit internal/oidc/signer.go:216-232: publicKeyCache.get holds the mutex across the KMS network call, and lock acquisition does not watch the context. During a sustained KMS outage every Mint/BuildJWKS call now makes a fresh serialized fetch with no backoff. Before this change, the cached error made those calls fail instantly. Waiters with an already-expired ctx fail fast once they get the lock, so the queue drains, and the PR body states the blocking behaviour. Optional: a short negative-cache or backoff window.

Verified (file:line)

• A03-009: invites are stored at internal/auth/service_user.go:242-244 with PasswordSetupExpiry (7d), in the same columns as reset tokens (PasswordResetExpiry, 1h, service_password.go:358). On main, resetRecentlyIssued computed a negative age for about 7 days, so RequestPasswordReset (service_password.go:321) returned nil with no write and no mail. The fix at service_password.go:306-311 classifies tokens with more than 1h remaining as invites. Expired invites and expired resets both reissue. Future-dated expiries beyond 7d stay rate-limited. The reissue still goes through the CAS at store_postgres_credentials.go:209-216 (password_reset_expiry IS NOT DISTINCT FROM $5), and the deactivated refusal at service_password.go:339 still runs first.
• A03-014: the three signers delegate to publicKeyCache, which caches only a success (signer.go:216-232). Errors propagate, so Mint (signer.go:114) and BuildJWKS (jwks.go:79-86) fail closed. The validation errors (curve, non-ECDSA, incomplete JWK) are preserved verbatim. Only get touches pubKey/kid, under the lock.
• I spot-checked that the "already covered" test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263 items exist on the merged tree (carveout_wildcard_test.go, service_mfa_test.go, service_recovery_db_test.go, service_test.go, service_password_test.go).

Attacked

Each mutation probe was run against the merged tree, then the file was restored and diff-verified:

• MA: old age formula (PasswordResetExpiry - remaining). Killed by TestService_RequestPasswordReset_InvitedUser and TestResetRecentlyIssued. This is the fail-before result.
• MB: the cache memoizes the error. Killed by TestKMSSignerRetriesFailedKeyLookup and TestKMSSignerConcurrentFirstResolution. Also a fail-before result.
• MC: mutex removed. Killed under -race by TestKMSSignerConcurrentFirstResolution.
• ME: the publicKey adapter returns (nil, nil) on error (fail-open). Killed by the wrong-curve and incomplete-key tests for AWS, GCP and Azure.

Local results (macOS, merged tree)

• go build ./... OK. go vet on auth/oidc is clean, with and without -tags integration.
• go test -race -count=1 ./internal/auth/... ./internal/oidc/... OK.
• go test -race -short -run 'Reset|Invite|Password|OIDC|JWKS|Signer|Federat' ./internal/api/ ./internal/credentials/ OK.
• go test -tags integration -race -run 'TestIntegration_InvitedUser|TestIntegration_.*Reset' ./internal/auth/ OK on real Postgres via testcontainers, under heavy.lock. TestIntegration_InvitedUserForgotPasswordReissuesLink ran and passed; it was not skipped.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE c0a4764

Both remaining bugs are fixed at the root. Everything passes on head and on the tree merged with origin/main, the new tests fail on base, and the behaviour-changing mutation probes are caught except two test gaps.

Findings (none blocking):

• [non-blocking] internal/auth/service_password.go:307-311 - Invite vs reset is decided from the token's remaining lifetime.
  • So an invite with 59-60 minutes left is aged as a reset token, and a request in that last minute is rate-limited.
  • Probe P7 (threshold +1m) survives, so this edge is untested.
  • The alternative, deciding by account state (resetTokenFlowFor, :478), would be worse: an invited user who already holds a 1h reset token would never be rate-limited again. So the remaining-lifetime approach is the right one.
• [non-blocking] internal/oidc/signer.go:217-232 - The mutex makes first callers queue, and failures are no longer shared.
  • During a KMS outage, N concurrent callers each run their own fetch one after another under the lock, so the last waits about N times the fetch latency. With sync.Once they all failed fast.
  • The lock also ignores each waiter's ctx. That was already true, but now each waiter also fetches.
  • If callers that arrive during a failing fetch should share its error without memoising it, singleflight is already a dependency (internal/auth/service.go).
  • The current behaviour is a defensible design choice, and the tests pin it.
• [non-blocking] signer.go:235-241 - No test covers the untyped-nil fix. Probe P4 (returning a typed nil *ecdsa.PublicKey again) survives. The only caller, BuildJWKS (internal/oidc/jwks.go:79), checks err first, so nothing can break today.

Root cause:

• A03-009:
  • An invite expires at now + 7d (service_user.go:242).
  • The old age formula assumed the 1h lifetime, so an invite's age came out negative and the request was silently dropped. The fix corrects that.
  • Confirm activates the account (service_password.go:427), and the integration test shows a re-sent link activating it.
• A03-014:
  • The old code cached the error inside sync.Once. The new cache stores only a success.
  • Wrong-curve and incomplete-key errors are still returned on every call.

Attacked:

• Blast radius: PublicKey and KeyID are called only at jwks.go:79/83 and signer.go:114 (Mint), and all check err. The removed struct fields were unexported, and the constructors are unchanged.
• Drift: main touched internal/auth after the base (dec9ad7), but merge-tree --write-tree origin/main <head> is clean, and internal/oidc is identical on both sides. No migration.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • P1, the invite lifetime switch removed: TestResetRecentlyIssued and TestService_RequestPasswordReset_InvitedUser FAIL.
  • P2, the cache memoises the error: both TestKMSSigner* tests FAIL, on all 3 backends.
  • P3, the mutex removed: a data race, with lookups 5/7/14 against a wanted 2.
  • P4, a typed nil on error: survives (gap above).
  • P5, the AWS curve check dropped: TestAWSKMSSignerRejectsWrongCurve FAILS.
  • P6, the invite threshold lowered by 2m: TestResetRecentlyIssued and TestService_RequestPasswordReset_RateLimit FAIL.
  • P7, the invite threshold raised by 1m: survives (edge above).

Verified locally, with no cloud credentials, under the heavy lock, using real Postgres via the repo's testcontainers harness:

• Head:
  • go build ./...: ok.
  • go vet on auth and oidc, and with -tags integration on auth: ok. gofmt: clean.
  • go test -race -count=1 ./internal/auth/ ./internal/oidc/ ./internal/mocks/: ok.
  • go test -tags integration -count=1 ./internal/auth/: ok.
  • go test -short on credentials, api, purchase, scheduler and server: ok.
• Merged tree: build ok, -race on auth and oidc ok, and -tags integration ./internal/auth/ ok.
• Fail-before, with the new tests on base, failing on assertions rather than compile errors:
  • TestService_RequestPasswordReset_InvitedUser and TestResetRecentlyIssued: FAIL.
  • TestKMSSigner*: FAIL ("callers that saw the transient failure = 16, want 1"; "key lookups = 1, want 2").
  • TestIntegration_InvitedUserForgotPasswordReissuesLink: FAILS ("[]" should have 1 item(s)).

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Merged origin/main (a14dc8f) to resolve the conflict, and closed the two test gaps from independent review 2. New head: ceae3074. No rebase or force-push.

Conflict: internal/oidc/azure_signer.go

• Main's change: 7eac792 (sec(oidc): check Azure Key Vault key type and curve before using it for ES256 #1167) added a kty/crv check (validateES256KeyType) inside resolveOnce.
• This PR's change: it replaced resolveOnce with fetchPublicKey, which sits behind publicKeyCache.
• Resolution: the check now runs in fetchPublicKey at azure_signer.go:106. It is in the same position as on main: after the missing X/Y check and before the coordinate parse. It returns its error with the same wording instead of memoizing it. validateES256KeyType/quotedOrMissing are kept as on main, and the struct keeps keys publicKeyCache.
• Probe: disabling the check fails main's TestAzureSigner_RejectsNonP256Key (file restored, cmp-verified).

Merge commit: d2ed0ec.

Review findings (both reviews APPROVE c0a4764)

1. cristim, nit (process): over the 400-line cap. Left as is. It's one fix per audit item plus their tests, and splitting it now would only churn the review.
2. Both reviews, non-blocking: publicKeyCache.get holds the mutex across the fetch, and failures aren't shared or backed off (signer.go:216). Left as is: it's a design choice. A negative-cache or backoff window, or singleflight so waiters share a failing fetch's error, can follow if you want it.
3. Review 2, non-blocking: invite/reset threshold edge, probe P7 survived (service_password.go:308). Added the row invite token about to expire to TestResetRecentlyIssued (service_password_invite_test.go:96): an invite with PasswordResetExpiry+30s left must not be limited. P7 (threshold +1m) now fails that row. The last-minute limitation you described (an invite with under 1h left is aged as a reset token) is unchanged, as you suggested.
4. Review 2, non-blocking: no test for the untyped nil, probe P4 survived. Added TestKMSSignerPublicKeyErrorIsUntypedNil (signer_test.go:427), which covers AWS, Azure and GCP. P4 (return pub, err with a typed-nil *ecdsa.PublicKey) now fails it.

Commit for 3 and 4: ceae307.

Verification on the merged tree (Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, no cloud calls)

• go build ./...: OK.
• go vet ./internal/auth/ ./internal/oidc/, with and without -tags integration: OK.
• gofmt: clean.
• go test -race -count=1, one package at a time, on ./internal/auth/, ./internal/oidc/ and ./internal/mocks/: all OK.
• go test -short -count=1 on credentials, api, purchase, scheduler and server: all OK.
• go test -tags integration -count=1 ./internal/oidc/: OK.
• go test -tags integration -count=1 ./internal/auth/: OK on d2ed0ec (real Postgres via testcontainers).
  • TestIntegration_InvitedUserForgotPasswordReissuesLink passes on ceae307. It ran and was not skipped.
  • Later full-suite reruns on this VM hit a testcontainers reaper race (wait for reaper ...: unexpected container status "removing"). Each time it hit one container-start test, under load from other parallel suites.
  • A pristine origin/main a14dc8f hits the same error, on TestIntegration_LoginSequentialFailures and TestIntegration_LoginLockoutExpiry. The affected test here, TestIntegration_CredentialWritesPreserveConcurrentMFA, passes when rerun on its own.
  • This is environmental. The PR doesn't cause it.
• golangci-lint v2.10.1 --allow-parallel-runners on ./internal/auth/... ./internal/oidc/..., with and without --build-tags integration: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/oidc internal/auth/service_password.go: silent.
• git-secrets --scan: exit 0. Setup: a fresh git init with an isolated HOME, the repo's setup-git-secrets.sh, and the PR's 11 changed files git added. A planted fake AWS key was flagged (exit 1).
• scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• git merge-tree of ceae307 against the current origin/main is clean.

The PR stays a draft.

🤖 Generated with Claude Code