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

Enable standard forms of GCP auth for oci sources #815

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

Conversation

thejosephstevens
Copy link

@thejosephstevens thejosephstevens commented Oct 13, 2024

This is related to #311 . We use k8s-based OIDC for all of our GCP auth, which allows for us to configure workloads in clusters in a simple, templated way to get access to cloud resources without ever having to exchange secret materials. This has some fairly straightforward security benefits, in addition to removing operational concerns of secret distribution, rotation, invalidation, etc...

The updated code piggybacks on the DefaultTokenSource method from the google auth package in order to support all standard methods of obtaining an auth token, in addition to the previously supported method requiring the well-known metadata endpoint to be available (only applicable in GCE and GKE clusters with Workload Identity enabled).

Note - This technically constitutes a breaking change for auth behavior, but it should be very uncommon for it to negatively impact users as they would have had to set up Flux with some form of ambient auth, and leave it in place even though it's not taking effect and they're falling back to metadata endpoint auth. If, for example, someone has a flux workload with GOOGLE_APPLICATION_CREDENTIALS configured, with the existing code that would be ignored, but with this change in place their auth behavior would change. I'm happy to write up full details on what scenarios would be impacted by this for release notes, where would be best for me to put that?

Edit:
I just wanted to note that I was able to create a build of source-controller based off of this pkg/oci revision and it appears to work fine. I'm not seeing any issues introduced, and I was able to pull a GCP Artifact Registry OCI repo with k8s-based OIDC from an EKS cluster.

@stefanprodan
Copy link
Member

Hey @thejosephstevens can you signoff your commits so we can run the tests. See https://github.com/fluxcd/pkg/pull/815/checks?check_run_id=31453839284

@stefanprodan stefanprodan added the area/oci OCI related issues and pull requests label Oct 14, 2024
@thejosephstevens thejosephstevens force-pushed the support-standard-gcp-oci-auth-methods branch from f6308e6 to 1ce9cc4 Compare October 14, 2024 18:44
@thejosephstevens
Copy link
Author

Hey @thejosephstevens can you signoff your commits so we can run the tests. See https://github.com/fluxcd/pkg/pull/815/checks?check_run_id=31453839284

Done!

@thejosephstevens thejosephstevens force-pushed the support-standard-gcp-oci-auth-methods branch from 1ce9cc4 to ded413e Compare October 14, 2024 19:33
@stefanprodan
Copy link
Member

@thejosephstevens please run make tidy and force push.

@thejosephstevens thejosephstevens force-pushed the support-standard-gcp-oci-auth-methods branch from ded413e to cb4a6bc Compare October 14, 2024 20:43
@stefanprodan
Copy link
Member

@darkowlzz can you fork this and run our GCP e2e tests please.

@stefanprodan
Copy link
Member

@thejosephstevens could you please run make test-oci and fix the failing ones.

@thejosephstevens thejosephstevens force-pushed the support-standard-gcp-oci-auth-methods branch from 590d595 to d48b493 Compare October 16, 2024 01:13
@thejosephstevens
Copy link
Author

@thejosephstevens could you please run make test-oci and fix the failing ones.

Just got them all working!

@matheuscscp
Copy link
Contributor

Hi @thejosephstevens, thanks for all the work here! 🏅

One question:

pull a GCP Artifact Registry OCI repo with k8s-based OIDC from an EKS cluster

Here, are you specifically referring to GCP Workload Identity Federation?

@@ -64,69 +63,57 @@ func WithProxyURL(proxyURL *url.URL) Option {
}
}

// WithTokenSource sets a custom token source for the client.
func (c *Client) WithTokenSource(ts oauth2.TokenSource) *Client {
Copy link
Contributor

@matheuscscp matheuscscp Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for this option?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is primarily to enable passing in a stub for testing. In general use I'm not seeing a reason to use anything other than the default resolve for fetching the token source (that keeps the behavior aligned with conventional GCP client behavior that people will expect)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since our tests use the same package gcp as the main code, we don't need to expose an exported API for specifying this mock/stub token source, you can directly assign a value to the unexported field c.tokenSource in the tests. I think I prefer it that way, so we don't introduce a new API without a concrete use case for it besides tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we have a very similar function WithTokenURL() here, so I assume you just followed the same style 👍

But since we're changing this, I think I prefer not introducing this API publicly.

@darkowlzz Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we have a very similar function WithTokenURL() here, so I assume you just followed the same style 👍

Pretty much, yeah. I think the argument to not make that API public without a use case makes sense, the issue I'm running into is that if it's not on a public method I can't override the private field directly from tests that aren't in the same package (specifically oci/auth/login/login_test). There's a couple options that come to mind:

  • I can create a distinct function in oci/auth/gcp/auth_test to return a client with a passed-in token source (functionally not very different, but it can have a super clear name like NewClientWithStubTokenSource)
  • I can implement the token source override as an Option like WithProxyUrl (again, not terrifically different in functionality, but that maybe makes it less confusing)

I don't see another way around this without refactoring the login and oci packages, which seems like it would be a lot to add to this PR. If you've got another idea I'd be happy to give it a shot, my go is a tad rusty, so there may be something I'm not thinking of.

Copy link
Contributor

@matheuscscp matheuscscp Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue I'm running into is that if it's not on a public method I can't override the private field directly from tests that aren't in the same package (specifically oci/auth/login/login_test).

Fair enough! (Sorry I didn't see you were using it in tests outside the package.)

@thejosephstevens
Copy link
Author

Hi @thejosephstevens, thanks for all the work here! 🏅

One question:

pull a GCP Artifact Registry OCI repo with k8s-based OIDC from an EKS cluster

Here, are you specifically referring to GCP Workload Identity Federation?

I am! We generate cross-cloud identities for our workloads backed by k8s OIDC through Workload Identity Federation (or equivalent) in each cloud. We just happen to run our core operations primarily in GCP, which is why I jumped in here to contribute.

At a super specific level, we use GOOGLE_APPLICATION_CREDENTIALS to point to an external_account json file (mounted by configmap) which maps to the GCP identity, and attach a service account token with the appropriate audience for that GCP Workload Identity Pool Provider.

@thejosephstevens thejosephstevens force-pushed the support-standard-gcp-oci-auth-methods branch from d48b493 to d1c9d10 Compare October 18, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oci OCI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants