Add support for ML-DSA signatures - #535
Conversation
arckoor
left a comment
arckoor
left a comment
There was a problem hiding this comment.
Very hasty review only, but looks like the right direction.
- How were the keys for the tests generated? OpenSSL, or through rust-crypto, or something else entirely? (just curious)
- The JWK dance with serde I'm not a huge fan of, I would like to find a way to make that unnecessary
I used OpenSSL to generate the test keys. I also don't like the serialization stuff. I guess than can be fixed with either changing the "JwkWire" representation or the AKP params. I tend to keeping the alg member in the AKP parameters to be RFC compliant and to future proof. |
|
The I'll try to do a full review sometime next week. |
arckoor
left a comment
arckoor
left a comment
There was a problem hiding this comment.
Sorry this took so long.
I played around with the JwkWire thing for a while, I didn't find a nice way to get rid of it. However I would not duplicate the alg field between them, and instead deser the json into JwkWire, reassemble into Jwk and beforehand check if alg needs to be present
if let AlgorithmParameters::AlgorithmKeyPair(_) = &algorithm {
common.key_algorithm.ok_or_else(|| de::Error::missing_field("alg"))?;
}| let mut signature = vec![0u8; self.0.algorithm().signature_len()]; | ||
| let len = self.0.sign(msg, &mut signature).map_err(Error::from_source)?; | ||
| signature.truncate(len); |
There was a problem hiding this comment.
Why the truncate? The doc for .signature_length does not read like it provides a bound, but rather an exact number of bytes. If the resulting signature length is not equal to what .sign produces, that sounds like an issue (and should instead be handled with an Err()?)
There was a problem hiding this comment.
Ah, you're right, I'll remove it. I think that's left over from an older version of the function I wrote and later missed.
| round_trip_test!(round_trip_ml_dsa_44, MlDsa44, Algorithm::MLDSA44); | ||
| round_trip_test!(round_trip_ml_dsa_65, MlDsa65, Algorithm::MLDSA65); | ||
| round_trip_test!(round_trip_ml_dsa_87, MlDsa87, Algorithm::MLDSA87); |
There was a problem hiding this comment.
I'd still prefer no macros in tests please
| pub fn from_mldsa_components(pub_key: &str) -> Result<Self> { | ||
| let decoded = b64_decode(pub_key)?; | ||
| // The raw public key must be one of the fixed FIPS 204 sizes. | ||
| match decoded.len() { | ||
| ML_DSA_44_PUBLIC_KEY_LEN | ML_DSA_65_PUBLIC_KEY_LEN | ML_DSA_87_PUBLIC_KEY_LEN => {} | ||
| _ => return Err(new_error(ErrorKind::InvalidKeyFormat)), | ||
| } | ||
| Ok(DecodingKey { | ||
| family: AlgorithmFamily::Mldsa, | ||
| kind: DecodingKeyKind::SecretOrDer(decoded), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do people actually use this? For a bit of background, I'd like to refactor the EncodingKey and DecodingKey interfaces in the future and combine all the from_*_pem methods into a single from_pem and the like, and I'm not yet sure how many of the non-PEM methods will stay. If there is a legit usecase for this I don't mind it being added, but if it's just there to have it I would omit it until there is some demand for it
There was a problem hiding this comment.
I don't think so, people should generally use from_jwt or other functions. Internally from_jwt uses the from_*_components functions for other sigs, but I didn't use it for ML-DSA to check against the specific key sizes that are standardized in FIPS204 and identified by the ML-DSA variant in alg.
What's the best play here? I'd keep the size check since ML-DSA variants are only standardized for specific sizes not like RS256 for example.
| /// Edwards curve public key family. | ||
| Ed, | ||
| /// ML-DSA public key family. | ||
| Mldsa, |
There was a problem hiding this comment.
I don't mind, any way is fine. I opted for Mldsa to mirror the other members of AlgorithmFamily like Hmac.
| impl fmt::Display for Algorithm { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let s = match self { | ||
| Algorithm::HS256 => "HS256", | ||
| Algorithm::HS384 => "HS384", | ||
| Algorithm::HS512 => "HS512", | ||
| Algorithm::ES256 => "ES256", | ||
| Algorithm::ES384 => "ES384", | ||
| Algorithm::RS256 => "RS256", | ||
| Algorithm::RS384 => "RS384", | ||
| Algorithm::RS512 => "RS512", | ||
| Algorithm::PS256 => "PS256", | ||
| Algorithm::PS384 => "PS384", | ||
| Algorithm::PS512 => "PS512", | ||
| Algorithm::EdDSA => "EdDSA", | ||
| Algorithm::MLDSA44 => "ML-DSA-44", | ||
| Algorithm::MLDSA65 => "ML-DSA-65", | ||
| Algorithm::MLDSA87 => "ML-DSA-87", | ||
| }; | ||
| f.write_str(s) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Either do both fmt::Display for Algorithm and KeyAlgorithm like this, or use other => write!(f, "{:?}", other) here
2 participants
Closes #534
Summary
Adds ML-DSA signature support following FIPS 204 and RFC 9964.
aws_lc_rsandrust_cryptobackends.kty: "AKP"JWK import, export, and thumbprints.Implementation Decisions
ML-DSA uses the RFC 9964 wire names (
ML-DSA-44,ML-DSA-65, andML-DSA-87) while retaining regular Rust variants such asAlgorithm::MLDSA44.Signing uses the deterministic variant of ML-DSA with an empty context.
Variable
algexists in both the common JWK parameters and the AKP algorithm parameters as specified in RFC 9964. Serialization toJwkWireflattens them to one parameter which causes all sorts of problems when doing naively. I opted to introduce custom (de-) serialization logic that uses both locations. Serialization rejects conflicting values, and emits onealgmember. Deserialization populates the AKP algorithm parameter and the common parameter.Dependencies Notes
aws-lc-rscrate updated from 1.15 to 1.18 because the stable ML-DSA API is only available in 1.18.ml-dsausessignature3.x while this crate still directly usessignature2.2.Open to discussion on the implementation decisions and how dependencies like the 'signature' crate is handled especially since other issues / PRs mention this too.