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

[Bradford Wetmore]

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