From 7a51c1d9c340103c23f68115602c6109f6fa497b Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Wed, 1 Jul 2026 21:01:28 +0900 Subject: [PATCH] fix(api): bound concurrent login bcrypt to shed CPU-pin floods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- cmd/felis/api.go | 14 ++++++- internal/api/api.go | 64 ++++++++++++++++++++++++++++++ internal/api/handlers_auth.go | 19 ++++++++- internal/api/handlers_auth_test.go | 39 ++++++++++++++++++ 4 files changed, 133 insertions(+), 3 deletions(-) diff --git a/cmd/felis/api.go b/cmd/felis/api.go index c3c2d65..d3a9ab4 100644 --- a/cmd/felis/api.go +++ b/cmd/felis/api.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "os" + goruntime "runtime" "time" "felis.lolicon.best/internal/api" @@ -137,6 +138,14 @@ func cmdAPI(args []string, stdout, stderr io.Writer) int { // agree on what local auth knows. repo := api.NewPGRepo(drv.DB()) + // Bound concurrent login bcrypt to roughly the core count (floored so even a 1–2 + // vCPU demo box tolerates a handful of simultaneous staff logins). bcrypt is + // CPU-costly and the public login route runs a full compare on every request, so + // this caps the work a login flood can pile on the scheduler; the excess is shed + // as a cheap 429. Staff password logins are rare (players never use this path), so + // the cap never bites legitimate use. + loginBcryptCap := max(goruntime.NumCPU(), 4) + a := &api.API{ Repo: repo, Cluster: api.NewK8sCluster(cl, cfg.K8s.Namespace), @@ -161,8 +170,9 @@ func cmdAPI(args []string, stdout, stderr io.Writer) int { RootDomain: cfg.Server.RootDomain, AdminHostname: cfg.Auth.AdminHostname, }, - RootDomain: cfg.Server.RootDomain, - WakeCooldown: 30 * time.Second, + RootDomain: cfg.Server.RootDomain, + WakeCooldown: 30 * time.Second, + MaxConcurrentLogins: loginBcryptCap, } fmt.Fprintln(stderr, "felis api: external face fails closed (Access JWKS key function not configured)") diff --git a/internal/api/api.go b/internal/api/api.go index ee6efb6..9becb18 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -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 diff --git a/internal/api/handlers_auth.go b/internal/api/handlers_auth.go index 2080415..47b3fe3 100644 --- a/internal/api/handlers_auth.go +++ b/internal/api/handlers_auth.go @@ -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 } diff --git a/internal/api/handlers_auth_test.go b/internal/api/handlers_auth_test.go index 9bd27ce..eb59a18 100644 --- a/internal/api/handlers_auth_test.go +++ b/internal/api/handlers_auth_test.go @@ -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.