Delete password files unless its being used elsewhere

common/account_utils/src/validator_definitions.rs

@@ -124,6 +124,16 @@ impl SigningDefinition {
             SigningDefinition::Web3Signer(_) => Ok(None),
         }
     }
+
+    pub fn voting_keystore_password_path(&self) -> Option<&PathBuf> {
+        match self {
+            SigningDefinition::LocalKeystore {
+                voting_keystore_password_path: Some(path),
+                ..
+            } => Some(path),
+            _ => None,
+        }
+    }
 }
 
 /// A validator that may be initialized by this validator client.
@@ -384,6 +394,13 @@ impl ValidatorDefinitions {
     pub fn as_mut_slice(&mut self) -> &mut [ValidatorDefinition] {
         self.0.as_mut_slice()
     }
+
+    // Returns an iterator over all the `voting_keystore_password_paths` in self.
+    pub fn iter_voting_keystore_password_paths(&self) -> impl Iterator<Item = &PathBuf> {
+        self.0
+            .iter()
+            .filter_map(|def| def.signing_definition.voting_keystore_password_path())
+    }
 }
 
 /// Perform an exhaustive tree search of `dir`, adding any discovered voting keystore paths to

validator_client/src/http_api/test_utils.rs

@@ -61,10 +61,15 @@ pub struct ApiTester {
     pub test_runtime: TestRuntime,
     pub _server_shutdown: oneshot::Sender<()>,
     pub validator_dir: TempDir,
+    pub secrets_dir: TempDir,
 }
 
 impl ApiTester {
     pub async fn new() -> Self {
+        Self::new_with_http_config(Self::default_http_config()).await
+    }
+
+    pub async fn new_with_http_config(http_config: HttpConfig) -> Self {
         let log = test_logger();
 
         let validator_dir = tempdir().unwrap();
@@ -127,14 +132,7 @@ impl ApiTester {
             graffiti_file: None,
             graffiti_flag: Some(Graffiti::default()),
             spec: E::default_spec(),
-            config: HttpConfig {
-                enabled: true,
-                listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)),
-                listen_port: 0,
-                allow_origin: None,
-                allow_keystore_export: true,
-                store_passwords_in_secrets_dir: false,
-            },
+            config: http_config,
             log,
             sse_logging_components: None,
             slot_clock,
@@ -168,6 +166,18 @@ impl ApiTester {
             test_runtime,
             _server_shutdown: shutdown_tx,
             validator_dir,
+            secrets_dir,
+        }
+    }
+
+    pub fn default_http_config() -> HttpConfig {
+        HttpConfig {
+            enabled: true,
+            listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)),
+            listen_port: 0,
+            allow_origin: None,
+            allow_keystore_export: true,
+            store_passwords_in_secrets_dir: false,
         }
     }
 

validator_client/src/initialized_validators.rs

@@ -115,6 +115,7 @@ pub enum Error {
     UnableToReadKeystoreFile(eth2_keystore::Error),
     UnableToSaveKeyCache(key_cache::Error),
     UnableToDecryptKeyCache(key_cache::Error),
+    UnableToDeletePasswordFile(PathBuf, io::Error),
 }
 
 impl From<LockfileError> for Error {
@@ -562,6 +563,7 @@ impl InitializedValidators {
         // We disable before removing so that in case of a crash the auto-discovery mechanism
         // won't re-activate the keystore.
         let mut uuid_opt = None;
+        let mut password_path_opt = None;
         let keystore_and_password = if let Some(def) = self
             .definitions
             .as_mut_slice()
@@ -577,9 +579,12 @@ impl InitializedValidators {
                 } if is_local_keystore => {
                     let password = match (voting_keystore_password, voting_keystore_password_path) {
                         (Some(password), _) => Some(password.clone()),
-                        (_, Some(path)) => read_password_string(path)
-                            .map(Option::Some)
-                            .map_err(Error::UnableToReadValidatorPassword)?,
+                        (_, Some(path)) => {
+                            password_path_opt = Some(path.clone());
+                            read_password_string(path)
+                                .map(Option::Some)
+                                .map_err(Error::UnableToReadValidatorPassword)?
+                        }
                         (None, None) => None,
                     };
                     let keystore = Keystore::from_json_file(voting_keystore_path)
@@ -648,6 +653,20 @@ impl InitializedValidators {
             .save(&self.validators_dir)
             .map_err(Error::UnableToSaveDefinitions)?;
 
+        // 4. Delete the keystore password if it's not being used by any definition.
+        if let Some(password_path) = password_path_opt.and_then(|p| p.canonicalize().ok()) {
+            if self
+                .definitions
+                .iter_voting_keystore_password_paths()
+                // Require canonicalized paths so we can do a true equality check.
+                .filter_map(|existing| existing.canonicalize().ok())
+                .all(|existing| existing != password_path)
+            {
+                fs::remove_file(&password_path)
+                    .map_err(|e| Error::UnableToDeletePasswordFile(password_path.into(), e))?;
+            }
+        }
+
         Ok(keystore_and_password)
     }
 
@@ -1282,4 +1301,10 @@ impl InitializedValidators {
 
         Ok(passwords)
     }
+
+    /// Prefer other methods in production. Arbitrarily modifying a validator
+    /// definition manually may result in inconsistencies.
+    pub fn as_mut_slice_testing_only(&mut self) -> &mut [ValidatorDefinition] {
+        self.definitions.as_mut_slice()
+    }
 }

validator_manager/src/import_validators.rs

@@ -240,7 +240,7 @@ pub mod tests {
     use crate::create_validators::tests::TestBuilder as CreateTestBuilder;
     use std::fs;
     use tempfile::{tempdir, TempDir};
-    use validator_client::http_api::test_utils::ApiTester;
+    use validator_client::http_api::{test_utils::ApiTester, Config as HttpConfig};
 
     const VC_TOKEN_FILE_NAME: &str = "vc_token.json";
 
@@ -255,8 +255,12 @@ pub mod tests {
 
     impl TestBuilder {
         pub async fn new() -> Self {
+            Self::new_with_http_config(ApiTester::default_http_config()).await
+        }
+
+        pub async fn new_with_http_config(http_config: HttpConfig) -> Self {
             let dir = tempdir().unwrap();
-            let vc = ApiTester::new().await;
+            let vc = ApiTester::new_with_http_config(http_config).await;
             let vc_token_path = dir.path().join(VC_TOKEN_FILE_NAME);
             fs::write(&vc_token_path, &vc.api_token).unwrap();