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

[Brad Wetmore]

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.

>> CipherBox.java
>> ==============
>> 312/564:  I don't remember why we decided to use the RuntimeException
>> AIOOBE here.  Ugh...anyway, can you also use exception chaining to help
>> debug any reported problems?
>>
> We don't have chaining constructor for ArrayIndexOutOfBoundsException class.

Well, that explains it.  :)  I wonder if there is already a RFE for this?

>> CipherSuite.java
>> ================
>> 510:  I don't think this statement is true anymore.  This is likley a
>> carryover from the 1.4 days when we used to have a crypto provider in
>> the JSSE jar file.  Not for this review, but maybe we should actually
>> check that implementations are available?  If we remove SunEC and
>> SunPKCS11 providers from the provider list, could we potentially
>> disable the EC suites?
>>
> Good catch.
>
> EC algorithm will be checked in KeyExchange, rather than BulkCipher.  If
> EC algorithm is not available, EC cipher suites will be disabled.
>
> I worry about the RC4 cipher. The AES/CBC/128 is the minimal
> requirements of a crypto provider, so I don't worry about AES/CBC/128.
> However, RC4 is not in the minimal requirements of a crypto provider, so
> we should check the availability of RC4 here.
>
> I will file a new for this issue.

Thanks.

>> 522:  Why are you also checking for CipherType.AEAD_CIPHER?
>>
> AEAD/GCM is not implemented in all providers, for example the SunPKCS11
> provider.  So we need to check the available of AEAD implementation.

Of course, now I see it.  The extra check for AES_256_GCM threw me.  If 
you're also checking for AEAD, you probably don't need the separate 
check for AES_256_GCM.  Maybe:

     if (this == B_AES_256 ||
         (this.cipherType == CipherType.AEAD_CIPHER)) {

is sufficient?

>> 970:  "before" -> "while"
>>
> It looks strange to me with two whiles:
>
>      // ...,  we decrease the
>      // priority of cipher suites in GCM mode for a while while GCM
>      // technologies become mature in the industry.
>
> I think "before" may be better word here.

Sorry, definitely not two whiles.  I think I probably meant to say "as" 
but obviously didn't.

Maybe:

     // Placeholder for cipher suites in GCM mode.
     //
     // For better compatibility and interoperability, we decrease the
     // priority of cipher suites in GCM mode for a while as GCM
     // technologies mature in the industry.  Eventually we'll move
     // the GCM suites here.

>> 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.

>> 306:  The original code was bad (double debug != null :) ), and I
>> realize the original code was lacking in parens "()", but can you
>> please add parens to indicate exactly what expression order you intend
>> here.  My head is spinning from parsing the various cases: I'm not sure
>> the logic here is correct.  I think we should output if debug is on
>> and either handshake/record is active or we're outputing a CCS.  That
>> is:
>>
>>      if ((debug ! null) &&
>>          ((Debug.isOn("record") || (Debug.isOn("handshake") ||
>>          (contentType() == ct_change_cipher_spec))))) {
>>
>> Is my thinking incorrect?
>>
> In the old code, there is no dump for "handshake" level log unless it is
> a change_cipher_spec message.  However, the log is dumped for "record"
> level log.   I think it was right because the log here is record
> information, but not handshake message.
>
> I think the update does not changes the behavior.

You are correct, the actual update does not change the observed 
behavior, but I had to write an app to prove it to myself.

However, this code is not following the failfast paradigm of Debug, 
though it does give the same answer because of the way the Debug.isOn() 
code was written.   debug!=null is supposed to be a lightweight check to 
see if any Debug options are on, and if so, then do the more heavyweight 
checks.

Abstracting a bit, test1 is the "debug!=null" test.  The resulting code is:

         if (test1() && test2() || test3() && test4()) {
             System.out...
         }

So if test1 fails, then we jump to the "test3 && test4" case, which then 
potentially has to go through the isOn processing twice.  Granted, 
because of the way isOn was written (if args == null) it's not adding a 
lot of overhead or throwing NullPointers, but it's not following the 
paradigm.

Looking a little more closely, I think the original logic was flawed.

     if ((debug && record) || handshake)
         if ((debug && record) || CCS)

both handshake/CCS are outside the debug check.  I think what was 
intended was:

     if (debug && (record || (handshake && CCS)))

My underlying original comment remains:  please add parens here to make 
it clear what you are intending.

(P.S.  If you want my test code, it's in my homedir under 
tmp/Template.java.)

>> Handshaker.java
>> ===============
>> This is a comment for ClientHandshaker/Handshaker.  I just want to
>> confirm some restrictions from Section 4 of RFC 5288:
>>
>> 1.  If a remote client offers <= TLS 1.1 and a GCM suite, we won't
>> possibly select a GCM suite.  I'm pretty sure this is ok, just wanted
>> to check.
>>
> Yes.
>
>> 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?

>> InputRecord.java
>> ================
>> 198:  Didn't follow this comment "before the exception occurs"?  Before
>> the MAC occurs?  Or in the case of an exception in signer.compute.
>>
> Need more comments here:
>
> count -= macLen;  // Set the count before any MAC checking
>                    // exception occurs, so that the following
>                    // process can read the actual decrypted
>                    // content (minus the MAC) in the fragment
>                    // if necessary.

Thanks.

Hope these comments are helpful to you.

Brad