Add Ed25519 DSA #394
Add Ed25519 DSA #394
Conversation
src/com/amazon/corretto/crypto/provider/AmazonCorrettoCryptoProvider.java
Outdated
Show resolved
Hide resolved
|
Git soapbox incoming - It's good practice to squash your commits prior to submitting your code a review (especially when you've got a messy history like this). That way, you can hide all of the intermediate states of the code from the reviewers and starts the whole thing off on a cleaner slate. Subsequent iterations once the review has started should not be squashed to allow for inter-revision diffs. Once the PR is approved we'll do a squash and rebase to cleanly apply it to the trunk. |
src/com/amazon/corretto/crypto/provider/AmazonCorrettoCryptoProvider.java
Outdated
Show resolved
Hide resolved
This sounds like a short term one-way door decision that we'd be stuck with for a long time. This breaks forwards compatibility with Java code that implements against the expected JDK classes. e.g. If the JSSE were to use EdDSA it would not be able to delegate to ACCP for the implementation without an upstream code change. Do we have a story around how we may add compatibility for the standard Java classes? How much investigation was done around working around the JDK 8 limitation and solving that? Do we have hard requirements to support Ed25519 for JDK8 code bases? |
tst/com/amazon/corretto/crypto/provider/test/EvpKeyFactoryTest.java
Outdated
Show resolved
Hide resolved
I don't think it would be too much work to add compatibility for the standard Java classes if they became available. The EvpEdPrivateKey class would just need to implement EdEcPrivateKey (the java class name) instead of PrivateKey, and an override tag would need to be added to the getPrivateKey method. The EvpEdPublicKey class would need the parent class to be changed, and also the getPoint method to be implemented (the EdEcPoint class doesn't exist until JDK 15, so the method can't be implemented yet). The EvpEdKey parent class would have to implement the EdEcKey class from Java, and a getParams method would need to be added (returns relevant NamedParameterSpec). NamedParameterSpec is used to specify default parameters for Ed25519, Ed448, X25519, and X448. This isn't an issue at the moment, since we just default to Ed25519, but might become necessary in the future if the rest are added. The reason we must support JDK8 is that to build ACCP, everything must compile on JDK 8 (See this file). We tried removing that line (and the --release 9 line above it), but it caused errors with JNI. |
This is what I was most curious about. How far along this line of inquiry you went... It would be interesting to know if there's some way for us to conditionally compile source code that imports JDK15 classes if the correct JDK is available to make this body of work "more complete" and compatible with JDK15+ code bases. I also understand that the build as it stands requires JDK8, but I wanted to know if we had a specific requirement to support ed25519 on JDK8. Lacking that requirement opens the door for us to avoid these kinds of private namespace workarounds. |
|
|
||
| EdGen(AmazonCorrettoCryptoProvider provider) { | ||
| Loader.checkNativeLibraryAvailability(); | ||
| provider_ = provider; | ||
| try { | ||
| kf = KeyFactory.getInstance("EdDSA", "SunEC"); |
There was a problem hiding this comment.
I'm a bit concerned about the FIPS implications. It looks like you're trying to export the KeyPair using JDK based interfaces by tumbling our Key classes into the SunEC KeyFactory. This helps with the interop story, but like we covered earlier there's a whole lot more work to get full interop working and doing it piecemeal can be counterproductive. Since this is along the lines of ASN.1 encode/decode it's probably okay, but it does break down our "all crypto comes from AWS-LC" story. Would like to get some more consensus from the team on how we should handle this.
There was a problem hiding this comment.
- There are places that ACCP relies on other providers for ASN.1 encode/decode, like GCM parameters.
- Without this, one cannot use keys generated by ACCP to perform sign/verify with another provider.
- It is expected that the private (public) key generated by ACCP can be type cast to EdECPrivateKey (EdECPublicKey).
There was a problem hiding this comment.
In addition to increasing our tacit dependency on JDK providers, running this through SunEC's KeyFactory also has some performance costs.
There was a problem hiding this comment.
I'm in favor of keeping this change to increase compatibility. This issue was accidentally caught by one of our own basic cross-compatibility unit tests between SunEC and ACCP. Any existing Ed25519 users coming from SunJCE who are thinking about migrating to ACCP will likely add their own similar cross-compatibility unit test, and could easily run into this exact same issue themselves. It's better for ACCP to solve it once, than have many customers re-discover and re-solve this issue themselves.
There was a problem hiding this comment.
Given likely use-cases for Ed25519 signature key generation, I agree that compatibility/user experience outweigh performance here. We can optimize the latter in the future dependent on customer need.
| final Signature jceSig = Signature.getInstance("Ed25519", "SunEC"); | ||
|
|
||
| // Sign with ACCP and verify with SunEC | ||
| final byte[] message = new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; |
There was a problem hiding this comment.
Can we have more tests here? Maybe more messages and of different lengths: from 1 byte to 1024?
|
|
||
| size_t bufSize; | ||
|
|
||
| CHECK_OPENSSL(EVP_PKEY_get_raw_private_key(key, NULL, &bufSize) == 1); |
There was a problem hiding this comment.
Does EVP_PKEY_get_raw_private_key(key, NULL, &bufSize) always return the exact size? If yes, please document it here.
There was a problem hiding this comment.
It sets bufSize to the size and returns 1 on success. The == 1 check here is just to confirm that the method was successful.
Issue #, if available:
Description of changes:
Description:
This PR adds Ed25519 digital signatures. The relevant classes and functions are added as part of ACCP’s EVP Key, Signature, and Keyfactory classes. Ed25519 pre hash mode has not been implemented, as it is dependent on the JDK 15+ interfaces that we cannot currently access.
We are unable to use the NamedParameterSpec, EdECKey, EdECPrivateKey, and EdECPublicKey classes for our implementation, as ACCP must be compiled on JDK 8, and the EdDSA classes are only available on JDK versions 15+. As a result, the implementation utilizes the PrivateKey and PublicKey parent classes available in older java versions for Ed keys.
SignatureRaw from the EVP API was used for Ed25519 signatures, as this form of digital signatures does not support digest updates. The signing process can only take place after the entire message has been received (one-shot signing). EvpSignatureRaw matches this functionality (only updating the message via a java buffer and passing the complete message to native methods for signing at the end), whereas the standard EvpSignature allows for partial signing and verifying.
The pkcs82evp native method was updated to allow the ACCP key factory to import BouncyCastle (BC) private keys. BC encodes the EdDSA public key as an additional field in the PKCS8 encoding of the private key. This caused issues when importing the private key to ACCP, as the method used for the translation (d2i_PKCS8_PRIV_KEY_INFO) was unable to process the additional information. We switched the implementation to use d2i_PrivateKey instead, which successfully pulls the private key from the PKCS8 encoding, regardless of the additional field.
To fix versioning issues for JDK < 15, JUnit flags were used for tests, and a boolean checking the Java version was used to conditionally register Ed25519 in AmazonCorrettoCryptoProvider.java.
Testing:
New unit tests are included in EdDSATest.java. These include tests for invalid inputs, key generation, sign/verify functionality, and interoperability with the BC and SunEC providers.
To Do:
amazon-corretto-crypto-provider/tst/com/amazon/corretto/crypto/provider/test/KeyReuseThreadStormTest.java
Lines 38 to 45 in d71028c
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.