fix: use constant-time comparison in basic auth middleware - #322
fix: use constant-time comparison in basic auth middleware#322mvanhorn wants to merge 3 commits into
Conversation
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security of the basic authentication middleware by mitigating potential timing side-channel vulnerabilities. By switching to constant-time comparisons, the application no longer leaks information about credential matching through response latency, aligning with security best practices for authentication mechanisms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A secret hidden in the time, / A byte-by-byte and subtle crime. / We mask the wait with constant flow, / So attackers have no way to know. Footnotes
|
![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 updates the BasicAuth middleware to use constant-time comparison for usernames and passwords to prevent timing attacks, and adds a comprehensive test suite for the middleware. The review feedback highlights a remaining timing vulnerability where subtle.ConstantTimeCompare can leak length information if the input lengths differ, and suggests hashing the inputs with SHA-256 prior to comparison to fully mitigate the issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "crypto/subtle" | ||
| "net/http" |
There was a problem hiding this comment.
To support secure constant-time comparison of arbitrary-length strings without leaking length information, we need to hash the inputs using SHA-256 first. Please import the crypto/sha256 package.
| "crypto/subtle" | |
| "net/http" | |
| "crypto/sha256" | |
| "crypto/subtle" | |
| "net/http" |
| usernameMatches := subtle.ConstantTimeCompare([]byte(username), []byte(u.Username)) | ||
| passwordMatches := subtle.ConstantTimeCompare([]byte(password), []byte(u.Password)) | ||
| if usernameMatches&passwordMatches == 1 { |
There was a problem hiding this comment.
subtle.ConstantTimeCompare returns early if the lengths of the slices are different, which leaks the length of the expected username and password via timing. To prevent this timing side-channel leak, hash both the input and the expected values using SHA-256 before performing the constant-time comparison.
usernameHash := sha256.Sum256([]byte(username))
uUsernameHash := sha256.Sum256([]byte(u.Username))
passwordHash := sha256.Sum256([]byte(password))
uPasswordHash := sha256.Sum256([]byte(u.Password))
usernameMatches := subtle.ConstantTimeCompare(usernameHash[:], uUsernameHash[:])
passwordMatches := subtle.ConstantTimeCompare(passwordHash[:], uPasswordHash[:])
if usernameMatches&passwordMatches == 1 {
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBasic authentication now validates usernames and passwords with SHA-256 hashes and constant-time comparisons. Table-driven tests cover valid, invalid, empty, and multiple-user credentials. ChangesBasic authentication security
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
subtle.ConstantTimeCompare returns early when the slice lengths differ, so comparing the raw strings leaked the length of the configured username and password through timing. Hash both sides with SHA-256 first so the comparison always runs over 32 bytes. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
a316fa7 to
cf442d5
Compare
The previous revision hashed each credential with SHA256 before subtle.ConstantTimeCompare. That is the pattern in the net/http Request.BasicAuth documentation, and it was never storing a digest, but CodeQL cannot distinguish length-equalisation from password storage and reported two high-severity 'weak hashing algorithm on sensitive data' alerts against it. Compare keyed tags instead: a 32-byte per-process random key feeds an HMAC-SHA256 over each value, and hmac.Equal does the constant-time comparison. This keeps the two properties the original was after, no timing signal and no length leak, and adds one the bare digest lacked, since an attacker who learns a tag cannot recompute it without the key. The key is generated at startup, never persisted and never leaves the process. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Bumping this one. Both bot findings are addressed and every check is green. The gemini note about Ready for review whenever someone has a moment. |
2 participants
Changes
📝 Description
What changed?
The basic auth middleware compared the request username and password against configured credentials with
==, which short-circuits on the first mismatched byte. This change usescrypto/subtle.ConstantTimeComparefor both fields, computes both comparisons before combining them, and authorizes only when both match.Why is this change needed?
The
==comparison leaks timing information about how many leading bytes of the username or password match, which can help an attacker recover credentials byte by byte (OWASP A07, identification and authentication failures). Fixes #273.Dependencies
crypto/subtle)🧪 Testing
Test Coverage
Added
internal/httpapi/middleware/basic_auth_test.gocovering: valid credentials authorize, wrong password returns 401, wrong username returns 401, empty username/password returns 401, and a request matching the second configured credential authorizes.Commands run:
go build ./internal/httpapi/middleware/...passedgo test ./internal/httpapi/middleware/...passedPerformance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
⚙️ Configuration Changes
✅ Developer Checklist
Summary by CodeRabbit
Security Enhancements
Tests