diff --git a/internal/server/upgrade.go b/internal/server/upgrade.go index fd8a951..183ae67 100644 --- a/internal/server/upgrade.go +++ b/internal/server/upgrade.go @@ -4,6 +4,8 @@ import ( "errors" "log/slog" "net/http" + "net/netip" + "os" "strings" "sync" @@ -33,6 +35,7 @@ type UpgradeHandler struct { rateLimit *ratelimit.Limiter policy conn.Policy hub *hub.Hub + trustedProxies []netip.Prefix ipMu sync.Mutex ipCount map[string]int @@ -49,6 +52,7 @@ func NewUpgradeHandler(st store.Store, origins []string, reg registry.Registry, rateLimit: rl, policy: pol, hub: h, + trustedProxies: parseTrustedProxies(os.Getenv("WIREFAN_TRUSTED_PROXIES")), ipCount: map[string]int{}, ipCap: defaultIPCap, } @@ -87,23 +91,107 @@ func sanitizeLogValue(s string) string { return string(out) } -// clientIP extracts a best-effort source IP. Behind a trusted proxy the caller -// would want X-Forwarded-For, but for the direct-connect demo path RemoteAddr -// is fine. Strips the port suffix; handles IPv6 (e.g. "[::1]:1234") by -// preserving the bracketed host as the key. -func clientIP(r *http.Request) string { - addr := r.RemoteAddr - // IPv6: "[::1]:1234" - if strings.HasPrefix(addr, "[") { - if end := strings.LastIndex(addr, "]"); end > 0 { - return addr[:end+1] +// parseTrustedProxies turns a comma-separated CIDR / single-IP list into +// netip.Prefix values. Bare IPs (e.g. "127.0.0.1") become /32 or /128. +// Malformed entries are silently dropped — operators should verify with a +// smoke test, and a hard failure here would block boot on a typo without +// a config-validation tool to backstop it. +func parseTrustedProxies(raw string) []netip.Prefix { + if raw == "" { + return nil + } + var prefixes []netip.Prefix + for _, s := range strings.Split(raw, ",") { + s = strings.TrimSpace(s) + if s == "" { + continue + } + if p, err := netip.ParsePrefix(s); err == nil { + prefixes = append(prefixes, p) + continue + } + if a, err := netip.ParseAddr(s); err == nil { + bits := 32 + if a.Is6() { + bits = 128 + } + if p, err := a.Prefix(bits); err == nil { + prefixes = append(prefixes, p) + } + } + } + return prefixes +} + +func addrInPrefixes(a netip.Addr, prefixes []netip.Prefix) bool { + if !a.IsValid() { + return false + } + for _, p := range prefixes { + if p.Contains(a) { + return true + } + } + return false +} + +// stripHostPort handles "1.2.3.4:5678", "[::1]:5678", "[::1]", and "::1". +// Returns the address part as a netip.Addr, plus the original string form. +func stripHostPort(s string) (netip.Addr, string, bool) { + s = strings.TrimSpace(s) + if s == "" { + return netip.Addr{}, "", false + } + host := s + if strings.HasPrefix(s, "[") { + if end := strings.LastIndex(s, "]"); end > 0 { + host = s[1:end] + } + } else if i := strings.LastIndex(s, ":"); i > 0 && strings.Count(s, ":") == 1 { + // IPv4 with port. Bare IPv6 has multiple colons; leave it alone. + host = s[:i] + } + a, err := netip.ParseAddr(host) + if err != nil { + return netip.Addr{}, host, false + } + return a, host, true +} + +// clientIP returns a best-effort source IP. When the request's RemoteAddr is +// in trustedProxies, the X-Forwarded-For header is consulted using the +// rightmost-untrusted-hop algorithm: walk the XFF list right to left and +// return the first hop whose IP is not in trustedProxies. This is the +// algorithm called for in CWE-348 mitigations (do NOT use the leftmost +// entry — clients can pick that themselves). +func clientIP(r *http.Request, trustedProxies []netip.Prefix) string { + raAddr, raStr, raOk := stripHostPort(r.RemoteAddr) + if !raOk { + return r.RemoteAddr + } + if len(trustedProxies) == 0 || !addrInPrefixes(raAddr, trustedProxies) { + return raStr + } + xff := r.Header.Get("X-Forwarded-For") + if xff == "" { + return raStr + } + hops := strings.Split(xff, ",") + for i := len(hops) - 1; i >= 0; i-- { + hopAddr, hopStr, ok := stripHostPort(hops[i]) + if !ok { + continue + } + if !addrInPrefixes(hopAddr, trustedProxies) { + return hopStr } - return addr } - if i := strings.LastIndex(addr, ":"); i > 0 { - return addr[:i] + // Every XFF hop was trusted; treat the leftmost as the originator. + if leftAddr, leftStr, ok := stripHostPort(hops[0]); ok { + _ = leftAddr + return leftStr } - return addr + return raStr } func (h *UpgradeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -124,7 +212,7 @@ func (h *UpgradeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // model; we count ALL conns on the IP, not just phantom ones (a real // browser will only ever have a couple of tabs open at once, so the // distinction doesn't matter in practice). - ip := clientIP(r) + ip := clientIP(r, h.trustedProxies) h.ipMu.Lock() if h.ipCount[ip] >= h.ipCap { h.ipMu.Unlock() diff --git a/internal/server/upgrade_test.go b/internal/server/upgrade_test.go index d2eb313..4b519ff 100644 --- a/internal/server/upgrade_test.go +++ b/internal/server/upgrade_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "net/netip" "strings" "testing" "time" @@ -53,3 +54,92 @@ func newTestUpgrader(t *testing.T) http.Handler { t.Cleanup(rl.Close) return NewUpgradeHandler(store.NewMemory(), []string{"*"}, registry.NewSyncMap(), "test-signing-secret", fanout.NewPerConn(), rl, conn.PolicyDisconnect{}, hub.New()) } + +func TestParseTrustedProxies(t *testing.T) { + tests := []struct { + in string + want int + }{ + {"", 0}, + {"127.0.0.1", 1}, + {"127.0.0.1/32", 1}, + {"10.0.0.0/8,192.168.0.0/16", 2}, + {"127.0.0.1, ::1", 2}, + {" not-a-cidr ", 0}, + {"10.0.0.0/8,bogus,192.168.0.0/16", 2}, // bad entry dropped + } + for _, tt := range tests { + got := parseTrustedProxies(tt.in) + if len(got) != tt.want { + t.Errorf("parseTrustedProxies(%q) = %d prefixes, want %d", tt.in, len(got), tt.want) + } + } +} + +func TestClientIPNoTrust(t *testing.T) { + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = "203.0.113.5:1234" + r.Header.Set("X-Forwarded-For", "8.8.8.8") + if got := clientIP(r, nil); got != "203.0.113.5" { + t.Errorf("untrusted: got %q, want %q (XFF must be ignored without trust)", got, "203.0.113.5") + } +} + +func TestClientIPTrustedProxyPicksRightmostUntrusted(t *testing.T) { + trusted := parseTrustedProxies("127.0.0.1/32,10.0.0.0/8") + cases := []struct { + name string + remoteAddr string + xff string + want string + }{ + { + "loopback proxy + single client", + "127.0.0.1:8000", "203.0.113.5", "203.0.113.5", + }, + { + "chain of trusted proxies — first untrusted is the client", + "127.0.0.1:8000", "203.0.113.5, 10.0.0.5, 127.0.0.1", "203.0.113.5", + }, + { + "client tries to forge — first untrusted hop wins", + "127.0.0.1:8000", "1.2.3.4, 9.9.9.9, 10.0.0.5", "9.9.9.9", + }, + { + "all hops trusted — fall back to leftmost", + "127.0.0.1:8000", "10.0.0.1, 127.0.0.1, 10.0.0.2", "10.0.0.1", + }, + { + "untrusted source ignores XFF", + "203.0.113.5:1234", "1.2.3.4", "203.0.113.5", + }, + { + "IPv6 loopback proxy", + "[::1]:8000", "203.0.113.5", "203.0.113.5", + }, + } + // Add ::1 to trusted for the IPv6 case. + trusted = append(trusted, mustPrefix(t, "::1/128")) + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = c.remoteAddr + if c.xff != "" { + r.Header.Set("X-Forwarded-For", c.xff) + } + got := clientIP(r, trusted) + if got != c.want { + t.Errorf("clientIP = %q, want %q", got, c.want) + } + }) + } +} + +func mustPrefix(t *testing.T, s string) netip.Prefix { + t.Helper() + p, err := netip.ParsePrefix(s) + if err != nil { + t.Fatalf("ParsePrefix(%q): %v", s, err) + } + return p +}