[rbac-manager] Support workload identity #330

jace-ys · 2023-12-08T15:34:56Z

Hey folks 👋🏻

Hope you don't mind this contribution but we'd like to see theatre support workload identity in the rbac-manager instead of using service account keys. I've made the change such that if workload identity is not configured, the rbac-manager will fallback to using service account keys.

This is how we're currently using it with workload identity in our GKE cluster (after removing GOOGLE_APPLICATION_CREDENTIALS):

Same change on our fork: duffelhq#3

# Config Connector CRDs
---
apiVersion: iam.cnrm.cloud.google.com/v1beta1
kind: IAMPolicy
metadata:
  name: theatre-workload-identity-user
  annotations:
    cnrm.cloud.google.com/project-id: duffel-prod
spec:
  bindings:
  - members:
    - serviceAccount:duffel-prod.svc.id.goog[theatre-system/theatre-rbac-manager]
    role: roles/iam.workloadIdentityUser
  - members:
    - serviceAccount:theatre@duffel-prod.iam.gserviceaccount.com
    # Required so that the theatre service account can impersonate itself
    role: roles/iam.serviceAccountTokenCreator
  resourceRef:
    apiVersion: iam.cnrm.cloud.google.com/v1beta1
    kind: IAMServiceAccount
    external: projects/duffel-prod/serviceAccounts/theatre@duffel-prod.iam.gserviceaccount.com

 kubectl annotate serviceaccount theatre-rbac-manager \
    --namespace theatre-system \
    iam.gke.io/gcp-service-account=theatre@duffel-prod.iam.gserviceaccount.com

jace-ys · 2023-12-08T18:07:16Z

@vinayvinay I think you're the one left in GC that I know..

Any idea who would be best suited to review this? 😁

jace-ys · 2024-08-19T20:30:51Z

Makefile

 	go install github.com/onsi/ginkgo/ginkgo@v1.16.5
 	go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
-	go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
+	go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.17


Shamelessly stolen from #335 to get my CI tests passing.. 😬

jace-ys · 2024-08-20T09:44:57Z

@mbfisher @bogvak @0x0013 Sorry for the direct tag but wanted to get this merged and saw that you folks have been recently active on this repo!

mbfisher · 2024-08-20T09:49:01Z

@mbfisher @bogvak @0x0013 Sorry for the direct tag but wanted to get this merged and saw that you folks have been recently active on this repo!

I'm a contributor so can't help I'm afraid, Core Infra own this so I'd raise a SPOC ticket with them to get a review

jace-ys · 2024-08-20T09:49:48Z

I'm a contributor so can't help I'm afraid, Core Infra own this so I'd raise a SPOC ticket with them to get a review

Thanks for letting me know! Unfortunately I'm no longer at GC so don't think I'd be able to do that 😅

benwh · 2024-09-09T12:02:59Z

Bumping - it'd be great to get this in!

👋 @ttamimi @VSpike @ijames-gc

0x0013

@jace-ys Thanks, this looks like a valuable addition.

I'm no expert in GCP authentication flow, so I would like to ask some clarifications before I can review, mostly for my own understanding. I've left these in code comments. Apologies if the answers to these are obvious.

0x0013 · 2024-11-25T14:28:36Z

cmd/rbac-manager/main.go

+			Subject:         subject,
+		}
+
+		ts, err := impersonate.CredentialsTokenSource(ctx, config, option.WithCredentials(creds))


Is it necessary for the account to impersonate itself, instead of using the TokenSource already returned by FindDefaultCredentials?

Good question. I'm no expert in GCP auth myself and it's not very well documented or discussed much, but from reading various sources, my understanding is that the federated access token obtained from the GCE
metadata server (via workload identity) is not sufficient for acting as the subject via domain-wide delegation to call Google Workspace Admin APIs.

For delegation to work, we need to sign a JWT with the the "sub" claim set to subject - this happens implicitly through impersonation.

You can kind of see this behaviour in userTokenSource: https://github.com/googleapis/google-api-go-client/blob/af0a938ba1e01e7c2ce7f96fef1e62478f5da784/impersonate/user.go#L96-L103

Whereas you don't see this in computeSource obtained by WID: https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.24.0:google/google.go;l=270

Hope that make sense!

Also, the changes in this PR were largely inspired / adapted from dexidp/dex#2989

Some useful reading:

https://blog.salrashid.dev/articles/2021/impersonation_and_domain_delegation

https://cloud.google.com/iam/docs/workload-identity-federation#access_management

That said, looks like there have been some new developments since I first opened this PR.. 👀

It looks like it's possible to assign Workspace Admin Groups Reader role to service accounts:

