Merge 08ce94d1c0 into 1f049e5ebd

CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ## Changes since v7.15.3
 
+- security: harden `X-Forwarded-For` (and other `--real-client-ip-header` values) parsing so that, once `--trusted-proxy-ip` is configured, the client IP is resolved by walking the hop chain from the newest (rightmost) entry and skipping known trusted proxies, instead of trusting the client-supplied leftmost value. This closes a spoofing gap where a client could set the header to an IP on the `--trusted-ip` allowlist and have it trusted even though a legitimate reverse proxy only appended (rather than replaced) the header.
+
 # V7.15.3
 
 ## Release Highlights

docs/docs/configuration/overview.md

@@ -195,6 +195,8 @@ Provider specific options can be found on their respective subpages.
 
 :::warning
 When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers.
+
+Once `--trusted-proxy-ip` is configured, the value used for `--real-client-ip-header` (e.g. `X-Forwarded-For`) is parsed by walking the comma-separated hop list from the newest (rightmost) entry inward, skipping over entries that are themselves trusted proxies. The first entry that isn't a trusted proxy is used as the client IP. This prevents a client from bypassing the restriction by setting the header to an arbitrary value themselves: since a trusted proxy only ever *appends* to the header rather than replacing it, a client-supplied leftmost value can no longer be trusted, only the value the nearest trusted proxy actually observed. If the direct connection isn't itself a trusted proxy, the header is ignored entirely.
 :::
 
 | Flag / Config Field                                                           | Type           | Description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           | Default     |

oauthproxy.go

@@ -630,7 +630,7 @@ func (p *OAuthProxy) isTrustedIP(req *http.Request) bool {
 		return false
 	}
 
