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: add support for Impersonated Service Account Credentials #580

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Oct 1, 2024

Although we've had the class ImpersonatedServiceAccountCredentials for a while, it does not actually make the request to exchange credentials for impersonated ones. So I'm not convinced this actually ever worked.

This PR adds support for the following for ImpersonatedServiceAccountCredentials:

Access Tokens ID Tokens Access Tokens with Universe Domain ID Tokens with Universe Domain (#581)
UserRefreshCredentials n/a n/a
ServiceAccountCredentials
ExternalAccountCredentials
Arbitrary Credentials (e.g. GCECredentials)

NOTE: Tests have been added for all rows for the first two columns above. Universe domain test cases have not been added because 1. they've been tested manually E2E and 2. There is no change of behavior in ImpersonatedServiceAccountCredentials for universe domain, as the logic is handled entirely by the underlying source credentials.

@bshaffer bshaffer marked this pull request as ready for review October 5, 2024 18:31
@bshaffer bshaffer requested a review from a team as a code owner October 5, 2024 18:31
@bshaffer bshaffer requested a review from Hectorhammett October 5, 2024 18:46
@bshaffer
Copy link
Contributor Author

bshaffer commented Oct 5, 2024

cc @gjvanahee I'd love for you to test this implementation and see if it works for your use-case!

@gjvanahee
Copy link

gjvanahee commented Oct 7, 2024 via email


$request = new Request(
'POST',
$this->serviceAccountImpersonationUrl,

Choose a reason for hiding this comment

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

I get an error (http 400) here about the json payload in the outgoing request "Invalid JSON payload received. Unknown name "audience": Cannot find ...". When I update the url by replacing generateAccessToken to generateIdToken everything works as expected!

This is what I changed:

        if ($this->isIdTokenRequest()) {
            $url = str_replace('generateAccessToken', 'generateIdToken', $this->serviceAccountImpersonationUrl);
            $body = [
                'audience' => $this->targetAudience,
                'includeEmail' => true,
            ];
        } else {
            $body = [
                'scope' => $this->targetScope,
                'delegates' => $this->delegates,
                'lifetime' => sprintf('%ss', $this->lifetime),
            ];
            $url = $this->serviceAccountImpersonationUrl;
        }

        $request = new Request(
            'POST',
            $url,
            $headers,
            (string) json_encode($body)
        );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gjvanahee that value is defined in your credentials in the service_account_impersonation_url JSON value. I do not want to do a string find-and-replace on values in the credentials, so I'm not sure what the appropriate approach is here.

Python does seem to do some sort of templating here, so maybe this requires further consideration:

https://github.com/googleapis/google-auth-library-python/blob/484c8db151690a4ae7b6b0ae38db0a8ede88df69/google/auth/iam.py#L41-L54

Choose a reason for hiding this comment

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

I agree with the str_replace. I just wanted to make the change more explicit. I don't like to have to change a generated key file. In my PR I used "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/{$impersonatedServiceAccount}:generateIdToken", but it looks a lot nicer to have all the options together in constants like in the python example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing we could do (and maybe this is what Python does, I need to llook into it still) is not support service_account_impersonation_url value in jsonKey for ID tokens (as this is tied directly to the JSON credentials file, which is not supported for ID tokens by gcloud), and instead use the IAM endpoint template.

Choose a reason for hiding this comment

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

Could Google\Auth\Iam class be changed to add methods for requesting an id token (and perhaps access_token)? All the actions on that endpoint would then be together in that class. Something like

    private const ID_TOKEN_PATH = '%s:generateIdToken';
    public function generateIdToken(string $email, string $targetAudience, string $accessToken): string
    {
        $httpHandler = $this->httpHandler;
        $name = sprintf(self::SERVICE_ACCOUNT_NAME, $email);
        $apiRoot = str_replace('UNIVERSE_DOMAIN', $this->universeDomain, self::IAM_API_ROOT_TEMPLATE);
        $uri = $apiRoot . '/' . sprintf(self::ID_TOKEN_PATH, $name);

        $body = [
            'audience' => $targetAudience,
            'includeEmail' => true,
        ];

        $headers = [
            'Authorization' => 'Bearer ' . $accessToken,
            'Content-Type' => 'application/json',
            'Cache-Control' => 'no-store',
        ];

        $request = new Psr7\Request(
            'POST',
            $uri,
            $headers,
            Utils::streamFor(json_encode($body))
        );

        $res = $httpHandler($request);
        $body = json_decode((string) $res->getBody(), true);

        return $body['token'];
    }

and some refactoring to avoid duplication of course.
It would skip the call to applyTokenEndpointMetrics though or that has to be used in the Iam class too...

Copy link
Contributor Author

@bshaffer bshaffer Oct 7, 2024

Choose a reason for hiding this comment

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

You seem to be describing the work I did in #581 :)

The only downside here is that we are not respecting the service_account_impersonation_url in the credentials, which might be fine, but we need to make sure this is okay to do. Well, we are respecting it in the sense that we are stripping out the clientEmail from it, which I'm not sure is better or worse.

Choose a reason for hiding this comment

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

Hehehe, it seems I'm always one step behind... I also found it strange that the service account being impersonated is not explicitly mentioned in the key file. The endpoints for the actions are documented and discoverable, but that is your call ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants