Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request/Response Checksum Behavior Updates #3277

Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cdcb6b0
Add Flexible Checksum v2 Config Options (#3271)
jonathan343 Oct 8, 2024
c38b116
Add support for CRC64NVME when the CRT is available. (#3273)
jonathan343 Oct 8, 2024
02fac6d
Make CRC32 the default checksum and remove MD5
jonathan343 Oct 8, 2024
50d7142
Add tests for
jonathan343 Oct 8, 2024
0a2971f
Fix CRT only tests
jonathan343 Oct 8, 2024
bf16ed7
Actually fix CRT only tests
jonathan343 Oct 8, 2024
88eb03b
PR Feedback
jonathan343 Oct 22, 2024
878cb93
Ignore x-amz-checksum-algorithm when checking for user supplied check…
jonathan343 Oct 24, 2024
7563334
Revert "Ignore x-amz-checksum-algorithm when checking for user suppli…
jonathan343 Oct 28, 2024
860dc3b
enable response checksum validation by default
jonathan343 Oct 28, 2024
2e24f7f
PR feedback
jonathan343 Oct 29, 2024
e9eade8
Condense conditional; Update private function warning to docstring
jonathan343 Oct 29, 2024
ce46e4a
Avoid importing _CHECKSUM_CLS directly.
jonathan343 Nov 1, 2024
d44bb57
Add Flexible Checksum v2 Config Options (#3271)
jonathan343 Oct 8, 2024
dc710ec
Add support for CRC64NVME when the CRT is available. (#3273)
jonathan343 Oct 8, 2024
0569615
merge flexible-checksums-v2
jonathan343 Nov 6, 2024
626c08a
merge flexible-checksums-v2
jonathan343 Nov 13, 2024
8f26df1
Make "handle_request_validation_mode_member" private.
jonathan343 Nov 20, 2024
253928d
Set the header targeted by the "requestAlgorithmMember" to the defaul…
jonathan343 Nov 20, 2024
ff4f122
Minor test cleanup changes.
jonathan343 Nov 20, 2024
a2b19bd
Resolve the "requestAlgorithmMember" header in "apply_request_checksum".
jonathan343 Nov 21, 2024
503ead6
PR Feedback - Make requestAlgorithmMember context more specific.
jonathan343 Nov 22, 2024
509440a
fix testing issue; Assert actual checksum values in tests.
jonathan343 Nov 22, 2024
43ba7d0
Handle unsigned and "s3" signature_version requests.
jonathan343 Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions botocore/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,7 @@ def add_query_compatibility_header(model, params, **kwargs):
params['headers']['x-amzn-query-mode'] = 'true'


def handle_request_validation_mode_member(params, model, **kwargs):
def _handle_request_validation_mode_member(params, model, **kwargs):
client_config = kwargs.get("context", {}).get("client_config")
if client_config is None:
return
Expand Down Expand Up @@ -1334,7 +1334,7 @@ def handle_request_validation_mode_member(params, model, **kwargs):
('before-parse.s3.*', handle_expires_header),
('before-parse.s3.*', _handle_200_error, REGISTER_FIRST),
('before-parameter-build', generate_idempotent_uuid),
('before-parameter-build', handle_request_validation_mode_member),
('before-parameter-build', _handle_request_validation_mode_member),
('before-parameter-build.s3', validate_bucket_name),
('before-parameter-build.s3', remove_bucket_from_url_paths_from_model),
(
Expand Down
51 changes: 46 additions & 5 deletions botocore/httpchecksum.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
from binascii import crc32
from hashlib import sha1, sha256

from botocore import UNSIGNED
from botocore.compat import HAS_CRT, urlparse
from botocore.exceptions import (
AwsChunkedWrapperError,
FlexibleChecksumError,
MissingDependencyException,
)
from botocore.model import StructureShape
from botocore.response import StreamingBody
from botocore.utils import determine_content_length, has_checksum_header

Expand Down Expand Up @@ -262,6 +264,7 @@ def resolve_request_checksum_algorithm(
if has_checksum_header(request):
return

checksum_context = request["context"].get("checksum", {})
request_checksum_calculation = request["context"][
SamRemis marked this conversation as resolved.
Show resolved Hide resolved
"client_config"
].request_checksum_calculation
Expand Down Expand Up @@ -293,7 +296,18 @@ def resolve_request_checksum_algorithm(
elif request_checksum_required or (
algorithm_member and request_checksum_calculation == "when_supported"
):
# Don't use a default checksum for presigned requests.
if request["context"].get("is_presign_request"):
return
algorithm_name = DEFAULT_CHECKSUM_ALGORITHM.lower()
algorithm_member_header = _get_request_algorithm_member_header(
operation_model, request, algorithm_member
)
if algorithm_member_header is not None:
checksum_context["request_algorithm_header"] = {
"name": algorithm_member_header,
"value": DEFAULT_CHECKSUM_ALGORITHM,
}
else:
return

Expand All @@ -302,22 +316,41 @@ def resolve_request_checksum_algorithm(
operation_model.has_streaming_input
and urlparse(request["url"]).scheme == "https"
):
# Operations with streaming input must support trailers.
# 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':
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Operations with streaming input must support trailers.
# We only support unsigned trailer checksums currently. As this
# disables payload signing we'll only use trailers over TLS.
location_type = "trailer"

algorithm = {
"algorithm": algorithm_name,
"in": location_type,
"name": f"x-amz-checksum-{algorithm_name}",
}

checksum_context = request["context"].get("checksum", {})
checksum_context["request_algorithm"] = algorithm
request["context"]["checksum"] = checksum_context


def _get_request_algorithm_member_header(
operation_model, request, algorithm_member
):
"""Get the name of the header targeted by the "requestAlgorithmMember"."""
operation_input_shape = operation_model.input_shape
if not isinstance(operation_input_shape, StructureShape):
return

algorithm_member_shape = operation_input_shape.members.get(
algorithm_member
)

return (
algorithm_member_shape.serialization.get("name")
if algorithm_member_shape
else None
)


def apply_request_checksum(request):
checksum_context = request.get("context", {}).get("checksum", {})
algorithm = checksum_context.get("request_algorithm")
Expand All @@ -333,6 +366,11 @@ def apply_request_checksum(request):
raise FlexibleChecksumError(
error_msg="Unknown checksum variant: {}".format(algorithm["in"])
)
if "request_algorithm_header" in checksum_context:
request_algorithm_header = checksum_context["request_algorithm_header"]
request["headers"][request_algorithm_header["name"]] = (
request_algorithm_header["value"]
)


def _apply_request_header_checksum(request):
Expand Down Expand Up @@ -375,6 +413,9 @@ def _apply_request_trailer_checksum(request):
# services such as S3 may require the decoded content length
headers["X-Amz-Decoded-Content-Length"] = str(content_length)

if request["context"]["client_config"].signature_version == UNSIGNED:
headers["X-Amz-Content-SHA256"] = "STREAMING-UNSIGNED-PAYLOAD-TRAILER"

if isinstance(body, (bytes, bytearray)):
body = io.BytesIO(body)

Expand Down
3 changes: 2 additions & 1 deletion tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,7 @@ def get_checksum_cls(algorithm=DEFAULT_CHECKSUM_ALGORITHM.lower()):
This pass through is grabbing our internally supported list of checksums
to ensure we stay in sync, while not exposing them publicly.

Returns the default checksum algorithm class if none is specified.
Returns a checksum algorithm class. The default checksum class is used
if one isn't specified.
"""
return _CHECKSUM_CLS[algorithm]
37 changes: 31 additions & 6 deletions tests/functional/test_httpchecksum.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,14 @@
def _request_checksum_calculation_cases():
request_payload = "Hello world"
cases = [
(
None,
request_payload,
{
"x-amz-request-algorithm": "CRC32",
"x-amz-checksum-crc32": "i9aeUg==",
},
),
(
"CRC32",
request_payload,
Expand Down Expand Up @@ -224,9 +232,10 @@ def test_request_checksum_calculation(
)
with ClientHTTPStubber(client, strict=True) as http_stubber:
http_stubber.add_response(status=200, body=b"<response/>")
client.http_checksum_operation(
body=request_payload, checksumAlgorithm=checksum_algorithm
)
operation_kwargs = {"body": request_payload}
if checksum_algorithm:
operation_kwargs["checksumAlgorithm"] = checksum_algorithm
client.http_checksum_operation(**operation_kwargs)
actual_headers = http_stubber.requests[0].headers
for key, val in expected_headers.items():
assert key in actual_headers
Expand All @@ -236,10 +245,21 @@ def test_request_checksum_calculation(
def _streaming_request_checksum_calculation_cases():
request_payload = "Hello world"
cases = [
(
None,
request_payload,
{
"x-amz-request-algorithm": "CRC32",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-crc32",
},
{"x-amz-checksum-crc32": "i9aeUg=="},
),
(
"CRC32",
request_payload,
{
"x-amz-request-algorithm": "CRC32",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-crc32",
},
Expand All @@ -249,6 +269,7 @@ def _streaming_request_checksum_calculation_cases():
"SHA1",
request_payload,
{
"x-amz-request-algorithm": "SHA1",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-sha1",
},
Expand All @@ -258,6 +279,7 @@ def _streaming_request_checksum_calculation_cases():
"SHA256",
request_payload,
{
"x-amz-request-algorithm": "SHA256",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-sha256",
},
Expand All @@ -273,6 +295,7 @@ def _streaming_request_checksum_calculation_cases():
"CRC32C",
request_payload,
{
"x-amz-request-algorithm": "CRC32C",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-crc32c",
},
Expand All @@ -282,6 +305,7 @@ def _streaming_request_checksum_calculation_cases():
"CRC64NVME",
request_payload,
{
"x-amz-request-algorithm": "CRC64NVME",
"content-encoding": "aws-chunked",
"x-amz-trailer": "x-amz-checksum-crc64nvme",
},
Expand Down Expand Up @@ -316,9 +340,10 @@ def test_streaming_request_checksum_calculation(
)
with ClientHTTPStubber(client, strict=True) as http_stubber:
http_stubber.add_response(status=200, body=b"<response/>")
client.http_checksum_streaming_operation(
body=request_payload, checksumAlgorithm=checksum_algorithm
)
operation_kwargs = {"body": request_payload}
if checksum_algorithm:
operation_kwargs["checksumAlgorithm"] = checksum_algorithm
client.http_checksum_streaming_operation(**operation_kwargs)
request = http_stubber.requests[0]
actual_headers = request.headers
for key, val in expected_headers.items():
Expand Down
6 changes: 3 additions & 3 deletions tests/functional/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -1534,7 +1534,7 @@ def test_trailing_checksum_set(self):
b"STREAMING-UNSIGNED-PAYLOAD-TRAILER",
)
body = self.http_stubber.requests[0].body.read()
self.assertIn(b"x-amz-checksum-crc32", body)
self.assertIn(b"x-amz-checksum-crc32:eCQEmA==", body)

def test_trailing_checksum_set_empty_body(self):
with self.http_stubber:
Expand All @@ -1551,7 +1551,7 @@ def test_trailing_checksum_set_empty_body(self):
b"STREAMING-UNSIGNED-PAYLOAD-TRAILER",
)
body = self.http_stubber.requests[0].body.read()
self.assertIn(b"x-amz-checksum-crc32", body)
self.assertIn(b"x-amz-checksum-crc32:AAAAAA==", body)

def test_trailing_checksum_set_empty_file(self):
with self.http_stubber:
Expand All @@ -1570,7 +1570,7 @@ def test_trailing_checksum_set_empty_file(self):
sent_headers["x-amz-content-sha256"],
b"STREAMING-UNSIGNED-PAYLOAD-TRAILER",
)
self.assertIn(b"x-amz-checksum-crc32", body)
self.assertIn(b"x-amz-checksum-crc32:AAAAAA==", body)

def test_content_sha256_not_set_if_config_value_is_true(self):
# By default, put_object() provides a trailing checksum and includes the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
)

Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1987,7 +1987,7 @@ def create_checksum_context(

def test_request_validation_mode_member_default(checksum_operation_model):
params = {}
handlers.handle_request_validation_mode_member(
handlers._handle_request_validation_mode_member(
params, checksum_operation_model, context=create_checksum_context()
)
assert params["ChecksumMode"] == "ENABLED"
Expand All @@ -2000,17 +2000,17 @@ def test_request_validation_mode_member_when_required(
context = create_checksum_context(
response_checksum_validation="when_required"
)
handlers.handle_request_validation_mode_member(
handlers._handle_request_validation_mode_member(
params, checksum_operation_model, context=context
)
assert "ChecksumMode" not in params


def test_request_validation_mode_member_is_not_enabled(
def test_request_validation_mode_member_set_by_user(
checksum_operation_model,
):
params = {"ChecksumMode": "FAKE_VALUE"}
handlers.handle_request_validation_mode_member(
handlers._handle_request_validation_mode_member(
params, checksum_operation_model, context=create_checksum_context()
)
assert params["ChecksumMode"] == "FAKE_VALUE"
Loading
Loading