From fd7517e488d06fe66589a3c9302ff6bd0bb512dc Mon Sep 17 00:00:00 2001 From: Anna-Karin Salander Date: Wed, 13 Sep 2023 08:34:29 -0700 Subject: [PATCH 1/2] Change config for Sigv4a signing to not perform path normalization, which causes signature mismatch errors with S3 service --- .../bugfix-AWSSDKforJavav2-750825c.json | 6 ++++++ .../signer/internal/SigningConfigProvider.java | 14 +++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-750825c.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-750825c.json b/.changes/next-release/bugfix-AWSSDKforJavav2-750825c.json new file mode 100644 index 000000000000..c5a66852f4b0 --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-750825c.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "Do not instruct the CRT Sigv4a signer to do path normalization to avoid signature mismatch errors" +} diff --git a/core/auth-crt/src/main/java/software/amazon/awssdk/authcrt/signer/internal/SigningConfigProvider.java b/core/auth-crt/src/main/java/software/amazon/awssdk/authcrt/signer/internal/SigningConfigProvider.java index 9f18c030dc03..0ed8c1645181 100644 --- a/core/auth-crt/src/main/java/software/amazon/awssdk/authcrt/signer/internal/SigningConfigProvider.java +++ b/core/auth-crt/src/main/java/software/amazon/awssdk/authcrt/signer/internal/SigningConfigProvider.java @@ -32,6 +32,7 @@ public class SigningConfigProvider { private static final Boolean DEFAULT_DOUBLE_URL_ENCODE = Boolean.TRUE; + private static final Boolean DEFAULT_PATH_NORMALIZATION = Boolean.TRUE; public SigningConfigProvider() { } @@ -89,13 +90,20 @@ private AwsSigningConfig createPresigningConfig(ExecutionAttributes executionAtt private AwsSigningConfig createDefaultRequestConfig(ExecutionAttributes executionAttributes) { AwsSigningConfig signingConfig = createStringToSignConfig(executionAttributes); - signingConfig.setShouldNormalizeUriPath(true); + if (executionAttributes.getAttribute(AwsSignerExecutionAttribute.SIGNER_NORMALIZE_PATH) != null) { + signingConfig.setShouldNormalizeUriPath( + executionAttributes.getAttribute(AwsSignerExecutionAttribute.SIGNER_NORMALIZE_PATH)); + } else { + signingConfig.setShouldNormalizeUriPath(DEFAULT_PATH_NORMALIZATION); + } + if (executionAttributes.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE) != null) { - signingConfig.setUseDoubleUriEncode(executionAttributes - .getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)); + signingConfig.setUseDoubleUriEncode( + executionAttributes.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)); } else { signingConfig.setUseDoubleUriEncode(DEFAULT_DOUBLE_URL_ENCODE); } + return signingConfig; } From 2a74a9fe973d020bc00892c40f58c3d0341c41ef Mon Sep 17 00:00:00 2001 From: Anna-Karin Salander Date: Thu, 14 Sep 2023 11:56:57 -0700 Subject: [PATCH 2/2] Adds different paths to MRAP integration test --- .../S3MrapIntegrationTest.java | 236 ++++++++++-------- 1 file changed, 134 insertions(+), 102 deletions(-) diff --git a/services/s3control/src/it/java/software.amazon.awssdk.services.s3control/S3MrapIntegrationTest.java b/services/s3control/src/it/java/software.amazon.awssdk.services.s3control/S3MrapIntegrationTest.java index d0abe039aef7..af18e2257d91 100644 --- a/services/s3control/src/it/java/software.amazon.awssdk.services.s3control/S3MrapIntegrationTest.java +++ b/services/s3control/src/it/java/software.amazon.awssdk.services.s3control/S3MrapIntegrationTest.java @@ -22,17 +22,21 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.time.Duration; -import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Optional; -import org.junit.BeforeClass; -import org.junit.Test; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import software.amazon.awssdk.auth.signer.AwsSignerExecutionAttribute; import software.amazon.awssdk.auth.signer.S3SignerExecutionAttribute; import software.amazon.awssdk.auth.signer.internal.SignerConstant; import software.amazon.awssdk.awscore.presigner.PresignedRequest; import software.amazon.awssdk.core.SdkRequest; -import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; import software.amazon.awssdk.core.interceptor.Context; import software.amazon.awssdk.core.interceptor.ExecutionAttributes; import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; @@ -48,7 +52,6 @@ import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.S3Configuration; -import software.amazon.awssdk.services.s3.model.Bucket; import software.amazon.awssdk.services.s3.model.BucketAlreadyOwnedByYouException; import software.amazon.awssdk.services.s3.model.NoSuchKeyException; import software.amazon.awssdk.services.s3.model.PutObjectRequest; @@ -57,7 +60,6 @@ import software.amazon.awssdk.services.s3.presigner.model.PresignedGetObjectRequest; import software.amazon.awssdk.services.s3control.model.BucketAlreadyExistsException; import software.amazon.awssdk.services.s3control.model.CreateMultiRegionAccessPointInput; -import software.amazon.awssdk.services.s3control.model.CreateMultiRegionAccessPointResponse; import software.amazon.awssdk.services.s3control.model.GetMultiRegionAccessPointResponse; import software.amazon.awssdk.services.s3control.model.ListMultiRegionAccessPointsResponse; import software.amazon.awssdk.services.s3control.model.MultiRegionAccessPointStatus; @@ -69,15 +71,14 @@ public class S3MrapIntegrationTest extends S3ControlIntegrationTestBase { private static final Logger log = Logger.loggerFor(S3MrapIntegrationTest.class); - private static final String CHUNKED_PAYLOAD_SIGNING = "STREAMING-AWS4-ECDSA-P256-SHA256-PAYLOAD"; + private static final String SIGV4A_CHUNKED_PAYLOAD_SIGNING = "STREAMING-AWS4-ECDSA-P256-SHA256-PAYLOAD"; + private static final String SIGV4_CHUNKED_PAYLOAD_SIGNING = "STREAMING-AWS4-HMAC-SHA256-PAYLOAD"; private static final String UNSIGNED_PAYLOAD = "UNSIGNED-PAYLOAD"; - private static final Region REGION = Region.US_WEST_2; private static String bucket; private static String mrapName; private static final String KEY = "aws-java-sdk-small-test-object"; private static final String CONTENT = "A short string for a small test object"; - private static final int RETRY_TIMES = 10; private static final int RETRY_DELAY_IN_SECONDS = 30; @@ -86,8 +87,9 @@ public class S3MrapIntegrationTest extends S3ControlIntegrationTestBase { private static String mrapAlias; private static StsClient stsClient; private static S3Client s3Client; + private static S3Client s3ClientWithPayloadSigning; - @BeforeClass + @BeforeAll public static void setupFixture() { captureInterceptor = new CaptureRequestInterceptor(); @@ -96,98 +98,92 @@ public static void setupFixture() { .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) .build(); - s3Client = S3Client.builder() - .region(REGION) - .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) - .build(); + s3Client = mrapEnabledS3Client(Collections.singletonList(captureInterceptor)); + s3ClientWithPayloadSigning = mrapEnabledS3Client(Arrays.asList(captureInterceptor, new PayloadSigningInterceptor())); stsClient = StsClient.builder() .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) .region(REGION) .build(); - String accountId = getAccountId(); + accountId = stsClient.getCallerIdentity().account(); bucket = "do-not-delete-s3mraptest-" + accountId; mrapName = "javaintegtest" + accountId; log.info(() -> "bucket " + bucket); - createBucketIfNotExist(bucket); - createMrapIfNotExist(mrapName); - mrapAlias = getMrapAliasAndVerify(); + createBucketIfNotExist(bucket); + createMrapIfNotExist(accountId, mrapName); + mrapAlias = getMrapAliasAndVerify(accountId, mrapName); } - private static void createBucketIfNotExist(String bucket) { - try { - s3Client.createBucket(b -> b.bucket(bucket)); - s3Client.waiter().waitUntilBucketExists(b -> b.bucket(bucket)); - } catch (BucketAlreadyOwnedByYouException | BucketAlreadyExistsException e) { - // ignore - } + @ParameterizedTest(name = "{index}:key = {1}, {0}") + @MethodSource("keys") + public void when_callingMrapWithDifferentPaths_unsignedPayload_requestIsAccepted(String name, String key, String expected) { + putGetDeleteObjectMrap(s3Client, UNSIGNED_PAYLOAD, key, expected); } - private static String getAccountId() { - return stsClient.getCallerIdentity().account(); + @ParameterizedTest(name = "{index}:key = {1}, {0}") + @MethodSource("keys") + public void when_callingMrapWithDifferentPaths_signedPayload_requestIsAccepted(String name, String key, String expected) { + putGetDeleteObjectMrap(s3ClientWithPayloadSigning, SIGV4A_CHUNKED_PAYLOAD_SIGNING, key, expected); } - public static String getMrapAliasAndVerify() { - GetMultiRegionAccessPointResponse mrap = s3control.getMultiRegionAccessPoint(r -> r.accountId(accountId).name(mrapName)); - assertThat(mrap.accessPoint()).isNotNull(); - assertThat(mrap.accessPoint().name()).isEqualTo(mrapName); - log.info(() -> "Alias: " + mrap.accessPoint().alias()); - return mrap.accessPoint().alias(); + @ParameterizedTest(name = "{index}:key = {1}, {0}") + @MethodSource("keys") + public void when_callingS3WithDifferentPaths_unsignedPayload_requestIsAccepted(String name, String key, String expected) { + putGetDeleteObjectStandard(s3Client, UNSIGNED_PAYLOAD, key, expected); } - @Test - public void object_lifecycle_workflow_through_mrap_unsigned_payload() { - signingWorkflow(Collections.emptyList(), UNSIGNED_PAYLOAD); + @ParameterizedTest(name = "{index}:key = {1}, {0}") + @MethodSource("keys") + public void when_callingS3WithDifferentPaths_signedPayload_requestIsAccepted(String name, String key, String expected) { + putGetDeleteObjectStandard(s3ClientWithPayloadSigning, SIGV4_CHUNKED_PAYLOAD_SIGNING, key, expected); } @Test - public void object_lifecycle_workflow_through_mrap_signed_payload() { - signingWorkflow(Collections.singletonList(new PayloadSigningInterceptor()), CHUNKED_PAYLOAD_SIGNING); - } - - public void signingWorkflow(List interceptors, String payloadSigningTag) { - S3Client s3 = s3Client(interceptors); + public void when_creatingPresignedMrapUrl_getRequestWorks() { S3Presigner presigner = s3Presigner(); + PresignedGetObjectRequest presignedGetObjectRequest = + presigner.presignGetObject(p -> p.getObjectRequest(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY)) + .signatureDuration(Duration.ofMinutes(10))); - listAndVerify(s3); - putAndVerify(s3, payloadSigningTag); - getAndVerify(s3); - presignGetAndVerify(presigner); - deleteAndVerify(s3); - } + deleteObjectIfExists(s3Client, constructMrapArn(accountId, mrapAlias), KEY); + s3Client.putObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY), RequestBody.fromString(CONTENT)); - private void listAndVerify(S3Client s3) { - List buckets = s3.listBuckets().buckets(); - assertThat(buckets.stream().map(Bucket::name)).contains(bucket); - verifySigv4SignedRequest(captureInterceptor.request()); + String object = applyPresignedUrl(presignedGetObjectRequest, null); + assertEquals(CONTENT, object); + verifySigv4aRequest(captureInterceptor.request(), UNSIGNED_PAYLOAD); } - private void putAndVerify(S3Client s3, String payloadSigningTag) { - deleteObjectIfExists(s3); - s3.putObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY), RequestBody.fromString(CONTENT)); + public void putGetDeleteObjectMrap(S3Client testClient, String payloadSigningTag, String key, String expected) { + deleteObjectIfExists(testClient, constructMrapArn(accountId, mrapAlias), key); + testClient.putObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(key), RequestBody.fromString(CONTENT)); verifySigv4aRequest(captureInterceptor.request(), payloadSigningTag); - } - private void getAndVerify(S3Client s3) { - String object = - s3.getObjectAsBytes(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY)).asString(StandardCharsets.UTF_8); + String object = testClient.getObjectAsBytes(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(key)).asString(StandardCharsets.UTF_8); assertEquals(CONTENT, object); verifySigv4aRequest(captureInterceptor.request(), UNSIGNED_PAYLOAD); - } - private void presignGetAndVerify(S3Presigner presigner) { - PresignedGetObjectRequest presignedGetObjectRequest = - presigner.presignGetObject(p -> p.getObjectRequest(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY)) - .signatureDuration(Duration.ofMinutes(10))); - String object = applyPresignedUrl(presignedGetObjectRequest, null); - assertEquals(CONTENT, object); + testClient.deleteObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(key)); verifySigv4aRequest(captureInterceptor.request(), UNSIGNED_PAYLOAD); + + assertThat(captureInterceptor.normalizePath).isNotNull().isEqualTo(false); + assertThat(captureInterceptor.request.encodedPath()).isEqualTo(expected); } - private void deleteAndVerify(S3Client s3) { - s3.deleteObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY)); - verifySigv4aRequest(captureInterceptor.request(), UNSIGNED_PAYLOAD); + public void putGetDeleteObjectStandard(S3Client testClient, String payloadSigningTag, String key, String expected) { + deleteObjectIfExists(testClient, bucket, key); + testClient.putObject(r -> r.bucket(bucket).key(key), RequestBody.fromString(CONTENT)); + verifySigv4Request(captureInterceptor.request(), payloadSigningTag); + + String object = testClient.getObjectAsBytes(r -> r.bucket(bucket).key(key)).asString(StandardCharsets.UTF_8); + assertEquals(CONTENT, object); + verifySigv4Request(captureInterceptor.request(), UNSIGNED_PAYLOAD); + + testClient.deleteObject(r -> r.bucket(bucket).key(key)); + verifySigv4Request(captureInterceptor.request(), UNSIGNED_PAYLOAD); + + assertThat(captureInterceptor.normalizePath).isNotNull().isEqualTo(false); + assertThat(captureInterceptor.request.encodedPath()).isEqualTo(expected); } private void verifySigv4aRequest(SdkHttpRequest signedRequest, String payloadSigningTag) { @@ -198,13 +194,35 @@ private void verifySigv4aRequest(SdkHttpRequest signedRequest, String payloadSig assertThat(signedRequest.headers().get("X-Amz-Region-Set").get(0)).isEqualTo("*"); } - private void verifySigv4SignedRequest(SdkHttpRequest signedRequest) { + private void verifySigv4Request(SdkHttpRequest signedRequest, String payloadSigningTag) { assertThat(signedRequest.headers().get("Authorization").get(0)).contains(SignerConstant.AWS4_SIGNING_ALGORITHM); - assertThat(signedRequest.headers().get("Host").get(0)).isEqualTo(String.format("s3.%s.amazonaws.com", REGION.id())); - assertThat(signedRequest.headers().get("x-amz-content-sha256").get(0)).isEqualTo(UNSIGNED_PAYLOAD); + assertThat(signedRequest.headers().get("Host").get(0)).isEqualTo(String.format("%s.s3.%s.amazonaws.com", + bucket, REGION.id())); + assertThat(signedRequest.headers().get("x-amz-content-sha256").get(0)).isEqualTo(payloadSigningTag); assertThat(signedRequest.headers().get("X-Amz-Date").get(0)).isNotEmpty(); } + private static Stream keys() { + return Stream.of( + Arguments.of("Slash -> unchanged", "/", "//"), + Arguments.of("Single segment with initial slash -> unchanged", "/foo", "//foo"), + Arguments.of("Single segment no slash -> slash prepended", "foo", "/foo"), + Arguments.of("Multiple segments -> unchanged", "/foo/bar", "//foo/bar"), + Arguments.of("Multiple segments with trailing slash -> unchanged", "/foo/bar/", "//foo/bar/"), + Arguments.of("Multiple segments, urlEncoded slash -> encodes percent", "/foo%2Fbar", "//foo%252Fbar"), + Arguments.of("Single segment, dot -> should remove dot", "/.", "//."), + Arguments.of("Multiple segments with dot -> should remove dot", "/foo/./bar", "//foo/./bar"), + Arguments.of("Multiple segments with ending dot -> should remove dot and trailing slash", "/foo/bar/.", "//foo/bar/."), + Arguments.of("Multiple segments with dots -> should remove dots and preceding segment", "/foo/bar/../baz", "//foo/bar/../baz"), + Arguments.of("First segment has colon -> unchanged, url encoded first", "foo:/bar", "/foo%3A/bar"), + Arguments.of("Multiple segments, urlEncoded slash -> encodes percent", "/foo%2F.%2Fbar", "//foo%252F.%252Fbar"), + Arguments.of("No url encode, Multiple segments with dot -> unchanged", "/foo/./bar", "//foo/./bar"), + Arguments.of("Multiple segments with dots -> unchanged", "/foo/bar/../baz", "//foo/bar/../baz"), + Arguments.of("double slash", "//H", "///H"), + Arguments.of("double slash in middle", "A//H", "/A//H") + ); + } + private String constructMrapArn(String account, String mrapAlias) { return String.format("arn:aws:s3::%s:accesspoint:%s", account, mrapAlias); } @@ -213,29 +231,6 @@ private String constructMrapHostname(String mrapAlias) { return String.format("%s.accesspoint.s3-global.amazonaws.com", mrapAlias); } - private void deleteObjectIfExists(S3Client s3) { - try { - s3.deleteObject(r -> r.bucket(constructMrapArn(accountId, mrapAlias)).key(KEY)); - } catch (NoSuchKeyException e) { - } - } - - private S3Client s3Client(List interceptors) { - List interceptorList = new ArrayList<>(interceptors); - interceptorList.add(captureInterceptor); - - return S3Client.builder() - .region(REGION) - .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) - .serviceConfiguration(S3Configuration.builder() - .useArnRegionEnabled(true) - .build()) - .overrideConfiguration(ClientOverrideConfiguration.builder() - .executionInterceptors(interceptorList) - .build()) - .build(); - } - private S3Presigner s3Presigner() { return S3Presigner.builder() .region(REGION) @@ -247,26 +242,25 @@ private S3Presigner s3Presigner() { .build(); } - private static void createMrapIfNotExist(String mrapName) { + private static void createMrapIfNotExist(String account, String mrapName) { software.amazon.awssdk.services.s3control.model.Region mrapRegion = software.amazon.awssdk.services.s3control.model.Region.builder().bucket(bucket).build(); - - if (s3control.listMultiRegionAccessPoints(r -> r.accountId(accountId)) - .accessPoints().stream().noneMatch(a -> a.name().equals(S3MrapIntegrationTest.mrapName))) { + boolean mrapNotExists = s3control.listMultiRegionAccessPoints(r -> r.accountId(account)) + .accessPoints().stream() + .noneMatch(a -> a.name().equals(S3MrapIntegrationTest.mrapName)); + if (mrapNotExists) { CreateMultiRegionAccessPointInput details = CreateMultiRegionAccessPointInput.builder() .name(mrapName) .regions(mrapRegion) .build(); log.info(() -> "Creating MRAP: " + mrapName); - CreateMultiRegionAccessPointResponse response = s3control.createMultiRegionAccessPoint(r -> r.accountId(accountId) - .details(details)); + s3control.createMultiRegionAccessPoint(r -> r.accountId(account).details(details)); waitForResourceCreation(mrapName); } } private static void waitForResourceCreation(String mrapName) throws IllegalStateException { - Waiter waiter = Waiter.builder(ListMultiRegionAccessPointsResponse.class) .addAcceptor(WaiterAcceptor.successOnResponseAcceptor(r -> @@ -279,6 +273,14 @@ private static void waitForResourceCreation(String mrapName) throws IllegalState waiter.run(() -> s3control.listMultiRegionAccessPoints(r -> r.accountId(accountId))); } + public static String getMrapAliasAndVerify(String account, String mrapName) { + GetMultiRegionAccessPointResponse mrap = s3control.getMultiRegionAccessPoint(r -> r.accountId(account).name(mrapName)); + assertThat(mrap.accessPoint()).isNotNull(); + assertThat(mrap.accessPoint().name()).isEqualTo(mrapName); + log.info(() -> "Alias: " + mrap.accessPoint().alias()); + return mrap.accessPoint().alias(); + } + private String applyPresignedUrl(PresignedRequest presignedRequest, String content) { try { HttpExecuteRequest.Builder builder = HttpExecuteRequest.builder().request(presignedRequest.httpRequest()); @@ -296,9 +298,38 @@ private String applyPresignedUrl(PresignedRequest presignedRequest, String conte return null; } + private static S3Client mrapEnabledS3Client(List executionInterceptors) { + return S3Client.builder() + .region(REGION) + .credentialsProvider(CREDENTIALS_PROVIDER_CHAIN) + .serviceConfiguration(S3Configuration.builder() + .useArnRegionEnabled(true) + .build()) + .overrideConfiguration(o -> o.executionInterceptors(executionInterceptors)) + .build(); + } + + private void deleteObjectIfExists(S3Client s31, String bucket1, String key) { + System.out.println(bucket1); + try { + s31.deleteObject(r -> r.bucket(bucket1).key(key)); + } catch (NoSuchKeyException e) { + } + } + + private static void createBucketIfNotExist(String bucket) { + try { + s3Client.createBucket(b -> b.bucket(bucket)); + s3Client.waiter().waitUntilBucketExists(b -> b.bucket(bucket)); + } catch (BucketAlreadyOwnedByYouException | BucketAlreadyExistsException e) { + // ignore + } + } + private static class CaptureRequestInterceptor implements ExecutionInterceptor { private SdkHttpRequest request; + private Boolean normalizePath; public SdkHttpRequest request() { return request; @@ -307,6 +338,7 @@ public SdkHttpRequest request() { @Override public void beforeTransmission(Context.BeforeTransmission context, ExecutionAttributes executionAttributes) { this.request = context.httpRequest(); + this.normalizePath = executionAttributes.getAttribute(AwsSignerExecutionAttribute.SIGNER_NORMALIZE_PATH); } }