-
Notifications
You must be signed in to change notification settings - Fork 50
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
misc: credentials search precedence change #1434
base: v1.4
Are you sure you want to change the base?
Changes from all commits
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. | ||
Comment on lines
+5
to
+7
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. Opinion: If we're going to get in a more consistent habit of stacking up breaking changes I think we should either set a date by when the new minor version will be released (hard) or change our messaging to indicate not so much the precise date but just the version (easy). We want to give users as much lead time as possible to read the announcement and prepare for the break and we can always update the announcement to include the date once it's firm. |
||
|
||
# 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. | ||
Comment on lines
+9
to
+22
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 | ||
Comment on lines
+9
to
+32
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: Separating the list into two seems confusing to me. Is there a reason to favor this over a single list? |
||
|
||
# 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. | ||
Comment on lines
+34
to
+38
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. Suggestion: Let's include an example of how users can configure their own creds chain if they truly wish to preserve the old precedence. |
||
|
||
# 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). |
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.) |
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
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 |
||
|
||
// 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") | ||
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. Nit: Throwing the chain-level no-creds-in-profile exception feels like a concern which should live outside this method (whose job it is to return process credentials). |
||
|
||
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
271
to
275
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. 👍 |
||
|
@@ -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.
Nit: "the AWS shared config files" → "AWS shared config files".