diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e1dc347..8be9736e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - [#3333](https://github.com/oauth2-proxy/oauth2-proxy/pull/3333) fix: invalidate session on fatal OAuth2 refresh errors (@frhack) - [GHSA-f24x-5g9q-753f](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-f24x-5g9q-753f) fix: clear session cookie at beginning of signinpage handler (@fnoehWM / @bella-WI / @tuunit) - [GHSA-5hvv-m4w4-gf6v](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-5hvv-m4w4-gf6v) fix: health check user-agent authentication bypass (@tuunit) +- [GHSA-7x63-xv5r-3p2x](https://github.com/oauth2-proxy/oauth2-proxy/security/advisories/GHSA-7x63-xv5r-3p2x) fix: authentication bypass via X-Forwarded-Uri header spoofing (@tuunit) # V7.15.1 diff --git a/contrib/local-environment/docker-compose-nginx.yaml b/contrib/local-environment/docker-compose-nginx.yaml index ed93d57c..23138eb4 100644 --- a/contrib/local-environment/docker-compose-nginx.yaml +++ b/contrib/local-environment/docker-compose-nginx.yaml @@ -23,7 +23,8 @@ version: "3.0" services: oauth2-proxy: image: quay.io/oauth2-proxy/oauth2-proxy:v7.15.1 - ports: [] + ports: + - 4180:4180/tcp hostname: oauth2-proxy container_name: oauth2-proxy command: --config /oauth2-proxy.cfg @@ -44,7 +45,7 @@ services: image: nginx:1.29 restart: unless-stopped ports: - - 80:80/tcp + - 8080:8080/tcp hostname: nginx volumes: - "./nginx.conf:/etc/nginx/conf.d/default.conf" diff --git a/contrib/local-environment/nginx.conf b/contrib/local-environment/nginx.conf index 15005bf6..f3761387 100644 --- a/contrib/local-environment/nginx.conf +++ b/contrib/local-environment/nginx.conf @@ -1,11 +1,12 @@ # Reverse proxy to oauth2-proxy server { - listen 80; - server_name oauth2-proxy.oauth2-proxy.localhost; + listen 8080; + server_name oauth2-proxy.localtest.me; location / { proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-Uri $request_uri; proxy_pass http://oauth2-proxy:4180/; } @@ -13,8 +14,8 @@ server { # Reverse proxy to httpbin server { - listen 80; - server_name httpbin.oauth2-proxy.localhost; + listen 8080; + server_name httpbin.localtest.me; auth_request /internal-auth/oauth2/auth; @@ -29,50 +30,7 @@ server { # Named location for OAuth2 sign-in redirect # Returns a proper 302 that works with --skip-provider-button location @oauth2_signin { - return 302 http://oauth2-proxy.oauth2-proxy.localhost/oauth2/sign_in?rd=$scheme://$host$request_uri; - } - - # auth_request must be a URI so this allows an internal path to then proxy to - # the real auth_request path. - # The trailing /'s are required so that nginx strips the prefix before proxying. - location /internal-auth/ { - internal; # Ensure external users can't access this path - - # Make sure the OAuth2 Proxy knows where the original request came from. - proxy_set_header Host $host; - proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-Uri $request_uri; - - proxy_pass http://oauth2-proxy:4180/; - } -} - -# Statically serve the nginx welcome -server { - listen 80; - server_name oauth2-proxy.localhost; - - location / { - auth_request /internal-auth/oauth2/auth; - - # On 401, redirect to the sign_in page via a named location - # This ensures a proper 302 redirect that browsers will follow - error_page 401 = @oauth2_signin; - - root /usr/share/nginx/html; - index index.html index.htm; - } - - # Named location for OAuth2 sign-in redirect - # Returns a proper 302 that works with --skip-provider-button - location @oauth2_signin { - return 302 http://oauth2-proxy.oauth2-proxy.localhost/oauth2/sign_in?rd=$scheme://$host$request_uri; - } - - # redirect server error pages to the static page /50x.html - error_page 500 502 503 504 /50x.html; - location = /50x.html { - root /usr/share/nginx/html; + return 302 http://oauth2-proxy.localtest.me:8080/oauth2/sign_in?rd=$scheme://$host$request_uri; } # auth_request must be a URI so this allows an internal path to then proxy to diff --git a/contrib/local-environment/oauth2-proxy-nginx.cfg b/contrib/local-environment/oauth2-proxy-nginx.cfg index 01b64a55..0a383ab7 100644 --- a/contrib/local-environment/oauth2-proxy-nginx.cfg +++ b/contrib/local-environment/oauth2-proxy-nginx.cfg @@ -1,14 +1,19 @@ http_address="0.0.0.0:4180" cookie_secret="OQINaROshtE9TcZkNAm-5Zs2Pv3xaWytBmc5W7sPX7w=" -provider="oidc" email_domains="example.com" -oidc_issuer_url="http://dex.localtest.me:5556/dex" +cookie_secure="false" +upstreams="static://200" +cookie_domains=[".localtest.me"] # Required so cookie can be read on all subdomains. +whitelist_domains=[".localtest.me"] # Required to allow redirection back to original requested target. + +# dex provider client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" client_id="oauth2-proxy" -cookie_secure="false" +redirect_url="http://oauth2-proxy.localtest.me:4180/oauth2/callback" + +oidc_issuer_url="http://dex.localtest.me:5556/dex" +provider="oidc" +provider_display_name="Dex" -redirect_url="http://oauth2-proxy.oauth2-proxy.localhost/oauth2/callback" -cookie_domains=".oauth2-proxy.localhost" # Required so cookie can be read on all subdomains. -whitelist_domains=".oauth2-proxy.localhost" # Required to allow redirection back to original requested target. # Enables the use of `X-Forwarded-*` headers to determine request correctly reverse_proxy="true" diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index a73e3acd..b145065a 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -193,6 +193,10 @@ Provider specific options can be found on their respective subpages. ### Proxy Options +:::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. +::: + | Flag / Config Field | Type | Description | Default | | ----------------------------------------------------------------------------- | -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | | flag: `--allow-query-semicolons`
toml: `allow_query_semicolons` | bool | allow the use of semicolons in query args ([required for some legacy applications](https://github.com/golang/go/issues/25192)) | `false` | @@ -211,6 +215,7 @@ Provider specific options can be found on their respective subpages. | flag: `--redirect-url`
toml: `redirect_url` | string | the OAuth Redirect URL, e.g. `"https://internalapp.yourcompany.com/oauth2/callback"` | | | flag: `--relative-redirect-url`
toml: `relative_redirect_url` | bool | allow relative OAuth Redirect URL.` | false | | flag: `--reverse-proxy`
toml: `reverse_proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-IP are accepted and allows X-Forwarded-\{Proto,Host,Uri\} headers to be used on redirect selection | false | +| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. Configure this to your reverse proxy addresses to prevent forwarded header spoofing. | `"0.0.0.0/0", "::/0"` | | flag: `--signature-key`
toml: `signature_key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | flag: `--skip-auth-preflight`
toml: `skip_auth_preflight` | bool | will skip authentication for OPTIONS requests | false | | flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | | diff --git a/docs/versioned_docs/version-7.15.x/configuration/overview.md b/docs/versioned_docs/version-7.15.x/configuration/overview.md index a73e3acd..b145065a 100644 --- a/docs/versioned_docs/version-7.15.x/configuration/overview.md +++ b/docs/versioned_docs/version-7.15.x/configuration/overview.md @@ -193,6 +193,10 @@ Provider specific options can be found on their respective subpages. ### Proxy Options +:::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. +::: + | Flag / Config Field | Type | Description | Default | | ----------------------------------------------------------------------------- | -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | | flag: `--allow-query-semicolons`
toml: `allow_query_semicolons` | bool | allow the use of semicolons in query args ([required for some legacy applications](https://github.com/golang/go/issues/25192)) | `false` | @@ -211,6 +215,7 @@ Provider specific options can be found on their respective subpages. | flag: `--redirect-url`
toml: `redirect_url` | string | the OAuth Redirect URL, e.g. `"https://internalapp.yourcompany.com/oauth2/callback"` | | | flag: `--relative-redirect-url`
toml: `relative_redirect_url` | bool | allow relative OAuth Redirect URL.` | false | | flag: `--reverse-proxy`
toml: `reverse_proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-IP are accepted and allows X-Forwarded-\{Proto,Host,Uri\} headers to be used on redirect selection | false | +| flag: `--trusted-proxy-ip`
toml: `trusted_proxy_ips` | string \| list | list of IPs or CIDR ranges allowed to supply `X-Forwarded-*` headers when `--reverse-proxy` is enabled. If not set, OAuth2 Proxy preserves backwards compatibility by trusting all source IPs (`0.0.0.0/0`, `::/0`) and logs a warning at startup. Configure this to your reverse proxy addresses to prevent forwarded header spoofing. | `"0.0.0.0/0", "::/0"` | | flag: `--signature-key`
toml: `signature_key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | flag: `--skip-auth-preflight`
toml: `skip_auth_preflight` | bool | will skip authentication for OPTIONS requests | false | | flag: `--skip-auth-regex`
toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | | diff --git a/oauthproxy.go b/oauthproxy.go index 0685bbbb..e2357c8d 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -59,6 +59,8 @@ const ( ) var ( + defaultTrustedProxyIPs = []string{"0.0.0.0/0", "::/0"} + // ErrNeedsLogin means the user should be redirected to the login page ErrNeedsLogin = errors.New("redirect to login page") @@ -183,13 +185,14 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr logger.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domains:%s path:%s samesite:%s refresh:%s", opts.Cookie.Name, opts.Cookie.Secure, opts.Cookie.HTTPOnly, opts.Cookie.Expire, strings.Join(opts.Cookie.Domains, ","), opts.Cookie.Path, opts.Cookie.SameSite, refresh) - trustedIPs := ip.NewNetSet() - for _, ipStr := range opts.TrustedIPs { - if ipNet := ip.ParseIPNet(ipStr); ipNet != nil { - trustedIPs.AddIPNet(*ipNet) - } else { - return nil, fmt.Errorf("could not parse IP network (%s)", ipStr) - } + trustedIPs, err := ip.ParseNetSet(opts.TrustedIPs) + if err != nil { + return nil, err + } + + trustedProxies, err := buildTrustedProxyNetSet(opts) + if err != nil { + return nil, err } allowedRoutes, err := buildRoutesAllowlist(opts) @@ -202,7 +205,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr return nil, err } - preAuthChain, err := buildPreAuthChain(opts, sessionStore) + preAuthChain, err := buildPreAuthChain(opts, sessionStore, trustedProxies) if err != nil { return nil, fmt.Errorf("could not build pre-auth chain: %v", err) } @@ -355,8 +358,8 @@ func (p *OAuthProxy) buildProxySubrouter(s *mux.Router) { // buildPreAuthChain constructs a chain that should process every request before // the OAuth2 Proxy authentication logic kicks in. // For example forcing HTTPS or health checks. -func buildPreAuthChain(opts *options.Options, sessionStore sessionsapi.SessionStore) (alice.Chain, error) { - chain := alice.New(middleware.NewScope(opts.ReverseProxy, opts.Logging.RequestIDHeader)) +func buildPreAuthChain(opts *options.Options, sessionStore sessionsapi.SessionStore, trustedProxies *ip.NetSet) (alice.Chain, error) { + chain := alice.New(middleware.NewScope(opts.ReverseProxy, opts.Logging.RequestIDHeader, trustedProxies)) if opts.ForceHTTPS { _, httpsPort, err := net.SplitHostPort(opts.Server.SecureBindAddress) @@ -395,6 +398,16 @@ func buildPreAuthChain(opts *options.Options, sessionStore sessionsapi.SessionSt return chain, nil } +func buildTrustedProxyNetSet(opts *options.Options) (*ip.NetSet, error) { + trustedProxyIPs := opts.TrustedProxyIPs + if opts.ReverseProxy && len(trustedProxyIPs) == 0 { + logger.Print("WARNING: --reverse-proxy is enabled but no --trusted-proxy-ip CIDRs were configured. All connecting IPs are trusted to supply X-Forwarded-* headers by default (0.0.0.0/0, ::/0). This preserves backwards compatibility but is a potential security risk; configure --trusted-proxy-ip to match your reverse proxy addresses.") + trustedProxyIPs = defaultTrustedProxyIPs + } + + return ip.ParseNetSet(trustedProxyIPs) +} + func buildSessionChain(opts *options.Options, provider providers.Provider, sessionStore sessionsapi.SessionStore, validator basic.Validator) alice.Chain { chain := alice.New() diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 46e39a90..df5912a0 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -2843,6 +2843,7 @@ func TestAllowedRequestWithForwardedUriHeader(t *testing.T) { t.Run(tc.name, func(t *testing.T) { req, err := http.NewRequest(tc.method, opts.ProxyPrefix+authOnlyPath, nil) req.Header.Set("X-Forwarded-Uri", tc.url) + req.RemoteAddr = "127.0.0.1:4180" assert.NoError(t, err) rw := httptest.NewRecorder() @@ -2857,6 +2858,43 @@ func TestAllowedRequestWithForwardedUriHeader(t *testing.T) { } } +func TestAllowedRequestWithForwardedUriHeaderRequiresTrustedProxy(t *testing.T) { + upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + t.Cleanup(upstreamServer.Close) + + opts := baseTestOptions() + opts.ReverseProxy = true + opts.TrustedProxyIPs = []string{"127.0.0.1/32"} + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, + }, + } + opts.SkipAuthRegex = []string{"^/skip/auth/regex$"} + + err := validation.Validate(opts) + assert.NoError(t, err) + + proxy, err := NewOAuthProxy(opts, func(_ string) bool { return true }) + assert.NoError(t, err) + + req, err := http.NewRequest(http.MethodGet, opts.ProxyPrefix+authOnlyPath, nil) + assert.NoError(t, err) + req.RemoteAddr = "192.0.2.10:4180" + req.Header.Set("X-Forwarded-Uri", "/skip/auth/regex") + + rw := httptest.NewRecorder() + proxy.ServeHTTP(rw, req) + + assert.Equal(t, 401, rw.Code) +} + func TestAllowedRequestNegateWithoutMethod(t *testing.T) { upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) diff --git a/pkg/apis/middleware/scope.go b/pkg/apis/middleware/scope.go index 2d84f00e..b778bd54 100644 --- a/pkg/apis/middleware/scope.go +++ b/pkg/apis/middleware/scope.go @@ -2,9 +2,12 @@ package middleware import ( "context" + "net" "net/http" + "strings" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" ) type scopeKey string @@ -18,9 +21,13 @@ const RequestScopeKey scopeKey = "request-scope" // within the chain. type RequestScope struct { // ReverseProxy tracks whether OAuth2-Proxy is operating in reverse proxy - // mode and if request `X-Forwarded-*` headers should be trusted + // mode and if request `X-Forwarded-*` headers may be trusted ReverseProxy bool + // TrustedProxies tracks which direct callers are allowed to supply + // forwarded headers when ReverseProxy mode is enabled. + TrustedProxies *ip.NetSet + // RequestID is set to the request's `X-Request-Id` header if set. // Otherwise a random UUID is set. RequestID string @@ -58,3 +65,43 @@ func AddRequestScope(req *http.Request, scope *RequestScope) *http.Request { ctx := context.WithValue(req.Context(), RequestScopeKey, scope) return req.WithContext(ctx) } + +// CanTrustForwardedHeaders returns whether forwarded headers should be +// processed for this request. +func (s *RequestScope) CanTrustForwardedHeaders(req *http.Request) bool { + if s == nil || req == nil || !s.ReverseProxy || s.TrustedProxies == nil { + return false + } + + if isUnixSocketRemoteAddr(req.RemoteAddr) { + return true + } + + remoteIP := parseRemoteAddrIP(req.RemoteAddr) + if remoteIP == nil { + return false + } + + return s.TrustedProxies.Has(remoteIP) +} + +func parseRemoteAddrIP(remoteAddr string) net.IP { + if remoteAddr == "" { + return nil + } + + if ip := net.ParseIP(remoteAddr); ip != nil { + return ip + } + + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + return nil + } + + return net.ParseIP(host) +} + +func isUnixSocketRemoteAddr(remoteAddr string) bool { + return remoteAddr == "@" || strings.HasPrefix(remoteAddr, "/") +} diff --git a/pkg/apis/middleware/scope_test.go b/pkg/apis/middleware/scope_test.go index f1845518..ec485b47 100644 --- a/pkg/apis/middleware/scope_test.go +++ b/pkg/apis/middleware/scope_test.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -53,4 +54,37 @@ var _ = Describe("Scope Suite", func() { }) }) }) + + Context("CanTrustForwardedHeaders", func() { + var request *http.Request + var scope *middleware.RequestScope + + BeforeEach(func() { + var err error + request, err = http.NewRequest("", "http://127.0.0.1/", nil) + Expect(err).ToNot(HaveOccurred()) + + trustedProxies, err := ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + scope = &middleware.RequestScope{ + ReverseProxy: true, + TrustedProxies: trustedProxies, + } + }) + + It("returns true for a trusted remote address", func() { + request.RemoteAddr = "127.0.0.1:4180" + Expect(scope.CanTrustForwardedHeaders(request)).To(BeTrue()) + }) + + It("returns false for an untrusted remote address", func() { + request.RemoteAddr = "192.0.2.10:4180" + Expect(scope.CanTrustForwardedHeaders(request)).To(BeFalse()) + }) + + It("returns true for unix socket callers", func() { + request.RemoteAddr = "@" + Expect(scope.CanTrustForwardedHeaders(request)).To(BeTrue()) + }) + }) }) diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index b57d5aed..ac2b13c8 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -24,6 +24,7 @@ type Options struct { ReadyPath string `flag:"ready-path" cfg:"ready_path"` ReverseProxy bool `flag:"reverse-proxy" cfg:"reverse_proxy"` RealClientIPHeader string `flag:"real-client-ip-header" cfg:"real_client_ip_header"` + TrustedProxyIPs []string `flag:"trusted-proxy-ip" cfg:"trusted_proxy_ips"` TrustedIPs []string `flag:"trusted-ip" cfg:"trusted_ips"` ForceHTTPS bool `flag:"force-https" cfg:"force_https"` RawRedirectURL string `flag:"redirect-url" cfg:"redirect_url"` @@ -119,6 +120,7 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Bool("reverse-proxy", false, "are we running behind a reverse proxy, controls whether headers like X-Real-Ip are accepted") flagSet.String("real-client-ip-header", "X-Real-IP", "Header used to determine the real IP of the client (one of: X-Forwarded-For, X-Real-IP, X-ProxyUser-IP, X-Envoy-External-Address, or CF-Connecting-IP)") + flagSet.StringSlice("trusted-proxy-ip", []string{}, "list of IPs or CIDR ranges that are allowed to set X-Forwarded-* headers when --reverse-proxy is enabled. Defaults to trusting all IPs for backwards compatibility; configure this to your reverse proxy addresses to prevent header spoofing.") flagSet.StringSlice("trusted-ip", []string{}, "list of IPs or CIDR ranges to allow to bypass authentication. WARNING: trusting by IP has inherent security flaws, read the configuration documentation for more information.") flagSet.Bool("force-https", false, "force HTTPS redirect for HTTP requests") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"") diff --git a/pkg/app/redirect/director_test.go b/pkg/app/redirect/director_test.go index 69c01d95..58d0e8c3 100644 --- a/pkg/app/redirect/director_test.go +++ b/pkg/app/redirect/director_test.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -33,9 +34,16 @@ var _ = Describe("Director Suite", func() { req.Header.Add(header, value) } } - req = middleware.AddRequestScope(req, &middleware.RequestScope{ + scope := &middleware.RequestScope{ ReverseProxy: in.reverseProxy, - }) + } + if in.reverseProxy { + req.RemoteAddr = "127.0.0.1:4180" + trustedProxies, err := ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + scope.TrustedProxies = trustedProxies + } + req = middleware.AddRequestScope(req, scope) redirect, err := appDirector.GetRedirect(req) Expect(err).ToNot(HaveOccurred()) @@ -174,4 +182,27 @@ var _ = Describe("Director Suite", func() { expectedRedirect: "https://a-service.example.com/foo/bar", }), ) + + It("ignores forwarded headers from an untrusted remote address", func() { + appDirector := NewAppDirector(AppDirectorOpts{ + ProxyPrefix: testProxyPrefix, + Validator: testValidator(true), + }) + + req, _ := http.NewRequest("GET", "https://oauth.example.com/foo?bar", nil) + req.RemoteAddr = "192.0.2.10:4180" + req.Header.Add("X-Forwarded-Proto", "https") + req.Header.Add("X-Forwarded-Host", "a-service.example.com") + req.Header.Add("X-Forwarded-Uri", fooBar) + trustedProxies, err := ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, + TrustedProxies: trustedProxies, + }) + + redirect, err := appDirector.GetRedirect(req) + Expect(err).ToNot(HaveOccurred()) + Expect(redirect).To(Equal("/foo?bar")) + }) }) diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go index b67f8a69..ba8b5480 100644 --- a/pkg/cookies/cookies_test.go +++ b/pkg/cookies/cookies_test.go @@ -6,6 +6,7 @@ import ( "time" middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -30,8 +31,13 @@ var _ = Describe("Cookie Tests", func() { if in.xForwardedHost != "" { req.Header.Add("X-Forwarded-Host", in.xForwardedHost) + req.RemoteAddr = "127.0.0.1:4180" + trustedProxies, err := ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{ - ReverseProxy: true, + ReverseProxy: true, + TrustedProxies: trustedProxies, }) } diff --git a/pkg/ip/parse_ip_net.go b/pkg/ip/parse_ip_net.go index 9cb37de2..4fc63664 100644 --- a/pkg/ip/parse_ip_net.go +++ b/pkg/ip/parse_ip_net.go @@ -1,6 +1,7 @@ package ip import ( + "fmt" "net" "strings" ) @@ -37,3 +38,18 @@ func ParseIPNet(s string) *net.IPNet { return ipNet } } + +func ParseNetSet(ipStrs []string) (*NetSet, error) { + netSet := NewNetSet() + + for _, ipStr := range ipStrs { + ipNet := ParseIPNet(ipStr) + if ipNet == nil { + return nil, fmt.Errorf("could not parse IP network (%s)", ipStr) + } + + netSet.AddIPNet(*ipNet) + } + + return netSet, nil +} diff --git a/pkg/ip/parse_ip_net_test.go b/pkg/ip/parse_ip_net_test.go new file mode 100644 index 00000000..41c1b4b9 --- /dev/null +++ b/pkg/ip/parse_ip_net_test.go @@ -0,0 +1,118 @@ +package ip + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseIPNet(t *testing.T) { + tests := []struct { + name string + input string + expectedIP net.IP + expectedMask net.IPMask + }{ + { + name: "ipv4 address", + input: "127.0.0.1", + expectedIP: net.ParseIP("127.0.0.1"), + expectedMask: net.CIDRMask(32, 32), + }, + { + name: "ipv6 address", + input: "::1", + expectedIP: net.ParseIP("::1"), + expectedMask: net.CIDRMask(128, 128), + }, + { + name: "ipv4 cidr", + input: "10.0.0.0/24", + expectedIP: net.ParseIP("10.0.0.0"), + expectedMask: net.CIDRMask(24, 32), + }, + { + name: "ipv6 cidr", + input: "2001:db8::/64", + expectedIP: net.ParseIP("2001:db8::"), + expectedMask: net.CIDRMask(64, 128), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ipNet := ParseIPNet(test.input) + + assert.NotNil(t, ipNet) + if ipNet == nil { + return + } + + assert.True(t, test.expectedIP.Equal(ipNet.IP)) + assert.Equal(t, test.expectedMask, ipNet.Mask) + }) + } +} + +func TestParseIPNetRejectsInvalidNetworks(t *testing.T) { + tests := []struct { + name string + input string + }{ + { + name: "invalid ip", + input: "not-an-ip", + }, + { + name: "ipv4 cidr with host bits set", + input: "10.0.0.1/24", + }, + { + name: "ipv6 cidr with host bits set", + input: "2001:db8::1/64", + }, + { + name: "invalid cidr mask", + input: "10.0.0.0/33", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Nil(t, ParseIPNet(test.input)) + }) + } +} + +func TestParseNetSet(t *testing.T) { + netSet, err := ParseNetSet([]string{ + "127.0.0.1", + "10.0.0.0/24", + "::1", + "2001:db8::/64", + }) + + assert.NoError(t, err) + assert.NotNil(t, netSet) + if netSet == nil { + return + } + + assert.True(t, netSet.Has(net.ParseIP("127.0.0.1"))) + assert.True(t, netSet.Has(net.ParseIP("10.0.0.55"))) + assert.True(t, netSet.Has(net.ParseIP("::1"))) + assert.True(t, netSet.Has(net.ParseIP("2001:db8::abcd"))) + + assert.False(t, netSet.Has(net.ParseIP("127.0.0.2"))) + assert.False(t, netSet.Has(net.ParseIP("10.0.1.1"))) + assert.False(t, netSet.Has(net.ParseIP("::2"))) + assert.False(t, netSet.Has(net.ParseIP("2001:db9::1"))) +} + +func TestParseNetSetReturnsErrorForInvalidNetwork(t *testing.T) { + netSet, err := ParseNetSet([]string{"127.0.0.1", "10.0.0.1/24"}) + + assert.Nil(t, netSet) + assert.EqualError(t, err, "could not parse IP network (10.0.0.1/24)") +} diff --git a/pkg/middleware/redirect_to_https_test.go b/pkg/middleware/redirect_to_https_test.go index 6e8a4368..9c7fb751 100644 --- a/pkg/middleware/redirect_to_https_test.go +++ b/pkg/middleware/redirect_to_https_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -39,6 +40,10 @@ var _ = Describe("RedirectToHTTPS suite", func() { scope := &middlewareapi.RequestScope{ ReverseProxy: in.reverseProxy, } + if in.reverseProxy { + req.RemoteAddr = "127.0.0.1:4180" + scope.TrustedProxies = newRedirectTrustedProxySet("127.0.0.1") + } req = middlewareapi.AddRequestScope(req, scope) rw := httptest.NewRecorder() @@ -207,3 +212,13 @@ var _ = Describe("RedirectToHTTPS suite", func() { }), ) }) + +func newRedirectTrustedProxySet(cidrs ...string) *ip.NetSet { + set := ip.NewNetSet() + for _, cidr := range cidrs { + ipNet := ip.ParseIPNet(cidr) + Expect(ipNet).ToNot(BeNil()) + set.AddIPNet(*ipNet) + } + return set +} diff --git a/pkg/middleware/scope.go b/pkg/middleware/scope.go index d0dd81ec..84b1573c 100644 --- a/pkg/middleware/scope.go +++ b/pkg/middleware/scope.go @@ -6,14 +6,16 @@ import ( "github.com/google/uuid" "github.com/justinas/alice" middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" ) -func NewScope(reverseProxy bool, idHeader string) alice.Constructor { +func NewScope(reverseProxy bool, idHeader string, trustedProxies *ip.NetSet) alice.Constructor { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { scope := &middlewareapi.RequestScope{ - ReverseProxy: reverseProxy, - RequestID: genRequestID(req, idHeader), + ReverseProxy: reverseProxy, + TrustedProxies: trustedProxies, + RequestID: genRequestID(req, idHeader), } req = middlewareapi.AddRequestScope(req, scope) next.ServeHTTP(rw, req) diff --git a/pkg/middleware/scope_test.go b/pkg/middleware/scope_test.go index fa680667..db2c3016 100644 --- a/pkg/middleware/scope_test.go +++ b/pkg/middleware/scope_test.go @@ -6,6 +6,7 @@ import ( "github.com/google/uuid" middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -32,7 +33,7 @@ var _ = Describe("Scope Suite", func() { Context("ReverseProxy is false", func() { BeforeEach(func() { - handler := NewScope(false, testRequestHeader)( + handler := NewScope(false, testRequestHeader, nil)( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { nextRequest = r w.WriteHeader(200) @@ -60,8 +61,15 @@ var _ = Describe("Scope Suite", func() { }) Context("ReverseProxy is true", func() { + var trustedProxies *ip.NetSet + BeforeEach(func() { - handler := NewScope(true, testRequestHeader)( + var err error + + trustedProxies, err = ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + + handler := NewScope(true, testRequestHeader, trustedProxies)( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { nextRequest = r w.WriteHeader(200) @@ -74,12 +82,18 @@ var _ = Describe("Scope Suite", func() { Expect(scope).ToNot(BeNil()) Expect(scope.ReverseProxy).To(BeTrue()) }) + + It("stores the trusted proxies on the scope", func() { + scope := middlewareapi.GetRequestScope(nextRequest) + Expect(scope).ToNot(BeNil()) + Expect(scope.TrustedProxies).To(BeIdenticalTo(trustedProxies)) + }) }) Context("Request ID header is present", func() { BeforeEach(func() { request.Header.Add(testRequestHeader, testRequestID) - handler := NewScope(false, testRequestHeader)( + handler := NewScope(false, testRequestHeader, nil)( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { nextRequest = r w.WriteHeader(200) @@ -97,7 +111,7 @@ var _ = Describe("Scope Suite", func() { BeforeEach(func() { uuid.SetRand(mockRand{}) - handler := NewScope(true, testRequestHeader)( + handler := NewScope(true, testRequestHeader, nil)( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { nextRequest = r w.WriteHeader(200) diff --git a/pkg/requests/util/util.go b/pkg/requests/util/util.go index 290f8059..7b9b4919 100644 --- a/pkg/requests/util/util.go +++ b/pkg/requests/util/util.go @@ -15,30 +15,30 @@ const ( ) // GetRequestProto returns the request scheme or X-Forwarded-Proto if present -// and the request is proxied. +// and the request came from a trusted reverse proxy. func GetRequestProto(req *http.Request) string { proto := req.Header.Get(XForwardedProto) - if !IsProxied(req) || proto == "" { + if !CanTrustForwardedHeaders(req) || proto == "" { proto = req.URL.Scheme } return proto } // GetRequestHost returns the request host header or X-Forwarded-Host if -// present and the request is proxied. +// present and the request came from a trusted reverse proxy. func GetRequestHost(req *http.Request) string { host := req.Header.Get(XForwardedHost) - if !IsProxied(req) || host == "" { + if !CanTrustForwardedHeaders(req) || host == "" { host = req.Host } return host } // GetRequestURI return the request URI or X-Forwarded-Uri if present and the -// request is proxied. +// request came from a trusted reverse proxy. func GetRequestURI(req *http.Request) string { uri := req.Header.Get(XForwardedURI) - if !IsProxied(req) || uri == "" { + if !CanTrustForwardedHeaders(req) || uri == "" { // Use RequestURI to preserve ?query uri = req.URL.RequestURI() } @@ -46,8 +46,8 @@ func GetRequestURI(req *http.Request) string { } // GetRequestPath returns the request URI or X-Forwarded-Uri if present and the -// request is proxied but always strips the query parameters and only returns -// the pure path +// request came from a trusted reverse proxy but always strips the query +// parameters and only returns the pure path. func GetRequestPath(req *http.Request) string { uri := GetRequestURI(req) @@ -64,17 +64,18 @@ func GetRequestPath(req *http.Request) string { return uri } -// IsProxied determines if a request was from a proxy based on the RequestScope -// ReverseProxy tracker. -func IsProxied(req *http.Request) bool { +// CanTrustForwardedHeaders determines if forwarded headers should be processed +// based on the RequestScope and the direct caller's address. +func CanTrustForwardedHeaders(req *http.Request) bool { scope := middlewareapi.GetRequestScope(req) if scope == nil { return false } - return scope.ReverseProxy + + return scope.CanTrustForwardedHeaders(req) } func IsForwardedRequest(req *http.Request) bool { - return IsProxied(req) && + return CanTrustForwardedHeaders(req) && req.Host != GetRequestHost(req) } diff --git a/pkg/requests/util/util_test.go b/pkg/requests/util/util_test.go index ba72c66d..ed7a88a6 100644 --- a/pkg/requests/util/util_test.go +++ b/pkg/requests/util/util_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -19,8 +20,13 @@ var _ = Describe("Util Suite", func() { uriNoQueryParams = "/test/endpoint" ) var req *http.Request + var trustedProxies *ip.NetSet BeforeEach(func() { + var err error + trustedProxies, err = ip.ParseNetSet([]string{"127.0.0.1"}) + Expect(err).ToNot(HaveOccurred()) + req = httptest.NewRequest( http.MethodGet, fmt.Sprintf("%s://%s%s", proto, host, uriWithQueryParams), @@ -29,7 +35,7 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestHost", func() { - Context("IsProxied is false", func() { + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) }) @@ -44,10 +50,12 @@ var _ = Describe("Util Suite", func() { }) }) - Context("IsProxied is true", func() { + Context("trusted forwarded headers are enabled", func() { BeforeEach(func() { + req.RemoteAddr = "127.0.0.1:4180" req = middleware.AddRequestScope(req, &middleware.RequestScope{ - ReverseProxy: true, + ReverseProxy: true, + TrustedProxies: trustedProxies, }) }) @@ -63,7 +71,7 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestProto", func() { - Context("IsProxied is false", func() { + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) }) @@ -78,10 +86,12 @@ var _ = Describe("Util Suite", func() { }) }) - Context("IsProxied is true", func() { + Context("trusted forwarded headers are enabled", func() { BeforeEach(func() { + req.RemoteAddr = "127.0.0.1:4180" req = middleware.AddRequestScope(req, &middleware.RequestScope{ - ReverseProxy: true, + ReverseProxy: true, + TrustedProxies: trustedProxies, }) }) @@ -97,7 +107,7 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestURI", func() { - Context("IsProxied is false", func() { + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) }) @@ -112,10 +122,12 @@ var _ = Describe("Util Suite", func() { }) }) - Context("IsProxied is true", func() { + Context("trusted forwarded headers are enabled", func() { BeforeEach(func() { + req.RemoteAddr = "127.0.0.1:4180" req = middleware.AddRequestScope(req, &middleware.RequestScope{ - ReverseProxy: true, + ReverseProxy: true, + TrustedProxies: trustedProxies, }) }) @@ -131,7 +143,7 @@ var _ = Describe("Util Suite", func() { }) Context("GetRequestPath", func() { - Context("IsProxied is false", func() { + Context("trusted forwarded headers are disabled", func() { BeforeEach(func() { req = middleware.AddRequestScope(req, &middleware.RequestScope{}) }) @@ -146,10 +158,12 @@ var _ = Describe("Util Suite", func() { }) }) - Context("IsProxied is true", func() { + Context("trusted forwarded headers are enabled", func() { BeforeEach(func() { + req.RemoteAddr = "127.0.0.1:4180" req = middleware.AddRequestScope(req, &middleware.RequestScope{ - ReverseProxy: true, + ReverseProxy: true, + TrustedProxies: trustedProxies, }) }) @@ -163,4 +177,30 @@ var _ = Describe("Util Suite", func() { }) }) }) + + Context("CanTrustForwardedHeaders", func() { + It("returns false when no scope is present", func() { + Expect(util.CanTrustForwardedHeaders(req)).To(BeFalse()) + }) + + It("returns true when the remote address is trusted", func() { + req.RemoteAddr = "127.0.0.1:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, + TrustedProxies: trustedProxies, + }) + + Expect(util.CanTrustForwardedHeaders(req)).To(BeTrue()) + }) + + It("returns false when the remote address is untrusted", func() { + req.RemoteAddr = "192.0.2.10:4180" + req = middleware.AddRequestScope(req, &middleware.RequestScope{ + ReverseProxy: true, + TrustedProxies: trustedProxies, + }) + + Expect(util.CanTrustForwardedHeaders(req)).To(BeFalse()) + }) + }) }) diff --git a/pkg/upstream/http_test.go b/pkg/upstream/http_test.go index a01d5c09..70af9e7e 100644 --- a/pkg/upstream/http_test.go +++ b/pkg/upstream/http_test.go @@ -498,7 +498,7 @@ var _ = Describe("HTTP Upstream Suite", func() { handler := newHTTPUpstreamProxy(upstream, u, nil, nil) - proxyServer = httptest.NewServer(middleware.NewScope(false, "X-Request-Id")(handler)) + proxyServer = httptest.NewServer(middleware.NewScope(false, "X-Request-Id", nil)(handler)) }) AfterEach(func() { @@ -549,7 +549,7 @@ var _ = Describe("HTTP Upstream Suite", func() { Expect(err).ToNot(HaveOccurred()) handler := newHTTPUpstreamProxy(upstream, u, nil, nil) - noPassHostServer := httptest.NewServer(middleware.NewScope(false, "X-Request-Id")(handler)) + noPassHostServer := httptest.NewServer(middleware.NewScope(false, "X-Request-Id", nil)(handler)) defer noPassHostServer.Close() origin := "http://example.localhost" diff --git a/pkg/validation/allowlist.go b/pkg/validation/allowlist.go index a74f4ae9..dcc0d361 100644 --- a/pkg/validation/allowlist.go +++ b/pkg/validation/allowlist.go @@ -16,6 +16,7 @@ func validateAllowlists(o *options.Options) []string { msgs = append(msgs, validateAuthRoutes(o)...) msgs = append(msgs, validateAuthRegexes(o)...) + msgs = append(msgs, validateTrustedProxyIPs(o)...) msgs = append(msgs, validateTrustedIPs(o)...) if len(o.TrustedIPs) > 0 && o.ReverseProxy { @@ -28,6 +29,17 @@ func validateAllowlists(o *options.Options) []string { return msgs } +// validateTrustedProxyIPs validates IP/CIDRs for trusted reverse proxies. +func validateTrustedProxyIPs(o *options.Options) []string { + msgs := []string{} + for i, ipStr := range o.TrustedProxyIPs { + if ip.ParseIPNet(ipStr) == nil { + msgs = append(msgs, fmt.Sprintf("trusted_proxy_ips[%d] (%s) could not be recognized", i, ipStr)) + } + } + return msgs +} + // validateAuthRoutes validates method=path routes passed with options.SkipAuthRoutes func validateAuthRoutes(o *options.Options) []string { msgs := []string{} diff --git a/pkg/validation/allowlist_test.go b/pkg/validation/allowlist_test.go index 9f6843dd..ae4c29ef 100644 --- a/pkg/validation/allowlist_test.go +++ b/pkg/validation/allowlist_test.go @@ -23,6 +23,11 @@ var _ = Describe("Allowlist", func() { errStrings []string } + type validateTrustedProxyIPsTableInput struct { + trustedProxyIPs []string + errStrings []string + } + DescribeTable("validateRoutes", func(r *validateRoutesTableInput) { opts := &options.Options{ @@ -121,4 +126,29 @@ var _ = Describe("Allowlist", func() { }, }), ) + + DescribeTable("validateTrustedProxyIPs", + func(t *validateTrustedProxyIPsTableInput) { + opts := &options.Options{ + TrustedProxyIPs: t.trustedProxyIPs, + } + Expect(validateTrustedProxyIPs(opts)).To(ConsistOf(t.errStrings)) + }, + Entry("Valid trusted proxy IPs", &validateTrustedProxyIPsTableInput{ + trustedProxyIPs: []string{ + "127.0.0.1", + "10.32.0.1/32", + "::1", + "2a12:105:ee7:9234:0:0:0:0/64", + }, + errStrings: []string{}, + }), + Entry("Invalid trusted proxy IPs", &validateTrustedProxyIPsTableInput{ + trustedProxyIPs: []string{"[::1]", "alkwlkbn/32"}, + errStrings: []string{ + "trusted_proxy_ips[0] ([::1]) could not be recognized", + "trusted_proxy_ips[1] (alkwlkbn/32) could not be recognized", + }, + }), + ) })