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

Xuelei Fan xuelei.fan at oracle.com
Sat Jan 19 03:10:23 UTC 2013

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

More information about the security-dev mailing list