ostree-ext: Serialize xattrs into tar stream as well

We really want this for coreos/rpm-ostree#5222
to be able to rebuild images from their container-synthesized rootfs.

Really, the only xattr we don't want to emit in to the tar stream
is security.selinux for now.

Eventually we should try to switch to putting that into the tar
stream too, but it needs more validation.

Signed-off-by: Colin Walters <[email protected]>

Author: cgwalters

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(())
     }
 

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.
@@ -379,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)> {
@@ -402,6 +440,7 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             // refer to. Otherwise the importing logic won't have the xattrs available
             // when importing file content.
             self.append_ostree_xattrs(checksum, &xattrs)?;
+            self.append_tarstream_xattrs(&xattrs)?;
 
             if let Some(instream) = instream {
                 ensure!(meta.file_type() == gio::FileType::Regular);

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(())
 }