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

inspect-key: Adds support for expect-public #10430

Merged
merged 9 commits into from
Dec 11, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
131 changes: 131 additions & 0 deletions client/cli/src/commands/inspect_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ use crate::{
utils::{self, print_from_public, print_from_uri},
with_crypto_scheme, CryptoSchemeFlag, Error, KeystoreParams, NetworkSchemeFlag, OutputTypeFlag,
};
use sp_core::crypto::{ExposeSecret, SecretString, SecretUri, Ss58Codec};
use std::str::FromStr;
use structopt::StructOpt;

/// The `inspect` command
#[derive(Debug, StructOpt)]
#[structopt(
Expand Down Expand Up @@ -59,6 +62,18 @@ pub struct InspectKeyCmd {
#[allow(missing_docs)]
#[structopt(flatten)]
pub crypto_scheme: CryptoSchemeFlag,

/// Expect that `--uri` has the given public key/account-id.
///
/// If `--uri` has any derivations, the public key is checked against the base `uri`, aka the
bkchr marked this conversation as resolved.
Show resolved Hide resolved
/// `uri` without any derivation applied. However, if `uri` has a password or there is one
/// given by `--password`, it will be used to decrypt `uri` before comparing the public
/// key/account-id.
///
/// If there is no derivation in `--uri`, the public key will be checked against the public key
/// of `--uri` directly.
#[structopt(long, conflicts_with = "public")]
pub expect_public: Option<String>,
}

impl InspectKeyCmd {
Expand All @@ -77,6 +92,13 @@ impl InspectKeyCmd {
)
)?;
} else {
if let Some(ref expect_public) = self.expect_public {
with_crypto_scheme!(
self.crypto_scheme.scheme,
expect_public_from_phrase(&&expect_public, &uri, password.as_ref(),)
)?;
}

with_crypto_scheme!(
self.crypto_scheme.scheme,
print_from_uri(
Expand All @@ -92,9 +114,50 @@ impl InspectKeyCmd {
}
}

/// Checks that `expect_public` is the public key of `suri`.
///
/// If `suri` has any derivations, `expect_public` is checked against the public key of the "bare"
/// `suri`, aka without any derivations.
bkchr marked this conversation as resolved.
Show resolved Hide resolved
///
/// Returns an error if the public key does not match.
fn expect_public_from_phrase<Pair: sp_core::Pair>(
expect_public: &str,
suri: &str,
password: Option<&SecretString>,
) -> Result<(), Error> {
let secret_uri = SecretUri::from_str(suri).map_err(|e| format!("{:?}", e))?;
let expected_public = if let Some(public) = expect_public.strip_prefix("0x") {
let hex_public = hex::decode(&public)
.map_err(|_| format!("Invalid expected public key hex: `{}`", expect_public))?;
Pair::Public::try_from(&hex_public)
.map_err(|_| format!("Invalid expected public key: `{}`", expect_public))?
} else {
Pair::Public::from_string_with_version(expect_public)
.map_err(|_| format!("Invalid expected account id: `{}`", expect_public))?
.0
};

let pair = Pair::from_string_with_seed(
secret_uri.phrase.expose_secret().as_str(),
password
.or_else(|| secret_uri.password.as_ref())
.map(|p| p.expose_secret().as_str()),
)
.map_err(|_| format!("Invalid secret uri: {}", suri))?
.0;

if pair.public() == expected_public {
Ok(())
} else {
Err(format!("Expected public ({}) key does not match.", expect_public).into())
}
}

#[cfg(test)]
mod tests {
use super::*;
use sp_core::crypto::{Pair, Public};
use sp_runtime::traits::IdentifyAccount;
use structopt::StructOpt;

#[test]
Expand All @@ -117,4 +180,72 @@ mod tests {
let inspect = InspectKeyCmd::from_iter(&["inspect-key", "--public", public]);
assert!(inspect.run().is_ok());
}

#[test]
fn inspect_with_expected_public_key() {
let check_cmd = |seed, expected_public, success| {
let inspect = InspectKeyCmd::from_iter(&[
"inspect-key",
"--expect-public",
expected_public,
seed,
]);
let res = inspect.run();

if success {
assert!(res.is_ok());
} else {
assert!(res.unwrap_err().to_string().contains(&format!(
"Expected public ({}) key does not match.",
expected_public
)));
}
};

let seed =
"remember fiber forum demise paper uniform squirrel feel access exclude casual effort";
let invalid_public = "0x12e76e0ae8ce41b6516cce52b3f23a08dcb4cfeed53c6ee8f5eb9f7367341069";
let valid_public = sp_core::sr25519::Pair::from_string_with_seed(seed, None)
.expect("Valid")
.0
.public();
let valid_public_hex = format!("0x{}", hex::encode(valid_public.as_slice()));
let valid_accountid = format!("{}", valid_public.into_account());

// It should fail with the invalid public key
check_cmd(seed, invalid_public, false);

// It should work with the valid public key & account id
check_cmd(seed, &valid_public_hex, true);
check_cmd(seed, &valid_accountid, true);

let password = "test12245";
let seed_with_password = format!("{}///{}", seed, password);
let valid_public_with_password =
sp_core::sr25519::Pair::from_string_with_seed(&seed_with_password, Some(password))
.expect("Valid")
.0
.public();
let valid_public_hex_with_password =
format!("0x{}", hex::encode(&valid_public_with_password.as_slice()));
let valid_accountid_with_password =
format!("{}", &valid_public_with_password.into_account());

// Only the public key that corresponds to the seed with password should be accepted.
check_cmd(&seed_with_password, &valid_public_hex, false);
check_cmd(&seed_with_password, &valid_accountid, false);

check_cmd(&seed_with_password, &valid_public_hex_with_password, true);
check_cmd(&seed_with_password, &valid_accountid_with_password, true);

let seed_with_password_and_derivation = format!("{}//test//account///{}", seed, password);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters much but we could also check that providing the public key for the derived account fails (since we check against root key).


// They should still be valid, because we check the base secret key.
check_cmd(&seed_with_password_and_derivation, &valid_public_hex_with_password, true);
check_cmd(&seed_with_password_and_derivation, &valid_accountid_with_password, true);

// And these should be invalid.
check_cmd(&seed_with_password_and_derivation, &valid_public_hex, false);
check_cmd(&seed_with_password_and_derivation, &valid_accountid, false);
}
}
117 changes: 108 additions & 9 deletions primitives/core/src/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ pub trait Public:
fn as_slice(&self) -> &[u8] {
self.as_ref()
}

/// Return `CryptoTypePublicPair` from public key.
fn to_public_crypto_pair(&self) -> CryptoTypePublicPair;
}
Expand Down Expand Up @@ -709,6 +710,104 @@ mod dummy {
}
}

