RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

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