[RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)
Bradford Wetmore
bradford.wetmore at oracle.com
Tue May 5 01:12:54 UTC 2020
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
More information about the security-dev
mailing list