Fix SEC-02: normalize lockout response
- REST login: change locked account response from HTTP 429 "account_locked" to HTTP 401 "invalid credentials" - gRPC login: change from ResourceExhausted to Unauthenticated with "invalid credentials" message - UI login: change from "account temporarily locked" to "invalid credentials" - REST password-change endpoint: same normalization - Audit logs still record "account_locked" internally - Added tests in all three layers verifying locked-account responses are indistinguishable from wrong-password responses Security: lockout responses now return identical status codes and messages as wrong-password failures across REST, gRPC, and UI, preventing user-enumeration via lockout differentiation. Internal audit logging of lockout events is preserved for operational use. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -60,7 +60,9 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest
|
|||||||
if locked {
|
if locked {
|
||||||
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
|
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
|
||||||
a.s.db.WriteAuditEvent(model.EventLoginFail, &acct.ID, nil, ip, `{"reason":"account_locked"}`) //nolint:errcheck
|
a.s.db.WriteAuditEvent(model.EventLoginFail, &acct.ID, nil, ip, `{"reason":"account_locked"}`) //nolint:errcheck
|
||||||
return nil, status.Error(codes.ResourceExhausted, "account temporarily locked")
|
// Security: return the same Unauthenticated / "invalid credentials" as wrong-password
|
||||||
|
// to prevent user-enumeration via lockout differentiation (SEC-02).
|
||||||
|
return nil, status.Error(codes.Unauthenticated, "invalid credentials")
|
||||||
}
|
}
|
||||||
|
|
||||||
ok, err := auth.VerifyPassword(req.Password, acct.PasswordHash)
|
ok, err := auth.VerifyPassword(req.Password, acct.PasswordHash)
|
||||||
|
|||||||
@@ -650,3 +650,71 @@ func TestCredentialFieldsAbsentFromAccountResponse(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestLoginLockedAccountReturnsUnauthenticated verifies that a locked-out
|
||||||
|
// account gets the same gRPC Unauthenticated / "invalid credentials" as a
|
||||||
|
// wrong-password attempt, preventing user-enumeration via lockout
|
||||||
|
// differentiation (SEC-02).
|
||||||
|
func TestLoginLockedAccountReturnsUnauthenticated(t *testing.T) {
|
||||||
|
e := newTestEnv(t)
|
||||||
|
acct := e.createHumanAccount(t, "lockgrpc")
|
||||||
|
|
||||||
|
// Lower the lockout threshold so we don't need 10 failures.
|
||||||
|
origThreshold := db.LockoutThreshold
|
||||||
|
db.LockoutThreshold = 3
|
||||||
|
t.Cleanup(func() { db.LockoutThreshold = origThreshold })
|
||||||
|
|
||||||
|
for range db.LockoutThreshold {
|
||||||
|
if err := e.db.RecordLoginFailure(acct.ID); err != nil {
|
||||||
|
t.Fatalf("RecordLoginFailure: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
locked, err := e.db.IsLockedOut(acct.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsLockedOut: %v", err)
|
||||||
|
}
|
||||||
|
if !locked {
|
||||||
|
t.Fatal("expected account to be locked out after threshold failures")
|
||||||
|
}
|
||||||
|
|
||||||
|
cl := mciasv1.NewAuthServiceClient(e.conn)
|
||||||
|
|
||||||
|
// Attempt login on the locked account.
|
||||||
|
_, lockedErr := cl.Login(context.Background(), &mciasv1.LoginRequest{
|
||||||
|
Username: "lockgrpc",
|
||||||
|
Password: "testpass123",
|
||||||
|
})
|
||||||
|
if lockedErr == nil {
|
||||||
|
t.Fatal("Login on locked account: expected error, got nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Attempt login with wrong password for comparison.
|
||||||
|
_, wrongErr := cl.Login(context.Background(), &mciasv1.LoginRequest{
|
||||||
|
Username: "lockgrpc",
|
||||||
|
Password: "wrongpassword",
|
||||||
|
})
|
||||||
|
if wrongErr == nil {
|
||||||
|
t.Fatal("Login with wrong password: expected error, got nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
lockedSt, _ := status.FromError(lockedErr)
|
||||||
|
wrongSt, _ := status.FromError(wrongErr)
|
||||||
|
|
||||||
|
// Both must return Unauthenticated, not ResourceExhausted.
|
||||||
|
if lockedSt.Code() != codes.Unauthenticated {
|
||||||
|
t.Errorf("locked: got code %v, want Unauthenticated", lockedSt.Code())
|
||||||
|
}
|
||||||
|
if wrongSt.Code() != codes.Unauthenticated {
|
||||||
|
t.Errorf("wrong password: got code %v, want Unauthenticated", wrongSt.Code())
|
||||||
|
}
|
||||||
|
|
||||||
|
// Messages must be identical.
|
||||||
|
if lockedSt.Message() != wrongSt.Message() {
|
||||||
|
t.Errorf("locked message %q differs from wrong-password message %q",
|
||||||
|
lockedSt.Message(), wrongSt.Message())
|
||||||
|
}
|
||||||
|
if lockedSt.Message() != "invalid credentials" {
|
||||||
|
t.Errorf("locked message = %q, want %q", lockedSt.Message(), "invalid credentials")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -238,7 +238,9 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) {
|
|||||||
if locked {
|
if locked {
|
||||||
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
|
_, _ = auth.VerifyPassword("dummy", auth.DummyHash())
|
||||||
s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"account_locked"}`)
|
s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"account_locked"}`)
|
||||||
middleware.WriteError(w, http.StatusTooManyRequests, "account temporarily locked", "account_locked")
|
// Security: return the same 401 "invalid credentials" as wrong-password
|
||||||
|
// to prevent user-enumeration via lockout differentiation (SEC-02).
|
||||||
|
middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1026,7 +1028,9 @@ func (s *Server) handleChangePassword(w http.ResponseWriter, r *http.Request) {
|
|||||||
}
|
}
|
||||||
if locked {
|
if locked {
|
||||||
s.writeAudit(r, model.EventPasswordChanged, &acct.ID, &acct.ID, `{"result":"locked"}`)
|
s.writeAudit(r, model.EventPasswordChanged, &acct.ID, &acct.ID, `{"result":"locked"}`)
|
||||||
middleware.WriteError(w, http.StatusTooManyRequests, "account temporarily locked", "account_locked")
|
// Security: return the same 401 as wrong-password to prevent
|
||||||
|
// user-enumeration via lockout differentiation (SEC-02).
|
||||||
|
middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -594,3 +594,76 @@ func TestRenewToken(t *testing.T) {
|
|||||||
t.Error("old token should be revoked after renewal")
|
t.Error("old token should be revoked after renewal")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestLoginLockedAccountReturns401 verifies that a locked-out account gets the
|
||||||
|
// same HTTP 401 / "invalid credentials" response as a wrong-password attempt,
|
||||||
|
// preventing user-enumeration via lockout differentiation (SEC-02).
|
||||||
|
func TestLoginLockedAccountReturns401(t *testing.T) {
|
||||||
|
srv, _, _, database := newTestServer(t)
|
||||||
|
acct := createTestHumanAccount(t, srv, "lockuser")
|
||||||
|
handler := srv.Handler()
|
||||||
|
|
||||||
|
// Lower the lockout threshold so we don't need 10 failures.
|
||||||
|
origThreshold := db.LockoutThreshold
|
||||||
|
db.LockoutThreshold = 3
|
||||||
|
t.Cleanup(func() { db.LockoutThreshold = origThreshold })
|
||||||
|
|
||||||
|
// Record enough failures to trigger lockout.
|
||||||
|
for range db.LockoutThreshold {
|
||||||
|
if err := database.RecordLoginFailure(acct.ID); err != nil {
|
||||||
|
t.Fatalf("RecordLoginFailure: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Confirm the account is locked.
|
||||||
|
locked, err := database.IsLockedOut(acct.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsLockedOut: %v", err)
|
||||||
|
}
|
||||||
|
if !locked {
|
||||||
|
t.Fatal("expected account to be locked out after threshold failures")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Attempt login on the locked account.
|
||||||
|
lockedRR := doRequest(t, handler, "POST", "/v1/auth/login", map[string]string{
|
||||||
|
"username": "lockuser",
|
||||||
|
"password": "testpass123",
|
||||||
|
}, "")
|
||||||
|
|
||||||
|
// Also attempt login with a wrong password (not locked) for comparison.
|
||||||
|
wrongRR := doRequest(t, handler, "POST", "/v1/auth/login", map[string]string{
|
||||||
|
"username": "lockuser",
|
||||||
|
"password": "wrongpassword",
|
||||||
|
}, "")
|
||||||
|
|
||||||
|
// Both must return 401, not 429.
|
||||||
|
if lockedRR.Code != http.StatusUnauthorized {
|
||||||
|
t.Errorf("locked account: status = %d, want %d", lockedRR.Code, http.StatusUnauthorized)
|
||||||
|
}
|
||||||
|
if wrongRR.Code != http.StatusUnauthorized {
|
||||||
|
t.Errorf("wrong password: status = %d, want %d", wrongRR.Code, http.StatusUnauthorized)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Parse the JSON bodies and compare — they must be identical.
|
||||||
|
type errResp struct {
|
||||||
|
Error string `json:"error"`
|
||||||
|
Code string `json:"code"`
|
||||||
|
}
|
||||||
|
var lockedBody, wrongBody errResp
|
||||||
|
if err := json.Unmarshal(lockedRR.Body.Bytes(), &lockedBody); err != nil {
|
||||||
|
t.Fatalf("unmarshal locked body: %v", err)
|
||||||
|
}
|
||||||
|
if err := json.Unmarshal(wrongRR.Body.Bytes(), &wrongBody); err != nil {
|
||||||
|
t.Fatalf("unmarshal wrong body: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if lockedBody != wrongBody {
|
||||||
|
t.Errorf("locked response %+v differs from wrong-password response %+v", lockedBody, wrongBody)
|
||||||
|
}
|
||||||
|
if lockedBody.Code != "unauthorized" {
|
||||||
|
t.Errorf("locked response code = %q, want %q", lockedBody.Code, "unauthorized")
|
||||||
|
}
|
||||||
|
if lockedBody.Error != "invalid credentials" {
|
||||||
|
t.Errorf("locked response error = %q, want %q", lockedBody.Error, "invalid credentials")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -80,7 +80,9 @@ func (u *UIServer) handleLoginPost(w http.ResponseWriter, r *http.Request) {
|
|||||||
if locked {
|
if locked {
|
||||||
_, _ = auth.VerifyPassword("dummy", u.dummyHash())
|
_, _ = auth.VerifyPassword("dummy", u.dummyHash())
|
||||||
u.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"account_locked"}`)
|
u.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"account_locked"}`)
|
||||||
u.render(w, "login", LoginData{Error: "account temporarily locked, please try again later"})
|
// Security: return the same "invalid credentials" as wrong-password
|
||||||
|
// to prevent user-enumeration via lockout differentiation (SEC-02).
|
||||||
|
u.render(w, "login", LoginData{Error: "invalid credentials"})
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ import (
|
|||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
"git.wntrmute.dev/kyle/mcias/internal/auth"
|
||||||
"git.wntrmute.dev/kyle/mcias/internal/config"
|
"git.wntrmute.dev/kyle/mcias/internal/config"
|
||||||
"git.wntrmute.dev/kyle/mcias/internal/db"
|
"git.wntrmute.dev/kyle/mcias/internal/db"
|
||||||
"git.wntrmute.dev/kyle/mcias/internal/model"
|
"git.wntrmute.dev/kyle/mcias/internal/model"
|
||||||
@@ -527,3 +528,77 @@ func TestAccountDetailShowsPGCredsSection(t *testing.T) {
|
|||||||
t.Error("human account detail page must not include pgcreds-section")
|
t.Error("human account detail page must not include pgcreds-section")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestLoginLockedAccountShowsInvalidCredentials verifies that a locked-out
|
||||||
|
// account gets the same "invalid credentials" error as a wrong-password
|
||||||
|
// attempt in the UI login form, preventing user-enumeration via lockout
|
||||||
|
// differentiation (SEC-02).
|
||||||
|
func TestLoginLockedAccountShowsInvalidCredentials(t *testing.T) {
|
||||||
|
u := newTestUIServer(t)
|
||||||
|
|
||||||
|
// Create an account with a known password.
|
||||||
|
hash, err := auth.HashPassword("testpass123", auth.ArgonParams{Time: 3, Memory: 65536, Threads: 4})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("hash password: %v", err)
|
||||||
|
}
|
||||||
|
acct, err := u.db.CreateAccount("lockuiuser", model.AccountTypeHuman, hash)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("CreateAccount: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Lower the lockout threshold so we don't need 10 failures.
|
||||||
|
origThreshold := db.LockoutThreshold
|
||||||
|
db.LockoutThreshold = 3
|
||||||
|
t.Cleanup(func() { db.LockoutThreshold = origThreshold })
|
||||||
|
|
||||||
|
for range db.LockoutThreshold {
|
||||||
|
if err := u.db.RecordLoginFailure(acct.ID); err != nil {
|
||||||
|
t.Fatalf("RecordLoginFailure: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
locked, err := u.db.IsLockedOut(acct.ID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("IsLockedOut: %v", err)
|
||||||
|
}
|
||||||
|
if !locked {
|
||||||
|
t.Fatal("expected account to be locked out after threshold failures")
|
||||||
|
}
|
||||||
|
|
||||||
|
mux := http.NewServeMux()
|
||||||
|
u.Register(mux)
|
||||||
|
|
||||||
|
// POST login for the locked account.
|
||||||
|
form := url.Values{}
|
||||||
|
form.Set("username", "lockuiuser")
|
||||||
|
form.Set("password", "testpass123")
|
||||||
|
req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
|
||||||
|
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||||
|
lockedRR := httptest.NewRecorder()
|
||||||
|
mux.ServeHTTP(lockedRR, req)
|
||||||
|
|
||||||
|
// POST login with wrong password for comparison.
|
||||||
|
form2 := url.Values{}
|
||||||
|
form2.Set("username", "lockuiuser")
|
||||||
|
form2.Set("password", "wrongpassword")
|
||||||
|
req2 := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form2.Encode()))
|
||||||
|
req2.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||||
|
wrongRR := httptest.NewRecorder()
|
||||||
|
mux.ServeHTTP(wrongRR, req2)
|
||||||
|
|
||||||
|
lockedBody := lockedRR.Body.String()
|
||||||
|
wrongBody := wrongRR.Body.String()
|
||||||
|
|
||||||
|
// Neither response should mention "locked" or "try again".
|
||||||
|
if strings.Contains(lockedBody, "locked") || strings.Contains(lockedBody, "try again") {
|
||||||
|
t.Error("locked account response leaks lockout state")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Both must contain "invalid credentials".
|
||||||
|
if !strings.Contains(lockedBody, "invalid credentials") {
|
||||||
|
t.Error("locked account response does not contain 'invalid credentials'")
|
||||||
|
}
|
||||||
|
if !strings.Contains(wrongBody, "invalid credentials") {
|
||||||
|
t.Error("wrong password response does not contain 'invalid credentials'")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user