diff --git a/common/account_utils/src/lib.rs b/common/account_utils/src/lib.rs index 1ec15fa4e23..3832801486c 100644 --- a/common/account_utils/src/lib.rs +++ b/common/account_utils/src/lib.rs @@ -76,6 +76,32 @@ pub fn create_with_600_perms>(path: P, bytes: &[u8]) -> Result<() Ok(()) } +/// Write a file atomically by using a temporary file as an intermediate. +/// +/// Care is taken to preserve the permissions of the file at `file_path` being written. +/// +/// If no file exists at `file_path` one will be created with restricted 0o600-equivalent +/// permissions. +pub fn write_file_via_temporary( + file_path: &Path, + temp_path: &Path, + bytes: &[u8], +) -> Result<(), io::Error> { + // If the file already exists, preserve its permissions by copying it. + // Otherwise, create a new file with restricted permissions. + if file_path.exists() { + fs::copy(&file_path, &temp_path)?; + fs::write(&temp_path, &bytes)?; + } else { + create_with_600_perms(&temp_path, &bytes)?; + } + + // With the temporary file created, perform an atomic rename. + fs::rename(&temp_path, &file_path)?; + + Ok(()) +} + /// Generates a random alphanumeric password of length `DEFAULT_PASSWORD_LEN`. pub fn random_password() -> PlainText { rand::thread_rng() diff --git a/common/account_utils/src/validator_definitions.rs b/common/account_utils/src/validator_definitions.rs index 466349cf95b..2deb95dd347 100644 --- a/common/account_utils/src/validator_definitions.rs +++ b/common/account_utils/src/validator_definitions.rs @@ -3,7 +3,7 @@ //! Serves as the source-of-truth of which validators this validator client should attempt (or not //! attempt) to load into the `crate::intialized_validators::InitializedValidators` struct. -use crate::{create_with_600_perms, default_keystore_password_path, ZeroizeString}; +use crate::{default_keystore_password_path, write_file_via_temporary, ZeroizeString}; use directory::ensure_dir_exists; use eth2_keystore::Keystore; use regex::Regex; @@ -19,6 +19,12 @@ use validator_dir::VOTING_KEYSTORE_FILE; /// The file name for the serialized `ValidatorDefinitions` struct. pub const CONFIG_FILENAME: &str = "validator_definitions.yml"; +/// The temporary file name for the serialized `ValidatorDefinitions` struct. +/// +/// This is used to achieve an atomic update of the contents on disk, without truncation. +/// See: https://github.com/sigp/lighthouse/issues/2159 +pub const CONFIG_TEMP_FILENAME: &str = ".validator_definitions.yml.tmp"; + #[derive(Debug)] pub enum Error { /// The config file could not be opened. @@ -29,7 +35,7 @@ pub enum Error { UnableToSearchForKeystores(io::Error), /// The config file could not be serialized as YAML. UnableToEncodeFile(serde_yaml::Error), - /// The config file could not be written to the filesystem. + /// The config file or temp file could not be written to the filesystem. UnableToWriteFile(io::Error), /// The public key from the keystore is invalid. InvalidKeystorePubkey, @@ -249,19 +255,19 @@ impl ValidatorDefinitions { Ok(new_defs_count) } - /// Encodes `self` as a YAML string it writes it to the `CONFIG_FILENAME` file in the - /// `validators_dir` directory. + /// Encodes `self` as a YAML string and atomically writes it to the `CONFIG_FILENAME` file in + /// the `validators_dir` directory. /// - /// Will create a new file if it does not exist or over-write any existing file. + /// Will create a new file if it does not exist or overwrite any existing file. pub fn save>(&self, validators_dir: P) -> Result<(), Error> { let config_path = validators_dir.as_ref().join(CONFIG_FILENAME); + let temp_path = validators_dir.as_ref().join(CONFIG_TEMP_FILENAME); let bytes = serde_yaml::to_vec(self).map_err(Error::UnableToEncodeFile)?; - if config_path.exists() { - fs::write(config_path, &bytes).map_err(Error::UnableToWriteFile) - } else { - create_with_600_perms(&config_path, &bytes).map_err(Error::UnableToWriteFile) - } + write_file_via_temporary(&config_path, &temp_path, &bytes) + .map_err(Error::UnableToWriteFile)?; + + Ok(()) } /// Adds a new `ValidatorDefinition` to `self`. @@ -392,7 +398,7 @@ mod tests { #[test] fn graffiti_checks() { - let no_graffiti = r#"--- + let no_graffiti = r#"--- description: "" enabled: true type: local_keystore @@ -402,7 +408,7 @@ mod tests { let def: ValidatorDefinition = serde_yaml::from_str(&no_graffiti).unwrap(); assert!(def.graffiti.is_none()); - let invalid_graffiti = r#"--- + let invalid_graffiti = r#"--- description: "" enabled: true type: local_keystore @@ -414,7 +420,7 @@ mod tests { let def: Result = serde_yaml::from_str(&invalid_graffiti); assert!(def.is_err()); - let valid_graffiti = r#"--- + let valid_graffiti = r#"--- description: "" enabled: true type: local_keystore diff --git a/validator_client/src/key_cache.rs b/validator_client/src/key_cache.rs index 6da06aaa1b2..66d69622300 100644 --- a/validator_client/src/key_cache.rs +++ b/validator_client/src/key_cache.rs @@ -1,4 +1,4 @@ -use account_utils::create_with_600_perms; +use account_utils::write_file_via_temporary; use bls::{Keypair, PublicKey}; use eth2_keystore::json_keystore::{ Aes128Ctr, ChecksumModule, Cipher, CipherModule, Crypto, EmptyMap, EmptyString, KdfModule, @@ -12,12 +12,15 @@ use rand::prelude::*; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs::OpenOptions; +use std::io; use std::path::{Path, PathBuf}; -use std::{fs, io}; /// The file name for the serialized `KeyCache` struct. pub const CACHE_FILENAME: &str = "validator_key_cache.json"; +/// The file name for the temporary `KeyCache`. +pub const TEMP_CACHE_FILENAME: &str = ".validator_key_cache.json.tmp"; + #[derive(Debug, Copy, Clone, PartialEq)] pub enum State { NotDecrypted, @@ -139,17 +142,14 @@ impl KeyCache { self.encrypt()?; let cache_path = validators_dir.as_ref().join(CACHE_FILENAME); + let temp_path = validators_dir.as_ref().join(TEMP_CACHE_FILENAME); let bytes = serde_json::to_vec(self).map_err(Error::UnableToEncodeFile)?; - let res = if cache_path.exists() { - fs::write(cache_path, &bytes).map_err(Error::UnableToWriteFile) - } else { - create_with_600_perms(&cache_path, &bytes).map_err(Error::UnableToWriteFile) - }; - if res.is_ok() { - self.state = State::DecryptedAndSaved; - } - res.map(|_| true) + write_file_via_temporary(&cache_path, &temp_path, &bytes) + .map_err(Error::UnableToWriteFile)?; + + self.state = State::DecryptedAndSaved; + Ok(true) } else { Ok(false) } @@ -243,7 +243,7 @@ pub enum Error { UnableToParseFile(serde_json::Error), /// The cache file could not be serialized as YAML. UnableToEncodeFile(serde_json::Error), - /// The cache file could not be written to the filesystem. + /// The cache file or its temporary could not be written to the filesystem. UnableToWriteFile(io::Error), /// Couldn't decrypt the cache file UnableToDecrypt(KeystoreError),