https://support.google.com/a/answer/9807615?sjid=13011041076049488223-AP#:~:text=Assign%20a%20role%20to%20a%20service%20account

https://cloud.google.com/identity/docs/how-to/setup#auth-no-dwd

Which might mean that there's no need for impersonation or domain-wide delegation anymore, which would greatly simplify the auth here (just google.FindDefaultCredentials might be all that's needed) 🤔

But I think that warrants a separate PR as it's a different feature altogether..

0x0013 · 2024-11-25T14:33:00Z

cmd/rbac-manager/main.go

+			return nil, err
+		}
+
+		return directoryv1.NewService(ctx, option.WithTokenSource(ts))


I wonder if the option when using a supplied json credentials (old behavior) could also use the TokenSource from Credentials returned by FindDefaultCredentials?

If so, likely the code could be simplified a little, instead of needing to call NewService with different option for each credential type.

Yeap that's a good point! I didn't change that initially as I didn't want to make too much changes to existing code.

But you're right that we can use the same WithTokenSource option for both the new and old behaviours. That said, it's not TokenSource from the Credentials returned by FindDefaultCredentials but the TokenSource from the jwt.Config, as we need the token source to have the (domain-wide delegation) subject set.

I've updated the PR to use WithTokenSource for both code paths 👍🏻

0x0013 · 2024-12-16T16:08:05Z

@jace-ys it seems the container-builder hasn't built an image for this branch - perhaps it didn't receive the webhook at the time of your last push.

Could you generate a push event on this PR, for example, by pushing an empty commit, to see if it triggers a build? Thanks!

jace-ys · 2024-12-16T16:18:03Z

@jace-ys it seems the container-builder hasn't built an image for this branch - perhaps it didn't receive the webhook at the time of your last push.

Could you generate a push event on this PR, for example, by pushing an empty commit, to see if it triggers a build? Thanks!

Tried that but doesn't look like it triggered a build either! 🙁

0x0013 · 2024-12-16T16:58:18Z

Tried that but doesn't look like it triggered a build either! 🙁

@jace-ys thanks for trying, it looks like container-builder is still not picking it up. I assume this is because the head branch is external from GC.

I'll see if there's anything that can be done about it.

jace-ys · 2025-02-27T12:42:15Z

Hey @0x0013, any luck getting the CI check to run?

0x0013 · 2025-03-03T15:34:05Z

Hi @jace-ys ,

Thanks for reminder. It's been a bit hard to commit time to it, but we do have a fix for this issue, just awaiting proper review.

