From 566328cd3e81a7ea8e7c28bf9c727506bdbda756 Mon Sep 17 00:00:00 2001 From: Bryant Eisenbach <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 18 Nov 2019 15:00:47 -0500 Subject: [PATCH] Add serde for Signature types (#4109) * refactor: Added `from_slice()` method to ECDSA signatures * doc: Modified ECDSA signature docstring to note Recovery ID * feat: Implemented serde for Signature types Note: using hexstring encoding * feat: Automatically derive serde for MultiSignature * refactor: Convert hex bytes using try_from instead of from_slice Avoids panicking in critical code Implemented from Peer Review * clean: spaces -> tabs * test: Added tests for Signature serialization Added dependency on serde_json for testing purposes --- substrate/Cargo.lock | 1 + substrate/primitives/core/Cargo.toml | 1 + substrate/primitives/core/src/ecdsa.rs | 53 ++++++++++++++++- substrate/primitives/core/src/ed25519.rs | 59 ++++++++++++++++--- substrate/primitives/core/src/sr25519.rs | 59 ++++++++++++++++--- substrate/primitives/sr-primitives/src/lib.rs | 1 + 6 files changed, 157 insertions(+), 17 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 5fe1e3003e..7ef704b972 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -5985,6 +5985,7 @@ dependencies = [ "rustc-hex 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "schnorrkel 0.8.5 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", + "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", "sha2 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)", "sr-std 2.0.0", "substrate-bip39 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/substrate/primitives/core/Cargo.toml b/substrate/primitives/core/Cargo.toml index f9888928dc..ea7bffb6a5 100644 --- a/substrate/primitives/core/Cargo.toml +++ b/substrate/primitives/core/Cargo.toml @@ -44,6 +44,7 @@ pretty_assertions = "0.6.1" hex-literal = "0.2.1" rand = "0.7.2" criterion = "0.2.11" +serde_json = "1.0" [[bench]] name = "bench" diff --git a/substrate/primitives/core/src/ecdsa.rs b/substrate/primitives/core/src/ecdsa.rs index f1d4d2446a..7ba750f49b 100644 --- a/substrate/primitives/core/src/ecdsa.rs +++ b/substrate/primitives/core/src/ecdsa.rs @@ -172,7 +172,7 @@ impl rstd::hash::Hash for Public { } } -/// A signature (a 512-bit value). +/// A signature (a 512-bit value, plus 8 bits for recovery ID). #[derive(Encode, Decode)] pub struct Signature([u8; 65]); @@ -190,6 +190,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature { } } +#[cfg(feature = "std")] +impl Serialize for Signature { + fn serialize(&self, serializer: S) -> Result where S: Serializer { + serializer.serialize_str(&hex::encode(self)) + } +} + +#[cfg(feature = "std")] +impl<'de> Deserialize<'de> for Signature { + fn deserialize(deserializer: D) -> Result where D: Deserializer<'de> { + let signature_hex = hex::decode(&String::deserialize(deserializer)?) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?; + Ok(Signature::try_from(signature_hex.as_ref()) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?) + } +} + impl Clone for Signature { fn clone(&self) -> Self { let mut r = [0u8; 65]; @@ -259,6 +276,16 @@ impl Signature { Signature(data) } + /// A new instance from the given slice that should be 65 bytes long. + /// + /// NOTE: No checking goes on to ensure this is a real signature. Only use it if + /// you are certain that the array actually is a signature. GIGO! + pub fn from_slice(data: &[u8]) -> Self { + let mut r = [0u8; 65]; + r.copy_from_slice(data); + Signature(r) + } + /// Recover the public key from this signature and a message. #[cfg(feature = "full_crypto")] pub fn recover>(&self, message: M) -> Option { @@ -501,6 +528,7 @@ mod test { use super::*; use hex_literal::hex; use crate::crypto::DEV_PHRASE; + use serde_json; #[test] fn default_phrase_should_be_used() { @@ -613,4 +641,27 @@ mod test { let cmp = Public::from_ss58check(&s).unwrap(); assert_eq!(cmp, public); } + + #[test] + fn signature_serialization_works() { + let pair = Pair::from_seed(b"12345678901234567890123456789012"); + let message = b"Something important"; + let signature = pair.sign(&message[..]); + let serialized_signature = serde_json::to_string(&signature).unwrap(); + // Signature is 65 bytes, so 130 chars + 2 quote chars + assert_eq!(serialized_signature.len(), 132); + let signature = serde_json::from_str(&serialized_signature).unwrap(); + assert!(Pair::verify(&signature, &message[..], &pair.public())); + } + + #[test] + fn signature_serialization_doesnt_panic() { + fn deserialize_signature(text: &str) -> Result { + Ok(serde_json::from_str(text)?) + } + assert!(deserialize_signature("Not valid json.").is_err()); + assert!(deserialize_signature("\"Not an actual signature.\"").is_err()); + // Poorly-sized + assert!(deserialize_signature("\"abc123\"").is_err()); + } } diff --git a/substrate/primitives/core/src/ed25519.rs b/substrate/primitives/core/src/ed25519.rs index 2b79f92d80..37933ee678 100644 --- a/substrate/primitives/core/src/ed25519.rs +++ b/substrate/primitives/core/src/ed25519.rs @@ -26,6 +26,8 @@ use codec::{Encode, Decode}; #[cfg(feature = "full_crypto")] use blake2_rfc; +#[cfg(feature = "full_crypto")] +use core::convert::TryFrom; #[cfg(feature = "std")] use substrate_bip39::seed_from_entropy; #[cfg(feature = "std")] @@ -85,11 +87,11 @@ impl AsMut<[u8]> for Public { } impl Deref for Public { - type Target = [u8]; + type Target = [u8]; - fn deref(&self) -> &Self::Target { - &self.0 - } + fn deref(&self) -> &Self::Target { + &self.0 + } } impl rstd::convert::TryFrom<&[u8]> for Public { @@ -127,11 +129,11 @@ impl From for H256 { #[cfg(feature = "std")] impl std::str::FromStr for Public { - type Err = crate::crypto::PublicError; + type Err = crate::crypto::PublicError; - fn from_str(s: &str) -> Result { - Self::from_ss58check(s) - } + fn from_str(s: &str) -> Result { + Self::from_ss58check(s) + } } impl UncheckedFrom<[u8; 32]> for Public { @@ -199,6 +201,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature { } } +#[cfg(feature = "std")] +impl Serialize for Signature { + fn serialize(&self, serializer: S) -> Result where S: Serializer { + serializer.serialize_str(&hex::encode(self)) + } +} + +#[cfg(feature = "std")] +impl<'de> Deserialize<'de> for Signature { + fn deserialize(deserializer: D) -> Result where D: Deserializer<'de> { + let signature_hex = hex::decode(&String::deserialize(deserializer)?) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?; + Ok(Signature::try_from(signature_hex.as_ref()) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?) + } +} + impl Clone for Signature { fn clone(&self) -> Self { let mut r = [0u8; 64]; @@ -531,6 +550,7 @@ mod test { use super::*; use hex_literal::hex; use crate::crypto::DEV_PHRASE; + use serde_json; #[test] fn default_phrase_should_be_used() { @@ -643,4 +663,27 @@ mod test { let cmp = Public::from_ss58check(&s).unwrap(); assert_eq!(cmp, public); } + + #[test] + fn signature_serialization_works() { + let pair = Pair::from_seed(b"12345678901234567890123456789012"); + let message = b"Something important"; + let signature = pair.sign(&message[..]); + let serialized_signature = serde_json::to_string(&signature).unwrap(); + // Signature is 64 bytes, so 128 chars + 2 quote chars + assert_eq!(serialized_signature.len(), 130); + let signature = serde_json::from_str(&serialized_signature).unwrap(); + assert!(Pair::verify(&signature, &message[..], &pair.public())); + } + + #[test] + fn signature_serialization_doesnt_panic() { + fn deserialize_signature(text: &str) -> Result { + Ok(serde_json::from_str(text)?) + } + assert!(deserialize_signature("Not valid json.").is_err()); + assert!(deserialize_signature("\"Not an actual signature.\"").is_err()); + // Poorly-sized + assert!(deserialize_signature("\"abc123\"").is_err()); + } } diff --git a/substrate/primitives/core/src/sr25519.rs b/substrate/primitives/core/src/sr25519.rs index 668bf1f0b4..9521e3bb6c 100644 --- a/substrate/primitives/core/src/sr25519.rs +++ b/substrate/primitives/core/src/sr25519.rs @@ -26,6 +26,8 @@ use rstd::vec::Vec; use schnorrkel::{signing_context, ExpansionMode, Keypair, SecretKey, MiniSecretKey, PublicKey, derive::{Derivation, ChainCode, CHAIN_CODE_LENGTH} }; +#[cfg(feature = "full_crypto")] +use core::convert::TryFrom; #[cfg(feature = "std")] use substrate_bip39::mini_secret_from_entropy; #[cfg(feature = "std")] @@ -91,11 +93,11 @@ impl AsMut<[u8]> for Public { } impl Deref for Public { - type Target = [u8]; + type Target = [u8]; - fn deref(&self) -> &Self::Target { - &self.0 - } + fn deref(&self) -> &Self::Target { + &self.0 + } } impl From for [u8; 32] { @@ -112,11 +114,11 @@ impl From for H256 { #[cfg(feature = "std")] impl std::str::FromStr for Public { - type Err = crate::crypto::PublicError; + type Err = crate::crypto::PublicError; - fn from_str(s: &str) -> Result { - Self::from_ss58check(s) - } + fn from_str(s: &str) -> Result { + Self::from_ss58check(s) + } } impl rstd::convert::TryFrom<&[u8]> for Public { @@ -200,6 +202,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature { } } +#[cfg(feature = "std")] +impl Serialize for Signature { + fn serialize(&self, serializer: S) -> Result where S: Serializer { + serializer.serialize_str(&hex::encode(self)) + } +} + +#[cfg(feature = "std")] +impl<'de> Deserialize<'de> for Signature { + fn deserialize(deserializer: D) -> Result where D: Deserializer<'de> { + let signature_hex = hex::decode(&String::deserialize(deserializer)?) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?; + Ok(Signature::try_from(signature_hex.as_ref()) + .map_err(|e| de::Error::custom(format!("{:?}", e)))?) + } +} + impl Clone for Signature { fn clone(&self) -> Self { let mut r = [0u8; 64]; @@ -606,6 +625,7 @@ mod test { use super::*; use crate::crypto::{Ss58Codec, DEV_PHRASE, DEV_ADDRESS}; use hex_literal::hex; + use serde_json; #[test] fn default_phrase_should_be_used() { @@ -748,4 +768,27 @@ mod test { )); assert!(Pair::verify(&js_signature, b"SUBSTRATE", &public)); } + + #[test] + fn signature_serialization_works() { + let pair = Pair::from_seed(b"12345678901234567890123456789012"); + let message = b"Something important"; + let signature = pair.sign(&message[..]); + let serialized_signature = serde_json::to_string(&signature).unwrap(); + // Signature is 64 bytes, so 128 chars + 2 quote chars + assert_eq!(serialized_signature.len(), 130); + let signature = serde_json::from_str(&serialized_signature).unwrap(); + assert!(Pair::verify(&signature, &message[..], &pair.public())); + } + + #[test] + fn signature_serialization_doesnt_panic() { + fn deserialize_signature(text: &str) -> Result { + Ok(serde_json::from_str(text)?) + } + assert!(deserialize_signature("Not valid json.").is_err()); + assert!(deserialize_signature("\"Not an actual signature.\"").is_err()); + // Poorly-sized + assert!(deserialize_signature("\"abc123\"").is_err()); + } } diff --git a/substrate/primitives/sr-primitives/src/lib.rs b/substrate/primitives/sr-primitives/src/lib.rs index 01ace2c004..33759f5d72 100644 --- a/substrate/primitives/sr-primitives/src/lib.rs +++ b/substrate/primitives/sr-primitives/src/lib.rs @@ -164,6 +164,7 @@ impl BuildStorage for (StorageOverlay, ChildrenStorageOverlay) { pub type ConsensusEngineId = [u8; 4]; /// Signature verify that can work with any known signature types.. +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Eq, PartialEq, Clone, Encode, Decode, RuntimeDebug)] pub enum MultiSignature { /// An Ed25519 signature.