RFR JDK-8247630: Use two key share entries

Xuelei Fan xuelei.fan at oracle.com
Mon Jul 27 23:42:31 UTC 2020


Looks good to me.  Thanks!

Xuelei

On 7/27/2020 4:03 PM, Jamil Nimeh wrote:
> All taken care of.
> 
> https://cr.openjdk.java.net/~jnimeh/reviews/8247630/webrev.03/
> 
> --Jamil
> 
> On 7/27/20 1:58 PM, Xuelei Fan wrote:
>> Hi Jamil,
>>
>> Thanks for taking the comment.  The webrev looks good to me. Just a 
>> few trivial comments about the coding style.  No more code review is 
>> required to me.
>>
>>
>> -    for (NamedGroup ng : namedGroups) {
>> +    while(ngTypes.size() < 2 && ngIter.hasNext()) {
>>
>> I prefer to use for-each as it has a clean coding style, and maybe 
>> better performance.  The loop could be broken when ngTypes.size() 
>> equals 2.
>>
>> +    byte[] shareData;
>> RFC 8446 defines the key share data as "key_exchange".  I may update 
>> the name to keyExchangeData.
>>
>> I may remove the debug enable code in the test cases, to speed up the 
>> automated testing.  It could be configured in the command line if 
>> needed to debug into test failure in the future.
>>      System.setProperty("javax.net.debug", "ssl:handshake");
>>
>> Thanks,
>> Xuelei
>>
>> On 7/27/2020 12:28 PM, Jamil Nimeh wrote:
>>> Hi Xuelei, I've updated the webrev based on your suggestion.  It 
>>> actually made the logic a lot simpler so that was a good suggestion 
>>> for sure.
>>>
>>> I also added a couple additional tests in ClientHelloKeyShares.java 
>>> to cover a few different namedGroup orderings.
>>>
>>> https://cr.openjdk.java.net/~jnimeh/reviews/8247630/webrev.02/
>>>
>>> --Jamil
>>>
>>> On 7/27/20 9:11 AM, Jamil Nimeh wrote:
>>>> Yes, I think I could restructure this to support that approach. 
>>>> You're right in that FFDHE gets the short end of the stick in the 
>>>> current scheme unless it's the only type in the namedGroups property 
>>>> and even then it takes the longest in terms of time. I'll 
>>>> restructure this and issue a new webrev.
>>>>
>>>> --Jamil
>>>>
>>>> On 7/27/2020 8:21 AM, Xuelei Fan wrote:
>>>>> I was just wondering, could we just simplify the implementation by 
>>>>> using two named groups for the top two-preferred categories, 
>>>>> without limited to XDH and ECDHE? For example, if FFDHE is on the 
>>>>> top 2, FFDHE will be used as well.  Normally, XDH and ECDHE will be 
>>>>> used, but the simplifying is a little bit more flexible if FFDHE is 
>>>>> preferred.  What do you think?
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 7/8/2020 3:57 PM, Jamil Nimeh wrote:
>>>>>> Hello all,
>>>>>>
>>>>>> This is a small enhancement to the TLS 1.3 client hello. With this 
>>>>>> change JSSE will make a best-effort attempt to select the 
>>>>>> most-preferred XDH and ECDHE named group.  In most cases this will 
>>>>>> be x25519 and secp256r1.  This was done to help reduce the number 
>>>>>> of times our clients get HelloRetryRequests due to servers wanting 
>>>>>> secp256r1 when we would just assert x25519.
>>>>>>
>>>>>> The asserted key shares can be affected by different settings for 
>>>>>> the jdk.tls.namedGroups System property.  If x25519 and/or 
>>>>>> secp256r1 are missing, the next-most-preferred named group of that 
>>>>>> class will be selected.  If no other named group in that class is 
>>>>>> in the supported_groups extension, then it will be skipped.  In 
>>>>>> the unlikely event that no XDH or ECDHE supported group is 
>>>>>> asserted, then the client will assert one key share, the most 
>>>>>> preferred one out of the supported groups.
>>>>>>
>>>>>> This also fixes one minor spec compliance issue where we weren't 
>>>>>> throwing an exception when a server returned an illegal 
>>>>>> HelloRetryRequest with a named group that was in the original 
>>>>>> key_shares extension from the client hello.
>>>>>>
>>>>>> Webrev: 
>>>>>> https://cr.openjdk.java.net/~jnimeh/reviews/8247630/webrev.01/
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247630
>>>>>>
>>>>>> --Jamil
>>>>>>



More information about the security-dev mailing list