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

[Weijun Wang]

I don't know what the final solution for getParameters now but we can be sure it won't return something that has a non-null getEncoded(), this means the following change is necessary:

diff --git a/src/java.base/share/classes/sun/security/x509/X509CertImpl.java b/src/java.base/share/classes/sun/security/x509/X509CertImpl.java
--- a/src/java.base/share/classes/sun/security/x509/X509CertImpl.java
+++ b/src/java.base/share/classes/sun/security/x509/X509CertImpl.java
@@ -602,8 +602,12 @@
                     null);
 
             // in case the name is reset
+            AlgorithmParameters p = null;
             if (signingParams != null) {
-                algId = AlgorithmId.get(sigEngine.getParameters());
+                p = sigEngine.getParameters();
+            }
+            if (p != null) {
+                algId = AlgorithmId.get(p);
             } else {
                 algId = AlgorithmId.get(algorithm);
             }

If we decide getParameters() should throw an exception, the check will be different.

PKCS10::encodeAndSign got it correctly.

X509CRLimpl::sign has not considered possible parameters at all. I've just asked for a code view at http://cr.openjdk.java.net/~weijun/8242184/webrev.00/ to add it but it will need to same adjustment once EdDSA is added.

Thanks,
Max


> On Apr 6, 2020, at 4:34 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> Some other issues:
> 
> 1. AlgorithmId::derEncode. Otherwise openssl does not like the extra NULL.
> 
> @@ -196,7 +205,8 @@
>             } else {
>                 bytes.putNull();
>             }*/
> -            if (algid.equals(RSASSA_PSS_oid)) {
> +            if (algid.equals(RSASSA_PSS_oid) || algid.equals(ed448_oid)
> +                    || algid.equals(ed25519_oid)) {
>                 // RFC 4055 3.3: when an RSASSA-PSS key does not require
>                 // parameter validation, field is absent.
>             } else {
> 
> 2. AlgorithmId::getWithParameterSpec. Please return "AlgorithmId.get(algName)" when "spec instanceof EdDSAParameterSpec". 
> 
> Thanks,
> Max
> 
>> On Apr 5, 2020, at 5:11 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>> 
>> OK, I undertand now:
>> 
>> 1. Crypto primitives (Signature/KeyFactory/KeyPairGenerator) would support all "EdDSA" and "Ed25519"/"Ed448", and their getAlgorithm() returns what was used back in getInstance().
>> 
>> 2. Key's getAlgorithm() always returns "EdDSA".
>> 
>> Thanks,
>> Max
>> 
>>> On Apr 4, 2020, at 6:02 AM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>>> 
>>> On 4/2/20 8:34 PM, Weijun Wang wrote:
>>>> Another question:
>>>> Why does getAlgorithm() of PublicKey and PrivateKey return "EdDSA"
>>>> instead of "Ed25519" and "Ed448"?
>>>> Do we suggest people using "EdDSA" or "Ed25519"/"Ed448" when calling
>>>> KeyFactory.getInstance() andKeyPairGenerator.getInstance()?
>>> 
>>> I don't think the code is suggesting anything. I believe the implementation is trying to be consistent with the API and other asymmetric keys factories and generators.  Just using EC as an example one uses "EC" for the getInstance() and provides the AlgorithmParameterSpec to generate the publicKey
>>> 
>>> kf = KeyFactory.getInstance("EC");
>>> ECParameterSpec.ep = ..
>>> kf.generatePublicKey(ep)
>>> 
>>> Being consistent for EDDSA, replace "EC" with "EDDSA" and specify a NamedParameterSpec to generate the public key; however, it is allowed to replace "EC" with ED25519. Similar to how XDH does it. Unfortunately generatePublicKey requires an AlgorithmParameterSpec, which is redundant in this case:
>>> ---
>>> kf = KeyFactory.getInstance("ED25519")
>>> ...
>>> kf.generatePublicKey(NamedParameterSpec.ED5519);
>>> ---
>>> 
>>> Since existing code follows the first example we should be consistent for apps adding EDDSA.
>>> 
>>> For KeyPairGenerator, initialize() isn't required to be called with getInstance("ED25519") to generate the key, but it will accept an initialize() call.  What's different is "EDDSA" will default to ED25519 and does not require initialize(), but it will accept initialize() to change it to ED448.  I believe this is to be consistent with EC and RSA that need initialize().
>>> 
>>> To complete all the EDDSA entries, Signature is different because, the key provides the details about the curve.  It's similar to KeyPairGenerator, "EDDSA" doesn't lock you into a particular curve, allowing the key to specify the curve, which follows the EC/RSA logic. Specifying the curve at getInstance() locks you into that curve so at least NoSuchAlgorithm will be thrown at getInstance() unlike other asymmetric algorithms.
>>> 
>>> So to wrap all this up, it makes sense for consistency with prior behavior that all 3 classes have an EDDSA entry.  And the specific curve usage is also consistent with what has already been integrated with XDH.
>>> 
>>> Tony
>>> 
>>> 
>>>> 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
>>> 
>> 
>