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 <[email protected]> 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 <[email protected]>
>> 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
>>>> <[email protected]> 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
>>
>
