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

[Bradford Wetmore]

Finally found the bottom of the review.  ;)  I'll send you minor nits 
(unused imports/etc.) in a separate email.


src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSAOperations.java
----------
136:  Here you are calling sun.security.util.ArrayUtil.reverse() (and
swap()), but you also added a reverse code in EdDSAPublicKeyImpl.java.
I would suggest removing the one in EdDSAPublicKeyImpl and make those
calls call into ArrayUtil.

141:  Wouldn't it be better to use a temporary variable here instead of
2 array reversals?

src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSASignature.java
----------
115-118:  You're calling edKey.getPoint() twice here.

139/150:  indent problem.

test/jdk/sun/security/ec/ed/TestEdDSA.java
----------
362:  Extra //XXX

test/jdk/sun/security/ec/ed/TestEdOps.java
----------
Please put in where you got these test vectors.

206:  Could you put in a quick note here why you're doing this?  i.e. to
avoid draining the system's entropy pool by using a seeded PRNG.

Thanks,

Brad



On 5/4/2020 6:12 PM, Bradford Wetmore wrote:
> All minor nits, can be done later if it won't be a problem to make minor 
> API wording tweaks.
> 
> On 5/4/2020 10:17 AM, Anthony Scarpino 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.
>>>
>>
>> Here is the final code review for the JEP. As the JEP is preparing to 
>> move to Propose-to-Target, if you have comments please state if they 
>> must be fixed as part of the initial putback.
>>
>> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.05/
> 
> Javadoc issues remain throughout java.security.*.
> 
> e.g. EdDCPoint
> @param y the y-coordinate, represented using a BigInteger
> ->
> @param y the y-coordinate, represented using a @{code BigInteger}
> 
> a boolean indicating whether the x-coordinate is odd.
> ->
> a boolean indicating whether the x-coordinate is odd
> 
> src/java.base/share/classes/sun/security/provider/SHA3.java
> ----------
> 114:  Is this comment about the 2-bit suffix still correct?
> 
> src/java.base/share/classes/sun/security/util/KeyUtil.java
> ----------
> 2:  Copyright date.
> 
> 110:  Do you want to go with the hardcoded values, or fix the commented 
> out code?
> 
> src/java.base/share/classes/java/security/interfaces/EdECPrivateKey.java
> ----------
> 38:  Very minor nit: EdEC reads a bit awkward to my eye: that term isn't 
> used in the RFC.  Change or keep.  Maybe:
> 
> An EdEC private key
> ->
> An Edwards-Curve private key ...
> or
> An {@code EdECPrivateKey} ...
> 
> Noticed some new typos, or you could do a minor replacement and reduce 
> some of the duplicate wording.
> 
> 51:
> the an {@code Optional}
> ->
> an {@code Optional}
> 
> 52:
> Ff
> ->
> If
> 
> Alternate idea:  You could take out the second sentence in the method
> description above and replace the @return with:
> 
>      * @return the private key byte array, or an empty {@code Optional} 
> if the
>      *     key cannot be extracted (e.g. if the provider is a hardware 
> token
>      *     and the private key is not allowed to leave the crypto 
> boundary).
> 
> This was based on the XEC wording.  Your call/choice.
> 
> src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
> ----------
> 37:  Same comment about EdEC.  Change or keep.
> 
> src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
> ----------
> 92:  Repeat of previous comment from last week.   You replied "Ok" but 
> didn't
> see a change yet.
> 
> "...bound to the signature" sounds premature.  This is the context bound
> to the EdDSAParameteerSpec, which hasn't been yet bound to the signature.
> 
> src/java.base/share/classes/java/security/spec/EdECPoint.java
> ----------
> 38:  Same comment about EdEC.  Change or keep.
> 
> test/jdk/sun/security/ec/ed/TestEdDSA.java
> ----------
> Nits:
> 
> 211-212:  Indention problem
> 
> 159/257/258/301:  Extra whitespace.
> 
> 313:  The context value was set correctly from a previous test, but
> wouldn't hurt to reiterate it here.
> 
> Brad
>