Request/Response Checksum Behavior Updates #3277

jonathan343 · 2024-10-08T20:27:05Z

This PR makes the following updates to the botocore flexible checksum behavior:

Request Checksum Calculation

Remove MD5 checksums and make CRC32 the default checksum algorithm.
- CRC32 is a suitable default algorithm because it is available with and without the optional awscrt dependency.
When the request_checksum_calculation config is set to when_supported (default value), a checksum will be generated whenever one of the following conditions is true:
- An operation applies the httpchecksum trait and defines a requestAlgorithmMember.
- An operation applies the httpchecksum trait and defines requestChecksumRequired: true.
When the request_checksum_calculation config is set to when_required, a checksum will be generated only when:
- An operation applies the httpchecksum trait and defines requestChecksumRequired: true.

S3 Customizations:

Deprecates existing customizations for MD5 checksums.

Response Checksum Validation

A value set for requestValidationModeMember by the user will be used by the SDK.
If the requestValidationModeMember value is not set by the user, the following behavior is experienced:
- When the response_checksum_validation config is set to when_supported (default value), the requestValidationModeMember will be set to ENABLED.
- When the response_checksum_validation config is set to when_required, the requestValidationModeMember will not be set and checksum validation is skipped.

codecov-commenter · 2024-10-08T21:18:50Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (flexible-checksums-v2@fac0754). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
botocore/utils.py	66.66%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                   Coverage Diff                    @@
##             flexible-checksums-v2    #3277   +/-   ##
========================================================
  Coverage                         ?   93.14%           
========================================================
  Files                            ?       66           
  Lines                            ?    14424           
  Branches                         ?        0           
========================================================
  Hits                             ?    13435           
  Misses                           ?      989           
  Partials                         ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

botocore/httpchecksum.py

botocore/handlers.py

botocore/httpchecksum.py

botocore/utils.py

tests/functional/test_s3.py

nateprewitt · 2024-10-11T17:38:27Z

tests/functional/test_s3.py

-        # `payload_signing_enabled` config overrides this logic and forces the
-        # header.
+    def test_content_sha256_not_set_if_config_value_is_true(self):
+        # By default, put_object() provides a trailing checksum and includes the


Is this right? I wouldn't expect us to be using trailing checksums by default on put_object.

Because CRC32 is the new default, put_object will use trailing checksums by default. This is because put_object has streaming input.

You can see this behavior in the current version of the SDK by running:

import boto3 boto3.set_stream_logger('') s3 = boto3.client("s3") response = s3.put_object( Body=b"Example data.", Bucket="<bucket_name>", Key="example_key.txt", ChecksumAlgorithm="CRC32", )

tests/functional/test_s3.py

botocore/handlers.py

tests/functional/test_s3.py

tests/unit/test_httpchecksum.py

botocore/utils.py

botocore/httpchecksum.py

tests/functional/test_s3.py

Add ``request_checksum_calculation`` and ``response_checksum_validation`` config options.

* Add support for CRC64NVME when the CRT is available. * Update crc64nvme priority

…sums.

…ed checksums." This reverts commit 93a47eb.

Add ``request_checksum_calculation`` and ``response_checksum_validation`` config options.

* Add support for CRC64NVME when the CRT is available. * Update crc64nvme priority

…t checksum algorithm.

botocore/httpchecksum.py

nateprewitt · 2024-11-21T16:53:11Z

botocore/httpchecksum.py

+    if "extra_headers" in algorithm:
+        request["headers"].update(algorithm["extra_headers"])


How does this work if the user already supplied these headers?

The header we're worried about would only get added when the default checksum is used. This means that the user didn't supply the "requestAlgorithmMember" member. The user's input would always take precedence.
I'll update this logic to be less generic as suggested by your other comment.

nateprewitt · 2024-11-21T16:54:55Z

botocore/httpchecksum.py

+    if has_checksum_header(request):
+        return
+
+    extra_headers = {}


Do we have a rough idea of what expansion we're expecting here? Generally grab-bag style containers like this tend to grow in unexpected ways. It may be worth constraining to the exact header we're expecting since it's tightly coupled to the checksum we're creating.

I updated this to instead store the following in our request checksum context:

"request_algorithm_header": { "name": "foo", "value": "bar", },

We can later check if "request_algorithm_header" in request["context"]["checksum"] and apply the header if it exists. Let me know if that's better.

tests/functional/test_s3.py

aemous · 2024-11-27T15:46:35Z

botocore/httpchecksum.py

-        # We only support unsigned trailer checksums currently. As this
-        # disables payload signing we'll only use trailers over TLS.
-        location_type = "trailer"
+        if request["context"]["client_config"].signature_version != 's3':


Nit: would we prefer this if condition to be added to the parent if viaand?

jonathan343 requested review from nateprewitt and SamRemis October 9, 2024 19:56

nateprewitt requested changes Oct 11, 2024

View reviewed changes

jonathan343 force-pushed the flexible-checksums-v2 branch from 2d66b37 to d586cba Compare October 22, 2024 03:11

jonathan343 force-pushed the request-checksum-calculation branch from ccbc9c0 to 17ef6f7 Compare October 22, 2024 03:45

