diff --git a/cmd/membershipd/main.go b/cmd/membershipd/main.go index b4912183..e28a7283 100644 --- a/cmd/membershipd/main.go +++ b/cmd/membershipd/main.go @@ -59,6 +59,7 @@ func main() { natsStore = flag.String("nats-store", "./local_files/jetstream", "embedded JetStream store dir") busAuth = flag.String("bus-auth", "off", "control-plane auth rollout: off|soft|enforce (feature flag bus-auth)") corsOrigins = flag.String("cors-origins", "", "comma-separated CORS allowlist of browser origins permitted to call the control plane (e.g. http://localhost:5173,https://chat.example.com); empty = CORS off. Enables the browser-native uniweb client (issue uniweb/0001)") + trustedProxies = flag.String("trusted-proxies", "", "comma-separated IPs/CIDRs of reverse proxies whose X-Forwarded-For/X-Real-IP is trusted for the per-IP rate limit; empty = trust the direct connection only. Set to the same-origin proxy's address (e.g. the Caddy node) so the rate limit stays per-client behind the proxy") tlsCert = flag.String("tls-cert", "", "PATH to the NATS server certificate (deploy/tls/server.crt); enables TLS on the embedded data plane") tlsKey = flag.String("tls-key", "", "path to the NATS server private key (deploy/tls/server.key); required with --tls-cert") // Cluster (issue 0003a): empty --cluster-name keeps the server standalone. @@ -357,6 +358,17 @@ func main() { srv.AllowedOrigins = origins log.Printf("CORS: allowing %d browser origin(s): %s", len(origins), strings.Join(origins, ", ")) } + // Trusted reverse proxies for the per-IP rate limit. Behind the same-origin + // Caddy proxy every request arrives with the proxy's IP, which would collapse + // the per-IP rate limit into one bucket for the whole world; naming the proxy + // here lets the limiter believe its X-Forwarded-For and key on the real client + // instead. Empty flag => trust the direct connection only (unchanged behavior). + if proxies := splitRoutes(*trustedProxies); len(proxies) > 0 { + if err := srv.SetTrustedProxies(proxies); err != nil { + log.Fatalf("invalid --trusted-proxies: %v", err) + } + log.Printf("rate limit: trusting forwarded client IP from proxies: %s", strings.Join(proxies, ", ")) + } // Replicated anti-replay (issue 0006a, audit 0008 N3): a clustered node MUST // share its nonce store across the cluster, or a request accepted on one node diff --git a/pkg/membership/ratelimit.go b/pkg/membership/ratelimit.go index 02b81161..d7da29b1 100644 --- a/pkg/membership/ratelimit.go +++ b/pkg/membership/ratelimit.go @@ -1,8 +1,10 @@ package membership import ( + "fmt" "net" "net/http" + "strings" "sync" "time" @@ -78,16 +80,117 @@ func (l *ipRateLimiter) reapLocked(now time.Time) { } } -// clientIP extracts the source IP of an HTTP request, stripping the port. It -// trusts the transport's RemoteAddr only (no X-Forwarded-For parsing): a public -// deployment terminates TLS at this process or behind a proxy that the operator -// controls, and honoring an attacker-supplied header would let a single IP fan -// its quota across forged identities. If parsing fails the whole RemoteAddr is -// used as the key (still a stable per-connection bucket). -func clientIP(r *http.Request) string { +// clientIP extracts the rate-limit key for a request: the source IP, with the +// port stripped. By default it trusts the transport's RemoteAddr ONLY (no +// X-Forwarded-For parsing): honoring an attacker-supplied header would let a +// single IP fan its quota across forged identities. When the operator runs the +// control plane behind a reverse proxy they control (the same-origin Caddy +// deployment), SetTrustedProxies names that proxy's address(es); only then, and +// only when the immediate peer is one of them, is the forwarded client IP +// believed. This keeps the per-IP rate limit meaningful behind the proxy, where +// every request would otherwise share the proxy's single IP. If parsing fails the +// whole RemoteAddr is used as the key (still a stable per-connection bucket). +func (s *Server) clientIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - return r.RemoteAddr + host = r.RemoteAddr + } + if !s.trustedProxies.has(host) { + return host + } + if fwd := forwardedClientIP(r, s.trustedProxies); fwd != "" { + return fwd } return host } + +// forwardedClientIP returns the real client IP a trusted proxy reported, or "" if +// none is present. X-Forwarded-For is read RIGHT-TO-LEFT: the rightmost entry is +// the one our immediate (trusted) proxy appended and therefore cannot be spoofed +// by the client, which can only prepend entries to the left. Trusted-proxy hops +// are skipped so a chain of proxies we own resolves to the first address none of +// them owns — the actual external client. X-Real-IP is a single-value fallback for +// proxies that set it instead. A non-trusted immediate peer never reaches here, so +// a direct attacker's forged header is ignored entirely. +func forwardedClientIP(r *http.Request, trusted trustedProxyMatcher) string { + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + parts := strings.Split(xff, ",") + for i := len(parts) - 1; i >= 0; i-- { + ip := strings.TrimSpace(parts[i]) + if ip == "" || trusted.has(ip) { + continue + } + if net.ParseIP(ip) != nil { + return ip + } + } + } + if xrip := strings.TrimSpace(r.Header.Get("X-Real-IP")); xrip != "" { + if net.ParseIP(xrip) != nil { + return xrip + } + } + return "" +} + +// trustedProxyMatcher is the set of reverse-proxy addresses whose forwarding +// headers may be honored. The zero value (nil) matches nothing, so the default +// behavior is RemoteAddr-only. +type trustedProxyMatcher []*net.IPNet + +// SetTrustedProxies configures the proxies whose X-Forwarded-For / X-Real-IP this +// server trusts for the per-IP rate limit. Each entry is an IP (treated as a /32 +// or /128) or a CIDR. It returns an error on the first unparseable entry and +// leaves the previous configuration unchanged. Passing no entries clears the set. +func (s *Server) SetTrustedProxies(entries []string) error { + m, err := parseTrustedProxies(entries) + if err != nil { + return err + } + s.trustedProxies = m + return nil +} + +// parseTrustedProxies turns a list of IPs/CIDRs into a matcher. A bare IP becomes +// a host route (/32 for IPv4, /128 for IPv6); blanks are skipped. +func parseTrustedProxies(entries []string) (trustedProxyMatcher, error) { + var m trustedProxyMatcher + for _, e := range entries { + e = strings.TrimSpace(e) + if e == "" { + continue + } + if _, ipnet, err := net.ParseCIDR(e); err == nil { + m = append(m, ipnet) + continue + } + ip := net.ParseIP(e) + if ip == nil { + return nil, fmt.Errorf("trusted proxy %q is not an IP or CIDR", e) + } + bits := 32 + if ip.To4() == nil { + bits = 128 + } + m = append(m, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + } + return m, nil +} + +// has reports whether host (an IP string with no port) falls inside any trusted +// range. A nil matcher and an unparseable host both report false. +func (m trustedProxyMatcher) has(host string) bool { + if len(m) == 0 { + return false + } + ip := net.ParseIP(host) + if ip == nil { + return false + } + for _, n := range m { + if n.Contains(ip) { + return true + } + } + return false +} diff --git a/pkg/membership/ratelimit_proxy_test.go b/pkg/membership/ratelimit_proxy_test.go new file mode 100644 index 00000000..8f36f393 --- /dev/null +++ b/pkg/membership/ratelimit_proxy_test.go @@ -0,0 +1,113 @@ +package membership + +import ( + "net/http" + "testing" +) + +// TestClientIPTrustedProxy covers the rate-limit key extraction behind a reverse +// proxy: forwarding headers are believed ONLY when the immediate peer is a +// configured trusted proxy, and never otherwise. This is what keeps the per-IP +// rate limit per-client once the control plane runs behind the same-origin Caddy +// proxy, without opening a quota-fanning hole for a direct attacker. +func TestClientIPTrustedProxy(t *testing.T) { + const caddy = "135.125.201.30" + + cases := []struct { + name string + proxies []string + remote string + xff string + xRealIP string + want string + }{ + { + name: "no trusted proxies ignores XFF", + remote: "203.0.113.7:5000", + xff: "1.2.3.4", + want: "203.0.113.7", + }, + { + name: "trusted proxy honors XFF client", + proxies: []string{caddy}, + remote: caddy + ":4451", + xff: "198.51.100.23", + want: "198.51.100.23", + }, + { + name: "loopback proxy honors XFF (magnus-local hop)", + proxies: []string{"127.0.0.1/32", "::1/128"}, + remote: "127.0.0.1:33344", + xff: "198.51.100.99", + want: "198.51.100.99", + }, + { + name: "untrusted peer cannot spoof XFF", + proxies: []string{caddy}, + remote: "203.0.113.7:5000", + xff: "10.0.0.1", + want: "203.0.113.7", + }, + { + name: "XFF read right-to-left, trusted hops skipped", + proxies: []string{caddy}, + remote: caddy + ":4451", + xff: "198.51.100.23, " + caddy, + want: "198.51.100.23", + }, + { + name: "client-prepended forgery is skipped, real appended wins", + proxies: []string{caddy}, + remote: caddy + ":4451", + xff: "9.9.9.9, 198.51.100.23", + want: "198.51.100.23", + }, + { + name: "X-Real-IP fallback when no XFF", + proxies: []string{caddy}, + remote: caddy + ":4451", + xRealIP: "198.51.100.77", + want: "198.51.100.77", + }, + { + name: "trusted peer but no forwarding header falls back to peer", + proxies: []string{caddy}, + remote: caddy + ":4451", + want: caddy, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := &Server{} + if len(tc.proxies) > 0 { + if err := s.SetTrustedProxies(tc.proxies); err != nil { + t.Fatalf("SetTrustedProxies(%v): %v", tc.proxies, err) + } + } + r, _ := http.NewRequest(http.MethodGet, "/rooms", nil) + r.RemoteAddr = tc.remote + if tc.xff != "" { + r.Header.Set("X-Forwarded-For", tc.xff) + } + if tc.xRealIP != "" { + r.Header.Set("X-Real-IP", tc.xRealIP) + } + if got := s.clientIP(r); got != tc.want { + t.Fatalf("clientIP = %q, want %q", got, tc.want) + } + }) + } +} + +// TestParseTrustedProxiesRejectsGarbage proves a malformed entry is a hard error +// (the command turns it into a startup failure) rather than a silently ignored +// misconfiguration that would leave the rate limit collapsed behind the proxy. +func TestParseTrustedProxiesRejectsGarbage(t *testing.T) { + if _, err := parseTrustedProxies([]string{"not-an-ip"}); err == nil { + t.Fatal("expected error for non-IP/CIDR entry, got nil") + } + if _, err := parseTrustedProxies([]string{"10.0.0.0/8", "127.0.0.1"}); err != nil { + t.Fatalf("valid entries rejected: %v", err) + } +} diff --git a/pkg/membership/server.go b/pkg/membership/server.go index ee59bb59..bd7b7f4e 100644 --- a/pkg/membership/server.go +++ b/pkg/membership/server.go @@ -97,6 +97,17 @@ type Server struct { // before — so this is opt-in per deployment. Entries are matched exactly (scheme // + host + port); never use "*" with credentials. Set by the command from a flag. AllowedOrigins []string + + // trustedProxies names the reverse proxies whose forwarding headers + // (X-Forwarded-For / X-Real-IP) the rate limiter is allowed to believe. It + // exists for the same-origin deployment where a single proxy (Caddy) fronts + // the control plane: without it every proxied request would share the proxy's + // one IP and collapse the per-IP rate limit into a single bucket for the whole + // world. Only when the immediate peer is one of these addresses is the + // forwarded client IP trusted; the zero value (nil) trusts nobody, preserving + // the RemoteAddr-only behavior that predates the flag. Set by the command via + // SetTrustedProxies. See clientIP. + trustedProxies trustedProxyMatcher } // Posture describes the security posture a membershipd node runs with. It is @@ -165,7 +176,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Per-IP rate limit runs first, ahead of auth and body reads, so a flood is // shed at the cheapest possible point. The health probe is exempt so liveness // checks are never throttled. - if !isAuthExempt(r) && !s.limiter.allow(clientIP(r), now) { + if !isAuthExempt(r) && !s.limiter.allow(s.clientIP(r), now) { writeErr(w, http.StatusTooManyRequests, "rate limit exceeded") return }