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."
- SunJCE.java
81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2,
PBE, "
Should add the ChaCha20 algorithm to the provider information String above.
- 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.
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.
- ChaCha20Poly1305Parameters.java
52 * @since 11
We don't typically put @since lines in impl. classes.
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.
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.
--Sean
