From 4d35b9527234b132fea6f7e149f1d95ed8983c7e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 13 Feb 2023 17:09:34 +0100 Subject: [PATCH] subkey: only decode hex if requested, CLI `0x` prefixed hex for all `stdout` (#13258) * Only decode hex if requested Signed-off-by: Oliver Tale-Yazdi * Cleanup code Signed-off-by: Oliver Tale-Yazdi * Add some tests Signed-off-by: Oliver Tale-Yazdi * Add license Signed-off-by: Oliver Tale-Yazdi * Docs Signed-off-by: Oliver Tale-Yazdi * Clippy Signed-off-by: Oliver Tale-Yazdi * bump version, breaking (tiny) change in output. * Move integration tests to own folder Signed-off-by: Oliver Tale-Yazdi --------- Signed-off-by: Oliver Tale-Yazdi Co-authored-by: Dan Shields --- Cargo.lock | 2 +- bin/utils/subkey/Cargo.toml | 2 +- client/cli/src/commands/mod.rs | 1 + client/cli/src/commands/sign.rs | 75 +++++++--- client/cli/src/commands/test/mod.rs | 21 +++ client/cli/src/commands/test/sig_verify.rs | 152 +++++++++++++++++++++ client/cli/src/commands/utils.rs | 19 +-- client/cli/src/commands/verify.rs | 69 ++++++++-- client/cli/src/params/message_params.rs | 121 ++++++++++++++++ client/cli/src/params/mod.rs | 3 +- 10 files changed, 412 insertions(+), 53 deletions(-) create mode 100644 client/cli/src/commands/test/mod.rs create mode 100644 client/cli/src/commands/test/sig_verify.rs create mode 100644 client/cli/src/params/message_params.rs diff --git a/Cargo.lock b/Cargo.lock index 3e5ab65c0ce20..ad3267984563a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10560,7 +10560,7 @@ dependencies = [ [[package]] name = "subkey" -version = "2.0.2" +version = "3.0.0" dependencies = [ "clap 4.1.4", "sc-cli", diff --git a/bin/utils/subkey/Cargo.toml b/bin/utils/subkey/Cargo.toml index 77c323821a508..5d2d0f7779ced 100644 --- a/bin/utils/subkey/Cargo.toml +++ b/bin/utils/subkey/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "subkey" -version = "2.0.2" +version = "3.0.0" authors = ["Parity Technologies "] edition = "2021" license = "GPL-3.0-or-later WITH Classpath-exception-2.0" diff --git a/client/cli/src/commands/mod.rs b/client/cli/src/commands/mod.rs index 8e84afa34e24a..eeb9f3be3eb14 100644 --- a/client/cli/src/commands/mod.rs +++ b/client/cli/src/commands/mod.rs @@ -31,6 +31,7 @@ mod purge_chain_cmd; mod revert_cmd; mod run_cmd; mod sign; +mod test; pub mod utils; mod vanity; mod verify; diff --git a/client/cli/src/commands/sign.rs b/client/cli/src/commands/sign.rs index 2c3ff3a1575fd..b3da31e56658b 100644 --- a/client/cli/src/commands/sign.rs +++ b/client/cli/src/commands/sign.rs @@ -17,9 +17,13 @@ // along with this program. If not, see . //! Implementation of the `sign` subcommand -use crate::{error, utils, with_crypto_scheme, CryptoSchemeFlag, KeystoreParams}; +use crate::{ + error, params::MessageParams, utils, with_crypto_scheme, CryptoSchemeFlag, KeystoreParams, +}; +use array_bytes::bytes2hex; use clap::Parser; use sp_core::crypto::SecretString; +use std::io::{BufRead, Write}; /// The `sign` command #[derive(Debug, Clone, Parser)] @@ -31,14 +35,9 @@ pub struct SignCmd { #[arg(long)] suri: Option, - /// Message to sign, if not provided you will be prompted to - /// pass the message via STDIN - #[arg(long)] - message: Option, - - /// The message on STDIN is hex-encoded data - #[arg(long)] - hex: bool, + #[allow(missing_docs)] + #[clap(flatten)] + pub message_params: MessageParams, #[allow(missing_docs)] #[clap(flatten)] @@ -52,15 +51,26 @@ pub struct SignCmd { impl SignCmd { /// Run the command pub fn run(&self) -> error::Result<()> { - let message = utils::read_message(self.message.as_ref(), self.hex)?; + let sig = self.sign(|| std::io::stdin().lock())?; + std::io::stdout().lock().write_all(sig.as_bytes())?; + Ok(()) + } + + /// Sign a message. + /// + /// The message can either be provided as immediate argument via CLI or otherwise read from the + /// reader created by `create_reader`. The reader will only be created in case that the message + /// is not passed as immediate. + pub(crate) fn sign(&self, create_reader: F) -> error::Result + where + R: BufRead, + F: FnOnce() -> R, + { + let message = self.message_params.message_from(create_reader)?; let suri = utils::read_uri(self.suri.as_ref())?; let password = self.keystore_params.read_password()?; - let signature = - with_crypto_scheme!(self.crypto_scheme.scheme, sign(&suri, password, message))?; - - println!("{}", signature); - Ok(()) + with_crypto_scheme!(self.crypto_scheme.scheme, sign(&suri, password, message)) } } @@ -70,26 +80,47 @@ fn sign( message: Vec, ) -> error::Result { let pair = utils::pair_from_suri::

(suri, password)?; - Ok(array_bytes::bytes2hex("", pair.sign(&message).as_ref())) + Ok(bytes2hex("0x", pair.sign(&message).as_ref())) } #[cfg(test)] mod test { use super::*; + const SEED: &str = "0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a"; + #[test] - fn sign() { - let seed = "0xad1fb77243b536b90cfe5f0d351ab1b1ac40e3890b41dc64f766ee56340cfca5"; + fn sign_arg() { + let cmd = SignCmd::parse_from(&[ + "sign", + "--suri", + &SEED, + "--message", + &SEED, + "--password", + "12345", + "--hex", + ]); + let sig = cmd.sign(|| std::io::stdin().lock()).expect("Must sign"); + + assert!(sig.starts_with("0x"), "Signature must start with 0x"); + assert!(array_bytes::hex2bytes(&sig).is_ok(), "Signature is valid hex"); + } - let sign = SignCmd::parse_from(&[ + #[test] + fn sign_stdin() { + let cmd = SignCmd::parse_from(&[ "sign", "--suri", - seed, + SEED, "--message", - &seed[2..], + &SEED, "--password", "12345", ]); - assert!(sign.run().is_ok()); + let sig = cmd.sign(|| SEED.as_bytes()).expect("Must sign"); + + assert!(sig.starts_with("0x"), "Signature must start with 0x"); + assert!(array_bytes::hex2bytes(&sig).is_ok(), "Signature is valid hex"); } } diff --git a/client/cli/src/commands/test/mod.rs b/client/cli/src/commands/test/mod.rs new file mode 100644 index 0000000000000..762918f70ff0c --- /dev/null +++ b/client/cli/src/commands/test/mod.rs @@ -0,0 +1,21 @@ +// This file is part of Substrate. + +// Copyright (C) 2023 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Integration tests for subkey commands. + +mod sig_verify; diff --git a/client/cli/src/commands/test/sig_verify.rs b/client/cli/src/commands/test/sig_verify.rs new file mode 100644 index 0000000000000..8d7247480be87 --- /dev/null +++ b/client/cli/src/commands/test/sig_verify.rs @@ -0,0 +1,152 @@ +// This file is part of Substrate. + +// Copyright (C) 2023 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +#![cfg(test)] + +//! Integration test that the `sign` and `verify` sub-commands work together. + +use crate::*; +use clap::Parser; + +const SEED: &str = "0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a"; +const ALICE: &str = "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY"; +const BOB: &str = "5FHneW46xGXgs5mUiveU4sbTyGBzmstUspZC92UhjJM694ty"; + +/// Sign a valid UFT-8 message which can be `hex` and passed either via `stdin` or as an argument. +fn sign(msg: &str, hex: bool, stdin: bool) -> String { + sign_raw(msg.as_bytes(), hex, stdin) +} + +/// Sign a raw message which can be `hex` and passed either via `stdin` or as an argument. +fn sign_raw(msg: &[u8], hex: bool, stdin: bool) -> String { + let mut args = vec!["sign", "--suri", SEED]; + if !stdin { + args.push("--message"); + args.push(std::str::from_utf8(msg).expect("Can only pass valid UTF-8 as arg")); + } + if hex { + args.push("--hex"); + } + let cmd = SignCmd::parse_from(&args); + cmd.sign(|| msg).expect("Static data is good; Must sign; qed") +} + +/// Verify a valid UFT-8 message which can be `hex` and passed either via `stdin` or as an argument. +fn verify(msg: &str, hex: bool, stdin: bool, who: &str, sig: &str) -> bool { + verify_raw(msg.as_bytes(), hex, stdin, who, sig) +} + +/// Verify a raw message which can be `hex` and passed either via `stdin` or as an argument. +fn verify_raw(msg: &[u8], hex: bool, stdin: bool, who: &str, sig: &str) -> bool { + let mut args = vec!["verify", sig, who]; + if !stdin { + args.push("--message"); + args.push(std::str::from_utf8(msg).expect("Can only pass valid UTF-8 as arg")); + } + if hex { + args.push("--hex"); + } + let cmd = VerifyCmd::parse_from(&args); + cmd.verify(|| msg).is_ok() +} + +/// Test that sig/verify works with UTF-8 bytes passed as arg. +#[test] +fn sig_verify_arg_utf8_work() { + let sig = sign("Something", false, false); + + assert!(verify("Something", false, false, ALICE, &sig)); + assert!(!verify("Something", false, false, BOB, &sig)); + + assert!(!verify("Wrong", false, false, ALICE, &sig)); + assert!(!verify("Not hex", true, false, ALICE, &sig)); + assert!(!verify("0x1234", true, false, ALICE, &sig)); + assert!(!verify("Wrong", false, false, BOB, &sig)); + assert!(!verify("Not hex", true, false, BOB, &sig)); + assert!(!verify("0x1234", true, false, BOB, &sig)); +} + +/// Test that sig/verify works with UTF-8 bytes passed via stdin. +#[test] +fn sig_verify_stdin_utf8_work() { + let sig = sign("Something", false, true); + + assert!(verify("Something", false, true, ALICE, &sig)); + assert!(!verify("Something", false, true, BOB, &sig)); + + assert!(!verify("Wrong", false, true, ALICE, &sig)); + assert!(!verify("Not hex", true, true, ALICE, &sig)); + assert!(!verify("0x1234", true, true, ALICE, &sig)); + assert!(!verify("Wrong", false, true, BOB, &sig)); + assert!(!verify("Not hex", true, true, BOB, &sig)); + assert!(!verify("0x1234", true, true, BOB, &sig)); +} + +/// Test that sig/verify works with hex bytes passed as arg. +#[test] +fn sig_verify_arg_hex_work() { + let sig = sign("0xaabbcc", true, false); + + assert!(verify("0xaabbcc", true, false, ALICE, &sig)); + assert!(verify("aabBcc", true, false, ALICE, &sig)); + assert!(verify("0xaAbbCC", true, false, ALICE, &sig)); + assert!(!verify("0xaabbcc", true, false, BOB, &sig)); + + assert!(!verify("0xaabbcc", false, false, ALICE, &sig)); +} + +/// Test that sig/verify works with hex bytes passed via stdin. +#[test] +fn sig_verify_stdin_hex_work() { + let sig = sign("0xaabbcc", true, true); + + assert!(verify("0xaabbcc", true, true, ALICE, &sig)); + assert!(verify("aabBcc", true, true, ALICE, &sig)); + assert!(verify("0xaAbbCC", true, true, ALICE, &sig)); + assert!(!verify("0xaabbcc", true, true, BOB, &sig)); + + assert!(!verify("0xaabbcc", false, true, ALICE, &sig)); +} + +/// Test that sig/verify works with random bytes. +#[test] +fn sig_verify_stdin_non_utf8_work() { + use rand::RngCore; + let mut rng = rand::thread_rng(); + + for _ in 0..100 { + let mut raw = [0u8; 32]; + rng.fill_bytes(&mut raw); + let sig = sign_raw(&raw, false, true); + + assert!(verify_raw(&raw, false, true, ALICE, &sig)); + assert!(!verify_raw(&raw, false, true, BOB, &sig)); + } +} + +/// Test that sig/verify works with invalid UTF-8 bytes. +#[test] +fn sig_verify_stdin_invalid_utf8_work() { + let raw = vec![192u8, 193]; + assert!(String::from_utf8(raw.clone()).is_err(), "Must be invalid UTF-8"); + + let sig = sign_raw(&raw, false, true); + + assert!(verify_raw(&raw, false, true, ALICE, &sig)); + assert!(!verify_raw(&raw, false, true, BOB, &sig)); +} diff --git a/client/cli/src/commands/utils.rs b/client/cli/src/commands/utils.rs index 1ce2b23221691..8ced185d7f3bc 100644 --- a/client/cli/src/commands/utils.rs +++ b/client/cli/src/commands/utils.rs @@ -31,7 +31,7 @@ use sp_core::{ Pair, }; use sp_runtime::{traits::IdentifyAccount, MultiSigner}; -use std::{io::Read, path::PathBuf}; +use std::path::PathBuf; /// Public key type for Runtime pub type PublicFor

