fix: add customization for models missing SIGV4A trait #1171

0marperez · 2024-01-05T16:28:43Z

Issue #

closes #625

Description of changes

Changed the default auth scheme for S3 and added a test to verify

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

github-actions · 2024-01-05T16:35:28Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

ianbotsf

Correctness: Let's add an E2E test for this as well.

ianbotsf · 2024-01-05T17:18:45Z

...k-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/ClientConfigIntegration.kt

+            .apply {
+                symbol = KotlinTypes.Collections.list(RuntimeTypes.Auth.HttpAuth.AuthScheme, default = "listOf(${RuntimeTypes.Auth.HttpAuthAws.SigV4AsymmetricAuthScheme}(${RuntimeTypes.Auth.Signing.AwsSigningStandard.DefaultAwsSigner}))")
+            }


Style: This type of default specification doesn't use imports and instead pastes large, fully-qualified names into the codegen output. Instead, we can use additionalImports to make the resulting generated code cleaner:

val AuthSchemes = RuntimeConfigProperty .AuthSchemes .toBuilder() .apply { propertyType = ConfigPropertyType.RequiredWithDefault("SigV4AsymmetricAuthScheme(DefaultAwsSigner)") additionalImports = listOf( RuntimeTypes.Auth.HttpAuthAws.SigV4AsymmetricAuthScheme, RuntimeTypes.Auth.Signing.AwsSigningStandard.DefaultAwsSigner, ) }

ianbotsf · 2024-01-05T17:25:28Z

...k-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/ClientConfigIntegration.kt

+        val AuthSchemes = RuntimeConfigProperty
+            .AuthSchemes
+            .toBuilder()
+            .apply {
+                symbol = KotlinTypes.Collections.list(RuntimeTypes.Auth.HttpAuth.AuthScheme, default = "listOf(${RuntimeTypes.Auth.HttpAuthAws.SigV4AsymmetricAuthScheme}(${RuntimeTypes.Auth.Signing.AwsSigningStandard.DefaultAwsSigner}))")
+            }
+            .build()
    }


Question: I'm a little confused about this addition. Won't users still have to replace the auth scheme with one that uses the CRT signer in order for MRAP to work?

Hmm, yes. I'm guessing we could avoid users having any additional configuration to use MRAP's by setting the CRT signer as the default for S3. Are there any implications/side effects we would need to worry about when doing that?

The reason for setting the default auth scheme to SIGV4A is because the requests were silently falling back to SIGV4 and would succeed sometimes.

So users would get the impression that everything is configured properly when that's not the case

CRT signer as the default for S3. Are there any implications/side effects we would need to worry about when doing that?
Yes, we can't use CRT by default.

Won't users still have to replace the auth scheme with one that uses the CRT signer in order for MRAP to work?

Yes, when testing this though what happens is sigv4 is registered in the model and sigv4a comes from the endpoint rule properties. So we get a prioritized list of auth options [sigv4a, sigv] after merging those. During selection we see that sigv4a isn't supported/configured and so don't consider it as an option and end up selecting sigv4. This actually works when the access point points to a bucket in the same region that you are configured to sign for. This means users would think sigv4a is working but it's not and in a failover scenario their requests would fail as they wouldn't be valid for a different region. I figured it was better to register support for sigv4a with the default signer and just fail loudly instead with an exception that the signer doesn't support that auth scheme.

aajtodd · 2024-01-05T19:21:19Z

...k-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/ClientConfigIntegration.kt

            documentation = """
                Flag to disable [S3 multi-region access points](https://docs.aws.amazon.com/AmazonS3/latest/userguide/MultiRegionAccessPoints.html).
            """.trimIndent()
        }
+
+        val AuthSchemes = RuntimeConfigProperty


fix:

This isn't the right approach. We want to register sigv4a support as a configured auth scheme but it doesn't belong in this list, these are user provided. We just want to register support for sigv4a as an entirely new auth scheme handler in smithy-kotlin (e.g. here is the sigv4 auth scheme handler).

There are some thorny questions to answer about where we should register this since the current SigV4AuthSchemeIntegration is handling a bit of both right now and there is some overlap (e.g. sigv4a re-uses the same config property credentialsProvider). Thats probably ok though since they resolve to the same types. My vote would be to create a new Sigv4AsymmetricAuthSchemeIntegration and migrate sigv4a support/logic there.

We will also need to update the AuthIndex to also consider endpoint rules since s3 and eventbridge don't model sigv4a usage, they are considered only from endpoint rule properties. Effectively if the sigv4a scheme name shows up in any endpoint property authSchemes array node (see here for what I'm talking about) we would consider sigv4a as being registered at the service level for possibly any operation.

NOTE: aws.auth#sigv4a was added as a known auth scheme/shape id in smithy 1.42.0

aajtodd · 2024-01-05T19:25:54Z