-	remoteAddr, err := ip.GetClientIP(p.realClientIPParser, req)
+	remoteAddr, err := ip.GetClientIP(p.realClientIPParser, req, requestTrustedProxies(req))
 	if err != nil {
 		logger.Errorf("Error obtaining real IP for trusted IP list: %v", err)
 		// Possibly spoofed X-Real-IP header
@@ -644,6 +644,15 @@ func (p *OAuthProxy) isTrustedIP(req *http.Request) bool {
 	return p.trustedIPs.Has(remoteAddr)
 }
 
+// requestTrustedProxies returns the set of proxies trusted to supply forwarded headers for
+// req, as computed for the request's RequestScope, or nil if no scope/restriction is set.
+func requestTrustedProxies(req *http.Request) ipapi.TrustedProxies {
+	if scope := middlewareapi.GetRequestScope(req); scope != nil {
+		return scope.TrustedProxies
+	}
+	return nil
+}
+
 // SignInPage writes the sign in template to the response
 func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code int) {
 	prepareNoCache(rw)
@@ -883,7 +892,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove
 // OAuthCallback is the OAuth2 authentication flow callback that finishes the
 // OAuth2 authentication flow
 func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
-	remoteAddr := ip.GetClientString(p.realClientIPParser, req, true)
+	remoteAddr := ip.GetClientString(p.realClientIPParser, req, true, requestTrustedProxies(req))
 
 	// finish the oauth cycle
 	// #nosec G120 -- The default max size in Go is already capped at 10MB so this would be the absolute max and is

oauthproxy_test.go

@@ -2177,6 +2177,7 @@ func TestTrustedIPs(t *testing.T) {
 	tests := []struct {
 		name               string
 		trustedIPs         []string
+		trustedProxyIPs    []string
 		reverseProxy       bool
 		realClientIPHeader string
 		req                *http.Request
@@ -2337,6 +2338,25 @@ func TestTrustedIPs(t *testing.T) {
 			}(),
 			expectTrusted: false,
 		},
+		// Reported vulnerability: a client sends X-Forwarded-For set to an IP on the
+		// --trusted-ip allowlist, hoping to bypass auth. The trusted reverse proxy appends the
+		// client's real (untrusted) IP rather than replacing the header. With --trusted-proxy-ip
+		// scoped to just the proxy, oauth2-proxy must walk to the rightmost hop (the attacker's
+		// real IP) instead of trusting the client-supplied leftmost value.
+		{
+			name:               "SpoofedLeftmostHopBehindTrustedProxyIsNotTrusted",
+			trustedIPs:         []string{"9.9.9.9"},
+			trustedProxyIPs:    []string{"10.0.0.5/32"},
+			reverseProxy:       true,
+			realClientIPHeader: "X-Forwarded-For",
+			req: func() *http.Request {
+				req, _ := http.NewRequest(http.MethodGet, "/", nil)
+				req.Header.Add("X-Forwarded-For", "9.9.9.9, 6.6.6.6")
+				req.RemoteAddr = "10.0.0.5:12345"
+				return req
+			}(),
+			expectTrusted: false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -2352,6 +2372,7 @@ func TestTrustedIPs(t *testing.T) {
 				},
 			}
 			opts.TrustedIPs = tt.trustedIPs
+			opts.TrustedProxyIPs = tt.trustedProxyIPs
 			opts.ReverseProxy = tt.reverseProxy
 			opts.RealClientIPHeader = tt.realClientIPHeader
 			err := validation.Validate(opts)

pkg/apis/ip/interfaces.go

@@ -5,7 +5,16 @@ import (
 	"net/http"
 )
 
+// TrustedProxies reports whether a given IP belongs to a proxy that is
+// trusted to supply forwarded headers.
+type TrustedProxies interface {
+	Has(net.IP) bool
+}
+
 // RealClientIPParser is an interface for a getting the client's real IP to be used for logging.
 type RealClientIPParser interface {
-	GetRealClientIP(http.Header) (net.IP, error)
+	// GetRealClientIP parses the configured forwarded-header out of h. remoteAddr is the
+	// IP of the direct connecting peer (nil if unknown) and trusted is the set of proxies
+	// allowed to supply forwarded headers (nil if no trusted-proxy restriction is configured).
+	GetRealClientIP(h http.Header, remoteAddr net.IP, trusted TrustedProxies) (net.IP, error)
 }

pkg/ip/realclientip.go

@@ -33,23 +33,52 @@ type xForwardedForClientIPParser struct {
 // GetRealClientIP obtain the IP address of the end-user (not proxy).
 // Parses headers sharing the format as specified by:
 // * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For.
-// Returns the `<client>` portion specified in the above document.
 // Additionally, is capable of parsing IPs with the port included, for v4 in the format "<ip>:<port>" and for v6 in the
 // format "[<ip>]:<port>".  With-port and without-port formats are seamlessly supported concurrently.
-func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, error) {
+//
-	var ipStr string
+// Each successive proxy may append itself, comma separated, to the end of the header. Blindly trusting the leftmost
-	if realIP := h.Get(p.header); realIP != "" {
+// (client-supplied) entry lets an untrusted client spoof it, so instead this walks the chain from the right (the
-		ipStr = realIP
+// most recently appended hop) and skips over entries that are themselves trusted proxies, returning the first entry
-	} else {
+// that isn't -- that's the address a trusted proxy is vouching for, which the originating client cannot forge. If
+// the direct connecting peer (remoteAddr) is not itself a trusted proxy, the header is ignored entirely and
+// remoteAddr is returned, since nothing in a client-controlled header can be believed in that case.
+func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header, remoteAddr net.IP, trusted ipapi.TrustedProxies) (net.IP, error) {
+	raw := h.Get(p.header)
+	if raw == "" {
 		return nil, nil
 	}
 
-	// Each successive proxy may append itself, comma separated, to the end of the X-Forwarded-for header.
+	isTrusted := func(candidate net.IP) bool {
-	// Select only the first IP listed, as it is the client IP recorded by the first proxy.
+		return trusted == nil || candidate == nil || trusted.Has(candidate)
-	if commaIndex := strings.IndexRune(ipStr, ','); commaIndex != -1 {
-		ipStr = ipStr[:commaIndex]
 	}
-	ipStr = strings.TrimSpace(ipStr)
+
+	if !isTrusted(remoteAddr) {
+		return remoteAddr, nil
+	}
+
+	hops := strings.Split(raw, ",")
+
+	var lastParsed net.IP
+	for i := len(hops) - 1; i >= 0; i-- {
+		hopIP, err := parseHopIP(hops[i], p.header)
+		if err != nil {
+			return nil, err
+		}
+		if !isTrusted(hopIP) {
+			return hopIP, nil
+		}
+		lastParsed = hopIP
+	}
+
+	// Every hop was itself a trusted proxy; fall back to the leftmost (oldest) entry, matching the historical
+	// behavior for when trusted proxies aren't restricted (trust-all default).
+	return lastParsed, nil
+}
+
+// parseHopIP parses a single comma-separated entry of a forwarded header into an IP,
+// stripping surrounding whitespace and an optional port.
+func parseHopIP(hop string, header string) (net.IP, error) {
+	ipStr := strings.TrimSpace(hop)
 
 	if ipHost, _, err := net.SplitHostPort(ipStr); err == nil {
 		ipStr = ipHost
@@ -57,16 +86,19 @@ func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, err
 
 	ip := net.ParseIP(ipStr)
 	if ip == nil {
-		return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(p.header))
+		return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(header))
 	}
 
 	return ip, nil
 }
 
 // GetClientIP obtains the perceived end-user IP address from headers if p != nil else from req.RemoteAddr.
-func GetClientIP(p ipapi.RealClientIPParser, req *http.Request) (net.IP, error) {
+func GetClientIP(p ipapi.RealClientIPParser, req *http.Request, trusted ipapi.TrustedProxies) (net.IP, error) {
 	if p != nil {
-		return p.GetRealClientIP(req.Header)
+		// Best-effort: an unparseable RemoteAddr becomes nil, which GetRealClientIP treats as trusted
+		// (e.g. unix sockets, or tests that don't set RemoteAddr), so this never blocks the header path.
+		remoteAddr, _ := getRemoteIP(req)
+		return p.GetRealClientIP(req.Header, remoteAddr, trusted)
 	}
 	return getRemoteIP(req)
 }
@@ -91,16 +123,18 @@ func getRemoteIP(req *http.Request) (net.IP, error) {
 }
 
 // GetClientString obtains the human readable string of the remote IP and optionally the real client IP if available
-func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool) (s string) {
+func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool, trusted ipapi.TrustedProxies) (s string) {
+	remoteIP, remoteErr := getRemoteIP(req)
+
 	var realClientIPStr string
 	if p != nil {
-		if realClientIP, err := p.GetRealClientIP(req.Header); err == nil && realClientIP != nil {
+		if realClientIP, err := p.GetRealClientIP(req.Header, remoteIP, trusted); err == nil && realClientIP != nil {
 			realClientIPStr = realClientIP.String()
 		}
 	}
 
 	var remoteIPStr string
-	if remoteIP, err := getRemoteIP(req); err == nil && remoteIP != nil {
+	if remoteErr == nil && remoteIP != nil {
 		remoteIPStr = remoteIP.String()
 	}
 

pkg/ip/realclientip_test.go

@@ -55,26 +55,77 @@ func TestXForwardedForClientIPParser(t *testing.T) {
 	p := &xForwardedForClientIPParser{header: http.CanonicalHeaderKey("X-Forwarded-For")}
 
 	tests := []struct {
+		name        string
 		headerValue string
+		remoteAddr  net.IP
+		trusted     *NetSet
 		errString   string
 		expectedIP  net.IP
 	}{
-		{"", "", nil},
+		// No trusted-proxy restriction configured (nil): behaves exactly like the historical
-		{"1.2.3.4", "", net.ParseIP("1.2.3.4")},
+		// leftmost-picking parser, regardless of remoteAddr.
-		{"10::23", "", net.ParseIP("10::23")},
+		{name: "empty header", headerValue: "", expectedIP: nil},
-		{"::1", "", net.ParseIP("::1")},
+		{name: "single hop", headerValue: "1.2.3.4", expectedIP: net.ParseIP("1.2.3.4")},
-		{"[::1]:1234", "", net.ParseIP("::1")},
+		{name: "single hop v6", headerValue: "10::23", expectedIP: net.ParseIP("10::23")},
-		{"10.0.10.11:1234", "", net.ParseIP("10.0.10.11")},
+		{name: "loopback v6", headerValue: "::1", expectedIP: net.ParseIP("::1")},
-		{"192.168.10.50, 10.0.0.1, 1.2.3.4", "", net.ParseIP("192.168.10.50")},
+		{name: "v6 with port", headerValue: "[::1]:1234", expectedIP: net.ParseIP("::1")},
-		{"nil", "unable to parse ip (nil) from X-Forwarded-For header", nil},
+		{name: "v4 with port", headerValue: "10.0.10.11:1234", expectedIP: net.ParseIP("10.0.10.11")},
-		{"10000.10000.10000.10000", "unable to parse ip (10000.10000.10000.10000) from X-Forwarded-For header", nil},
+		{name: "no trusted proxies falls back to leftmost", headerValue: "192.168.10.50, 10.0.0.1, 1.2.3.4", expectedIP: net.ParseIP("192.168.10.50")},
+		{name: "unparseable hop", headerValue: "nil", errString: "unable to parse ip (nil) from X-Forwarded-For header"},
+		{name: "malformed hop", headerValue: "10000.10000.10000.10000", errString: "unable to parse ip (10000.10000.10000.10000) from X-Forwarded-For header"},
+
+		// The reported attack: a client sends X-Forwarded-For set to an IP it wants to
+		// impersonate; the trusted reverse proxy appends the real client IP rather than
+		// replacing the header. Only the proxy's own /32 is trusted, so the walk must stop at
+		// the rightmost (attacker) hop instead of trusting the client-supplied leftmost value.
+		{
+			name:        "spoofed leftmost hop behind a trusted proxy",
+			headerValue: "9.9.9.9, 6.6.6.6",
+			remoteAddr:  net.ParseIP("10.0.0.5"),
+			trusted:     mustNetSet(t, "10.0.0.5/32"),
+			expectedIP:  net.ParseIP("6.6.6.6"),
+		},
+		// Direct peer isn't a trusted proxy at all: the header must be ignored entirely and
+		// the real connecting peer used, since nothing in the header can be believed.
+		{
+			name:        "untrusted direct peer ignores header",
+			headerValue: "9.9.9.9",
+			remoteAddr:  net.ParseIP("6.6.6.6"),
+			trusted:     mustNetSet(t, "10.0.0.5/32"),
+			expectedIP:  net.ParseIP("6.6.6.6"),
+		},
+		// A chain of two trusted proxies (client -> ProxyA(10.0.0.6) -> ProxyB(10.0.0.5) ->
+		// us) still resolves to the real (untrusted) client hop, skipping past both trusted
+		// proxy-appended hops and the attacker's forged leftmost claim.
+		{
+			name:        "chain of trusted proxies",
+			headerValue: "9.9.9.9, 6.6.6.6, 10.0.0.6",
+			remoteAddr:  net.ParseIP("10.0.0.5"),
+			trusted:     mustNetSet(t, "10.0.0.5/32", "10.0.0.6/32"),
+			expectedIP:  net.ParseIP("6.6.6.6"),
+		},
+		// Degenerate case: every hop (and the direct peer) is itself a trusted proxy; there's
+		// no untrusted hop to find, so fall back to the oldest (leftmost) entry.
+		{
+			name:        "all hops trusted falls back to leftmost",
+			headerValue: "10.0.0.7, 10.0.0.6",
+			remoteAddr:  net.ParseIP("10.0.0.5"),
+			trusted:     mustNetSet(t, "10.0.0.5/32", "10.0.0.6/32", "10.0.0.7/32"),
+			expectedIP:  net.ParseIP("10.0.0.7"),
+		},
 	}
 
 	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
 			h := http.Header{}
 			h.Add("X-Forwarded-For", test.headerValue)
 
-		ip, err := p.GetRealClientIP(h)
+			var trusted ipapi.TrustedProxies
+			if test.trusted != nil {
+				trusted = test.trusted
+			}
+
+			ip, err := p.GetRealClientIP(h, test.remoteAddr, trusted)
 
 			if test.errString == "" {
 				assert.Nil(t, err)
@@ -89,6 +140,7 @@ func TestXForwardedForClientIPParser(t *testing.T) {
 				assert.NotNil(t, ip)
 				assert.Equal(t, test.expectedIP, ip)
 			}
+		})
 	}
 }
 
@@ -100,12 +152,22 @@ func TestXForwardedForClientIPParserIgnoresOthers(t *testing.T) {
 	h.Add("X-Real-IP", "10.0.0.1")
 	h.Add("X-ProxyUser-IP", "10.0.0.1")
 	h.Add("X-Forwarded-For", expectedIPString)
-	ip, err := p.GetRealClientIP(h)
+	ip, err := p.GetRealClientIP(h, nil, nil)
 	assert.Nil(t, err)
 	assert.NotNil(t, ip)
 	assert.Equal(t, ip, net.ParseIP(expectedIPString))
 }
 
+// mustNetSet builds a *NetSet from CIDR/IP strings, failing the test on error.
+func mustNetSet(t *testing.T, ipStrs ...string) *NetSet {
+	t.Helper()
+	set, err := ParseNetSet(ipStrs)
+	if err != nil {
+		t.Fatalf("failed to build NetSet: %v", err)
+	}
+	return set
+}
+
 func TestGetRemoteIP(t *testing.T) {
 	tests := []struct {
 		remoteAddr string
@@ -174,10 +236,10 @@ func TestGetClientString(t *testing.T) {
 			RemoteAddr: test.remoteAddr,
 		}
 
-		client := GetClientString(test.parser, req, false)
+		client := GetClientString(test.parser, req, false, nil)
 		assert.Equal(t, test.expectedClient, client)
 
-		clientFull := GetClientString(test.parser, req, true)
+		clientFull := GetClientString(test.parser, req, true, nil)
 		assert.Equal(t, test.expectedClientFull, clientFull)
 	}
 }

pkg/validation/options.go

@@ -8,6 +8,8 @@ import (
 	"net/url"
 	"strings"
 
+	ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip"
+	middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/hmacauth"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
@@ -89,7 +91,11 @@ func Validate(o *options.Options) error {
 
 		// Allow the logger to get client IPs
 		logger.SetGetClientFunc(func(r *http.Request) string {
-			return ip.GetClientString(o.GetRealClientIPParser(), r, false)
+			var trusted ipapi.TrustedProxies
+			if scope := middlewareapi.GetRequestScope(r); scope != nil {
+				trusted = scope.TrustedProxies
+			}
+			return ip.GetClientString(o.GetRealClientIPParser(), r, false, trusted)
 		})
 	}