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.

Sign up for GitHub

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

Jump to bottom

misc: credentials search precedence change #1434

Merged
merged 9 commits into from
Nov 4, 2024
Merged

misc: credentials search precedence change #1434

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b426037
fix: credentials search precedence
0marperez Oct 7, 2024
23e25d7
Changelogs
0marperez Oct 7, 2024
6ef7a60
Breaking announcement temp
0marperez Oct 7, 2024
15337bd
requiresMinorVersionBump set to true in changelog, StsWebIdentityProv…
0marperez Oct 10, 2024
a78d614
Self review
0marperez Oct 10, 2024
0e77b6a
misc: add lazily initialized creds provider and use it in default cre…
0marperez Oct 11, 2024
ec792a5
Make providerName LazilyInitializedCredentialsProvider specific
0marperez Oct 11, 2024
75e9644
Override toString() on every implementation of CredentialsProvider
0marperez Oct 11, 2024
de5dc7f
misc: credentials business metrics (#1442)
0marperez Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/0b5b53ab-70c0-4c1b-a445-8663ae86d6d1.json
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correctness: Add requiresMinorVersionBump

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"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",
"requiresMinorVersionBump": true
}
6 changes: 6 additions & 0 deletions .changes/99a099e1-26c1-4ba1-b0d3-435609ea4e94.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"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",
"requiresMinorVersionBump": true
}
109 changes: 109 additions & 0 deletions .changes/announcement.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
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 AWS shared config files.

# Release date

This change will be included in the upcoming **v1.4.x** release, expected in the
upcoming months.

# What's changing

The order of credentials resolution for the default credentials provider chain,
and the order of credentials resolution for AWS shared config files (profile chain).

## Default credentials provider chain

The table below outlines the current and new order in which the SDK will
resolve credentials from the default credentials provider chain.

| # | Current Order | New Order |
|---|------------------------------------------------------------------------|------------------------------------------------------------------------|
| 1 | System properties | System properties |
| 2 | Environment variables | Environment variables |
| 3 | **Shared credentials and config files (profile credentials provider)** | **Assume role with web identity token** |
| 4 | **Assume role with web identity token** | **Shared credentials and config files (profile credentials provider)** |
| 5 | Amazon ECS container credentials | Amazon ECS container credentials |
| 6 | Amazon EC2 Instance Metadata Service | 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.

## Profile chain

The table below outlines the current and new order in which the SDK will
resolve credentials from AWS shared config files.

