-
Notifications
You must be signed in to change notification settings - Fork 192
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
base: main
Are you sure you want to change the base?
Conversation
….com:googleapis/google-auth-library-php into add-impersonated-service-account-credentials
cc @gjvanahee I'd love for you to test this implementation and see if it works for your use-case! |
I will test it today! Let you know
…On Sat, 5 Oct 2024 at 20:48, Brent Shaffer ***@***.***> wrote:
cc @gjvanahee <https://github.com/gjvanahee> I'd love for you to test
this implementation and see if it works for your use-case!
—
Reply to this email directly, view it on GitHub
<#580 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABR76SPYQHQ7KN55KMXB5Y3Z2AX6BAVCNFSM6AAAAABPF6GVASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGE2TKNJUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
||
$request = new Request( | ||
'POST', | ||
$this->serviceAccountImpersonationUrl, |
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.
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)
);
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.
@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:
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.
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
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.
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.
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.
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...
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.
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.
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.
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 ;)
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
: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.