feat: Add auth token generators for RDS and DSQL #1495
+228
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
ianbotsf
requested changes
Jan 8, 2025
Comment on lines
20
to
25
| /** | ||
| * Generates an authentication token, which is a SigV4-signed URL with the HTTP scheme removed. | ||
| * @param service The name of the service the token is being generated for | ||
| * @param credentials The credentials to use when generating the auth token, defaults to resolving credentials from the [DefaultChainCredentialsProvider] | ||
| */ | ||
| public class AuthTokenGenerator( |
There was a problem hiding this comment.
Opinion: This really feels like it belongs in smithy-kotlin's aws-signing-common alongside other things like signing config, presigners, etc. Any reason it's located here instead?
There was a problem hiding this comment.
This felt specific to AWS services so I didn't think to put it in smithy-kotlin. Should smithy-kotlin even have a module named aws-signing-common? Shouldn't that eventually get moved to aws-sdk-kotlin?
There was a problem hiding this comment.
Spoke offline, this aws-signing-common module actually was moved from aws-sdk-kotlin to smithy-kotlin a while back. Relocating AuthTokenGenerator there...
| */ | ||
| public class AuthTokenGenerator( | ||
| public val service: String, | ||
| public val credentials: Credentials? = runBlocking { DefaultChainCredentialsProvider().resolve() }, |
There was a problem hiding this comment.
Nit: We should generally defer fetching credentials until the moment we need them. If someone instantiates AuthTokenGenerator but then calls generateAuthToken much later, the resident credentials may be invalid. I think this constructor argument should be a CredentialsProvider instead.
| public val service: String, | ||
| public val credentials: Credentials? = runBlocking { DefaultChainCredentialsProvider().resolve() }, | ||
| ) { | ||
| private fun String.trimScheme() = removePrefix("http://").removePrefix("https://") |
There was a problem hiding this comment.
Correctness: This only accounts for two URI schemes but there are more. I think it might be better for Url itself to handle this by providing a property similar to hostAndPort or requestRelativePath.
There was a problem hiding this comment.
I ended up making a private extension function Url.trimScheme which filters out only the scheme that the URL is actually using. I didn't think a property was the right option because we need the whole URL, other properties focus on specific parts of the URL like host and port
| signatureType = AwsSignatureType.HTTP_REQUEST_VIA_QUERY_PARAMS | ||
| } | ||
|
|
||
| return DefaultAwsSigner.sign(req, config).output.url.toString().trimScheme() |
There was a problem hiding this comment.
Nit: I think we should allow users to swap in their preferred signer if they wish. Of course if they don't we can default to DefaultAwsSigner.
Comment on lines
19
to
22
| public class AuthTokenGenerator( | ||
| public val credentials: Credentials? = runBlocking { DefaultChainCredentialsProvider().resolve() }, | ||
| ) { | ||
| private val generator = AuthTokenGenerator("dsql", credentials) |
There was a problem hiding this comment.
Style: This DSQL/RDS classes and this common class are named identically which is very confusing (i.e., class AuthTokenGenerator { val generator = AuthTokenGenerator(...) }). Can we name this something different (e.g., GenericAuthTokenGenerator) or name the service-specific ones something different (e.g., RdsAuthTokenGenerator, DsqlAuthTokenGenerator, etc.)?
There was a problem hiding this comment.
The spec says each service-specific class SHOULD be named AuthTokenGenerator, but I agree, I'd prefer to name them RdsAuthTokenGenerator/DsqlAuthTokenGenerator, so I'll go for that
Comment on lines
32
to
33
| // Match the X-Amz-Credential parameter for any signing date | ||
| val credentialRegex = Regex("X-Amz-Credential=akid%2F(\\d{8})%2Fus-east-1%2Fdsql%2Faws4_request") |
There was a problem hiding this comment.
Opinion: We should be able to assert the correct signing date. Can we provide a Clock parameter in the generator?
There was a problem hiding this comment.
I was trying to avoid that to keep the API surface clean, but sure, that's the only way we'll be able to mock the signing time
Comment on lines
22
to
27
| val token = AuthTokenGenerator(credentials) | ||
| .generateDbConnectAuthToken( | ||
| endpoint = Url { host = Host.parse("peccy.dsql.us-east-1.on.aws") }, | ||
| region = "us-east-1", | ||
| expiration = 450.seconds, | ||
| ) |
There was a problem hiding this comment.
Suggestion: The token assertions might be easier if you parsed it back into a Url first. Then you could access decoded and structured properties in the token.
There was a problem hiding this comment.
Unless you have a strong opinion, I think this is fine. The spec(s) provided these query parameter assertions
…s, change [Credentials] param to [CredentialsProvider]
…th-token-generator
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
0marperez
approved these changes
Jan 9, 2025
ianbotsf
reviewed
Jan 9, 2025
Comment on lines
41
to
45
| parameters.apply { | ||
| decodedParameters { | ||
| add("Action", "DbConnect") | ||
| } | ||
| } |
There was a problem hiding this comment.
Style: Consider simplifying:
parameters.decodedParameters.add("Action", "DbConnect")
Comment on lines
40
to
46
| val dbConnectEndpoint = endpoint.toBuilder().apply { | ||
| parameters.apply { | ||
| decodedParameters { | ||
| add("Action", "DbConnect") | ||
| } | ||
| } | ||
| }.build() |
There was a problem hiding this comment.
Question: Do we want add or put for the parameter? This is a multi-map meaning that it can contain multiple copies of the same key-value pair (as could happen with add)...
There was a problem hiding this comment.
put is probably safer but we have full control over the input request here, and signing doesn't add an Action parameter, so it's unlikely to make a difference
edit: I see how a customer might add Action themselves and cause issues...
Comment on lines
+21
to
+43
| fun testGenerateDbConnectAuthToken() = runTest { | ||
| val clock = ManualClock(Instant.fromEpochSeconds(1724716800)) | ||
|
|
||
| val credentials = Credentials("akid", "secret") | ||
| val credentialsProvider = StaticCredentialsProvider(credentials) | ||
|
|
||
| val token = DsqlAuthTokenGenerator(credentialsProvider, clock = clock) | ||
| .generateDbConnectAuthToken( | ||
| endpoint = Url { host = Host.parse("peccy.dsql.us-east-1.on.aws") }, | ||
| region = "us-east-1", | ||
| expiration = 450.seconds, | ||
| ) | ||
|
|
||
| // Token should have a parameter Action=DbConnect | ||
| assertContains(token, "peccy.dsql.us-east-1.on.aws?Action=DbConnect") | ||
| assertContains(token, "X-Amz-Credential=akid%2F20240827%2Fus-east-1%2Fdsql%2Faws4_request") | ||
| assertContains(token, "X-Amz-Expires=450") | ||
|
|
||
| // Token should not contain a scheme | ||
| listOf("http://", "https://").forEach { | ||
| assertFalse(token.contains(it)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: We've added the ability to inject a Clock to the generator and you've injected a ManualClock here but we're still not asserting it's used correctly.
There was a problem hiding this comment.
It's indirectly tested through validating the X-Amz-Credential string. Is there something additional you'd like to see?
I could parse the date from X-Amz-Credential and compare it to clock.now() but it seems a little redundant
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #
Closes #217
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.