diff --git a/CHANGELOG.md b/CHANGELOG.md index f317767..7c1a61f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version - ci: disable provenance attestation on release image push, so the ghcr.io manifest list shows only `linux/amd64`/`linux/arm64` (no `unknown/unknown`). +- security: optionally honour `X-Forwarded-For` for login/setup rate-limiting + when the request's direct peer is in the new `TRUSTED_PROXY_CIDR` list, + giving real per-client limits behind a reverse proxy instead of one global + bucket. Unset by default (unchanged `RemoteAddr`-only behaviour). ## [0.1.0] - 2026-07-15 diff --git a/cmd/panel/httpserver.go b/cmd/panel/httpserver.go index fdf3e8c..b721e04 100644 --- a/cmd/panel/httpserver.go +++ b/cmd/panel/httpserver.go @@ -26,12 +26,13 @@ func serveHTTP(ctx context.Context, cfg config, st *store.Store) error { domains := domain.NewService(st, domain.NewOpenDKIM(cfg.opendkimDir), apps, cfg.dkimSelectorDef) srvApp, err := web.New(st, domains, apps, web.Config{ - Hostname: cfg.hostname, - CookieSecure: cfg.cookieSecure, - MailLogPath: cfg.mailLog, - DataDir: cfg.dataDir, - DBPath: cfg.dbPath, - Version: buildinfo.Version, + Hostname: cfg.hostname, + CookieSecure: cfg.cookieSecure, + MailLogPath: cfg.mailLog, + DataDir: cfg.dataDir, + DBPath: cfg.dbPath, + Version: buildinfo.Version, + TrustedProxyCIDRs: cfg.trustedProxies, }, cfg.setupTokenPath) if err != nil { return err diff --git a/cmd/panel/main.go b/cmd/panel/main.go index 81e0541..2c6087a 100644 --- a/cmd/panel/main.go +++ b/cmd/panel/main.go @@ -12,10 +12,12 @@ import ( "flag" "fmt" "log" + "net" "os" "os/signal" "path/filepath" "strconv" + "strings" "sync" "syscall" @@ -56,6 +58,7 @@ type config struct { setupTokenPath string hostname string cookieSecure bool + trustedProxies []*net.IPNet opendkimDir string dkimSelectorDef string @@ -83,6 +86,11 @@ func loadConfig() config { // Secure cookies by default (spec 7.6.6); PANEL_COOKIE_SECURE=false is a // development-only escape hatch for testing over plain HTTP. cookieSecure: envDefault("PANEL_COOKIE_SECURE", "true") != "false", + // Reverse-proxy addresses allowed to supply X-Forwarded-For for + // rate-limiting (plan.md item A.1). Empty by default: an untrusted peer's + // XFF header is trivially forgeable, so it's ignored unless the panel is + // told which proxy to trust. + trustedProxies: parseTrustedProxies(os.Getenv("TRUSTED_PROXY_CIDR")), // Per-domain DKIM state (spec 6). The directory layout matches what // entrypoint.sh prepares (setgid, shared `selfpost` group). @@ -131,6 +139,37 @@ func envInt(key string, def int) int { return def } +// parseTrustedProxies parses a comma-separated list of CIDRs (bare IPs are +// accepted and treated as /32 or /128). Invalid entries are logged and +// skipped rather than failing startup, matching envInt's tolerance of +// misconfiguration. +func parseTrustedProxies(raw string) []*net.IPNet { + if raw == "" { + return nil + } + var nets []*net.IPNet + for _, part := range strings.Split(raw, ",") { + cidr := strings.TrimSpace(part) + if cidr == "" { + continue + } + if !strings.Contains(cidr, "/") { + if ip := net.ParseIP(cidr); ip != nil && ip.To4() != nil { + cidr += "/32" + } else { + cidr += "/128" + } + } + _, n, err := net.ParseCIDR(cidr) + if err != nil { + log.Printf("ignoring invalid TRUSTED_PROXY_CIDR entry %q: %v", part, err) + continue + } + nets = append(nets, n) + } + return nets +} + // run starts the panel's three roles and blocks until a shutdown signal or the // first fatal error from any role. A signal triggers a clean stop of all roles; // a role error cancels the others and is returned so the process exits non-zero diff --git a/deploy/.env.example b/deploy/.env.example index 1c5f942..c73d4cb 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -15,3 +15,11 @@ RATE_LIMIT_WINDOW_SECONDS=3600 # Days of send-log history kept before the background sweep deletes rows. SEND_LOG_RETENTION_DAYS=90 + +# Comma-separated CIDRs (bare IPs allowed) of reverse proxies allowed to +# supply X-Forwarded-For for login/setup rate-limiting. Leave unset unless +# you know the exact address of your reverse proxy — trusting the wrong +# source lets a client spoof its rate-limit key. Behind the default Apache +# host-network setup this is typically the Docker bridge gateway, e.g. +# TRUSTED_PROXY_CIDR=172.18.0.1 + diff --git a/docs/implementation-plan.md b/docs/implementation-plan.md index 011e576..d2aa895 100644 --- a/docs/implementation-plan.md +++ b/docs/implementation-plan.md @@ -22,7 +22,7 @@ ### A. Безопасность — hardening сверх обязательного 7.6 -1. **Rate-limit за обратным прокси кеится по `RemoteAddr`** ([internal/web/web.go](../internal/web/web.go) `clientIP`). За дефолтным Apache это адрес прокси → лимитеры логина и `/setup` фактически **глобальны**. Осознанный выбор: не парсить `X-Forwarded-For` (иначе тривиально обходится подделкой заголовка). По ТЗ 7.6.1 реальная защита setup — 128-бит энтропии токена, rate-limit там defense-in-depth. **Побочный эффект:** атакующий может исчерпать общий bucket логина (10 попыток/15 мин) и на 15 минут заблокировать вход легитимному админу (lockout-DoS). **Варианты:** (а) оставить как есть (просто, по ТЗ достаточно); (б) парсить XFF **только от доверенного прокси** (`TRUSTED_PROXY_CIDR`) → настоящий per-client лимит; (в) вместо жёсткой блокировки — экспоненциальная задержка ответа, чтобы brute-force тормозился, но легитимный вход не блокировался. +1. **Rate-limit за обратным прокси кеился по `RemoteAddr`** ([internal/web/web.go](../internal/web/web.go) `clientIP`). **Решено:** вариант (б) — парсить `X-Forwarded-For`, но только когда прямой peer (`RemoteAddr`) входит в `TRUSTED_PROXY_CIDR` (список CIDR через запятую, env, по умолчанию пусто); тогда используется последний элемент XFF (адрес, добавленный самим доверенным прокси). Без настройки `TRUSTED_PROXY_CIDR` поведение не меняется (лимит по `RemoteAddr`, глобальный за прокси). См. `deploy/.env.example`. 2. **Нет security-заголовков ответа** — панель не шлёт `Strict-Transport-Security`, `Content-Security-Policy`, `X-Frame-Options`/`frame-ancestors`, `X-Content-Type-Options: nosniff`, `Referrer-Policy`. XSS уже закрыт автоэкранированием `html/template` (7.6.7), CSRF — `SameSite=Lax`, но заголовки — дешёвый второй эшелон (clickjacking, downgrade, sniffing). **Вопрос:** эмитить их из панели (единый мидлварь, ~10 строк) или переложить на reverse-proxy и задокументировать в `deploy/`? Рекомендация — минимальный набор из панели (HSTS/nosniff/`frame-ancestors 'none'`/строгий CSP `default-src 'self'`), т.к. панель знает свою модель контента, а прокси у всех разный. 3. **CSRF — только `SameSite=Lax`, без токенов.** Достаточно для современных браузеров (все мутации — POST, все GET read-only), но не защищает при downgrade до старого браузера/особых прокси и не даёт защиты на уровне «per-request». **Вопрос:** считать `SameSite=Lax` достаточным для single-admin панели (моя рекомендация — да) или добавить double-submit CSRF-токен. 4. **Cookie без префикса `__Host-`.** Сейчас `selfpost_session` (`Secure`/`HttpOnly`/`SameSite=Lax`/`Path=/`). Префикс `__Host-` дал бы браузерный гарант «только HTTPS, только этот хост, без Domain». Мелочь, но бесплатная. **Вопрос:** переименовать (учесть dev-режим `PANEL_COOKIE_SECURE=false` — `__Host-` требует `Secure`, т.е. только когда secure включён). diff --git a/internal/web/handlers_auth.go b/internal/web/handlers_auth.go index 6213b3d..d50b892 100644 --- a/internal/web/handlers_auth.go +++ b/internal/web/handlers_auth.go @@ -50,7 +50,7 @@ func (s *Server) renderLogin(w http.ResponseWriter, status int, formErr string) func (s *Server) submitLogin(w http.ResponseWriter, r *http.Request) { // Brute-force throttle by client IP (spec 7.6.5). - if !s.loginLimiter.Allow(clientIP(r)) { + if !s.loginLimiter.Allow(clientIP(r, s.trustedProxies)) { s.renderLogin(w, http.StatusTooManyRequests, "Too many attempts. Please wait and try again.") return } diff --git a/internal/web/handlers_setup.go b/internal/web/handlers_setup.go index 6e26257..020aeeb 100644 --- a/internal/web/handlers_setup.go +++ b/internal/web/handlers_setup.go @@ -13,7 +13,7 @@ import ( // page, also 404. func (s *Server) handleSetup(w http.ResponseWriter, r *http.Request) { // Route-specific rate limit, separate from login (spec 7.6.1). - if !s.setupLimiter.Allow(clientIP(r)) { + if !s.setupLimiter.Allow(clientIP(r, s.trustedProxies)) { http.Error(w, "too many requests", http.StatusTooManyRequests) return } diff --git a/internal/web/web.go b/internal/web/web.go index 1595590..d896202 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -8,6 +8,7 @@ import ( "log" "net" "net/http" + "strings" "time" "codeberg.org/mix/selfpost/internal/app" @@ -37,6 +38,12 @@ type Config struct { DataDir string DBPath string Version string + // TrustedProxyCIDRs are the reverse-proxy addresses allowed to supply + // X-Forwarded-For (plan.md item A.1: TRUSTED_PROXY_CIDR). A request whose + // direct peer (RemoteAddr) is not in this list never has its XFF header + // honoured, so the header can't be spoofed by anyone but a trusted proxy. + // Empty (the default) keeps rate-limiting keyed on RemoteAddr only. + TrustedProxyCIDRs []*net.IPNet } // Server is the panel HTTP application. @@ -51,6 +58,8 @@ type Server struct { loginLimiter *rateLimiter setupLimiter *rateLimiter + + trustedProxies []*net.IPNet } // New builds the panel server. setupTokenPath is where the current setup token @@ -74,6 +83,8 @@ func New(st *store.Store, domains *domain.Service, apps *app.Service, cfg Config setupLimiter: newRateLimiter(10, time.Minute), // Login: throttle brute-force by IP (spec 7.6.5). loginLimiter: newRateLimiter(10, 15*time.Minute), + + trustedProxies: cfg.TrustedProxyCIDRs, } s.setup = newSetupManager(st, cfg.Hostname, setupTokenPath) return s, nil @@ -144,18 +155,42 @@ func handleHealth(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("ok\n")) } -// clientIP extracts the peer IP for rate-limiting. It uses the transport peer -// (RemoteAddr), not client-supplied headers, so it cannot be spoofed; behind a -// reverse proxy this is the proxy address, which is an acceptable backstop for -// a single-admin panel. -func clientIP(r *http.Request) string { +// clientIP extracts the peer IP for rate-limiting. By default it is the +// transport peer (RemoteAddr), which cannot be spoofed. If RemoteAddr matches +// one of trustedProxies, the last entry of X-Forwarded-For is used instead — +// that is the address the trusted proxy itself appended, so a client can't +// forge it by sending its own XFF header (plan.md item A.1). With no trusted +// proxies configured, behind a reverse proxy this is the proxy's own address, +// which is an acceptable backstop for a single-admin panel. +func clientIP(r *http.Request, trustedProxies []*net.IPNet) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - return r.RemoteAddr + host = r.RemoteAddr } + + if len(trustedProxies) > 0 { + if peer := net.ParseIP(host); peer != nil && ipInAny(peer, trustedProxies) { + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + parts := strings.Split(xff, ",") + if ip := net.ParseIP(strings.TrimSpace(parts[len(parts)-1])); ip != nil { + return ip.String() + } + } + } + } + return host } +func ipInAny(ip net.IP, nets []*net.IPNet) bool { + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false +} + // logf is a thin wrapper so handlers log with a consistent prefix. func logf(format string, args ...any) { log.Printf(format, args...)