TLDR is that our container-builder, which also is (and should be) a required CI check, currently only acts on organisation-internal commits - which is good from the point of security (we don't want to run automated builds on arbitrary commits), but is obviously not ideal for accepting external contributions. So I've been trying to implement a way to manually trigger it after someone from GC evaluates the changes in the PR. Hopefully we have the reviews done this week.

0x0013 · 2025-03-05T13:38:31Z

/build container

0x0013 · 2025-03-05T14:30:54Z

Looks like it doesn't work yet, will investigate.

jace-ys · 2025-03-05T14:49:31Z

Hi @jace-ys ,

Thanks for reminder. It's been a bit hard to commit time to it, but we do have a fix for this issue, just awaiting proper review.

TLDR is that our container-builder, which also is (and should be) a required CI check, currently only acts on organisation-internal commits - which is good from the point of security (we don't want to run automated builds on arbitrary commits), but is obviously not ideal for accepting external contributions. So I've been trying to implement a way to manually trigger it after someone from GC evaluates the changes in the PR. Hopefully we have the reviews done this week.

Thanks for looking into this, appreciate your time!

0x0013 · 2025-03-10T12:38:40Z

/build container

0x0013 · 2025-03-12T05:24:50Z

/build container

0x0013 · 2025-03-13T13:05:03Z

@jace-ys looks like container-builder is finally able to build the container and also return the status to GitHub CI check.

Could you please rebase this PR to master so we can test the resulting container image?

jace-ys · 2025-03-13T15:27:37Z

@jace-ys looks like container-builder is finally able to build the container and also return the status to GitHub CI check.

Could you please rebase this PR to master so we can test the resulting container image?

Thanks for doing this! I've rebased 👍🏻

0x0013 · 2025-03-13T20:54:23Z

/build container

0x0013

Sorry this took so long to review due to the CI check issues, and thanks for addressing my previous comments.

The change looks good to me. I've tested on lab to ensure the original behavior (mounted credentials file) still works, and it seems that it does.

Thanks again for submitting the PR.

0x0013 · 2025-03-19T15:16:16Z

@jace-ys it looks like there are some merge conflicts since the dependency updates we pushed yesterday. Could I ask you to rebase once again?

jace-ys · 2025-03-20T08:56:37Z

@jace-ys it looks like there are some merge conflicts since the dependency updates we pushed yesterday. Could I ask you to rebase once again?

Awesome, thanks for helping out with this. I've rebased now.

0x0013 · 2025-03-20T15:00:40Z

/build container

0x0013

Thanks for rebasing. Looks good.

And thanks again for your contribution.

jace-ys · 2025-03-20T15:14:22Z

~~@0x0013 Side question, I noticed that the theatre image is still pointing to gcr.io when Google have deprecated Container Registry for Artifact Registry.~~

~~So wanted to know if GC have redirected all gcr.io to Artifact Registry, or should we publish our own image to Artifact Registry?~~

Edit: Nevermind, just realized the upstream image is not public

jace-ys marked this pull request as draft December 8, 2023 16:23

jace-ys marked this pull request as ready for review December 8, 2023 17:56

jace-ys mentioned this pull request Dec 11, 2023

Google: Implement groups fetch by default service account from metadata (support for GKE workload identity) dexidp/dex#2989

Merged

jace-ys force-pushed the jace/rbac-manager-workload-identity branch 3 times, most recently from b4d17a9 to 44d5970 Compare August 19, 2024 20:25

jace-ys commented Aug 19, 2024

View reviewed changes

0x0013 reviewed Nov 25, 2024

View reviewed changes

jace-ys force-pushed the jace/rbac-manager-workload-identity branch from 44d5970 to 2a18959 Compare November 30, 2024 06:30

jace-ys force-pushed the jace/rbac-manager-workload-identity branch from 2a18959 to 3cdb482 Compare December 16, 2024 16:11

jace-ys force-pushed the jace/rbac-manager-workload-identity branch from 3cdb482 to 32b96b6 Compare February 27, 2025 12:44

jace-ys force-pushed the jace/rbac-manager-workload-identity branch from 32b96b6 to efc8c4b Compare March 13, 2025 14:45

0x0013 previously approved these changes Mar 17, 2025

View reviewed changes

jace-ys dismissed 0x0013’s stale review via 32b96b6 March 20, 2025 08:48

jace-ys force-pushed the jace/rbac-manager-workload-identity branch from efc8c4b to 32b96b6 Compare March 20, 2025 08:48

jace-ys added 2 commits March 20, 2025 08:50

[rbac-manager] Support workload identity

857b5f8

Address comments

b774edb

jace-ys force-pushed the jace/rbac-manager-workload-identity branch 2 times, most recently from c131e13 to b774edb Compare March 20, 2025 08:54

Run go mod tidy

d330dca

0x0013 approved these changes Mar 20, 2025

View reviewed changes

0x0013 merged commit d5344ee into gocardless:master Mar 20, 2025
5 checks passed

jace-ys deleted the jace/rbac-manager-workload-identity branch March 20, 2025 15:53

[rbac-manager] Support workload identity #330

[rbac-manager] Support workload identity #330

Uh oh!

Conversation

jace-ys commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jace-ys commented Dec 8, 2023

Uh oh!

jace-ys Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

jace-ys commented Aug 20, 2024

Uh oh!

mbfisher commented Aug 20, 2024

Uh oh!

jace-ys commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benwh commented Sep 9, 2024

Uh oh!

0x0013 left a comment

Choose a reason for hiding this comment

Uh oh!

0x0013 Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024

Choose a reason for hiding this comment

Uh oh!

0x0013 Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jace-ys Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0013 commented Dec 16, 2024

Uh oh!

jace-ys commented Dec 16, 2024

Uh oh!

0x0013 commented Dec 16, 2024

Uh oh!

jace-ys commented Feb 27, 2025

Uh oh!

0x0013 commented Mar 3, 2025

Uh oh!

0x0013 commented Mar 5, 2025

Uh oh!

0x0013 commented Mar 5, 2025

Uh oh!

jace-ys commented Mar 5, 2025

Uh oh!

0x0013 commented Mar 10, 2025

Uh oh!

0x0013 commented Mar 12, 2025

Uh oh!

0x0013 commented Mar 13, 2025

Uh oh!

jace-ys commented Mar 13, 2025

Uh oh!

0x0013 commented Mar 13, 2025

Uh oh!

0x0013 left a comment

Choose a reason for hiding this comment

Uh oh!

jace-ys commented Dec 8, 2023 •

edited

Loading

jace-ys commented Aug 20, 2024 •

edited

Loading

jace-ys Nov 30, 2024 •

edited

Loading

jace-ys Nov 30, 2024 •

edited

Loading

jace-ys Nov 30, 2024 •

edited

Loading

jace-ys commented Mar 20, 2025 •

edited

Loading