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

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

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 |

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

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

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