Conversation
- Introduce BackupCodeManager for recovery codes:
- Generates unique, user-facing codes and Argon2id-hashed values for storage
- Input normalization (spaces/hyphens removed, uppercased), grouping, and
configurable count/length/alphabet/hasher; returns remaining hashes on use
- Add rate limiting to verification paths via RateLimiter interface and
WithTOTPRateLimiter/WithHOTPRateLimiter/WithBackupRateLimiter; return
ErrMFARateLimited on denial
- TOTP: add VerifyWithStep to return the matched time step for replay
protection; Verify delegates to the new flow
- HOTP: add Resync to recover a drifting counter using two consecutive codes;
WithHOTPResyncWindow config and internal generateCode refactor
- Extend errors for conflicting options and backup code operations
- Docs: update README, usage, and security checklist (backup codes, resync,
rate limiting); add examples
- Tests: coverage for backup codes, TOTP VerifyWithStep, HOTP resync, and
rate-limiting; update cspell dictionary
No breaking changes expected.
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive MFA enhancements including backup codes, rate limiting, TOTP replay protection via time steps, and HOTP counter resynchronization. The changes maintain backward compatibility while adding critical security and reliability features for MFA implementations.
Changes:
- Added
BackupCodeManagerfor generating and verifying recovery codes with Argon2id/bcrypt hashing - Introduced
RateLimiterinterface with support across TOTP, HOTP, and backup code verification - Enhanced TOTP with
VerifyWithStepmethod to return matched time steps for replay protection - Added HOTP
Resyncmethod to recover drifting counters using two consecutive codes
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mfa/types.go | Defines RateLimiter interface for verification rate limiting |
| pkg/mfa/totp.go | Adds VerifyWithStep, rate limiting support, and refactored verification with step tracking |
| pkg/mfa/totp_test.go | Tests for VerifyWithStep, invalid options validation |
| pkg/mfa/hotp.go | Adds Resync method, rate limiting support, and refactored code generation |
| pkg/mfa/hotp_test.go | Tests for Resync with consecutive/non-consecutive codes, invalid options |
| pkg/mfa/backup.go | Complete backup code manager implementation with hashing and verification |
| pkg/mfa/backup_test.go | Tests for backup code generation, verification, normalization, and invalid inputs |
| pkg/mfa/rate_limit_test.go | Tests for rate limiting across TOTP, HOTP, and backup codes |
| pkg/mfa/helpers.go | Adds checkRateLimiter utility and new constants for resync window |
| pkg/mfa/errors.go | New errors for rate limiting, backup operations, and conflicting options |
| docs/usage.md | Documents new APIs including VerifyWithStep, Resync, backup codes, and rate limiting |
| docs/security-checklist.md | Updates with rate limiting and backup code best practices |
| README.md | Adds backup code example |
| cspell.json | Adds "resync", "Resync", "uppercasing" to dictionary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| offsetValue, err := converters.SafeInt64FromUint64(uint64(offset)) | ||
| if err != nil { | ||
| return false, 0, fmt.Errorf(mfaWrapFormat, ErrInvalidMFAConfig, err) | ||
| } |
There was a problem hiding this comment.
The conversion from uint offset to int64 using SafeInt64FromUint64 is unnecessary here. Since opts.Skew is validated to be at most totpMaxSkew (3), the offset value will never exceed the maximum int64 value. Consider simplifying this to a direct cast: offsetValue := int64(offset) to reduce complexity and improve readability.
| offsetValue, err := converters.SafeInt64FromUint64(uint64(offset)) | |
| if err != nil { | |
| return false, 0, fmt.Errorf(mfaWrapFormat, ErrInvalidMFAConfig, err) | |
| } | |
| offsetValue := int64(offset) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No breaking changes expected.