-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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)) | ||
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()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use camelCase for field names since this generates JSON - and prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
And then load the service locally up by setting the Then, without this change, one will see an error such as: Currently, I made the code to allow both |
||
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) | ||
|
@@ -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 | ||
} |
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.
IMO now that we do much more stuff we should factor this out to a
newTokenSource
method or similar that configures a token sourceThere 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 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 :)