Code Review Request: TLS 1.3 Implementation

Fri Jun 8 02:36:56 UTC 2018

I'm working my way through CipherSuite.java, slow going as I'm making 
sure all of the ciphersuites got updated to the new format.

SSLServerSocketFactoryImpl.java
-------------------------------
This comment is actually an overall comment in other files in your 
review.  Typically we don't make changes just for style (e.g.   foo () 
-> foo() , lines 56/73), but I'm guessing since so much has changed, 
you're just going to clean things up now?  So I'm wondering why you 
didn't do it in line /75/80/82/87/89/etc.  I'm all for making this 
change now since the world is changing anyway.

38/73:  Why not just make it one line?  It's under 80 chars.

41:  context can be final.

CipherSuite.java
----------------
28-33:  Just wondering why you chose to enumerate all of the java.util.*;

ProtocolVersion.java
--------------------
29-32:  Same question about enumerating all instances.

41:  And in SSLConfiguration: 99-101.  Why not just get rid of this now? 
  It's going to be 0x0304 in the final RFC.

49:  I believe this protocol number is your anticipated number, I don't 
see it mentioned anywhere in the current DTLS spec:

     https://tools.ietf.org/html/draft-ietf-tls-dtls13-26

Just alluded to in section 5.3, but I haven't see anything in my further 
IETF scanning about a specific DTLS version number having been assigned.

Later down, you mention DTLS 1.3 quite a bit (e.g. PROTOCOLS_TO_13), is 
this going to cause any adverse reactions since there is no DTLS 1.3 
implementation yet?

95:  Why does this comment say 1.3 if this is a _OF_30?

136-146:  These "T" names are a little non-intuitive. 
PROTOCOLS_TO_TLS11?  Should there be a similar for _TO_TLS13?  What 
about PROTCOLS_TO_DTLS*?

229:  I probably don't understand the callers of this code well enough. 
If DTLS, you return true if it's DLTS10/12/13 or later.  But false if 
less.  Why are you allowing DTLS 1.0 and all later (unknown) versions? 
Same question for the TLS arm.   Why fail earlier than SSLv3, and allow 
above TLSv1.3?  Also suggest you move the 243 into the else arm.

289:  You already checked this condition in 285-287 above.  And then 304 
is no longer needed.

Brad

On 6/7/2018 10:28 AM, Bradford Wetmore wrote:
> Also reviewed SSLSocketFactoryImpl.java
> 
> 
> BaseSSLSocketImpl.java
> ----------------------
> I don't see where requireCloseNotify is actually being used in the 
> remaining code.  If you are removing this functionality, you should 
> probably be calling that out in your CSR.
> 
> SSLSessionContextImpl.java
> --------------------------
> The only change here was to reorder the import of Vector (obsolete) and 
> update the copyright?  May I ask why?  If this was somehow a netbeans 
> suggestion, there were several other things that could be updated that 
> seem more important.  e.g.  37-39 could be made final.  207: unnecessary 
> unboxing.
> 
> Brad
> 
> 
> 
> 
> On 6/6/2018 5:45 PM, Bradford Wetmore wrote:
>> Today, I looked over SunJSSE.java, Utilities.java, and 
>> module-info.java.   Mostly nits below, some style things.
>>
>>
>> Utilities.java
>> --------------
>>
>> 39:  hexDigits can be private.
>>
>> 42:  Extra level of indent.  4 or 8.
>>
>> 39-41:  Nits: When variables are static finals, they are usually 
>> written uppercase.
>>
>> 115:  Extra " " and can we get SNI cap'd?
>>
>> HostName for  server name indication
>> ->
>> HostName for Server Name Indication
>>
>> 150:  Minor Nit:  You're concating ("+") and then using 
>> StringBuilder.append(), instead of simply append chaining which might 
>> be slightly more efficient.
>>
>> 167-217:  The code looks ok, but this might be reworked to take better 
>> advantage of StringBuilder.append() chaining, rather than the mix of 
>> concat and append here now.  e.g. what you did with toHexString(long). 
>> I see much of the same code repeated over and over here.  Can be done 
>> later.
>>
>> Brad
>>
>>
>>
>> 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