From 5bdaa8977fa62722211f12ad20c29302e9c18e75 Mon Sep 17 00:00:00 2001 From: codegirl-007 Date: Sat, 22 Aug 2026 11:54:10 -0700 Subject: [PATCH] Fix auth throttle DoS and serialize admin bootstrap. Evict/cap limiter keys, replace hard username lockouts with IP+user progressive delays cleared on success, and create bootstrap admins under the same advisory/mutex lock as role changes. --- internal/store/memory.go | 10 ++ internal/store/memory_test.go | 49 ++++++++- internal/store/postgres_store.go | 63 ++++++++++- internal/web/auth.go | 24 ++--- internal/web/server.go | 8 +- internal/web/throttle.go | 176 ++++++++++++++++++++++++++++--- internal/web/throttle_test.go | 122 +++++++++++++++++++++ 7 files changed, 413 insertions(+), 39 deletions(-) create mode 100644 internal/web/throttle_test.go diff --git a/internal/store/memory.go b/internal/store/memory.go index 39c86a7..7db3e67 100644 --- a/internal/store/memory.go +++ b/internal/store/memory.go @@ -54,7 +54,17 @@ func (m *Memory) CreateUser(_ context.Context, u *User) error { if u.CreatedAt == "" { u.CreatedAt = time.Now().UTC().Format(time.RFC3339) } + role := u.Role + if role == RoleAdmin { + for _, existing := range m.users { + if existing.Role == RoleAdmin { + role = RoleUser + break + } + } + } cp := *u + cp.Role = role cp.db = nil m.users[cp.ID] = &cp m.byName[cp.Username] = cp.ID diff --git a/internal/store/memory_test.go b/internal/store/memory_test.go index c5e3239..6c63541 100644 --- a/internal/store/memory_test.go +++ b/internal/store/memory_test.go @@ -9,14 +9,20 @@ import ( func TestMemoryConcurrentLastAdminDemotion(t *testing.T) { m := NewMemory() ctx := context.Background() - a := &User{Username: "admin_a", PasswordHash: "x", Role: RoleAdmin} - b := &User{Username: "admin_b", PasswordHash: "x", Role: RoleAdmin} + a := &User{Username: "admin_a", PasswordHash: "x", Role: RoleUser} + b := &User{Username: "admin_b", PasswordHash: "x", Role: RoleUser} if err := m.CreateUser(ctx, a); err != nil { t.Fatal(err) } if err := m.CreateUser(ctx, b); err != nil { t.Fatal(err) } + if err := m.SetUserRole(ctx, a.ID, RoleAdmin); err != nil { + t.Fatal(err) + } + if err := m.SetUserRole(ctx, b.ID, RoleAdmin); err != nil { + t.Fatal(err) + } var wg sync.WaitGroup errs := make(chan error, 2) @@ -54,3 +60,42 @@ func TestMemoryConcurrentLastAdminDemotion(t *testing.T) { t.Fatalf("admins remaining = %d, want 1", n) } } + +func TestMemoryConcurrentBootstrapAdmin(t *testing.T) { + m := NewMemory() + ctx := context.Background() + a := &User{Username: "boot_a", PasswordHash: "x", Role: RoleAdmin} + b := &User{Username: "boot_b", PasswordHash: "x", Role: RoleAdmin} + + var wg sync.WaitGroup + errs := make(chan error, 2) + wg.Add(2) + go func() { + defer wg.Done() + errs <- m.CreateUser(ctx, a) + }() + go func() { + defer wg.Done() + errs <- m.CreateUser(ctx, b) + }() + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatal(err) + } + } + n, err := m.CountAdmins(ctx) + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Fatalf("bootstrap race left %d admins, want 1", n) + } + if a.Role == RoleAdmin && b.Role == RoleAdmin { + t.Fatal("both users kept RoleAdmin") + } + if a.Role != RoleAdmin && b.Role != RoleAdmin { + t.Fatal("neither user is admin") + } +} diff --git a/internal/store/postgres_store.go b/internal/store/postgres_store.go index e054677..8dbe634 100644 --- a/internal/store/postgres_store.go +++ b/internal/store/postgres_store.go @@ -3,6 +3,12 @@ package store import ( "context" "database/sql" + "fmt" + "time" + + "github.com/google/uuid" + + "plumber/internal/store/sqlc" ) // Postgres implements Store against a sqlc-backed database. @@ -16,8 +22,63 @@ func NewPostgres(db *sql.DB) *Postgres { } func (p *Postgres) CreateUser(ctx context.Context, u *User) error { + if u == nil { + return fmt.Errorf("user: nil") + } + if u.Role != RoleUser && u.Role != RoleAdmin { + return fmt.Errorf("invalid role") + } + u.Username = NormalizeUsername(u.Username) + if u.ID == "" { + u.ID = uuid.NewString() + } + if u.Name == "" { + u.Name = u.Username + } + if u.CreatedAt == "" { + u.CreatedAt = time.Now().UTC().Format(time.RFC3339) + } + + if u.Role != RoleAdmin { + u.db = p.db + return u.Create(ctx) + } + + // Bootstrap admin: serialize count+insert so two setup-secret registers + // cannot both observe zero admins. + tx, err := p.db.BeginTx(ctx, nil) + if err != nil { + return err + } + defer tx.Rollback() + if _, err := tx.ExecContext(ctx, `SELECT pg_advisory_xact_lock($1)`, adminRoleLockKey); err != nil { + return err + } + q := sqlc.New(tx) + n, err := q.CountAdmins(ctx, string(RoleAdmin)) + if err != nil { + return err + } + role := RoleAdmin + if n > 0 { + role = RoleUser + } + if err := q.CreateUser(ctx, sqlc.CreateUserParams{ + ID: u.ID, + Username: u.Username, + Name: u.Name, + PasswordHash: u.PasswordHash, + Role: string(role), + CreatedAt: u.CreatedAt, + }); err != nil { + return err + } + if err := tx.Commit(); err != nil { + return err + } + u.Role = role u.db = p.db - return u.Create(ctx) + return nil } func (p *Postgres) UserByID(ctx context.Context, id string) (*User, error) { diff --git a/internal/web/auth.go b/internal/web/auth.go index 1b5c9e9..ac56f81 100644 --- a/internal/web/auth.go +++ b/internal/web/auth.go @@ -71,7 +71,9 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { username := strings.TrimSpace(r.PostFormValue("username")) password := r.PostFormValue("password") next := safeNext(r.PostFormValue("next")) - if !s.allowLoginAttempt(w, r, store.NormalizeUsername(username)) { + userKey := store.NormalizeUsername(username) + ip := s.clientIP(r) + if !s.allowLoginAttempt(w, r, userKey) { return } @@ -81,6 +83,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { hash = []byte(u.PasswordHash) } if err != nil || bcrypt.CompareHashAndPassword(hash, []byte(password)) != nil { + s.loginFail.record(loginFailKey(ip, userKey)) w.WriteHeader(http.StatusUnauthorized) s.exec(w, "login", authPage{ page: s.basePage(r, "Sign in"), @@ -90,6 +93,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { }) return } + s.loginFail.clear(loginFailKey(ip, userKey)) if err := s.sessions.RenewToken(r.Context()); err != nil { http.Error(w, "could not start session", http.StatusInternalServerError) return @@ -133,8 +137,8 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { return } role := store.RoleUser - if s.consumeAdminSetup(r, setupSecret) { - role = store.RoleAdmin + if setupSecretMatches(s.cfg.AdminSetupSecret, setupSecret) { + role = store.RoleAdmin // store downgrades if an admin already exists } u := &store.User{ Username: username, @@ -154,21 +158,11 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/", http.StatusSeeOther) } -// consumeAdminSetup grants first-admin when a strong one-time setup secret matches -// and no admin exists yet. Username alone is never enough. -func (s *Server) consumeAdminSetup(r *http.Request, provided string) bool { - want := s.cfg.AdminSetupSecret +func setupSecretMatches(want, provided string) bool { if want == "" || provided == "" { return false } - if subtle.ConstantTimeCompare([]byte(provided), []byte(want)) != 1 { - return false - } - n, err := s.store.CountAdmins(r.Context()) - if err != nil || n > 0 { - return false - } - return true + return subtle.ConstantTimeCompare([]byte(provided), []byte(want)) == 1 } func (s *Server) handleAuthPrompt(w http.ResponseWriter, r *http.Request) { diff --git a/internal/web/server.go b/internal/web/server.go index eee10be..5d84339 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -40,8 +40,8 @@ type Server struct { cfg Config static http.Handler loginIP *throttle - loginUser *throttle registerIP *throttle + loginFail *failureTracker } type page struct { @@ -136,9 +136,9 @@ func New(st store.Store, sessionStore scs.Store, templateFS fs.FS, staticFS fs.F tmpl: tmpl, cfg: cfg, static: http.StripPrefix("/static/", http.FileServer(http.FS(sub))), - loginIP: newThrottle(20, 15*time.Minute), - loginUser: newThrottle(10, 15*time.Minute), - registerIP: newThrottle(10, 15*time.Minute), + loginIP: newThrottle(20, 15*time.Minute, defaultThrottleMaxKeys), + registerIP: newThrottle(10, 15*time.Minute, defaultThrottleMaxKeys), + loginFail: newFailureTracker(15*time.Minute, defaultThrottleMaxKeys), }, nil } diff --git a/internal/web/throttle.go b/internal/web/throttle.go index 73f31c0..9956015 100644 --- a/internal/web/throttle.go +++ b/internal/web/throttle.go @@ -8,19 +8,26 @@ import ( "time" ) -// throttle is a simple sliding-window rate limiter for auth endpoints. +const defaultThrottleMaxKeys = 10_000 + +// throttle is a sliding-window rate limiter with expired-key eviction and a cap. type throttle struct { - mu sync.Mutex - hits map[string][]time.Time - limit int - window time.Duration + mu sync.Mutex + hits map[string][]time.Time + limit int + window time.Duration + maxKeys int } -func newThrottle(limit int, window time.Duration) *throttle { +func newThrottle(limit int, window time.Duration, maxKeys int) *throttle { + if maxKeys <= 0 { + maxKeys = defaultThrottleMaxKeys + } return &throttle{ - hits: map[string][]time.Time{}, - limit: limit, - window: window, + hits: map[string][]time.Time{}, + limit: limit, + window: window, + maxKeys: maxKeys, } } @@ -31,8 +38,46 @@ func (t *throttle) allow(key string) bool { t.mu.Lock() defer t.mu.Unlock() now := time.Now() + t.evictExpiredLocked(now) + + xs := pruneTimes(t.hits[key], now.Add(-t.window)) + if len(xs) >= t.limit { + if len(xs) == 0 { + delete(t.hits, key) + } else { + t.hits[key] = xs + } + return false + } + if _, exists := t.hits[key]; !exists && len(t.hits) >= t.maxKeys { + t.evictExpiredLocked(now) + if len(t.hits) >= t.maxKeys { + return false + } + } + t.hits[key] = append(xs, now) + return true +} + +func (t *throttle) lenKeys() int { + t.mu.Lock() + defer t.mu.Unlock() + return len(t.hits) +} + +func (t *throttle) evictExpiredLocked(now time.Time) { cutoff := now.Add(-t.window) - xs := t.hits[key] + for k, xs := range t.hits { + xs = pruneTimes(xs, cutoff) + if len(xs) == 0 { + delete(t.hits, k) + } else { + t.hits[k] = xs + } + } +} + +func pruneTimes(xs []time.Time, cutoff time.Time) []time.Time { n := 0 for _, ts := range xs { if ts.After(cutoff) { @@ -40,13 +85,102 @@ func (t *throttle) allow(key string) bool { n++ } } - xs = xs[:n] - if len(xs) >= t.limit { - t.hits[key] = xs - return false + return xs[:n] +} + +// failureTracker records auth failures for progressive delay (not a hard lockout). +type failureTracker struct { + mu sync.Mutex + fails map[string]failState + window time.Duration + maxKeys int +} + +type failState struct { + count int + last time.Time +} + +func newFailureTracker(window time.Duration, maxKeys int) *failureTracker { + if maxKeys <= 0 { + maxKeys = defaultThrottleMaxKeys + } + return &failureTracker{ + fails: map[string]failState{}, + window: window, + maxKeys: maxKeys, + } +} + +func (f *failureTracker) delay(key string) time.Duration { + if f == nil || key == "" { + return 0 + } + f.mu.Lock() + defer f.mu.Unlock() + now := time.Now() + f.evictExpiredLocked(now) + st, ok := f.fails[key] + if !ok { + return 0 + } + return progressiveDelay(st.count) +} + +func (f *failureTracker) record(key string) { + if f == nil || key == "" { + return + } + f.mu.Lock() + defer f.mu.Unlock() + now := time.Now() + f.evictExpiredLocked(now) + st := f.fails[key] + if st.count == 0 && len(f.fails) >= f.maxKeys { + return + } + st.count++ + st.last = now + f.fails[key] = st +} + +func (f *failureTracker) clear(key string) { + if f == nil || key == "" { + return + } + f.mu.Lock() + defer f.mu.Unlock() + delete(f.fails, key) +} + +func (f *failureTracker) lenKeys() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.fails) +} + +func (f *failureTracker) evictExpiredLocked(now time.Time) { + cutoff := now.Add(-f.window) + for k, st := range f.fails { + if st.last.Before(cutoff) { + delete(f.fails, k) + } + } +} + +func progressiveDelay(failCount int) time.Duration { + switch { + case failCount <= 1: + return 0 + case failCount == 2: + return 200 * time.Millisecond + case failCount == 3: + return 500 * time.Millisecond + case failCount == 4: + return time.Second + default: + return 2 * time.Second } - t.hits[key] = append(xs, now) - return true } func (s *Server) clientIP(r *http.Request) string { @@ -66,11 +200,19 @@ func authTooMany(w http.ResponseWriter) { http.Error(w, "too many attempts; try again later", http.StatusTooManyRequests) } +func loginFailKey(ip, usernameKey string) string { + return ip + "\x00" + usernameKey +} + func (s *Server) allowLoginAttempt(w http.ResponseWriter, r *http.Request, usernameKey string) bool { - if !s.loginIP.allow(s.clientIP(r)) || !s.loginUser.allow(usernameKey) { + ip := s.clientIP(r) + if !s.loginIP.allow(ip) { authTooMany(w) return false } + if d := s.loginFail.delay(loginFailKey(ip, usernameKey)); d > 0 { + time.Sleep(d) + } return true } diff --git a/internal/web/throttle_test.go b/internal/web/throttle_test.go new file mode 100644 index 0000000..b234980 --- /dev/null +++ b/internal/web/throttle_test.go @@ -0,0 +1,122 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" +) + +func TestThrottleWindowAndEviction(t *testing.T) { + th := newThrottle(2, 50*time.Millisecond, 100) + if !th.allow("a") || !th.allow("a") { + t.Fatal("first two should pass") + } + if th.allow("a") { + t.Fatal("third within window should fail") + } + time.Sleep(60 * time.Millisecond) + if !th.allow("a") { + t.Fatal("after window should pass") + } + // Expired empty keys should be removed on next allow of another key path. + time.Sleep(60 * time.Millisecond) + _ = th.allow("b") + if th.lenKeys() > 2 { + t.Fatalf("expected eviction of stale keys, got %d", th.lenKeys()) + } +} + +func TestThrottleMaxKeys(t *testing.T) { + th := newThrottle(5, time.Minute, 2) + if !th.allow("one") || !th.allow("two") { + t.Fatal("first keys should pass") + } + if th.allow("three") { + t.Fatal("over maxKeys should reject new key") + } + if th.lenKeys() != 2 { + t.Fatalf("keys=%d want 2", th.lenKeys()) + } +} + +func TestThrottleConcurrent(t *testing.T) { + th := newThrottle(50, time.Minute, 1000) + var wg sync.WaitGroup + var okCount int + var mu sync.Mutex + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if th.allow("same") { + mu.Lock() + okCount++ + mu.Unlock() + } + }() + } + wg.Wait() + if okCount != 50 { + t.Fatalf("ok=%d want 50", okCount) + } +} + +func TestFailureTrackerProgressiveAndClear(t *testing.T) { + f := newFailureTracker(time.Minute, 100) + if d := f.delay("k"); d != 0 { + t.Fatalf("fresh delay=%v", d) + } + f.record("k") + f.record("k") + if d := f.delay("k"); d != 200*time.Millisecond { + t.Fatalf("delay after 2 fails=%v", d) + } + f.clear("k") + if d := f.delay("k"); d != 0 { + t.Fatalf("after clear delay=%v", d) + } +} + +func TestFailureTrackerEvictsExpired(t *testing.T) { + f := newFailureTracker(30*time.Millisecond, 100) + f.record("old") + time.Sleep(40 * time.Millisecond) + _ = f.delay("other") // triggers eviction + if f.lenKeys() != 0 { + t.Fatalf("expired key remained, keys=%d", f.lenKeys()) + } +} + +func TestClientIPTrustProxy(t *testing.T) { + srv := &Server{cfg: Config{TrustProxy: true}} + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.1:1234" + req.Header.Set("X-Forwarded-For", "203.0.113.9, 10.0.0.1") + if got := srv.clientIP(req); got != "203.0.113.9" { + t.Fatalf("trusted xff got %q", got) + } + + srv.cfg.TrustProxy = false + if got := srv.clientIP(req); got != "10.0.0.1" { + t.Fatalf("untrusted should use RemoteAddr host, got %q", got) + } +} + +func TestNoGlobalUsernameHardLockout(t *testing.T) { + // Victim IP should still be allowed after another IP burns attempts for the same username. + srv := &Server{ + loginIP: newThrottle(20, time.Minute, 100), + loginFail: newFailureTracker(time.Minute, 100), + } + for i := 0; i < 20; i++ { + srv.loginFail.record(loginFailKey("1.1.1.1", "alice")) + } + victim := httptest.NewRequest(http.MethodPost, "/login", nil) + victim.RemoteAddr = "2.2.2.2:9" + w := httptest.NewRecorder() + if !srv.allowLoginAttempt(w, victim, "alice") { + t.Fatal("victim IP must not be hard-locked by username-only attempts") + } +}