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

Dynamically set password after ZipFile creation #44

Closed
frankMilde opened this issue Aug 14, 2019 · 6 comments
Closed

Dynamically set password after ZipFile creation #44

frankMilde opened this issue Aug 14, 2019 · 6 comments
Assignees
Labels
new-feature New feature or request

Comments

@frankMilde
Copy link

Hey @srikanth-lingala,

thanks for maintaining such a top notch library. I wanted to upgrade from 1.3.2 to 2.1.2, but have a question concerning the API change.

Use case:
I have to process incoming zip files. They can be encrypyed or not, I do not know upfront. If they are encrypted i know how to generate the password from the file name.

With the old API, I could create a ZipFile and check its encryption status, and if true I could set the password:

Zipfile zip = new ZipFile(filename);
if( zip.isEncrypted() ){
  String password = generatePassword(filename);
  zip.setPassword(password);
}
// ... process zip

AFAIK, with the new API, I can only set the password when creating the ZipFile. I cannot set it afterwards. Also not via the ZipParameters, which was also possible with the old API.

Question:
How can i handle this use case with the new API?
Or in other term, what happens when I proactively/preventively supply a password, but the zip is not actually encrypted. Does extraction still work correctly?

String meaninglessPassword = generatePassword(nonEncryptedFilename);
Zipfile zip = new ZipFile(nonEncryptedFilename, meaninglessPassword);
// ... process zip correctly?

Thanks for looking into this.

@srikanth-lingala
Copy link
Owner

srikanth-lingala commented Aug 14, 2019

You have a very valid point here. The idea behind removing ZipFile.setPassword() is that a zip file is either encrypted or not*. And if it is encrypted, then the password can already be set in the constructor, which will save an additional line of code. But this obviously took away the flexibility to pass in password later on, as in your case.

what happens when I proactively/preventively supply a password, but the zip is not actually encrypted. Does extraction still work correctly?

Yes, it will still extract correctly. The password is only used if the zip file is encrypted. I know that this is not the most cleanest way to code, but unfortunately, at the moment in v2.x, this is the only possibility.

I will re-introduce ZipFile.setPassword() in the next release. But unfortunately, I do not have a date planned for next release. I would suggest you to use the fallback (to pass in a password as discussed above) in the meanwhile, and I will update this issue once this feature is ready to be used again.

Please let me know if you face any issues or passing in a password even if it is not encrypted does not work. I will prioritise the next release in this case and include a fix for it.

*Technically speaking a zip file can also be both encrypted and non-encrypted at the same time. In other words, it is possible that some files in the same zip file are encrypted and some are not. But this is a very rare scenario. But zip specification allows such use cases.

@frankMilde
Copy link
Author

Thanks for the quick response and looking into this matter. No hurry with the new release.

Since you are looking into tweaking the API, here is another observation when comparing the old and new API that might be useful:

Supplying the password and setting the parameters to use encryption are logically coupled, but are set on different objects now.

Before, setting the password and encryption parameters were coupled and centralized in the ZipParameters:

outStream = new ZipOutputStream(new FileOutputStream(file));

ZipParameters params = createParameterSet(getPassWord());
params.setFileNameInZip(fileName);
lOutputStream.putNextEntry(null, params);

outStream.write(data);

with

private ZipParameters createParameterSet(String password)
{
	ZipParameters params = new ZipParameters();

	// ... setting other params

	if (password != null && !password.isEmpty())
	{
		params.setEncryptFiles(true);
		params.setEncryptionMethod(Zip4jConstants.ENC_METHOD_AES);
		params.setAesKeyStrength(Zip4jConstants.AES_STRENGTH_256);
		params.setPassword(password);
	}
	return params;
}

Now this logical coupling is less obvious as you set the password on the stream

ZipOutputStream outStream = new ZipOutputStream(new FileOutputStream(file), getPassPhrase())

and must simultaneously know to handle another object, the ZipParameters, right via params.setEncryptionFiles(true).

So I think it makes sense to either allow to set the password on the params separately (and not on the ZipFile/Stream) or, if you do initialize ZipFile/Stream with a password, automatically set the params to encrption=true or even more, provide a default set of encryptionFiles/Method/Keystrength settings in this case.

