From 37592d4a579abf25576417744f88773ff18949b6 Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 20:15:35 +0900 Subject: [PATCH 1/2] Route the last two atomic writes through fs_util 0.1.4 of the shared blocks extends the atomic-write rule twice: the temporary file is created with the permissions the finished file needs, and atomic replacement is not durability -- a file the program reads back to resume needs sync_all before the rename. fs_util::atomic_write already did both, and cert_group's stage_key_file does them by hand. fast_poll's state and trust's rotation-state.json did neither: fs::write left the temp at whatever the umask gave, rename carried that inode to the destination, and nothing was ever flushed. Both files are exactly the case the rule names -- the next tick resumes from one, the rotation command resumes from the other. Rather than repeat the logic a third time, atomic_write splits: the body was already a closure inside spawn_blocking, so it becomes atomic_write_blocking and the async form is a wrapper. trust.rs is sync all the way to its callers in rotate/ca.rs, so it takes the blocking half; fast_poll takes the async one. All three paths are now the same code. 0o600 on both. Neither file is read by anything but the process that writes it, and the crate already writes the agent config that way. That is a decision rather than a discovery -- previously they were whatever the umask happened to be, which nobody chose. create_rotation_state gets the same mode and a sync_all too. It was create_new with no mode, so leaving it alone would have left the file's permissions depending on whether it was created or updated last. Not included: syncing the containing directory after the rename, which no path in this crate does. That is a decision per file about what must survive a power loss, and it belongs with cert_group and fs_util rather than bolted onto two callers. Closes #803 --- src/commands/trust.rs | 25 +++++++++++++------------ src/fast_poll.rs | 31 +++++++++++-------------------- src/fs_util.rs | 23 +++++++++++++++++++---- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/src/commands/trust.rs b/src/commands/trust.rs index 6746edd0..ae2a8201 100644 --- a/src/commands/trust.rs +++ b/src/commands/trust.rs @@ -1,9 +1,11 @@ use std::collections::BTreeMap; use std::fs::{self, OpenOptions}; use std::io::Write; +use std::os::unix::fs::OpenOptionsExt; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use bootroot::fs_util; use bootroot::openbao::OpenBaoClient; use bootroot::trust_bootstrap::CA_BUNDLE_PEM_KEY; use serde::{Deserialize, Serialize}; @@ -16,6 +18,12 @@ use crate::state::ServiceEntry; pub(crate) const SERVICE_TRUST_KV_SUFFIX: &str = "trust"; const ROTATION_STATE_FILENAME: &str = "rotation-state.json"; +/// Mode for `rotation-state.json`. Only the rotation command writes +/// and reads it, so it is created owner-only rather than at whatever +/// the umask happens to be — and the same mode on both write paths, +/// so the file does not change permissions depending on which one ran. +const ROTATION_STATE_MODE: u32 = 0o600; + /// Describes which CA components are included in the rotation. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub(crate) enum RotationMode { @@ -58,10 +66,13 @@ pub(crate) fn create_rotation_state( let mut file = OpenOptions::new() .write(true) .create_new(true) + .mode(ROTATION_STATE_MODE) .open(&path) .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; file.write_all(json.as_bytes()) .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + file.sync_all() + .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; Ok(()) } @@ -74,18 +85,8 @@ pub(crate) fn update_rotation_state( let path = rotation_state_path(state_dir); let json = serde_json::to_string_pretty(state) .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - let temp_path = state_dir.join(format!( - "{ROTATION_STATE_FILENAME}.tmp.{}.{}", - std::process::id(), - std::time::SystemTime::now() - .duration_since(std::time::SystemTime::UNIX_EPOCH) - .map_or(0, |d| d.as_nanos()) - )); - fs::write(&temp_path, json.as_bytes()) - .with_context(|| messages.error_write_file_failed(&temp_path.display().to_string()))?; - fs::rename(&temp_path, &path) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - Ok(()) + fs_util::atomic_write_blocking(&path, json.as_bytes(), ROTATION_STATE_MODE) + .with_context(|| messages.error_write_file_failed(&path.display().to_string())) } /// Loads `rotation-state.json` if it exists. Returns `None` if absent. diff --git a/src/fast_poll.rs b/src/fast_poll.rs index 526195a1..8796ab67 100644 --- a/src/fast_poll.rs +++ b/src/fast_poll.rs @@ -103,6 +103,11 @@ fn log_poll_outcomes(kind: &str, outcomes: &[PollApplyOutcome], needs_relogin: & /// renewals but failed to be acknowledged back to `OpenBao`. Missing /// entries in `last_reissue_seen_version` mean "never seen a reissue /// request". +/// Mode for the fast-poll state file. It is the agent's own resume +/// record — nobody else reads it — so it is created owner-only rather +/// than at whatever the umask happens to be. +const STATE_FILE_MODE: u32 = 0o600; + #[derive(Debug, Default, Serialize, Deserialize, Clone)] pub(crate) struct FastPollState { #[serde(default)] @@ -190,7 +195,9 @@ impl FastPollState { } } - /// Persists the state file atomically via a `.tmp` rename. + /// Persists the state file atomically, owner-only and flushed: it + /// is what the next tick resumes from, so it has to survive a + /// power loss rather than merely replace cleanly. pub(crate) async fn save(&self, path: &Path) -> Result<()> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() @@ -202,27 +209,11 @@ impl FastPollState { ) })?; } - let tmp_path: PathBuf = { - let mut os = path.as_os_str().to_owned(); - os.push(".tmp"); - PathBuf::from(os) - }; let body = serde_json::to_string_pretty(self).context("Failed to serialize fast-poll state")?; - fs::write(&tmp_path, body).await.with_context(|| { - format!( - "Failed to write fast-poll state tmp file at {}", - tmp_path.display() - ) - })?; - fs::rename(&tmp_path, path).await.with_context(|| { - format!( - "Failed to rename {} -> {}", - tmp_path.display(), - path.display() - ) - })?; - Ok(()) + fs_util::atomic_write(path, body.as_bytes(), STATE_FILE_MODE) + .await + .with_context(|| format!("Failed to write fast-poll state at {}", path.display())) } } diff --git a/src/fs_util.rs b/src/fs_util.rs index 2f871d99..dc4637e9 100644 --- a/src/fs_util.rs +++ b/src/fs_util.rs @@ -279,13 +279,30 @@ async fn write_owned_impl(path: &Path, contents: &[u8], mode: u32, replace: bool /// permissioned, chowned (when ownership preservation is needed), or /// renamed. pub async fn atomic_write(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + let dest = path.to_path_buf(); + let payload = contents.to_vec(); + tokio::task::spawn_blocking(move || atomic_write_blocking(&dest, &payload, mode)) + .await + .context("Atomic write task panicked")? +} + +/// The blocking half of [`atomic_write`], for callers that are not async. +/// +/// Same guarantees, same order: staged in the destination's directory, +/// written, `sync_all`ed, permissioned, ownership-preserved, renamed. +/// Callers in an async context use [`atomic_write`] instead, which runs +/// this on a blocking thread. +/// +/// # Errors +/// Returns an error under the same conditions as [`atomic_write`]. +pub fn atomic_write_blocking(path: &Path, contents: &[u8], mode: u32) -> Result<()> { let parent = path .parent() .filter(|p| !p.as_os_str().is_empty()) .map_or_else(|| std::path::PathBuf::from("."), Path::to_path_buf); let dest = path.to_path_buf(); let payload = contents.to_vec(); - tokio::task::spawn_blocking(move || -> Result<()> { + { // Capture the existing destination's uid/gid (if any) so the // rename does not strip operator-meaningful ownership. Missing // file -> None; do not chown the staged file in that case so a @@ -330,9 +347,7 @@ pub async fn atomic_write(path: &Path, contents: &[u8], mode: u32) -> Result<()> ) })?; Ok(()) - }) - .await - .context("Atomic write task panicked")? + } } /// Ensures the secrets directory exists and has secure permissions. From 6fc7125cedf4f196e7d9d897b6069fdc6d54a345 Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 20:47:47 +0900 Subject: [PATCH 2/2] Borrow in the blocking half rather than copying again Splitting atomic_write left the copies where they were: the async wrapper owns path and contents so it can move them into spawn_blocking, and the extracted function then copied both a second time. Before the split there was one copy; after it there were two. The copies in the blocking half only ever existed to satisfy the move. Nothing in the body needs ownership -- metadata, write_all and persist all take borrows -- so it takes the arguments as they come. The wrapper keeps its single copy, which the move still requires. The scope the closure needed goes with them. --- src/fs_util.rs | 90 ++++++++++++++++++++++++-------------------------- 1 file changed, 44 insertions(+), 46 deletions(-) diff --git a/src/fs_util.rs b/src/fs_util.rs index dc4637e9..bcd2b23a 100644 --- a/src/fs_util.rs +++ b/src/fs_util.rs @@ -279,6 +279,8 @@ async fn write_owned_impl(path: &Path, contents: &[u8], mode: u32, replace: bool /// permissioned, chowned (when ownership preservation is needed), or /// renamed. pub async fn atomic_write(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + // Owned once, to move into the blocking task. The blocking half + // borrows, so this is the only copy either path makes. let dest = path.to_path_buf(); let payload = contents.to_vec(); tokio::task::spawn_blocking(move || atomic_write_blocking(&dest, &payload, mode)) @@ -300,54 +302,50 @@ pub fn atomic_write_blocking(path: &Path, contents: &[u8], mode: u32) -> Result< .parent() .filter(|p| !p.as_os_str().is_empty()) .map_or_else(|| std::path::PathBuf::from("."), Path::to_path_buf); - let dest = path.to_path_buf(); - let payload = contents.to_vec(); - { - // Capture the existing destination's uid/gid (if any) so the - // rename does not strip operator-meaningful ownership. Missing - // file -> None; do not chown the staged file in that case so a - // fresh create keeps process default ownership. - let existing_owner = std::fs::metadata(&dest).ok().map(|m| (m.uid(), m.gid())); - - let mut tmp = tempfile::NamedTempFile::new_in(&parent) - .with_context(|| format!("Failed to create temp file in {}", parent.display()))?; - tmp.as_file_mut() - .write_all(&payload) - .with_context(|| format!("Failed to write temp file for {}", dest.display()))?; - tmp.as_file_mut() - .sync_all() - .with_context(|| format!("Failed to fsync temp file for {}", dest.display()))?; - std::fs::set_permissions(tmp.path(), std::fs::Permissions::from_mode(mode)).with_context( - || { - format!( - "Failed to set mode {mode:o} on temp file for {}", - dest.display() - ) - }, - )?; - if let Some((dest_uid, dest_gid)) = existing_owner { - let tmp_meta = std::fs::metadata(tmp.path()) - .with_context(|| format!("Failed to stat temp file for {}", dest.display()))?; - if tmp_meta.uid() != dest_uid || tmp_meta.gid() != dest_gid { - std::os::unix::fs::chown(tmp.path(), Some(dest_uid), Some(dest_gid)).with_context( - || { - format!( - "Failed to preserve existing uid={dest_uid} gid={dest_gid} on {}", - dest.display() - ) - }, - )?; - } - } - tmp.persist(&dest).map_err(|e| { - anyhow::anyhow!( - "Failed to rename temp file to {}: {}", - dest.display(), - e.error + // Capture the existing destination's uid/gid (if any) so the + // rename does not strip operator-meaningful ownership. Missing + // file -> None; do not chown the staged file in that case so a + // fresh create keeps process default ownership. + let existing_owner = std::fs::metadata(path).ok().map(|m| (m.uid(), m.gid())); + + let mut tmp = tempfile::NamedTempFile::new_in(&parent) + .with_context(|| format!("Failed to create temp file in {}", parent.display()))?; + tmp.as_file_mut() + .write_all(contents) + .with_context(|| format!("Failed to write temp file for {}", path.display()))?; + tmp.as_file_mut() + .sync_all() + .with_context(|| format!("Failed to fsync temp file for {}", path.display()))?; + std::fs::set_permissions(tmp.path(), std::fs::Permissions::from_mode(mode)).with_context( + || { + format!( + "Failed to set mode {mode:o} on temp file for {}", + path.display() ) - })?; - Ok(()) + }, + )?; + if let Some((dest_uid, dest_gid)) = existing_owner { + let tmp_meta = std::fs::metadata(tmp.path()) + .with_context(|| format!("Failed to stat temp file for {}", path.display()))?; + if tmp_meta.uid() != dest_uid || tmp_meta.gid() != dest_gid { + std::os::unix::fs::chown(tmp.path(), Some(dest_uid), Some(dest_gid)).with_context( + || { + format!( + "Failed to preserve existing uid={dest_uid} gid={dest_gid} on {}", + path.display() + ) + }, + )?; + } } + tmp.persist(path).map_err(|e| { + anyhow::anyhow!( + "Failed to rename temp file to {}: {}", + path.display(), + e.error + ) + })?; + Ok(()) } /// Ensures the secrets directory exists and has secure permissions.