Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Crypto Pair trait refactory #13657

Merged
merged 10 commits into from
Mar 22, 2023
Merged

Crypto Pair trait refactory #13657

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9c1c381
Crypto pair refactory
davxy Mar 20, 2023
aa48fca
Remove unused method
davxy Mar 20, 2023
219327c
Merge branch 'master' into davxy-crypto-pair-refactory
davxy Mar 20, 2023
e86ed44
Apply review suggestions
davxy Mar 21, 2023
b712209
Cargo fmt
davxy Mar 21, 2023
14c1465
Remove leftovers
davxy Mar 22, 2023
5c619c1
Associated type is not really required
davxy Mar 22, 2023
282095a
Fix after refactory
davxy Mar 22, 2023
dd5e206
Merge branch 'master' into davxy-crypto-pair-refactory
davxy Mar 22, 2023
b559b10
Fix benchmark-ui test
davxy Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 44 additions & 9 deletions primitives/core/src/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -692,39 +694,45 @@ mod 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<Iter: Iterator<Item = DeriveJunction>>(
&self,
_: Iter,
_: Option<Dummy>,
) -> Result<(Self, Option<Dummy>), Self::DeriveError> {
Ok((Self, None))
}
fn from_seed(_: &Self::Seed) -> Self {
Self
}

fn from_seed_slice(_: &[u8]) -> Result<Self, SecretStringError> {
Ok(Self)
}

fn sign(&self, _: &[u8]) -> Self::Signature {
Self
}

fn verify<M: AsRef<[u8]>>(_: &Self::Signature, _: M, _: &Self::Public) -> bool {
true
}

fn verify_weak<P: AsRef<[u8]>, M: AsRef<[u8]>>(_: &[u8], _: M, _: P) -> bool {
true
}

fn public(&self) -> Self::Public {
Self
}

fn to_raw_vec(&self) -> Vec<u8> {
vec![]
}
Expand Down Expand Up @@ -866,14 +874,33 @@ 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 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 = Self::Seed::default();
let seed_slice = seed.as_mut();
let minlen = seed_slice.len().min(big_seed.len());
seed_slice[0..minlen].copy_from_slice(&big_seed[0..minlen]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add some debug assert that big_seed.len() > seed_slice.len()? Otherwise there may will be some implementation that will just use some zeros?

Copy link
Member Author

@davxy davxy Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably would be better to fail than filling the tail of the seed with zeroes.

But what about embedding the only function that substrate-bip39 offers directly into substrate?
As I said that library provides 1 function (the other is not used anymore after this pr).

The function is this:

pub fn seed_from_entropy(entropy: &[u8], password: &str) -> Result<[u8; 64], Error> {
    if entropy.len() < 16 || entropy.len() > 32 || entropy.len() % 4 != 0 {
        return Err(Error::InvalidEntropy);
    }
    let mut salt = String::with_capacity(8 + password.len());
    salt.push_str("mnemonic");
    salt.push_str(password);
    let mut seed = [0u8; 64];
    pbkdf2::<Hmac<Sha512>>(entropy, salt.as_bytes(), 2048, &mut seed);
    salt.zeroize();
    Ok(seed)
}

Sounds a bit bombastic to define a library only to provide a function...

Furthermore, we can then easily modify it to fill a mut slice (passed as parameter) to fill it with an arbitrary number of bytes... Thus there would be no need to check if we can fill the seed buffer because we always can

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no real opinion here. We can copy the function, I don't really care. I don't know the history behind the substrate-bip39 crate.

Self::from_seed_slice(seed_slice).map(|x| (x, seed))
}

/// Derive a child key from a series of given junctions.
fn derive<Iter: Iterator<Item = DeriveJunction>>(
Expand All @@ -886,7 +913,9 @@ pub trait Pair: CryptoType + Sized + Clone + Send + Sync + 'static {
///
/// @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`.
Expand Down Expand Up @@ -1207,9 +1236,11 @@ mod tests {
fn generate() -> (Self, <Self as Pair>::Seed) {
(TestPair::Generated, [0u8; 8])
}

fn generate_with_phrase(_password: Option<&str>) -> (Self, String, <Self as Pair>::Seed) {
(TestPair::GeneratedWithPhrase, "".into(), [0u8; 8])
}

fn from_phrase(
phrase: &str,
password: Option<&str>,
Expand All @@ -1222,6 +1253,7 @@ mod tests {
[0u8; 8],
))
}

fn derive<Iter: Iterator<Item = DeriveJunction>>(
&self,
path_iter: Iter,
Expand All @@ -1246,28 +1278,31 @@ mod tests {
None,
))
}
fn from_seed(_seed: &<TestPair as Pair>::Seed) -> Self {
TestPair::Seed(_seed.as_ref().to_owned())
}

fn sign(&self, _message: &[u8]) -> Self::Signature {
[]
}

fn verify<M: AsRef<[u8]>>(_: &Self::Signature, _: M, _: &Self::Public) -> bool {
true
}

fn verify_weak<P: AsRef<[u8]>, M: AsRef<[u8]>>(
_sig: &[u8],
_message: M,
_pubkey: P,
) -> bool {
true
}

fn public(&self) -> Self::Public {
TestPublic
}

fn from_seed_slice(seed: &[u8]) -> Result<Self, SecretStringError> {
Ok(TestPair::Seed(seed.to_owned()))
}

fn to_raw_vec(&self) -> Vec<u8> {
vec![]
}
Expand Down
39 changes: 0 additions & 39 deletions primitives/core/src/ecdsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ use crate::{
crypto::{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")]
Expand Down Expand Up @@ -389,43 +387,6 @@ impl TraitPair for Pair {
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`.
///
Expand Down
41 changes: 0 additions & 41 deletions primitives/core/src/ed25519.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ use crate::crypto::{
};
#[cfg(feature = "full_crypto")]
use crate::crypto::{DeriveJunction, Pair as TraitPair, SecretStringError};
#[cfg(feature = "std")]
use bip39::{Language, Mnemonic, MnemonicType};
#[cfg(feature = "full_crypto")]
use core::convert::TryFrom;
#[cfg(feature = "full_crypto")]
Expand All @@ -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");
Expand Down Expand Up @@ -399,43 +395,6 @@ impl TraitPair for Pair {
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`.
///
Expand Down
Loading