Add TOTP/MFA (Google Authenticator-style time-based OTP)#5
Conversation
- Add TOTPSecret type, TOTPStore interface, and ErrTOTPNotFound / ErrInvalidTOTPCode sentinel errors to auth/types.go - Add auth/totp.go with GenerateTOTPSecret, TOTPProvisioningURI, ValidateTOTP (RFC 6238) and the internal hotpCode implementing RFC 4226 - Add auth/totp_test.go with 13 tests including RFC 4226 Appendix D vectors - Add handler/totp.go with TOTPHandler (Status, Generate, Enroll, Verify, Disable) - Add handler/totp_test.go with 20 tests covering all handler endpoints and error paths Agent-Logs-Url: https://github.com/amalgamated-tools/goauth/sessions/505e8648-be11-46b2-b751-332f1be5ea2b Co-authored-by: veverkap <22348+veverkap@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds RFC 6238 TOTP-based MFA support to the auth and handler packages, introducing secret generation/validation utilities plus HTTP endpoints and tests to support enrollment, verification, status checks, and disabling.
Changes:
- Introduces
auth-level TOTP primitives (GenerateTOTPSecret,TOTPProvisioningURI,ValidateTOTP) and HOTP implementation. - Adds
TOTPStore/TOTPSecrettypes and new sentinel errors for store/verification flows. - Implements
handler.TOTPHandlerendpoints and comprehensive handler/unit tests for the lifecycle.
Show a summary per file
| File | Description |
|---|---|
auth/types.go |
Adds TOTP store interface, secret model, and sentinel errors. |
auth/totp.go |
Implements secret generation, provisioning URI creation, and TOTP validation. |
auth/totp_test.go |
Adds unit coverage for TOTP/HOTP generation and validation behavior. |
handler/totp.go |
Adds HTTP handlers for generate/enroll/verify/status/disable TOTP flows. |
handler/totp_test.go |
Adds handler tests covering success/error paths across endpoints. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
|
/grumpy |
|
😤 Fine. Grumpy Code Reviewer 🔥 finished the review. It wasn't completely terrible. I guess. 🙄 |
There was a problem hiding this comment.
The TOTP implementation is structurally sound — RFC 4226 vectors pass, the enrollment flow is two-step, tests exist. But there are real problems that need fixing before this merges: no replay protection (a fundamental MFA requirement), leaking database/sql through the interface abstraction, copy-pasted crypto in tests, and a status handler that silently lies when the store errors. Fix those and I'll grudgingly admit it's adequate.
😤 Reluctantly reviewed by Grumpy Code Reviewer 🔥
To install this agentic workflow, run
gh aw add amalgamated-tools/biblioteka/.github/workflows/grumpy-reviewer.md@0dff8ccb0ced8d634877b4201f25795e659dced0
- Lowercase ErrTOTPNotFound message to follow Go error string conventions - Use math.Pow10(totpDigits) and %0*d in hotpCode to respect the constant - Document replay-protection gap in ValidateTOTP warning comment - Export GenerateTOTPCode for testing and tooling use - Fix TestValidateTOTPPreviousStep to skip near step boundaries (flaky) - Remove database/sql import from handler layer; check only auth.ErrTOTPNotFound - Return 500 from Status on unexpected store errors instead of silently reporting enrolled=false - Enforce minimum 20-byte decoded secret length in Enroll - Replace copy-pasted currentCode() in handler tests with auth.GenerateTOTPCode - Add TestTOTPStatusStoreError to cover the new 500 path
Kept TOTP sentinel errors (ErrTOTPNotFound, ErrInvalidTOTPCode) and TOTP types/store alongside main's new ErrNotFound sentinel, PasswordResetToken type, and PasswordResetStore interface. Updated TOTPStore.GetTOTPSecret doc to drop stale sql.ErrNoRows reference.
Implements RFC 6238 TOTP support with a
TOTPStoreinterface for secret persistence, plus HTTP handlers covering the full enrollment and verification lifecycle.authpackageTOTPStoreinterface —CreateTOTPSecret,GetTOTPSecret,DeleteTOTPSecret; consuming apps implement against their DB layerTOTPSecrettype —ID,UserID,Secret(unpadded base32; apps may store encrypted),CreatedAtErrTOTPNotFound/ErrInvalidTOTPCodesentinel errorsGenerateTOTPSecret()— 20-byte CSPRNG secret as unpadded base32TOTPProvisioningURI(secret, accountName, issuer)— standardotpauth://totp/…URI for QR displayValidateTOTP(secret, code)— HMAC-SHA1/RFC 4226 with ±1 step clock-skew windowhandlerpackageTOTPHandler— router-agnostic, mirrors the existing handler patterns:GET /totp/status{"enrolled": bool}POST /totp/generate{secret, provisioning_uri}; no DB write — secret is unconfirmedPOST /totp/enroll{secret, code}, verifies code, then persistsPOST /totp/verifyDELETE /totpEnrollment is intentionally two-step:
generate→ user scans QR →enrollwith first code, ensuring the authenticator is correctly configured before the secret is committed.