-
Notifications
You must be signed in to change notification settings - Fork 749
deps(auth): remove dependence on deprecated and outdated @aws-sdk/* packages.
#6474
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
Conversation
|
@aws-sdk/* packages. (WIP)@aws-sdk/* packages.
| // No role to assume, return static credentials. | ||
| if (!profile.role_arn) { | ||
| return { | ||
| accessKeyId: profile.aws_access_key_id!, |
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.
(I know very little about auth but had a couple questions). It looks like you're asserting that profile.aws_access_key_id is not undefined. Is that for sure? Or just to satisfy the accessKeyId type? I'm wonder if it would make sense to warn if access key id/secret access key aren't defined?
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.
My understanding is that these fields are required. Authenticating from a config file can be done by directly using the access key and secret key, or using it to assume an IAM role for authentication. However, either way the access key and secret key is required in the current profile or the source profile.
To enforce this, we validate the config file here:
aws-toolkit-vscode/packages/core/src/auth/providers/sharedCredentialsProvider.ts
Lines 178 to 201 in c358a2a
| public validate(): string | undefined { | |
| if (hasProps(this.profile, SharedCredentialsKeys.CREDENTIAL_SOURCE)) { | |
| return this.validateSourcedCredentials() | |
| } else if (hasProps(this.profile, SharedCredentialsKeys.ROLE_ARN)) { | |
| return this.validateSourceProfileChain() | |
| } else if (hasProps(this.profile, SharedCredentialsKeys.CREDENTIAL_PROCESS)) { | |
| // No validation. Don't check anything else. | |
| return undefined | |
| } else if ( | |
| hasProps(this.profile, SharedCredentialsKeys.AWS_ACCESS_KEY_ID) || | |
| hasProps(this.profile, SharedCredentialsKeys.AWS_SECRET_ACCESS_KEY) || | |
| hasProps(this.profile, SharedCredentialsKeys.AWS_SESSION_TOKEN) | |
| ) { | |
| return validateProfile( | |
| this.profile, | |
| SharedCredentialsKeys.AWS_ACCESS_KEY_ID, | |
| SharedCredentialsKeys.AWS_SECRET_ACCESS_KEY | |
| ) | |
| } else if (isSsoProfile(this.profile)) { | |
| return undefined | |
| } else { | |
| return 'not supported by the Toolkit' | |
| } | |
| } |
makeSharedIniFileCredentialsProvider) when we have the right conditions: aws-toolkit-vscode/packages/core/src/auth/providers/sharedCredentialsProvider.ts
Lines 300 to 350 in c358a2a
| private makeCredentialsProvider(loadedCreds?: ParsedIniData): AWS.CredentialProvider { | |
| const logger = getLogger() | |
| if (hasProps(this.profile, SharedCredentialsKeys.CREDENTIAL_SOURCE)) { | |
| logger.verbose( | |
| `Profile ${this.profileName} contains ${SharedCredentialsKeys.CREDENTIAL_SOURCE} - treating as Environment Credentials` | |
| ) | |
| return this.makeSourcedCredentialsProvider() | |
| } | |
| if (hasProps(this.profile, SharedCredentialsKeys.ROLE_ARN)) { | |
| logger.verbose( | |
| `Profile ${this.profileName} contains ${SharedCredentialsKeys.ROLE_ARN} - treating as regular Shared Credentials` | |
| ) | |
| return this.makeSharedIniFileCredentialsProvider(loadedCreds) | |
| } | |
| if (hasProps(this.profile, SharedCredentialsKeys.CREDENTIAL_PROCESS)) { | |
| logger.verbose( | |
| `Profile ${this.profileName} contains ${SharedCredentialsKeys.CREDENTIAL_PROCESS} - treating as Process Credentials` | |
| ) | |
| return fromProcess({ profile: this.profileName }) | |
| } | |
| if (hasProps(this.profile, SharedCredentialsKeys.AWS_SESSION_TOKEN)) { | |
| logger.verbose( | |
| `Profile ${this.profileName} contains ${SharedCredentialsKeys.AWS_SESSION_TOKEN} - treating as regular Shared Credentials` | |
| ) | |
| return this.makeSharedIniFileCredentialsProvider(loadedCreds) | |
| } | |
| if (hasProps(this.profile, SharedCredentialsKeys.AWS_ACCESS_KEY_ID)) { | |
| logger.verbose( | |
| `Profile ${this.profileName} contains ${SharedCredentialsKeys.AWS_ACCESS_KEY_ID} - treating as regular Shared Credentials` | |
| ) | |
| return this.makeSharedIniFileCredentialsProvider(loadedCreds) | |
| } | |
| if (isSsoProfile(this.profile)) { | |
| logger.verbose(`Profile ${this.profileName} is an SSO profile - treating as SSO Credentials`) | |
| return this.makeSsoCredentaislProvider() | |
| } | |
| logger.error(`Profile ${this.profileName} did not contain any supported properties`) | |
| throw new Error(`Shared Credentials profile ${this.profileName} is not supported`) | |
| } |
Its a little hard to understand because this code lives in two places, but I don't believes its possible this function is called without the necessary keys existing.
|
Should we link to those somehow in https://github.com/aws/aws-toolkit-vscode/blob/master/docs/faq-credentials.md ? Gathering relevant doc for credentials is difficult, and it looks like you found some good references. |
justinmk3
left a comment
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.
🚀
|
Added some documentation with the links I used above. Also, do not merge this yet, need to confirm with sagemaker this doesn't affect them. |
|
/runintegrationtests |
… packages. (aws#6474) The auth code relies on old versions of `@aws-sdk/*` that have since been deprecated or are no longer backward compatible, making versions bumps impossible. - `@aws-sdk/credential-provider-imds` has since been [deprecated](https://www.npmjs.com/package/@aws-sdk/credential-provider-imds) - `fromIni` from `@aws-sdk/credential-provider-ini` no longer supports passing a `loadedConfig`. - `AssumeRoleParams` is no longer exported by `@aws-sdk/credential-provider-ini`. We need to be able to bump these `@aws-sdk/*` package versions to continue to consume newer generated clients. Being pinned to older versions is also a security risk. See aws#6439 for more information. - write custom credentials provider to replace `fromIni` with `loadedConfig` option. - drop dependency on `@aws-sdk/credential-provider-ini` since its no longer used. - add direct dependency on `@aws-sdk/credential-provider-env` since this was installed as part of `@aws-sdk-credential-provider-ini` before. - Fix many (not all) of the deprecation warnings in auth code related to credentials provider. Before, we used `fromIni` with the `loadedConfig` option which allows us to avoid reading the config file from disk on each credentials fetch and allows us to merge the current credentials with those found in the `.ini` file. To achieve the same behavior without the `loadedConfig` option, we need to write our own credentials provider that supports MFA and role assumption, and returns the desired merged credentials, rather than reading from disk. - Manually verify this role assumption works by following the steps [here](https://docs.aws.amazon.com/sdkref/latest/guide/access-assume-role.html). - Manually verify MFA works via adapting [this](https://docs.aws.amazon.com/cli/v1/userguide/cli-configure-role.html#:~:text=This%20policy%20allows%20the%20user,they%20authenticate%20by%20using%20MFA.&text=Next%2C%20add%20a%20line%20to,by%20the%20role's%20trust%20policy.&text=The%20mfa_serial%20setting%20can%20take,command%20with%20this%20profile%20fails.&text=The%20second%20profile%20entry%2C%20role,%22:%20%5B%20%7B%20...). (Used DuoMobile) - Add unit tests with API calls stubbed. - There are two tests that can now be re-enabled because of this version bump, undoing aws@db27ebb - The steps to test role assumption could become an integ/e2e test. Right now requires setting many resources up in console, but perhaps this can all be done by the SDKs with an account on admin access. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
|
Seems to have caused a regression: |
Problem
The auth code relies on old versions of
@aws-sdk/*that have since been deprecated or are no longer backward compatible, making versions bumps impossible.@aws-sdk/credential-provider-imdshas since been deprecatedfromInifrom@aws-sdk/credential-provider-inino longer supports passing aloadedConfig.AssumeRoleParamsis no longer exported by@aws-sdk/credential-provider-ini.We need to be able to bump these
@aws-sdk/*package versions to continue to consume newer generated clients. Being pinned to older versions is also a security risk. See #6439 for more information.Solution
fromIniwithloadedConfigoption.@aws-sdk/credential-provider-inisince its no longer used.@aws-sdk/credential-provider-envsince this was installed as part of@aws-sdk-credential-provider-inibefore.Custom Credentials Provider
Before, we used
fromIniwith theloadedConfigoption which allows us to avoid reading the config file from disk on each credentials fetch and allows us to merge the current credentials with those found in the.inifile. To achieve the same behavior without theloadedConfigoption, we need to write our own credentials provider that supports MFA and role assumption, and returns the desired merged credentials, rather than reading from disk.Testing
Future Work
feature/xbranches will not be squash-merged at release time.