diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e38f0cd..5d0d8021 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ## Important Notes +- [#964](https://github.com/oauth2-proxy/oauth2-proxy/pull/964) Redirect URL generation will attempt secondary strategies + in the priority chain if any fail the `IsValidRedirect` security check. Previously any failures fell back to `/`. - [#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`. @@ -36,6 +38,11 @@ ## Breaking Changes +- [#964](https://github.com/oauth2-proxy/oauth2-proxy/pull/964) `--reverse-proxy` must be true to trust `X-Forwarded-*` headers as canonical. + These are used throughout the application in redirect URLs, cookie domains and host logging logic. These are the headers: + - `X-Forwarded-Proto` instead of `req.URL.Scheme` + - `X-Forwarded-Host` instead of `req.Host` + - `X-Forwarded-Uri` instead of `req.URL.RequestURI()` - [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) In config files & envvar configs, `keycloak_group` is now the plural `keycloak_groups`. Flag configs are still `--keycloak-group` but it can be passed multiple times. - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Specifying a non-existent provider will cause OAuth2-Proxy to fail on startup instead of defaulting to "google". @@ -60,6 +67,7 @@ ## Changes since v6.1.1 - [#995](https://github.com/oauth2-proxy/oauth2-proxy/pull/995) Add Security Policy (@JoelSpeed) +- [#964](https://github.com/oauth2-proxy/oauth2-proxy/pull/964) Require `--reverse-proxy` true to trust `X-Forwareded-*` type headers (@NickMeves) - [#970](https://github.com/oauth2-proxy/oauth2-proxy/pull/970) Fix joined cookie name for those containing underline in the suffix (@peppered) - [#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) diff --git a/oauthproxy.go b/oauthproxy.go index 28f667b3..36c58c46 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -955,7 +955,9 @@ func (p *OAuthProxy) getAppRedirect(req *http.Request) (string, error) { p.getXForwardedHeadersRedirect, p.getURIRedirect, } { - if redirect := rdGetter(req); redirect != "" { + redirect := rdGetter(req) + // Call `p.IsValidRedirect` again here a final time to be safe + if redirect != "" && p.IsValidRedirect(redirect) { return redirect, nil } } @@ -972,24 +974,32 @@ func (p *OAuthProxy) hasProxyPrefix(path string) bool { return strings.HasPrefix(path, fmt.Sprintf("%s/", p.ProxyPrefix)) } -// getRdQuerystringRedirect handles this getAppRedirect strategy: -// - `rd` querysting parameter -func (p *OAuthProxy) getRdQuerystringRedirect(req *http.Request) string { - redirect := req.Form.Get("rd") +func (p *OAuthProxy) validateRedirect(redirect string, errorFormat string) string { if p.IsValidRedirect(redirect) { return redirect } + if redirect != "" { + logger.Errorf(errorFormat, redirect) + } return "" } +// getRdQuerystringRedirect handles this getAppRedirect strategy: +// - `rd` querysting parameter +func (p *OAuthProxy) getRdQuerystringRedirect(req *http.Request) string { + return p.validateRedirect( + req.Form.Get("rd"), + "Invalid redirect provided in rd querystring parameter: %s", + ) +} + // getXAuthRequestRedirect handles this getAppRedirect strategy: // - `X-Auth-Request-Redirect` Header func (p *OAuthProxy) getXAuthRequestRedirect(req *http.Request) string { - redirect := req.Header.Get("X-Auth-Request-Redirect") - if p.IsValidRedirect(redirect) { - return redirect - } - return "" + return p.validateRedirect( + req.Header.Get("X-Auth-Request-Redirect"), + "Invalid redirect provided in X-Auth-Request-Redirect header: %s", + ) } // getXForwardedHeadersRedirect handles these getAppRedirect strategies: @@ -1012,10 +1022,8 @@ func (p *OAuthProxy) getXForwardedHeadersRedirect(req *http.Request) string { uri, ) - if p.IsValidRedirect(redirect) { - return redirect - } - return "" + return p.validateRedirect(redirect, + "Invalid redirect generated from X-Forwarded-* headers: %s") } // getURIRedirect handles these getAppRedirect strategies: @@ -1023,8 +1031,11 @@ func (p *OAuthProxy) getXForwardedHeadersRedirect(req *http.Request) string { // - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*) // - `/` func (p *OAuthProxy) getURIRedirect(req *http.Request) string { - redirect := requestutil.GetRequestURI(req) - if !p.IsValidRedirect(redirect) { + redirect := p.validateRedirect( + requestutil.GetRequestURI(req), + "Invalid redirect generated from X-Forwarded-Uri header: %s", + ) + if redirect == "" { redirect = req.URL.RequestURI() }