From f21f6d81b00aaa3f1663925af0b65eb40fbe6aa1 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 15 Jan 2025 16:44:39 -0500 Subject: [PATCH] ostree-ext: Serialize xattrs into tar stream as well We really want this for https://github.com/coreos/rpm-ostree/pull/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 --- ostree-ext/src/fixture.rs | 55 ++++++++++++++++++++------ ostree-ext/src/tar/export.rs | 39 +++++++++++++++++++ ostree-ext/tests/it/main.rs | 75 ++++++++++++++++++++++++++++++++++-- 3 files changed, 154 insertions(+), 15 deletions(-) diff --git a/ostree-ext/src/fixture.rs b/ostree-ext/src/fixture.rs index ca27e232a..85ac94dce 100644 --- a/ostree-ext/src/fixture.rs +++ b/ostree-ext/src/fixture.rs @@ -8,6 +8,7 @@ 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 camino::{Utf8Component, Utf8Path, Utf8PathBuf}; @@ -25,6 +26,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 +48,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 +75,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> = parts + .map(|xattr| -> Result { + let (k, v) = xattr + .split_once('=') + .ok_or_else(|| anyhow::anyhow!("Invalid xattr: {xattr}"))?; + let mut k: Vec = 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 +101,7 @@ impl TryFrom<&'static str> for FileDef { gid: 0, mode: 0o644, path: Cow::Borrowed(name.into()), + xattrs, ty, }) } @@ -165,6 +187,7 @@ static OWNERS: Lazy> = 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 +207,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 +230,7 @@ m 0 0 1755 d tmp "## }; pub const CONTENTS_CHECKSUM_V0: &str = - "acc42fb5c796033f034941dc688643bf8beddfd9068d87165344d2b99906220a"; + "4449a2b27dd907ffb5e3556018584cfa048edaf3eee4a11ecf21dcd61b6c7a1c"; // 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 +291,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 +309,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 +655,17 @@ 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 { + Some(xattrs.to_variant()) + }; let xattrs = xattrs.as_ref(); let checksum = match &def.ty { FileDefType::Regular(contents) => self diff --git a/ostree-ext/src/tar/export.rs b/ostree-ext/src/tar/export.rs index 5543ac421..eac79ab60 100644 --- 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. @@ -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); diff --git a/ostree-ext/tests/it/main.rs b/ostree-ext/tests/it/main.rs index f460a423c..73d48be82 100644 --- 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}; @@ -20,6 +22,7 @@ use ostree_ext::{fixture, ostree_manual}; use ostree_ext::{gio, glib}; use std::borrow::Cow; use std::collections::{HashMap, HashSet}; +use std::fs::File; use std::io::{BufReader, BufWriter}; use std::process::{Command, Stdio}; use std::time::SystemTime; @@ -221,6 +224,7 @@ struct TarExpected { path: &'static str, etype: tar::EntryType, mode: u32, + should_have_security_capability: bool, } #[allow(clippy::from_over_into)] @@ -230,6 +234,19 @@ impl Into 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 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 +261,7 @@ fn validate_tar_expected( 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 +282,21 @@ fn validate_tar_expected( 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 +344,9 @@ fn common_tar_contents_all() -> impl Iterator { ] .into_iter() .map(Into::into) + .chain(std::iter::once( + ("sysroot/ostree/repo/objects/0f/49c39f52b33941d8e283f927902d4c10ff5b62e261fe5f4138045a25b26432.file", Regular, 0o755, true).into(), + )) } /// Validate metadata (prelude) in a v1 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 a2b98a97aae864c69360010c16c871b087ce2f86e915091caff1c61b824e7f54" ); - 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::().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(()) }