fix: use constant time in user lookups - #1002
Conversation
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughLogin user lookup errors now use a constant-time helper that computes the response, pads processing to 45ms when needed, and then sends the appropriate JSON payload while preserving failure recording, auditing, and logging. ChangesLogin Error Timing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/user_controller.go (1)
478-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
constantTimeby returning the result directly.The current design uses a callback (
rf) to process the response, which creates unnecessary inversion of control. ReturningconstantTimeResdirectly makes the control flow more linear and easier to read. Additionally,time.Sincecan be used for a cleaner elapsed time calculation.
internal/controller/user_controller.go#L478-L492: Remove therfcallback parameter, return the result directly, and usetime.Sincefor the elapsed time calculation.internal/controller/user_controller.go#L92-L115: Capture the returned result fromconstantTimeand callc.JSONdirectly instead of passing a callback.♻️ Proposed refactor
Function definition (
internal/controller/user_controller.go#L478-L492)-func (controller *UserController) constantTime(f func() constantTimeRes, rf func(res constantTimeRes), targetTime time.Duration) { +func (controller *UserController) constantTime(f func() constantTimeRes, targetTime time.Duration) constantTimeRes { tStart := time.Now() res := f() - tEnd := time.Now() - if tEnd.Sub(tStart) < targetTime { - time.Sleep(targetTime - tEnd.Sub(tStart)) + if elapsed := time.Since(tStart); elapsed < targetTime { + time.Sleep(targetTime - elapsed) } - rf(res) + return res }Function caller (
internal/controller/user_controller.go#L92-L115)- controller.constantTime(func() constantTimeRes { + res := controller.constantTime(func() constantTimeRes { if errors.Is(err, service.ErrUserNotFound) { controller.log.App.Warn().Str("username", req.Username).Msg("User not found during login attempt") controller.auth.RecordLoginAttempt(req.Username, false) controller.log.AuditLoginFailure(req.Username, "unknown", c.ClientIP(), "user not found") return constantTimeRes{ Code: 401, Res: gin.H{ "status": 401, "message": "Unauthorized", }, } } controller.log.App.Error().Err(err).Str("username", req.Username).Msg("Error searching for user during login attempt") return constantTimeRes{ Code: 500, Res: gin.H{ "status": 500, "message": "Internal Server Error", }, } - }, func(res constantTimeRes) { - c.JSON(res.Code, res.Res) - }, time.Millisecond*45) + }, time.Millisecond*45) + c.JSON(res.Code, res.Res) return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/user_controller.go` around lines 478 - 492, Refactor UserController.constantTime to remove the response callback, return the computed constantTimeRes directly, and use time.Since for elapsed-time calculation while preserving the target delay. At internal/controller/user_controller.go lines 92-115, capture the returned result from constantTime and invoke c.JSON directly with it; update the caller and function signature consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/user_controller.go`:
- Around line 113-115: Replace the hardcoded time.Millisecond*45 delay in the
constant-time response callback with dummy password verification using the same
work factor, or an equivalent dummy LDAP bind, when CheckUserPassword reports
ErrUserNotFound. Preserve the existing c.JSON response behavior and ensure the
missing-user path performs comparable verification work without relying on a
fixed sleep.
---
Nitpick comments:
In `@internal/controller/user_controller.go`:
- Around line 478-492: Refactor UserController.constantTime to remove the
response callback, return the computed constantTimeRes directly, and use
time.Since for elapsed-time calculation while preserving the target delay. At
internal/controller/user_controller.go lines 92-115, capture the returned result
from constantTime and invoke c.JSON directly with it; update the caller and
function signature consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb5234f7-64dd-4758-8110-c9a84d4b4eec
📒 Files selected for processing (1)
internal/controller/user_controller.go
| }, func(res constantTimeRes) { | ||
| c.JSON(res.Code, res.Res) | ||
| }, time.Millisecond*45) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Hardcoded timing delay is brittle against user enumeration.
While this PR aims to prevent user enumeration via a constant-time response, hardcoding 45ms is brittle. The actual time taken by CheckUserPassword (e.g., bcrypt hashing or LDAP binding) will vary significantly across different environments, hardware specifications, and server loads, or if password work-factors are updated in the future. Consequently, an attacker can still enumerate users by distinguishing between this fixed 45ms sleep and the actual, variable password verification time on the server.
To reliably mask the timing difference, consider performing a dummy password verification (e.g., computing a dummy hash with the same work factor or making a dummy LDAP bind) for the ErrUserNotFound case instead of using a hardcoded time.Sleep. If a sleep must be used, consider calibrating it dynamically based on the moving average of actual verification times.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/user_controller.go` around lines 113 - 115, Replace the
hardcoded time.Millisecond*45 delay in the constant-time response callback with
dummy password verification using the same work factor, or an equivalent dummy
LDAP bind, when CheckUserPassword reports ErrUserNotFound. Preserve the existing
c.JSON response behavior and ensure the missing-user path performs comparable
verification work without relying on a fixed sleep.
|
Yeah nevermind that's stupid. |
1 participant
Summary by CodeRabbit