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

Anthony Scarpino anthony.scarpino at oracle.com
Thu Apr 2 05:33:52 UTC 2020 

I can agree with #2. We won’t be setting a precedent by returning null.  Either 1 or 2 allow us to better support a future RFC with an encoding.  #3, probably creates more potential problems than we need.

I believe a majority of EdDSA usage doesn’t use the parameters. As far as I know browsers and ssh don’t support them, so I don’t think we are significantly harming anyone by doing this. 

Tony 

> On Apr 1, 2020, at 9:31 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> I just read https://tools.ietf.org/html/rfc8410 and it intentionally did not encode prehash and context anywhere. If AlgorithmParameters' main purpose is to decode/encode this, then looks like it's useless here.
> 
> The EDDSA case is quite unusual because normally anything set by setParameter on the signing side should be available as byte array thru getParameters().getEncoded() so that it can encoded inside the signature and readable by the verifying side. But here, according to RFC 8410, prehash should be used; according to RFC 8032, context "SHOULD be a constant string specified by the protocol using it" and therefore should be negotiated/determined elsewhere.
> 
> Anyway, so far we have found 3 ways to deal with this:
> 
> 1. EdDSASignature::engineGetParameters throws UOE.
> 2. EdDSASignature::engineGetParameters returns null.
> 3. EdDSASignature::engineGetParameters returns something but it.getEncoded() returns null
> 
> Neither is perfect.
> 
> 1. No one might expect an UOE.
> 2. Conflict with spec, when setParameter() was called.
> 3. No AlgorithmParametersSpi::engineGetEncoded has ever returned a null.
> 
> My current preference is 2), seems like the least chance to break existing programs (at least RSASignature has returned null before). This means a spec change in Signature::getParameters is need to clarify this. We might also need to add a paragraph into the spec of EdDSAParameterSpec saying it does not have a counterpart AlgorithmParameters type because of RFC 8410. We can define an AlgorithmParameters later if a future RFC defined anything.
> 
> Thanks,
> Max
> 
>> On Apr 2, 2020, at 10:45 AM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>> 
>>> 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
>>>> 
>> 
>

More information about the security-dev mailing list