From f4ad3d03fbb0e14c0ecba06ac221e524dd9fb083 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 10 Nov 2025 18:36:31 -0500 Subject: [PATCH] store: Preserve /sysroot readonly for read-only operations PR #1718 introduced a regression where /sysroot was left writable after running `bootc status`. This occurred because BootedStorage::new() unconditionally calls set_mount_namespace_in_use(), which tells ostree it can safely remount /sysroot as writable. When sysroot.load() is called without actually being in a mount namespace, it leaves the global /sysroot writable. Fix by introducing an Environment enum that detects the runtime environment (ostree, composefs, container, or other) early in the execution flow. Callers now detect the environment and call prepare_for_write() if needed before constructing BootedStorage. This ensures a single call to prepare_for_write() per execution path and eliminates the previous layering violation where storage code called into CLI code. The Environment abstraction also makes it clearer when mount namespace setup is required and provides a foundation for future environment-specific behavior. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters --- crates/lib/src/bootc_composefs/status.rs | 1 + crates/lib/src/cli.rs | 5 +- crates/lib/src/status.rs | 7 +- crates/lib/src/store/mod.rs | 121 +++++++++++++------ tmt/tests/booted/readonly/001-test-status.nu | 27 ++++- 5 files changed, 115 insertions(+), 46 deletions(-) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index 04624243b..29ae212ba 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -36,6 +36,7 @@ use crate::composefs_consts::{ use crate::spec::Bootloader; /// A parsed composefs command line +#[derive(Clone)] pub(crate) struct ComposefsCmdline { #[allow(dead_code)] pub insecure: bool, diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index 9331db466..ad2989066 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -721,8 +721,11 @@ pub(crate) fn ensure_self_unshared_mount_namespace() -> Result<()> { /// This prepares the process for write operations (re-exec, mount namespace, etc). #[context("Initializing storage")] pub(crate) async fn get_storage() -> Result { + let env = crate::store::Environment::detect()?; + // Always call prepare_for_write() for write operations - it checks + // for container, root privileges, mount namespace setup, etc. prepare_for_write()?; - let r = BootedStorage::new() + let r = BootedStorage::new(env) .await? .ok_or_else(|| anyhow!("System not booted via bootc"))?; Ok(r) diff --git a/crates/lib/src/status.rs b/crates/lib/src/status.rs index 55e7146b3..e8b4192a2 100644 --- a/crates/lib/src/status.rs +++ b/crates/lib/src/status.rs @@ -369,7 +369,12 @@ pub(crate) fn get_status( } async fn get_host() -> Result { - let Some(storage) = BootedStorage::new().await? else { + let env = crate::store::Environment::detect()?; + if env.needs_mount_namespace() { + crate::cli::prepare_for_write()?; + } + + let Some(storage) = BootedStorage::new(env).await? else { // If we're not booted, then return a default. return Ok(Host::default()); }; diff --git a/crates/lib/src/store/mod.rs b/crates/lib/src/store/mod.rs index 38a91f585..c0ea340a5 100644 --- a/crates/lib/src/store/mod.rs +++ b/crates/lib/src/store/mod.rs @@ -97,6 +97,43 @@ pub struct BootedComposefs { /// Discriminated union representing the boot storage backend. /// +/// The runtime environment in which bootc is executing. +pub(crate) enum Environment { + /// System booted via ostree + OstreeBooted, + /// System booted via composefs + ComposefsBooted(ComposefsCmdline), + /// Running in a container + Container, + /// Other (not booted via bootc) + Other, +} + +impl Environment { + /// Detect the current runtime environment. + pub(crate) fn detect() -> Result { + if ostree_ext::container_utils::running_in_container() { + return Ok(Self::Container); + } + + if let Some(cmdline) = composefs_booted()? { + return Ok(Self::ComposefsBooted(cmdline.clone())); + } + + if ostree_booted()? { + return Ok(Self::OstreeBooted); + } + + Ok(Self::Other) + } + + /// Returns true if this environment requires entering a mount namespace + /// before loading storage (to avoid leaving /sysroot writable). + pub(crate) fn needs_mount_namespace(&self) -> bool { + matches!(self, Self::OstreeBooted | Self::ComposefsBooted(_)) + } +} + /// A system can boot via either ostree or composefs; this enum /// allows code to handle both cases while maintaining type safety. pub(crate) enum BootedStorageKind<'a> { @@ -105,54 +142,58 @@ pub(crate) enum BootedStorageKind<'a> { } impl BootedStorage { - /// Create a new booted storage accessor. - /// - /// This detects whether the system is booted via composefs or ostree - /// and initializes the appropriate storage backend. + /// Create a new booted storage accessor for the given environment. /// - /// Note: For write operations, callers should call `prepare_for_write()` - /// before calling this function to ensure the process is in the correct - /// mount namespace. - pub(crate) async fn new() -> Result> { + /// The caller must have already called `prepare_for_write()` if + /// `env.needs_mount_namespace()` is true. + pub(crate) async fn new(env: Environment) -> Result> { let physical_root = Dir::open_ambient_dir("/sysroot", cap_std::ambient_authority()) .context("Opening /sysroot")?; let run = Dir::open_ambient_dir("/run", cap_std::ambient_authority()).context("Opening /run")?; - let r = if let Some(cmdline) = composefs_booted()? { - let mut composefs = ComposefsRepository::open_path(&physical_root, COMPOSEFS)?; - if cmdline.insecure { - composefs.set_insecure(true); + let r = match env { + Environment::ComposefsBooted(cmdline) => { + let mut composefs = ComposefsRepository::open_path(&physical_root, COMPOSEFS)?; + if cmdline.insecure { + composefs.set_insecure(true); + } + let composefs = Arc::new(composefs); + + let storage = Storage { + physical_root, + run, + ostree: Default::default(), + composefs: OnceCell::from(composefs), + imgstore: Default::default(), + }; + + Some(Self { storage }) } - let composefs = Arc::new(composefs); - - let storage = Storage { - physical_root, - run, - ostree: Default::default(), - composefs: OnceCell::from(composefs), - imgstore: Default::default(), - }; - - Some(Self { storage }) - } else if ostree_booted()? { - let sysroot = ostree::Sysroot::new_default(); - sysroot.set_mount_namespace_in_use(); - let sysroot = ostree_ext::sysroot::SysrootLock::new_from_sysroot(&sysroot).await?; - sysroot.load(gio::Cancellable::NONE)?; - - let storage = Storage { - physical_root, - run, - ostree: OnceCell::from(sysroot), - composefs: Default::default(), - imgstore: Default::default(), - }; - - Some(Self { storage }) - } else { - None + Environment::OstreeBooted => { + // The caller must have entered a private mount namespace before + // calling this function. This is because ostree's sysroot.load() will + // remount /sysroot as writable, and we call set_mount_namespace_in_use() + // to indicate we're in a mount namespace. Without actually being in a + // mount namespace, this would leave the global /sysroot writable. + + let sysroot = ostree::Sysroot::new_default(); + sysroot.set_mount_namespace_in_use(); + let sysroot = ostree_ext::sysroot::SysrootLock::new_from_sysroot(&sysroot).await?; + sysroot.load(gio::Cancellable::NONE)?; + + let storage = Storage { + physical_root, + run, + ostree: OnceCell::from(sysroot), + composefs: Default::default(), + imgstore: Default::default(), + }; + + Some(Self { storage }) + } + Environment::Container | Environment::Other => None, }; Ok(r) } diff --git a/tmt/tests/booted/readonly/001-test-status.nu b/tmt/tests/booted/readonly/001-test-status.nu index cabb4b77d..a4a119ae7 100644 --- a/tmt/tests/booted/readonly/001-test-status.nu +++ b/tmt/tests/booted/readonly/001-test-status.nu @@ -3,6 +3,22 @@ use tap.nu tap begin "verify bootc status output formats" +let st = bootc status --json | from json +# Detect composefs by checking if composefs field is present +let is_composefs = ($st.status.booted.composefs? != null) + +# FIXME: Should be mounting /sysroot readonly in composefs by default +if not $is_composefs { + # Verify /sysroot is not writable initially (read-only operations should not make it writable) + let is_writable = (do -i { /bin/test -w /sysroot } | complete | get exit_code) == 0 + assert (not $is_writable) "/sysroot should not be writable initially" + + # Double-check with findmnt + let mnt = (findmnt /sysroot -J | from json) + let opts = ($mnt.filesystems.0.options | split row ",") + assert ($opts | any { |o| $o == "ro" }) "/sysroot should be mounted read-only" +} + let st = bootc status --json | from json assert equal $st.apiVersion org.containers.bootc/v1 @@ -11,8 +27,6 @@ assert equal $st.apiVersion org.containers.bootc/v1 let st = bootc status --format=yaml | from yaml assert equal $st.apiVersion org.containers.bootc/v1 -# Detect composefs by checking if composefs field is present -let is_composefs = ($st.status.booted.composefs? != null) if not $is_composefs { assert ($st.status.booted.image.timestamp != null) } # else { TODO composefs: timestamp is not populated with composefs } @@ -23,12 +37,17 @@ if $ostree != null { let st = bootc status --json --booted | from json assert equal $st.apiVersion org.containers.bootc/v1 -# Detect composefs by checking if composefs field is present -let is_composefs = ($st.status.booted.composefs? != null) if not $is_composefs { assert ($st.status.booted.image.timestamp != null) } # else { TODO composefs: timestamp is not populated with composefs } assert (($st.status | get rollback | default null) == null) assert (($st.status | get staged | default null) == null) +# FIXME: See above re /sysroot ro +if not $is_composefs { + # Verify /sysroot is still not writable after bootc status (regression test for PR #1718) + let is_writable = (do -i { /bin/test -w /sysroot } | complete | get exit_code) == 0 + assert (not $is_writable) "/sysroot should remain read-only after bootc status" +} + tap ok