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

Support AES encryption in FastZip.CreateZip #380

Merged
merged 1 commit into from
Aug 9, 2020

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Aug 25, 2019

Provide a means of enabling AES encryption when creating zips with FastZip (by settings the AESKeySize of the entries it creates prior to giving them to ZipOutputStream).

Notes:

  • I added a new mode enum in its own file rather than in FastZip.cs (easy to merge if needed).
  • I'm not sure if the encryption mode should have a 'None' option, given that the setting only applies if the password property has been set seperately?
  • The call to TestArchive in the unit tests is commented out because of ZipFile.TestArchive seems to always return false for AES encrypted zips #317

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@piksel piksel self-requested a review August 29, 2019 15:26
@piksel piksel added encryption zip Related to ZIP file format labels Aug 29, 2019
@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 9, 2020

A question about naming on the encryption mode enum:

If ZipFile gets support for adding AES encrypted entries to archives then it will need some means of specifying the encryption method to use, which might mean a new property on ZipFile.
If it had such a property, the type could be the same as for FastZip, or it could be different, not sure which is best (two enums vs. the possibility that they might end up different at some point).

Basically: Is 'FastZipEncryptionMethod' a reasonable name, or would something more shareable be better?

@piksel
Copy link
Member

piksel commented Feb 9, 2020

Hm, why would they ever need to be different? They are both working with the same spec., do you mean support-wise?

@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 9, 2020

Yes, support wise (though spec changes aside I don't think that would happen outside of someone implementing that PKWare strong encryption method in one and not the other, which seems unlikely in general)

@piksel
Copy link
Member

piksel commented Feb 9, 2020

I would probably go for using a common Enum.

@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 9, 2020

ok

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 12, 2020

Thinking about my second question ("should there be 'None' encryption option") - If you were using the same enum for ZipFile then you might possibly want to be able to open a file that contains encrypted entries and add new ones which are unencrypted, which might not work if you're just basing the application of encryption on whether a password has been provided? (hence a non-option being useful).

Not sure what the best api change for ZipFile is though

@Numpsy Numpsy force-pushed the rw/fz_aes branch 2 times, most recently from 5c9faf1 to 885fd76 Compare June 28, 2020 21:55
@Numpsy Numpsy changed the title [WIP] Support AES encryption in FastZip.CreateZip Support AES encryption in FastZip.CreateZip Jun 28, 2020
@Numpsy
Copy link
Contributor Author

Numpsy commented Jun 28, 2020

With regards to my suggestion in #454 about creating a custom entry factory that sets the AES key size properties:

This change has a possible conflict with that, in that if you set FastZip to use ZipCrypto encryption (the default), and then set the AES key size to something non-zero in the entry factory, then FastZip would overwrite that with it's own value, Not sure how much that counts as a problem? (The way ZipOutputStream works just off the key size, and that being an int that defaults to 0, means that you can't really just put the entry factory in charge as you'd end up overriding the FastZip option with ZipCrypto in the general case.)

You could maybe make ConfigureEntryEncryption just upgrade the encryption strength or some such, but then you might just say that FastZip is for simple use cases, and this is a non-issue.

@piksel
Copy link
Member

piksel commented Jul 4, 2020

Couldn't FastZip be modified to look at the AES key size, and not modify it if it's non-zero? Or am I missing the problem here?

@Numpsy
Copy link
Contributor Author

Numpsy commented Jul 13, 2020

yes (it's just something where it needs a descision on how it should work.)

@Numpsy
Copy link
Contributor Author

Numpsy commented Jul 14, 2020

updated

@piksel piksel self-assigned this Jul 15, 2020
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

I think this looks good. Does this fully enable AES encryption for FastZip, or is something else needed?

@piksel piksel added the ready PR ready for merging when appropriate label Jul 23, 2020
@Numpsy
Copy link
Contributor Author

Numpsy commented Jul 23, 2020

I think this looks good. Does this fully enable AES encryption for FastZip, or is something else needed?

I think that might be it as far as FastZip is concerned, as all the actual work is done in ZipOutputStream, though it might need #460 being completed first to avoid it creating bad entries for folders.

@piksel piksel merged commit 59e3080 into icsharpcode:master Aug 9, 2020
@Numpsy Numpsy deleted the rw/fz_aes branch August 9, 2020 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encryption ready PR ready for merging when appropriate zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants