Merge pull request #1032 from cgwalters/export-xattrs

cgwalters · web-flow · commit 2aed31d3efe1 · 2025-01-16T18:46:44.000-05:00
ostree-ext: Serialize xattrs into tar stream as well
diff --git a/ostree-ext/src/fixture.rs b/ostree-ext/src/fixture.rs
@@ -8,8 +8,10 @@ use crate::container::{Config, ExportOpts, ImageReference, Transport};
 use crate::objectsource::{ObjectMeta, ObjectSourceMeta};
 use crate::objgv::gv_dirtree;
 use crate::prelude::*;
+use crate::tar::SECURITY_SELINUX_XATTR_C;
 use crate::{gio, glib};
 use anyhow::{anyhow, Context, Result};
+use bootc_utils::CommandRunExt;
 use camino::{Utf8Component, Utf8Path, Utf8PathBuf};
 use cap_std::fs::Dir;
 use cap_std_ext::cap_std;
@@ -25,6 +27,7 @@ use ocidir::oci_spec::image::ImageConfigurationBuilder;
 use once_cell::sync::Lazy;
 use regex::Regex;
 use std::borrow::Cow;
+use std::ffi::CString;
 use std::fmt::Write as _;
 use std::io::{self, Write};
 use std::ops::Add;
@@ -46,12 +49,19 @@ enum FileDefType {
     Directory,
 }
 
+#[derive(Debug)]
+struct Xattr {
+    key: CString,
+    value: Box<[u8]>,
+}
+
 #[derive(Debug)]
 pub struct FileDef {
     uid: u32,
     gid: u32,
     mode: u32,
     path: Cow<'static, Utf8Path>,
+    xattrs: Box<[Xattr]>,
     ty: FileDefType,
 }
 
@@ -66,9 +76,21 @@ impl TryFrom<&'static str> for FileDef {
         let name = parts.next().ok_or_else(|| anyhow!("Missing file name"))?;
         let contents = parts.next();
         let contents = move || contents.ok_or_else(|| anyhow!("Missing file contents: {}", value));
-        if parts.next().is_some() {
-            anyhow::bail!("Invalid filedef: {}", value);
-        }
+        let xattrs: Result<Vec<_>> = parts
+            .map(|xattr| -> Result<Xattr> {
+                let (k, v) = xattr
+                    .split_once('=')
+                    .ok_or_else(|| anyhow::anyhow!("Invalid xattr: {xattr}"))?;
+                let mut k: Vec<u8> = k.to_owned().into();
+                k.push(0);
+                let r = Xattr {
+                    key: CString::from_vec_with_nul(k).unwrap(),
+                    value: Vec::from(v.to_owned()).into(),
+                };
+                Ok(r)
+            })
+            .collect();
+        let xattrs = xattrs?.into();
         let ty = match tydef {
             "r" => FileDefType::Regular(contents()?.into()),
             "l" => FileDefType::Symlink(Cow::Borrowed(contents()?.into())),
@@ -80,6 +102,7 @@ impl TryFrom<&'static str> for FileDef {
             gid: 0,
             mode: 0o644,
             path: Cow::Borrowed(name.into()),
+            xattrs,
             ty,
         })
     }
