[RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)
Anthony Scarpino
anthony.scarpino at oracle.com
Fri May 1 18:32:21 UTC 2020
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
>
More information about the security-dev
mailing list