feat(rt)!: implement kmp profile credentials provider #478

aajtodd · 2021-12-21T19:54:17Z

Issue #

N/A

Description of changes

Implements KMP compatible profile credentials provider in pure Kotlin.
- The underlying representation (ProfileChain) and tests are mostly ported from Rust (thanks Russell)
Adds provider name field to Credentials for easier debugging

Testing

Tested with assuming a role:

    StsClient{
        region = "us-east-2"
        credentialsProvider = ProfileCredentialsProvider()
    }.use { client ->
        val resp = client.getCallerIdentity {}

        println("Account: ${resp.account}")
        println("UserID: ${resp.userId}")
        println("ARN: ${resp.arn}")
    }

Profile:

[profile foo]
role_arn = arn:aws:iam::123456789:role/RoleA
region = us-east-2
source_profile = baz

[profile baz]
# static credentials

NOTE: Additional test cases that cover more involved scenarios will be added when we replace the default chain.

Questions

What should the behavior be in the code snippet above? region is provided to the StsClient but not the profile provider. There is no good way to currently plumb this information through. The profile provider requires region to be set still in either the profile or env/system property. I think this is correct but wanted to bring it up for discussion.

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

github-actions · 2021-12-21T19:58:24Z

A new generated diff is ready to view: __generated-main...__generated-feat-profile-provider

kggilmer

Quite clean and easy to follow. i think every log emission needs the first word capitalized otherwise no concerns found.

kggilmer · 2022-01-04T19:18:53Z

...time/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/EcsCredentialsProvider.kt

-    public constructor() : this(Platform, CrtHttpEngine())
+    public constructor() : this(Platform)
+
+    private val manageEngine = httpClientEngine == null


style

IMO owned is more descriptive than manage as to the intent of this property.

Yeah true, I made it manageEngine to match the corresponding parameter that sdkHttpClient uses.

I kind of expect this is going to change when we decide on our ownership direction

kggilmer · 2022-01-04T19:26:36Z

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

