fix: the basicauth middleware explicitly accepts sha... in basicauth.go#4257
Conversation
Automated security fix generated by Orbis Security AI
WalkthroughRefactors basic auth password verification to support only bcrypt hashes, removing SHA-256/SHA-512 support. Simplifies imports, adds centralized default configuration via ConfigDefault helper, and streamlines password parsing logic to bcrypt-only verification. ChangesBasic Auth Bcrypt Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![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)
middleware/basicauth/config.go (1)
233-241: 💤 Low valueStale comment and dead-code algorithm branch in
betterThanLine 233: the comment "prefers stronger hash families first (bcrypt > SHA-512 > SHA-256)" references removed hash types. Since
parseHashedPasswordnow only accepts bcrypt,s.algorithmandother.algorithmare alwaysverifierStrengthBcrypt, making thes.algorithm != other.algorithmbranch (lines 236–238) unreachable dead code.♻️ Proposed simplification
-// betterThan prefers stronger hash families first (bcrypt > SHA-512 > SHA-256) -// and uses the bcrypt cost as a tiebreaker within the same algorithm family. +// betterThan uses bcrypt cost to prefer higher-work-factor hashes. func (s verifierStrength) betterThan(other verifierStrength) bool { - if s.algorithm != other.algorithm { - return s.algorithm > other.algorithm - } - return s.cost > other.cost }🤖 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 `@middleware/basicauth/config.go` around lines 233 - 241, The comment and algorithm-comparison branch in verifierStrength.betterThan are stale because parseHashedPassword only produces verifierStrengthBcrypt; remove the unreachable s.algorithm != other.algorithm branch and simplify betterThan to compare only cost (e.g., return s.cost > other.cost), and update the comment to reflect that only bcrypt cost is used as the strength metric; keep references to verifierStrength and betterThan so the change is easy to locate.
🤖 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 `@middleware/basicauth/config.go`:
- Around line 243-251: Update the tests to reflect that parseHashedPassword now
rejects non-bcrypt hashes: in Test_parseHashedPassword change the
SHA-512/SHA-256/SHA-256-hex/SHA-256-b64 cases to expect an error rather than
require.NoError; in the test exercising buildVerifiers replace the
sha256Hash/sha512Hash inputs with bcrypt hashes or assert that buildVerifiers
returns an error for those SHA entries; and in Test_BasicAuth_HashVariants
replace SHA-based user entries passed to New (which calls configDefault →
buildVerifiers) with bcrypt-hashed credentials or change assertions to expect a
panic/error. Use the function names parseHashedPassword, buildVerifiers, New,
and configDefault to locate the code to update.
---
Nitpick comments:
In `@middleware/basicauth/config.go`:
- Around line 233-241: The comment and algorithm-comparison branch in
verifierStrength.betterThan are stale because parseHashedPassword only produces
verifierStrengthBcrypt; remove the unreachable s.algorithm != other.algorithm
branch and simplify betterThan to compare only cost (e.g., return s.cost >
other.cost), and update the comment to reflect that only bcrypt cost is used as
the strength metric; keep references to verifierStrength and betterThan so the
change is easy to locate.
🪄 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: e9d5c0b4-9606-47d5-8a15-56280de0c082
📒 Files selected for processing (1)
middleware/basicauth/config.go
| func parseHashedPassword(h string) (passwordVerifier, error) { | ||
| switch { | ||
| case strings.HasPrefix(h, "$2"): | ||
| hash := []byte(h) | ||
| return func(p string) bool { | ||
| return bcrypt.CompareHashAndPassword(hash, []byte(p)) == nil | ||
| }, nil | ||
| case strings.HasPrefix(h, "{SHA512}"): | ||
| b, err := base64.StdEncoding.DecodeString(h[len("{SHA512}"):]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode SHA512 password: %w", err) | ||
| } | ||
| return func(p string) bool { | ||
| sum := sha512.Sum512([]byte(p)) | ||
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | ||
| }, nil | ||
| case strings.HasPrefix(h, "{SHA256}"): | ||
| b, err := base64.StdEncoding.DecodeString(h[len("{SHA256}"):]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode SHA256 password: %w", err) | ||
| } | ||
| return func(p string) bool { | ||
| sum := sha256.Sum256([]byte(p)) | ||
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | ||
| }, nil | ||
| default: | ||
| b, err := hex.DecodeString(h) | ||
| if err != nil || len(b) != sha256.Size { | ||
| if b, err = base64.StdEncoding.DecodeString(h); err != nil { | ||
| return nil, fmt.Errorf("decode SHA256 password: %w", err) | ||
| } | ||
| if len(b) != sha256.Size { | ||
| return nil, ErrInvalidSHA256PasswordLength | ||
| } | ||
| } | ||
| return func(p string) bool { | ||
| sum := sha256.Sum256([]byte(p)) | ||
| return subtle.ConstantTimeCompare(sum[:], b) == 1 | ||
| }, nil | ||
| if !strings.HasPrefix(h, "$2") { | ||
| return nil, errors.New("basicauth: only bcrypt password hashes are supported; SHA-256 and SHA-512 are not suitable for password storage") | ||
| } | ||
| hash := []byte(h) | ||
| return func(p string) bool { | ||
| return bcrypt.CompareHashAndPassword(hash, []byte(p)) == nil | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Tests must be updated to reflect bcrypt-only support
parseHashedPassword now hard-rejects all non-$2 hashes with an error, but basicauth_test.go was not updated and contains multiple test cases that will fail:
Test_parseHashedPassword(snippet 1, lines 546–575): four of five cases (sha512,sha256,sha256-hex,sha256-b64) callrequire.NoError(t, err)— all will now get an error.buildVerifierstest (snippet 2, lines 580–597): passessha256Hash(...)andsha512Hash(...)entries tobuildVerifiersand then callsrequire.NoError(t, err)— this will now return an error.Test_BasicAuth_HashVariants(snippet 3, lines 611–638): passes SHA-512 / SHA-256 / SHA-256-hex hashes toNew(Config{Users: ...}), which callsconfigDefault→buildVerifiers→panicat runtime.
All three test functions must be updated: either remove or flip the SHA-256/SHA-512 cases to expect the new error, and replace the buildVerifiers/New() calls that use those hash types with bcrypt-only inputs.
🤖 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 `@middleware/basicauth/config.go` around lines 243 - 251, Update the tests to
reflect that parseHashedPassword now rejects non-bcrypt hashes: in
Test_parseHashedPassword change the SHA-512/SHA-256/SHA-256-hex/SHA-256-b64
cases to expect an error rather than require.NoError; in the test exercising
buildVerifiers replace the sha256Hash/sha512Hash inputs with bcrypt hashes or
assert that buildVerifiers returns an error for those SHA entries; and in
Test_BasicAuth_HashVariants replace SHA-based user entries passed to New (which
calls configDefault → buildVerifiers) with bcrypt-hashed credentials or change
assertions to expect a panic/error. Use the function names parseHashedPassword,
buildVerifiers, New, and configDefault to locate the code to update.
![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 restricts the basicauth middleware to only support bcrypt password hashes, removing previous support for SHA-256 and SHA-512 as they are unsuitable for secure password storage. The review feedback highlights that this is a significant breaking change requiring documentation in release notes. It also suggests improving the error message for unsupported hashes and ensuring the dummy verifier maintains timing attack resistance by consistently using bcrypt.
| if !strings.HasPrefix(h, "$2") { | ||
| return nil, errors.New("basicauth: only bcrypt password hashes are supported; SHA-256 and SHA-512 are not suitable for password storage") | ||
| } |
There was a problem hiding this comment.
This change introduces a significant breaking change by explicitly rejecting SHA-256 and SHA-512 hashes. While justified for security (SHA-2 is unsuitable for password storage), ensure this is documented in release notes. Also, consider a more general error message like basicauth: only bcrypt password hashes (starting with '$2') are supported. Additionally, ensure the dummy verifier for timing attack resistance deterministically uses the strongest available algorithm (bcrypt) to ensure consistent timing for unknown-user lookups.
References
- When selecting a dummy verifier for timing attack resistance in a system with multiple hashing algorithms, deterministically choose the strongest available algorithm to ensure consistent timing for unknown-user lookups.
|
@orbisai0security Closing for improper report of a security issue. It is up to the user to choose what to use. For example, Traefik supports bcrypt, md5, and sha1: https://doc.traefik.io/traefik/reference/routing-configuration/http/middlewares/basicauth/ |
|
Thanks for the clarification, that makes sense. I agree that this should not be framed as a direct Fiber security vulnerability if the hash format is an intentionally user-selected configuration. The current PR is also disruptive because it removes existing SHA-256/SHA-512 support and would break users relying on those formats. |
3 participants
Summary
Fix high severity security issue in
middleware/basicauth/basicauth.go.Vulnerability
V-001middleware/basicauth/basicauth.go:1Description: The BasicAuth middleware explicitly accepts SHA-256 hex-encoded passwords as a valid authentication format, and also supports SHA-512 prefixed hashes. SHA-256 and SHA-512 are general-purpose cryptographic hash functions designed for speed — they are not password hashing functions. They lack per-user salting and can be computed at billions of iterations per second on commodity GPU hardware. This makes any password stored in SHA-256 or SHA-512 format trivially vulnerable to offline brute-force and rainbow table attacks once the hash is obtained. By contrast, bcrypt (which is also accepted) is specifically designed for password storage with a configurable work factor that limits attack speed.
Changes
middleware/basicauth/config.goVerification
Automated security fix by OrbisAI Security