Code Review Request: TLS 1.3 Implementation

[Bradford Wetmore]

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

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