misc: credentials search precedence change #1434

0marperez · 2024-10-07T15:43:59Z

Issue #

Description of changes

-Changes to the credential provider chain and profile credentials provider to abide to new specification.
-Old tests were modified to accommodate the new order and precedence specific tests were added.
-Draft of breaking change announcement

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-10-07T15:52:28Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

ianbotsf · 2024-10-07T18:19:07Z

.changes/announcement.md

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


Nit: "the AWS shared config files" → "AWS shared config files".

ianbotsf · 2024-10-07T18:21:26Z

.changes/announcement.md

+# Release date
+
+This feature will ship with the **v1.4.x** release on xx/xx/xxxx.


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.

ianbotsf · 2024-10-07T18:23:36Z

.changes/announcement.md

+# 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.


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.

ianbotsf · 2024-10-07T18:25:14Z

.changes/announcement.md

+# 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.


Suggestion: Let's include an example of how users can configure their own creds chain if they truly wish to preserve the old precedence.

ianbotsf · 2024-10-07T18:26:43Z

.changes/announcement.md

+# 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.
+
+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


Question: Separating the list into two seems confusing to me. Is there a reason to favor this over a single list?

I didn't combine them because of concerns I had about how users would tell apart profile only credentials locations. I think maybe using 4.x for the profile chain would work or indentation.

It can also be made clearer if you separate these into two separate sections (## Default credentials provider chain, ## Profile credentials provider)

ianbotsf · 2024-10-07T18:29:00Z

...untime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt

-                // 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


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?

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.

...untime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt

ianbotsf · 2024-10-07T20:12:21Z

...untime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt

    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: Nice improvement to this error message. 👍

ianbotsf · 2024-10-07T20:14:49Z

...untime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt

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

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.

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.

lauzadis

correctness: this PR is missing the emission of business metrics

lauzadis · 2024-10-08T15:49:09Z

.changes/0b5b53ab-70c0-4c1b-a445-8663ae86d6d1.json

correctness: Add requiresMinorVersionBump

lauzadis · 2024-10-08T15:50:21Z

.changes/announcement.md

+# 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.
+
+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


It can also be made clearer if you separate these into two separate sections (## Default credentials provider chain, ## Profile credentials provider)

…ider now public, announcement draft updates

github-actions · 2024-10-10T13:48:01Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

lauzadis · 2024-10-10T16:01:08Z

...config/common/src/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProvider.kt

+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)


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

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

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

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

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.

…dentials provider chain

github-actions · 2024-10-11T15:58:04Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

github-actions · 2024-10-11T21:37:42Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

sonarcloud · 2024-11-04T20:06:10Z

Quality Gate failed

Failed conditions
5.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

github-actions · 2024-11-04T20:15:08Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez added 3 commits October 6, 2024 23:02

fix: credentials search precedence

b426037

Changelogs

23e25d7

Breaking announcement temp

6ef7a60

0marperez mentioned this pull request Oct 7, 2024

fix: default credentials provider chain order #1324

Closed

0marperez marked this pull request as ready for review October 7, 2024 16:02

0marperez requested a review from a team as a code owner October 7, 2024 16:02

ianbotsf requested changes Oct 7, 2024

View reviewed changes

lauzadis reviewed Oct 8, 2024

View reviewed changes

0marperez added 2 commits October 10, 2024 01:06

requiresMinorVersionBump set to true in changelog, StsWebIdentityProv…

15337bd

…ider now public, announcement draft updates

Self review

a78d614

0marperez requested review from ianbotsf and lauzadis October 10, 2024 15:29

lauzadis reviewed Oct 10, 2024

View reviewed changes

misc: add lazily initialized creds provider and use it in default cre…

0e77b6a

…dentials provider chain

0marperez added 2 commits October 11, 2024 16:57

Make providerName LazilyInitializedCredentialsProvider specific

ec792a5

Override toString() on every implementation of CredentialsProvider

75e9644

ianbotsf approved these changes Oct 11, 2024

View reviewed changes

misc: credentials business metrics (#1442)

de5dc7f

0marperez merged commit e297a98 into v1.4 Nov 4, 2024
10 of 11 checks passed

0marperez deleted the credentials-chain-order branch November 4, 2024 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: credentials search precedence change #1434

misc: credentials search precedence change #1434

0marperez commented Oct 7, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024

ianbotsf Oct 7, 2024

ianbotsf Oct 7, 2024

ianbotsf Oct 7, 2024

ianbotsf Oct 7, 2024

ianbotsf Oct 7, 2024

0marperez Oct 8, 2024

lauzadis Oct 8, 2024

ianbotsf Oct 7, 2024

0marperez Oct 8, 2024

ianbotsf Oct 7, 2024

0marperez Oct 8, 2024

ianbotsf Oct 7, 2024

0marperez Oct 8, 2024

ianbotsf Oct 8, 2024

lauzadis left a comment

lauzadis Oct 8, 2024

lauzadis Oct 8, 2024

github-actions bot commented Oct 10, 2024

lauzadis Oct 10, 2024

0marperez Oct 10, 2024

0marperez Oct 10, 2024 •

edited

Loading

lauzadis Oct 10, 2024

0marperez Oct 10, 2024

github-actions bot commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

sonarcloud bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

		# Release date

		This feature will ship with the v1.4.x release on xx/xx/xxxx.

misc: credentials search precedence change #1434

misc: credentials search precedence change #1434

Conversation

0marperez commented Oct 7, 2024 • edited Loading

Issue #

Description of changes

github-actions bot commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0marperez Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

sonarcloud bot commented Nov 4, 2024

Quality Gate failed

github-actions bot commented Nov 4, 2024

0marperez commented Oct 7, 2024 •

edited

Loading

0marperez Oct 10, 2024 •

edited

Loading