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

fix(credentials-provider-ini): include general http provider when sourcing EcsContainer credentials #6132

Merged
merged 7 commits into from
May 30, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented May 23, 2024

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:

  • When resolving EcsContainer as source in the ini provider, include also fromHttp next to fromContainerMetadata.
  • When resolving credentials in ini-provider using an assume role "source_profile", do not require the source_profile to declare a role_arn. The role_arn should be declared by the origin profile.
  • improve logging information for the credential chain process

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.ts

  • the real root cause has two parts

    1. the @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 newer fromHttp provider, only the older fromContainerMetadata provider. The fix PR will add this.
    2. the same package, when merging a source_profile, expects the source_profile to also have a role_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 a role_arn, it is the root profile.

Testing

  • add to credential chain integration test

// 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,
Copy link

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));
Copy link

@jgrigg jgrigg May 25, 2024

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.

Copy link
Contributor Author

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

@kuhe kuhe force-pushed the fix/ecs_credential_source branch from f29905c to 6355890 Compare May 30, 2024 16:52
@kuhe kuhe marked this pull request as ready for review May 30, 2024 16:52
@kuhe kuhe requested a review from a team as a code owner May 30, 2024 16:52
@kuhe kuhe merged commit 8cdaf0a into aws:main May 30, 2024
3 checks passed
@kuhe kuhe deleted the fix/ecs_credential_source branch May 30, 2024 17:33
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 Jun 14, 2024
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.

3 participants