Introduce sign_with method in keystore (#4925)

* Add KEY_KIND_ID to the public trait This change is being introduced for the purpose of identifying a public key with it's identifier and algorithm "kind". * Use `sign_with` as implemented in BareCryptoStore * Implement `sign_with` in sc_keystore * Fix inconsistencies, use *_KIND_ID in sp_core testing * Rename KeyKindId to CryptoTypeId * Remove pair-returning functions from BareCryptoStore trait * Define CryptoTypeId in app-crypto macros * Add functions to get keys supported by keystore * Fix sign_with signature to include CryptoTypePublicPair * Add `sign_with_any` and `sign_with_all` * Use keystore.sign_with in auth_discovery * Rename get_supported_keys -> supported_keys * Added headers to function docstrings * Use chain instead of extending a temp vector * Fixed some code formatting * Restrict size of CryptoTypeId This is to be able to use Encode/Decode derives and the overcome having the size being unknown at compile-time. * Implement sign_with in the trait itself * Remove whitespace * Use key_type also as a CryptoTypeId in app_crypto macros * Rename `get_keys` to `keys` in BareCryptoStore * Remove usage of key_pair funcs in tests * Adjust docstring for *_CYPTO_ID constants * Fix failures * Simplify mapping on keys * Remove one let * Fixed typo * PR feedback * remove whitespace * Zip keys and signatures * Use into_iter & remove cloned * Pass index to MissingSignature * Use typed errors instead of strings for BareCryptoStore * Implement Debug for trait error * Use hashsets for better performance for supported_keys * Make sure keys are inserted into the keystore * Make sign_with_all return type consistent with `sign_with` * Rename Error to BareCryptoStoreError * Rename CRYPT_TYPE_ID -> CRYPTO_ID * Remove unnecessary CRYPTO_ID declaration in Public trait * Convert pub key to CryptoTypePublicPair * Fix use * Fix code style * Implement From on CryptoTypePublicPair in app_crypto macros * Change CryptoTypePublicPair to a struct * Implement Display on CryptoTypePublicPair * Pass CryptoTypePublicPair to MissingSignature error * Adjust docs according to function signature * Unify keys implementation * Fix RPC author tests * Fix stackoverflow * Tabify spaces * Pass KeyTypeId to error for easier debugging * Fix asserts * Use ToHex to format public key * Use constants from sp_core * Rename testing KeyTypeId constants * Please compiler * Restore KeyTypeId names apparently, they're not only used in tests * Use BareCryptoStoreError instead of String * Document return value * Fix borrow check * Convert to hashset internally * WIP - iter_keys * Return raw_public_keys * Address PR feedback * Address PR Feedback * Fix hexdisplay import error * Update primitives/core/src/traits.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
2026-06-15 06:51:07 +00:00 · 2020-03-30 13:18:59 +02:00
parent 462eaa3f41
commit e17a23e907
17 changed files with 474 additions and 152 deletions
@@ -16,6 +16,8 @@

 //! Authority discovery errors.

+use sp_core::crypto::CryptoTypePublicPair;
+
 /// AuthorityDiscovery Result.
 pub type Result<T> = std::result::Result<T, Error>;

@@ -46,6 +48,10 @@ pub enum Error {
 	EncodingDecodingScale(codec::Error),
 	/// Failed to parse a libp2p multi address.
 	ParsingMultiaddress(libp2p::core::multiaddr::Error),
+	/// Failed to sign using a specific public key
+	MissingSignature(CryptoTypePublicPair),
+	/// Failed to sign using all public keys
+	Signing,
 	/// Failed to register Prometheus metric.
 	Prometheus(prometheus_endpoint::PrometheusError),
 }
@@ -63,7 +63,7 @@ use prost::Message;
 use sc_client_api::blockchain::HeaderBackend;
 use sc_network::{DhtEvent, ExHashT, NetworkStateInfo};
 use sp_authority_discovery::{AuthorityDiscoveryApi, AuthorityId, AuthoritySignature, AuthorityPair};
-use sp_core::crypto::{key_types, Pair};
+use sp_core::crypto::{key_types, CryptoTypePublicPair, Pair};
 use sp_core::traits::BareCryptoStorePtr;
 use sp_runtime::{traits::Block as BlockT, generic::BlockId};
 use sp_api::ProvideRuntimeApi;
@@ -283,19 +283,36 @@ where
 			.encode(&mut serialized_addresses)
 			.map_err(Error::EncodingProto)?;

-		for key in self.get_priv_keys_within_authority_set()?.into_iter() {
-			let signature = key.sign(&serialized_addresses);
+		let keys: Vec<CryptoTypePublicPair> = self.get_own_public_keys_within_authority_set()?
+			.into_iter()
+			.map(Into::into)
+			.collect();

+		let signatures = self.key_store
+			.read()
+			.sign_with_all(
+				key_types::AUTHORITY_DISCOVERY,
+				keys.clone(),
+				serialized_addresses.as_slice(),
+			)
+			.map_err(|_| Error::Signing)?;
+
+		for (sign_result, key) in signatures.iter().zip(keys) {
 			let mut signed_addresses = vec![];
+
+			// sign_with_all returns Result<Signature, Error> signature
+			// is generated for a public key that is supported.
+			// Verify that all signatures exist for all provided keys.
+			let signature = sign_result.as_ref().map_err(|_| Error::MissingSignature(key.clone()))?;
 			schema::SignedAuthorityAddresses {
 				addresses: serialized_addresses.clone(),
-				signature: signature.encode(),
+				signature: Encode::encode(&signature),
 			}
 			.encode(&mut signed_addresses)
 				.map_err(Error::EncodingProto)?;

 			self.network.put_value(
-				hash_authority_id(key.public().as_ref())?,
+				hash_authority_id(key.1.as_ref())?,
 				signed_addresses,
 			);
 		}
@@ -446,21 +463,6 @@ where
 		Ok(())
 	}

-	/// Retrieve all local authority discovery private keys that are within the current authority
-	/// set.
-	fn get_priv_keys_within_authority_set(&mut self) -> Result<Vec<AuthorityPair>> {
-		let keys = self.get_own_public_keys_within_authority_set()?
-			.into_iter()
-			.map(std::convert::Into::into)
-			.filter_map(|pub_key| {
-				self.key_store.read().sr25519_key_pair(key_types::AUTHORITY_DISCOVERY, &pub_key)
-			})
-			.map(std::convert::Into::into)
-			.collect();
-
-		Ok(keys)
-	}
-
 	/// Retrieve our public keys within the current authority set.
 	//
 	// A node might have multiple authority discovery keys within its keystore, e.g. an old one and
@@ -17,10 +17,11 @@
 //! Keystore (and session key management) for ed25519 based chains like Polkadot.

 #![warn(missing_docs)]
-
-use std::{collections::HashMap, path::PathBuf, fs::{self, File}, io::{self, Write}, sync::Arc};
+use std::{collections::{HashMap, HashSet}, path::PathBuf, fs::{self, File}, io::{self, Write}, sync::Arc};
 use sp_core::{
-	crypto::{KeyTypeId, Pair as PairT, Public, IsWrappedBy, Protected}, traits::BareCryptoStore,
+	crypto::{IsWrappedBy, CryptoTypePublicPair, KeyTypeId, Pair as PairT, Protected, Public},
+	traits::{BareCryptoStore, BareCryptoStoreError as TraitError},
+	Encode,
 };
 use sp_application_crypto::{AppKey, AppPublic, AppPair, ed25519, sr25519};
 use parking_lot::RwLock;
@@ -44,6 +45,12 @@ pub enum Error {
 	/// Invalid seed
 	#[display(fmt="Invalid seed")]
 	InvalidSeed,
+	/// Public key type is not supported
+	#[display(fmt="Key crypto type is not supported")]
+	KeyNotSupported(KeyTypeId),
+	/// Pair not found for public key and KeyTypeId
+	#[display(fmt="Pair not found for {} public key", "_0")]
+	PairNotFound(String),
 	/// Keystore unavailable
 	#[display(fmt="Keystore unavailable")]
 	Unavailable,
@@ -52,6 +59,21 @@ pub enum Error {
 /// Keystore Result
 pub type Result<T> = std::result::Result<T, Error>;

+impl From<Error> for TraitError {
+	fn from(error: Error) -> Self {
+		match error {
+			Error::KeyNotSupported(id) => TraitError::KeyNotSupported(id),
+			Error::PairNotFound(e) => TraitError::PairNotFound(e),
+			Error::InvalidSeed | Error::InvalidPhrase | Error::InvalidPassword => {
+				TraitError::ValidationError(error.to_string())
+			},
+			Error::Unavailable => TraitError::Unavailable,
+			Error::Io(e) => TraitError::Other(e.to_string()),
+			Error::Json(e) => TraitError::Other(e.to_string()),
+		}
+	}
+}
+
 impl std::error::Error for Error {
 	fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
 		match self {
@@ -220,16 +242,35 @@ impl Store {
 		self.key_pair_by_type::<Pair::Generic>(IsWrappedBy::from_ref(public), Pair::ID).map(Into::into)
 	}

-	/// Get public keys of all stored keys that match the given key type.
-	pub fn public_keys_by_type<TPublic: Public>(&self, key_type: KeyTypeId) -> Result<Vec<TPublic>> {
-		let mut public_keys: Vec<TPublic> = self.additional.keys()
-			.filter_map(|(ty, public)| {
-				if *ty == key_type {
-					Some(TPublic::from_slice(public))
-				} else {
-					None
-				}
+	/// Get public keys of all stored keys that match the key type.
+	///
+	/// This will just use the type of the public key (a list of which to be returned) in order
+	/// to determine the key type. Unless you use a specialized application-type public key, then
+	/// this only give you keys registered under generic cryptography, and will not return keys
+	/// registered under the application type.
+	pub fn public_keys<Public: AppPublic>(&self) -> Result<Vec<Public>> {
+		self.raw_public_keys(Public::ID)
+			.map(|v| {
+				v.into_iter()
+				 .map(|k| Public::from_slice(k.as_slice()))
+				 .collect()
 			})
+	}
+
+	/// Returns the file path for the given public key and key type.
+	fn key_file_path(&self, public: &[u8], key_type: KeyTypeId) -> Option<PathBuf> {
+		let mut buf = self.path.as_ref()?.clone();
+		let key_type = hex::encode(key_type.0);
+		let key = hex::encode(public);
+		buf.push(key_type + key.as_str());
+		Some(buf)
+	}
+
+	/// Returns a list of raw public keys filtered by `KeyTypeId`
+	fn raw_public_keys(&self, id: KeyTypeId) -> Result<Vec<Vec<u8>>> {
+		let mut public_keys: Vec<Vec<u8>> = self.additional.keys()
+			.into_iter()
+    		.filter_map(|k| if k.0 == id { Some(k.1.clone()) } else { None })
 			.collect();

 		if let Some(path) = &self.path {
@@ -241,8 +282,10 @@ impl Store {
 				if let Some(name) = path.file_name().and_then(|n| n.to_str()) {
 					match hex::decode(name) {
 						Ok(ref hex) if hex.len() > 4 => {
-							if &hex[0..4] != &key_type.0 { continue }
-							let public = TPublic::from_slice(&hex[4..]);
+							if &hex[0..4] != &id.0 {
+								continue;
+							}
+							let public = hex[4..].to_vec();
 							public_keys.push(public);
 						}
 						_ => continue,
@@ -253,71 +296,104 @@ impl Store {

 		Ok(public_keys)
 	}
-
-	/// Get public keys of all stored keys that match the key type.
-	///
-	/// This will just use the type of the public key (a list of which to be returned) in order
-	/// to determine the key type. Unless you use a specialized application-type public key, then
-	/// this only give you keys registered under generic cryptography, and will not return keys
-	/// registered under the application type.
-	pub fn public_keys<Public: AppPublic>(&self) -> Result<Vec<Public>> {
-		self.public_keys_by_type::<Public::Generic>(Public::ID)
-			.map(|v| v.into_iter().map(Into::into).collect())
-	}
-
-	/// Returns the file path for the given public key and key type.
-	fn key_file_path(&self, public: &[u8], key_type: KeyTypeId) -> Option<PathBuf> {
-		let mut buf = self.path.as_ref()?.clone();
-		let key_type = hex::encode(key_type.0);
-		let key = hex::encode(public);
-		buf.push(key_type + key.as_str());
-		Some(buf)
-	}
 }

 impl BareCryptoStore for Store {
+	fn keys(
+		&self,
+		id: KeyTypeId
+	) -> std::result::Result<Vec<CryptoTypePublicPair>, TraitError> {
+		let raw_keys = self.raw_public_keys(id)?;
+		Ok(raw_keys.into_iter()
+			.fold(Vec::new(), |mut v, k| {
+				v.push(CryptoTypePublicPair(sr25519::CRYPTO_ID, k.clone()));
+				v.push(CryptoTypePublicPair(ed25519::CRYPTO_ID, k.clone()));
+				v
+			}))
+	}
+
+	fn supported_keys(
+		&self,
+		id: KeyTypeId,
+		keys: Vec<CryptoTypePublicPair>
+	) -> std::result::Result<Vec<CryptoTypePublicPair>, TraitError> {
+		let all_keys = self.keys(id)?.into_iter().collect::<HashSet<_>>();
+		Ok(keys.into_iter()
+		   .filter(|key| all_keys.contains(key))
+		   .collect::<Vec<_>>())
+	}
+
+	fn sign_with(
+		&self,
+		id: KeyTypeId,
+		key: &CryptoTypePublicPair,
+		msg: &[u8],
+	) -> std::result::Result<Vec<u8>, TraitError> {
+		match key.0 {
+			ed25519::CRYPTO_ID => {
+				let pub_key = ed25519::Public::from_slice(key.1.as_slice());
+				let key_pair: ed25519::Pair = self
+					.key_pair_by_type::<ed25519::Pair>(&pub_key, id)
+					.map_err(|e| TraitError::from(e))?;
+				Ok(key_pair.sign(msg).encode())
+			}
+			sr25519::CRYPTO_ID => {
+				let pub_key = sr25519::Public::from_slice(key.1.as_slice());
+				let key_pair: sr25519::Pair = self
+					.key_pair_by_type::<sr25519::Pair>(&pub_key, id)
+					.map_err(|e| TraitError::from(e))?;
+				Ok(key_pair.sign(msg).encode())
+			}
+			_ => Err(TraitError::KeyNotSupported(id))
+		}
+	}
+
 	fn sr25519_public_keys(&self, key_type: KeyTypeId) -> Vec<sr25519::Public> {
-		self.public_keys_by_type::<sr25519::Public>(key_type).unwrap_or_default()
+		self.raw_public_keys(key_type)
+			.map(|v| {
+				v.into_iter()
+				 .map(|k| sr25519::Public::from_slice(k.as_slice()))
+				 .collect()
+			})
+    		.unwrap_or_default()
 	}

 	fn sr25519_generate_new(
 		&mut self,
 		id: KeyTypeId,
 		seed: Option<&str>,
-	) -> std::result::Result<sr25519::Public, String> {
+	) -> std::result::Result<sr25519::Public, TraitError> {
 		let pair = match seed {
 			Some(seed) => self.insert_ephemeral_from_seed_by_type::<sr25519::Pair>(seed, id),
 			None => self.generate_by_type::<sr25519::Pair>(id),
-		}.map_err(|e| e.to_string())?;
+		}.map_err(|e| -> TraitError { e.into() })?;

 		Ok(pair.public())
 	}

-	fn sr25519_key_pair(&self, id: KeyTypeId, pub_key: &sr25519::Public) -> Option<sr25519::Pair> {
-		self.key_pair_by_type::<sr25519::Pair>(pub_key, id).ok()
-	}
-
 	fn ed25519_public_keys(&self, key_type: KeyTypeId) -> Vec<ed25519::Public> {
-		self.public_keys_by_type::<ed25519::Public>(key_type).unwrap_or_default()
+		self.raw_public_keys(key_type)
+			.map(|v| {
+				v.into_iter()
+				 .map(|k| ed25519::Public::from_slice(k.as_slice()))
+				 .collect()
+			})
+    		.unwrap_or_default()
 	}

 	fn ed25519_generate_new(
 		&mut self,
 		id: KeyTypeId,
 		seed: Option<&str>,
-	) -> std::result::Result<ed25519::Public, String> {
+	) -> std::result::Result<ed25519::Public, TraitError> {
 		let pair = match seed {
 			Some(seed) => self.insert_ephemeral_from_seed_by_type::<ed25519::Pair>(seed, id),
 			None => self.generate_by_type::<ed25519::Pair>(id),
-		}.map_err(|e| e.to_string())?;
+		}.map_err(|e| -> TraitError { e.into() })?;

 		Ok(pair.public())
 	}

-	fn ed25519_key_pair(&self, id: KeyTypeId, pub_key: &ed25519::Public) -> Option<ed25519::Pair> {
-		self.key_pair_by_type::<ed25519::Pair>(pub_key, id).ok()
-	}
-
 	fn insert_unknown(&mut self, key_type: KeyTypeId, suri: &str, public: &[u8])
 		-> std::result::Result<(), ()>
 	{
@@ -337,7 +413,7 @@ impl BareCryptoStore for Store {
 mod tests {
 	use super::*;
 	use tempfile::TempDir;
-	use sp_core::{testing::{SR25519}, crypto::{Ss58Codec}};
+	use sp_core::{testing::SR25519, crypto::Ss58Codec};

 	#[test]
 	fn basic_store() {
@@ -451,7 +527,7 @@ mod tests {
 		fs::write(file_name, "test").expect("Invalid file is written");

 		assert!(
-			store.read().public_keys_by_type::<sr25519::AppPublic>(SR25519).unwrap().is_empty(),
+			store.read().sr25519_public_keys(SR25519).is_empty(),
 		);
 	}
 }
@@ -21,7 +21,8 @@ use assert_matches::assert_matches;
 use codec::Encode;
 use sp_core::{
 	H256, blake2_256, hexdisplay::HexDisplay, testing::{ED25519, SR25519, KeyStore},
-	traits::BareCryptoStorePtr, ed25519, crypto::{Pair, Public},
+	traits::BareCryptoStorePtr, ed25519, sr25519,
+	crypto::{CryptoTypePublicPair, Pair, Public},
 };
 use rpc::futures::Stream as _;
 use substrate_test_runtime_client::{
@@ -173,7 +174,7 @@ fn should_return_pending_extrinsics() {

 	let ex = uxt(AccountKeyring::Alice, 0);
 	AuthorApi::submit_extrinsic(&p, ex.encode().into()).wait().unwrap();
- 	assert_matches!(
+	assert_matches!(
 		p.pending_extrinsics(),
 		Ok(ref expected) if *expected == vec![Bytes(ex.encode())]
 	);
@@ -199,7 +200,7 @@ fn should_remove_extrinsics() {
 		hash::ExtrinsicOrHash::Extrinsic(ex1.encode().into()),
 	]).unwrap();

- 	assert_eq!(removed.len(), 3);
+	assert_eq!(removed.len(), 3);
 }

 #[test]
@@ -215,10 +216,9 @@ fn should_insert_key() {
 		key_pair.public().0.to_vec().into(),
 	).expect("Insert key");

-	let store_key_pair = setup.keystore.read()
-		.ed25519_key_pair(ED25519, &key_pair.public()).expect("Key exists in store");
+	let public_keys = setup.keystore.read().keys(ED25519).unwrap();

-	assert_eq!(key_pair.public(), store_key_pair.public());
+	assert!(public_keys.contains(&CryptoTypePublicPair(ed25519::CRYPTO_ID, key_pair.public().to_raw_vec())));
 }

 #[test]
@@ -231,18 +231,11 @@ fn should_rotate_keys() {
 	let session_keys = SessionKeys::decode(&mut &new_public_keys[..])
 		.expect("SessionKeys decode successfully");

-	let ed25519_key_pair = setup.keystore.read().ed25519_key_pair(
-		ED25519,
-		&session_keys.ed25519.clone().into(),
-	).expect("ed25519 key exists in store");
+	let ed25519_public_keys = setup.keystore.read().keys(ED25519).unwrap();
+	let sr25519_public_keys = setup.keystore.read().keys(SR25519).unwrap();

-	let sr25519_key_pair = setup.keystore.read().sr25519_key_pair(
-		SR25519,
-		&session_keys.sr25519.clone().into(),
-	).expect("sr25519 key exists in store");
-
-	assert_eq!(session_keys.ed25519, ed25519_key_pair.public().into());
-	assert_eq!(session_keys.sr25519, sr25519_key_pair.public().into());
+	assert!(ed25519_public_keys.contains(&CryptoTypePublicPair(ed25519::CRYPTO_ID, session_keys.ed25519.to_raw_vec())));
+	assert!(sr25519_public_keys.contains(&CryptoTypePublicPair(sr25519::CRYPTO_ID, session_keys.sr25519.to_raw_vec())));
 }

 #[test]