From cde5fce1880e0f85acd44f052bf527e980aace8f Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 22 Mar 2023 11:09:10 +0100 Subject: [PATCH] Crypto Pair trait refactory (#13657) * Crypto pair refactory * Remove unused method * Apply review suggestions * Remove leftovers * Associated type is not really required * Fix after refactory * Fix benchmark-ui test --------- Co-authored-by: Anton --- ...bad_return_type_blank_with_question.stderr | 2 +- primitives/application-crypto/src/lib.rs | 5 +- primitives/core/src/crypto.rs | 79 ++++++++++++----- primitives/core/src/ecdsa.rs | 49 +---------- primitives/core/src/ed25519.rs | 51 +---------- primitives/core/src/sr25519.rs | 85 +++---------------- 6 files changed, 75 insertions(+), 196 deletions(-) diff --git a/frame/support/test/tests/benchmark_ui/bad_return_type_blank_with_question.stderr b/frame/support/test/tests/benchmark_ui/bad_return_type_blank_with_question.stderr index 1f8c9f1e171eb..601bbd20fb73d 100644 --- a/frame/support/test/tests/benchmark_ui/bad_return_type_blank_with_question.stderr +++ b/frame/support/test/tests/benchmark_ui/bad_return_type_blank_with_question.stderr @@ -7,4 +7,4 @@ error[E0277]: the `?` operator can only be used in a function that returns `Resu 15 | something()?; | ^ cannot use the `?` operator in a function that returns `()` | - = help: the trait `FromResidual>` is not implemented for `()` + = help: the trait `FromResidual>` is not implemented for `()` diff --git a/primitives/application-crypto/src/lib.rs b/primitives/application-crypto/src/lib.rs index 992ecd1d05621..0685477ddff8d 100644 --- a/primitives/application-crypto/src/lib.rs +++ b/primitives/application-crypto/src/lib.rs @@ -23,7 +23,7 @@ pub use sp_core::crypto::{key_types, CryptoTypeId, KeyTypeId}; #[doc(hidden)] #[cfg(feature = "full_crypto")] -pub use sp_core::crypto::{DeriveJunction, Pair, SecretStringError, Ss58Codec}; +pub use sp_core::crypto::{DeriveError, DeriveJunction, Pair, SecretStringError, Ss58Codec}; #[doc(hidden)] pub use sp_core::{ self, @@ -129,7 +129,6 @@ macro_rules! app_crypto_pair { type Public = Public; type Seed = <$pair as $crate::Pair>::Seed; type Signature = Signature; - type DeriveError = <$pair as $crate::Pair>::DeriveError; $crate::app_crypto_pair_functions_if_std!($pair); @@ -137,7 +136,7 @@ macro_rules! app_crypto_pair { &self, path: Iter, seed: Option, - ) -> Result<(Self, Option), Self::DeriveError> { + ) -> Result<(Self, Option), $crate::DeriveError> { self.0.derive(path, seed).map(|x| (Self(x.0), x.1)) } fn from_seed(seed: &Self::Seed) -> Self { diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index 16af3d06963ab..80e65b342cbd7 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -24,6 +24,8 @@ use crate::hexdisplay::HexDisplay; use crate::{ed25519, sr25519}; #[cfg(feature = "std")] use base58::{FromBase58, ToBase58}; +#[cfg(feature = "std")] +use bip39::{Language, Mnemonic, MnemonicType}; use codec::{Decode, Encode, MaxEncodedLen}; #[cfg(feature = "std")] use rand::{rngs::OsRng, RngCore}; @@ -52,10 +54,6 @@ pub const DEV_PHRASE: &str = /// The address of the associated root phrase for our publicly known keys. pub const DEV_ADDRESS: &str = "5DfhGyQdFobKM8NsWvEeAKk5EQQgYe9AydgJ7rMB6E1EqRzV"; -/// The infallible type. -#[derive(crate::RuntimeDebug)] -pub enum Infallible {} - /// The length of the junction identifier. Note that this is also referred to as the /// `CHAIN_CODE_LENGTH` in the context of Schnorrkel. #[cfg(feature = "full_crypto")] @@ -108,6 +106,16 @@ pub enum SecretStringError { InvalidPath, } +/// An error when deriving a key. +#[cfg_attr(feature = "std", derive(thiserror::Error))] +#[derive(Debug, Clone, PartialEq, Eq)] +#[cfg(feature = "full_crypto")] +pub enum DeriveError { + /// A soft key was found in the path (and is unsupported). + #[cfg_attr(feature = "std", error("Soft key in path"))] + SoftKeyInPath, +} + /// A since derivation junction description. It is the single parameter used when creating /// a new secret key from an existing secret key and, in the case of `SoftRaw` and `SoftIndex` /// a new public key from an existing public key. @@ -691,40 +699,45 @@ mod dummy { type Public = Dummy; type Seed = Dummy; type Signature = Dummy; - type DeriveError = (); + #[cfg(feature = "std")] fn generate_with_phrase(_: Option<&str>) -> (Self, String, Self::Seed) { Default::default() } + #[cfg(feature = "std")] fn from_phrase(_: &str, _: Option<&str>) -> Result<(Self, Self::Seed), SecretStringError> { Ok(Default::default()) } + fn derive>( &self, _: Iter, _: Option, - ) -> Result<(Self, Option), Self::DeriveError> { + ) -> Result<(Self, Option), DeriveError> { Ok((Self, None)) } - fn from_seed(_: &Self::Seed) -> Self { - Self - } + fn from_seed_slice(_: &[u8]) -> Result { Ok(Self) } + fn sign(&self, _: &[u8]) -> Self::Signature { Self } + fn verify>(_: &Self::Signature, _: M, _: &Self::Public) -> bool { true } + fn verify_weak, M: AsRef<[u8]>>(_: &[u8], _: M, _: P) -> bool { true } + fn public(&self) -> Self::Public { Self } + fn to_raw_vec(&self) -> Vec { vec![] } @@ -845,9 +858,6 @@ pub trait Pair: CryptoType + Sized + Clone + Send + Sync + 'static { /// and verified with the message and a public key. type Signature: AsRef<[u8]>; - /// Error returned from the `derive` function. - type DeriveError; - /// Generate new secure (random) key pair. /// /// This is only for ephemeral keys really, since you won't have access to the secret key @@ -866,27 +876,47 @@ pub trait Pair: CryptoType + Sized + Clone + Send + Sync + 'static { /// This is generally slower than `generate()`, so prefer that unless you need to persist /// the key from the current session. #[cfg(feature = "std")] - fn generate_with_phrase(password: Option<&str>) -> (Self, String, Self::Seed); + fn generate_with_phrase(password: Option<&str>) -> (Self, String, Self::Seed) { + let mnemonic = Mnemonic::new(MnemonicType::Words12, Language::English); + let phrase = mnemonic.phrase(); + let (pair, seed) = Self::from_phrase(phrase, password) + .expect("All phrases generated by Mnemonic are valid; qed"); + (pair, phrase.to_owned(), seed) + } /// Returns the KeyPair from the English BIP39 seed `phrase`, or `None` if it's invalid. #[cfg(feature = "std")] fn from_phrase( phrase: &str, password: Option<&str>, - ) -> Result<(Self, Self::Seed), SecretStringError>; + ) -> Result<(Self, Self::Seed), SecretStringError> { + let mnemonic = Mnemonic::from_phrase(phrase, Language::English) + .map_err(|_| SecretStringError::InvalidPhrase)?; + let big_seed = + substrate_bip39::seed_from_entropy(mnemonic.entropy(), password.unwrap_or("")) + .map_err(|_| SecretStringError::InvalidSeed)?; + let mut seed = Self::Seed::default(); + let seed_slice = seed.as_mut(); + let seed_len = seed_slice.len(); + debug_assert!(seed_len <= big_seed.len()); + seed_slice[..seed_len].copy_from_slice(&big_seed[..seed_len]); + Self::from_seed_slice(seed_slice).map(|x| (x, seed)) + } /// Derive a child key from a series of given junctions. fn derive>( &self, path: Iter, seed: Option, - ) -> Result<(Self, Option), Self::DeriveError>; + ) -> Result<(Self, Option), DeriveError>; /// Generate new key pair from the provided `seed`. /// /// @WARNING: THIS WILL ONLY BE SECURE IF THE `seed` IS SECURE. If it can be guessed /// by an attacker then they can also derive your key. - fn from_seed(seed: &Self::Seed) -> Self; + fn from_seed(seed: &Self::Seed) -> Self { + Self::from_seed_slice(seed.as_ref()).expect("seed has valid length; qed") + } /// Make a new key pair from secret seed material. The slice must be the correct size or /// it will return `None`. @@ -1202,14 +1232,15 @@ mod tests { type Public = TestPublic; type Seed = [u8; 8]; type Signature = [u8; 0]; - type DeriveError = (); fn generate() -> (Self, ::Seed) { (TestPair::Generated, [0u8; 8]) } + fn generate_with_phrase(_password: Option<&str>) -> (Self, String, ::Seed) { (TestPair::GeneratedWithPhrase, "".into(), [0u8; 8]) } + fn from_phrase( phrase: &str, password: Option<&str>, @@ -1222,11 +1253,12 @@ mod tests { [0u8; 8], )) } + fn derive>( &self, path_iter: Iter, _: Option<[u8; 8]>, - ) -> Result<(Self, Option<[u8; 8]>), Self::DeriveError> { + ) -> Result<(Self, Option<[u8; 8]>), DeriveError> { Ok(( match self.clone() { TestPair::Standard { phrase, password, path } => TestPair::Standard { @@ -1240,21 +1272,21 @@ mod tests { if path_iter.count() == 0 { x } else { - return Err(()) + return Err(DeriveError::SoftKeyInPath) }, }, None, )) } - fn from_seed(_seed: &::Seed) -> Self { - TestPair::Seed(_seed.as_ref().to_owned()) - } + fn sign(&self, _message: &[u8]) -> Self::Signature { [] } + fn verify>(_: &Self::Signature, _: M, _: &Self::Public) -> bool { true } + fn verify_weak, M: AsRef<[u8]>>( _sig: &[u8], _message: M, @@ -1262,12 +1294,15 @@ mod tests { ) -> bool { true } + fn public(&self) -> Self::Public { TestPublic } + fn from_seed_slice(seed: &[u8]) -> Result { Ok(TestPair::Seed(seed.to_owned())) } + fn to_raw_vec(&self) -> Vec { vec![] } diff --git a/primitives/core/src/ecdsa.rs b/primitives/core/src/ecdsa.rs index d68ba39a0fb79..e63b283d26ee3 100644 --- a/primitives/core/src/ecdsa.rs +++ b/primitives/core/src/ecdsa.rs @@ -29,11 +29,9 @@ use crate::crypto::{ }; #[cfg(feature = "full_crypto")] use crate::{ - crypto::{DeriveJunction, Pair as TraitPair, SecretStringError}, + crypto::{DeriveError, DeriveJunction, Pair as TraitPair, SecretStringError}, hashing::blake2_256, }; -#[cfg(feature = "std")] -use bip39::{Language, Mnemonic, MnemonicType}; #[cfg(all(feature = "full_crypto", not(feature = "std")))] use secp256k1::Secp256k1; #[cfg(feature = "std")] @@ -367,13 +365,6 @@ fn derive_hard_junction(secret_seed: &Seed, cc: &[u8; 32]) -> Seed { ("Secp256k1HDKD", secret_seed, cc).using_encoded(sp_core_hashing::blake2_256) } -/// An error when deriving a key. -#[cfg(feature = "full_crypto")] -pub enum DeriveError { - /// A soft key was found in the path (and is unsupported). - SoftKeyInPath, -} - /// A key pair. #[cfg(feature = "full_crypto")] #[derive(Clone)] @@ -387,44 +378,6 @@ impl TraitPair for Pair { type Public = Public; type Seed = Seed; type Signature = Signature; - type DeriveError = DeriveError; - - /// Generate new secure (random) key pair and provide the recovery phrase. - /// - /// You can recover the same key later with `from_phrase`. - #[cfg(feature = "std")] - fn generate_with_phrase(password: Option<&str>) -> (Pair, String, Seed) { - let mnemonic = Mnemonic::new(MnemonicType::Words12, Language::English); - let phrase = mnemonic.phrase(); - let (pair, seed) = Self::from_phrase(phrase, password) - .expect("All phrases generated by Mnemonic are valid; qed"); - (pair, phrase.to_owned(), seed) - } - - /// Generate key pair from given recovery phrase and password. - #[cfg(feature = "std")] - fn from_phrase( - phrase: &str, - password: Option<&str>, - ) -> Result<(Pair, Seed), SecretStringError> { - let big_seed = substrate_bip39::seed_from_entropy( - Mnemonic::from_phrase(phrase, Language::English) - .map_err(|_| SecretStringError::InvalidPhrase)? - .entropy(), - password.unwrap_or(""), - ) - .map_err(|_| SecretStringError::InvalidSeed)?; - let mut seed = Seed::default(); - seed.copy_from_slice(&big_seed[0..32]); - Self::from_seed_slice(&big_seed[0..32]).map(|x| (x, seed)) - } - - /// Make a new key pair from secret seed material. - /// - /// You should never need to use this; generate(), generate_with_phrase - fn from_seed(seed: &Seed) -> Pair { - Self::from_seed_slice(&seed[..]).expect("seed has valid length; qed") - } /// Make a new key pair from secret seed material. The slice must be 32 bytes long or it /// will return `None`. diff --git a/primitives/core/src/ed25519.rs b/primitives/core/src/ed25519.rs index 76b3064ad5a44..503c127a9e06b 100644 --- a/primitives/core/src/ed25519.rs +++ b/primitives/core/src/ed25519.rs @@ -35,9 +35,7 @@ use crate::crypto::{ CryptoType, CryptoTypeId, CryptoTypePublicPair, Derive, Public as TraitPublic, UncheckedFrom, }; #[cfg(feature = "full_crypto")] -use crate::crypto::{DeriveJunction, Pair as TraitPair, SecretStringError}; -#[cfg(feature = "std")] -use bip39::{Language, Mnemonic, MnemonicType}; +use crate::crypto::{DeriveError, DeriveJunction, Pair as TraitPair, SecretStringError}; #[cfg(feature = "full_crypto")] use core::convert::TryFrom; #[cfg(feature = "full_crypto")] @@ -46,8 +44,6 @@ use ed25519_zebra::{SigningKey, VerificationKey}; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use sp_runtime_interface::pass_by::PassByInner; use sp_std::ops::Deref; -#[cfg(feature = "std")] -use substrate_bip39::seed_from_entropy; /// An identifier used to match public keys against ed25519 keys pub const CRYPTO_ID: CryptoTypeId = CryptoTypeId(*b"ed25"); @@ -385,56 +381,11 @@ fn derive_hard_junction(secret_seed: &Seed, cc: &[u8; 32]) -> Seed { ("Ed25519HDKD", secret_seed, cc).using_encoded(sp_core_hashing::blake2_256) } -/// An error when deriving a key. -#[cfg(feature = "full_crypto")] -pub enum DeriveError { - /// A soft key was found in the path (and is unsupported). - SoftKeyInPath, -} - #[cfg(feature = "full_crypto")] impl TraitPair for Pair { type Public = Public; type Seed = Seed; type Signature = Signature; - type DeriveError = DeriveError; - - /// Generate new secure (random) key pair and provide the recovery phrase. - /// - /// You can recover the same key later with `from_phrase`. - #[cfg(feature = "std")] - fn generate_with_phrase(password: Option<&str>) -> (Pair, String, Seed) { - let mnemonic = Mnemonic::new(MnemonicType::Words12, Language::English); - let phrase = mnemonic.phrase(); - let (pair, seed) = Self::from_phrase(phrase, password) - .expect("All phrases generated by Mnemonic are valid; qed"); - (pair, phrase.to_owned(), seed) - } - - /// Generate key pair from given recovery phrase and password. - #[cfg(feature = "std")] - fn from_phrase( - phrase: &str, - password: Option<&str>, - ) -> Result<(Pair, Seed), SecretStringError> { - let big_seed = seed_from_entropy( - Mnemonic::from_phrase(phrase, Language::English) - .map_err(|_| SecretStringError::InvalidPhrase)? - .entropy(), - password.unwrap_or(""), - ) - .map_err(|_| SecretStringError::InvalidSeed)?; - let mut seed = Seed::default(); - seed.copy_from_slice(&big_seed[0..32]); - Self::from_seed_slice(&big_seed[0..32]).map(|x| (x, seed)) - } - - /// Make a new key pair from secret seed material. - /// - /// You should never need to use this; generate(), generate_with_phrase - fn from_seed(seed: &Seed) -> Pair { - Self::from_seed_slice(&seed[..]).expect("seed has valid length; qed") - } /// Make a new key pair from secret seed material. The slice must be 32 bytes long or it /// will return `None`. diff --git a/primitives/core/src/sr25519.rs b/primitives/core/src/sr25519.rs index 809d83aaf08a8..8dbcca9e2673e 100644 --- a/primitives/core/src/sr25519.rs +++ b/primitives/core/src/sr25519.rs @@ -24,9 +24,7 @@ #[cfg(feature = "std")] use crate::crypto::Ss58Codec; #[cfg(feature = "full_crypto")] -use crate::crypto::{DeriveJunction, Infallible, Pair as TraitPair, SecretStringError}; -#[cfg(feature = "std")] -use bip39::{Language, Mnemonic, MnemonicType}; +use crate::crypto::{DeriveError, DeriveJunction, Pair as TraitPair, SecretStringError}; #[cfg(feature = "full_crypto")] use schnorrkel::{ derive::{ChainCode, Derivation, CHAIN_CODE_LENGTH}, @@ -34,8 +32,6 @@ use schnorrkel::{ }; #[cfg(feature = "full_crypto")] use sp_std::vec::Vec; -#[cfg(feature = "std")] -use substrate_bip39::mini_secret_from_entropy; use crate::{ crypto::{ @@ -458,16 +454,6 @@ impl TraitPair for Pair { type Public = Public; type Seed = Seed; type Signature = Signature; - type DeriveError = Infallible; - - /// Make a new key pair from raw secret seed material. - /// - /// This is generated using schnorrkel's Mini-Secret-Keys. - /// - /// A MiniSecretKey is literally what Ed25519 calls a SecretKey, which is just 32 random bytes. - fn from_seed(seed: &Seed) -> Pair { - Self::from_seed_slice(&seed[..]).expect("32 bytes can always build a key; qed") - } /// Get the public key. fn public(&self) -> Public { @@ -476,10 +462,12 @@ impl TraitPair for Pair { Public(pk) } - /// Make a new key pair from secret seed material. The slice must be 32 bytes long or it - /// will return `None`. + /// Make a new key pair from raw secret seed material. + /// + /// This is generated using schnorrkel's Mini-Secret-Keys. /// - /// You should never need to use this; generate(), generate_with_phrase(), from_phrase() + /// A `MiniSecretKey` is literally what Ed25519 calls a `SecretKey`, which is just 32 random + /// bytes. fn from_seed_slice(seed: &[u8]) -> Result { match seed.len() { MINI_SECRET_KEY_LENGTH => Ok(Pair( @@ -495,42 +483,16 @@ impl TraitPair for Pair { _ => Err(SecretStringError::InvalidSeedLength), } } - #[cfg(feature = "std")] - fn generate_with_phrase(password: Option<&str>) -> (Pair, String, Seed) { - let mnemonic = Mnemonic::new(MnemonicType::Words12, Language::English); - let phrase = mnemonic.phrase(); - let (pair, seed) = Self::from_phrase(phrase, password) - .expect("All phrases generated by Mnemonic are valid; qed"); - (pair, phrase.to_owned(), seed) - } - #[cfg(feature = "std")] - fn from_phrase( - phrase: &str, - password: Option<&str>, - ) -> Result<(Pair, Seed), SecretStringError> { - Mnemonic::from_phrase(phrase, Language::English) - .map_err(|_| SecretStringError::InvalidPhrase) - .map(|m| Self::from_entropy(m.entropy(), password)) - } fn derive>( &self, path: Iter, seed: Option, - ) -> Result<(Pair, Option), Self::DeriveError> { - let seed = if let Some(s) = seed { - if let Ok(msk) = MiniSecretKey::from_bytes(&s) { - if msk.expand(ExpansionMode::Ed25519) == self.0.secret { - Some(msk) - } else { - None - } - } else { - None - } - } else { - None - }; + ) -> Result<(Pair, Option), DeriveError> { + let seed = seed + .and_then(|s| MiniSecretKey::from_bytes(&s).ok()) + .filter(|msk| msk.expand(ExpansionMode::Ed25519) == self.0.secret); + let init = self.0.secret.clone(); let (result, seed) = path.fold((init, seed), |(acc, acc_seed), j| match (j, acc_seed) { (DeriveJunction::Soft(cc), _) => (acc.derived_key_simple(ChainCode(cc), &[]).0, None), @@ -572,18 +534,6 @@ impl TraitPair for Pair { #[cfg(feature = "std")] impl Pair { - /// Make a new key pair from binary data derived from a valid seed phrase. - /// - /// This uses a key derivation function to convert the entropy into a seed, then returns - /// the pair generated from it. - pub fn from_entropy(entropy: &[u8], password: Option<&str>) -> (Pair, Seed) { - let mini_key: MiniSecretKey = mini_secret_from_entropy(entropy, password.unwrap_or("")) - .expect("32 bytes can always build a key; qed"); - - let kp = mini_key.expand_to_keypair(ExpansionMode::Ed25519); - (Pair(kp), mini_key.to_bytes()) - } - /// Verify a signature on a message. Returns `true` if the signature is good. /// Supports old 0.1.1 deprecated signatures and should be used only for backward /// compatibility. @@ -652,10 +602,8 @@ pub fn verify_batch( #[cfg(test)] mod compatibility_test { use super::*; - use crate::crypto::DEV_PHRASE; - - // NOTE: tests to ensure addresses that are created with the `0.1.x` version (pre-audit) are - // still functional. + use crate::crypto::{Ss58Codec, DEV_ADDRESS, DEV_PHRASE}; + use serde_json; #[test] fn derive_soft_known_pair_should_work() { @@ -690,13 +638,6 @@ mod compatibility_test { assert!(Pair::verify_deprecated(&signature, &message[..], &public)); assert!(!Pair::verify(&signature, &message[..], &public)); } -} - -#[cfg(test)] -mod test { - use super::*; - use crate::crypto::{Ss58Codec, DEV_ADDRESS, DEV_PHRASE}; - use serde_json; #[test] fn default_phrase_should_be_used() {