RFR 8171279: Support X25519 and X448 in TLS 1.3

[Xuelei Fan]

On 9/5/2018 12:49 PM, Adam Petcher wrote:
> New webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.02/
> 
> On 9/5/2018 1:35 PM, Xuelei Fan wrote:
> 
>> On 9/5/2018 10:09 AM, Adam Petcher wrote:
>>
>>> 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.
> 
> The algorithm name is not quite sufficient. See the new methods that 
> were added to ECUtil that encode/decode public keys. We also need to 
> know the key length (which is in XECParameters) in order to 
> encode/decode public keys.
> 
I did not get your point.  The public key sizes for x25519 and x448 are 
fixed, right?

>>
>>>
>>> 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.
> 
> Yes, it supports "TlsPreMasterSecret" in the webrev now. Perhaps I'm not 
> sure what you are suggesting here.
> 
OKay, let me say it in another way.

Thank you for making it works with the SunJSSE provider as you use a 
SunJSSE provider private algorithm name "TlsPreMasterSecret", and 
implement the algorithm in the cyrpto provider.  That's good.

The "TlsPreMasterSecret" is not a public one, and it is not expected to 
be used in other JSSE provider.  If I want to implement a JSSE provider 
by myself, and I use the name "x25519", the crypto provider will deny 
it.  So I have to use the "TlsPreMasterSecret" for my provider. 
However, the name is not supported, and the name can be changed at 
anytime in the future.  How should I do to use the crypto provider for 
my JSSE provider?  Looks like no way to use the JDK cyrpto provider 
unless I use the SunJSSE private name.  Should I implement my own crypto 
provider?  I don't want to do that unless it is really necessary.

For the KeyAgreement.generateSecret​(String algorithm) method, it seems 
that the supported algorithms of each provider are missed in the 
documentation.  As may make the method hard to use.

This issue is not specific to XDH.  I'm fine if you don't want to 
address it in this update.

>>
>>>>
>>>> 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?
> 
> In the latest webrev, I changed it so there is a single static 
> NamedGroupFunctions of each type, and the NamedGroup is passed in as the 
> first argument to each method that requires it (rather than being a 
> member of NamedGroupFunctions).
> 
After the re-org, I guess you can put the inner classes in NamedGroup 
enum and declare them as private?  The getFunctions() method may be 
unnecessary then.

Thanks,
Xuelei