RFR JDK-8247630: Use two key share entries

[Xuelei Fan]

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