Allow to set validator password via reimport (sigp#2868)

## Issue Addressed Resolves sigp#2854 ## Proposed Changes If validator was imported first without entering password and then imported again with valid password update the password in validator_definitions.yml ## Additional Info There can be other cases for updating existing validator during import. They are not covered here. Co-authored-by: Michael Sproul <[email protected]>
realbigsean · Dec 21, 2021 · 60d917d · 60d917d
1 parent 3b61ac9
commit 60d917d
Show file tree

Hide file tree

Showing 2 changed files with 149 additions and 4 deletions.
diff --git a/account_manager/src/validator/import.rs b/account_manager/src/validator/import.rs
@@ -1,4 +1,5 @@
 use crate::wallet::create::{PASSWORD_FLAG, STDIN_INPUTS_FLAG};
+use account_utils::validator_definitions::SigningDefinition;
 use account_utils::{
     eth2_keystore::Keystore,
     read_password_from_user,
@@ -208,10 +209,35 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin
             }
         };
 
+        let voting_pubkey = keystore
+            .public_key()
+            .ok_or_else(|| format!("Keystore public key is invalid: {}", keystore.pubkey()))?;
+
         // The keystore is placed in a directory that matches the name of the public key. This
         // provides some loose protection against adding the same keystore twice.
         let dest_dir = validator_dir.join(format!("0x{}", keystore.pubkey()));
         if dest_dir.exists() {
+            // Check if we should update password for existing validator in case if it was provided via reimport: #2854
+            let old_validator_def_opt = defs
+                .as_mut_slice()
+                .iter_mut()
+                .find(|def| def.voting_public_key == voting_pubkey);
+            if let Some(ValidatorDefinition {
+                signing_definition:
+                    SigningDefinition::LocalKeystore {
+                        voting_keystore_password: ref mut old_passwd,
+                        ..
+                    },
+                ..
+            }) = old_validator_def_opt
+            {
+                if old_passwd.is_none() && password_opt.is_some() {
+                    *old_passwd = password_opt;
+                    defs.save(&validator_dir)
+                        .map_err(|e| format!("Unable to save {}: {:?}", CONFIG_FILENAME, e))?;
+                    eprintln!("Password updated for public key {}", voting_pubkey);
+                }
+            }
             eprintln!(
                 "Skipping import of keystore for existing public key: {:?}",
                 src_keystore
@@ -234,9 +260,6 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin
             .map_err(|e| format!("Unable to copy keystore: {:?}", e))?;
 
         // Register with slashing protection.
-        let voting_pubkey = keystore
-            .public_key()
-            .ok_or_else(|| format!("Keystore public key is invalid: {}", keystore.pubkey()))?;
         slashing_protection
             .register_validator(voting_pubkey.compress())
             .map_err(|e| {

diff --git a/lighthouse/tests/account_manager.rs b/lighthouse/tests/account_manager.rs
@@ -22,7 +22,7 @@ use std::env;
 use std::fs::{self, File};
 use std::io::{BufRead, BufReader, Write};
 use std::path::{Path, PathBuf};
-use std::process::{Command, Output, Stdio};
+use std::process::{Child, Command, Output, Stdio};
 use std::str::from_utf8;
 use tempfile::{tempdir, TempDir};
 use types::{Keypair, PublicKey};
@@ -528,6 +528,128 @@ fn validator_import_launchpad() {
     );
 }
 
+#[test]
+fn validator_import_launchpad_no_password_then_add_password() {
+    const PASSWORD: &str = "cats";
+    const KEYSTORE_NAME: &str = "keystore-m_12381_3600_0_0_0-1595406747.json";
+    const NOT_KEYSTORE_NAME: &str = "keystore-m_12381_3600_0_0-1595406747.json";
+
+    let src_dir = tempdir().unwrap();
+    let dst_dir = tempdir().unwrap();
+
+    let keypair = Keypair::random();
+    let keystore = KeystoreBuilder::new(&keypair, PASSWORD.as_bytes(), "".into())
+        .unwrap()
+        .build()
+        .unwrap();
+
+    let dst_keystore_dir = dst_dir.path().join(format!("0x{}", keystore.pubkey()));
+
+    // Create a keystore in the src dir.
+    File::create(src_dir.path().join(KEYSTORE_NAME))
+        .map(|mut file| keystore.to_json_writer(&mut file).unwrap())
+        .unwrap();
+
+    // Create a not-keystore file in the src dir.
+    File::create(src_dir.path().join(NOT_KEYSTORE_NAME)).unwrap();
+
+    let validator_import_key_cmd = || {
+        validator_cmd()
+            .arg(format!("--{}", VALIDATOR_DIR_FLAG))
+            .arg(dst_dir.path().as_os_str())
+            .arg(IMPORT_CMD)
+            .arg(format!("--{}", STDIN_INPUTS_FLAG)) // Using tty does not work well with tests.
+            .arg(format!("--{}", import::DIR_FLAG))
+            .arg(src_dir.path().as_os_str())
+            .stderr(Stdio::piped())
+            .stdin(Stdio::piped())
+            .spawn()
+            .unwrap()
+    };
+
+    let wait_for_password_prompt = |child: &mut Child| {
+        let mut stderr = child.stderr.as_mut().map(BufReader::new).unwrap().lines();
+
+        loop {
+            if stderr.next().unwrap().unwrap() == import::PASSWORD_PROMPT {
+                break;
+            }
+        }
+    };
+
+    let mut child = validator_import_key_cmd();
+    wait_for_password_prompt(&mut child);
+    let stdin = child.stdin.as_mut().unwrap();
+    stdin.write("\n".as_bytes()).unwrap();
+    child.wait().unwrap();
+
+    assert!(
+        src_dir.path().join(KEYSTORE_NAME).exists(),
+        "keystore should not be removed from src dir"
+    );
+    assert!(
+        src_dir.path().join(NOT_KEYSTORE_NAME).exists(),
+        "not-keystore should not be removed from src dir."
+    );
+
+    let voting_keystore_path = dst_keystore_dir.join(KEYSTORE_NAME);
+
+    assert!(
+        voting_keystore_path.exists(),
+        "keystore should be present in dst dir"
+    );
+    assert!(
+        !dst_dir.path().join(NOT_KEYSTORE_NAME).exists(),
+        "not-keystore should not be present in dst dir"
+    );
+
+    // Validator should be registered with slashing protection.
+    check_slashing_protection(&dst_dir, std::iter::once(keystore.public_key().unwrap()));
+
+    let defs = ValidatorDefinitions::open(&dst_dir).unwrap();
+
+    let expected_def = ValidatorDefinition {
+        enabled: true,
+        description: "".into(),
+        graffiti: None,
+        voting_public_key: keystore.public_key().unwrap(),
+        signing_definition: SigningDefinition::LocalKeystore {
+            voting_keystore_path,
+            voting_keystore_password_path: None,
+            voting_keystore_password: None,
+        },
+    };
+
+    assert!(
+        defs.as_slice() == &[expected_def.clone()],
+        "validator defs file should be accurate"
+    );
+
+    let mut child = validator_import_key_cmd();
+    wait_for_password_prompt(&mut child);
+    let stdin = child.stdin.as_mut().unwrap();
+    stdin.write(format!("{}\n", PASSWORD).as_bytes()).unwrap();
+    child.wait().unwrap();
+
+    let expected_def = ValidatorDefinition {
+        enabled: true,
+        description: "".into(),
+        graffiti: None,
+        voting_public_key: keystore.public_key().unwrap(),
+        signing_definition: SigningDefinition::LocalKeystore {
+            voting_keystore_path: dst_keystore_dir.join(KEYSTORE_NAME),
+            voting_keystore_password_path: None,
+            voting_keystore_password: Some(ZeroizeString::from(PASSWORD.to_string())),
+        },
+    };
+
+    let defs = ValidatorDefinitions::open(&dst_dir).unwrap();
+    assert!(
+        defs.as_slice() == &[expected_def.clone()],
+        "validator defs file should be accurate"
+    );
+}
+
 #[test]
 fn validator_import_launchpad_password_file() {
     const PASSWORD: &str = "cats";