diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 1ef883c..abb01c0 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -550,3 +550,34 @@ func TestAdminCreatesUserAndGrants(t *testing.T) { } func itoa(n int64) string { return strconv.FormatInt(n, 10) } + +// ── login rate limiting ───────────────────────────────────────────────── + +// AllowLogin/ResetLogin are what every password-checking surface calls, so the +// limiter can sit on the Authenticator and one budget covers all of them. +func TestAllowLoginRefusesAfterTenAndResetClears(t *testing.T) { + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + a := NewAuthenticator(nil, "", time.Hour, false, log) + + for i := 1; i <= 10; i++ { + if !a.AllowLogin("10.0.0.1", "alice") { + t.Fatalf("attempt %d refused, want it under the limit", i) + } + } + if a.AllowLogin("10.0.0.1", "alice") { + t.Error("11th attempt was allowed") + } + + // The window is per ip and username, so nobody else is locked out. + if !a.AllowLogin("10.0.0.2", "alice") { + t.Error("another ip was refused") + } + if !a.AllowLogin("10.0.0.1", "bob") { + t.Error("another username was refused") + } + + a.ResetLogin("10.0.0.1", "alice") + if !a.AllowLogin("10.0.0.1", "alice") { + t.Error("attempt refused after a successful login cleared the window") + } +} diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index f37e567..54e4a4a 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -5,7 +5,6 @@ import ( "log/slog" "net/http" "strconv" - "sync" "time" "github.com/mithral/vellum/internal/httpx" @@ -13,18 +12,13 @@ import ( // Handlers serves the authentication and user-management endpoints. type Handlers struct { - auth *Authenticator - log *slog.Logger - limiter *loginLimiter + auth *Authenticator + log *slog.Logger } // NewHandlers builds the auth HTTP layer. func NewHandlers(a *Authenticator, log *slog.Logger) *Handlers { - return &Handlers{ - auth: a, - log: log, - limiter: newLoginLimiter(10, 5*time.Minute), - } + return &Handlers{auth: a, log: log} } // Register mounts the routes onto the shared mux. @@ -84,7 +78,7 @@ func (h *Handlers) handleLogin(w http.ResponseWriter, r *http.Request) { } ip := h.auth.ClientIP(r) - if !h.limiter.allow(ip + "|" + req.Username) { + if !h.auth.AllowLogin(ip, req.Username) { h.log.Warn("login rate limited", "ip", ip, "username", req.Username) httpx.WriteError(w, http.StatusTooManyRequests, "too many login attempts, try again later") return @@ -112,7 +106,7 @@ func (h *Handlers) handleLogin(w http.ResponseWriter, r *http.Request) { } h.auth.SetSessionCookie(w, token, time.Now().Add(h.auth.CookieTTL)) - h.limiter.reset(ip + "|" + req.Username) + h.auth.ResetLogin(ip, req.Username) h.log.Info("login ok", "user", u.Username, "ip", ip) httpx.WriteJSON(w, http.StatusOK, map[string]any{ "user": toDTO(u), @@ -313,59 +307,3 @@ func pathInt(w http.ResponseWriter, r *http.Request, name string) (int64, bool) } return n, true } - -// ── login rate limiting ──────────────────────────────────────────────── - -// loginLimiter is a fixed-window counter keyed by "ip|username". -// -// In-memory is the right scope for a single-process server: it cannot be -// bypassed by restarting a client, and there is no second process to share it -// with. Persisting failed-login counts would be overkill for a LAN server. -type loginLimiter struct { - mu sync.Mutex - hits map[string]*hitWindow - max int - window time.Duration -} - -type hitWindow struct { - count int - reset time.Time -} - -func newLoginLimiter(max int, window time.Duration) *loginLimiter { - return &loginLimiter{hits: make(map[string]*hitWindow), max: max, window: window} -} - -func (l *loginLimiter) allow(key string) bool { - l.mu.Lock() - defer l.mu.Unlock() - - now := time.Now() - h, ok := l.hits[key] - if !ok || now.After(h.reset) { - l.hits[key] = &hitWindow{count: 1, reset: now.Add(l.window)} - l.pruneLocked(now) - return true - } - h.count++ - return h.count <= l.max -} - -func (l *loginLimiter) reset(key string) { - l.mu.Lock() - defer l.mu.Unlock() - delete(l.hits, key) -} - -// pruneLocked drops expired windows so the map cannot grow without bound. -func (l *loginLimiter) pruneLocked(now time.Time) { - if len(l.hits) < 1000 { - return - } - for k, h := range l.hits { - if now.After(h.reset) { - delete(l.hits, k) - } - } -} diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index a629d81..88e000d 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "strings" + "sync" "time" "github.com/mithral/vellum/internal/httpx" @@ -38,6 +39,12 @@ type Authenticator struct { // X-Forwarded-For. Empty means trust nobody. Set from cfg.TrustedProxies. TrustedProxies []*net.IPNet + // limiter throttles password attempts. It lives here rather than on any one + // handler so that every surface which checks a password — the web login, + // the Jellyfin login, Subsonic's per-request auth — spends one budget, and + // an attacker cannot multiply it by guessing through a different surface. + limiter *loginLimiter + // touchEvery is the minimum age of a session's last_seen before Middleware // will write it again. A zero value means "touch on every request" (the // pre-throttling behaviour), which is a safe fallback for hand-built values. @@ -55,6 +62,7 @@ func NewAuthenticator(db *sql.DB, cookieName string, ttl time.Duration, secure b CookieTTL: ttl, Secure: secure, Log: log, + limiter: newLoginLimiter(10, 5*time.Minute), touchEvery: defaultTouchEvery, } } @@ -232,3 +240,75 @@ func (a *Authenticator) trusts(ip string) bool { } return false } + +// ── login rate limiting ───────────────────────────────────────────────── + +// AllowLogin records a password attempt and reports whether it is still within +// budget. Call it before checking a password, on every surface that checks one. +func (a *Authenticator) AllowLogin(ip, username string) bool { + return a.limiter.allow(loginKey(ip, username)) +} + +// ResetLogin clears the window after a successful login, so a user who fumbled +// a few guesses is not left locked out of their own account. +func (a *Authenticator) ResetLogin(ip, username string) { + a.limiter.reset(loginKey(ip, username)) +} + +// loginKey builds the limiter key. It lives here so the web login, Jellyfin and +// Subsonic all count one thing as one attempt. +func loginKey(ip, username string) string { return ip + "|" + username } + +// loginLimiter is a fixed-window counter keyed by "ip|username". +// +// In-memory is the right scope for a single-process server: it cannot be +// bypassed by restarting a client, and there is no second process to share it +// with. Persisting failed-login counts would be overkill for a LAN server. +type loginLimiter struct { + mu sync.Mutex + hits map[string]*hitWindow + max int + window time.Duration +} + +type hitWindow struct { + count int + reset time.Time +} + +func newLoginLimiter(max int, window time.Duration) *loginLimiter { + return &loginLimiter{hits: make(map[string]*hitWindow), max: max, window: window} +} + +func (l *loginLimiter) allow(key string) bool { + l.mu.Lock() + defer l.mu.Unlock() + + now := time.Now() + h, ok := l.hits[key] + if !ok || now.After(h.reset) { + l.hits[key] = &hitWindow{count: 1, reset: now.Add(l.window)} + l.pruneLocked(now) + return true + } + h.count++ + return h.count <= l.max +} + +func (l *loginLimiter) reset(key string) { + l.mu.Lock() + defer l.mu.Unlock() + delete(l.hits, key) +} + +// pruneLocked drops expired windows so the map cannot grow without bound. +func (l *loginLimiter) pruneLocked(now time.Time) { + if len(l.hits) < 1000 { + return + } + for k, h := range l.hits { + if now.After(h.reset) { + delete(l.hits, k) + } + } +} diff --git a/internal/jellyfin/jellyfin_test.go b/internal/jellyfin/jellyfin_test.go index b59e9fb..b1e1932 100644 --- a/internal/jellyfin/jellyfin_test.go +++ b/internal/jellyfin/jellyfin_test.go @@ -309,6 +309,66 @@ func TestStreamRefusesUnreadableItem(t *testing.T) { // ── authenticate ──────────────────────────────────────────────────────── +// postLogin calls /Users/AuthenticateByName and returns the status code. +func (f *fixture) postLogin(t *testing.T, username, pw string) int { + t.Helper() + req, err := http.NewRequest("POST", f.srv.URL+"/Users/AuthenticateByName", + strings.NewReader(`{"Username":"`+username+`","Pw":"`+pw+`"}`)) + if err != nil { + t.Fatalf("new request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + resp, err := f.srv.Client().Do(req) + if err != nil { + t.Fatalf("do: %v", err) + } + _ = resp.Body.Close() + return resp.StatusCode +} + +// This surface must share the login limiter with the web login, or a Jellyfin +// client is an unthrottled way to guess the same password. +func TestAuthenticateByNameIsRateLimited(t *testing.T) { + f := newFixture(t) + + for i := 1; i <= 10; i++ { + if code := f.postLogin(t, "admin", "wrong"); code != http.StatusUnauthorized { + t.Fatalf("attempt %d: status = %d, want 401", i, code) + } + } + if code := f.postLogin(t, "admin", "wrong"); code != http.StatusTooManyRequests { + t.Errorf("11th attempt: status = %d, want 429", code) + } + // The window is per ip and username: another account still logs in. + if code := f.postLogin(t, "granted", "pw"); code != http.StatusOK { + t.Errorf("another user: status = %d, want 200", code) + } +} + +// A correct password clears the window, so a user who fumbled a few guesses is +// not locked out of their own account for the rest of it. +func TestAuthenticateByNameSuccessResetsTheWindow(t *testing.T) { + f := newFixture(t) + + for i := 1; i <= 5; i++ { + if code := f.postLogin(t, "admin", "wrong"); code != http.StatusUnauthorized { + t.Fatalf("attempt %d: status = %d, want 401", i, code) + } + } + if code := f.postLogin(t, "admin", "pw"); code != http.StatusOK { + t.Fatalf("login after failures: status = %d, want 200", code) + } + // Had the success not cleared the counter, the sixth of these would be 429. + for i := 1; i <= 10; i++ { + if code := f.postLogin(t, "admin", "wrong"); code != http.StatusUnauthorized { + t.Fatalf("post-reset attempt %d: status = %d, want 401", i, code) + } + } + if code := f.postLogin(t, "admin", "wrong"); code != http.StatusTooManyRequests { + t.Errorf("11th post-reset attempt: status = %d, want 429", code) + } +} + func TestAuthenticateByName(t *testing.T) { f := newFixture(t) diff --git a/internal/jellyfin/system.go b/internal/jellyfin/system.go index d909a45..a1f0815 100644 --- a/internal/jellyfin/system.go +++ b/internal/jellyfin/system.go @@ -35,6 +35,15 @@ func (h *Handlers) handleAuthenticateByName(w http.ResponseWriter, r *http.Reque return } + ip := h.a.ClientIP(r) + if !h.a.AllowLogin(ip, req.Username) { + // Same wording as the web login: this surface shares its budget, and a + // client cannot tell which one refused it. + h.log.Warn("jellyfin login rate limited", "ip", ip, "username", req.Username) + httpx.WriteError(w, http.StatusTooManyRequests, "too many login attempts, try again later") + return + } + u, err := auth.Authenticate(r.Context(), h.db, req.Username, req.Pw) if err != nil { if errors.Is(err, auth.ErrInvalidCredentials) { @@ -46,6 +55,7 @@ func (h *Handlers) handleAuthenticateByName(w http.ResponseWriter, r *http.Reque h.fail(w, "authenticate", err) return } + h.a.ResetLogin(ip, req.Username) label := deviceLabel(r) token, err := apitoken.Mint(r.Context(), h.db, u.ID, apitoken.KindJellyfin, label) diff --git a/internal/subsonic/auth.go b/internal/subsonic/auth.go index 1997a58..3df4732 100644 --- a/internal/subsonic/auth.go +++ b/internal/subsonic/auth.go @@ -28,13 +28,30 @@ import ( // A missing user, a user with no Subsonic key, or a wrong credential all // collapse to "not authenticated": the caller must not be able to tell them // apart, and the response is always error code 40. -func (h *Handlers) authenticate(r *http.Request) (auth.User, bool) { +func (h *Handlers) authenticate(r *http.Request) (_ auth.User, ok bool) { ctx := r.Context() q := r.FormValue username := strings.TrimSpace(q("u")) apiKey := strings.TrimSpace(q("apiKey")) + // Subsonic has no login step: authenticate runs on EVERY request, so the + // login limiter is consulted only when a credential was actually supplied. + // Charging playback against the login budget would lock out a client that + // is simply using the server, and a request with no credentials must not + // touch the limiter at all. + if username != "" && (q("p") != "" || q("t") != "" || q("token") != "" || apiKey != "") { + ip := h.auth.ClientIP(r) + if !h.auth.AllowLogin(ip, username) { + return auth.User{}, false // The caller answers with code 40. + } + defer func() { + if ok { + h.auth.ResetLogin(ip, username) + } + }() + } + // apiKey auth carries no username; resolve the key to its owner. if username == "" { if apiKey == "" { diff --git a/internal/subsonic/subsonic_test.go b/internal/subsonic/subsonic_test.go index 8187c99..eb61eac 100644 --- a/internal/subsonic/subsonic_test.go +++ b/internal/subsonic/subsonic_test.go @@ -245,6 +245,55 @@ func TestAuthUnknownUserIsCode40(t *testing.T) { } } +// ── login rate limiting ───────────────────────────────────────────────── + +// Subsonic is a third way to guess a password, so it shares the login budget: +// otherwise the web login's limit is bypassed by pointing a client here. The +// refusal is indistinguishable from any other credential failure — code 40. +func TestFailedAuthIsRateLimited(t *testing.T) { + srv, f := newFixture(t) + bad := "u=alice&s=abc123&t=deadbeef&v=1.16.1&c=test&f=json" + + for i := 1; i <= 10; i++ { + root := jsonEnvelope(t, readBody(t, get(t, srv.URL+"/rest/ping.view?"+bad))) + if root["status"] != "failed" { + t.Fatalf("attempt %d: status = %v, want failed", i, root["status"]) + } + } + // The correct key is refused too: that is the limiter, not the key. + root := jsonEnvelope(t, readBody(t, get(t, srv.URL+"/rest/ping.view?"+f.authQuery()+"&f=json"))) + if root["status"] != "failed" { + t.Errorf("the correct key still worked after 10 failures; the limiter is not engaged") + } +} + +// The budget must move only on a failed check with a credential behind it. +// authenticate runs on EVERY request, so charging a working client would lock a +// listener out mid-album, and a request that supplies no credential must not +// spend anything either. +func TestSuccessAndUncredentialedDoNotConsumeTheBudget(t *testing.T) { + srv, f := newFixture(t) + + for i := 1; i <= 25; i++ { + // A username but no credential: charged against nothing, so a client + // that sends one cannot lock the real user out. + readBody(t, get(t, srv.URL+"/rest/ping.view?u=alice&v=1.16.1&c=test&f=json")) + } + root := jsonEnvelope(t, readBody(t, get(t, srv.URL+"/rest/ping.view?"+f.authQuery()+"&f=json"))) + if root["status"] != "ok" { + t.Fatalf("the correct key was refused after 25 requests carrying no credential") + } + + // A success does not spend budget either: authenticate runs on every + // request, so a listener browsing for hours must never be locked out. + for i := 1; i <= 25; i++ { + root := jsonEnvelope(t, readBody(t, get(t, srv.URL+"/rest/ping.view?"+f.authQuery()+"&f=json"))) + if root["status"] != "ok" { + t.Fatalf("request %d with the correct key: status = %v, want ok", i, root["status"]) + } + } +} + // ── system / browse ──────────────────────────────────────────────────── func TestPingJSONAndXML(t *testing.T) {