<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi John, comments in-line<br>
<br>
<div class="moz-cite-prefix">On 5/21/2018 8:30 PM,
<a class="moz-txt-link-abbreviated" href="mailto:sha.jiang@oracle.com">sha.jiang@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:441c12c7-3620-f1bd-86af-6a24d47470b9@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>Hi Jamil,<br>
<br>
-- ChaCha20Cipher.java<br>
497 /**<br>
498 * Perform additional initialization actions based on
the key and operation<br>
499 * type.<br>
500 *<br>
501 * @param opmode the type of operation to do. This
value must be either<br>
502 * {@code Cipher.ENCRYPT_MODE} or {@code
Cipher.DECRYPT_MODE}<br>
503 * @param key a 256-bit key suitable for ChaCha20<br>
504 * @param newNonce the new nonce value for this
initialization.<br>
505 *<br>
506 * @throws UnsupportedOperationException if the {@code
opmode} parameter<br>
507 * is {@code Cipher.WRAP_MODE} or {@code
Cipher.UNWRAP_MODE}<br>
508 * (currently unsupported).<br>
509 * @throws InvalidKeyException if the {@code opmode}
parameter is not<br>
510 * {@code Cipher.ENCRYPT_MODE} or {@code
Cipher.DECRYPT_MODE}, or<br>
511 * if the key format is not {@code RAW}.<br>
512 */<br>
513 private void init(int opmode, Key key, byte[] newNonce)<br>
514 throws InvalidKeyException {<br>
515 if ((opmode == Cipher.WRAP_MODE) || (opmode ==
Cipher.UNWRAP_MODE)) {<br>
516 throw new UnsupportedOperationException(<br>
517 "WRAP_MODE and UNWRAP_MODE are not
currently supported");<br>
518 } else if ((opmode != Cipher.ENCRYPT_MODE)
&&<br>
519 (opmode != Cipher.DECRYPT_MODE)) {<br>
520 throw new InvalidKeyException("Unknown opmode:
" + opmode);<br>
521 }<br>
With my understanding, WRAP_MODE and UNWRAP_MODE can be allowed,
though they are not supported yet. But the doc on param opmode
stats the allowed modes are only ENCRYPT_MODE and DECRYPT_MODE.<br>
In fact, the doc on param opmode in method engineInit(int
opmode, Key key, SecureRandom random) implies that WRAP_MODE can
be accepted.<br>
</p>
</blockquote>
JN: Given that Cipher.init and CipherSpi.init both state that UOE
should be thrown if WRAP/UNWRAP_MODE is provided and the underlying
CipherSpi does not support those modes, you should NOT accept those
values for the opmode parameter. If and when we do support those
modes of operation, then I will loosen the check and allow it. But
you shouldn't allow a Cipher object to be initialized with a mode
that you know isn't supported. I would much rather see things
behave in a fail-fast manner where it is plausible, and this is one
of those cases.<br>
<br>
I will clean up the method header comments for engineInit to reflect
the change I made down in the private init common method.<br>
<blockquote type="cite"
cite="mid:441c12c7-3620-f1bd-86af-6a24d47470b9@oracle.com">
<p> 242 * @param opmode the type of operation to do. This
value may not be<br>
243 * {@code Cipher.DECRYPT_MODE} or {@code
Cipher.UNWRAP} mode because<br>
244 * it must generate random parameters like the
nonce.<br>
BTW, Cipher.UNWRAP or Cipher.UNWRAP_MODE?<br>
</p>
</blockquote>
It should be UNWRAP_MODE, good catch.<br>
<blockquote type="cite"
cite="mid:441c12c7-3620-f1bd-86af-6a24d47470b9@oracle.com">
<p> <br>
Best regards,<br>
John Jiang<br>
</p>
<div class="moz-cite-prefix">On 22/05/2018 02:36, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d88d9bec-d0ff-1ef3-b7bd-c3c41820f827@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>And the fun just keeps going! Round 7.</p>
<p>This revision removes wrap/unwrap support for ChaCha20 and
ChaCha20-Poly1305 until there is a standardized key wrapping
approach for these ciphers (similar to how AES has its own key
wrapping scheme in RFC 3394).</p>
<p>Initializing the cipher with WRAP/UNWRAP mode will throw
UnsupportedOperationException and likewise the wrap/unwrap
methods will throw IllegalStateException.</p>
<p><a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.07/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/</a></p>
<p>Thanks,</p>
<p>--Jamil<br>
</p>
<br>
<div class="moz-cite-prefix">On 05/16/2018 12:05 PM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3dda646c-2854-cdf2-cc1b-4d2c3133d787@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>Round 6.</p>
<p>This brings ChaCha20/ChaCha20-Poly1305 into conformance
with Cipher's specification when forms of init that take
AlgorithmParameters or AlgorithmParameterSpec are used.
Previously, a non-null AP or APS object was required. In
order to better conform to the specification, if a null AP
or APS is provided for these ciphers, a random nonce will be
generated and the counter will be set to 1, just as is
currently done with valid forms of init that don't take an
AP or APS object (e.g. Cipher.init(int, Key, SecureRandom)
). Per the spec in Cipher, this is only true for
ENCRYPT_MODE and will throw InvalidKeyException when done in
DECRYPT_MODE.</p>
<p>I also added a few test cases that exercise these code
paths in the ChaCha20Poly1305Parameters.java test case.<br>
</p>
<p><a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.06/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/</a></p>
<p>Thanks,</p>
<p>--Jamil<br>
</p>
<br>
<div class="moz-cite-prefix">On 05/04/2018 07:06 AM, Jamil
Nimeh wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d50c2610-42d9-64f0-8a91-796c2588fcf9@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>Round 5.</p>
<p>This adds Sean's comments. Sean, I was never able to
execute a case on init where a half-baked object would
fail. In most cases it would fail in checks in
javax.crypto.Cipher before it ever made it down to my
code. I'm pretty confident the init sequence is OK. I
did move the setting of a few data members toward the end
of the init sequence but setting the key and nonce is
necessary before creating the initial state, which is then
used for generating an authentication key for AEAD mode
and generating keystream.<br>
</p>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05</a><br>
<br>
Also the CSR has been finalized and can be found here:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8198925"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8198925</a><br>
<br>
--Jamil<br>
<br>
<div class="moz-cite-prefix">On 04/27/2018 02:21 PM, Jamil
Nimeh wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e1877cbe-c013-d7ab-a4c3-8c1bc6a4edd4@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<p>Round 4 of updates for ChaCha20 and ChaCha20-Poly1305,
minor stuff mostly:</p>
<ul>
<li>Added words in the description of
javax.crypto.Cipher recommending callers reinitialize
the Cipher to use different nonces after each complete
encryption or decryption (similar language to what
exists already for AES-GCM encryption).</li>
<li>Added an additional test case for ChaCha20NoReuse</li>
<li>Made accessor methods for ChaCha20ParameterSpec
final and cleaned up the code a bit based on comments
from the field.</li>
</ul>
<p><a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/</a></p>
<p>Thanks!</p>
<p>--Jamil<br>
</p>
<br>
<div class="moz-cite-prefix">On 04/13/2018 11:59 AM, Jamil
Nimeh wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4cdfa771-b222-2986-b1fa-2a67a89b9212@oracle.com">Round
3 of updates for ChaCha20 and ChaCha20-Poly1305: <br>
<br>
* Removed the key field in ChaCha20 and Poly1305
implementations and only retain the key bytes as an
object field (thanks Thomas for catching this) <br>
<br>
* Added additional protections against key/nonce reuse.
This is a behavioral change to ChaCha20 and
ChaCha20-Poly1305. Instances of these ciphers will no
longer allow you to do subsequent doUpdate/doFinal calls
after the first doFinal without re-initializing the
cipher with either a new key or nonce. Attempting to
reuse the cipher without a new initialization will throw
an IllegalStateException. This is similar to the
behavior of AES-GCM in encrypt mode, but for ChaCha20 it
needs to be done for both encrypt and decrypt. <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/</a>
<br>
<br>
Thanks, <br>
--Jamil <br>
<br>
On 04/10/2018 03:34 PM, Jamil Nimeh wrote: <br>
<blockquote type="cite">Hello everyone, <br>
<br>
This is a quick update to the previous webrev: <br>
<br>
* When using the form of engineInit that does only
takes op, key and random, the nonce will always be
random even if the random parameter is null. A
default instance of SecureRandom will be used to
create the nonce in this case, instead of all zeroes.
<br>
<br>
* Unused debug code was removed from the
ChaCha20Cipher.java file <br>
<br>
* ChaCha20Parameters.engineToString no longer obtains
the line separator from a System property directly.
It calls System.lineSeparator() similar to how other
AlgorithmParameter classes in com.sun.crypto.provider
do it. <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/</a>
<br>
<br>
Thanks, <br>
<br>
--Jamil <br>
<br>
<br>
On 03/26/2018 12:08 PM, Jamil Nimeh wrote: <br>
<blockquote type="cite">Hello all, <br>
<br>
This is a request for review for the ChaCha20 and
ChaCha20-Poly1305 cipher implementations. Links to
the webrev and the JEP which outlines the
characteristics and behavior of the ciphers are
listed below. <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejnimeh/reviews/8153028/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/</a>
<br>
<a class="moz-txt-link-freetext"
href="http://openjdk.java.net/jeps/329"
moz-do-not-send="true">http://openjdk.java.net/jeps/329</a>
<br>
<br>
Thanks, <br>
--Jamil <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>