treefile: Add ignore-devices

We hit another case where people are pulling a container image with devices in `/dev` in the tar stream; they're then trying to commit this to an ostree. There's much better ways to fix this: - Change the image to stop including devices as there's no reason to do so - Switch to logically bound images instead of physically bound - Use the composefs backend for c/storage Eventually I may look at "quoting" generally in ostree, but it's fairly invasive: ostreedev/ostree#2568 In practice today, simply ignoring the files will happen to work for "podman run" of such images; podman will just use overlayfs to stitch together the `diff` directories, and doesn't try to do any validation of their contents today. (Queue the composefs integration, which *would* do that but would also fix this anwyays) Signed-off-by: Colin Walters <[email protected]>
cgwalters · Oct 14, 2024 · 97fe71c · 97fe71c
1 parent b8667b9
commit 97fe71c
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 29 deletions.
diff --git a/docs/treefile.md b/docs/treefile.md
@@ -40,6 +40,10 @@ It supports the following parameters:
  * `selinux`: boolean, optional: Defaults to `true`.  If `false`, then
    no SELinux labeling will be performed on the server side.
 
+ * `ignore-devices`: boolean, optional: Defaults to `true`. If `true`, then
+   all character and block device files found in the target root (except overlayfs
+   whiteouts, which are automatically "quoted") will be ignored.
+
  * `ima`: boolean, optional: Defaults to `false`.  Propagate any
    IMA signatures in input RPMs into the final OSTree commit.
 

diff --git a/rpmostree-cxxrs.cxx b/rpmostree-cxxrs.cxx
@@ -192,6 +192,8 @@ class Slice final : private detail::copy_assignable_if<std::is_const<T>::value>
   Slice () noexcept;
   Slice (T *, std::size_t count) noexcept;
 
+  template <typename C> explicit Slice (C &c) : Slice (c.data (), c.size ()) {}
+
   Slice &operator= (const Slice<T> &) &noexcept = default;
   Slice &operator= (Slice<T> &&) &noexcept = default;
 
@@ -2206,8 +2208,8 @@ extern "C"
       ::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile &treefile, ::rust::Str next_version,
       bool unified_core) noexcept;
 
-  ::rust::repr::PtrLen
-  rpmostreecxx$cxxbridge1$compose_postprocess_final_pre (::std::int32_t rootfs_dfd) noexcept;
+  ::rust::repr::PtrLen rpmostreecxx$cxxbridge1$compose_postprocess_final_pre (
+      ::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile const &treefile) noexcept;
 
   ::rust::repr::PtrLen rpmostreecxx$cxxbridge1$compose_postprocess_final (
       ::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile const &treefile) noexcept;
@@ -4011,9 +4013,10 @@ compose_postprocess (::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile &treefi
 }
 
 void
