RFR 8171279: Support X25519 and X448 in TLS 1.3

[Xuelei Fan]

On 9/5/2018 10:09 AM, Adam Petcher wrote:
> 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.
Okay, it is used indirectly.

> It 
> is used indirectly through ECUtil to encode and decode keys, and in this 
> case the name is used to look up the parameters.
Can the public NamedParameterSpec be used, instead of the private 
XECParameters?  I think XECParameters may be better in the EC provider, 
please try to mitigate the dependence between modules.  If one day the 
sun.security.util.XECParameters get updated, the EC implementation get 
impacted a lot.  I prefer to use public APIs if possible.

> Is there some place in 
> the code where JSSE is doing something too complicated related to these 
> parameters?
> 
Yes, the algorithm name is sufficient, I'm not sure the necessary to use 
XECParameters in sun.security.util.

>>
>> 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.
It could support "TlsPreMasterSecret", right?  My concern is about not 
limited to this one only.

> 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.
> 
See above.

>>
>> 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.
> 
It looks like a problem to me to use this before it constructed.
     this.functions = new ECDHFunctions(this);

and the use of new object for each named group is not effective.  The 
current NamedGroupFunctions abstract class does not sound good enough to 
me, considering the numbers of the named groups.

I have no idea so far, but I think you can improve it.  Probably use 
static methods?

>>
>> 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.
> 
Thanks, I like it the new design.

Xuelei