Merge pull request #1762 from ianldgs/negate-route

Support negating for skip auth routes

CHANGELOG.md

@@ -26,7 +26,6 @@ N/A
 
 - [#1774](https://github.com/oauth2-proxy/oauth2-proxy/pull/1774) Fix vulnerabilities CVE-2022-27191, CVE-2021-44716 and CVE-2022-29526
 
-
 - [#1667](https://github.com/oauth2-proxy/oauth2-proxy/issues/1667) Rename configuration file flag for PKCE
 to remain consistent with CLI flags. You should specify `code_challenge_method` in your configuration instead of
 `force_code_challenge_method`.
@@ -36,6 +35,8 @@ to remain consistent with CLI flags. You should specify `code_challenge_method`
     This feature allows parallel callbacks and by default it is disabled. 
   - Add flag "--cookie-csrf-expire" to define a different expiration time for the CSRF cookie. By default, it is 15 minutes.
 
+- [#1762](https://github.com/oauth2-proxy/oauth2-proxy/pull/1762) Support negating for skip auth routes
+
 # V7.3.0
 
 ## Release Highlights

docs/docs/configuration/overview.md

@@ -186,7 +186,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--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 \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | |
-| `--skip-auth-route` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR path_regex alone for all methods | |
+| `--skip-auth-route` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex  | |
 | `--skip-auth-strip-headers` | bool | strips `X-Forwarded-*` style authentication headers & `Authorization` header if they would be set by oauth2-proxy | true |
 | `--skip-jwt-bearer-tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false |
 | `--skip-oidc-discovery` | bool | bypass OIDC endpoint discovery. `--login-url`, `--redeem-url` and `--oidc-jwks-url` must be configured in this case | false |

oauthproxy.go

@@ -64,6 +64,7 @@ var (
 // allowedRoute manages method + path based allowlists
 type allowedRoute struct {
 	method    string
+	negate    bool
 	pathRegex *regexp.Regexp
 }
 
@@ -445,9 +446,10 @@ func buildRoutesAllowlist(opts *options.Options) ([]allowedRoute, error) {
 		var (
 			method string
 			path   string
+			negate = strings.Contains(methodPath, "!=")
 		)
 
-		parts := strings.SplitN(methodPath, "=", 2)
+		parts := regexp.MustCompile("!?=").Split(methodPath, 2)
 		if len(parts) == 1 {
 			method = ""
 			path = parts[0]
@@ -463,6 +465,7 @@ func buildRoutesAllowlist(opts *options.Options) ([]allowedRoute, error) {
 		logger.Printf("Skipping auth - Method: %s | Path: %s", method, path)
 		routes = append(routes, allowedRoute{
 			method:    method,
+			negate:    negate,
 			pathRegex: compiledRegex,
 		})
 	}
@@ -516,10 +519,24 @@ func (p *OAuthProxy) IsAllowedRequest(req *http.Request) bool {
 	return isPreflightRequestAllowed || p.isAllowedRoute(req) || p.isTrustedIP(req)
 }
 
+func isAllowedMethod(req *http.Request, route allowedRoute) bool {
+	return route.method == "" || req.Method == route.method
+}
+
+func isAllowedPath(req *http.Request, route allowedRoute) bool {
+	matches := route.pathRegex.MatchString(req.URL.Path)
+
+	if route.negate {
+		return !matches
+	}
+
+	return matches
+}
+
 // IsAllowedRoute is used to check if the request method & path is allowed without auth
 func (p *OAuthProxy) isAllowedRoute(req *http.Request) bool {
 	for _, route := range p.allowedRoutes {
-		if (route.method == "" || req.Method == route.method) && route.pathRegex.MatchString(req.URL.Path) {
+		if isAllowedMethod(req, route) && isAllowedPath(req, route) {
 			return true
 		}
 	}

oauthproxy_test.go

@@ -2248,6 +2248,7 @@ func TestTrustedIPs(t *testing.T) {
 func Test_buildRoutesAllowlist(t *testing.T) {
 	type expectedAllowedRoute struct {
 		method      string
+		negate      bool
 		regexString string
 	}
 
@@ -2275,10 +2276,12 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 			expectedRoutes: []expectedAllowedRoute{
 				{
 					method:      "",
+					negate:      false,
 					regexString: "^/foo/bar",
 				},
 				{
 					method:      "",
+					negate:      false,
 					regexString: "^/baz/[0-9]+/thing",
 				},
 			},
@@ -2293,28 +2296,45 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"^/all/methods$",
 				"WEIRD=^/methods/are/allowed",
 				"PATCH=/second/equals?are=handled&just=fine",
+				"!=^/api",
+				"METHOD!=^/api",
 			},
 			expectedRoutes: []expectedAllowedRoute{
 				{
 					method:      "GET",
+					negate:      false,
 					regexString: "^/foo/bar",
 				},
 				{
 					method:      "POST",
+					negate:      false,
 					regexString: "^/baz/[0-9]+/thing",
 				},
 				{
 					method:      "",
+					negate:      false,
 					regexString: "^/all/methods$",
 				},
 				{
 					method:      "WEIRD",
+					negate:      false,
 					regexString: "^/methods/are/allowed",
 				},
 				{
 					method:      "PATCH",
+					negate:      false,
 					regexString: "/second/equals?are=handled&just=fine",
 				},
+				{
+					method:      "",
+					negate:      true,
+					regexString: "^/api",
+				},
+				{
+					method:      "METHOD",
+					negate:      true,
+					regexString: "^/api",
+				},
 			},
 			shouldError: false,
 		},
@@ -2394,6 +2414,7 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 			for i, route := range routes {
 				assert.Greater(t, len(tc.expectedRoutes), i)
 				assert.Equal(t, route.method, tc.expectedRoutes[i].method)
+				assert.Equal(t, route.negate, tc.expectedRoutes[i].negate)
 				assert.Equal(t, route.pathRegex.String(), tc.expectedRoutes[i].regexString)
 			}
 		})
@@ -2475,6 +2496,212 @@ func TestAllowedRequest(t *testing.T) {
 			url:     "/skip/auth/routes/wrong/path",
 			allowed: false,
 		},
+		{
+			name:    "Route denied with wrong path and method",
+			method:  "POST",
+			url:     "/skip/auth/routes/wrong/path",
+			allowed: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			req, err := http.NewRequest(tc.method, tc.url, nil)
+			assert.NoError(t, err)
+			assert.Equal(t, tc.allowed, proxy.isAllowedRoute(req))
+
+			rw := httptest.NewRecorder()
+			proxy.ServeHTTP(rw, req)
+
+			if tc.allowed {
+				assert.Equal(t, 200, rw.Code)
+				assert.Equal(t, "Allowed Request", rw.Body.String())
+			} else {
+				assert.Equal(t, 403, rw.Code)
+			}
+		})
+	}
+}
+
+func TestAllowedRequestNegateWithoutMethod(t *testing.T) {
+	upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		_, err := w.Write([]byte("Allowed Request"))
+		if err != nil {
+			t.Fatal(err)
+		}
+	}))
+	t.Cleanup(upstreamServer.Close)
+
+	opts := baseTestOptions()
+	opts.UpstreamServers = options.UpstreamConfig{
+		Upstreams: []options.Upstream{
+			{
+				ID:   upstreamServer.URL,
+				Path: "/",
+				URI:  upstreamServer.URL,
+			},
+		},
+	}
+	opts.SkipAuthRoutes = []string{
+		"!=^/api", // any non-api routes
+		"POST=^/api/public-entity/?$",
+	}
+	err := validation.Validate(opts)
+	assert.NoError(t, err)
+	proxy, err := NewOAuthProxy(opts, func(_ string) bool { return true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testCases := []struct {
+		name    string
+		method  string
+		url     string
+		allowed bool
+	}{
+		{
+			name:    "Some static file allowed",
+			method:  "GET",
+			url:     "/static/file.txt",
+			allowed: true,
+		},
+		{
+			name:    "POST to contact form allowed",
+			method:  "POST",
+			url:     "/contact",
+			allowed: true,
+		},
+		{
+			name:    "Regex POST allowed",
+			method:  "POST",
+			url:     "/api/public-entity",
+			allowed: true,
+		},
+		{
+			name:    "Regex POST with trailing slash allowed",
+			method:  "POST",
+			url:     "/api/public-entity/",
+			allowed: true,
+		},
+		{
+			name:    "Regex GET api route denied",
+			method:  "GET",
+			url:     "/api/users",
+			allowed: false,
+		},
+		{
+			name:    "Regex POST api route denied",
+			method:  "POST",
+			url:     "/api/users",
+			allowed: false,
+		},
+		{
+			name:    "Regex DELETE api route denied",
+			method:  "DELETE",
+			url:     "/api/users/1",
+			allowed: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			req, err := http.NewRequest(tc.method, tc.url, nil)
+			assert.NoError(t, err)
+			assert.Equal(t, tc.allowed, proxy.isAllowedRoute(req))
+
+			rw := httptest.NewRecorder()
+			proxy.ServeHTTP(rw, req)
+
+			if tc.allowed {
+				assert.Equal(t, 200, rw.Code)
+				assert.Equal(t, "Allowed Request", rw.Body.String())
+			} else {
+				assert.Equal(t, 403, rw.Code)
+			}
+		})
+	}
+}
+
+func TestAllowedRequestNegateWithMethod(t *testing.T) {
+	upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		_, err := w.Write([]byte("Allowed Request"))
+		if err != nil {
+			t.Fatal(err)
+		}
+	}))
+	t.Cleanup(upstreamServer.Close)
+
+	opts := baseTestOptions()
+	opts.UpstreamServers = options.UpstreamConfig{
+		Upstreams: []options.Upstream{
+			{
+				ID:   upstreamServer.URL,
+				Path: "/",
+				URI:  upstreamServer.URL,
+			},
+		},
+	}
+	opts.SkipAuthRoutes = []string{
+		"GET!=^/api", // any non-api routes
+		"POST=^/api/public-entity/?$",
+	}
+	err := validation.Validate(opts)
+	assert.NoError(t, err)
+	proxy, err := NewOAuthProxy(opts, func(_ string) bool { return true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testCases := []struct {
+		name    string
+		method  string
+		url     string
+		allowed bool
+	}{
+		{
+			name:    "Some static file allowed",
+			method:  "GET",
+			url:     "/static/file.txt",
+			allowed: true,
+		},
+		{
+			name:    "POST to contact form not allowed",
+			method:  "POST",
+			url:     "/contact",
+			allowed: false,
+		},
+		{
+			name:    "Regex POST allowed",
+			method:  "POST",
+			url:     "/api/public-entity",
+			allowed: true,
+		},
+		{
+			name:    "Regex POST with trailing slash allowed",
+			method:  "POST",
+			url:     "/api/public-entity/",
+			allowed: true,
+		},
+		{
+			name:    "Regex GET api route denied",
+			method:  "GET",
+			url:     "/api/users",
+			allowed: false,
+		},
+		{
+			name:    "Regex POST api route denied",
+			method:  "POST",
+			url:     "/api/users",
+			allowed: false,
+		},
+		{
+			name:    "Regex DELETE api route denied",
+			method:  "DELETE",
+			url:     "/api/users/1",
+			allowed: false,
+		},
 	}
 
 	for _, tc := range testCases {

pkg/apis/options/options.go

@@ -115,7 +115,7 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.Bool("force-https", false, "force HTTPS redirect for HTTP requests")
 	flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"")
 	flagSet.StringSlice("skip-auth-regex", []string{}, "(DEPRECATED for --skip-auth-route) bypass authentication for requests path's that match (may be given multiple times)")
-	flagSet.StringSlice("skip-auth-route", []string{}, "bypass authentication for requests that match the method & path. Format: method=path_regex OR path_regex alone for all methods")
+	flagSet.StringSlice("skip-auth-route", []string{}, "bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_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")