RFR 8171279: Support X25519 and X448 in TLS 1.3

[Xuelei Fan]

On 9/10/2018 8:14 AM, Adam Petcher wrote:
> Xuelei,
> 
> Here is the latest webrev: 
> http://cr.openjdk.java.net/~apetcher/8171279/webrev.04/
> 
> I modified the TLS implementation so that it uses X509EncodedKeySpec 
> when interacting with the provider. I did a small amount of refactoring 
> in X509Key to expose the functionality I needed to do this.

I don't think it is not straightforward choice to us X509EncodedKeySpec. 
  We have to use the right OID, and make sure the key bytes for X.509 
protocols are exactly the same as TLS.  Not mention to the cost to add 
the OID and remove the OID.

Maybe, using EncodedKeySpec is more simple.
     package sun.security.ssl;

     private class XDHEncodeKeySpec extension EncodedKeySpec {
         // algorithm: x25519 or x448
         XDHEncodeKeySpec(byte[] encodedKey, String algorithm) {
            super(encodedKey, algorithm);
         }

         @Override
         String getFormat() {
             return "raw";
         }
         ...
     }


     package sun.security.ec;
     public class XDHKeyFactory extends KeyFactorySpi {


         private PublicKey generatePublicImpl(KeySpec keySpec)
             throws InvalidKeyException, InvalidKeySpecException {
             ...
             } else if (keySpec instanceof EncodedKeySpec &&
                encodedKeySpec.getAlgorithm() is "x25519" &&
                encodedKeySpec.getFormat is "raw") {
                // raw public key
                byte[] radPublicKey = encodedKeySpec.getEncoded();
             } else {
             }
         }
     }



> This change 
> removed the dependency on XECParameters, and I moved it back into 
> jdk.crypto.ec. I also moved some more functions into NamedGroup (e.g. 
> isAvailableGroup).
> 
> This webrev also has the change to NamedGroup that makes 
> NamedGroupFunctions private. I put the NamedGroup enum into its own 
> file, which I think is reasonable because it is used in several places 
> other than SupportedGroupsExtension. It also makes sense to separate all 
> of the known named groups and their properties (NamedGroups.java) from 
> the supported_groups extension and the logic related to which groups are 
> supported for key exchange (SupportedGroupsExtension.java). This change 
> required changes to the import statements of several files.
> 
They do not sound like good reasons to me.  You may not want to do that 
again if you understand the following questions you have.

> I'm not totally sure I understood your concern related to the 
> AlgorithmParameters map. The map is always created, but we only create 
> and cache parameters when they are first needed. 
In the current implementation, there is only one map and the parameters 
are not create unless they are supported.

In your implementation, there are three maps, which are created always, 
even if FFDHE/XDH is not supported.  This is a minor issue to me.

If I did not miss something, in your implementation, the parameters can 
be created and push to the map even it is not supported?  Am I right? 
We used to try avoid to generate parameters for unsupported groups.


> Of course, the map is 
> not a proper cache, and the objects stay in memory forever once they are 
> created. Is this your concern, or is it something else?
> 
> I've made several structural changes since the first webrev, and I 
> haven't been paying too close attention to style, so you probably 
> shouldn't either. Once the approach and structure look good to you, I 
> can clean up the code and submit another webrev so you can check it for 
> style, formatting, etc. I also haven't merged in changes from the 
> default branch in a while, so I'll need to do that, too.
> 
Yes, let's work out the big picture at first.

Help it helps.

Xuelei

> 
> On 9/7/2018 12:12 PM, Xuelei Fan wrote:
>> On 9/7/2018 9:03 AM, Adam Petcher wrote:
>>> This is more clear, thanks. See below.
>>>
>>>
>>> On 9/7/2018 11:34 AM, Xuelei Fan wrote:
>>>> EncodedKeySpec keySpec = ... // find a way to construct the keySpec
>>>>                              // at least, we can use:
>>>>                              //    new EncodedKeySpec(byte[]);
>>>>                              // But please check if there's a better 
>>>> one
>>>
>>> Do you mean X509EncodedKeySpec, or some class that is specific to XDH? 
>> I did not search the spec definitions.  At least we can use the 
>> EncodedKeySpec class.   It's nice if you can find a better one, or 
>> define a new one for XDH.
>>
>> Your following comments make sense to me.
>>
>> Thanks,
>> Xuelei
>>
>>> An X509EncodedKeySpec would probably work---we would just need to put 
>>> the key into a SubjectPublicKeyInfo, which means I need the OID. Is 
>>> it okay for me to put the OID in JSSE? I could put it in the 
>>> NamedGroup enum, like this:
>>>
>>>      X25519      (0x001D, "x25519", true, "x25519", "1.3.101.110",
>>>          ProtocolVersion.PROTOCOLS_TO_13),
>>>      X448        (0x001E, "x448", true, "x448", "1.3.101.111",
>>>          ProtocolVersion.PROTOCOLS_TO_13),
>>>
>>> I'm not sure if the second x25519/x448 strings (for algorithm and 
>>> NamedParameterSpec names) would still be needed, since I can use the 
>>> OID in some of these places. If it's not needed, then perhaps I can 
>>> remove it and only use the OID to interact with the JCA provider.
>>>
>>> We don't have a type for XDH encoded public keys. It would be nice to 
>>> be able to do something simple like:
>>>
>>> byte[] keyBytes = ...
>>> NamedParameterSpec paramSpec = new NamedParameterSpec("X25519");
>>> XECEncodedPublicKeySpec keySpec = new XECEncodedKeySpec(paramSpec, 
>>> keyBytes);
>>>
>>> and then give keySpec to the "XDH" key factory. But this 
>>> XECEncodedKeySpec type does not exist, so this would require an 
>>> enhancement to the API.
>