diff --git a/Cargo.toml b/Cargo.toml index 51ffc9a..41accbb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ log = "0.4" clap = { version = "4.0", features = ["derive"] } config = "0.15.13" pretty_env_logger = "0.5.0" -nix = "0.31.1" +nix = { version = "0.31.1", features = ["sched", "fs", "user"] } glob = "0.3.0" serde = "1.0" serde_json = "1.0" diff --git a/src/lib/mount.rs b/src/lib/mount.rs index 15babd6..369617d 100644 --- a/src/lib/mount.rs +++ b/src/lib/mount.rs @@ -1,195 +1,155 @@ // SPDX-License-Identifier: BSD-3-Clause -use log::{info, warn}; +#[cfg(not(feature = "test-remount"))] +use log::info; +#[cfg(not(feature = "test-remount"))] use std::fs; use std::path::Path; #[cfg(not(feature = "test-remount"))] use std::process::{Command, Stdio}; use thiserror::Error; -/// Shared path to mount info used by default helpers -static MOUNT_INFO_PATH: &str = "/proc/mounts"; - #[derive(Debug, Error)] pub enum MountError { #[error("Failed to remount /boot: {0}")] RemountFailed(String), - #[error("Failed to read mount info")] - MountInfoError, + #[error("Failed to create mount namespace: {0}")] + NamespaceError(String), + #[error("Failed to check /boot writable state: {0}")] + BootCheckError(String), } -fn is_boot_rw_at(mounts_path: &Path) -> Result { - let mounts = fs::read_to_string(mounts_path).map_err(|_| MountError::MountInfoError)?; - for line in mounts.lines() { - let parts: Vec<&str> = line.split_whitespace().collect(); - if parts.len() >= 4 && parts.get(1) == Some(&"/boot") { - let options = parts[3]; - return Ok(options.contains("rw") && !options.contains("ro")); - } +/// Check if the process is already in a private mount namespace by comparing +/// the mount namespace of PID 1 with the current process. +#[cfg(not(feature = "test-remount"))] +fn is_in_private_namespace() -> Result { + let pid1_ns = fs::read_link("/proc/1/ns/mnt") + .map_err(|e| MountError::NamespaceError(format!("Failed to read /proc/1/ns/mnt: {e}")))?; + let self_ns = fs::read_link("/proc/self/ns/mnt").map_err(|e| { + MountError::NamespaceError(format!("Failed to read /proc/self/ns/mnt: {e}")) + })?; + Ok(pid1_ns != self_ns) +} + +/// Ensure the process is running inside a private mount namespace. +/// If already in a private namespace (e.g. via systemd PrivateMounts=yes), this is a no-op. +/// Otherwise, calls unshare(CLONE_NEWNS) to create one. +#[cfg(not(feature = "test-remount"))] +pub fn ensure_mount_namespace() -> Result<(), MountError> { + if is_in_private_namespace()? { + info!("Already in a private mount namespace; skipping unshare"); + return Ok(()); } - Err(MountError::MountInfoError) + info!("Not in a private mount namespace; creating one via unshare(CLONE_NEWNS)"); + nix::sched::unshare(nix::sched::CloneFlags::CLONE_NEWNS).map_err(|e| { + MountError::NamespaceError(format!("Failed to create mount namespace: {e}")) + })?; + info!("Mount namespace created successfully"); + Ok(()) } -/// Default helper: check /boot RW state using shared MOUNT_INFO_PATH -pub fn is_boot_rw() -> Result { - is_boot_rw_at(Path::new(MOUNT_INFO_PATH)) +#[cfg(feature = "test-remount")] +pub fn ensure_mount_namespace() -> Result<(), MountError> { + Ok(()) } -#[cfg(not(feature = "test-remount"))] -fn remount_boot_ro_at(mounts_path: &Path) -> Result<(), MountError> { - match is_boot_rw_at(mounts_path)? { - true => { - let output = Command::new("mount") - .arg("-o") - .arg("remount,bind,ro") - .arg("/boot") - .stderr(Stdio::piped()) // Capture stderr for error handling - .output(); - - match output { - Ok(output) => { - if output.status.success() { - Ok(()) - } else { - let error_message = String::from_utf8_lossy(&output.stderr); - warn!("Failed to remount /boot as RO using shell: {error_message}"); - Err(MountError::RemountFailed(error_message.to_string())) - } - } - Err(e) => { - warn!("Failed to execute mount command: {e}"); - Err(MountError::RemountFailed(format!( - "Failed to execute mount: {e}" - ))) - } - } - } - false => { - info!("/boot is already read-only"); - Ok(()) - } +/// Check if a path is writable using POSIX access(2) with W_OK. +/// This correctly detects read-only mounts, unlike metadata mode bits +/// which only reflect inode permissions. Equivalent to shell `test -w`. +fn is_path_writable_at(path: &Path) -> Result { + match nix::unistd::access(path, nix::unistd::AccessFlags::W_OK) { + Ok(()) => Ok(true), + Err(nix::errno::Errno::EACCES | nix::errno::Errno::EROFS) => Ok(false), + Err(e) => Err(MountError::BootCheckError(format!( + "{}: {e}", + path.display() + ))), } } -#[cfg(not(feature = "test-remount"))] -fn remount_boot_rw_at(mounts_path: &Path) -> Result<(), MountError> { - match is_boot_rw_at(mounts_path)? { - false => { - let output = Command::new("mount") - .arg("-o") - .arg("remount,rw") - .arg("/boot") - .stderr(Stdio::piped()) // Capture stderr for error handling - .output(); - - match output { - Ok(output) => { - if output.status.success() { - Ok(()) - } else { - let error_message = String::from_utf8_lossy(&output.stderr); - warn!("Failed to remount /boot as RW using shell: {error_message}"); - Err(MountError::RemountFailed(error_message.to_string())) - } - } - Err(e) => { - warn!("Failed to execute mount command: {e}"); - Err(MountError::RemountFailed(format!( - "Failed to execute mount: {e}" - ))) - } - } - } - true => { - info!("/boot is already read-write"); - Ok(()) - } - } +/// Check if /boot is currently writable. +pub fn is_boot_writable() -> Result { + is_boot_writable_at(Path::new("/boot")) } -/// Default helper: remount /boot RO using shared MOUNT_INFO_PATH -#[cfg(not(feature = "test-remount"))] -pub fn remount_boot_ro() -> Result<(), MountError> { - remount_boot_ro_at(Path::new(MOUNT_INFO_PATH)) +/// Check if a given path is currently writable. Testable variant of `is_boot_writable`. +pub fn is_boot_writable_at(path: &Path) -> Result { + is_path_writable_at(path) } -/// Default helper: remount /boot RW using shared MOUNT_INFO_PATH +/// Remount /boot as read-write. With mount namespaces, there is no need to +/// restore read-only state afterward — the namespace handles cleanup on exit. #[cfg(not(feature = "test-remount"))] pub fn remount_boot_rw() -> Result<(), MountError> { - remount_boot_rw_at(Path::new(MOUNT_INFO_PATH)) -} + if is_boot_writable()? { + info!("/boot is already writable; no remount needed"); + return Ok(()); + } -/// For testing without actually remounting /mount -#[cfg(feature = "test-remount")] -fn remount_boot_rw_at(_mounts_path: &Path) -> Result<(), MountError> { - Ok(()) -} -/// For testing without actually remounting /mount -#[cfg(feature = "test-remount")] -fn remount_boot_ro_at(_mounts_path: &Path) -> Result<(), MountError> { - Ok(()) + info!("Remounting /boot as read-write"); + let output = Command::new("mount") + .arg("-o") + .arg("remount,rw") + .arg("/boot") + .stderr(Stdio::piped()) + .output(); + + match output { + Ok(output) => { + if output.status.success() { + Ok(()) + } else { + let error_message = String::from_utf8_lossy(&output.stderr); + Err(MountError::RemountFailed(error_message.to_string())) + } + } + Err(e) => Err(MountError::RemountFailed(format!( + "Failed to execute mount: {e}" + ))), + } } -/// For testing feature: default helpers no-op #[cfg(feature = "test-remount")] pub fn remount_boot_rw() -> Result<(), MountError> { Ok(()) } -#[cfg(feature = "test-remount")] -pub fn remount_boot_ro() -> Result<(), MountError> { - Ok(()) -} #[cfg(test)] mod test { use super::*; + use std::fs; + use std::os::unix::fs::PermissionsExt; - fn create_mock_file(content: &str) -> std::path::PathBuf { + #[test] + fn test_is_boot_writable_at_on_writable_dir() { let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("mounts"); - std::fs::write(&path, content).unwrap(); - Box::leak(Box::new(dir)); - path + assert!(is_boot_writable_at(dir.path()).unwrap()); } - #[cfg(not(feature = "test-remount"))] #[test] - fn test_remount_boot_ro_when_already_ro() { - let mounts_content = "rootfs / rootfs rw 0 0\n\ - none /boot tmpfs ro 0 0\n"; - let mounts_path = create_mock_file(mounts_content); + fn test_is_boot_writable_at_on_readonly_dir() { + if nix::unistd::geteuid().is_root() { + eprintln!("Skipping: root bypasses permission bits in access(W_OK)"); + return; + } - let result = remount_boot_ro_at(&mounts_path); - assert!(result.is_ok()); - } + let dir = tempfile::tempdir().unwrap(); + let mut perms = fs::metadata(dir.path()).unwrap().permissions(); + perms.set_mode(0o555); + fs::set_permissions(dir.path(), perms).unwrap(); - #[cfg(not(feature = "test-remount"))] - #[test] - fn test_remount_boot_rw_when_already_rw() { - let mounts_content = "rootfs / rootfs rw 0 0\n\ - none /boot tmpfs rw 0 0\n"; - let mounts_path = create_mock_file(mounts_content); + assert!(!is_boot_writable_at(dir.path()).unwrap()); - let result = remount_boot_rw_at(&mounts_path); - assert!(result.is_ok()); + let mut perms = fs::metadata(dir.path()).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(dir.path(), perms).unwrap(); } #[test] - fn test_is_boot_rw_detection() { - // Test RW case - should return true - let rw_path = create_mock_file("device /boot ext4 rw,relatime 0 0"); - assert!(is_boot_rw_at(&rw_path).unwrap()); - - // Test RO case - should return false - let ro_path = create_mock_file("device /boot ext4 ro,relatime 0 0"); - assert!(!is_boot_rw_at(&ro_path).unwrap()); - - // Test missing /boot - should error - let missing_path = create_mock_file("device /other ext4 rw 0 0"); - assert!(is_boot_rw_at(&missing_path).is_err()); - - // Test malformed line - should error - let malformed_path = create_mock_file("incomplete fields"); - assert!(is_boot_rw_at(&malformed_path).is_err()); + fn test_is_boot_writable_at_nonexistent_path() { + let result = is_boot_writable_at(Path::new("/nonexistent/path/boot")); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("/nonexistent/path/boot")); } } diff --git a/src/main.rs b/src/main.rs index 2211915..a581c22 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,12 +4,12 @@ use anyhow::{Context, Result, bail}; use clap::{Parser, Subcommand, ValueEnum}; use config::{Config, File, FileFormat}; use greenboot::detect_os_deployment; +use greenboot::{ensure_mount_namespace, remount_boot_rw}; use greenboot::{ get_boot_counter, get_rollback_trigger, handle_motd, handle_reboot, handle_rollback, run_diagnostics, run_green, run_red, set_boot_counter, set_boot_status, set_rollback_trigger, unset_boot_counter, unset_rollback_trigger, }; -use greenboot::{is_boot_rw, remount_boot_ro, remount_boot_rw}; use std::{process::Command, sync::OnceLock}; /// greenboot config path @@ -128,7 +128,9 @@ fn running_in_container() -> bool { }) } -/// Execute a mutating GRUB operation while ensuring /boot is temporarily remounted RW if needed +/// Execute a mutating GRUB operation, remounting /boot as RW if needed. +/// With mount namespaces, no read-only restore is necessary — the namespace +/// handles cleanup automatically when the process exits. fn with_boot_rw(f: F) -> Result<()> where F: FnOnce() -> Result<()>, @@ -138,29 +140,8 @@ where return f(); } - let was_rw = - is_boot_rw().map_err(|e| anyhow::anyhow!("Failed to check boot mount state: {}", e))?; - - log::info!( - "Initial /boot mount state: {}", - if was_rw { "rw" } else { "ro" } - ); - - if !was_rw { - log::info!("Remounting /boot as rw for operation"); - remount_boot_rw().context("Failed to remount /boot as rw")?; - } else { - log::info!("/boot is already rw; no remount needed"); - } - - let op_result = f(); - - if !was_rw { - log::info!("Restoring /boot mount to ro"); - remount_boot_ro().context("Failed to remount /boot as ro")?; - } - - op_result + remount_boot_rw().context("Failed to remount /boot as rw")?; + f() } /// Check if greenboot-rollback.service successfully ran in the previous boot @@ -398,6 +379,12 @@ fn main() -> Result<()> { .filter_level(cli.log_level.to_log()) .init(); + if !running_in_container() { + ensure_mount_namespace().context("Failed to set up mount namespace")?; + } else { + log::info!("Container environment detected; skipping mount namespace setup"); + } + match cli.command { Commands::HealthCheck => health_check(), Commands::SetRollbackTrigger => { diff --git a/tests/mount_namespace.rs b/tests/mount_namespace.rs new file mode 100644 index 0000000..5b28957 --- /dev/null +++ b/tests/mount_namespace.rs @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: BSD-3-Clause +// +// Integration tests for mount namespace refactoring (issue #120). +// All sensitive system paths are mocked using tempdir — no real /boot, +// /proc, mount, or unshare operations are performed. + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; + +// --------------------------------------------------------------------------- +// Mount namespace setup +// --------------------------------------------------------------------------- + +#[test] +fn ensure_mount_namespace_succeeds() { + let result = greenboot::ensure_mount_namespace(); + assert!(result.is_ok(), "ensure_mount_namespace should succeed"); +} + +// --------------------------------------------------------------------------- +// Boot writable check +// --------------------------------------------------------------------------- + +#[test] +fn is_boot_writable_at_detects_writable_path() { + let mock_boot = tempfile::tempdir().unwrap(); + let result = greenboot::is_boot_writable_at(mock_boot.path()); + assert!(result.is_ok()); + assert!(result.unwrap(), "Writable mock /boot should return true"); +} + +#[test] +fn is_boot_writable_at_detects_readonly_path() { + if nix::unistd::geteuid().is_root() { + eprintln!("Skipping: root bypasses permission bits in access(W_OK)"); + return; + } + + let mock_boot = tempfile::tempdir().unwrap(); + let mut perms = fs::metadata(mock_boot.path()).unwrap().permissions(); + perms.set_mode(0o555); + fs::set_permissions(mock_boot.path(), perms).unwrap(); + + let result = greenboot::is_boot_writable_at(mock_boot.path()); + assert!(result.is_ok()); + assert!(!result.unwrap(), "Read-only mock /boot should return false"); + + let mut perms = fs::metadata(mock_boot.path()).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(mock_boot.path(), perms).unwrap(); +} + +#[test] +fn is_boot_writable_at_errors_on_missing_path() { + let result = greenboot::is_boot_writable_at(Path::new("/nonexistent/mock/boot")); + assert!(result.is_err(), "Missing path should return error"); +} + +// --------------------------------------------------------------------------- +// Boot remount in namespace +// --------------------------------------------------------------------------- + +#[test] +fn remount_boot_rw_succeeds() { + let result = greenboot::remount_boot_rw(); + assert!(result.is_ok(), "remount_boot_rw should succeed"); +} + +// --------------------------------------------------------------------------- +// Public API surface — compile-time + runtime validation +// --------------------------------------------------------------------------- + +#[test] +fn public_api_surface() { + let _: Result<(), greenboot::MountError> = greenboot::ensure_mount_namespace(); + let _: Result<(), greenboot::MountError> = greenboot::remount_boot_rw(); + + let _fn_ref: fn() -> Result = greenboot::is_boot_writable; + + let _: greenboot::MountError = greenboot::MountError::RemountFailed("test".to_string()); + let _: greenboot::MountError = greenboot::MountError::NamespaceError("test".to_string()); + let _: greenboot::MountError = greenboot::MountError::BootCheckError("test".to_string()); +} diff --git a/usr/lib/systemd/system/greenboot-set-rollback-trigger.service b/usr/lib/systemd/system/greenboot-set-rollback-trigger.service index 0fe87b5..aad9471 100644 --- a/usr/lib/systemd/system/greenboot-set-rollback-trigger.service +++ b/usr/lib/systemd/system/greenboot-set-rollback-trigger.service @@ -7,11 +7,13 @@ Before=sysinit.target shutdown.target Conflicts=shutdown.target ConditionNeedsUpdate=|/etc ConditionNeedsUpdate=|/var +RequiresMountsFor=/boot [Service] Type=oneshot Restart=no ExecStart=/usr/libexec/greenboot/greenboot set-rollback-trigger +PrivateMounts=yes [Install] WantedBy=systemd-update-done.service greenboot-healthcheck.service \ No newline at end of file