[Merged by Bors] - Write validator definitions atomically

common/account_utils/src/lib.rs

@@ -76,6 +76,32 @@ pub fn create_with_600_perms<P: AsRef<Path>>(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()

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<P: AsRef<Path>>(&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<ValidatorDefinition, _> = serde_yaml::from_str(&invalid_graffiti);
         assert!(def.is_err());
 
-        let valid_graffiti = r#"--- 
+        let valid_graffiti = r#"---
         description: ""
         enabled: true
         type: local_keystore

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),