On 4/28/20 5:58 PM, Bradford Wetmore wrote:
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
I have no answer why 255 and 448 are stored, but changing them breaks
the underlying implementation. If a problem occurs where we need 256 &
456, it can be addressed in a different way.
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.
The variants are not the normal case it would seem. Even the spec hints
at this. I suspect Adam didn't want to create all these algorithm names
just for a few variations. I can understand the logic. If a parameter
spec can address them, why not. The base is Ed25519/Ed448.
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?
That is the plan. The reviews you were looking at hadn't fixed that
entirely.
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.
See comments below
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.
I do not know. That file is used to create the Interpolynomial files.
It's probably a comment that explains what is created using the constant
time code. I have not had to run this file.
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.
It may have been created when I was trying different ideas for the CSR
for potentially subclassing the EdDSA algorithm parameter spec. I
deleted it
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?)
The method in question reads the algorithm string searching for "with".
There is no "with" here in the algorithm string. The digest is internal
part of the algorithm and not interchangeable like the other Signatures.
Finally ED448's digest algorithm is SHAKE256, which we do not have a
standard name for nor support in a provider. Max was ok with this decision.
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?
SHAKE256.java
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.
I'm changing this for consistency with XECPrivateKey. Personally I'm
not in favor of interfaces with this sort of requirement and think it's
up to the underlying implementation. If there is a situation where you
don't want the copy, now the spec is in conflict. I don't have an
example where this would happen, I just feel it's over-specifying the
interface.
When you get to Signature.getParameters(), and the follow-on bug & CSR
to fix that, you may understand why I say this.
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.
Ok for both
src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
------------------------------------------ > 47: @code the EdECPoint.
Ok
src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
--------------------------------------------
Do you want to include checks/restrictions for contexts > 255 chars?
Sure
52/65: @code the EdDSAParameterSpec
ok
92: "...bound to the signature" sounds premature. This is the context
bound to the EdDSAParameteerSpec, which hasn't been yet bound to the
signature.
ok
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?"
I just put Optional in front of it, I think that makes is clear. I know
it can get confusing with null and empty.
src/java.base/share/classes/java/security/spec/EdECPrivateKeySpec.java
--------------------------------------------
33: Same comment about representation as above.
Sorry, I don't understand. What above comment about representation?
test/jdk/sun/security/ec/ed/TestEdDSA.java
--------------------------------------------
40: Should not need this kind of SecureRandom initialization anymore.
Remove the seed byte array.
ok
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.
I put it in the @summary
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?
That's a really good catch.. I do not know why he did that, Adam said
all the tests passed, so I didn't have reason to go back and make sure
all the tests from the RFC were include. I do not know why he
implemented about half of them.
After I added the tests I saw the Ed448ph fail too. Took me a while to
finally fine out that the code was using on the message SHAKE256 with a
length of 114 instead of a length of 64. Everything is working in my
workspace now.
Thanks, I'll continue on tomorrow.
Ok.. that should have happen on wednesday :)
Due to the schedule I can't keep dragging this out longer. If you have
them please send them to me today.
Tony
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
