caddyhttp: Security enhancements for client IP parsing (#5805)

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

caddyconfig/httpcaddyfile/serveroptions.go

@@ -46,6 +46,7 @@ type serverOptions struct {
 	Protocols            []string
 	StrictSNIHost        *bool
 	TrustedProxiesRaw    json.RawMessage
+	TrustedProxiesStrict int
 	ClientIPHeaders      []string
 	ShouldLogCredentials bool
 	Metrics              *caddyhttp.Metrics
@@ -217,6 +218,12 @@ func unmarshalCaddyfileServerOptions(d *caddyfile.Dispenser) (any, error) {
 				)
 				serverOpts.TrustedProxiesRaw = jsonSource
 
+			case "trusted_proxies_strict":
+				if d.NextArg() {
+					return nil, d.ArgErr()
+				}
+				serverOpts.TrustedProxiesStrict = 1
+
 			case "client_ip_headers":
 				headers := d.RemainingArgs()
 				for _, header := range headers {
@@ -340,6 +347,7 @@ func applyServerOptions(
 		server.StrictSNIHost = opts.StrictSNIHost
 		server.TrustedProxiesRaw = opts.TrustedProxiesRaw
 		server.ClientIPHeaders = opts.ClientIPHeaders
+		server.TrustedProxiesStrict = opts.TrustedProxiesStrict
 		server.Metrics = opts.Metrics
 		if opts.ShouldLogCredentials {
 			if server.Logs == nil {

modules/caddyhttp/server.go

@@ -173,6 +173,19 @@ type Server struct {
 	// remote IP address.
 	ClientIPHeaders []string `json:"client_ip_headers,omitempty"`
 
+	// If greater than zero, enables strict ClientIPHeaders
+	// (default X-Forwarded-For) parsing. If enabled, the
+	// ClientIPHeaders will be parsed from right to left, and
+	// the first value that is both valid and doesn't match the
+	// trusted proxy list will be used as client IP. If zero,
+	// the ClientIPHeaders will be parsed from left to right,
+	// and the first value that is a valid IP address will be
+	// used as client IP.
+	//
+	// This depends on `trusted_proxies` being configured.
+	// This option is disabled by default.
+	TrustedProxiesStrict int `json:"trusted_proxies_strict,omitempty"`
+
 	// Enables access logging and configures how access logs are handled
 	// in this server. To minimally enable access logs, simply set this
 	// to a non-null, empty struct.
@@ -839,17 +852,28 @@ func determineTrustedProxy(r *http.Request, s *Server) (bool, string) {
 	if s.trustedProxies == nil {
 		return false, ipAddr.String()
 	}
-	for _, ipRange := range s.trustedProxies.GetIPRanges(r) {
+
-		if ipRange.Contains(ipAddr) {
+	if isTrustedClientIP(ipAddr, s.trustedProxies.GetIPRanges(r)) {
-			// We trust the proxy, so let's try to
+		if s.TrustedProxiesStrict > 0 {
-			// determine the real client IP
+			return true, strictUntrustedClientIp(r, s.ClientIPHeaders, s.trustedProxies.GetIPRanges(r), ipAddr.String())
-			return true, trustedRealClientIP(r, s.ClientIPHeaders, ipAddr.String())
 		}
+		return true, trustedRealClientIP(r, s.ClientIPHeaders, ipAddr.String())
 	}
 
 	return false, ipAddr.String()
 }
 
+// isTrustedClientIP returns true if the given IP address is
+// in the list of trusted IP ranges.
+func isTrustedClientIP(ipAddr netip.Addr, trusted []netip.Prefix) bool {
+	for _, ipRange := range trusted {
+		if ipRange.Contains(ipAddr) {
+			return true
+		}
+	}
+	return false
+}
+
 // trustedRealClientIP finds the client IP from the request assuming it is
 // from a trusted client. If there is no client IP headers, then the
 // direct remote address is returned. If there are client IP headers,
@@ -884,6 +908,29 @@ func trustedRealClientIP(r *http.Request, headers []string, clientIP string) str
 	return clientIP
 }
 
+// strictUntrustedClientIp iterates through the list of client IP headers,
+// parses them from right-to-left, and returns the first valid IP address
+// that is untrusted. If no valid IP address is found, then the direct
+// remote address is returned.
+func strictUntrustedClientIp(r *http.Request, headers []string, trusted []netip.Prefix, clientIP string) string {
+	for _, headerName := range headers {
+		ips := strings.Split(strings.Join(r.Header.Values(headerName), ","), ",")
+
+		for i := len(ips) - 1; i >= 0; i-- {
+			ip, _, _ := strings.Cut(strings.TrimSpace(ips[i]), "%")
+			ipAddr, err := netip.ParseAddr(ip)
+			if err != nil {
+				continue
+			}
+			if !isTrustedClientIP(ipAddr, trusted) {
+				return ipAddr.String()
+			}
+		}
+	}
+
+	return clientIP
+}
+
 // cloneURL makes a copy of r.URL and returns a
 // new value that doesn't reference the original.
 func cloneURL(from, to *url.URL) {

modules/caddyhttp/server_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/netip"
 	"testing"
 	"time"
 
@@ -144,3 +145,294 @@ func BenchmarkServer_LogRequest_WithTraceID(b *testing.B) {
 		s.logRequest(accLog, req, wrec, &duration, repl, bodyReader, false)
 	}
 }
+func TestServer_TrustedRealClientIP_NoTrustedHeaders(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	ip := trustedRealClientIP(req, []string{}, "192.0.2.1")
+
+	assert.Equal(t, ip, "192.0.2.1")
+}
+
+func TestServer_TrustedRealClientIP_OneTrustedHeaderEmpty(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip, "192.0.2.1")
+}
+
+func TestServer_TrustedRealClientIP_OneTrustedHeaderInvalid(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	req.Header.Set("X-Forwarded-For", "not, an, ip")
+	ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip, "192.0.2.1")
+}
+
+func TestServer_TrustedRealClientIP_OneTrustedHeaderValid(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	req.Header.Set("X-Forwarded-For", "10.0.0.1")
+	ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip, "10.0.0.1")
+}
+
+func TestServer_TrustedRealClientIP_OneTrustedHeaderValidArray(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	req.Header.Set("X-Forwarded-For", "1.1.1.1, 2.2.2.2, 3.3.3.3")
+	ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip, "1.1.1.1")
+}
+
+func TestServer_TrustedRealClientIP_SkipsInvalidIps(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	req.Header.Set("X-Forwarded-For", "not an ip, bad bad, 10.0.0.1")
+	ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip, "10.0.0.1")
+}
+
+func TestServer_TrustedRealClientIP_MultipleTrustedHeaderValidArray(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+	req.Header.Set("Real-Client-IP", "1.1.1.1, 2.2.2.2, 3.3.3.3")
+	req.Header.Set("X-Forwarded-For", "3.3.3.3, 4.4.4.4")
+	ip1 := trustedRealClientIP(req, []string{"X-Forwarded-For", "Real-Client-IP"}, "192.0.2.1")
+	ip2 := trustedRealClientIP(req, []string{"Real-Client-IP", "X-Forwarded-For"}, "192.0.2.1")
+	ip3 := trustedRealClientIP(req, []string{"Missing-Header-IP", "Real-Client-IP", "X-Forwarded-For"}, "192.0.2.1")
+
+	assert.Equal(t, ip1, "3.3.3.3")
+	assert.Equal(t, ip2, "1.1.1.1")
+	assert.Equal(t, ip3, "1.1.1.1")
+}
+
+func TestServer_DetermineTrustedProxy_NoConfig(t *testing.T) {
+	server := &Server{}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "192.0.2.1:12345"
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.False(t, trusted)
+	assert.Equal(t, clientIP, "192.0.2.1")
+}
+
+func TestServer_DetermineTrustedProxy_NoConfigIpv6(t *testing.T) {
+	server := &Server{}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "[::1]:12345"
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.False(t, trusted)
+	assert.Equal(t, clientIP, "::1")
+}
+
+func TestServer_DetermineTrustedProxy_NoConfigIpv6Zones(t *testing.T) {
+	server := &Server{}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "[::1%eth2]:12345"
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.False(t, trusted)
+	assert.Equal(t, clientIP, "::1")
+}
+
+func TestServer_DetermineTrustedProxy_TrustedLoopback(t *testing.T) {
+	loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{loopbackPrefix},
+		},
+		ClientIPHeaders: []string{"X-Forwarded-For"},
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "127.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "31.40.0.10")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "31.40.0.10")
+}
+
+func TestServer_DetermineTrustedProxy_UntrustedPrefix(t *testing.T) {
+	loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{loopbackPrefix},
+		},
+		ClientIPHeaders: []string{"X-Forwarded-For"},
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "31.40.0.10")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.False(t, trusted)
+	assert.Equal(t, clientIP, "10.0.0.1")
+}
+
+func TestServer_DetermineTrustedProxy_MultipleTrustedPrefixes(t *testing.T) {
+	loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8")
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{loopbackPrefix, localPrivatePrefix},
+		},
+		ClientIPHeaders: []string{"X-Forwarded-For"},
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "31.40.0.10")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "31.40.0.10")
+}
+
+func TestServer_DetermineTrustedProxy_MultipleTrustedClientHeaders(t *testing.T) {
+	loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8")
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{loopbackPrefix, localPrivatePrefix},
+		},
+		ClientIPHeaders: []string{"CF-Connecting-IP", "X-Forwarded-For"},
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("CF-Connecting-IP", "1.1.1.1, 2.2.2.2")
+	req.Header.Set("X-Forwarded-For", "3.3.3.3, 4.4.4.4")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "1.1.1.1")
+}
+
+func TestServer_DetermineTrustedProxy_MatchLeftMostValidIp(t *testing.T) {
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{localPrivatePrefix},
+		},
+		ClientIPHeaders: []string{"X-Forwarded-For"},
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.1")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "30.30.30.30")
+}
+
+func TestServer_DetermineTrustedProxy_MatchRightMostUntrusted(t *testing.T) {
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{localPrivatePrefix},
+		},
+		ClientIPHeaders:      []string{"X-Forwarded-For"},
+		TrustedProxiesStrict: 1,
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.1")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "45.54.45.54")
+}
+
+func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedSkippingEmpty(t *testing.T) {
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{localPrivatePrefix},
+		},
+		ClientIPHeaders:      []string{"Missing-Header", "CF-Connecting-IP", "X-Forwarded-For"},
+		TrustedProxiesStrict: 1,
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("CF-Connecting-IP", "not a real IP")
+	req.Header.Set("X-Forwarded-For", "30.30.30.30, bad, 45.54.45.54, not real")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "45.54.45.54")
+}
+
+func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedSkippingTrusted(t *testing.T) {
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{localPrivatePrefix},
+		},
+		ClientIPHeaders:      []string{"CF-Connecting-IP", "X-Forwarded-For"},
+		TrustedProxiesStrict: 1,
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("CF-Connecting-IP", "10.0.0.1, 10.0.0.2, 10.0.0.3")
+	req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.4")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "45.54.45.54")
+}
+
+func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedFirst(t *testing.T) {
+	localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")
+
+	server := &Server{
+		trustedProxies: &StaticIPRange{
+			ranges: []netip.Prefix{localPrivatePrefix},
+		},
+		ClientIPHeaders:      []string{"CF-Connecting-IP", "X-Forwarded-For"},
+		TrustedProxiesStrict: 1,
+	}
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("CF-Connecting-IP", "10.0.0.1, 90.100.110.120, 10.0.0.2, 10.0.0.3")
+	req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.4")
+
+	trusted, clientIP := determineTrustedProxy(req, server)
+
+	assert.True(t, trusted)
+	assert.Equal(t, clientIP, "90.100.110.120")
+}