Comments in-line:
On 5/1/2018 10:20 AM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* 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).
* Added an additional test case for ChaCha20NoReuse
* Made accessor methods for ChaCha20ParameterSpec final and cleaned up
the code a bit based on comments from the field.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/
Some comments so far, will continue my review later ...
- Cipher.java
After the paragraph, I would add a reference for more info, ex:
"Please see <a href="http://www.ietf.org/rfc/rfc7539.txt";> RFC 7539
</a> for more information on the ChaCha20 and ChaCha20-Poly1305
algorithms."
JN: Sounds good. I'll do that.
- SunJCE.java
81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2,
PBE, "
Should add the ChaCha20 algorithm to the provider information String
above.
JN: Yep, missed that. Will fix.
- ChaCha20ParameterSpec.java
Thinking more about this -- if you don't think there is any good
reason that a provider would want to subclass this, I would just make
the whole class final.
JN: Given what ChaCha20 takes as variable inputs, I cannot see what
anyone could gain from subclassing it. I can set it to final and update
the CSR.
60 * @throws IllegalArgumentException if {@code nonce} is not
12 bytes
61 * in length.
Remove period since this is a fragment that is not followed by a
sentence.
87 * @return the counter value.
Same as above.
JN: Will fix.
- ChaCha20Poly1305Parameters.java
52 * @since 11
We don't typically put @since lines in impl. classes.
JN: Consider it gone.
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding
error");
102 if (val.tag == DerValue.tag_OctetString) {
103 // Get the nonce value
104 nonce = val.getOctetString();
I would probably just call val.getOctetString() directly and not
proactively check the tag, since it will throw IOE if it is the wrong
tag anyway. Then you don't need these lines:
109 } else {
110 throw new IOException(
111 "ChaCha20-Poly1305 Parameter ASN.1 encoding
error");
112 }
There are other reasons an IOException can be thrown, so it doesn't
seem necessary to have a special-case for one type of error.
JN: Sure, that seems reasonable. Will fix.
134 throw new IllegalArgumentException(
135 "Unsupported parameter format: " +
decodingMethod);
Although this seems like a reasonable exception to throw,
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I
think you should throw an IOE instead.
202 throw new IllegalArgumentException(
203 "Unsupported encoding format: " +
encodingMethod);
Similar comment, AlgorithmParametersSpi.engineGetEncoded is not
spec'ed to throw this, so I think you should throw an IOE instead.
JN: Fair enough. I had figured IAE would be OK since it's an unchecked
exception and seemed more appropriate to the situation. But IOE is fine
too. Will fix.
--Jamil
