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 <oliver.tale-yazdi@parity.io>

* Cleanup code

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add some tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add license

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Docs

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Clippy

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* bump version, breaking (tiny) change in output.

* Move integration tests to own folder

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Dan Shields <nukemandan@protonmail.com>

substrate/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;

substrate/client/cli/src/commands/sign.rs

@@ -17,9 +17,13 @@
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
 //! 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<String>,
 
-	/// Message to sign, if not provided you will be prompted to
-	/// pass the message via STDIN
-	#[arg(long)]
-	message: Option<String>,
-
-	/// 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<F, R>(&self, create_reader: F) -> error::Result<String>
+	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<P: sp_core::Pair>(
 	message: Vec<u8>,
 ) -> error::Result<String> {
 	let pair = utils::pair_from_suri::<P>(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::*;
 
-	#[test]
-	fn sign() {
-		let seed = "0xad1fb77243b536b90cfe5f0d351ab1b1ac40e3890b41dc64f766ee56340cfca5";
+	const SEED: &str = "0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a";
 
-		let sign = SignCmd::parse_from(&[
+	#[test]
+	fn sign_arg() {
+		let cmd = SignCmd::parse_from(&[
 			"sign",
 			"--suri",
-			seed,
+			&SEED,
 			"--message",
-			&seed[2..],
+			&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");
+	}
+
+	#[test]
+	fn sign_stdin() {
+		let cmd = SignCmd::parse_from(&[
+			"sign",
+			"--suri",
+			SEED,
+			"--message",
+			&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");
 	}
 }

substrate/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 <https://www.gnu.org/licenses/>.
+
+//! Integration tests for subkey commands.
+
+mod sig_verify;

substrate/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 <https://www.gnu.org/licenses/>.
+
+#![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));
+}

substrate/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<P> = <P as sp_core::Pair>::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<Vec<u8>, 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 {

substrate/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<String>,
 
-	/// Message to verify, if not provided you will be prompted to
-	/// pass the message via STDIN
-	#[arg(long)]
-	message: Option<String>,
-
-	/// 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<F, R>(&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());
+	}
+}