diff --git a/Dockerfile b/Dockerfile index 36a36986b2..0cfd817493 100644 --- a/Dockerfile +++ b/Dockerfile @@ -422,6 +422,9 @@ if test "${boot_type}" = "uki"; then kver=$(ls /run/kernel) /run/packaging/finalize-uki /run/sealed-uki/out "$kver" fi +# TODO: drop once https://src.fedoraproject.org/rpms/filesystem/pull-request/44 or equivalent +# is widely available. For now, manually ensure that /usr has the correct permissions +chmod 0555 /usr EORUN # And finally, test our linting # lint: allow non-tmpfs - we want to detect leaked files in /run and /tmp diff --git a/crates/ostree-ext/src/container/store.rs b/crates/ostree-ext/src/container/store.rs index 9316d85470..f678ce06db 100644 --- a/crates/ostree-ext/src/container/store.rs +++ b/crates/ostree-ext/src/container/store.rs @@ -158,7 +158,9 @@ use oci_spec::image::{ }; use ocidir::oci_spec::distribution::Reference; use ostree::prelude::{Cast, FileEnumeratorExt, FileExt, ToVariant}; -use ostree::{gio, glib}; +use ostree::{DirMetaParsed, gio, glib}; +use rustix::fs::{Gid, Mode, OFlags, ResolveFlags, Uid}; +use rustix::io::Errno; use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::fmt::Write as _; use std::iter::FromIterator; @@ -642,6 +644,178 @@ fn cleanup_root(root: &Dir) -> Result<()> { Ok(()) } +/// Collect the dirmeta checksum for every directory in the given dir, keyed by path +/// relative to the commit root. Traverses over child directories recursively. +fn collect_dirmeta_checksums( + dir: &ostree::RepoFile, + path: &Utf8Path, + out: &mut HashMap, + cancellable: Option<&gio::Cancellable>, +) -> Result<()> { + dir.ensure_resolved()?; + + let checksum = dir + .tree_get_metadata_checksum() + .ok_or_else(|| anyhow!("Missing metadata checksum for {path}"))?; + out.insert(path.to_owned(), checksum); + + // Recurse over child directories, without following symlinks + let query_flags = gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS; + let files = + dir.enumerate_children("standard::name,standard::type", query_flags, cancellable)?; + while let Some(child_info) = files.next_file(cancellable)? { + if child_info.file_type() != gio::FileType::Directory { + continue; + } + let name = child_info.name(); + let name = name.to_str().expect("UTF-8 ostree name"); + let child = files + .child(&child_info) + .downcast::() + .expect("downcast"); + collect_dirmeta_checksums(&child, &path.join(name), out, cancellable)?; + } + + Ok(()) +} + +/// Apply directory mode and ownership from an ostree DirMeta to a filesystem +/// directory. +/// +/// We intentionally skip xattrs here: the SELinux relabelling pass in +/// `write_dfd_to_mtree` overwrites `security.selinux` on every directory, +/// and no other directory xattrs are meaningful in practice. +fn apply_dirmeta( + dir: impl rustix::fd::AsFd, + metadata: &ostree::DirMetaParsed, + should_fchown: bool, +) -> Result<()> { + if should_fchown { + rustix::fs::fchown( + &dir, + Some(Uid::from_raw(metadata.uid)), + Some(Gid::from_raw(metadata.gid)), + ) + .context("fchown")?; + } + rustix::fs::fchmod(&dir, Mode::from_raw_mode(metadata.mode)).context("fchmod")?; + Ok(()) +} + +/// A `UnionFiles` checkout keeps the directory metadata from the first layer +/// that created the directory, so later layers that only change a directory's +/// mode or ownership (e.g. `RUN chmod 0555 /usr`) are silently lost. +/// OCI semantics (and composefs-rs) require last-layer-wins, so we reapply +/// directory metadata after all layers have been checked out. +/// +/// For efficiency, we first collect only the cheap dirmeta *checksums* from +/// each layer. Only directories whose final checksum differs from the base +/// (first) layer need to be updated, which should let us avoid having to +/// process the vast majority of directories. +fn apply_layer_dirmeta( + repo: &ostree::Repo, + base_commit: Option<&str>, + layer_commits: &[LayerRef], + root: &Dir, + should_fchown: bool, + cancellable: Option<&gio::Cancellable>, +) -> Result<()> { + // Collect the base layer's dirmeta checksums so we can skip directories + // whose metadata doesn't differ from this layer + let base_checksums = match base_commit { + Some(base) => { + let mut base_map = HashMap::new(); + let (base_root, _) = repo.read_commit(base, cancellable)?; + let base_root = base_root.downcast::().unwrap(); + collect_dirmeta_checksums(&base_root, Utf8Path::new("."), &mut base_map, cancellable) + .context("Reading base directory metadata checksums")?; + base_map + } + None => HashMap::new(), + }; + + // Walk the derived layers in order, where the last writer for a given path + // wins via HashMap::insert. + let mut final_checksums: HashMap = HashMap::new(); + for lc in layer_commits { + let (commit_root, _) = repo.read_commit(&lc.commit, cancellable)?; + let commit_root = commit_root + .downcast::() + .expect("downcast"); + collect_dirmeta_checksums( + &commit_root, + Utf8Path::new("."), + &mut final_checksums, + cancellable, + ) + .with_context(|| format!("Reading directory metadata checksums of {}", lc.commit))?; + } + + // Only parse + apply directories whose checksum differs from the base. + for (path, checksum) in &final_checksums { + if base_checksums + .get(path) + .is_some_and(|s| s.as_str() == checksum.as_str()) + { + tracing::debug!("Skipping {path}, metadata checksum is the same as the base layer"); + continue; + } + tracing::debug!("Applying directory metadata for {path}"); + + let variant = repo + .load_variant(ostree::ObjectType::DirMeta, checksum) + .with_context(|| format!("Loading dirmeta {checksum} for {path}"))?; + let parsed = ostree::DirMetaParsed::from_variant(&variant) + .map_err(|e| anyhow!("Parsing dirmeta for {path}: {e}"))?; + + let flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC; + let resolve = ResolveFlags::BENEATH | ResolveFlags::NO_SYMLINKS; + let fd = match rustix::fs::openat2(root, path.as_std_path(), flags, Mode::empty(), resolve) + { + Ok(fd) => fd, + // The directory was removed by a later layer, or it / a parent + // was replaced by a non-directory. + Err(Errno::NOENT | Errno::NOTDIR | Errno::LOOP) => { + continue; + } + Err(e) => return Err(e).with_context(|| format!("Getting FD for {path}")), + }; + apply_dirmeta(fd, &parsed, should_fchown) + .with_context(|| format!("Updating metadata for {path}"))?; + } + Ok(()) +} + +/// Copy the `/usr` directory mode and ownership to the root, matching +/// composefs-rs, since OCI images don't reliably include root dir metadata. +/// This should be called before the merged tree is committed, so the root +/// still gets its own SELinux label. +/// +/// We intentionally don't copy xattrs here: the SELinux relabelling pass +/// in `write_dfd_to_mtree` will assign the correct `root_t` label to `/`. +/// +/// See: +/// - +/// - +/// - +fn normalize_root_dirmeta(root: &Dir, should_fchown: bool) -> Result<()> { + let usr = root.symlink_metadata("usr").context("looking up /usr")?; + if !usr.is_dir() { + anyhow::bail!("/usr is not a directory"); + } + // `root` may be an `O_PATH` fd, on which fchmod/fchown fail with EBADF. + let flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC; + let rootfd = rustix::fs::openat(root, ".", flags, Mode::empty()).context("opening root")?; + + let metadata = DirMetaParsed { + uid: usr.uid(), + gid: usr.gid(), + mode: usr.mode(), + xattrs: vec![], + }; + apply_dirmeta(rootfd, &metadata, should_fchown).context("applying /usr metadata to root") +} + impl ImageImporter { /// The metadata key used in ostree commit metadata to serialize const CACHED_KEY_MANIFEST_DIGEST: &'static str = "ostree-ext.cached.manifest-digest"; @@ -1431,6 +1605,20 @@ impl ImageImporter { cleanup_root(&root_dir)?; + // If we are running as root, we should fchown to apply uid/gid + let should_fchown = checkout_mode == ostree::RepoCheckoutMode::None; + apply_layer_dirmeta( + repo, + base_commit, + layer_commits, + &root_dir, + should_fchown, + cancellable, + ) + .context("applying layer directory metadata")?; + normalize_root_dirmeta(&root_dir, should_fchown) + .context("normalizing root dir metadata")?; + let mt = ostree::MutableTree::new(); repo.write_dfd_to_mtree( (*td).as_raw_fd(), diff --git a/crates/ostree-ext/tests/it/main.rs b/crates/ostree-ext/tests/it/main.rs index 8f973d3c44..d55d25738d 100644 --- a/crates/ostree-ext/tests/it/main.rs +++ b/crates/ostree-ext/tests/it/main.rs @@ -60,6 +60,47 @@ fn assert_err_contains(r: Result, s: impl AsRef) { } } +/// Verify that a commit's root directory mode/uid/gid matches `/usr`. +fn assert_root_dirmeta_matches_usr(repo: &ostree::Repo, commit: &str) { + let root = repo.read_commit(commit, gio::Cancellable::NONE).unwrap().0; + let root = root.downcast::().unwrap(); + root.ensure_resolved().unwrap(); + + let usr = root.resolve_relative_path("usr"); + let usr = usr.downcast::().unwrap(); + usr.ensure_resolved().unwrap(); + + let queryflags = gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS; + let attrs = "unix::mode,unix::uid,unix::gid"; + let root_info = root + .query_info(attrs, queryflags, gio::Cancellable::NONE) + .unwrap(); + let usr_info = usr + .query_info(attrs, queryflags, gio::Cancellable::NONE) + .unwrap(); + + let root_mode = root_info.attribute_uint32("unix::mode"); + let usr_mode = usr_info.attribute_uint32("unix::mode"); + assert_eq!( + root_mode, usr_mode, + "root mode '{root_mode:#o}' should match /usr mode '{usr_mode:#o}'" + ); + + let root_uid = root_info.attribute_uint32("unix::uid"); + let usr_uid = usr_info.attribute_uint32("unix::uid"); + assert_eq!( + root_uid, usr_uid, + "root uid '{root_uid}' should match /usr uid '{usr_uid}'" + ); + + let root_gid = root_info.attribute_uint32("unix::gid"); + let usr_gid = usr_info.attribute_uint32("unix::gid"); + assert_eq!( + root_gid, usr_gid, + "root gid '{root_gid}' should match /usr gid '{usr_gid}'" + ); +} + static TEST_REGISTRY: LazyLock = LazyLock::new(|| match std::env::var_os("TEST_REGISTRY") { Some(t) => t.to_str().unwrap().to_owned(), @@ -709,6 +750,7 @@ async fn test_export_as_container_nonderived() -> Result<()> { let reimported = fixture.must_import(&dest).await?; let reimport_ls = fixture::ostree_ls(fixture.destrepo(), &reimported.merge_commit).unwrap(); similar_asserts::assert_eq!(initimport_ls, reimport_ls); + assert_root_dirmeta_matches_usr(fixture.destrepo(), &reimported.merge_commit); Ok(()) } @@ -1024,6 +1066,69 @@ async fn test_export_etc_remapping() -> Result<()> { Ok(()) } +/// Verify that a derived layer that changes `/usr` mode (e.g. `RUN chmod 0555 /usr`) +/// is preserved through the merge commit. +#[tokio::test] +async fn test_derived_layer_usr_mode() -> Result<()> { + if !check_skopeo() { + return Ok(()); + } + let fixture = Fixture::new_v1()?; + let src_imgref = fixture.export_container().await.unwrap().0; + + // Build a derived image with a layer that changes /usr to mode 0555. + let derived_path = &fixture.path.join("derived-usr-mode.oci"); + oci_clone(src_imgref.name.as_str(), derived_path) + .await + .unwrap(); + ostree_ext::integrationtest::generate_derived_oci_from_tar( + derived_path, + |w| { + let mut layer_tar = tar::Builder::new(w); + let mut h = tar::Header::new_gnu(); + h.set_uid(0); + h.set_gid(0); + h.set_size(0); + h.set_mode(0o555); + h.set_entry_type(tar::EntryType::Directory); + layer_tar.append_data(&mut h, "usr", &mut std::io::empty())?; + layer_tar.finish()?; + Ok(()) + }, + None, + None, + )?; + + let derived_imgref = ImageReference { + transport: Transport::OciDir, + name: derived_path.to_string(), + }; + let imp = fixture.must_import(&derived_imgref).await?; + + // Verify /usr has mode 0555 in the merge commit. + let root = fixture + .destrepo() + .read_commit(&imp.merge_commit, gio::Cancellable::NONE)? + .0; + let usr = root.resolve_relative_path("usr"); + let usr = usr.downcast::().unwrap(); + usr.ensure_resolved().unwrap(); + + let info = usr.query_info( + "unix::mode", + gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS, + gio::Cancellable::NONE, + )?; + let mode = info.attribute_uint32("unix::mode") & 0o7777; // mask to get only permissions bits + assert_eq!( + mode, 0o555, + "/usr mode should be 0555 after derived layer, got '{mode:#o}'" + ); + + assert_root_dirmeta_matches_usr(fixture.destrepo(), &imp.merge_commit); + Ok(()) +} + #[tokio::test] async fn test_unencapsulate_unbootable() -> Result<()> { if !check_skopeo() { @@ -1653,6 +1758,7 @@ async fn test_non_ostree() -> Result<()> { similar_asserts::assert_eq!(&src_manifest, &dest_manifest); } assert_eq!(imp.manifest_digest, src_digest); + assert_root_dirmeta_matches_usr(fixture.destrepo(), &imp.merge_commit); Ok(()) } diff --git a/tmt/tests/booted/readonly/040-test-baseconfigs.nu b/tmt/tests/booted/readonly/040-test-baseconfigs.nu index a8f6e510d1..a988bf0e33 100644 --- a/tmt/tests/booted/readonly/040-test-baseconfigs.nu +++ b/tmt/tests/booted/readonly/040-test-baseconfigs.nu @@ -21,9 +21,8 @@ for config in $configs { let fstype = $mnt.filesystems.0.fstype assert equal $fstype "overlay" "/etc should be mounted as overlay when etc-transient is active" - print "# Checking / has 755 permissions (overlay upper dir must not block traversal)" - let perms = (stat -c "%a" / | str trim) - assert equal $perms "755" "/ should have 755 permissions" + # root permission checks done in 055-test-root-dirmeta + # (overlay upper dir must not block traversal) print "# Checking /etc is writable (transient overlay)" let result = (do { ^touch /etc/bootc-baseconfig-test } | complete) @@ -36,9 +35,7 @@ for config in $configs { let fstype = $mnt.filesystems.0.fstype assert equal $fstype "overlay" "/ should be mounted as overlay when root-transient is active" - print "# Checking / has 755 permissions" - let perms = (stat -c "%a" / | str trim) - assert equal $perms "755" "/ should have 755 permissions" + # root permission checks done in 055-test-root-dirmeta print "# Checking / is writable (transient overlay)" let result = (do { ^touch /bootc-baseconfig-root-test } | complete) diff --git a/tmt/tests/booted/readonly/055-test-root-dirmeta.nu b/tmt/tests/booted/readonly/055-test-root-dirmeta.nu new file mode 100644 index 0000000000..e555b85155 --- /dev/null +++ b/tmt/tests/booted/readonly/055-test-root-dirmeta.nu @@ -0,0 +1,18 @@ +# Verify that the root dir's uid, gid, and permissions match /usr. +# +# See: https://github.com/coreos/fedora-coreos-tracker/issues/2238 +use std assert +use tap.nu + +tap begin "verify root dir metadata matches /usr" + +let root_meta = (stat -c "mode: %a, gid: %g, uid: %u" / | str trim) +let usr_meta = (stat -c "mode: %a, gid: %g, uid: %u" /usr | str trim) + +print $"# root: ($root_meta)" +print $"# /usr: ($usr_meta)" + +assert equal $root_meta $usr_meta $"root dir metadata \(($root_meta)\) should match /usr \(($usr_meta)\)" +assert equal $root_meta "mode: 555, gid: 0, uid: 0" $"root dir metadata \(($root_meta)\) should match expected \(mode: 555, gid: 0, uid: 0\)" + +tap ok