Self review

Author: 0marperez

runtime/auth/http-auth-aws/common/src/aws/smithy/kotlin/runtime/http/auth/AwsHttpSigner.kt

@@ -164,7 +164,7 @@ public class AwsHttpSigner(private val config: Config) : HttpSigner {
             hashSpecification = when {
                 contextHashSpecification != null -> contextHashSpecification
                 body is HttpBody.Empty -> HashSpecification.EmptyBody
-                (body.isEligibleForAwsChunkedStreaming && enableAwsChunked) -> { // TODO: Enable AWS chunked for all ?!
+                body.isEligibleForAwsChunkedStreaming && enableAwsChunked -> {
                     if (request.headers.contains("x-amz-trailer")) {
                         if (config.isUnsignedPayload) HashSpecification.StreamingUnsignedPayloadWithTrailers else HashSpecification.StreamingAws4HmacSha256PayloadWithTrailers
                     } else {

runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

@@ -18,20 +18,43 @@ import aws.smithy.kotlin.runtime.http.request.HttpRequest
 import aws.smithy.kotlin.runtime.http.request.header
 import aws.smithy.kotlin.runtime.http.request.toBuilder
 import aws.smithy.kotlin.runtime.io.*
+import aws.smithy.kotlin.runtime.telemetry.logging.Logger
+import aws.smithy.kotlin.runtime.telemetry.logging.debug
 import aws.smithy.kotlin.runtime.telemetry.logging.logger
 import aws.smithy.kotlin.runtime.text.encoding.encodeBase64String
 import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.job
 import kotlin.coroutines.coroutineContext
 
 /**
- * TODO -
+ * Calculates a request's checksum.
+ *
+ * If a user supplies a checksum via an HTTP header no calculation will be done. The exception is MD5, if a user
+ * supplies an MD5 checksum header it will be ignored.
+ *
+ * If the request configuration and model requires checksum calculation:
+ * - Check if the user configured a checksum algorithm for the request and attempt to use that.
+ * - If no checksum is configured for the request then use the default checksum algorithm to calculate a checksum.
+ *
+ * If the request will be streamed:
+ * - The checksum calculation is done asynchronously using a hashing & completing body.
+ * - The checksum will be sent in a trailing header, once the request is consumed.
+ *
+ * If the request will not be streamed:
+ * - The checksum calculation is done synchronously
+ * - The checksum will be sent in a header
+ *
+ * Business metrics MUST be emitted for the checksum algorithm used.
+ *
+ * @param requestChecksumRequired Model sourced flag indicating if checksum calculation is mandatory.
+ * @param requestChecksumCalculation Configuration option that determines when checksum calculation should be done.
+ * @param userSelectedChecksumAlgorithm The checksum algorithm that the user selected for the request, may be null.
  */
 @InternalApi
 public class FlexibleChecksumsRequestInterceptor(
     requestChecksumRequired: Boolean,
     requestChecksumCalculation: ChecksumConfigOption?,
-    private val userSelectedChecksumAlgorithm: String?,
+    userSelectedChecksumAlgorithm: String?,
 ) : AbstractChecksumInterceptor() {
     private val forcedToCalculateChecksum = requestChecksumRequired || requestChecksumCalculation == ChecksumConfigOption.WHEN_SUPPORTED
     private val checksumHeader = StringBuilder("x-amz-checksum-")
@@ -55,7 +78,7 @@ public class FlexibleChecksumsRequestInterceptor(
     override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
         val logger = coroutineContext.logger<FlexibleChecksumsRequestInterceptor>()
 
-        userProviderChecksumHeader(context.protocolRequest)?.let {
+        userProviderChecksumHeader(context.protocolRequest, logger)?.let {
             logger.debug { "User supplied a checksum via header, skipping checksum calculation" }
 
             val request = context.protocolRequest.toBuilder()
@@ -72,8 +95,6 @@ public class FlexibleChecksumsRequestInterceptor(
 
         val request = context.protocolRequest.toBuilder()
 
-//        throw Exception("\nBody type: ${request.body::class.simpleName}\nEligible for chunked streaming: ${request.body.isEligibleForAwsChunkedStreaming}\nContent Length: ${request.body.contentLength}\nIs one shot: ${request.body.isOneShot}")
-
         if (request.body.isEligibleForAwsChunkedStreaming) {
             val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
 
@@ -216,11 +237,17 @@ public class FlexibleChecksumsRequestInterceptor(
      * The header must start with "x-amz-checksum-" followed by the checksum algorithm's name.
      * MD5 is not considered a valid checksum algorithm.
      */
-    private fun userProviderChecksumHeader(request: HttpRequest): String? {
+    private fun userProviderChecksumHeader(request: HttpRequest, logger: Logger): String? {
         request.headers.entries().forEach { header ->
             val headerName = header.key.lowercase()
-            if (headerName.startsWith("x-amz-checksum-") && !headerName.endsWith("md5")) {
-                return headerName
+            if (headerName.startsWith("x-amz-checksum-")) {
+                if (headerName.endsWith("md5")) {
+                    logger.debug {
+                        "User provided md5 request checksum via headers, md5 is not a valid algorithm, ignoring header"
+                    }
+                } else {
+                    return headerName
+                }
             }
         }
         return null

runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor.kt

@@ -19,7 +19,6 @@ import aws.smithy.kotlin.runtime.http.response.copy
 import aws.smithy.kotlin.runtime.http.toHashingBody
 import aws.smithy.kotlin.runtime.http.toHttpBody
 import aws.smithy.kotlin.runtime.io.*
-import aws.smithy.kotlin.runtime.telemetry.logging.debug
 import aws.smithy.kotlin.runtime.telemetry.logging.logger
 import aws.smithy.kotlin.runtime.text.encoding.encodeBase64String
 import kotlin.coroutines.coroutineContext
@@ -41,12 +40,12 @@ internal val CHECKSUM_HEADER_VALIDATION_PRIORITY_LIST: List<String> = listOf(
  *
  * Users can check which checksum was validated by referencing the `ResponseChecksumValidated` execution context variable.
  *
- * @param responseValidation Flag indicating if the checksum validation is mandatory.
+ * @param responseValidationRequired Flag indicating if the checksum validation is mandatory.
  * @param responseChecksumValidation Configuration option that determines when checksum validation should be done.
  */
 @InternalApi
 public class FlexibleChecksumsResponseInterceptor(
-    private val responseValidation: Boolean,
+    private val responseValidationRequired: Boolean,
     private val responseChecksumValidation: ChecksumConfigOption?,
 ) : HttpInterceptor {
     @InternalApi
@@ -58,40 +57,47 @@ public class FlexibleChecksumsResponseInterceptor(
     override suspend fun modifyBeforeDeserialization(context: ProtocolResponseInterceptorContext<Any, HttpRequest, HttpResponse>): HttpResponse {
         val logger = coroutineContext.logger<FlexibleChecksumsResponseInterceptor>()
 
-        val forcedToVerifyChecksum = responseValidation || responseChecksumValidation == ChecksumConfigOption.WHEN_SUPPORTED
+        val forcedToVerifyChecksum = responseValidationRequired || responseChecksumValidation == ChecksumConfigOption.WHEN_SUPPORTED
         if (!forcedToVerifyChecksum) return context.protocolResponse
 
         val checksumHeader = CHECKSUM_HEADER_VALIDATION_PRIORITY_LIST
             .firstOrNull { context.protocolResponse.headers.contains(it) } ?: run {
             logger.warn { "User requested checksum validation, but the response headers did not contain any valid checksums" }
             return context.protocolResponse
         }
-        val checksumAlgorithm = checksumHeader.removePrefix("x-amz-checksum-").toHashFunction() ?: throw ClientException("Could not parse checksum algorithm from header $checksumHeader")
-        val checksumValue = context.protocolResponse.headers[checksumHeader]!!
+        val serviceChecksumValue = context.protocolResponse.headers[checksumHeader]!!
 
-        if (checksumValue.isCompositeChecksum()) {
+        if (serviceChecksumValue.isCompositeChecksum()) {
             logger.debug { "Service returned composite checksum. Skipping validation." }
             return context.protocolResponse
         }
 
         logger.debug { "Validating checksum from $checksumHeader" }
         context.executionContext[ChecksumHeaderValidated] = checksumHeader
 
+        val checksumAlgorithm = checksumHeader
+            .removePrefix("x-amz-checksum-")
+            .toHashFunction() ?: throw ClientException("Could not parse checksum algorithm from header $checksumHeader")
+
         if (context.protocolResponse.body is HttpBody.Bytes) {
+            // Calculate checksum
             checksumAlgorithm.update(
                 context.protocolResponse.body.readAll() ?: byteArrayOf(),
             )
+            val sdkChecksumValue = checksumAlgorithm.digest().encodeBase64String()
+
             validateAndThrow(
-                checksumValue,
-                checksumAlgorithm.digest().encodeBase64String(),
+                serviceChecksumValue,
+                sdkChecksumValue,
             )
+
             return context.protocolResponse
         } else {
             // Wrap the response body in a hashing body
             return context.protocolResponse.copy(
                 body = context.protocolResponse.body
                     .toHashingBody(checksumAlgorithm, context.protocolResponse.body.contentLength)
-                    .toChecksumValidatingBody(checksumValue),
+                    .toChecksumValidatingBody(serviceChecksumValue),
             )
         }
     }

runtime/protocol/http-client/common/test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptorTest.kt

@@ -75,7 +75,7 @@ class FlexibleChecksumsResponseInterceptorTest {
 
             op.interceptors.add(
                 FlexibleChecksumsResponseInterceptor(
-                    responseValidation = true,
+                    responseValidationRequired = true,
                     responseChecksumValidation = ChecksumConfigOption.WHEN_SUPPORTED,
                 ),
             )
@@ -102,7 +102,7 @@ class FlexibleChecksumsResponseInterceptorTest {
 
             op.interceptors.add(
                 FlexibleChecksumsResponseInterceptor(
-                    responseValidation = true,
+                    responseValidationRequired = true,
                     responseChecksumValidation = ChecksumConfigOption.WHEN_SUPPORTED,
                 ),
             )
@@ -130,7 +130,7 @@ class FlexibleChecksumsResponseInterceptorTest {
 
         op.interceptors.add(
             FlexibleChecksumsResponseInterceptor(
-                responseValidation = true,
+                responseValidationRequired = true,
                 responseChecksumValidation = ChecksumConfigOption.WHEN_SUPPORTED,
             ),
         )
@@ -155,7 +155,7 @@ class FlexibleChecksumsResponseInterceptorTest {
 
         op.interceptors.add(
             FlexibleChecksumsResponseInterceptor(
-                responseValidation = true,
+                responseValidationRequired = true,
                 responseChecksumValidation = ChecksumConfigOption.WHEN_SUPPORTED,
             ),
         )

runtime/runtime-core/api/runtime-core.api

@@ -697,11 +697,6 @@ public final class aws/smithy/kotlin/runtime/hashing/HashFunctionKt {
 	public static final fun toHashFunction (Ljava/lang/String;)Laws/smithy/kotlin/runtime/hashing/HashFunction;
 }
 
-public final class aws/smithy/kotlin/runtime/hashing/HashingAttributes {
-	public static final field INSTANCE Laws/smithy/kotlin/runtime/hashing/HashingAttributes;
-	public final fun getChecksumStreamingRequest ()Laws/smithy/kotlin/runtime/collections/AttributeKey;
-}
-
 public final class aws/smithy/kotlin/runtime/hashing/HmacKt {
 	public static final fun hmac ([B[BLaws/smithy/kotlin/runtime/hashing/HashFunction;)[B
 	public static final fun hmac ([B[BLkotlin/jvm/functions/Function0;)[B

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/hashing/HashingAttributes.kt

@@ -1,40 +1,40 @@
 package aws.smithy.kotlin.runtime.client.config
 
 /**
- * todo
+ * Client config for HTTP checksums
  */
 public interface HttpChecksumClientConfig {
     /**
-     * todo
+     * Configures request checksum calculation
      */
     public val requestChecksumCalculation: ChecksumConfigOption?
 
     /**
-     * todo
+     * Configures response checksum validation
      */
     public val responseChecksumValidation: ChecksumConfigOption?
 
     public interface Builder {
         /**
-         * todo
+         * Configures request checksum calculation
          */
         public var requestChecksumCalculation: ChecksumConfigOption?
 
         /**
-         * todo
+         * Configures response checksum validation
          */
         public var responseChecksumValidation: ChecksumConfigOption?
     }
 }
 
 public enum class ChecksumConfigOption {
     /**
-     * todo
+     * SDK will create/validate checksum if the service marks it as required or if this is set.
      */
     WHEN_SUPPORTED,
 
     /**
-     * todo
+     * SDK will only create/validate checksum if the service marks it as required.
      */
     WHEN_REQUIRED,
 }