Add bindings for ML-DSA#2519
Conversation
Splitting up rust-openssl#2405 into a few parts as suggest by @alex. Adapting ML-DSA changes from rust-openssl#2405 to use the OsslParamBuilder changes over the last few PRs and to juggle lifetimes appropriately Co-authored-by: Jakub Jelen <jjelen@redhat.com> Co-authored-by: Justus Winter <justus@sequoia-pgp.org>
alex
left a comment
There was a problem hiding this comment.
(I only started reviewing, didn't finish, but wanted to leave these since I won't get back to this for a bit)
| /// Creates a private key from its raw byte representation using a string keytype | ||
| #[corresponds(EVP_PKEY_new_raw_private_key)] | ||
| #[cfg(ossl300)] | ||
| pub fn private_key_from_raw_bytes_ex( |
There was a problem hiding this comment.
is there anyways to consolidate this with the existing private key from bytes?
There was a problem hiding this comment.
We could refactor Id to have a string internally and then change private_key_from_raw_bytes to use the _ex method for all key types. I'm not sure if this will have larger implications though (and it would be a larger refactoring effort).
As I understand it the supported key types from private_key_from_raw_bytes_ex are a superset of private_key_from_raw_bytes.
There was a problem hiding this comment.
Also EVP_PKEY_new_raw_public_key_ex was only introduced in ossl300, while EVP_PKEY_new_raw_public_key exists since ossl110.
|
Checking back in on this. I should have some capacity for rust-openssl now, and wanted to see if I could assist with the ML-DSA or ML-KEM work? |
|
@alex Do you have a rough estimate when you will get time to review the PR? I’m not trying to rush you, just trying to get a sense of timing so I can organize my work on |
|
I do not, and I apologize for that. Very candidly the API choices and complexity that OpenSSL have introduced have really sapped my motivation to engage on this part of the codebase. Hopefully one of the other maintainers feels differently. |
|
Understood. I'm not a maintainer, but I'm happy to assist if I can with reviewing or code. |
|
I won't have time for this anytime soon, but I can try to take a look when I get some time. I should say that I'm most likely the least interested in OpenSSL 3 among the maintainers - I don't use it. |
|
@botovq Thank you, I just pinged because you were the first maintainer I could find by looking at the recently merged PRs. If there are other maintainers (the |
|
@alex I just read https://cryptography.io/en/latest/statements/state-of-openssl/, which explicitly mentions ML-KEM and ML-DSA. Do you want me to close this (and the ML-KEM) PR for now? I would also be happy to help out with any changes necessary to get these algorithms working for LibreSSL/BoringSSL/AWS-LC, if required. |
|
No need to close this -- hopefully it provides some color though. (I expect we'll eventually land ML-DSA/ML-KEM for OpenSSL, but we won't block getting the other implementation on having an OpenSSL one.) |
|
@FinnRG, @alex: I have pushed my pyca changes that add ML-DSA, ML-KEM, and hybrid KEMs to https://github.com/abbra/cryptography/tree/abbra-add-pqc. It requires both @FinnRG pull requests to rust-openssl to build. Let me know if I should open a PR to https://github.com/pyca/cryptography or it should wait until these two PRs merged to rust-openssl. |
|
I also pushed a generated documentation to https://abbra.fedorapeople.org/.draft/pyca-pqc/hazmat/primitives/asymmetric/mldsa44.html |
|
Here is an upstream CI workflow running: https://github.com/abbra/cryptography/actions/runs/21213933260. It is mostly green, except documentation spell-checking which cannot handle encapsulate/decapsulate. And there is a bug in Windows openssl binary as it is misconfigured and cannot read files. |
| fn is_key_type(&self, key_type: &str) -> Result<bool, ErrorStack> { | ||
| let key_type = CString::new(key_type).unwrap(); |
There was a problem hiding this comment.
Any reason to not take &CStr and avoid an allocation?
There was a problem hiding this comment.
To do this neatly and consistently is a bigger change than it seems at first. I will have a go at it and see if the juice is worth the squeeze.
|
Hi @FinnRG and all, Based on this PR and @abbra's experimental branch, I have pushed this work a bit further including adding BoringSSL support. I am in the process of cleaning up the code and commit history and will prepare a new PR(s) soon. Fixup commits, and in some cases more substantive commits, for the sake of clarity and ease of reviewing, will be squashed down into the relevant preceding commits. In cases where commit authorship differs, I acknowledge the contributions via In line with expressed priorities of python-cryptography upstream, I also intend to reorder the work somewhat to ensure BoringSSL support comes before OpenSSL. And certainly ML-DSA before ML-KEM. |
Based on #2459. I addressed all review comments and refactored the key generation API.
For example
pkey_ml_dsa::new_generate(variant)is now a method on PKey:PKey::generate_mldsa(variant). This is more in line with the other key types.I had to extend
openssl-syswith various new methods, becauseML-DSAdoesn't have a NID and needs to be handled with new methods likeEVP_PKEY_new_raw_private_key_ex,EVP_PKEY_new_raw_public_key_exandEVP_PKEY_CTX_new_from_name.I've also added a few new tests based on boringssl test vectors.