Skip to content

Conversation

@kggilmer
Copy link
Contributor

Issue #

#325
#19

Description of changes

  • Provide SDK types for aws-crt-kotlin credential providers: sts assume role and sts web identity

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

@kggilmer kggilmer requested review from aajtodd and ianbotsf October 19, 2021 23:34
@kggilmer
Copy link
Contributor Author

I didn't setup the cross-repo branches for this so the aws-crt-kotlin change will need to be merged before CI will pass.

*/
public class StsAssumeRoleCredentialsProvider public constructor(
credentialsProvider: CredentialsProvider,
roleArn: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

correctness

Aren't some of these required? If so I think we should make them required arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch.

}

// Adapt SDK credential provider to CRT version
internal fun adapt(credentialsProvider: CredentialsProvider): CredentialsProviderCrt =
Copy link
Contributor

Choose a reason for hiding this comment

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

sigh the amount of layers here is painful and admittedly hard to follow at times (nothing you did wrong of course)

Comment on lines 38 to 49
// Adapt SDK credential provider to CRT version
internal fun CredentialsProvider.toCrtCredentialsProvider(): CredentialsProviderCrt =
object : CredentialsProviderCrt {
override fun close() { }

override suspend fun getCredentials(): Credentials {
val credentials = this@toCrtCredentialsProvider.getCredentials()
return Credentials(credentials.accessKeyId, credentials.secretAccessKey, credentials.sessionToken)
}

override suspend fun waitForShutdown() { }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: Doesn't this already exist as CredentialsProviderCrtProxy?

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 surely does, nice catch

override val crtProvider: StsAssumeRoleCredentialsProviderCrt = StsAssumeRoleCredentialsProviderCrt.build {
clientBootstrap = SdkDefaultIO.ClientBootstrap
tlsContext = SdkDefaultIO.TlsContext
this.credentialsProvider = CredentialsProviderCrtProxy(credentialsProvider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: Now that I'm looking at the usage of the CRT proxy a little closer I believe there may be room for improvement. We've got CrtCredentialsProvider for wrapping CRT in SDK and we've got CredentialsProviderCrtProxy for wrapping SDK in CRT. But if someone passes in a CrtCredentialsProvider to this STS impl, we don't need to re-wrap it as CRT...we can just unwrap it from SDK right?

I think we may need a new helper method in CrtCredentialUtils.kt:

internal fun asCrt(sdkProvider: CredentialsProvider): CredentialsProviderCrt = when (sdkProvider) {
    is CrtCredentialsProvider -> sdkProvider.crtProvider
    else -> CredentialsProviderCrtProxy(sdkProvider)
}

@kggilmer kggilmer merged commit dbde5bd into main Oct 20, 2021
@kggilmer kggilmer deleted the feat-sts-credential-providers branch October 20, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants