Signature traits #1080

wysiwys · 2025-07-23T09:45:19Z

This PR implements the Signature traits.

keygen()
sign()
verify()

Resolves #1034

Includes implementations for:

ed25519 (arrayref, slice, owned)
libcrux-ml-dsa (owned)
ecdsa (arrayref, slice, owned)

keks

I think this goes in a good direction, nice!

I am not sure what to best do with the Aux, it looks like it could be confusing to users. I wonder if being a bit less generic could help here.

Otherwise I left a few comments. I have one remark on it but then realize it affects a lot of things: There are a lot of doc comments missing.

ecdsa/src/impl_signature_trait.rs

ed25519/src/impl_signature_trait.rs

libcrux-ml-dsa/src/impl_signature_trait.rs

traits/src/signature/arrayref.rs

traits/src/signature/secrets.rs

traits/src/signature/slice.rs

traits/src/signature/arrayref.rs

wysiwys · 2025-08-12T10:48:09Z

Thanks for the review and feedback @keks! I addressed most of the feedback points, and added some comments for the other points.

For the trait organization (specifically how to handle randomness and auxiliary data), I think there is room for improvement, too, and I think there is likely a combination of the different options from our discussion above that will address many of the concerns around ergonomics/complexity. I will take another look at the different combinations of input data we need to support, and look at the pros and cons of different approaches.

jschneider-bensch

I think the traits look good!

I have some comments:

There are some TODOs left in different places. Are these for follow-ups, or do you want to address them in this PR?
Could you try and add a generic test for some of the APIs, e.g. like with the KEM trait, s.t. we know the implementation works?
Could you summarize in the PR description which scheme now implements which APIs? Or are they all covered via blanket impls?

traits/src/signature/arrayref.rs

rsa/src/impl_signature_trait.rs

libcrux-ml-dsa/src/impl_signature_trait.rs

rsa/src/impl_signature_trait.rs

keks · 2025-08-13T07:10:21Z

What I am wondering is whether our Signature traits should support all uses of context. We also don't support all the features of the blake2 hash (e.g. personalisation or keying of the hash).

We could pretty easily support the use of contexts known at compile time:

