From 74564bb045093af6a3637102c91964bf453b596b Mon Sep 17 00:00:00 2001 From: Cylae Date: Sun, 13 Sep 2026 20:27:43 +0200 Subject: [PATCH] fix(persistence): lock state updates and protect stored credentials --- docs/audit/CONTRACT.md | 2 +- server_manager/src/core/atomic_io.rs | 113 ++++----- server_manager/src/core/config.rs | 236 +++++------------- server_manager/src/core/lock.rs | 18 +- server_manager/src/core/secrets.rs | 18 +- server_manager/src/interface/cli.rs | 2 +- .../tests/regression_persistence.rs | 124 +++++++++ 7 files changed, 263 insertions(+), 250 deletions(-) create mode 100644 server_manager/tests/regression_persistence.rs diff --git a/docs/audit/CONTRACT.md b/docs/audit/CONTRACT.md index f8ebf53..a270d82 100644 --- a/docs/audit/CONTRACT.md +++ b/docs/audit/CONTRACT.md @@ -30,7 +30,7 @@ This document freezes the established user-facing contract of `server_manager`. | `/opt/server_manager/config.yaml` | YAML | `0644` | System configuration (domain, port bindings, profile, enabled services). | | `/opt/server_manager/secrets.yaml` | YAML | `0600` | Administrative credentials, database passwords, API tokens. | | `/opt/server_manager/users.yaml` | YAML | `0600` | User account database (usernames, password hashes, roles, quotas, installed apps). | -| `/opt/server_manager/docker-compose.yml` | YAML | `0644` | Generated Docker Compose stack definition. | +| `/opt/server_manager/docker-compose.yml` | YAML | `0600` | Generated Docker Compose stack definition. | --- diff --git a/server_manager/src/core/atomic_io.rs b/server_manager/src/core/atomic_io.rs index 07e2030..bcae03e 100644 --- a/server_manager/src/core/atomic_io.rs +++ b/server_manager/src/core/atomic_io.rs @@ -4,81 +4,68 @@ use std::io::Write; use std::path::Path; #[cfg(unix)] -use std::os::unix::fs::OpenOptionsExt; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; -/// Atomically writes content to a file. -/// -/// Steps: -/// 1. Creates a temporary file in the same directory as `path` (guaranteeing same filesystem/mount). -/// 2. Sets explicit permissions on creation (e.g. 0600 or 0644 on Unix). -/// 3. Writes content and flushes buffers. -/// 4. Synchronizes to disk via `fsync` (`sync_all`). -/// 5. Atomically renames the temporary file to the destination path. +/// Persist a complete file with explicit permissions and file + directory fsync. +/// Callers must additionally lock an entire read-modify-write transaction. pub fn atomic_write>(path: P, content: &[u8], mode: u32) -> Result<()> { let dest = path.as_ref(); - let parent = dest.parent().unwrap_or_else(|| Path::new(".")); - if !parent.as_os_str().is_empty() { - fs::create_dir_all(parent) - .with_context(|| format!("Failed to create parent directory {}", parent.display()))?; - } - - let file_name = dest - .file_name() - .map(|s| s.to_string_lossy().to_string()) - .unwrap_or_else(|| "temp_file".to_string()); - - let pid = std::process::id(); - let nanos = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos(); - - let tmp_name = format!(".tmp.{}.{}.{}", file_name, pid, nanos); - let tmp_path = parent.join(tmp_name); - - let write_result = (|| -> Result<()> { - let mut options = OpenOptions::new(); - options.write(true).create_new(true); - - #[cfg(unix)] - options.mode(mode); - - let mut file = options - .open(&tmp_path) - .with_context(|| format!("Failed to create temporary file {}", tmp_path.display()))?; + let parent = dest + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + let name = dest.file_name().context("Destination must name a file")?; + fs::create_dir_all(parent).context("Failed to create output directory")?; + let lock_path = parent.join(format!(".{}.lock", name.to_string_lossy())); + let _lock = crate::core::lock::ProcessLock::acquire(lock_path, false)?; - file.write_all(content) - .with_context(|| format!("Failed to write content to {}", tmp_path.display()))?; - file.flush() - .with_context(|| format!("Failed to flush temporary file {}", tmp_path.display()))?; - file.sync_all() - .with_context(|| format!("Failed to fsync temporary file {}", tmp_path.display()))?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let _ = fs::set_permissions(&tmp_path, fs::Permissions::from_mode(mode)); + // Preserve timestamps on idempotent writes; never follow destination symlinks. + if let Ok(meta) = fs::symlink_metadata(dest) { + if meta.is_file() && fs::read(dest).is_ok_and(|old| old == content) { + #[cfg(unix)] + if meta.permissions().mode() & 0o777 != mode { + let file = OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(dest)?; + file.set_permissions(fs::Permissions::from_mode(mode))?; + file.sync_all()?; + } + return Ok(()); } + } - fs::rename(&tmp_path, dest).with_context(|| { - format!( - "Failed to atomically rename {} to {}", - tmp_path.display(), - dest.display() - ) - })?; - + let tmp_path = parent.join(format!( + ".tmp.{}.{:032x}", + name.to_string_lossy(), + rand::random::() + )); + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(mode); + // Only remove a temporary file after this process successfully created it. + let mut file = options + .open(&tmp_path) + .context("Failed to create temporary output")?; + let result = (|| -> Result<()> { + #[cfg(unix)] + file.set_permissions(fs::Permissions::from_mode(mode)) + .context("Failed to set output permissions")?; + file.write_all(content).context("Failed to write output")?; + file.sync_all().context("Failed to fsync output")?; + fs::rename(&tmp_path, dest).context("Failed to atomically replace output")?; + fs::File::open(parent)? + .sync_all() + .context("Failed to fsync output directory")?; Ok(()) })(); - - if write_result.is_err() && tmp_path.exists() { + if result.is_err() { let _ = fs::remove_file(&tmp_path); } - - write_result + result } -/// Helper function to atomically write a string slice. pub fn atomic_write_str>(path: P, content: &str, mode: u32) -> Result<()> { atomic_write(path, content.as_bytes(), mode) } diff --git a/server_manager/src/core/config.rs b/server_manager/src/core/config.rs index d63df40..60a26a3 100644 --- a/server_manager/src/core/config.rs +++ b/server_manager/src/core/config.rs @@ -4,212 +4,100 @@ use serde::{Deserialize, Serialize}; use std::collections::HashSet; use std::fs; use std::path::{Path, PathBuf}; -use std::sync::OnceLock; -use std::time::SystemTime; -use tokio::sync::RwLock; - -#[derive(Debug, Clone)] -struct CachedConfig { - config: Config, - last_mtime: Option, - loaded_path: Option, -} - -static CONFIG_CACHE: OnceLock> = OnceLock::new(); #[derive(Debug, Serialize, Deserialize, Default, Clone)] pub struct Config { - #[serde(default)] + #[serde(default, serialize_with = "serialize_sorted")] pub disabled_services: HashSet, } +fn serialize_sorted( + set: &HashSet, + serializer: S, +) -> Result { + let mut values: Vec<_> = set.iter().collect(); + values.sort(); + values.serialize(serializer) +} + impl Config { - fn get_config_path() -> PathBuf { - let local_path = Path::new("./config.yaml"); - let opt_path = Path::new("/opt/server_manager/config.yaml"); - if local_path.exists() { - local_path.to_path_buf() - } else if opt_path.exists() { - opt_path.to_path_buf() + pub fn get_config_path() -> PathBuf { + let local = Path::new("config.yaml"); + let installed = Path::new("/opt/server_manager/config.yaml"); + if local.exists() || !installed.exists() { + local.into() } else { - // Default to local if neither exists - PathBuf::from("./config.yaml") + installed.into() } } - pub fn load() -> Result { - let path = Self::get_config_path(); - if path.exists() { - let content = fs::read_to_string(&path) - .with_context(|| format!("Failed to read {}", path.display()))?; - if content.trim().is_empty() { - return Ok(Config::default()); - } - serde_yaml_ng::from_str(&content) - .with_context(|| format!("Failed to parse {}", path.display())) - } else { - Ok(Config::default()) + pub fn load_from(path: &Path) -> Result { + match fs::read_to_string(path) { + Ok(content) if content.trim().is_empty() => Ok(Self::default()), + Ok(content) => serde_yaml_ng::from_str(&content) + .map_err(|_| anyhow::anyhow!("Invalid config YAML")), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Self::default()), + Err(e) => Err(e).context("Failed to read config YAML"), } } + pub fn load() -> Result { + Self::load_from(&Self::get_config_path()) + } pub async fn load_async() -> Result { - let cache = CONFIG_CACHE.get_or_init(|| { - RwLock::new(CachedConfig { - config: Config::default(), - last_mtime: None, - loaded_path: None, - }) - }); - - let path = Self::get_config_path(); - - // Fast path: Optimistic read - { - let guard = cache.read().await; - if let Some(cached_mtime) = guard.last_mtime { - if let Ok(metadata) = tokio::fs::metadata(&path).await { - if let Ok(modified) = metadata.modified() { - if modified == cached_mtime { - return Ok(guard.config.clone()); - } - } - } - } - } - - // Slow path: Update cache - let mut guard = cache.write().await; - - let metadata_res = tokio::fs::metadata(&path).await; - - match metadata_res { - Ok(metadata) => { - let modified = metadata.modified().unwrap_or_else(|_| SystemTime::now()); - - if let Some(cached_mtime) = guard.last_mtime { - if modified == cached_mtime { - return Ok(guard.config.clone()); - } - } - - match tokio::fs::read_to_string(&path).await { - Ok(content) => { - let config = if content.trim().is_empty() { - Config::default() - } else { - serde_yaml_ng::from_str(&content) - .with_context(|| format!("Failed to parse {}", path.display()))? - }; - - guard.config = config.clone(); - guard.last_mtime = Some(modified); - guard.loaded_path = Some(path); - Ok(config) - } - Err(e) => { - Err(anyhow::Error::new(e) - .context(format!("Failed to read {}", path.display()))) - } - } - } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - guard.config = Config::default(); - guard.last_mtime = None; - guard.loaded_path = Some(path); - Ok(guard.config.clone()) - } - Err(e) => Err(anyhow::Error::new(e) - .context(format!("Failed to read metadata for {}", path.display()))), - } + tokio::task::spawn_blocking(Self::load).await? } - pub fn save(&self) -> Result<()> { - let path = Self::get_config_path(); + pub fn save_to(&self, path: &Path) -> Result<()> { let content = serde_yaml_ng::to_string(self)?; - crate::core::atomic_io::atomic_write_str(&path, &content, 0o644) - .with_context(|| format!("Failed to write {}", path.display()))?; - Ok(()) + crate::core::atomic_io::atomic_write_str(path, &content, 0o644) } - - pub fn is_enabled(&self, service_name: &str) -> bool { - !self.disabled_services.contains(service_name) + pub fn save(&self) -> Result<()> { + self.save_to(&Self::get_config_path()) } - - pub fn enable_service(&mut self, service_name: &str) { - if self.disabled_services.remove(service_name) { - info!("Enabled service: {}", service_name); + pub fn is_enabled(&self, name: &str) -> bool { + !self.disabled_services.contains(name) + } + pub fn enable_service(&mut self, name: &str) { + if self.disabled_services.remove(name) { + info!("Enabled service: {}", name); } } - - pub fn disable_service(&mut self, service_name: &str) { - if self.disabled_services.insert(service_name.to_string()) { - info!("Disabled service: {}", service_name); + pub fn disable_service(&mut self, name: &str) { + if self.disabled_services.insert(name.into()) { + info!("Disabled service: {}", name); } } - pub async fn update_service_async(service_name: &str, update_fn: F) -> Result<()> + pub fn update_service_at(path: &Path, name: &str, update: F) -> Result<()> where F: FnOnce(&mut Self, &str) -> bool, { - let cache = CONFIG_CACHE.get_or_init(|| { - RwLock::new(CachedConfig { - config: Config::default(), - last_mtime: None, - loaded_path: None, - }) - }); - - let mut guard = cache.write().await; - let path = Self::get_config_path(); - - // Reload if stale before applying modification - if let Ok(metadata) = tokio::fs::metadata(&path).await { - let modified = metadata.modified().unwrap_or_else(|_| SystemTime::now()); - if guard.last_mtime != Some(modified) { - if let Ok(content) = tokio::fs::read_to_string(&path).await { - guard.config = if content.trim().is_empty() { - Config::default() - } else { - serde_yaml_ng::from_str(&content).unwrap_or_default() - }; - } - } + crate::core::validate::validate_service_name(name)?; + let _lock = crate::core::lock::ProcessLock::acquire( + path.with_extension("transaction.lock"), + false, + )?; + let mut config = Self::load_from(path)?; + if update(&mut config, name) { + config.save_to(path)?; } - - let changed = update_fn(&mut guard.config, service_name); - - if changed { - let content = serde_yaml_ng::to_string(&guard.config)?; - tokio::fs::write(&path, content).await?; - if let Ok(metadata) = tokio::fs::metadata(&path).await { - guard.last_mtime = Some(metadata.modified().unwrap_or_else(|_| SystemTime::now())); - } - } - Ok(()) } - pub async fn enable_service_async(service_name: &str) -> Result<()> { - Self::update_service_async(service_name, |config, name| { - if config.disabled_services.remove(name) { - info!("Enabled service: {}", name); - true - } else { - false - } - }) - .await + pub async fn update_service_async(name: &str, update: F) -> Result<()> + where + F: FnOnce(&mut Self, &str) -> bool + Send + 'static, + { + let path = Self::get_config_path(); + let name = name.to_owned(); + tokio::task::spawn_blocking(move || Self::update_service_at(&path, &name, update)).await? } - - pub async fn disable_service_async(service_name: &str) -> Result<()> { - Self::update_service_async(service_name, |config, name| { - if config.disabled_services.insert(name.to_string()) { - info!("Disabled service: {}", name); - true - } else { - false - } - }) - .await + pub async fn enable_service_async(name: &str) -> Result<()> { + Self::update_service_async(name, |cfg, name| cfg.disabled_services.remove(name)).await + } + pub async fn disable_service_async(name: &str) -> Result<()> { + Self::update_service_async(name, |cfg, name| cfg.disabled_services.insert(name.into())) + .await } } diff --git a/server_manager/src/core/lock.rs b/server_manager/src/core/lock.rs index fb3e8cd..1a549b7 100644 --- a/server_manager/src/core/lock.rs +++ b/server_manager/src/core/lock.rs @@ -18,16 +18,22 @@ impl ProcessLock { let target = path.as_ref(); let parent = target.parent().unwrap_or_else(|| Path::new(".")); if !parent.as_os_str().is_empty() { - let _ = std::fs::create_dir_all(parent); + std::fs::create_dir_all(parent).context("Failed to create lock directory")?; } - let file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) + let mut options = OpenOptions::new(); + options.read(true).write(true).create(true).truncate(false); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC); + } + let file = options .open(target) .with_context(|| format!("Failed to open lockfile {}", target.display()))?; + anyhow::ensure!(file.metadata()?.is_file(), "Lock must be a regular file"); #[cfg(unix)] { diff --git a/server_manager/src/core/secrets.rs b/server_manager/src/core/secrets.rs index 934c785..fb8aee9 100644 --- a/server_manager/src/core/secrets.rs +++ b/server_manager/src/core/secrets.rs @@ -92,11 +92,19 @@ impl Secrets { pub fn load_or_create() -> Result { let path = Self::get_secrets_path(); + Self::load_or_create_at(&path) + } + + pub fn load_or_create_at(path: &Path) -> Result { + let _lock = crate::core::lock::ProcessLock::acquire( + path.with_extension("transaction.lock"), + false, + )?; let mut secrets: Secrets = if path.exists() { - let content = fs::read_to_string(&path) + let content = fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display()))?; serde_yaml_ng::from_str(&content) - .with_context(|| format!("Failed to parse {}", path.display()))? + .map_err(|_| anyhow::anyhow!("Invalid secrets YAML; values withheld"))? } else { Secrets::default() }; @@ -149,10 +157,10 @@ impl Secrets { if changed { info!("Generated new secrets."); - let content = serde_yaml_ng::to_string(&secrets)?; - atomic_io::atomic_write_str(&path, &content, 0o600) - .with_context(|| format!("Failed to write {}", path.display()))?; } + let content = serde_yaml_ng::to_string(&secrets)?; + atomic_io::atomic_write_str(path, &content, 0o600) + .with_context(|| format!("Failed to write {}", path.display()))?; Ok(secrets) } diff --git a/server_manager/src/interface/cli.rs b/server_manager/src/interface/cli.rs index e2106bf..e8b0b59 100644 --- a/server_manager/src/interface/cli.rs +++ b/server_manager/src/interface/cli.rs @@ -664,7 +664,7 @@ async fn generate_compose( info!("Generating docker-compose.yml based on hardware profile..."); let yaml_output = crate::generate_compose_yaml(hw, secrets, config)?; - crate::core::atomic_io::atomic_write_str("docker-compose.yml", &yaml_output, 0o644) + crate::core::atomic_io::atomic_write_str("docker-compose.yml", &yaml_output, 0o600) .context("Failed to write docker-compose.yml")?; info!("docker-compose.yml generated."); diff --git a/server_manager/tests/regression_persistence.rs b/server_manager/tests/regression_persistence.rs new file mode 100644 index 0000000..b1b945d --- /dev/null +++ b/server_manager/tests/regression_persistence.rs @@ -0,0 +1,124 @@ +use server_manager::core::{ + atomic_io::atomic_write, config::Config, lock::ProcessLock, secrets::Secrets, +}; +use std::{fs, path::PathBuf, sync::Arc}; + +struct Directory(PathBuf); +impl Directory { + fn new() -> Self { + let path = std::env::temp_dir().join(format!( + "server-manager-regression-{:032x}", + rand::random::() + )); + fs::create_dir_all(&path).unwrap(); + Self(path) + } +} +impl Drop for Directory { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +#[test] +fn regression_a07_atomic_write_preserves_bytes_and_mtime_on_repetition() { + let dir = Directory::new(); + let path = dir.0.join("data"); + atomic_write(&path, b"first", 0o600).unwrap(); + let modified = fs::metadata(&path).unwrap().modified().unwrap(); + atomic_write(&path, b"first", 0o600).unwrap(); + assert_eq!(modified, fs::metadata(&path).unwrap().modified().unwrap()); + atomic_write(&path, b"second", 0o600).unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"second"); + assert!(!fs::read_dir(&dir.0).unwrap().any(|entry| entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".tmp."))); +} + +#[cfg(unix)] +#[test] +fn regression_a07_rejects_symlink_lock_without_touching_target() { + use std::os::unix::fs::symlink; + let dir = Directory::new(); + let victim = dir.0.join("victim"); + fs::write(&victim, "unchanged").unwrap(); + let lock = dir.0.join("lock"); + symlink(&victim, &lock).unwrap(); + assert!(ProcessLock::acquire(&lock, true).is_err()); + assert_eq!(fs::read_to_string(victim).unwrap(), "unchanged"); +} + +#[cfg(unix)] +#[test] +fn regression_a02_existing_secrets_permissions_are_repaired_without_rotation() { + use std::os::unix::fs::PermissionsExt; + let dir = Directory::new(); + let path = dir.0.join("secrets.yaml"); + let first = Secrets::load_or_create_at(&path).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + let second = Secrets::load_or_create_at(&path).unwrap(); + assert_eq!( + first.server_manager_admin_password, + second.server_manager_admin_password + ); + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); +} + +#[test] +fn regression_a07_corrupt_state_is_never_overwritten() { + let dir = Directory::new(); + let path = dir.0.join("config.yaml"); + let corrupt = b"disabled_services: [unclosed"; + fs::write(&path, corrupt).unwrap(); + assert!(Config::update_service_at(&path, "plex", |cfg, name| cfg + .disabled_services + .insert(name.into())) + .is_err()); + assert_eq!(fs::read(&path).unwrap(), corrupt); + let secret_path = dir.0.join("secrets.yaml"); + let secret = "mysql_root_password: [PRIVATE_SENTINEL"; + fs::write(&secret_path, secret).unwrap(); + let error = Secrets::load_or_create_at(&secret_path).unwrap_err(); + assert!(!format!("{error:#}").contains("PRIVATE_SENTINEL")); + assert_eq!(fs::read_to_string(&secret_path).unwrap(), secret); +} + +#[test] +fn regression_a07_concurrent_config_updates_do_not_lose_changes() { + let dir = Directory::new(); + let path = Arc::new(dir.0.join("config.yaml")); + let barrier = Arc::new(std::sync::Barrier::new(8)); + let threads: Vec<_> = (0..8) + .map(|i| { + let path = Arc::clone(&path); + let barrier = Arc::clone(&barrier); + std::thread::spawn(move || { + barrier.wait(); + Config::update_service_at(&path, &format!("service{i}"), |cfg, name| { + cfg.disabled_services.insert(name.into()) + }) + .unwrap(); + }) + }) + .collect(); + for thread in threads { + thread.join().unwrap(); + } + assert_eq!(Config::load_from(&path).unwrap().disabled_services.len(), 8); +} + +#[test] +fn regression_a11_config_reads_use_the_requested_file() { + let dir = Directory::new(); + let first = dir.0.join("first.yaml"); + let second = dir.0.join("second.yaml"); + fs::write(&first, "disabled_services: [plex]\n").unwrap(); + fs::write(&second, "disabled_services: [jellyfin]\n").unwrap(); + assert!(!Config::load_from(&first).unwrap().is_enabled("plex")); + assert!(Config::load_from(&second).unwrap().is_enabled("plex")); +}