Skip to content

imgstorage: Set selinux labels for imgstorage #1198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/src/boundimage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,11 @@ pub(crate) async fn pull_images(
sysroot: &Storage,
bound_images: Vec<crate::boundimage::BoundImage>,
) -> Result<()> {
// Only do work like initializing the image storage if we have images to pull.
// Always initialize the img store to ensure labels are set when upgrading
let imgstore = sysroot.get_ensure_imgstore()?;
if bound_images.is_empty() {
return Ok(());
}
let imgstore = sysroot.get_ensure_imgstore()?;
pull_images_impl(imgstore, bound_images).await
}

Expand Down
56 changes: 54 additions & 2 deletions lib/src/imgstorage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use cap_std_ext::cap_tempfile::TempDir;
use cap_std_ext::cmdext::CapStdExtCommandExt;
use cap_std_ext::dirext::CapStdExtDirExt;
use fn_error_context::context;
use ostree_ext::ostree::{self};
use std::os::fd::OwnedFd;
use tokio::process::Command as AsyncCommand;

Expand All @@ -35,6 +36,8 @@ pub(crate) const STORAGE_ALIAS_DIR: &str = "/run/bootc/storage";
/// We pass this via /proc/self/fd to the child process.
const STORAGE_RUN_FD: i32 = 3;

const LABELED: &str = ".bootc_labeled";

/// The path to the image storage, relative to the bootc root directory.
pub(crate) const SUBPATH: &str = "storage";
/// The path to the "runroot" with transient runtime state; this is
Expand Down Expand Up @@ -161,24 +164,68 @@ impl Storage {
Ok(())
}

