Do not validate MD5 if checksum value supplied

.changes/next-release/bugfix-AmazonS3-a1f2f3a.json

@@ -0,0 +1,6 @@
+{
+    "category": "Amazon S3", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Fixes bug where Md5 validation is performed for S3 PutObject even if checksum value is supplied"
+}

services/s3/src/it/java/software/amazon/awssdk/services/s3/PutObjectIntegrationTest.java

@@ -17,6 +17,7 @@
 package software.amazon.awssdk.services.s3;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static software.amazon.awssdk.services.s3.internal.checksums.ChecksumsEnabledValidator.CHECKSUM;
 import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName;
 import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely;
 
@@ -26,13 +27,19 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
+import java.util.Base64;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import software.amazon.awssdk.core.async.BlockingInputStreamAsyncRequestBody;
+import software.amazon.awssdk.core.interceptor.Context;
+import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
+import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
 import software.amazon.awssdk.core.sync.RequestBody;
 import software.amazon.awssdk.http.ContentStreamProvider;
 import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
@@ -46,9 +53,8 @@ public class PutObjectIntegrationTest extends S3IntegrationTestBase {
     private static final String BUCKET = temporaryBucketName(PutObjectIntegrationTest.class);
     private static final String ASYNC_KEY = "async-key";
     private static final String SYNC_KEY = "sync-key";
-
-    private static final byte[] CONTENT = "Hello".getBytes(StandardCharsets.UTF_8);
     private static final String TEXT_CONTENT_TYPE = "text/plain";
+    private static final byte[] CONTENT = "Hello".getBytes(StandardCharsets.UTF_8);
 
     @BeforeClass
     public static void setUp() throws Exception {
@@ -61,6 +67,28 @@ public static void tearDown() {
         deleteBucketAndAllContents(BUCKET);
     }
 
+    @Test
+    public void putObject_withUserCalculatedChecksum_doesNotPerformMd5Validation() throws NoSuchAlgorithmException {
+        CapturingInterceptor capturingInterceptor = new CapturingInterceptor();
+        S3Client s3Client = s3ClientBuilder()
+            .overrideConfiguration(o -> o.addExecutionInterceptor(capturingInterceptor))
+            .build();
+
+        MessageDigest md = MessageDigest.getInstance("SHA-1");
+        md.update(CONTENT);
+        byte[] checksum = md.digest();
+        String checksumVal = Base64.getEncoder().encodeToString(checksum);
+
+        PutObjectRequest request = PutObjectRequest.builder()
+                                                   .bucket(BUCKET)
+                                                   .key(SYNC_KEY)
+                                                   .checksumSHA1(checksumVal)
+                                                   .build();
+
+        s3Client.putObject(request, RequestBody.fromString("Hello"));
+        assertThat(capturingInterceptor.isMd5Enabled).isFalse();
+    }
+
     @Test
     public void objectInputStreamsAreClosed() {
         TestContentProvider provider = new TestContentProvider(CONTENT);
@@ -148,4 +176,17 @@ boolean isClosed() {
             return isClosed;
         }
     }
+
+    private static class CapturingInterceptor implements ExecutionInterceptor {
+        private boolean isMd5Enabled;
+
+        @Override
+        public void beforeTransmission(Context.BeforeTransmission context, ExecutionAttributes executionAttributes) {
+            isMd5Enabled = executionAttributes.getAttribute(CHECKSUM) != null;
+        }
+
+        public boolean isMd5Enabled() {
+            return isMd5Enabled;
+        }
+    }
 }

services/s3/src/it/java/software/amazon/awssdk/services/s3/s3express/S3ExpressIntegrationTest.java

@@ -18,7 +18,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static software.amazon.awssdk.services.s3.internal.checksums.ChecksumsEnabledValidator.CHECKSUM;
 import static software.amazon.awssdk.testutils.service.AwsTestBase.CREDENTIALS_PROVIDER_CHAIN;
 import static software.amazon.awssdk.testutils.service.S3BucketUtils.temporaryBucketName;
 
@@ -207,7 +207,7 @@ public void s3Express_nonObjectTransferApis_Async(AsyncTestCase tc) {
     }
 
     @Test
-    public void putObject_withUserCalculatedChecksum_doesNotAddMultipleHeaders() throws NoSuchAlgorithmException {
+    public void putObject_withUserCalculatedChecksum_doesNotAddMultipleHeadersOrPerformMd5Validation() throws NoSuchAlgorithmException {
         MessageDigest md = MessageDigest.getInstance("SHA-1");
         byte[] data = CONTENTS.getBytes(StandardCharsets.UTF_8);
 
@@ -221,11 +221,9 @@ public void putObject_withUserCalculatedChecksum_doesNotAddMultipleHeaders() thr
                                                    .checksumSHA1(checksumVal)
                                                    .build();
 
-        // TODO (s3Express) - checksum calculation incorrect, but original bug is fixed
-        RetryableException exception = assertThrows(RetryableException.class, () ->
-            s3.putObject(request, RequestBody.fromString(CONTENTS)));
-        assertThat(exception.getMessage()).doesNotContain("Expecting a single x-amz-checksum- header");
-        assertThat(exception.getMessage()).contains("Data read has a different checksum than expected");
+        s3.putObject(request, RequestBody.fromString(CONTENTS));
+        assertThat(capturingInterceptor.capturedRequests()).hasSize(1);
+        assertThat(capturingInterceptor.isMd5Enabled).isFalse();
     }
 
     @Test
@@ -471,10 +469,12 @@ protected static void runAndVerify(AsyncTestCase testCase) {
 
     private static class CapturingInterceptor implements ExecutionInterceptor {
         private final List<SdkHttpRequest> capturedRequests = new ArrayList<>();
+        private boolean isMd5Enabled;
 
         @Override
         public void beforeTransmission(Context.BeforeTransmission context, ExecutionAttributes executionAttributes) {
             capturedRequests.add(context.httpRequest());
+            isMd5Enabled = executionAttributes.getAttribute(CHECKSUM) != null;
         }
 
         public void reset() {
@@ -484,5 +484,9 @@ public void reset() {
         public List<SdkHttpRequest> capturedRequests() {
             return Collections.unmodifiableList(capturedRequests);
         }
+
+        public boolean isMd5Enabled() {
+            return isMd5Enabled;
+        }
     }
 }

services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/checksums/ChecksumsEnabledValidator.java

@@ -19,6 +19,7 @@
 
 import java.util.Arrays;
 import java.util.Optional;
+import java.util.stream.Stream;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.auth.signer.AwsSignerExecutionAttribute;
 import software.amazon.awssdk.core.ClientType;
@@ -110,14 +111,18 @@ public static boolean shouldRecordChecksum(SdkRequest sdkRequest,
         }
 
         //Checksum validation is done at Service side when HTTP Checksum algorithm attribute is set.
-        if (isHttpCheckSumValidationEnabled(executionAttributes)) {
+        if (isHttpCheckSumValidationEnabled(executionAttributes, sdkRequest)) {
             return false;
         }
 
         return checksumEnabledPerConfig(executionAttributes);
     }
 
-    private static boolean isHttpCheckSumValidationEnabled(ExecutionAttributes executionAttributes) {
+    private static boolean isHttpCheckSumValidationEnabled(ExecutionAttributes executionAttributes, SdkRequest request) {
+        if (isChecksumValueSpecified(request)) {
+            return true;
+        }
+
         Optional<ChecksumSpecs> resolvedChecksum =
             executionAttributes.getOptionalAttribute(SdkExecutionAttribute.RESOLVED_CHECKSUM_SPECS);
         if (resolvedChecksum.isPresent()) {
@@ -127,6 +132,11 @@ private static boolean isHttpCheckSumValidationEnabled(ExecutionAttributes execu
         return false;
     }
 
+    private static boolean isChecksumValueSpecified(SdkRequest request) {
+        return Stream.of("ChecksumCRC32", "ChecksumCRC32C", "ChecksumSHA1", "ChecksumSHA256")
+                     .anyMatch(s -> request.getValueForField(s, String.class).isPresent());
+    }
+
     public static boolean responseChecksumIsValid(SdkHttpResponse httpResponse) {
         return !hasServerSideEncryptionHeader(httpResponse);
     }

services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/S3ExpressChecksumInterceptor.java

@@ -21,6 +21,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.stream.Stream;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.checksums.DefaultChecksumAlgorithm;
 import software.amazon.awssdk.checksums.spi.ChecksumAlgorithm;
@@ -97,10 +98,8 @@ public SdkRequest modifyRequest(Context.ModifyRequest context, ExecutionAttribut
     }
 
     private boolean requestContainsUserCalculatedChecksum(SdkRequest request) {
-        return request.getValueForField("ChecksumCRC32", String.class).isPresent()
-               || request.getValueForField("ChecksumCRC32C", String.class).isPresent()
-               || request.getValueForField("ChecksumSHA1", String.class).isPresent()
-               || request.getValueForField("ChecksumSHA256", String.class).isPresent();
+        return Stream.of("ChecksumCRC32", "ChecksumCRC32C", "ChecksumSHA1", "ChecksumSHA256")
+                     .anyMatch(s -> request.getValueForField(s, String.class).isPresent());
     }
 
     private boolean shouldAlwaysAddChecksum(ChecksumSpecs checksumSpecs, SdkRequest request) {

services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumsEnabledValidatorTest.java

@@ -154,6 +154,54 @@ public void putObjectChecksumEnabled_serverSideEncryption_false() {
                                         response)).isFalse();
     }
 
+    @Test
+    public void putObject_crc32ValueSupplied_shouldNotValidateMd5() {
+        ExecutionAttributes executionAttributes = getSyncExecutionAttributes();
+
+        assertThat(shouldRecordChecksum(PutObjectRequest.builder()
+                                                        .checksumCRC32("checksumVal")
+                                                        .build(),
+                                        ClientType.SYNC,
+                                        executionAttributes,
+                                        emptyHttpRequest().build())).isFalse();
+    }
+
+    @Test
+    public void putObject_crc32cValueSupplied_shouldNotValidateMd5() {
+        ExecutionAttributes executionAttributes = getSyncExecutionAttributes();
+
+        assertThat(shouldRecordChecksum(PutObjectRequest.builder()
+                                                        .checksumCRC32C("checksumVal")
+                                                        .build(),
+                                        ClientType.SYNC,
+                                        executionAttributes,
+                                        emptyHttpRequest().build())).isFalse();
+    }
+
+    @Test
+    public void putObject_sha1ValueSupplied_shouldNotValidateMd5() {
+        ExecutionAttributes executionAttributes = getSyncExecutionAttributes();
+
+        assertThat(shouldRecordChecksum(PutObjectRequest.builder()
+                                                        .checksumSHA1("checksumVal")
+                                                        .build(),
+                                        ClientType.SYNC,
+                                        executionAttributes,
+                                        emptyHttpRequest().build())).isFalse();
+    }
+
+    @Test
+    public void putObject_sha256ValueSupplied_shouldNotValidateMd5() {
+        ExecutionAttributes executionAttributes = getSyncExecutionAttributes();
+
+        assertThat(shouldRecordChecksum(PutObjectRequest.builder()
+                                                        .checksumSHA256("checksumVal")
+                                                        .build(),
+                                        ClientType.SYNC,
+                                        executionAttributes,
+                                        emptyHttpRequest().build())).isFalse();
+    }
+
     @Test
     public void responseChecksumIsValid_defaultTrue() {
         assertThat(responseChecksumIsValid(SdkHttpResponse.builder().build())).isTrue();