Improved Google OIDC connector #9697

espadolini · 2022-01-07T17:22:11Z

Optional Google Cloud Identity support to fetch transitive groups (i.e. groups that a user belongs to both directly and indirectly through nested groups) for SSO.

This bumps the version of the oidc resources from v2 to v3, supporting both versions, with no change to the actual spec for the resource; the behavior of v2 connectors will stay the same, while v3 connectors will not filter the returned groups by domain name, and (depending on the permissions of the configured service account) will attempt to fetch both direct and indirect groups.

Fixes #8122.
Fixes #5521.

rosstimothy

Can you add test coverage?

Either of the following would allow you to test with out needing to talk to google:

Migrating the groupsFromGsuite* functions to an interface would allow you to mock the client
Utilizing WithEndpoint or WithGRPCConn would allow you to mock google

rosstimothy

Let a few small comments. Looks good otherwise.

lib/auth/oidc_test.go

constants.go

lib/auth/oidc_google.go

lib/auth/oidc_test.go

lib/auth/oidc_google.go

lib/auth/oidc.go

lib/auth/oidc_google.go

russjones · 2022-01-17T18:49:04Z

lib/auth/oidc_google.go

+	// actual docs for the API call are at
+	// https://cloud.google.com/identity/docs/reference/rest/v1/groups.memberships/searchTransitiveGroups .
+	err = service.Groups.Memberships.SearchTransitiveGroups("groups/-").
+		Query(fmt.Sprintf("member_key_id == '%s' && 'cloudidentity.googleapis.com/groups.discussion_forum' in labels", email)).


How does this query language work? Does it have the equivalent of parameterized queries?

I'm wondering if you can do something like SQL injection with it?

