From 2f52425387fa71a881e97375b0aa25512abd1eb9 Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Fri, 5 Aug 2016 23:33:14 +0200 Subject: [PATCH] Unlock account with timeout for geth compatibility (#1854) * Unlock account with timeout for geth compatibility * Fixed test --- ethcore/src/account_provider.rs | 61 ++++++++++++++++++++--------- parity/rpc_apis.rs | 2 +- rpc/src/v1/impls/personal.rs | 16 ++++++-- rpc/src/v1/tests/mocked/personal.rs | 2 +- 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/ethcore/src/account_provider.rs b/ethcore/src/account_provider.rs index 2c735ce59..88f7b6f4f 100644 --- a/ethcore/src/account_provider.rs +++ b/ethcore/src/account_provider.rs @@ -18,7 +18,8 @@ use std::fmt; use std::collections::HashMap; -use util::{Address as H160, H256, H520, RwLock}; +use std::time::{Instant, Duration}; +use util::{Address as H160, H256, H520, Mutex, RwLock}; use ethstore::{SecretStore, Error as SSError, SafeAccount, EthStore}; use ethstore::dir::{KeyDirectory}; use ethstore::ethkey::{Address as SSAddress, Message as SSMessage, Secret as SSSecret, Random, Generator}; @@ -32,6 +33,8 @@ enum Unlock { /// Account unlocked permantently can always sign message. /// Use with caution. Perm, + /// Account unlocked with a timeout + Timed((Instant, u32)), } /// Data associated with account. @@ -131,7 +134,7 @@ impl KeyDirectory for NullDir { /// Account management. /// Responsible for unlocking accounts. pub struct AccountProvider { - unlocked: RwLock>, + unlocked: Mutex>, sstore: Box, } @@ -160,7 +163,7 @@ impl AccountProvider { /// Creates new account provider. pub fn new(sstore: Box) -> Self { AccountProvider { - unlocked: RwLock::new(HashMap::new()), + unlocked: Mutex::new(HashMap::new()), sstore: sstore, } } @@ -168,7 +171,7 @@ impl AccountProvider { /// Creates not disk backed provider. pub fn transient_provider() -> Self { AccountProvider { - unlocked: RwLock::new(HashMap::new()), + unlocked: Mutex::new(HashMap::new()), sstore: Box::new(EthStore::open(Box::new(NullDir::default())).unwrap()) } } @@ -236,12 +239,10 @@ impl AccountProvider { let _ = try!(self.sstore.sign(&account, &password, &Default::default())); // check if account is already unlocked pernamently, if it is, do nothing - { - let unlocked = self.unlocked.read(); - if let Some(data) = unlocked.get(&account) { - if let Unlock::Perm = data.unlock { - return Ok(()) - } + let mut unlocked = self.unlocked.lock(); + if let Some(data) = unlocked.get(&account) { + if let Unlock::Perm = data.unlock { + return Ok(()) } } @@ -250,7 +251,6 @@ impl AccountProvider { password: password, }; - let mut unlocked = self.unlocked.write(); unlocked.insert(account, data); Ok(()) } @@ -265,10 +265,15 @@ impl AccountProvider { self.unlock_account(account, password, Unlock::Temp) } + /// Unlocks account temporarily with a timeout. + pub fn unlock_account_timed(&self, account: A, password: String, duration_ms: u32) -> Result<(), Error> where Address: From { + self.unlock_account(account, password, Unlock::Timed((Instant::now(), duration_ms))) + } + /// Checks if given account is unlocked pub fn is_unlocked(&self, account: A) -> bool where Address: From { let account = Address::from(account).into(); - let unlocked = self.unlocked.read(); + let unlocked = self.unlocked.lock(); unlocked.get(&account).is_some() } @@ -278,15 +283,20 @@ impl AccountProvider { let message = Message::from(message).into(); let data = { - let unlocked = self.unlocked.read(); - try!(unlocked.get(&account).ok_or(Error::NotUnlocked)).clone() + let mut unlocked = self.unlocked.lock(); + let data = try!(unlocked.get(&account).ok_or(Error::NotUnlocked)).clone(); + if let Unlock::Temp = data.unlock { + unlocked.remove(&account).expect("data exists: so key must exist: qed"); + } + if let Unlock::Timed((ref start, ref duration)) = data.unlock { + if start.elapsed() > Duration::from_millis(*duration as u64) { + unlocked.remove(&account).expect("data exists: so key must exist: qed"); + return Err(Error::NotUnlocked); + } + } + data }; - if let Unlock::Temp = data.unlock { - let mut unlocked = self.unlocked.write(); - unlocked.remove(&account).expect("data exists: so key must exist: qed"); - } - let signature = try!(self.sstore.sign(&account, &data.password, &message)); Ok(H520(signature.into())) } @@ -304,6 +314,7 @@ impl AccountProvider { mod tests { use super::AccountProvider; use ethstore::ethkey::{Generator, Random}; + use std::time::Duration; #[test] fn unlock_account_temp() { @@ -329,4 +340,16 @@ mod tests { assert!(ap.sign(kp.address(), [0u8; 32]).is_ok()); assert!(ap.sign(kp.address(), [0u8; 32]).is_ok()); } + + #[test] + fn unlock_account_timer() { + let kp = Random.generate().unwrap(); + let ap = AccountProvider::transient_provider(); + assert!(ap.insert_account(kp.secret().clone(), "test").is_ok()); + assert!(ap.unlock_account_timed(kp.address(), "test1".into(), 2000).is_err()); + assert!(ap.unlock_account_timed(kp.address(), "test".into(), 2000).is_ok()); + assert!(ap.sign(kp.address(), [0u8; 32]).is_ok()); + ::std::thread::sleep(Duration::from_millis(2000)); + assert!(ap.sign(kp.address(), [0u8; 32]).is_err()); + } } diff --git a/parity/rpc_apis.rs b/parity/rpc_apis.rs index 4228743f9..a4b855e08 100644 --- a/parity/rpc_apis.rs +++ b/parity/rpc_apis.rs @@ -179,7 +179,7 @@ pub fn setup_rpc(server: T, deps: Arc, apis: ApiSet } }, Api::Personal => { - server.add_delegate(PersonalClient::new(&deps.secret_store, &deps.client, &deps.miner, deps.signer_port).to_delegate()); + server.add_delegate(PersonalClient::new(&deps.secret_store, &deps.client, &deps.miner, deps.signer_port, deps.geth_compatibility).to_delegate()); }, Api::Signer => { server.add_delegate(SignerClient::new(&deps.secret_store, &deps.client, &deps.miner, &deps.signer_queue).to_delegate()); diff --git a/rpc/src/v1/impls/personal.rs b/rpc/src/v1/impls/personal.rs index eacb6525a..958efe3a8 100644 --- a/rpc/src/v1/impls/personal.rs +++ b/rpc/src/v1/impls/personal.rs @@ -33,16 +33,18 @@ pub struct PersonalClient where C: MiningBlockChainClient, M: MinerService client: Weak, miner: Weak, signer_port: Option, + allow_perm_unlock: bool, } impl PersonalClient where C: MiningBlockChainClient, M: MinerService { /// Creates new PersonalClient - pub fn new(store: &Arc, client: &Arc, miner: &Arc, signer_port: Option) -> Self { + pub fn new(store: &Arc, client: &Arc, miner: &Arc, signer_port: Option, allow_perm_unlock: bool) -> Self { PersonalClient { accounts: Arc::downgrade(store), client: Arc::downgrade(client), miner: Arc::downgrade(miner), signer_port: signer_port, + allow_perm_unlock: allow_perm_unlock, } } @@ -89,11 +91,17 @@ impl Personal for PersonalClient where C: MiningBl fn unlock_account(&self, params: Params) -> Result { try!(self.active()); - from_params::<(RpcH160, String, u64)>(params).and_then( - |(account, account_pass, _)|{ + from_params::<(RpcH160, String, Option)>(params).and_then( + |(account, account_pass, duration)|{ let account: Address = account.into(); let store = take_weak!(self.accounts); - match store.unlock_account_temporarily(account, account_pass) { + let r = match (self.allow_perm_unlock, duration) { + (false, _) => store.unlock_account_temporarily(account, account_pass), + (true, Some(0)) => store.unlock_account_permanently(account, account_pass), + (true, Some(d)) => store.unlock_account_timed(account, account_pass, d as u32 * 1000), + (true, None) => store.unlock_account_timed(account, account_pass, 300_000), + }; + match r { Ok(_) => Ok(Value::Bool(true)), Err(_) => Ok(Value::Bool(false)), } diff --git a/rpc/src/v1/tests/mocked/personal.rs b/rpc/src/v1/tests/mocked/personal.rs index 290a63c63..35d57967e 100644 --- a/rpc/src/v1/tests/mocked/personal.rs +++ b/rpc/src/v1/tests/mocked/personal.rs @@ -50,7 +50,7 @@ fn setup(signer: Option) -> PersonalTester { let accounts = accounts_provider(); let client = blockchain_client(); let miner = miner_service(); - let personal = PersonalClient::new(&accounts, &client, &miner, signer); + let personal = PersonalClient::new(&accounts, &client, &miner, signer, false); let io = IoHandler::new(); io.add_delegate(personal.to_delegate());