From afed3fb713aa09258e030744cb76445b5fda0d3b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 14 Dec 2022 12:50:47 +1100 Subject: [PATCH] Add test for supplying password --- .../src/initialized_validators.rs | 29 ++++++++ validator_manager/src/import_validators.rs | 2 +- validator_manager/src/move_validators.rs | 74 +++++++++++++++++-- 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/validator_client/src/initialized_validators.rs b/validator_client/src/initialized_validators.rs index 2e9f83de0e..bd9053b18a 100644 --- a/validator_client/src/initialized_validators.rs +++ b/validator_client/src/initialized_validators.rs @@ -1177,4 +1177,33 @@ impl InitializedValidators { val.index = Some(index); } } + + /// Deletes any passwords store in the validator definitions file and + /// returns a map of pubkey to deleted password. + pub fn delete_passwords_from_validator_definitions( + &mut self, + ) -> Result, Error> { + let mut passwords = HashMap::default(); + + for def in self.definitions.as_mut_slice() { + match &mut def.signing_definition { + SigningDefinition::LocalKeystore { + ref mut voting_keystore_password, + .. + } => { + if let Some(password) = voting_keystore_password.take() { + passwords.insert(def.voting_public_key.clone(), password); + } + } + // Remote signers don't have passwords. + SigningDefinition::Web3Signer { .. } => (), + }; + } + + self.definitions + .save(&self.validators_dir) + .map_err(Error::UnableToSaveDefinitions)?; + + Ok(passwords) + } } diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index 92af5ed673..c9e077eb85 100644 --- a/validator_manager/src/import_validators.rs +++ b/validator_manager/src/import_validators.rs @@ -246,7 +246,7 @@ pub mod tests { pub struct TestBuilder { import_config: ImportConfig, - vc: ApiTester, + pub vc: ApiTester, /// Holds the temp directory owned by the `CreateTestBuilder` so it doesn't get cleaned-up /// before we can read it. create_dir: Option, diff --git a/validator_manager/src/move_validators.rs b/validator_manager/src/move_validators.rs index a368403965..67b559f75a 100644 --- a/validator_manager/src/move_validators.rs +++ b/validator_manager/src/move_validators.rs @@ -1,6 +1,6 @@ use super::common::*; use crate::DumpConfig; -use account_utils::read_password_from_user; +use account_utils::{read_password_from_user, ZeroizeString}; use clap::{App, Arg, ArgMatches}; use eth2::{ lighthouse_vc::{ @@ -37,6 +37,30 @@ const NO_VALIDATORS_MSG: &str = "No validators present on source validator clien const UPLOAD_RETRY_WAIT: Duration = Duration::from_secs(5); +#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)] +pub enum PasswordSource { + /// Reads the password from the user via the terminal. + Interactive { stdin_inputs: bool }, + /// This variant is panic-y and should only be used during testing. + Testing(HashMap>), +} + +impl PasswordSource { + fn read_password(&mut self, pubkey: &PublicKeyBytes) -> Result { + match self { + PasswordSource::Interactive { stdin_inputs } => read_password_from_user(*stdin_inputs), + // This path with panic if the password list is empty. Since the + // password prompt will just keep retrying on a failed password, the + // panic helps us break the loop if we misconfigure the test. + PasswordSource::Testing(passwords) => Ok(passwords + .get_mut(pubkey) + .expect("pubkey should be known") + .remove(0) + .into()), + } + } +} + pub fn cli_app<'a, 'b>() -> App<'a, 'b> { App::new(CMD) .about( @@ -168,7 +192,7 @@ pub struct MoveConfig { pub builder_proposals: Option, pub fee_recipient: Option
, pub gas_limit: Option, - pub stdin_inputs: bool, + pub password_source: PasswordSource, } impl MoveConfig { @@ -182,7 +206,9 @@ impl MoveConfig { builder_proposals: clap_utils::parse_optional(matches, BUILDER_PROPOSALS_FLAG)?, fee_recipient: clap_utils::parse_optional(matches, FEE_RECIPIENT_FLAG)?, gas_limit: clap_utils::parse_optional(matches, GAS_LIMIT_FLAG)?, - stdin_inputs: cfg!(windows) || matches.is_present(STDIN_INPUTS_FLAG), + password_source: PasswordSource::Interactive { + stdin_inputs: cfg!(windows) || matches.is_present(STDIN_INPUTS_FLAG), + }, }) } } @@ -209,7 +235,7 @@ async fn run<'a>(config: MoveConfig) -> Result<(), String> { builder_proposals, fee_recipient, gas_limit, - stdin_inputs, + mut password_source, } = config; // Moving validators between the same VC is unlikely to be useful and probably indicates a user @@ -366,7 +392,7 @@ async fn run<'a>(config: MoveConfig) -> Result<(), String> { // Read the password from the user, retrying if the password is incorrect. loop { - match read_password_from_user(stdin_inputs) { + match password_source.read_password(&pubkey_to_move) { Ok(password) => { if let Err(e) = keystore.decrypt_keypair(password.as_ref()) { eprintln!("Failed to decrypt keystore: {:?}", e); @@ -603,6 +629,7 @@ mod test { duplicates: usize, dir: TempDir, move_back_again: bool, + passwords: HashMap>, } impl TestBuilder { @@ -614,6 +641,7 @@ mod test { duplicates: 0, dir, move_back_again: false, + passwords: <_>::default(), } } @@ -645,6 +673,28 @@ mod test { self } + fn remove_passwords_from_src_vc(mut self) -> Self { + let passwords = self + .src_import_builder + .as_ref() + .expect("src validators must be created before passwords can be removed") + .vc + .initialized_validators + .write() + .delete_passwords_from_validator_definitions() + .unwrap(); + self.passwords = passwords + .into_iter() + .map(|(pubkey, password)| { + ( + PublicKeyBytes::from(&pubkey), + vec![password.as_str().to_string()], + ) + }) + .collect(); + self + } + async fn move_validators( &self, gen_validators_enum: F, @@ -684,7 +734,7 @@ mod test { builder_proposals: None, fee_recipient: None, gas_limit: None, - stdin_inputs: false, + password_source: PasswordSource::Testing(self.passwords.clone()), }; let result = run(move_config).await; @@ -984,4 +1034,16 @@ mod test { .await .assert_ok(); } + + #[tokio::test] + async fn one_validator_move_all_passwords_removed() { + TestBuilder::new() + .await + .with_src_validators(1, 0) + .await + .remove_passwords_from_src_vc() + .run_test(|_| Validators::All) + .await + .assert_ok(); + } }