BouncyCastle implementation of "AES/CBC/PKCS5Padding" and "AES/GCM/NoPadding"

[william-tran]

Fixes #2917

I ran a very cursory JMH benchmark on encrypt/decrypt of a 1 MB byte array of random bytes and this implementation is 3x slower than JCE, and for a 1 kB byte array this is 4.7x slower.

[rwinch]

We should not use a null IV as this is not safe. We always need to create IV for AES/CBC.

[william-tran]

This matches the behaviour of the NULL_IV_GENERATOR in AesBytesEncryptor, which is shown in BouncyCastleAesBytesEncryptorEquivalencyTest.bcJceWithoutIvEquivalent(), and Encryptors.queryableText produces this sort of encryptor.

[rwinch]

Yes that is also incorrect (but we cannot change).

NOTE Also note there is a slight difference given AesBytesEncryptor is package scope and the static methods are a bit more descriptive. Technically though this is still broken and we should not repeat it.

[william-tran]

Ok so I'll remove that constructor and expect an IV generator in the code. Should I also make it package scope and final and add a method to Encryptors?

[rwinch]

Let's leave it public. I think the static methods in Encryptors need to go eventually. The problem with them is that what is "standard", "stronger", etc changes with time. However we cannot modify these methods and stay passive.

[william-tran]

I ran some more benchmarks comparing using a new instance of BlockCipher per invocation vs synchronized reuse of the BlockCipher. The test uses 4 threads sharing the same BouncyCastleAesBytesEncryptor, with 10 sec warmup, and 10 sec sampling.

synchronized reuse:

Benchmark                        Mode  Samples      Score      Error  Units
o.s.MyBenchmark.bcEncryptor     thrpt       10  21407.710 ±  855.481  ops/s
o.s.MyBenchmark.jceEncryptor    thrpt       10  92248.023 ± 2361.111  ops/s

new

Benchmark                        Mode  Samples      Score      Error  Units
o.s.MyBenchmark.bcEncryptor     thrpt       10  44368.225 ± 2895.580  ops/s
o.s.MyBenchmark.jceEncryptor    thrpt       10  92362.021 ± 5653.288  ops/s

That's 2x throughput than synchronized. I ran it off my dual core macbook air so the numbers make sense given 4 threads.

Then I tested the GC differences with a single thread running 1 million test cycles, against -Xms1g -Xmx1g -XX:+UseParallelGC. jvisualvm reports these numbers:
BC synchronized:

BC new:

JCE (synchronized):

All three show very similar minor GC counts (23, 25, 24), and actually JCE shows higher GC cpu time (50, 57, 81) ms.

Given all this, I think using a new instance of the cipher per invocation is the way to go. So I think that's it, should I squash the commits?

[rwinch]

A few additional comments:

• I think we should provide a constructor with a default IV generator that makes it easy for users
• We should likely provide a GCM mode since this is the preferred mechanism today

[william-tran]

Github was having issues yesterday and seems to not be kicking off Travis... lemme see if I can make a few force pushes to get that going again

[rwinch]

@william-tran Thanks for the updates!

Looking at the code it appears my suggestion to use a base class might have caused more code/complexity than intended. I'm wondering if the base class makes sense. Do you think we could try to:

• Remove the need for OutputStreams
• Remove passing in a boolean for ENCRYPT or DECRYPT. To me this is confusing even if the BC APIs define it this way. I'd much rather have some duplicate code.

I think the best way we could do this is to create the two classes totally independent of one another (without thinking about the other code). If there are similarities those can be extracted into a package private superclass. If not, then they live in their own classes.

[william-tran]

The whole need for OutputStreams is so that method can return two objects, a properly sized ByteArrayOutputStream and a properly initialized CipherOutputStream, then common code can process those. Both those objects depend on a BlockCipher that doesn't share a common subclass. (though have the same method names). The process method is a bit ugly due to the need to try/catch/finally/close streams so I didn't want to repeat that. While at first glance what you're asking for would seem clearer, it'll result in the 4x duplication (encrypt/decrypt, CBC/GCM) of a lot of complexity.

[rwinch]

@william-tran Thanks for the response. I'm much prefer the duplication of the code to needing to add classes like OutputStreams which adds its own variation of complexity.

[william-tran]

I prefer the previous revision, but what matters the most to me is that it works. We can entirely eliminate the common class by duplicating key generation and OutputStream processing, I only went half way.

[rwinch]

cipherOutputStream is closed in the finally block on line 69. Is it intentional to close cipherOutputStream twice?

[william-tran]

The second call could go in the catch instead of a finally. close() does a doFinal and releases resources, in case the write() throws exception we should try to release those resources

[william-tran]

moved the second close() to the catch block and added comments to clarify things.

[william-tran]

In case this isn't an IOException, we won't attempt to release resources and wrap in IllegalStateException. What about just catching any Exception?

[rwinch]

Thanks for the hard work @william-tran! This is now merged into master

[william-tran]

Woohoo! Thanks for guiding this to landing.