diff --git a/CHANGELOG.md b/CHANGELOG.md index eda96f1a..c3037b42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ ## Important Notes +- [#789](https://github.com/oauth2-proxy/oauth2-proxy/pull/789) `--skip-auth-route` is (almost) backwards compatible with `--skip-auth-regex` + - We are marking `--skip-auth-regex` as DEPRECATED and will remove it in the next major version. + - If your regex contains an `=` and you want it for all methods, you will need to add a leading `=` (this is the area where `--skip-auth-regex` doesn't port perfectly) - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Sessions from v5.1.1 or earlier will no longer validate since they were not signed with SHA1. - Sessions from v6.0.0 or later had a graceful conversion to SHA256 that resulted in no reauthentication - Upgrading from v5.1.1 or earlier will result in a reauthentication @@ -23,6 +26,7 @@ ## Changes since v6.1.1 - [#753](https://github.com/oauth2-proxy/oauth2-proxy/pull/753) Pass resource parameter in login url (@codablock) +- [#789](https://github.com/oauth2-proxy/oauth2-proxy/pull/789) Add `--skip-auth-route` configuration option for `METHOD=pathRegex` based allowlists (@NickMeves) - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Stop accepting legacy SHA1 signed cookies (@NickMeves) - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) Validate Redis configuration options at startup (@NickMeves) - [#791](https://github.com/oauth2-proxy/oauth2-proxy/pull/791) Remove GetPreferredUsername method from provider interface (@NickMeves) diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md index 0370cdf4..8fbc406d 100644 --- a/docs/configuration/configuration.md +++ b/docs/configuration/configuration.md @@ -119,8 +119,9 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example | `--signature-key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | `--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-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-strip-headers` | bool | strips `X-Forwarded-*` style authentication headers & `Authorization` header if they would be set by oauth2-proxy for allowlisted requests (`--skip-auth-route`, `--skip-auth-regex`, `--skip-auth-preflight`, `--trusted-ip`) | 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 092dcc93..e64ffe91 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -48,6 +48,12 @@ var ( invalidRedirectRegex = regexp.MustCompile(`[/\\](?:[\s\v]*|\.{1,2})[/\\]`) ) +// allowedRoute manages method + path based allowlists +type allowedRoute struct { + method string + pathRegex *regexp.Regexp +} + // OAuthProxy is the main authentication proxy type OAuthProxy struct { CookieSeed string @@ -70,6 +76,7 @@ type OAuthProxy struct { AuthOnlyPath string UserInfoPath string + allowedRoutes []allowedRoute redirectURL *url.URL // the url to receive requests at whitelistDomains []string provider providers.Provider @@ -90,13 +97,11 @@ type OAuthProxy struct { SetAuthorization bool PassAuthorization bool PreferEmailToUser bool - skipAuthRegex []string skipAuthPreflight bool skipAuthStripHeaders bool skipJwtBearerTokens bool mainJwtBearerVerifier *oidc.IDTokenVerifier extraJwtBearerVerifiers []*oidc.IDTokenVerifier - compiledRegex []*regexp.Regexp templates *template.Template realClientIPParser ipapi.RealClientIPParser trustedIPs *ip.NetSet @@ -121,10 +126,6 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr return nil, fmt.Errorf("error initialising upstream proxy: %v", err) } - for _, u := range opts.GetCompiledRegex() { - logger.Printf("compiled skip-auth-regex => %q", u) - } - if opts.SkipJwtBearerTokens { logger.Printf("Skipping JWT tokens from configured OIDC issuer: %q", opts.OIDCIssuerURL) for _, issuer := range opts.ExtraJwtIssuers { @@ -163,6 +164,11 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr } } + allowedRoutes, err := buildRoutesAllowlist(opts) + if err != nil { + return nil, err + } + sessionChain := buildSessionChain(opts, sessionStore, basicAuthValidator) return &OAuthProxy{ @@ -192,14 +198,13 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr sessionStore: sessionStore, serveMux: upstreamProxy, redirectURL: redirectURL, + allowedRoutes: allowedRoutes, whitelistDomains: opts.WhitelistDomains, - skipAuthRegex: opts.SkipAuthRegex, skipAuthPreflight: opts.SkipAuthPreflight, skipAuthStripHeaders: opts.SkipAuthStripHeaders, skipJwtBearerTokens: opts.SkipJwtBearerTokens, mainJwtBearerVerifier: opts.GetOIDCVerifier(), extraJwtBearerVerifiers: opts.GetJWTBearerVerifiers(), - compiledRegex: opts.GetCompiledRegex(), realClientIPParser: opts.GetRealClientIPParser(), SetXAuthRequest: opts.SetXAuthRequest, PassBasicAuth: opts.PassBasicAuth, @@ -277,6 +282,53 @@ func buildSignInMessage(opts *options.Options) string { return msg } +// buildRoutesAllowlist builds an []allowedRoute list from either the legacy +// SkipAuthRegex option (paths only support) or newer SkipAuthRoutes option +// (method=path support) +func buildRoutesAllowlist(opts *options.Options) ([]allowedRoute, error) { + routes := make([]allowedRoute, 0, len(opts.SkipAuthRegex)+len(opts.SkipAuthRoutes)) + + for _, path := range opts.SkipAuthRegex { + compiledRegex, err := regexp.Compile(path) + if err != nil { + return nil, err + } + logger.Printf("Skipping auth - Method: ALL | Path: %s", path) + routes = append(routes, allowedRoute{ + method: "", + pathRegex: compiledRegex, + }) + } + + for _, methodPath := range opts.SkipAuthRoutes { + var ( + method string + path string + ) + + parts := strings.SplitN(methodPath, "=", 2) + if len(parts) == 1 { + method = "" + path = parts[0] + } else { + method = strings.ToUpper(parts[0]) + path = parts[1] + } + + compiledRegex, err := regexp.Compile(path) + if err != nil { + return nil, err + } + logger.Printf("Skipping auth - Method: %s | Path: %s", method, path) + routes = append(routes, allowedRoute{ + method: method, + pathRegex: compiledRegex, + }) + } + + return routes, nil +} + // GetRedirectURI returns the redirectURL that the upstream OAuth Provider will // redirect clients to once authenticated func (p *OAuthProxy) GetRedirectURI(host string) string { @@ -584,16 +636,16 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool { } } -// IsWhitelistedRequest is used to check if auth should be skipped for this request -func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) bool { +// IsAllowedRequest is used to check if auth should be skipped for this request +func (p *OAuthProxy) IsAllowedRequest(req *http.Request) bool { isPreflightRequestAllowed := p.skipAuthPreflight && req.Method == "OPTIONS" - return isPreflightRequestAllowed || p.IsWhitelistedPath(req.URL.Path) || p.IsTrustedIP(req) + return isPreflightRequestAllowed || p.isAllowedRoute(req) || p.IsTrustedIP(req) } -// IsWhitelistedPath is used to check if the request path is allowed without auth -func (p *OAuthProxy) IsWhitelistedPath(path string) bool { - for _, u := range p.compiledRegex { - if u.MatchString(path) { +// 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) { return true } } @@ -643,7 +695,7 @@ func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { switch path := req.URL.Path; { case path == p.RobotsPath: p.RobotsTxt(rw) - case p.IsWhitelistedRequest(req): + case p.IsAllowedRequest(req): p.SkipAuthProxy(rw, req) case path == p.SignInPath: p.SignIn(rw, req) @@ -831,7 +883,7 @@ func (p *OAuthProxy) AuthenticateOnly(rw http.ResponseWriter, req *http.Request) rw.WriteHeader(http.StatusAccepted) } -// SkipAuthProxy proxies whitelisted requests and skips authentication +// SkipAuthProxy proxies allowlisted requests and skips authentication func (p *OAuthProxy) SkipAuthProxy(rw http.ResponseWriter, req *http.Request) { if p.skipAuthStripHeaders { p.stripAuthHeaders(req) @@ -1026,7 +1078,7 @@ func (p *OAuthProxy) addHeadersForProxying(rw http.ResponseWriter, req *http.Req } } -// stripAuthHeaders removes Auth headers for whitelisted routes from skipAuthRegex +// stripAuthHeaders removes Auth headers for allowlisted routes from skipAuthRegex func (p *OAuthProxy) stripAuthHeaders(req *http.Request) { if p.PassBasicAuth { req.Header.Del("X-Forwarded-User") diff --git a/oauthproxy_test.go b/oauthproxy_test.go index d0feec2d..d0fd9481 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -1482,28 +1482,28 @@ func TestAuthOnlyEndpointSetBasicAuthFalseRequestHeaders(t *testing.T) { } func TestAuthSkippedForPreflightRequests(t *testing.T) { - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) _, err := w.Write([]byte("response")) if err != nil { t.Fatal(err) } })) - t.Cleanup(upstream.Close) + t.Cleanup(upstreamServer.Close) opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ { - ID: upstream.URL, + ID: upstreamServer.URL, Path: "/", - URI: upstream.URL, + URI: upstreamServer.URL, }, } opts.SkipAuthPreflight = true err := validation.Validate(opts) assert.NoError(t, err) - upstreamURL, _ := url.Parse(upstream.URL) + upstreamURL, _ := url.Parse(upstreamServer.URL) opts.SetProvider(NewTestProvider(upstreamURL, "")) proxy, err := NewOAuthProxy(opts, func(string) bool { return false }) @@ -1561,17 +1561,17 @@ func NewSignatureTest() (*SignatureTest, error) { opts.EmailDomains = []string{"acm.org"} authenticator := &SignatureAuthenticator{} - upstream := httptest.NewServer( + upstreamServer := httptest.NewServer( http.HandlerFunc(authenticator.Authenticate)) - upstreamURL, err := url.Parse(upstream.URL) + upstreamURL, err := url.Parse(upstreamServer.URL) if err != nil { return nil, err } opts.UpstreamServers = options.Upstreams{ { - ID: upstream.URL, + ID: upstreamServer.URL, Path: "/", - URI: upstream.URL, + URI: upstreamServer.URL, }, } @@ -1590,7 +1590,7 @@ func NewSignatureTest() (*SignatureTest, error) { return &SignatureTest{ opts, - upstream, + upstreamServer, upstreamURL.Host, provider, make(http.Header), @@ -1974,20 +1974,20 @@ func Test_prepareNoCache(t *testing.T) { } func Test_noCacheHeaders(t *testing.T) { - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, err := w.Write([]byte("upstream")) if err != nil { t.Error(err) } })) - t.Cleanup(upstream.Close) + t.Cleanup(upstreamServer.Close) opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ { - ID: upstream.URL, + ID: upstreamServer.URL, Path: "/", - URI: upstream.URL, + URI: upstreamServer.URL, }, } opts.SkipAuthRegex = []string{".*"} @@ -2224,7 +2224,8 @@ func TestTrustedIPs(t *testing.T) { opts.TrustedIPs = tt.trustedIPs opts.ReverseProxy = tt.reverseProxy opts.RealClientIPHeader = tt.realClientIPHeader - validation.Validate(opts) + err := validation.Validate(opts) + assert.NoError(t, err) proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) assert.NoError(t, err) @@ -2240,6 +2241,255 @@ func TestTrustedIPs(t *testing.T) { } } +func Test_buildRoutesAllowlist(t *testing.T) { + type expectedAllowedRoute struct { + method string + regexString string + } + + testCases := []struct { + name string + skipAuthRegex []string + skipAuthRoutes []string + expectedRoutes []expectedAllowedRoute + shouldError bool + }{ + { + name: "No skip auth configured", + skipAuthRegex: []string{}, + skipAuthRoutes: []string{}, + expectedRoutes: []expectedAllowedRoute{}, + shouldError: false, + }, + { + name: "Only skipAuthRegex configured", + skipAuthRegex: []string{ + "^/foo/bar", + "^/baz/[0-9]+/thing", + }, + skipAuthRoutes: []string{}, + expectedRoutes: []expectedAllowedRoute{ + { + method: "", + regexString: "^/foo/bar", + }, + { + method: "", + regexString: "^/baz/[0-9]+/thing", + }, + }, + shouldError: false, + }, + { + name: "Only skipAuthRoutes configured", + skipAuthRegex: []string{}, + skipAuthRoutes: []string{ + "GET=^/foo/bar", + "POST=^/baz/[0-9]+/thing", + "^/all/methods$", + "WEIRD=^/methods/are/allowed", + "PATCH=/second/equals?are=handled&just=fine", + }, + expectedRoutes: []expectedAllowedRoute{ + { + method: "GET", + regexString: "^/foo/bar", + }, + { + method: "POST", + regexString: "^/baz/[0-9]+/thing", + }, + { + method: "", + regexString: "^/all/methods$", + }, + { + method: "WEIRD", + regexString: "^/methods/are/allowed", + }, + { + method: "PATCH", + regexString: "/second/equals?are=handled&just=fine", + }, + }, + shouldError: false, + }, + { + name: "Both skipAuthRegexes and skipAuthRoutes configured", + skipAuthRegex: []string{ + "^/foo/bar/regex", + "^/baz/[0-9]+/thing/regex", + }, + skipAuthRoutes: []string{ + "GET=^/foo/bar", + "POST=^/baz/[0-9]+/thing", + "^/all/methods$", + }, + expectedRoutes: []expectedAllowedRoute{ + { + method: "", + regexString: "^/foo/bar/regex", + }, + { + method: "", + regexString: "^/baz/[0-9]+/thing/regex", + }, + { + method: "GET", + regexString: "^/foo/bar", + }, + { + method: "POST", + regexString: "^/baz/[0-9]+/thing", + }, + { + method: "", + regexString: "^/all/methods$", + }, + }, + shouldError: false, + }, + { + name: "Invalid skipAuthRegex entry", + skipAuthRegex: []string{ + "^/foo/bar", + "^/baz/[0-9]+/thing", + "(bad[regex", + }, + skipAuthRoutes: []string{}, + expectedRoutes: []expectedAllowedRoute{}, + shouldError: true, + }, + { + name: "Invalid skipAuthRoutes entry", + skipAuthRegex: []string{}, + skipAuthRoutes: []string{ + "GET=^/foo/bar", + "POST=^/baz/[0-9]+/thing", + "^/all/methods$", + "PUT=(bad[regex", + }, + expectedRoutes: []expectedAllowedRoute{}, + shouldError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + opts := &options.Options{ + SkipAuthRegex: tc.skipAuthRegex, + SkipAuthRoutes: tc.skipAuthRoutes, + } + routes, err := buildRoutesAllowlist(opts) + if tc.shouldError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + 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.pathRegex.String(), tc.expectedRoutes[i].regexString) + } + }) + } +} + +func TestAllowedRequest(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.Upstreams{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, + } + opts.SkipAuthRegex = []string{ + "^/skip/auth/regex$", + } + opts.SkipAuthRoutes = []string{ + "GET=^/skip/auth/routes/get", + } + 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: "Regex GET allowed", + method: "GET", + url: "/skip/auth/regex", + allowed: true, + }, + { + name: "Regex POST allowed ", + method: "POST", + url: "/skip/auth/regex", + allowed: true, + }, + { + name: "Regex denied", + method: "GET", + url: "/wrong/denied", + allowed: false, + }, + { + name: "Route allowed", + method: "GET", + url: "/skip/auth/routes/get", + allowed: true, + }, + { + name: "Route denied with wrong method", + method: "PATCH", + url: "/skip/auth/routes/get", + allowed: false, + }, + { + name: "Route denied with wrong path", + method: "GET", + 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 TestProxyAllowedGroups(t *testing.T) { tests := []struct { name string @@ -2265,18 +2515,18 @@ func TestProxyAllowedGroups(t *testing.T) { CreatedAt: &created, } - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) })) - t.Cleanup(upstream.Close) + t.Cleanup(upstreamServer.Close) test, err := NewProcessCookieTestWithOptionsModifiers(func(opts *options.Options) { opts.AllowedGroups = tt.allowedGroups opts.UpstreamServers = options.Upstreams{ { - ID: upstream.URL, + ID: upstreamServer.URL, Path: "/", - URI: upstream.URL, + URI: upstreamServer.URL, }, } }) @@ -2287,7 +2537,8 @@ func TestProxyAllowedGroups(t *testing.T) { test.req, _ = http.NewRequest("GET", "/", nil) test.req.Header.Add("accept", applicationJSON) - test.SaveSession(session) + err = test.SaveSession(session) + assert.NoError(t, err) test.proxy.ServeHTTP(test.rw, test.req) if tt.expectUnauthorized { diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index bcb600e9..a79d1520 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -3,7 +3,6 @@ package options import ( "crypto" "net/url" - "regexp" oidc "github.com/coreos/go-oidc" ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" @@ -67,6 +66,7 @@ type Options struct { UpstreamServers Upstreams `cfg:",internal"` SkipAuthRegex []string `flag:"skip-auth-regex" cfg:"skip_auth_regex"` + SkipAuthRoutes []string `flag:"skip-auth-route" cfg:"skip_auth_routes"` 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"` @@ -114,7 +114,6 @@ type Options struct { // internal values that are set after config validation redirectURL *url.URL - compiledRegex []*regexp.Regexp provider providers.Provider signatureData *SignatureData oidcVerifier *oidc.IDTokenVerifier @@ -124,7 +123,6 @@ type Options struct { // Options for Getting internal values func (o *Options) GetRedirectURL() *url.URL { return o.redirectURL } -func (o *Options) GetCompiledRegex() []*regexp.Regexp { return o.compiledRegex } func (o *Options) GetProvider() providers.Provider { return o.provider } func (o *Options) GetSignatureData() *SignatureData { return o.signatureData } func (o *Options) GetOIDCVerifier() *oidc.IDTokenVerifier { return o.oidcVerifier } @@ -133,7 +131,6 @@ func (o *Options) GetRealClientIPParser() ipapi.RealClientIPParser { return o.re // Options for Setting internal values func (o *Options) SetRedirectURL(s *url.URL) { o.redirectURL = s } -func (o *Options) SetCompiledRegex(s []*regexp.Regexp) { o.compiledRegex = s } func (o *Options) SetProvider(s providers.Provider) { o.provider = s } func (o *Options) SetSignatureData(s *SignatureData) { o.signatureData = s } func (o *Options) SetOIDCVerifier(s *oidc.IDTokenVerifier) { o.oidcVerifier = s } @@ -195,8 +192,9 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Bool("pass-access-token", false, "pass OAuth access_token to upstream via X-Forwarded-Access-Token header") 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.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.Bool("skip-auth-strip-headers", false, "strips `X-Forwarded-*` style authentication headers & `Authorization` header if they would be set by oauth2-proxy for allowlisted requests (`--skip-auth-route`, `--skip-auth-regex`, `--skip-auth-preflight`, `--trusted-ip`)") 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") diff --git a/pkg/validation/allowlist.go b/pkg/validation/allowlist.go new file mode 100644 index 00000000..56a3fd4c --- /dev/null +++ b/pkg/validation/allowlist.go @@ -0,0 +1,70 @@ +package validation + +import ( + "fmt" + "os" + "regexp" + "strings" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" +) + +func validateAllowlists(o *options.Options) []string { + msgs := []string{} + + msgs = append(msgs, validateRoutes(o)...) + msgs = append(msgs, validateRegexes(o)...) + msgs = append(msgs, validateTrustedIPs(o)...) + + if len(o.TrustedIPs) > 0 && o.ReverseProxy { + _, err := fmt.Fprintln(os.Stderr, "WARNING: mixing --trusted-ip with --reverse-proxy is a potential security vulnerability. An attacker can inject a trusted IP into an X-Real-IP or X-Forwarded-For header if they aren't properly protected outside of oauth2-proxy") + if err != nil { + panic(err) + } + } + + return msgs +} + +// validateRoutes validates method=path routes passed with options.SkipAuthRoutes +func validateRoutes(o *options.Options) []string { + msgs := []string{} + for _, route := range o.SkipAuthRoutes { + var regex string + parts := strings.SplitN(route, "=", 2) + if len(parts) == 1 { + regex = parts[0] + } else { + regex = parts[1] + } + _, err := regexp.Compile(regex) + if err != nil { + msgs = append(msgs, fmt.Sprintf("error compiling regex /%s/: %v", regex, err)) + } + } + return msgs +} + +// validateRegex validates regex paths passed with options.SkipAuthRegex +func validateRegexes(o *options.Options) []string { + msgs := []string{} + for _, regex := range o.SkipAuthRegex { + _, err := regexp.Compile(regex) + if err != nil { + msgs = append(msgs, fmt.Sprintf("error compiling regex /%s/: %v", regex, err)) + } + } + return msgs +} + +// validateTrustedIPs validates IP/CIDRs for IP based allowlists +func validateTrustedIPs(o *options.Options) []string { + msgs := []string{} + for i, ipStr := range o.TrustedIPs { + if nil == ip.ParseIPNet(ipStr) { + msgs = append(msgs, fmt.Sprintf("trusted_ips[%d] (%s) could not be recognized", i, ipStr)) + } + } + return msgs +} diff --git a/pkg/validation/allowlist_test.go b/pkg/validation/allowlist_test.go new file mode 100644 index 00000000..4600a718 --- /dev/null +++ b/pkg/validation/allowlist_test.go @@ -0,0 +1,125 @@ +package validation + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" +) + +var _ = Describe("Allowlist", func() { + type validateRoutesTableInput struct { + routes []string + errStrings []string + } + + type validateRegexesTableInput struct { + regexes []string + errStrings []string + } + + type validateTrustedIPsTableInput struct { + trustedIPs []string + errStrings []string + } + + DescribeTable("validateRoutes", + func(r *validateRoutesTableInput) { + opts := &options.Options{ + SkipAuthRoutes: r.routes, + } + Expect(validateRoutes(opts)).To(ConsistOf(r.errStrings)) + }, + Entry("Valid regex routes", &validateRoutesTableInput{ + routes: []string{ + "/foo", + "POST=/foo/bar", + "PUT=^/foo/bar$", + "DELETE=/crazy/(?:regex)?/[^/]+/stuff$", + }, + errStrings: []string{}, + }), + Entry("Bad regexes do not compile", &validateRoutesTableInput{ + routes: []string{ + "POST=/(foo", + "OPTIONS=/foo/bar)", + "GET=^]/foo/bar[$", + "GET=^]/foo/bar[$", + }, + errStrings: []string{ + "error compiling regex //(foo/: error parsing regexp: missing closing ): `/(foo`", + "error compiling regex //foo/bar)/: error parsing regexp: unexpected ): `/foo/bar)`", + "error compiling regex /^]/foo/bar[$/: error parsing regexp: missing closing ]: `[$`", + "error compiling regex /^]/foo/bar[$/: error parsing regexp: missing closing ]: `[$`", + }, + }), + ) + + DescribeTable("validateRegexes", + func(r *validateRegexesTableInput) { + opts := &options.Options{ + SkipAuthRegex: r.regexes, + } + Expect(validateRegexes(opts)).To(ConsistOf(r.errStrings)) + }, + Entry("Valid regex routes", &validateRegexesTableInput{ + regexes: []string{ + "/foo", + "/foo/bar", + "^/foo/bar$", + "/crazy/(?:regex)?/[^/]+/stuff$", + }, + errStrings: []string{}, + }), + Entry("Bad regexes do not compile", &validateRegexesTableInput{ + regexes: []string{ + "/(foo", + "/foo/bar)", + "^]/foo/bar[$", + "^]/foo/bar[$", + }, + errStrings: []string{ + "error compiling regex //(foo/: error parsing regexp: missing closing ): `/(foo`", + "error compiling regex //foo/bar)/: error parsing regexp: unexpected ): `/foo/bar)`", + "error compiling regex /^]/foo/bar[$/: error parsing regexp: missing closing ]: `[$`", + "error compiling regex /^]/foo/bar[$/: error parsing regexp: missing closing ]: `[$`", + }, + }), + ) + + DescribeTable("validateTrustedIPs", + func(t *validateTrustedIPsTableInput) { + opts := &options.Options{ + TrustedIPs: t.trustedIPs, + } + Expect(validateTrustedIPs(opts)).To(ConsistOf(t.errStrings)) + }, + Entry("Non-overlapping valid IPs", &validateTrustedIPsTableInput{ + trustedIPs: []string{ + "127.0.0.1", + "10.32.0.1/32", + "43.36.201.0/24", + "::1", + "2a12:105:ee7:9234:0:0:0:0/64", + }, + errStrings: []string{}, + }), + Entry("Overlapping valid IPs", &validateTrustedIPsTableInput{ + trustedIPs: []string{ + "135.180.78.199", + "135.180.78.199/32", + "d910:a5a1:16f8:ddf5:e5b9:5cef:a65e:41f4", + "d910:a5a1:16f8:ddf5:e5b9:5cef:a65e:41f4/128", + }, + errStrings: []string{}, + }), + Entry("Invalid IPs", &validateTrustedIPsTableInput{ + trustedIPs: []string{"[::1]", "alkwlkbn/32"}, + errStrings: []string{ + "trusted_ips[0] ([::1]) could not be recognized", + "trusted_ips[1] (alkwlkbn/32) could not be recognized", + }, + }), + ) +}) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 12631eb9..2ea35f9f 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -9,7 +9,6 @@ import ( "net/http" "net/url" "os" - "regexp" "strings" "github.com/coreos/go-oidc" @@ -184,15 +183,6 @@ func Validate(o *options.Options) error { o.SetRedirectURL(redirectURL) msgs = append(msgs, validateUpstreams(o.UpstreamServers)...) - - for _, u := range o.SkipAuthRegex { - compiledRegex, err := regexp.Compile(u) - if err != nil { - msgs = append(msgs, fmt.Sprintf("error compiling regex=%q %s", u, err)) - continue - } - o.SetCompiledRegex(append(o.GetCompiledRegex(), compiledRegex)) - } msgs = parseProviderInfo(o, msgs) if len(o.GoogleGroups) > 0 || o.GoogleAdminEmail != "" || o.GoogleServiceAccountJSON != "" { @@ -223,18 +213,8 @@ func Validate(o *options.Options) error { }) } - if len(o.TrustedIPs) > 0 && o.ReverseProxy { - _, err := fmt.Fprintln(os.Stderr, "WARNING: trusting of IPs with --reverse-proxy poses risks if a header spoofing attack is possible.") - if err != nil { - panic(err) - } - } - - for i, ipStr := range o.TrustedIPs { - if nil == ip.ParseIPNet(ipStr) { - msgs = append(msgs, fmt.Sprintf("trusted_ips[%d] (%s) could not be recognized", i, ipStr)) - } - } + // Do this after ReverseProxy validation for TrustedIP coordinated checks + msgs = append(msgs, validateAllowlists(o)...) if len(msgs) != 0 { return fmt.Errorf("invalid configuration:\n %s", diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 1f418f82..f88ef7af 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -2,7 +2,6 @@ package validation import ( "crypto" - "errors" "io/ioutil" "net/url" "os" @@ -78,12 +77,19 @@ func TestClientSecretFileOption(t *testing.T) { if err != nil { t.Fatalf("failed to create temp file: %v", err) } - f.WriteString("testcase") + _, err = f.WriteString("testcase") + if err != nil { + t.Fatalf("failed to write to temp file: %v", err) + } if err := f.Close(); err != nil { t.Fatalf("failed to close temp file: %v", err) } clientSecretFileName := f.Name() - defer os.Remove(clientSecretFileName) + defer func(t *testing.T) { + if err := os.Remove(clientSecretFileName); err != nil { + t.Fatalf("failed to delete temp file: %v", err) + } + }(t) o := options.NewOptions() o.Cookie.Secret = cookieSecret @@ -144,41 +150,6 @@ func TestRedirectURL(t *testing.T) { assert.Equal(t, expected, o.GetRedirectURL()) } -func TestCompiledRegex(t *testing.T) { - o := testOptions() - regexps := []string{"/foo/.*", "/ba[rz]/quux"} - o.SkipAuthRegex = regexps - assert.Equal(t, nil, Validate(o)) - actual := make([]string, 0) - for _, regex := range o.GetCompiledRegex() { - actual = append(actual, regex.String()) - } - assert.Equal(t, regexps, actual) -} - -func TestCompiledRegexError(t *testing.T) { - o := testOptions() - o.SkipAuthRegex = []string{"(foobaz", "barquux)"} - err := Validate(o) - assert.NotEqual(t, nil, err) - - expected := errorMsg([]string{ - "error compiling regex=\"(foobaz\" error parsing regexp: " + - "missing closing ): `(foobaz`", - "error compiling regex=\"barquux)\" error parsing regexp: " + - "unexpected ): `barquux)`"}) - assert.Equal(t, expected, err.Error()) - - o.SkipAuthRegex = []string{"foobaz", "barquux)"} - err = Validate(o) - assert.NotEqual(t, nil, err) - - expected = errorMsg([]string{ - "error compiling regex=\"barquux)\" error parsing regexp: " + - "unexpected ): `barquux)`"}) - assert.Equal(t, expected, err.Error()) -} - func TestDefaultProviderApiSettings(t *testing.T) { o := testOptions() assert.Equal(t, nil, Validate(o)) @@ -337,45 +308,6 @@ func TestRealClientIPHeader(t *testing.T) { assert.Nil(t, o.GetRealClientIPParser()) } -func TestIPCIDRSetOption(t *testing.T) { - tests := []struct { - name string - trustedIPs []string - err error - }{ - { - "TestSomeIPs", - []string{"127.0.0.1", "10.32.0.1/32", "43.36.201.0/24", "::1", "2a12:105:ee7:9234:0:0:0:0/64"}, - nil, - }, { - "TestOverlappingIPs", - []string{"135.180.78.199", "135.180.78.199/32", "d910:a5a1:16f8:ddf5:e5b9:5cef:a65e:41f4", "d910:a5a1:16f8:ddf5:e5b9:5cef:a65e:41f4/128"}, - nil, - }, { - "TestInvalidIPs", - []string{"[::1]", "alkwlkbn/32"}, - errors.New( - "invalid configuration:\n" + - " trusted_ips[0] ([::1]) could not be recognized\n" + - " trusted_ips[1] (alkwlkbn/32) could not be recognized", - ), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - o := testOptions() - o.TrustedIPs = tt.trustedIPs - err := Validate(o) - if tt.err == nil { - assert.Nil(t, err) - } else { - assert.Equal(t, tt.err.Error(), err.Error()) - } - }) - } -} - func TestProviderCAFilesError(t *testing.T) { file, err := ioutil.TempFile("", "absent.*.crt") assert.NoError(t, err)