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