-
Notifications
You must be signed in to change notification settings - Fork 946
Add support for MLDSA in hsmtool #29202
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
Updates the cryptoki crate to version 0.12.0 and cryptoki-sys to 0.5.0. Removes local patches that are now upstreamed and updates the library initialization code to match the new API. Signed-off-by: Willy Zhang <[email protected]>
| // Data is a slice of plaintext: hash. | ||
| SignData::Slice(a, b) => Self::data_plain_text(&input[*a..*b]), | ||
| }, | ||
| KeyType::MlDsa => match self { |
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.
Does ML-DSA have a notion of input types categorized into signing domains like SLH-DSA does?
If no, then this is the correct way to handle the data preparation.
If yes, then we may need something a bit like the spx_prepare function below.
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.
Yes there is a "pure" and "prehash" version of MLDSA as well. I updated with mldsa_prepare below.
|
It would be nice to add some tests for MLDSA into //sw/host/hsmtool/tests. |
9147cb3 to
7be7c25
Compare
Adds commands to generate, export, sign, and verify using the MLDSA algorithm (defaulting to ML-DSA-87). Includes support for exporting CSRs with the correct ML-DSA-87 OID. Signed-off-by: Willy Zhang <[email protected]>
7be7c25 to
7ceb7c3
Compare
Yeah, I'll file an issue to add tests for MLDSA, currently softhsmv2 MLDSA is still in a PR softhsm/SoftHSMv2#809 (hopefully merged soon). I think it makes most sense to update the softhsmv2 dependency once MLDSA support is merged. I only need this change so i can test opentitan-provisioning locally against the softhsmv2 fork. |
| /// MLDSA algorithm type (e.g. 3 for MLDSA 87). | ||
| #[arg(long, default_value = "3")] | ||
| #[serde(default = "default_mldsa_type")] | ||
| mldsa_type: u64, |
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.
Would it make sense for this type to be enum so that the CLI argument can be a string like mldsa_87 which is much more readable than 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.
BTW, you could probably accomplish this rather easily using clap's ValueEnum:
#[repr(u64)]
#[derive(Copy, Clone, Default, Debug, ValueEnum)]
pub enum MlDsaAlgorithmn {
MlDsa44 = 1, // I'm guessing at these values
MlDsa65= 2,
MlDsa87 = 3,
}
// Then, in the args:
#[arg(long, default_value="MlDsa87", value_enum)]
mldsa_type: MlDsaAlgorithm,
// Then, when you need it as an integer, cast with `as`:
public_template.insert(AttributeType::ParameterSet, AttrData::from(self.mldsa_type as u64));
| let object = helper::find_one_object(session, &attrs)?; | ||
|
|
||
| let data = fs::read(&self.input)?; | ||
| let data = self.format.prepare(KeyType::MlDsa, &data)?; |
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.
In the mldsa command dispatch, any prepare should be replaced with mldsa_prepare. You'll need to accept the ML-DSA domain as a command line argument.
You might also need the domain when you create the key. To complicate matters more, different PKCS#11 implementations will probably have different behaviors around when you need the domain.
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.
Ah sorry, I understand how that worked well enough. I updated all of the sign, validate and generate commands to call mldsa_prepare.
If i understand correctly, for sphincsplus you always set the domain for all use cases, did that work for all of the HSM's you tested against?
| if self.little_endian { | ||
| result.reverse(); | ||
| } |
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.
We may not actually need to worry about endianness for MLDSA (ie: we don't for slh-dsa).
The RSA and ECDSA algorithms treat their key material and signatures like integers, and thus, the byte ordering is an important consideration in the implementation on the target device (e.g. OpenTitan Earlgrey is a little-endian machine and wants integers in that order, whereas cryptographers and standards authors want to write integers in big-endian order).
I expect that ML-DSA (like SLH-DSA) treats key material and signatures as byte buffers. In that case, the endianness is meaningless. I'd suggest eliminating the endianness consideration for any command in the mldsa hierarchy.
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.
Makes sense. Removed all of the little_endian logic.
| /// MLDSA algorithm type (e.g. 3 for MLDSA 87). | ||
| #[arg(long, default_value = "3")] | ||
| #[serde(default = "default_mldsa_type")] | ||
| mldsa_type: u64, |
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.
BTW, you could probably accomplish this rather easily using clap's ValueEnum:
#[repr(u64)]
#[derive(Copy, Clone, Default, Debug, ValueEnum)]
pub enum MlDsaAlgorithmn {
MlDsa44 = 1, // I'm guessing at these values
MlDsa65= 2,
MlDsa87 = 3,
}
// Then, in the args:
#[arg(long, default_value="MlDsa87", value_enum)]
mldsa_type: MlDsaAlgorithm,
// Then, when you need it as an integer, cast with `as`:
public_template.insert(AttributeType::ParameterSet, AttrData::from(self.mldsa_type as u64));
| } | ||
|
|
||
| #[derive(clap::Args, Debug, Serialize, Deserialize)] | ||
| pub struct Generate { |
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.
Do you need to know the ML-DSA domain when you create the key (same goes for import)?
As I recall, the CloudKMS backend for SLH-DSA does need to know the domain at key creation time. I don't know if other HSM implementations have the same restrictions.
When I wrote the sphincsplus implementation for the Nitrokey token (keys stored on token, computation done by the host), I only need to know the domain at sign/verify time (whereas CloudKMS needs it at creation time).
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'm not 100% sure i've only tested against softhsm so far. For now i'll do something similar to sphincsplus, and we'll adjust as we test against hsms
Signed-off-by: Willy Zhang <[email protected]>
Bumps up cryptoki dep version and adds MLDSA support. Tested against fork of softhsmv2 with MLDSA support on the opentitan-provisioning repo.