Skip to content

New OpenSSL 3.* API for managing EVP_PKEY objects #2368

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

Closed
wants to merge 1 commit into from

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Feb 14, 2025

The OpenSSL 3.* users now do not have a way to use non-deprecated API by using this rust bindings, which is not sustainable in the long term as either distributions will stop building with the deprecated API or it will be eventually removed.

This is now mostly PoC on using RSA and ECDSA keys using the new API in tests. It does not expose all possible API that are available as I did not have a good way to test the unused API yet.

I do not know if this API is available in some other *SSL libraries right now so for now all of the additions are marked with #[cfg(ossl300)].

This is partially based on #2051 which was abandoned.

Opening as a draft. I tried to follow existing logic in the project, but I might have missed some hints. Additionally, there are few TODO comments that point out to possible problematic API, where I would be happy to hear if that is acceptable or if we should try to figure out a better interface and how it should look like.

Fixes: #2047

@Jakuje Jakuje force-pushed the openssl-3-evp_pkey branch 2 times, most recently from 11c520b to 2ce1b9b Compare February 14, 2025 21:09
@Jakuje Jakuje marked this pull request as ready for review February 27, 2025 10:14
@Jakuje Jakuje force-pushed the openssl-3-evp_pkey branch 2 times, most recently from a7d1739 to 7d520ac Compare February 28, 2025 14:31
The OpenSSL 3.* users now do not have a way to use non-deprecated
API by using this rust bindings, which is not sustainable in the
long term as either distributions will stop building with the
deprecated API or it will be eventually removed.

This is now mostly PoC on using RSA and ECDSA keys using the new
API in tests. It does not expose all possible API that are available
as I did not have a good way to test the unused API yet.

I do not know if this API is available in some other *SSL libraries
right now so for now all of the additions are marked with #[cfg(ossl300)].

This is partially based on sfackler#2051 which was abandoned.

Fixes: sfackler#2047
@Jakuje Jakuje force-pushed the openssl-3-evp_pkey branch from 7d520ac to fec04ce Compare February 28, 2025 14:41
@Jakuje
Copy link
Contributor Author

Jakuje commented Mar 4, 2025

For the record, there is a draft MR for sequoia to use the new API: https://gitlab.com/sequoia-pgp/sequoia/-/merge_requests/1765

There are still some rough edges that will need to be changed:

  • The selection parameter to todata() function needs some constant in rust-openssl (or export from -sys?)
  • The key parameters require trailing null bytes and need to exist until the build is completed. It would be better to expose the OSSL_PARAM_* constants from OpenSSL, but there is hundreds of them
  • The value parameters for OSSL_PARAMS objects need to exist until the to_params() is called. This is not not expressed in the API and rust gc could remove them without us storing the object somewhere or introducing lifetimes. I need to explore this direction, but would be happy to hear your thoughts

@alex
Copy link
Collaborator

alex commented Mar 5, 2025

Simo asked me if I could take a look at this and share some thoughts as to whether this was reasonable. That's what I just did (i.e., I looked at a high level at the new APIs, I didn't really focus on their implementation.)

Unfortunately, I think my response is not going to make anyone happy.

OSSL_PARAM was a catastrophically bad design choice by the OpenSSL team. I don't know what motivated them to choose that path (though I'm guessing it was ABI stability), but it was not a good path.

For that reason, I'm incredibly loathe to expose it as a public API in rust-openssl. In addition to just being shitty, impossible to document, and error prone, it also (inevitably?) leads to memory corruption issues (e.g. openssl/openssl#22629). Thus far, rust-openssl APIs that require OSSL_PARAM handle it purely internally (see, e.g., argon2 or rfc6979).

Unfortunately, however, OpenSSL has decided OSSL_PARAM is the direction they're going for all new APIs. And the vast majority of this project's API surface is doing the bare minimum to make OpenSSL's APIs comport to Rust's safety requirements. The combination of which suggests this PR is basically the correct direction.

However, merely looking at this makes me want to have nothing to do with OpenSSL. It's not your fault, they just made a bunch of really bad choices, and now I'd much rather put my energy into using some other cryptography library.

If there's some direction that allows OSSL_PARAM to remain entirely internal to this project, and not a public API for users, that would be my strongest possible preference.

If, however, that's not possible, and supporting all of OpenSSL's new APIs (and removal of old ones) effectively requires making these public APIs for rust-openssl, then that's probably my cue that it's time to move on.

Sorry, it's not for your fault, but unfortunately, those are the consequences of OpenSSL's decisions.

@Jakuje
Copy link
Contributor Author

Jakuje commented Mar 5, 2025

Thank you for the comment @alex! This was first shot and I agree that this API might not be well suitable for the rust-openssl, see the notes I highlighted in the previous comments.

The option to bury the OSSL_PARAMS in internal functions was one of the other options I wanted to explore. It will give much less flexibility and will need separate objects for building (and disassembling) all different key types we have now (and new PQC are coming), which is something I wanted to avoid initially, but I agree that it should give more sensible API that would be more similar to the RsaPrivateKeyBuilder (unfortunately not the same as we would not like to touch the low-level RSA objects).

I will try to experiment with one key type to see what can be done here.

@Jakuje
Copy link
Contributor Author

Jakuje commented Mar 5, 2025

I did give a try to rewrite the RSA operations in #2380. It will likely need some more cleanup, but the basic tests seems to work, I am able to construct the RSA key from parameters, generate keys and extract parameters from the PKey structures like this. The structure is mostly based on the openssl/src/rsa.rs. I will likely need to split up the builder+generator+disassembler to be less confusing, but would such approach be something acceptable @alex ?

@Jakuje
Copy link
Contributor Author

Jakuje commented Mar 13, 2025

Closing this one as this does not look like going to be acceptable. Follow-up discussion is in #2380.

@Jakuje Jakuje closed this Mar 13, 2025
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.

Fixing use of OpenSSL 3 deprecated functions
2 participants