But this is overall a minor issue and just my two cents. Thanks again.

@srikanth-lingala
Copy link
Owner

srikanth-lingala commented Aug 18, 2019

The reasoning I made when moving the password from the ZipParameters level to the ZipInputStream level is, as mentioned above, a password belongs to the zip level and therefore belongs to the ZipInputStream instead of passing in each time with zip parameters and hence avoiding duplication of data. But I see your point. This might also lead to unclean code. You probably have to do something like this now:

ZipOutputStream zipOutputStream;

if (isEncrypted) {
  zipOutputStream = new ZipOutputStream(outputstream, password);
} else {
  zipOutputStream = new ZipOutputStream(outputstream);
}

And pass in the isEncrypted flag to createParameterSet() method in your case. One tip is that you can always set the password to null if zip stream is not encrypted. This way you can avoid if...else block.

When I redesigned the API, removing the password from ZipParameters felt right to me, because in older versions the password was on the ZipFile object (with a setter) and also on the ZipParameters object which lead to confusion and also inconsistency. I could not just include it on the ZipParameters object because this is only used for zipping and not used when unzipping, and therefore the only choice left was to have it on ZipFile object. One more reason is that with ZipFile api it was one additional line of code which can be passed in with a constructor.

@srikanth-lingala srikanth-lingala self-assigned this Aug 21, 2019
@srikanth-lingala srikanth-lingala added the new-feature New feature or request label Aug 21, 2019
@LeeYoung624
Copy link
Contributor

LeeYoung624 commented Sep 6, 2019

Hey guys. I'm working on this issue. As zip4j is well designed, I don't want to mess it up. Fixing this issue may introduce a new API, so please let me make it clear. Please let me know if I have any misunderstandings.

Basicly I think re-introducing the ZipFile.setPassword() would solve this problem. I think the setPassword should be limited in ZipFile only, not in ZipOutputStream or ZipParameters, just as @srikanth-lingala noticed:

I could not just include it on the ZipParameters object because this is only used for zipping and not used when unzipping, and therefore the only choice left was to have it on ZipFile object.

But for this suggestion as @frankMilde mentioned:

if you do initialize ZipFile/Stream with a password, automatically set the params to encrption=true or even more, provide a default set of encryptionFiles/Method/Keystrength settings in this case

I'm not sure if this is a good idea. Currently the password in ZipFile is already used in unzipping files(e.g. extractFile, extractFile), but not in zipping(e.g. addFile, addFiles, addFolder, addStream). They will create a new ZipParameters if the ZipParameters is not specified. The new created ZipParameters is default to set encryptFiles=false, even if password is specified in ZipFile. I think @frankMilde would like us to provide a default set of settings when password is specified in ZipFile, for example:

encryptFiles = true
encryptionMethod = EncryptionMethod.ZIP_STANDARD

Therefore the user do not need to create a new ZipParameters and set these settings. Am I right @frankMilde ?

But I think this may mislead the users. They may be unaware of the encryption method that is default to be used then. Maybe the default encryption method could not meet their need, and they may not even know this setting if we provide the default settings. I'm not sure about this. What do you think @srikanth-lingala ?

srikanth-lingala added a commit that referenced this issue Sep 8, 2019
@srikanth-lingala
Copy link
Owner

@LeeYoung624 We only had to add a setPassword(char[]) to the ZipFile object. Actually, I had the fixes already on my machine. Thanks for looking into it, and I apologise that I did not update the thread about my fix. I just pushed the code changes for this issue.

@srikanth-lingala
Copy link
Owner

Feature added in v2.1.3

LeeYoung624 added a commit to LeeYoung624/zip4j that referenced this issue Sep 10, 2019
revert getCanonicalPath back to getPath in tests in commit srikanth-lingala#44. 19 occurrences in total need to be reverted
LeeYoung624 added a commit to LeeYoung624/zip4j that referenced this issue Sep 12, 2019
revert getCanonicalPath back to getPath in tests in commit srikanth-lingala#44. 19 occurrences in total need to be reverted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants