NGINX: Correctly determine client IP when using PROXY protocol and multiple proxies are involved

Author: dmeremyanin

internal/ingress/controller/config/config.go

@@ -569,6 +569,12 @@ type Configuration struct {
 	// Default is X-Forwarded-For
 	ForwardedForHeader string `json:"forwarded-for-header,omitempty"`
 
+	// Sets the name of the intermediate header used to determine the client's originating IP
+	// when both use-proxy-protocol and use-forwarded-headers are enabled. This doesn't impact
+	// functionality and should not typically be modified.
+	// Default is X-Forwarded-For-Proxy-Protocol
+	ForwardedForProxyProtocolHeader string `json:"forwarded-for-proxy-protocol-header,omitempty"`
+
 	// Append the remote address to the X-Forwarded-For header instead of replacing it
 	// Default: false
 	ComputeFullForwardedFor bool `json:"compute-full-forwarded-for,omitempty"`
@@ -780,6 +786,7 @@ func NewDefault() Configuration {
 		UseForwardedHeaders:              false,
 		EnableRealIP:                     false,
 		ForwardedForHeader:               "X-Forwarded-For",
+		ForwardedForProxyProtocolHeader:  "X-Forwarded-For-Proxy-Protocol",
 		ComputeFullForwardedFor:          false,
 		ProxyAddOriginalURIHeader:        false,
 		GenerateRequestID:                true,

rootfs/etc/nginx/template/nginx.tmpl

@@ -77,7 +77,9 @@ http {
     {{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}}
     {{/* we use the value of the real IP for the geo_ip module */}}
     {{ if or (or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol) $cfg.EnableRealIP }}
-    {{ if $cfg.UseProxyProtocol }}
+    {{ if and $cfg.UseForwardedHeaders $cfg.UseProxyProtocol }}
+    real_ip_header      {{ $cfg.ForwardedForProxyProtocolHeader }};
+    {{ else if $cfg.UseProxyProtocol }}
     real_ip_header      proxy_protocol;
     {{ else }}
     real_ip_header      {{ $cfg.ForwardedForHeader }};
@@ -409,6 +411,39 @@ http {
 
     {{ end }}
 
+    {{ if and $cfg.UseForwardedHeaders $cfg.UseProxyProtocol }}
+    # When the proxy protocol is enabled, we cannot rely solely on the proxy protocol address
+    # due to potential proxy chain issues. Multiple proxies may modify the client IP before
+    # it reaches the ingress controller. We use the proxy-real-ip-cidr list to trust specific proxy
+    # addresses and determine the correct client IP from the forwarded headers.
+    #
+    # --------     --------------             ---------------------                       -----------------
+    # | User | --> | HTTP Proxy | -- http --> | TCP Load Balancer | -- proxy protocol --> | Ingress Nginx |
+    # --------     --------------             ---------------------                       -----------------
+    #
+    # The algorithm for determining the header to be used with the real_ip_header:
+    # 1. Check if the $proxy_protocol_addr is trusted (i.e., whether it's in the proxy-real-ip-cidr list).
+    # 2. If trusted, use the configured forwarded-for header (X-Forwarded-For by default).
+    # 3. If not trusted, fall back to using the $proxy_protocol_addr.
+    #
+    geo $proxy_protocol_addr $proxy_protocol_addr_trusted {
+        default           0;
+        {{ range $trusted_ip := $cfg.ProxyRealIPCIDR }}
+        {{ $trusted_ip }} 1;
+        {{ end }}
+    }
+
+    map $proxy_protocol_addr_trusted $forwarded_for_proxy_protocol {
+        default   $proxy_protocol_addr;
+        1         {{ buildForwardedFor $cfg.ForwardedForHeader }};
+    }
+
+    # The realip module does not support variables for the real_ip_header directive
+    # so we need to define a custom header.
+    more_set_input_headers "{{ $cfg.ForwardedForProxyProtocolHeader }}: $forwarded_for_proxy_protocol";
+
+    {{ end }}
+
     # Create a variable that contains the literal $ character.
     # This works because the geo module will not resolve variables.
     geo $literal_dollar {

test/e2e/settings/proxy_protocol.go

@@ -44,6 +44,7 @@ var _ = framework.DescribeSetting("use-proxy-protocol", func() {
 		f.NewEchoDeployment()
 		f.UpdateNginxConfigMapData(setting, "false")
 	})
+
 	//nolint:dupl // Ignore dupl errors for similar test case
 	ginkgo.It("should respect port passed by the PROXY Protocol", func() {
 		host := proxyProtocol
@@ -227,4 +228,86 @@ var _ = framework.DescribeSetting("use-proxy-protocol", func() {
 		assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs")
 		assert.Contains(ginkgo.GinkgoT(), logs, `192.168.0.1`)
 	})
+
+	ginkgo.Context("when use-forwarded-headers setting is true", func() {
+		cmapData := map[string]string{}
+
+		cmapData[setting] = "true"
+		cmapData["use-forwarded-headers"] = "true"
+
+		ginkgo.It("should not trust X-Forwarded headers when the client IP address is not trusted", func() {
+			host := proxyProtocol
+
+			f.SetNginxConfigMapData(cmapData)
+
+			f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, nil))
+
+			f.WaitForNginxServer(host,
+				func(server string) bool {
+					return strings.Contains(server, "server_name proxy-protocol") &&
+						strings.Contains(server, "listen 80 proxy_protocol")
+				})
+
+			ip := f.GetNginxIP()
+
+			conn, err := net.Dial("tcp", net.JoinHostPort(ip, "80"))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error creating connection to %s:80", ip)
+			defer conn.Close()
+
+			header := "PROXY TCP4 192.168.0.1 192.168.0.11 56324 1234\r\n"
+			_, err = conn.Write([]byte(header))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error writing header")
+
+			_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: proxy-protocol\r\nX-Forwarded-For: 192.168.0.111\r\n\r\n"))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error writing request")
+
+			data, err := io.ReadAll(conn)
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error reading connection data")
+
+			body := string(data)
+			assert.Contains(ginkgo.GinkgoT(), body, fmt.Sprintf("host=%v", proxyProtocol))
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-port=1234")
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-proto=http")
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-for=192.168.0.1")
+		})
+
+		ginkgo.It("should trust X-Forwarded headers when the client IP address is trusted", func() {
+			host := proxyProtocol
+
+			// Trust IPs from the private network CIDR block and the client IP address in the proxy protocol header
+			cmapData["proxy-real-ip-cidr"] = "10.0.0.0/8,192.168.0.1/32"
+
+			f.SetNginxConfigMapData(cmapData)
+
+			f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, nil))
+
+			f.WaitForNginxServer(host,
+				func(server string) bool {
+					return strings.Contains(server, "server_name proxy-protocol") &&
+						strings.Contains(server, "listen 80 proxy_protocol")
+				})
+
+			ip := f.GetNginxIP()
+
+			conn, err := net.Dial("tcp", net.JoinHostPort(ip, "80"))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error creating connection to %s:80", ip)
+			defer conn.Close()
+
+			header := "PROXY TCP4 192.168.0.1 192.168.0.11 56324 1234\r\n"
+			_, err = conn.Write([]byte(header))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error writing header")
+
+			_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: proxy-protocol\r\nX-Forwarded-For: 192.168.0.111\r\n\r\n"))
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error writing request")
+
+			data, err := io.ReadAll(conn)
+			assert.Nil(ginkgo.GinkgoT(), err, "unexpected error reading connection data")
+
+			body := string(data)
+			assert.Contains(ginkgo.GinkgoT(), body, fmt.Sprintf("host=%v", proxyProtocol))
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-port=1234")
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-proto=http")
+			assert.Contains(ginkgo.GinkgoT(), body, "x-forwarded-for=192.168.0.111")
+		})
+	})
 })