@@ -165,6 +188,7 @@ static OWNERS: Lazy<Vec<(Regex, &str)>> = Lazy::new(|| {
         ("usr/lib/modules/.*/initramfs", "initramfs"),
         ("usr/lib/modules", "kernel"),
         ("usr/bin/(ba)?sh", "bash"),
+        ("usr/bin/arping", "arping"),
         ("usr/lib.*/emptyfile.*", "bash"),
         ("usr/bin/hardlink.*", "testlink"),
         ("usr/etc/someconfig.conf", "someconfig"),
@@ -184,6 +208,7 @@ r usr/lib/modules/5.10.18-200.x86_64/initramfs this-is-an-initramfs
 m 0 0 755
 r usr/bin/bash the-bash-shell
 l usr/bin/sh bash
+r usr/bin/arping arping-binary security.capability=0sAAAAAgAgAAAAAAAAAAAAAAAAAAA=
 m 0 0 644
 # Some empty files
 r usr/lib/emptyfile 
@@ -206,7 +231,7 @@ m 0 0 1755
 d tmp
 "## };
 pub const CONTENTS_CHECKSUM_V0: &str =
-    "acc42fb5c796033f034941dc688643bf8beddfd9068d87165344d2b99906220a";
+    "bd3d13c3059e63e6f8a3d6d046923ded730d90bd7a055c9ad93312111ea7d395";
 // 1 for ostree commit, 2 for max frequency packages, 3 as empty layer
 pub const LAYERS_V0_LEN: usize = 3usize;
 pub const PKGS_V0_LEN: usize = 7usize;
@@ -267,11 +292,10 @@ impl SeLabel {
     }
 
     pub fn xattrs(&self) -> Vec<(&[u8], &[u8])> {
-        vec![(b"security.selinux\0", self.to_str().as_bytes())]
-    }
-
-    pub fn new_xattrs(&self) -> glib::Variant {
-        self.xattrs().to_variant()
+        vec![(
+            SECURITY_SELINUX_XATTR_C.to_bytes_with_nul(),
+            self.to_str().as_bytes(),
+        )]
     }
 }
 
@@ -286,7 +310,7 @@ pub fn create_dirmeta(path: &Utf8Path, selinux: bool) -> glib::Variant {
     } else {
         None
     };
-    let xattrs = label.map(|v| v.new_xattrs());
+    let xattrs = label.map(|v| v.xattrs().to_variant());
     ostree::create_directory_metadata(&finfo, xattrs.as_ref())
 }
 
@@ -632,7 +656,18 @@ impl Fixture {
         } else {
             None
         };
-        let xattrs = label.map(|v| v.new_xattrs());
+        let mut xattrs = label.as_ref().map(|v| v.xattrs()).unwrap_or_default();
+        xattrs.extend(
+            def.xattrs
+                .iter()
+                .map(|xattr| (xattr.key.as_bytes_with_nul(), &xattr.value[..])),
+        );
+        let xattrs = if xattrs.is_empty() {
+            None
+        } else {
+            xattrs.sort_by(|a, b| a.0.cmp(b.0));
+            Some(xattrs.to_variant())
+        };
         let xattrs = xattrs.as_ref();
         let checksum = match &def.ty {
             FileDefType::Regular(contents) => self
@@ -724,6 +759,21 @@ impl Fixture {
             gio::Cancellable::NONE,
         )?;
 
+        // Verify that this is what is expected.
+        let commit_object = self.srcrepo.load_commit(&commit)?.0;
+        let content_checksum = ostree::commit_get_content_checksum(&commit_object).unwrap();
+        if content_checksum != CONTENTS_CHECKSUM_V0 {
+            // Only spew this once
+            static DUMP_OSTREE: std::sync::Once = std::sync::Once::new();
+            DUMP_OSTREE.call_once(|| {
+                let _ = Command::new("ostree")
+                    .arg(format!("--repo={}", self.path.join("src/repo")))
+                    .args(["ls", "-X", "-C", "-R", commit.as_str()])
+                    .run();
+            });
+        }
+        assert_eq!(CONTENTS_CHECKSUM_V0, content_checksum.as_str());
+
         Ok(())
     }
 
diff --git a/ostree-ext/src/tar/export.rs b/ostree-ext/src/tar/export.rs
@@ -13,11 +13,23 @@ use ostree::gio;
 use std::borrow::Borrow;
 use std::borrow::Cow;
 use std::collections::HashSet;
+use std::ffi::CStr;
 use std::io::BufReader;
 
 /// The repository mode generated by a tar export stream.
 pub const BARE_SPLIT_XATTRS_MODE: &str = "bare-split-xattrs";
 
+/// The SELinux xattr. Because the ostree xattrs require an embedded NUL, we
+/// store that version as a constant.
+pub(crate) const SECURITY_SELINUX_XATTR_C: &CStr = c"security.selinux";
+/// Then derive a string version (without the NUL) from the above.
+pub(crate) const SECURITY_SELINUX_XATTR: &str = const {
+    match SECURITY_SELINUX_XATTR_C.to_str() {
+        Ok(r) => r,
+        Err(_) => unreachable!(),
+    }
+};
+
 // This is both special in the tar stream *and* it's in the ostree commit.
 const SYSROOT: &str = "sysroot";
 // This way the default ostree -> sysroot/ostree symlink works.
@@ -347,9 +359,13 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
         Ok(())
     }
 
-    /// Export xattrs to the tar stream, return whether content was written.
+    /// Export xattrs in ostree-container style format, which is a .xattrs file.
+    /// This is different from xattrs which may appear as e.g. PAX metadata, which we don't use
+    /// at the moment.
+    ///
+    /// Return whether content was written.
     #[context("Writing xattrs")]
