Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

On 6/11/2018 5:17 PM, Bradford Wetmore wrote:
> 
> 
> On 6/8/2018 7:45 PM, Xuelei Fan wrote:
>>  > BaseSSLSocketImpl.java
>>  > ----------------------
>> Update: http://hg.openjdk.java.net/jdk/sandbox/rev/bad9f4c7eeec
>>
>> Update to handle EOFException and requireCloseNotify property.
> 
> Couple new typo nits:
> 
> 588/843/:
> // if no exceptio thrown
> ->
> // if no exception thrown
> 
Ooops! Updated in my local workspace.  I will integrate it with my next 
changeset.

Thanks,
Xuelei

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