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.

Reply via email to