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

[Xuelei Fan]

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.


>   * 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).

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