diff --git a/Cargo.lock b/Cargo.lock index 056e4dd..4f25462 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -329,6 +329,7 @@ dependencies = [ "const_format", "domain", "domain-kmip", + "fs2", "futures", "indenter", "jiff", @@ -341,6 +342,7 @@ dependencies = [ "rayon", "regex", "ring", + "same-file", "serde", "serde_json", "supports-color", @@ -524,6 +526,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs2" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "futures" version = "0.3.32" @@ -1464,6 +1476,15 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "same-file" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502" +dependencies = [ + "winapi-util", +] + [[package]] name = "scheduled-thread-pool" version = "0.2.7" @@ -2011,6 +2032,37 @@ dependencies = [ "rustls-pki-types", ] +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + +[[package]] +name = "winapi-util" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" +dependencies = [ + "windows-sys 0.61.2", +] + +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + [[package]] name = "windows-core" version = "0.62.2" diff --git a/Cargo.toml b/Cargo.toml index d919a05..2d65606 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,6 +68,8 @@ tracing = "0.1.41" tracing-subscriber = "0.3.19" url = "2.5.4" futures = "0.3.31" +fs2 = "0.4.3" +same-file = "1.0.6" supports-color = "3.0.2" [dev-dependencies] diff --git a/src/commands/keyset/cmd.rs b/src/commands/keyset/cmd.rs index 58a74d9..400c4c4 100644 --- a/src/commands/keyset/cmd.rs +++ b/src/commands/keyset/cmd.rs @@ -44,15 +44,17 @@ use domain_kmip as kmip; use domain_kmip::dep::kmip::client::pool::SyncConnPool; #[cfg(feature = "kmip")] use domain_kmip::KeyUrl; +use fs2::FileExt; use futures::future::join_all; use jiff::{Span, SpanRelativeTo}; +use same_file::Handle; use serde::{Deserialize, Serialize}; use std::cmp::max; use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::From; use std::ffi::OsStr; use std::fmt::{Debug, Display, Formatter}; -use std::fs::{create_dir_all, remove_file, File}; +use std::fs::{create_dir_all, remove_file, rename, File}; use std::io::{self, Write}; use std::net::{IpAddr, SocketAddr}; use std::path::{absolute, Path, PathBuf}; @@ -73,6 +75,9 @@ use super::kmip::{format_key_label, kmip_command, KmipCommands, KmipState}; /// with the key tags of existing keys. const MAX_KEY_TAG_TRIES: u8 = 10; +/// Number of times to try locking a file. +const MAX_FILE_LOCK_TRIES: u8 = 10; + /// Wait this amount before retrying for network errors, DNS errors, etc. const DEFAULT_WAIT: Duration = Duration::from_secs(10 * 60); @@ -629,6 +634,9 @@ struct WorkSpace { /// Whether the command to update DS records has to be executed. run_update_ds_command: bool, + /// Store the locked config file to avoid accidental unlocking. + _locked_config_file: Option, + #[cfg(feature = "kmip")] /// The current set of KMIP server pools. pools: HashMap, @@ -711,32 +719,26 @@ impl Keyset { ) })?; - let json = serde_json::to_string_pretty(&kss).expect("should not fail"); - let mut file = File::create(&state_file) - .map_err(|e| format!("unable to create file {}: {e}", state_file.display()))?; - write!(file, "{json}") - .map_err(|e| format!("unable to write to file {}: {e}", state_file.display()))?; + let ws = WorkSpace { + config: ksc, + state: kss, + config_changed: false, + state_changed: false, + run_update_ds_command: false, + _locked_config_file: None, + #[cfg(feature = "kmip")] + pools: HashMap::new(), + }; + + ws.write_state()?; + ws.write_config(&self.keyset_conf)?; - let json = serde_json::to_string_pretty(&ksc).expect("should not fail"); - let mut file = File::create(&self.keyset_conf).map_err(|e| { - format!("unable to create file {}: {e}", self.keyset_conf.display()) - })?; - write!(file, "{json}").map_err(|e| { - format!( - "unable to write to file {}: {e}", - self.keyset_conf.display() - ) - })?; return Ok(()); } - let file = File::open(self.keyset_conf.clone()).map_err(|e| { - format!( - "unable to open config file {}: {e}", - self.keyset_conf.display() - ) - })?; - let ksc: KeySetConfig = serde_json::from_reader(file) + let config_file = file_with_write_lock(&self.keyset_conf)?; + + let ksc: KeySetConfig = serde_json::from_reader(&config_file) .map_err(|e| format!("error loading {:?}: {e}\n", self.keyset_conf))?; let file = File::open(ksc.state_file.clone()).map_err(|e| { format!( @@ -753,6 +755,7 @@ impl Keyset { config_changed: false, state_changed: false, run_update_ds_command: false, + _locked_config_file: Some(config_file), #[cfg(feature = "kmip")] pools: HashMap::new(), }; @@ -1556,31 +1559,10 @@ impl Keyset { ws.state_changed = true; } if ws.config_changed { - let json = serde_json::to_string_pretty(&ws.config).expect("should not fail"); - let mut file = File::create(&self.keyset_conf).map_err(|e| { - format!("unable to create file {}: {e}", self.keyset_conf.display()) - })?; - write!(file, "{json}").map_err(|e| { - format!( - "unable to write to file {}: {e}", - self.keyset_conf.display() - ) - })?; + ws.write_config(&self.keyset_conf)?; } if ws.state_changed { - let json = serde_json::to_string_pretty(&ws.state).expect("should not fail"); - let mut file = File::create(&ws.config.state_file).map_err(|e| { - format!( - "unable to create file {}: {e}", - ws.config.state_file.display() - ) - })?; - write!(file, "{json}").map_err(|e| { - format!( - "unable to write to file {}: {e}", - ws.config.state_file.display() - ) - })?; + ws.write_state()?; } // Now check if we need to run the update_ds_command. Make sure that @@ -3760,6 +3742,43 @@ impl WorkSpace { let new_algs = HashSet::from([self.config.algorithm.to_generate_params().algorithm()]); curr_algs != new_algs } + + /// Write config to a file. + fn write_config(&self, keyset_conf: &PathBuf) -> Result<(), Error> { + let json = serde_json::to_string_pretty(&self.config).expect("should not fail"); + Self::write_to_new_and_rename(&json, keyset_conf) + } + + /// Write state to a file. + fn write_state(&self) -> Result<(), Error> { + let json = serde_json::to_string_pretty(&self.state).expect("should not fail"); + Self::write_to_new_and_rename(&json, &self.config.state_file) + } + + /// First write to a new filename and then rename to make sure that + /// changes are atomic. + fn write_to_new_and_rename(json: &str, filename: &PathBuf) -> Result<(), Error> { + let mut filename_new = filename.clone(); + // It would be nice to use add_extension here, but it is only in + // Rust 1.91.0 and above. Use strings instead. + // if !state_file_new.add_extension("new") { + // return Err(format!("unable to add extension 'new' to {}", + // ws.config.state_file.display()).into()); + // } + filename_new.as_mut_os_string().push(".new"); + let mut file = File::create(&filename_new) + .map_err(|e| format!("unable to create file {}: {e}", filename_new.display()))?; + write!(file, "{json}") + .map_err(|e| format!("unable to write to file {}: {e}", filename_new.display()))?; + rename(&filename_new, filename).map_err(|e| { + format!( + "unable to rename {} to {}: {e}", + filename_new.display(), + filename.display() + ) + })?; + Ok(()) + } } /// Create CDS and CDNSKEY RRsets. @@ -5658,6 +5677,46 @@ fn show_automatic_roll_state( } } +/// Open filename, get an exclusive lock and return the open file. +/// +/// Assume changes are saved by creating a new file and renaming. After +/// locking the file, the function has to check if the locked file is the +/// same as the current file under that name. +fn file_with_write_lock(filename: &PathBuf) -> Result { + // The config file is updated by writing to a new file and then renaming. + // We might have locked the old file. Check. Try a number of times and + // then give up. Lock contention is expected to be low. + for _try in 0..MAX_FILE_LOCK_TRIES { + let file = File::open(filename) + .map_err(|e| format!("unable to open file {}: {e}", filename.display()))?; + + file.lock_exclusive() + .map_err(|e| format!("unable to lock {}: {e}", filename.display()))?; + + let file_clone = file + .try_clone() + .map_err(|e| format!("unable to clone locked file {}: {e}", filename.display()))?; + let locked_file_handle = Handle::from_file(file_clone).map_err(|e| { + format!( + "Unable to get handle from locked file {}: {e}", + filename.display() + ) + })?; + let current_file_handle = Handle::from_path(filename) + .map_err(|e| format!("Unable to get handle from file {}: {e}", filename.display()))?; + + if locked_file_handle != current_file_handle { + continue; + } + return Ok(file); + } + Err(format!( + "unable to lock {} after {MAX_FILE_LOCK_TRIES} tries", + filename.display() + ) + .into()) +} + /// Helper function for serde. /// /// Return the default autoremove delay.