Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support "raw" format for XEC KeySpecs. #1187

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Conversation

prbprbprb
Copy link
Collaborator

XdhKeySpec now implements "raw" EncodedKeySpec, allowing better interoperability between a Conscrypt client API and a difference Conscrypt implementation.

As a precursor to this, added missing tests for XDH keys and key factories and tidied the implementation.

@prbprbprb
Copy link
Collaborator Author

prbprbprb commented Dec 11, 2023

NB tests for XECPublicKeySpec (and private) still incomplete and difficult to do under Gradle, will complete them on AOSP.

Copy link
Contributor

@davidben davidben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand Java and JCA enough to review this, so deferring to other reviewers unless there's something in particular you'd like me to look at.

@prbprbprb
Copy link
Collaborator Author

I don't think I understand Java and JCA enough to review this, so deferring to other reviewers unless there's something in particular you'd like me to look at.

Yeah sorry, I should have mentioned, 90% of the change is just tests and the raw EncodedKeySpec stuff, but was looking for your insights on the key detail parts you already commented on (thanks!).

Copy link
Contributor

@davidben davidben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JNI bits lgtm

common/src/jni/main/cpp/conscrypt/native_crypto.cc Outdated Show resolved Hide resolved
@@ -352,12 +352,14 @@ int throwForEvpError(JNIEnv* env, int reason, const char* message,
case EVP_R_MISSING_PARAMETERS:
case EVP_R_INVALID_PEER_KEY:
case EVP_R_DECODE_ERROR:
case EVP_R_NOT_A_PRIVATE_KEY:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, you will never get this error in the function you've added. This error exists because OpenSSL uses a single type for public and private keys, and so we have to rely on a runtime check. But EVP_parse_private_key will never give you a public key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I did contrive a scenario for this, because the current XDH usage I have seen is apps storing raw XEC keys, so it's not entirely infeasible that a bug or user error causes them to accidentally wrap public key with a PKCS#8 preamble and later try to decode it.

Tracing the x25519 part of the code, it looks like that would get at least as far as x25519_set_priv_raw and X25519_public_from_private (which would presumably generate rubbish) but then I guess the EVP code would error out?

prbprbprb and others added 3 commits December 14, 2023 11:51
XdhKeySpec now implements "raw" EncodedKeySpec, allowing
better interoperability between a Conscrypt client API
and a difference Conscrypt implementation.

As a precursor to this, added missing tests for XDH
keys and key factories and tidied the implementation.
@prbprbprb prbprbprb merged commit 79b06e5 into google:master Dec 14, 2023
5 checks passed
@prbprbprb prbprbprb deleted the xdh_keys branch December 14, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants