Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

On 6/12/2018 5:43 PM, Bradford Wetmore wrote:
> I really don't have much to say about SSLEngineImpl.  I like the 
> refactoring you've done.  Just some nits.
> 
> Ignore my previous suggestion about conContext, after seeing it a bit 
> more often, it makes sense now.
> 
> SSLSocketImpl.java
> ------------------
> 72:  I noticed you removed the public here from SSLEngineImpl, and there 
> wasn't one from SSLServerSocketImpl, but public is still here in 
> SSLSocketImpl.  Maybe we can remove it?  Is there a reason to keep it 
> public?
> I tried, but SSLSocketImpl is used by 
sun.net.www.protocol.https.HttpsClient.java.

> 
> SSLEngineImpl.java
> ------------------
> 116:  You could put this on one line if you want.
> 
Yes, it is exactly 80 characters.

> 122-123:  Indent 8 spaces here.
> 
If using 8 spaces, the line exceeds 80 characters.  I normally have 4 
spaces then, and have a blank line followed.

> 757:  At line 735, you use
> 
> CipherSuite.validValuesOf(suites);
> 
> but here you use:
> 
> ProtocolVersion.namesOf(protocols);
> 
> Seems like it's basically the same kind of method, do you want to make 
> the names consistent.
> 
There is a little bit different.  The validValuesOf() method will check 
if the suite are supported or not.  About the name, we can make an 
improvement.  This enhancement is tracked with:
     https://bugs.openjdk.java.net/browse/JDK-8204636

Thanks,
Xuelei

> Brad
> 
> 
> On 6/8/2018 10:21 AM, Xuelei Fan wrote:
>> Here is the 3rd full webrev:
>>     http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02
>>
>> and the delta update to the 1st webrev:
>>     http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.01
>>
>> Xuelei
>>
>> 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