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(credential-provider-node): add support for web identity provider #2260

Merged
merged 4 commits into from
May 5, 2021

Conversation

ejhayes
Copy link
Contributor

@ejhayes ejhayes commented Apr 15, 2021

Issue

Fixes #2148

Description

Adds default support for web identity credentials. This is useful when using this SDK with IAM Roles for Service accounts in EKS which set AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE behind the scenes. Without this the default behavior of the SDK is to use credentials from IMDS.

Testing

Tests have been updated.

Additional context

Additional updates include:

  • Update README.md to include web identity params and default load order information (fixes typo)
  • Set init param of fromTokenFile to optional. Default to the default web identity assume role function from client-sts if not provided

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ejhayes ejhayes force-pushed the main branch 2 times, most recently from e7c94cc to 186fea0 Compare April 15, 2021 21:23
@ejhayes ejhayes changed the title feat(credential-provider-node): add support for web identity provider. feat(credential-provider-node): add support for web identity provider Apr 15, 2021
@ejhayes ejhayes force-pushed the main branch 2 times, most recently from 77cac09 to 608cc2a Compare April 16, 2021 02:19
@ejhayes
Copy link
Contributor Author

ejhayes commented Apr 16, 2021

@AllanZhengYP I'm a tad stumped on this particular error getting thrown. Looks like you ran into something similar in #2055. Unfortunately adding to noHoist doesn't seem to make a difference on the builds. Any thoughts would be greatly appreciated!

@AllanZhengYP
Copy link
Contributor

Hi @ejhayes,

The original issue should already be fixed:

import { AssumeRoleWithWebIdentityParams, fromTokenFile } from "@aws-sdk/credential-provider-web-identity";

Now the fromIni() should be able to resolve the EKS credentials already in the default node credential provider. Can you try with the latest client?

@ejhayes
Copy link
Contributor Author

ejhayes commented Apr 16, 2021

Thanks for the quick response @AllanZhengYP -- fromIni works as expected if an ini config file exists. What doesn't work though is setting these values as environment variables (which is done when using this with EKS):

const {getSignedUrl} = require("@aws-sdk/s3-request-presigner");
const {S3Client,GetObjectCommand} = require("@aws-sdk/client-s3");

getSignedUrl(new S3Client(), new GetObjectCommand({
    Bucket: 'mybucket',
    Key: 'somekey'
}), {
    expiresIn: 3600
}).then((url) => {
    console.log(url);
    console.log('Done');
})
.catch((err) => {
    console.log(err);
});
AWS_WEB_IDENTITY_TOKEN_FILE=token AWS_ROLE_ARN=arn:aws:iam::****:role/**** AWS_REGION=**** node test.js
Error: TimeoutError
    at ClientRequest.<anonymous> (****/node_modules/@aws-sdk/credential-provider-imds/dist/cjs/remoteProvider/httpRequest.js:17:20)
    at ClientRequest.emit (events.js:310:20)
    at Socket.emitRequestTimeout (_http_client.js:709:9)
    at Object.onceWrapper (events.js:416:28)
    at Socket.emit (events.js:322:22)
    at Socket._onTimeout (net.js:479:8)
    at listOnTimeout (internal/timers.js:549:17)
    at processTimers (internal/timers.js:492:7) {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}

This works fine with the cli:

$ aws --version
aws-cli/2.1.28 Python/3.9.2 Darwin/20.3.0 source/x86_64 prompt/off
$ AWS_WEB_IDENTITY_TOKEN_FILE=token AWS_ROLE_ARN=arn:aws:iam::****:role/**** AWS_REGION=**** aws s3 presign s3://mybucket/somekey
https://mybucket.s3.****.amazonaws.com/somekey?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIA****%2Fs3%2Faws4_request&X-Amz-Date=****&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Security-Token=****&X-Amz-Signature=****

@carlosescura
Copy link

Same issue here, we can't have our code to seamlessly work between local environments and EKS because of the lack of "auto-config" using ENV vars.

@AllanZhengYP AllanZhengYP self-requested a review April 19, 2021 18:44
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

The reason to the failed CI is from the circular dependency you introduced in the credential-provider-web-identity package. I provided some explanation for why it's the case and how to fix it.

package.json Outdated Show resolved Hide resolved
packages/credential-provider-node/src/index.ts Outdated Show resolved Hide resolved
packages/credential-provider-web-identity/package.json Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@2384e24). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2260   +/-   ##
=======================================
  Coverage        ?   60.06%           
=======================================
  Files           ?      472           
  Lines           ?    24838           
  Branches        ?     5883           
=======================================
  Hits            ?    14918           
  Misses          ?     9920           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2384e24...e00022d. Read the comment docs.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-staging-test
  • Commit ID: 7d6792c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ejhayes
Copy link
Contributor Author

ejhayes commented Apr 20, 2021

Thanks for the input @AllanZhengYP! Looks like all tests are passing now

@ejhayes
Copy link
Contributor Author

ejhayes commented Apr 20, 2021

#2177

@ejhayes
Copy link
Contributor Author

ejhayes commented Apr 27, 2021

@AllanZhengYP Is this something that could be merged in? Would be great to have this functionality supported in the default credential provider!

@ejhayes
Copy link
Contributor Author

ejhayes commented May 3, 2021

Hey @trivikr or @AllanZhengYP -- any way these changes could be merged in? Would be great to have this SDK working in EKS using the environment variables that are automatically set rather than having to create a config file!

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

@ejhayes This looks awesome. 🚢 Thank you a lot for the contribution!

@ejhayes
Copy link
Contributor Author

ejhayes commented May 5, 2021

Thanks @AllanZhengYP! Looks like this is still blocked from merging. What else is needed to merge these changes in?

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call fromTokenFile credential provider by default in credential-provider-node
5 participants