[RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

Thu Apr 2 02:45:15 UTC 2020

On 4/1/20 6:44 PM, Weijun Wang wrote:
> Two comments:
> 
> 1. Signature::getParameters explicitly specifies that the return value should match that set in setParameter(). Therefore although the types of params are different we need to make sure the contents are essentially the same. This means the getParameters() result should be designed after EdDSAParameterSpec.

If that is the case, then at the least both 
AlgorithmParameter.getEncoded methods will not be supported or return an 
empty array because there is no official encoding.

> 
> 2. If the Signature is of "EDDSA" type (i.e. not ed25519 or ed448), then before calling initSign() one does not know what named curve will be used. This means the result will not include curve info.
If I remember correctly that's the way all the other EC curves work, so 
this is nothing new.
In any case for Signature EDDSA is defaulting to 25519. I saw the 
default today and I'm not sure I like it, I would rather the user 
specify ED25519 and ED448.  The generic way EDDSA is used sometimes in 
this code I feel makes it more confusing.

> 
> --Max
> 
>> On Apr 2, 2020, at 9:21 AM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>>
>>
>> I understand what the spec says, but here is why I believe the code returns UOE.
>>
>> EdDSASignature.setParameters only takes EdDSAParameterSpec and does not take NamedParameterSpec, which can define the curve.  The curve is defined during getInstance().  While there is an ASN.1 for the curve there is none defined for the values contained in EdDSAParameterSpec, prehash and context.  https://tools.ietf.org/html/rfc8410#page-3
>>
>> I think it is confusing for EdDSASignature.getParameters() to give a AlgorithmParameters class that provides NamedParameterSpec which cannot be used with EdDSASignature.setParameters().
>>
>> Below is what I could do to satisfy the API. I'm just not sure it has any more value than that:
>> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.04/raw_files/new/src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSAAlgorithmParameters.java
>>
>> Tony
>>
>> On 4/1/20 12:22 AM, Weijun Wang wrote:
>>> While SignatureSpi says engineGetParameters() would throw an UOE when it's not overridden, it is not specified in Signature::getParameters:
>>>       * <p> If this signature has been previously initialized with parameters
>>>       * (by calling the {@code setParameter} method), this method returns
>>>       * the same parameters. If this signature has not been initialized with
>>>       * parameters, this method may return a combination of default and
>>>       * randomly generated parameter values if the underlying
>>>       * signature implementation supports it and can successfully generate
>>>       * them. Otherwise, {@code null} is returned.
>>> I'm afraid we'll have to return something here, maybe a mixture of a curve name and fields inside EdDSAParameterSpec.
>>> --Max
>>>> On Mar 31, 2020, at 12:25 PM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>>>>
>>>> On 3/30/20 8:52 PM, Anthony Scarpino wrote:
>>>>> On 3/30/20 7:54 PM, Weijun Wang wrote:
>>>>>>
>>>>>>
>>>>>>> On Mar 31, 2020, at 10:50 AM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>>>>>>>
>>>>>>> On 3/30/20 11:52 AM, Anthony Scarpino wrote:
>>>>>>>> On 3/30/20 6:21 AM, Weijun Wang wrote:
>>>>>>>>> I was playing with keytool with your patch and noticed
>>>>>>>>> sun.security.util.KeyUtil.getKeySize(Key) does not support an
>>>>>>>>> EdECKey. While we use curve name instead of key size in EC to
>>>>>>>>> describe the parameters, the size is still useful in determining the
>>>>>>>>> strength.
>>>>>>>> I think I should be able to access the parameter with the key's NamedParameterSpec to return the size.
>>>>>>>
>>>>>>> I was wrong about this.  The parameters are part of jdk.crypto.ec while KeyUtil is part of java.base, so I cannot access the internal EdDSAParameters class.
>>>>>>>
>>>>>>> The easiest way would be to look at the NamedParameterSpec and return a hardcoded length based on the curve used. It's not ideal, but all these curves don't change lengths unlike for RSA, AES, etc.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>> Tony
>>>>>>>
>>>>>>>>>
>>>>>>>>> There is also a KeyUtil.getKeySize(AlgorithmParameters) nearby. I'm
>>>>>>>>> not involved with previous discussions on EdDSA design, but why does
>>>>>>>>> EdDSASignature.engineGetParameters() throw an UOE?
>>>>>>>> Because the public API for engineGetParameter(String param) is deprecated would be my suspicion.
>>>>>>
>>>>>> engineGetParameter() is deprecated, engineGetParameters() is not.
>>>>> Oh sorry.  I'm not sure why, but I have to ask the question what is the point of this method?  If the user needs to set the parameters to do the a signature of verify, what is the need for a method that gets the parameter from Signature that should have just set?  Are the parameters returned changed from the initial setting?  In the EdDSA case they are not.
>>>>> I don't have an immediate problem in having EdDSA return the same parameters back, I'm just not sure why it's necessary and I haven't see anything in the JEP as to why Adam decided against this.
>>>>> Tony
>>>>
>>>> Ok.. That's frustrating that engineSetParameters takes AlgorithmParameterSpec, but engineGetParameters returns AlgorithmParameter.  That confused me.
>>>>
>>>> So I would say the reason EdDSASignature.engineGetParameters() is UOE is because the parameters are not exposed publicly.  The NamedParameterSpec tells the internals of EdDSA what parameters to use.  I know this to be a choice by Adam as he found it unnecessary to expose APIs that were unnecessary at this time with predefined EdDSA curves and with a implementation that did not allow arbitrary curves.
>>>>
>>>> Tony
>>>>
>>>>>>
>>>>>>>>> Another small problem:
>>>>>>>>>
>>>>>>>>> You reverted the copyright year from 2020 to an earlier year in
>>>>>>>>> module-info.java, keytool/Main.java.
>>>>>>>> The copyright has not been reverted, the jdk repo was updated to 2020 from another changeset.
>>>>>>
>>>>>> Ah yes, I applied your patch to my old repo.
>>>>>>
>>>>>> --Max
>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks, Max
>>>>>>>>>
>>>>>>>>>> On Mar 24, 2020, at 2:53 AM, Anthony Scarpino
>>>>>>>>>> <anthony.scarpino at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 2/25/20 12:49 PM, Anthony Scarpino wrote:
>>>>>>>>>>> Hi I need a code review for the EdDSA support in JEP 339.  The
>>>>>>>>>>> code builds on the existing java implemented constant time
>>>>>>>>>>> classes used for XDH and the NIST curves.  The change also adds
>>>>>>>>>>> classes to the public API to support EdDSA operations. All
>>>>>>>>>>> information about the JEP is located at: JEP 339:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8199231 CSR:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8190219 webrev:
>>>>>>>>>>> https://cr.openjdk.java.net/~ascarpino/8166597/webrev/ thanks Tony
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I updated the webrev with some minor updates that were commented
>>>>>>>>>> previously.
>>>>>>>>>>
>>>>>>>>>> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Tony
>>
> 