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.w...@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.scarp...@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.scarp...@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.scarp...@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.scarp...@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.scarp...@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 >>>> >> >