/// A secret uri (`SURI`) that can be used to generate a key pair.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

///
/// The `SURI` can be parsed from a string. The string is interpreted in the following way:
///
/// - If `string` is a possibly `0x` prefixed 64-digit hex string, then it will be interpreted
/// directly as a `MiniSecretKey` (aka "seed" in `subkey`).
/// - If `string` is a valid BIP-39 key phrase of 12, 15, 18, 21 or 24 words, then the key will
/// be derived from it. In this case:
/// - the phrase may be followed by one or more items delimited by `/` characters.
/// - the path may be followed by `///`, in which case everything after the `///` is treated
/// as a password.
/// - If `string` begins with a `/` character it is prefixed with the Substrate public `DEV_PHRASE`
/// and interpreted as above.
///
/// In this case they are interpreted as HDKD junctions; purely numeric items are interpreted as
/// integers, non-numeric items as strings. Junctions prefixed with `/` are interpreted as soft
/// junctions, and with `//` as hard junctions.
///
/// There is no correspondence mapping between `SURI` strings and the keys they represent.
/// Two different non-identical strings can actually lead to the same secret being derived.
/// Notably, integer junction indices may be legally prefixed with arbitrary number of zeros.
/// Similarly an empty password (ending the `SURI` with `///`) is perfectly valid and will
/// generally be equivalent to no password at all.
///
/// # Example
///
/// Parse [`DEV_PHRASE`] secret uri with junction:
///
/// ```
/// # use sp_core::crypto::{SecretUri, DeriveJunction, DEV_PHRASE, ExposeSecret};
/// # use std::str::FromStr;
/// let suri = SecretUri::from_str("//Alice").expect("Parse SURI");
///
/// assert_eq!(vec![DeriveJunction::from("Alice").harden()], suri.junctions);
/// assert_eq!(DEV_PHRASE, suri.phrase.expose_secret());
/// assert!(suri.password.is_none());
/// ```
///
/// Parse [`DEV_PHRASE`] secret ui with junction and password:
///
/// ```
/// # use sp_core::crypto::{SecretUri, DeriveJunction, DEV_PHRASE, ExposeSecret};
/// # use std::str::FromStr;
/// let suri = SecretUri::from_str("//Alice///SECRET_PASSWORD").expect("Parse SURI");
///
/// assert_eq!(vec![DeriveJunction::from("Alice").harden()], suri.junctions);
/// assert_eq!(DEV_PHRASE, suri.phrase.expose_secret());
/// assert_eq!("SECRET_PASSWORD", suri.password.unwrap().expose_secret());
/// ```
///
/// Parse [`DEV_PHRASE`] secret ui with hex phrase and junction:
///
/// ```
/// # use sp_core::crypto::{SecretUri, DeriveJunction, DEV_PHRASE, ExposeSecret};
/// # use std::str::FromStr;
/// let suri = SecretUri::from_str("0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a//Alice").expect("Parse SURI");
///
/// assert_eq!(vec![DeriveJunction::from("Alice").harden()], suri.junctions);
/// assert_eq!("0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a", suri.phrase.expose_secret());
/// assert!(suri.password.is_none());
/// ```
#[cfg(feature = "std")]
pub struct SecretUri {
/// The phrase to derive the private key.
///
/// This can either be a 64-bit hex string or a BIP-39 key phrase.
pub phrase: SecretString,
/// Optional password as given as part of the uri.
pub password: Option<SecretString>,
/// The junctions as part of the uri.
pub junctions: Vec<DeriveJunction>,
}

