diff --git a/apps/backend/internal/auth/httpmw/middleware.go b/apps/backend/internal/auth/httpmw/middleware.go index 29f9ec7399..5e1ea211a0 100644 --- a/apps/backend/internal/auth/httpmw/middleware.go +++ b/apps/backend/internal/auth/httpmw/middleware.go @@ -13,10 +13,13 @@ package httpmw import ( + "crypto/sha256" "net" "net/http" "net/netip" "strings" + "sync" + "unicode/utf8" "github.com/gin-gonic/gin" "go.uber.org/zap" @@ -64,26 +67,122 @@ func Middleware(svc *auth.Service) gin.HandlerFunc { // its own port-scoped cookie name, by setting the header directly. The same // trusted list that gates X-Forwarded-For (gin's ClientIP via // SetTrustedProxies) gates this header so the two trust decisions cannot -// diverge; an untrusted value is dropped (with a warning) and the resolver -// falls back to the request Host. backendapp passes the list returned by its +// diverge; an untrusted value is dropped (warned once per distinct peer and +// host, see forwardedHostWarnSet) and the resolver falls back to the request +// Host. backendapp passes the list returned by its // configureTrustedProxies, so the two uses share one configuration. func StripUntrustedForwardedHost(trusted []string, log *logger.Logger) gin.HandlerFunc { matcher := newTrustedProxyMatcher(trusted) + warned := newForwardedHostWarnSet() return func(c *gin.Context) { - if c.GetHeader("X-Forwarded-Host") == "" { + forwardedHost := c.GetHeader("X-Forwarded-Host") + if forwardedHost == "" { return } peer := remoteAddrHost(c.Request.RemoteAddr) if matcher.contains(peer) { return } - log.Warn("ignoring X-Forwarded-Host from untrusted peer", - zap.String("peer", peer), - zap.String("forwarded_host", c.GetHeader("X-Forwarded-Host"))) + if warned.first(peer, forwardedHost) { + log.Warn("ignoring X-Forwarded-Host from untrusted peer", + zap.String("peer", peer), + zap.String("forwarded_host", truncateForLog(forwardedHost)), + zap.String("hint", forwardedHostWarnHint)) + } c.Request.Header.Del("X-Forwarded-Host") } } +// forwardedHostWarnHint tells the operator how to fix the common cause: a real +// reverse proxy missing from the trusted list. The warning is a configuration +// signal, not a per-request event, so the fix travels with it. +const forwardedHostWarnHint = "set KANDEV_TRUSTED_PROXIES to this peer's IP or CIDR if it is your reverse proxy" + +// forwardedHostWarnLimit bounds the distinct (peer, forwarded host) pairs +// remembered for warn deduplication. A misconfigured proxy produces one pair; +// the cap keeps a client that varies either value from growing the set without +// bound, at the cost of only losing warnings once it is reached. +const forwardedHostWarnLimit = 64 + +// forwardedHostLogMaxLen caps the forwarded-host bytes written to the log. The +// header is unauthenticated and may run to the server's whole header budget +// (backendapp leaves MaxHeaderBytes at net/http's 1 MiB default), and dumping a +// megabyte per line would recreate, in one line, the flooding this +// deduplication exists to stop. The prefix is enough to recognize a hostname. +const forwardedHostLogMaxLen = 256 + +// truncateForLog shortens an attacker-controlled value to a bounded, readable +// prefix, cutting on a rune boundary so the log field stays valid UTF-8. +func truncateForLog(value string) string { + if len(value) <= forwardedHostLogMaxLen { + return value + } + cut := forwardedHostLogMaxLen + for cut > 0 && !utf8.RuneStart(value[cut]) { + cut-- + } + return value[:cut] + "...(truncated)" +} + +// forwardedHostWarnSet deduplicates the strip warning. The stripped header is +// a static property of the deployment (an untrusted proxy keeps forwarding on +// every request), so warning per request buries the rest of the log in +// thousands of identical lines while adding nothing after the first. One +// warning per distinct (peer, forwarded host) pair keeps the operator signal +// and still surfaces a genuinely new peer or host. +// +// Keys are SHA-256 digests rather than the pair itself: the forwarded host is +// unauthenticated and can approach the server's whole header budget, so +// retaining the raw values would let 64 oversized requests pin tens of MiB for +// the process lifetime despite the entry cap. A fixed-size digest bounds +// retention at forwardedHostWarnLimit * 32 bytes whatever the header size. +type forwardedHostWarnSet struct { + mu sync.RWMutex + seen map[[sha256.Size]byte]struct{} +} + +func newForwardedHostWarnSet() *forwardedHostWarnSet { + return &forwardedHostWarnSet{seen: make(map[[sha256.Size]byte]struct{})} +} + +// forwardedHostWarnKey digests a (peer, forwarded host) pair into a fixed-size +// dedup key. The NUL separator cannot appear in either value, so no pair can +// forge another pair's digest by moving the boundary. +func forwardedHostWarnKey(peer, forwardedHost string) [sha256.Size]byte { + digest := sha256.New() + digest.Write([]byte(peer)) + digest.Write([]byte{0}) + digest.Write([]byte(forwardedHost)) + var key [sha256.Size]byte + copy(key[:], digest.Sum(nil)) + return key +} + +// first reports whether this (peer, forwarded host) pair has not been warned +// about yet, recording it when so. It returns false once the pair is known or +// the set is full. +func (s *forwardedHostWarnSet) first(peer, forwardedHost string) bool { + key := forwardedHostWarnKey(peer, forwardedHost) + s.mu.RLock() + _, seen := s.seen[key] + atLimit := len(s.seen) >= forwardedHostWarnLimit + s.mu.RUnlock() + if seen || atLimit { + return false + } + + s.mu.Lock() + defer s.mu.Unlock() + if _, ok := s.seen[key]; ok { + return false + } + if len(s.seen) >= forwardedHostWarnLimit { + return false + } + s.seen[key] = struct{}{} + return true +} + // trustedProxyMatcher matches a peer IP against the trusted-proxy list (bare // IPs and CIDRs, the same entries configureTrustedProxies handed to gin). type trustedProxyMatcher struct { diff --git a/apps/backend/internal/auth/httpmw/middleware_test.go b/apps/backend/internal/auth/httpmw/middleware_test.go index 6a84be6147..b1f9e5859b 100644 --- a/apps/backend/internal/auth/httpmw/middleware_test.go +++ b/apps/backend/internal/auth/httpmw/middleware_test.go @@ -2,10 +2,14 @@ package httpmw import ( "context" + "crypto/sha256" + "fmt" "net/http" "net/http/httptest" "strings" + "sync" "testing" + "unicode/utf8" "github.com/gin-gonic/gin" "github.com/jmoiron/sqlx" @@ -525,4 +529,216 @@ func TestStripUntrustedForwardedHostWarns(t *testing.T) { if !warned { t.Fatal("no X-Forwarded-Host warning was logged") } + assertWarningHints(t, logs) +} + +func assertWarningHints(t *testing.T, logs *observer.ObservedLogs) { + t.Helper() + for i, entry := range logs.All() { + hint := "" + for _, field := range entry.Context { + if field.Key == "hint" { + hint = field.String + break + } + } + if hint != forwardedHostWarnHint { + t.Fatalf("warning %d hint = %q, want %q", i, hint, forwardedHostWarnHint) + } + } +} + +// stripWarnRouter builds a strip-middleware router over an observed logger and +// returns both, so a test can count the warnings a request sequence produces. +func stripWarnRouter(t *testing.T) (*gin.Engine, *observer.ObservedLogs) { + t.Helper() + 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() + router.Use(StripUntrustedForwardedHost(nil, log)) + router.GET("/", echoXFH) + return router, logs +} + +// serveStrip sends one request with the given peer and X-Forwarded-Host, +// asserting the header was stripped so warn counting never masks a behavior +// regression. +func serveStrip(t *testing.T, router *gin.Engine, peer, forwardedHost string) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = peer + req.Header.Set("X-Forwarded-Host", forwardedHost) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if got := rec.Body.String(); got != "stripped" { + t.Fatalf("peer %s host %s: body = %q, want stripped", peer, forwardedHost, got) + } +} + +// TestStripUntrustedForwardedHostWarnsOncePerPeer pins the deduplication: an +// untrusted proxy forwards on every request, so warning per request buries the +// log in thousands of identical lines. The first request warns; repeats from +// the same peer and host stay silent, and the header is still stripped. +func TestStripUntrustedForwardedHostWarnsOncePerPeer(t *testing.T) { + router, logs := stripWarnRouter(t) + for range 50 { + serveStrip(t, router, "203.0.113.9:5555", "public.example:8443") + } + if got := logs.Len(); got != 1 { + t.Fatalf("warnings for 50 identical requests = %d, want 1", got) + } + assertWarningHints(t, logs) +} + +// TestStripUntrustedForwardedHostWarnsPerDistinctPair pins the other half: a +// new peer, or a known peer presenting a new forwarded host, is genuinely new +// information and warns again. +func TestStripUntrustedForwardedHostWarnsPerDistinctPair(t *testing.T) { + router, logs := stripWarnRouter(t) + serveStrip(t, router, "203.0.113.9:5555", "public.example:8443") + serveStrip(t, router, "203.0.113.9:5555", "public.example:8443") // repeat: silent + serveStrip(t, router, "198.51.100.4:5555", "public.example:8443") + serveStrip(t, router, "203.0.113.9:5555", "other.example:8443") + if got := logs.Len(); got != 3 { + t.Fatalf("warnings for 3 distinct pairs = %d, want 3", got) + } + assertWarningHints(t, logs) +} + +func TestForwardedHostWarnSetFirstIsAtMostOnce(t *testing.T) { + set := newForwardedHostWarnSet() + const calls = 100 + results := make(chan bool, calls) + var wg sync.WaitGroup + for range calls { + wg.Add(1) + go func() { + defer wg.Done() + results <- set.first("203.0.113.9", "public.example:8443") + }() + } + wg.Wait() + close(results) + + first := 0 + for result := range results { + if result { + first++ + } + } + if first != 1 { + t.Fatalf("first returned true %d times, want 1", first) + } +} + +// TestStripUntrustedForwardedHostWarnSetIsBounded pins the memory bound: a +// client that varies the forwarded host cannot grow the dedup set without +// limit. Warnings stop at the cap; stripping does not. +func TestStripUntrustedForwardedHostWarnSetIsBounded(t *testing.T) { + router, logs := stripWarnRouter(t) + for i := range forwardedHostWarnLimit * 2 { + serveStrip(t, router, "203.0.113.9:5555", fmt.Sprintf("host-%d.example:8443", i)) + } + if got := logs.Len(); got != forwardedHostWarnLimit { + t.Fatalf("warnings for %d distinct hosts = %d, want %d (cap)", + forwardedHostWarnLimit*2, got, forwardedHostWarnLimit) + } +} + +// oversizedHost builds a forwarded host near net/http's default 1 MiB header +// budget, which backendapp leaves unset: the largest value an unauthenticated +// client can actually get past the server. +func oversizedHost(seed int) string { + return fmt.Sprintf("%d.", seed) + strings.Repeat("a", 1<<20-16) + ".example:8443" +} + +// TestStripUntrustedForwardedHostRetainsFixedSizeKeys pins the memory bound in +// bytes, not just in entries: the forwarded host is unauthenticated and can +// approach the whole header budget, so retaining raw pair values would let 64 +// oversized requests pin tens of MiB for the process lifetime despite the entry +// cap. Keys are fixed-size digests, so retention does not scale with header +// size, and distinct oversized hosts still dedup independently. +func TestStripUntrustedForwardedHostRetainsFixedSizeKeys(t *testing.T) { + set := newForwardedHostWarnSet() + for i := range forwardedHostWarnLimit { + host := oversizedHost(i) + if !set.first("203.0.113.9", host) { + t.Fatalf("first oversized host %d: got false, want true", i) + } + if set.first("203.0.113.9", host) { + t.Fatalf("repeat oversized host %d: got true, want false", i) + } + } + retained := 0 + for key := range set.seen { + retained += len(key) + } + if want := forwardedHostWarnLimit * sha256.Size; retained != want { + t.Fatalf("retained key bytes = %d, want %d (fixed-size digests)", retained, want) + } +} + +// TestStripUntrustedForwardedHostDistinguishesLongSharedPrefixes pins what the +// digest buys over simply retaining a fixed-size prefix of the pair: hosts that +// differ only past the key width must still be told apart. A prefix key would +// collide here and silently swallow the second host's warning, which is the one +// an operator most needs when a proxy starts forwarding something new. +func TestStripUntrustedForwardedHostDistinguishesLongSharedPrefixes(t *testing.T) { + shared := strings.Repeat("a", 1<<20-64) + router, logs := stripWarnRouter(t) + serveStrip(t, router, "203.0.113.9:5555", shared+"-one.example:8443") + serveStrip(t, router, "203.0.113.9:5555", shared+"-two.example:8443") + if got := logs.Len(); got != 2 { + t.Fatalf("warnings for 2 hosts sharing a %d-byte prefix = %d, want 2", len(shared), got) + } +} + +// TestStripUntrustedForwardedHostTruncatesLoggedHost pins the other half of the +// same bound: an oversized header must not be echoed into the log verbatim, +// which would recreate in one line the flooding this deduplication exists to +// stop. The value is still stripped and still warned about once. +func TestStripUntrustedForwardedHostTruncatesLoggedHost(t *testing.T) { + router, logs := stripWarnRouter(t) + host := oversizedHost(0) + serveStrip(t, router, "203.0.113.9:5555", host) + serveStrip(t, router, "203.0.113.9:5555", host) + if got := logs.Len(); got != 1 { + t.Fatalf("warnings for repeated oversized host = %d, want 1", got) + } + logged := "" + for _, field := range logs.All()[0].Context { + if field.Key == "forwarded_host" { + logged = field.String + } + } + if want := forwardedHostLogMaxLen + len("...(truncated)"); len(logged) > want { + t.Fatalf("logged forwarded_host = %d bytes, want <= %d", len(logged), want) + } + if !strings.HasSuffix(logged, "...(truncated)") { + t.Fatalf("logged forwarded_host = %q, want a truncation marker", logged) + } + if !strings.HasPrefix(host, strings.TrimSuffix(logged, "...(truncated)")) { + t.Fatalf("logged forwarded_host %q is not a prefix of the header", logged) + } +} + +// TestTruncateForLogPreservesShortValuesAndRuneBoundaries pins the two edges of +// the truncation helper: a normal hostname is logged verbatim, and an oversized +// multi-byte value is cut on a rune boundary so the log field stays valid UTF-8. +func TestTruncateForLogPreservesShortValuesAndRuneBoundaries(t *testing.T) { + if got := truncateForLog("public.example:8443"); got != "public.example:8443" { + t.Fatalf("short value = %q, want it unchanged", got) + } + // "é" is 2 bytes, so the cap lands mid-rune for at least one repeat count. + for _, extra := range []int{0, 1} { + value := strings.Repeat("é", forwardedHostLogMaxLen/2+extra+8) + got := truncateForLog(value) + if !utf8.ValidString(got) { + t.Fatalf("truncated multi-byte value is not valid UTF-8: %q", got) + } + } } diff --git a/docs/public/configuration.md b/docs/public/configuration.md index 626d710a09..94f6000e9e 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -184,6 +184,14 @@ 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 same list gates `X-Forwarded-Host`, which a proxy sends so the port-scoped +session cookie can be resolved from the browser's original host. A proxy that +sends it while missing from the list logs +`ignoring X-Forwarded-Host from untrusted peer` with the peer address and the +forwarded host. That warning is logged once per distinct peer and host, not per +request, so it names the misconfiguration without flooding the log. Add the +peer it names to `KANDEV_TRUSTED_PROXIES` if it is your reverse proxy. + 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