RFR JDK-8247630: Use two key share entries

[Jamil Nimeh]

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