cloud/gcp: add assume role for GCS and GCP KMS #80417

rhu713 · 2022-04-22T22:35:25Z

Add service account impersonation for GCS and GCP KMS. This is the assume role
equivalent for GCP that allows the service account from implicit or specified
credentials to obtain temporary credentials for another service account.

Release note (enterprise change): Storage and KMS URIs for GCP in Backup and
restore now accept an "ASSUME_ROLE" parameter, which informs the current
service account authenticated by either implicit or specified credentials to
obtain temporary credentials for the service account specified by the
ASSUME_ROLE parameter in order to access the resource specified by the URI.
Below are examples of URIs with "ASSUME_ROLE" parameter:

GCS: gs://<bucket>/<key>?AUTH=specified&ASSUME_ROLE=<service_account>@<project>.iam.gserviceaccount.com&CREDENTIALS=<credentials>
KMS GCP: gs:///<key_resource_name>?AUTH=implicit&ASSUME_ROLE=<service_account>@<project>.iam.gserviceaccount.com

cockroach-teamcity · 2022-04-22T22:35:32Z

This change is

rhu713 · 2022-05-18T14:59:57Z

@adityamaru do you mind reviewing this PR?

adityamaru · 2022-05-18T15:12:06Z

on it, sorry for the delay

adityamaru

Mostly nits and questions. Can we edit the Release Note to maybe add an example of what form a URL with ASSUME and TARGET would look like?

adityamaru · 2022-05-18T15:30:27Z

pkg/roachpb/api.proto

