From d65acc65bc2637b6fb6f33ecc361bae20c755920 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:15:35 -0700 Subject: [PATCH 1/6] Do not validate Md5 if checksum value supplied --- .../services/s3/PutObjectIntegrationTest.java | 76 +++++++++++-------- .../s3express/S3ExpressIntegrationTest.java | 18 +++-- .../checksums/ChecksumsEnabledValidator.java | 14 +++- .../S3ExpressChecksumInterceptor.java | 7 +- 4 files changed, 72 insertions(+), 43 deletions(-) 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..e44eb766594e 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,39 +27,63 @@ 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; import software.amazon.awssdk.services.s3.model.PutObjectRequest; -import software.amazon.awssdk.services.s3.model.PutObjectResponse; /** * Integration tests for {@code PutObject}. */ public class PutObjectIntegrationTest extends S3IntegrationTestBase { - private static final String BUCKET = temporaryBucketName(PutObjectIntegrationTest.class); + private static final String BUCKET = "embers-test-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"; @BeforeClass public static void setUp() throws Exception { S3IntegrationTestBase.setUp(); - createBucket(BUCKET); + //createBucket(BUCKET); } @AfterClass public static void tearDown() { - deleteBucketAndAllContents(BUCKET); + //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 @@ -72,28 +97,6 @@ public void objectInputStreamsAreClosed() { } } - @Test - public void blockingInputStreamAsyncRequestBody_withContentType_isHonored() { - BlockingInputStreamAsyncRequestBody requestBody = - BlockingInputStreamAsyncRequestBody.builder() - .contentLength((long) CONTENT.length) - .contentType(TEXT_CONTENT_TYPE) - .build(); - - PutObjectRequest.Builder request = PutObjectRequest.builder() - .bucket(BUCKET) - .key(ASYNC_KEY); - - CompletableFuture responseFuture = s3Async.putObject(request.build(), requestBody); - requestBody.writeInputStream(new ByteArrayInputStream(CONTENT)); - responseFuture.join(); - - HeadObjectResponse response = s3Async.headObject(r -> r.bucket(BUCKET).key(ASYNC_KEY)).join(); - - assertThat(response.contentLength()).isEqualTo(CONTENT.length); - assertThat(response.contentType()).isEqualTo(TEXT_CONTENT_TYPE); - } - @Test public void s3Client_usingHttpAndDisableChunkedEncoding() { try (S3Client s3Client = s3ClientBuilder() @@ -148,4 +151,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) { From 5210b0b4faf8cca8d5daa70096355fb1ccf9e335 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:17:48 -0700 Subject: [PATCH 2/6] Do not validate Md5 if checksum value supplied --- .../awssdk/services/s3/PutObjectIntegrationTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 e44eb766594e..21cc993b4150 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 @@ -46,8 +46,7 @@ * Integration tests for {@code PutObject}. */ public class PutObjectIntegrationTest extends S3IntegrationTestBase { - private static final String BUCKET = "embers-test-bucket"; - //temporaryBucketName(PutObjectIntegrationTest.class); + private static final String BUCKET = temporaryBucketName(PutObjectIntegrationTest.class); private static final String ASYNC_KEY = "async-key"; private static final String SYNC_KEY = "sync-key"; @@ -56,12 +55,12 @@ public class PutObjectIntegrationTest extends S3IntegrationTestBase { @BeforeClass public static void setUp() throws Exception { S3IntegrationTestBase.setUp(); - //createBucket(BUCKET); + createBucket(BUCKET); } @AfterClass public static void tearDown() { - //deleteBucketAndAllContents(BUCKET); + deleteBucketAndAllContents(BUCKET); } @Test From 04f20b0cdbd9bace4dabc07fbf12e3aee61d1ddc Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:39:04 -0700 Subject: [PATCH 3/6] Add changelog --- .changes/next-release/bugfix-AmazonS3-a1f2f3a.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/bugfix-AmazonS3-a1f2f3a.json 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" +} From ca628f9260775964d6fcdcab2cc240b5968c62c4 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:50:50 -0700 Subject: [PATCH 4/6] Add unit tests --- .../ChecksumsEnabledValidatorTest.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) 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(); From ad90a1620a2e53a7d0263e89e6ca90bddbb5a07b Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:56:59 -0700 Subject: [PATCH 5/6] Add back deleted test --- .../services/s3/PutObjectIntegrationTest.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) 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 21cc993b4150..4b2efc7b093b 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 @@ -96,6 +96,28 @@ public void objectInputStreamsAreClosed() { } } + @Test + public void blockingInputStreamAsyncRequestBody_withContentType_isHonored() { + BlockingInputStreamAsyncRequestBody requestBody = + BlockingInputStreamAsyncRequestBody.builder() + .contentLength((long) CONTENT.length) + .contentType(TEXT_CONTENT_TYPE) + .build(); + + PutObjectRequest.Builder request = PutObjectRequest.builder() + .bucket(BUCKET) + .key(ASYNC_KEY); + + CompletableFuture responseFuture = s3Async.putObject(request.build(), requestBody); + requestBody.writeInputStream(new ByteArrayInputStream(CONTENT)); + responseFuture.join(); + + HeadObjectResponse response = s3Async.headObject(r -> r.bucket(BUCKET).key(ASYNC_KEY)).join(); + + assertThat(response.contentLength()).isEqualTo(CONTENT.length); + assertThat(response.contentType()).isEqualTo(TEXT_CONTENT_TYPE); + } + @Test public void s3Client_usingHttpAndDisableChunkedEncoding() { try (S3Client s3Client = s3ClientBuilder() From 31601c6bed4cf2b4b6019fa7c2c4765b582b540d Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 10 Jun 2024 09:58:39 -0700 Subject: [PATCH 6/6] Add back deleted test --- .../amazon/awssdk/services/s3/PutObjectIntegrationTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 4b2efc7b093b..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 @@ -32,15 +32,19 @@ 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; import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.PutObjectResponse; /** * Integration tests for {@code PutObject}. @@ -49,7 +53,7 @@ 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 String TEXT_CONTENT_TYPE = "text/plain"; private static final byte[] CONTENT = "Hello".getBytes(StandardCharsets.UTF_8); @BeforeClass