From 23b2355f852bf66aaccc47087003a1a3f4578fe7 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 18 Oct 2020 18:14:32 -0700 Subject: [PATCH] Allow group authZ in AuthOnly endpoint via Querystring --- CHANGELOG.md | 5 ++ oauthproxy.go | 50 ++++++++++++++++++-- oauthproxy_test.go | 115 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 147 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84f54dec..ef5a77aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim` - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled +- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) `/oauth2/auth` `allowed_groups` querystring parameter can be paired with the `allowed-groups` configuration option. + - In this scenario, the user's group must be in both lists to not get a 401 response code. + - The `allowed_group` querystring parameter can be specified multiple times to support multiple groups. + - The `allowed_groups` querystring parameter can specify multiple comma delimited groups. - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication. - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped. - [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) The behavior of the Google provider Groups restriction changes with this @@ -56,6 +60,7 @@ - [#918](https://github.com/oauth2-proxy/oauth2-proxy/pull/918) Fix log header output (@JoelSpeed) - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Validate provider type on startup. - [#869](https://github.com/oauth2-proxy/oauth2-proxy/pull/869) Streamline provider interface method names and signatures (@NickMeves) +- [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) Support group authorization on `oauth2/auth` endpoint via `allowed_group` & `allowed_groups` querystring parameters (@NickMeves) - [#906](https://github.com/oauth2-proxy/oauth2-proxy/pull/906) Set up v6.1.x versioned documentation as default documentation (@JoelSpeed) - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Remove v5 legacy sessions support (@NickMeves) - [#904](https://github.com/oauth2-proxy/oauth2-proxy/pull/904) Set `skip-auth-strip-headers` to `true` by default (@NickMeves) diff --git a/oauthproxy.go b/oauthproxy.go index 693d5a7a..81152082 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -744,7 +744,7 @@ func (p *OAuthProxy) serveHTTP(rw http.ResponseWriter, req *http.Request) { case path == p.OAuthCallbackPath: p.OAuthCallback(rw, req) case path == p.AuthOnlyPath: - p.AuthenticateOnly(rw, req) + p.AuthOnly(rw, req) case path == p.UserInfoPath: p.UserInfo(rw, req) default: @@ -925,14 +925,22 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { } } -// AuthenticateOnly checks whether the user is currently logged in -func (p *OAuthProxy) AuthenticateOnly(rw http.ResponseWriter, req *http.Request) { +// AuthOnly checks whether the user is currently logged in (both authentication +// and optional authorization via `allowed_groups` querystring). +func (p *OAuthProxy) AuthOnly(rw http.ResponseWriter, req *http.Request) { session, err := p.getAuthenticatedSession(rw, req) if err != nil { http.Error(rw, "unauthorized request", http.StatusUnauthorized) return } + // Allow secondary group restrictions based on the `allowed_group` or + // `allowed_groups` querystring parameter + if !checkAllowedGroups(req, session) { + http.Error(rw, "unauthorized request", http.StatusUnauthorized) + return + } + // we are authenticated p.addHeadersForProxying(rw, req, session) p.headersChain.Then(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { @@ -1016,6 +1024,42 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R return session, nil } +func checkAllowedGroups(req *http.Request, session *sessionsapi.SessionState) bool { + allowedGroups := extractAllowedGroups(req) + if len(allowedGroups) == 0 { + return true + } + + for _, group := range session.Groups { + if _, ok := allowedGroups[group]; ok { + return true + } + } + + return false +} + +func extractAllowedGroups(req *http.Request) map[string]struct{} { + groups := map[string]struct{}{} + query := req.URL.Query() + + // multi-key singular support + if multiGroups, ok := query["allowed_group"]; ok { + for _, group := range multiGroups { + groups[group] = struct{}{} + } + } + + // single key plural comma delimited support + for _, group := range strings.Split(query.Get("allowed_groups"), ",") { + if group != "" { + groups[group] = struct{}{} + } + } + + return groups +} + // addHeadersForProxying adds the appropriate headers the request / response for proxying func (p *OAuthProxy) addHeadersForProxying(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) { if session.Email == "" { diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 866109ca..3a607b94 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -1197,18 +1197,20 @@ func TestUserInfoEndpointUnauthorizedOnNoCookieSetError(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, test.rw.Code) } -func NewAuthOnlyEndpointTest(modifiers ...OptionsModifier) (*ProcessCookieTest, error) { +func NewAuthOnlyEndpointTest(querystring string, modifiers ...OptionsModifier) (*ProcessCookieTest, error) { pcTest, err := NewProcessCookieTestWithOptionsModifiers(modifiers...) if err != nil { return nil, err } - pcTest.req, _ = http.NewRequest("GET", - pcTest.opts.ProxyPrefix+"/auth", nil) + pcTest.req, _ = http.NewRequest( + "GET", + fmt.Sprintf("%s/auth%s", pcTest.opts.ProxyPrefix, querystring), + nil) return pcTest, nil } func TestAuthOnlyEndpointAccepted(t *testing.T) { - test, err := NewAuthOnlyEndpointTest() + test, err := NewAuthOnlyEndpointTest("") if err != nil { t.Fatal(err) } @@ -1226,7 +1228,7 @@ func TestAuthOnlyEndpointAccepted(t *testing.T) { } func TestAuthOnlyEndpointUnauthorizedOnNoCookieSetError(t *testing.T) { - test, err := NewAuthOnlyEndpointTest() + test, err := NewAuthOnlyEndpointTest("") if err != nil { t.Fatal(err) } @@ -1238,7 +1240,7 @@ func TestAuthOnlyEndpointUnauthorizedOnNoCookieSetError(t *testing.T) { } func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) { - test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) { + test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) { opts.Cookie.Expire = time.Duration(24) * time.Hour }) if err != nil { @@ -1258,7 +1260,7 @@ func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) { } func TestAuthOnlyEndpointUnauthorizedOnEmailValidationFailure(t *testing.T) { - test, err := NewAuthOnlyEndpointTest() + test, err := NewAuthOnlyEndpointTest("") if err != nil { t.Fatal(err) } @@ -1960,7 +1962,7 @@ func TestGetJwtSession(t *testing.T) { verifier := oidc.NewVerifier("https://issuer.example.com", keyset, &oidc.Config{ClientID: "https://test.myapp.com", SkipExpiryCheck: true}) - test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) { + test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) { opts.InjectRequestHeaders = []options.Header{ { Name: "Authorization", @@ -2028,7 +2030,6 @@ func TestGetJwtSession(t *testing.T) { }, }, } - opts.SkipJwtBearerTokens = true opts.SetJWTBearerVerifiers(append(opts.GetJWTBearerVerifiers(), verifier)) }) @@ -2692,32 +2693,106 @@ func TestProxyAllowedGroups(t *testing.T) { } func TestAuthOnlyAllowedGroups(t *testing.T) { - tests := []struct { + testCases := []struct { name string allowedGroups []string groups []string + querystring string expectUnauthorized bool }{ - {"NoAllowedGroups", []string{}, []string{}, false}, - {"NoAllowedGroupsUserHasGroups", []string{}, []string{"a", "b"}, false}, - {"UserInAllowedGroup", []string{"a"}, []string{"a", "b"}, false}, - {"UserNotInAllowedGroup", []string{"a"}, []string{"c"}, true}, + { + name: "NoAllowedGroups", + allowedGroups: []string{}, + groups: []string{}, + querystring: "", + expectUnauthorized: false, + }, + { + name: "NoAllowedGroupsUserHasGroups", + allowedGroups: []string{}, + groups: []string{"a", "b"}, + querystring: "", + expectUnauthorized: false, + }, + { + name: "UserInAllowedGroup", + allowedGroups: []string{"a"}, + groups: []string{"a", "b"}, + querystring: "", + expectUnauthorized: false, + }, + { + name: "UserNotInAllowedGroup", + allowedGroups: []string{"a"}, + groups: []string{"c"}, + querystring: "", + expectUnauthorized: true, + }, + { + name: "UserInQuerystringGroup", + allowedGroups: []string{"a", "b"}, + groups: []string{"a", "c"}, + querystring: "?allowed_group=a", + expectUnauthorized: false, + }, + { + name: "UserInOnlyQuerystringGroup", + allowedGroups: []string{}, + groups: []string{"a", "c"}, + querystring: "?allowed_groups=a,b", + expectUnauthorized: false, + }, + { + name: "UserInMultiParamQuerystringGroup", + allowedGroups: []string{"a", "b"}, + groups: []string{"b"}, + querystring: "?allowed_group=a&allowed_group=b", + expectUnauthorized: false, + }, + { + name: "UserInDelimitedQuerystringGroup", + allowedGroups: []string{"a", "b", "c"}, + groups: []string{"c"}, + querystring: "?allowed_groups=a,c", + expectUnauthorized: false, + }, + { + name: "UserNotInQuerystringGroup", + allowedGroups: []string{}, + groups: []string{"c"}, + querystring: "?allowed_group=a&allowed_group=b", + expectUnauthorized: true, + }, + { + name: "UserInConfigGroupNotInQuerystringGroup", + allowedGroups: []string{"a", "b", "c"}, + groups: []string{"c"}, + querystring: "?allowed_group=a&allowed_group=b", + expectUnauthorized: true, + }, + { + name: "UserInQuerystringGroupNotInConfigGroup", + allowedGroups: []string{"a", "b"}, + groups: []string{"c"}, + querystring: "?allowed_groups=b,c", + expectUnauthorized: true, + }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { emailAddress := "test" created := time.Now() session := &sessions.SessionState{ - Groups: tt.groups, + Groups: tc.groups, Email: emailAddress, AccessToken: "oauth_token", CreatedAt: &created, } - test, err := NewAuthOnlyEndpointTest(func(opts *options.Options) { - opts.AllowedGroups = tt.allowedGroups + test, err := NewAuthOnlyEndpointTest(tc.querystring, func(opts *options.Options) { + opts.AllowedGroups = tc.allowedGroups }) if err != nil { t.Fatal(err) @@ -2728,7 +2803,7 @@ func TestAuthOnlyAllowedGroups(t *testing.T) { test.proxy.ServeHTTP(test.rw, test.req) - if tt.expectUnauthorized { + if tc.expectUnauthorized { assert.Equal(t, http.StatusUnauthorized, test.rw.Code) } else { assert.Equal(t, http.StatusAccepted, test.rw.Code)