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

Support for GCP Service Account as JSON or Path in Default Application Credentials #953

Merged
merged 5 commits into from
May 6, 2022
Merged

Conversation

joshkaplinsky
Copy link
Contributor

@joshkaplinsky joshkaplinsky commented Oct 27, 2021

There are many use cases where systems require mounting a GCP Service Account JSON as an environment variable directly - and cannot support mounting a file and using the default application credentials as a path to the mounted file.

This is common in many CICD pipelines, where secrets are managed by the system but passed to Docker Containers as environment variables.

These changes support a custom lookup to GOOGLE_APPLICATION_CREDENTIALS and will run os.Stat() to check if the environment variable resolves as a path to a file.

  • If err == nil - the content of GOOGLE_APPLICATION_CREDENTIALS is a path to a file
    • Will use ioutil.ReadFile and return the contents of the file as []byte or an error (if one occurs)
  • if err != nil - an error is dropped, and will use the content of the os.GetEnv("GOOGLE_APPLICATION_CREDENTIALS"), converting the returned string to []byte
    • If the environment variable is not present, an empty string would be returned - and that error would be handled when trying to create the client.

Changes in creating the client, remove the deprecated cloudkms.New() in favor of cloudkms.NewService() this allows for passing an optional client.Option - in the changes here - it passes option.WithCredentialsJSON() and the returned creds as []byte from getDefaultApplicationCredentials() described above.

This resolves issue: #681

gcpkms/keysource.go Outdated Show resolved Hide resolved
@joshkaplinsky joshkaplinsky marked this pull request as ready for review October 27, 2021 17:28
@multani
Copy link

multani commented Dec 17, 2021

@jkapdev Would it make sense to use another dedicated variable instead of piggy backing on the variable from the Google SDK?

Something like GOOGLE_CREDENTIALS.

This would make the behavior less surprising and would make it more compatible with other systems which are doing the same (cough terraform cough)

@joshkaplinsky
Copy link
Contributor Author

This would make the behavior less surprising and would make it more compatible with other systems which are doing the same (cough terraform cough)

Seems like that would kill 2 birds with 1 stone. Will get that updated when I have a chance!

@joshkaplinsky
Copy link
Contributor Author

Sorry to bug @autrilla - anyway we can get some eyes on this PR? Or get the community to help out in reviewing some changes to sops, moving forward?

@MikeSchlosser16
Copy link

We are also looking for this functionality

@eakman-nomad
Copy link

Any news on this? We'd also like to see this PR advance.

@ajvb
Copy link
Contributor

ajvb commented Apr 25, 2022

@joshkaplinsky Can you merge the current develop into this branch so that tests are re-ran?

@joshkaplinsky
Copy link
Contributor Author

@ajvb Has been merged - let me know if there's anything else I need to do.

@ajvb ajvb added this to the v3.7.3 milestone May 2, 2022
@ajvb ajvb added priority/low Low priority issues keyservice/gcpkms labels May 2, 2022
// getDefaultApplicationCredentials allows for passing GCP Service Account
// Credentials as either a path to a file, or directly as an environment variable
// in JSON format.
func getDefaultApplicationCredentials() (token []byte, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use this when https://pkg.go.dev/golang.org/x/oauth2/google#FindDefaultCredentials exists? Shouldn't we check if it returns an error first and then do this custom implementation?

Copy link

Choose a reason for hiding this comment

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

It would be worth:

So that we have the special use-case when GOOGLE_CREDENTIALS is set, otherwise sops fallback to the default behavior of the SDK, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, though I'd suggest that we reverse the order. It's good to stay as close to the common cloud provider SDK's as possible and this would ensure we don't break any backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

I think @multani's suggestion already is backwards compatible. Given it is not a standard environment variable at the moment, there is some intent for it to be used.

A regular user would not have this set, while by changing it around, it would not be taken into account on GCP instances where the right environment default values are automatically inject at times. Which makes FindDefaultCredentials always work, and GOOGLE_CREDENTIALS unreachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @multani's thoughts here. I'll get the the call to FindDefaultCredentials integrated. Seems like it would be better to support both cases GOOGLE_CREDENTIALS and GOOGLE_APPLICATION_CREDENTIALS to integrate with a wider set of implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok sounds good.

// Credentials as either a path to a file, or directly as an environment variable
// in JSON format.
func getDefaultApplicationCredentials() (token []byte, err error) {
var defaultCredentials = os.Getenv("GOOGLE_CREDENTIALS")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be GOOGLE_APPLICATION_CREDENTIALS?

Copy link

@multani multani May 2, 2022

Choose a reason for hiding this comment

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

(responding on behalf of Josh, because I'm also very interested to have this change released and used by the Terraform sops provider))

One of the goal of this pull request is to be able to read credentials directly out of an environment variable; GOOGLE_APPLICATION_CREDENTIALS contains the path to a file containing the credentials, not the credentials itself.

This is not supported natively by the Google SDK (I can find the related issues in their repository, if needed), but other consumers may provide this, for example the Terraform Google provider. Being able to read the credentials directly out of an environment variable would open up the possibility to use sops in context where it's not possible to read the credentials otherwise.

Hence, the deliberate use of this GOOGLE_CREDENTIALS environment variable, like the Google provider for Terraform does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agreed, thank you @multani.

Apologies, I misread your original comment. I've updated these changes to support using either GOOGLE_CREDENTIALS or GOOGLE_APPLICATION_CREDENTIALS such that if a key or path isn't provided in GOOGLE_CREDENTIALS it will fallback to the expected behavior from cloudkms.NewService() -- which uses the default application credentials automatically.

Comment on lines +138 to +139
} else if len(credentials) > 0 {
options = append(options, option.WithCredentialsJSON(credentials))
Copy link
Contributor Author

@joshkaplinsky joshkaplinsky May 3, 2022

Choose a reason for hiding this comment

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

Setting client option only if a credential is returned from GOOGLE_CREDENTIALS, either reading from a file or the service account is set directly as the value of the environment variable.

Otherwise, fallback to using default behavior as defined in the cloudkms.NewService() - this will use the default application credentials.

@joshkaplinsky joshkaplinsky requested a review from ajvb May 3, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants