diff --git a/src/fold_node/config.rs b/src/fold_node/config.rs index 599381ee..bb81ed41 100644 --- a/src/fold_node/config.rs +++ b/src/fold_node/config.rs @@ -229,8 +229,12 @@ pub fn save_node_config(config: &NodeConfig) -> Result<(), String> { let config_json = serde_json::to_string_pretty(config) .map_err(|e| format!("Failed to serialize config: {}", e))?; - fs::write(&config_path, config_json) - .map_err(|e| format!("Failed to write config file: {}", e))?; + crate::utils::fs_atomic::write_atomic( + std::path::Path::new(&config_path), + config_json.as_bytes(), + None, + ) + .map_err(|e| format!("Failed to write config file: {}", e))?; Ok(()) } diff --git a/src/ingestion/config.rs b/src/ingestion/config.rs index 7c5b0551..ffd275ae 100644 --- a/src/ingestion/config.rs +++ b/src/ingestion/config.rs @@ -720,7 +720,8 @@ impl IngestionConfig { std::fs::create_dir_all(parent)?; } let content = serde_json::to_string_pretty(&to_save)?; - std::fs::write(&config_path, content)?; + crate::utils::fs_atomic::write_atomic(&config_path, content.as_bytes(), None) + .map_err(|e| -> Box { e.into() })?; if let Some(key) = key_to_persist { crate::ingestion::anthropic_key_store::save(config_dir, &key)?; @@ -741,7 +742,8 @@ impl IngestionConfig { std::fs::create_dir_all(parent)?; } let content = serde_json::to_string_pretty(saved)?; - std::fs::write(path, content)?; + crate::utils::fs_atomic::write_atomic(path, content.as_bytes(), None) + .map_err(|e| -> Box { e.into() })?; Ok(()) } diff --git a/src/sensitive_io.rs b/src/sensitive_io.rs index 82b17fd6..e5558417 100644 --- a/src/sensitive_io.rs +++ b/src/sensitive_io.rs @@ -3,17 +3,16 @@ //! - `os-keychain` enabled: encrypts via OS keychain master key (AES-256-GCM) //! - `os-keychain` disabled: writes plaintext with 0o600 Unix permissions //! -//! All writes go through [`write_atomic_0600`], which stages bytes into -//! `.tmp`, fsyncs, then renames onto the target. Power loss between -//! steps therefore leaves either the previous good file or the staged -//! tmpfile — never a half-written final path. AES-GCM auth-tag failures -//! from a torn write would force the user to re-enter the credential, so -//! the rename atomicity is what protects them. +//! All writes go through [`write_atomic_0600`], which delegates to +//! [`crate::utils::fs_atomic::write_atomic`] (tmpfile + fsync + rename, plus +//! a best-effort parent-dir fsync). Power loss between steps therefore leaves +//! either the previous good file or the staged tmpfile — never a half-written +//! final path. AES-GCM auth-tag failures from a torn write would force the +//! user to re-enter the credential, so the rename atomicity is what protects +//! them. -use std::ffi::OsString; use std::fs; -use std::io::Write; -use std::path::{Path, PathBuf}; +use std::path::Path; /// Write sensitive data to disk, encrypted if `os-keychain` is enabled. pub fn write_sensitive(path: &Path, data: &[u8]) -> Result<(), String> { @@ -44,63 +43,11 @@ pub fn read_sensitive(path: &Path) -> Result, String> { /// Atomically write `data` to `path`, with mode 0o600 on Unix. /// -/// Stages the bytes in `.tmp`, fsyncs the file, renames onto `path`, -/// then best-effort fsyncs the parent directory. On any error, the tmp -/// file is removed so a retry starts clean. Callers are responsible for +/// Thin wrapper over [`crate::utils::fs_atomic::write_atomic`] that pins the +/// Unix mode at 0o600 for sensitive files. Callers are responsible for /// ensuring the parent directory exists. pub(crate) fn write_atomic_0600(path: &Path, data: &[u8]) -> Result<(), String> { - let tmp_path = { - let mut s: OsString = path.as_os_str().into(); - s.push(".tmp"); - PathBuf::from(s) - }; - - let result: Result<(), String> = (|| { - #[cfg(unix)] - let mut file = { - use std::fs::OpenOptions; - use std::os::unix::fs::OpenOptionsExt; - OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(&tmp_path) - .map_err(|e| format!("Failed to open temp file {}: {}", tmp_path.display(), e))? - }; - #[cfg(not(unix))] - let mut file = fs::File::create(&tmp_path) - .map_err(|e| format!("Failed to create temp file {}: {}", tmp_path.display(), e))?; - - file.write_all(data) - .map_err(|e| format!("Failed to write temp file {}: {}", tmp_path.display(), e))?; - file.sync_all() - .map_err(|e| format!("Failed to fsync temp file {}: {}", tmp_path.display(), e))?; - drop(file); - - fs::rename(&tmp_path, path).map_err(|e| { - format!( - "Failed to rename {} -> {}: {}", - tmp_path.display(), - path.display(), - e - ) - })?; - - #[cfg(unix)] - if let Some(parent) = path.parent() { - // Best-effort: parent-dir fsync hardens the rename against power - // loss. A failure here does not unwind the successful rename. - let _ = fs::File::open(parent).and_then(|d| d.sync_all()); - } - - Ok(()) - })(); - - if result.is_err() { - let _ = fs::remove_file(&tmp_path); - } - result + crate::utils::fs_atomic::write_atomic(path, data, Some(0o600)) } #[cfg(test)] diff --git a/src/utils/fs_atomic.rs b/src/utils/fs_atomic.rs new file mode 100644 index 00000000..962fa4a3 --- /dev/null +++ b/src/utils/fs_atomic.rs @@ -0,0 +1,135 @@ +//! Crash-safe file writes via tmpfile + fsync + rename. +//! +//! Stages bytes in `.tmp`, fsyncs the tmp file, renames onto `path`, +//! and best-effort fsyncs the parent directory. Power loss (or a daemon +//! crash) between steps therefore leaves either the previous good file or +//! the staged tmpfile — never a half-written final path. Callers are +//! responsible for ensuring the parent directory exists. +//! +//! [`crate::sensitive_io::write_atomic_0600`] delegates here with +//! `mode = Some(0o600)`; plaintext config writers pass `mode = None` so +//! the umask applies (typically 0o644). + +use std::ffi::OsString; +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +/// Atomically write `data` to `path`. +/// +/// On Unix, when `mode` is `Some(m)` the tmp file is opened with that mode +/// (which then survives the rename). When `mode` is `None`, the file is +/// created with default options so the process umask applies. On non-Unix +/// platforms the `mode` argument is ignored. +pub fn write_atomic(path: &Path, data: &[u8], mode: Option) -> Result<(), String> { + let tmp_path = { + let mut s: OsString = path.as_os_str().into(); + s.push(".tmp"); + PathBuf::from(s) + }; + + let result: Result<(), String> = (|| { + #[cfg(unix)] + let mut file = { + use std::fs::OpenOptions; + use std::os::unix::fs::OpenOptionsExt; + let mut opts = OpenOptions::new(); + opts.write(true).create(true).truncate(true); + if let Some(m) = mode { + opts.mode(m); + } + opts.open(&tmp_path) + .map_err(|e| format!("Failed to open temp file {}: {}", tmp_path.display(), e))? + }; + #[cfg(not(unix))] + let mut file = { + let _ = mode; + fs::File::create(&tmp_path) + .map_err(|e| format!("Failed to create temp file {}: {}", tmp_path.display(), e))? + }; + + file.write_all(data) + .map_err(|e| format!("Failed to write temp file {}: {}", tmp_path.display(), e))?; + file.sync_all() + .map_err(|e| format!("Failed to fsync temp file {}: {}", tmp_path.display(), e))?; + drop(file); + + fs::rename(&tmp_path, path).map_err(|e| { + format!( + "Failed to rename {} -> {}: {}", + tmp_path.display(), + path.display(), + e + ) + })?; + + #[cfg(unix)] + if let Some(parent) = path.parent() { + // Best-effort: parent-dir fsync hardens the rename against power + // loss. A failure here does not unwind the successful rename. + let _ = fs::File::open(parent).and_then(|d| d.sync_all()); + } + + Ok(()) + })(); + + if result.is_err() { + let _ = fs::remove_file(&tmp_path); + } + result +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(unix)] + #[test] + fn write_atomic_with_explicit_mode_sets_unix_permissions() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("secret"); + write_atomic(&path, b"hello", Some(0o600)).unwrap(); + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "expected 0o600, got {:o}", mode); + } + + #[cfg(unix)] + #[test] + fn write_atomic_without_mode_honors_umask() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("plain.json"); + write_atomic(&path, b"hello", None).unwrap(); + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + // Whatever the umask gave us, it must not be the locked-down 0o600 + // that sensitive_io uses — that's the regression this guards against. + assert_ne!(mode, 0o600, "plaintext write must not use 0o600"); + // The Unix default with the standard 0o022 umask is 0o644; allow the + // group/other bits to be looser if a tighter umask is in effect. + assert!( + mode & 0o600 == 0o600, + "owner read+write missing, got {:o}", + mode + ); + } + + #[test] + fn write_atomic_leaves_no_tmp_after_success() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("plain.json"); + write_atomic(&path, b"payload", None).unwrap(); + let tmp_sibling = path.with_file_name("plain.json.tmp"); + assert!(path.exists()); + assert!(!tmp_sibling.exists(), "stale tmp file at {tmp_sibling:?}"); + } + + #[test] + fn write_atomic_overwrites_preserve_atomicity() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("plain.json"); + write_atomic(&path, b"first", None).unwrap(); + write_atomic(&path, b"second", None).unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"second"); + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index ce9baea6..26538cce 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,3 +1,4 @@ pub mod crypto; +pub mod fs_atomic; pub mod http_errors; pub mod paths;