Code Review Request: TLS 1.3 Implementation

Sat Jun 9 02:55:16 UTC 2018

CipherSuite.java
----------------
74:  There is a mix of ciphersuite initialization styles, which I find 
confusing.  In the *_OF_12, you pass the MAC of M_NULL, with H_* being 
used for PRF/HASH, which is also used for the MAC value for new suites, 
IIUC.  Would you consider specifying both in the enum constructor rather 
than M_NULL?

     KeyExchange keyExchange, SSLCipher cipher,
     MacAlg macAlg, HashAlg hashAlg)

to_12
             0x0018, false, "SSL_DH_anon_WITH_RC4_128_MD5", "",
             ProtocolVersion.PROTOCOLS_TO_T12,
             K_DH_ANON, B_RC4_128, M_MD5, H_SHA256),

of_12
             0x00A2, true, "TLS_DHE_DSS_WITH_AES_128_GCM_SHA256", "",
             ProtocolVersion.PROTOCOLS_OF_12,
             K_DHE_DSS, B_AES_128_GCM, M_NULL, H_SHA256),
->
             K_DHE_DSS, B_AES_128_GCM, H_SHA256, H_SHA256),

124-287:  All of a sudden the indention style changed, and then 
corrected itself back a little later at the "non-enabled" suites.  Can 
you remove these extra spaces here?

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.

377:  Maybe provide the RFC numbers (e.g. 4346, 5469, 5246, etc) for 
reference?

455:  This "other values" info is way out of date.  If anything, I would 
suggest we simply provide a link at the top of this file to the https 
(not http) page, and be done with it.

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.

840:  Is this else/break needed?

863:  This @throws is incorrect.  869 returns an emptyList instead of an 
Exception.

891:  "...with currently installed providers"?  This wording is strange, 
since it's all part of the SunJSSE provider.  Can you just say "name 
unsupported"?

910:  Can you please add a quick comment as to why/when you want a 
keyExchange == null.  i.e. TLSv1.3.  It took me a while to figure out 
when you would ever have a keyExchange==null.

995:  For readability, you can line this up using a 80 char line.

     K_RSA           ("RSA",            true,  false,  NAMED_GROUP_NONE),
     ...deleted...

SSLSessionImpl.java
-------------------
90:  I think we need to revisit this decision, but not now.

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

SSLConfiguration.java
---------------------
99:  This is a followup to the comment from last night.  tls13VN is 
pretty much set, let's get this temporary stuff out of here.

108:  Should we reverse the two tests here?  Checking for System 
property and then not checking for a SunTlsExtendedMasterSecret would be 
a faster option in the false case, vs driving the JCE lookup machinery 
and then checking whether we'll be using it or not.

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.

76:  Are you set on the name conContext?  I'm still not 100% sure what 
it stands for.  Alternate transportContext or connectionContext?

92:  trustNameService could be private and all in caps.

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.

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.

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.

682-683:  These can be final.

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