-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[EngineSigner]: don't sign message with only zeroes #11524
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,7 @@ mod accounts { | |
| mod accounts { | ||
| use super::*; | ||
| use upgrade::upgrade_key_location; | ||
| use ethereum_types::H160; | ||
| use ethereum_types::{H160, H256}; | ||
| use std::str::FromStr; | ||
|
|
||
| pub use accounts::AccountProvider; | ||
|
|
@@ -132,9 +132,17 @@ mod accounts { | |
| LocalAccounts(account_provider) | ||
| } | ||
|
|
||
| pub fn miner_author(spec: &SpecType, dirs: &Directories, account_provider: &Arc<AccountProvider>, engine_signer: Address, passwords: &[Password]) -> Result<Option<::ethcore::miner::Author>, String> { | ||
| pub fn miner_author( | ||
| spec: &SpecType, | ||
| dirs: &Directories, | ||
| account_provider: &Arc<AccountProvider>, | ||
| engine_signer: Address, | ||
| passwords: &[Password] | ||
| ) -> Result<Option<ethcore::miner::Author>, String> { | ||
| use engine::signer::EngineSigner; | ||
|
|
||
| const SECP_TEST_MESSAGE: H256 = H256([1_u8; 32]); | ||
|
|
||
| // Check if engine signer exists | ||
| if !account_provider.has_account(engine_signer) { | ||
| return Err(format!("Consensus signer account not found for the current chain. {}", build_create_account_hint(spec, &dirs.keys))); | ||
|
|
@@ -145,25 +153,27 @@ mod accounts { | |
| return Err(format!("No password found for the consensus signer {}. {}", engine_signer, VERIFY_PASSWORD_HINT)); | ||
| } | ||
|
|
||
| let mut author = None; | ||
| for password in passwords { | ||
| let mut invalid_reasons = std::collections::HashSet::new(); | ||
| for (idx, password) in passwords.iter().enumerate() { | ||
| let signer = parity_rpc::signer::EngineSigner::new( | ||
| account_provider.clone(), | ||
| engine_signer, | ||
| password.clone(), | ||
| ); | ||
| if signer.sign(Default::default()).is_ok() { | ||
| author = Some(::ethcore::miner::Author::Sealer(Box::new(signer))); | ||
| if let Err(e) = signer.sign(SECP_TEST_MESSAGE) { | ||
| debug!(target: "account", "Signing test of `EngineSigner ({})` with password index: {} failed because of: {:?}", engine_signer, idx, e); | ||
| invalid_reasons.insert(e.to_string()); | ||
| } else { | ||
| return Ok(Some(ethcore::miner::Author::Sealer(Box::new(signer)))); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. once we have found a valid password then return it directly, no need to iterate over the rest. |
||
| } | ||
| } | ||
| if author.is_none() { | ||
| return Err(format!("No valid password for the consensus signer {}. {}", engine_signer, VERIFY_PASSWORD_HINT)); | ||
| } | ||
|
|
||
| Ok(author) | ||
| Err(format!( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will look like: No valid password found for EngineSigner 0x5100…cae4, the following errors were found during testing: {"custom crypto error: invalid AES message", "custom crypto error: Invalid password"}. Make sure valid password is present in files passed using `--password` or in the configuration file. |
||
| "No valid password found for EngineSigner {}, the following errors were found during testing: {:?}. {}", | ||
| engine_signer, invalid_reasons, VERIFY_PASSWORD_HINT | ||
| )) | ||
| } | ||
|
|
||
|
|
||
| mod private_tx { | ||
| use super::*; | ||
| use parity_crypto::publickey::{Signature, Message}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,16 +36,14 @@ impl EngineSigner { | |
|
|
||
| impl engine::signer::EngineSigner for EngineSigner { | ||
| fn sign(&self, message: Message) -> Result<Signature, Error> { | ||
| match self.accounts.sign(self.address, Some(self.password.clone()), message) { | ||
| Ok(ok) => Ok(ok), | ||
| Err(_) => Err(Error::InvalidSecretKey), | ||
| } | ||
| self.accounts.sign(self.address, Some(self.password.clone()), message).map_err(|e| { | ||
| Error::Custom(e.to_string()) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. propagate real error cause instead of making assumptions. |
||
| }) | ||
| } | ||
|
|
||
| fn decrypt(&self, auth_data: &[u8], cipher: &[u8]) -> Result<Vec<u8>, Error> { | ||
| self.accounts.decrypt(self.address, None, auth_data, cipher).map_err(|e| { | ||
| warn!("Unable to decrypt message: {:?}", e); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this, because it is better that the caller handles this error accordingly. Thoughts? |
||
| Error::InvalidMessage | ||
| Error::Custom(e.to_string()) | ||
| }) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parity_crypto::public::Errordoesn't implEq, that's why it is converted to a StringThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should? :)