Code Review Request: TLS 1.3 Implementation

Tue Jun 12 00:35:57 UTC 2018

On 6/11/2018 5:14 PM, Bradford Wetmore wrote:
> 
>>> ProtocolVersion.java
>>> --------------------
> 
>>> 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?
>>>
>> I removed DTLS 1.3 as we don't actually support it right now.
> 
> Ok.  Too late now, but I suppose you could have left it as a comment.
> 
>      static final ProtocolVersion[] PROTOCOLS_TO_13 =
>          new ProtocolVersion[] {
>              TLS13, TLS12, TLS11, TLS10, SSL30,
>              /* DTLS13, */ DTLS12, DTLS10
>          };
> 
Hm, much better.  I'm moving too fast to use good comments sometimes.

>>> 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*?
>>>
>> Updated.
> 
> Thanks.
> 
>>> 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.
>>>
>> I added a comment for this method.  It is used to check if the 
>> requested version number is beyond the minimal supported numbers.  For 
>> example, if client request ox0304, because of version negotiation, 
>> 0x0303 (TLS 1.2) should be negotiated.  However, if the requested 
>> number is 0x0101, the receiver should reject the connection as the 
>> version number is too small to be supported.
> 
> I see what you're doing, but I'm not understanding why we're not 
> checking the other direction also?
> 
> e.g.  if (p < SSLv3 && not SSLv2) || (p > TLSv1.3 ) {
> 
Per TLS 1.2 version negotiation specification, the higher number is not 
limited.  For example, if client request for TLS 1.9, the server can 
response with TLS 1.2.   If the version if 1.9, this method does not 
reject it here.  However, it may be not necessary in practice.

Thanks,
Xuelei

> Thanks,
> 
> Brad
> 
> 
>>> 289:  You already checked this condition in 285-287 above.  And then 
>>> 304 is no longer needed.
>>>
>> Good catch!
>>
>> Thanks,
>> Xuelei
>>
>>> 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