I have no finished the full code review. So far, I have a few question
about the struct of the code.
1. XECParameters
I can see the reason to dynamic parameters for something other than
X25519/X448. But for JSSE, currently, only named curves are used. It
would be nice to use the name for the static parameters.
2. "TlsPremasterSecret" only XDH key agreement
It would be nice the JCE implementation can support key agreement other
than TLS protocols and providers other than SunJSSE provider. It would
be nice if the JCE implementation does not bind to the SunJSSE provider
private algorithm name.
We used to use SunJSSE private name in the JCE crypto implementation.
But there are some known problems with this dependence.
Is there a problem to support generic key agreement?
3. NamedGroupFunctions
It might be more straightforward to define these functions in
NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec()
and namedGroup.getParameterSpec(), I'd prefer the latter.
4. SSLKeyAgreementCredentials
I did not see too much benefit of this new interface. It is not always
true that a key agreement could have a public key. Although we can
simplify the key agreement for public key based, but it also add
additional layers.
I know where this improvement comes from. Maybe, you can consolidate
the named group functions, and encode/decode the credentials there.
Xuelei
On 8/30/2018 8:58 AM, Adam Petcher wrote:
Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8171279
Please review the following change to add support for X25519 and X448
(XDH) to TLS 1.3. This change includes some refactoring to remove code
that was duplicated for DH and ECDH, and to avoid adding more for XDH.
In addition to running the included regression test, I tested by
connecting to an openssl server and confirmed that the connection was
established using TLS 1.3 and X25519/X448.
Here are some detailed notes:
*) The NamedGroupFunctions class was added to hold the functions that
are needed for key agreement with some named group. Most of the
group-specific code was moved into subclasses of NamedGroupFunctions.
This allowed me to remove a bunch of code like "if (type == ECDHE) {...}
else if (type == FFDHE) {...}".
*) There are a couple of files in the webrev with no changes due to a
webrev issue. Please ignore them.
*) I moved some code related to XDH parameters and encoding into
java.base. ECUtil now has code to encode/decode XDH public keys, and the
XECParameters file was moved into java.base/sun.security.util. This
organization is similar to how CurveDB and NamedCurve are in java.base,
and it allows the TLS implementation to encode/decode keys without using
the jdk.crypto.ec module.
