Code Review Request: TLS 1.3 Implementation

Bradford Wetmore bradford.wetmore at oracle.com
Tue Jun 12 00:56:47 UTC 2018


>>> CipherSuite.java
>>> ----------------
>>> 74:  There is a mix of ciphersuite initialization styles, which I 
>>> find confusing.  
 >>>
>> These two suites may not be the best comparison, since you have a 
>> non-AEAD stream cipher up against AES-GCM.  For the latter, I think 
>> M_MULL in the MacAlg field is the right thing because GCM doesn't have 
>> a separate HMAC.  The authentication is performed through GHASH.  The 
>> only place for SHA-256 in this suite is for the PRF, IIUC.  As a 
>> second data point ChaCha20-Poly1305 suites (when we eventually do 
>> them) would also use M_NULL for the MacAlg since Poly1305 takes care 
>> of the authentication internally and SHA-256 is just used with PRF 
>> and/or HKDF in the TLS 1.3 case.
 >>
> I agreed with Jamil.

Makes perfect sense now, thanks.

>>> 262:  What is the point of the aliases argument in the constructor? 
>>> Was the idea to provide a mapping between suites we originally 
>>> created with the SSL_ prefix vs the more current TLS_ prefix we used 
>>> in the later TLS protocols?  There is only an empty string in every 
>>> constructor, so this code doesn't do anything.
>>>
> Added the aliases.

Great, thanks.  Once minor formatting comment which would help 
comparability/readability.  Take or leave it.

     SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA(
             0x0016, true, "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
             "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
             ProtocolVersion.PROTOCOLS_TO_12,
             K_DHE_RSA, B_3DES, M_SHA, H_SHA256),
->
     SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA(
             0x0016, true, "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
                           "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
             ProtocolVersion.PROTOCOLS_TO_12,
             K_DHE_RSA, B_3DES, M_SHA, H_SHA256),

>>> 803/814/824/867:  I'm concerned of the performance impact of 
>>> repeatedly iterating over 300+ ciphersuite ids/names using values(). 
>>> You should benchmark and see if it makes sense to switch to using a 
>>> HashMap (or even TreeMap) here.  For the limited number of Protocols 
>>> (< 10 for TLSv1.x), this approach is fine, but this has quite a bit 
>>> larger search space.
>>>
> Good point!  This enhancement now is tracked with
>      https://bugs.openjdk.java.net/browse/JDK-8204636

Good, thanks.

>>> 840:  Is this else/break needed?
>>>
> Yes.  It is mostly for performance by ignoring unsupported cipher suites 
> look up.

Ah, because they are coming out of .values() ordered, and the 
unsupported are at the end.  Can you change the comment:

the following cipher suites are not supported.
->
values() is ordered, remaining cipher suites are not supported.


>>> SSLServerSocketImpl.java
>>> ------------------------
>>> 160:  You should allow multiple changes between server to client, and 
>>> switch enabled protocols/ciphersuites accordingly.
>>>
> Yes, multiple changes are allowed.

I didn't see a change here.  If you go from server (default) to client, 
and then back again, shouldn't you reset to the original server values? 
And set sslConfig.useClientMode()

e.g.
     sslServerSocket.setUseClientMode(true);
     sslServerSocket.setUseClientMode(false);

The current code won't change back to server.

>>>
>>> SSLConfiguration.java
>>> ---------------------

40:  There is a new unused import here.

>>> SSLSocketImpl.java
>>> ------------------
>>> 28:  I'm not a fan of unnecessarily expanding the imports. Netbeans 
>>> has/had a default of 5, but apparently turned it off by default in 
>>> favor of single class imports.
>>>
> I'm a fan of expanding the imports now.  It is more clear about the 
> imported classes and the packet of a particular class.
> 
> For example, we can use:
> import java.net.*;
> import java.io.*;
> 
> Or
> import java.net.Socket;
> import java.io.StringReader;
> 
> If using the "*" option, we don't actually know if Socket is in packet 
> java.net or java.io.  So if IDE has made the update, I'm not included to 
> update them back any more.

