<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Xuelei, one question and one nit, the rest looks pretty good.<br>
<ul>
<li>SupportedGroupsExtension</li>
<ul>
<li>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?</li>
</ul>
<li>NamedGroup</li>
<ul>
<li>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.</li>
</ul>
</ul>
<p>--Jamil<br>
</p>
<br>
<div class="moz-cite-prefix">On 4/14/2017 10:48 AM, Xuelei Fan
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:543886f9-8a5b-89a0-73c9-85aa8f1f8234@oracle.com">Hi,
<br>
<br>
Adam Petcher have some good comments in a private mail to me. The
webrev is updated accordingly:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~xuelei/8140436/webrev.01/">http://cr.openjdk.java.net/~xuelei/8140436/webrev.01/</a>
<br>
<br>
Major update: No customization is allowed for the FFDHE
parameters.
<br>
<br>
Thanks,
<br>
Xuelei
<br>
<br>
On 4/13/2017 11:15 AM, Xuelei Fan wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Please review the enhancement to support Finite Field
Diffie-Hellman
<br>
Ephemeral (FFDHE) Parameters negotiation in SSL/TLS/DTLS
implementation.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~xuelei/8140436/webrev.00/">http://cr.openjdk.java.net/~xuelei/8140436/webrev.00/</a>
<br>
<br>
Updates:
<br>
1. Support predefined FFDHE parameters.
<br>
JDK will support the following FFDHE parameters defined in RFC
7919, in
<br>
preference order:
<br>
name | key size (bits)
<br>
---------------+-------------------
<br>
ffdhe2048 | 2048
<br>
---------------+-------------------
<br>
ffdhe3072 | 3072
<br>
---------------+-------------------
<br>
ffdhe4096 | 4096
<br>
---------------+-------------------
<br>
ffdhe6144 | 6144
<br>
---------------+-------------------
<br>
ffdhe8192 | 8192
<br>
---------------+-------------------
<br>
<br>
<br>
2. Define a new System Property so as to disable the FFDHE
mechanism
<br>
For RFC 7919 compatible client, the predefined FFDHE parameter
names are
<br>
present in the "supported_groups" TLS extension. Some server
may not be
<br>
able to handle this extension or the FFDHE groups in the
extension. If
<br>
there is an interop issue, the new defined System Property,
<br>
"jsse.enableFFDHE", can be used to dismiss the predefined FFDHE
<br>
parameters for DHE cipher suites.
<br>
<br>
3. Redefine the jdk.tls.ephemeralDHKeySize System Property.
<br>
For connection request from RFC 7919 compatible clients, the
server
<br>
would prefer to use FFDHE mechanism at first unless
<br>
"jdk.tls.ephemeralDHKeySize" is defined to use "legacy" mode for
<br>
compatibility reason.
<br>
<br>
jdk.tls.ephemeralDHKeySize | FFDHE | Server
behavior
<br>
---------------------------+----------------------+----------------------
<br>
"legacy" | in any case | Use legacy
mode.
<br>
---------------------------+----------------------+----------------------
<br>
not "legacy" | Not present in the | Use DHE
parameters
<br>
| ClientHello message | compatible
to the
<br>
| | System
Property.
<br>
---------------------------+----------------------+----------------------
<br>
not "legacy" | Present in the | Use the
FFDHE
<br>
| ClientHello message | defined
parameters.
<br>
<br>
Note: Exportable cipher suites do not use the FFDHE mechanism.
<br>
<br>
4. Extend the "jdk.tls.namedGroups" System Property
<br>
Extend the "jdk.tls.namedGroups" System Property to support
customized
<br>
FFDHE groups. The following names are now supported by the
System
<br>
Property.
<br>
<br>
Names for named group | For EC or DH | Is it new in the
update?
<br>
------------------------+---------------+-------------------------
<br>
secp256r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
secp384r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
secp521r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect283k1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect283r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect409k1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect409r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect571k1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
sect571r1 | ECDHE | No
<br>
------------------------+---------------+-------------------------
<br>
ffdhe2048 | FFDHE | Yes
<br>
------------------------+---------------+-------------------------
<br>
ffdhe3072 | FFDHE | Yes
<br>
------------------------+---------------+-------------------------
<br>
ffdhe4096 | FFDHE | Yes
<br>
------------------------+---------------+-------------------------
<br>
ffdhe6144 | FFDHE | Yes
<br>
------------------------+---------------+-------------------------
<br>
ffdhe8192 | FFDHE | Yes
<br>
------------------------+---------------+-------------------------
<br>
<br>
Thanks,
<br>
Xuelei
<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>