@@ -1398,6 +1398,8 @@ message ExternalStorage {
    string billing_project = 4;

    string credentials = 5;
+
+    string target = 6;


Can we add a comment describing what target is? I think it's a generic enough word that it might help to provide some gcs specific context.

changed to service account, which should be more self descriptive.

adityamaru · 2022-05-18T15:35:31Z

pkg/cloud/gcp/gcs_kms.go

@@ -68,11 +70,21 @@ func MakeGCSKMS(uri string, env cloud.KMSEnv) (cloud.KMS, error) {

 	// Client options to authenticate and start a GCS KMS session.
 	// Currently only accepting json of service account.
-	var credentialsOpt []option.ClientOption
+	var clientOpts []option.ClientOption


Was there any reason to pull this out? It seems like its logic that is specific to only cloud.AuthParamSpecified so should we move it back inside the case stmt for readability or am I missing something.

Oops. Reverted this section back to what it was. I originally had this code reused for specified credentials for the assume AUTH but that code turned out to be slightly different in the end.

pkg/cloud/gcp/gcs_kms.go

adityamaru · 2022-05-18T15:38:36Z

pkg/cloud/gcp/gcs_storage.go

@@ -43,6 +45,9 @@ const (
 	// CredentialsParam is the query parameter for the base64-encoded contents of
 	// the Google Application Credentials JSON file.
 	CredentialsParam = "CREDENTIALS"
+	// TargetPrincipalParam is the target principal to impersonate. The target


just for my edification what is a principal? Do you think it is worth spelling that out here?

I'll change this to SERVICE_ACCOUNT actually (and also in other places target is used). I originally wanted to use the same params as the API in the impersonate package but it's probably more clear to just call everything "service account"

pkg/cloud/gcp/gcs_storage.go

adityamaru · 2022-05-18T15:42:22Z

pkg/cloud/gcp/gcs_storage.go

@@ -116,26 +122,38 @@ func makeGCSStorage(

 	switch conf.Auth {
 	case cloud.AuthParamImplicit:
-		// Do nothing; use implicit params:
+		// Use implicit params to access storage or impersonate target:


How does AuthParamImplicit interact with impersonating a target? I'm probably missing something here but this case doesn't have fallthrough so implicit is doing what it used to do i.e. not setup any options?

Oops, removed this comment change. There's no interaction with implicit auth and impersonate.

pkg/cloud/gcp/gcs_kms.go

adityamaru · 2022-05-18T15:49:09Z

pkg/cloud/gcp/gcs_storage.go

+
+		assumeOpt, err := createImpersonateCredentials(ctx, conf.Target, []string{scope}, conf.Credentials)
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to assume %s", conf.Target)


will Target have sensitive information?

hmm you're right. I suppose that there could be information revealed about what the service account does from its email address. I've removed it from the logging.

rhu713

Addressed comments. Also modified the release note to add some example URIs with this change.

rhu713 · 2022-05-19T16:26:58Z

pkg/cloud/gcp/gcs_kms.go

@@ -68,11 +70,21 @@ func MakeGCSKMS(uri string, env cloud.KMSEnv) (cloud.KMS, error) {

 	// Client options to authenticate and start a GCS KMS session.
 	// Currently only accepting json of service account.
-	var credentialsOpt []option.ClientOption
+	var clientOpts []option.ClientOption


Oops. Reverted this section back to what it was. I originally had this code reused for specified credentials for the assume AUTH but that code turned out to be slightly different in the end.

pkg/cloud/gcp/gcs_kms.go

rhu713 · 2022-05-19T16:39:28Z

pkg/cloud/gcp/gcs_storage.go

@@ -43,6 +45,9 @@ const (
 	// CredentialsParam is the query parameter for the base64-encoded contents of
 	// the Google Application Credentials JSON file.
 	CredentialsParam = "CREDENTIALS"
+	// TargetPrincipalParam is the target principal to impersonate. The target


I'll change this to SERVICE_ACCOUNT actually (and also in other places target is used). I originally wanted to use the same params as the API in the impersonate package but it's probably more clear to just call everything "service account"

rhu713 · 2022-05-19T16:40:53Z

pkg/cloud/gcp/gcs_storage.go

@@ -116,26 +122,38 @@ func makeGCSStorage(

 	switch conf.Auth {
 	case cloud.AuthParamImplicit:
-		// Do nothing; use implicit params:
+		// Use implicit params to access storage or impersonate target:


Oops, removed this comment change. There's no interaction with implicit auth and impersonate.

rhu713 · 2022-05-19T16:44:38Z

pkg/cloud/gcp/gcs_storage.go

+
+		assumeOpt, err := createImpersonateCredentials(ctx, conf.Target, []string{scope}, conf.Credentials)
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to assume %s", conf.Target)


hmm you're right. I suppose that there could be information revealed about what the service account does from its email address. I've removed it from the logging.

pkg/cloud/gcp/gcs_storage.go

rhu713 · 2022-05-19T16:56:17Z

pkg/roachpb/api.proto

@@ -1398,6 +1398,8 @@ message ExternalStorage {
    string billing_project = 4;

    string credentials = 5;
+
+    string target = 6;


changed to service account, which should be more self descriptive.

adityamaru · 2022-05-20T13:11:13Z

pkg/cloud/gcp/gcs_storage.go

+	// ServiceAccountParam is the email of the service account to impersonate.
+	// This service account should have the necessary access to the relevant cloud
+	// services.
+	ServiceAccountParam = "SERVICE_ACCOUNT"


nit: I definitely think ServiceAccount is clearer than target but does this now give the impression that we are passing the service account that will be used to access cloud services? I guess in a way we are but should we be more explicit and call it ASSUMED_SERVICE_ACCOUNT or something? Might also be worth asking a few more folks for a bikeshed?

Done, now we are using the param ASSUME_ROLE and not using an additional auth method

adityamaru · 2022-05-20T13:13:39Z

pkg/roachpb/api.proto

@@ -1424,6 +1424,8 @@ message ExternalStorage {
    string billing_project = 4;

    string credentials = 5;
+
+    string service_account = 6;


depending on how we address the comment on the URI param name we should change this accordingly. A comment might still be nice

added comment

pkg/cloud/gcp/gcs_storage_test.go

adityamaru · 2022-05-20T13:39:58Z

go.mod

@@ -5,7 +5,7 @@ go 1.17
 require (
 	cloud.google.com/go/kms v1.1.0
 	cloud.google.com/go/pubsub v1.16.0
-	cloud.google.com/go/storage v1.21.0
+	cloud.google.com/go/storage v1.22.0


Just checking what motivated this bump?

no longer bumped with the separate vendor PR

rhu713 · 2022-05-25T18:02:53Z

@adityamaru this should be ready FAL

adityamaru · 2022-05-26T20:18:17Z

pkg/cloud/gcp/gcs_storage.go

+		credentialsOpt = append(credentialsOpt, authOption)
+	}
+
+	opts := []option.ClientOption{option.WithScopes(scope)}


nit: can we add a quick comment mentioning that once we have resolved AUTH as implicit or specified we check if we need to use these creds to assume a role.

adityamaru · 2022-05-26T21:20:16Z

pkg/cmd/roachtest/tests/backup.go

+				m.Go(func(ctx context.Context) error {
+					t.Status(`running backup`)
+					c.Run(ctx, c.Node(1), fmt.Sprintf(
+						"./cockroach sql --insecure -e \"BACKUP bank.bank TO '%s' WITH KMS='%s'\"",


nit: can we use the BACKUP INTO syntax? We want to get rid of TO soonish so we'll have to cleanup all roachtests eventually.

Is it useful to RESTORE from the backup and check if the table has expected rows as well? It'll be a more complete test imo, wdyt?

adityamaru · 2022-05-26T21:22:10Z

nit: We should update PR description with updated commit message

Add service account impersonation for GCS and GCP KMS. This is the assume role equivalent for GCP that allows the service account from implicit or specified credentials to obtain temporary credentials for another service account. Release note (enterprise change): Storage and KMS URIs for GCP in Backup and restore now accept an "ASSUME_ROLE" parameter, which informs the current service account authenticated by either implicit or specified credentials to obtain temporary credentials for the service account specified by the ASSUME_ROLE parameter in order to access the resource specified by the URI. Below are examples of URIs with "ASSUME_ROLE" parameter: ``` GCS: gs://<bucket>/<key>?AUTH=specified&ASSUME_ROLE=<service_account>@<project>.iam.gserviceaccount.com&CREDENTIALS=<credentials> KMS GCP: gs:///<key_resource_name>?AUTH=implicit&ASSUME_ROLE=<service_account>@<project>.iam.gserviceaccount.com ```

rhu713 · 2022-05-30T19:52:12Z

bors r+

craig · 2022-05-30T20:48:25Z

Build succeeded:

GitHub CI (Cockroach)

rhu713 force-pushed the assume-role-gcp branch 4 times, most recently from 46ae65c to 8a99af4 Compare May 2, 2022 12:16

rhu713 force-pushed the assume-role-gcp branch from 8a99af4 to 63be6b6 Compare May 2, 2022 13:55

rhu713 marked this pull request as ready for review May 2, 2022 13:55

rhu713 requested a review from a team as a code owner May 2, 2022 13:55

rhu713 requested a review from a team May 2, 2022 13:55

rhu713 requested a review from a team as a code owner May 2, 2022 13:55

rhu713 requested review from msbutler and removed request for a team May 2, 2022 13:55

rhu713 force-pushed the assume-role-gcp branch 2 times, most recently from e274c6e to 23f89f6 Compare May 3, 2022 15:29

rhu713 requested review from adityamaru and removed request for msbutler May 3, 2022 18:03

adityamaru reviewed May 18, 2022

View reviewed changes

rhu713 force-pushed the assume-role-gcp branch from 23f89f6 to 5bfa587 Compare May 19, 2022 17:41

rhu713 commented May 19, 2022

View reviewed changes

rhu713 force-pushed the assume-role-gcp branch from 5bfa587 to 9b9eea5 Compare May 19, 2022 17:53

adityamaru self-requested a review May 20, 2022 13:04

adityamaru suggested changes May 20, 2022

View reviewed changes

adityamaru self-requested a review May 20, 2022 13:40

rhu713 force-pushed the assume-role-gcp branch 5 times, most recently from 3f4b9fd to 61c1cc4 Compare May 25, 2022 15:23

adityamaru approved these changes May 26, 2022

View reviewed changes

rhu713 force-pushed the assume-role-gcp branch from 61c1cc4 to d45ddaa Compare May 27, 2022 21:39

craig bot merged commit 451fcd6 into cockroachdb:master May 30, 2022

cockroach-teamcity mentioned this pull request May 30, 2022

cloud/gcp: add assume role for GCS and GCP KMS cockroachdb/docs#13985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud/gcp: add assume role for GCS and GCP KMS #80417

cloud/gcp: add assume role for GCS and GCP KMS #80417

rhu713 commented Apr 22, 2022 •

edited

Loading

cockroach-teamcity commented Apr 22, 2022

rhu713 commented May 18, 2022

adityamaru commented May 18, 2022

adityamaru left a comment

adityamaru May 18, 2022

rhu713 May 19, 2022

adityamaru May 18, 2022

rhu713 May 19, 2022

adityamaru May 18, 2022

rhu713 May 19, 2022

adityamaru May 18, 2022

rhu713 May 19, 2022

adityamaru May 18, 2022

rhu713 May 19, 2022

rhu713 left a comment

rhu713 May 19, 2022

rhu713 May 19, 2022

rhu713 May 19, 2022

rhu713 May 19, 2022

rhu713 May 19, 2022

adityamaru May 20, 2022

rhu713 May 25, 2022

adityamaru May 20, 2022

rhu713 May 25, 2022

adityamaru May 20, 2022

rhu713 May 25, 2022

rhu713 commented May 25, 2022

adityamaru May 26, 2022

adityamaru May 26, 2022

adityamaru May 26, 2022

adityamaru commented May 26, 2022

rhu713 commented May 30, 2022

craig bot commented May 30, 2022

cloud/gcp: add assume role for GCS and GCP KMS #80417

cloud/gcp: add assume role for GCS and GCP KMS #80417

Conversation

rhu713 commented Apr 22, 2022 • edited Loading

cockroach-teamcity commented Apr 22, 2022

rhu713 commented May 18, 2022

adityamaru commented May 18, 2022

adityamaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhu713 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhu713 commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru commented May 26, 2022

rhu713 commented May 30, 2022

craig bot commented May 30, 2022

rhu713 commented Apr 22, 2022 •

edited

Loading