Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/
On 9/4/2018 3:25 PM, Xuelei Fan wrote:
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.
Sorry, I don't think I understand what you are suggesting here. As far
as I know, nothing in sun.security.ssl uses XECParameters directly. It
is used indirectly through ECUtil to encode and decode keys, and in this
case the name is used to look up the parameters. Is there some place in
the code where JSSE is doing something too complicated related to these
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?
I simply continued the pattern that was used in DH and ECDH. We can use
a different string here, but I worry that people will be surprised if DH
and ECDH support "TlsPreMasterSecret" but XDH doesn't. I would rather
continue to use "TlsPreMasterSecret" for now, and then add support for a
standard, TLS-independent name (that means the same thing) in a separate
ticket. But if you feel strongly that XDH should not support
"TlsPreMasterSecret", then perhaps we can use a different string now. In
this case, let me know if you have any suggestions for what string to use.
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.
A simple way to accomplish this is to leave the structure alone and add
methods in NamedGroup that pass through to the corresponding methods in
NamedGroupFunctions. I made this change in the updated webrev. Let me
know what you think.
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.
This interface is only used to validate public keys against algorithm
constraints. In the latest webrev, I moved all of this functionality
into NamedGroupFunctions and removed the SSLKeyAgreementCredentials
interface.
