Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow Workload Identity Federation for Google connector #3170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

midu-git
Copy link

Overview

  • A new Google connector option have been introduced, i.e.,
    serviceAccountToImpersonate. If this field is non-empty, it is
    assumed that Workload Identity Federation shall be used, and the
    linked service account needs to be configured for domain-wide
    delegation. Moreover, the service account used for Workload Identity
    Federation must include Service Account Token Creator for this
    service account.
  • Print some warnings if the configuration is not consistent or erroneous.
  • Fix fetching groups to rely on groups as scope. In the case groups
    is specified as a scope, the oauth authentication call will fail as
    Google doesn't support it. Moreover, as fetching groups requires the
    group directory service, it is enough to assume the existence of this
    service as a prerequisite for the fetch. If 'groups' is specified as a
    scope, a warning is printed, instead of erroring out, for backwards
    compatibility reasons.
  • When specifying groups in the configuration, but no group directory
    service will be created, a warning is printed that the groups
    configuration will be ignored.

What this PR does / why we need it

The code doesn't support Workload Identity Federation so far. Workload Identity Federation enables interaction with Google services without requiring to provide any sensitive credential data on the caller side, c.f. here.

Special notes for your reviewer

Does this PR introduce a user-facing change?

* A new Google connector option have been introduced, i.e.,
  "serviceAccountToImpersonate". If this field is non-empty, it is
  assumed that Workload Identity Federation shall be used, and the
  linked service account needs to be configured for domain-wide
  delegation. Moreover, the service account used for Workload Identity
  Federation must include "Service Account Token Creator" for this
  service account.
* Print some warnings if the configuration is not consistent or erroneous.
* Fix fetching groups to rely on "groups" as scope. In the case "groups"
  is specified as a scope, the oauth authentication call will fail as
  Google doesn't support it. Moreover, as fetching groups requires the
  group directory service, it is enough to assume the existence of this
  service as a prerequisite for the fetch. If "groups" is specified as a
  scope, a warning is printed, instead of erroring out, for backwards
  compatibility reasons.
* When specifying "groups" in the configuration, but no group directory
  service will be created, a warning is printed that the groups
  configuration will be ignored.

@midu-git midu-git force-pushed the CLOUD-394 branch 2 times, most recently from be1f1b0 to 113be40 Compare October 26, 2023 05:21
* A new Google connector option have been introduced, i.e.,
  `serviceAccountToImpersonate`. If this field is non-empty, it is
  assumed that Workload Identity Federation shall be used, and the
  linked service account needs to be configured for domain-wide
  delegation. Moreover, the service account used for Workload Identity
  Federation must include `Service Account Token Creator` for this
  service account.
* Print some warnings if the configuration is not consistent or erroneous.
* Fix fetching groups to rely on `groups` as scope. In the case `groups`
  is specified as a scope, the oauth authentication call will fail as
  Google doesn't support it. Moreover, as fetching groups requires the
  group directory service, it is enough to assume the existence of this
  service as a prerequisite for the fetch. If `groups` is specified as a
  scope, a warning is printed, instead of erroring out, for backwards
  compatibility reasons.
* When specifying `groups` in the configuration, but no group directory
  service will be created, a warning is printed that the groups
  configuration will be ignored.

Signed-off-by: Michael Dudzinski <[email protected]>
@midu-git
Copy link
Author

midu-git commented Nov 21, 2023

@nabokihms Can I somehow support the process of reviewing this PR?

@midu-git
Copy link
Author

midu-git commented Jan 3, 2024

@nabokihms I know that you guys are very busy, but this feature is very important for us. Therefore, can I support the progress here somehow?

@jacek-jablonski
Copy link

I've manually tested this PR and it works fine. It also fixes: #3348

It would be great if it was merged.

@Pionerd
Copy link

Pionerd commented Feb 24, 2024

Please consider using the Cloud Identity API for the groups. This great project by @jkroepke has a PR open for it and it works like a charm. It's no longer necessary to use a service account and/or domain admin email to impersonate.

jkroepke/openvpn-auth-oauth2#196

@jkroepke
Copy link

jkroepke commented Feb 24, 2024

Please consider using the Cloud Identity API for the groups.

You can use the token of the users access token with scope https://www.googleapis.com/auth/cloud-identity.groups.readonly to fetch the groups, without an additional identity.

Some credits: https://github.com/grafana/grafana/blob/110028706a160af8c66672d06554f616d660d82c/pkg/login/social/connectors/google_oauth.go#L242-L298

API docs: https://cloud.google.com/identity/docs/reference/rest/v1beta1/groups.memberships/searchDirectGroups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants