Write validator definitions atomically (#2338)

## Issue Addressed

Closes #2159

## Proposed Changes

Rather than trying to write the validator definitions to disk directly, use a temporary file called `.validator_defintions.yml.tmp` and then atomically rename it to `validator_definitions.yml`. This avoids truncating the primary file, which can cause permanent damage when the disk is full.

The same treatment is also applied to the validator key cache, although the situation is less dire if it becomes corrupted because it can just be deleted without the user having to reimport keys or resupply passwords.

## Additional Info

* `File::create` truncates upon opening: https://doc.rust-lang.org/std/fs/struct.File.html#method.create
* `fs::rename` uses `rename` on UNIX and `MoveFileEx` on Windows: https://doc.rust-lang.org/std/fs/fn.rename.html
* UNIX `rename` call is atomic: https://unix.stackexchange.com/questions/322038/is-mv-atomic-on-my-fs
* Windows `MoveFileEx` is _not_ atomic in general, and Windows lacks any clear API for atomic file renames :(
   https://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows

## Further Work

* Consider whether we want to try a different Windows syscall as part of #2333. The `rust-atomicwrites` crate seems promising, but actually uses the same syscall under the hood presently: untitaker/rust-atomicwrites#27.

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