Improve `ssi-crypto`. #642

timothee-haudebourg · 2025-03-18T13:49:22Z

Description

Improves the ssi-crypto library:

Make the purpose of the library clearer: a flexible dynamic interface for cryptographic primitives built on top of RustCrypto (and ring), where algorithms can be selected at run time instead of compile time.
Add signing and verification traits that can be used by JWK, COSE and Data Integrity (in the future), instead of format specific traits that must be re-implemented (e.g. JwsSigner).
Re-export RustCrypto libraries (depending on the features), so that dependents don't have to depend on dozens of crypto libraries themselves.

The important changes are in the ssi-crypto library. The only reason why there are so many changes in other files is because I renamed a few algorithm variants to follow Rust naming conventions, and merged the ssi-crypto error types into one.

Tested

Original test suite still passes.
Added unit test for each supported cryptographic function.

sbihel

Are the changes to the algorithms casing really that important? I understand it's not exactly idiomatic, but at the same time they're essentially acronyms so it doesn't matter that much, it's additional breaking changes, and it makes this PR harder to review

timothee-haudebourg · 2025-03-19T15:01:50Z

Are the changes to the algorithms casing really that important?

No its not important, but it's the convention and I feel like it's now or never to make breaking changes. I can make that in another PR, but would that really help?

Alternatively I can revert the names for the ssi_jwk::Algorithm type only. It wasn't really my intention to change them, but they are defined with a macro that binds the names to the ssi_crypto::Algorithm names, so my IDE automatically changed them. It's really the change to ssi_jwk::Algorithm that bleeds out to the rest of the crates.

cobward · 2025-03-19T14:29:47Z

crates/bbs/src/lib.rs

+// pub struct
+


Suggested change

// pub struct

cobward · 2025-03-19T14:33:36Z

crates/crypto/src/key/type/ecdsa/mod.rs

