-
Notifications
You must be signed in to change notification settings - Fork 494
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
deps(auth): remove dependence on deprecated and outdated @aws-sdk/*
packages.
#6474
base: master
Are you sure you want to change the base?
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.
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-imds
has since been deprecatedfromIni
from@aws-sdk/credential-provider-ini
no longer supports passing aloadedConfig
.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 #6439 for more information.Solution
fromIni
withloadedConfig
option.@aws-sdk/credential-provider-ini
since its no longer used.@aws-sdk/credential-provider-env
since this was installed as part of@aws-sdk-credential-provider-ini
before.Custom Credentials Provider
Before, we used
fromIni
with theloadedConfig
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 theloadedConfig
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.Testing
Future Work
feature/x
branches will not be squash-merged at release time.