Just seems like a lot of extra necessary cruft when you have lots of 
specifics (e.g. >5 like in SSLEngineImpl.java), but that's just my 
personal preference.  And IDEs can be set to do all kinds of formatting.  :)

>>> 76:  Are you set on the name conContext?  I'm still not 100% sure 
>>> what it stands for.  Alternate transportContext or connectionContext?
>>>
> The name is implying connection context, but it is an instance of 
> TransportContext.  Do you like, traContext, for transport context?  I'd 
> like to use a short name as it is used a lot that it is easy to exceed 
> the 80 character per line limit.

Nothing really jump at me.

Brad


>>> 92:  trustNameService could be private and all in caps.
>>>
> Updated.
> 
>>> 102-269:  Any chance of combining the commonalities of these 
>>> different constructors into 1, and then do the constructor-specific 
>>> things at the end?  It will help with future maintenance to not have 
>>> to make the same changes in multiple places.
>>>
> It is not clear to me now.  The issue is tracked with:
>      https://bugs.openjdk.java.net/browse/JDK-8204636
> 
>>> 484:  A couple comments here would be appreciated about what these 
>>> App* classes do.  The old comments in App[InputStream would be 
>>> sufficient, plus some of the other comments stripped out inside the 
>>> class:
>>>
>>>      Read data if needed ... notice that the connection guarantees
>>>      that handshake, alert, and change cipher spec data streams are
>>>      handled as they arrive, so we never see them here.
>>>
> Updated.
> 
>>> Nice job on this refactoring in this class, there was a heck of a lot 
>>> of cruft in this file after 20+ years.  I am anxious to see how the 
>>> rest of the code is laid out.
>>>
>>>
>>> TransportContext.java
>>> ---------------------
>>> 90:  Any chance of combining these 3 constructors into 1?  Almost 
>>> identical duplicate code here.
>>>
> This issue had been addressed in one of Tony's changesets:
>     http://hg.openjdk.java.net/jdk/sandbox/rev/b152d06ed6a9
> 
>>> 682-683:  These can be final.
>>>
> Updated.
> 
> Thanks,
> Xuelei
> 
>>>
>>> Thanks,
>>>
>>> Brad
>>>
>>>
>>>
>>> On 6/8/2018 10:21 AM, Xuelei Fan wrote:
>>>> Here is the 3rd full webrev:
>>>>     http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02
>>>>
>>>> and the delta update to the 1st webrev:
>>>>     http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.01
>>>>
>>>> Xuelei
>>>>
>>>> On 6/3/2018 9:43 PM, Xuelei Fan wrote:
>>>>> Hi,
>>>>>
>>>>> Here it the 2nd full webrev:
>>>>>    http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01
>>>>>
>>>>> and the delta update to the 1st webrev:
>>>>>    http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.00/
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 5/25/2018 4:45 PM, Xuelei Fan wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I'd like to invite you to review the TLS 1.3 implementation.  I 
>>>>>> appreciate it if I could have compatibility and specification 
>>>>>> feedback before May 31, 2018, and implementation feedback before 
>>>>>> June 7, 2018.
>>>>>>
>>>>>> Here is the webrev:
>>>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
>>>>>>
>>>>>> The formal TLS 1.3 specification is not finalized yet, although it 
>>>>>> had been approved to be a standard.  The implementation is based 
>>>>>> on the draft version 28:
>>>>>>      https://tools.ietf.org/html/draft-ietf-tls-tls13-28
>>>>>>
>>>>>> For the overall description of this enhancement, please refer to 
>>>>>> JEP 332:
>>>>>>      http://openjdk.java.net/jeps/332
>>>>>>
>>>>>> For the compatibility and specification update, please refer to 
>>>>>> CSR 8202625:
>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8202625
>>>>>>
>>>>>> Note that we are using the sandbox for the development right now. 
>>>>>> For more information, please refer to Bradford's previous email:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html 
>>>>>>
>>>>>>
>>>>>> Thanks & Regards,
>>>>>> Xuelei
>>


More information about the security-dev mailing list