Feature/2fa totp - #4
Merged
Merged
Conversation
CryptoRandTokenGenerator.New() returned ("", nil) when rand.Read
failed, silently treating an empty string as a valid token with no
error to catch it. rand.Read failing is rare on Linux but not
theoretically impossible (exhausted entropy, sandboxed environments),
and swallowing the error is a real correctness bug independent of how
rare the trigger is.
Swaps crypto/rand.Reader for a deterministic failing reader to exercise the New() error path, which never fails in practice under normal conditions and so had no prior coverage.
When GetByEmail found no user, Login returned immediately, skipping hasher.Compare entirely. A login attempt against a nonexistent email returned far faster than one against a real email with a wrong password (which pays bcrypt's cost) — a textbook user-enumeration side channel via response timing alone, independent of the error message (which was already identical either way). Fix: run a dummy hasher.Hash call on the nonexistent-user path so its timing profile matches a real wrong-password attempt. hasher.Hash and hasher.Compare run the same underlying bcrypt cost function, so this doesn't require a separately-maintained dummy hash.
Coarse smoke test (not a precision timing analysis) asserting the nonexistent-user path isn't dramatically faster than a real wrong-password attempt — enough to catch a future regression that removes the dummy hasher.Hash call.
Reversible symmetric encryption for secrets that must be recovered in plaintext later — unlike Hasher, which is deliberately one-way. Needed for TOTP secrets: the engine must decrypt a secret back to its raw value to validate a code against it, so hashing doesn't apply here.
Round-trip, distinct nonce per call, wrong key fails to decrypt, empty key rejected at construction.
Wraps github.com/pquerna/otp rather than hand-rolling RFC 6238 against the stdlib the way Hasher/RateLimiter/IDGenerator do — TOTP has enough real edge cases (base32 padding, clock-skew windows, Google-Authenticator-compatible defaults) that a battle-tested implementation is worth the one dependency. Adds github.com/pquerna/otp and its transitive boombuler/barcode dependency (used internally by otp's package-level QR-image helper, which this engine never calls) to go.mod. Run 'go mod tidy' to populate go.sum.
Secret/URL generation, correct-code acceptance, wrong-code rejection, expired-code rejection outside the skew window.
One secret per user. EncryptedSecret is encrypted at rest, never hashed (validating a code requires recovering the original secret). ConfirmedAt is nil until the user proves possession with one valid code — an unconfirmed secret must never gate login. Also adds totp_enabled/totp_disabled/totp_challenge_failed audit event types.
For tests and local experimentation only, matching the existing in-memory store conventions (not a supported production backend).
The v2 production TOTPStore backend. Upsert always resets confirmed_at to NULL on conflict, so restarting enrollment can never leave a stale confirmed secret active alongside a new unconfirmed one.
Short-lived (5 min, fixed, not configurable), stateless token proving a caller already presented a correct password for the embedded userID and is now expected to complete login with a second factor. Signed with the same secret as JWTIssuer but distinguished by a dedicated 'typ' claim, checked on Verify, specifically to prevent a real access token ever being accepted in its place.
- EnrollTOTP: generates a secret, encrypts it at rest, returns the otpauth:// URL. Does not gate login yet. Rejects re-enrollment once a secret is already confirmed. - ConfirmTOTP: activates a pending secret once the user proves possession with one valid code. A secret that's never confirmed can never gate login — prevents an interrupted enrollment (browser closed before scanning the QR code) from locking the user out. - DisableTOTP: requires the current password as re-confirmation, same reasoning as ChangePassword/DeleteAccount — a stolen access token alone should never be enough to weaken an account's own auth requirements. - CompleteLoginWithTOTP: verifies a pending token, checks the code, and issues real tokens on success. Adds *ErrTOTPRequired (struct type, retrievable via errors.As, same pattern as *ErrOAuthEmailConflict), ErrTOTPNotEnabled, ErrTOTPAlreadyEnabled, ErrInvalidTOTPCode, and ErrInvalidPendingLogin.
Covers: enroll stores an unconfirmed secret, re-enrollment rejected once confirmed, confirm rejects a wrong code without confirming, confirm accepts a correct code, disable requires the correct password and leaves the secret untouched on a rejected attempt.
Login takes two new params (totpStore, pendingIssuer), both nil-safe — an Engine built without Config.TOTP passes nil for both and Login behaves exactly as before. After password verification, if the account has a confirmed TOTP secret, Login now issues a pending token and returns *ErrTOTPRequired instead of tokens. Extracts the post-verification tail (session creation, access token issuance, audit record) into a shared finishLogin helper, used by both Login (password-only path) and CompleteLoginWithTOTP (second-factor path), so a completed login produces an identical session regardless of which path got it there. This changes auth.Login's internal signature, not the public facade — cryden.Login(ctx, e, email, password, callerIP, userAgent) is unchanged; auth is documented as implementation detail, imported only by the top-level cryden package. Updates existing lockout_test.go/login_test.go call sites to pass nil, nil for the two new params.
Covers: confirmed TOTP pauses login and issues a pending token; no TOTP enrolled logs in directly as before; unconfirmed TOTP never gates login; correct/wrong code completion; a tampered pending token is rejected; and specifically, a real access token cannot be substituted for a pending token (the 'typ' claim check).
- Config.TOTP (optional, store.TOTPStore), Config.EncryptionKey (required if TOTP is set), Config.TOTPIssuerName (optional, defaults to "Cryden"). - New() validates EncryptionKey is set whenever TOTP is, and constructs the pending-login issuer, encryptor, and TOTP generator only when TOTP is configured — they stay nil otherwise. - New facade functions: EnrollTOTP, ConfirmTOTP, DisableTOTP, CompleteLoginWithTOTP, each returning cryden.ErrTOTPNotConfigured if called without Config.TOTP set. - Login's facade signature is unchanged; it now threads e.totp and e.pendingIssuer through to auth.Login internally.
Runnable end-to-end check with no database dependency: go run ./cmd/smoketest/2fa-totp. Walks the happy path (signup, login before enrollment, enroll, login before confirming, confirm, login paused, complete) and the negative cases (wrong confirm code, wrong login code, garbage pending token, a real access token substituted for a pending token, wrong password on disable), printing a pass/fail line per step.
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.
No description provided.