Closed iarenaza closed 6 months ago
ptaoussanis
commented
6 months ago @iarenaza This looks great Iñaki, very nice work 👍 My only reservation on a quick read is re: the documented "reset" workaround. Would be nice to find a cleaner solution if possible, but would need to take a closer look to comment on practical alternatives.
I'm planning the v1 beta for January, will definitely get your work included for that release 👍
iarenaza
commented
6 months ago @ptaoussanis I've dug a bit more into this. I've searched the OpenJDK issue tracker for issues on Chacha20 and Chacha20-Poly1305, and I've found that the behaviour I documented in my PR is actually considered a real issue[1][2][3]. And has been fixed in OpenJDK 21.0-ga, to have the same behaviour as AES-GCM (allow key and IV reuse in decryption mode only)[4].
As I tested the code locally with OpenJDK 17.0.9 (the one shipped by default in my Linux distro), I assumed the behaviour hadn't changed in any later versions (as it hadn't from JDK 11 to 17).
I don't know if enabling Chacha20-Poly1305 conditionally, depending on the JDK version used, is an option you would like to consider.
[1] https://bugs.openjdk.org/browse/JDK-8305822 [2] https://bugs.openjdk.org/browse/JDK-8305091 [3] https://bugs.openjdk.org/browse/JDK-8308476 [4] https://github.com/openjdk/jdk/blob/jdk-21-ga/src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java#L554C1-L556
ptaoussanis
commented
6 months ago Thanks for the additional info 👍
Haven't looked into any of this in detail yet, but some high-level comments in case you're interested in pursuing alternatives yourself so long:
If we choose to support ChaCha20, we'd want to ensure that it's capable of operating the same way as the other ciphers. This'd mean no additional constraints on users - any workarounds would need to be internal, and at a higher level than the unit tests.
I'd be on board for conditional support, and/or a conditional (internal) workaround to address the mentioned bug/s. For example:
In that case we'd want to identify the cheapest possible workaround and document the additional expense if it's non-trivial, etc.
iarenaza
commented
6 months ago I pushed a second commit with a workaround for Java < 21. In my laptop (using an AMD 7 PRO 3700U) the workaround adds about 2 micro-seconds according to criterium/bench. Which is only noticeable for small ciphertexts (less than 32-64 encrypted bytes; above that size, the additional cost seems to be amortized as the decryption phase time dominates).
ptaoussanis
commented
6 months ago This looks like a good improvement, nice work!
Planning to review and merge for the next release sometime this month (January) 👍
ptaoussanis
commented
6 months ago Reviewing fully today for manual merging 👍
ptaoussanis
commented
6 months ago Merged manually 👍
@iarenaza Please note that I made some upstream changes to help minimize the size of this PR. In particular, I updated the symmetric cipher tests to make it easier to add tests for extra ciphers.
You may want to compare your fork against the new master, and reset if you're happy with the changes.
Now I'm just waiting on a stable Encore v3.75.0, likely later this month. Once that's ready, I'll release Tempel v1.0.0-beta1.
Cheers :-)
ptaoussanis
commented
5 months ago @iarenaza Hi Iñaki Arenaza, just pinging to let you know that I've pushed beta1 to Clojars 👍
Support for ChaCha20-Poly1305 was added in JDK 11 in 2018 (https://openjdk.org/jeps/329). It is a very popular alternative to AES-GCM, due to its performance [1], and the fact that its used in a lot of security network protocols and tools[2].
[1] Faster than AEC-GCM without hardware AES acceleration, and similar or better performace in multi-core machines even with AES hardware acceleration; see https://en.wikipedia.org/wiki/ChaCha20-Poly1305#Performance
[2] IPSec, TLS 1.2, DTLS 1.2, TLS 1.3, Wireguard, OTRv4 and multipe other protocols (according to https://en.wikipedia.org/wiki/ChaCha20-Poly1305#Use)
[Re: #1]