-
Notifications
You must be signed in to change notification settings - Fork 586
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
fix(credentials-provider-ini): include general http provider when sourcing EcsContainer credentials #6132
Conversation
// This assigns the role_arn of the "root" profile | ||
// to the credential_source profile so this recursive call knows | ||
// what role to assume. | ||
role_arn: data.role_arn ?? profiles[source_profile].role_arn, |
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.
👏
const { fromHttp } = await import("@aws-sdk/credential-provider-http"); | ||
const { fromContainerMetadata } = await import("@smithy/credential-provider-imds"); | ||
logger?.debug("@aws-sdk/credential-provider-ini - credential_source is EcsContainer"); | ||
return chain(fromHttp(options ?? {}), fromContainerMetadata(options)); |
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.
Just an observation. Its does feel a little bit jank that EcsContainer
is being overloaded here i.e it was previously just IMDS(container meta) but now being interpreted as IMDS OR HTTP. It would feel cleaner to define a new credential source like FromHTTP
. I guess that would be a much bigger change though as all the SDKs would need to be updated.
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.
yes, it would be out of scope to declare an entirely new ini credential source in only this SDK. In this context fromHttp is only used for its ability to connect to EKS/ECS
…rcing EcsContainer credentials
f29905c
to
6355890
Compare
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. |
Issue
investigating smithy-lang/smithy-typescript#1288
this PR requires that smithy-lang/smithy-typescript#1290 be published. It is using new APIs from that PR.
Description
Fixes:
role_arn
. Therole_arn
should be declared by the origin profile.Explanation from smithy-ts#1288:
CredentialsProviderError: 169.254.170.23 is not a valid container metadata service hostname
this is the superficial error, which makes it look like we need to add more hosts to the hardcoded allowlist within packages/credential-provider-imds/src/fromContainerMetadata.tsthe real root cause has two parts
@aws-sdk/credential-provider-ini
pkg, which is part of the default chain and is responsible for file-configured assumeRole credential resolution, does not route to the newerfromHttp
provider, only the olderfromContainerMetadata
provider. The fix PR will add this.source_profile
, expects thesource_profile
to also have arole_arn
. This is a separate bug, and I believe this isn't consistent with our docs at https://docs.aws.amazon.com/sdkref/latest/guide/feature-assume-role-credentials.html. It is not the source profile that should have arole_arn
, it is the root profile.Testing