feat(cloudclient): support id_token with impersonated_service_account type #273

liufuyang · 2022-09-12T08:26:32Z

While trying to run a backend service locally (with SA impersonation) and calling other internal backend services, I encountered an error like this:

idtoken: credential must be service_account, found "impersonated_service_account"

While google's google-api-go-client is not supporting using Application Default Credentials (via the employee account auth), there seems to be a workaround mentioned in the issue and here we basically implement the same.

Tested this locally while using it and it seems working correctly.

odsod · 2022-09-12T09:16:36Z

cloudclient/dial.go

+		}
+		logger.Sugar().Warnf("%s. Using Application Default Credentials to fetch id_token again. "+
+			"This should never happen in prod env.", err.Error())
+		gts, err := google.DefaultTokenSource(ctx)


I understand what the idea is here but have we thought about the implications of making it easier to run backend services as user identities? The effective permissions the service gets will vary depending on the permissions of the user. What use case is this solving?

I would like to develop a backend service by running it locally and using dev_env database and other downstream services in dev_env. I guess the user impersonation with some specific service account can allow us to do local development much easier and no need for encrypted keys anywhere.

Just some back ground: As we use CloudSQL IAM user, so in dev env, we have a service A with service_account SA connects to a postgres and also calling onto service B.

And if I want to locally run a copy of service A which can also connect to dev env's postgres and service B, the simplest way seems to just impersonate my local principal employee account as SA and load the service up locally.

If we have security concerns, I think perhaps one way to solve this is to find a way to make sure we don't have Service Account Token Creator role on SAs used in production environment. I had to add this Service Account Token Creator role on the dev env SA that I was impersonating to make it work.

@liufuyang After our discussion at the backend guild, I understand much better! So we assume that we are running this in combination with SA impersonation, so that the DefaultTokenSource will generate impersonated SA tokens instead of personal identity tokens?

Yep, I think that is the idea :)

So basically one would need to do:

gcloud --impersonate-service-account xxx@xxx.iam.gserviceaccount.com auth application-default login, (cat the local application_default_credentials.json key file seeing "type": "impersonated_service_account")

And then load the service locally up by setting the GOOGLE_APPLICATION_CREDENTIALS env to the application_default_credentials.json file.

Then, without this change, one will see an error such as:
idtoken: credential must be service_account, found "impersonated_service_account"

Currently, I made the code to allow both authorized_user and impersonated_service_account to pass but apparently I can change it to only allow impersonated_service_account to pass as this for sure scopes the access right onto that impersonated account, so feels more aligned with what we need and not giving extra privliges.

alethenorio

Isn't it better to do this on the client side and pass the proper token there?

Also have you tried doing local calls to the service you tested? I recently attempted this on the client side and it failed because Google washes away the signature of a user token (even with this workaround) and the token can therefore not be used to call other Google services.

liufuyang · 2022-09-12T09:52:14Z

I have tested this locally and it works 😆 (And I have to call the service with a token that is generated also from the impersonated service_account)

cloudclient/dial.go

odsod · 2022-09-12T16:12:22Z

cloudclient/dial.go

@@ -21,8 +25,28 @@ func DialService(ctx context.Context, target string, opts ...grpc.DialOption) (*
 	audience := "https://" + trimPort(target)
 	idTokenSource, err := idtoken.NewTokenSource(ctx, audience, option.WithAudiences(audience))


IMO now that we do much more stuff we should factor this out to a newTokenSource method or similar that configures a token source

I am new to Goland and also the code base is new, probably need some guidance on how to do this exactly if we need the change in this PR :)

liufuyang · 2022-09-14T12:41:24Z

Isn't it better to do this on the client side and pass the proper token there?

@alethenorio Forgot to answer your first question - this cloudrunner dail is called when the services loads up, without the suggested change in this PR, the service just stops and gave error log such as:

2022-09-14T14:37:23.743+0200    info    server/connections.go:56        connecting to service   {"connection": {"Address":"vehicletype-eu-tws6u2gxqq-ew.a.run.app"}}
2022-09-14T14:37:23.743+0200    info    cloudrunner@v0.30.0/run.go:133  goodbye
2022/09/14 14:37:23 init: dial vehicletype-eu-tws6u2gxqq-ew.a.run.app: idtoken: credential must be service_account, found "impersonated_service_account"

So without a runnable service locally, we cannot test any token passing from the client side :)

odsod · 2022-09-20T15:02:12Z

cloudclient/dial.go

+		// Related issue page: https://github.com/googleapis/google-api-go-client/issues/873
+		defaultCred, defaultCredErr := google.FindDefaultCredentials(ctx)
+		if defaultCredErr == nil {
+			var credMap map[string]interface{}


in Go it's usually better to unmarshal JSON into a struct, when you want a specific field:

var credential struct { Type string `json:"type"` }

Aha, that is nice.

odsod · 2022-09-20T15:03:13Z

cloudclient/dial.go

+			return nil, err
+		}
+
+		logger.Sugar().Warnf("%s. Using Application Default Credentials to fetch id_token again. "+


IMO avoid the sugared API, it doesn't add much readability over the default API, and that's what's used most commonly

odsod · 2022-09-21T10:50:27Z

cloudclient/dial.go

@@ -16,13 +21,59 @@ import (
 	"google.golang.org/grpc/keepalive"
 )

+type credTypeHolder struct {


since this is only used in one place, please use this as an inline var type like I showed in the example

… type

odsod · 2022-09-21T12:52:21Z

cloudclient/dial.go

+
+		logger.Warn(
+			"Using Application Default Credentials to fetch id_token again. This should never happen in prod env.",
+			zap.String("ignored_error", err.Error()),


nit: use camelCase for field names since this generates JSON - and prefer zap.Error for errors

Sorry I missed this, will create a patch to update it then :)

liufuyang requested review from odsod and ericwenn as code owners September 12, 2022 08:26