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
