-
Notifications
You must be signed in to change notification settings - Fork 877
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
Conversation
@jkapdev Would it make sense to use another dedicated variable instead of piggy backing on the variable from the Google SDK? Something like 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! |
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? |
We are also looking for this functionality |
Any news on this? We'd also like to see this PR advance. |
@joshkaplinsky Can you merge the current |
@ajvb Has been merged - let me know if there's anything else I need to do. |
gcpkms/keysource.go
Outdated
// 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth:
- Checking if
GOOGLE_CREDENTIALS
is set, in which caseCredentialsFromJSON
can be called with its content - Otherwise, call
FindDefaultCredentials
and directly return its result
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good.
gcpkms/keysource.go
Outdated
// 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") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
} else if len(credentials) > 0 { | ||
options = append(options, option.WithCredentialsJSON(credentials)) |
There was a problem hiding this comment.
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.
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 runos.Stat()
to check if the environment variable resolves as a path to a file.err == nil
- the content ofGOOGLE_APPLICATION_CREDENTIALS
is a path to a fileioutil.ReadFile
and return the contents of the file as[]byte
or an error (if one occurs)err != nil
- an error is dropped, and will use the content of theos.GetEnv("GOOGLE_APPLICATION_CREDENTIALS")
, converting the returned string to[]byte
Changes in creating the client, remove the deprecated
cloudkms.New()
in favor ofcloudkms.NewService()
this allows for passing an optionalclient.Option
- in the changes here - it passesoption.WithCredentialsJSON()
and the returnedcreds
as[]byte
fromgetDefaultApplicationCredentials()
described above.This resolves issue: #681