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

deps(auth): remove dependence on deprecated and outdated @aws-sdk/* packages. #6474

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Jan 30, 2025

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 deprecated
  • 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 #6439 for more information.

Solution

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

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

Testing

  • Manually verify this role assumption works by following the steps here.
  • Manually verify MFA works via adapting this. (Used DuoMobile)
  • Add unit tests with API calls stubbed.

Future Work

  • There are two tests that can now be re-enabled because of this version bump, undoing 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.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@Hweinstock Hweinstock changed the title deps(auth): remove dependence on deprecated and outdated @aws-sdk/* packages. (WIP) deps(auth): remove dependence on deprecated and outdated @aws-sdk/* packages. Feb 3, 2025
@Hweinstock Hweinstock marked this pull request as ready for review February 3, 2025 15:14
@Hweinstock Hweinstock requested a review from a team as a code owner February 3, 2025 15:14
@Hweinstock Hweinstock marked this pull request as draft February 3, 2025 15:46
@Hweinstock Hweinstock marked this pull request as ready for review February 3, 2025 16:09
// No role to assume, return static credentials.
if (!profile.role_arn) {
return {
accessKeyId: profile.aws_access_key_id!,
Copy link
Contributor

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?

Copy link
Contributor Author

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:

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'
}
}
and then only call this code(makeSharedIniFileCredentialsProvider) when we have the right conditions:
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.

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.

2 participants