From 98db560d832d14785d336e7b76cedcc66d56026e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Sat, 15 Aug 2026 23:18:57 +0200 Subject: [PATCH] fix(backend): honor X-Forwarded-For via configurable trusted proxies (#2701) --- .../internal/auth/httpapi/handlers_test.go | 122 +++++++++++++- apps/backend/internal/backendapp/main.go | 19 +-- .../internal/backendapp/trustedproxies.go | 76 +++++++++ .../backendapp/trustedproxies_test.go | 132 ++++++++++++++++ docs/plans/trusted-proxies/plan.md | 149 ++++++++++++++++++ .../task-01-backend-trusted-proxies.md | 111 +++++++++++++ .../task-02-auth-session-ip-tests.md | 126 +++++++++++++++ .../task-03-configuration-docs.md | 99 ++++++++++++ docs/public/configuration.md | 35 ++++ docs/specs/INDEX.md | 1 + docs/specs/auth/trusted-proxies.md | 86 ++++++++++ 11 files changed, 944 insertions(+), 12 deletions(-) create mode 100644 apps/backend/internal/backendapp/trustedproxies.go create mode 100644 apps/backend/internal/backendapp/trustedproxies_test.go create mode 100644 docs/plans/trusted-proxies/plan.md create mode 100644 docs/plans/trusted-proxies/task-01-backend-trusted-proxies.md create mode 100644 docs/plans/trusted-proxies/task-02-auth-session-ip-tests.md create mode 100644 docs/plans/trusted-proxies/task-03-configuration-docs.md create mode 100644 docs/specs/auth/trusted-proxies.md diff --git a/apps/backend/internal/auth/httpapi/handlers_test.go b/apps/backend/internal/auth/httpapi/handlers_test.go index 37c14e16c7..8f647af978 100644 --- a/apps/backend/internal/auth/httpapi/handlers_test.go +++ b/apps/backend/internal/auth/httpapi/handlers_test.go @@ -22,8 +22,11 @@ import ( // newAPIFixture builds the full production HTTP stack for auth: global // middleware + auth API routes on one router. authEnabled maps to the -// features.auth flag (on ⇒ setup mode until the wizard runs). -func newAPIFixture(t *testing.T, authEnabled bool) (*gin.Engine, *auth.Service) { +// features.auth flag (on ⇒ setup mode until the wizard runs). The router +// mirrors production by default: gin.New() trusts all proxies, so +// SetTrustedProxies is called with the variadic trusted list (nil clears the +// insecure default) exactly like buildHTTPServer does. +func newAPIFixture(t *testing.T, authEnabled bool, trustedProxies ...string) (*gin.Engine, *auth.Service) { t.Helper() gin.SetMode(gin.TestMode) conn, err := sqlx.Open("sqlite3", ":memory:") @@ -50,6 +53,12 @@ func newAPIFixture(t *testing.T, authEnabled bool) (*gin.Engine, *auth.Service) t.Fatalf("auth service: %v", err) } router := gin.New() + if len(trustedProxies) == 0 { + trustedProxies = nil + } + if err := router.SetTrustedProxies(trustedProxies); err != nil { + t.Fatalf("set trusted proxies: %v", err) + } router.Use(authhttpmw.Middleware(svc)) RegisterRoutes(router, svc, nil) return router, svc @@ -61,7 +70,7 @@ type apiClient struct { cookie *http.Cookie } -func (c *apiClient) do(method, path string, body any) *httptest.ResponseRecorder { +func (c *apiClient) do(method, path string, body any, mutate ...func(*http.Request)) *httptest.ResponseRecorder { c.t.Helper() var reader *bytes.Reader if body != nil { @@ -78,6 +87,9 @@ func (c *apiClient) do(method, path string, body any) *httptest.ResponseRecorder if c.cookie != nil { req.AddCookie(c.cookie) } + for _, m := range mutate { + m(req) + } rec := httptest.NewRecorder() c.router.ServeHTTP(rec, req) // Capture session cookie updates (login/setup/logout). @@ -93,6 +105,110 @@ func (c *apiClient) do(method, path string, body any) *httptest.ResponseRecorder return rec } +// loginSessionIP bootstraps the admin via setup, performs a login request +// with the given transport attributes, and returns the stored IP of the +// current session (the login cookie is current; the setup session stays in +// the list and is not selected). +func loginSessionIP(t *testing.T, router *gin.Engine, remoteAddr, forwardedFor, xRealIP string) string { + t.Helper() + client := &apiClient{t: t, router: router} + rec := client.do(http.MethodPost, "/api/v1/auth/setup", map[string]any{ + "email": "admin@x.dev", "password": "adminpass123", "display_name": "Admin", + }) + if rec.Code != http.StatusOK { + t.Fatalf("setup: %d body=%s", rec.Code, rec.Body.String()) + } + rec = client.do(http.MethodPost, "/api/v1/auth/login", map[string]any{ + "email": "admin@x.dev", "password": "adminpass123", + }, func(req *http.Request) { + req.RemoteAddr = remoteAddr + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } + if xRealIP != "" { + req.Header.Set("X-Real-IP", xRealIP) + } + }) + if rec.Code != http.StatusOK { + t.Fatalf("login: %d body=%s", rec.Code, rec.Body.String()) + } + rec = client.do(http.MethodGet, "/api/v1/auth/sessions", nil) + if rec.Code != http.StatusOK { + t.Fatalf("sessions: %d body=%s", rec.Code, rec.Body.String()) + } + var out struct { + Sessions []struct { + IP string `json:"ip"` + Current bool `json:"current"` + } `json:"sessions"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatalf("decode sessions: %v", err) + } + for _, s := range out.Sessions { + if s.Current { + return s.IP + } + } + t.Fatal("no current session in list") + return "" +} + +// TestLoginSessionIPNoTrustedProxiesIgnoresForwardedFor locks the secure +// default: with no trusted proxies, X-Forwarded-For is ignored and the session +// IP is the TCP peer. +func TestLoginSessionIPNoTrustedProxiesIgnoresForwardedFor(t *testing.T) { + router, _ := newAPIFixture(t, true) + if got := loginSessionIP(t, router, "10.0.0.5:1234", "203.0.113.7", ""); got != "10.0.0.5" { + t.Fatalf("session IP = %q, want peer 10.0.0.5", got) + } +} + +// TestLoginSessionIPTrustedPeerUsesForwardedFor locks the opt-in behavior: +// when the TCP peer is in the trusted list, X-Forwarded-For is honored. +func TestLoginSessionIPTrustedPeerUsesForwardedFor(t *testing.T) { + router, _ := newAPIFixture(t, true, "10.0.0.0/8") + if got := loginSessionIP(t, router, "10.0.0.5:1234", "203.0.113.7", ""); got != "203.0.113.7" { + t.Fatalf("session IP = %q, want 203.0.113.7", got) + } +} + +// TestLoginSessionIPUntrustedPeerIgnoresForwardedFor locks the boundary: a +// trusted list that does not contain the peer leaves the header ignored. +func TestLoginSessionIPUntrustedPeerIgnoresForwardedFor(t *testing.T) { + router, _ := newAPIFixture(t, true, "192.168.0.0/16") + if got := loginSessionIP(t, router, "10.0.0.5:1234", "203.0.113.7", ""); got != "10.0.0.5" { + t.Fatalf("session IP = %q, want peer 10.0.0.5", got) + } +} + +// TestLoginSessionIPUsesXRealIP locks the secondary forwarded header: gin +// reads X-Real-IP when X-Forwarded-For is absent. +func TestLoginSessionIPUsesXRealIP(t *testing.T) { + router, _ := newAPIFixture(t, true, "10.0.0.0/8") + if got := loginSessionIP(t, router, "10.0.0.5:1234", "", "203.0.113.7"); got != "203.0.113.7" { + t.Fatalf("session IP = %q, want 203.0.113.7", got) + } +} + +// TestLoginSessionIPMalformedXFFFallsBackToXRealIP locks gin's fallback: a +// syntactically invalid X-Forwarded-For is skipped and X-Real-IP is read. +func TestLoginSessionIPMalformedXFFFallsBackToXRealIP(t *testing.T) { + router, _ := newAPIFixture(t, true, "10.0.0.0/8") + if got := loginSessionIP(t, router, "10.0.0.5:1234", "not-an-ip", "203.0.113.7"); got != "203.0.113.7" { + t.Fatalf("session IP = %q, want 203.0.113.7", got) + } +} + +// TestLoginSessionIPNoForwardedHeaderUsesPeer locks the fallback: a trusted +// peer with no forwarded-IP header still records the TCP peer. +func TestLoginSessionIPNoForwardedHeaderUsesPeer(t *testing.T) { + router, _ := newAPIFixture(t, true, "10.0.0.0/8") + if got := loginSessionIP(t, router, "10.0.0.5:1234", "", ""); got != "10.0.0.5" { + t.Fatalf("session IP = %q, want peer 10.0.0.5", got) + } +} + func decode(t *testing.T, rec *httptest.ResponseRecorder) map[string]any { t.Helper() out := map[string]any{} diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index ef2f9c8c91..0d2e88dc6b 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -1962,15 +1962,16 @@ func buildHTTPServer( ) (*http.Server, error) { gin.SetMode(gin.ReleaseMode) router := gin.New() - // Do not trust X-Forwarded-For by default: gin trusts all proxies out of - // the box, which would let a directly-reachable backend accept a spoofed - // client IP and defeat the login rate limiter (keyed on ClientIP). With no - // trusted proxies, ClientIP() falls back to the real peer RemoteAddr. - // Deployments behind a real proxy should front kandev with one that sets a - // trusted hop; revisit if a configurable trusted-proxy CIDR is added. - if err := router.SetTrustedProxies(nil); err != nil { - log.Warn("failed to clear trusted proxies", zap.Error(err)) - } + // Trusted-proxy configuration for X-Forwarded-For via KANDEV_TRUSTED_PROXIES + // (comma-separated IPs/CIDRs). gin trusts all proxies out of the box, + // which would let a directly-reachable backend accept a spoofed client IP + // and defeat the login rate limiter (keyed on ClientIP). The default is no + // trusted proxies: ClientIP() falls back to the real peer RemoteAddr and + // forwarded headers are ignored. Deployments behind a real proxy set the + // env var to the proxy's IPs/CIDRs; a directly-reachable backend with the + // var set can have X-Forwarded-For spoofed, which also defeats the + // ClientIP-keyed login rate limiter. + configureTrustedProxies(router, log) router.Use(httpmw.RequestLogger(log, kandevName)) router.Use(httpmw.OtelTracing(kandevName)) router.Use(gin.Recovery()) diff --git a/apps/backend/internal/backendapp/trustedproxies.go b/apps/backend/internal/backendapp/trustedproxies.go new file mode 100644 index 0000000000..958749ea3f --- /dev/null +++ b/apps/backend/internal/backendapp/trustedproxies.go @@ -0,0 +1,76 @@ +package backendapp + +import ( + "net" + "os" + "strings" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/common/logger" +) + +// trustedProxiesEnv lists the reverse proxies whose X-Forwarded-For header +// the backend trusts. Comma-separated IPs and CIDRs. +const trustedProxiesEnv = "KANDEV_TRUSTED_PROXIES" + +// emptyTrustedProxyEntry is the placeholder name used when a blank component +// appears inside a nonblank KANDEV_TRUSTED_PROXIES value (e.g. a trailing or +// doubled comma). Blank is not a valid IP or CIDR, so it fails the whole list +// closed like any other unparsable entry. +const emptyTrustedProxyEntry = "" + +const trustedProxiesWarningMsg = "KANDEV_TRUSTED_PROXIES contains invalid entries; ignoring X-Forwarded-For entirely" + +// resolveTrustedProxies parses the comma-separated KANDEV_TRUSTED_PROXIES +// value into gin-compatible proxy origins (bare IPs or CIDRs). An entirely +// blank value means "no trusted proxies" (trusted nil, no invalid entries). +// Any unparsable entry, including a blank component inside a nonblank value, +// makes the whole list untrustworthy: trusted is nil then (fail closed) and +// invalid names the bad values for the startup warning. The returned strings +// are the original entries, already validated, so SetTrustedProxies cannot +// reject them. +func resolveTrustedProxies(raw string) (trusted []string, invalid []string) { + if strings.TrimSpace(raw) == "" { + return nil, nil + } + for _, part := range strings.Split(raw, ",") { + entry := strings.TrimSpace(part) + if entry == "" { + invalid = append(invalid, emptyTrustedProxyEntry) + continue + } + if strings.Contains(entry, "/") { + if _, _, err := net.ParseCIDR(entry); err != nil { + invalid = append(invalid, entry) + continue + } + } else if net.ParseIP(entry) == nil { + invalid = append(invalid, entry) + continue + } + trusted = append(trusted, entry) + } + if len(invalid) > 0 { + return nil, invalid + } + return trusted, nil +} + +// configureTrustedProxies reads KANDEV_TRUSTED_PROXIES and applies it to the +// router. Unset, empty, or fully invalid lists disable forwarded-header trust +// (SetTrustedProxies(nil)); an invalid entry logs a warning naming the bad +// value(s). gin trusts all proxies out of the box, so without this call a +// directly reachable backend would accept a spoofed X-Forwarded-For and +// defeat the ClientIP-keyed login rate limiter. +func configureTrustedProxies(router *gin.Engine, log *logger.Logger) { + trusted, invalid := resolveTrustedProxies(os.Getenv(trustedProxiesEnv)) + if len(invalid) > 0 { + log.Warn(trustedProxiesWarningMsg, zap.Strings("invalid_entries", invalid)) + trusted = nil + } + if err := router.SetTrustedProxies(trusted); err != nil { + log.Warn("failed to configure trusted proxies", zap.Error(err)) + } +} diff --git a/apps/backend/internal/backendapp/trustedproxies_test.go b/apps/backend/internal/backendapp/trustedproxies_test.go new file mode 100644 index 0000000000..4ebe52f2df --- /dev/null +++ b/apps/backend/internal/backendapp/trustedproxies_test.go @@ -0,0 +1,132 @@ +package backendapp + +import ( + "fmt" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" +) + +func TestResolveTrustedProxies(t *testing.T) { + tests := []struct { + name string + raw string + trusted []string + invalid []string + }{ + {name: "unset", raw: "", trusted: nil, invalid: nil}, + {name: "blank", raw: " ", trusted: nil, invalid: nil}, + {name: "single CIDR", raw: "10.0.0.0/8", trusted: []string{"10.0.0.0/8"}, invalid: nil}, + {name: "multiple CIDRs with whitespace", raw: "10.0.0.0/8, 192.168.0.0/16", trusted: []string{"10.0.0.0/8", "192.168.0.0/16"}, invalid: nil}, + {name: "bare IPv4", raw: "10.0.0.1", trusted: []string{"10.0.0.1"}, invalid: nil}, + {name: "bare IPv6", raw: "2001:db8::1", trusted: []string{"2001:db8::1"}, invalid: nil}, + {name: "IPv6 CIDR", raw: "2001:db8::/32", trusted: []string{"2001:db8::/32"}, invalid: nil}, + {name: "invalid entry", raw: "not-a-cidr", trusted: nil, invalid: []string{"not-a-cidr"}}, + {name: "invalid IP", raw: "999.1.1.1", trusted: nil, invalid: []string{"999.1.1.1"}}, + {name: "mixed valid and invalid fails closed", raw: "10.0.0.0/8, not-a-cidr", trusted: nil, invalid: []string{"not-a-cidr"}}, + {name: "trailing comma fails closed", raw: "10.0.0.0/8,", trusted: nil, invalid: []string{emptyTrustedProxyEntry}}, + {name: "leading comma fails closed", raw: ",10.0.0.0/8", trusted: nil, invalid: []string{emptyTrustedProxyEntry}}, + {name: "doubled comma fails closed", raw: "10.0.0.0/8,,192.168.0.0/16", trusted: nil, invalid: []string{emptyTrustedProxyEntry}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + trusted, invalid := resolveTrustedProxies(tt.raw) + if !reflect.DeepEqual(trusted, tt.trusted) { + t.Fatalf("resolveTrustedProxies(%q) trusted = %v, want %v", tt.raw, trusted, tt.trusted) + } + if !reflect.DeepEqual(invalid, tt.invalid) { + t.Fatalf("resolveTrustedProxies(%q) invalid = %v, want %v", tt.raw, invalid, tt.invalid) + } + }) + } +} + +// clientIPThroughConfig builds a router, applies configureTrustedProxies with +// the current environment, and returns ClientIP for a request from peer. +func clientIPThroughConfig(t *testing.T, peer, forwardedFor, xRealIP string) string { + t.Helper() + gin.SetMode(gin.TestMode) + router := gin.New() + configureTrustedProxies(router, testLogger(t)) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, c.ClientIP()) + }) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = peer + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } + if xRealIP != "" { + req.Header.Set("X-Real-IP", xRealIP) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec.Body.String() +} + +func TestConfigureTrustedProxiesHonorsForwardedForWhenPeerTrusted(t *testing.T) { + t.Setenv(trustedProxiesEnv, "10.0.0.0/8") + if got := clientIPThroughConfig(t, "10.0.0.5:1234", "203.0.113.7", ""); got != "203.0.113.7" { + t.Fatalf("trusted peer ClientIP = %q, want 203.0.113.7", got) + } +} + +func TestConfigureTrustedProxiesUnsetIgnoresHeader(t *testing.T) { + t.Setenv(trustedProxiesEnv, "") + if got := clientIPThroughConfig(t, "10.0.0.5:1234", "203.0.113.7", ""); got != "10.0.0.5" { + t.Fatalf("unset ClientIP = %q, want peer 10.0.0.5", got) + } +} + +func TestConfigureTrustedProxiesUntrustedPeerIgnoresHeader(t *testing.T) { + t.Setenv(trustedProxiesEnv, "192.168.0.0/16") + if got := clientIPThroughConfig(t, "10.0.0.5:1234", "203.0.113.7", ""); got != "10.0.0.5" { + t.Fatalf("untrusted peer ClientIP = %q, want peer 10.0.0.5", got) + } +} + +func TestConfigureTrustedProxiesInvalidEntryWarnsAndFallsBack(t *testing.T) { + t.Setenv(trustedProxiesEnv, "10.0.0.0/8, not-a-cidr") + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("observer logger: %v", err) + } + gin.SetMode(gin.TestMode) + router := gin.New() + configureTrustedProxies(router, log) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, c.ClientIP()) + }) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.5:1234" + req.Header.Set("X-Forwarded-For", "203.0.113.7") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if got := rec.Body.String(); got != "10.0.0.5" { + t.Fatalf("invalid env ClientIP = %q, want peer 10.0.0.5", got) + } + warned := false + for _, entry := range logs.All() { + if !strings.Contains(entry.Message, "KANDEV_TRUSTED_PROXIES") { + continue + } + for _, field := range entry.Context { + if field.Key == "invalid_entries" && strings.Contains(fmt.Sprint(field.Interface), "not-a-cidr") { + warned = true + } + } + } + if !warned { + t.Fatalf("expected startup warning naming not-a-cidr, got logs: %v", logs.All()) + } +} diff --git a/docs/plans/trusted-proxies/plan.md b/docs/plans/trusted-proxies/plan.md new file mode 100644 index 0000000000..b228236447 --- /dev/null +++ b/docs/plans/trusted-proxies/plan.md @@ -0,0 +1,149 @@ +--- +spec: docs/specs/auth/trusted-proxies.md +created: 2026-08-15 +status: building +--- + +# Implementation Plan: Trusted Proxies for X-Forwarded-For + +## Overview + +Make the backend honor `X-Forwarded-For` only when the request's TCP peer is +one of the proxies listed in `KANDEV_TRUSTED_PROXIES` (comma-separated IPs / +CIDRs), while keeping the current secure default (no trusted proxies) when the +variable is unset, empty, or contains an unparsable entry. The change is one +env-var read plus a `SetTrustedProxies` call at the single router construction +site in `buildHTTPServer`; every `ClientIP()` consumer (login / setup / invite-accept / plugin-SSO +session IPs and the login rate-limiter key) picks up the behavior with no +per-callsite changes; `RequestLogger` never reads `ClientIP()` and stays +untouched. + +Order: Tasks 01 (parser + wiring with backendapp unit tests) and 02 (auth +HTTP behavioral tests that pin the observable session-IP contract; they +configure the router at the fixture level and are independent of the wiring) +are parallel-safe Wave-1 candidates with disjoint files, sequential by default +absent user authorization; Task 03 (public configuration docs) lands as Wave +2 after them. + +## Backend + +### Env parsing and router wiring (`apps/backend/internal/backendapp/trustedproxies.go`, new) + +- `const trustedProxiesEnv = "KANDEV_TRUSTED_PROXIES"`. +- `func resolveTrustedProxies(raw string) (trusted []string, invalid []string)`: + an entirely blank raw value is unset (`nil`, no invalid). Otherwise split on + `,`, trim each entry, and validate each with `net.ParseCIDR` when it + contains `/`, else `net.ParseIP`; a blank component inside a nonblank value + is an invalid entry (named ``), because empty is not an IP or CIDR. + Any invalid entry makes the whole list fail closed: `trusted` is `nil` and + `invalid` names the bad values. (Reasons: gin v1.9.1 `SetTrustedProxies` + errors on an unparsable entry, and the task requires never partially + trusting a list.) +- `func configureTrustedProxies(router *gin.Engine, log *logger.Logger)`: + reads `os.Getenv(trustedProxiesEnv)`, calls `resolveTrustedProxies`, logs a + `Warn` naming the invalid values when any exist (and forces `trusted = + nil`), then calls `router.SetTrustedProxies(trusted)` with the existing + `log.Warn` fallback on error. `nil` matches the current production call. +- Env-reading follows the repo pattern of direct `os.Getenv` at the call site + (compare `KANDEV_WEB_DIST_DIR` in `helpers.go`, `KANDEV_CONSOLE_LOG_LEVEL` in + `main.go`). No new config framework, no YAML key. + +### Router construction site (`apps/backend/internal/backendapp/main.go`) + +- In `buildHTTPServer`, replace the comment block at lines 1924-1930 and the + `router.SetTrustedProxies(nil)` call with the new explanatory comment + (documents `KANDEV_TRUSTED_PROXIES`, the spoofing tradeoff for a directly + reachable backend, and the no-trusted-proxies default) and a call to + `configureTrustedProxies(router, log)`. +- No other router construction sites exist for the main HTTP surface; the + agentctl control server is loopback-only and out of scope. + +## Frontend + +No UI changes. `ClientIP()` feeds the session IP shown in +Settings > Account > Security, which renders the already-stored value. + +## Tests + +- **Parse rules** (`apps/backend/internal/backendapp/trustedproxies_test.go`): + table test over `resolveTrustedProxies` covering entirely blank raw value, + single CIDR, multiple CIDRs with whitespace, bare IPv4/IPv6, IPv6 CIDR, + unparsable entry, mixed valid+invalid (must return `trusted == nil`), and + blank components inside a nonblank value (leading/trailing/doubled commas) + must return `trusted == nil` with `` in `invalid`. +- **Env wiring + warning** (same file): `configureTrustedProxies` with + `t.Setenv`; behavioral assertion via a `gin.New()` router serving a handler + that returns `c.ClientIP()`: trusted peer + `X-Forwarded-For` header + resolves the header value; unset env and not-matching CIDR return the TCP + peer; invalid env logs a warning naming the bad value (zap observer pattern: + `zaptest/observer` + `logger.NewFromZap`, used across the repo) and behaves + like unset. +- **Session IP contract** (`apps/backend/internal/auth/httpapi/handlers_test.go`): + add a request-mutation seam to the test client (variadic mutator on + `apiClient.do`) so tests can set `RemoteAddr` and forwarded headers, and + extend `newAPIFixture` to clear trusted proxies by default (`router. + SetTrustedProxies(nil)`, mirroring production) and accept an optional + trusted-proxy list. Named tests (anchored `-run '^TestLoginSessionIP'`), + login with `req.RemoteAddr = "10.0.0.5:1234"`, then read the stored session + IP via `GET /api/v1/auth/sessions`: + - trusted `10.0.0.0/8` + `X-Forwarded-For: 203.0.113.7` → session IP `203.0.113.7`; + - no trusted proxies + the same header → session IP `10.0.0.5`; + - trusted `192.168.0.0/16` (peer not in list) + the same header → session IP `10.0.0.5`; + - trusted peer + `X-Real-IP: 203.0.113.7` only → session IP `203.0.113.7`; + - trusted peer + `X-Forwarded-For: not-an-ip` + valid `X-Real-IP` → session IP from `X-Real-IP`; + - trusted peer + neither header → session IP `10.0.0.5`. + In listed order these exercise spec scenarios 2 (trusted peer + XFF), 1 + (unset), 3 (untrusted CIDR), 8 (X-Real-IP only), 9 (malformed XFF + valid + X-Real-IP), 7 (neither header). Spec scenario 4 (invalid env → warning + + nil trust) is covered by Task 01's backendapp tests; scenarios 5-6 + (spoofing) are the trust mechanics of scenarios 2/3 from an attacker + framing and need no separate test: a peer inside a trusted range is + believed, a peer outside every range is ignored. The pre-fixture RED case + is the no-trusted-proxies assertion (gin's trust-all default would honor + the header); the trusted cases are GREEN at the fixture level regardless of + Task 01, so Task 02 does not depend on it. Acceptance c (invalid value) is + covered by the backendapp warning + nil-fallback test combined with the + no-trusted-proxies session test. +- No E2E: no user-facing flow changes (the Settings page renders a stored + value; the behavior difference is only which IP is stored). +- **Manual integration smoke** (post-implementation gate, after Tasks 01-02 + land): run the backend with `KANDEV_TRUSTED_PROXIES=`, log in + through the proxy, and confirm Settings > Account > Security shows the real + client IP. If the environment cannot run a proxied instance, record the + concrete blocker in the plan's Verification Results instead of dropping the + check. + +## Verification Results + +- Task 01 parser + wiring: `cd apps/backend && go test ./internal/backendapp -run '^Test(ResolveTrustedProxies|ConfigureTrustedProxies)' -count=1` — RED (build failed: undefined symbols) then GREEN (13 subtests + 4 behavioral tests). Regression `go test ./internal/backendapp/... -count=1` — ok. `go build ./...` — ok. +- Task 02 session-IP contract: `cd apps/backend && go test ./internal/auth/httpapi -run '^TestLoginSessionIP' -count=1` — RED (unset case stored forwarded IP under gin's trust-all default) then GREEN (6 tests). Full auth suite `go test ./internal/auth/... -count=1` — ok. +- Task 03 docs: `node scripts/validate-public-docs.mjs && git diff --check` — "Validated 41 published docs pages." + clean diff. +- Combined gate: `cd apps/backend && go build ./... && go test ./internal/auth/... ./internal/backendapp/... -count=1` — all ok (auth, auth/httpapi, auth/httpmw, auth/store, backendapp, backendapp/ownershiplock). +- Manual integration smoke: BLOCKED — no proxied kandev deployment available in this harness (needs a running backend + reverse proxy + browser login). The observable contract is covered by the session-IP tests above; run the smoke in the Test step against a real proxy deployment. + +## Implementation Waves And Parallel Candidates + +Wave 1 (parallel candidates — user authorization required; files are disjoint: +`internal/backendapp/*` vs `internal/auth/httpapi/*`): + +- [x] [Task 01: Backend trusted-proxies parser and wiring](task-01-backend-trusted-proxies.md) +- [x] [Task 02: Auth session-IP behavioral tests](task-02-auth-session-ip-tests.md) + +Wave 2: + +- [x] [Task 03: Configuration docs](task-03-configuration-docs.md) + +Task 02 exercises the router at the fixture level and does not depend on Task +01's code; Task 03 is disjoint from both. All sequential by default per repo +convention; no subagent execution is authorized by this plan. + +## Risks and out of scope + +- gin v1.9.1 `New()` trusts all proxies (`0.0.0.0/0`, `::/0`) by default, so + both production and the auth test fixture must keep an explicit + `SetTrustedProxies(nil)` default; the fixture change is part of Task 02. +- Setting the variable while the backend is directly reachable allows + `X-Forwarded-For` spoofing and defeats the ClientIP-keyed login rate + limiter; this is the documented tradeoff, not a defect. +- `X-Forwarded-Proto`, WebSocket `remote_addr` logging, and `RequestLogger` + fields stay untouched. diff --git a/docs/plans/trusted-proxies/task-01-backend-trusted-proxies.md b/docs/plans/trusted-proxies/task-01-backend-trusted-proxies.md new file mode 100644 index 0000000000..cab96f18b4 --- /dev/null +++ b/docs/plans/trusted-proxies/task-01-backend-trusted-proxies.md @@ -0,0 +1,111 @@ +--- +id: "01-backend-trusted-proxies" +title: "Backend trusted-proxies parser and wiring" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/auth/trusted-proxies.md" +--- + +# Task 01: Backend trusted-proxies parser and wiring + +Parse `KANDEV_TRUSTED_PROXIES` at the router construction site and pass the +result to `router.SetTrustedProxies`, keeping the no-trusted-proxies default +and a fail-closed warning path for invalid entries. + +## Acceptance + +- `resolveTrustedProxies` returns `(nil, nil)` for an entirely blank raw + value; the valid CIDRs/IPs for a fully valid comma-separated list; and + `(nil, [bad…])` when any entry is unparsable, including a blank component + inside a nonblank value (named ``): never a partially trusted list, + never silent trust from a trailing/doubled comma. +- `configureTrustedProxies` reads the env var, applies valid lists to the + router, and for invalid entries logs a startup WARNING naming the bad + value(s) and applies `SetTrustedProxies(nil)`. +- `buildHTTPServer` calls `configureTrustedProxies` in place of the current + hardcoded `SetTrustedProxies(nil)`, and the replaced comment documents the + env var, the spoofing tradeoff (a directly reachable backend with the var + set can have `X-Forwarded-For` spoofed, which also defeats the + ClientIP-keyed login rate limiter), and the default (no trusted proxies). +- Behavior: with the env var set to a CIDR containing the peer, a request + carrying `X-Forwarded-For` resolves the header value via `c.ClientIP()`; + with the var unset, or set to a CIDR not containing the peer, or invalid, + `c.ClientIP()` returns the TCP peer. + +## Files likely touched + +- `apps/backend/internal/backendapp/trustedproxies.go` (new) +- `apps/backend/internal/backendapp/trustedproxies_test.go` (new) +- `apps/backend/internal/backendapp/main.go` (comment + call at ~line 1924) + +## Dependencies + +None. + +## Parallelism + +`sequential` by default. Files are disjoint from Task 02's auth fixture tests; +no subagent execution is authorized by this plan. + +## Inputs + +- Spec sections **What**, **Failure modes**, **Scenarios**. +- `apps/backend/internal/backendapp/main.go` `buildHTTPServer` (router + construction at lines 1924-1933). +- gin v1.9.1 semantics: `SetTrustedProxies(nil)` disables header trust; + `SetTrustedProxies` returns an error on unparsable entries, so pre-validate + with `net.ParseIP` / `net.ParseCIDR`. +- Env pattern: direct `os.Getenv` at the call site (e.g. `KANDEV_WEB_DIST_DIR` + in `helpers.go`). `net` and `strings` are already imported in `main.go`. +- Log-warning assertion pattern: `go.uber.org/zap/zaptest/observer` + + `logger.NewFromZap(zap.New(core))` (used across `internal/backendapp`). + +## TDD sequence + +1. Write `trustedproxies_test.go` first: table test for `resolveTrustedProxies` + (entirely blank raw, single CIDR, multi with whitespace, bare IPv4/IPv6, + IPv6 CIDR, invalid, mixed valid+invalid, blank components from + leading/trailing/doubled commas → invalid ``) and behavioral + `configureTrustedProxies` tests (env set/valid, unset, not-matching CIDR, + invalid with warning) using a `gin.New()` router serving + `c.String(http.StatusOK, c.ClientIP())` with `httptest.NewRequest` + + `RemoteAddr` + `X-Forwarded-For`. Run and confirm RED. +2. Implement `trustedproxies.go`. +3. Update `main.go` comment + call. Run the targeted tests GREEN, then + `go build ./...` and `go test ./internal/backendapp/...` for regressions. + +## Verification + +```bash +cd apps/backend && go test ./internal/backendapp -run '^Test(ResolveTrustedProxies|ConfigureTrustedProxies)' -count=1 && go test ./internal/backendapp/... -count=1 && go build ./... +``` + +## Risks + +- Forgetting that gin trusts all proxies by default would silently defeat the + secure default; the unset-env test asserting the TCP peer guards this. +- Passing an unvalidated list to `SetTrustedProxies` returns an error and + leaves ambiguous state; the parser must never forward invalid entries. + +## Output contract + +Report the RED failure, GREEN result, exact files changed, the warning-log +evidence, remaining risks, and synchronized task/plan status. Record every +exact command and outcome in `## Results`. + +## Results + +Implemented `apps/backend/internal/backendapp/trustedproxies.go` +(`trustedProxiesEnv`, `emptyTrustedProxyEntry`, `resolveTrustedProxies`, +`configureTrustedProxies`), replaced the hardcoded `SetTrustedProxies(nil)` +block in `buildHTTPServer` (`main.go`) with the documented +`configureTrustedProxies(router, log)` call, and added +`trustedproxies_test.go`. + +- RED: `cd apps/backend && go test ./internal/backendapp -run '^Test(ResolveTrustedProxies|ConfigureTrustedProxies)' -count=1` — build failed with undefined `resolveTrustedProxies`, `configureTrustedProxies`, `trustedProxiesEnv`, `emptyTrustedProxyEntry`. +- GREEN: the same command after implementation — ok (13 parse subtests + 4 behavioral tests). One test-only fix: the warning assertion inspected `field.String` on a `zap.Strings` array field, which does not render; switched to `fmt.Sprint(field.Interface)` (the warning itself was emitted correctly, with `invalid_entries: [not-a-cidr]`). +- Regression: `go test ./internal/backendapp/... -count=1` — ok (backendapp 11.4s, ownershiplock). +- Build: `go build ./...` — ok. +- Security: fail-closed rule enforced in `resolveTrustedProxies` (any invalid entry, including blank components from trailing/doubled commas, yields `trusted == nil` and names the bad values); the warning is logged before `SetTrustedProxies(nil)`. diff --git a/docs/plans/trusted-proxies/task-02-auth-session-ip-tests.md b/docs/plans/trusted-proxies/task-02-auth-session-ip-tests.md new file mode 100644 index 0000000000..6af4e2b7dd --- /dev/null +++ b/docs/plans/trusted-proxies/task-02-auth-session-ip-tests.md @@ -0,0 +1,126 @@ +--- +id: "02-auth-session-ip-tests" +title: "Auth session-IP behavioral tests" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/auth/trusted-proxies.md" +--- + +# Task 02: Auth session-IP behavioral tests + +Lock the observable contract: the session IP stored by login (and recorded for +setup / invite acceptance) honors `X-Forwarded-For` only when the TCP peer is a +trusted proxy, and falls back to the peer otherwise. + +## Acceptance + +- With trusted proxies containing the peer (e.g. `10.0.0.0/8` for peer + `10.0.0.5`), a login request carrying `X-Forwarded-For: 203.0.113.7` stores + session IP `203.0.113.7` (read back via `GET /api/v1/auth/sessions`). +- With no trusted proxies (the default) or a CIDR not containing the peer + (e.g. `192.168.0.0/16`), the same request stores session IP `10.0.0.5`. +- With a trusted peer and only `X-Real-IP: 203.0.113.7`, the stored session + IP is `203.0.113.7`. +- With a trusted peer, `X-Forwarded-For: not-an-ip` and + `X-Real-IP: 203.0.113.7`, the stored session IP is `203.0.113.7`. +- With a trusted peer and neither forwarded header, the stored session IP is + the peer `10.0.0.5`. +- The auth test fixture mirrors production: `gin.New()` defaults to trusting + all proxies, so `newAPIFixture` must explicitly clear them + (`router.SetTrustedProxies(nil)`) unless the test passes a trusted list. + +## Files likely touched + +- `apps/backend/internal/auth/httpapi/handlers_test.go` (fixture + + new tests; existing tests keep passing unchanged) + +## Dependencies + +None. The fixture-level tests configure the router directly +(`SetTrustedProxies` via `newAPIFixture`), so they exercise gin's behavior at +the auth surface without Task 01's wiring; Task 01 covers env parsing and the +production wiring in `backendapp`. + +## Parallelism + +`sequential` by default; parallel-safe candidate with Task 01 (disjoint +files: `internal/auth/httpapi/*` vs `internal/backendapp/*`) only when the +user authorizes subagents. + +## Inputs + +- Spec **Scenarios** (all nine; the six login tests exercise scenarios 1, 2, + 3, 7, 8, 9 as mapped in plan.md Tests; scenarios 4-6 are covered there and + in Task 01) and acceptance a/b. +- `apps/backend/internal/auth/httpapi/handlers_test.go`: + `newAPIFixture`, `apiClient.do`, `TestFullLifecycle` (setup + invite + flow) as the request-shape reference. +- `apps/backend/internal/auth/httpapi/handlers.go:85-106` (`setup`, `login`) + and `handlers_admin.go:120-126` (`acceptInvite`) pass `c.ClientIP()` into + the service; session IP is exposed by `listSessions` as the `ip` field. +- The test client needs a request seam: `apiClient.do` currently builds the + request and serves it immediately, so add a variadic request mutator + (e.g. `do(method, path, body, mutate ...func(*http.Request))`) to set + `RemoteAddr` and forwarded headers. +- Account bootstrap: `newAPIFixture(t, true)` starts in setup mode (no admin + yet), so tests must first `POST /api/v1/auth/setup` (as `TestFullLifecycle` + does) before login has an identity. After the mutated login, `GET + /api/v1/auth/sessions` lists both the setup session and the login session; + select the `current: true` row (the login cookie is current) and assert its + `ip` field. +- `httptest.NewRequest` sets `RemoteAddr` to `192.0.2.1:1234` by default; + tests must override `req.RemoteAddr` and set the `X-Forwarded-For` header + explicitly. + +## TDD sequence + +1. Add the request seam to `apiClient.do`, write the no-trusted-proxies + (peer-IP) test (bootstrap via `POST /api/v1/auth/setup`, mutated login, + then select the `current: true` session), and run it RED against the + current fixture: gin's trust-all default would honor the header, so the + peer-IP assertion fails. +2. Extend `newAPIFixture(t, authEnabled, trustedProxies ...string)` to call + `router.SetTrustedProxies(trustedProxies)` (variadic; no args → `nil`, + matching production), add the remaining cases (trusted peer → header IP; + not-matching CIDR → peer IP; `X-Real-IP` only; malformed XFF + valid + `X-Real-IP`; neither header → peer IP), and run GREEN. +3. Confirm the full auth HTTP suite stays green: `go test ./internal/auth/...`. + +## Verification + +```bash +cd apps/backend && go test ./internal/auth/httpapi -run '^TestLoginSessionIP' -count=1 && go test ./internal/auth/... -count=1 +``` + +The through-proxy manual smoke is the plan-level integration gate (see +plan.md "Manual integration smoke"), executed after Tasks 01-02 land, because +it needs the production wiring plus the running backend; record the concrete +blocker in `## Results` if the environment cannot run a proxied instance. + +## Risks + +- If the fixture keeps gin's trust-all default, the "unset → header ignored" + assertion is meaningless; the fixture change is mandatory, not optional. +- gin reads `X-Real-IP` as a secondary header; tests must set only + `X-Forwarded-For` so the assertion isolates that path. + +## Output contract + +Report the RED failure, GREEN result, exact files changed, fixture-change +evidence, remaining risks, and synchronized task/plan status. Record every +exact command and outcome in `## Results`. + +## Results + +Extended `apps/backend/internal/auth/httpapi/handlers_test.go`: `apiClient.do` +now takes a variadic request mutator; `newAPIFixture` accepts a variadic +trusted-proxy list and calls `SetTrustedProxies` (nil by default, mirroring +production); added `loginSessionIP` (setup bootstrap → mutated login → +`current: true` session) and 6 `TestLoginSessionIP*` tests. + +- RED: `cd apps/backend && go test ./internal/auth/httpapi -run '^TestLoginSessionIPNoTrustedProxiesIgnoresForwardedFor$' -count=1` — `session IP = "203.0.113.7", want peer 10.0.0.5` (gin's trust-all fixture default honored the header). +- GREEN: `go test ./internal/auth/httpapi -run '^TestLoginSessionIP' -count=1` — ok (6 tests: no-trusted-proxies, trusted peer, untrusted CIDR, X-Real-IP only, malformed XFF + valid X-Real-IP, neither header). +- Full auth suite: `go test ./internal/auth/... -count=1` — ok (auth, auth/httpapi, auth/httpmw, auth/store). +- Manual smoke is the plan-level integration gate; blocked in this harness (no proxied deployment). diff --git a/docs/plans/trusted-proxies/task-03-configuration-docs.md b/docs/plans/trusted-proxies/task-03-configuration-docs.md new file mode 100644 index 0000000000..bd6e9e17b8 --- /dev/null +++ b/docs/plans/trusted-proxies/task-03-configuration-docs.md @@ -0,0 +1,99 @@ +--- +id: "03-configuration-docs" +title: "Configuration docs" +status: done +wave: 2 +depends_on: [] +plan: "plan.md" +spec: "../../specs/auth/trusted-proxies.md" +--- + +# Task 03: Configuration docs + +Document `KANDEV_TRUSTED_PROXIES` in the public startup-configuration +reference. + +## Acceptance + +- `docs/public/configuration.md` documents the variable: comma-separated IPs / + CIDRs (IPv4 and IPv6), default (unset = no trusted proxies, header ignored), + the spoofing security implication (a directly reachable backend whose peer + falls inside a trusted range can have `X-Forwarded-For` spoofed, which also + defeats the ClientIP-keyed login rate limiter), invalid-entry behavior + (startup warning naming the bad value, whole variable ignored, no crash), + and the note that the variable must reach the backend process: set it in + the environment of the process that launches kandev (service unit, + container, or parent launcher env, e.g. a systemd drop-in for + `kandev.service`). `~/.kandev/supervisor/launch.json` is launcher-generated + (rewritten every launch, env filtered by an allowlist) and is NOT an + environment configuration source. The variable is read once at startup. +- Copy follows the env-only variable prose pattern of + `KANDEV_TASK_PREPARATION_TIMEOUT` in the same file, and the no-em-dash rule + for public copy (ADR 2026-08-10). + +## Files likely touched + +- `docs/public/configuration.md` (new `###` section after the Authentication / + feature-flags section, before Logging) + +## Dependencies + +None (text describes the Task 01 contract; files are disjoint from Tasks +01-02). + +## Parallelism + +`sequential` by default; scheduled as Wave 2 after Tasks 01-02. Parallel-safe +candidate only when the user authorizes subagents. + +## Inputs + +- Spec sections **What**, **Failure modes**; the task description's docs + requirements. +- `docs/public/configuration.md` structure (`## Complete backend reference`, + `### Authentication, Office, Plugins, and feature flags`, env-only prose + pattern in `### Setup and launch timing`). +- Launcher env flow: `apps/backend/internal/launcher/env.go` (`backendEnv` + starts from `os.Environ()`, so the launcher's own environment reaches the + backend child) and `apps/backend/internal/launcher/supervisor.go` + (`prepareSupervisorEnv`/`buildManifest`/`allowedSupervisorEnv`: the + `launch.json` manifest is rewritten every launch and its env is an + allowlist that omits `KANDEV_TRUSTED_PROXIES`, so it is not an injection + point). + +## Verification + +```bash +node scripts/validate-public-docs.mjs && git diff --check && git diff --stat docs/public/configuration.md +``` + +Read the rendered section back and confirm the format, default, security +implication, invalid-entry behavior, and reach-the-backend note are all +present; `scripts/validate-public-docs.mjs` enforces the no-em-dash rule for +`docs/public/**` (ADR 2026-08-10). + +## Risks + +- Adding a YAML-key row to the reference tables would imply a `config.yaml` + key that does not exist; the variable is environment-only and must be + documented as prose like `KANDEV_TASK_PREPARATION_TIMEOUT`. + +## Output contract + +Report the exact files changed, the added section text, diff-check evidence, +remaining risks, and synchronized task/plan status. Record every exact command +and outcome in `## Results`. + +## Results + +Added the `### Trusted proxies for X-Forwarded-For` section to +`docs/public/configuration.md` between the Authentication / feature-flags +section and Logging: format, default (unset = no trusted proxies), security +implication (callers inside a listed range can spoof; rate-limiter impact), +fail-closed invalid-entry behavior (including trailing/doubled commas), and +reach-the-backend guidance (process-launcher environment, e.g. systemd +drop-in; `launch.json` explicitly noted as launcher-generated and not an +environment source). + +- `node scripts/validate-public-docs.mjs` — "Validated 41 published docs pages." (no em dashes; ADR 2026-08-10). +- `git diff --check` — clean. diff --git a/docs/public/configuration.md b/docs/public/configuration.md index 045b6bbe07..67764f310f 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -155,6 +155,41 @@ Do not infer security from `auth.jwtSecret`: setting it currently does not turn [Voice Mode plugin](https://github.com/kdlbs/kandev-plugin-voice), and its transcription key lives in that plugin's own settings. Kandev ignores the old key and no longer serves `/api/v1/transcribe`. +### Trusted proxies for X-Forwarded-For + +`KANDEV_TRUSTED_PROXIES` lists the reverse proxies whose forwarded client IP +Kandev trusts. Format: a comma-separated list of IP addresses or CIDR ranges, +for example `10.0.0.0/8,192.168.0.0/16`. IPv6 addresses and CIDRs are +accepted. When the TCP peer of a request is in the list, the client IP is +read from `X-Forwarded-For` (then `X-Real-IP`); otherwise those headers are +ignored and the TCP peer address is used. The resolved IP feeds the login +session record (Settings > Account > Security) and the login rate-limiter +key. + +Default: unset, meaning no trusted proxies. Forwarded headers are ignored +entirely and the recorded client IP is always the TCP peer. This is the +secure default: gin would otherwise trust every proxy by default, which lets +a directly reachable backend accept a spoofed client IP. + +Security implication: only list proxies you control that always overwrite the +forwarded headers. A backend reached directly by a caller whose address +falls inside a listed range can have `X-Forwarded-For` spoofed by that +caller, which also defeats the ClientIP-keyed login rate limiter (login +attempts are limited per IP+email). Callers outside the listed ranges still +fall back to their own peer address. + +An entry that is neither a valid IP nor a valid CIDR is rejected at startup +with a warning naming the bad value, and the whole variable is ignored (fail +closed: no partial trust). A trailing or doubled comma is treated the same +way. The backend never crashes on a bad value. + +The variable is read once at startup and must reach the backend process: set +it in the environment of the process that launches kandev, for example a +systemd drop-in for `kandev.service` (`systemctl --user edit +kandev.service`) or the container environment. The supervisor manifest at +`~/.kandev/supervisor/launch.json` is generated by the launcher on every +launch and is not an environment configuration source. + ### Logging | YAML key | Environment variable | Default | Current behavior | diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 5a013b0de0..0e30e84c00 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -266,6 +266,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [subagent-context-persistence](subagent-context-persistence/spec.md) | draft | | [auth](auth/spec.md) | building | | [auth — self-actions guard](auth/self-actions-guard.md) | shipped | +| [auth — trusted proxies](auth/trusted-proxies.md) | building | | [create-local-repository](create-local-repository/spec.md) | shipped | | [workflow-cycle-guardrails](workflow-cycle-guardrails/spec.md) | building | | [improve-kandev](improve-kandev/spec.md) | building | diff --git a/docs/specs/auth/trusted-proxies.md b/docs/specs/auth/trusted-proxies.md new file mode 100644 index 0000000000..957829ebca --- /dev/null +++ b/docs/specs/auth/trusted-proxies.md @@ -0,0 +1,86 @@ +--- +status: building +created: 2026-08-15 +owner: tbd +--- + +# Trusted Proxies for X-Forwarded-For + +## Why + +Behind a reverse proxy, Kandev records the proxy's IP as the login session IP +(Settings > Account > Security) and keys the login rate limiter on it, even +when the proxy forwards the real client in `X-Forwarded-For`. Operators who +front Kandev with a proxy have no way to make the recorded IP match the actual +client. + +## What + +- Kandev SHALL read a `KANDEV_TRUSTED_PROXIES` environment variable at + startup: a comma-separated list of IP addresses or CIDR ranges. +- When the TCP peer of a request is in the trusted list, the client IP SHALL + resolve from the first valid forwarded-IP header in gin's configured order + (`X-Forwarded-For`, then `X-Real-IP`), falling back to the peer only when + neither header yields a valid address. +- When the variable is unset or empty, or the peer is not in the list, the + client IP SHALL be the TCP peer; forwarded headers are ignored. +- When any entry fails to parse as an IP or CIDR, startup SHALL log a warning + naming the bad value(s) and the whole variable SHALL be ignored (fail + closed; no partial trust). Startup SHALL NOT fail. +- The resolved client IP feeds the same consumers as today: the session IP + recorded for login, setup, invite acceptance, and plugin-provided SSO + (`AuthenticateExternal`), and the login rate-limiter key. + +## Failure modes + +- **Invalid entries.** A warning names the bad values and the variable is + ignored entirely; `X-Forwarded-For` is never trusted and the recorded IP + stays the TCP peer. No crash. +- **No valid forwarded-IP header** on a request from a trusted peer: gin + falls back to the TCP peer (only when neither `X-Forwarded-For` nor + `X-Real-IP` yields a valid address). +- **Spoofing.** An operator who sets the variable while the backend is + directly reachable lets any caller whose peer address falls inside a + configured trusted IP/CIDR forge the client IP, including the login + rate-limiter key. Callers outside every configured range still fall back to + their own peer address. This is a documented tradeoff; network placement + stays the operator's responsibility. + +## Scenarios + +- **GIVEN** `KANDEV_TRUSTED_PROXIES` unset, **WHEN** a login request arrives + from a proxy carrying `X-Forwarded-For: `, **THEN** the session IP + is the proxy's address (the TCP peer). +- **GIVEN** `KANDEV_TRUSTED_PROXIES=10.0.0.0/8` and a request whose TCP peer + is `10.0.0.5`, **WHEN** the request carries `X-Forwarded-For: 203.0.113.7`, + **THEN** the login session IP is `203.0.113.7`. +- **GIVEN** `KANDEV_TRUSTED_PROXIES=192.168.0.0/16` and a request whose TCP + peer is `10.0.0.5`, **WHEN** the request carries `X-Forwarded-For: + 203.0.113.7`, **THEN** the session IP is `10.0.0.5` (header ignored). +- **GIVEN** `KANDEV_TRUSTED_PROXIES=10.0.0.0/8,not-a-cidr`, **WHEN** the + backend starts, **THEN** a startup warning names `not-a-cidr` and + `X-Forwarded-For` is ignored entirely. +- **GIVEN** `KANDEV_TRUSTED_PROXIES=10.0.0.0/8` and a backend reachable + directly, **WHEN** a caller whose peer address is `10.0.0.6` (inside the + trusted range) sends `X-Forwarded-For: `, **THEN** the forged IP is + recorded (documented operator tradeoff). +- **GIVEN** `KANDEV_TRUSTED_PROXIES=10.0.0.0/8` and a backend reachable + directly, **WHEN** a caller whose peer address is `203.0.113.99` (outside + every trusted range) sends `X-Forwarded-For: `, **THEN** the forged + header is ignored and the session IP is `203.0.113.99`. +- **GIVEN** a trusted peer, **WHEN** a request carries no `X-Forwarded-For` + and no `X-Real-IP` header, **THEN** the session IP is the TCP peer. +- **GIVEN** a trusted peer, **WHEN** a request carries only + `X-Real-IP: 203.0.113.7`, **THEN** the session IP is `203.0.113.7`. +- **GIVEN** a trusted peer, **WHEN** a request carries + `X-Forwarded-For: not-an-ip` and `X-Real-IP: 203.0.113.7`, **THEN** the + session IP is `203.0.113.7` (gin skips the malformed header and reads the + next configured one). + +## Out of scope + +- No change to `X-Forwarded-Proto` handling (session cookie secure flag, + update checks), WebSocket `remote_addr` logging, or `RequestLogger` fields. +- No proxy auto-detection, no launcher CLI flag, no UI changes. +- No per-consumer overrides: the trusted-proxy list applies to every + `ClientIP()` consumer, exactly as the unconfigured default does today.