diff --git a/web/server.go b/web/server.go index 2ec8179..ec834be 100644 --- a/web/server.go +++ b/web/server.go @@ -84,10 +84,7 @@ func (s *Server) ListenAndServe(ctx context.Context) error { func (s *Server) rateLimitMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ip, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - ip = r.RemoteAddr - } + ip := clientIP(r) if !s.rateLimiter.Allow(ip) { s.metrics.RateLimitHits.Add(1) @@ -100,6 +97,41 @@ func (s *Server) rateLimitMiddleware(next http.Handler) http.Handler { }) } +// clientIP returns the real client address for rate limiting and logging. +// +// Herald runs behind a local Caddy reverse proxy, so every request arrives +// from loopback and r.RemoteAddr is useless as an identity -- keying the rate +// limiter on it collapses all clients into a single bucket. Caddy forwards the +// real address in X-Forwarded-For. +// +// The header is only trusted when the direct peer is loopback (i.e. the proxy). +// A direct connection to the HTTP port is keyed on its socket address instead, +// so an attacker reaching the port cannot spoof arbitrary IPs via the header. +// When trusted, the rightmost entry is used: that is the address the proxy +// observed, whereas any values to its left were supplied by the client and +// must not be believed. +func clientIP(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + host = r.RemoteAddr + } + + peer := net.ParseIP(host) + if peer == nil || !peer.IsLoopback() { + return host + } + + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + parts := strings.Split(xff, ",") + for i := len(parts) - 1; i >= 0; i-- { + if ip := strings.TrimSpace(parts[i]); net.ParseIP(ip) != nil { + return ip + } + } + } + return host +} + func (s *Server) loggingMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -120,7 +152,7 @@ func (s *Server) loggingMiddleware(next http.Handler) http.Handler { "path", r.URL.Path, "status", lrw.statusCode, "duration_ms", duration.Milliseconds(), - "remote_addr", r.RemoteAddr, + "remote_addr", clientIP(r), ) if lrw.statusCode >= 500 { diff --git a/web/server_test.go b/web/server_test.go new file mode 100644 index 0000000..a6b480a --- /dev/null +++ b/web/server_test.go @@ -0,0 +1,67 @@ +package web + +import ( + "net/http" + "testing" +) + +func TestClientIP(t *testing.T) { + tests := []struct { + name string + remoteAddr string + xff string + want string + }{ + { + name: "direct connection ignores forwarded header", + remoteAddr: "203.0.113.5:44321", + xff: "1.2.3.4", + want: "203.0.113.5", + }, + { + name: "proxied request trusts forwarded header", + remoteAddr: "127.0.0.1:8085", + xff: "198.51.100.7", + want: "198.51.100.7", + }, + { + name: "proxied request uses rightmost forwarded entry", + remoteAddr: "127.0.0.1:8085", + xff: "1.2.3.4, 198.51.100.7", + want: "198.51.100.7", + }, + { + name: "spoofed non-ip entry is skipped", + remoteAddr: "127.0.0.1:8085", + xff: "198.51.100.7, not-an-ip", + want: "198.51.100.7", + }, + { + name: "proxied request without header falls back to peer", + remoteAddr: "127.0.0.1:8085", + xff: "", + want: "127.0.0.1", + }, + { + name: "ipv6 loopback peer is trusted", + remoteAddr: "[::1]:8085", + xff: "198.51.100.7", + want: "198.51.100.7", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &http.Request{ + RemoteAddr: tt.remoteAddr, + Header: http.Header{}, + } + if tt.xff != "" { + r.Header.Set("X-Forwarded-For", tt.xff) + } + if got := clientIP(r); got != tt.want { + t.Errorf("clientIP() = %q, want %q", got, tt.want) + } + }) + } +}