Add support for Ed25519 to NativeCrypto #1287
Conversation
BoringSSL doesn't support Ed25519 with EVP_DigestUpdate, only with EVP_DigestSign and EVP_DigestVerify. So we need to add wrappers of these functions to NativeCrypto.
|
Test org.conscrypt.java.security.cert.X509CertificateTest.utcTimeWithOffset fails. I don't see how this is related to my changes. |
|
Yeah, that's a BoringSSL change, I'll ping them. |
| if (md == nullptr && (EVP_PKEY_id(pkey) != EVP_PKEY_ED25519)) { | ||
| JNI_TRACE("ctx=%p %s => md == null", mdCtx, jniName); |
There was a problem hiding this comment.
n00b question: Why is this?
There was a problem hiding this comment.
For all other key types, md must not be null. But for Ed25519, it must always be null, see:
https://docs.openssl.org/3.0/man7/EVP_SIGNATURE-ED25519/#ed25519-and-ed448-signature-parameters
or
https://github.com/google/boringssl/blob/master/include/openssl/evp.h#L239
Maybe I should make this check stronger, and reject a non-null md for ed25519.
There was a problem hiding this comment.
Understood. Maybe just add a comment so the next person along understands :)
There was a problem hiding this comment.
Also raises the question of what we do in SignatureSpi... Have update() cache the data and do a one-shot operation on sign() or verify(), I guess?
There was a problem hiding this comment.
Yes, I think in SignatureSpi, we have to keep a copy somewhere. My plan is to do it in a similar way as in OpenSSLSignatureRawECDSA.java.
prbprbprb
left a comment
prbprbprb
left a comment
There was a problem hiding this comment.
Overall LGTM but I want to re-check my working later on a couple of bits and maybe ask davidben to give it a quick once over in case we're missing anything on the BoringSSL side.
|
Is the intention to then call this from the higher level Java bits, just in a separate PR? (My understanding was that NativeCrypto was not public API in Conscrypt.) |
Exactly! Next steps would would be to wire up Java implementations of Further down the road we should make this (and X25519) able to use the RI interface and key classes, but there's a whole conditional compilation issue there as those classes don't exist on many of the platforms we need to support. |
| if (md == nullptr && (EVP_PKEY_id(pkey) != EVP_PKEY_ED25519)) { | ||
| JNI_TRACE("ctx=%p %s => md == null", mdCtx, jniName); |
There was a problem hiding this comment.
Also raises the question of what we do in SignatureSpi... Have update() cache the data and do a one-shot operation on sign() or verify(), I guess?
|
|
||
| size_t array_size = static_cast<size_t>(env->GetArrayLength(inJavaBytes)); | ||
| if (ARRAY_CHUNK_INVALID(array_size, inOffset, inLength)) { | ||
| conscrypt::jniutil::throwException(env, "java/lang/ArrayIndexOutOfBoundsException", |
There was a problem hiding this comment.
Not this CL but we should add a method for this as it gets thrown in a many places so we might as well cache the class.
|
|
||
| if (outPublic.size() != ED25519_PUBLIC_KEY_LEN) { | ||
| conscrypt::jniutil::throwException(env, "java/lang/IllegalArgumentException", | ||
| "Output public key array length != 32"); |
There was a problem hiding this comment.
conscrypt::jniutil::throwIllegalArgumentException()
| } | ||
|
|
||
| if (outPrivate.size() != ED25519_PRIVATE_KEY_LEN) { | ||
| conscrypt::jniutil::throwException(env, "java/lang/IllegalArgumentException", |
3 participants
BoringSSL doesn't support Ed25519 with EVP_DigestUpdate, only with EVP_DigestSign and EVP_DigestVerify. So we need to add wrappers of these functions to NativeCrypto.