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 AWS credential source #474

Merged
merged 20 commits into from
Oct 5, 2023
Merged

feat: add AWS credential source #474

merged 20 commits into from
Oct 5, 2023

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Aug 25, 2023

Depends on #473

  • Successfully used to retrieve a token on AWS!
  • Verify implementation using test cases used by the other languages
  • Add Quota Project support

@bshaffer bshaffer mentioned this pull request Aug 25, 2023
src/CredentialSource/AwsNativeSource.php Outdated Show resolved Hide resolved
src/Credentials/ExternalAccountCredentials.php Outdated Show resolved Hide resolved
src/CredentialSource/AwsNativeSource.php Outdated Show resolved Hide resolved
src/CredentialSource/AwsNativeSource.php Show resolved Hide resolved
Base automatically changed from sts to main September 7, 2023 18:02
@bshaffer bshaffer marked this pull request as ready for review September 7, 2023 19:12
@bshaffer bshaffer requested a review from a team as a code owner September 7, 2023 19:12
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

The PR looks great! I've reviewed the changes and tested them on an Amazon EC2 machine to ensure they work correctly without failures. And an access token is generated successfully which can be used to make other calls.

However, I'd like to bring up a potential improvement and concern:

Improvement:

It might be a good idea for later into the PHP Client Library to better handle the project_id field. Currently, the AWS JSON key doesn't seem to include include project_id or projectId by default, unlike other authentication flows (e.g., service accounts) that do contain this key. While the audience URL does include a project number, there may be cases where having the project_id explicitly in the JSON key could be beneficial.

Concern:

I've noticed that the StorageClient may encounter issues in situations where users rely on automatic detection of the project_id. Other clients may also typically expect this field to be provided explicitly or get derived automatically from the JSON key. In the absence of the project_id, these cases could potentially lead to failures in user's application.

How I generated the token:

use Google\Auth\Credentials\ExternalAccountCredentials;

putenv(sprintf('GOOGLE_APPLICATION_CREDENTIALS=%s/%s', __DIR__, 'key.json'));
$creds = ApplicationDefaultCredentials::getCredentials('https://www.googleapis.com/auth/cloud-platform');
print_r($creds->fetchAuthToken())

@bshaffer
Copy link
Contributor Author

bshaffer commented Oct 5, 2023

@vishwarajanand

the AWS JSON key doesn't seem to include include project_id or projectId by default, unlike other authentication flows (e.g., service accounts) that do contain this key

Credentials return the "quota project ID", which is something different. However, it may be a good idea to have ExternalAccountCredentials support quota_project_id, which it currently doesn't support.

I've noticed that the StorageClient may encounter issues in situations where users rely on automatic detection of the project_id

We've gone through a few iterations of "project ID detection", but it's a complex problem because the various client libraries do it a little differently (env var, client option, credential field, etc). However, this is a separate issue that is out of scope of this PR.

@bshaffer
Copy link
Contributor Author

bshaffer commented Oct 5, 2023

@vishwarajanand I stand corrected, it IS supported (via ProjectIdProviderInterface). It doesn't seem like this is supported in the NodeJS implementation or the python implementation, at least not from JSON credentials

https://github.com/googleapis/google-auth-library-nodejs/blob/bf23bad915cae8994b2953b406d04112848c5224/src/auth/baseexternalclient.ts#L68-L83

https://github.com/googleapis/google-auth-library-python/blob/117e3253d27ca4519dabe169825a3a0473aff482/google/auth/external_account.py#L350-L353

I'll do some more research here...

@bshaffer
Copy link
Contributor Author

bshaffer commented Oct 5, 2023

Okay it looks like there IS a way you can potentially get the project ID from the audience or from the workload identity pool, but after speaking with @aeitzman, this is a part of Workforce credentials, and can be a follow-up PR.

I've created this bug to track it: #483

@bshaffer bshaffer closed this Oct 5, 2023
@bshaffer bshaffer reopened this Oct 5, 2023
@bshaffer bshaffer enabled auto-merge (squash) October 5, 2023 20:08
@bshaffer bshaffer merged commit e5bc897 into main Oct 5, 2023
18 checks passed
@bshaffer bshaffer deleted the aws-for-sts branch October 5, 2023 20:08
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