-
Notifications
You must be signed in to change notification settings - Fork 55
misc: credentials search precedence change #1434
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
Changes from 3 commits
b426037
23e25d7
6ef7a60
15337bd
a78d614
0e77b6a
ec792a5
75e9644
de5dc7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "id": "0b5b53ab-70c0-4c1b-a445-8663ae86d6d1", | ||
| "type": "misc", | ||
| "description": "The order of credentials resolution in config files has been updated to: static credentials, assume role with source profile OR assume role with named provider, web identity token, SSO session, legacy SSO, process" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "id": "99a099e1-26c1-4ba1-b0d3-435609ea4e94", | ||
| "type": "misc", | ||
| "description": "The order of credentials resolution in the credentials provider chain has been updated to: system properties, environment variables, web identity tokens, profile, ECS, EC2" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| An upcoming release of the **AWS SDK for Kotlin** will change the order of | ||
| credentials resolution for the [default credentials provider chain](https://docs.aws.amazon.com/sdk-for-kotlin/latest/developer-guide/credential-providers.html#default-credential-provider-chain) | ||
| and the order of credentials resolution for the AWS shared config files. | ||
|
||
|
|
||
| # Release date | ||
|
|
||
| This feature will ship with the **v1.4.x** release on xx/xx/xxxx. | ||
|
||
|
|
||
| # What's changing | ||
|
|
||
| The SDK will be changing the order in which credentials are resolved when | ||
| using the default credentials provider chain. The new order will be: | ||
|
|
||
| 1. System properties | ||
| 2. Environment variables | ||
| 3. Assume role with web identity token | ||
| 4. Shared credentials and config files (profile) | ||
| 5. Amazon ECS container credentials | ||
| 6. Amazon EC2 Instance Metadata Service | ||
|
|
||
| The [default credentials provider chain documentation](https://docs.aws.amazon.com/sdk-for-kotlin/latest/developer-guide/credential-providers.html#default-credential-provider-chain) | ||
| contains more details on each credential source. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: Somewhere in this announcement we should show users what the old order was and how this differs. Could be in a table that lists the old/new orders in different columns, parentheticals on the numbered lists for items that have changed position, etc. |
||
|
|
||
| The SDK will also be changing the order in which credentials are resolved from | ||
| in the shared credentials and config files. The new order will be: | ||
|
|
||
| 1. Static credentials | ||
| 2. Assume role with source profile OR assume role with named provider (mutually exclusive) | ||
| 3. Web identity token | ||
| 4. SSO session | ||
| 5. Legacy SSO | ||
| 6. Process | ||
|
||
|
|
||
| # How to migrate | ||
|
|
||
| 1. Upgrade all of your AWS SDK for Kotlin dependencies to **v.1.4.x**. | ||
| 2. Verify that the changes to the default credentials provider chain and credentials files do not introduce any issues in your program. | ||
| 3. If issues arise review the new credentials resolution order and adjust your configuration as needed. | ||
|
||
|
|
||
| # Feedback | ||
|
|
||
| If you have any questions concerning this change, please feel free to engage | ||
| with us in this discussion. If you encounter a bug with these changes, please | ||
| [file an issue](https://github.com/awslabs/aws-sdk-kotlin/issues/new/choose). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: It feels like the precedence logic is a bit spread out in this file (some of which predates your changes). How easy/possible would it be to centralize the logic into a single place, preferably as a data structure like a list, so that we had a single place to look for the ordering specifically? (Not saying to move the actual resolution logic into a single place, just the precedence order.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible, and I think it would make this file more readable. I think it would take maybe a day or two of just working on this to refactor it to use a list and have the assume role logic fit into that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK then unless you've already started on it then let's skip the refactor for now. Let's just address the other comments and get this shipped. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,16 @@ internal data class ProfileChain( | |
| val roles: List<RoleArn>, | ||
| ) { | ||
| companion object { | ||
| /** | ||
| * Resolves profile chain with the following precedence: | ||
| * | ||
| * 1. Static credentials | ||
| * 2. Assume role with source profile OR assume role with named provider (mutually exclusive) | ||
| * 3. Web ID token file & role arn | ||
| * 4. SSO session | ||
| * 5. Legacy SSO | ||
| * 6. Process | ||
| */ | ||
| internal fun resolve(config: AwsSharedConfig): ProfileChain { | ||
| val visited = mutableSetOf<String>() | ||
| val chain = mutableListOf<RoleArn>() | ||
|
|
@@ -53,11 +63,9 @@ internal data class ProfileChain( | |
| throw ProviderConfigurationException("profile formed an infinite loop: ${visited.joinToString(separator = " -> ")} -> $sourceProfileName") | ||
| } | ||
|
|
||
| // when chaining assume role profiles, SDKs MUST terminate the chain as soon as they hit a profile with static credentials | ||
| if (visited.size > 1) { | ||
| leaf = profile.staticCredsOrNull() | ||
| if (leaf != null) break@loop | ||
| } | ||
| // static credentials have the highest precedence | ||
| leaf = profile.staticCredsOrNull() | ||
| if (leaf != null) break@loop | ||
|
Comment on lines
-56
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: We previously performed the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change in order makes it unnecessary. The check was there because static credentials were of highest precedence in the assume role profile only. Now that static credentials are always highest precedence there's no need to check if we're in the |
||
|
|
||
| // the existence of `role_arn` is the only signal that multiple profiles will be chained | ||
| val roleArn = profile.roleArnOrNull() | ||
|
|
@@ -132,8 +140,11 @@ internal const val SSO_SESSION = "sso_session" | |
| internal const val CREDENTIAL_PROCESS = "credential_process" | ||
|
|
||
| private fun AwsProfile.roleArnOrNull(): RoleArn? { | ||
| val validSource = contains(CREDENTIAL_SOURCE) || contains(SOURCE_PROFILE) | ||
|
|
||
| // chained roles have higher precedence than web id token file | ||
| // web identity tokens are leaf providers, not chained roles | ||
| if (contains(WEB_IDENTITY_TOKEN_FILE)) return null | ||
| if (!validSource && contains(WEB_IDENTITY_TOKEN_FILE)) return null | ||
|
|
||
| val roleArn = getOrNull(ROLE_ARN) ?: return null | ||
|
|
||
|
|
@@ -237,11 +248,12 @@ private fun AwsProfile.ssoSessionCreds(config: AwsSharedConfig): LeafProviderRes | |
| } | ||
|
|
||
| /** | ||
| * Attempt to load [LeafProvider.Process] from the current profile or `null` if the current profile does not contain | ||
| * Attempt to load [LeafProvider.Process] from the current profile or exception if the current profile does not contain | ||
| * a credentials process command to execute | ||
| */ | ||
| private fun AwsProfile.processCreds(): LeafProviderResult? { | ||
| if (!contains(CREDENTIAL_PROCESS)) return null | ||
| private fun AwsProfile.processCreds(): LeafProviderResult { | ||
| // Process is last in precedence - credentials not found means no credentials in profile | ||
| if (!contains(CREDENTIAL_PROCESS)) return LeafProviderResult.Err("profile ($name) did not contain credential information") | ||
0marperez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| val credentialProcess = getOrNull(CREDENTIAL_PROCESS) ?: return LeafProviderResult.Err("profile ($name) missing `$CREDENTIAL_PROCESS`") | ||
|
|
||
|
|
@@ -257,7 +269,7 @@ private fun AwsProfile.staticCreds(): LeafProviderResult { | |
| val secretKey = getOrNull(AWS_SECRET_ACCESS_KEY) | ||
| val accountId = getOrNull(AWS_ACCOUNT_ID) | ||
| return when { | ||
| accessKeyId == null && secretKey == null -> LeafProviderResult.Err("profile ($name) did not contain credential information") | ||
| accessKeyId == null && secretKey == null -> LeafProviderResult.Err("profile ($name) missing `aws_access_key_id` & `aws_secret_access_key`") | ||
| accessKeyId == null -> LeafProviderResult.Err("profile ($name) missing `aws_access_key_id`") | ||
| secretKey == null -> LeafProviderResult.Err("profile ($name) missing `aws_secret_access_key`") | ||
| else -> { | ||
|
Comment on lines
292
to
296
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment: Nice improvement to this error message. 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
|
|
@@ -315,7 +327,6 @@ private fun AwsProfile.leafProvider(config: AwsSharedConfig): LeafProvider { | |
| return webIdentityTokenCreds() | ||
| .orElse { ssoSessionCreds(config) } | ||
| .orElse(::legacySsoCreds) | ||
| .orElse(::processCreds) | ||
| .unwrapOrElse(::staticCreds) | ||
| .unwrapOrElse(::processCreds) | ||
| .unwrap() | ||
| } | ||
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.
correctness: Add requiresMinorVersionBump