...k-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/ClientConfigIntegration.kt

+        val AuthSchemes = RuntimeConfigProperty
+            .AuthSchemes
+            .toBuilder()
+            .apply {
+                symbol = KotlinTypes.Collections.list(RuntimeTypes.Auth.HttpAuth.AuthScheme, default = "listOf(${RuntimeTypes.Auth.HttpAuthAws.SigV4AsymmetricAuthScheme}(${RuntimeTypes.Auth.Signing.AwsSigningStandard.DefaultAwsSigner}))")
+            }
+            .build()
    }


CRT signer as the default for S3. Are there any implications/side effects we would need to worry about when doing that?
Yes, we can't use CRT by default.

Won't users still have to replace the auth scheme with one that uses the CRT signer in order for MRAP to work?

Yes, when testing this though what happens is sigv4 is registered in the model and sigv4a comes from the endpoint rule properties. So we get a prioritized list of auth options [sigv4a, sigv] after merging those. During selection we see that sigv4a isn't supported/configured and so don't consider it as an option and end up selecting sigv4. This actually works when the access point points to a bucket in the same region that you are configured to sign for. This means users would think sigv4a is working but it's not and in a failover scenario their requests would fail as they wouldn't be valid for a different region. I figured it was better to register support for sigv4a with the default signer and just fail loudly instead with an exception that the signer doesn't support that auth scheme.

github-actions · 2024-01-12T21:28:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-01-12T22:38:15Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-01-16T20:18:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-01-16T22:44:08Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

lauzadis · 2024-01-17T14:33:38Z

...k-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/ClientConfigIntegration.kt

