diff --git a/internal/web/controller/login_limiter.go b/internal/web/controller/login_limiter.go index 6c9edaed2..760f85d88 100644 --- a/internal/web/controller/login_limiter.go +++ b/internal/web/controller/login_limiter.go @@ -14,6 +14,9 @@ const ( // caller-supplied username, so an unauthenticated attacker rotating // usernames would otherwise grow the map without bound. loginLimitMaxRecords = 10000 + // Failures one address may spread over all usernames, as a multiple of the per-username + // limit, before the address is blocked as a whole: bounds the records one peer can own. + loginLimitIPFactor = 5 ) var defaultLoginLimiter = newLoginLimiter(loginLimitMaxFailures, loginLimitWindow, loginLimitCooldown) @@ -25,10 +28,14 @@ type loginLimiter struct { window time.Duration cooldown time.Duration attempts map[string]*loginLimitRecord + ipAttempts map[string]*loginLimitRecord } +// loginLimitRecord counts failures and admitted attempts still being checked: a burst +// sent at once would otherwise all pass before bcrypt reports the first failure. type loginLimitRecord struct { failures []time.Time + inFlight []time.Time blockedUntil time.Time } @@ -39,27 +46,47 @@ func newLoginLimiter(maxFailures int, window, cooldown time.Duration) *loginLimi window: window, cooldown: cooldown, attempts: make(map[string]*loginLimitRecord), + ipAttempts: make(map[string]*loginLimitRecord), } } +// allow admits an attempt and reserves a slot for it in both the (ip, username) and the +// address-wide record; the caller reports the outcome through registerFailure or registerSuccess. func (l *loginLimiter) allow(ip, username string) (time.Time, bool) { l.mu.Lock() defer l.mu.Unlock() - key := loginLimitKey(ip, username) - record := l.attempts[key] - if record == nil { - return time.Time{}, true - } now := l.now() + ipKey := strings.TrimSpace(ip) + if blockedUntil, ok := l.admit(l.ipAttempts, ipKey, l.maxFailures*loginLimitIPFactor, now); !ok { + return blockedUntil, false + } + if blockedUntil, ok := l.admit(l.attempts, loginLimitKey(ip, username), l.maxFailures, now); !ok { + l.ipAttempts[ipKey].dropReservation() + return blockedUntil, false + } + return time.Time{}, true +} + +// admit refuses a blocked key, or one whose failures and pending attempts already fill the +// budget (starting its cooldown), and otherwise reserves a slot. Callers hold l.mu. +func (l *loginLimiter) admit(records map[string]*loginLimitRecord, key string, limit int, now time.Time) (time.Time, bool) { + record := records[key] + if record == nil { + l.evictForRoom(records, now) + record = &loginLimitRecord{} + records[key] = record + } if now.Before(record.blockedUntil) { return record.blockedUntil, false } record.blockedUntil = time.Time{} - record.failures = pruneLoginFailures(record.failures, now.Add(-l.window)) - if len(record.failures) == 0 { - delete(l.attempts, key) + record.prune(now.Add(-l.window)) + if len(record.failures)+len(record.inFlight) >= limit { + record.block(now.Add(l.cooldown)) + return record.blockedUntil, false } + record.inFlight = append(record.inFlight, now) return time.Time{}, true } @@ -68,18 +95,26 @@ func (l *loginLimiter) registerFailure(ip, username string) (time.Time, bool) { defer l.mu.Unlock() now := l.now() - key := loginLimitKey(ip, username) - record := l.attempts[key] - if record == nil { - l.evictForRoom(now) - record = &loginLimitRecord{} - l.attempts[key] = record + ipBlockedUntil, ipBlocked := l.fail(l.ipAttempts, strings.TrimSpace(ip), l.maxFailures*loginLimitIPFactor, now) + blockedUntil, blocked := l.fail(l.attempts, loginLimitKey(ip, username), l.maxFailures, now) + if blocked { + return blockedUntil, true } - record.failures = pruneLoginFailures(record.failures, now.Add(-l.window)) + return ipBlockedUntil, ipBlocked +} + +func (l *loginLimiter) fail(records map[string]*loginLimitRecord, key string, limit int, now time.Time) (time.Time, bool) { + record := records[key] + if record == nil { + l.evictForRoom(records, now) + record = &loginLimitRecord{} + records[key] = record + } + record.prune(now.Add(-l.window)) + record.dropReservation() record.failures = append(record.failures, now) - if len(record.failures) >= l.maxFailures { - record.failures = nil - record.blockedUntil = now.Add(l.cooldown) + if len(record.failures) >= limit { + record.block(now.Add(l.cooldown)) return record.blockedUntil, true } return time.Time{}, false @@ -89,39 +124,59 @@ func (l *loginLimiter) registerSuccess(ip, username string) { l.mu.Lock() defer l.mu.Unlock() delete(l.attempts, loginLimitKey(ip, username)) + if record := l.ipAttempts[strings.TrimSpace(ip)]; record != nil { + record.dropReservation() + } } -// evictForRoom keeps the attempts map bounded before inserting a new record. +func (r *loginLimitRecord) prune(cutoff time.Time) { + r.failures = pruneLoginFailures(r.failures, cutoff) + r.inFlight = pruneLoginFailures(r.inFlight, cutoff) +} + +func (r *loginLimitRecord) block(until time.Time) { + r.failures = nil + r.inFlight = nil + r.blockedUntil = until +} + +func (r *loginLimitRecord) dropReservation() { + if r != nil && len(r.inFlight) > 0 { + r.inFlight = r.inFlight[1:] + } +} + +// evictForRoom keeps a records map bounded before inserting a new record. // It first reclaims records that are no longer blocked and whose failures have // aged out of the window; if the map is still at the ceiling (a genuine -// broad flood), it drops one arbitrary record so memory can never grow past the -// cap. Callers hold l.mu. -func (l *loginLimiter) evictForRoom(now time.Time) { - if len(l.attempts) < loginLimitMaxRecords { +// broad flood), it drops one record that is neither blocked nor awaiting an +// outcome, then any, so memory can never grow past the cap. Callers hold l.mu. +func (l *loginLimiter) evictForRoom(records map[string]*loginLimitRecord, now time.Time) { + if len(records) < loginLimitMaxRecords { return } cutoff := now.Add(-l.window) - for key, record := range l.attempts { + for key, record := range records { if now.Before(record.blockedUntil) { continue } - record.failures = pruneLoginFailures(record.failures, cutoff) - if len(record.failures) == 0 { - delete(l.attempts, key) + record.prune(cutoff) + if len(record.failures) == 0 && len(record.inFlight) == 0 { + delete(records, key) } } - if len(l.attempts) < loginLimitMaxRecords { + if len(records) < loginLimitMaxRecords { return } - for key, record := range l.attempts { - if now.Before(record.blockedUntil) { + for key, record := range records { + if now.Before(record.blockedUntil) || len(record.inFlight) > 0 { continue } - delete(l.attempts, key) + delete(records, key) return } - for key := range l.attempts { - delete(l.attempts, key) + for key := range records { + delete(records, key) return } } diff --git a/internal/web/controller/login_limiter_test.go b/internal/web/controller/login_limiter_test.go index 7420a851d..166c941e2 100644 --- a/internal/web/controller/login_limiter_test.go +++ b/internal/web/controller/login_limiter_test.go @@ -118,3 +118,45 @@ func TestLoginLimiterSeparatesIPAndUsername(t *testing.T) { t.Fatal("different username should not be blocked") } } + +// Without a per-address bound, one address can fill the table with its own blocked +// records and evict the record holding its admin failures, resetting the count. +func TestLoginLimiterBlocksAnAddressSpreadingFailuresOverUsernames(t *testing.T) { + now := time.Date(2026, 5, 6, 12, 0, 0, 0, time.UTC) + limiter := newLoginLimiter(5, 5*time.Minute, 15*time.Minute) + limiter.now = func() time.Time { return now } + + for i := range 5 * loginLimitIPFactor { + if _, ok := limiter.allow("192.0.2.10", "decoy-"+strconv.Itoa(i)); !ok { + t.Fatalf("attempt %d refused before the address limit", i+1) + } + limiter.registerFailure("192.0.2.10", "decoy-"+strconv.Itoa(i)) + } + if _, ok := limiter.allow("192.0.2.10", "admin"); ok { + t.Fatal("an address past its failure budget may still try the admin account") + } + if _, ok := limiter.allow("192.0.2.11", "admin"); !ok { + t.Fatal("another address was blocked too") + } +} + +// The count must cover attempts admitted and still being checked: bcrypt is slow, and +// a burst sent at once would otherwise all pass before the first failure is recorded. +func TestLoginLimiterCountsAttemptsStillBeingChecked(t *testing.T) { + now := time.Date(2026, 5, 6, 12, 0, 0, 0, time.UTC) + limiter := newLoginLimiter(5, 5*time.Minute, 15*time.Minute) + limiter.now = func() time.Time { return now } + + for i := range 5 { + if _, ok := limiter.allow("192.0.2.10", "admin"); !ok { + t.Fatalf("attempt %d of a burst refused", i+1) + } + } + if _, ok := limiter.allow("192.0.2.10", "admin"); ok { + t.Fatal("a sixth concurrent attempt was admitted") + } + now = now.Add(20 * time.Minute) + if _, ok := limiter.allow("192.0.2.10", "admin"); !ok { + t.Fatal("attempts that never reported back still block after the window and cooldown") + } +}