diff --git a/.changes/next-release/bugfix-AmazonS3-a1f2f3a.json b/.changes/next-release/bugfix-AmazonS3-a1f2f3a.json new file mode 100644 index 000000000000..e22ad99af328 --- /dev/null +++ b/.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" +} diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/PutObjectIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/PutObjectIntegrationTest.java index f54d20e0487d..720589078ecb 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/PutObjectIntegrationTest.java +++ b/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; + } + } } diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/s3express/S3ExpressIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/s3express/S3ExpressIntegrationTest.java index eebb1706f4ad..7e93b299c272 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/s3express/S3ExpressIntegrationTest.java +++ b/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 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 capturedRequests() { return Collections.unmodifiableList(capturedRequests); } + + public boolean isMd5Enabled() { + return isMd5Enabled; + } } } diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/checksums/ChecksumsEnabledValidator.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/checksums/ChecksumsEnabledValidator.java index cb0bde9f4019..07c26c582e89 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/checksums/ChecksumsEnabledValidator.java +++ b/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 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); } diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/S3ExpressChecksumInterceptor.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/S3ExpressChecksumInterceptor.java index c303ba248fe5..2cd3401f7487 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/S3ExpressChecksumInterceptor.java +++ b/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) { diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumsEnabledValidatorTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumsEnabledValidatorTest.java index ef2f653da88f..eebe16d636e0 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumsEnabledValidatorTest.java +++ b/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();