jonathan343 changed the title ~~Default to CRC32 for Request Checksum Calculation~~ Request/Response Checksum Updates Oct 28, 2024

jonathan343 changed the title ~~Request/Response Checksum Updates~~ Request/Response Behavior Checksum Updates Oct 28, 2024

jonathan343 changed the title ~~Request/Response Behavior Checksum Updates~~ Request/Response Checksum Behavior Updates Oct 28, 2024

nateprewitt reviewed Oct 28, 2024

View reviewed changes

botocore/handlers.py Outdated Show resolved Hide resolved

tests/functional/test_s3.py Outdated Show resolved Hide resolved

tests/unit/test_httpchecksum.py Show resolved Hide resolved

nateprewitt reviewed Oct 29, 2024

View reviewed changes

botocore/utils.py Outdated Show resolved Hide resolved

SamRemis reviewed Oct 29, 2024

View reviewed changes

botocore/httpchecksum.py Show resolved Hide resolved

botocore/httpchecksum.py Show resolved Hide resolved

nateprewitt reviewed Oct 29, 2024

View reviewed changes

tests/functional/test_s3.py Outdated Show resolved Hide resolved

tests/functional/test_s3.py Outdated Show resolved Hide resolved

jonathan343 and others added 16 commits November 1, 2024 10:40

Add Flexible Checksum v2 Config Options (boto#3271)

cdcb6b0

Add ``request_checksum_calculation`` and ``response_checksum_validation`` config options.

Add support for CRC64NVME when the CRT is available. (boto#3273)

c38b116

* Add support for CRC64NVME when the CRT is available. * Update crc64nvme priority

Make CRC32 the default checksum and remove MD5

02fac6d

Add tests for

50d7142

Fix CRT only tests

0a2971f

Actually fix CRT only tests

bf16ed7

PR Feedback

88eb03b

Ignore x-amz-checksum-algorithm when checking for user supplied check…

878cb93

…sums.

Revert "Ignore x-amz-checksum-algorithm when checking for user suppli…

7563334

…ed checksums." This reverts commit 93a47eb.

enable response checksum validation by default

860dc3b

PR feedback

2e24f7f

Condense conditional; Update private function warning to docstring

e9eade8

Avoid importing _CHECKSUM_CLS directly.

ce46e4a

Add Flexible Checksum v2 Config Options (boto#3271)

d44bb57

Add ``request_checksum_calculation`` and ``response_checksum_validation`` config options.

Add support for CRC64NVME when the CRT is available. (boto#3273)

dc710ec

* Add support for CRC64NVME when the CRT is available. * Update crc64nvme priority

merge flexible-checksums-v2

0569615

jonathan343 force-pushed the flexible-checksums-v2 branch from d586cba to fac0754 Compare November 13, 2024 16:27

merge flexible-checksums-v2

626c08a

jonathan343 force-pushed the request-checksum-calculation branch from 009ea3d to 626c08a Compare November 13, 2024 17:23

jonathan343 added 3 commits November 20, 2024 14:49

Make "handle_request_validation_mode_member" private.

8f26df1

Set the header targeted by the "requestAlgorithmMember" to the defaul…

253928d

…t checksum algorithm.

Minor test cleanup changes.

ff4f122

jonathan343 requested review from nateprewitt and SamRemis November 20, 2024 20:42

Resolve the "requestAlgorithmMember" header in "apply_request_checksum".

a2b19bd

nateprewitt reviewed Nov 21, 2024

View reviewed changes

jonathan343 added 3 commits November 22, 2024 10:07

PR Feedback - Make requestAlgorithmMember context more specific.

503ead6

fix testing issue; Assert actual checksum values in tests.

509440a

Handle unsigned and "s3" signature_version requests.

43ba7d0

jonathan343 requested a review from nateprewitt November 26, 2024 16:30

aemous reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request/Response Checksum Behavior Updates #3277

Request/Response Checksum Behavior Updates #3277

jonathan343 commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading

nateprewitt Oct 11, 2024

jonathan343 Oct 22, 2024

nateprewitt Nov 21, 2024

jonathan343 Nov 21, 2024

nateprewitt Nov 21, 2024

jonathan343 Nov 22, 2024

aemous Nov 27, 2024

		if "extra_headers" in algorithm:
		request["headers"].update(algorithm["extra_headers"])

Request/Response Checksum Behavior Updates #3277

Are you sure you want to change the base?

Request/Response Checksum Behavior Updates #3277

Conversation

jonathan343 commented Oct 8, 2024 • edited Loading

Request Checksum Calculation

S3 Customizations:

Response Checksum Validation

codecov-commenter commented Oct 8, 2024 • edited Loading

Codecov Report

nateprewitt Oct 11, 2024

Choose a reason for hiding this comment

jonathan343 Oct 22, 2024

Choose a reason for hiding this comment

nateprewitt Nov 21, 2024

Choose a reason for hiding this comment

jonathan343 Nov 21, 2024

Choose a reason for hiding this comment

nateprewitt Nov 21, 2024

Choose a reason for hiding this comment

jonathan343 Nov 22, 2024

Choose a reason for hiding this comment

aemous Nov 27, 2024

Choose a reason for hiding this comment

jonathan343 commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading