feat(crypto): CRP-2579 Add support for derivation to ecdsa_secp256r1 crate #1730
Conversation
…crate In order to remove the dependency on the internal threshold ECDSA protocol implementation from ic-crypto-utils-canister-threshold-sig we have to have derivation available in the 3 signature utility crates.
There was a problem hiding this comment.
Thanks, @randombit! Looks good to me! Find some minor comments below.
| ) -> (Self, [u8; 32]) { | ||
| use p256::NonZeroScalar; | ||
|
|
||
| let public_key: AffinePoint = self.key.verifying_key().as_affine().clone(); |
There was a problem hiding this comment.
Clippy has a comment here:
error: using `clone` on type `AffinePoint<NistP256>` which implements the `Copy` trait
--> rs/crypto/ecdsa_secp256r1/src/lib.rs:473:39
|
473 | let public_key: AffinePoint = self.key.verifying_key().as_affine().clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*self.key.verifying_key().as_affine()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: `-D clippy::clone-on-copy` implied by `-D clippy::all`
= help: to override `-D clippy::all` add `#[allow(clippy::clone_on_copy)]`
| /// As long as each index of the derivation path is a 4-byte input with the highest | ||
| /// bit cleared, this derivation scheme matches SLIP-10 | ||
| /// | ||
| /// See <https://internetcomputer.org/docs/current/references/ic-interface-spec#ic-ecdsa_public_key> | ||
| /// for details on the derivation scheme. |
There was a problem hiding this comment.
The given URL actually doesn't give any details on the derivation scheme for curves different from secp256k1. How about creating a ticket to improve this (e.g., to mention SLIP-10)?
| /// Create a BIP32-style derivation path | ||
| pub fn new_bip32(bip32: &[u32]) -> Self { |
There was a problem hiding this comment.
It this intentionally called new_bip32, given that BIP32 isn't defined for secp256r1? IIUC, the derivation is done as per SLIP10, but given that SLIP10 is a generalization of BIP32, it might make sense to call it new_bip32 nevertheless.
| NonZeroScalar::new(derived_scalar).expect("Derivation always produces non-zero sum"); | ||
|
|
||
| let derived_key = Self { | ||
| key: p256::SecretKey::from(nz_ds).into(), |
There was a problem hiding this comment.
It seems we can also go directly: p256::ecdsa::SigningKey::from(nz_ds)
| @@ -374,7 +554,7 @@ impl PublicKey { | |||
| /// valid signature, then (r,-s) is also valid. To avoid this malleability, | |||
| /// some systems require that s be "normalized" to the smallest value. | |||
| /// | |||
| /// This normalization is quite common on secp256k1, but is virtually | |||
| /// This normalization is quite common on secp256r1, but is virtually | |||
There was a problem hiding this comment.
Was this change intentional? The text now says: "normalization is common on r1, but is unknown for r1".
| key: p256::PublicKey::from( | ||
| p256::PublicKey::from_affine(pt).expect("Derived point is valid"), | ||
| ) | ||
| .into(), |
There was a problem hiding this comment.
It seems one conversion layer is to itself, so it should be obsolete. Also, how about being explicit (instead of doing into). As a result, I suggest we do
key: p256::ecdsa::VerifyingKey::from(
p256::PublicKey::from_affine(pt).expect("Derived point is valid"),
),
| assert!(derived_pk.verify_signature(&msg, &derived_sig)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
For some key derivations, we also have a test private_derivation_also_works_for_derived_keys: would that be applicable here as well?
In order to remove the dependency on the internal threshold ECDSA protocol implementation from ic-crypto-utils-canister-threshold-sig we have to have derivation available in the 3 signature utility crates.