-    fn append_xattrs(&mut self, checksum: &str, xattrs: &glib::Variant) -> Result<bool> {
+    fn append_ostree_xattrs(&mut self, checksum: &str, xattrs: &glib::Variant) -> Result<bool> {
         let xattrs_data = xattrs.data_as_bytes();
         let xattrs_data = xattrs_data.as_ref();
 
@@ -375,6 +391,32 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
         Ok(true)
     }
 
+    /// Append all xattrs to the tar stream *except* security.selinux, because
+    /// that one doesn't become visible in `podman run` anyways, so we couldn't
+    /// rely on it in some cases.
+    /// https://github.com/containers/storage/blob/0d4a8d2aaf293c9f0464b888d932ab5147a284b9/pkg/archive/archive.go#L85
+    #[context("Writing tar xattrs")]
+    fn append_tarstream_xattrs(&mut self, xattrs: &glib::Variant) -> Result<()> {
+        let v = xattrs.data_as_bytes();
+        let v = v.try_as_aligned().unwrap();
+        let v = gvariant::gv!("a(ayay)").cast(v);
+        let mut pax_extensions = Vec::new();
+        for entry in v {
+            let (k, v) = entry.to_tuple();
+            let k = CStr::from_bytes_with_nul(k).unwrap();
+            let k = k
+                .to_str()
+                .with_context(|| format!("Found non-UTF8 xattr: {k:?}"))?;
+            if k == SECURITY_SELINUX_XATTR {
+                continue;
+            }
+            pax_extensions.push((format!("SCHILY.xattr.{k}"), v));
+        }
+        self.out
+            .append_pax_extensions(pax_extensions.iter().map(|(k, v)| (k.as_str(), *v)))?;
+        Ok(())
+    }
+
     /// Write a content object, returning the path/header that should be used
     /// as a hard link to it in the target path. This matches how ostree checkouts work.
     fn append_content(&mut self, checksum: &str) -> Result<(Utf8PathBuf, tar::Header)> {
@@ -397,7 +439,8 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             // The xattrs objects need to be exported before the regular object they
             // refer to. Otherwise the importing logic won't have the xattrs available
             // when importing file content.
-            self.append_xattrs(checksum, &xattrs)?;
+            self.append_ostree_xattrs(checksum, &xattrs)?;
+            self.append_tarstream_xattrs(&xattrs)?;
 
             if let Some(instream) = instream {
                 ensure!(meta.file_type() == gio::FileType::Regular);
diff --git a/ostree-ext/tests/it/main.rs b/ostree-ext/tests/it/main.rs
@@ -5,6 +5,8 @@ use camino::Utf8Path;
 use cap_std::fs::{Dir, DirBuilder, DirBuilderExt};
 use cap_std_ext::cap_std;
 use containers_image_proxy::oci_spec;
+use gvariant::aligned_bytes::TryAsAligned;
+use gvariant::{Marker, Structure};
 use oci_image::ImageManifest;
 use oci_spec::image as oci_image;
 use ocidir::oci_spec::image::{Arch, DigestAlgorithm};
@@ -221,6 +223,7 @@ struct TarExpected {
     path: &'static str,
     etype: tar::EntryType,
     mode: u32,
+    should_have_security_capability: bool,
 }
 
 #[allow(clippy::from_over_into)]
@@ -230,6 +233,19 @@ impl Into<TarExpected> for (&'static str, tar::EntryType, u32) {
             path: self.0,
             etype: self.1,
             mode: self.2,
+            should_have_security_capability: false,
+        }
+    }
+}
+
+#[allow(clippy::from_over_into)]
+impl Into<TarExpected> for (&'static str, tar::EntryType, u32, bool) {
+    fn into(self) -> TarExpected {
+        TarExpected {
+            path: self.0,
+            etype: self.1,
+            mode: self.2,
+            should_have_security_capability: self.3,
         }
     }
 }
@@ -244,7 +260,7 @@ fn validate_tar_expected<T: std::io::Read>(
     let mut seen_paths = HashSet::new();
     // Verify we're injecting directories, fixes the absence of `/tmp` in our
     // images for example.
-    for entry in entries {
+    for mut entry in entries {
         if expected.is_empty() {
             return Ok(());
         }
@@ -265,6 +281,21 @@ fn validate_tar_expected<T: std::io::Read>(
                 header.entry_type(),
                 entry_path
             );
+            if exp.should_have_security_capability {
+                let pax = entry
+                    .pax_extensions()?
+                    .ok_or_else(|| anyhow::anyhow!("Missing pax extensions for {entry_path}"))?;
+                let mut found = false;
+                for ent in pax {
+                    let ent = ent?;
+                    if ent.key_bytes() != b"SCHILY.xattr.security.capability" {
+                        continue;
+                    }
+                    found = true;
+                    break;
+                }
+                assert!(found, "Expected security.capability in {entry_path}");
+            }
         }
     }
 
@@ -312,6 +343,9 @@ fn common_tar_contents_all() -> impl Iterator<Item = TarExpected> {
     ]
     .into_iter()
     .map(Into::into)
+    .chain(std::iter::once(
+        ("sysroot/ostree/repo/objects/b0/48a4f451e9fdfaaec911c1fe07d5d1d39be02f932b827c25458d3b15ae589e.file", Regular, 0o755, true).into(),
+    ))
 }
 
 /// Validate metadata (prelude) in a v1 tar.
@@ -335,6 +369,7 @@ fn test_tar_export_structure() -> Result<()> {
     let fixture = Fixture::new_v1()?;
 
     let src_tar = fixture.export_tar()?;
+    std::fs::copy(fixture.path.join(src_tar), "/tmp/test.tar").unwrap();
     let mut src_tar = fixture
         .dir
         .open(src_tar)
@@ -932,7 +967,7 @@ async fn test_container_chunked() -> Result<()> {
                 .created_by()
                 .as_ref()
                 .unwrap(),
-            "8 components"
+            "9 components"
         );
     }
     let import = imp.import(prep).await.context("Init pull derived").unwrap();
@@ -991,9 +1026,9 @@ r usr/bin/bash bash-v0
     assert!(second.0.commit.is_none());
     assert_eq!(
         first.1,
-        "ostree export of commit fe4ba8bbd8f61a69ae53cde0dd53c637f26dfbc87717b2e71e061415d931361e"
+        "ostree export of commit 6e6afc49d902daa2456c858818e0ad8bf9afe79cdcca738c5676c0e175c1def1"
     );
-    assert_eq!(second.1, "8 components");
+    assert_eq!(second.1, "9 components");
 
     assert_eq!(store::list_images(fixture.destrepo()).unwrap().len(), 1);
     let n = store::count_layer_references(fixture.destrepo())? as i64;
@@ -1785,6 +1820,36 @@ async fn test_container_xattr() -> Result<()> {
         _ => unreachable!(),
     };
 
+    // Verify security.capability is in the ostree commit
+    let arping = "/usr/bin/arping";
+    {
+        let ostree_root = fixture
+            .srcrepo()
+            .read_commit(fixture.testref(), gio::Cancellable::NONE)?
+            .0;
+        let arping_ostree = ostree_root.resolve_relative_path(arping);
+        assert_eq!(
+            arping_ostree.query_file_type(
+                gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS,
+                gio::Cancellable::NONE
+            ),
+            gio::FileType::Regular
+        );
+        let arping_ostree = arping_ostree.downcast_ref::<ostree::RepoFile>().unwrap();
+        let arping_ostree_xattrs = arping_ostree.xattrs(gio::Cancellable::NONE)?;
+        let v = arping_ostree_xattrs.data_as_bytes();
+        let v = v.try_as_aligned().unwrap();
+        let v = gvariant::gv!("a(ayay)").cast(v);
+        assert!(v
+            .iter()
+            .find(|entry| {
+                let k = entry.to_tuple().0;
+                let k = std::ffi::CStr::from_bytes_with_nul(k).unwrap();
+                k.to_str().ok() == Some("security.capability")
+            })
+            .is_some());
+    }
+
     // Build a derived image
     let derived_path = &fixture.path.join("derived.oci");
     oci_clone(basepath, derived_path).await?;
@@ -1832,6 +1897,8 @@ async fn test_container_xattr() -> Result<()> {
     )
     .read()?;
     assert!(out.contains("'user.foo', [byte 0x62, 0x61, 0x72]"));
+    let out = cmd!(sh, "ostree --repo=dest/repo ls -X {merge_commit} {arping}").read()?;
+    assert!(out.contains("'security.capability'"));
 
     Ok(())
 }
