ML-KEM/ML-DSA part 2: param builder #2451

swenson · 2025-08-29T04:08:14Z

Splitting up #2405 into a few parts as suggest by @alex.

This adds the param-builder.

Original commit message:

Add internal module to simplify working with OSSL_PARAM structure We discussed that this API is not well suitable for the end users but still, it required for several operations in OpenSSL 3.* so instead of calling to FFI for every use of this API, this introduces simple wrappers that allow building of the params and their usage.

alex · 2025-08-29T04:09:40Z

If you can put the openssl-sys changes into their own PR, it makes it considerably easier to review. thanks.

swenson · 2025-08-29T04:10:46Z

No problem.

swenson · 2025-08-29T04:16:14Z

@alex this is just the param builder and the related Argon changes now, I believe.

alex · 2025-08-29T04:17:04Z

Will look tomorrow, thanks.

…

On Fri, Aug 29, 2025 at 12:16 AM Christopher Swenson < ***@***.***> wrote: *swenson* left a comment (sfackler/rust-openssl#2451) <#2451 (comment)> @alex <https://github.com/alex> this is just the param builder and the related Argon changes now, I believe. — Reply to this email directly, view it on GitHub <#2451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBB6QUDQV46UYFDOOL33P7H2JAVCNFSM6AAAAACFDKWLTGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMZVGY3DAOJQGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

alex · 2025-08-29T18:04:15Z

Merge conflicts, sorry :-(

@alex

Splitting up sfackler#2405 into a few parts as suggest by @alex. This adds the param-builder and other openssl-sys changes. Original commit message: Add internal module to simplify working with OSSL_PARAM structure We discussed that this API is not well suitable for the end users but still, it required for several operations in OpenSSL 3.* so instead of calling to FFI for every use of this API, this introduces simple wrappers that allow building of the params and their usage. Signed-off-by: Jakub Jelen <[email protected]> Co-authored-by: Justus Winter <[email protected]>

swenson · 2025-08-29T18:15:48Z

@alex no worries, rebased

openssl/src/ossl_param.rs

alex · 2025-08-29T22:11:11Z

openssl/src/ossl_param.rs

+    pub fn locate(&self, key: &[u8]) -> Result<&OsslParamRef, ErrorStack> {
+        unsafe {
+            let param = cvt_p(ffi::OSSL_PARAM_locate(
+                self.as_ptr(),


This isn't correct either -- OSSL_PARAM_locate takes a pointer to an array of params that is null terminated.

It's not quite as bad as that, thankfully -- it looks like it should be terminated with a param with a null key, but the param itself should not be null AFAICT:

https://github.com/openssl/openssl/blob/master/include/openssl/core.h#L83

So, I think this array of params will always be okay in Rust?

Thinking about this more, this presents a problem I don't see elsewhere in this code base: this conflates a &OsslParam with an array of OsslParam, which to C is the same, but in Rust is a very different structure.

So, while this code is fine as-is if we assume the &OsslParam comes from OsslParamBuilder, which will always correctly build an array of OsslParams.

Currently there is no other way to construct an OsslParam since it has a private tuple element, so someone could not, in safe Rust, construct an OsslParam without going through OsslParamBuilder.

However, there are some safety concerns with the to_param method -- while it says it is returning a Result<OsslParam, ErrorStack>, in reality it is returning Result<&mut [OsslParam; ?], ErrorStack, which we are then calling from_ptr on the first element of.

I think this is problematic if the Rust compiler decides to move the returned OsslParam, since it won't know that it is secretly an array. We could maybe solve this with Pin to prevent the compiler from moving the OsslParam, but that might be papering over the problem.

I don't think the current foreign_types crate is capable of encapsulating these semantics.

So, I see a few options here, but I'm open to suggestions:

Make these pub(crate) only so that they can't be used by users directly, but only for our convenience when we can be sure of their safety, possibly when combined with

Changing basically everything to unsafe so that we have to always think about our usage of this array type

Something something Pin? I feel like this doesn't really solve the underlying problem

Deleting this code since it isn't really safe Rust.

Converting the result of to_param to a proper Vec<OsslParam> with an OsslParam that is safe and sound, and doing a just-in-time conversion of that to an *OsslParam (array) pointer that has an element at the end with a null key when we call locate(). This is safe, but it could be problematic if the underlying OpenSSL C code ever decides in a future version to change how the array is terminated.

Try to wrangle a foreign_type! macro that lets us make some sense of this array / reference mangling.

I'm happy to move forward with whichever path you think makes the most sense, or if we have some precedent I don't know about, I'm happy to follow that.

Seeing how the original PR uses these makes me even more worried, since the compiler could be moving these arrays around, e.g.,

let mut params: *mut ffi::OSSL_PARAM = ptr::null_mut(); cvt(ffi::EVP_PKEY_todata(pkey.as_ptr(), selection, &mut params))?; Ok(PKeyMlDsaParams::<T> { params: OsslParam::from_ptr(params), _m: ::std::marker::PhantomData, })

Another option: we don’t have a bare OsslParam but instead have a wrapper struct that only has *OsslParam, so that the compiler doesn’t “know” about the bare array and try to move it.

Nevermind -- I realize now my understanding foreign_types was incorrect -- it isn't type OsslParam = OSSL_PARAM, but more struct OsslParam(*OSSL_PARAM), so it is already wrapping a pointer.

I think after all, the code as-is is probably sound. Sorry for the PR noise as I thought out loud and please correct me if I'm wrong.

Fundamentally, I don't think it's correct for us to confuse OSSL_PARAM * (pointer to a single OSSL_PARAM) with OSSL_PARAM * (pointer to an array of them). This makes the invariants incredibly fragile -- we need distinct types for representing a single OSSL_PARAM and representing an array of them. No other place in rust-openssl treats them this way, and it means we get things like destructors wrong.

I agree. Obviously this bothers me too. Let me think about this a little to come up with a way to represent this. (Open to suggestions.)

I renamed this to OsslParamArray to alleviate some of the confusion -- these params are always allocated as an array, and OSSL_PARAM_free, which we call in our Drop, will correctly free the entire array.

I think this is correct now. I added clarifying comments as well.

openssl/src/ossl_param.rs

…which is not available yet Use more precise dead code annotations

alex · 2025-09-01T00:06:03Z

openssl/src/ossl_param.rs

+        }
+    }
+
+    /// Get `BigNum` from the current `OsslParamArray`


What does this mean? I think this operation only makes sense in the context of a single OSSL_PARAM?

You're right. I'll clarify these comments.

openssl/src/ossl_param.rs

…am elements

alex · 2025-09-01T02:02:42Z

openssl/src/ossl_param.rs

+    /// Get `BigNum` from the current OSSL_PARAM at the top of this
+    /// `OsslParamArray` reference.


I'm not sure this is a useful behavior -- I think we really need separate types for these.

(I think it logically follows that locate has the wrong return type ATM.)

Gotcha. I’ll think on that.

I separated out the overall OSSL_PARAM array and added a separate struct for an interior individual OSSL_PARAM struct (wrapping an internal reference tied to the lifetime over the entire array). I think this should be cleaner.

…nterior element

swenson force-pushed the pqc-param-builder branch from 8720778 to 935f6cf Compare August 29, 2025 04:14

swenson force-pushed the pqc-param-builder branch from 935f6cf to 69182d1 Compare August 29, 2025 18:15

alex reviewed Aug 29, 2025

View reviewed changes

swenson added 2 commits August 29, 2025 19:02

Use &CStr instead of &[u8] for keys; builder should be mutable

00e12f7

Remove allow dead code from param builder

131cddd

swenson force-pushed the pqc-param-builder branch from 643db6e to 131cddd Compare August 30, 2025 02:21

swenson added 2 commits August 29, 2025 19:38

Use CStr::from_bytes_with_nul_unchecked instead of c-string literal, …

6e4440f

…which is not available yet Use more precise dead code annotations

Rename OsslParam to OsslParamArray and add clarifying comments

7642cc6

alex reviewed Sep 1, 2025

View reviewed changes

Make OsslParam and friends pub(crate); clarify comments about OsslPar…

99520b0

…am elements

alex reviewed Sep 1, 2025

View reviewed changes

Create separate type for the OSSL_PARAM array and a reference to an i…

20091b3

…nterior element

		/// Get `BigNum` from the current OSSL_PARAM at the top of this
		/// `OsslParamArray` reference.

Uh oh!

ML-KEM/ML-DSA part 2: param builder #2451

Are you sure you want to change the base?

ML-KEM/ML-DSA part 2: param builder #2451

Uh oh!

Conversation

swenson commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex commented Aug 29, 2025

Uh oh!

swenson commented Aug 29, 2025

Uh oh!

swenson commented Aug 29, 2025

Uh oh!

alex commented Aug 29, 2025 via email

Uh oh!

alex commented Aug 29, 2025

Uh oh!

swenson commented Aug 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swenson Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swenson commented Aug 29, 2025 •

edited

Loading

swenson Aug 30, 2025 •

edited

Loading