[RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)
Bradford Wetmore
bradford.wetmore at oracle.com
Wed Apr 29 00:58:15 UTC 2020
Hi Tony,
Apologies for the delay.
> I updated the webrev with some minor updates that were commented
> previously.
>
> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.01/
I've finished the APIs and a fair chunk of the implementation, but had
some initial comments.
Overall, the APIs look reasonable and I (or whoever) should have no
problems adding TLS support for EdDSA once this is in place.
https://bugs.openjdk.java.net/browse/JDK-8166596
BTW, I reviewed based off the webrev.01 version (above), but I found in
a later email exchange there was mention of a .04:
> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.04/
dated April 1. I'm hoping that was just a proposed change idea, and not
a new version to review. Please advise if so.
In my comments below, if I say "you" below, it's the colloquial "you."
Could be you or the previous engineer.
If some of these questions have already been answered, I'll apologize in
advance. I'm getting up to speed in RFC 8032 specifics so forgive any
"duh" questions.
----
Common: Throughout the API's, I noticed multiple minor javadoc
style issues, such as:
. periods inconsistencies (missing or extras) (e.g. EdECPoint)
. omissions of @code (e.g. EdDSAParameterSpec/EdECPoint/etc.)
Suggest a javadoc sweep before final CSR. I started calling them out,
but stopped.
For keysize in things like KeyPairGenerator, why are we using 255/448
(externally and internally) instead of 256/456? From RFC 8032:
section 3.2: "An EdDSA private key is a b-bit string k"
+
section 5.1.5/Ed25519: "The private key is 32 octets (256 bits,
corresponding to b) of cryptographically secure random data..."
section 5.2.5/Ed448: "The private key is 57 octets (456 bits,
corresponding to b) of cryptographically secure random data..."
and
section 3, parameter 2: "EdDSA public keys have exactly b bits"
and
section 5.1: "(parameter) b: 256"
section 5.2: "(parameter) b: 456"
The Wikipedia entry also lists as 256. https://en.wikipedia.org/wiki/EdDSA
I notice BC does actually use both.
https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L365
https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L371
Why did you decide to not use the full algorithm names for the variants e.g.
Ed25519ctx/Ed25519ph/Ed448ph instead of just Ed25519/Ed448 and let the
selection be done using the Parameters. More for my understanding than
an issue.
Also, I thing you and Max might have discussed EdDSA vs EDDSA (case),
but in a different context. RFC 8410/Section 8, the full names are
"EdDSA"/"Ed25519"/"Ed448",
but you're using "EDDSA"/"ED25519"/"ED448".
There is precedence for upper/lower case in our Java Standard Names.
https://docs.oracle.com/en/java/javase/14/docs/specs/security/standard-names.html
, so just wondering why you're standardizing on the upper case variant?
Is Ed448ph working? I've been playing with your test vectors from RFC
8032 in test/jdk/sun/security/ec/ed/TestEdDSA.java, where you included
Ed25519/Ed25519ctx/Ed25519ph, but omitted several (6 of 9) of the Ed448
tests and all of the Ed448ph tests. I tried very quickly to add the two
Ed448ph tests in in section 7.5, but no luck. Could be pilot error, but
I tried a lot of different combos (with/without ph boolean, null array
vs. empty array, etc), but seems like it should have been
straightforward given the test framework.
Specific file comments:
make/jdk/src/classes/build/tools/intpoly/FieldGen.java
--------------------------------------------
Lines 643-650 are creating a bunch of comment like this which are
the BigInteger values used later:
//(0%nat,16110573)::(26%nat,10012311)::(52%nat,30238081)::(78%nat,-8746018)::(104%nat,1367802)::nil.
but what is the format of comment expressing? What ae %nat/nil/::/etc.
src/java.base/share/classes/java/security/spec/NamedParameterSpec.java
--------------------------------------------
94: This never seems to be called in our main test suite, so not sure
why this is here. Please add a comment as to why you added this new
no-args constructor. (was it some Class.newInstance() somewhere/prevent
unexpected object creation/some kind of reflection?)
Minor nit, you could put this constructor above the other constructor,
it gets kind of lost here visually.
src/java.base/share/classes/sun/security/pkcs/PKCS7.java
--------------------------------------------
832: Seems like the right thing to do for making things bulletproof.
You're convinced this shouldn't happen in our code somewhere? (i.e.
unexpected new failure?)
src/java.base/share/classes/sun/security/provider/SHA3.java
--------------------------------------------
I see what you did here and made 0x06 a parameter in each of the
implementation classes, but wondering why? Is there another change I
haven't gotten to yet?
src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
--------------------------------------------
74: Why 255 and not 256? (See comment above)
src/java.base/share/classes/sun/security/util/math/intpoly/IntegerPolynomial.java
--------------------------------------------
OK - I didn't check the math closely in this package, but the
refactoring looks reasonable.
src/java.base/share/classes/java/security/interfaces/EdECPrivateKey.java
--------------------------------------------
47: Do you want to mention a new copy of the array is returned here?
You do say this in the spec version, and we do in the XECPrivateKey
interface.
48: @code the Optional.
53: Might suggest reworking this to mention the Optional, since it is
actually the return type. See XECPrivatekey for an example.
src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
--------------------------------------------
47: @code the EdECPoint.
src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
--------------------------------------------
Do you want to include checks/restrictions for contexts > 255 chars?
52/65: @code the EdDSAParameterSpec
92: "...bound to the signature" sounds premature. This is the context
bound to the EdDSAParameteerSpec, which hasn't been yet bound to the
signature.
94: The word "empty" here seems overloaded between empty arrays which
are array objects with no elements, and empty Optionals which are
Optionals with no values which would later throw a
NoSuchElementException on a get(). Wondering if there is a better way
to say this to not cause confusion? Maybe saying this is an "empty
Optional?"
src/java.base/share/classes/java/security/spec/EdECPrivateKeySpec.java
--------------------------------------------
33: Same comment about representation as above.
test/jdk/sun/security/ec/ed/TestEdDSA.java
--------------------------------------------
40: Should not need this kind of SecureRandom initialization anymore.
Remove the seed byte array.
51: Please add some comments in here to indicate that these are the
test vectors from RFC 8032, so that folks can mentally map if they so
choose.
194: Comment is above, but more details here. All of the test vectors
were included to this point, then it just stopped after 3 Ed448 tests,
leaving out 6 of 9 tests, including all of the Ed448ph tests. RFC pages
32-39. Intentional? I did try to stick in the two Ed448ph tests from
7.5, and it failed, so maybe I'm missing something?
Thanks, I'll continue on tomorrow.
Brad
On 3/23/2020 11:53 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.
>>
>> All information about the JEP is located at:
>> JEP 339: https://bugs.openjdk.java.net/browse/JDK-8199231
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8190219
>>
>> webrev: https://cr.openjdk.java.net/~ascarpino/8166597/webrev/
>>
>> thanks
>>
>> Tony
More information about the security-dev
mailing list