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.

Sign up for GitHub

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

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea8b3bb
support gcp credentials as env var
joshkaplinsky Oct 27, 2021
c0dc484
Update keysource.go
joshkaplinsky Jan 18, 2022
07aea97
Merge remote-tracking branch 'upstream/develop' into develop
joshkaplinsky Apr 25, 2022
4ffb54c
Use custom GOOGLE_CREDENTIALS or fallback to default
joshkaplinsky May 3, 2022
17fb03f
Merge remote-tracking branch 'upstream/develop' into develop
joshkaplinsky May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions gcpkms/keysource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@ package gcpkms //import "go.mozilla.org/sops/v3/gcpkms"
import (
"encoding/base64"
"fmt"
"google.golang.org/api/option"
"io/ioutil"
"os"
"regexp"
"strings"
"time"

"go.mozilla.org/sops/v3/logging"

"golang.org/x/net/context"
"golang.org/x/oauth2/google"

"github.com/sirupsen/logrus"
"golang.org/x/net/context"
cloudkms "google.golang.org/api/cloudkms/v1"
)

Expand Down Expand Up @@ -131,12 +132,13 @@ func (key MasterKey) createCloudKMSService() (*cloudkms.Service, error) {
}

ctx := context.Background()
client, err := google.DefaultClient(ctx, cloudkms.CloudPlatformScope)

creds, err := getDefaultApplicationCredentials()
if err != nil {
return nil, err
}

cloudkmsService, err := cloudkms.New(client)
cloudkmsService, err := cloudkms.NewService(ctx, option.WithCredentialsJSON(creds))
if err != nil {
return nil, err
}
Expand All @@ -151,3 +153,19 @@ func (key MasterKey) ToMap() map[string]interface{} {
out["created_at"] = key.CreationDate.UTC().Format(time.RFC3339)
return out
}

// 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.

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 •
edited
Loading

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.


if _, err := os.Stat(defaultCredentials); err == nil {
if token, err = ioutil.ReadFile(defaultCredentials); err != nil {
joshkaplinsky marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
} else {
token = []byte(defaultCredentials)
}
return
}