diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c44f180..d89f2d92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ - [#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 + - Either `--google-group` or the new `--allowed-group` will work for Google now (`--google-group` will be used it both are set) + - Group membership lists will be passed to the backend with the `X-Forwarded-Groups` header + - If you change the list of allowed groups, existing sessions that now don't have a valid group will be logged out immediately. + - Previously, group membership was only checked on session creation and refresh. - [#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) @@ -18,6 +23,9 @@ ## Breaking Changes - [#911](https://github.com/oauth2-proxy/oauth2-rpoxy/pull/911) Specifying a non-existent provider will cause OAuth2-Proxy to fail on startup instead of defaulting to "google". +- [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) Security changes to Google provider group authorization flow + - If you change the list of allowed groups, existing sessions that now don't have a valid group will be logged out immediately. + - Previously, group membership was only checked on session creation and refresh. - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) When a Redis session store is configured, OAuth2-Proxy will fail to start up unless connection and health checks to Redis pass - [#800](https://github.com/oauth2-proxy/oauth2-proxy/pull/800) Fix import path for v7. The import path has changed to support the go get installation. - You can now `go get github.com/oauth2-proxy/oauth2-proxy/v7` to get the latest `v7` version of OAuth2 Proxy @@ -40,6 +48,7 @@ - [#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) - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) Integrate new header injectors into project (@JoelSpeed) +- [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) Create universal Authorization behavior across providers (@NickMeves) - [#898](https://github.com/oauth2-proxy/oauth2-proxy/pull/898) Migrate documentation to Docusaurus (@JoelSpeed) - [#754](https://github.com/oauth2-proxy/oauth2-proxy/pull/754) Azure token refresh (@codablock) - [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index c06ff3be..132c5d0a 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -1133,8 +1133,6 @@ func TestUserInfoEndpointAccepted(t *testing.T) { Email: "john.doe@example.com", AccessToken: "my_access_token"} err = test.SaveSession(startSession) assert.NoError(t, err) - - return } func TestUserInfoEndpointUnauthorizedOnNoCookieSetError(t *testing.T) { diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 121fe6b4..839c2035 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -257,7 +257,13 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { if err != nil { msgs = append(msgs, "invalid Google credentials file: "+o.GoogleServiceAccountJSON) } else { - p.SetGroupRestriction(o.GoogleGroups, o.GoogleAdminEmail, file) + groups := o.AllowedGroups + // Backwards compatibility with `--google-group` option + if len(o.GoogleGroups) > 0 { + groups = o.GoogleGroups + p.SetAllowedGroups(groups) + } + p.SetGroupRestriction(groups, o.GoogleAdminEmail, file) } } case *providers.BitbucketProvider: diff --git a/providers/google.go b/providers/google.go index 97ea52d7..640f40cf 100644 --- a/providers/google.go +++ b/providers/google.go @@ -27,11 +27,14 @@ type GoogleProvider struct { *ProviderData RedeemRefreshURL *url.URL + // GroupValidator is a function that determines if the user in the passed // session is a member of any of the configured Google groups. - GroupValidator func(*sessions.SessionState, bool) bool - - allowedGroups map[string]struct{} + // + // This hits the Google API for each group, so it is called on Redeem & + // Refresh. `Authorize` uses the results of this saved in `session.Groups` + // Since it is called on every request. + GroupValidator func(*sessions.SessionState) bool } var _ Provider = (*GoogleProvider)(nil) @@ -89,7 +92,7 @@ func NewGoogleProvider(p *ProviderData) *GoogleProvider { ProviderData: p, // Set a default GroupValidator to just always return valid (true), it will // be overwritten if we configured a Google group restriction. - GroupValidator: func(*sessions.SessionState, bool) bool { + GroupValidator: func(*sessions.SessionState) bool { return true }, } @@ -172,45 +175,27 @@ func (p *GoogleProvider) Redeem(ctx context.Context, redirectURL, code string) ( Email: c.Email, User: c.Subject, } - p.GroupValidator(s, true) + p.GroupValidator(s) return s, nil } -func (p *GoogleProvider) Authorize(ctx context.Context, s *sessions.SessionState) (bool, error) { - return p.GroupValidator(s, false), nil -} - // SetGroupRestriction configures the GoogleProvider to restrict access to the // specified group(s). AdminEmail has to be an administrative email on the domain that is // checked. CredentialsFile is the path to a json file containing a Google service // account credentials. func (p *GoogleProvider) SetGroupRestriction(groups []string, adminEmail string, credentialsReader io.Reader) { adminService := getAdminService(adminEmail, credentialsReader) - for _, group := range groups { - p.allowedGroups[group] = struct{}{} - } - - p.GroupValidator = func(s *sessions.SessionState, sync bool) bool { - if sync { - // Reset our saved Groups in case membership changed - s.Groups = make([]string, 0, len(groups)) - for _, group := range groups { - if userInGroup(adminService, group, s.Email) { - s.Groups = append(s.Groups, group) - } - } - return len(s.Groups) > 0 - } - - // Don't resync with Google, handles when OAuth2-Proxy settings - // alter allowed groups but existing sessions are still valid - for _, group := range s.Groups { - if _, ok := p.allowedGroups[group]; ok { - return true + p.GroupValidator = func(s *sessions.SessionState) bool { + // Reset our saved Groups in case membership changed + // This is used by `Authorize` on every request + s.Groups = make([]string, 0, len(groups)) + for _, group := range groups { + if userInGroup(adminService, group, s.Email) { + s.Groups = append(s.Groups, group) } } - return false + return len(s.Groups) > 0 } } @@ -282,7 +267,7 @@ func (p *GoogleProvider) RefreshSessionIfNeeded(ctx context.Context, s *sessions } // re-check that the user is in the proper google group(s) - if !p.GroupValidator(s, true) { + if !p.GroupValidator(s) { return false, fmt.Errorf("%s is no longer in the group(s)", s.Email) } diff --git a/providers/google_test.go b/providers/google_test.go index 5e678f22..d2222790 100644 --- a/providers/google_test.go +++ b/providers/google_test.go @@ -10,7 +10,7 @@ import ( "net/url" "testing" - "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" admin "google.golang.org/api/admin/directory/v1" @@ -110,19 +110,19 @@ func TestGoogleProviderGetEmailAddress(t *testing.T) { assert.Equal(t, "refresh12345", session.RefreshToken) } -func TestGoogleProviderAuthorize(t *testing.T) { +func TestGoogleProviderGroupValidator(t *testing.T) { const sessionEmail = "michael.bland@gsa.gov" testCases := map[string]struct { session *sessions.SessionState - validatorFunc func(*sessions.SessionState, bool) bool + validatorFunc func(*sessions.SessionState) bool expectedAuthZ bool }{ "Email is authorized with GroupValidator": { session: &sessions.SessionState{ Email: sessionEmail, }, - validatorFunc: func(s *sessions.SessionState, _ bool) bool { + validatorFunc: func(s *sessions.SessionState) bool { return s.Email == sessionEmail }, expectedAuthZ: true, @@ -131,7 +131,7 @@ func TestGoogleProviderAuthorize(t *testing.T) { session: &sessions.SessionState{ Email: sessionEmail, }, - validatorFunc: func(s *sessions.SessionState, _ bool) bool { + validatorFunc: func(s *sessions.SessionState) bool { return s.Email != sessionEmail }, expectedAuthZ: false, @@ -151,9 +151,7 @@ func TestGoogleProviderAuthorize(t *testing.T) { if tc.validatorFunc != nil { p.GroupValidator = tc.validatorFunc } - authorized, err := p.Authorize(context.Background(), tc.session) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(authorized).To(Equal(tc.expectedAuthZ)) + g.Expect(p.GroupValidator(tc.session)).To(Equal(tc.expectedAuthZ)) }) } } diff --git a/providers/provider_data.go b/providers/provider_data.go index e446bcd6..0881a1c6 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -52,7 +52,7 @@ func (p *ProviderData) GetClientSecret() (clientSecret string, err error) { // SetAllowedGroups organizes a group list into the AllowedGroups map // to be consumed by Authorize implementations func (p *ProviderData) SetAllowedGroups(groups []string) { - p.AllowedGroups = map[string]struct{}{} + p.AllowedGroups = make(map[string]struct{}, len(groups)) for _, group := range groups { p.AllowedGroups[group] = struct{}{} }