From 72565761e3b01474462ddafbea719aaac31f1a18 Mon Sep 17 00:00:00 2001 From: Benjamin Simon Date: Wed, 22 Jan 2025 19:12:05 +0100 Subject: [PATCH 1/2] fix pre-signed URL with JS SDK, enable test_presigned_url_v4_x_amz_in_qs --- localstack-core/localstack/services/s3/presigned_url.py | 5 +++++ tests/aws/services/s3/test_s3.py | 7 +++++-- tests/aws/services/s3/test_s3.snapshot.json | 2 +- tests/aws/services/s3/test_s3.validation.json | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/localstack-core/localstack/services/s3/presigned_url.py b/localstack-core/localstack/services/s3/presigned_url.py index daa8d77cf897a..69cbe5ce5abc2 100644 --- a/localstack-core/localstack/services/s3/presigned_url.py +++ b/localstack-core/localstack/services/s3/presigned_url.py @@ -658,6 +658,11 @@ def _get_signed_headers_and_filtered_query_string( # specially in the old JS SDK v2 headers.add(qs_param_low, qs_value) else: + # The JS SDK is adding the `x-amz-checksum-crc32` header to query parameters, even though it cannot + # know in advance the actual checksum. Those are ignored by AWS, if they're not put in the + # SignedHeaders + if qs_param_low.startswith("x-amz-checksum-"): + continue query_args_to_headers[qs_param_low] = qs_value new_query_args[qs_parameter] = qs_value diff --git a/tests/aws/services/s3/test_s3.py b/tests/aws/services/s3/test_s3.py index 393cbd721ccc6..25c43b2115a44 100644 --- a/tests/aws/services/s3/test_s3.py +++ b/tests/aws/services/s3/test_s3.py @@ -7486,7 +7486,6 @@ def test_presigned_url_signature_authentication_multi_part( assert response.content == data @pytest.mark.skipif(condition=TEST_S3_IMAGE, reason="Lambda not enabled in S3 image") - @pytest.mark.skip(reason="flaky") @markers.aws.validated def test_presigned_url_v4_x_amz_in_qs( self, @@ -7544,6 +7543,8 @@ def test_presigned_url_v4_x_amz_in_qs( # assert that the Javascript SDK hoists it in the URL, unlike Boto assert StorageClass.STANDARD in presigned_url assert "bar-complicated-no-random" in presigned_url + # the JS SDK also adds a default checksum now even for pre-signed URLs + assert "x-amz-checksum-crc32=AAAAAA%3D%3D" in presigned_url # missing Content-MD5 response = requests.put(presigned_url, verify=False, data=b"123456") @@ -7560,7 +7561,9 @@ def test_presigned_url_v4_x_amz_in_qs( assert response.status_code == 200 # verify that we properly saved the data - head_object = aws_client.s3.head_object(Bucket=function_name, Key=object_key) + head_object = aws_client.s3.head_object( + Bucket=function_name, Key=object_key, ChecksumMode="ENABLED" + ) snapshot.match("head-object", head_object) @pytest.mark.skipif(condition=TEST_S3_IMAGE, reason="Lambda not enabled in S3 image") diff --git a/tests/aws/services/s3/test_s3.snapshot.json b/tests/aws/services/s3/test_s3.snapshot.json index 8c087519c52fc..55949ca680ab0 100644 --- a/tests/aws/services/s3/test_s3.snapshot.json +++ b/tests/aws/services/s3/test_s3.snapshot.json @@ -11773,7 +11773,7 @@ } }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_v4_x_amz_in_qs": { - "recorded-date": "21-01-2025, 18:25:21", + "recorded-date": "22-01-2025, 18:05:11", "recorded-content": { "head-object": { "AcceptRanges": "bytes", diff --git a/tests/aws/services/s3/test_s3.validation.json b/tests/aws/services/s3/test_s3.validation.json index bf4e18592b637..5675b297557e3 100644 --- a/tests/aws/services/s3/test_s3.validation.json +++ b/tests/aws/services/s3/test_s3.validation.json @@ -708,7 +708,7 @@ "last_validated_date": "2025-01-21T18:25:34+00:00" }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_v4_x_amz_in_qs": { - "last_validated_date": "2025-01-21T18:25:21+00:00" + "last_validated_date": "2025-01-22T18:05:11+00:00" }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_with_different_user_credentials": { "last_validated_date": "2025-01-21T18:23:55+00:00" From 0e5a9830f2167884ee4c9a5f5d6ce2c8bec37b74 Mon Sep 17 00:00:00 2001 From: Benjamin Simon Date: Wed, 22 Jan 2025 19:25:00 +0100 Subject: [PATCH 2/2] add checks for modifying checksum --- .../localstack/services/s3/presigned_url.py | 5 ++--- tests/aws/services/s3/test_s3.py | 10 ++++++++++ tests/aws/services/s3/test_s3.snapshot.json | 2 +- tests/aws/services/s3/test_s3.validation.json | 2 +- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/localstack-core/localstack/services/s3/presigned_url.py b/localstack-core/localstack/services/s3/presigned_url.py index 69cbe5ce5abc2..743b54f062293 100644 --- a/localstack-core/localstack/services/s3/presigned_url.py +++ b/localstack-core/localstack/services/s3/presigned_url.py @@ -661,9 +661,8 @@ def _get_signed_headers_and_filtered_query_string( # The JS SDK is adding the `x-amz-checksum-crc32` header to query parameters, even though it cannot # know in advance the actual checksum. Those are ignored by AWS, if they're not put in the # SignedHeaders - if qs_param_low.startswith("x-amz-checksum-"): - continue - query_args_to_headers[qs_param_low] = qs_value + if not qs_param_low.startswith("x-amz-checksum-"): + query_args_to_headers[qs_param_low] = qs_value new_query_args[qs_parameter] = qs_value diff --git a/tests/aws/services/s3/test_s3.py b/tests/aws/services/s3/test_s3.py index 25c43b2115a44..5c5cf03aee7a9 100644 --- a/tests/aws/services/s3/test_s3.py +++ b/tests/aws/services/s3/test_s3.py @@ -7560,6 +7560,16 @@ def test_presigned_url_v4_x_amz_in_qs( ) assert response.status_code == 200 + # assert that the checksum-crc-32 value is still validated and important for the signature + bad_presigned_url = presigned_url.replace("crc32=AAAAAA%3D%3D", "crc32=BBBBBB%3D%3D") + response = requests.put( + bad_presigned_url, + data=b"123456", + verify=False, + headers={"Content-MD5": "4QrcOUm6Wau+VuBX8g+IPg=="}, + ) + assert response.status_code == 403 + # verify that we properly saved the data head_object = aws_client.s3.head_object( Bucket=function_name, Key=object_key, ChecksumMode="ENABLED" diff --git a/tests/aws/services/s3/test_s3.snapshot.json b/tests/aws/services/s3/test_s3.snapshot.json index 55949ca680ab0..ab54bdc510a0c 100644 --- a/tests/aws/services/s3/test_s3.snapshot.json +++ b/tests/aws/services/s3/test_s3.snapshot.json @@ -11773,7 +11773,7 @@ } }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_v4_x_amz_in_qs": { - "recorded-date": "22-01-2025, 18:05:11", + "recorded-date": "22-01-2025, 18:21:12", "recorded-content": { "head-object": { "AcceptRanges": "bytes", diff --git a/tests/aws/services/s3/test_s3.validation.json b/tests/aws/services/s3/test_s3.validation.json index 5675b297557e3..16e9abc9e3e8e 100644 --- a/tests/aws/services/s3/test_s3.validation.json +++ b/tests/aws/services/s3/test_s3.validation.json @@ -708,7 +708,7 @@ "last_validated_date": "2025-01-21T18:25:34+00:00" }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_v4_x_amz_in_qs": { - "last_validated_date": "2025-01-22T18:05:11+00:00" + "last_validated_date": "2025-01-22T18:21:12+00:00" }, "tests/aws/services/s3/test_s3.py::TestS3PresignedUrl::test_presigned_url_with_different_user_credentials": { "last_validated_date": "2025-01-21T18:23:55+00:00"