Code Review Request: TLS 1.3 Implementation

[Bradford Wetmore]

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

Ah, Appendix E.1 of RFC 5246.

    If a TLS server receives a ClientHello containing a version number
    greater than the highest version supported by the server, it MUST
    reply according to the highest version supported by the server.

Thanks.

Brad

> 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