From 1d74a51cd70875362b09d77f3d5f9824d3d4d564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0lteri=C5=9F=20Ero=C4=9Flu?= Date: Sat, 2 Jan 2021 02:23:11 +0300 Subject: [PATCH] Use X-Forwarded-{Proto,Host,Uri} on redirect as last resort (#957) --- CHANGELOG.md | 2 + docs/docs/configuration/overview.md | 69 +++++++++++++++- oauthproxy.go | 29 ++++++- oauthproxy_test.go | 120 ++++++++++++++++++++++++++++ pkg/util/util.go | 19 +++++ pkg/util/util_test.go | 26 ++++++ 6 files changed, 263 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6af45d6a..2846d28e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) Keycloak will now use `--profile-url` if set for the userinfo endpoint instead of `--validate-url`. `--validate-url` will still work for backwards compatibility. +- [#957](https://github.com/oauth2-proxy/oauth2-proxy/pull/957) To use X-Forwarded-{Proto,Host,Uri} on redirect detection, `--reverse-proxy` must be `true`. - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim` - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled - [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) `/oauth2/auth` `allowed_groups` querystring parameter can be paired with the `allowed-groups` configuration option. @@ -59,6 +60,7 @@ ## Changes since v6.1.1 - [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) Migrate Keycloak to EnrichSession & support multiple groups for authorization (@NickMeves) +- [#957](https://github.com/oauth2-proxy/oauth2-proxy/pull/957) Use X-Forwarded-{Proto,Host,Uri} on redirect as last resort (@linuxgemini) - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Add support for Gitlab project based authentication (@factorysh) - [#907](https://github.com/oauth2-proxy/oauth2-proxy/pull/907) Introduce alpha configuration option to enable testing of structured configuration (@JoelSpeed) - [#938](https://github.com/oauth2-proxy/oauth2-proxy/pull/938) Cleanup missed provider renaming refactor methods (@NickMeves) diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index a91c6264..457842e9 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -106,7 +106,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--request-logging` | bool | Log requests | true | | `--request-logging-format` | string | Template for request log lines | see [Logging Configuration](#logging-configuration) | | `--resource` | string | The resource that is protected (Azure AD only) | | -| `--reverse-proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-IP are accepted | false | +| `--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 | | `--scope` | string | OAuth scope specification | | | `--session-cookie-minimal` | bool | strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only) | false | | `--session-store-type` | string | [Session data storage backend](sessions.md); redis or cookie | cookie | @@ -354,6 +354,73 @@ It is recommended to use `--session-store-type=redis` when expecting large sessi You have to substitute *name* with the actual cookie name you configured via --cookie-name parameter. If you don't set a custom cookie name the variable should be "$upstream_cookie__oauth2_proxy_1" instead of "$upstream_cookie_name_1" and the new cookie-name should be "_oauth2_proxy_1=" instead of "name_1=". +## Configuring for use with the Traefik (v2) `ForwardAuth` middleware + +**This option requires `--reverse-proxy` option to be set.** + +The [Traefik v2 `ForwardAuth` middleware](https://doc.traefik.io/traefik/middlewares/forwardauth/) allows Traefik to authenticate requests via the oauth2-proxy's `/oauth2/auth` endpoint on every request, which only returns a 202 Accepted response or a 401 Unauthorized response without proxying the whole request through. For example, on Dynamic File (YAML) Configuration: + +```yaml +http: + routers: + a-service: + rule: "Host(`a-service.example.com`)" + service: a-service-backend + middlewares: + - oauth-errors + - oauth-auth + tls: + certResolver: default + domains: + - main: "example.com" + sans: + - "*.example.com" + oauth: + rule: "Host(`a-service.example.com`, `oauth.example.com`) && PathPrefix(`/oauth2/`)" + middlewares: + - auth-headers + service: oauth-backend + tls: + certResolver: default + domains: + - main: "example.com" + sans: + - "*.example.com" + + services: + a-service-backend: + loadBalancer: + servers: + - url: http://172.16.0.2:7555 + oauth-backend: + loadBalancer: + servers: + - url: http://172.16.0.1:4180 + + middlewares: + auth-headers: + headers: + sslRedirect: true + stsSeconds: 315360000 + browserXssFilter: true + contentTypeNosniff: true + forceSTSHeader: true + sslHost: example.com + stsIncludeSubdomains: true + stsPreload: true + frameDeny: true + oauth-auth: + forwardAuth: + address: https://oauth.example.com/oauth2/auth + trustForwardHeader: true + oauth-errors: + errors: + status: + - "401-403" + service: oauth-backend + query: "/oauth2/sign_in" +``` + :::note If you set up your OAuth2 provider to rotate your client secret, you can use the `client-secret-file` option to reload the secret when it is updated. ::: diff --git a/oauthproxy.go b/oauthproxy.go index f97af98b..74ed6dc0 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -98,6 +98,7 @@ type OAuthProxy struct { SetAuthorization bool PassAuthorization bool PreferEmailToUser bool + ReverseProxy bool skipAuthPreflight bool skipJwtBearerTokens bool templates *template.Template @@ -200,6 +201,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr UserInfoPath: fmt.Sprintf("%s/userinfo", opts.ProxyPrefix), ProxyPrefix: opts.ProxyPrefix, + ReverseProxy: opts.ReverseProxy, provider: opts.GetProvider(), providerNameOverride: opts.ProviderName, sessionStore: sessionStore, @@ -578,10 +580,18 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error) if req.Form.Get("rd") != "" { redirect = req.Form.Get("rd") } + // Quirk: On reverse proxies that doesn't have support for + // "X-Auth-Request-Redirect" header or dynamic header/query string + // manipulation (like Traefik v1 and v2), we can try if the header + // X-Forwarded-Host exists or not. + if redirect == "" && isForwardedRequest(req, p.ReverseProxy) { + redirect = p.getRedirectFromForwardHeaders(req) + } if !p.IsValidRedirect(redirect) { // Use RequestURI to preserve ?query redirect = req.URL.RequestURI() - if strings.HasPrefix(redirect, p.ProxyPrefix) { + + if strings.HasPrefix(redirect, fmt.Sprintf("%s/", p.ProxyPrefix)) { redirect = "/" } } @@ -589,6 +599,17 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error) return } +// getRedirectFromForwardHeaders returns the redirect URL based on X-Forwarded-{Proto,Host,Uri} headers +func (p *OAuthProxy) getRedirectFromForwardHeaders(req *http.Request) string { + uri := util.GetRequestURI(req) + + if strings.HasPrefix(uri, fmt.Sprintf("%s/", p.ProxyPrefix)) { + uri = "/" + } + + return fmt.Sprintf("%s://%s%s", util.GetRequestProto(req), util.GetRequestHost(req), uri) +} + // splitHostPort separates host and port. If the port is not valid, it returns // the entire input as host, and it doesn't check the validity of the host. // Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric. @@ -686,6 +707,12 @@ func (p *OAuthProxy) isAllowedRoute(req *http.Request) bool { return false } +// isForwardedRequest is used to check if X-Forwarded-Host header exists or not +func isForwardedRequest(req *http.Request, reverseProxy bool) bool { + isForwarded := req.Host != util.GetRequestHost(req) + return isForwarded && reverseProxy +} + // See https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching?hl=en var noCacheHeaders = map[string]string{ "Expires": time.Unix(0, 0).Format(time.RFC1123), diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 56e046c2..572e1ec9 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -1750,6 +1750,8 @@ func TestRequestSignature(t *testing.T) { func TestGetRedirect(t *testing.T) { opts := baseTestOptions() + opts.WhitelistDomains = append(opts.WhitelistDomains, ".example.com") + opts.WhitelistDomains = append(opts.WhitelistDomains, ".example.com:8443") err := validation.Validate(opts) assert.NoError(t, err) require.NotEmpty(t, opts.ProxyPrefix) @@ -1761,27 +1763,145 @@ func TestGetRedirect(t *testing.T) { tests := []struct { name string url string + headers map[string]string + reverseProxy bool expectedRedirect string }{ { name: "request outside of ProxyPrefix redirects to original URL", url: "/foo/bar", + headers: nil, + reverseProxy: false, expectedRedirect: "/foo/bar", }, { name: "request with query preserves query", url: "/foo?bar", + headers: nil, + reverseProxy: false, expectedRedirect: "/foo?bar", }, { name: "request under ProxyPrefix redirects to root", url: proxy.ProxyPrefix + "/foo/bar", + headers: nil, + reverseProxy: false, expectedRedirect: "/", }, + { + name: "proxied request outside of ProxyPrefix redirects to proxied URL", + url: "https://oauth.example.com/foo/bar", + headers: map[string]string{ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "a-service.example.com", + "X-Forwarded-Uri": "/foo/bar", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com/foo/bar", + }, + { + name: "non-proxied request with spoofed proxy headers wouldn't redirect", + url: "https://oauth.example.com/foo?bar", + headers: map[string]string{ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "a-service.example.com", + "X-Forwarded-Uri": "/foo/bar", + }, + reverseProxy: false, + expectedRedirect: "/foo?bar", + }, + { + name: "proxied request under ProxyPrefix redirects to root", + url: "https://oauth.example.com" + proxy.ProxyPrefix + "/foo/bar", + headers: map[string]string{ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "a-service.example.com", + "X-Forwarded-Uri": proxy.ProxyPrefix + "/foo/bar", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com/", + }, + { + name: "proxied request with port under ProxyPrefix redirects to root", + url: "https://oauth.example.com" + proxy.ProxyPrefix + "/foo/bar", + headers: map[string]string{ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "a-service.example.com:8443", + "X-Forwarded-Uri": proxy.ProxyPrefix + "/foo/bar", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com:8443/", + }, + { + name: "proxied request with missing uri header would still redirect to desired redirect", + url: "https://oauth.example.com/foo?bar", + headers: map[string]string{ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "a-service.example.com", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com/foo?bar", + }, + { + name: "request with headers proxy not being set (and reverse proxy enabled) would still redirect to desired redirect", + url: "https://oauth.example.com/foo?bar", + headers: nil, + reverseProxy: true, + expectedRedirect: "/foo?bar", + }, + { + name: "proxied request with X-Auth-Request-Redirect being set outside of ProxyPrefix redirects to proxied URL", + url: "https://oauth.example.com/foo/bar", + headers: map[string]string{ + "X-Auth-Request-Redirect": "https://a-service.example.com/foo/bar", + "X-Forwarded-Proto": "", + "X-Forwarded-Host": "", + "X-Forwarded-Uri": "", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com/foo/bar", + }, + { + name: "proxied request with rd query string redirects to proxied URL", + url: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbar", + headers: nil, + reverseProxy: false, + expectedRedirect: "https://a-service.example.com/foo/bar", + }, + { + name: "proxied request with rd query string and all headers set (and reverse proxy not enabled) redirects to proxied URL on rd query string", + url: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz", + headers: map[string]string{ + "X-Auth-Request-Redirect": "https://a-service.example.com/foo/baz", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "another-service.example.com", + "X-Forwarded-Uri": "/seasons/greetings", + }, + reverseProxy: false, + expectedRedirect: "https://a-service.example.com/foo/jazz", + }, + { + name: "proxied request with rd query string and some headers set redirects to proxied URL on rd query string", + url: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbaz", + headers: map[string]string{ + "X-Auth-Request-Redirect": "", + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "another-service.example.com", + "X-Forwarded-Uri": "/seasons/greetings", + }, + reverseProxy: true, + expectedRedirect: "https://a-service.example.com/foo/baz", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { req, _ := http.NewRequest("GET", tt.url, nil) + for header, value := range tt.headers { + if value != "" { + req.Header.Add(header, value) + } + } + proxy.ReverseProxy = tt.reverseProxy redirect, err := proxy.GetRedirect(req) assert.NoError(t, err) diff --git a/pkg/util/util.go b/pkg/util/util.go index b39c1032..4eeabbf7 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -25,6 +25,15 @@ func GetCertPool(paths []string) (*x509.CertPool, error) { return pool, nil } +// GetRequestProto return the request host header or X-Forwarded-Proto if present +func GetRequestProto(req *http.Request) string { + proto := req.Header.Get("X-Forwarded-Proto") + if proto == "" { + proto = req.URL.Scheme + } + return proto +} + // GetRequestHost return the request host header or X-Forwarded-Host if present func GetRequestHost(req *http.Request) string { host := req.Header.Get("X-Forwarded-Host") @@ -33,3 +42,13 @@ func GetRequestHost(req *http.Request) string { } return host } + +// GetRequestURI return the request host header or X-Forwarded-Uri if present +func GetRequestURI(req *http.Request) string { + uri := req.Header.Get("X-Forwarded-Uri") + if uri == "" { + // Use RequestURI to preserve ?query + uri = req.URL.RequestURI() + } + return uri +} diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index c1e3d688..d032025e 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -110,3 +110,29 @@ func TestGetRequestHost(t *testing.T) { extHost := GetRequestHost(proxyReq) g.Expect(extHost).To(Equal("external.example.com")) } + +func TestGetRequestProto(t *testing.T) { + g := NewWithT(t) + + req := httptest.NewRequest("GET", "https://example.com", nil) + proto := GetRequestProto(req) + g.Expect(proto).To(Equal("https")) + + proxyReq := httptest.NewRequest("GET", "https://internal.example.com", nil) + proxyReq.Header.Add("X-Forwarded-Proto", "http") + extProto := GetRequestProto(proxyReq) + g.Expect(extProto).To(Equal("http")) +} + +func TestGetRequestURI(t *testing.T) { + g := NewWithT(t) + + req := httptest.NewRequest("GET", "https://example.com/ping", nil) + uri := GetRequestURI(req) + g.Expect(uri).To(Equal("/ping")) + + proxyReq := httptest.NewRequest("GET", "http://internal.example.com/bong", nil) + proxyReq.Header.Add("X-Forwarded-Uri", "/ping") + extURI := GetRequestURI(proxyReq) + g.Expect(extURI).To(Equal("/ping")) +}