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