fix: correctly apply resolved endpoint to presigned requests (#1025)

Author: ianbotsf

.changes/80e3c346-9a4a-4d47-84f1-1655742f9711.json

@@ -0,0 +1,8 @@
+{
+    "id": "80e3c346-9a4a-4d47-84f1-1655742f9711",
+    "type": "bugfix",
+    "description": "Correctly apply resolved endpoint to presigned requests",
+    "issues": [
+        "awslabs/aws-sdk-kotlin#1173"
+    ]
+}

runtime/auth/aws-signing-common/build.gradle.kts

@@ -23,6 +23,9 @@ kotlin {
         commonTest {
             dependencies {
                 implementation(libs.kotlinx.coroutines.test)
+
+                // Needed for an actual signer implementation in tests
+                implementation(project(":runtime:auth:aws-signing-default"))
             }
         }
 

runtime/auth/aws-signing-common/common/src/aws/smithy/kotlin/runtime/auth/awssigning/Presigner.kt

@@ -12,9 +12,9 @@ import aws.smithy.kotlin.runtime.collections.emptyAttributes
 import aws.smithy.kotlin.runtime.http.*
 import aws.smithy.kotlin.runtime.http.operation.EndpointResolver
 import aws.smithy.kotlin.runtime.http.operation.ResolveEndpointRequest
+import aws.smithy.kotlin.runtime.http.operation.setResolvedEndpoint
 import aws.smithy.kotlin.runtime.http.request.HttpRequest
 import aws.smithy.kotlin.runtime.http.request.HttpRequestBuilder
-import aws.smithy.kotlin.runtime.http.request.header
 import aws.smithy.kotlin.runtime.net.url.Url
 import aws.smithy.kotlin.runtime.operation.ExecutionContext
 
@@ -32,9 +32,9 @@ public suspend fun presignRequest(
     val credentials = credentialsProvider.resolve()
     val eprRequest = ResolveEndpointRequest(ctx, unsignedRequestBuilder.build(), credentials)
     val endpoint = endpointResolver.resolve(eprRequest)
-    val signingContext = endpoint.authOptions.firstOrNull { it.schemeId == AuthSchemeId.AwsSigV4 }?.attributes ?: emptyAttributes()
+    setResolvedEndpoint(unsignedRequestBuilder, ctx, endpoint)
 
-    val unsignedRequest = unsignedRequestBuilder.apply { header("host", endpoint.uri.host.toString()) }.build()
+    val signingContext = endpoint.authOptions.firstOrNull { it.schemeId == AuthSchemeId.AwsSigV4 }?.attributes ?: emptyAttributes()
 
     val config = AwsSigningConfig {
         region = signingContext.getOrNull(AwsSigningAttributes.SigningRegion)
@@ -48,7 +48,7 @@ public suspend fun presignRequest(
         signingConfig()
     }
 
-    val result = signer.sign(unsignedRequest, config)
+    val result = signer.sign(unsignedRequestBuilder.build(), config)
     val signedRequest = result.output
 
     return HttpRequest(

runtime/auth/aws-signing-common/common/test/aws/smithy/kotlin/runtime/auth/awssigning/PresignerTest.kt

@@ -19,8 +19,7 @@ import aws.smithy.kotlin.runtime.operation.ExecutionContext
 import kotlinx.coroutines.test.runTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
-
-private const val NON_HTTPS_URL = "http://localhost:8080/path/to/resource?foo=bar"
+import kotlin.test.assertTrue
 
 class PresignerTest {
     // Verify that custom endpoint URL schemes aren't changed.
@@ -40,7 +39,7 @@ class PresignerTest {
         val ctx = ExecutionContext()
         val credentialsProvider = TestCredentialsProvider(Credentials("foo", "bar"))
         val endpointResolver = TestEndpointResolver(Endpoint(expectedUrl))
-        val signer = TestSigner(HttpRequest { url(expectedUrl) })
+        val signer = DefaultAwsSigner
         val signingConfig: AwsSigningConfig.Builder.() -> Unit = {
             service = "launch-service"
             region = "the-moon"
@@ -61,7 +60,10 @@ class PresignerTest {
         assertEquals(expectedUrl.host, actualUrl.host)
         assertEquals(expectedUrl.port, actualUrl.port)
         assertEquals(expectedUrl.path, actualUrl.path)
-        assertEquals(expectedUrl.parameters, actualUrl.parameters)
+
+        expectedUrl.parameters.encodedParameters.entryValues.forEach { (key, value) ->
+            assertTrue(actualUrl.parameters.encodedParameters.contains(key, value))
+        }
     }
 }
 

runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/operation/OperationEndpoint.kt

@@ -8,6 +8,7 @@ package aws.smithy.kotlin.runtime.http.operation
 import aws.smithy.kotlin.runtime.InternalApi
 import aws.smithy.kotlin.runtime.client.endpoints.Endpoint
 import aws.smithy.kotlin.runtime.http.request.HttpRequest
+import aws.smithy.kotlin.runtime.http.request.HttpRequestBuilder
 import aws.smithy.kotlin.runtime.http.request.url
 import aws.smithy.kotlin.runtime.identity.Identity
 import aws.smithy.kotlin.runtime.net.Host
@@ -48,10 +49,21 @@ public data class ResolveEndpointRequest(
  */
 @InternalApi
 public fun setResolvedEndpoint(req: SdkHttpRequest, endpoint: Endpoint) {
-    val hostPrefix = req.context.getOrNull(HttpOperationContext.HostPrefix) ?: ""
+    setResolvedEndpoint(req.subject, req.context, endpoint)
+}
+
+/**
+ * Update an existing request with a resolved endpoint.
+ *
+ * Any values serialized to the HTTP path or query string are preserved (in the case of path, the existing serialized one
+ * is appended to what was resolved).
+ */
+@InternalApi
+public fun setResolvedEndpoint(req: HttpRequestBuilder, ctx: ExecutionContext, endpoint: Endpoint) {
+    val hostPrefix = ctx.getOrNull(HttpOperationContext.HostPrefix) ?: ""
     val hostname = "$hostPrefix${endpoint.uri.host}"
 
-    req.subject.url {
+    req.url {
         // Can't use Url.Builder.copyFrom because we need to keep existing path/parameters and merge in new ones
         scheme = endpoint.uri.scheme
         userInfo.copyFrom(endpoint.uri.userInfo)
@@ -65,6 +77,6 @@ public fun setResolvedEndpoint(req: SdkHttpRequest, endpoint: Endpoint) {
         encodedFragment = endpoint.uri.fragment?.encoded
     }
 
-    req.subject.headers["Host"] = hostname
-    endpoint.headers?.let { req.subject.headers.appendAll(it) }
+    req.headers["Host"] = hostname
+    endpoint.headers?.let { req.headers.appendAll(it) }
 }