RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

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