diff --git a/internal/auth/account.go b/internal/auth/account.go index 2cb666d..ec9f6da 100644 --- a/internal/auth/account.go +++ b/internal/auth/account.go @@ -403,7 +403,7 @@ func (s *Handlers) handleChangePassword(w http.ResponseWriter, r *http.Request) newPassword := r.FormValue("new_password") if !ValidPassword(newPassword) { - accountRedirect(w, r, "error", "New password must be 8–72 characters.") + accountRedirect(w, r, "error", "New password must be at least 8 characters.") return } // When a password already exists, require the current one to change it. diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 62c810e..5f19da5 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -409,18 +409,14 @@ func NormalizePasskeyName(s string) string { return s } -// Password length bounds. The lower bound is a basic strength floor; the upper -// bound is bcrypt's hard limit — GenerateFromPassword rejects inputs longer than -// 72 bytes, so we reject them at the form boundary with a clear message rather -// than surfacing a generic hashing failure. -const ( - MinPasswordLen = 8 - MaxPasswordLen = 72 -) +// MinPasswordLen is a basic strength floor. There is deliberately no maximum: +// passwords are pre-hashed before bcrypt (see hashPassword), so bcrypt's 72-byte +// input limit never reaches the user. +const MinPasswordLen = 8 -// ValidPassword reports whether p is within the accepted length bounds. +// ValidPassword reports whether p meets the minimum length. func ValidPassword(p string) bool { - return len(p) >= MinPasswordLen && len(p) <= MaxPasswordLen + return len(p) >= MinPasswordLen } // ValidUsername reports whether s is 3–32 chars of [a-z0-9_.-]. diff --git a/internal/auth/password_handlers.go b/internal/auth/password_handlers.go index ca14e3b..51c4f37 100644 --- a/internal/auth/password_handlers.go +++ b/internal/auth/password_handlers.go @@ -14,7 +14,7 @@ func (s *Service) SignupPassword(w http.ResponseWriter, r *http.Request) { displayName := NormalizeDisplayName(r.FormValue("display_name")) password := r.FormValue("password") if !ValidUsername(username) || !ValidPassword(password) { - redirectErr(w, r, "/signup", "Choose a username (3–32 chars: letters, digits, _ . -) and a password of 8–72 characters.") + redirectErr(w, r, "/signup", "Choose a username (3–32 chars: letters, digits, _ . -) and a password of at least 8 characters.") return } diff --git a/internal/auth/service.go b/internal/auth/service.go index 86c8b75..7885c93 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -2,6 +2,8 @@ package auth import ( "context" + "crypto/sha256" + "encoding/base64" "errors" "net/http" "time" @@ -182,9 +184,15 @@ func (s *Service) VerifyPassword(ctx context.Context, username, password string) if u.PasswordHash == nil { return nil, ErrInvalidCredentials } - if err := bcrypt.CompareHashAndPassword([]byte(*u.PasswordHash), []byte(password)); err != nil { + ok, legacy := comparePassword(*u.PasswordHash, password) + if !ok { return nil, ErrInvalidCredentials } + // Transparently migrate a legacy (raw-bcrypt) hash to the pre-hash scheme on + // successful login; best-effort, never blocks the sign-in. + if legacy { + _ = s.SetPassword(ctx, u.ID, password) + } return u, nil } @@ -234,16 +242,50 @@ func (s *Service) PasswordMatches(u *store.User, password string) bool { if u.PasswordHash == nil { return false } - return bcrypt.CompareHashAndPassword([]byte(*u.PasswordHash), []byte(password)) == nil + ok, _ := comparePassword(*u.PasswordHash, password) + return ok } // SetPassword hashes and stores a password for a user. func (s *Service) SetPassword(ctx context.Context, userID int64, password string) error { - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + hash, err := hashPassword(password) if err != nil { return err } - return s.store.SetPassword(ctx, userID, string(hash)) + return s.store.SetPassword(ctx, userID, hash) +} + +// bcrypt only considers the first 72 bytes of its input (and stops at the first +// NUL byte), so we can't feed it a raw password without imposing a length limit. +// Pre-hashing with SHA-256 and base64-encoding the digest collapses any-length +// password to a fixed 44-byte, NUL-free token that always fits — so users face +// no visible maximum. (This is the standard bcrypt-pre-hash construction.) +func prehashPassword(password string) []byte { + sum := sha256.Sum256([]byte(password)) + enc := base64.StdEncoding.EncodeToString(sum[:]) + return []byte(enc) +} + +// hashPassword produces the stored bcrypt hash for a password (pre-hash scheme). +func hashPassword(password string) (string, error) { + h, err := bcrypt.GenerateFromPassword(prehashPassword(password), bcrypt.DefaultCost) + return string(h), err +} + +// comparePassword checks password against a stored bcrypt hash, accepting both +// the current pre-hash scheme and the legacy raw-bcrypt scheme (hashes written +// before pre-hashing existed). legacy reports which one matched, so the caller +// can upgrade the stored hash. Legacy hashes only ever held passwords ≤72 bytes +// (that limit was enforced then), so the raw comparison is safe and can't be +// confused with a pre-hash match. +func comparePassword(hash, password string) (ok, legacy bool) { + if bcrypt.CompareHashAndPassword([]byte(hash), prehashPassword(password)) == nil { + return true, false + } + if bcrypt.CompareHashAndPassword([]byte(hash), []byte(password)) == nil { + return true, true + } + return false, false } // ErrInvalidCredentials is returned for any failed password authentication. diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 5822679..9f1b679 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -3,17 +3,19 @@ package auth import ( "strings" "testing" + + "golang.org/x/crypto/bcrypt" ) -// TestValidPassword pins the accepted length window: an 8-char floor and a -// 72-byte ceiling (bcrypt's hard input limit). Reject just outside both ends. +// TestValidPassword pins the 8-char floor and confirms there is no upper bound: +// pre-hashing removes bcrypt's 72-byte input limit, so long passwords are valid. func TestValidPassword(t *testing.T) { t.Parallel() cases := []struct { n int want bool }{ - {7, false}, {8, true}, {72, true}, {73, false}, + {7, false}, {8, true}, {72, true}, {73, true}, {5000, true}, } for _, c := range cases { if got := ValidPassword(strings.Repeat("a", c.n)); got != c.want { @@ -22,6 +24,47 @@ func TestValidPassword(t *testing.T) { } } +// TestPasswordHashRoundTrip: a password far past bcrypt's 72-byte input limit +// hashes and verifies, and — crucially — two passwords that differ only after +// byte 72 do NOT collide (raw bcrypt would treat them as equal; pre-hashing is +// exactly what prevents that). +func TestPasswordHashRoundTrip(t *testing.T) { + t.Parallel() + long := strings.Repeat("correct horse battery staple ", 10) // ~290 bytes + hash, err := hashPassword(long) + if err != nil { + t.Fatalf("hashPassword: %v", err) + } + if ok, legacy := comparePassword(hash, long); !ok || legacy { + t.Fatalf("comparePassword(correct) = (%v, %v), want (true, false)", ok, legacy) + } + if ok, _ := comparePassword(hash, long+"x"); ok { + t.Fatal("comparePassword accepted a wrong password") + } + + a, _ := hashPassword(strings.Repeat("a", 72) + "1") + if ok, _ := comparePassword(a, strings.Repeat("a", 72)+"2"); ok { + t.Fatal("passwords differing only after byte 72 collided (pre-hash not applied?)") + } +} + +// TestComparePasswordLegacy: a hash written the old way (raw bcrypt of the +// password) still verifies and is flagged legacy so the caller can upgrade it. +func TestComparePasswordLegacy(t *testing.T) { + t.Parallel() + const pw = "legacy-secret" + raw, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) + if err != nil { + t.Fatalf("bcrypt: %v", err) + } + if ok, legacy := comparePassword(string(raw), pw); !ok || !legacy { + t.Fatalf("legacy compare = (%v, %v), want (true, true)", ok, legacy) + } + if ok, _ := comparePassword(string(raw), "wrong"); ok { + t.Fatal("legacy compare accepted a wrong password") + } +} + func TestSafeLocalPath(t *testing.T) { t.Parallel() cases := []struct { diff --git a/internal/core/endpoints_auth_test.go b/internal/core/endpoints_auth_test.go index 3209b83..a50b28b 100644 --- a/internal/core/endpoints_auth_test.go +++ b/internal/core/endpoints_auth_test.go @@ -5,8 +5,11 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" + "golang.org/x/crypto/bcrypt" + "github.com/jleight/meshtender/internal/store" ) @@ -124,6 +127,50 @@ func TestPasskeyBeginDefersAccount(t *testing.T) { } } +// TestPasswordNoMaxLengthAndLegacyMigration: a password well past bcrypt's +// 72-byte limit works end to end (pre-hashing removes the ceiling), and a stored +// legacy raw-bcrypt hash still authenticates and is transparently upgraded to the +// pre-hash scheme on login. +func TestPasswordNoMaxLengthAndLegacyMigration(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + // A 200-char password: sign up, then sign in with it. + longPw := strings.Repeat("MeshTender-", 20) // 220 chars + up := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"longpw"}, "password": {longPw}}) + up.Body.Close() + if up.StatusCode != http.StatusSeeOther || cookieByName(up, "meshtender_session") == nil { + t.Fatalf("long-password signup = %d, want 303 + session", up.StatusCode) + } + in := post(t, ts, h.auth, "/login/password", url.Values{"username": {"longpw"}, "password": {longPw}}) + in.Body.Close() + if in.StatusCode != http.StatusSeeOther || cookieByName(in, "meshtender_session") == nil { + t.Fatalf("long-password login = %d, want 303 + session", in.StatusCode) + } + + // Plant a legacy (raw-bcrypt) hash and confirm login works and upgrades it. + legacyUser, err := st.CreateUser(ctx, "legacypw", "") + if err != nil { + t.Fatalf("create user: %v", err) + } + raw, _ := bcrypt.GenerateFromPassword([]byte("old-password"), bcrypt.DefaultCost) + if err := st.SetPassword(ctx, legacyUser.ID, string(raw)); err != nil { + t.Fatalf("set legacy hash: %v", err) + } + legacyLogin := post(t, ts, h.auth, "/login/password", url.Values{"username": {"legacypw"}, "password": {"old-password"}}) + legacyLogin.Body.Close() + if legacyLogin.StatusCode != http.StatusSeeOther || cookieByName(legacyLogin, "meshtender_session") == nil { + t.Fatalf("legacy login = %d, want 303 + session", legacyLogin.StatusCode) + } + after, err := st.GetUserByUsername(ctx, "legacypw") + if err != nil { + t.Fatalf("reload user: %v", err) + } + if after.PasswordHash == nil || *after.PasswordHash == string(raw) { + t.Fatal("legacy hash was not upgraded on login") + } +} + // assertAccountOK asserts a 303 back to /account carrying a success ("ok") flash. func assertAccountOK(t *testing.T, resp *http.Response, label string) { t.Helper()