From 9992f6fecc2c705515e3bf733f370f371761dace Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 6 Sep 2026 01:09:52 +0200 Subject: [PATCH 1/2] fix(basicauth): add salted PBKDF2 password hashing, deprecate SHA-256 HashPassword MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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$$$, 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 --- middleware/basicauth/config.go | 42 +++-- middleware/basicauth/doc.go | 46 ++++- middleware/basicauth/example_test.go | 22 ++- middleware/basicauth/pbkdf2.go | 160 +++++++++++++++++ middleware/basicauth/pbkdf2_test.go | 258 +++++++++++++++++++++++++++ 5 files changed, 506 insertions(+), 22 deletions(-) create mode 100644 middleware/basicauth/pbkdf2.go create mode 100644 middleware/basicauth/pbkdf2_test.go diff --git a/middleware/basicauth/config.go b/middleware/basicauth/config.go index 323773b2..a9c3d874 100644 --- a/middleware/basicauth/config.go +++ b/middleware/basicauth/config.go @@ -35,15 +35,18 @@ type Config struct { // HashedUsers maps usernames to opaque hash strings. The format is // determined by HashedUsersFunc — bcrypt's $2y$..., argon2id's $argon2.., // scrypt, etc. HashedUsersFunc is REQUIRED whenever HashedUsers is - // non-empty; basicauth.New() panics otherwise. There is no built-in - // password-hash default because all general-purpose hashes (SHA-2, + // non-empty, with one exception: when every value was produced by + // [HashPasswordPBKDF2] (tag "pbkdf2-sha256$"), [VerifyPassword] is + // wired in automatically. basicauth.New() panics otherwise. There is + // no fast-hash default because all general-purpose hashes (SHA-2, // SHA-3, BLAKE2) are too fast to safely store credentials with. HashedUsers map[string]string // HashedUsersFunc receives the stored hash string and the plaintext - // candidate; returns true on match. Required when HashedUsers is set. - // Callers typically wrap bcrypt.CompareHashAndPassword or - // argon2.IDKey + subtle.ConstantTimeCompare. + // candidate; returns true on match. Required when HashedUsers is set + // (unless all hashes are pbkdf2-sha256, see HashedUsers). Callers + // typically pass [VerifyPassword] or wrap bcrypt.CompareHashAndPassword + // or argon2.IDKey + subtle.ConstantTimeCompare. // // IMPORTANT: The function MUST take constant time for any input, // including empty or invalid hash strings. For bcrypt, this means @@ -105,11 +108,18 @@ func applyDefaults(cfg Config) Config { } if cfg.Validator == nil && cfg.ValidatorWithContext == nil && len(cfg.HashedUsers) > 0 { if cfg.HashedUsersFunc == nil { - // SHA-256 is fast — adversaries can crack it on commodity GPUs - // at billions of guesses per second. The default has been - // removed so callers must wire bcrypt / scrypt / argon2 (or - // equivalent) explicitly. See package docs for an example. - panic("basicauth: HashedUsers requires HashedUsersFunc (use bcrypt or argon2; SHA-256 is not credential-grade)") + if !allPBKDF2(cfg.HashedUsers) { + // SHA-256 is fast — adversaries can crack it on commodity + // GPUs at billions of guesses per second. There is no + // fast-hash default: callers must wire VerifyPassword, + // bcrypt / scrypt / argon2 (or equivalent) explicitly. + // See package docs for the migration path. + panic("basicauth: HashedUsers requires HashedUsersFunc unless every hash is pbkdf2-sha256 " + + "(use HashPasswordPBKDF2 + VerifyPassword, bcrypt, or argon2; plain SHA-256 is not credential-grade)") + } + // Every hash is a slow, salted KDF we produced ourselves, so a + // built-in verifier is safe here. + cfg.HashedUsersFunc = VerifyPassword } hashCopy := make(map[string]string, len(cfg.HashedUsers)) for u, h := range cfg.HashedUsers { @@ -142,11 +152,13 @@ func hmacSHA256(key, data []byte) []byte { // HashPassword returns the hex-encoded SHA-256 hash of password. // -// DEPRECATED: SHA-256 is not credential-grade — adversaries can crack it -// at billions of guesses per second on commodity GPUs. Use bcrypt or -// argon2 with [Config.HashedUsersFunc] instead. This helper is retained -// for backwards-compatibility but may be removed in a future major -// release. +// Deprecated: an unsalted, fast SHA-256 digest is not a credential-storage +// hash — identical passwords share a digest and it is brute-forceable at +// GPU speed (CodeQL go/weak-sensitive-data-hashing, celeris#503). Use +// [HashPasswordPBKDF2] to produce new hashes; [VerifyPassword] accepts both +// formats so existing stores can migrate one entry at a time. This helper's +// behaviour is frozen for backwards-compatibility and it may be removed in +// a future major release. func HashPassword(password string) string { h := sha256.Sum256([]byte(password)) return hex.EncodeToString(h[:]) diff --git a/middleware/basicauth/doc.go b/middleware/basicauth/doc.go index 06f83c68..a3cca5b5 100644 --- a/middleware/basicauth/doc.go +++ b/middleware/basicauth/doc.go @@ -10,8 +10,9 @@ // Exactly one credential source is required; [New] panics otherwise: // - [Config].Users — plaintext map, auto-generates a constant-time HMAC validator. // - [Config].HashedUsers + [Config].HashedUsersFunc — opaque hash strings with -// a caller-supplied compare function (bcrypt, argon2id, scrypt, etc.). -// HashedUsersFunc is mandatory when HashedUsers is set. +// a compare function: the built-in [VerifyPassword], or a caller-supplied +// one (bcrypt, argon2id, scrypt, etc.). HashedUsersFunc may be omitted +// only when every hash was produced by [HashPasswordPBKDF2]. // - [Config].Validator — arbitrary func(user, pass string) bool. // - [Config].ValidatorWithContext — same, with request context access. // @@ -23,11 +24,48 @@ // }, // })) // +// Hashed credentials without a third-party KDF dependency: +// +// // Generate once (e.g. `go run` a tiny tool) and paste the string into +// // config; every call yields a different salt. +// hash := basicauth.HashPasswordPBKDF2("secret") +// // -> pbkdf2-sha256$600000$$ +// +// server.Use(basicauth.New(basicauth.Config{ +// HashedUsers: map[string]string{"admin": hash}, +// // HashedUsersFunc defaults to basicauth.VerifyPassword when every +// // hash is pbkdf2-sha256. +// })) +// // Use [UsernameFromContext] to retrieve the authenticated username downstream. // Set [Config].Skip or [Config].SkipPaths to bypass the middleware selectively. // -// Note: [HashPassword] (SHA-256) is deprecated and credential-grade only with a -// modern KDF. Use bcrypt or argon2 via [Config].HashedUsersFunc instead. +// # Migrating from HashPassword (SHA-256) +// +// [HashPassword] is deprecated: it produces an unsalted, fast SHA-256 digest, +// which is not a credential-storage hash (identical passwords collide and +// the digest is brute-forceable at GPU speed). Nothing breaks for existing +// deployments — HashPassword's output is unchanged and any HashedUsersFunc +// you already supply keeps working — but new hashes should come from +// [HashPasswordPBKDF2] (PBKDF2-HMAC-SHA256, random 16-byte salt, 600,000 +// iterations, 32-byte key; stdlib crypto/pbkdf2, no new dependencies). +// +// To migrate an existing HashedUsers store incrementally: +// +// 1. Set HashedUsersFunc to [VerifyPassword]. It detects the format of each +// stored hash — "pbkdf2-sha256$..." or a bare hex SHA-256 digest — and +// compares with crypto/subtle.ConstantTimeCompare either way, so mixed +// stores authenticate correctly. +// 2. Re-hash each user with HashPasswordPBKDF2 (at the next password +// change, or in one sweep if you hold the plaintexts) and replace the +// stored value. +// 3. Once no legacy digests remain, drop the explicit HashedUsersFunc — the +// default kicks in for all-PBKDF2 stores. Mixed or legacy-only stores +// without a HashedUsersFunc still panic at [New], by design. +// +// Verification costs one PBKDF2 derivation per request (hundreds of +// milliseconds at 600k iterations); keep a session or token layer in front +// of hot endpoints rather than lowering the count. // // # Documentation // diff --git a/middleware/basicauth/example_test.go b/middleware/basicauth/example_test.go index d6008006..04cbc802 100644 --- a/middleware/basicauth/example_test.go +++ b/middleware/basicauth/example_test.go @@ -27,15 +27,31 @@ func ExampleNew_validator() { } func ExampleNew_hashedUsers() { - // SHA-256 hashed passwords — avoids storing plaintext in source/config. + // PBKDF2-HMAC-SHA256 hashed passwords — avoids storing plaintext in + // source/config. In practice generate the strings once and paste them + // into config; HashedUsersFunc defaults to basicauth.VerifyPassword + // when every hash is pbkdf2-sha256. _ = basicauth.New(basicauth.Config{ HashedUsers: map[string]string{ - "admin": basicauth.HashPassword("secret"), - "user": basicauth.HashPassword("password"), + "admin": basicauth.HashPasswordPBKDF2("secret"), + "user": basicauth.HashPasswordPBKDF2("password"), }, }) } +func ExampleVerifyPassword() { + // Migrating a store that still holds legacy HashPassword digests: + // VerifyPassword accepts both formats, so entries can be re-hashed one + // at a time. + _ = basicauth.New(basicauth.Config{ + HashedUsers: map[string]string{ + "admin": basicauth.HashPasswordPBKDF2("secret"), + "legacy": "2bb80d537b1da3e38bd30361aa855686bde0eacd7162fef6a25fe97bf527a25b", // sha256("secret") + }, + HashedUsersFunc: basicauth.VerifyPassword, + }) +} + func ExampleNew_contextValidator() { // Context-aware validator for per-request auth decisions. _ = basicauth.New(basicauth.Config{ diff --git a/middleware/basicauth/pbkdf2.go b/middleware/basicauth/pbkdf2.go new file mode 100644 index 00000000..98ba00be --- /dev/null +++ b/middleware/basicauth/pbkdf2.go @@ -0,0 +1,160 @@ +package basicauth + +import ( + "crypto/pbkdf2" + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/base64" + "encoding/hex" + "strconv" + "strings" +) + +// PBKDF2Iterations is the PBKDF2-HMAC-SHA256 iteration count used by +// [HashPasswordPBKDF2]. 600,000 is the OWASP (2023+) recommendation for +// PBKDF2-HMAC-SHA256 credential storage. +const PBKDF2Iterations = 600_000 + +const ( + // pbkdf2Tag is the algorithm tag that opens every HashPasswordPBKDF2 + // string; VerifyPassword dispatches on it. + pbkdf2Tag = "pbkdf2-sha256" + // pbkdf2SaltLen is the salt length in bytes generated by HashPasswordPBKDF2. + pbkdf2SaltLen = 16 + // pbkdf2KeyLen is the derived-key length in bytes; fixed at the SHA-256 + // digest size so a truncated (weaker) stored hash is rejected outright. + pbkdf2KeyLen = sha256.Size +) + +// HashPasswordPBKDF2 derives a salted, slow credential hash of password +// suitable for storing in [Config].HashedUsers. +// +// The output is PBKDF2-HMAC-SHA256 over a fresh 16-byte crypto/rand salt +// with [PBKDF2Iterations] iterations and a 32-byte derived key, encoded as +// +// pbkdf2-sha256$$$ +// +// (standard, padded base64). Every call produces a different string for the +// same password because the salt is random; compare with [VerifyPassword], +// never with ==. The function panics only if crypto/rand fails, which is +// treated as unrecoverable (as in the rest of this package). +// +// As with every PBKDF2 implementation, the password is the HMAC key, so a +// password shorter than the SHA-256 block size is equivalent to itself with +// trailing NUL bytes appended. RFC 7617 restricts Basic passwords to TEXT +// (no control characters), so this has no practical effect here. +func HashPasswordPBKDF2(password string) string { + salt := make([]byte, pbkdf2SaltLen) + if _, err := rand.Read(salt); err != nil { + panic("basicauth: crypto/rand failed: " + err.Error()) + } + key, err := pbkdf2.Key(sha256.New, password, salt, PBKDF2Iterations, pbkdf2KeyLen) + if err != nil { + // Only reachable under FIPS-140 mode with parameters below the + // module's floor; ours are well above it. + panic("basicauth: pbkdf2: " + err.Error()) + } + return pbkdf2Tag + "$" + strconv.Itoa(PBKDF2Iterations) + "$" + + base64.StdEncoding.EncodeToString(salt) + "$" + + base64.StdEncoding.EncodeToString(key) +} + +// VerifyPassword reports whether password matches the stored hash. It is +// a ready-made [Config].HashedUsersFunc that understands both hash formats +// this package has ever produced: +// +// - pbkdf2-sha256$$$ from [HashPasswordPBKDF2] +// (preferred); +// - a bare hex SHA-256 digest from the deprecated [HashPassword], kept so +// existing deployments keep authenticating while they migrate. +// +// The comparison uses [subtle.ConstantTimeCompare]. A malformed hash still +// performs a derivation before returning false, so the format-error branch +// is not distinguishable by timing from a wrong password. Per-request cost +// is one PBKDF2 derivation (hundreds of milliseconds at 600k iterations), +// which is the point: cache authenticated sessions upstream if the +// endpoint is hot. +func VerifyPassword(hash, password string) bool { + if isPBKDF2Hash(hash) { + return verifyPBKDF2(hash, password) + } + return verifyLegacySHA256(hash, password) +} + +// isPBKDF2Hash reports whether hash carries the HashPasswordPBKDF2 tag. +func isPBKDF2Hash(hash string) bool { + return strings.HasPrefix(hash, pbkdf2Tag+"$") +} + +// verifyPBKDF2 parses a pbkdf2-sha256$$$ string and +// checks password against it in constant time. +func verifyPBKDF2(hash, password string) bool { + iter, salt, want, ok := parsePBKDF2(hash) + if !ok { + // Burn the same work a well-formed hash would, then fail. + var dummySalt [pbkdf2SaltLen]byte + var dummy [pbkdf2KeyLen]byte + got, _ := pbkdf2.Key(sha256.New, password, dummySalt[:], PBKDF2Iterations, pbkdf2KeyLen) + _ = subtle.ConstantTimeCompare(got, dummy[:]) + return false + } + got, err := pbkdf2.Key(sha256.New, password, salt, iter, pbkdf2KeyLen) + if err != nil { + return false + } + return subtle.ConstantTimeCompare(got, want) == 1 +} + +// parsePBKDF2 splits and decodes a HashPasswordPBKDF2 string. It rejects +// anything that is not exactly four fields, a positive decimal iteration +// count, a non-empty base64 salt, and a base64 hash of exactly +// pbkdf2KeyLen bytes. +func parsePBKDF2(hash string) (iter int, salt, key []byte, ok bool) { + parts := strings.Split(hash, "$") + if len(parts) != 4 || parts[0] != pbkdf2Tag { + return 0, nil, nil, false + } + // ParseUint (not Atoi) refuses signs and is bounded to 31 bits so a + // hostile iteration count can neither go negative nor overflow int + // on 32-bit targets. + n, err := strconv.ParseUint(parts[1], 10, 31) + if err != nil || n == 0 { + return 0, nil, nil, false + } + salt, err = base64.StdEncoding.DecodeString(parts[2]) + if err != nil || len(salt) == 0 { + return 0, nil, nil, false + } + key, err = base64.StdEncoding.DecodeString(parts[3]) + if err != nil || len(key) != pbkdf2KeyLen { + return 0, nil, nil, false + } + return int(n), salt, key, true +} + +// verifyLegacySHA256 checks password against a bare hex SHA-256 digest as +// produced by the deprecated HashPassword. Constant-time per the +// HashedUsersFunc contract, including on malformed input. +func verifyLegacySHA256(hash, password string) bool { + want, err := hex.DecodeString(hash) + got := sha256.Sum256([]byte(password)) + if err != nil || len(want) != sha256.Size { + var dummy [sha256.Size]byte + _ = subtle.ConstantTimeCompare(got[:], dummy[:]) + return false + } + return subtle.ConstantTimeCompare(got[:], want) == 1 +} + +// allPBKDF2 reports whether every stored hash carries the pbkdf2-sha256 +// tag — the only shape for which applyDefaults may wire VerifyPassword in +// without an explicit HashedUsersFunc. +func allPBKDF2(hashes map[string]string) bool { + for _, h := range hashes { + if !isPBKDF2Hash(h) { + return false + } + } + return true +} diff --git a/middleware/basicauth/pbkdf2_test.go b/middleware/basicauth/pbkdf2_test.go new file mode 100644 index 00000000..2e36b97f --- /dev/null +++ b/middleware/basicauth/pbkdf2_test.go @@ -0,0 +1,258 @@ +package basicauth + +import ( + "encoding/base64" + "encoding/hex" + "strconv" + "strings" + "sync" + "testing" + + "github.com/goceleris/celeris" + "github.com/goceleris/celeris/celeristest" + + "github.com/goceleris/celeris/middleware/internal/testutil" +) + +// --- HashPasswordPBKDF2 / VerifyPassword (celeris#503) --- +// +// Every derivation below costs 600k HMAC-SHA256 iterations (~0.2s native, +// seconds under -race), so the tests share one pre-computed hash where the +// salt does not matter, keep their case tables tight, and run in parallel. + +// secretHash returns a single pbkdf2-sha256 hash of "secret", computed once +// per test binary. +var secretHash = sync.OnceValue(func() string { return HashPasswordPBKDF2("secret") }) + +func TestHashPasswordPBKDF2RoundTrip(t *testing.T) { + t.Parallel() + h := secretHash() + if !VerifyPassword(h, "secret") { + t.Fatalf("VerifyPassword rejected the password it was derived from: %q", h) + } +} + +func TestHashPasswordPBKDF2WrongPasswordRejected(t *testing.T) { + t.Parallel() + h := secretHash() + // Note: "secret\x00" is deliberately absent. PBKDF2 feeds the password + // in as the HMAC key, and HMAC zero-pads keys shorter than the block + // size, so a trailing NUL is a documented PBKDF2 equivalence rather + // than a verifier bug. + for _, pw := range []string{"wrong", "", "Secret"} { + if VerifyPassword(h, pw) { + t.Fatalf("VerifyPassword accepted wrong password %q for %q", pw, h) + } + } +} + +func TestHashPasswordPBKDF2OutputFormat(t *testing.T) { + t.Parallel() + h := secretHash() + parts := strings.Split(h, "$") + if len(parts) != 4 { + t.Fatalf("want 4 $-separated fields, got %d in %q", len(parts), h) + } + if parts[0] != "pbkdf2-sha256" { + t.Fatalf("algorithm tag: got %q, want %q", parts[0], "pbkdf2-sha256") + } + iter, err := strconv.Atoi(parts[1]) + if err != nil { + t.Fatalf("iterations field %q not an integer: %v", parts[1], err) + } + if iter != PBKDF2Iterations || PBKDF2Iterations != 600000 { + t.Fatalf("iterations: got %d (const %d), want 600000", iter, PBKDF2Iterations) + } + salt, err := base64.StdEncoding.DecodeString(parts[2]) + if err != nil { + t.Fatalf("salt field %q not base64: %v", parts[2], err) + } + if len(salt) != 16 { + t.Fatalf("salt length: got %d bytes, want 16", len(salt)) + } + key, err := base64.StdEncoding.DecodeString(parts[3]) + if err != nil { + t.Fatalf("hash field %q not base64: %v", parts[3], err) + } + if len(key) != 32 { + t.Fatalf("derived key length: got %d bytes, want 32", len(key)) + } + + // A salted hash must not be deterministic: two hashes of the same + // password share nothing but the tag and iteration count. + h2 := HashPasswordPBKDF2("secret") + if h2 == h { + t.Fatalf("two HashPasswordPBKDF2 calls produced identical output (unsalted?): %q", h) + } + if strings.Split(h2, "$")[2] == parts[2] { + t.Fatalf("salt reused across calls: %q", parts[2]) + } +} + +func TestVerifyPasswordTamperedRejected(t *testing.T) { + t.Parallel() + h := secretHash() + parts := strings.Split(h, "$") + if len(parts) != 4 { + t.Fatalf("want 4 fields, got %d in %q", len(parts), h) + } + join := func(tag, iter, salt, key string) string { + return tag + "$" + iter + "$" + salt + "$" + key + } + flipFirst := func(s string) string { + // Swap the first character for a different valid base64 char so + // the field still decodes but to different bytes. + if s[0] == 'A' { + return "B" + s[1:] + } + return "A" + s[1:] + } + saltRaw, _ := base64.StdEncoding.DecodeString(parts[2]) + shortSalt := base64.StdEncoding.EncodeToString(saltRaw[:8]) + keyRaw, _ := base64.StdEncoding.DecodeString(parts[3]) + shortKey := base64.StdEncoding.EncodeToString(keyRaw[:16]) + + cases := map[string]string{ + "fewer iterations": join(parts[0], "1000", parts[2], parts[3]), + "more iterations": join(parts[0], "600001", parts[2], parts[3]), + "zero iterations": join(parts[0], "0", parts[2], parts[3]), + "negative iterations": join(parts[0], "-600000", parts[2], parts[3]), + "non-numeric iterations": join(parts[0], "abc", parts[2], parts[3]), + "flipped salt": join(parts[0], parts[1], flipFirst(parts[2]), parts[3]), + "truncated salt": join(parts[0], parts[1], shortSalt, parts[3]), + "invalid base64 salt": join(parts[0], parts[1], "!!!!", parts[3]), + "flipped hash": join(parts[0], parts[1], parts[2], flipFirst(parts[3])), + "truncated hash": join(parts[0], parts[1], parts[2], shortKey), + "missing field": parts[0] + "$" + parts[1] + "$" + parts[2], + "extra field": h + "$extra", + "wrong algorithm tag": join("pbkdf2-sha512", parts[1], parts[2], parts[3]), + "tag only": "pbkdf2-sha256$", + "empty": "", + } + for name, tampered := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() // most cases cost a full 600k-iteration derivation + if VerifyPassword(tampered, "secret") { + t.Fatalf("VerifyPassword accepted tampered hash %q", tampered) + } + }) + } +} + +func TestVerifyPasswordLegacySHA256(t *testing.T) { + t.Parallel() + legacy := HashPassword("secret") + if !VerifyPassword(legacy, "secret") { + t.Fatalf("legacy sha256 hash %q no longer verifies", legacy) + } + if VerifyPassword(legacy, "wrong") { + t.Fatal("legacy sha256 hash accepted wrong password") + } + // hex is case-insensitive; upper-case hex is the same digest. + if !VerifyPassword(strings.ToUpper(legacy), "secret") { + t.Fatal("upper-case legacy hex hash should verify") + } + for _, bad := range []string{ + legacy[:63], // truncated + legacy + "00", // too long + "zz" + legacy[2:], // not hex + } { + if VerifyPassword(bad, "secret") { + t.Fatalf("malformed legacy hash %q accepted", bad) + } + } + // HashPassword's behaviour is unchanged: still the plain hex digest. + if _, err := hex.DecodeString(legacy); err != nil || len(legacy) != 64 { + t.Fatalf("HashPassword output changed: %q", legacy) + } +} + +// TestHashedUsersPBKDF2Default: a HashedUsers map containing only +// pbkdf2-sha256 hashes no longer needs an explicit HashedUsersFunc — +// VerifyPassword is wired in by default. +func TestHashedUsersPBKDF2Default(t *testing.T) { + t.Parallel() + mw := New(Config{ + HashedUsers: map[string]string{"admin": secretHash()}, + }) + tests := []struct { + name string + user string + pass string + wantCode int + }{ + {"valid", "admin", "secret", 200}, + {"wrong password", "admin", "wrong", 401}, + {"unknown user", "nobody", "secret", 401}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var storedUser string + handler := func(c *celeris.Context) error { + storedUser = UsernameFromContext(c) + return c.String(200, "ok") + } + rec, err := testutil.RunChain(t, []celeris.HandlerFunc{mw, handler}, "GET", "/", + celeristest.WithBasicAuth(tt.user, tt.pass)) + if tt.wantCode == 200 { + testutil.AssertNoError(t, err) + testutil.AssertStatus(t, rec, 200) + if storedUser != tt.user { + t.Fatalf("stored user: got %q, want %q", storedUser, tt.user) + } + } else { + testutil.AssertHTTPError(t, err, tt.wantCode) + } + }) + } +} + +// Legacy sha256 hashes (or anything else) without a HashedUsersFunc must +// still panic — the default is only safe when every hash is a slow KDF. +func TestHashedUsersLegacyWithoutFuncStillPanics(t *testing.T) { + t.Parallel() + defer func() { + if recover() == nil { + t.Fatal("expected panic: mixed legacy sha256 + pbkdf2 store without HashedUsersFunc") + } + }() + New(Config{HashedUsers: map[string]string{ + "admin": secretHash(), + "old": HashPassword("legacy"), + }}) +} + +// Mixed stores migrate incrementally: VerifyPassword accepts both formats. +func TestHashedUsersVerifyPasswordMixedStore(t *testing.T) { + t.Parallel() + mw := New(Config{ + HashedUsers: map[string]string{ + "new": secretHash(), + "old": HashPassword("legacy"), + }, + HashedUsersFunc: VerifyPassword, + }) + for _, tt := range []struct { + name, user, pass string + wantCode int + }{ + {"pbkdf2 entry", "new", "secret", 200}, + {"legacy entry", "old", "legacy", 200}, + {"pbkdf2 entry wrong password", "new", "legacy", 401}, + {"legacy entry wrong password", "old", "secret", 401}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + handler := func(c *celeris.Context) error { return c.String(200, "ok") } + rec, err := testutil.RunChain(t, []celeris.HandlerFunc{mw, handler}, "GET", "/", + celeristest.WithBasicAuth(tt.user, tt.pass)) + if tt.wantCode == 200 { + testutil.AssertNoError(t, err) + testutil.AssertStatus(t, rec, 200) + } else { + testutil.AssertHTTPError(t, err, tt.wantCode) + } + }) + } +} From fe621d541d7a3cc8c5f522d01d2df36e7c74a317 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 6 Sep 2026 02:03:09 +0200 Subject: [PATCH 2/2] fix(basicauth): bound PBKDF2 parameters and make VerifyPassword cost-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. --- middleware/basicauth/config.go | 58 +++-- middleware/basicauth/doc.go | 15 +- middleware/basicauth/pbkdf2.go | 136 +++++++++--- middleware/basicauth/pbkdf2_test.go | 314 +++++++++++++++++++++++++--- 4 files changed, 455 insertions(+), 68 deletions(-) diff --git a/middleware/basicauth/config.go b/middleware/basicauth/config.go index a9c3d874..bca7e0c7 100644 --- a/middleware/basicauth/config.go +++ b/middleware/basicauth/config.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "crypto/subtle" "encoding/hex" + "strconv" "github.com/goceleris/celeris" ) @@ -35,11 +36,13 @@ type Config struct { // HashedUsers maps usernames to opaque hash strings. The format is // determined by HashedUsersFunc — bcrypt's $2y$..., argon2id's $argon2.., // scrypt, etc. HashedUsersFunc is REQUIRED whenever HashedUsers is - // non-empty, with one exception: when every value was produced by - // [HashPasswordPBKDF2] (tag "pbkdf2-sha256$"), [VerifyPassword] is - // wired in automatically. basicauth.New() panics otherwise. There is - // no fast-hash default because all general-purpose hashes (SHA-2, - // SHA-3, BLAKE2) are too fast to safely store credentials with. + // non-empty, with one exception: when every value carries the + // "pbkdf2-sha256$" tag of [HashPasswordPBKDF2] and parses within the + // window [VerifyPassword] accepts, VerifyPassword is wired in + // automatically. basicauth.New() panics otherwise — naming the entry + // when a tagged value is malformed or out of window. There is no + // fast-hash default because all general-purpose hashes (SHA-2, SHA-3, + // BLAKE2) are too fast to safely store credentials with. HashedUsers map[string]string // HashedUsersFunc receives the stored hash string and the plaintext @@ -53,6 +56,8 @@ type Config struct { // pre-computing a dummy hash (via bcrypt.GenerateFromPassword) and // comparing against it for unknown users, rather than letting // bcrypt.CompareHashAndPassword fail instantly on an empty hash. + // [VerifyPassword] meets this: it performs one PBKDF2 derivation for + // every input, whatever format the stored hash is in. HashedUsersFunc func(hash, password string) bool // Realm is the authentication realm. Default: "Restricted". @@ -117,19 +122,25 @@ func applyDefaults(cfg Config) Config { panic("basicauth: HashedUsers requires HashedUsersFunc unless every hash is pbkdf2-sha256 " + "(use HashPasswordPBKDF2 + VerifyPassword, bcrypt, or argon2; plain SHA-256 is not credential-grade)") } - // Every hash is a slow, salted KDF we produced ourselves, so a - // built-in verifier is safe here. + if u, bad := malformedPBKDF2Entry(cfg.HashedUsers); bad { + // A tagged value VerifyPassword cannot honour would 401 + // that user on every request; fail at startup instead and + // say which entry. + panic("basicauth: HashedUsers entry for " + strconv.Quote(u) + " is not a valid pbkdf2-sha256 hash " + + "(want pbkdf2-sha256$$$ with " + + strconv.Itoa(minPBKDF2Iterations) + " <= iter <= " + strconv.Itoa(maxPBKDF2Iterations) + + ", salt of at least " + strconv.Itoa(minPBKDF2SaltLen) + " bytes, hash of exactly " + + strconv.Itoa(pbkdf2KeyLen) + " bytes)") + } + // Every hash is a slow, salted KDF inside the window we + // enforce, so a built-in verifier is safe here. cfg.HashedUsersFunc = VerifyPassword } hashCopy := make(map[string]string, len(cfg.HashedUsers)) for u, h := range cfg.HashedUsers { hashCopy[u] = h } - var dummyHash string - for _, h := range hashCopy { - dummyHash = h - break - } + dummyHash := pickDummyHash(hashCopy) verifyFn := cfg.HashedUsersFunc cfg.Validator = func(user, pass string) bool { h, ok := hashCopy[user] @@ -143,6 +154,29 @@ func applyDefaults(cfg Config) Config { return cfg } +// pickDummyHash chooses the stored hash the auto-generated Validator +// verifies unknown usernames against. It is a real stored value so a +// caller-supplied verifier (bcrypt, argon2) pays its genuine cost on a +// miss. A pbkdf2-sha256 entry is preferred when the store is mixed — +// VerifyPassword costs the same for every format, but a custom verifier +// that dispatches on the tag may not — and ties break on username so the +// choice does not depend on map-iteration order. Returns "" for an empty +// map. +func pickDummyHash(hashes map[string]string) string { + var best, bestUser string + bestRank := -1 + for u, h := range hashes { + rank := 0 + if isPBKDF2Hash(h) { + rank = 1 + } + if rank > bestRank || (rank == bestRank && u < bestUser) { + best, bestUser, bestRank = h, u, rank + } + } + return best +} + // hmacSHA256 computes HMAC-SHA256(key, data) and returns the 32-byte tag. func hmacSHA256(key, data []byte) []byte { mac := hmac.New(sha256.New, key) diff --git a/middleware/basicauth/doc.go b/middleware/basicauth/doc.go index a3cca5b5..65e02884 100644 --- a/middleware/basicauth/doc.go +++ b/middleware/basicauth/doc.go @@ -55,7 +55,12 @@ // 1. Set HashedUsersFunc to [VerifyPassword]. It detects the format of each // stored hash — "pbkdf2-sha256$..." or a bare hex SHA-256 digest — and // compares with crypto/subtle.ConstantTimeCompare either way, so mixed -// stores authenticate correctly. +// stores authenticate correctly. Every call costs one PBKDF2 derivation +// whatever the format (legacy entries and unknown users burn a +// default-cost one), so response time does not reveal which users have +// migrated. The one cost-related signal left is a non-default iteration +// count in a pbkdf2-sha256 entry, as with any tunable KDF; hashes from +// HashPasswordPBKDF2 all carry the default. // 2. Re-hash each user with HashPasswordPBKDF2 (at the next password // change, or in one sweep if you hold the plaintexts) and replace the // stored value. @@ -64,8 +69,12 @@ // without a HashedUsersFunc still panic at [New], by design. // // Verification costs one PBKDF2 derivation per request (hundreds of -// milliseconds at 600k iterations); keep a session or token layer in front -// of hot endpoints rather than lowering the count. +// milliseconds at 600k iterations) for every entry, legacy digests +// included; keep a session or token layer in front of hot endpoints rather +// than lowering the count. Stored pbkdf2-sha256 parameters are honoured +// within 600,000–10,000,000 iterations and a salt of at least 16 bytes; a +// value outside that window never verifies and, in an auto-wired store, +// makes [New] panic naming the entry. // // # Documentation // diff --git a/middleware/basicauth/pbkdf2.go b/middleware/basicauth/pbkdf2.go index 98ba00be..c2aacd24 100644 --- a/middleware/basicauth/pbkdf2.go +++ b/middleware/basicauth/pbkdf2.go @@ -25,6 +25,42 @@ const ( // pbkdf2KeyLen is the derived-key length in bytes; fixed at the SHA-256 // digest size so a truncated (weaker) stored hash is rejected outright. pbkdf2KeyLen = sha256.Size + + // VerifyPassword honours the parameters carried by a stored hash only + // inside the window below. HashPasswordPBKDF2 itself always emits + // PBKDF2Iterations and pbkdf2SaltLen; the window exists to bound what + // a hostile or mistyped stored value can make the verifier do — a + // stored hash reaches the verifier on every request for that user and, + // through applyDefaults' dummy hash, on every request for an unknown + // user too — while leaving headroom for hashes produced elsewhere at a + // higher cost. + // + // minPBKDF2Iterations refuses downgrades: an entry below the count this + // package emits would verify at a fraction of the intended cost, which + // is exactly the fast-hash storage the package refuses to default to. + // It is a literal, not an alias of PBKDF2Iterations: if the default is + // ever raised, this stays at the lowest count ever emitted so hashes + // already sitting in stores keep verifying. + minPBKDF2Iterations = 600_000 + // maxPBKDF2Iterations caps the CPU one verification can burn. Cost is + // linear in the count (~0.2 s at the default, ~3 s at this cap on a + // 2026 laptop); the 31-bit count the parser used to accept would have + // cost ~10 minutes per request. + maxPBKDF2Iterations = 10_000_000 + // minPBKDF2SaltLen refuses salts shorter than the 128 bits this package + // emits (NIST SP 800-132 §5.1). Same rule as the iteration floor: a + // literal pinned at the shortest salt ever emitted. + minPBKDF2SaltLen = 16 +) + +// Compile-time guard: HashPasswordPBKDF2's own parameters must sit inside +// the window VerifyPassword enforces, or the default wiring would reject +// every hash it produces. A negative untyped constant does not convert to +// uint, so a bad edit fails to build. +const ( + _ = uint(PBKDF2Iterations - minPBKDF2Iterations) + _ = uint(maxPBKDF2Iterations - PBKDF2Iterations) + _ = uint(pbkdf2SaltLen - minPBKDF2SaltLen) ) // HashPasswordPBKDF2 derives a salted, slow credential hash of password @@ -65,16 +101,30 @@ func HashPasswordPBKDF2(password string) string { // this package has ever produced: // // - pbkdf2-sha256$$$ from [HashPasswordPBKDF2] -// (preferred); +// (preferred). The stored parameters are honoured only within a fixed +// window — 600,000 ≤ iterations ≤ 10,000,000, salt of at least 16 +// bytes, key of exactly 32 bytes — so a stored value can neither +// downgrade the derivation below the cost this package emits nor make +// it cost minutes of CPU. Anything outside the window fails, after the +// same work as below; // - a bare hex SHA-256 digest from the deprecated [HashPassword], kept so // existing deployments keep authenticating while they migrate. // -// The comparison uses [subtle.ConstantTimeCompare]. A malformed hash still -// performs a derivation before returning false, so the format-error branch -// is not distinguishable by timing from a wrong password. Per-request cost -// is one PBKDF2 derivation (hundreds of milliseconds at 600k iterations), -// which is the point: cache authenticated sessions upstream if the -// endpoint is hot. +// Timing: every call performs exactly one PBKDF2 derivation — at the +// stored iteration count for a well-formed pbkdf2-sha256 hash, and at +// [PBKDF2Iterations] for everything else (a legacy digest, a malformed or +// out-of-window string, or "" as callers pass for unknown users) — followed +// by a [subtle.ConstantTimeCompare]. The format of the stored hash is +// therefore not recoverable from response time, which is what the +// HashedUsersFunc contract asks for. What does remain observable is a +// non-default iteration count in a pbkdf2-sha256 entry, exactly as +// bcrypt's cost factor is; the microsecond-scale parsing that differs +// between the two formats is lost in the hundreds of milliseconds of +// derivation. +// +// That derivation is the per-request cost for every entry, legacy digests +// included — which is the point: cache authenticated sessions upstream if +// the endpoint is hot. func VerifyPassword(hash, password string) bool { if isPBKDF2Hash(hash) { return verifyPBKDF2(hash, password) @@ -92,11 +142,7 @@ func isPBKDF2Hash(hash string) bool { func verifyPBKDF2(hash, password string) bool { iter, salt, want, ok := parsePBKDF2(hash) if !ok { - // Burn the same work a well-formed hash would, then fail. - var dummySalt [pbkdf2SaltLen]byte - var dummy [pbkdf2KeyLen]byte - got, _ := pbkdf2.Key(sha256.New, password, dummySalt[:], PBKDF2Iterations, pbkdf2KeyLen) - _ = subtle.ConstantTimeCompare(got, dummy[:]) + burnPBKDF2(password) return false } got, err := pbkdf2.Key(sha256.New, password, salt, iter, pbkdf2KeyLen) @@ -106,24 +152,34 @@ func verifyPBKDF2(hash, password string) bool { return subtle.ConstantTimeCompare(got, want) == 1 } +// burnPBKDF2 derives a key from password at the default cost and discards +// it. Every VerifyPassword path that does not derive against a stored +// pbkdf2-sha256 hash calls this instead, so the verifier costs one +// derivation whatever the stored value looks like. +func burnPBKDF2(password string) { + var dummySalt [pbkdf2SaltLen]byte + var dummy [pbkdf2KeyLen]byte + got, _ := pbkdf2.Key(sha256.New, password, dummySalt[:], PBKDF2Iterations, pbkdf2KeyLen) + _ = subtle.ConstantTimeCompare(got, dummy[:]) +} + // parsePBKDF2 splits and decodes a HashPasswordPBKDF2 string. It rejects -// anything that is not exactly four fields, a positive decimal iteration -// count, a non-empty base64 salt, and a base64 hash of exactly -// pbkdf2KeyLen bytes. +// anything that is not exactly four fields, a decimal iteration count in +// [minPBKDF2Iterations, maxPBKDF2Iterations], a base64 salt of at least +// minPBKDF2SaltLen bytes, and a base64 hash of exactly pbkdf2KeyLen bytes. func parsePBKDF2(hash string) (iter int, salt, key []byte, ok bool) { parts := strings.Split(hash, "$") if len(parts) != 4 || parts[0] != pbkdf2Tag { return 0, nil, nil, false } - // ParseUint (not Atoi) refuses signs and is bounded to 31 bits so a - // hostile iteration count can neither go negative nor overflow int - // on 32-bit targets. - n, err := strconv.ParseUint(parts[1], 10, 31) - if err != nil || n == 0 { + // ParseUint (not Atoi) refuses signs; the window check is the real + // bound, and keeps int(n) safe on 32-bit targets as a side effect. + n, err := strconv.ParseUint(parts[1], 10, 32) + if err != nil || n < minPBKDF2Iterations || n > maxPBKDF2Iterations { return 0, nil, nil, false } salt, err = base64.StdEncoding.DecodeString(parts[2]) - if err != nil || len(salt) == 0 { + if err != nil || len(salt) < minPBKDF2SaltLen { return 0, nil, nil, false } key, err = base64.StdEncoding.DecodeString(parts[3]) @@ -134,17 +190,24 @@ func parsePBKDF2(hash string) (iter int, salt, key []byte, ok bool) { } // verifyLegacySHA256 checks password against a bare hex SHA-256 digest as -// produced by the deprecated HashPassword. Constant-time per the -// HashedUsersFunc contract, including on malformed input. +// produced by the deprecated HashPassword. It burns one default-cost PBKDF2 +// derivation first — on the valid-hex and the malformed path alike, "" +// included — so a legacy entry, an unknown user and a pbkdf2-sha256 entry +// all cost the same. The candidate digest comes from HashPassword itself, +// the one deliberately retained (deprecated, CodeQL-tracked) fast-hash +// site, rather than a second inline SHA-256 over the password; the stored +// hex is re-encoded so both sides are 64-byte lower-case strings for the +// constant-time compare. func verifyLegacySHA256(hash, password string) bool { + burnPBKDF2(password) want, err := hex.DecodeString(hash) - got := sha256.Sum256([]byte(password)) - if err != nil || len(want) != sha256.Size { - var dummy [sha256.Size]byte - _ = subtle.ConstantTimeCompare(got[:], dummy[:]) - return false + malformed := err != nil || len(want) != sha256.Size + if malformed { + want = make([]byte, sha256.Size) } - return subtle.ConstantTimeCompare(got[:], want) == 1 + match := subtle.ConstantTimeCompare( + []byte(HashPassword(password)), []byte(hex.EncodeToString(want))) == 1 + return match && !malformed } // allPBKDF2 reports whether every stored hash carries the pbkdf2-sha256 @@ -158,3 +221,18 @@ func allPBKDF2(hashes map[string]string) bool { } return true } + +// malformedPBKDF2Entry returns the username of a stored hash that carries +// the pbkdf2-sha256 tag but does not parse within the accepted window +// (bad=true), or bad=false when every entry parses. applyDefaults uses it +// to turn a mistyped iteration count or salt in an auto-wired store into a +// startup panic that names the entry, instead of a 401 on every request +// for that user. +func malformedPBKDF2Entry(hashes map[string]string) (user string, bad bool) { + for u, h := range hashes { + if _, _, _, ok := parsePBKDF2(h); !ok { + return u, true + } + } + return "", false +} diff --git a/middleware/basicauth/pbkdf2_test.go b/middleware/basicauth/pbkdf2_test.go index 2e36b97f..42b5c77b 100644 --- a/middleware/basicauth/pbkdf2_test.go +++ b/middleware/basicauth/pbkdf2_test.go @@ -1,12 +1,15 @@ package basicauth import ( + "crypto/pbkdf2" + "crypto/sha256" "encoding/base64" "encoding/hex" "strconv" "strings" "sync" "testing" + "time" "github.com/goceleris/celeris" "github.com/goceleris/celeris/celeristest" @@ -16,14 +19,32 @@ import ( // --- HashPasswordPBKDF2 / VerifyPassword (celeris#503) --- // -// Every derivation below costs 600k HMAC-SHA256 iterations (~0.2s native, -// seconds under -race), so the tests share one pre-computed hash where the -// salt does not matter, keep their case tables tight, and run in parallel. +// Every VerifyPassword call costs one 600k-iteration derivation (~0.2s +// native, seconds under -race) — legacy digests, "" and malformed input +// included, by design — so the tests share one pre-computed hash where the +// salt does not matter, keep their case tables tight, probe the parser +// window directly where a derivation would prove nothing extra, and run in +// parallel. TestVerifyPasswordCostUniform is the deliberate exception: it +// measures, so it runs alone. // secretHash returns a single pbkdf2-sha256 hash of "secret", computed once // per test binary. var secretHash = sync.OnceValue(func() string { return HashPasswordPBKDF2("secret") }) +// mkPBKDF2 builds a pbkdf2-sha256 string for password with explicit +// parameters, bypassing HashPasswordPBKDF2's fixed defaults, so tests can +// probe the accepted window with hashes whose derived key is correct. +func mkPBKDF2(t *testing.T, password string, iter int, salt []byte) string { + t.Helper() + key, err := pbkdf2.Key(sha256.New, password, salt, iter, pbkdf2KeyLen) + if err != nil { + t.Fatalf("pbkdf2.Key(iter=%d, salt=%d bytes): %v", iter, len(salt), err) + } + return pbkdf2Tag + "$" + strconv.Itoa(iter) + "$" + + base64.StdEncoding.EncodeToString(salt) + "$" + + base64.StdEncoding.EncodeToString(key) +} + func TestHashPasswordPBKDF2RoundTrip(t *testing.T) { t.Parallel() h := secretHash() @@ -109,17 +130,25 @@ func TestVerifyPasswordTamperedRejected(t *testing.T) { } saltRaw, _ := base64.StdEncoding.DecodeString(parts[2]) shortSalt := base64.StdEncoding.EncodeToString(saltRaw[:8]) + tinySalt := base64.StdEncoding.EncodeToString(saltRaw[:1]) keyRaw, _ := base64.StdEncoding.DecodeString(parts[3]) shortKey := base64.StdEncoding.EncodeToString(keyRaw[:16]) cases := map[string]string{ - "fewer iterations": join(parts[0], "1000", parts[2], parts[3]), - "more iterations": join(parts[0], "600001", parts[2], parts[3]), + // An in-window parameter change parses fine and fails on the + // derived key. The window's edges are pinned by TestParsePBKDF2Window. + "more iterations (key mismatch)": join(parts[0], "600001", parts[2], parts[3]), + // Out-of-window parameters are refused before any derivation + // against them and take the burn-then-false path. + "below-floor iterations": join(parts[0], "599999", parts[2], parts[3]), + "downgraded iterations": join(parts[0], "1000", parts[2], parts[3]), + "31-bit max iterations": join(parts[0], "2147483647", parts[2], parts[3]), "zero iterations": join(parts[0], "0", parts[2], parts[3]), "negative iterations": join(parts[0], "-600000", parts[2], parts[3]), "non-numeric iterations": join(parts[0], "abc", parts[2], parts[3]), + "8-byte salt": join(parts[0], parts[1], shortSalt, parts[3]), + "1-byte salt": join(parts[0], parts[1], tinySalt, parts[3]), "flipped salt": join(parts[0], parts[1], flipFirst(parts[2]), parts[3]), - "truncated salt": join(parts[0], parts[1], shortSalt, parts[3]), "invalid base64 salt": join(parts[0], parts[1], "!!!!", parts[3]), "flipped hash": join(parts[0], parts[1], parts[2], flipFirst(parts[3])), "truncated hash": join(parts[0], parts[1], parts[2], shortKey), @@ -131,7 +160,7 @@ func TestVerifyPasswordTamperedRejected(t *testing.T) { } for name, tampered := range cases { t.Run(name, func(t *testing.T) { - t.Parallel() // most cases cost a full 600k-iteration derivation + t.Parallel() // every case costs a full 600k-iteration derivation if VerifyPassword(tampered, "secret") { t.Fatalf("VerifyPassword accepted tampered hash %q", tampered) } @@ -139,32 +168,184 @@ func TestVerifyPasswordTamperedRejected(t *testing.T) { } } -func TestVerifyPasswordLegacySHA256(t *testing.T) { +// TestParsePBKDF2Window pins the edges of the parameter window without +// paying for a derivation per case: iterations in [600,000, 10,000,000], a +// salt of at least 16 bytes, a key of exactly 32 bytes, strict decimal and +// strict (padded) base64. The probes that used to parse — 2147483647 +// iterations, 1 iteration, a 1-byte salt — are all here. +func TestParsePBKDF2Window(t *testing.T) { t.Parallel() - legacy := HashPassword("secret") - if !VerifyPassword(legacy, "secret") { - t.Fatalf("legacy sha256 hash %q no longer verifies", legacy) + b64 := func(n int) string { return base64.StdEncoding.EncodeToString(make([]byte, n)) } + salt, key := b64(pbkdf2SaltLen), b64(pbkdf2KeyLen) + mk := func(iter, salt, key string) string { return pbkdf2Tag + "$" + iter + "$" + salt + "$" + key } + + cases := []struct { + name string + hash string + ok bool + iter int + }{ + {"default", mk("600000", salt, key), true, 600000}, + {"floor", mk(strconv.Itoa(minPBKDF2Iterations), salt, key), true, minPBKDF2Iterations}, + {"cap", mk("10000000", salt, key), true, 10000000}, + {"one above default", mk("600001", salt, key), true, 600001}, + {"below floor", mk(strconv.Itoa(minPBKDF2Iterations-1), salt, key), false, 0}, + {"RFC 8018 minimum", mk("1000", salt, key), false, 0}, + {"one", mk("1", salt, key), false, 0}, + {"zero", mk("0", salt, key), false, 0}, + {"above cap", mk("10000001", salt, key), false, 0}, + {"31-bit max", mk("2147483647", salt, key), false, 0}, + {"32-bit max", mk("4294967295", salt, key), false, 0}, + {"64-bit max", mk("18446744073709551615", salt, key), false, 0}, + {"plus sign", mk("+600000", salt, key), false, 0}, + {"minus sign", mk("-600000", salt, key), false, 0}, + {"leading space", mk(" 600000", salt, key), false, 0}, + {"underscore", mk("600_000", salt, key), false, 0}, + {"hex", mk("0x927C0", salt, key), false, 0}, + {"exponent", mk("6e5", salt, key), false, 0}, + {"empty iterations", mk("", salt, key), false, 0}, + {"17-byte salt", mk("600000", b64(17), key), true, 600000}, + {"15-byte salt", mk("600000", b64(15), key), false, 0}, + {"8-byte salt", mk("600000", b64(8), key), false, 0}, + {"1-byte salt", mk("600000", b64(1), key), false, 0}, + {"empty salt", mk("600000", "", key), false, 0}, + {"unpadded base64 salt", mk("600000", strings.TrimRight(salt, "="), key), false, 0}, + {"31-byte key", mk("600000", salt, b64(31)), false, 0}, + {"33-byte key", mk("600000", salt, b64(33)), false, 0}, + {"empty key", mk("600000", salt, ""), false, 0}, } - if VerifyPassword(legacy, "wrong") { - t.Fatal("legacy sha256 hash accepted wrong password") + for _, tc := range cases { + iter, s, k, ok := parsePBKDF2(tc.hash) + if ok != tc.ok { + t.Errorf("%s: parsePBKDF2(%q) ok=%v, want %v", tc.name, tc.hash, ok, tc.ok) + continue + } + if !ok { + continue + } + if iter != tc.iter { + t.Errorf("%s: iterations = %d, want %d", tc.name, iter, tc.iter) + } + if len(s) < minPBKDF2SaltLen || len(k) != pbkdf2KeyLen { + t.Errorf("%s: accepted salt of %d bytes / key of %d bytes", tc.name, len(s), len(k)) + } } - // hex is case-insensitive; upper-case hex is the same digest. - if !VerifyPassword(strings.ToUpper(legacy), "secret") { - t.Fatal("upper-case legacy hex hash should verify") + + // HashPasswordPBKDF2's own parameters must sit inside the window it + // is verified against, or the default wiring would reject its output. + // (Also enforced at compile time in pbkdf2.go; this is the readable + // failure.) + if PBKDF2Iterations < minPBKDF2Iterations || PBKDF2Iterations > maxPBKDF2Iterations || + pbkdf2SaltLen < minPBKDF2SaltLen { + t.Fatalf("HashPasswordPBKDF2 defaults (iter=%d, salt=%d) fall outside the accepted window", + PBKDF2Iterations, pbkdf2SaltLen) } - for _, bad := range []string{ - legacy[:63], // truncated - legacy + "00", // too long - "zz" + legacy[2:], // not hex - } { - if VerifyPassword(bad, "secret") { - t.Fatalf("malformed legacy hash %q accepted", bad) - } +} + +// The window is a real bound on the derivation, not just on the parser: +// a hash with the correct key at in-window non-default parameters +// verifies, and the same key one step outside the window does not. +func TestVerifyPasswordWindowEdges(t *testing.T) { + t.Parallel() + salt := make([]byte, pbkdf2SaltLen) + for i := range salt { + salt[i] = byte(i + 1) } + t.Run("in-window count above the default verifies", func(t *testing.T) { + t.Parallel() + h := mkPBKDF2(t, "secret", PBKDF2Iterations+1, salt) + if !VerifyPassword(h, "secret") { + t.Fatalf("in-window hash rejected: %q", h) + } + }) + t.Run("correct key one iteration below the floor is refused", func(t *testing.T) { + t.Parallel() + h := mkPBKDF2(t, "secret", minPBKDF2Iterations-1, salt) + if VerifyPassword(h, "secret") { + t.Fatalf("downgraded iteration count accepted with a correct key: %q", h) + } + }) + t.Run("correct key with one salt byte too few is refused", func(t *testing.T) { + t.Parallel() + h := mkPBKDF2(t, "secret", PBKDF2Iterations, salt[:minPBKDF2SaltLen-1]) + if VerifyPassword(h, "secret") { + t.Fatalf("%d-byte salt accepted with a correct key: %q", minPBKDF2SaltLen-1, h) + } + }) +} + +func TestVerifyPasswordLegacySHA256(t *testing.T) { + t.Parallel() + legacy := HashPassword("secret") // HashPassword's behaviour is unchanged: still the plain hex digest. if _, err := hex.DecodeString(legacy); err != nil || len(legacy) != 64 { t.Fatalf("HashPassword output changed: %q", legacy) } + cases := []struct { + name, hash, pass string + want bool + }{ + {"valid", legacy, "secret", true}, + {"wrong password", legacy, "wrong", false}, + {"upper-case hex", strings.ToUpper(legacy), "secret", true}, // hex is case-insensitive + {"truncated", legacy[:63], "secret", false}, + {"too long", legacy + "00", "secret", false}, + {"not hex", "zz" + legacy[2:], "secret", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() // every legacy verification burns a default-cost derivation + if got := VerifyPassword(tc.hash, tc.pass); got != tc.want { + t.Fatalf("VerifyPassword(%q, %q) = %v, want %v", tc.hash, tc.pass, got, tc.want) + } + }) + } +} + +// TestVerifyPasswordCostUniform pins the HashedUsersFunc contract for the +// built-in verifier: a legacy digest, an empty hash (what callers pass for +// unknown users) and a hostile out-of-window pbkdf2 string must all cost +// about one default derivation, the same as a genuine pbkdf2-sha256 entry. +// Before the legacy path burned a derivation, "" and hex digests returned +// in microseconds against ~170 ms for a pbkdf2 hash — a 10^5x gap that +// sorted usernames into {legacy, pbkdf2, unknown} by response time. The +// bound is deliberately loose (10x) so scheduler noise cannot trip it +// while any regression of that kind still does; a lost iteration cap +// would show up here as a multi-minute stall instead. Runs serially +// because it measures. +func TestVerifyPasswordCostUniform(t *testing.T) { + if testing.Short() { + t.Skip("four full derivations; skipped under -short") + } + h := secretHash() + parts := strings.Split(h, "$") + hostile := parts[0] + "$2147483647$" + parts[2] + "$" + parts[3] + legacy := HashPassword("secret") + + measure := func(hash string, want bool) time.Duration { + start := time.Now() + got := VerifyPassword(hash, "secret") + d := time.Since(start) + if got != want { + t.Fatalf("VerifyPassword(%q, \"secret\") = %v, want %v", hash, got, want) + } + return d + } + samples := map[string]time.Duration{ + "pbkdf2": measure(h, true), + "legacy": measure(legacy, true), + "empty": measure("", false), + "hostile count": measure(hostile, false), + } + fastest, slowest := time.Duration(1<<62), time.Duration(0) + for _, d := range samples { + fastest, slowest = min(fastest, d), max(slowest, d) + } + t.Logf("VerifyPassword cost by stored-hash shape: %v", samples) + if slowest > 10*fastest { + t.Fatalf("VerifyPassword cost is not uniform across hash formats (fastest %v, slowest %v): %v", + fastest, slowest, samples) + } } // TestHashedUsersPBKDF2Default: a HashedUsers map containing only @@ -223,6 +404,41 @@ func TestHashedUsersLegacyWithoutFuncStillPanics(t *testing.T) { }}) } +// An auto-wired store (no HashedUsersFunc) holding a pbkdf2-sha256 entry +// outside the accepted window fails at New, naming the entry, rather than +// silently answering 401 to that user on every request. +func TestHashedUsersOutOfWindowPBKDF2Panics(t *testing.T) { + t.Parallel() + parts := strings.Split(secretHash(), "$") + rewrite := func(iter string) string { return parts[0] + "$" + iter + "$" + parts[2] + "$" + parts[3] } + for name, typo := range map[string]string{ + "above cap": rewrite("60000000"), // one zero too many + "below floor": rewrite("60000"), // one zero too few + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + var msg string + func() { + defer func() { + if r := recover(); r != nil { + msg, _ = r.(string) + } + }() + New(Config{HashedUsers: map[string]string{ + "admin": secretHash(), + "typo": typo, + }}) + }() + if msg == "" { + t.Fatalf("expected panic: auto-wired store with out-of-window hash %q", typo) + } + if !strings.Contains(msg, `"typo"`) || !strings.Contains(msg, "pbkdf2-sha256") { + t.Fatalf("panic should name the entry and the format, got: %q", msg) + } + }) + } +} + // Mixed stores migrate incrementally: VerifyPassword accepts both formats. func TestHashedUsersVerifyPasswordMixedStore(t *testing.T) { t.Parallel() @@ -256,3 +472,53 @@ func TestHashedUsersVerifyPasswordMixedStore(t *testing.T) { }) } } + +// pickDummyHash prefers a pbkdf2-sha256 entry and otherwise breaks ties on +// username, so the unknown-user path never depends on map-iteration order. +// Repeated because a single map walk could hit the right order by luck. +func TestPickDummyHash(t *testing.T) { + t.Parallel() + pb := secretHash() + if got := pickDummyHash(nil); got != "" { + t.Fatalf("pickDummyHash(nil) = %q, want \"\"", got) + } + for range 16 { + mixed := map[string]string{"a": HashPassword("a"), "m": pb, "z": HashPassword("z")} + if got := pickDummyHash(mixed); got != pb { + t.Fatalf("mixed store: pickDummyHash = %q, want the pbkdf2-sha256 entry", got) + } + legacyOnly := map[string]string{"zed": HashPassword("z"), "amy": HashPassword("a"), "bob": HashPassword("b")} + if got := pickDummyHash(legacyOnly); got != HashPassword("a") { + t.Fatalf("legacy-only store: pickDummyHash = %q, want the entry of the smallest username", got) + } + } +} + +// In a mixed store the unknown-user path must hand the verifier a +// pbkdf2-sha256 entry, not whichever value map iteration yields first, so +// a caller-supplied verifier that dispatches on the tag pays a +// deterministic cost on a miss. Repeated so a lucky iteration order +// cannot pass it (three legacy entries to one pbkdf2: 4^-32 by chance). +func TestHashedUsersUnknownUserDummyPrefersPBKDF2(t *testing.T) { + t.Parallel() + pb := secretHash() + handler := func(c *celeris.Context) error { return c.String(200, "ok") } + for i := range 32 { + var seen string + mw := New(Config{ + HashedUsers: map[string]string{ + "old1": HashPassword("a"), + "old2": HashPassword("b"), + "old3": HashPassword("c"), + "new": pb, + }, + HashedUsersFunc: func(hash, _ string) bool { seen = hash; return false }, + }) + _, err := testutil.RunChain(t, []celeris.HandlerFunc{mw, handler}, "GET", "/", + celeristest.WithBasicAuth("nobody", "x")) + testutil.AssertHTTPError(t, err, 401) + if seen != pb { + t.Fatalf("iteration %d: unknown-user dummy hash = %q, want the pbkdf2-sha256 entry", i, seen) + } + } +}