-compose_postprocess_final_pre (::std::int32_t rootfs_dfd)
+compose_postprocess_final_pre (::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile const &treefile)
 {
-  ::rust::repr::PtrLen error$ = rpmostreecxx$cxxbridge1$compose_postprocess_final_pre (rootfs_dfd);
+  ::rust::repr::PtrLen error$
+      = rpmostreecxx$cxxbridge1$compose_postprocess_final_pre (rootfs_dfd, treefile);
   if (error$.ptr)
     {
       throw ::rust::impl< ::rust::Error>::error (error$);

diff --git a/rpmostree-cxxrs.h b/rpmostree-cxxrs.h
@@ -191,6 +191,8 @@ class Slice final : private detail::copy_assignable_if<std::is_const<T>::value>
   Slice () noexcept;
   Slice (T *, std::size_t count) noexcept;
 
+  template <typename C> explicit Slice (C &c) : Slice (c.data (), c.size ()) {}
+
   Slice &operator= (const Slice<T> &) &noexcept = default;
   Slice &operator= (Slice<T> &&) &noexcept = default;
 
@@ -1867,7 +1869,8 @@ void composepost_nsswitch_altfiles (::std::int32_t rootfs_dfd);
 void compose_postprocess (::std::int32_t rootfs_dfd, ::rpmostreecxx::Treefile &treefile,
                           ::rust::Str next_version, bool unified_core);
 
-void compose_postprocess_final_pre (::std::int32_t rootfs_dfd);
+void compose_postprocess_final_pre (::std::int32_t rootfs_dfd,
+                                    ::rpmostreecxx::Treefile const &treefile);
 
 void compose_postprocess_final (::std::int32_t rootfs_dfd,
                                 ::rpmostreecxx::Treefile const &treefile);

diff --git a/rust/src/composepost.rs b/rust/src/composepost.rs
@@ -295,40 +295,69 @@ fn is_overlay_whiteout(meta: &cap_std::fs::Metadata) -> bool {
     (meta.mode() & libc::S_IFMT) == libc::S_IFCHR && meta.rdev() == 0
 }
 
-/// Auto-synthesize embedded overlayfs whiteouts; for more information
-/// see https://github.com/ostreedev/ostree/pull/2722/commits/0085494e350c72599fc5c0e00422885d80b3c660
-#[context("Postprocessing embedded overlayfs")]
-fn postprocess_embedded_ovl_whiteouts(root: &Dir) -> Result<()> {
+/// Automatically "quote" embeded overlayfs whiteouts as regular files, and
+/// if configured error out on devices or ignore them.
+/// For more on overlayfs, see https://github.com/ostreedev/ostree/pull/2722/commits/0085494e350c72599fc5c0e00422885d80b3c660
+#[context("Postprocessing devices")]
+fn postprocess_devices(root: &Dir, treefile: &Treefile) -> Result<()> {
     const OSTREE_WHITEOUT_PREFIX: &str = ".ostree-wh.";
-    fn recurse(root: &Dir, path: &Utf8Path) -> Result<u32> {
-        let mut n = 0;
+    let mut n_overlay = 0u64;
+    let mut n_devices = 0u64;
+    fn recurse(
+        root: &Dir,
+        path: &Utf8Path,
+        ignore_devices: bool,
+        n_overlay: &mut u64,
+        n_devices: &mut u64,
+    ) -> Result<()> {
         for entry in root.read_dir(path)? {
             let entry = entry?;
             let meta = entry.metadata()?;
             let name = PathBuf::from(entry.file_name());
             let name: Utf8PathBuf = name.try_into()?;
             if meta.is_dir() {
-                n += recurse(root, &path.join(name))?;
+                recurse(root, &path.join(name), ignore_devices, n_overlay, n_devices)?;
                 continue;
             }
-            if !is_overlay_whiteout(&meta) {
+            let is_device = matches!(meta.mode() & libc::S_IFMT, libc::S_IFCHR | libc::S_IFBLK);
+            if !is_device {
                 continue;
-            };
+            }
             let srcpath = path.join(&name);
-            let targetname = format!("{OSTREE_WHITEOUT_PREFIX}{name}");
-            let destpath = path.join(&targetname);
-            root.remove_file(srcpath)?;
-            root.atomic_write_with_perms(destpath, "", meta.permissions())?;
-            n += 1;
+            if is_overlay_whiteout(&meta) {
+                let targetname = format!("{OSTREE_WHITEOUT_PREFIX}{name}");
+                let destpath = path.join(&targetname);
+                root.remove_file(srcpath)?;
+                root.atomic_write_with_perms(destpath, "", meta.permissions())?;
+                *n_overlay += 1;
+                continue;
+            }
+            if ignore_devices {
+                root.remove_file(srcpath)?;
+                *n_devices += 1;
+            } else {
+                anyhow::bail!("Unsupported device file: {srcpath}")
+            }
         }
-        Ok(n)
+        Ok(())
     }
-    let n = recurse(root, ".".into())?;
-    if n > 0 {
-        println!("Processed {n} embedded whiteouts");
+    recurse(
+        root,
+        ".".into(),
+        treefile.get_ignore_devices(),
+        &mut n_overlay,
+        &mut n_devices,
+    )?;
+    if n_overlay > 0 {
+        println!("Processed {n_overlay} embedded whiteouts");
     } else {
         println!("No embedded whiteouts found");
     }
+    if n_devices > 0 {
+        println!("Ignored {n_devices} device files");
+    } else {
+        println!("No device files found");
+    }
     Ok(())
 }
 
@@ -420,7 +449,7 @@ pub(crate) fn postprocess_cleanup_rpmdb(rootfs_dfd: i32) -> CxxResult<()> {
 /// it as the bits of that function that we've chosen to implement in Rust.
 /// It takes care of all things that are really required to use rpm-ostree
 /// on the target host.
-pub fn compose_postprocess_final_pre(rootfs_dfd: i32) -> CxxResult<()> {
+pub fn compose_postprocess_final_pre(rootfs_dfd: i32, treefile: &Treefile) -> CxxResult<()> {
     let rootfs_dfd = unsafe { &crate::ffiutil::ffi_dirfd(rootfs_dfd)? };
     // These tasks can safely run in parallel, so just for fun we do so via rayon.
     let tasks = [
@@ -430,7 +459,7 @@ pub fn compose_postprocess_final_pre(rootfs_dfd: i32) -> CxxResult<()> {
     ];
     tasks.par_iter().try_for_each(|f| f(rootfs_dfd))?;
     // This task recursively traverses the filesystem and hence should be serial.
-    postprocess_embedded_ovl_whiteouts(rootfs_dfd)?;
+    postprocess_devices(rootfs_dfd, treefile)?;
     Ok(())
 }
 
@@ -1536,11 +1565,12 @@ OSTREE_VERSION='33.4'
         // We don't actually test creating whiteout devices here as that
         // may not work.
         let td = cap_tempfile::tempdir(cap_std::ambient_authority())?;
+        let tf = crate::treefile::tests::new_test_tf_basic("")?;
         // Verify no-op case
-        postprocess_embedded_ovl_whiteouts(&td).unwrap();
+        postprocess_devices(&td, &tf).unwrap();
         td.create("foo")?;
         td.symlink("foo", "bar")?;
-        postprocess_embedded_ovl_whiteouts(&td).unwrap();
+        postprocess_devices(&td, &tf).unwrap();
         assert!(td.try_exists("foo")?);
         assert!(td.try_exists("bar")?);
 

diff --git a/rust/src/lib.rs b/rust/src/lib.rs
@@ -299,7 +299,7 @@ pub mod ffi {
             next_version: &str,
             unified_core: bool,
         ) -> Result<()>;
-        fn compose_postprocess_final_pre(rootfs_dfd: i32) -> Result<()>;
+        fn compose_postprocess_final_pre(rootfs_dfd: i32, treefile: &Treefile) -> Result<()>;
         fn compose_postprocess_final(rootfs_dfd: i32, treefile: &Treefile) -> Result<()>;
         fn convert_var_to_tmpfiles_d(rootfs_dfd: i32, cancellable: &GCancellable) -> Result<()>;
         fn rootfs_prepare_links(

diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs
@@ -450,6 +450,7 @@ fn treefile_merge(dest: &mut TreeComposeConfig, src: &mut TreeComposeConfig) {
         rojig,
         selinux,
         selinux_label_version,
+        ignore_devices,
         ima,
         gpg_key,
         include,
@@ -1320,6 +1321,10 @@ impl Treefile {
         self.parsed.base.selinux.unwrap_or(true)
     }
 
+    pub(crate) fn get_ignore_devices(&self) -> bool {
+        self.parsed.base.ignore_devices.unwrap_or_default()
+    }
+
     pub(crate) fn get_selinux_label_version(&self) -> u32 {
         self.parsed.base.selinux_label_version.unwrap_or_default()
     }
@@ -2505,6 +2510,8 @@ pub(crate) struct BaseComposeConfigFields {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub(crate) selinux: Option<bool>,
     #[serde(skip_serializing_if = "Option::is_none")]
+    pub(crate) ignore_devices: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub(crate) selinux_label_version: Option<u32>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub(crate) ima: Option<bool>,

diff --git a/src/libpriv/rpmostree-postprocess.cxx b/src/libpriv/rpmostree-postprocess.cxx
@@ -381,7 +381,7 @@ postprocess_final (int rootfs_dfd, rpmostreecxx::Treefile &treefile, gboolean un
 
   auto selinux = treefile.get_selinux ();
 
-  ROSCXX_TRY (compose_postprocess_final_pre (rootfs_dfd), error);
+  ROSCXX_TRY (compose_postprocess_final_pre (rootfs_dfd, treefile), error);
 
   if (selinux)
     {

diff --git a/tests/compose/test-installroot.sh b/tests/compose/test-installroot.sh
@@ -7,6 +7,8 @@ dn=$(cd "$(dirname "$0")" && pwd)
 
 # This is used to test postprocessing with treefile vs not
 treefile_set "boot-location" '"new"'
+# On by default now:
+# treefile_set "ignore-devices" 'True'
 
 # This test is a bit of a degenerative case of the supermin abstration. We need
 # to be able to interact with the compose output directly, feed it back to
@@ -56,6 +58,7 @@ testdate=$(date)
 runasroot sh -xec "
 # https://github.com/ostreedev/ostree/pull/2717/commits/e234b630f85b97e48ecf45d5aaba9b1aa64e6b54
 mknod -m 000 ${instroot}-directcommit/usr/share/foowhiteout c 0 0
+mknod -m 000 ${instroot}-directcommit/usr/share/devzero c 1 5
 echo \"${testdate}\" > ${instroot}-directcommit/usr/share/rpm-ostree-composetest-split.txt
 ! test -f ${instroot}-directcommit/${integrationconf}
 rpm-ostree compose commit --repo=${repo} ${treefile} ${instroot}-directcommit
@@ -69,6 +72,11 @@ ostree --repo=${repo} ls ${treeref} /usr/share
 ostree --repo=${repo} ls ${treeref} /usr/share/.ostree-wh.foowhiteout >out.txt
 grep -Ee '^-00000' out.txt
 
+# And the devzero should have been ignored
+if ostree --repo=${repo} ls ${treeref} /usr/share/devzero; then
+  echo "found devzero" 1>&2; exit 1
+fi
+
 ostree --repo=${repo} cat ${treeref} /usr/share/rpm-ostree-composetest-split.txt >out.txt
 grep \"${testdate}\" out.txt
 ostree --repo=${repo} cat ${treeref} /${integrationconf}