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

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

Merged
merged 1 commit into from Sep 21, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 73 additions & 1 deletion cloudclient/dial.go
Expand Up @@ -3,11 +3,16 @@ package cloudclient
import (
"context"
"crypto/x509"
"encoding/json"
"fmt"
"strconv"
"strings"
"time"

"go.einride.tech/cloudrunner/cloudzap"
"go.uber.org/zap"
"golang.org/x/oauth2"
"golang.org/x/oauth2/google"
"google.golang.org/api/idtoken"
"google.golang.org/api/option"
"google.golang.org/grpc"
Expand All @@ -21,8 +26,52 @@ func DialService(ctx context.Context, target string, opts ...grpc.DialOption) (*
audience := "https://" + trimPort(target)
idTokenSource, err := idtoken.NewTokenSource(ctx, audience, option.WithAudiences(audience))
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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 :)

if err != nil {
return nil, fmt.Errorf("dial %s: %w", target, err)
logger, ok := cloudzap.GetLogger(ctx)
if !ok {
logger = zap.NewNop()
}
// Google's idtoken package does not support credential type other than `service_account`.
// This blocks local development with using `impersonated_service_account` type credentials. If that happens,
// we work it around by using our Application Default Credentials (which is impersonated already) to fetch
// an id_token on the fly.
// This however still blocks `authorized_user` type of credentials passing through.
// Related issue page: https://github.com/googleapis/google-api-go-client/issues/873
defaultCred, defaultCredErr := google.FindDefaultCredentials(ctx)
if defaultCredErr == nil {
var credTypeHolder struct {
Type string `json:"type"`
}
if jsonErr := json.Unmarshal(defaultCred.JSON, &credTypeHolder); jsonErr != nil {
// Ignoring jsonErr if it happens.
return nil, err
}
if credTypeHolder.Type != "impersonated_service_account" {
// We only patching the case where type of "impersonated_service_account" is used
// if not return original error.
return nil, err
}
} else {
// Ignoring defaultCredErr if it happens.
return nil, err
}

// Here we know the err is due to "impersonated_service_account" type of credential is used
// Just verify again with err's error message to be sure
if !strings.Contains(err.Error(), "impersonated_service_account") {
return nil, err
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
gts, err := google.DefaultTokenSource(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@liufuyang liufuyang Sep 12, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@liufuyang liufuyang Sep 12, 2022

Choose a reason for hiding this comment

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

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.

if err != nil {
return nil, err
}
idTokenSource = oauth2.ReuseTokenSource(nil, &idTokenSourceWrapper{TokenSource: gts})
}

systemCertPool, err := x509.SystemCertPool()
if err != nil {
return nil, fmt.Errorf("dial %s: %w", target, err)
Expand Down Expand Up @@ -62,3 +111,26 @@ func withDefaultPort(target string, port int) string {
}
return target
}

// idTokenSourceWrapper is an oauth2.TokenSource wrapper used for getting id_token for local development using
// `authorized_user` type credentials
// It takes the id_token from TokenSource and passes that on as a bearer token.
type idTokenSourceWrapper struct {
TokenSource oauth2.TokenSource
}

func (s *idTokenSourceWrapper) Token() (*oauth2.Token, error) {
token, err := s.TokenSource.Token()
if err != nil {
return nil, err
}
idToken, ok := token.Extra("id_token").(string)
if !ok {
return nil, fmt.Errorf("token did not contain an id_token")
}
return &oauth2.Token{
AccessToken: idToken,
TokenType: "Bearer",
Expiry: token.Expiry,
}, nil
}