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