=

::Public; @@ -273,23 +273,6 @@ where format!("0x{}", HexDisplay::from(&public_key.into().into_account().as_ref())) } -/// checks if message is Some, otherwise reads message from stdin and optionally decodes hex -pub fn read_message(msg: Option<&String>, should_decode: bool) -> Result, Error> { - let mut message = vec![]; - match msg { - Some(m) => { - message = array_bytes::hex2bytes(m.as_str())?; - }, - None => { - std::io::stdin().lock().read_to_end(&mut message)?; - if should_decode { - message = array_bytes::hex2bytes(array_bytes::hex_bytes2hex_str(&message)?)?; - } - }, - } - Ok(message) -} - /// Allows for calling $method with appropriate crypto impl. #[macro_export] macro_rules! with_crypto_scheme { diff --git a/client/cli/src/commands/verify.rs b/client/cli/src/commands/verify.rs index 82554fbf268fa..183bf507bd114 100644 --- a/client/cli/src/commands/verify.rs +++ b/client/cli/src/commands/verify.rs @@ -18,9 +18,10 @@ //! implementation of the `verify` subcommand -use crate::{error, utils, with_crypto_scheme, CryptoSchemeFlag}; +use crate::{error, params::MessageParams, utils, with_crypto_scheme, CryptoSchemeFlag}; use clap::Parser; use sp_core::crypto::{ByteArray, Ss58Codec}; +use std::io::BufRead; /// The `verify` command #[derive(Debug, Clone, Parser)] @@ -37,14 +38,9 @@ pub struct VerifyCmd { /// If not given, you will be prompted for the URI. uri: Option, - /// Message to verify, if not provided you will be prompted to - /// pass the message via STDIN - #[arg(long)] - message: Option, - - /// The message on STDIN is hex-encoded data - #[arg(long)] - hex: bool, + #[allow(missing_docs)] + #[clap(flatten)] + pub message_params: MessageParams, #[allow(missing_docs)] #[clap(flatten)] @@ -54,7 +50,20 @@ pub struct VerifyCmd { impl VerifyCmd { /// Run the command pub fn run(&self) -> error::Result<()> { - let message = utils::read_message(self.message.as_ref(), self.hex)?; + self.verify(|| std::io::stdin().lock()) + } + + /// Verify a signature for a message. + /// + /// The message can either be provided as immediate argument via CLI or otherwise read from the + /// reader created by `create_reader`. The reader will only be created in case that the message + /// is not passed as immediate. + pub(crate) fn verify(&self, create_reader: F) -> error::Result<()> + where + R: BufRead, + F: FnOnce() -> R, + { + let message = self.message_params.message_from(create_reader)?; let sig_data = array_bytes::hex2bytes(&self.sig)?; let uri = utils::read_uri(self.uri.as_ref())?; let uri = if let Some(uri) = uri.strip_prefix("0x") { uri } else { &uri }; @@ -86,3 +95,43 @@ where Ok(()) } + +#[cfg(test)] +mod test { + use super::*; + + const ALICE: &str = "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY"; + const SIG1: &str = "0x4eb25a2285a82374888880af0024eb30c3a21ce086eae3862888d345af607f0ad6fb081312f11730932564f24a9f8ebcee2d46861413ae61307eca58db2c3e81"; + const SIG2: &str = "0x026342225155056ea797118c1c8c8b3cc002aa2020c36f4217fa3c302783a572ad3dcd38c231cbaf86cadb93984d329c963ceac0685cc1ee4c1ed50fa443a68f"; + + // Verify work with `--message` argument. + #[test] + fn verify_immediate() { + let cmd = VerifyCmd::parse_from(&["verify", SIG1, ALICE, "--message", "test message"]); + assert!(cmd.run().is_ok(), "Alice' signature should verify"); + } + + // Verify work without `--message` argument. + #[test] + fn verify_stdin() { + let cmd = VerifyCmd::parse_from(&["verify", SIG1, ALICE]); + let message = "test message"; + assert!(cmd.verify(|| message.as_bytes()).is_ok(), "Alice' signature should verify"); + } + + // Verify work with `--message` argument for hex message. + #[test] + fn verify_immediate_hex() { + let cmd = VerifyCmd::parse_from(&["verify", SIG2, ALICE, "--message", "0xaabbcc", "--hex"]); + assert!(cmd.run().is_ok(), "Alice' signature should verify"); + } + + // Verify work without `--message` argument for hex message. + #[test] + fn verify_stdin_hex() { + let cmd = VerifyCmd::parse_from(&["verify", SIG2, ALICE, "--hex"]); + assert!(cmd.verify(|| "0xaabbcc".as_bytes()).is_ok()); + assert!(cmd.verify(|| "aabbcc".as_bytes()).is_ok()); + assert!(cmd.verify(|| "0xaABBcC".as_bytes()).is_ok()); + } +} diff --git a/client/cli/src/params/message_params.rs b/client/cli/src/params/message_params.rs new file mode 100644 index 0000000000000..9f88c24dcd222 --- /dev/null +++ b/client/cli/src/params/message_params.rs @@ -0,0 +1,121 @@ +// This file is part of Substrate. + +// Copyright (C) 2023 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Params to configure how a message should be passed into a command. + +use crate::error::Error; +use array_bytes::{hex2bytes, hex_bytes2hex_str}; +use clap::Parser; +use std::io::BufRead; + +/// Params to configure how a message should be passed into a command. +#[derive(Parser, Debug, Clone)] +pub struct MessageParams { + /// Message to process. Will be read from STDIN otherwise. + /// + /// The message is assumed to be raw bytes per default. Use `--hex` for hex input. Can + /// optionally be prefixed with `0x` in the hex case. + #[arg(long)] + message: Option, + + /// The message is hex-encoded data. + #[arg(long)] + hex: bool, +} + +impl MessageParams { + /// Produces the message by either using its immediate value or reading from stdin. + /// + /// This function should only be called once and the result cached. + pub(crate) fn message_from(&self, create_reader: F) -> Result, Error> + where + R: BufRead, + F: FnOnce() -> R, + { + let raw = match &self.message { + Some(raw) => raw.as_bytes().to_vec(), + None => { + let mut raw = vec![]; + create_reader().read_to_end(&mut raw)?; + raw + }, + }; + if self.hex { + hex2bytes(hex_bytes2hex_str(&raw)?).map_err(Into::into) + } else { + Ok(raw) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Test that decoding an immediate message works. + #[test] + fn message_decode_immediate() { + for (name, input, hex, output) in test_closures() { + println!("Testing: immediate_{}", name); + let params = MessageParams { message: Some(input.into()), hex }; + let message = params.message_from(|| std::io::stdin().lock()); + + match output { + Some(output) => { + let message = message.expect(&format!("{}: should decode but did not", name)); + assert_eq!(message, output, "{}: decoded a wrong message", name); + }, + None => { + message.err().expect(&format!("{}: should not decode but did", name)); + }, + } + } + } + + /// Test that decoding a message from a stream works. + #[test] + fn message_decode_stream() { + for (name, input, hex, output) in test_closures() { + println!("Testing: stream_{}", name); + let params = MessageParams { message: None, hex }; + let message = params.message_from(|| input.as_bytes()); + + match output { + Some(output) => { + let message = message.expect(&format!("{}: should decode but did not", name)); + assert_eq!(message, output, "{}: decoded a wrong message", name); + }, + None => { + message.err().expect(&format!("{}: should not decode but did", name)); + }, + } + } + } + + /// Returns (test_name, input, hex, output). + fn test_closures() -> Vec<(&'static str, &'static str, bool, Option<&'static [u8]>)> { + vec![ + ("decode_no_hex_works", "Hello this is not hex", false, Some(b"Hello this is not hex")), + ("decode_no_hex_with_hex_string_works", "0xffffffff", false, Some(b"0xffffffff")), + ("decode_hex_works", "0x00112233", true, Some(&[0, 17, 34, 51])), + ("decode_hex_without_prefix_works", "00112233", true, Some(&[0, 17, 34, 51])), + ("decode_hex_uppercase_works", "0xaAbbCCDd", true, Some(&[170, 187, 204, 221])), + ("decode_hex_wrong_len_errors", "0x0011223", true, None), + ] + } +} diff --git a/client/cli/src/params/mod.rs b/client/cli/src/params/mod.rs index 3197deb101bcc..4ec7d1d958e8f 100644 --- a/client/cli/src/params/mod.rs +++ b/client/cli/src/params/mod.rs @@ -18,6 +18,7 @@ mod database_params; mod import_params; mod keystore_params; +mod message_params; mod network_params; mod node_key_params; mod offchain_worker_params; @@ -35,7 +36,7 @@ use sp_runtime::{ use std::{fmt::Debug, str::FromStr}; pub use crate::params::{ - database_params::*, import_params::*, keystore_params::*, network_params::*, + database_params::*, import_params::*, keystore_params::*, message_params::*, network_params::*, node_key_params::*, offchain_worker_params::*, pruning_params::*, shared_params::*, transaction_pool_params::*, };