+/// Key type.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[non_exhaustive]
+pub enum EcdsaKeyType {


Curve instead of KeyType?

I went back and forth between KeyType and Curve 😅 If you prefer Curve I'll use that instead.

cobward · 2025-03-19T14:41:06Z

crates/claims/crates/jws/src/lib.rs

+            // let padding;
+            // let hashed;


Suggested change

// let padding;

// let hashed;

cobward · 2025-03-19T14:45:01Z

crates/crypto/src/algorithm/static.rs

+
+/// ECDSA using secp256k1 (K-256) and SHA-256, with or without recovery bit.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub enum AnyES {


Suggested change

pub enum AnyES {

pub enum AnyES256K {

I intend to remove what's in the static module. Those types are used by the Data Integrity libs, but now that we have low-level Signer and Verifier traits we'll be able to use the Algorithm type directly. Also I feel like defining static types for crypto algorithm is RustCrypto's job, not ssi-crypto's.

cobward · 2025-03-19T15:00:53Z

crates/crypto/src/key/type/ecdsa/k256.rs

+        bytes.push(0x04);
+        bytes.extend(x);
+        bytes.extend(y);
+        Self::from_k256_sec1_bytes(&bytes)


FWIW you can use k256::EncodedPoint::from_untagged_bytes or from_affine_coordinates to avoid messing around with SEC1 tags.

Most of the key conversion functions are coming straight from ssi_jwk.

avoid messing around with SEC1 tags

Oh that's what the 0x04 is? Yeah that's why I did change anything, I wasn't sure what format that was.

cobward · 2025-03-19T15:02:20Z

crates/crypto/src/key/type/symmetric.rs

+use super::KeyType;
+
+#[derive(Clone, ZeroizeOnDrop)]
+pub struct SymmetricKey(Box<[u8]>);


Why Box<[u8]> instead of Vec?

Vec provides more capabilities like resizing that we don't need here. So to me the question is more "Why Vec when Box<[u8]> is enough"?

Because in most applications an owned slice will be a Vec, so we're adding unnecessary overhead by requiring conversion between Box<[u8]> and Vec.

Additional capabilities on a Vec don't actually cause any issues right? There's just an extra byte of memory used to store the capacity of the slice.

Conversion from Vec to Box<[u8]> requires shrinking the Vec to capacity, manually dropping and reading the vec into a new buffer. I'm not an expert but I think that also implies that the original Vec buffer is not reused and therefore not zeroized. So overall I think it makes more sense to take ownership of the Vec. I was wrong about that part, the Box manually drops the original buffer

RustCrypo always use fixed-length arrays, not Vec, so I'm not worried about shrinking stuff. But it's true that many functions are expecting Vecs so I'm not going to fight for it, I'll remove the Box<[u8]>s.

cobward · 2025-03-19T15:03:13Z

crates/crypto/src/options.rs

+    *<Box<dyn Send + Sync + Any>>::downcast(t).unwrap()
+}
+
+#[allow(clippy::borrowed_box)]
+fn downcast_ref<T: 'static>(t: &Box<dyn Send + Sync + Any>) -> &T {
+    t.downcast_ref().unwrap()
+}
+
+#[allow(clippy::borrowed_box)]
+fn downcast_mut<T: 'static>(t: &mut Box<dyn Send + Sync + Any>) -> &mut T {
+    t.downcast_mut().unwrap()


Can you leave comments describing why the unwrap won't ever panic.

timothee-haudebourg · 2025-03-24T12:44:10Z

@cobward, @sbihel, ready for another review.

timothee-haudebourg force-pushed the new-crypto-crate branch from 666717c to 25af31b Compare March 18, 2025 14:44

timothee-haudebourg changed the base branch from main to make-clippy-happy March 18, 2025 14:44

Base automatically changed from make-clippy-happy to main March 18, 2025 17:38

timothee-haudebourg added 9 commits March 19, 2025 14:23

Trigger CI

7bcbf74

Replace ssi-crypto.

99a2b24

Fix clippy warnings.

153d5c0

Nested key types.

8d6e40e

Key type names & generation.

533227a

Implement remaining signature algorithms.

ea3f382

Tweak features & documentation.

edd17f9

Move error type.

dd2204b

Fix some naming convensions + doc.

24e098f

timothee-haudebourg force-pushed the new-crypto-crate branch from eb5bf82 to 24e098f Compare March 19, 2025 13:23

timothee-haudebourg marked this pull request as ready for review March 19, 2025 13:24

timothee-haudebourg added 9 commits March 19, 2025 14:26

Trigger CI

89c1ee3

Fix some clippy warnings.

7ed7228

Add feature gate in doc test.

31dcc1d

Add README.md

f69749f

Fix feature gates.

12c6156

Fix some names.

33bc7dc

Re-export signature library for use in ssi-jwk.

2c54b5c

Require signature/std feature.

92bb839

Remove eprints.

09e2388

timothee-haudebourg requested a review from sbihel March 19, 2025 14:28

sbihel reviewed Mar 19, 2025

View reviewed changes

cobward reviewed Mar 19, 2025

View reviewed changes

timothee-haudebourg added 3 commits March 20, 2025 11:47

Use Curve instead of KeyType for ECDSA and EdDSA keys.

0e31ffd

Revert ssi_jwk::Algorithm name changes.

63da6f2

Revert some more name changes.

69949e7

timothee-haudebourg added 3 commits March 21, 2025 17:02

Fix doc test.

4284c4a

Avoid some unwrap-ing.

1aa4ba2

Use Vec<u8> instead of Box<[u8]>.

b25caac

timothee-haudebourg force-pushed the new-crypto-crate branch from efae969 to b25caac Compare March 24, 2025 12:33

timothee-haudebourg requested review from sbihel and cobward March 24, 2025 12:43

timothee-haudebourg mentioned this pull request Mar 25, 2025

ssi-data-integrity: Move away from type-oriented cryptosuite definitions. #663

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `ssi-crypto`. #642

Improve `ssi-crypto`. #642

timothee-haudebourg commented Mar 18, 2025 •

edited

Loading

sbihel left a comment

timothee-haudebourg commented Mar 19, 2025 •

edited

Loading

cobward Mar 19, 2025

cobward Mar 19, 2025

timothee-haudebourg Mar 19, 2025

cobward Mar 19, 2025

cobward Mar 19, 2025

timothee-haudebourg Mar 19, 2025

cobward Mar 19, 2025

timothee-haudebourg Mar 19, 2025

cobward Mar 19, 2025

timothee-haudebourg Mar 19, 2025

cobward Mar 20, 2025 •

edited

Loading

timothee-haudebourg Mar 21, 2025

cobward Mar 19, 2025

timothee-haudebourg commented Mar 24, 2025

Improve ssi-crypto. #642

Are you sure you want to change the base?

Improve ssi-crypto. #642

Conversation

timothee-haudebourg commented Mar 18, 2025 • edited Loading

Description

Tested

sbihel left a comment

Choose a reason for hiding this comment

timothee-haudebourg commented Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobward Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothee-haudebourg commented Mar 24, 2025

Improve `ssi-crypto`. #642

Improve `ssi-crypto`. #642

timothee-haudebourg commented Mar 18, 2025 •

edited

Loading

timothee-haudebourg commented Mar 19, 2025 •

edited

Loading

cobward Mar 20, 2025 •

edited

Loading