I tried looking for a "proper" way to do this, but the only library to handle this (https://github.com/google/cel-go) is apparently only for evaluating CEL expressions in text form, not for building them; I don't think it's a problem here, because the email parameter we use to format the string is the email that came as part of the OIDC claims, so it's something that Google considers to be a valid email address - which I don't believe can include any 's.

russjones · 2022-01-17T19:09:12Z

@espadolini Just to make sure I understand this correctly, this PR solves two problems.

Fetch all domain groups for a user.
Fetch transitive group membership for a user.

For (1), the issue is resolved by no longer filtering on a particular domain when we make the groups.list query. However, this does entail a change in default behavior as you will get all domains a user is part of.

For (2), from what I can tell the main issue is that to support getting transitive group membership, we have to call different API endpoints that not all users may have access to due to needing to be on a Google Workspace Enterprise plan.

Is there any other way to solve (2) or is the Enterprise plan the only way to get transitive group membership for a user?

espadolini · 2022-01-17T19:22:46Z

That's correct. The change in default behavior could be mitigated by putting the groups that don't match the domain of the user into a separate claim (foreign_groups?) - to maintain the old behavior with the new API we have to do the filtering ourselves, anyway.

I don't think there's a reasonable way to get transitive group memberships without using the intended API - anything ad-hoc that we build ourselves would be too spammy and fragile.

russjones · 2022-01-19T01:43:51Z

@espadolini For (1), let's bump the version of the connector to OIDCConnectorV3 and in this version drop the domain filter. This way anyone using the old connector will continue to get domain filtering and no unexpected behavior changes but if you upgrade you get all groups.

For (2), can we update the logic to always call service.Groups.Memberships.SearchTransitiveGroups first and if it fails, then fallback to service.Groups.List? This way we don't have to add an additional field and will still get the same functionality.

russjones · 2022-01-20T18:08:32Z

@espadolini Please add documentation the two changes in behavior for OIDCConnectorV3.

ibeckermayer · 2022-01-21T16:42:30Z

lib/auth/oidc_google.go

+				return nil, trace.Wrap(err)
+			}
+		} else {
+			credentials, err := getGoogleWorkspaceCredentials(ctx, connector, directory.AdminDirectoryGroupReadonlyScope)


Similar logic to the V2 logic below, consider parameterizing into a function.

I had tried refactoring it a bit but this is still the clearest way to write it that I've found, I'm afraid. 😢

ibeckermayer · 2022-01-21T16:46:31Z

lib/services/oidc.go

@@ -102,8 +102,8 @@ func UnmarshalOIDCConnector(bytes []byte, opts ...MarshalOption) (types.OIDCConn
 		return nil, trace.Wrap(err)
 	}
 	switch h.Version {
-	case types.V2:
-		var c types.OIDCConnectorV2
+	case types.V2, types.V3:


Perhaps this is standard practice elsewhere in the codebase, in which case ignore this comment, however if not then it may be confusing why we are using an OIDCConnectorV3 for case V2 and worthy of a clarifying comment.

I have a comment in (*OIDCConnectorV3).CheckAndSetDefaults() already, but it's probably useful to restate it here as well.

go get: upgraded cloud.google.com/go v0.60.0 => v0.100.2 go get: upgraded github.com/golang/snappy v0.0.1 => v0.0.3 go get: upgraded github.com/googleapis/gax-go/v2 v2.0.5 => v2.1.1 go get: upgraded go.opencensus.io v0.22.5 => v0.23.0 go get: upgraded golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d => v0.0.0-20211104180415-d3ed0bb246c8 go get: upgraded google.golang.org/api v0.29.0 => v0.65.0

This undoes the user account impersonation changes, and always requires an admin account again.

This also removes the extra boolean flag that was added previously.

Enterprise builds will break unless gravitational/teleport.e#385 is included.

* go get google.golang.org/api go get: upgraded cloud.google.com/go v0.60.0 => v0.100.2 go get: upgraded github.com/golang/snappy v0.0.1 => v0.0.3 go get: upgraded github.com/googleapis/gax-go/v2 v2.0.5 => v2.1.1 go get: upgraded go.opencensus.io v0.22.5 => v0.23.0 go get: upgraded golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d => v0.0.0-20211104180415-d3ed0bb246c8 go get: upgraded golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 => v0.0.0-20211216021012-1d35b9e2eb4e go get: upgraded google.golang.org/api v0.29.0 => v0.65.0 go get: upgraded google.golang.org/genproto v0.0.0-20210602131652-f16073e35f0c => v0.0.0-20220107163113-42d7afdf6368 go get: upgraded google.golang.org/protobuf v1.26.0 => v1.27.1 * Optionally fetch transitive groups in the Google OIDC connector * Refactor the google workspace parts of the OIDC code * Further refactoring This undoes the user account impersonation changes, and always requires an admin account again. * Test coverage * Address review comments * Minor refactor and name changes * Allow domain filtering, tests now bypass addGoogleWorkspaceClaims * Update `OIDCConnectorV2` to `OIDCConnectorV3` * Backwards compatibility for OIDCConnector v2 This also removes the extra boolean flag that was added previously. * Update e-ref Enterprise builds will break unless gravitational/teleport.e#387 is included.

espadolini mentioned this pull request Jan 7, 2022

Update google.golang.org/api to v0.65.0 #9608

Closed

espadolini force-pushed the espadolini/google-cloudidentity branch from a8f4359 to 751b125 Compare January 10, 2022 16:32

espadolini changed the base branch from master to espadolini/update-etcd January 10, 2022 16:33

espadolini mentioned this pull request Jan 10, 2022

Google SSO nested groups #8122

Closed

Base automatically changed from espadolini/update-etcd to master January 10, 2022 18:44

espadolini force-pushed the espadolini/google-cloudidentity branch from 751b125 to 50a0369 Compare January 10, 2022 18:50

espadolini marked this pull request as ready for review January 10, 2022 18:51

github-actions bot requested review from ibeckermayer and rosstimothy January 10, 2022 18:51

espadolini force-pushed the espadolini/google-cloudidentity branch 3 times, most recently from 9dbb4f2 to fa0e2ec Compare January 12, 2022 10:06

rosstimothy reviewed Jan 12, 2022

View reviewed changes

ibeckermayer approved these changes Jan 13, 2022

View reviewed changes

espadolini force-pushed the espadolini/google-cloudidentity branch 2 times, most recently from 52dda21 to 544c2e6 Compare January 13, 2022 18:37

espadolini mentioned this pull request Jan 14, 2022

Provide ability to pull multiple domain groups from Google Workspace (formerly gsuite) #5521

Closed

espadolini force-pushed the espadolini/google-cloudidentity branch 3 times, most recently from dce94e7 to b324fe3 Compare January 14, 2022 18:35

rosstimothy approved these changes Jan 14, 2022

View reviewed changes

espadolini force-pushed the espadolini/google-cloudidentity branch from b324fe3 to 1eb37e2 Compare January 17, 2022 15:47

espadolini added docs-required needs-product-decision labels Jan 17, 2022

espadolini force-pushed the espadolini/google-cloudidentity branch from 1eb37e2 to 2e5b33a Compare January 17, 2022 18:42

russjones reviewed Jan 17, 2022

View reviewed changes

russjones removed the needs-product-decision label Jan 19, 2022

espadolini force-pushed the espadolini/google-cloudidentity branch from fa75539 to c49bdb3 Compare January 20, 2022 11:00

rosstimothy approved these changes Jan 20, 2022

View reviewed changes

espadolini force-pushed the espadolini/google-cloudidentity branch from c49bdb3 to 2e342c9 Compare January 21, 2022 10:36

ibeckermayer approved these changes Jan 21, 2022

View reviewed changes

espadolini force-pushed the espadolini/google-cloudidentity branch 2 times, most recently from f709b35 to 2353bb6 Compare January 21, 2022 17:54

espadolini added 11 commits January 21, 2022 18:54

Optionally fetch transitive groups in the Google OIDC connector

131a98f

Refactor the google workspace parts of the OIDC code

f4a6864

Further refactoring

28e637d

This undoes the user account impersonation changes, and always requires an admin account again.

Test coverage

620a4e9

Address review comments

47f434a

Minor refactor and name changes

e2d96a8

Allow domain filtering, tests now bypass addGoogleWorkspaceClaims

ef54150

Update OIDCConnectorV2 to OIDCConnectorV3

81e534c

Backwards compatibility for OIDCConnector v2

9abac05

This also removes the extra boolean flag that was added previously.

Update e-ref

c463543

Enterprise builds will break unless gravitational/teleport.e#385 is included.

espadolini force-pushed the espadolini/google-cloudidentity branch from 2353bb6 to c463543 Compare January 21, 2022 17:55

espadolini enabled auto-merge (squash) January 21, 2022 17:55

espadolini merged commit e254076 into master Jan 21, 2022

espadolini deleted the espadolini/google-cloudidentity branch January 21, 2022 18:26

This was referenced Jan 24, 2022

Updated docs for the improved Google OIDC connector #9907

Merged

[v8] backport #9697 (improved Google OIDC) #9926

Merged

espadolini removed the docs-required label Jan 26, 2022

justinas mentioned this pull request Jan 28, 2022

[v8] Bump e-ref #9844

Closed

webvictim mentioned this pull request Mar 4, 2022

Opened in error #10856

Closed

espadolini mentioned this pull request Sep 1, 2022

Restrict Google JSON credentials to the service_account type. #16003

Merged

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Google OIDC connector #9697

Improved Google OIDC connector #9697

espadolini commented Jan 7, 2022 •

edited

Loading

rosstimothy left a comment

rosstimothy left a comment

russjones Jan 17, 2022

espadolini Jan 17, 2022

russjones commented Jan 17, 2022 •

edited

Loading

espadolini commented Jan 17, 2022

russjones commented Jan 19, 2022

russjones commented Jan 20, 2022 •

edited

Loading

ibeckermayer Jan 21, 2022

espadolini Jan 21, 2022

ibeckermayer Jan 21, 2022

espadolini Jan 21, 2022

Improved Google OIDC connector #9697

Improved Google OIDC connector #9697

Conversation

espadolini commented Jan 7, 2022 • edited Loading

rosstimothy left a comment

Choose a reason for hiding this comment

rosstimothy left a comment

Choose a reason for hiding this comment

russjones Jan 17, 2022

Choose a reason for hiding this comment

espadolini Jan 17, 2022

Choose a reason for hiding this comment

russjones commented Jan 17, 2022 • edited Loading

espadolini commented Jan 17, 2022

russjones commented Jan 19, 2022

russjones commented Jan 20, 2022 • edited Loading

ibeckermayer Jan 21, 2022

Choose a reason for hiding this comment

espadolini Jan 21, 2022

Choose a reason for hiding this comment

ibeckermayer Jan 21, 2022

Choose a reason for hiding this comment

espadolini Jan 21, 2022

Choose a reason for hiding this comment

espadolini commented Jan 7, 2022 •

edited

Loading

russjones commented Jan 17, 2022 •

edited

Loading

russjones commented Jan 20, 2022 •

edited

Loading