#[context("Labeling imgstorage dirs")]
fn label_dirs(root: &Dir, sepolicy: Option<&ostree::SePolicy>) -> Result<()> {
if root.try_exists(LABELED)? {
return Ok(());
}
let Some(sepolicy) = sepolicy else {
return Ok(());
};

// recursively set the labels because they were previously set to usr_t,
// and there is no policy defined to set them to the c/storage labels
crate::lsm::ensure_dir_labeled_recurse_policy(
&root,
".",
Some(Utf8Path::new("/var/lib/containers/storage")),
0o755.into(),
Some(sepolicy),
)
.context("labeling storage root")?;

let paths = ["overlay", "overlay-images", "overlay-layers", "volumes"];
for p in paths {
let full_label_path = format!("/var/lib/containers/storage/{}", p);
crate::lsm::ensure_dir_labeled_recurse_policy(
&root,
p,
Some(Utf8Path::new(full_label_path.as_str())),
0o755.into(),
Some(sepolicy),
)
.context(format!("labeling storage subpath: {}", p))?;
}

root.create(LABELED)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in order to add a bit more crash resilence here it'd be a good idea to do e.g.:

rustix::fs::fsync(root.as_fd());
root.create(LABELED)?;
rustix::fs::fsync(root.as_fd());

or so - that should help ensure that we don't end up in a situation where (on a system crash) the LABELED file exists but the pending writes to do the relabeling didn't land.

(arg, in a quick test this fails because a cap-std Dir is an O_PATH fd and we can't fsync on that, I will look at a little helper for this)


Ok(())
}

#[context("Creating imgstorage")]
pub(crate) fn create(sysroot: &Dir, run: &Dir) -> Result<Self> {
pub(crate) fn create(
sysroot: &Dir,
run: &Dir,
sepolicy: Option<&ostree::SePolicy>,
) -> Result<Self> {
Self::init_globals()?;
let subpath = &Self::subpath();

// SAFETY: We know there's a parent
let parent = subpath.parent().unwrap();
let tmp = format!("{subpath}.tmp");
if !sysroot
.try_exists(subpath)
.with_context(|| format!("Querying {subpath}"))?
{
let tmp = format!("{subpath}.tmp");
sysroot.remove_all_optional(&tmp).context("Removing tmp")?;
sysroot
.create_dir_all(parent)
.with_context(|| format!("Creating {parent}"))?;
sysroot.create_dir_all(&tmp).context("Creating tmpdir")?;
let storage_root = sysroot.open_dir(&tmp).context("Open tmp")?;
Self::label_dirs(&storage_root, sepolicy)?;

// There's no explicit API to initialize a containers-storage:
// root, simply passing a path will attempt to auto-create it.
// We run "podman images" in the new root.
Expand All @@ -192,7 +239,12 @@ impl Storage {
.rename(&tmp, sysroot, subpath)
.context("Renaming tmpdir")?;
tracing::debug!("Created image store");
} else {
// the storage already exists, make sure it has selinux labels
let storage_root = sysroot.open_dir(subpath).context("opening storage dir")?;
Self::label_dirs(&storage_root, sepolicy)?;
}

Self::open(sysroot, run)
}

Expand Down
25 changes: 12 additions & 13 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,10 @@ pub(crate) fn print_configuration() -> Result<()> {
}

#[context("Creating ostree deployment")]
async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result<(Storage, bool)> {
async fn initialize_ostree_root(
state: &State,
root_setup: &RootSetup,
) -> Result<(Storage, bool, crate::imgstorage::Storage)> {
let sepolicy = state.load_policy()?;
let sepolicy = sepolicy.as_ref();
// Load a fd for the mounted target physical root
Expand Down Expand Up @@ -671,14 +674,9 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result

state.tempdir.create_dir("temp-run")?;
let temp_run = state.tempdir.open_dir("temp-run")?;
sysroot_dir
.create_dir_all(Utf8Path::new(crate::imgstorage::SUBPATH).parent().unwrap())
.context("creating bootc dir")?;
let imgstore = crate::imgstorage::Storage::create(&sysroot_dir, &temp_run)?;
// And drop it again - we'll reopen it after this
drop(imgstore);

// Bootstrap the initial labeling of the /ostree directory as usr_t
// and create the imgstorage with the same labels as /var/lib/containers
if let Some(policy) = sepolicy {
let ostree_dir = rootfs_dir.open_dir("ostree")?;
crate::lsm::ensure_dir_labeled(
Expand All @@ -690,9 +688,11 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
)?;
}

let imgstore = crate::imgstorage::Storage::create(&sysroot_dir, &temp_run, sepolicy)?;

sysroot.load(cancellable)?;
let sysroot = SysrootLock::new_from_sysroot(&sysroot).await?;
Ok((Storage::new(sysroot, &temp_run)?, has_ostree))
Ok((Storage::new(sysroot, &temp_run)?, has_ostree, imgstore))
}

#[context("Creating ostree deployment")]
Expand Down Expand Up @@ -1322,6 +1322,7 @@ async fn install_with_sysroot(
boot_uuid: &str,
bound_images: BoundImages,
has_ostree: bool,
imgstore: &crate::imgstorage::Storage,
) -> Result<()> {
// And actually set up the container in that root, returning a deployment and
// the aleph state (see below).
Expand Down Expand Up @@ -1349,10 +1350,6 @@ async fn install_with_sysroot(

tracing::debug!("Perfoming post-deployment operations");

// Note that we *always* initialize this container storage, even if there are no bound images
// today.
let imgstore = sysroot.get_ensure_imgstore()?;

match bound_images {
BoundImages::Skip => {}
BoundImages::Resolved(resolved_bound_images) => {
Expand Down Expand Up @@ -1438,14 +1435,16 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re

// Initialize the ostree sysroot (repo, stateroot, etc.)
{
let (sysroot, has_ostree) = initialize_ostree_root(state, rootfs).await?;
let (sysroot, has_ostree, imgstore) = initialize_ostree_root(state, rootfs).await?;

install_with_sysroot(
state,
rootfs,
&sysroot,
&boot_uuid,
bound_images,
has_ostree,
&imgstore,
)
.await?;
// We must drop the sysroot here in order to close any open file
Expand Down
11 changes: 10 additions & 1 deletion lib/src/install/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ use fn_error_context::context;
use ostree_ext::{gio, ostree};
use rustix::fs::Mode;
use rustix::fs::OFlags;
use std::os::fd::AsRawFd;

use crate::utils::deployment_fd;

use super::config;

Expand Down Expand Up @@ -289,9 +292,15 @@ pub(crate) async fn impl_completion(
// ostree-ext doesn't do logically bound images
let bound_images = crate::boundimage::query_bound_images_for_deployment(sysroot, deployment)?;
if !bound_images.is_empty() {
// load the selinux policy from the target ostree deployment
let deployment_fd = deployment_fd(sysroot, deployment)?;
let sepolicy =
&ostree::SePolicy::new_at(deployment_fd.as_raw_fd(), gio::Cancellable::NONE)?;

// When we're run through ostree, we only lazily initialize the podman storage to avoid
// having a hard dependency on it.
let imgstorage = &crate::imgstorage::Storage::create(&sysroot_dir, &rundir)?;
let imgstorage =
&crate::imgstorage::Storage::create(&sysroot_dir, &rundir, Some(sepolicy))?;
crate::boundimage::pull_images_impl(imgstorage, bound_images)
.await
.context("pulling bound images")?;
Expand Down
30 changes: 30 additions & 0 deletions lib/src/lsm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,36 @@ pub(crate) fn ensure_labeled(
Ok(r)
}

pub(crate) fn ensure_dir_labeled_recurse_policy(
root: &Dir,
destname: impl AsRef<Utf8Path>,
as_path: Option<&Utf8Path>,
mode: rustix::fs::Mode,
policy: Option<&ostree::SePolicy>,
) -> Result<()> {
ensure_dir_labeled(root, &destname, as_path, mode, policy)?;
let mut dest_path = destname.as_ref().to_path_buf();

for ent in root.read_dir(destname.as_ref())? {
let ent = ent?;
let metadata = ent.metadata()?;
let name = ent.file_name();
let name = name
.to_str()
.ok_or_else(|| anyhow::anyhow!("Invalid non-UTF-8 filename: {name:?}"))?;
dest_path.push(name);

if metadata.is_dir() {
ensure_dir_labeled_recurse_policy(root, &dest_path, as_path, mode, policy)?;
} else {
ensure_dir_labeled(root, &dest_path, as_path, mode, policy)?
}
dest_path.pop();
}

Ok(())
}

/// A wrapper for creating a directory, also optionally setting a SELinux label.
/// The provided `skip` parameter is a device/inode that we will ignore (and not traverse).
pub(crate) fn ensure_dir_labeled_recurse(
Expand Down
17 changes: 15 additions & 2 deletions lib/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ use cap_std_ext::cap_std::fs::Dir;
use cap_std_ext::dirext::CapStdExtDirExt;
use clap::ValueEnum;
use fn_error_context::context;
use std::os::fd::AsRawFd;

use ostree_ext::container::OstreeImageReference;
use ostree_ext::keyfileext::KeyFileExt;
use ostree_ext::ostree;
use ostree_ext::sysroot::SysrootLock;
use ostree_ext::{gio, ostree};

use crate::spec::ImageStatus;
use crate::utils::deployment_fd;

mod ostree_container;

Expand Down Expand Up @@ -85,7 +87,18 @@ impl Storage {
return Ok(imgstore);
}
let sysroot_dir = crate::utils::sysroot_dir(&self.sysroot)?;
let imgstore = crate::imgstorage::Storage::create(&sysroot_dir, &self.run)?;

if self.sysroot.booted_deployment().is_none() {
anyhow::bail!("Not a bootc system (this shouldn't be possible)");
}

// load the sepolicy from the booted ostree deployment so the imgstorage can be
// properly labeled with /var/lib/container/storage labels
let dep = self.sysroot.booted_deployment().unwrap();
let dep_fs = deployment_fd(&self.sysroot, &dep)?;
let sepolicy = &ostree::SePolicy::new_at(dep_fs.as_raw_fd(), gio::Cancellable::NONE)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The booted deployment's directory is going to be equivalent (mostly) to /...actually where it won't is if there are locally modified policy in /etc - which we probably want to use?

If we did that it'd argue to just open up the / directory...basically everywhere instead of passing e.g. self.run above we just pass a fd for the root. OR we could pass a SePolicy instance.

But...eh, this can all be followup.


let imgstore = crate::imgstorage::Storage::create(&sysroot_dir, &self.run, Some(sepolicy))?;
Ok(self.imgstore.get_or_init(|| imgstore))
}

Expand Down
8 changes: 8 additions & 0 deletions tests/booted/test-logically-bound-install.nu
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,15 @@ def test_bootc_image_list [] {
validate_images $images
}

def test_storage_labels [] {
let root_labeled = getfattr -n security.selinux /var/lib/containers/storage | grep container_var_lib_t | complete
assert equal $root_labeled.exit_code 0
let overlay_labeled = getfattr -n security.selinux /usr/lib/bootc/storage/overlay | grep container_ro_file_t | complete
assert equal $overlay_labeled.exit_code 0
}

test_logically_bound_images_in_storage
test_bootc_image_list
test_storage_labels

tap ok