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