diff --git a/internal/grpcserver/auth.go b/internal/grpcserver/auth.go index 64a7cf7..4c867e0 100644 --- a/internal/grpcserver/auth.go +++ b/internal/grpcserver/auth.go @@ -60,7 +60,9 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest if locked { _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) 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) diff --git a/internal/grpcserver/grpcserver_test.go b/internal/grpcserver/grpcserver_test.go index 9a34e5b..c79168c 100644 --- a/internal/grpcserver/grpcserver_test.go +++ b/internal/grpcserver/grpcserver_test.go @@ -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") + } +} diff --git a/internal/server/server.go b/internal/server/server.go index aa135a1..8d93545 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -238,7 +238,9 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { if locked { _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) 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 } @@ -1026,7 +1028,9 @@ func (s *Server) handleChangePassword(w http.ResponseWriter, r *http.Request) { } if 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 } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index f7325fe..6620066 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -594,3 +594,76 @@ func TestRenewToken(t *testing.T) { 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") + } +} diff --git a/internal/ui/handlers_auth.go b/internal/ui/handlers_auth.go index 250341a..9855d6c 100644 --- a/internal/ui/handlers_auth.go +++ b/internal/ui/handlers_auth.go @@ -80,7 +80,9 @@ func (u *UIServer) handleLoginPost(w http.ResponseWriter, r *http.Request) { if locked { _, _ = auth.VerifyPassword("dummy", u.dummyHash()) 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 } diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go index 7b48050..953c06a 100644 --- a/internal/ui/ui_test.go +++ b/internal/ui/ui_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "git.wntrmute.dev/kyle/mcias/internal/auth" "git.wntrmute.dev/kyle/mcias/internal/config" "git.wntrmute.dev/kyle/mcias/internal/db" "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") } } + +// 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'") + } +}