Add a trait Context { context() -> &'static [u8]; } to the mldsa crate
Then impl<Ctx: Context> RandomizedSignature for MlDsa<Ctx>, where we call the context function to get the value
The context function should be marked inline(always). Maybe we can define a macro that defines a context struct and implements the trait for it, so the user doesn't forget to set it?

Review re-requested

traits/src/signature/slice.rs

jschneider-bensch · 2025-08-18T15:01:47Z

traits/src/signature/slice.rs

+#[macro_export]
+macro_rules! impl_verify_slice_trait {
+    ($type:ty => $vk_len:expr, $sig_len:expr, $verify_aux:ty, $verify_aux_param:tt) => {
+        /// The [`mod@slice`] version of the Verify trait.


This doesn't link correctly for me. Maybe something like [slice](libcrux_traits::signature::slice) would work?

Thanks for catching that! I updated the link to the suggestion. Could you let me know if the link works correctly on your side now?

keks

Nice, even RSA is looking better and better :)

I noticed some files had weird whitespace. Did you run rustfmt? I also found a bunch of TODOs that note that something doesn't show up in documentation. What exactly isn't showing up? The comments read like the trait doesn't show up, but that makes sense because the block only contains the impl.

rsa/src/impl_signature_trait.rs

ecdsa/src/impl_signature_trait.rs

ed25519/src/impl_signature_trait.rs

libcrux-ml-dsa/src/impl_signature_trait.rs

rsa/src/impl_hacl.rs

franziskuskiefer · 2025-08-25T09:12:59Z

Converting this to a draft for now. See #1107.

keks

I think this looks pretty good! I have a few questions on some design details that I would like to discuss before we proceed, but the fundamentals look like they are in place!

libcrux-ml-dsa/src/impl_signature_trait.rs

traits/src/signature/arrayref.rs

wysiwys · 2025-08-26T15:05:15Z

I added an initial implementation of keygen() to the traits in this PR.

Review re-requested

keks

I think this mostly looks good, just a few nitpicks.

traits/src/signature/arrayref.rs

libcrux-ml-dsa/src/impl_signature_trait.rs

traits/src/signature/arrayref.rs

ecdsa/src/impl_signature_trait.rs

…g work

keks

Left some comments on my changes

keks · 2025-10-02T13:56:27Z

libcrux-ml-dsa/src/ml_dsa_generic/multiplexing.rs

Somehow sign_mut existed in the instantiations but not in multiplexed. Added so we can impl arrayref

keks · 2025-10-02T13:58:59Z

libcrux-ml-dsa/src/impl_signature_trait.rs

I did a more complicate thing here for bounding the context sizes, because I didn't see a good way to return an error regarding this. This way we use the type system to ensure the error can't happen.

Dynamic contexts already were not supported in this API because we need to be able to determine the context from the type, so I hope that is fine.

franziskuskiefer

Thanks, a couple thoughts.

Dropping old APIs we do in a separate PR?

franziskuskiefer · 2025-10-02T14:56:27Z

crates/primitives/signature/src/lib.rs

@@ -0,0 +1,98 @@
+#[cfg(any(feature = "ecdsa", feature = "ed25519", feature = "mldsa"))]


A lib comment up here would be nice with a little info and an example on how to use this.

libcrux-ml-dsa/Cargo.toml

libcrux-ml-dsa/src/impl_signature_trait.rs

franziskuskiefer · 2025-10-02T15:45:08Z

ed25519/src/impl_signature_trait.rs

+            payload: &[u8],
+            signing_key: &[U8; SIGNING_KEY_LEN],
+            signature: &mut [u8; SIGNATURE_LEN],
+            _aux: (),


Having a useless parameter in the API isn't great. Maybe the aux version should be a different trait? Or maybe traits just make it more complicated than necessary for signatures.

franziskuskiefer · 2025-10-02T15:48:21Z

libcrux-ml-dsa/src/impl_signature_trait.rs

+            }
+        };
+    }
+    pub use impl_context;


Asking the user to implement these types is a little odd. Also, I'm not sure how well this works for more dynamic contexts?
We could make this slightly different to just have array references and the caller just wraps them. I made a quick example here https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=661ba82dce767334432a31da021307e1

Co-authored-by: Franziskus Kiefer <[email protected]>

wysiwys self-assigned this Jul 23, 2025

wysiwys force-pushed the wysiwys/signature-trait branch from d7ee4d0 to e0ada6b Compare August 11, 2025 14:26

wysiwys marked this pull request as ready for review August 11, 2025 16:41

wysiwys requested review from a team as code owners August 11, 2025 16:41

wysiwys requested a review from jschneider-bensch August 11, 2025 16:41

keks previously requested changes Aug 11, 2025

View reviewed changes

jschneider-bensch reviewed Aug 12, 2025

View reviewed changes

traits/src/signature/arrayref.rs Outdated Show resolved Hide resolved

rsa/src/impl_signature_trait.rs Outdated Show resolved Hide resolved

libcrux-ml-dsa/src/impl_signature_trait.rs Outdated Show resolved Hide resolved

rsa/src/impl_signature_trait.rs Outdated Show resolved Hide resolved

wysiwys requested a review from keks August 18, 2025 09:03

wysiwys requested a review from jschneider-bensch August 18, 2025 09:03

wysiwys force-pushed the wysiwys/signature-trait branch from 085d432 to 238ffd0 Compare August 18, 2025 12:11

jschneider-bensch reviewed Aug 18, 2025

View reviewed changes

keks previously requested changes Aug 18, 2025

View reviewed changes

wysiwys force-pushed the wysiwys/signature-trait branch from 7c1e998 to 1ee2dae Compare August 19, 2025 08:04

franziskuskiefer mentioned this pull request Aug 25, 2025

