Keyset locking

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]

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<File>,
+
     #[cfg(feature = "kmip")]
     /// The current set of KMIP server pools.
     pools: HashMap<String, SyncConnPool>,
@@ -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<File, Error> {
+    // 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.