[RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)
Bradford Wetmore
bradford.wetmore at oracle.com
Sat May 2 01:21:48 UTC 2020
>> 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.
Tony and I have talked over this offline, and we've decided we'll follow
the sizings of what was done for XDH (x25519/x448 Key Exchange) which is
255/448. Further comments captured here for posterity:
---begin---
I wasn't talking changing about the internal representation, my question
was more about whether the "keysize" we present to things like
KeyPairGenerator should be the actual keysize in storage bits, or the
"keysize" based on the p value. i.e.
KeyPairGenerator kpg = KeyPairGenerator.getInstance("EdDSA");
kpg.initialize(XXX);
Right now, if I call kgp.initialize(256), it fails with an j.s.IPE.
Only 255 and 448 are allowed.
My initial thought was that it should be the keysize representation,
which would be 32/57 bytes (256/456 bits), but I can see the other way
since the curve is over the fields 2^255 - 19 and 2^448 - 2^224 - 1, I
am not sure what should be going here.
KeyPairGenerator
All key pair generators share the concepts of a keysize...The
keysize is interpreted differently for different algorithms (e.g.,
in the case of the DSA algorithm, the keysize corresponds to the
length of the modulus).
void KeypairGenerator.initalize(int keysize);
Initializes the key pair generator for a certain keysize
keysize - the keysize. This is an algorithm-specific metric,
such as modulus length, specified in number of bits.
---end---
>> 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.
I can buy that. It's made pretty clear in the EdDSAParameterSpec how to
set.
>> 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.
Tony/I talked further, he is going to go with the RFC naming style:
EdDSA/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.
>
> 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
Thanks, I was scratching my head! ;)
>> 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.
Ok.
>> 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
Ok, I see it now. Thanks.
>> 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.
Ok.
>> src/java.base/share/classes/java/security/interfaces/EdECPublicKey.java
>> ------------------------------------------ > 47: @code the EdECPoint.
Just a quick reminder, there were other javadoc issues throughout.
Would be good to take a pass looking at javadoc again. I only called
out a few.
>> src/java.base/share/classes/java/security/spec/EdDSAParameterSpec.java
>> --------------------------------------------
>> Do you want to include checks/restrictions for contexts > 255 chars?
>
> Sure
Thanks.
>> 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.
I'll try to remember to take a look at the final version when it's
ready. Not quite envisioning what you ended with.
>> 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?
My bad, I had a previous comment that I decided against making, and then
forgot to remove this one.
>> 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
That's probably close enough to where the tests are defined that folks
can find the source easily.
>> 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.
I didn't review the fix itself, but it did pass the one test in my
workspace. I'll get to that soon.
>
>>
>> Thanks, I'll continue on tomorrow.
>
> Ok.. that should have happen on wednesday :)
Still working on them.
> Due to the schedule I can't keep dragging this out longer. If you have
> them please send them to me today.
I'll see if I can get some more time over the weekend. The APIs are ok
with me tho, given what we talked about. I'm down in the impl code at
this point, which can be fixed later if issues arise.
Thanks,
Brad
More information about the security-dev
mailing list