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

[Xuelei Fan]

webrev: http://cr.openjdk.java.net./~xuelei/7030966/webrev.02/

No significant changes in the new webrev.

<removed some comments that is clear to me>

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

> Minor nits:
> ===========
> What is the indentation style you are using?  I noticed a few places
> where it's not 4/8 spaces, and it's not lining up with anything on the
> previous line.  For example CipherSuite.java:522:544.
> 
I usually use 4 spaces for new lines. But if the next line is a
continuation of the previous line, I did not follow the 8 spaces strictly.

> Also, in several of your new methods, you are writing pseudo-javadoc
> style (@return/etc), but you're not starting the comment with "/**".
> This probably doesn't matter since we don't generate javadoc for
> internal classes.  Just mentioning since you took the time to format
> it and hoping it would render.
> 
I just want to use the tag in order to make the comments clear and easy
understood.

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

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

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

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

> 1054: From http://www.rfc-editor.org/rfc/rfc6460.txt
> 
>    A Suite B TLS client configured at a minimum level of security of 128
>    bits MUST offer the TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 or the
>    TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite in the
>    ClientHello message.  The TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
>    cipher suite is preferred; if offered, it MUST appear before the
>    TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite.
> 
> I would have thought what you have is the correct order, but it
> doesn't follow the RFC. Please add a note to remind us, because this
> order will not make sense in 6 months.  ;)
> 
You are right. The current default cipher suites preference is not
Suite-B/RFC6460 compliant.  We need additional work to configure the
cipher suites in order to comply with RFC 6460.  We may want to add a
new feature in the future.

> EngineInputRecord.java
> ======================
> 191:  Your comment is a little confusing here.  Does this mean it does
> this internally?
> 
Need to remove this line. Removed.

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

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

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

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

> 
> Record.java
> ===========
> 55:  Never noticed this before.  Is maxIVLength really 256, or were you
> just being overly cautious?  All of our current block ciphers (e.g.
> AES-CBC) max out at 16 bytes, and there's only 8 bytes for TLS GCM mode
> nonce-explicit?  For each OutputRecord, I think you're reserving an
> extra 245 bytes here that will never be used.  (5 + 256 - 16)  If
> you're nervous about future suites bumping the size, you could add a
> simple check during CipherSuite.java initialization.
> 
Good point.  Need to address this in a new bug so that we can backport
the update.

Thanks,
Xuelei