From 08ce94d1c00d6d064ee3054a01e646ca5492c16d Mon Sep 17 00:00:00 2001 From: Georgi Georgiev <310867+chutzimir@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:13:55 +0900 Subject: [PATCH] security: harden X-Forwarded-For parsing against spoofing The real-client-IP lookup used by the --trusted-ip auth-bypass allowlist blindly trusted the leftmost value in X-Forwarded-For (or other configured --real-client-ip-header). When trusting X-Forwarded-For (which is the only header set by AWS ALB for example) the connecting peer's address is appended to X-Forwarded-For rather than replacing the header, and a client could set X-Forwarded-For to an allowlisted IP and have it trusted regardless of who actually connected to the proxy. The parser now walks the hop chain from the newest (rightmost) entry inward, skipping hops that are themselves trusted proxies, and returns the first entry that isn't. If the direct connecting peer isn't itself a trusted proxy, the header is ignored entirely. This reuses the existing --trusted-proxy-ip configuration, so behavior for deployments that leave it unset (today's documented trust-all default) is unchanged. Signed-off-by: Georgi Georgiev <310867+chutzimir@users.noreply.github.com> --- CHANGELOG.md | 2 + docs/docs/configuration/overview.md | 2 + oauthproxy.go | 13 +++- oauthproxy_test.go | 21 +++++ pkg/apis/ip/interfaces.go | 11 ++- pkg/ip/realclientip.go | 68 ++++++++++++---- pkg/ip/realclientip_test.go | 116 +++++++++++++++++++++------- pkg/validation/options.go | 8 +- 8 files changed, 193 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 788e82c2..199e7355 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 965953fa..9673d354 100644 --- a/docs/docs/configuration/overview.md +++ b/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 | diff --git a/oauthproxy.go b/oauthproxy.go index f8dc5471..7c0f8b99 100644 --- a/oauthproxy.go +++ b/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 diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b3271e5b..9b2e45ac 100644 --- a/oauthproxy_test.go +++ b/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) diff --git a/pkg/apis/ip/interfaces.go b/pkg/apis/ip/interfaces.go index 02f3937f..7409b142 100644 --- a/pkg/apis/ip/interfaces.go +++ b/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) } diff --git a/pkg/ip/realclientip.go b/pkg/ip/realclientip.go index db8f2595..3e9470d8 100644 --- a/pkg/ip/realclientip.go +++ b/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 `` portion specified in the above document. // Additionally, is capable of parsing IPs with the port included, for v4 in the format ":" and for v6 in the // format "[]:". With-port and without-port formats are seamlessly supported concurrently. -func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, error) { - var ipStr string - if realIP := h.Get(p.header); realIP != "" { - ipStr = realIP - } else { +// +// Each successive proxy may append itself, comma separated, to the end of the header. Blindly trusting the leftmost +// (client-supplied) entry lets an untrusted client spoof it, so instead this walks the chain from the right (the +// most recently appended hop) and skips over entries that are themselves trusted proxies, returning the first entry +// 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. - // Select only the first IP listed, as it is the client IP recorded by the first proxy. - if commaIndex := strings.IndexRune(ipStr, ','); commaIndex != -1 { - ipStr = ipStr[:commaIndex] + isTrusted := func(candidate net.IP) bool { + return trusted == nil || candidate == nil || trusted.Has(candidate) } - 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() } diff --git a/pkg/ip/realclientip_test.go b/pkg/ip/realclientip_test.go index 3cbca114..88fa42e7 100644 --- a/pkg/ip/realclientip_test.go +++ b/pkg/ip/realclientip_test.go @@ -55,40 +55,92 @@ 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}, - {"1.2.3.4", "", net.ParseIP("1.2.3.4")}, - {"10::23", "", net.ParseIP("10::23")}, - {"::1", "", net.ParseIP("::1")}, - {"[::1]:1234", "", net.ParseIP("::1")}, - {"10.0.10.11:1234", "", net.ParseIP("10.0.10.11")}, - {"192.168.10.50, 10.0.0.1, 1.2.3.4", "", net.ParseIP("192.168.10.50")}, - {"nil", "unable to parse ip (nil) from X-Forwarded-For header", nil}, - {"10000.10000.10000.10000", "unable to parse ip (10000.10000.10000.10000) from X-Forwarded-For header", nil}, + // No trusted-proxy restriction configured (nil): behaves exactly like the historical + // leftmost-picking parser, regardless of remoteAddr. + {name: "empty header", headerValue: "", expectedIP: nil}, + {name: "single hop", headerValue: "1.2.3.4", expectedIP: net.ParseIP("1.2.3.4")}, + {name: "single hop v6", headerValue: "10::23", expectedIP: net.ParseIP("10::23")}, + {name: "loopback v6", headerValue: "::1", expectedIP: net.ParseIP("::1")}, + {name: "v6 with port", headerValue: "[::1]:1234", expectedIP: net.ParseIP("::1")}, + {name: "v4 with port", headerValue: "10.0.10.11:1234", expectedIP: net.ParseIP("10.0.10.11")}, + {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 { - h := http.Header{} - h.Add("X-Forwarded-For", test.headerValue) + 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 + } - if test.errString == "" { - assert.Nil(t, err) - } else { - assert.NotNil(t, err) - assert.Equal(t, test.errString, err.Error()) - } + ip, err := p.GetRealClientIP(h, test.remoteAddr, trusted) - if test.expectedIP == nil { - assert.Nil(t, ip) - } else { - assert.NotNil(t, ip) - assert.Equal(t, test.expectedIP, ip) - } + if test.errString == "" { + assert.Nil(t, err) + } else { + assert.NotNil(t, err) + assert.Equal(t, test.errString, err.Error()) + } + + if test.expectedIP == nil { + assert.Nil(t, ip) + } else { + 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) } } diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 13ce2e0b..aa2da1cf 100644 --- a/pkg/validation/options.go +++ b/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) }) }