Use X-Forwarded-{Proto,Host,Uri} on redirect as last resort (#957)

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)

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.
 :::

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

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)

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
+}

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"))
+}