diff --git a/AUDIT.md b/AUDIT.md index 62bcf82..818277f 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -228,11 +228,11 @@ The REST `handleTokenIssue` and gRPC `IssueServiceToken` both revoke the existin | No | F-05 | LOW | No `nbf` claim in issued JWTs | Trivial | | No | F-06 | LOW | `HasRole` uses non-constant-time comparison | Trivial | | Yes | F-07 | LOW | Dummy Argon2 hash timing mismatch | Small | -| No | F-08 | LOW | No account lockout after repeated failures | Medium | +| Yes | F-08 | LOW | No account lockout after repeated failures | Medium | | No | F-09 | LOW | `synchronous=NORMAL` risks audit data loss | Trivial | | No | F-10 | LOW | No maximum token expiry validation | Small | -| No | F-12 | LOW | No username length/charset validation | Small | -| No | F-13 | LOW | No minimum password length enforcement | Small | +| Yes | F-12 | LOW | No username length/charset validation | Small | +| Yes | F-13 | LOW | No minimum password length enforcement | Small | | No | F-14 | LOW | Passphrase string not zeroed after KDF | Small | | Yes | F-16 | LOW | UI system token issuance skips old token revocation | Small | | No | F-15 | INFO | Bearer prefix check inconsistency | Trivial | diff --git a/CLAUDE.md b/CLAUDE.md index 845f224..13a39f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,3 +72,4 @@ This is a security-critical project. The following rules are non-negotiable: - `ARCHITECTURE.md` — **Required before any implementation.** Covers token lifecycle, session management, multi-app trust boundaries, and database schema. Do not generate code until this document exists. - `PROJECT_PLAN.md` — Discrete implementation steps (to be written) - `PROGRESS.md` — Development progress tracking (to be written) +- `openapi.yaml` - Must be kept in sync with any API changes. diff --git a/internal/db/accounts.go b/internal/db/accounts.go index 5f95cca..11cc6ea 100644 --- a/internal/db/accounts.go +++ b/internal/db/accounts.go @@ -479,6 +479,87 @@ func (db *DB) ReadPGCredentials(accountID int64) (*model.PGCredential, error) { return &cred, nil } +// ---- Login lockout (F-08) ---- + +// LockoutWindow is the rolling window for failed-login counting. +// LockoutThreshold is the number of failures within the window that triggers a lockout. +// LockoutDuration is how long the lockout lasts after threshold is reached. +// These are package-level vars (not consts) so tests can override them. +// +// Security: 10 failures in 15 minutes is conservative for a personal SSO; it +// stops fast dictionary attacks while rarely affecting legitimate users. +var ( + LockoutWindow = 15 * time.Minute + LockoutThreshold = 10 + LockoutDuration = 15 * time.Minute +) + +// IsLockedOut returns true if the account has exceeded the failed-login +// threshold within the current window and the lockout period has not expired. +func (db *DB) IsLockedOut(accountID int64) (bool, error) { + var windowStartStr string + var count int + err := db.sql.QueryRow(` + SELECT window_start, attempt_count FROM failed_logins WHERE account_id = ? + `, accountID).Scan(&windowStartStr, &count) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("db: is locked out %d: %w", accountID, err) + } + + windowStart, err := parseTime(windowStartStr) + if err != nil { + return false, err + } + + // Window has expired — not locked out. + if time.Since(windowStart) > LockoutWindow+LockoutDuration { + return false, nil + } + + // Under threshold — not locked out. + if count < LockoutThreshold { + return false, nil + } + + // Threshold exceeded; locked out until window_start + LockoutDuration. + lockedUntil := windowStart.Add(LockoutDuration) + return time.Now().Before(lockedUntil), nil +} + +// RecordLoginFailure increments the failed-login counter for the account. +// If the current window has expired a new window is started. +func (db *DB) RecordLoginFailure(accountID int64) error { + n := now() + windowCutoff := time.Now().Add(-LockoutWindow).UTC().Format(time.RFC3339) + + // Upsert: if a row exists and the window is still active, increment; + // otherwise reset to a fresh window with count 1. + _, err := db.sql.Exec(` + INSERT INTO failed_logins (account_id, window_start, attempt_count) + VALUES (?, ?, 1) + ON CONFLICT(account_id) DO UPDATE SET + window_start = CASE WHEN window_start < ? THEN excluded.window_start ELSE window_start END, + attempt_count = CASE WHEN window_start < ? THEN 1 ELSE attempt_count + 1 END + `, accountID, n, windowCutoff, windowCutoff) + if err != nil { + return fmt.Errorf("db: record login failure %d: %w", accountID, err) + } + return nil +} + +// ClearLoginFailures resets the failed-login counter for the account. +// Called on successful login. +func (db *DB) ClearLoginFailures(accountID int64) error { + _, err := db.sql.Exec(`DELETE FROM failed_logins WHERE account_id = ?`, accountID) + if err != nil { + return fmt.Errorf("db: clear login failures %d: %w", accountID, err) + } + return nil +} + // WriteAuditEvent appends an audit log entry. // Details must never contain credential material. func (db *DB) WriteAuditEvent(eventType string, actorID, targetID *int64, ipAddress, details string) error { @@ -1010,3 +1091,78 @@ func (db *DB) GetSystemToken(accountID int64) (*model.SystemToken, error) { } return &st, nil } + +// Lockout parameters (package-level vars so tests can override them). +// +// Security (F-08): per-account failed-login tracking prevents brute-force +// attacks. LockoutWindow defines the rolling window during which failures +// are counted; LockoutThreshold is the number of failures that triggers a +// lockout; LockoutDuration is how long the account remains locked after the +// threshold is reached. All three are intentionally kept as vars (not +// consts) so that tests can reduce them to millisecond-scale values without +// recompiling. +var ( + LockoutWindow = 15 * time.Minute + LockoutThreshold = 10 + LockoutDuration = 15 * time.Minute +) + +// IsLockedOut returns true if accountID has exceeded LockoutThreshold +// failures within the current LockoutWindow and the LockoutDuration has not +// yet elapsed since the window opened. +func (db *DB) IsLockedOut(accountID int64) (bool, error) { + var windowStartStr string + var count int + err := db.sql.QueryRow(` + SELECT window_start, attempt_count + FROM failed_logins WHERE account_id = ? + `, accountID).Scan(&windowStartStr, &count) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("db: is locked out %d: %w", accountID, err) + } + + windowStart, err := parseTime(windowStartStr) + if err != nil { + return false, fmt.Errorf("db: parse lockout window_start: %w", err) + } + + // The window has expired: the record is stale, the account is not locked. + if time.Now().After(windowStart.Add(LockoutWindow)) { + return false, nil + } + + return count >= LockoutThreshold, nil +} + +// RecordLoginFailure increments the failure counter for accountID within the +// current rolling window. If the window has expired the counter resets to 1 +// and the window_start is updated. Uses an UPSERT so the operation is safe +// to call without a prior existence check. +func (db *DB) RecordLoginFailure(accountID int64) error { + n := now() + windowCutoff := time.Now().Add(-LockoutWindow).UTC().Format(time.RFC3339) + _, err := db.sql.Exec(` + INSERT INTO failed_logins (account_id, window_start, attempt_count) + VALUES (?, ?, 1) + ON CONFLICT(account_id) DO UPDATE SET + window_start = CASE WHEN window_start < ? THEN excluded.window_start ELSE window_start END, + attempt_count = CASE WHEN window_start < ? THEN 1 ELSE attempt_count + 1 END + `, accountID, n, windowCutoff, windowCutoff) + if err != nil { + return fmt.Errorf("db: record login failure for account %d: %w", accountID, err) + } + return nil +} + +// ClearLoginFailures removes the failure record for accountID. Called on a +// successful login to reset the lockout state. +func (db *DB) ClearLoginFailures(accountID int64) error { + _, err := db.sql.Exec(`DELETE FROM failed_logins WHERE account_id = ?`, accountID) + if err != nil { + return fmt.Errorf("db: clear login failures for account %d: %w", accountID, err) + } + return nil +} diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 8067055..835e7fa 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -473,3 +473,127 @@ func TestRevokeAllUserTokens(t *testing.T) { } } } + +// TestLockoutNotLockedInitially verifies a fresh account is not locked out. +func TestLockoutNotLockedInitially(t *testing.T) { + d := openTestDB(t) + acct, err := d.CreateAccount("locktest", model.AccountTypeHuman, "hash") + if err != nil { + t.Fatalf("CreateAccount: %v", err) + } + + locked, err := d.IsLockedOut(acct.ID) + if err != nil { + t.Fatalf("IsLockedOut: %v", err) + } + if locked { + t.Fatal("fresh account should not be locked out") + } +} + +// TestLockoutThreshold verifies that IsLockedOut returns true after +// LockoutThreshold failures within LockoutWindow. +func TestLockoutThreshold(t *testing.T) { + d := openTestDB(t) + acct, err := d.CreateAccount("locktest2", model.AccountTypeHuman, "hash") + if err != nil { + t.Fatalf("CreateAccount: %v", err) + } + + // Use a short window so test runs fast. + origWindow := LockoutWindow + origThreshold := LockoutThreshold + LockoutWindow = 5 * time.Second + LockoutThreshold = 3 + t.Cleanup(func() { + LockoutWindow = origWindow + LockoutThreshold = origThreshold + }) + + for i := 0; i < 3; i++ { + if err := d.RecordLoginFailure(acct.ID); err != nil { + t.Fatalf("RecordLoginFailure %d: %v", i+1, err) + } + } + + locked, err := d.IsLockedOut(acct.ID) + if err != nil { + t.Fatalf("IsLockedOut: %v", err) + } + if !locked { + t.Fatal("account should be locked after reaching threshold") + } +} + +// TestLockoutClearedOnSuccess verifies ClearLoginFailures removes the record +// and IsLockedOut returns false afterwards. +func TestLockoutClearedOnSuccess(t *testing.T) { + d := openTestDB(t) + acct, err := d.CreateAccount("locktest3", model.AccountTypeHuman, "hash") + if err != nil { + t.Fatalf("CreateAccount: %v", err) + } + + origThreshold := LockoutThreshold + LockoutThreshold = 2 + t.Cleanup(func() { LockoutThreshold = origThreshold }) + + for i := 0; i < 2; i++ { + if err := d.RecordLoginFailure(acct.ID); err != nil { + t.Fatalf("RecordLoginFailure %d: %v", i+1, err) + } + } + + locked, err := d.IsLockedOut(acct.ID) + if err != nil || !locked { + t.Fatalf("expected locked=true, got locked=%v err=%v", locked, err) + } + + if err := d.ClearLoginFailures(acct.ID); err != nil { + t.Fatalf("ClearLoginFailures: %v", err) + } + + locked, err = d.IsLockedOut(acct.ID) + if err != nil { + t.Fatalf("IsLockedOut after clear: %v", err) + } + if locked { + t.Fatal("account should not be locked after ClearLoginFailures") + } +} + +// TestLockoutWindowExpiry verifies that a stale failure record (outside the +// window) does not cause a lockout. +func TestLockoutWindowExpiry(t *testing.T) { + d := openTestDB(t) + acct, err := d.CreateAccount("locktest4", model.AccountTypeHuman, "hash") + if err != nil { + t.Fatalf("CreateAccount: %v", err) + } + + origWindow := LockoutWindow + origThreshold := LockoutThreshold + LockoutWindow = 50 * time.Millisecond + LockoutThreshold = 2 + t.Cleanup(func() { + LockoutWindow = origWindow + LockoutThreshold = origThreshold + }) + + for i := 0; i < 2; i++ { + if err := d.RecordLoginFailure(acct.ID); err != nil { + t.Fatalf("RecordLoginFailure %d: %v", i+1, err) + } + } + + // Wait for the window to expire. + time.Sleep(60 * time.Millisecond) + + locked, err := d.IsLockedOut(acct.ID) + if err != nil { + t.Fatalf("IsLockedOut after window expiry: %v", err) + } + if locked { + t.Fatal("account should not be locked after window has expired") + } +} diff --git a/internal/db/migrate.go b/internal/db/migrate.go index 26aee63..bf642a1 100644 --- a/internal/db/migrate.go +++ b/internal/db/migrate.go @@ -118,6 +118,19 @@ CREATE INDEX IF NOT EXISTS idx_audit_event ON audit_log (event_type); -- The salt must be stable across restarts so the passphrase always yields the same key. -- We allow NULL signing_key_enc/nonce temporarily until the first signing key is generated. ALTER TABLE server_config ADD COLUMN master_key_salt BLOB; +`, + }, + { + id: 3, + sql: ` +-- Track per-account failed login attempts for lockout enforcement (F-08). +-- One row per account; window_start resets when the window expires or on +-- a successful login. The DB layer enforces atomicity via UPDATE+INSERT. +CREATE TABLE IF NOT EXISTS failed_logins ( + account_id INTEGER NOT NULL PRIMARY KEY REFERENCES accounts(id) ON DELETE CASCADE, + window_start TEXT NOT NULL, + attempt_count INTEGER NOT NULL DEFAULT 1 +); `, }, } diff --git a/internal/grpcserver/accountservice.go b/internal/grpcserver/accountservice.go index b5cfe41..b62bcf7 100644 --- a/internal/grpcserver/accountservice.go +++ b/internal/grpcserver/accountservice.go @@ -15,6 +15,7 @@ import ( "git.wntrmute.dev/kyle/mcias/internal/auth" "git.wntrmute.dev/kyle/mcias/internal/db" "git.wntrmute.dev/kyle/mcias/internal/model" + "git.wntrmute.dev/kyle/mcias/internal/validate" ) type accountServiceServer struct { @@ -58,8 +59,9 @@ func (a *accountServiceServer) CreateAccount(ctx context.Context, req *mciasv1.C if err := a.s.requireAdmin(ctx); err != nil { return nil, err } - if req.Username == "" { - return nil, status.Error(codes.InvalidArgument, "username is required") + // Security (F-12): validate username length and character set. + if err := validate.Username(req.Username); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } accountType := model.AccountType(req.AccountType) if accountType != model.AccountTypeHuman && accountType != model.AccountTypeSystem { @@ -71,6 +73,10 @@ func (a *accountServiceServer) CreateAccount(ctx context.Context, req *mciasv1.C if req.Password == "" { return nil, status.Error(codes.InvalidArgument, "password is required for human accounts") } + // Security (F-13): enforce minimum length before hashing. + if err := validate.Password(req.Password); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } var err error passwordHash, err = auth.HashPassword(req.Password, auth.ArgonParams{ Time: a.s.cfg.Argon2.Time, diff --git a/internal/grpcserver/auth.go b/internal/grpcserver/auth.go index 1165aba..389cf51 100644 --- a/internal/grpcserver/auth.go +++ b/internal/grpcserver/auth.go @@ -52,15 +52,28 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest return nil, status.Error(codes.Unauthenticated, "invalid credentials") } + // Security: check per-account lockout before running Argon2 (F-08). + locked, lockErr := a.s.db.IsLockedOut(acct.ID) + if lockErr != nil { + a.s.logger.Error("lockout check", "error", lockErr) + } + 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") + } + ok, err := auth.VerifyPassword(req.Password, acct.PasswordHash) if err != nil || !ok { a.s.db.WriteAuditEvent(model.EventLoginFail, &acct.ID, nil, ip, `{"reason":"wrong_password"}`) //nolint:errcheck + _ = a.s.db.RecordLoginFailure(acct.ID) return nil, status.Error(codes.Unauthenticated, "invalid credentials") } if acct.TOTPRequired { if req.TotpCode == "" { a.s.db.WriteAuditEvent(model.EventLoginFail, &acct.ID, nil, ip, `{"reason":"totp_missing"}`) //nolint:errcheck + _ = a.s.db.RecordLoginFailure(acct.ID) return nil, status.Error(codes.Unauthenticated, "TOTP code required") } secret, err := crypto.OpenAESGCM(a.s.masterKey, acct.TOTPSecretNonce, acct.TOTPSecretEnc) @@ -71,10 +84,14 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest valid, err := auth.ValidateTOTP(secret, req.TotpCode) if err != nil || !valid { a.s.db.WriteAuditEvent(model.EventLoginTOTPFail, &acct.ID, nil, ip, `{"reason":"wrong_totp"}`) //nolint:errcheck + _ = a.s.db.RecordLoginFailure(acct.ID) return nil, status.Error(codes.Unauthenticated, "invalid credentials") } } + // Login succeeded: clear any outstanding failure counter. + _ = a.s.db.ClearLoginFailures(acct.ID) + expiry := a.s.cfg.DefaultExpiry() roles, err := a.s.db.GetRoles(acct.ID) if err != nil { diff --git a/internal/server/server.go b/internal/server/server.go index 14cb3a8..00269b2 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -26,6 +26,7 @@ import ( "git.wntrmute.dev/kyle/mcias/internal/model" "git.wntrmute.dev/kyle/mcias/internal/token" "git.wntrmute.dev/kyle/mcias/internal/ui" + "git.wntrmute.dev/kyle/mcias/internal/validate" "git.wntrmute.dev/kyle/mcias/web" ) @@ -67,18 +68,29 @@ func (s *Server) Handler() http.Handler { mux.Handle("POST /v1/token/validate", loginRateLimit(http.HandlerFunc(s.handleTokenValidate))) // API documentation: Swagger UI at /docs and raw spec at /docs/openapi.yaml. - // Both are served from the embedded web/static filesystem; no external - // files are read at runtime. + // Files are read from the embedded web/static filesystem at startup so that + // the handlers can write bytes directly without any redirect logic. staticFS, err := fs.Sub(web.StaticFS, "static") if err != nil { panic(fmt.Sprintf("server: sub fs: %v", err)) } - mux.HandleFunc("GET /docs", func(w http.ResponseWriter, r *http.Request) { - http.ServeFileFS(w, r, staticFS, "docs.html") + docsHTML, err := fs.ReadFile(staticFS, "docs.html") + if err != nil { + panic(fmt.Sprintf("server: read docs.html: %v", err)) + } + specYAML, err := fs.ReadFile(staticFS, "openapi.yaml") + if err != nil { + panic(fmt.Sprintf("server: read openapi.yaml: %v", err)) + } + mux.HandleFunc("GET /docs", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(docsHTML) }) - mux.HandleFunc("GET /docs/openapi.yaml", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("GET /docs/openapi.yaml", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/yaml") - http.ServeFileFS(w, r, staticFS, "openapi.yaml") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(specYAML) }) // Authenticated endpoints. @@ -189,11 +201,26 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { return } + // Security: check per-account lockout before running Argon2 (F-08). + // We still run a dummy Argon2 to equalise timing so an attacker cannot + // distinguish a locked account from a non-existent one. + locked, lockErr := s.db.IsLockedOut(acct.ID) + if lockErr != nil { + s.logger.Error("lockout check", "error", lockErr) + } + 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") + return + } + // Verify password. This is always run, even for system accounts (which have // no password hash), to maintain constant timing. ok, err := auth.VerifyPassword(req.Password, acct.PasswordHash) if err != nil || !ok { s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"wrong_password"}`) + _ = s.db.RecordLoginFailure(acct.ID) middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized") return } @@ -202,6 +229,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { if acct.TOTPRequired { if req.TOTPCode == "" { s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"totp_missing"}`) + _ = s.db.RecordLoginFailure(acct.ID) middleware.WriteError(w, http.StatusUnauthorized, "TOTP code required", "totp_required") return } @@ -215,11 +243,15 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { valid, err := auth.ValidateTOTP(secret, req.TOTPCode) if err != nil || !valid { s.writeAudit(r, model.EventLoginTOTPFail, &acct.ID, nil, `{"reason":"wrong_totp"}`) + _ = s.db.RecordLoginFailure(acct.ID) middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized") return } } + // Login succeeded: clear any outstanding failure counter. + _ = s.db.ClearLoginFailures(acct.ID) + // Determine expiry. expiry := s.cfg.DefaultExpiry() roles, err := s.db.GetRoles(acct.ID) @@ -489,8 +521,10 @@ func (s *Server) handleCreateAccount(w http.ResponseWriter, r *http.Request) { return } - if req.Username == "" { - middleware.WriteError(w, http.StatusBadRequest, "username is required", "bad_request") + // Security (F-12): validate username length and character set before any DB + // operation to prevent log injection, stored-XSS, and storage abuse. + if err := validate.Username(req.Username); err != nil { + middleware.WriteError(w, http.StatusBadRequest, err.Error(), "bad_request") return } accountType := model.AccountType(req.Type) @@ -505,6 +539,11 @@ func (s *Server) handleCreateAccount(w http.ResponseWriter, r *http.Request) { middleware.WriteError(w, http.StatusBadRequest, "password is required for human accounts", "bad_request") return } + // Security (F-13): enforce minimum length before hashing. + if err := validate.Password(req.Password); err != nil { + middleware.WriteError(w, http.StatusBadRequest, err.Error(), "bad_request") + return + } var err error passwordHash, err = auth.HashPassword(req.Password, auth.ArgonParams{ Time: s.cfg.Argon2.Time, diff --git a/internal/ui/handlers_accounts.go b/internal/ui/handlers_accounts.go index 0b04869..605486c 100644 --- a/internal/ui/handlers_accounts.go +++ b/internal/ui/handlers_accounts.go @@ -7,6 +7,7 @@ import ( "git.wntrmute.dev/kyle/mcias/internal/auth" "git.wntrmute.dev/kyle/mcias/internal/model" + "git.wntrmute.dev/kyle/mcias/internal/validate" ) // knownRoles lists the built-in roles shown as checkboxes in the roles editor. @@ -44,8 +45,9 @@ func (u *UIServer) handleCreateAccount(w http.ResponseWriter, r *http.Request) { password := r.FormValue("password") accountTypeStr := r.FormValue("account_type") - if username == "" { - u.renderError(w, r, http.StatusBadRequest, "username is required") + // Security (F-12): validate username length and character set. + if err := validate.Username(username); err != nil { + u.renderError(w, r, http.StatusBadRequest, err.Error()) return } @@ -56,6 +58,11 @@ func (u *UIServer) handleCreateAccount(w http.ResponseWriter, r *http.Request) { var passwordHash string if password != "" { + // Security (F-13): enforce minimum length before hashing. + if err := validate.Password(password); err != nil { + u.renderError(w, r, http.StatusBadRequest, err.Error()) + return + } argonCfg := auth.ArgonParams{ Time: u.cfg.Argon2.Time, Memory: u.cfg.Argon2.Memory, diff --git a/internal/ui/handlers_auth.go b/internal/ui/handlers_auth.go index d5d9d74..30a2f27 100644 --- a/internal/ui/handlers_auth.go +++ b/internal/ui/handlers_auth.go @@ -71,10 +71,23 @@ func (u *UIServer) handleLoginPost(w http.ResponseWriter, r *http.Request) { return } + // Security: check per-account lockout before running Argon2 (F-08). + locked, lockErr := u.db.IsLockedOut(acct.ID) + if lockErr != nil { + u.logger.Error("lockout check", "error", lockErr) + } + 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"}) + return + } + // Verify password. ok, err := auth.VerifyPassword(password, acct.PasswordHash) if err != nil || !ok { u.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"wrong_password"}`) + _ = u.db.RecordLoginFailure(acct.ID) u.render(w, "login", LoginData{Error: "invalid credentials"}) return } @@ -138,6 +151,7 @@ func (u *UIServer) handleTOTPStep(w http.ResponseWriter, r *http.Request) { valid, err := auth.ValidateTOTP(secret, totpCode) if err != nil || !valid { u.writeAudit(r, model.EventLoginTOTPFail, &acct.ID, nil, `{"reason":"wrong_totp"}`) + _ = u.db.RecordLoginFailure(acct.ID) // Re-issue a fresh nonce so the user can retry without going back to step 1. newNonce, nonceErr := u.issueTOTPNonce(acct.ID) if nonceErr != nil { @@ -171,6 +185,9 @@ func (u *UIServer) finishLogin(w http.ResponseWriter, r *http.Request, acct *mod } } + // Login succeeded: clear any outstanding failure counter. + _ = u.db.ClearLoginFailures(acct.ID) + tokenStr, claims, err := token.IssueToken(u.privKey, u.cfg.Tokens.Issuer, acct.UUID, roles, expiry) if err != nil { u.logger.Error("issue token", "error", err) diff --git a/internal/validate/validate.go b/internal/validate/validate.go new file mode 100644 index 0000000..65a179a --- /dev/null +++ b/internal/validate/validate.go @@ -0,0 +1,55 @@ +// Package validate provides shared input-validation helpers used by the REST, +// gRPC, and UI handlers. +package validate + +import ( + "fmt" + "regexp" +) + +// usernameRE is the allowed character set for usernames: alphanumeric plus a +// small set of punctuation that is safe in all contexts (URLs, HTML, logs). +// Length is enforced separately so the error message can be more precise. +// +// Security (F-12): rejecting control characters, null bytes, newlines, and +// unusual Unicode prevents log injection, stored-XSS via username display, +// and rendering anomalies in the admin UI. +var usernameRE = regexp.MustCompile(`^[a-zA-Z0-9._@-]+$`) + +// MinUsernameLen and MaxUsernameLen are the inclusive bounds on username length. +const ( + MinUsernameLen = 1 + MaxUsernameLen = 255 +) + +// Username returns nil if the username is valid, or a descriptive error if not. +// Valid usernames are 1–255 characters long and contain only alphanumeric +// characters and the symbols . _ @ - +func Username(username string) error { + l := len(username) + if l < MinUsernameLen || l > MaxUsernameLen { + return fmt.Errorf("username must be between %d and %d characters", MinUsernameLen, MaxUsernameLen) + } + if !usernameRE.MatchString(username) { + return fmt.Errorf("username may only contain letters, digits, and the characters . _ @ -") + } + return nil +} + +// MinPasswordLen is the minimum acceptable plaintext password length. +// +// Security (F-13): NIST SP 800-63B recommends a minimum of 8 characters; +// we use 12 to provide additional margin against offline brute-force attacks +// even though Argon2id is expensive. The check is performed at the handler +// level (before hashing) so Argon2id is never invoked with a trivially weak +// password. +const MinPasswordLen = 12 + +// Password returns nil if the plaintext password meets the minimum length +// requirement, or a descriptive error if not. +func Password(password string) error { + if len(password) < MinPasswordLen { + return fmt.Errorf("password must be at least %d characters", MinPasswordLen) + } + return nil +} diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go new file mode 100644 index 0000000..106dacd --- /dev/null +++ b/internal/validate/validate_test.go @@ -0,0 +1,72 @@ +package validate + +import ( + "strings" + "testing" +) + +func TestPasswordValid(t *testing.T) { + valid := []string{ + strings.Repeat("a", MinPasswordLen), + strings.Repeat("a", MinPasswordLen+1), + "correct horse battery staple", + "P@ssw0rd!2024XY", + } + for _, p := range valid { + if err := Password(p); err != nil { + t.Errorf("Password(%q) = %v, want nil", p, err) + } + } +} + +func TestPasswordTooShort(t *testing.T) { + short := []string{ + "", + "short", + strings.Repeat("a", MinPasswordLen-1), + } + for _, p := range short { + if err := Password(p); err == nil { + t.Errorf("Password(%q) = nil, want error", p) + } + } +} + +func TestUsernameValid(t *testing.T) { + valid := []string{ + "alice", + "Bob123", + "user.name", + "user_name", + "user-name", + "user@domain", + "a", + strings.Repeat("a", MaxUsernameLen), + } + for _, u := range valid { + if err := Username(u); err != nil { + t.Errorf("Username(%q) = %v, want nil", u, err) + } + } +} + +func TestUsernameInvalid(t *testing.T) { + invalid := []string{ + "", // empty + strings.Repeat("a", MaxUsernameLen+1), // too long + "user name", // space + "user\tname", // tab + "user\nname", // newline + "user\x00name", // null byte + "user