subkey: fix hex message decoding and printing logic

client/cli/src/commands/mod.rs

@@ -31,6 +31,7 @@ mod purge_chain_cmd;
 mod revert_cmd;
 mod run_cmd;
 mod sign;
+mod test_sig_verify;
 pub mod utils;
 mod vanity;
 mod verify;

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::*;
 
+ 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");
  }
 }

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 super::*;
+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));
+}

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 {