🐛 bug: BasicAuth verifier for unknown users#4245
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWhen Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BasicAuthMiddleware as Middleware
participant Verifier as VerifierStore
participant UserDB as ConfiguredHashes
Client->>Middleware: send request with Authorization header
Middleware->>UserDB: lookup username -> hashed entry?
alt user found
Middleware->>Verifier: select user verifier for stored hash
else user not found
Middleware->>Verifier: use dummyVerify (selected by strongest configured hash)
end
Verifier->>Verifier: verify(password)
Verifier-->>Middleware: result (true/false)
Middleware-->>Client: allow if ok && result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4245 +/- ##
========================================
Coverage 91.17% 91.18%
========================================
Files 123 126 +3
Lines 12084 12341 +257
========================================
+ Hits 11018 11253 +235
- Misses 668 683 +15
- Partials 398 405 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces timing attack protection in the basic authentication middleware by ensuring a password verification step is always performed, even if the user does not exist. The review feedback suggests simplifying the logic for initializing the dummy verifier by removing the redundant hasDummy flag and streamlining the conditional check within the authorizer function to improve code clarity while maintaining the security fix.
gaby
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: df83d1f | Previous: f6ecd99 | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd (github.com/gofiber/fiber/v3/middleware/compress) - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce timing-based user enumeration in the BasicAuth middleware by ensuring password verification work is performed even when the username is not found, while keeping authorization outcomes unchanged.
Changes:
- Capture a “dummy” password verifier during
cfg.Usersparsing to use for unknown usernames. - Update the default
Authorizerto always run a verifier (real or dummy) and returnok && res. - Add initialization logic (
hasDummy) to avoid calling a nil verifier.
| dummyVerify := func(string) bool { return false } | ||
| hasDummy := false | ||
| for u, hpw := range cfg.Users { | ||
| v, err := parseHashedPassword(hpw) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| verifiers[u] = v | ||
| if !hasDummy { | ||
| dummyVerify = v | ||
| hasDummy = true | ||
| } |
There was a problem hiding this comment.
This was already addressed in a prior commit via sort.Strings(keys) — the dummy is now selected deterministically by iterating sorted keys and picking the strongest verifier. Additionally in e19d29b, the hasDummy flag was removed since the zero-value comparison handles the first-entry case naturally.
| dummyVerify := func(string) bool { return false } | ||
| hasDummy := false | ||
| for u, hpw := range cfg.Users { | ||
| v, err := parseHashedPassword(hpw) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| verifiers[u] = v | ||
| if !hasDummy { | ||
| dummyVerify = v | ||
| hasDummy = true | ||
| } |
There was a problem hiding this comment.
This was already addressed in a prior commit — fallbackDummyVerify performs a SHA-512 hash + subtle.ConstantTimeCompare for the empty-users case. In e19d29b, the preimage value ("fiber-basicauth-dummy") is now documented at the fallbackDummySHA512 declaration site.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/basicauth/config.go`:
- Around line 128-148: The current dummyVerify approach only exercises one
verifier and leaks timing differences across hash types; replace it by
collecting all parsed verifiers (from parseHashedPassword while iterating
cfg.Users into verifiers map) and in cfg.Authorizer always run every verifier
against the supplied password (or at least every verifier type/cost class) and
combine results so the function's runtime is independent of whether the username
exists: use the real user's verifier result as the authoritative boolean but
still execute the remaining verifiers to normalize cost; reference
parseHashedPassword, verifiers, dummyVerify (remove/repurpose), cfg.Authorizer
and cfg.Users when making this change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e3eb3bc-4692-4947-aa0c-1305ab70a95d
📒 Files selected for processing (1)
middleware/basicauth/config.go
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
middleware/basicauth/config.go (1)
132-143:⚠️ Potential issue | 🟠 MajorSingle dummy verifier still leaks mixed-hash timing differences.
Line 142 still runs exactly one verifier. In a config that mixes bcrypt, SHA-512, and SHA-256 credentials, unknown usernames take the selected dummy path while known users take their account-specific verifier, so cheaper hash types remain distinguishable from “missing user”. Please normalize work across all configured verifier classes on every request, and keep only the matched user’s result authoritative.
Also applies to: 173-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/basicauth/config.go` around lines 132 - 143, The current cfg.Authorizer closure uses a single dummyVerify and runs only one verifier per request, leaking timing differences between missing users and users with cheaper hash types; modify the Authorizer (the anonymous func assigned to cfg.Authorizer that uses verifiers, dummyVerify and the verifiers map returned by buildVerifiers) to always execute the full set of verifier functions for every configured hash class on every request (e.g., iterate all entries/types or a canonical list of verifier functions) and aggregate their work/latency, but only use the matched user's verifier result as the authoritative authentication outcome; ensure dummy stubs for each hash type are invoked when the username is unknown so execution time is constant across missing vs existing users (apply the same change pattern to the other authorizer block referenced in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@middleware/basicauth/config.go`:
- Around line 132-143: The current cfg.Authorizer closure uses a single
dummyVerify and runs only one verifier per request, leaking timing differences
between missing users and users with cheaper hash types; modify the Authorizer
(the anonymous func assigned to cfg.Authorizer that uses verifiers, dummyVerify
and the verifiers map returned by buildVerifiers) to always execute the full set
of verifier functions for every configured hash class on every request (e.g.,
iterate all entries/types or a canonical list of verifier functions) and
aggregate their work/latency, but only use the matched user's verifier result as
the authoritative authentication outcome; ensure dummy stubs for each hash type
are invoked when the username is unknown so execution time is constant across
missing vs existing users (apply the same change pattern to the other authorizer
block referenced in the file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e6b3f63-38ce-41d4-a6d6-74e1d260b53d
📒 Files selected for processing (2)
middleware/basicauth/basicauth_test.gomiddleware/basicauth/config.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
middleware/basicauth/config.go (1)
139-150:⚠️ Potential issue | 🟠 MajorSingle-verifier execution still leaves a timing side channel.
This path runs exactly one verifier per request:
dummyVerifyfor unknown users, or the matched verifier for known users. In mixed configs, existing SHA-256/SHA-512 users or lower-cost bcrypt users remain measurably different from the unknown-user path, so username enumeration is still possible. Normalize work across all configured verifier classes/costs for every request, and use only the matched user’s result as the auth decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/basicauth/config.go` around lines 139 - 150, The authorizer currently runs only one verifier (dummyVerify or the matched verifier) leaving timing differences; modify cfg.Authorizer (the closure using verifiers, dummyVerify, v(pass)) to call every verifier function on the supplied password on every request (iterate the verifiers map and also call dummyVerify) to equalize cost, but only use the matched user's verifier result as the authentication decision (capture the result of verifiers[user] when ok, or dummyVerify when user missing) and return ok && matchedResult; ensure other verifier results are discarded but executed to normalize timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@middleware/basicauth/config.go`:
- Around line 139-150: The authorizer currently runs only one verifier
(dummyVerify or the matched verifier) leaving timing differences; modify
cfg.Authorizer (the closure using verifiers, dummyVerify, v(pass)) to call every
verifier function on the supplied password on every request (iterate the
verifiers map and also call dummyVerify) to equalize cost, but only use the
matched user's verifier result as the authentication decision (capture the
result of verifiers[user] when ok, or dummyVerify when user missing) and return
ok && matchedResult; ensure other verifier results are discarded but executed to
normalize timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 58414d7a-da7e-457b-82ad-89e0e1a7803f
📒 Files selected for processing (2)
middleware/basicauth/basicauth_test.gomiddleware/basicauth/config.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
middleware/basicauth/config.go (1)
148-159:⚠️ Potential issue | 🟠 MajorSingle dummy verifier still leaves mixed-hash deployments enumerable.
This still executes only one verifier per request. In a mixed setup, unknown usernames will match the timing of the selected dummy, but they remain distinguishable from real accounts backed by cheaper verifiers or lower-cost bcrypt hashes. To actually close the username-existence side channel, the default path needs to run a normalized verifier workload for every request and use only the matched user’s result as the auth decision.
Also applies to: 189-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/basicauth/config.go` around lines 148 - 159, The current cfg.Authorizer uses a single dummyVerify and then returns ok && v(pass), which still leaks username existence via timing because only one verifier is executed or its result short-circuited; change cfg.Authorizer so it always executes a normalized verifier workload for every request (i.e., always call a verifier workload for the supplied password), but only use the matched user's verifier result as the final auth decision: lookup verify := verifiers[user], set fallbackVerify := dummyVerify and always call both (or run the verified workload then combine) such that timing is equalized, then return verifyResultOnlyIfUserExists (use verify result when ok is true, otherwise discard verify result but ensure dummyVerify was executed to match timing). Ensure this logic replaces the current use of v(pass) and the ok && res return so unknown usernames run the same amount of work as real ones.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@middleware/basicauth/config.go`:
- Around line 148-159: The current cfg.Authorizer uses a single dummyVerify and
then returns ok && v(pass), which still leaks username existence via timing
because only one verifier is executed or its result short-circuited; change
cfg.Authorizer so it always executes a normalized verifier workload for every
request (i.e., always call a verifier workload for the supplied password), but
only use the matched user's verifier result as the final auth decision: lookup
verify := verifiers[user], set fallbackVerify := dummyVerify and always call
both (or run the verified workload then combine) such that timing is equalized,
then return verifyResultOnlyIfUserExists (use verify result when ok is true,
otherwise discard verify result but ensure dummyVerify was executed to match
timing). Ensure this logic replaces the current use of v(pass) and the ok && res
return so unknown usernames run the same amount of work as real ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32cebc50-d43c-4c03-8203-f7f9a7f8e7a2
📒 Files selected for processing (2)
middleware/basicauth/basicauth_test.gomiddleware/basicauth/config.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/478fa5a0-8f00-4517-a078-381cae21db9e Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Addressed in 7963956. The BasicAuth dummy verifier is now selected deterministically from the strongest configured hash, empty-user configs use a fixed-work fallback verifier, and tests cover both behaviors. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
middleware/basicauth/config.go (1)
152-160:⚠️ Potential issue | 🟠 MajorSingle dummy verification still leaks timing in mixed-hash user sets.
Line 152–160 normalizes unknown users to one verifier only. If configured users mix bcrypt/SHA-512/SHA-256 (or bcrypt costs), timings can still distinguish unknown vs some existing usernames. Normalize against all configured verifier classes (or all verifiers) per request, while keeping the matched user result authoritative.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/basicauth/config.go` around lines 152 - 160, The current cfg.Authorizer uses a single dummyVerify for unknown users which can leak timing when verifiers vary; modify cfg.Authorizer to call all configured verifiers (the functions in the verifiers map) for every request to normalize timing, but still return the authoritative result of the matched user: locate cfg.Authorizer, remove the single dummyVerify substitution, iterate over verifiers (or verifier classes) and invoke each one with the provided pass to burn equal time, capture the matched user's verifier result separately (if user exists in verifiers use that result), and finally return ok && matchedResult so timing is normalized while the matched user's boolean determines acceptance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@middleware/basicauth/config.go`:
- Around line 152-160: The current cfg.Authorizer uses a single dummyVerify for
unknown users which can leak timing when verifiers vary; modify cfg.Authorizer
to call all configured verifiers (the functions in the verifiers map) for every
request to normalize timing, but still return the authoritative result of the
matched user: locate cfg.Authorizer, remove the single dummyVerify substitution,
iterate over verifiers (or verifier classes) and invoke each one with the
provided pass to burn equal time, capture the matched user's verifier result
separately (if user exists in verifiers use that result), and finally return ok
&& matchedResult so timing is normalized while the matched user's boolean
determines acceptance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 057d2466-5f04-4b8e-addf-8f0dbb1208c8
📒 Files selected for processing (2)
middleware/basicauth/basicauth_test.gomiddleware/basicauth/config.go
- Remove redundant hasDummy flag (zero-value verifierStrength always loses to real values) - Simplify Authorizer closure by directly reassigning verify variable - Document fallbackDummySHA512 preimage value at declaration site - Replace obfuscated hex byte array with string literal in test - Document mixed-hash timing limitation on buildVerifiers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ReneWerner87
left a comment
ReneWerner87
left a comment
There was a problem hiding this comment.
Review Summary
Pushed e19d29b addressing the open review comments. Changes:
Resolved:
- Removed
hasDummyflag (Gemini) — zero-valueverifierStrength{}naturally loses to any real strength, making the flag redundant - Simplified Authorizer closure (Gemini) — eliminated intermediate
vvariable, directly reassignverifywhen user not found - Deterministic dummy selection (Copilot) — was already fixed via
sort.Strings(keys)in prior commits - Empty users fallback (Copilot) — was already fixed via
fallbackDummyVerifywith SHA-512
Additionally:
- Documented
fallbackDummySHA512preimage value ("fiber-basicauth-dummy") at declaration site - Replaced obfuscated hex byte array in test with plain string literal for readability
- Added doc comment on
buildVerifiersexplicitly documenting the mixed-hash timing limitation as an accepted trade-off
Acknowledged limitation (CodeRabbit):
The single-dummy-verifier approach doesn't fully normalize timing in mixed-hash deployments. This is now documented. Running all verifier types per request would be prohibitively expensive and impractical for a middleware default.
ReneWerner87
deleted the
update-default-authorizer-for-password-verification
branch
Motivation
Description
dummyVerifyfunction from the first parsed hashed password duringverifiersconstruction to use for missing users.Authorizerto calldummyVerifywhen a username is not present, and otherwise call the real verifier, while returningok && resto preserve outcome.hasDummyflag and safe initialization to avoid nil verifier invocation and rely onparseHashedPasswordresults.