diff --git a/internal/api/ratelimit.go b/internal/api/ratelimit.go index 7c44bd6..211612b 100644 --- a/internal/api/ratelimit.go +++ b/internal/api/ratelimit.go @@ -1,7 +1,9 @@ package api import ( + "net" "net/http" + "os" "strconv" "strings" "sync" @@ -48,33 +50,72 @@ func (l *limiter) allow(key string, rate, burst float64) bool { return true } -// clientIP extracts the client IP for rate-limit keying (#85). +// clientIP extracts the client IP for rate-limit keying (#85, #280). // -// Trust boundary: palette runs behind exactly ONE trusted reverse proxy -// (Traefik in the k3s pod network). Traefik APPENDS the real client IP to -// X-Forwarded-For, so the RIGHTMOST entry is the last value the trusted -// proxy observed and is unspoofable by the client (a client-supplied fake -// entry only lands on the LEFT and is ignored). This matches chi's -// middleware.RealIP semantics for a single trusted proxy hop. +// Trust boundary (revised in #280): the deployed ingress does NOT append the +// peer address to X-Forwarded-For, so every XFF entry (and X-Real-Ip) is +// client-controlled. Keying on any client-supplied header lets an attacker +// rotate the header per request and get a fresh rate-limit bucket every +// time. The bucket key therefore defaults to the PEER address (RemoteAddr), +// which the client cannot influence. // -// Direct connections (no XFF header) fall back to RemoteAddr. Directly -// reachable deployments must NOT expose the app to untrusted networks -// without a proxy in front, or attackers could forge the rightmost entry. +// Deployments that pin their proxy chain can restore per-client granularity: +// set PALETTE_CLIENT_IP_HEADER (e.g. "CF-Connecting-IP" once the proxy chain +// is pinned, e.g. Traefik forwardedHeaders.trustedIPs limited to Cloudflare +// ranges or an origin firewall locked to CF) and the header value is honored +// ONLY when the immediate peer (RemoteAddr) is inside PALETTE_TRUSTED_PROXIES +// (default: loopback + RFC1918 private ranges, i.e. the in-cluster Traefik +// hop). A public peer never triggers header trust, and the header value must +// parse as an IP. func clientIP(r *http.Request) string { - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - if i := strings.LastIndex(xff, ","); i >= 0 { - return strings.TrimSpace(xff[i+1:]) + peer := hostOnly(r.RemoteAddr) + if hdr := os.Getenv("PALETTE_CLIENT_IP_HEADER"); hdr != "" && trustedProxy(peer) { + if v := strings.TrimSpace(r.Header.Get(hdr)); net.ParseIP(v) != nil { + return v } - return strings.TrimSpace(xff) } - if xr := r.Header.Get("X-Real-Ip"); xr != "" { - return strings.TrimSpace(xr) + return peer +} + +// trustedProxy reports whether peer (an IP without port) falls inside any of +// the configured trusted proxy CIDRs (PALETTE_TRUSTED_PROXIES, default +// loopback + RFC1918 private ranges). Parsed once and cached. +func trustedProxy(peer string) bool { + trustedOnce.Do(func() { + spec := os.Getenv("PALETTE_TRUSTED_PROXIES") + if strings.TrimSpace(spec) == "" { + spec = "127.0.0.0/8,::1/128,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" + } + for _, c := range strings.Split(spec, ",") { + if _, cidr, err := net.ParseCIDR(strings.TrimSpace(c)); err == nil { + trustedCIDRs = append(trustedCIDRs, cidr) + } + } + }) + ip := net.ParseIP(peer) + if ip == nil { + return false } - host := r.RemoteAddr - if i := strings.LastIndex(host, ":"); i > 0 { - host = host[:i] + for _, cidr := range trustedCIDRs { + if cidr.Contains(ip) { + return true + } } - return host + return false +} + +var ( + trustedOnce sync.Once + trustedCIDRs []*net.IPNet +) + +// hostOnly strips the port from a host:port address. +func hostOnly(addr string) string { + host, _, err := net.SplitHostPort(addr) + if err != nil { + host = addr + } + return strings.TrimSpace(host) } var globalLimiter = newLimiter() diff --git a/internal/api/ratelimit_xff_test.go b/internal/api/ratelimit_xff_test.go index 8674636..788783b 100644 --- a/internal/api/ratelimit_xff_test.go +++ b/internal/api/ratelimit_xff_test.go @@ -1,85 +1,135 @@ package api -// Issue #85: the rate limit key must use the rightmost X-Forwarded-For entry -// (appended by the trusted Traefik proxy), never the raw/leftmost header -// value a client can forge. A spoofed FIRST XFF entry must not bypass the -// limit or rotate buckets. +// Issues #85 + #280: the rate limit bucket key must NEVER be derived from a +// client-controlled header. The deployed ingress does not append the peer +// address to X-Forwarded-For, so every XFF entry is attacker-controlled and +// an attacker can rotate the header per request to get a fresh bucket. The +// key defaults to the peer (RemoteAddr); an opt-in trusted header is honored +// only when the immediate peer is a trusted proxy. import ( "bytes" "net/http/httptest" + "sync" "testing" ) -func TestClientIPTakesRightmostXFF(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "10.42.0.7:51000" // trusted Traefik pod - r.Header.Set("X-Forwarded-For", "1.2.3.4, 1.2.3.5, 203.0.113.9") - if got := clientIP(r); got != "203.0.113.9" { - t.Fatalf("clientIP = %q, want rightmost 203.0.113.9", got) - } +// resetTrustedProxyCache clears the sync.Once-cached CIDR list so each test +// re-parses PALETTE_TRUSTED_PROXIES under its own env. +func resetTrustedProxyCache(t *testing.T) { + t.Helper() + t.Cleanup(func() { + trustedOnce = sync.Once{} + trustedCIDRs = nil + }) + trustedOnce = sync.Once{} + trustedCIDRs = nil } -func TestClientIPXRealIPFallback(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "10.42.0.7:51000" - r.Header.Set("X-Real-Ip", "203.0.113.10") - if got := clientIP(r); got != "203.0.113.10" { - t.Fatalf("clientIP = %q, want 203.0.113.10", got) - } -} - -func TestClientIPDirectFallback(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "198.51.100.5:51000" - if got := clientIP(r); got != "198.51.100.5" { - t.Fatalf("clientIP = %q, want 198.51.100.5", got) - } -} - -// TestRateLimitSpoofedFirstXFFDoesNotBypass: an attacker rotating a fake -// leftmost XFF entry stays limited on their real (rightmost) IP. -func TestRateLimitSpoofedFirstXFFDoesNotBypass(t *testing.T) { +// Rotating XFF must not create fresh buckets: all requests share the peer's +// bucket. This is the #280 pentest repro (8 creates, 6x201 before the fix). +func TestRateLimitRotatingXFFSharesPeerBucket(t *testing.T) { srv := newTestServer(t) h := srv.routes() - for i := 0; i < 5; i++ { + codes := []int{} + for i := 0; i < 8; i++ { req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - // each request spoofs a DIFFERENT leftmost entry - req.Header.Set("X-Forwarded-For", spoofN(i)+", 203.0.113.9") + req.RemoteAddr = "203.0.113.9:51000" // public peer + req.Header.Set("X-Forwarded-For", "9.9.9."+string(rune('0'+i))) rr := httptest.NewRecorder() h.ServeHTTP(rr, req) - if rr.Code != 201 { - t.Fatalf("req %d: want 201, got %d", i, rr.Code) + codes = append(codes, rr.Code) + } + allowed := 0 + for _, c := range codes { + if c == 201 { + allowed++ } } - // 6th request, still the same real IP, new spoofed prefix: must 429 + if allowed > 5 { + t.Fatalf("rotating XFF bypassed the limit: allowed=%d codes=%v", allowed, codes) + } + // and the no-header bucket is the SAME bucket: fully exhausted req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - req.Header.Set("X-Forwarded-For", "9.9.9.9, 203.0.113.9") + req.RemoteAddr = "203.0.113.9:51000" rr := httptest.NewRecorder() h.ServeHTTP(rr, req) if rr.Code != 429 { - t.Fatalf("spoofed 6th req: want 429, got %d", rr.Code) + t.Fatalf("no-header request after rotating XFF: want 429, got %d", rr.Code) } } -func spoofN(i int) string { - return "1.2.3." + string(rune('0'+i)) +// clientIP unit checks. +func TestClientIPIgnoresClientHeadersByDefault(t *testing.T) { + r := httptest.NewRequest("POST", "/", nil) + r.RemoteAddr = "198.51.100.5:51000" + r.Header.Set("X-Forwarded-For", "1.2.3.4, 5.6.7.8") + r.Header.Set("X-Real-Ip", "9.9.9.9") + if got := clientIP(r); got != "198.51.100.5" { + t.Fatalf("clientIP = %q, want peer 198.51.100.5", got) + } } -// Distinct real IPs must still get distinct buckets (no over-limiting). -func TestRateLimitDistinctRightmostIPsIndependent(t *testing.T) { - srv := newTestServer(t) - h := srv.routes() - for _, ip := range []string{"203.0.113.20", "203.0.113.21"} { - req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - req.Header.Set("X-Forwarded-For", "6.6.6.6, "+ip) - rr := httptest.NewRecorder() - h.ServeHTTP(rr, req) - if rr.Code != 201 { - t.Fatalf("ip %s: want 201, got %d", ip, rr.Code) +// Opt-in trusted header: honored only from a trusted private peer and only +// with a parseable IP value. +func TestClientIPTrustedHeaderFromPrivatePeer(t *testing.T) { + t.Setenv("PALETTE_CLIENT_IP_HEADER", "Cf-Connecting-Ip") + // reset the cached CIDR parse (sync.Once) for a clean run + resetTrustedProxyCache(t) + + r := httptest.NewRequest("POST", "/", nil) + r.RemoteAddr = "10.42.0.7:51000" // in-cluster proxy hop + r.Header.Set("Cf-Connecting-Ip", "203.0.113.42") + if got := clientIP(r); got != "203.0.113.42" { + t.Fatalf("clientIP = %q, want 203.0.113.42", got) + } + // same header from a PUBLIC peer must be ignored (peer not trusted) + r2 := httptest.NewRequest("POST", "/", nil) + r2.RemoteAddr = "203.0.113.9:51000" + r2.Header.Set("Cf-Connecting-Ip", "6.6.6.6") + if got := clientIP(r2); got != "203.0.113.9" { + t.Fatalf("clientIP = %q, want peer 203.0.113.9 (untrusted peer)", got) + } + // garbage header value must be ignored + r3 := httptest.NewRequest("POST", "/", nil) + r3.RemoteAddr = "10.42.0.7:51000" + r3.Header.Set("Cf-Connecting-Ip", "not-an-ip; 6.6.6.6") + if got := clientIP(r3); got != "10.42.0.7" { + t.Fatalf("clientIP = %q, want peer 10.42.0.7 (bad header value)", got) + } +} + +// Custom trusted proxy CIDR list is honored. +func TestClientIPCustomTrustedProxies(t *testing.T) { + t.Setenv("PALETTE_CLIENT_IP_HEADER", "Cf-Connecting-Ip") + t.Setenv("PALETTE_TRUSTED_PROXIES", "192.168.50.0/24") + resetTrustedProxyCache(t) + + r := httptest.NewRequest("POST", "/", nil) + r.RemoteAddr = "192.168.50.10:51000" + r.Header.Set("Cf-Connecting-Ip", "203.0.113.77") + if got := clientIP(r); got != "203.0.113.77" { + t.Fatalf("clientIP = %q, want 203.0.113.77", got) + } + // outside the custom list + r2 := httptest.NewRequest("POST", "/", nil) + r2.RemoteAddr = "10.42.0.7:51000" + r2.Header.Set("Cf-Connecting-Ip", "203.0.113.77") + if got := clientIP(r2); got != "10.42.0.7" { + t.Fatalf("clientIP = %q, want peer 10.42.0.7 (outside custom trusted list)", got) + } +} + +// IPv6 peer without port must not be mangled. +func TestHostOnly(t *testing.T) { + cases := map[string]string{ + "198.51.100.5:51000": "198.51.100.5", + "[2001:db8::1]:8080": "2001:db8::1", + "198.51.100.5": "198.51.100.5", + } + for in, want := range cases { + if got := hostOnly(in); got != want { + t.Errorf("hostOnly(%q) = %q, want %q", in, got, want) } } }