From abeb0236d87969243a6b061c5a601c8daffc424c Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 14 Jul 2020 15:46:44 -0700 Subject: [PATCH] Strip X-Forwarded auth headers from whitelisted paths (#624) * Strip X-Forwarded auth headers from whitelisted paths For any paths that match skip-auth-regex, strip normal X-Forwarded headers that would be sent based on pass-user-headers or pass-access-token settings. This prevents malicious injecting of authentication headers through the skip-auth-regex paths in cases where the regex might be misconfigured and too open. Control this behavior with --skip-auth-strip-headers flag. This flag is set to TRUE by default (this is secure by default, but potentially breaks some legacy configurations). Only x-Forwarded headers stripped, left the Authorization header untouched. * Strip authorization header if it would be set * Improve TestStripAuthHeaders test table * Improve --skip-auth-strip-headers flag documentation --- CHANGELOG.md | 1 + docs/configuration/configuration.md | 1 + oauthproxy.go | 36 +++++++- oauthproxy_test.go | 136 ++++++++++++++++++++++++++++ pkg/apis/options/options.go | 3 + 5 files changed, 176 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc58b625..62344892 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ## Changes since v6.0.0 +- [#624](https://github.com/oauth2-proxy/oauth2-proxy/pull/624) Allow stripping authentication headers from whitelisted requests with `--skip-auth-strip-headers` (@NickMeves) - [#673](https://github.com/oauth2-proxy/oauth2-proxy/pull/673) Add --session-cookie-minimal option to create session cookies with no tokens (@NickMeves) - [#632](https://github.com/oauth2-proxy/oauth2-proxy/pull/632) Reduce session size by encoding with MessagePack and using LZ4 compression (@NickMeves) - [#675](https://github.com/oauth2-proxy/oauth2-proxy/pull/675) Fix required ruby version and deprecated option for building docs (@mkontani) diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md index 6bf5c4f5..0e073ed6 100644 --- a/docs/configuration/configuration.md +++ b/docs/configuration/configuration.md @@ -116,6 +116,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example | `--silence-ping-logging` | bool | disable logging of requests to ping endpoint | false | | `--skip-auth-preflight` | bool | will skip authentication for OPTIONS requests | false | | `--skip-auth-regex` | string | bypass authentication for requests paths that match (may be given multiple times) | | +| `--skip-auth-strip-headers` | bool | strips `X-Forwarded-*` style authentication headers & `Authorization` header if they would be set by oauth2-proxy for request paths in `--skip-auth-regex` | false | | `--skip-jwt-bearer-tokens` | bool | will skip requests that have verified JWT bearer tokens | false | | `--skip-oidc-discovery` | bool | bypass OIDC endpoint discovery. `--login-url`, `--redeem-url` and `--oidc-jwks-url` must be configured in this case | false | | `--skip-provider-button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false | diff --git a/oauthproxy.go b/oauthproxy.go index 49a34b2a..c05c793d 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -111,6 +111,7 @@ type OAuthProxy struct { PreferEmailToUser bool skipAuthRegex []string skipAuthPreflight bool + skipAuthStripHeaders bool skipJwtBearerTokens bool mainJwtBearerVerifier *oidc.IDTokenVerifier extraJwtBearerVerifiers []*oidc.IDTokenVerifier @@ -343,6 +344,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr whitelistDomains: opts.WhitelistDomains, skipAuthRegex: opts.SkipAuthRegex, skipAuthPreflight: opts.SkipAuthPreflight, + skipAuthStripHeaders: opts.SkipAuthStripHeaders, skipJwtBearerTokens: opts.SkipJwtBearerTokens, mainJwtBearerVerifier: opts.GetOIDCVerifier(), extraJwtBearerVerifiers: opts.GetJWTBearerVerifiers(), @@ -718,7 +720,7 @@ func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { case path == p.RobotsPath: p.RobotsTxt(rw) case p.IsWhitelistedRequest(req): - p.serveMux.ServeHTTP(rw, req) + p.SkipAuthProxy(rw, req) case path == p.SignInPath: p.SignIn(rw, req) case path == p.SignOutPath: @@ -891,6 +893,14 @@ func (p *OAuthProxy) AuthenticateOnly(rw http.ResponseWriter, req *http.Request) rw.WriteHeader(http.StatusAccepted) } +// SkipAuthProxy proxies whitelisted requests and skips authentication +func (p *OAuthProxy) SkipAuthProxy(rw http.ResponseWriter, req *http.Request) { + if p.skipAuthStripHeaders { + p.stripAuthHeaders(req) + } + p.serveMux.ServeHTTP(rw, req) +} + // Proxy proxies the user request if the user is authenticated else it prompts // them to authenticate func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { @@ -1122,6 +1132,30 @@ func (p *OAuthProxy) addHeadersForProxying(rw http.ResponseWriter, req *http.Req } } +// stripAuthHeaders removes Auth headers for whitelisted routes from skipAuthRegex +func (p *OAuthProxy) stripAuthHeaders(req *http.Request) { + if p.PassBasicAuth { + req.Header.Del("X-Forwarded-User") + req.Header.Del("X-Forwarded-Email") + req.Header.Del("X-Forwarded-Preferred-Username") + req.Header.Del("Authorization") + } + + if p.PassUserHeaders { + req.Header.Del("X-Forwarded-User") + req.Header.Del("X-Forwarded-Email") + req.Header.Del("X-Forwarded-Preferred-Username") + } + + if p.PassAccessToken { + req.Header.Del("X-Forwarded-Access-Token") + } + + if p.PassAuthorization { + req.Header.Del("Authorization") + } +} + // CheckBasicAuth checks the requests Authorization header for basic auth // credentials and authenticates these against the proxies HtpasswdFile func (p *OAuthProxy) CheckBasicAuth(req *http.Request) (*sessionsapi.SessionState, error) { diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 5bafd048..40b2d9d1 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -723,6 +723,142 @@ func TestPassUserHeadersWithEmail(t *testing.T) { } } +func TestStripAuthHeaders(t *testing.T) { + testCases := map[string]struct { + SkipAuthStripHeaders bool + PassBasicAuth bool + PassUserHeaders bool + PassAccessToken bool + PassAuthorization bool + StrippedHeaders map[string]bool + }{ + "Default options": { + SkipAuthStripHeaders: true, + PassBasicAuth: true, + PassUserHeaders: true, + PassAccessToken: false, + PassAuthorization: false, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": true, + "X-Forwarded-Email": true, + "X-Forwarded-Preferred-Username": true, + "X-Forwarded-Access-Token": false, + "Authorization": true, + }, + }, + "Pass access token": { + SkipAuthStripHeaders: true, + PassBasicAuth: true, + PassUserHeaders: true, + PassAccessToken: true, + PassAuthorization: false, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": true, + "X-Forwarded-Email": true, + "X-Forwarded-Preferred-Username": true, + "X-Forwarded-Access-Token": true, + "Authorization": true, + }, + }, + "Nothing setting Authorization": { + SkipAuthStripHeaders: true, + PassBasicAuth: false, + PassUserHeaders: true, + PassAccessToken: true, + PassAuthorization: false, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": true, + "X-Forwarded-Email": true, + "X-Forwarded-Preferred-Username": true, + "X-Forwarded-Access-Token": true, + "Authorization": false, + }, + }, + "Only Authorization header modified": { + SkipAuthStripHeaders: true, + PassBasicAuth: false, + PassUserHeaders: false, + PassAccessToken: false, + PassAuthorization: true, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": false, + "X-Forwarded-Email": false, + "X-Forwarded-Preferred-Username": false, + "X-Forwarded-Access-Token": false, + "Authorization": true, + }, + }, + "Don't strip any headers (default options)": { + SkipAuthStripHeaders: false, + PassBasicAuth: true, + PassUserHeaders: true, + PassAccessToken: false, + PassAuthorization: false, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": false, + "X-Forwarded-Email": false, + "X-Forwarded-Preferred-Username": false, + "X-Forwarded-Access-Token": false, + "Authorization": false, + }, + }, + "Don't strip any headers (custom options)": { + SkipAuthStripHeaders: false, + PassBasicAuth: true, + PassUserHeaders: true, + PassAccessToken: true, + PassAuthorization: false, + StrippedHeaders: map[string]bool{ + "X-Forwarded-User": false, + "X-Forwarded-Email": false, + "X-Forwarded-Preferred-Username": false, + "X-Forwarded-Access-Token": false, + "Authorization": false, + }, + }, + } + + initialHeaders := map[string]string{ + "X-Forwarded-User": "9fcab5c9b889a557", + "X-Forwarded-Email": "john.doe@example.com", + "X-Forwarded-Preferred-Username": "john.doe", + "X-Forwarded-Access-Token": "AccessToken", + "Authorization": "bearer IDToken", + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + opts := baseTestOptions() + opts.SkipAuthStripHeaders = tc.SkipAuthStripHeaders + opts.PassBasicAuth = tc.PassBasicAuth + opts.PassUserHeaders = tc.PassUserHeaders + opts.PassAccessToken = tc.PassAccessToken + opts.PassAuthorization = tc.PassAuthorization + err := validation.Validate(opts) + assert.NoError(t, err) + + req, _ := http.NewRequest("GET", fmt.Sprintf("%s/testCase", opts.ProxyPrefix), nil) + for header, val := range initialHeaders { + req.Header.Set(header, val) + } + + proxy, err := NewOAuthProxy(opts, func(_ string) bool { return true }) + assert.NoError(t, err) + if proxy.skipAuthStripHeaders { + proxy.stripAuthHeaders(req) + } + + for header, stripped := range tc.StrippedHeaders { + if stripped { + assert.Equal(t, req.Header.Get(header), "") + } else { + assert.Equal(t, req.Header.Get(header), initialHeaders[header]) + } + } + }) + } +} + type PassAccessTokenTest struct { providerServer *httptest.Server proxy *OAuthProxy diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 8b9f84b5..5e26ea1e 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -66,6 +66,7 @@ type Options struct { Upstreams []string `flag:"upstream" cfg:"upstreams"` SkipAuthRegex []string `flag:"skip-auth-regex" cfg:"skip_auth_regex"` + SkipAuthStripHeaders bool `flag:"skip-auth-strip-headers" cfg:"skip_auth_strip_headers"` SkipJwtBearerTokens bool `flag:"skip-jwt-bearer-tokens" cfg:"skip_jwt_bearer_tokens"` ExtraJwtIssuers []string `flag:"extra-jwt-issuers" cfg:"extra_jwt_issuers"` PassBasicAuth bool `flag:"pass-basic-auth" cfg:"pass_basic_auth"` @@ -159,6 +160,7 @@ func NewOptions() *Options { AzureTenant: "common", SetXAuthRequest: false, SkipAuthPreflight: false, + SkipAuthStripHeaders: false, FlushInterval: time.Duration(1) * time.Second, PassBasicAuth: true, SetBasicAuth: false, @@ -202,6 +204,7 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Bool("pass-authorization-header", false, "pass the Authorization Header to upstream") flagSet.Bool("set-authorization-header", false, "set Authorization response headers (useful in Nginx auth_request mode)") flagSet.StringSlice("skip-auth-regex", []string{}, "bypass authentication for requests path's that match (may be given multiple times)") + flagSet.Bool("skip-auth-strip-headers", false, "strips X-Forwarded-* style authentication headers & Authorization header if they would be set by oauth2-proxy for request paths in --skip-auth-regex") flagSet.Bool("skip-provider-button", false, "will skip sign-in-page to directly reach the next step: oauth/start") flagSet.Bool("skip-auth-preflight", false, "will skip authentication for OPTIONS requests") flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS providers")