Checksum Integrity Validation #228
Conversation
Currently only FULL_OBJECT checksum type is supported.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 33.44% 34.56% +1.11%
==========================================
Files 129 130 +1
Lines 19570 20046 +476
Branches 19570 20046 +476
==========================================
+ Hits 6546 6929 +383
- Misses 12244 12316 +72
- Partials 780 801 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lfbrehm
left a comment
lfbrehm
left a comment
There was a problem hiding this comment.
It would be nice to also include all calculated hashes for GetObject and HeadObject calls, but I am aware of the ongoing discussion about these and multipart objects.
| } | ||
|
|
||
| impl ChecksumHandler { | ||
| pub fn new(required_checksum: Option<IntegrityChecksum>) -> Self { |
There was a problem hiding this comment.
Why pub if you only use from_headers outside?
There was a problem hiding this comment.
But it is used outside, for example at:
😐
| pub fn get_validation_checksum(&self) -> &Option<String> { | ||
| if let Some(checksum) = &self.required_checksum { | ||
| match checksum { | ||
| IntegrityChecksum::CRC32(val) | ||
| | IntegrityChecksum::CRC32C(val) | ||
| | IntegrityChecksum::CRC64NVME(val) | ||
| | IntegrityChecksum::SHA1(val) | ||
| | IntegrityChecksum::SHA256(val) => val, | ||
| } | ||
| } else { | ||
| &None | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this unused? If this is here for potential future use, can you at least disable the warnings?
There was a problem hiding this comment.
The functions were intended for future full implementation of checksum integrity validation in order to validate the checksum provided by the request against the calculated checksum. However, I have now included the functionality in the PR, at least for PutFile and UploadPart.
| pub fn get_calculated_checksum(&self) -> Option<String> { | ||
| if let Some(algo) = &self.required_checksum { | ||
| self.calculated_checksums.get(&algo.to_string()).cloned() | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Explanation above.
| pub fn upsert_checksum(&mut self, key: &str, checksum: &str) -> Option<String> { | ||
| self.calculated_checksums | ||
| .insert(key.to_string(), checksum.to_string()) | ||
| } | ||
|
|
||
| pub fn validate_checksum(&self) -> bool { | ||
| if let Some(checksum) = &self.required_checksum { | ||
| let calculated = self | ||
| .calculated_checksums | ||
| .get(&checksum.to_string()) | ||
| .cloned(); | ||
| return self.get_validation_checksum().eq(&calculated); | ||
| } | ||
|
|
||
| // If no checksum is required | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
I removed the unused upsert_checksum(...) function and refactored the validate_checksum(...) function which is now in use.
- Includes conversion from/to hex encoding before communication with server for backwards compatibility
das-Abroxas
had a problem deploying
to
coverage
GitHub Actions
Failure
das-Abroxas
changed the title
This integrates checksum integrity validation from the S3 specification into Aruna's S3-compatible interface for the implemented upload and get operations. The implementation validates data integrity using checksums provided by clients during PutObject, UploadPart, and CompleteMultipartUpload operations and provides all calculated checksums with the HeadObject and GetObject operations.
Changes
UploadPartstruct to optionally store checksums for multipart uploadsNotes
Currently only
FULL_OBJECTchecksum type is supported.Closes
Resolves #225