Code Review Request: TLS 1.3 Implementation

Sat Jun 9 06:00:34 UTC 2018

Hi Brad, just one follow-up to your CipherSuite comment, please see below.

--Jamil

On 06/08/2018 07:55 PM, Bradford Wetmore wrote:
>
> 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),
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.
>
> 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