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

Added mutable max buffer size for Encryptor and Decryptor #15

Merged
merged 2 commits into from
Mar 11, 2018

Conversation

amorillas
Copy link
Contributor

These are the changes that we talked about in Issue #14 . I haven't included any change in versions, as I thought you would want to do it when you are happy for releasing a new one.

I'm not sure if the buffer size must be a power of 2, but I've made this assumption.

Any comment or correction is welcome!

@justinludwig
Copy link
Owner

Thanks for the pull request! I have a few suggestions about the property name, and about the sizing of the buffers:

  1. For the property name (and corresponding getters/setters), how about calling it maxFileBufferSize instead of just maxBufferSize, to better distinguish it from other internal buffers?

  2. For the input file buffer size, it doesn't actually need to be a power of 2, so using the file's length exactly should be just fine.

  3. For the output file buffer size, it's hard to predict the size needed, because of compression. But if compression has been turned off, or if the file is already well compressed, then the encrypted file will be a little bit bigger than the decrypted file (because of the extra PGP headers and packeting). And if ASCII-armoring is used, then the encrypted file will be about an additional 1/3 bigger than the decrypted file (because of Base64 encoding).

    I think it's better to over-size the buffer somewhat, rather than under-size it, so for decryption, I think using the same size for the output file buffer as the input is the best way to go. But for encryption, I'd suggest using a formula like this for determining the best output buffer size:

output buffer size = input file size
output buffer size += (1 + number of encryption keys + number of signing keys) * 512
if (ascii armored) {
    output buffer size *= 4/3 * (64 + line separator length)/64
    output buffer size += 80
}

My reasoning is that each key with which the message is encrypted is going to add about 500 bytes, and each signature is going to add about another 500 bytes, and the rest of the various headers are going to add about another 500 bytes -- so I'd add 512 bytes to the buffer size for each of those things.

Then if ASCII-armoring is used, it's going to increase the total file size by 4/3 (because Base64 encoding will generate 4 characters for every 3 bytes) and then again by either 65/64 or 66/64 (as lines will be wrapped after 64 characters -- with a ratio of 65/64 on systems where the java line.separator property is just \n, and 66/64 where it is \r\n). Plus ASCII-armoring will add another 80 characters or so for the armor header/tail lines (-----BEGIN PGP MESSAGE-----) and version header (Version: BCPG v1.59) and checksum.

So those things (together with the input file size) should make for a decent rough guess for the encrypted output file size.

@amorillas
Copy link
Contributor Author

OK, no problem with the improvements you proposed although I'm not an expert in encryption algorithms, but they seem very reasonable. I've pushed them for you to review.

@justinludwig
Copy link
Owner

Thanks -- looks good! I will merge and release it this weekend.

@justinludwig justinludwig merged commit dd63974 into justinludwig:master Mar 11, 2018
justinludwig added a commit that referenced this pull request Mar 11, 2018
To use a better file io buffer size when encrypting/decrypting files,
and allow the max buffer size to be customized. The previous size was
always 4K; the new size is the smaller of 1M or the actual file size.
@justinludwig
Copy link
Owner

I merged and released as version 0.3.0. I did change the long to int conversions from using Math.toIntExact() to just an unsafe cast as (int), for java 7 compatibility (but as long as the code short-circuts to return the maxFileBufferSize whenever the actual file size is bigger, it shouldn't be an issue). I also added some unit tests to the EncryptorSpec for the Encryptor.estimateOutFileSize() method. Thanks again!

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.

2 participants