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

Add ecs and eks imds ip adresses to greengrass list #1288

Closed
wants to merge 1 commit into from

Conversation

jgrigg
Copy link

@jgrigg jgrigg commented May 23, 2024

The AWS Profile EcsContainer credential provider is broken for application code running within the EKS environment using the AWS Pod Identity Service for credentials. This is because the AWS JSv3 SDK (using this library) configures itself using the fromContainerMetadata method which has an allow-list to only the ECS service (or localhost).
The EcsContainer credential provider is intended to be used with either ECS or EKS credential services (see https://docs.aws.amazon.com/sdkref/latest/guide/feature-container-credentials.html). This library restricts this provider to usage only with the ECS credential service.
This PR allow-lists the EKS credential service in addition to the ECS service to match the supported use-cases when configuring from the AWS profile config. This brings the AWS JSv3 SDK (which is supported) which uses this library back into feature-parity with the other SDKs supporting the EKS Pod Identity Service such as the Go SDK.
It’s been noted in other PRs/issues around this issue that the fromHttp method can be used for the EKS Pod Identity Service. This is correct however this is for programmatic configuration whereas this PR fix is about fixing the supported use-case of configuring the SDK via the AWS Profile Config.

Note1: Equivalent PR from November 2023 in the Go SDK

Note2: this is effectively reopening #1144

@jgrigg jgrigg requested review from a team as code owners May 23, 2024 00:33
@jgrigg jgrigg requested a review from milesziemer May 23, 2024 00:33
@kuhe
Copy link
Contributor

kuhe commented May 23, 2024

what are the ENV vars and config file data needed to reproduce the issue?

@kuhe kuhe self-assigned this May 23, 2024
@kuhe
Copy link
Contributor

kuhe commented May 23, 2024

please provide, in addition to the environment variables and config file data, a code sample used to initialize an SDK client and perform the first request

@jgrigg
Copy link
Author

jgrigg commented May 23, 2024

Thanks George

ENV (those that are relevant):

        - name: AWS_CONFIG_FILE
          value: /workdir/.aws/config
        - name: AWS_STS_REGIONAL_ENDPOINTS
          value: regional
        - name: AWS_DEFAULT_REGION
          value: ap-southeast-2
        - name: AWS_REGION
          value: ap-southeast-2
        - name: AWS_CONTAINER_CREDENTIALS_FULL_URI
          value: 'http://169.254.170.23/v1/credentials'
        - name: AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE
          value: >-
            /var/run/secrets/pods.eks.amazonaws.com/serviceaccount/eks-pod-identity-token

/workdir/.aws/config

    # Content of the AWS Config file
    [default]
    source_profile = cluster_role
    role_arn = arn:aws:iam::1234567890:role/some-role

    [profile cluster_role]
    credential_source=EcsContainer

getSSMParameter.ts

import { GetParameterCommand, SSMClient } from '@aws-sdk/client-ssm';

export const getSSMParameter = async () => {
  const client = new SSMClient({}); // Default credential chain!

  const command = new GetParameterCommand({
    Name: 'my-param',
  });
  const response = await client.send(command);

  return response.Parameter?.Value;
};

@kuhe kuhe added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label May 23, 2024
@kuhe
Copy link
Contributor

kuhe commented May 23, 2024

It looks like you want this sequence to happen based on that information:

  • merge the source_profile options into the default profile indicating the credential_source is EcsContainer
  • get initial credentials from AWS_CONTAINER_CREDENTIALS_FULL_URI
  • make an assume role call with STS to the default profile's role_arn
    • use these secondary credentials in the client for further requests like GetParameter

is that correct?

Secondly, can you provide the output of using the default credential chain (except explicitly) and providing it a logger?

import { STS } from "@aws-sdk/client-sts";
import { defaultProvider } from "@aws-sdk/credential-provider-node";

const client = new STS({
  credentials: defaultProvider({
    logger: console,
  }),
});

await client.getCallerIdentity();

The output should look something like

@aws-sdk/credential-provider-node defaultProvider::fromEnv
@aws-sdk/credential-provider-env fromEnv
@aws-sdk/credential-provider-node defaultProvider::fromSSO
@aws-sdk/credential-provider-node defaultProvider::fromIni
@aws-sdk/credential-provider-ini fromIni
@aws-sdk/credential-provider-ini resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-node defaultProvider::fromProcess
@aws-sdk/credential-provider-process fromProcess
@aws-sdk/credential-provider-node defaultProvider::fromTokenFile
@aws-sdk/credential-provider-web-identity fromTokenFile
@aws-sdk/credential-provider-node defaultProvider::remoteProvider
@aws-sdk/credential-provider-node remoteProvider::fromHttp/fromContainerMetadata
@aws-sdk/credential-provider-http fromHttp

and it indicates the order of credential providers attempted in the chain.

@jgrigg
Copy link
Author

jgrigg commented May 24, 2024

Yes that's exactly what I'm trying to do :)

Logs as requested (baseline):