Figure out how to best multiplex between algorithms #1104

Closed

franziskuskiefer marked this pull request as draft August 25, 2025 09:13

keks reviewed Aug 26, 2025

View reviewed changes

libcrux-ml-dsa/src/impl_signature_trait.rs Outdated Show resolved Hide resolved

libcrux-ml-dsa/src/impl_signature_trait.rs Show resolved Hide resolved

traits/src/signature/arrayref.rs Outdated Show resolved Hide resolved

wysiwys force-pushed the wysiwys/signature-trait branch from 03c79fb to 9bff06d Compare August 26, 2025 14:51

wysiwys force-pushed the wysiwys/signature-trait branch from 1a1e602 to ecc056f Compare September 29, 2025 08:02

wysiwys requested a review from keks September 30, 2025 13:05

keks reviewed Sep 30, 2025

View reviewed changes

traits/src/signature/arrayref.rs Outdated Show resolved Hide resolved

libcrux-ml-dsa/src/impl_signature_trait.rs Outdated Show resolved Hide resolved

traits/src/signature/arrayref.rs Outdated Show resolved Hide resolved

ecdsa/src/impl_signature_trait.rs Show resolved Hide resolved

wysiwys marked this pull request as ready for review October 1, 2025 07:24

wysiwys force-pushed the wysiwys/signature-trait branch 2 times, most recently from ebe3469 to d632d86 Compare October 1, 2025 14:29

wysiwys and others added 19 commits October 2, 2025 15:51

add context

384401f

begin implementing keygen

6d7ebf6

replace std::marker::PhantomData with core::marker::PhantomData

a870aca

improve doc comments and arguments

7a69374

exclude signers for eurydice

5cbd348

implement Sign trait on standalone struct

2f5c4ed

remove generic Signer struct

2694838

update feature name

2a529f3

use literal for context in impl_context macro

1ae6d54

formatting

6982e57

add key-centric API

68a9eb4

add libcrux-signature crate

57fd42f

remove VerifyAux from traits

f5e9a15

update doctests

2dac988

move SignAux type to SignTypes

197f2d8

update generic test

8edd064

update secrets types

3089f62

Add a whole lot of documentation and make secret independence checkin…

b6d36fb

…g work

clean up errors, constrain context

1c65231

keks force-pushed the wysiwys/signature-trait branch from 6304db4 to 1c65231 Compare October 2, 2025 13:52

keks reviewed Oct 2, 2025

View reviewed changes

keks requested a review from jschneider-bensch October 2, 2025 14:00

hax: don't extract ml-dsa trait implementations

e903a0c

franziskuskiefer reviewed Oct 2, 2025

View reviewed changes

wysiwys and others added 6 commits October 6, 2025 17:15

Update libcrux-ml-dsa/Cargo.toml

e736c85

Co-authored-by: Franziskus Kiefer <[email protected]>

don't use DeclassifyRefMut in keygen

e4bbf46

fix lints

4df177d

formatting

21f0a64

use imports to simplify error matching

7370be0

move error conversions to separate From implementations

8b06bfc

		@@ -0,0 +1,98 @@
		#[cfg(any(feature = "ecdsa", feature = "ed25519", feature = "mldsa"))]

Signature traits #1080

Are you sure you want to change the base?

Signature traits #1080

Conversation

wysiwys commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jschneider-bensch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keks commented Aug 13, 2025

Uh oh!

Uh oh!

jschneider-bensch Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

wysiwys Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

keks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

franziskuskiefer commented Aug 25, 2025

Uh oh!

keks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wysiwys commented Aug 26, 2025

Uh oh!

keks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keks left a comment

Choose a reason for hiding this comment

Uh oh!

keks Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

keks Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

franziskuskiefer left a comment

Choose a reason for hiding this comment

Uh oh!

franziskuskiefer Oct 2, 2025

wysiwys commented Jul 23, 2025 •

edited

Loading

wysiwys commented Aug 12, 2025 •

edited

Loading