@@ -54,10 +54,10 @@ class ClientConfigIntegration : KotlinIntegration {
            """.trimIndent()
        }

-        // FIXME: default signer doesn't yet implement sigv4a, default to mrap OFF until it does
+        // FIXME: default signer doesn't yet implement sigv4a


This FIXME can just be removed, it's not descriptive / in the right place anymore

lauzadis · 2024-01-17T14:34:36Z

...ommon/src/aws/sdk/kotlin/runtime/http/interceptors/UnsupportedSigningAlgorithmInterceptor.kt

It would be good to have some tests for this interceptor

github-actions · 2024-01-17T15:56:23Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

ianbotsf · 2024-01-17T17:22:19Z

...ommon/src/aws/sdk/kotlin/runtime/http/interceptors/UnsupportedSigningAlgorithmInterceptor.kt

+    override suspend fun modifyBeforeCompletion(context: ResponseInterceptorContext<Any, Any, HttpRequest?, HttpResponse?>): Result<Any> {
+        context.response.exceptionOrNull()?.let {
+            if (it is UnsupportedSigningAlgorithm && it.isSigV4a) {
+                throw UnsupportedSigningAlgorithm(
+                    it.message!!, // TODO: Add a message and link pointing to AWS SDK for Kotlin developer guide
+                    true,
+                )
+            }
+        }
+        return super.modifyBeforeCompletion(context)
+    }


Question: It seems strange to me to throw an exception from this interceptor rather than return one. Would other following interceptors get the chance to modify the UnsupportedSigningAlgorithm? Does returning the exception achieve the same result?

Does returning the exception achieve the same result?

Yeah, it does👍
I changed it so it returns the exception like you said

ianbotsf · 2024-01-17T17:25:53Z

...ommon/src/aws/sdk/kotlin/runtime/http/interceptors/UnsupportedSigningAlgorithmInterceptor.kt

+            if (it is UnsupportedSigningAlgorithm && it.isSigV4a) {
+                throw UnsupportedSigningAlgorithm(
+                    it.message!!, // TODO: Add a message and link pointing to AWS SDK for Kotlin developer guide
+                    true,
+                )
+            }


Nit: Use the original UnsupportedSigningAlgorithm exception as the cause for the newly-thrown one so the stack trace isn't lost.

lauzadis · 2024-01-17T17:43:15Z

.../test/aws/sdk/kotlin/runtime/http/interceptors/UnsupportedSigningAlgorithmInterceptorTest.kt

+                )
+        }
+
+        assertEquals(UnsupportedSigningAlgorithm::class, exception::class)


You can use assertIs here

github-actions · 2024-01-17T19:04:51Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

ianbotsf

Nit: We still haven't added an E2E test here which I think we should do as a follow-up . Please create a backlog item.

github-actions · 2024-01-17T19:51:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

He's OOTO

sonarqubecloud · 2024-01-17T21:10:59Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

4 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-01-17T21:13:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

aajtodd · 2024-01-22T14:21:17Z

...ain/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnsupportedSigningAlgorithmIntegration.kt

+ */
+class UnsupportedSigningAlgorithmIntegration : KotlinIntegration {
+    override fun enabledForService(model: Model, settings: KotlinSettings): Boolean =
+        model.expectShape<ServiceShape>(settings.service).isS3


fix: This doesn't match the set of sdkIds where we apply the sigv4a trait customization

aajtodd · 2024-01-22T14:23:52Z

...en/src/main/kotlin/aws/sdk/kotlin/codegen/customization/SigV4AsymmetricTraitCustomization.kt

+                            mutableSetOf(
+                                SigV4ATrait
+                                    .builder()
+                                    .name(shape.expectTrait<ServiceTrait>().arnNamespace)


fix: This should fallback to the sigv4 trait if the arnNamespace isn't present/doesn't match the sigv4 trait.

This value SHOULD match the arnNamespace property of the aws.api#service trait if present and the name property of the aws.auth#sigv4 trait.

aajtodd · 2024-01-22T14:30:36Z

...en/src/main/kotlin/aws/sdk/kotlin/codegen/customization/SigV4AsymmetricTraitCustomization.kt

+                                    .builder()
+                                    .name(shape.expectTrait<ServiceTrait>().arnNamespace)
+                                    .build(),
+                                AuthTrait(mutableSetOf(SigV4ATrait.ID, SigV4Trait.ID)),


fix: This isn't right. (1) if the service has an auth trait applied we need to respect it and modify it rather than replace it. (2) the auth trait is a prioritized list, this places sigv4a in front of sigv4 which if we configure sigv4a by default then every operation would fail if endpoint rules doesn't return anything for an auth option. The reason it doesn't seem to for S3 right now is they are returning sigv4 and/or sigv4a from their endpoint provider for pretty much every branch. Nothing in endpoint rules dictates this and indeed Eventbridge doesn't appear to return sigv4 anywhere just sigv4a for specific branches (which we appear to have already triggered a customer issue).

See auth trait spec:

Change default auth scheme to SIGV4A for S3

8b272e8

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Jan 5, 2024

0marperez requested a review from a team as a code owner January 5, 2024 16:28

ianbotsf requested changes Jan 5, 2024

View reviewed changes

aajtodd previously requested changes Jan 5, 2024

View reviewed changes

Pushing to help solve cloudfront issue

3e18f69

Fix sdk ID

1b5daa5

lauzadis added 2 commits January 16, 2024 12:28

Merge branch 'main' of github.com:awslabs/aws-sdk-kotlin into mrap-ux

22d1727

ktlint

1dbcf5a

0marperez added 2 commits January 16, 2024 17:33

Fixed signing region bug

15d05ac

Fix merge conflicts

01ba0b7

0marperez changed the title ~~misc: change default auth scheme to SIGV4A for S3~~ fix: add customization for models missing SIGV4A trait Jan 16, 2024

lauzadis reviewed Jan 17, 2024

View reviewed changes

Added tests for unsupported signing algorithm interceptor

84ab837

ianbotsf approved these changes Jan 17, 2024

View reviewed changes

lauzadis approved these changes Jan 17, 2024

View reviewed changes

Misc changes

fadf63d

ianbotsf approved these changes Jan 17, 2024

View reviewed changes

Changed smithy version to snapshot

9f3ecc0

Bump smithy versions

7d27b86

Making punctuation change to prompt github tests to run

d1256c8

0marperez merged commit 6017f93 into main Jan 17, 2024
15 of 16 checks passed

0marperez deleted the mrap-ux branch January 17, 2024 22:29

aajtodd reviewed Jan 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add customization for models missing SIGV4A trait #1171

fix: add customization for models missing SIGV4A trait #1171

0marperez commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

ianbotsf left a comment

ianbotsf Jan 5, 2024

ianbotsf Jan 5, 2024

0marperez Jan 5, 2024

0marperez Jan 5, 2024

0marperez Jan 5, 2024

aajtodd Jan 5, 2024

aajtodd Jan 5, 2024

aajtodd Jan 5, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

lauzadis Jan 17, 2024

lauzadis Jan 17, 2024

github-actions bot commented Jan 17, 2024

ianbotsf Jan 17, 2024

0marperez Jan 17, 2024

ianbotsf Jan 17, 2024

lauzadis Jan 17, 2024

github-actions bot commented Jan 17, 2024

ianbotsf left a comment

github-actions bot commented Jan 17, 2024

sonarqubecloud bot commented Jan 17, 2024

github-actions bot commented Jan 17, 2024

aajtodd Jan 22, 2024

aajtodd Jan 22, 2024

aajtodd Jan 22, 2024

fix: add customization for models missing SIGV4A trait #1171

fix: add customization for models missing SIGV4A trait #1171

Conversation

0marperez commented Jan 5, 2024

Issue #

Description of changes

github-actions bot commented Jan 5, 2024

ianbotsf left a 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

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 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

github-actions bot commented Jan 17, 2024

ianbotsf left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2024

sonarqubecloud bot commented Jan 17, 2024

Quality Gate passed

github-actions bot commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment