Code Review Request: TLS 1.3 Implementation

Bradford Wetmore bradford.wetmore at oracle.com
Tue Jun 12 00:14:01 UTC 2018


>> 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
         };

>> 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 ) {

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



More information about the security-dev mailing list