Skip to content

fix(basicauth): add salted PBKDF2 password hashing, deprecate SHA-256 HashPassword - #508

Open
FumingPower3925 wants to merge 2 commits into
mainfrom
fix/basicauth-pbkdf2
Open

fix(basicauth): add salted PBKDF2 password hashing, deprecate SHA-256 HashPassword#508
FumingPower3925 wants to merge 2 commits into
mainfrom
fix/basicauth-pbkdf2

Conversation

@FumingPower3925

@FumingPower3925 FumingPower3925 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What

middleware/basicauth.HashPassword produces an unsalted, fast SHA-256 digest, which is not a credential-storage hash: identical passwords share a digest and the digest is brute-forceable at GPU speed (CodeQL go/weak-sensitive-data-hashing, alert #9, config.go:151).

This PR adds a proper slow, salted KDF with zero new dependencies (Go 1.27 stdlib crypto/pbkdf2, already used by driver/postgres/protocol/scram.go):

  • HashPasswordPBKDF2(password) string — PBKDF2-HMAC-SHA256, 16-byte crypto/rand salt, PBKDF2Iterations = 600_000, 32-byte key, encoded as pbkdf2-sha256$<iter>$<salt-b64>$<hash-b64> (standard padded base64).
  • VerifyPassword(hash, password) bool — a ready-made Config.HashedUsersFunc. Dispatches on the pbkdf2-sha256$ tag: parses iterations / salt / hash strictly (exactly 4 fields; a decimal iteration count within the bounded window 600_000 <= iter <= 10_000_000; a salt of at least 16 bytes; a hash of exactly 32 bytes), re-derives and compares with crypto/subtle.ConstantTimeCompare. Anything without the tag is treated as a legacy hex SHA-256 digest from HashPassword and verified in constant time too, so existing stores keep authenticating. VerifyPassword is cost-uniform: every call performs exactly one derivation at the default iteration count (a burnPBKDF2 on the legacy, malformed, out-of-window and empty paths), so the format taken is not timing-distinguishable. The only residual is that a non-default stored iteration count is observable, as with bcrypt cost. The legacy candidate digest is derived through the deprecated HashPassword, so the PR adds no new SHA-256 sink for CodeQL.
  • Default wiringHashedUsers stores whose values are all pbkdf2-sha256 get VerifyPassword as the default HashedUsersFunc. Legacy or mixed stores without a HashedUsersFunc still panic at New, exactly as before (no fast-hash default is reinstated); the panic message now names the migration path.
  • HashPassword — behaviour frozen (still the bare hex digest, TestHashPasswordDeterministic / TestHashPasswordMatchesSHA256 untouched and green). The doc comment is now a proper Deprecated: notice (the old one said DEPRECATED:, which tooling does not recognise) pointing at HashPasswordPBKDF2 / VerifyPassword.
  • Docsdoc.go gains a "Migrating from HashPassword (SHA-256)" section with the three-step incremental migration (set HashedUsersFunc: VerifyPassword → re-hash entries → drop the explicit func once no legacy digests remain), plus a PBKDF2 usage snippet. ExampleNew_hashedUsers now uses HashPasswordPBKDF2; a new ExampleVerifyPassword shows a mixed store. There is no middleware/README.md in this repo, so the package doc is the README-equivalent.

Fail-first evidence

middleware/basicauth/pbkdf2_test.go was written first and run against unmodified main (9a8fc6a):

$ go test ./middleware/basicauth/ -run 'PBKDF2|VerifyPassword|HashedUsersLegacyWithoutFunc|MixedStore'
# github.com/goceleris/celeris/middleware/basicauth [github.com/goceleris/celeris/middleware/basicauth.test]
middleware/basicauth/pbkdf2_test.go:19:7: undefined: HashPasswordPBKDF2
middleware/basicauth/pbkdf2_test.go:20:6: undefined: VerifyPassword
middleware/basicauth/pbkdf2_test.go:26:7: undefined: HashPasswordPBKDF2
middleware/basicauth/pbkdf2_test.go:28:6: undefined: VerifyPassword
middleware/basicauth/pbkdf2_test.go:35:7: undefined: HashPasswordPBKDF2
middleware/basicauth/pbkdf2_test.go:47:13: undefined: PBKDF2Iterations
...
FAIL	github.com/goceleris/celeris/middleware/basicauth [build failed]

After the fix:

--- PASS: TestVerifyPasswordLegacySHA256 (0.00s)
--- PASS: TestHashedUsersLegacyWithoutFuncStillPanics (0.20s)
--- PASS: TestHashPasswordPBKDF2OutputFormat (1.01s)
--- PASS: TestHashPasswordPBKDF2RoundTrip (1.06s)
--- PASS: TestHashedUsersVerifyPasswordMixedStore (0.20s)
--- PASS: TestHashedUsersPBKDF2Default (0.20s)
--- PASS: TestVerifyPasswordTamperedRejected (0.21s)
--- PASS: TestHashPasswordPBKDF2WrongPasswordRejected (2.15s)
ok  	github.com/goceleris/celeris/middleware/basicauth	2.463s

$ go test -race -count=1 ./middleware/basicauth/
ok  	github.com/goceleris/celeris/middleware/basicauth	26.925s

One case surfaced during development and was removed from the wrong-password table on purpose: "secret\x00" verifies against the hash of "secret". That is inherent to PBKDF2 (the password is the HMAC key; HMAC zero-pads keys shorter than the block size), not a verifier bug; it is documented on HashPasswordPBKDF2, and RFC 7617 restricts Basic passwords to TEXT anyway.

Tests

  • round trip; wrong password rejected
  • output format parses: 4 $ fields, pbkdf2-sha256 tag, iterations == PBKDF2Iterations == 600000, 16-byte salt, 32-byte key; two hashes of the same password differ (fresh salt)
  • tampered hashes rejected: out-of-window (below 600k, above 10M, 2147483647, 1, 1000) / zero / negative / non-numeric iterations, salts of 1 and 15 bytes (16 and 17 accepted), flipped / truncated / invalid-base64 salt, flipped / truncated hash, missing / extra field, wrong algorithm tag, bare tag, empty string
  • legacy HashPassword hex digests still verify (incl. upper-case hex); truncated / overlong / non-hex rejected; HashPassword output unchanged
  • end-to-end middleware: all-PBKDF2 store with no HashedUsersFunc (default wiring); mixed store with HashedUsersFunc: VerifyPassword; mixed store without a func still panics

Test cost note: each derivation is 600k iterations (~0.2 s native, several seconds under -race), so the new tests share one pre-computed hash where the salt is irrelevant and run with t.Parallel(). Package -race time goes from ~1.4 s to ~27 s on a 10-core arm64 laptop; that is the price of exercising the real iteration count rather than a test hook in production code.

Review follow-up (fe621d5)

The adversarial review found two things the first commit got wrong, both fixed in fe621d5:

  • Unbounded, downgradable parameters. The parser honoured any positive 31-bit iteration count and any non-empty salt, so a stored pbkdf2-sha256$2147483647$… entry was a per-request DoS and a $1$ entry a downgrade. The window is now pinned as literals with compile-time guards (minPBKDF2Iterations = 600_000, deliberately not an alias of the default so a future raise never invalidates stored hashes; maxPBKDF2Iterations = 10_000_000; minPBKDF2SaltLen = 16). Out-of-window hashes take the burn-then-false path, and a store that is auto-wired to VerifyPassword (no HashedUsersFunc) now fails fast: New() panics naming the malformed entry instead of returning 401 for that user forever.
  • Timing not uniform across formats. Only the malformed branch burned a derivation; the legacy hex path was a fast SHA-256, so the two formats were distinguishable. Every non-derivation path now runs burnPBKDF2, and pickDummyHash prefers a PBKDF2 entry with a deterministic username tiebreak so the unknown-user path is uniform as well. Cost samples (pbkdf2 / legacy / empty / hostile count) agree within 2% under -race.

Fail-first: against the first commit the cost-uniformity test stalled for over 300 s because the parser honoured 2,147,483,647 iterations. New tests: parser-window table, window-edge round trips, serial 10x cost-uniformity, out-of-window New() panic, TestPickDummyHash. gofmt, go vet, golangci-lint clean; go test -race ./middleware/basicauth/ green.

Checks

gofmt clean, go vet ./... clean, golangci-lint run ./middleware/basicauth/... 0 issues, go build ./... ok, go test -race ./middleware/basicauth/ green.

Fixes #503

… HashPassword

HashPassword produced an unsalted, fast SHA-256 digest — not a
credential-storage hash (CodeQL go/weak-sensitive-data-hashing).

Add HashPasswordPBKDF2 (stdlib crypto/pbkdf2, HMAC-SHA256, 16-byte
crypto/rand salt, 600000 iterations, 32-byte key) encoded as
pbkdf2-sha256$<iter>$<salt-b64>$<hash-b64>, and VerifyPassword, a
ready-made HashedUsersFunc that detects the format of each stored hash,
verifies pbkdf2-sha256 strings and legacy hex SHA-256 digests alike, and
compares with crypto/subtle.ConstantTimeCompare (malformed input still
burns a derivation so the error branch is not timing-distinguishable).

HashedUsers stores made entirely of pbkdf2-sha256 hashes get
VerifyPassword wired in by default; legacy or mixed stores without a
HashedUsersFunc still panic at New, as before. HashPassword keeps its
exact behaviour and gains a proper Deprecated: notice pointing at
HashPasswordPBKDF2; the package doc gains a migration section.

Fixes #503
Comment thread middleware/basicauth/pbkdf2.go Fixed
…uniform

Address two review findings on #508.

Parameter window. VerifyPassword honours the parameters carried by a
stored pbkdf2-sha256 hash only within 600,000 <= iterations <= 10,000,000
and a salt of at least 16 bytes: the values HashPasswordPBKDF2 has ever
emitted, pinned as literals so a future raise of the default keeps old
hashes valid, with a compile-time guard that the defaults sit inside the
window. The parser previously accepted any 31-bit count, so a stored
`pbkdf2-sha256$2147483647$...` cost ~10 minutes of CPU per verification
(for unknown users too, since applyDefaults feeds a real stored hash to
the verifier on a miss), and it accepted 1 iteration and 1-byte salts.
Out-of-window values now take the existing burn-then-false path, and an
auto-wired HashedUsers store that contains one panics at New naming the
entry instead of answering 401 to that user on every request.

Cost uniformity. Every VerifyPassword call performs exactly one PBKDF2
derivation: the legacy SHA-256 path (valid hex, malformed and "" alike)
burns a default-cost derivation first, so in a mixed store the stored
format is no longer recoverable from response time, as the
HashedUsersFunc contract requires. The legacy candidate digest goes
through the deprecated HashPassword rather than a second inline
sha256.Sum256, so no new weak-hash sink is introduced (CodeQL
go/weak-sensitive-data-hashing). applyDefaults picks the unknown-user
dummy hash deterministically (pbkdf2-sha256 entry preferred, ties broken
on username). Docs state the timing property precisely, including the
residual signal of a non-default iteration count.

Tests: parser window edges without paying for derivations, correct-key
hashes one step outside the window refused, New panics on out-of-window
entries, dummy-hash preference and tie-break, legacy verification as
parallel subtests, and a serial cost-uniformity check (pbkdf2, legacy,
empty and a hostile 2^31 count within 10x; measured within 2% under
-race). Against the pre-fix sources the window, panic, dummy and edge
tests fail and the cost check stalls on the hostile count until the test
timeout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

basicauth: HashPassword uses unsalted SHA-256 for credential hashing (CodeQL go/weak-sensitive-data-hashing, config.go:151)

2 participants