| # | Current Order | New Order |
|---|----------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------|
| 1 | **Assume role with source profile OR assume role with named provider (mutually exclusive)** | **Static credentials** |
| 2 | Web identity token | **Assume role with source profile OR assume role with named provider (mutually exclusive)** |
| 3 | SSO session | Web identity token |
| 4 | Legacy SSO | SSO session |
| 5 | Process | Legacy SSO |
| 6 | **Static credentials (moves up to #1 when in a source profile, shifting other credential sources down)** | 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 profile chain do not introduce any issues in your program.
3. If issues arise review the new credentials resolution order, the subsections below, and adjust your configuration as needed.

## Default credentials provider chain

You can preserve the current default credentials provider chain behavior by setting
the credentials provider to a credentials provider chain with the current order, e.g.

```kotlin
S3Client{
credentialsProvider = CredentialsProviderChain(
SystemPropertyCredentialsProvider(),
EnvironmentCredentialsProvider(),
StsWebIdentityProvider(),
ProfileCredentialsProvider(),
EcsCredentialsProvider(),
ImdsCredentialsProvider(),
)
}
```

## Profile credentials provider

The order in which credentials are resolved for shared credentials and config
files cannot be customized. If your AWS config file(s) contain multiple valid
credential sources within a single profile, you may need to update them to align
with the new resolution order. For example, config file `A` should be updated to
match config file `B`. This is necessary because static credentials will now
take precedence and be selected before assume role credentials with a source profile.
Similar adjustments to your configuration may be necessary to maintain current
behavior. Use the new order as a guide for any required changes.

Config file `A`
```ini
[default]
role_arn = arn:aws:iam::123456789:role/Role
source_profile = A
aws_access_key_id = 0
aws_secret_access_key = 0

[profile A]
aws_access_key_id = 1
aws_secret_access_key = 2
```

Config file `B`
```ini
[default]
role_arn = arn:aws:iam::123456789:role/Role
source_profile = A

[profile A]
aws_access_key_id = 1
aws_secret_access_key = 2
```

# 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 when
released, please [file an issue](https://github.com/awslabs/aws-sdk-kotlin/issues/new/choose).
8 changes: 8 additions & 0 deletions aws-runtime/aws-config/api/aws-config.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,14 @@ public final class aws/sdk/kotlin/runtime/auth/credentials/StsWebIdentityCredent
public static synthetic fun fromEnvironment-TUY-ock$default (Laws/sdk/kotlin/runtime/auth/credentials/StsWebIdentityCredentialsProvider$Companion;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;JLaws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/http/engine/HttpClientEngine;ILjava/lang/Object;)Laws/sdk/kotlin/runtime/auth/credentials/StsWebIdentityCredentialsProvider;
}

public final class aws/sdk/kotlin/runtime/auth/credentials/StsWebIdentityProvider : aws/smithy/kotlin/runtime/auth/awscredentials/CloseableCredentialsProvider {
public fun <init> ()V
public fun <init> (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/http/engine/HttpClientEngine;Ljava/lang/String;)V
public synthetic fun <init> (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/http/engine/HttpClientEngine;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun close ()V
public fun resolve (Laws/smithy/kotlin/runtime/collections/Attributes;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
}

public final class aws/sdk/kotlin/runtime/auth/credentials/SystemPropertyCredentialsProvider : aws/smithy/kotlin/runtime/auth/awscredentials/CredentialsProvider {
public fun <init> ()V
public fun <init> (Lkotlin/jvm/functions/Function1;)V
Expand Down
19 changes: 10 additions & 9 deletions ...fig/common/src/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ import aws.smithy.kotlin.runtime.util.PlatformProvider
*
* Resolution order:
*
* 1. Environment variables ([EnvironmentCredentialsProvider])
* 2. Profile ([ProfileCredentialsProvider])
* 1. System properties ([SystemPropertyCredentialsProvider])
* 2. Environment variables ([EnvironmentCredentialsProvider])
* 3. Web Identity Tokens ([StsWebIdentityCredentialsProvider]]
* 4. ECS (IAM roles for tasks) ([EcsCredentialsProvider])
* 5. EC2 Instance Metadata (IMDSv2) ([ImdsCredentialsProvider])
* 4. Profile ([ProfileCredentialsProvider])
* 5. ECS (IAM roles for tasks) ([EcsCredentialsProvider])
* 6. EC2 Instance Metadata (IMDSv2) ([ImdsCredentialsProvider])
*
* The chain is decorated with a [CachedCredentialsProvider].
*
Expand All @@ -54,9 +55,9 @@ public class DefaultChainCredentialsProvider constructor(
private val chain = CredentialsProviderChain(
SystemPropertyCredentialsProvider(platformProvider::getProperty),
EnvironmentCredentialsProvider(platformProvider::getenv),
ProfileCredentialsProvider(profileName = profileName, platformProvider = platformProvider, httpClient = engine, region = region),
// STS web identity provider can be constructed from either the profile OR 100% from the environment
StsWebIdentityProvider(platformProvider = platformProvider, httpClient = engine, region = region),
ProfileCredentialsProvider(profileName = profileName, platformProvider = platformProvider, httpClient = engine, region = region),
EcsCredentialsProvider(platformProvider, engine),
ImdsCredentialsProvider(
client = lazy {
Expand Down Expand Up @@ -85,10 +86,10 @@ public class DefaultChainCredentialsProvider constructor(
* Wrapper around [StsWebIdentityCredentialsProvider] that delays any exceptions until [resolve] is invoked.
* This allows it to be part of the default chain and any failures result in the chain to move onto the next provider.
*/
private class StsWebIdentityProvider(
val platformProvider: PlatformProvider = PlatformProvider.System,
val httpClient: HttpClientEngine? = null,
val region: String? = null,
public class StsWebIdentityProvider(
private val platformProvider: PlatformProvider = PlatformProvider.System,
private val httpClient: HttpClientEngine? = null,
private val region: String? = null,
) : CloseableCredentialsProvider {
override suspend fun resolve(attributes: Attributes): Credentials {
val wrapped = StsWebIdentityCredentialsProvider.fromEnvironment(platformProvider = platformProvider, httpClient = httpClient, region = region)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StsWebIdentityCredentialsProvider is already public, why do we need to make the wrapper class public too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper delays any exceptions until resolve() is called so it can be part of a credentials provider chain. Without the wrapper some exceptions are thrown outside of the resolve() call and not caught by the credentials provider chain.

For example using the client below, if there's no valid env config for StsWebIdentityCredentialsProvider an exception is thrown early.

S3Client{
    region = "us-west-2"
    credentialsProvider = CredentialsProviderChain(
        SystemPropertyCredentialsProvider(),
        EnvironmentCredentialsProvider(),
        StsWebIdentityCredentialsProvider.fromEnvironment(),
        ProfileCredentialsProvider(),
        EcsCredentialsProvider(),
        ImdsCredentialsProvider(),
    )
}.use { client ->
    client.listBuckets(ListBucketsRequest{})
}
Exception in thread "main" aws.sdk.kotlin.runtime.auth.credentials.ProviderConfigurationException: Required field `roleArn` could not be automatically inferred for StsWebIdentityCredentialsProvider. Either explicitly pass a value, set the environment variable `AWS_ROLE_ARN`, or set the JVM system property `aws.roleArn`

Instead of what we want:

Exception in thread "main" aws.smithy.kotlin.runtime.identity.IdentityProviderException: No identity could be resolved from the chain: CredentialsProviderChain -> SystemPropertyCredentialsProvider -> EnvironmentCredentialsProvider -> StsWebIdentityProvider -> ProfileCredentialsProvider -> EcsCredentialsProvider -> ImdsCredentialsProvider .......

Copy link
Contributor Author

@0marperez 0marperez Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I think the name doesn't help, maybe we could call it CredentialsChainStsWebIdentityProvider or something similar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for the chain to fail early when users are manually configuring a custom chain. We just don't want it to fail early in the default chain because not every user will have a valid environment config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid failing early in any credentials provider chain not just the default one. IMO a credentials provider chain is supposed to keep looking for credentials and only fail once the options are exhausted. If we fail early it could be seen as a bug.

Expand Down
35 changes: 23 additions & 12 deletions ...ime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The 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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Up @@ -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>()
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: We previously performed the staticCredsOrNull check only if we'd already visited at least one node. Was that always wrong or did something change that makes it unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 1 + n visited profile.


// the existence of `role_arn` is the only signal that multiple profiles will be chained
val roleArn = profile.roleArnOrNull()
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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`")

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: Nice improvement to this error message. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Expand Down Expand Up @@ -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()
}
Loading
Loading