-
-
Notifications
You must be signed in to change notification settings - Fork 792
ML-KEM/ML-DSA part 4: ML-DSA #2459
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
base: master
Are you sure you want to change the base?
Conversation
Splitting up sfackler#2405 into a few parts as suggest by @alex. Adapting ML-DSA changes from sfackler#2405 to use the OsslParamBuilder changes over the last few PRs and to juggle lifetimes appropriately Co-authored-by: Jakub Jelen <[email protected]> Co-authored-by: Justus Winter <[email protected]>
openssl/src/pkey_ml_dsa.rs
Outdated
} | ||
} | ||
|
||
pub struct PKeyMlDsaBuilder<'a, 'opb, T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a builder? There's exactly 2 APIs of consequnce: generate a new key, and load one from a seed.
Ok(NonceType(nonce_type)) | ||
} | ||
|
||
/// Initializes a conversion from `OsslParam` to `PKey` on given `PkeyCtx`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a really wacky API, and I don't even understand the theory of it. Why is fromdata
an operation you'd do with an EVPK_PKEY_CTX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs:
EVP_PKEY_fromdata_init() initializes a public key algorithm context for creating a key or key parameters from user data
So if we're going to pass a param builder in, then we have to call this first it looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel almost stupid for asking this, but why is an EVP_PKEY_CTX even involved in creating an EVP_PKEY? This is the complete opposite of how EVP_PKEY normally are (where you have a key and then you uuse the CTX to do operations with it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use EVP_PKEY_Q_keygen
for quick key generation, but normally EVP_PKEY_CTX is a structure keeping the operational context so it is involved.
openssl/src/pkey_ctx.rs
Outdated
|
||
/// Generates a new public/private keypair. | ||
/// | ||
/// New OpenSSL 3.0 function that should do the same thing as keygen() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wat.
if they're behaviorally identical, a) why did OpenSSL expose it, b) can we just change what API keygen() uses on OpenSSL 3+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this comment is not accurate. This performs either paramgen or keygen depending on context. I'll update the comment. Either is an option.
EVP_PKEY_paramgen() and EVP_PKEY_keygen() do exactly the same thing as EVP_PKEY_generate(), after checking that the corresponding EVP_PKEY_paramgen_init() or EVP_PKEY_keygen_init() was used to initialize ctx. These are older functions that are kept for backward compatibility. It is safe to use EVP_PKEY_generate() instead
I agree that we could trim this down a bit. I was trying to be more or less faithful to the original pull request to start with. I’ll work on it.On Sep 9, 2025, at 15:20, Alex Gaynor ***@***.***> wrote:
@alex commented on this pull request.
In openssl/src/pkey_ml_dsa.rs:
+ Variant::MlDsa44 => MLDSA44_STR,
+ Variant::MlDsa65 => MLDSA65_STR,
+ Variant::MlDsa87 => MLDSA87_STR,
+ }
+ }
+
+ pub(crate) fn as_cstr(&self) -> &'static CStr {
+ match self {
+ Variant::MlDsa44 => MLDSA44_CSTR,
+ Variant::MlDsa65 => MLDSA65_CSTR,
+ Variant::MlDsa87 => MLDSA87_CSTR,
+ }
+ }
+}
+
+pub struct PKeyMlDsaBuilder<'a, 'opb, T>
Why do we need a builder? There's exactly 2 APIs of consequnce: generate a new key, and load one from a seed.
In openssl/src/pkey_ctx.rs:
@@ -913,6 +990,14 @@ impl<T> PkeyCtxRef<T> {
}
Ok(NonceType(nonce_type))
}
+
+ /// Initializes a conversion from `OsslParam` to `PKey` on given `PkeyCtx`.
This seems like a really wacky API, and I don't even understand the theory of it. Why is fromdata an operation you'd do with an EVPK_PKEY_CTX?
In openssl/src/pkey_ctx.rs:
@@ -889,6 +952,20 @@ impl<T> PkeyCtxRef<T> {
Ok(())
}
+ /// Generates a new public/private keypair.
+ ///
+ /// New OpenSSL 3.0 function that should do the same thing as keygen()
wat.
if they're behaviorally identical, a) why did OpenSSL expose it, b) can we just change what API keygen() uses on OpenSSL 3+?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
@alex updated; sorry for the delay, but have been swamped lately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have a test case or two using a fixed seed from known vectors.
Ok(NonceType(nonce_type)) | ||
} | ||
|
||
/// Initializes a conversion from `OsslParam` to `PKey` on given `PkeyCtx`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel almost stupid for asking this, but why is an EVP_PKEY_CTX even involved in creating an EVP_PKEY? This is the complete opposite of how EVP_PKEY normally are (where you have a key and then you uuse the CTX to do operations with it)?
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: OsslParamArray::from_ptr(params), | ||
_m: PhantomData, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What, if any, are the lifetime constraints an OSSL_PARAM
from EVP_PKEY_todata
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are allocated on export and should be explicitly freed.
impl Signature { | ||
/// Creates a new `Signature` for use with ML-DSA. | ||
#[cfg(ossl350)] | ||
pub fn for_ml_dsa(variant: crate::pkey_ml_dsa::Variant) -> Result<Signature, ErrorStack> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, what's the use case for this being public?
|
||
/// Returns the Private ML-DSA PKey from the provided parameters. | ||
/// Creates a new `PKeyMlDsaBuilder` to generate a new ML-DSA key | ||
/// pair. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is out of date
} | ||
|
||
/// Generate an ML-DSA PKey. | ||
#[corresponds(EVP_PKEY_fromdata)] |
There was a problem hiding this 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 this corresponds is particularly useful, it's too generic. We can drop it. (Same for all others like this)
impl PKeyMlDsaParams<Public> { | ||
/// Creates a new `PKeyMlDsaParams` from existing Public ML-DSA PKey. | ||
#[corresponds(EVP_PKEY_todata)] | ||
pub fn from_pkey<S>(pkey: &PKey<S>) -> Result<PKeyMlDsaParams<Public>, ErrorStack> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the pkey using functions seem to actually verify the type of pkey?
Also, this inverts the way these APIs usually work. Usually there's a PKey method to get the type-specific reprsentation.
Splitting up #2405 into a few parts as suggest by @alex.
Adapting ML-DSA changes from #2405 to use the OsslParamBuilder changes over the last few PRs and to juggle lifetimes appropriately