Code Review Request: TLS 1.3 Implementation

Xuelei Fan xuelei.fan at oracle.com
Mon Jun 11 04:52:35 UTC 2018 

Update: http://hg.openjdk.java.net/jdk/sandbox/rev/da9979451b7a

On 6/8/2018 11:00 PM, Jamil Nimeh wrote:
> 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.
I agreed with Jamil.

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

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

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

>> 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.
>>
Updated.  Move the https line to the top of the file.

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

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

>> 863:  This @throws is incorrect.  869 returns an emptyList instead of 
>> an Exception.
>>
Good catch!  Updated.

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

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

>> 995:  For readability, you can line this up using a 80 char line.
>>
>>     K_RSA           ("RSA",            true,  false, NAMED_GROUP_NONE),
>>     ...deleted...
>>
Yes, these lines can have a better layout.

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

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

>>
>> 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.
>>
It's for interop testing right now.  The code will be removed before we 
shop the produce.  The issue is tracked with:
     https://bugs.openjdk.java.net/browse/JDK-8204636

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

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


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

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