From fb7eca98ea33e163dcd05e5501157fe20befcf5b Mon Sep 17 00:00:00 2001 From: Jaykumar Gosar Date: Wed, 4 Oct 2023 11:30:08 -0700 Subject: [PATCH 1/2] Uncomment previously disabled AsyncHttpChecksumIntegrationTest --- .../http/pipeline/stages/HttpChecksumStage.java | 4 ++-- .../s3/checksum/AsyncHttpChecksumIntegrationTest.java | 11 ----------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java index 90b2e72bd407..1d9b429e31ac 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java @@ -75,8 +75,8 @@ public SdkHttpFullRequest.Builder execute(SdkHttpFullRequest.Builder request, Re return request; } - // If SRA is enabled, we skip flexible checksum in header - // TODO(post-sra-identity-auth): we should remove this after SRA is fully released + // If SRA is enabled, we skip flexible checksum in header, as it is not supported in the HttpSigner right now + // TODO: we should remove this after it is supported if (sraSigningEnabled(context)) { return request; } diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java index 404e50abca27..86e3dcec25b7 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java @@ -49,7 +49,6 @@ import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.PutObjectResponse; import software.amazon.awssdk.services.s3.utils.CaptureChecksumValidationInterceptor; -import software.amazon.awssdk.services.s3.utils.ChecksumUtils; import software.amazon.awssdk.testutils.RandomTempFile; public class AsyncHttpChecksumIntegrationTest extends S3IntegrationTestBase { @@ -234,11 +233,6 @@ void asyncValidSignedTrailerChecksumCalculatedBySdkClient() { assertThat(response).isEqualTo("Hello world"); } - /** - * If http is used, payload signing will be enforced, but it's not currently supported in async path - * TODO: re-enable it once it's supported - */ - @Disabled("Payload signing is not supported for S3 async client") @Test public void putObject_with_bufferCreatedFromEmptyString() { @@ -261,11 +255,6 @@ public void putObject_with_bufferCreatedFromEmptyString() { assertThat(response).isEqualTo(""); } - /** - * If http is used, payload signing will be enforced, but it's not currently supported in async path - * TODO: re-enable it once it's supported - */ - @Disabled("Payload signing is not supported for S3 async client") @Test public void putObject_with_bufferCreatedFromZeroCapacityByteBuffer() { ByteBuffer content = ByteBuffer.allocate(0); From db5a4889557db9e274348a52fcea7fa4c59286cc Mon Sep 17 00:00:00 2001 From: Jaykumar Gosar Date: Wed, 4 Oct 2023 13:54:07 -0700 Subject: [PATCH 2/2] Update comment to explain more about those tests --- .../http/pipeline/stages/HttpChecksumStage.java | 6 ++---- .../AsyncHttpChecksumIntegrationTest.java | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java index 1d9b429e31ac..ed57ed8ab18f 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/HttpChecksumStage.java @@ -75,8 +75,7 @@ public SdkHttpFullRequest.Builder execute(SdkHttpFullRequest.Builder request, Re return request; } - // If SRA is enabled, we skip flexible checksum in header, as it is not supported in the HttpSigner right now - // TODO: we should remove this after it is supported + // If SRA is enabled, skip flexible checksum in header, since it is handled by SRA signer if (sraSigningEnabled(context)) { return request; } @@ -136,8 +135,7 @@ private void addMd5ChecksumInHeader(SdkHttpFullRequest.Builder request) { private boolean flexibleChecksumInTrailerRequired(RequestExecutionContext context, ChecksumSpecs checksumSpecs) { - // If SRA is enabled and it's sync client, - // skip it since flexible checksum trailer is handled in SRA signer + // If SRA is enabled and it's sync client, skip flexible checksum trailer, since it is handled in SRA signer if (sraSigningEnabled(context) && clientType == ClientType.SYNC) { return false; } diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java index 86e3dcec25b7..1f296345290c 100644 --- a/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java +++ b/services/s3/src/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java @@ -233,9 +233,16 @@ void asyncValidSignedTrailerChecksumCalculatedBySdkClient() { assertThat(response).isEqualTo("Hello world"); } + /** + * S3 clients by default don't do payload signing. But when http is used, payload signing is expected to be enforced. But + * payload signing is not currently supported in async path (for both pre/post SRA signers). + * However, this test passes, because of https://github + * .com/aws/aws-sdk-java-v2/blob/38e221bd815af31a6c6b91557499af155103c21a/core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAwsS3V4Signer.java#L279-L285. + * Keeping this test enabled, to ensure moving to SRA Identity & Auth, does not break current behavior. + * TODO: Update this test with right asserts when payload signing is supported in async. + */ @Test public void putObject_with_bufferCreatedFromEmptyString() { - s3HttpAsync.putObject(PutObjectRequest.builder() .bucket(BUCKET) .key(KEY) @@ -255,6 +262,14 @@ public void putObject_with_bufferCreatedFromEmptyString() { assertThat(response).isEqualTo(""); } + /** + * S3 clients by default don't do payload signing. But when http is used, payload signing is expected to be enforced. But + * payload signing is not currently supported in async path (for both pre/post SRA signers). + * However, this test passes, because of https://github + * .com/aws/aws-sdk-java-v2/blob/38e221bd815af31a6c6b91557499af155103c21a/core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAwsS3V4Signer.java#L279-L285. + * Keeping this test enabled, to ensure moving to SRA Identity & Auth, does not break current behavior. + * TODO: Update this test with right asserts when payload signing is supported in async. + */ @Test public void putObject_with_bufferCreatedFromZeroCapacityByteBuffer() { ByteBuffer content = ByteBuffer.allocate(0);