search

corretto / amazon-corretto-crypto-provider

The Amazon Corretto Crypto Provider is a collection of high-performance cryptographic implementations exposed via standard JCA/JCE interfaces.
Apache License 2.0
238 stars 56 forks source link

Add Ed25519 DSA #394

Closed sp717 closed 3 months ago

sp717 commented 4 months ago

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:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

geedo0 commented 3 months ago

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.

geedo0 commented 3 months ago

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.

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?

sp717 commented 3 months ago

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.

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?

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.

geedo0 commented 3 months ago

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.