refactor(rt): generated sts and sts web identity credential providers #470
Conversation
|
A new generated diff is ready to view: __generated-main...__generated-feat-sts-providers |
| public class StsAssumeRoleCredentialsProvider public constructor( | ||
| credentialsProvider: CredentialsProvider, | ||
| roleArn: String, | ||
| sessionName: String, | ||
| durationSeconds: Int? = null, | ||
| ) : CrtCredentialsProvider { | ||
| override val crtProvider: StsAssumeRoleCredentialsProviderCrt = StsAssumeRoleCredentialsProviderCrt.build { | ||
| clientBootstrap = SdkDefaultIO.ClientBootstrap | ||
| tlsContext = SdkDefaultIO.TlsContext | ||
| this.credentialsProvider = asCrt(credentialsProvider) | ||
| this.roleArn = roleArn | ||
| this.sessionName = sessionName | ||
| this.durationSeconds = durationSeconds | ||
| @OptIn(ExperimentalTime::class) | ||
| public class StsAssumeRoleCredentialsProvider( | ||
| private val credentialsProvider: CredentialsProvider, | ||
| private val roleArn: String, | ||
| private val region: String? = null, | ||
| private val roleSessionName: String? = null, | ||
| private val externalId: String? = null, | ||
| private val duration: Duration = Duration.seconds(DEFAULT_CREDENTIALS_REFRESH_SECONDS), | ||
| private val httpClientEngine: HttpClientEngine? = null | ||
| ) : CredentialsProvider { |
There was a problem hiding this comment.
Nit: This is a breaking change so the commit message should have a ! after the type/scope.
There was a problem hiding this comment.
Can/will be fixed up in the squashed commit
|
|
||
| override suspend fun getCredentials(): Credentials { | ||
| val logger = Logger.getLogger<StsAssumeRoleCredentialsProvider>() | ||
| logger.debug { "retrieving assumed credentials" } |
There was a problem hiding this comment.
Comment: This log message could be made more informative by including additional details like the role ARN, session name, etc.
There was a problem hiding this comment.
Is role ARN potentially considered sensitive?
| client.close() | ||
| } | ||
|
|
||
| logger.debug { "obtained assumed credentials" } |
There was a problem hiding this comment.
Comment: This log message could be made more informative by including additional details like expiration time.
kggilmer
left a comment
kggilmer
left a comment
There was a problem hiding this comment.
looking to close on handling of exceptions as mentioned in feedback
| roleSessionName = provider.roleSessionName ?: defaultSessionName() | ||
| durationSeconds = provider.duration.inWholeSeconds.toInt() | ||
| } | ||
| } catch (ex: Exception) { |
There was a problem hiding this comment.
concern
This catch seems overly broad. It seems that we'd want to pass I/O exceptions (any anything else that is volatile/environmental) back up directly rather than wrapping in another exception type.
There was a problem hiding this comment.
after team discussion it seems that this is the preferred treatment for io exceptions
| durationSeconds = provider.duration.inWholeSeconds.toInt() | ||
| } | ||
| } catch (ex: Exception) { | ||
| logger.debug { "sts refused to grant assumed role credentials" } |
There was a problem hiding this comment.
nit
Related to previous comment, in the case of say an IO exception this debug message may be misleading to readers, implying some policy constraint in STS where in fact it was something more basic.
| logger.debug { "sts refused to grant assumed role credentials" } | ||
| when (ex) { | ||
| is RegionDisabledException -> throw ProviderConfigurationException( | ||
| "STS is not activated in the requested region. Please check your configuration and activate STS in the target region if necessary", |
There was a problem hiding this comment.
nit
seems like it would be safe to specify the region itself that was the problem in this message.
|
|
||
| // role session name must be provided to assume a role, when the user doesn't provide one we choose a name for them | ||
| internal fun defaultSessionName(platformEnvironProvider: PlatformEnvironProvider = Platform): String = | ||
| AwsSdkSetting.AwsRoleSessionName.resolve(platformEnvironProvider) ?: "aws-sdk-kotlin-${Instant.now().epochMilliseconds}" |
There was a problem hiding this comment.
question
I assume the prefix aws-sdk-kotlin is meant to disambiguate from something else? where would this string be visible to to the customer, if anywhere? (I'm trying to understand the value of the static string prefix)
| durationSeconds = provider.duration.inWholeSeconds.toInt() | ||
| roleSessionName = provider.roleSessionName ?: defaultSessionName(platformProvider) | ||
| } | ||
| } catch (ex: Exception) { |
There was a problem hiding this comment.
concern
same as above regarding capturing I/O exception types
kggilmer
left a comment
kggilmer
left a comment
There was a problem hiding this comment.
concerns regarding exception handling addressed in offline discussion
|
A new generated diff is ready to view: __generated-main...__generated-feat-sts-providers |
|
SonarCloud Quality Gate failed.
|
|
A new generated diff is ready to view: __generated-main...__generated-feat-sts-providers |
…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>
3 participants
Issue #
N/A
Description of changes
Refactor the STS credential providers to use internal generated service clients rather than CRT.
Couple of benefits of this route:
Testing Done:
{ // val provider = StsAssumeRoleCredentialsProvider( // credentialsProvider = DefaultChainCredentialsProvider(), // roleArn = "<test-role>", // region = "us-west-2" // // ) val provider = StsWebIdentityCredentialsProvider( roleArn = "<test-role>", roleSessionName = "test-sts-web-ident-provider", webIdentityTokenFilePath = "/path/to/my/token.jwt", region = "us-west-2" ) StsClient{ region = "us-east-2" credentialsProvider = provider }.use { client -> val resp = client.getCallerIdentity {} println("Account: ${resp.account}") println("UserID: ${resp.userId}") println("ARN: ${resp.arn}") } }STS Assume role is fairly easy to test. The web identity provider is a bit more involved and requires setting up an OIDC provider. There is an internal setup script in the repo referenced from the (in-progress) SEP that can do this by configuring an S3 bucket and as an OIDC provider. You have to turn on public access temporarily to get it to work though.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.