diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 8802f48..c8912bb 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -10,6 +10,9 @@ on: - main workflow_dispatch: +permissions: + contents: read + env: CARGO_TERM_COLOR: always @@ -45,3 +48,15 @@ jobs: run: | chmod +x ./verify.sh ./verify.sh + + - name: Clippy Analysis + run: cd server_manager && cargo clippy --all-targets --all-features -- -D warnings + + - name: Unit & Integration Tests + run: cd server_manager && cargo test --all-features + + - name: Dependency License & Bans Check + run: cd server_manager && cargo deny check + + - name: Vulnerability Audit + run: cd server_manager && cargo audit diff --git a/AUDIT_REPORT.md b/AUDIT_REPORT.md index 97b5ef8..6538f6c 100644 --- a/AUDIT_REPORT.md +++ b/AUDIT_REPORT.md @@ -21,3 +21,11 @@ A comprehensive security and robustness audit of `server_manager` was performed, ## Next Steps All deployed changes have been systematically verified using the project's native contract testing suite (`./verify.sh`), which successfully confirmed functional integrity without introducing performance degradation. + +## 4. Remaining Strict Path and Argument Boundary Defenses +- **Finding (Medium):** The initial audit remediations enforcing absolute paths missed fallbacks in secondary utilities (`df`, `apt-get`, `sysctl`, `systemctl`, `journalctl`, `timedatectl`, `git`) which would still fallback to `$PATH` if the absolute path was absent, re-introducing path substitution vulnerability. Furthermore, argument bounds (`--`) were missing in `systemctl` commands. +- **Remediation:** Removed string fallbacks for all binary lookups ensuring hard failures if the binary does not exist at the trusted absolute path. Added explicit `--` bounds to `systemctl` arguments to protect against injection. + +## 5. Test File Persistence Hazards +- **Finding (Low):** Raw `std::fs::write` usages remained within `config.rs` and `journal.rs` test suites. +- **Remediation:** Replaced remaining `fs::write` calls in tests with the project native `atomic_io::atomic_write_str`. diff --git a/server_manager/src/core/config.rs b/server_manager/src/core/config.rs index 1d3bca0..9756405 100644 --- a/server_manager/src/core/config.rs +++ b/server_manager/src/core/config.rs @@ -116,12 +116,13 @@ mod tests { assert!(cfg.disabled_services.is_empty()); let empty_file = temp_dir.join("empty.yaml"); - fs::write(&empty_file, " \n ").unwrap(); + crate::core::atomic_io::atomic_write_str(&empty_file, " \n ", 0o644).unwrap(); let cfg2 = Config::load_from(&empty_file).unwrap(); assert!(cfg2.disabled_services.is_empty()); let invalid_file = temp_dir.join("invalid.yaml"); - fs::write(&invalid_file, ": : invalid yaml :::").unwrap(); + crate::core::atomic_io::atomic_write_str(&invalid_file, ": : invalid yaml :::", 0o644) + .unwrap(); let err = Config::load_from(&invalid_file); assert!(err.is_err()); assert_eq!(err.unwrap_err().to_string(), "Invalid config YAML"); diff --git a/server_manager/src/core/doctor.rs b/server_manager/src/core/doctor.rs index 18d7e35..edd1665 100644 --- a/server_manager/src/core/doctor.rs +++ b/server_manager/src/core/doctor.rs @@ -339,7 +339,12 @@ pub fn check_disk_space() -> DoctorCheckResult { } else if std::path::Path::new("/usr/bin/df").exists() { "/usr/bin/df" } else { - "df" + return DoctorCheckResult { + name: "Disk Capacity".to_string(), + status: CheckStatus::Skipped, + message: "df utility not available or disk space query failed".to_string(), + details: None, + }; }; if let Ok(output) = Command::new(df_path).arg("-Pk").arg(".").output() { if output.status.success() { @@ -390,7 +395,12 @@ pub fn check_ntp_sync() -> DoctorCheckResult { } else if std::path::Path::new("/bin/timedatectl").exists() { "/bin/timedatectl" } else { - "timedatectl" + return DoctorCheckResult { + name: "NTP Time Sync".to_string(), + status: CheckStatus::Skipped, + message: "timedatectl or /etc/localtime not accessible".to_string(), + details: None, + }; }; if let Ok(output) = Command::new(timedatectl_path).arg("status").output() { if output.status.success() { diff --git a/server_manager/src/core/journal.rs b/server_manager/src/core/journal.rs index 9406695..8abbe74 100644 --- a/server_manager/src/core/journal.rs +++ b/server_manager/src/core/journal.rs @@ -303,8 +303,9 @@ mod tests { let target_file = temp_dir.join("config.txt"); let backup_file = temp_dir.join("config.txt.bak"); - fs::write(&target_file, "corrupted state").unwrap(); - fs::write(&backup_file, "original good state").unwrap(); + crate::core::atomic_io::atomic_write_str(&target_file, "corrupted state", 0o644).unwrap(); + crate::core::atomic_io::atomic_write_str(&backup_file, "original good state", 0o644) + .unwrap(); let action = CompensatoryAction::RestoreFile { path: target_file.clone(), @@ -345,7 +346,7 @@ mod tests { let op_id = generate_op_id(); let target_file = temp_dir.join("target.txt"); - fs::write(&target_file, "should be deleted").unwrap(); + crate::core::atomic_io::atomic_write_str(&target_file, "should be deleted", 0o644).unwrap(); let step1 = JournalEntry { timestamp: now_iso8601(), diff --git a/server_manager/src/core/ops.rs b/server_manager/src/core/ops.rs index 9ad955d..4dac3b7 100644 --- a/server_manager/src/core/ops.rs +++ b/server_manager/src/core/ops.rs @@ -76,7 +76,7 @@ impl SystemOps for RealSystemOps { fn is_service_active(&self, service_name: &str) -> bool { Command::new("/usr/bin/systemctl") - .args(["is-active", "--quiet", service_name]) + .args(["is-active", "--quiet", "--", service_name]) .status() .map(|s| s.success()) .unwrap_or(false) @@ -84,10 +84,10 @@ impl SystemOps for RealSystemOps { fn stop_system_service(&self, service_name: &str) -> Result<()> { let _ = Command::new("/usr/bin/systemctl") - .args(["stop", service_name]) + .args(["stop", "--", service_name]) .status(); let _ = Command::new("/usr/bin/systemctl") - .args(["disable", service_name]) + .args(["disable", "--", service_name]) .status(); Ok(()) } @@ -97,7 +97,7 @@ impl SystemOps for RealSystemOps { let path = if Path::new("/usr/bin/journalctl").exists() { "/usr/bin/journalctl" } else { - "journalctl" + bail!("journalctl not found at absolute path /usr/bin/journalctl"); }; let status = Command::new(path) .arg(&arg) diff --git a/server_manager/src/core/system.rs b/server_manager/src/core/system.rs index 8daf448..dacb970 100644 --- a/server_manager/src/core/system.rs +++ b/server_manager/src/core/system.rs @@ -45,7 +45,7 @@ pub fn install_dependencies() -> Result<()> { let apt_path = if std::path::Path::new("/usr/bin/apt-get").exists() { "/usr/bin/apt-get" } else { - "apt-get" + bail!("apt-get not found at absolute path /usr/bin/apt-get"); }; let update_status = Command::new(apt_path) .env("DEBIAN_FRONTEND", "noninteractive") @@ -74,7 +74,7 @@ pub fn install_dependencies() -> Result<()> { let systemctl_path = if std::path::Path::new("/usr/bin/systemctl").exists() { "/usr/bin/systemctl" } else { - "systemctl" + bail!("systemctl not found at absolute path /usr/bin/systemctl"); }; let _ = Command::new(systemctl_path) .args(["enable", "--now", "fail2ban"]) @@ -222,7 +222,7 @@ fn get_home_device() -> Result { } else if std::path::Path::new("/usr/bin/df").exists() { "/usr/bin/df" } else { - "df" + bail!("df not found at absolute paths /bin/df or /usr/bin/df"); }; let output = Command::new(df_path) .arg("-P") @@ -330,7 +330,7 @@ net.core.wmem_max=1048576 } else if std::path::Path::new("/usr/sbin/sysctl").exists() { "/usr/sbin/sysctl" } else { - "sysctl" + bail!("sysctl not found at absolute paths /sbin/sysctl or /usr/sbin/sysctl"); }; let status = Command::new(sysctl_path) .arg("--system") diff --git a/server_manager/src/core/updater.rs b/server_manager/src/core/updater.rs index 0b12218..62c624b 100644 --- a/server_manager/src/core/updater.rs +++ b/server_manager/src/core/updater.rs @@ -32,7 +32,12 @@ pub fn check_for_updates() -> Result { let git_path = if std::path::Path::new("/usr/bin/git").exists() { "/usr/bin/git" } else { - "git" + return Ok(UpdateInfo { + current_version: current, + latest_version: latest, + update_available: false, + release_notes: "Git not found; cannot check for updates.".to_string(), + }); }; if let Ok(git_output) = Command::new(git_path) .args(["describe", "--tags", "--abbrev=0"]) @@ -96,7 +101,7 @@ pub fn self_update() -> Result { let git_path = if std::path::Path::new("/usr/bin/git").exists() { "/usr/bin/git" } else { - "git" + bail!("git not found at absolute path /usr/bin/git"); }; let pull_status = Command::new(git_path) .current_dir(repo_dir) diff --git a/server_manager/src/interface/web.rs b/server_manager/src/interface/web.rs index 7e8f8ff..9bfef21 100644 --- a/server_manager/src/interface/web.rs +++ b/server_manager/src/interface/web.rs @@ -1412,11 +1412,9 @@ async fn toggle_service_in_process(service_name: &str, enable: bool) -> anyhow:: use crate::core::{config, hardware, lock::ProcessLock, secrets}; // Serialize service mutations with an advisory lock across processes and concurrent requests - let _lock = tokio::task::spawn_blocking(|| { - ProcessLock::acquire_default_blocking(false) - }) - .await - .map_err(|e| anyhow::anyhow!("Task join error acquiring lock: {}", e))??; + let _lock = tokio::task::spawn_blocking(|| ProcessLock::acquire_default_blocking(false)) + .await + .map_err(|e| anyhow::anyhow!("Task join error acquiring lock: {}", e))??; let mut config = config::Config::load_async().await?; if enable {