#[cfg(feature = "std")]
impl sp_std::str::FromStr for SecretUri {
type Err = SecretStringError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let cap = SECRET_PHRASE_REGEX.captures(s).ok_or(SecretStringError::InvalidFormat)?;

let junctions = JUNCTION_REGEX
.captures_iter(&cap["path"])
.map(|f| DeriveJunction::from(&f[1]))
.collect::<Vec<_>>();

let phrase = cap.name("phrase").map(|r| r.as_str()).unwrap_or(DEV_PHRASE);
let password = cap.name("password");

Ok(Self {
phrase: SecretString::from_str(phrase).expect("Returns infallible error; qed"),
password: password.map(|v| {
SecretString::from_str(v.as_str()).expect("Returns infallible error; qed")
}),
junctions,
})
}
}

/// Trait suitable for typical cryptographic PKI key pair type.
///
/// For now it just specifies how to create a key from a phrase and derivation path.
Expand Down Expand Up @@ -821,14 +920,12 @@ pub trait Pair: CryptoType + Sized + Clone + Send + Sync + 'static {
s: &str,
password_override: Option<&str>,
) -> Result<(Self, Option<Self::Seed>), SecretStringError> {
let cap = SECRET_PHRASE_REGEX.captures(s).ok_or(SecretStringError::InvalidFormat)?;

let path = JUNCTION_REGEX.captures_iter(&cap["path"]).map(|f| DeriveJunction::from(&f[1]));

let phrase = cap.name("phrase").map(|r| r.as_str()).unwrap_or(DEV_PHRASE);
let password = password_override.or_else(|| cap.name("password").map(|m| m.as_str()));
use sp_std::str::FromStr;
let SecretUri { junctions, phrase, password } = SecretUri::from_str(s)?;
let password =
password_override.or_else(|| password.as_ref().map(|p| p.expose_secret().as_str()));

let (root, seed) = if let Some(stripped) = phrase.strip_prefix("0x") {
let (root, seed) = if let Some(stripped) = phrase.expose_secret().strip_prefix("0x") {
hex::decode(stripped)
.ok()
.and_then(|seed_vec| {
Expand All @@ -842,9 +939,11 @@ pub trait Pair: CryptoType + Sized + Clone + Send + Sync + 'static {
})
.ok_or(SecretStringError::InvalidSeed)?
} else {
Self::from_phrase(phrase, password).map_err(|_| SecretStringError::InvalidPhrase)?
Self::from_phrase(phrase.expose_secret().as_str(), password)
.map_err(|_| SecretStringError::InvalidPhrase)?
};
root.derive(path, Some(seed)).map_err(|_| SecretStringError::InvalidPath)
root.derive(junctions.into_iter(), Some(seed))
.map_err(|_| SecretStringError::InvalidPath)
}

/// Interprets the string `s` in order to generate a key pair.
Expand Down