From f53754808be745af4dda8868eed4576cb8920546 Mon Sep 17 00:00:00 2001 From: Ian Serpa Date: Fri, 19 Aug 2022 12:46:25 +0200 Subject: [PATCH] Support negating for skip auth routes --- CHANGELOG.md | 3 +- docs/docs/configuration/overview.md | 2 +- oauthproxy.go | 21 ++- oauthproxy_test.go | 227 ++++++++++++++++++++++++++++ pkg/apis/options/options.go | 2 +- 5 files changed, 250 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d4cc89..d87139c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 524e341..76121e9 100644 --- a/docs/docs/configuration/overview.md +++ b/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 | diff --git a/oauthproxy.go b/oauthproxy.go index c2c25a8..93a6977 100644 --- a/oauthproxy.go +++ b/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 } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b2abe37..657de44 100644 --- a/oauthproxy_test.go +++ b/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 { diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index eac42ea..a52c6fa 100644 --- a/pkg/apis/options/options.go +++ b/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")