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>
This commit is contained in:
Georgi Georgiev 2026-07-21 03:13:55 +09:00
parent 10b68716e5
commit 08ce94d1c0
8 changed files with 193 additions and 48 deletions

View File

@ -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

View File

@ -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 |

View File

@ -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

View File

@ -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)

View File

@ -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)
}

View File

@ -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
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()
}

View File

@ -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)
}
}

View File

@ -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)
})
}