From df1f6a622338daa5159fd049cc83b56992cfe391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marin=20Ver=C5=A1i=C4=87?= Date: Mon, 25 Apr 2022 14:34:03 +0200 Subject: [PATCH] [feature] #2121: Check keypair is valid when constructed (#2130) --- cli/src/config.rs | 15 ++- cli/src/samples.rs | 3 +- client/benches/torii.rs | 4 +- client/src/client.rs | 20 ++-- client/src/config.rs | 2 +- .../integration/multisignature_transaction.rs | 9 +- client_cli/src/main.rs | 12 +-- core/src/genesis.rs | 4 +- core/src/sumeragi/config.rs | 2 +- core/test_network/src/lib.rs | 7 +- crypto/src/lib.rs | 100 ++++++++++++++++-- crypto/src/signature.rs | 2 +- data_model/tests/data_model.rs | 5 +- 13 files changed, 139 insertions(+), 46 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index d47b5ae1ff7..534db88ed78 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -123,13 +123,15 @@ impl Configuration { let mut configuration: Configuration = serde_json::from_reader(reader).wrap_err( format!("Failed to parse {:?} as Iroha peer configuration.", path), )?; - configuration.finalize(); + configuration.finalize()?; Ok(configuration) } - fn finalize(&mut self) { - self.sumeragi.key_pair = self.key_pair(); + fn finalize(&mut self) -> Result<()> { + self.sumeragi.key_pair = KeyPair::new(self.public_key.clone(), self.private_key.clone())?; self.sumeragi.peer_id = PeerId::new(&self.torii.p2p_addr, &self.public_key.clone()); + + Ok(()) } /// Loads configuration from environment @@ -139,14 +141,9 @@ impl Configuration { /// - Configuration `TrustedPeers` contains entries with duplicate public keys pub fn load_environment(&mut self) -> Result<()> { iroha_config::Configurable::load_environment(self)?; - self.finalize(); + self.finalize()?; Ok(()) } - - /// Get `public_key` and `private_key` configuration parameters. - pub fn key_pair(&self) -> iroha_crypto::KeyPair { - iroha_crypto::KeyPair::new(self.public_key.clone(), self.private_key.clone()) - } } #[cfg(test)] diff --git a/cli/src/samples.rs b/cli/src/samples.rs index 2f6ce4a682b..ac42cff00ce 100644 --- a/cli/src/samples.rs +++ b/cli/src/samples.rs @@ -77,7 +77,8 @@ pub fn get_config(trusted_peers: HashSet, key_pair: Option) -> ..KuraConfiguration::default() }, sumeragi: SumeragiConfiguration { - key_pair: KeyPair::new(public_key.clone(), private_key.clone()), + key_pair: KeyPair::new(public_key.clone(), private_key.clone()) + .expect("Key pair mismatch"), peer_id: PeerId::new(DEFAULT_TORII_P2P_ADDR, &public_key), trusted_peers: TrustedPeers { peers: trusted_peers, diff --git a/client/benches/torii.rs b/client/benches/torii.rs index 1008e200dd4..77acb73ef1e 100644 --- a/client/benches/torii.rs +++ b/client/benches/torii.rs @@ -76,7 +76,7 @@ fn query_requests(criterion: &mut Criterion) { client_config.torii_telemetry_url = small::SmallStr::from_string(format!("http://{}", client_config.torii_telemetry_url)); } - let mut iroha_client = Client::new(&client_config); + let mut iroha_client = Client::new(&client_config).expect("Invalid client configuration"); thread::sleep(std::time::Duration::from_millis(5000)); let _ = iroha_client @@ -157,7 +157,7 @@ fn instruction_submits(criterion: &mut Criterion) { client_config.torii_telemetry_url = small::SmallStr::from_string(format!("http://{}", client_config.torii_telemetry_url)); } - let mut iroha_client = Client::new(&client_config); + let mut iroha_client = Client::new(&client_config).expect("Invalid client configuration"); thread::sleep(std::time::Duration::from_millis(5000)); let _ = iroha_client .submit_all(vec![create_domain.into(), create_account.into()]) diff --git a/client/src/client.rs b/client/src/client.rs index 61b40c59eb7..04083795644 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -52,31 +52,37 @@ pub struct Client { /// Representation of `Iroha` client. impl Client { /// Constructor for client from configuration - pub fn new(configuration: &Configuration) -> Self { + /// + /// # Errors + /// If configuration isn't valid (e.g public/private keys don't match) + pub fn new(configuration: &Configuration) -> Result { Self::with_headers(configuration, http_client::Headers::default()) } /// Constructor for client from configuration and headers /// /// *Authentication* header will be added, if `login` and `password` fields are presented + /// + /// # Errors + /// If configuration isn't valid (e.g public/private keys don't match) pub fn with_headers( configuration: &Configuration, mut headers: HashMap, - ) -> Self { + ) -> Result { if let Some(basic_auth) = &configuration.basic_auth { let credentials = format!("{}:{}", basic_auth.web_login, basic_auth.password); let encoded = base64::encode(credentials); headers.insert(String::from("Authorization"), format!("Basic {}", encoded)); } - Self { + Ok(Self { torii_url: configuration.torii_api_url.clone(), telemetry_url: configuration.torii_telemetry_url.clone(), transaction_limits: configuration.transaction_limits, key_pair: KeyPair::new( configuration.public_key.clone(), configuration.private_key.clone(), - ), + )?, proposed_transaction_ttl_ms: configuration.transaction_time_to_live_ms, transaction_status_timeout: Duration::from_millis( configuration.transaction_status_timeout_ms, @@ -84,7 +90,7 @@ impl Client { account_id: configuration.account_id.clone(), headers, add_transaction_nonce: configuration.add_transaction_nonce, - } + }) } /// Builds transaction out of supplied instructions or wasm. @@ -738,7 +744,7 @@ mod tests { add_transaction_nonce: true, ..Configuration::default() }; - let client = Client::new(&cfg); + let client = Client::new(&cfg).expect("Invalid client configuration"); let build_transaction = || { client @@ -765,7 +771,7 @@ mod tests { basic_auth: Some(basic_auth), ..Configuration::default() }; - let client = Client::new(&cfg); + let client = Client::new(&cfg).expect("Invalid client configuration"); let value = client .headers diff --git a/client/src/config.rs b/client/src/config.rs index ac2794ba5ed..a168c5a9603 100644 --- a/client/src/config.rs +++ b/client/src/config.rs @@ -131,7 +131,7 @@ impl Configuration { "9ac47abf59b356e0bd7dcbbbb4dec080e302156a48ca907e47cb6aea1d32719e7233bfc89dcbd68c19fde6ce6158225298ec1131b6a130d1aeb454c1ab5183c0" ).expect("Private key not hex encoded"); - KeyPair::new(public_key, private_key) + KeyPair::new(public_key, private_key).expect("Key pair mismatch") } /// Account ID used by default for demo purposes diff --git a/client/tests/integration/multisignature_transaction.rs b/client/tests/integration/multisignature_transaction.rs index a0926e9c8f8..8807e79a094 100644 --- a/client/tests/integration/multisignature_transaction.rs +++ b/client/tests/integration/multisignature_transaction.rs @@ -48,7 +48,8 @@ fn multisignature_transactions_should_wait_for_all_signatures() { &network.genesis.api_address, &network.genesis.telemetry_address, ); - let mut iroha_client = Client::new(&client_configuration); + let mut iroha_client = + Client::new(&client_configuration).expect("Invalid client configuration"); iroha_client .submit_all(vec![ create_domain.into(), @@ -70,7 +71,7 @@ fn multisignature_transactions_should_wait_for_all_signatures() { client_configuration.account_id = account_id.clone(); client_configuration.public_key = public_key1; client_configuration.private_key = private_key1; - let iroha_client = Client::new(&client_configuration); + let iroha_client = Client::new(&client_configuration).expect("Invalid client configuration"); let instructions: Vec = vec![mint_asset.clone().into()]; let transaction = iroha_client .build_transaction(instructions.into(), UnlimitedMetadata::new()) @@ -88,7 +89,7 @@ fn multisignature_transactions_should_wait_for_all_signatures() { client_configuration.torii_api_url = small::SmallStr::from_string( "http://".to_owned() + &network.peers.values().last().unwrap().api_address, ); - let iroha_client_1 = Client::new(&client_configuration); + let iroha_client_1 = Client::new(&client_configuration).expect("Invalid client configuration"); let request = client::asset::by_account_id(account_id); assert!(iroha_client_1 .request(request.clone()) @@ -97,7 +98,7 @@ fn multisignature_transactions_should_wait_for_all_signatures() { let (public_key2, private_key2) = key_pair_2.into(); client_configuration.public_key = public_key2; client_configuration.private_key = private_key2; - let iroha_client_2 = Client::new(&client_configuration); + let iroha_client_2 = Client::new(&client_configuration).expect("Invalid client configuration"); let instructions: Vec = vec![mint_asset.into()]; let transaction = iroha_client_2 .build_transaction(instructions.into(), UnlimitedMetadata::new()) diff --git a/client_cli/src/main.rs b/client_cli/src/main.rs index f73ca055e86..d059a15e242 100644 --- a/client_cli/src/main.rs +++ b/client_cli/src/main.rs @@ -149,7 +149,7 @@ pub fn submit( metadata: UnlimitedMetadata, ) -> Result<()> { let instruction = instruction.into(); - let iroha_client = Client::new(cfg); + let iroha_client = Client::new(cfg)?; #[cfg(debug_assertions)] let err_msg = format!( "Failed to build transaction from instruction {:?}", @@ -208,7 +208,7 @@ mod events { } pub fn listen(filter: EventFilter, cfg: &Configuration) -> Result<()> { - let mut iroha_client = Client::new(cfg); + let mut iroha_client = Client::new(cfg)?; println!("Listening to events with filter: {:?}", filter); for event in iroha_client .listen_for_events(filter) @@ -274,7 +274,7 @@ mod domain { impl RunArgs for List { fn run(self, cfg: &ClientConfiguration) -> Result<()> { - let client = Client::new(cfg); + let client = Client::new(cfg)?; let vec = match self { Self::All => client @@ -398,7 +398,7 @@ mod account { impl RunArgs for List { fn run(self, cfg: &ClientConfiguration) -> Result<()> { - let client = Client::new(cfg); + let client = Client::new(cfg)?; let vec = match self { Self::All => client @@ -560,7 +560,7 @@ mod asset { impl RunArgs for Get { fn run(self, cfg: &ClientConfiguration) -> Result<()> { let Self { account, asset } = self; - let iroha_client = Client::new(cfg); + let iroha_client = Client::new(cfg)?; let asset_id = AssetId::new(asset, account); let value = iroha_client .request(asset::by_id(asset_id)) @@ -579,7 +579,7 @@ mod asset { impl RunArgs for List { fn run(self, cfg: &ClientConfiguration) -> Result<()> { - let client = Client::new(cfg); + let client = Client::new(cfg)?; let vec = match self { Self::All => client diff --git a/core/src/genesis.rs b/core/src/genesis.rs index ab0237d06ed..65fa15ec807 100644 --- a/core/src/genesis.rs +++ b/core/src/genesis.rs @@ -189,7 +189,7 @@ impl GenesisNetworkTrait for GenesisNetwork { .account_private_key .clone() .ok_or_else(|| eyre!("Genesis account private key is empty."))?, - ); + )?; raw_transaction.sign_and_accept(genesis_key_pair, tx_limits) }) @@ -364,7 +364,7 @@ pub mod config { "d748e18ce60cb30dea3e73c9019b7af45a8d465e3d71bcc9a5ef99a008205e534cffd0ee429b1bdd36b3910ec570852b8bb63f18750341772fb46bc856c5caaf" ).expect("Private key not hex encoded"); - KeyPair::new(public_key, private_key) + KeyPair::new(public_key, private_key).expect("Key pair mismatch") } } diff --git a/core/src/sumeragi/config.rs b/core/src/sumeragi/config.rs index 7d292306001..2aa3d2b2745 100644 --- a/core/src/sumeragi/config.rs +++ b/core/src/sumeragi/config.rs @@ -80,7 +80,7 @@ impl SumeragiConfiguration { "282ed9f3cf92811c3818dbc4ae594ed59dc1a2f78e4241e31924e101d6b1fb831c61faf8fe94e253b93114240394f79a607b7fa55f9e5a41ebec74b88055768b" ).expect("Private key not hex encoded"); - KeyPair::new(public_key, private_key) + KeyPair::new(public_key, private_key).expect("Key pair mismatch") } fn placeholder_peer_id() -> PeerId { diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index 114d2b558ff..88ca04219ef 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -112,7 +112,7 @@ pub fn get_key_pair() -> KeyPair { Algorithm::Ed25519, "9AC47ABF 59B356E0 BD7DCBBB B4DEC080 E302156A 48CA907E 47CB6AEA 1D32719E 7233BFC8 9DCBD68C 19FDE6CE 61582252 98EC1131 B6A130D1 AEB454C1 AB5183C0", ).expect("Private key not hex encoded") - ) + ).expect("Key pair mismatch") } /// Trait used to differentiate a test instance of `genesis`. @@ -805,6 +805,7 @@ impl TestClientConfiguration for ClientConfiguration { impl TestClient for Client { fn test(api_url: &str, telemetry_url: &str) -> Self { Client::new(&ClientConfiguration::test(api_url, telemetry_url)) + .expect("Invalid client configuration") } fn test_with_key(api_url: &str, telemetry_url: &str, keys: KeyPair) -> Self { @@ -812,7 +813,7 @@ impl TestClient for Client { let (public_key, private_key) = keys.into(); configuration.public_key = public_key; configuration.private_key = private_key; - Client::new(&configuration) + Client::new(&configuration).expect("Invalid client configuration") } fn test_with_account( @@ -826,7 +827,7 @@ impl TestClient for Client { let (public_key, private_key) = keys.into(); configuration.public_key = public_key; configuration.private_key = private_key; - Client::new(&configuration) + Client::new(&configuration).expect("Invalid client configuration") } fn for_each_event(mut self, event_filter: EventFilter, f: impl Fn(Result)) { diff --git a/crypto/src/lib.rs b/crypto/src/lib.rs index 4b1d0403ec2..17611c11103 100644 --- a/crypto/src/lib.rs +++ b/crypto/src/lib.rs @@ -112,11 +112,12 @@ impl TryFrom for UrsaKeyGenOption { KeyGenOption::FromPrivateKey(key) => { let algorithm = key.digest_function(); - if algorithm == Algorithm::Ed25519 || algorithm == Algorithm::Secp256k1 { - return Ok(Self::FromSecretKey(UrsaPrivateKey(key.payload))); + match algorithm { + Algorithm::Ed25519 | Algorithm::Secp256k1 => { + Ok(Self::FromSecretKey(UrsaPrivateKey(key.payload))) + } + _ => Err(Self::Error {}), } - - Err(Self::Error {}) } } } @@ -210,14 +211,58 @@ impl KeyPair { self.private_key.digest_function() } - /// Construct `KeyPair` - pub fn new(public_key: PublicKey, private_key: PrivateKey) -> Self { + /// Construct `KeyPair` from a matching pair of public and private key. + /// It is up to the user to ensure that the given keys indeed make a pair. + #[cfg(not(feature = "std"))] + pub fn new_unchecked(public_key: PublicKey, private_key: PrivateKey) -> Self { Self { public_key, private_key, } } + /// Construct `KeyPair` + /// + /// # Errors + /// If public and private key don't match, i.e. if they don't make a pair + #[cfg(feature = "std")] + pub fn new(public_key: PublicKey, private_key: PrivateKey) -> Result { + let algorithm = private_key.digest_function(); + + if algorithm != public_key.digest_function() { + return Err(Error::KeyGen(String::from("Mismatch of key algorithms"))); + } + + match algorithm { + Algorithm::Ed25519 | Algorithm::Secp256k1 => { + let key_pair = Self::generate_with_configuration(KeyGenConfiguration { + key_gen_option: Some(KeyGenOption::FromPrivateKey(private_key)), + algorithm, + })?; + + if *key_pair.public_key() != public_key { + return Err(Error::KeyGen(String::from("Key pair mismatch"))); + } + + Ok(key_pair) + } + Algorithm::BlsNormal | Algorithm::BlsSmall => { + let dummy_payload = 1_u8; + + let key_pair = Self { + public_key, + private_key, + }; + + SignatureOf::new(key_pair.clone(), &dummy_payload)? + .verify(&dummy_payload) + .map_err(|_err| Error::KeyGen(String::from("Key pair mismatch")))?; + + Ok(key_pair) + } + } + } + /// Generates a pair of Public and Private key with [`Algorithm::default()`] selected as generation algorithm. /// /// # Errors @@ -259,11 +304,14 @@ impl KeyPair { } } +#[cfg(feature = "std")] impl<'de> Deserialize<'de> for KeyPair { fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, { + use serde::de::Error as _; + #[derive(Deserialize)] struct KeyPair { public_key: PublicKey, @@ -271,7 +319,7 @@ impl<'de> Deserialize<'de> for KeyPair { } let key_pair = KeyPair::deserialize(deserializer)?; - Ok(Self::new(key_pair.public_key, key_pair.private_key)) + Self::new(key_pair.public_key, key_pair.private_key).map_err(D::Error::custom) } } @@ -533,6 +581,44 @@ mod tests { use super::*; + #[test] + fn key_pair_match() { + assert!(KeyPair::new("ed012059c8a4da1ebb5380f74aba51f502714652fdcce9611fafb9904e4a3c4d382774" + .parse() + .expect("Public key not in mulithash format"), + PrivateKey::from_hex( + Algorithm::Ed25519, + "93ca389fc2979f3f7d2a7f8b76c70de6d5eaf5fa58d4f93cb8b0fb298d398acc59c8a4da1ebb5380f74aba51f502714652fdcce9611fafb9904e4a3c4d382774" + ).expect("Private key not hex encoded")).is_ok()); + + assert!(KeyPair::new("ea0161040fcfade2fc5d9104a9acf9665ea545339ddf10ae50343249e01af3b8f885cd5d52956542cce8105db3a2ec4006e637a7177faaea228c311f907daafc254f22667f1a1812bb710c6f4116a1415275d27bb9fb884f37e8ef525cc31f3945e945fa" + .parse() + .expect("Public key not in mulithash format"), + PrivateKey::from_hex( + Algorithm::BlsNormal, + "0000000000000000000000000000000049bf70187154c57b97af913163e8e875733b4eaf1f3f0689b31ce392129493e9" + ).expect("Private key not hex encoded")).is_ok()); + } + + #[test] + fn key_pair_mismatch() { + assert!(KeyPair::new("ed012059c8a4da1ebb5380f74aba51f502714652fdcce9611fafb9904e4a3c4d382774" + .parse() + .expect("Public key not in mulithash format"), + PrivateKey::from_hex( + Algorithm::Ed25519, + "0000000000000000000000000000000049bf70187154c57b97af913163e8e875733b4eaf1f3f0689b31ce392129493e9" + ).expect("Private key not hex encoded")).is_err()); + + assert!(KeyPair::new("ea0161040fcfade2fc5d9104a9acf9665ea545339ddf10ae50343249e01af3b8f885cd5d52956542cce8105db3a2ec4006e637a7177faaea228c311f907daafc254f22667f1a1812bb710c6f4116a1415275d27bb9fb884f37e8ef525cc31f3945e945fa" + .parse() + .expect("Public key not in mulithash format"), + PrivateKey::from_hex( + Algorithm::BlsNormal, + "93ca389fc2979f3f7d2a7f8b76c70de6d5eaf5fa58d4f93cb8b0fb298d398acc59c8a4da1ebb5380f74aba51f502714652fdcce9611fafb9904e4a3c4d382774" + ).expect("Private key not hex encoded")).is_err()); + } + #[test] fn display_public_key() { assert_eq!( diff --git a/crypto/src/signature.rs b/crypto/src/signature.rs index d699dc2eeaa..9d035cce316 100644 --- a/crypto/src/signature.rs +++ b/crypto/src/signature.rs @@ -66,7 +66,7 @@ impl Signature { fn new(key_pair: KeyPair, payload: &[u8]) -> Result { let (public_key, private_key) = key_pair.into(); - let algorithm: Algorithm = public_key.digest_function(); + let algorithm: Algorithm = private_key.digest_function(); let private_key = UrsaPrivateKey(private_key.payload); let signature = match algorithm { diff --git a/data_model/tests/data_model.rs b/data_model/tests/data_model.rs index 59d80d5ca43..f4a09058f77 100644 --- a/data_model/tests/data_model.rs +++ b/data_model/tests/data_model.rs @@ -155,7 +155,7 @@ fn find_rate_and_make_exchange_isi_should_succeed() { Algorithm::Ed25519, "9AC47ABF 59B356E0 BD7DCBBB B4DEC080 E302156A 48CA907E 47CB6AEA 1D32719E 7233BFC8 9DCBD68C 19FDE6CE 61582252 98EC1131 B6A130D1 AEB454C1 AB5183C0" ).expect("Valid"), - ); + ).expect("Valid"); let mut peer = ::new().expect("Failed to create peer"); let configuration = get_config(std::iter::once(peer.id.clone()).collect(), Some(kp.clone())); let pipeline_time = Duration::from_millis(configuration.sumeragi.pipeline_time_ms()); @@ -180,7 +180,8 @@ fn find_rate_and_make_exchange_isi_should_succeed() { client_configuration.torii_api_url = SmallStr::from_string("http://".to_owned() + &peer.api_address); - let mut iroha_client = Client::new(&client_configuration); + let mut iroha_client = + Client::new(&client_configuration).expect("Invalid client configuration"); iroha_client .submit_all(vec![ register::domain("exchange").into(),