8000 implement Content-MD5 check for PutObject by bentsku · Pull Request #9064 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

implement Content-MD5 check for PutObject #9064

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

Merged
merged 2 commits into from
Sep 13, 2023
Merged

implement Content-MD5 check for PutObject #9064

merged 2 commits into from
Sep 13, 2023

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Sep 5, 2023

Motivation

As requested and reported in #8929, we were missing Content-MD5 validation for PutObject, which was implemented in the legacy provider.

ChecksumAlgorithm or Content-MD5 allows to verify that the payload sent is properly received in the right shape.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/checking-object-integrity.html#checking-object-integrity-md5

This will only concern PutObject for now, but many operation have this parameter (I think around 21, some like PutBucketAcl for example).

As we had a look with @alexrashed a long, long time ago, the specs implement some kind of way to know which operations have to have some kind of required checksum.

The end goal would be to have a handler to manage the checksum verification for all operations, except the ones with a streaming body like PutObject, as those require special handling (reading the body only once). This is in the backlog and will be tackled in the future.

Changes

Implemented different strategies depending on the provider.
For the default and streaming providers, I've done the same kind of logic as the checksum for the streaming provider, but using the ETag of the object, which represents the hexdigest of the md5 hash. The Content-MD5 header is the base64 encoded digest of the hash, so it needs a bit of encoding/decoding to be able to compare them.
I did not do the same logic as the regular checksum in the default provider, because if the request is encoded using aws-chunk, the request data is not equal to the object body, and should be decoded twice (once in LocalStack and once in moto), so I'm doing after moto already calculated the ETag.

Same logic for the v3 provider, we first set the object to read the body only once, and we then verify the b64 encoded etag vs the provided Content-MD5 value.

Removed the xfail for the related test and improved it a bit.

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Sep 5, 2023
@bentsku bentsku self-assigned this Sep 5, 2023
@github-actions
Copy link
github-actions bot commented Sep 5, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 26m 56s ⏱️
2 152 tests 1 677 ✔️ 475 💤 0
2 153 runs  1 677 ✔️ 476 💤 0

Results for commit 64a28a2.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review September 5, 2023 20:58
@bentsku bentsku added this to the 2.3 milestone Sep 13, 2023
Copy link
Member
@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! nicely done 👍

@thrau thrau merged commit 236b984 into master Sep 13, 2023
@thrau thrau deleted the fix-s3-content-md5 branch September 13, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: PUT request on pre-signed URL with Content-MD5 does not fail when the MD5 hash does not match the content
3 participants
0