RFR 8171279: Support X25519 and X448 in TLS 1.3

[Xuelei Fan]

On 9/6/2018 10:17 AM, Adam Petcher wrote:
>>>>
>>>> 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.
>>
>> I don't know if that works, exactly, due to the fact that I can't 
>> reference static enum members in the body of an enum constructor. How 
>> about this alternative? I can move the NamedGroup enum and all the 
>> NamedGroupFunction classes into a separate class (in a separate file) 
>> called NamedGroups. Then all the NamedGroupFunction classes can be 
>> private in this class, but the NamedGroup enum can still have package 
>> access. I would prefer to leave the getFunctions() method of 
>> NamedGroup (and keep it private) because the functions object may be 
>> missing and the Optional return type of getFunctions() forces me to 
>> deal with this when I call it from within NamedGroup.
>>
I think it should be able to use the "functions" field directly.
    Optional ngf = getFunctions()
    if (ngf.isEmpty() {
       ...
    }

    V.S.

    if (functions != null) {
       ...
    }

I did not see the benefits of the getFunctions() method.

>>
> Actually, it does work. I just have to move the static members of each 
> NamedGroupFunctions subclass into its subclass (e.g. make them 
> singletons). Still, I like my proposed alternative better, because it 
> allows us to simplify SupportedGroupsExtension. Let me know if you have 
> a preference.
My concerns are mainly about:
1. the NamedGroupFunctions should be private and should not be used 
other than the NamedGroup enum impl.
2. the ffdh/ecdh/xdhFunctions static fields should be private of the 
NamedGroup enum as well, and better be lazy instantiated as you are 
using Map objects in the NamedGroupFunctions implementation (for 
performance).
3. NamedGroupFunctions.namedGroupParams is fine in general, but in this 
context, it means the map will always be generated.  We used to use a 
SupportedGroups to wrap and cache the parameters, and don't care about 
the unsupported groups.  But in the new re-org, looks like the 
unsupported groups may also have a chance to cache/use the parameters.

#3 is a new find when I'm trying to understand your proposal.  It would 
be nice if you could think about the SupportedGroups impact.

For the question about how it works, there are a few approaches. You can 
use singletons as you said or inner enum (See CipherSuite.java).

Xuelei