diff --git a/traffic_core/src/atomic.rs b/traffic_core/src/atomic.rs new file mode 100644 index 0000000..2ed9900 --- /dev/null +++ b/traffic_core/src/atomic.rs @@ -0,0 +1,245 @@ +//! Atomic, symlink-refusing writes for the files this package owns under `/etc` and +//! `/var/lib`. +//! +//! Every write here runs as root, so following a symlink means truncating whatever the +//! link points at. `fs::write` and `File::create` both follow links, which made the fstab +//! backup and the swap ownership marker into arbitrary-file-overwrite primitives: a +//! symlink planted at `/etc/fstab.awg2-harness.bak` or at the marker path would be +//! followed and its target overwritten. +//! +//! The contract every writer here obeys: +//! +//! 1. inspect the destination with `symlink_metadata`; +//! 2. refuse anything that is not a regular file or absent; +//! 3. create a sibling temp with `create_new`, so an existing temp is never followed; +//! 4. write, `sync_all`, set the mode; +//! 5. `rename` over the destination; +//! 6. flush the directory entry. +//! +//! The rename is what makes the replacement atomic: readers see either the old file or +//! the new one, never a truncated one. + +use std::{ + fs, + io::{ErrorKind, Write}, + path::{Path, PathBuf}, +}; + +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; + +pub const TEMP_SUFFIX: &str = ".awg2-harness.tmp"; + +/// Fails if the path exists as anything other than a regular file. +/// +/// A symlink is rejected rather than resolved: the caller asked for this path, and +/// following it would act on a file nobody configured. +pub fn assert_regular_or_absent(path: &Path) -> anyhow::Result<()> { + match fs::symlink_metadata(path) { + Ok(md) if md.file_type().is_symlink() => Err(anyhow::anyhow!( + "refusing to write through symlink: {}", + path.display() + )), + Ok(md) if !md.file_type().is_file() => Err(anyhow::anyhow!( + "refusing to write over non-regular file: {}", + path.display() + )), + Ok(_) => Ok(()), + Err(e) if e.kind() == ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("failed to stat {}: {e}", path.display())), + } +} + +pub fn temp_sibling(path: &Path) -> anyhow::Result { + let name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("invalid path: {}", path.display()))?; + let mut temp = name.to_os_string(); + temp.push(TEMP_SUFFIX); + Ok(path.with_file_name(temp)) +} + +/// Writes `content` to `path` atomically, refusing to follow a symlink at either the +/// destination or the temp path. +pub fn write_atomic_no_follow(path: &Path, content: &str, mode: u32) -> anyhow::Result<()> { + assert_regular_or_absent(path)?; + + let temp = temp_sibling(path)?; + // A leftover temp from a killed run is removed rather than opened, so a symlink + // planted at the temp path cannot be followed either. + match fs::remove_file(&temp) { + Ok(()) => {} + Err(e) if e.kind() == ErrorKind::NotFound => {} + Err(e) => { + return Err(anyhow::anyhow!( + "failed to clear stale {}: {e}", + temp.display() + )); + } + } + + let result = write_temp_then_rename(&temp, path, content, mode); + if result.is_err() { + let _ = fs::remove_file(&temp); + } + result +} + +fn write_temp_then_rename( + temp: &Path, + path: &Path, + content: &str, + mode: u32, +) -> anyhow::Result<()> { + let mut f = fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(temp) + .map_err(|e| anyhow::anyhow!("failed to create {}: {e}", temp.display()))?; + f.write_all(content.as_bytes()) + .map_err(|e| anyhow::anyhow!("failed to write {}: {e}", temp.display()))?; + f.sync_all() + .map_err(|e| anyhow::anyhow!("failed to flush {}: {e}", temp.display()))?; + drop(f); + + set_mode(temp, mode)?; + + fs::rename(temp, path).map_err(|e| { + anyhow::anyhow!( + "failed to install {} from {}: {e}", + path.display(), + temp.display() + ) + })?; + + sync_parent_dir(path) +} + +/// Flushes the directory entry so the rename survives a crash. +/// +/// Returns a `Result` because the modules using this advertise crash-durable writes; a +/// silent failure here would make that claim untrue. +pub fn sync_parent_dir(target: &Path) -> anyhow::Result<()> { + let Some(dir) = target.parent() else { + return Ok(()); + }; + if dir.as_os_str().is_empty() { + return Ok(()); + } + sync_dir_impl(dir) +} + +#[cfg(unix)] +fn sync_dir_impl(dir: &Path) -> anyhow::Result<()> { + let f = fs::File::open(dir) + .map_err(|e| anyhow::anyhow!("failed to open {} for sync: {e}", dir.display()))?; + f.sync_all() + .map_err(|e| anyhow::anyhow!("failed to sync directory {}: {e}", dir.display())) +} + +/// Windows has no directory fsync; this build target exists only so the parsers and +/// transaction logic stay testable off Linux. +#[cfg(not(unix))] +fn sync_dir_impl(_dir: &Path) -> anyhow::Result<()> { + Ok(()) +} + +#[cfg(unix)] +fn set_mode(path: &Path, mode: u32) -> anyhow::Result<()> { + fs::set_permissions(path, fs::Permissions::from_mode(mode)) + .map_err(|e| anyhow::anyhow!("failed to chmod {mode:o} {}: {e}", path.display())) +} + +#[cfg(not(unix))] +fn set_mode(_path: &Path, _mode: u32) -> anyhow::Result<()> { + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn scratch(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("awg2_atomic_{}_{}", name, std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("scratch dir"); + dir + } + + #[test] + fn writes_and_replaces_a_regular_file() { + let dir = scratch("write"); + let target = dir.join("f"); + write_atomic_no_follow(&target, "one\n", 0o644).expect("first"); + assert_eq!(fs::read_to_string(&target).expect("read"), "one\n"); + write_atomic_no_follow(&target, "two\n", 0o644).expect("second"); + assert_eq!(fs::read_to_string(&target).expect("read"), "two\n"); + assert!(!temp_sibling(&target).expect("temp").exists()); + } + + #[test] + fn absent_destination_is_allowed() { + let dir = scratch("absent"); + assert!(assert_regular_or_absent(&dir.join("nope")).is_ok()); + } + + #[test] + fn directory_destination_is_refused() { + let dir = scratch("dir_dest"); + let err = assert_regular_or_absent(&dir).expect_err("must refuse"); + assert!(err.to_string().contains("non-regular file")); + } + + /// The arbitrary-file-overwrite case: root writing through a planted symlink. + #[cfg(unix)] + #[test] + fn symlink_destination_is_refused_and_target_untouched() { + let dir = scratch("symlink_dest"); + let victim = dir.join("victim"); + fs::write(&victim, "precious\n").expect("victim"); + let link = dir.join("link"); + std::os::unix::fs::symlink(&victim, &link).expect("symlink"); + + let err = write_atomic_no_follow(&link, "attacker\n", 0o644).expect_err("must refuse"); + assert!( + err.to_string() + .contains("refusing to write through symlink") + ); + assert_eq!(fs::read_to_string(&victim).expect("intact"), "precious\n"); + } + + /// A symlink at the temp path must not be followed either. + #[cfg(unix)] + #[test] + fn symlink_at_temp_path_is_not_followed() { + let dir = scratch("symlink_temp"); + let victim = dir.join("victim"); + fs::write(&victim, "precious\n").expect("victim"); + let target = dir.join("f"); + std::os::unix::fs::symlink(&victim, temp_sibling(&target).expect("temp")) + .expect("symlink temp"); + + write_atomic_no_follow(&target, "content\n", 0o644).expect("write"); + assert_eq!(fs::read_to_string(&victim).expect("intact"), "precious\n"); + assert_eq!(fs::read_to_string(&target).expect("read"), "content\n"); + } + + #[test] + fn stale_temp_file_does_not_block_write() { + let dir = scratch("stale"); + let target = dir.join("f"); + fs::write(temp_sibling(&target).expect("temp"), "garbage").expect("stale"); + write_atomic_no_follow(&target, "content\n", 0o644).expect("write"); + assert_eq!(fs::read_to_string(&target).expect("read"), "content\n"); + } + + #[cfg(unix)] + #[test] + fn mode_is_applied() { + let dir = scratch("mode"); + let target = dir.join("f"); + write_atomic_no_follow(&target, "x\n", 0o600).expect("write"); + let md = fs::metadata(&target).expect("stat"); + assert_eq!(md.permissions().mode() & 0o777, 0o600); + } +} diff --git a/traffic_core/src/config.rs b/traffic_core/src/config.rs index 4a42413..0f9748c 100644 --- a/traffic_core/src/config.rs +++ b/traffic_core/src/config.rs @@ -375,6 +375,11 @@ impl Config { "swapoff", "sysctl", "dd", + // The candidate /etc/fstab is structurally validated with + // `findmnt --verify` before it replaces the live file. That check is + // mandatory, so its absence must fail preflight rather than be discovered + // at rewrite time. + "findmnt", ]); } @@ -498,6 +503,7 @@ fn tool_exists(tool: &str) -> bool { "swapon" => &[&["--help"]], "swapoff" => &[&["--help"]], "sysctl" => &[&["--version"], &["--help"]], + "findmnt" => &[&["--version"]], "dd" => &[&["--help"]], _ => &[&["--version"], &["--help"], &["-V"]], }; diff --git a/traffic_core/src/fstab.rs b/traffic_core/src/fstab.rs index e3f00fa..0c8fc75 100644 --- a/traffic_core/src/fstab.rs +++ b/traffic_core/src/fstab.rs @@ -18,7 +18,12 @@ use std::{ process::Command, }; +use crate::atomic; + pub const FSTAB_PATH: &str = "/etc/fstab"; +/// Matches the Debian default for `/etc/fstab`; used for the backup and pristine copies, +/// which must not be more permissive than the file they mirror. +const FSTAB_MODE: u32 = 0o644; /// Rolling backup: the state immediately before the most recent rewrite. const BACKUP_SUFFIX: &str = ".awg2-harness.bak"; @@ -60,6 +65,9 @@ pub fn escape_field(raw: &str) -> String { out } +/// Validates a candidate fstab before it replaces the live one. +type CandidateVerifier = fn(&Path) -> anyhow::Result<()>; + /// An in-progress rewrite. Nothing is modified until [`FstabTransaction::commit`]. pub struct FstabTransaction { target: PathBuf, @@ -85,6 +93,15 @@ impl FstabTransaction { /// so there is no window in which the target is about to be replaced and no recovery /// copy exists. pub fn commit(self, candidate: &str) -> anyhow::Result { + self.commit_with(candidate, verify_candidate) + } + + /// Same as [`FstabTransaction::commit`], with the candidate validator injected. + /// + /// Production always passes [`verify_candidate`], which shells out to `findmnt`. The + /// seam exists so the transaction can be tested on a host without `util-linux`, and so + /// a rejecting validator can be exercised directly. + fn commit_with(self, candidate: &str, verify: CandidateVerifier) -> anyhow::Result { if candidate == self.original { return Ok(false); } @@ -103,7 +120,7 @@ impl FstabTransaction { return Err(e); } - if let Err(e) = verify_candidate(&temp) { + if let Err(e) = verify(&temp) { let _ = fs::remove_file(&temp); return Err(e); } @@ -117,23 +134,26 @@ impl FstabTransaction { ) })?; - sync_parent_dir(&self.target); + atomic::sync_parent_dir(&self.target)?; // Past the commit point: confirm what actually landed, and put the previous // content back if it is not what we wrote. match fs::read_to_string(&self.target) { - Ok(written) if written == candidate => Ok(true), + Ok(written) if written == candidate => { + reload_systemd_generators(); + Ok(true) + } Ok(_) => { - self.restore_backup()?; + self.restore_original()?; Err(anyhow::anyhow!( - "{} does not match the written content after rename; restored backup", + "{} does not match the written content after rename; restored original", self.target.display() )) } Err(e) => { - self.restore_backup()?; + self.restore_original()?; Err(anyhow::anyhow!( - "failed to re-read {} after rename: {e}; restored backup", + "failed to re-read {} after rename: {e}; restored original", self.target.display() )) } @@ -142,36 +162,37 @@ impl FstabTransaction { fn write_backups(&self) -> anyhow::Result<()> { let backup = sibling_path(&self.target, BACKUP_SUFFIX)?; - write_file_synced(&backup, &self.original) + atomic::write_atomic_no_follow(&backup, &self.original, FSTAB_MODE) .map_err(|e| anyhow::anyhow!("failed to write backup {}: {e}", backup.display()))?; let original = sibling_path(&self.target, ORIGINAL_SUFFIX)?; - match fs::OpenOptions::new() - .create_new(true) - .write(true) - .open(&original) - { - Ok(mut f) => { - f.write_all(self.original.as_bytes()).map_err(|e| { - anyhow::anyhow!("failed to write pristine copy {}: {e}", original.display()) - })?; - f.sync_all() - .map_err(|e| anyhow::anyhow!("failed to flush {}: {e}", original.display()))?; + // A pristine copy already exists: it records the host before this package first + // ran and must never be replaced by an already-managed fstab. It still has to be + // a regular file - a symlink or directory there means something is wrong. + match fs::symlink_metadata(&original) { + Ok(md) if md.file_type().is_file() => return Ok(()), + Ok(_) => { + return Err(anyhow::anyhow!( + "pristine copy {} is not a regular file", + original.display() + )); } - // Already captured by an earlier run: that copy is the one to keep. - Err(e) if e.kind() == ErrorKind::AlreadyExists => {} + Err(e) if e.kind() == ErrorKind::NotFound => {} Err(e) => { return Err(anyhow::anyhow!( - "failed to create pristine copy {}: {e}", + "failed to stat {}: {e}", original.display() )); } } - Ok(()) + atomic::write_atomic_no_follow(&original, &self.original, FSTAB_MODE).map_err(|e| { + anyhow::anyhow!("failed to write pristine copy {}: {e}", original.display()) + }) } fn write_candidate(&self, temp: &Path, candidate: &str) -> anyhow::Result<()> { - // Stale temp from a killed run must not be silently reused. + // Stale temp from a killed run must not be silently reused, and a symlink planted + // there must not be followed. match fs::remove_file(temp) { Ok(()) => {} Err(e) if e.kind() == ErrorKind::NotFound => {} @@ -198,17 +219,18 @@ impl FstabTransaction { Ok(()) } - fn restore_backup(&self) -> anyhow::Result<()> { - let backup = sibling_path(&self.target, BACKUP_SUFFIX)?; - fs::copy(&backup, &self.target).map_err(|e| { + /// Puts the original content back through the same atomic path as a forward write. + /// + /// The source is `self.original`, held in memory since `begin`, not the backup file: + /// re-reading the backup would add a failure mode to the recovery path, and a plain + /// `fs::copy` onto `/etc/fstab` could leave it truncated if it failed midway. + fn restore_original(&self) -> anyhow::Result<()> { + atomic::write_atomic_no_follow(&self.target, &self.original, FSTAB_MODE).map_err(|e| { anyhow::anyhow!( - "rollback failed: could not restore {} from {}: {e}", - self.target.display(), - backup.display() + "rollback failed: could not restore {}: {e}", + self.target.display() ) - })?; - sync_parent_dir(&self.target); - Ok(()) + }) } } @@ -221,28 +243,48 @@ fn sibling_path(target: &Path, suffix: &str) -> anyhow::Result { Ok(target.with_file_name(sibling)) } -fn write_file_synced(path: &Path, content: &str) -> std::io::Result<()> { - let mut f = fs::File::create(path)?; - f.write_all(content.as_bytes())?; - f.sync_all() -} - /// Asks `findmnt` whether the candidate parses as a valid tab file. /// -/// Skipped when `findmnt` cannot be spawned; when it does run, a rejection is fatal. +/// Mandatory, not best-effort. `findmnt` ships in `util-linux`, which the package already +/// depends on, so being unable to run it means the host is not in the state the package +/// requires - silently skipping the only structural validation of a file that decides +/// whether the machine boots is not an acceptable degradation. fn verify_candidate(temp: &Path) -> anyhow::Result<()> { let out = Command::new("findmnt") .args(["--verify", "--tab-file"]) .arg(temp) - .output(); - match out { - Ok(o) if o.status.success() => Ok(()), - Ok(o) => Err(anyhow::anyhow!( - "candidate fstab rejected by findmnt --verify: {}{}", - String::from_utf8_lossy(&o.stdout).trim(), - String::from_utf8_lossy(&o.stderr).trim() - )), - Err(_) => Ok(()), + .output() + .map_err(|e| { + anyhow::anyhow!("failed to execute findmnt while validating candidate fstab: {e}") + })?; + if out.status.success() { + return Ok(()); + } + Err(anyhow::anyhow!( + "candidate fstab rejected by findmnt --verify: {}{}", + String::from_utf8_lossy(&out.stdout).trim(), + String::from_utf8_lossy(&out.stderr).trim() + )) +} + +/// `/etc/fstab` feeds systemd's fstab generator as well as `mount`, so units stay stale +/// until the generators re-run. +/// +/// Best-effort by design: the fstab change has already been committed and verified, and +/// failing the whole operation because a notification did not go through would be worse +/// than a stale generator state that the next boot resolves anyway. +fn reload_systemd_generators() { + match Command::new("systemctl").arg("daemon-reload").output() { + Ok(out) if out.status.success() => {} + Ok(out) => tracing::warn!( + target: "fstab", + "systemctl daemon-reload failed after rewriting {FSTAB_PATH}: {}", + String::from_utf8_lossy(&out.stderr).trim() + ), + Err(e) => tracing::warn!( + target: "fstab", + "could not run systemctl daemon-reload after rewriting {FSTAB_PATH}: {e}" + ), } } @@ -259,20 +301,6 @@ fn copy_permissions(_from: &Path, _to: &Path) -> anyhow::Result<()> { Ok(()) } -/// Best-effort: the rename already made the change visible, and there is no useful -/// recovery if flushing the directory entry fails. -#[cfg(unix)] -fn sync_parent_dir(target: &Path) { - if let Some(dir) = target.parent() - && let Ok(f) = fs::File::open(dir) - { - let _ = f.sync_all(); - } -} - -#[cfg(not(unix))] -fn sync_parent_dir(_target: &Path) {} - #[cfg(test)] mod tests { use super::*; @@ -292,6 +320,17 @@ mod tests { const SAMPLE: &str = "UUID=aaaa / ext4 defaults 0 1\nUUID=bbbb /boot ext4 defaults 0 2\n"; + /// Stands in for `findmnt`, which does not exist off Linux. + fn accept_any(_: &Path) -> anyhow::Result<()> { + Ok(()) + } + + fn reject_any(_: &Path) -> anyhow::Result<()> { + Err(anyhow::anyhow!( + "candidate fstab rejected by findmnt --verify" + )) + } + /// The regression that motivates this module: an unreadable fstab must never be /// treated as empty, because the rewrite would then drop every real mount. #[test] @@ -322,7 +361,7 @@ mod tests { let candidate = format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"); let tx = FstabTransaction::begin(&target).expect("begin"); - assert!(tx.commit(&candidate).expect("commit")); + assert!(tx.commit_with(&candidate, accept_any).expect("commit")); assert_eq!(fs::read_to_string(&target).expect("read"), candidate); let backup = dir.join(format!("fstab{BACKUP_SUFFIX}")); @@ -336,7 +375,7 @@ mod tests { let dir = scratch("noop"); let target = seed(&dir, SAMPLE); let tx = FstabTransaction::begin(&target).expect("begin"); - assert!(!tx.commit(SAMPLE).expect("commit")); + assert!(!tx.commit_with(SAMPLE, accept_any).expect("commit")); assert!(!dir.join(format!("fstab{BACKUP_SUFFIX}")).exists()); } @@ -345,7 +384,9 @@ mod tests { let dir = scratch("empty"); let target = seed(&dir, SAMPLE); let tx = FstabTransaction::begin(&target).expect("begin"); - let err = tx.commit(" \n").expect_err("must refuse"); + let err = tx + .commit_with(" \n", accept_any) + .expect_err("must refuse"); assert!(err.to_string().contains("refusing to write an empty")); assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); } @@ -359,13 +400,13 @@ mod tests { let first = format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"); FstabTransaction::begin(&target) .expect("begin") - .commit(&first) + .commit_with(&first, accept_any) .expect("commit"); let second = format!("{SAMPLE}/swapfile none swap sw,pri=5 0 0\n"); FstabTransaction::begin(&target) .expect("begin") - .commit(&second) + .commit_with(&second, accept_any) .expect("commit"); let original = dir.join(format!("fstab{ORIGINAL_SUFFIX}")); @@ -383,7 +424,7 @@ mod tests { let candidate = format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"); FstabTransaction::begin(&target) .expect("begin") - .commit(&candidate) + .commit_with(&candidate, accept_any) .expect("commit"); assert_eq!(fs::read_to_string(&target).expect("read"), candidate); assert!(!dir.join(format!("fstab{TEMP_SUFFIX}")).exists()); @@ -400,12 +441,76 @@ mod tests { let tx = FstabTransaction::begin(&target).expect("begin"); let err = tx - .commit(&format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n")) + .commit_with( + &format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"), + accept_any, + ) .expect_err("must fail"); assert!(err.to_string().contains("failed to write backup")); assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); } + /// A rejected candidate must never reach the live file. + #[test] + fn rejected_candidate_leaves_target_untouched() { + let dir = scratch("rejected"); + let target = seed(&dir, SAMPLE); + let tx = FstabTransaction::begin(&target).expect("begin"); + + let err = tx + .commit_with( + &format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"), + reject_any, + ) + .expect_err("must reject"); + assert!(err.to_string().contains("rejected by findmnt")); + assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); + assert!(!dir.join(format!("fstab{TEMP_SUFFIX}")).exists()); + } + + /// The pristine copy must be a regular file; a symlink or directory there means the + /// recovery path cannot be trusted, so the commit refuses rather than proceeding. + #[test] + fn non_regular_pristine_copy_is_refused() { + let dir = scratch("pristine_bad"); + let target = seed(&dir, SAMPLE); + fs::create_dir(dir.join(format!("fstab{ORIGINAL_SUFFIX}"))).expect("blocker"); + + let tx = FstabTransaction::begin(&target).expect("begin"); + let err = tx + .commit_with( + &format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"), + accept_any, + ) + .expect_err("must refuse"); + assert!(err.to_string().contains("is not a regular file")); + assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); + } + + /// The backup is written through the no-follow atomic path, so a symlink planted at + /// the backup name cannot be used to make root overwrite its target. + #[cfg(unix)] + #[test] + fn symlinked_backup_path_is_refused() { + let dir = scratch("backup_symlink"); + let target = seed(&dir, SAMPLE); + let victim = dir.join("victim"); + fs::write(&victim, "precious\n").expect("victim"); + std::os::unix::fs::symlink(&victim, dir.join(format!("fstab{BACKUP_SUFFIX}"))) + .expect("symlink"); + + let tx = FstabTransaction::begin(&target).expect("begin"); + let err = tx + .commit_with( + &format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"), + accept_any, + ) + .expect_err("must refuse"); + assert!(err.to_string().contains("failed to write backup")); + assert_eq!(fs::read_to_string(&victim).expect("intact"), "precious\n"); + assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); + } + #[test] fn escapes_fstab_field_separators() { assert_eq!(escape_field("/var/my swap"), "/var/my\\040swap"); diff --git a/traffic_core/src/main.rs b/traffic_core/src/main.rs index fa8a632..02e99c4 100644 --- a/traffic_core/src/main.rs +++ b/traffic_core/src/main.rs @@ -1,3 +1,4 @@ +mod atomic; mod cli; mod config; mod daemon; diff --git a/traffic_core/src/managed_file.rs b/traffic_core/src/managed_file.rs index 3b98694..f7ce203 100644 --- a/traffic_core/src/managed_file.rs +++ b/traffic_core/src/managed_file.rs @@ -12,14 +12,12 @@ //! - a file whose first line does not carry our marker is never written or removed; //! - writes are atomic: temp sibling, `sync_all`, rename, directory flush. -use std::{ - fs, - io::{ErrorKind, Write}, - path::{Path, PathBuf}, -}; +use std::{fs, io::ErrorKind, path::PathBuf}; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; +use crate::atomic; + +/// Debian default for files under `/etc/sysctl.d` and `/etc/modules-load.d`. +const MANAGED_MODE: u32 = 0o644; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Ownership { @@ -54,12 +52,17 @@ impl ManagedFile { } } + /// Ownership is decided by an **exact** first line, not a substring. + /// + /// `contains` accepted a file whose first line merely embedded the marker, so + /// `# NOT Managed by awg2-harness sysctl_tuning` counted as ours and became eligible + /// for overwrite and removal. pub fn ownership(&self) -> anyhow::Result { let Some(content) = self.read()? else { return Ok(Ownership::Absent); }; let first = content.lines().next().unwrap_or_default(); - if first.contains(&self.marker) { + if first.trim_end() == self.marker.trim_end() { Ok(Ownership::Managed) } else { Ok(Ownership::Foreign) @@ -86,7 +89,7 @@ impl ManagedFile { } Ownership::Absent => {} } - write_atomic(&self.path, content)?; + atomic::write_atomic_no_follow(&self.path, content, MANAGED_MODE)?; Ok(true) } @@ -109,73 +112,10 @@ impl ManagedFile { } } -fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> { - let mut temp_name = path - .file_name() - .ok_or_else(|| anyhow::anyhow!("invalid managed file path: {}", path.display()))? - .to_os_string(); - temp_name.push(".awg2-harness.tmp"); - let temp = path.with_file_name(temp_name); - - match fs::remove_file(&temp) { - Ok(()) => {} - Err(e) if e.kind() == ErrorKind::NotFound => {} - Err(e) => { - return Err(anyhow::anyhow!( - "failed to clear stale {}: {e}", - temp.display() - )); - } - } - - let mut f = fs::OpenOptions::new() - .create_new(true) - .write(true) - .open(&temp) - .map_err(|e| anyhow::anyhow!("failed to create {}: {e}", temp.display()))?; - let write_result = f - .write_all(content.as_bytes()) - .and_then(|()| f.sync_all()) - .map_err(|e| anyhow::anyhow!("failed to write {}: {e}", temp.display())); - drop(f); - if let Err(e) = write_result { - let _ = fs::remove_file(&temp); - return Err(e); - } - - #[cfg(unix)] - if let Err(e) = fs::set_permissions(&temp, fs::Permissions::from_mode(0o644)) { - let _ = fs::remove_file(&temp); - return Err(anyhow::anyhow!( - "failed to set permissions on {}: {e}", - temp.display() - )); - } - - fs::rename(&temp, path).map_err(|e| { - let _ = fs::remove_file(&temp); - anyhow::anyhow!("failed to install {}: {e}", path.display()) - })?; - - sync_parent_dir(path); - Ok(()) -} - -#[cfg(unix)] -fn sync_parent_dir(target: &Path) { - if let Some(dir) = target.parent() - && let Ok(f) = fs::File::open(dir) - { - let _ = f.sync_all(); - } -} - -#[cfg(not(unix))] -fn sync_parent_dir(_target: &Path) {} - #[cfg(test)] mod tests { use super::*; + use std::path::Path; const MARKER: &str = "# Managed by awg2-harness test"; @@ -270,4 +210,33 @@ mod tests { assert!(f.write(&format!("{MARKER}\nkey = 1\n")).expect("write")); assert!(!dir.join("conf.awg2-harness.tmp").exists()); } + + /// `contains` used to accept any first line that embedded the marker, so a file + /// explicitly declaring it is *not* managed became eligible for overwrite. + #[test] + fn marker_must_match_the_whole_first_line() { + let dir = scratch("marker_exact"); + let f = managed(&dir); + for first_line in [ + format!("# NOT {}", MARKER.trim_start_matches("# ")), + format!("{MARKER} but actually hand written"), + format!("prefix {MARKER}"), + ] { + fs::write(dir.join("conf"), format!("{first_line}\nkey = 1\n")).expect("seed"); + assert_eq!( + f.ownership().expect("ownership"), + Ownership::Foreign, + "must not claim ownership of first line {first_line:?}" + ); + } + } + + /// Trailing whitespace on the marker line is not a reason to disown our own file. + #[test] + fn marker_comparison_ignores_trailing_whitespace() { + let dir = scratch("marker_trailing"); + let f = managed(&dir); + fs::write(dir.join("conf"), format!("{MARKER} \nkey = 1\n")).expect("seed"); + assert_eq!(f.ownership().expect("ownership"), Ownership::Managed); + } }