fix(api): bound concurrent login bcrypt to shed CPU-pin floods
The public /auth/login route runs a full-cost bcrypt compare on every request — including the anti-enumeration dummy-hash compare for an unknown user — with no bound on how many run at once. A flood of concurrent logins therefore pins every core in bcrypt, starving the rest of the API. Cap the simultaneous compares with a small non-blocking concurrency limiter (a buffered-channel semaphore): a login that cannot take a slot is shed with 429 auth_busy before the compare, rather than piling more work onto the scheduler. The slot guards only the hash and is released the instant the compare returns. It is a concurrency cap, not a per-account lockout, so it never fences out the one admin trying to break-glass in, and the 429 lands before any credential distinction so it leaks nothing about the username. The cap follows the existing "zero disables" lever idiom (WakeCooldown, MaxRunningServers); cmd/felis wires it to the core count (floored at 4).
This commit is contained in:
4 files changed
+133
-3
No files matched your search
@@ -99,6 +99,17 @@ type API struct {
|
||||
// a positive value. Enforced via withinRunningCap on the wake path.
|
||||
MaxRunningServers int
|
||||
|
||||
// MaxConcurrentLogins bounds how many password logins may run their (CPU-costly)
|
||||
// bcrypt compare at once on the public /auth/login route. bcrypt is deliberately
|
||||
// expensive and the anti-enumeration path runs a full compare on EVERY request,
|
||||
// so an unbounded flood of concurrent logins would pin every core; capping the
|
||||
// simultaneous compares sheds the excess with a cheap 429 instead. Zero — the
|
||||
// default — disables the cap (same "zero disables" idiom as WakeCooldown /
|
||||
// MaxRunningServers); cmd/felis wires a positive value. It is a concurrency cap,
|
||||
// NOT a per-account lockout, so it never fences a break-glass admin out of the
|
||||
// one account they need. Enforced via loginLimiter in handleLogin.
|
||||
MaxConcurrentLogins int
|
||||
|
||||
// Now is the clock, injectable for tests. Defaults to time.Now.
|
||||
Now func() time.Time
|
||||
|
||||
@@ -107,6 +118,9 @@ type API struct {
|
||||
|
||||
otpCooldownOnce sync.Once
|
||||
otpCooldown *cooldownLimiter
|
||||
|
||||
loginCapOnce sync.Once
|
||||
loginCap *concurrencyLimiter
|
||||
}
|
||||
|
||||
// now returns the current time using the injected clock.
|
||||
@@ -136,6 +150,16 @@ func (a *API) otpLimiter() *cooldownLimiter {
|
||||
return a.otpCooldown
|
||||
}
|
||||
|
||||
// loginLimiter lazily builds the login bcrypt concurrency cap bound to
|
||||
// MaxConcurrentLogins. A zero cap yields a disabled limiter that admits every
|
||||
// caller, so a deployment (or test) that leaves it unset pays nothing.
|
||||
func (a *API) loginLimiter() *concurrencyLimiter {
|
||||
a.loginCapOnce.Do(func() {
|
||||
a.loginCap = newConcurrencyLimiter(a.MaxConcurrentLogins)
|
||||
})
|
||||
return a.loginCap
|
||||
}
|
||||
|
||||
// apiRoute is one served HTTP route. Each face exposes its routes as a single
|
||||
// table (internalAPIRoutes / externalAPIRoutes) so that one declaration drives
|
||||
// BOTH handler construction here AND the OpenAPI parity test (openapi_test.go):
|
||||
@@ -503,6 +527,46 @@ func (c *cooldownLimiter) release(name string, reservedAt time.Time) {
|
||||
}
|
||||
}
|
||||
|
||||
// ---- login concurrency cap ----
|
||||
|
||||
// concurrencyLimiter bounds how many holders may run a guarded section at once. It
|
||||
// backs the public login route's bcrypt cap (handleLogin): a buffered channel of n
|
||||
// tokens; acquire takes one WITHOUT blocking (returning ok=false when the section
|
||||
// is already full), release returns it. Unlike cooldownLimiter — a per-key time
|
||||
// window — this bounds simultaneity, not frequency, which is the right shape for a
|
||||
// CPU-costly section a flood would otherwise pin every core running. A non-positive
|
||||
// cap disables it (acquire always admits, release is a no-op), mirroring the "zero
|
||||
// disables" idiom of WakeCooldown and MaxRunningServers.
|
||||
type concurrencyLimiter struct {
|
||||
slots chan struct{}
|
||||
}
|
||||
|
||||
// newConcurrencyLimiter builds a limiter admitting at most n concurrent holders. A
|
||||
// non-positive n yields a disabled limiter (nil slots) that admits everyone.
|
||||
func newConcurrencyLimiter(n int) *concurrencyLimiter {
|
||||
if n <= 0 {
|
||||
return &concurrencyLimiter{}
|
||||
}
|
||||
return &concurrencyLimiter{slots: make(chan struct{}, n)}
|
||||
}
|
||||
|
||||
// acquire tries to take a slot without blocking. It returns a release func and true
|
||||
// on success, or nil and false when the section is already at capacity. The disabled
|
||||
// limiter (nil slots) always admits and returns a no-op release. release MUST be
|
||||
// called exactly once on the success path, so it reads naturally as `release, ok :=
|
||||
// l.acquire(); if !ok { shed }; defer/inline release()`.
|
||||
func (l *concurrencyLimiter) acquire() (release func(), ok bool) {
|
||||
if l.slots == nil {
|
||||
return func() {}, true
|
||||
}
|
||||
select {
|
||||
case l.slots <- struct{}{}:
|
||||
return func() { <-l.slots }, true
|
||||
default:
|
||||
return nil, false
|
||||
}
|
||||
}
|
||||
|
||||
// ---- running-server cap ----
|
||||
|
||||
// withinRunningCap reports whether waking info's server is allowed under the
|
||||
|
||||
@@ -82,7 +82,24 @@ func (a *API) handleLogin(w http.ResponseWriter, r *http.Request) {
|
||||
if u != nil && u.PasswordHash != "" {
|
||||
hash = []byte(u.PasswordHash)
|
||||
}
|
||||
if bcrypt.CompareHashAndPassword(hash, []byte(body.Password)) != nil || u == nil || u.PasswordHash == "" {
|
||||
|
||||
// Bound concurrent bcrypt: this public route runs a full-cost compare on every
|
||||
// request (the anti-enumeration dummy included), so an unbounded flood of
|
||||
// simultaneous logins would pin every core. Take one of a fixed number of compare
|
||||
// slots and shed the excess with a 429 rather than adding to the CPU pile. The
|
||||
// slot guards only the hash — it is released the instant the compare returns,
|
||||
// before the session I/O — and being a concurrency cap (not a per-username
|
||||
// lockout) it never fences the break-glass admin out. The 429 lands before any
|
||||
// credential distinction, so it leaks nothing about the username either.
|
||||
release, ok := a.loginLimiter().acquire()
|
||||
if !ok {
|
||||
writeError(w, r, newError(http.StatusTooManyRequests, "auth_busy",
|
||||
"authentication is busy; retry in a moment"))
|
||||
return
|
||||
}
|
||||
matched := bcrypt.CompareHashAndPassword(hash, []byte(body.Password)) == nil
|
||||
release()
|
||||
if !matched || u == nil || u.PasswordHash == "" {
|
||||
writeError(w, r, errInvalidCredentials)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -121,6 +121,45 @@ func TestHandleLoginContentTypeGuard(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleLoginConcurrencyCap pins the audit-hardening bound on the public login
|
||||
// route: bcrypt is CPU-costly and runs on every request (the anti-enumeration dummy
|
||||
// included), so at most MaxConcurrentLogins compares may be in flight at once and the
|
||||
// excess is shed with a 429 rather than piling more onto every core. Holding the sole
|
||||
// slot makes the next login — with otherwise-valid credentials — return 429 auth_busy
|
||||
// with no cookie BEFORE any credential check; releasing it lets the identical request
|
||||
// succeed, proving the 429 was the cap, not the password. A concurrency cap, not a
|
||||
// per-account lockout: the same account gets in the moment the burst clears.
|
||||
func TestHandleLoginConcurrencyCap(t *testing.T) {
|
||||
api, _ := seedAuthAPI(t, "correct-horse-battery", false)
|
||||
api.MaxConcurrentLogins = 1
|
||||
h := api.ExternalHandler()
|
||||
body := `{"username":"owner","password":"correct-horse-battery"}`
|
||||
|
||||
// Occupy the one compare slot so the handler finds the cap full. loginLimiter is
|
||||
// lazily built from MaxConcurrentLogins (set just above), so this and the handler
|
||||
// share the same one-token limiter.
|
||||
release, ok := api.loginLimiter().acquire()
|
||||
if !ok {
|
||||
t.Fatal("could not acquire the sole login slot in test setup")
|
||||
}
|
||||
w := do(h, "POST", "/api/v1/auth/login", body, jsonHeader)
|
||||
if w.Code != http.StatusTooManyRequests {
|
||||
t.Fatalf("with the slot held: code = %d, want 429 (%s)", w.Code, w.Body.String())
|
||||
}
|
||||
if code := decodeErr(t, w); code != "auth_busy" {
|
||||
t.Fatalf("error code = %q, want auth_busy", code)
|
||||
}
|
||||
if len(w.Result().Cookies()) != 0 {
|
||||
t.Fatal("no session cookie may be set on a shed login")
|
||||
}
|
||||
|
||||
// Release the slot: the identical request now runs the compare and succeeds.
|
||||
release()
|
||||
if w := do(h, "POST", "/api/v1/auth/login", body, jsonHeader); w.Code != http.StatusOK {
|
||||
t.Fatalf("after releasing the slot: code = %d, want 200 (%s)", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleLoginInvalidCredentials proves the anti-enumeration uniformity: a wrong
|
||||
// password and an unknown username return the SAME 401 invalid_credentials with no
|
||||
// cookie, so a caller cannot learn which usernames carry a password.
|
||||
|
||||
Reference in new issue
Block a user