Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)

[Xuelei Fan]

On 1/19/2013 7:55 AM, Brad Wetmore wrote:
> I've pulled out things that need no further discussion.
> 
>> I will be doing a putback of the JCE providers, so I can do your SunJCE
>> signed provider putback for you.  Let's coordinate when you are ready.
>> I will probably be ready early next week.
> 
> I will likely be putting back on Monday, maybe Tuesday.  I'll try to
> coordinate with you over the weekend.
> 
OK.

I think we are on the same page except the following minor comment
style.  Thanks for the code review.

>>> EngineOutputRecord.java
>>> =======================
>>> 294/296:  Another great comment.  I might suggest reversing the
>>> comments so that the comment about AEAD is in the AEAD arm, and CBC is
>>> outside.
>>>
>> I'm not sure I catch your ideas. ;-) Would you please show me the code?
> 
> Just a simple reversal of the lines so that the code you're talking
> about is contained in the block that handles it:
> 
>     if (!writeCipher.isAEADMode()) {
>         // DON'T encrypt the nonce_explicit for AEAD mode
>         dstBB.position(dstPos + headerSize);
>     }   // The explicit IV in TLS 1.1 and later can be encrypted.
> 
> Hope that's clearer.
> 
Looks like my logic is correct.  If the cipher is not AEAD mode, the
explicit IV can be encrypted; (otherwise) if the cipher is AEAD mode,
don't encrypt the nonce_explicit.

   if (!writeCipher.isAEADMode()) {
       // The explicit IV in TLS 1.1 and later can be encrypted.
       dstBB.position(dstPos + headerSize);
   }   // Otherwise, DON'T encrypt the nonce_explicit for AEAD mode


>>> Handshaker.java
>>> ===============
>>> 2.  If a remote server selects a GCM ciphersuite and a version other
>>> than TLS 1.2, our client will send an alert_illegal_parameter.   I
>>> think this is done at ClientHandshaker:461 calling into
>>> Handshaker:isNegotiable(), but the comments in getActiveCipherSuites()
>>> only talk about the initial request, not the CipherSuite we actually
>>> get back from the server.  Can you please double check this?
>>>
>> Good catch.  We did not check the cipher suite and protocol version
>> together.  Need to address this in a new bug so that we can backport the
>> update.
> 
> Not following what you mean by "backport the update"?  GCM only exists
> in 8 at the moment.  Or do you mean check the other TLS 1.2 ciphersuites
> besides GCM?
> 
Yes, besides the GCM cipher suites, there are some new cipher suites are
defined since TLS 1.2. I think we'd better check the mismatching for all
such cipher suites, in both JDK 7 and 8.

Thanks for the code review.  It helps a lot to me.

Xuelei