-
Notifications
You must be signed in to change notification settings - Fork 93
fix(aes): Allow AES encryption while streaming #463
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @roblabla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a limitation where AES encryption could not be utilized with streaming writers due to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly enables AES encryption for streaming use cases by defaulting to the AE-2 encryption specification and only updating the AES extra data (which requires seeking) when not in streaming mode. The changes are logical and also include some nice refactoring that improves code clarity, such as decoupling the CRC calculation logic. I have one minor suggestion regarding a comment to improve maintainability.
src/write.rs
Outdated
| // If using a data descriptor, it's because the underlying | ||
| // Write implementation doesn't support seek. In this case, we | ||
| // shouldn't update the AES Extra Data. | ||
| update_aes_extra_data(writer, file)?; | ||
| update_local_file_header(writer, file)?; | ||
| writer.seek(SeekFrom::Start(file_end))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is slightly confusing because it describes the case for the if branch (using_data_descriptor is true) but is located in the else branch. A comment explaining what happens in the else branch would be clearer.
| // If using a data descriptor, it's because the underlying | |
| // Write implementation doesn't support seek. In this case, we | |
| // shouldn't update the AES Extra Data. | |
| update_aes_extra_data(writer, file)?; | |
| update_local_file_header(writer, file)?; | |
| writer.seek(SeekFrom::Start(file_end))?; | |
| // Not using a data descriptor means the underlying writer supports seeking, | |
| // so we can go back and update the file headers. | |
| update_aes_extra_data(writer, file)?; | |
| update_local_file_header(writer, file)?; | |
| writer.seek(SeekFrom::Start(file_end))?; |
The update_aes_extra_data function requires the underlying writer to support seeking to succeed. In effect, this function is only used to switch the mode between AE2 and AE1 depending on file size. This is not strictly necessary so long as we use the strong mode by default, so we now only call this function for the non-streaming case.
feef040 to
d972f3b
Compare
The
update_aes_extra_datafunction requires the underlying writer to support seeking to succeed. In effect, this function is only used to switch the mode between AE2 and AE1 depending on file size, using AE2 only for small files (as the CRC in AE1 could leak too much information on those small files).Changing the type isn't strictly necessary, however - I assume it's only there to slightly increase compatibility?
So, in order to support streaming use-cases, I suggest we default to writing a header using AE2, and only run the
update_aes_extra_datafunction if not using a data descriptor.