From 75da0ed1b6e053916f11a71a0ad3a923f4e1393f Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 10 May 2021 10:30:40 +1000 Subject: [PATCH 1/2] Write validator definitions atomically --- .../src/validator_definitions.rs | 38 ++++++++++++------- validator_client/src/key_cache.rs | 27 +++++++------ 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/common/account_utils/src/validator_definitions.rs b/common/account_utils/src/validator_definitions.rs index 466349cf95b..1272169ca87 100644 --- a/common/account_utils/src/validator_definitions.rs +++ b/common/account_utils/src/validator_definitions.rs @@ -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,8 +35,10 @@ 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. - UnableToWriteFile(io::Error), + /// The temporary config file could not be written to the filesystem. + UnableToWriteTempFile(io::Error), + /// The temporary config file could not be renamed into the real config file. + UnableToRenameFile(io::Error), /// The public key from the keystore is invalid. InvalidKeystorePubkey, /// The keystore was unable to be opened. @@ -249,19 +257,23 @@ 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 to the temporary path first. If this fails then the real validator definitions + // file is unscathed. + create_with_600_perms(&temp_path, &bytes).map_err(Error::UnableToWriteTempFile)?; + + // With the temporary file created, perform an atomic rename. + fs::rename(&temp_path, &config_path).map_err(Error::UnableToRenameFile)?; + + Ok(()) } /// Adds a new `ValidatorDefinition` to `self`. @@ -392,7 +404,7 @@ mod tests { #[test] fn graffiti_checks() { - let no_graffiti = r#"--- + let no_graffiti = r#"--- description: "" enabled: true type: local_keystore @@ -402,7 +414,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 +426,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..02564933e88 100644 --- a/validator_client/src/key_cache.rs +++ b/validator_client/src/key_cache.rs @@ -18,6 +18,9 @@ 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,17 @@ 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) + // Create and write to temporary. + create_with_600_perms(&temp_path, &bytes).map_err(Error::UnableToWriteTempFile)?; + + // Rename atomically. + fs::rename(&temp_path, &cache_path).map_err(Error::UnableToRenameFile)?; + + self.state = State::DecryptedAndSaved; + Ok(true) } else { Ok(false) } @@ -243,8 +246,10 @@ 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. - UnableToWriteFile(io::Error), + /// The temporary cache file could not be written to the filesystem. + UnableToWriteTempFile(io::Error), + /// The temporary cache file could not be renamed to replace the official cache file. + UnableToRenameFile(io::Error), /// Couldn't decrypt the cache file UnableToDecrypt(KeystoreError), UnableToEncrypt(KeystoreError), From 9db481190138a23dc9e21f13729ed468d880f4e6 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 11 May 2021 10:44:26 +1000 Subject: [PATCH 2/2] Preserve permissions --- common/account_utils/src/lib.rs | 26 +++++++++++++++++++ .../src/validator_definitions.rs | 16 ++++-------- validator_client/src/key_cache.rs | 17 +++++------- 3 files changed, 37 insertions(+), 22 deletions(-) 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 1272169ca87..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; @@ -35,10 +35,8 @@ pub enum Error { UnableToSearchForKeystores(io::Error), /// The config file could not be serialized as YAML. UnableToEncodeFile(serde_yaml::Error), - /// The temporary config file could not be written to the filesystem. - UnableToWriteTempFile(io::Error), - /// The temporary config file could not be renamed into the real config file. - UnableToRenameFile(io::Error), + /// 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, /// The keystore was unable to be opened. @@ -266,12 +264,8 @@ impl ValidatorDefinitions { let temp_path = validators_dir.as_ref().join(CONFIG_TEMP_FILENAME); let bytes = serde_yaml::to_vec(self).map_err(Error::UnableToEncodeFile)?; - // Write to the temporary path first. If this fails then the real validator definitions - // file is unscathed. - create_with_600_perms(&temp_path, &bytes).map_err(Error::UnableToWriteTempFile)?; - - // With the temporary file created, perform an atomic rename. - fs::rename(&temp_path, &config_path).map_err(Error::UnableToRenameFile)?; + write_file_via_temporary(&config_path, &temp_path, &bytes) + .map_err(Error::UnableToWriteFile)?; Ok(()) } diff --git a/validator_client/src/key_cache.rs b/validator_client/src/key_cache.rs index 02564933e88..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,8 +12,8 @@ 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"; @@ -145,11 +145,8 @@ impl KeyCache { let temp_path = validators_dir.as_ref().join(TEMP_CACHE_FILENAME); let bytes = serde_json::to_vec(self).map_err(Error::UnableToEncodeFile)?; - // Create and write to temporary. - create_with_600_perms(&temp_path, &bytes).map_err(Error::UnableToWriteTempFile)?; - - // Rename atomically. - fs::rename(&temp_path, &cache_path).map_err(Error::UnableToRenameFile)?; + write_file_via_temporary(&cache_path, &temp_path, &bytes) + .map_err(Error::UnableToWriteFile)?; self.state = State::DecryptedAndSaved; Ok(true) @@ -246,10 +243,8 @@ pub enum Error { UnableToParseFile(serde_json::Error), /// The cache file could not be serialized as YAML. UnableToEncodeFile(serde_json::Error), - /// The temporary cache file could not be written to the filesystem. - UnableToWriteTempFile(io::Error), - /// The temporary cache file could not be renamed to replace the official cache file. - UnableToRenameFile(io::Error), + /// The cache file or its temporary could not be written to the filesystem. + UnableToWriteFile(io::Error), /// Couldn't decrypt the cache file UnableToDecrypt(KeystoreError), UnableToEncrypt(KeystoreError),