From 8e8541a5d84166f50c861db44fdb59011c6997ab Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sat, 31 Oct 2026 09:04:23 +0000 Subject: [PATCH] fix(web): count login attempts per address and while they are checked The login limiter counted failures per (address, username) only after the check finished, so attempts sent together all passed before the first failure was recorded, and one address could own every record of the bounded table and push out the one holding its own failures. An admitted attempt now holds a slot until it reports back, and an address is blocked as a whole after five times the per-username limit spread over any names, which also caps the records one peer can own. --- internal/web/controller/login_limiter.go | 121 +++++++++++++----- internal/web/controller/login_limiter_test.go | 42 ++++++ 2 files changed, 130 insertions(+), 33 deletions(-) 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") + } +}