@aws-sdk/credential-provider-node defaultProvider::fromEnv
@aws-sdk/credential-provider-env fromEnv
@aws-sdk/credential-provider-node defaultProvider::fromSSO
@aws-sdk/credential-provider-node defaultProvider::fromIni
@aws-sdk/credential-provider-ini fromIni
@aws-sdk/credential-provider-ini resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-ini resolveAssumeRoleCredentials (STS)
[2024-04-15T23:57:51.447] [ERROR] default - {
  clientName: 'SSMClient',
  commandName: 'GetParameterCommand',
  input: { Name: 'AUTOMAT_REFERENCE_APP_TEST_PARAMETER' },
  error: CredentialsProviderError: 169.254.170.23 is not a valid container metadata service hostname
      at getCmdsUri (/workdir/node_modules/@smithy/credential-provider-imds/dist-cjs/index.js:158:13)
      at /workdir/node_modules/@smithy/credential-provider-imds/dist-cjs/index.js:118:34
      at retry (/workdir/node_modules/@smithy/credential-provider-imds/dist-cjs/index.js:104:17)
      at /workdir/node_modules/@smithy/credential-provider-imds/dist-cjs/index.js:117:16
      at resolveAssumeRoleCredentials (/workdir/node_modules/@aws-sdk/credential-provider-ini/dist-cjs/index.js:108:85)
      at async /workdir/node_modules/@smithy/property-provider/dist-cjs/index.js:79:27
      at async coalesceProvider (/workdir/node_modules/@smithy/property-provider/dist-cjs/index.js:106:18)
      at async /workdir/node_modules/@smithy/property-provider/dist-cjs/index.js:124:18
      at async /workdir/node_modules/@smithy/core/dist-cjs/index.js:82:17
      at async /workdir/node_modules/@aws-sdk/middleware-logger/dist-cjs/index.js:33:22 {
    tryNextLink: false
  },
  metadata: undefined
}

After applying this patch:

@aws-sdk/credential-provider-node defaultProvider::fromEnv
@aws-sdk/credential-provider-env fromEnv
@aws-sdk/credential-provider-node defaultProvider::fromSSO
@aws-sdk/credential-provider-node defaultProvider::fromIni
@aws-sdk/credential-provider-ini fromIni
@aws-sdk/credential-provider-ini resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-node defaultProvider::fromProcess
@aws-sdk/credential-provider-process fromProcess
@aws-sdk/credential-provider-node defaultProvider::fromTokenFile
@aws-sdk/credential-provider-web-identity fromTokenFile
@aws-sdk/credential-provider-node defaultProvider::remoteProvider
@aws-sdk/credential-provider-node remoteProvider::fromHttp/fromContainerMetadata
@aws-sdk/credential-provider-http fromHttp

☝️ Succeeds to get creds but not assume the role in the config file

In other news I've had a small win. I've managed to patch both your change here aws/aws-sdk-js-v3#6132 AND the changes in this PR into my test harness. The role assumption is successful when i use the following config (the one i quoted above using source_profile still fails):

    # Content of the AWS Config file. This works!!!
    [default]
    role_arn = arn:aws:iam::1234567890:role/some-role
    credential_source=EcsContainer

...and the logs to go with it:

@aws-sdk/credential-provider-node defaultProvider::fromEnv
@aws-sdk/credential-provider-env fromEnv
@aws-sdk/credential-provider-node defaultProvider::fromSSO
@aws-sdk/credential-provider-node defaultProvider::fromIni
@aws-sdk/credential-provider-ini fromIni
@aws-sdk/credential-provider-ini resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-ini credential_source EcsContainer
@aws-sdk/credential-provider-http fromHttp

@kuhe
Copy link
Contributor

kuhe commented May 24, 2024

I think I have a full picture of the issue now, and fixes will be applied in the linked PRs.

Here is a summary:

  • 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

    • however, this is misleading, since the default credential provider chain in AWS SDKs for Node.js has many steps, and only reports the final terminal exception. There are actually many thrown & caught exceptions along the way as each credential provider within the chain is attempted.
    • as a fix, we will improve the logging of the AWS SDK's node default credential chain, chore: add logging for CredentialsProviderError #1290 & downstream in aws-sdk-js-v3 PR 6132.
  • 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.

Both bugs are planned to be addressed by aws/aws-sdk-js-v3#6132. Release is pending review and testing.


After the patch the credential debug output should look like this:

@aws-sdk/credential-provider-node defaultProvider::fromEnv
@aws-sdk/credential-provider-env - fromEnv
@smithy/property-provider -> Unable to find environment variable credentials.
@aws-sdk/credential-provider-node defaultProvider::fromSSO
@smithy/property-provider -> Skipping SSO provider in default chain (inputs do not include SSO fields).
@aws-sdk/credential-provider-node defaultProvider::fromIni
@aws-sdk/credential-provider-ini - fromIni
    default isAssumeRoleWithSourceProfile source_profile=cluster_role
@aws-sdk/credential-provider-ini - resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-ini - finding credential resolver using source_profile=[cluster_role]
    cluster_role isCredentialSourceProfile credential_source=EcsContainer
@aws-sdk/credential-provider-ini - resolveAssumeRoleCredentials (STS)
@aws-sdk/credential-provider-ini - finding credential resolver using profile=[cluster_role]
@aws-sdk/credential-provider-ini - credential_source is EcsContainer
@aws-sdk/credential-provider-http - fromHttp
@aws-sdk/client-sts::resolveRegion accepting first of: undefined (provider) <<YOUR_REGION>> (parent client) us-east-1 (STS default)

That is, it should go directly from resolveAssumeRoleCredentials (STS) to fromHttp. With your PR's change, it looks like it is exiting the ini file-based provider and hitting the fromHttp/fromContainerMetadata providers later in the chain. In that scenario it is only seeing the environment variables and resolving container credentials in a terminal way without seeing the config file and its role_arn.

@kuhe kuhe added pending-release This issue will be fixed by an approved PR that hasn't been released yet. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels May 24, 2024
Copy link
Contributor

@kuhe kuhe left a comment

Choose a reason for hiding this comment

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

will be fixed in aws/aws-sdk-js-v3#6132

@jgrigg
Copy link
Author

jgrigg commented May 24, 2024

That all makes sense George thank you for following up on this 🙏

@jgrigg jgrigg closed this May 24, 2024
@kuhe kuhe removed the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label May 28, 2024
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.

None yet

2 participants