+    override suspend fun getCredentials(): Credentials {
+        val logger = Logger.getLogger<ProfileCredentialsProvider>()
+        val source = resolveConfigSource(platform, profileName)
+        logger.debug { "loading credentials from profile `${source.profile}`" }


nit

title case for log entry, for all log emissions in this file

did we document this convention somewhere. I remember discussing it and promptly forgot and I will probably continue to forget until we get it written down in a style guide

kggilmer · 2022-01-04T19:35:43Z

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

+ * A standalone member of the profile chain. Leaf providers do not require
+ * input credentials to provide their own credentials (e.g. IMDS, ECS, Environment, etc)
+ */
+internal sealed class LeafProvider {


question

it's not connecting for me, what meaning does 'leaf' mean for this type? Is it that it's terminal, as in a leaf in a tree?

yeah the provider that doesn't depend on any other providers. In the Rust implementation they called it BaseProvider. I didn't find that descriptive enough for my tastes.

kggilmer · 2022-01-04T19:39:14Z

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

+
+                if (visited.contains(sourceProfileName)) {
+                    // we're in a loop, break out
+                    throw ProviderConfigurationException("profile formed an infinite loop: ${visited.joinToString(separator = " -> ")} -> $sourceProfileName")


question

assuming a customer would see this error message, is there some additional text we could add to explain what the customer should do to resolve it?

kggilmer · 2022-01-04T19:49:18Z

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

+    val accessKeyId = get(AWS_ACCESS_KEY_ID)
+    val secretKey = get(AWS_SECRET_ACCESS_KEY)
+    return when {
+        accessKeyId == null && secretKey == null -> throw ProviderConfigurationException("profile ($name) did not contain credential information")


nit

"profile ($name) did not contained neither an access key nor secret key" may be more actionable to a user

kggilmer · 2022-01-04T19:52:10Z

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

+        }
+        // loop back into this profile and pick up the credential source
+        sourceProfile == null && credSource != null -> NextProfile.SelfReference
+        else -> error("shouldn't be able to get here, this is a bug please file a ticket")


suggestion

link to gh issues so the customer knows where to file the ticket

kggilmer · 2022-01-04T19:54:50Z

aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/auth/credentials/ProfileChainTest.kt

+import kotlin.test.assertEquals
+import kotlin.test.fail
+
+class ProfileChainTest {


wow nice 🤩

ianbotsf · 2022-01-04T19:17:20Z

aws-runtime/aws-types/common/src/aws/sdk/kotlin/runtime/auth/credentials/Credentials.kt

    val secretAccessKey: String,
    val sessionToken: String? = null,
    val expiration: Instant? = null,
+    val providerName: String? = null,


Question: Where do we make use of this?

We don't really but it seems very useful for debugging purposes. I could be convinced to remove it though

ianbotsf · 2022-01-04T19:32:39Z

...time/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/EcsCredentialsProvider.kt

+private const val PROVIDER_NAME = "EcsContainer"
+


Question: Would using the class name be simpler than custom names for each provider?

Provider names are actually called out in an SEP

ianbotsf · 2022-01-04T20:51:27Z

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

+) {
+    companion object {
+        internal fun resolve(profiles: ProfileMap, profileName: String): ProfileChain {
+            val visited = mutableListOf<String>()


Nit: Could be a set for clearer semantics. Bonus, you can simplify contains/add to:

if (!visited.add(sourceProfileName)) { throw ... }

It can't be a set because the order matters for the error message we form on L53. If we don't use that formatting we I think we could make it a set. Although I didn't go look and see if there is an ordered set available...

mutableSetOf returns a set with order-insertion guaranteed.

Perfect. Will update.

ianbotsf · 2022-01-04T20:54:18Z

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

+                visited.add(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) {


Question: Why do we not use static credentials and break if there's only 1 element in visited?

I'm not sure I follow. As the comment suggests the chain terminates as soon as it hits a profile with static creds. In the case of a single profile with static creds it would break out at L71

The if condition is visited.size > 1. Why shouldn't we run the code inside the if-block if we've only visited one element so far?

Ahh so as the comment on L64 indicates the only indication that multiple profiles are chained is the existince of role_arn. This check exists only to terminate a chain at the first profile with credentials, e.g.

[profile A] role_arn = arn::test::foo::blah source_profile = B [profile B] access_key_id = AKID secret_access_key= secret source_profile = C # profile A chain must terminate here and not look at C for creds [profile C] access_key_id = incorrect_key secret_access_key= incorrect_secret

The chain must terminate at the first profile that has static creds when multiple are involved. The singular profile with static creds that has no chain:

[profile A] access_key_id = AKID secret_access_key= secret

is handled by the else branch starting on L68 (just below).

As I type all this I'm now realizing your actual question. I'll leave all the above as context but the answer is in this test case

ianbotsf · 2022-01-04T21:02:13Z

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

+        else -> {
+            LeafProvider.WebIdentityTokenRole(roleArn, tokenFile, sessionName)
+        }


Nit: Unnecessary braces

ianbotsf · 2022-01-04T21:06:20Z

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

+private inline fun ProfileMap.getOrThrow(name: String, lazyException: () -> Throwable): AwsProfile {
+    val props = get(name) ?: throw lazyException()
+    return AwsProfile(name, props)
+}


Comment: This seems slightly strange to me. The function accepts a () -> Throwable whereas other similar functions (e.g., require, check, etc.) accept a () -> Any and handle the exception initialization themselves. Making this change could also simplify the call site.

Yeah true but the exception type would be wrong. I want a ProviderConfigurationException. What I really want is check(condition, MyErrorType::class) { "lazy message" } I guess but not sure that works

Right sorry, I wasn't suggesting using require or check...I was suggesting keeping this function but constructing the ProviderConfigurationException inside of it rather than in calling code.

ianbotsf · 2022-01-04T21:14:16Z

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

+        accessKeyId == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_access_key_id`")
+        secretKey == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_secret_access_key`")


Question: throw throw? Does this compile?

oof yes but I will fix it, must be copy pasta, good eye

ianbotsf · 2022-01-04T21:18:04Z

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

+/**
+ * Load [LeafProvider.AccessKey] from the current profile or throw an exception if the profile does not contain
+ * credentials
+ */
+private fun AwsProfile.staticCreds(): LeafProvider {
+    val accessKeyId = get(AWS_ACCESS_KEY_ID)
+    val secretKey = get(AWS_SECRET_ACCESS_KEY)
+    return when {
+        accessKeyId == null && secretKey == null -> throw ProviderConfigurationException("profile ($name) did not contain credential information")
+        accessKeyId == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_access_key_id`")
+        secretKey == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_secret_access_key`")
+        else -> {
+            val sessionToken = get(AWS_SESSION_TOKEN)
+            LeafProvider.AccessKey(Credentials(accessKeyId, secretKey, sessionToken))
+        }
+    }
+}
+
+/**
+ * Attempt to load [LeafProvider.AccessKey] from the current profile or `null` if the current profile does not contain
+ * credentials
+ */
+private fun AwsProfile.staticCredsOrNull(): LeafProvider? = try {
+    staticCreds()
+} catch (_: ProviderConfigurationException) {
+    null
+}


Comment: At least in JVM, exceptions are slow and thus generally avoided for flow control. This may be more performant by using some other type to encapsulate the result of getting static credentials (e.g., an ADT) and then throwing or returning null as appropriate at a higher level:

sealed class LeafProviderResult { data class Success(provider: LeafProvider) : LeafProviderResult() data class Failure(errorMessage: String) : LeafProviderResult() } private fun AwsProfile.staticCredsResult(): LeafProviderResult { val accessKeyId = get(AWS_ACCESS_KEY_ID) val secretKey = get(AWS_SECRET_ACCESS_KEY) return when { accessKeyId == null && secretKey == null -> LeafProviderResult.Failure("profile ($name) did not contain credential information") accessKeyId == null -> LeafProviderResult.Failure("profile ($name) missing `aws_access_key_id`") secretKey == null -> LeafProviderResult.Failure("profile ($name) missing `aws_secret_access_key`") else -> { val sessionToken = get(AWS_SESSION_TOKEN) LeafProviderResult.Success(LeafProvider.AccessKey(Credentials(accessKeyId, secretKey, sessionToken))) } } } private fun AwsProfile.staticCredsOrNull(): LeafProvider? = when (val result = staticCredsResult()) { is LeafProviderResult.Success -> result.provider else -> null } private fun AwsProfile.staticCredsOrThrow(): LeafProvider = when (val result = staticCredsResult()) { is LeafProviderResult.Success -> result.provider else -> throw ProviderConfigurationException(result.errorMessage) }

Ya I can look into this.

ianbotsf · 2022-01-04T21:25:29Z

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

+    return when {
+        sourceProfile != null && credSource != null -> throw ProviderConfigurationException("profile ($name) contained both `source_profile` and `credential_source`. Only one or the other can be defined.")
+        sourceProfile == null && credSource == null -> throw ProviderConfigurationException("profile ($name) must contain `source_profile` or `credential_source` but neither were defined")
+        sourceProfile != null && credSource == null -> if (sourceProfile == name) {
+            NextProfile.SelfReference
+        } else {
+            NextProfile.Named(sourceProfile)
+        }
+        // loop back into this profile and pick up the credential source
+        sourceProfile == null && credSource != null -> NextProfile.SelfReference
+        else -> error("shouldn't be able to get here, this is a bug please file a ticket")
+    }


Comment: I'm surprised the compiler doesn't detect else as dead code. Should be able to avoid the else -> error case by rewriting this as nested if/else statements:

return if (sourceProfile == null) { if (credSource == null) { throw ProviderConfigurationException(...) } else { // credSource != null NextProfile.SelfReference } } else { // sourceProfile != null if (credSource == null) { if (sourceProfile == name) NextProfile.SelfReference else NextProfile.Named(sourceProfile) } else { // credSource != null throw ProviderConfigurationException(...) } }

true but I find the when cleaner personally, I was surprised as well

sonarqubecloud · 2022-01-05T19:16:41Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
6.7% Duplication

github-actions · 2022-01-05T19:20:16Z

A new generated diff is ready to view: __generated-main...__generated-feat-profile-provider

…ible (#469) Refactor credential providers to remove CRT dependency and make them KMP compatible. Added SSO provider to default chain. Lots of misc cleanup and improvements. * feat(rt): standalone sso credentials provider (#462) * refactor(rt)!: generated sts and sts web identity credential providers (#470) * refactor(rt)!: implement kmp ecs provider (#475) * feat(rt)!: implement kmp profile credentials provider (#478) * feat(rt)!: kmp default credentials provider chain (#491) * fix: work around machine-specific Gradle bug with aws-config variants (#496) * fix: credentials provider ownership (#498) Co-authored-by: Ian Botsford <83236726+ianbotsf@users.noreply.github.com>

aajtodd added 7 commits December 20, 2021 15:11

feat(rt): implement profile chain

b12f814

impl profile provider; fix http engine lifetimes

48dc8df

add logging

c0f7961

add provider name to credentials for easier debugging

6a213cc

add consts for provider names

613032d

cleanup todos

a6ae977

ensure region precedence

8934ffa

aajtodd requested a review from a team as a code owner December 21, 2021 19:54

aajtodd requested review from ianbotsf and kggilmer December 21, 2021 19:54

kggilmer approved these changes Jan 4, 2022

View reviewed changes

ianbotsf approved these changes Jan 4, 2022

View reviewed changes

aajtodd added 3 commits January 5, 2022 10:47

cleanup feedback

acd8b82

remove exception as control flow

680107a

fix log format

d1e355d

ianbotsf approved these changes Jan 5, 2022

View reviewed changes

aajtodd merged commit 1396bde into feat-cred-providers Jan 5, 2022

aajtodd deleted the feat-profile-provider branch February 17, 2022 15:08

		accessKeyId == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_access_key_id`")
		secretKey == null -> throw throw ProviderConfigurationException("profile ($name) missing `aws_secret_access_key`")

feat(rt)!: implement kmp profile credentials provider #478

feat(rt)!: implement kmp profile credentials provider #478

Uh oh!

Conversation

aajtodd commented Dec 21, 2021

Issue #

Description of changes

Testing

Questions

Uh oh!

github-actions bot commented Dec 21, 2021

Uh oh!

kggilmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 5, 2022