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.

Sign up for GitHub

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

Jump to bottom

Add support for checksum algorithms in AWS #3873

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Add support for checksum algorithms in AWS #3873

merged 4 commits into from
Mar 21, 2023

Conversation

trueleo
Copy link
Contributor

@trueleo trueleo commented Mar 16, 2023

Which issue does this PR close?

Closes #3725

Rationale for this change

What changes are included in this PR?

  • Add with_checksum_algorithm option in AmazonS3Builder
  • Add Checksum enum
  • Request Signer can take optional payload sha256 checksum to reuse for content-sha256.

Are there any user-facing changes?

  • Added with_checksum_algorithm option in AmazonS3Builder.
  • Add Checksum enum

@github-actions github-actions bot added the object-store Object Store Interface label Mar 16, 2023
@tustvold
Copy link
Contributor

What is this waiting on?

@trueleo
Copy link
Contributor Author

trueleo commented Mar 21, 2023 •
edited
Loading

@tustvold Sorry, I couldn't find time to get back to it. I need to add optional dependencies for crc32 and crc32c. Any suggestions on which crate to use ?

tustvold
tustvold reviewed Mar 21, 2023
View reviewed changes
object_store/src/aws/client.rs Outdated
builder = builder.body(bytes)
payload_checksum = match self.config().checksum {
Some(checksum) => {
let digest = checksum.digest(&bytes);
Copy link
Contributor

@tustvold tustvold Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think passing this to the request signing is only correct if the checksum is a sha256 digest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah will fix it

@tustvold
Copy link
Contributor

tustvold commented Mar 21, 2023 •
edited
Loading

I need to add optional dependencies for crc32 and crc32c.

Perhaps we could reduce the scope initially and only support sha256 digests, as these already need to be computed for request signing anyway? To be honest I wonder if we could just always include them, given they have been computed 🤔

@trueleo
Copy link
Contributor Author

trueleo commented Mar 21, 2023

Perhaps we could reduce the scope initially and only support sha256 digests

@tustvold Fine by me. I'll change the Checksum enum to only have Sha1 and Sha256 as those are already provided by ring

@trueleo
Copy link
Contributor Author

trueleo commented Mar 21, 2023

To be honest I wonder if we could just always include them, given they have been computed

@tustvold ? Like setting x-amz-checksum-sha256 when unsigned payload option is not set ?
I don't know if that's a good idea. I too find existence of x-amz-content-sha256 and x-amz-checksum-sha256 weird but it's better to keep behaviour similar if we are going to support other algorithms.

I'll change the Checksum enum to only have Sha1 and Sha256

Removed SHA1 as well for now. It's deprecated. ref briansmith/ring#754

@trueleo trueleo marked this pull request as ready for review March 21, 2023 13:39
@tustvold tustvold merged commit 90cb00d into apache:master Mar 21, 2023
@tustvold
Copy link
Contributor

Thank you

@trueleo trueleo deleted the checksum_256 branch March 24, 2023 08:31
spebern pushed a commit to spebern/arrow-rs that referenced this pull request Mar 25, 2023
@trueleo @spebern
Add support for checksum algorithms in AWS (apache#3873)
6c62352 
* Add support for checksum algorithms in aws

* Remove other algorithms

* Only set when checksum algorithm is sha256

* Fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tustvold tustvold tustvold approved these changes

Assignees
No one assigned
Labels
object-store Object Store Interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Content-MD5 or checksum header for using an Object Locked S3
2 participants
@trueleo @tustvold