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

Fix CBC padding issues in chunk decryption and remove index from EncryptedChunk #392

Closed
wants to merge 2 commits into from

Conversation

dirvine
Copy link
Member

@dirvine dirvine commented Oct 30, 2024

Fix CBC padding issues in chunk decryption

Summary

This PR fixes issues with CBC padding errors during chunk decryption by improving how we handle chunk boundaries and ordering. The main changes focus on ensuring proper chunk handling during decryption operations, particularly during seek operations.

Changes

  • Remove parallel processing in decrypt to maintain proper chunk boundaries and ordering
  • Improve chunk ordering logic using DataMap indices
  • Add better error messages for missing chunks
  • Fix seek_and_join test to properly handle chunk boundaries
  • Fix overflow issues in seek_with_length_over_data_size test
  • Add more defensive checks for chunk boundaries

Testing

All tests are now passing, including:

  • seek_and_join
  • seek_with_length_over_data_size
  • seek_over_chunk_limit

Impact

This change improves reliability of chunk decryption without changing the underlying encryption format, maintaining backward compatibility with existing encrypted data.

* Remove parallel processing in decrypt to maintain chunk boundaries
* Improve chunk ordering and boundary handling
* Add better error messages for missing chunks
* Fix seek_and_join test to properly handle chunk boundaries
* Fix overflow issues in seek_with_length_over_data_size test
@dirvine dirvine requested a review from a team as a code owner October 30, 2024 15:13

let raw_chunks: Vec<(usize, Bytes)> = encrypted_chunks
.chunks(batch_size)
.par_bridge()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we want to get rid of parallelization here. Has that been any issue recently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it caused spurious failures in decrypt (encrypt was OK though)

Copy link
Member Author

Choose a reason for hiding this comment

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

I am on the get it working 100% correct first, then speed up later, if that makes sense

@@ -8,7 +8,7 @@ license = "GPL-3.0"
name = "self_encryption"
readme = "README.md"
repository = "https://github.com/maidsafe/self_encryption"
version = "0.30.0"
version = "0.30.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick question. If this is a breaking change, do we want to bump to 0.31.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it's not breaking this one.

@dirvine dirvine closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants