Code Review Request, JDK-8140436, Support the FFDHE TLS extension

[Jamil Nimeh]

Hi Xuelei, comments in-line:

On 4/17/2017 8:18 PM, Xuelei Fan wrote:
> On 4/17/2017 4:47 PM, Jamil Nimeh wrote:
>> Hi Xuelei, one question and one nit, the rest looks pretty good.
>>
>>   * SupportedGroupsExtension
>>       o Line 267 and 278: From what I can see historically and with TLS
>>         1.3 draft 19 the underlying list for elliptic_curves and/or
>>         supported_groups extension data cannot be empty.  Should there
>>         be guards in the two constructors to prevent use of an empty
>>         array or an incoming empty vector?
> Good catch!
>
> Line 267 is a private constructor.  The caller does not use empty 
> groups (line 356-364).
>
> For the comment for line 278, I added a check in line 274 (see the 
> updated webrev.01):
> -274         if (((len & 1) != 0) || (k + 2 != len)) {
> +274         if (((len & 1) != 0) || (k == 0) || (k + 2 != len)) {
>
> An SSLProtocolException will be thrown if the incoming vector is empty.
Looks good to me.
>
>
>>   * NamedGroup
>>       o This is a nit, but I'm curious why you didn't just overload
>>         valueOf as valueOf(int) and valueOf(String) similar to how
>>         Integer does it and others like it.  I'm fine with it as-is if
>>         you prefer to keep it that way.
>>
> The valueOf(String) is a builtin method of Java enum, and cannot be 
> overrode.  So I use an alternative method, nameOf(String).
Okay, that works for me.
>
> Thanks,
> Xuelei
>
>> --Jamil
>>
>>
>> On 4/14/2017 10:48 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Adam Petcher have some good comments in a private mail to me. The
>>> webrev is updated accordingly:
>>>
>>>    http://cr.openjdk.java.net/~xuelei/8140436/webrev.01/
>>>
>>> Major update: No customization is allowed for the FFDHE parameters.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 4/13/2017 11:15 AM, Xuelei Fan wrote:
>>>> Hi,
>>>>
>>>> Please review the enhancement to support Finite Field Diffie-Hellman
>>>> Ephemeral (FFDHE) Parameters negotiation in SSL/TLS/DTLS 
>>>> implementation.
>>>>
>>>>    http://cr.openjdk.java.net/~xuelei/8140436/webrev.00/
>>>>
>>>> Updates:
>>>> 1. Support predefined FFDHE parameters.
>>>> JDK will support the following FFDHE parameters defined in RFC 
>>>> 7919, in
>>>> preference order:
>>>>       name        |    key size (bits)
>>>>    ---------------+-------------------
>>>>     ffdhe2048     |    2048
>>>>    ---------------+-------------------
>>>>     ffdhe3072     |    3072
>>>>    ---------------+-------------------
>>>>     ffdhe4096     |    4096
>>>>    ---------------+-------------------
>>>>     ffdhe6144     |    6144
>>>>    ---------------+-------------------
>>>>     ffdhe8192     |    8192
>>>>    ---------------+-------------------
>>>>
>>>>
>>>> 2. Define a new System Property so as to disable the FFDHE mechanism
>>>> For RFC 7919 compatible client, the predefined FFDHE parameter 
>>>> names are
>>>> present in the "supported_groups" TLS extension.  Some server may 
>>>> not be
>>>> able to handle this extension or the FFDHE groups in the 
>>>> extension.   If
>>>> there is an interop issue, the new defined System Property,
>>>> "jsse.enableFFDHE", can be used to dismiss the predefined FFDHE
>>>> parameters for DHE cipher suites.
>>>>
>>>> 3. Redefine the jdk.tls.ephemeralDHKeySize System Property.
>>>> For connection request from RFC 7919 compatible clients, the server
>>>> would prefer to use FFDHE mechanism at first unless
>>>> "jdk.tls.ephemeralDHKeySize" is defined to use "legacy" mode for
>>>> compatibility reason.
>>>>
>>>> jdk.tls.ephemeralDHKeySize | FFDHE                | Server behavior
>>>> ---------------------------+----------------------+---------------------- 
>>>>
>>>>
>>>>   "legacy"                 | in any case          | Use legacy mode.
>>>> ---------------------------+----------------------+---------------------- 
>>>>
>>>>
>>>>   not "legacy"             | Not present in the   | Use DHE parameters
>>>>                            | ClientHello message  | compatible to the
>>>>                            |                      | System Property.
>>>> ---------------------------+----------------------+---------------------- 
>>>>
>>>>
>>>>   not "legacy"             | Present in the       | Use the FFDHE
>>>>                            | ClientHello message  | defined 
>>>> parameters.
>>>>
>>>> Note: Exportable cipher suites do not use the FFDHE mechanism.
>>>>
>>>> 4. Extend the "jdk.tls.namedGroups" System Property
>>>> Extend the "jdk.tls.namedGroups" System Property to support customized
>>>> FFDHE groups.  The following names are now supported by the System
>>>> Property.
>>>>
>>>>     Names for named group  | For EC or DH  | Is it new in the update?
>>>> ------------------------+---------------+-------------------------
>>>>     secp256r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     secp384r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     secp521r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect283k1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect283r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect409k1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect409r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect571k1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     sect571r1              |  ECDHE        | No
>>>> ------------------------+---------------+-------------------------
>>>>     ffdhe2048              |  FFDHE        | Yes
>>>> ------------------------+---------------+-------------------------
>>>>     ffdhe3072              |  FFDHE        | Yes
>>>> ------------------------+---------------+-------------------------
>>>>     ffdhe4096              |  FFDHE        | Yes
>>>> ------------------------+---------------+-------------------------
>>>>     ffdhe6144              |  FFDHE        | Yes
>>>> ------------------------+---------------+-------------------------
>>>>     ffdhe8192              |  FFDHE        | Yes
>>>> ------------------------+---------------+-------------------------
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>