Skip to content

Commit

Permalink
Merge branch 'master' into dongie/fix-changelog-entry
Browse files Browse the repository at this point in the history
  • Loading branch information
@dagnir
dagnir authored Jun 12, 2024
2 parents d0d7af7 + 859c2b1 commit 8f342c1
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 15 deletions.
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AmazonS3-a1f2f3a.json
View file
Original file line number Diff line number Diff line change
@@ -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"
}
45 changes: 43 additions & 2 deletions services/s3/src/it/java/software/amazon/awssdk/services/s3/PutObjectIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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 {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
}
18 changes: 11 additions & 7 deletions ...s3/src/it/java/software/amazon/awssdk/services/s3/s3express/S3ExpressIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);

Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand All @@ -484,5 +484,9 @@ public void reset() {
public List<SdkHttpRequest> capturedRequests() {
return Collections.unmodifiableList(capturedRequests);
}

public boolean isMd5Enabled() {
return isMd5Enabled;
}
}
}
14 changes: 12 additions & 2 deletions ...java/software/amazon/awssdk/services/s3/internal/checksums/ChecksumsEnabledValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand All @@ -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);
}
Expand Down
7 changes: 3 additions & 4 deletions ...va/software/amazon/awssdk/services/s3/internal/handlers/S3ExpressChecksumInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
48 changes: 48 additions & 0 deletions ...test/java/software/amazon/awssdk/services/s3/checksums/ChecksumsEnabledValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 8f342c1

Please sign in to comment.