From 87c17d4854714d3f6c52ca6210ac5a368eb92f6b Mon Sep 17 00:00:00 2001 From: mithral Date: Mon, 12 Oct 2026 00:45:27 -0400 Subject: [PATCH] Make the login limiter correct behind a reverse proxy ClientIP used r.RemoteAddr and ignored X-Forwarded-For. That was right about not trusting the header from an untrusted peer, but it is only half the design: behind a proxy every request arrives from the proxy, so the limiter key in handlers.go was "|" for every client on the internet. One attacker locked out a username for everyone, an attacker rotating usernames never hit a limit, and sessions.ip recorded the proxy so the device list was useless. - VELLUM_TRUSTED_PROXIES is a CIDR list. X-Forwarded-For is honoured only when RemoteAddr is inside it, and the chain is walked right-to-left so the first address that is not itself a trusted proxy wins. Everything to its left was appended by a client we do not control, so a spoofed leftmost entry is discarded. - VELLUM_PUBLIC_URL is the external origin. CookieSecure now defaults to true when it is https://, so a TLS deployment that forgets VELLUM_COOKIE_SECURE no longer ships a plaintext session cookie. - A malformed CIDR fails the boot rather than being skipped: silently dropping a proxy network widens the trust boundary, which is the exact failure this setting exists to prevent. ClientIP became a method on Authenticator so it can see the trusted set, which is why handlers.go and main.go changed as well. Verified red-green: neutering the trusted-peer path fails five ClientIP tests, including the spoofed-leftmost case. Known gap, pre-existing and not fixed here: this limiter still covers only /api/v1/auth/login. Jellyfin AuthenticateByName and Subsonic auth call auth.Authenticate with no limiting, so the same brute force is available through those two surfaces. --- cmd/vellum/main.go | 1 + internal/auth/clientip_test.go | 118 +++++++++++++++++++++++++++++++++ internal/auth/handlers.go | 2 +- internal/auth/middleware.go | 51 ++++++++++++-- internal/core/config.go | 45 +++++++++++-- internal/core/config_test.go | 67 +++++++++++++++++++ 6 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 internal/auth/clientip_test.go diff --git a/cmd/vellum/main.go b/cmd/vellum/main.go index 59203cf..782a07b 100644 --- a/cmd/vellum/main.go +++ b/cmd/vellum/main.go @@ -88,6 +88,7 @@ func run(cfg core.Config, log *slog.Logger) error { api := httpapi.New(db, log, version) authenticator := auth.NewAuthenticator(db, cfg.CookieName, cfg.SessionTTL, cfg.CookieSecure, log) + authenticator.TrustedProxies = cfg.TrustedProxies if !cfg.CookieSecure { log.Warn("VELLUM_COOKIE_SECURE is false; session cookies will be sent over plain HTTP. " + "Set it to true when serving over TLS.") diff --git a/internal/auth/clientip_test.go b/internal/auth/clientip_test.go new file mode 100644 index 0000000..86ebbe0 --- /dev/null +++ b/internal/auth/clientip_test.go @@ -0,0 +1,118 @@ +package auth + +import ( + "net" + "net/http" + "net/http/httptest" + "testing" +) + +// clientIPFor builds an Authenticator trusting the given CIDRs and returns the +// address it derives from a request arriving from peer with header XFF. +func clientIPFor(t *testing.T, peer, xff string, trusted ...string) string { + t.Helper() + a := &Authenticator{} + for _, c := range trusted { + _, n, err := net.ParseCIDR(c) + if err != nil { + t.Fatalf("parse test CIDR %q: %v", c, err) + } + a.TrustedProxies = append(a.TrustedProxies, n) + } + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = peer + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + return a.ClientIP(r) +} + +func TestClientIPUntrustedPeerIgnoresForwardedFor(t *testing.T) { + // The security case: an untrusted peer sending X-Forwarded-For must not be + // believed, or anyone could evade (or poison) the login rate limiter. + got := clientIPFor(t, "203.0.113.9:44321", "1.2.3.4", "172.16.0.0/12") + if got != "203.0.113.9" { + t.Errorf("ClientIP = %q, want the peer 203.0.113.9", got) + } +} + +func TestClientIPTrustedPeerUsesForwardedFor(t *testing.T) { + // The reverse-proxy case: every request arrives from the proxy, so without + // the header every client shares one limiter key. + got := clientIPFor(t, "172.20.0.5:51000", "1.2.3.4", "172.16.0.0/12") + if got != "1.2.3.4" { + t.Errorf("ClientIP = %q, want the client 1.2.3.4 from the header", got) + } +} + +func TestClientIPChain(t *testing.T) { + tests := []struct { + name string + peer string + xff string + trusted []string + want string + }{ + { + name: "no trusted proxies at all", + peer: "172.20.0.5:51000", xff: "1.2.3.4", + want: "172.20.0.5", + }, + { + name: "trusted peer without header", + peer: "172.20.0.5:51000", + trusted: []string{"172.16.0.0/12"}, + want: "172.20.0.5", + }, + { + name: "spans proxies on the right of the client", + peer: "172.20.0.5:51000", xff: "1.2.3.4, 172.18.0.1", + trusted: []string{"172.16.0.0/12"}, + want: "1.2.3.4", + }, + { + // A client-supplied leftmost entry is attacker-controlled, so the + // walk must stop at the first untrusted hop counting from the right. + name: "ignores a spoofed leftmost entry", + peer: "172.20.0.5:51000", xff: "9.9.9.9, 1.2.3.4, 172.18.0.1", + trusted: []string{"172.16.0.0/12"}, + want: "1.2.3.4", + }, + { + name: "every hop trusted falls back to the leftmost", + peer: "172.20.0.5:51000", xff: "172.19.0.1, 172.18.0.1, 172.17.0.1", + trusted: []string{"172.16.0.0/12"}, + want: "172.19.0.1", + }, + { + name: "multiple trusted networks", + peer: "127.0.0.1:51000", xff: "1.2.3.4", + trusted: []string{"172.16.0.0/12", "127.0.0.1/32"}, + want: "1.2.3.4", + }, + { + name: "unparseable hop is not trusted", + peer: "172.20.0.5:51000", xff: "not-an-ip", + trusted: []string{"172.16.0.0/12"}, + want: "not-an-ip", + }, + { + name: "remote address without a port", + peer: "172.20.0.5", xff: "1.2.3.4", + trusted: []string{"172.16.0.0/12"}, + want: "1.2.3.4", + }, + { + name: "untrusted peer without a port", + peer: "203.0.113.9", xff: "1.2.3.4", + want: "203.0.113.9", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := clientIPFor(t, tc.peer, tc.xff, tc.trusted...); got != tc.want { + t.Errorf("ClientIP = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 1127024..f37e567 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -83,7 +83,7 @@ func (h *Handlers) handleLogin(w http.ResponseWriter, r *http.Request) { return } - ip := ClientIP(r) + ip := h.auth.ClientIP(r) if !h.limiter.allow(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") diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 851473d..a629d81 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -6,6 +6,7 @@ import ( "log/slog" "net" "net/http" + "strings" "time" "github.com/mithral/vellum/internal/httpx" @@ -33,6 +34,9 @@ type Authenticator struct { CookieTTL time.Duration Secure bool Log *slog.Logger + // TrustedProxies are the networks allowed to name the real client in + // X-Forwarded-For. Empty means trust nobody. Set from cfg.TrustedProxies. + TrustedProxies []*net.IPNet // 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 @@ -182,12 +186,49 @@ func (a *Authenticator) TokenFromRequest(r *http.Request) (string, error) { } // ClientIP extracts a best-effort client address for logging and rate limiting. -// It deliberately does NOT trust X-Forwarded-For: behind an untrusted network -// that header is attacker-controlled, and this server is LAN-facing. -func ClientIP(r *http.Request) string { +// +// Behind a reverse proxy every request arrives from the proxy, so RemoteAddr +// alone keys the login limiter identically for every client on the internet: +// one attacker locks out a username for everyone, and sessions.ip records the +// proxy. X-Forwarded-For fixes that, but only for a peer we already trust — +// from an untrusted peer that header is attacker-controlled, so it is ignored +// outright and the connection's own address is used, as before. +// +// With a trusted peer the chain is walked right-to-left and the first address +// that is not itself a trusted proxy wins: everything to its right is a hop we +// control, and everything to its left was appended by a client we do not. If +// every hop is trusted the leftmost entry is the best available answer. +func (a *Authenticator) ClientIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - return r.RemoteAddr + host = r.RemoteAddr } - return host + if !a.trusts(host) { + return host + } + chain := r.Header.Get("X-Forwarded-For") + if chain == "" { + return host + } + hops := strings.Split(chain, ",") + for i := len(hops) - 1; i >= 0; i-- { + if hop := strings.TrimSpace(hops[i]); !a.trusts(hop) { + return hop + } + } + return strings.TrimSpace(hops[0]) +} + +// trusts reports whether ip falls inside one of the configured proxy networks. +func (a *Authenticator) trusts(ip string) bool { + parsed := net.ParseIP(ip) + if parsed == nil { + return false + } + for _, n := range a.TrustedProxies { + if n.Contains(parsed) { + return true + } + } + return false } diff --git a/internal/core/config.go b/internal/core/config.go index 1af166b..da8c4fe 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -2,6 +2,7 @@ package core import ( "fmt" + "net" "os" "path/filepath" "strconv" @@ -44,6 +45,15 @@ type Config struct { CookieName string CookieSecure bool + // PublicURL is the external origin Vellum is reached at, e.g. + // "https://media.example.com". Empty means "not published", which is the + // case for plain-HTTP LAN use. It is the signal that the deployment sits + // behind a TLS-terminating proxy, so it also drives the CookieSecure default. + PublicURL string + // TrustedProxies are the networks allowed to speak for a client in + // X-Forwarded-For. Empty means trust nobody: only RemoteAddr is used. + TrustedProxies []*net.IPNet + SessionTTL time.Duration ReadTimeout time.Duration WriteTimeout time.Duration @@ -54,6 +64,7 @@ type Config struct { // LoadConfig reads configuration from the environment and fills in defaults. // It does not create directories; call Validate for that. func LoadConfig() (Config, error) { + publicURL := env("VELLUM_PUBLIC_URL", "") c := Config{ Listen: env("VELLUM_LISTEN", ":7788"), DataDir: env("VELLUM_DATA_DIR", "./data"), @@ -69,10 +80,11 @@ func LoadConfig() (Config, error) { IGDBClientID: env("VELLUM_IGDB_CLIENT_ID", ""), IGDBClientSecret: env("VELLUM_IGDB_CLIENT_SECRET", ""), CookieName: env("VELLUM_COOKIE_NAME", "vellum_session"), - // Secure defaults to false so plain-HTTP development works. Production - // behind TLS must set VELLUM_COOKIE_SECURE=true; boot logs a warning - // when it is off. - CookieSecure: envBool("VELLUM_COOKIE_SECURE", false), + // Secure defaults to true for an https:// PublicURL — a published origin + // is TLS-terminated, so plain-HTTP dev still gets false with it unset. + // VELLUM_COOKIE_SECURE overrides both ways; boot logs a warning when off. + CookieSecure: envBool("VELLUM_COOKIE_SECURE", strings.HasPrefix(publicURL, "https://")), + PublicURL: publicURL, SessionTTL: envDuration("VELLUM_SESSION_TTL", 30*24*time.Hour), ReadTimeout: envDuration("VELLUM_READ_TIMEOUT", 15*time.Second), WriteTimeout: envDuration("VELLUM_WRITE_TIMEOUT", 0), // 0 = no limit; streaming needs this @@ -80,6 +92,12 @@ func LoadConfig() (Config, error) { ShutdownGrace: envDuration("VELLUM_SHUTDOWN_GRACE", 15*time.Second), } + proxies, err := envCIDRs("VELLUM_TRUSTED_PROXIES") + if err != nil { + return Config{}, err + } + c.TrustedProxies = proxies + abs, err := filepath.Abs(c.DataDir) if err != nil { return Config{}, fmt.Errorf("resolve data dir: %w", err) @@ -149,6 +167,25 @@ func envList(key string, def []string) []string { return out } +// envCIDRs parses a comma-separated CIDR list, e.g. +// "172.16.0.0/12,127.0.0.1/32". A malformed entry is an error rather than a +// silent skip: dropping a proxy network quietly widens the trust boundary, +// which is exactly the failure this setting exists to prevent. +func envCIDRs(key string) ([]*net.IPNet, error) { + var out []*net.IPNet + for _, part := range strings.Split(env(key, ""), ",") { + if part = strings.TrimSpace(part); part == "" { + continue + } + _, n, err := net.ParseCIDR(part) + if err != nil { + return nil, fmt.Errorf("%s: %w", key, err) + } + out = append(out, n) + } + return out, nil +} + func envDuration(key string, def time.Duration) time.Duration { v, ok := os.LookupEnv(key) if !ok || v == "" { diff --git a/internal/core/config_test.go b/internal/core/config_test.go index 2773261..6f67066 100644 --- a/internal/core/config_test.go +++ b/internal/core/config_test.go @@ -1,6 +1,7 @@ package core import ( + "net" "path/filepath" "strings" "testing" @@ -50,6 +51,72 @@ func TestLoadConfigFromEnv(t *testing.T) { } } +func TestLoadConfigTrustedProxies(t *testing.T) { + t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, 127.0.0.1/32") + + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if len(cfg.TrustedProxies) != 2 { + t.Fatalf("TrustedProxies = %v, want 2 entries", cfg.TrustedProxies) + } + if !cfg.TrustedProxies[0].Contains(net.ParseIP("172.20.0.5")) { + t.Errorf("TrustedProxies[0] = %v, want it to contain 172.20.0.5", cfg.TrustedProxies[0]) + } + if cfg.TrustedProxies[1].Contains(net.ParseIP("172.20.0.5")) { + t.Errorf("TrustedProxies[1] = %v, want it to contain only 127.0.0.1", cfg.TrustedProxies[1]) + } +} + +func TestLoadConfigRejectsBadTrustedProxy(t *testing.T) { + // A malformed entry must fail the boot: silently dropping it would quietly + // narrow the trust boundary and the operator would never know. + t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, not-a-cidr") + + _, err := LoadConfig() + if err == nil { + t.Fatal("expected error for malformed VELLUM_TRUSTED_PROXIES") + } + if !strings.Contains(err.Error(), "not-a-cidr") { + t.Errorf("error %q should name the offending entry", err) + } +} + +func TestCookieSecureDefaultsFromPublicURL(t *testing.T) { + tests := []struct { + name string + public string + cookie string // empty means unset + want bool + }{ + {name: "https origin secures the cookie", public: "https://media.example.com", want: true}, + {name: "http origin leaves it off", public: "http://media.example.com", want: false}, + {name: "unset origin leaves it off", want: false}, + {name: "explicit false overrides https", public: "https://media.example.com", cookie: "false", want: false}, + {name: "explicit true overrides http", public: "http://media.example.com", cookie: "true", want: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("VELLUM_PUBLIC_URL", tc.public) + // An empty value reads as unset to the env helpers, and t.Setenv + // restores the ambient value either way. + t.Setenv("VELLUM_COOKIE_SECURE", tc.cookie) + + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + if cfg.PublicURL != tc.public { + t.Errorf("PublicURL = %q, want %q", cfg.PublicURL, tc.public) + } + if cfg.CookieSecure != tc.want { + t.Errorf("CookieSecure = %v, want %v", cfg.CookieSecure, tc.want) + } + }) + } +} + func TestValidateRejectsMissingMediaDir(t *testing.T) { cfg := Config{ DataDir: t.TempDir(),