From ecc33c6bd5d903da45d638657c94787b8bb5fac4 Mon Sep 17 00:00:00 2001 From: Paco Date: Tue, 13 Oct 2026 14:20:53 -0700 Subject: [PATCH 1/3] fix(tailor): disable SELinux labeling for janitor containers only Give ownership and removal janitors a per-container label=disable option without changing IC security options, privilege or guarded bind paths. Document the unresolved live-loop and mount hazard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/tailor/crates/tailor-core/src/ports.rs | 2 + .../tailor-exec/src/container/runtime.rs | 78 +++++++++++++--- .../tailor/crates/tailor-exec/src/executor.rs | 1 + .../tailor/crates/tailor-exec/src/janitor.rs | 91 +++++++++++++++++++ tools/tailor/docs/explanation/threat-model.md | 14 ++- 5 files changed, 172 insertions(+), 14 deletions(-) diff --git a/tools/tailor/crates/tailor-core/src/ports.rs b/tools/tailor/crates/tailor-core/src/ports.rs index fa7b5ab0f0..5d7ac3f786 100644 --- a/tools/tailor/crates/tailor-core/src/ports.rs +++ b/tools/tailor/crates/tailor-core/src/ports.rs @@ -218,6 +218,8 @@ pub struct ContainerConfig { pub args: Vec, pub binds: Vec, pub privileged: bool, + /// Disable SELinux container labeling for this request only (janitor cleanup on labeled hosts). + pub disable_selinux_label: bool, /// The cell slug, tagged onto every re-emitted IC log event (`cell = `); empty for /// non-cell containers such as the ownership janitor (`meta/docs/2026-06-29-logging.md` §5.3). pub cell_slug: String, diff --git a/tools/tailor/crates/tailor-exec/src/container/runtime.rs b/tools/tailor/crates/tailor-exec/src/container/runtime.rs index 96e80c14b3..e6e56aa051 100644 --- a/tools/tailor/crates/tailor-exec/src/container/runtime.rs +++ b/tools/tailor/crates/tailor-exec/src/container/runtime.rs @@ -181,23 +181,13 @@ impl ContainerRuntime for BollardRuntime { let cell_slug = config.cell_slug.clone(); let log_source = config.log_source; let log_file = config.log_file.clone(); - let docker_config = DockerConfig { - image: Some(config.image_ref), - cmd: Some(config.args), - attach_stdout: Some(true), - attach_stderr: Some(true), - host_config: Some(HostConfig { - binds: Some(config.binds), - privileged: Some(config.privileged), - ..Default::default() - }), - ..Default::default() - }; + let platform = config.platform.clone(); + let docker_config = container_docker_config(config); self.docker .create_container( Some(CreateContainerOptions { name: name.clone(), - platform: Some(config.platform), + platform: Some(platform), }), docker_config, ) @@ -313,6 +303,24 @@ impl ContainerRuntime for BollardRuntime { } } +fn container_docker_config(config: ContainerConfig) -> DockerConfig { + DockerConfig { + image: Some(config.image_ref), + cmd: Some(config.args), + attach_stdout: Some(true), + attach_stderr: Some(true), + host_config: Some(HostConfig { + binds: Some(config.binds), + privileged: Some(config.privileged), + security_opt: config + .disable_selinux_label + .then(|| vec!["label=disable".to_owned()]), + ..Default::default() + }), + ..Default::default() + } +} + /// A [`ContainerRuntime`] that never connects to an engine — the stand-in for `build --dry-run`, /// which renders container invocations without contacting any daemon (`meta/docs/2026-06-29-container-runtimes.md` /// §4). Dry-run execution returns before touching the runtime, so these methods are never reached; if @@ -479,6 +487,50 @@ fn map_runtime_error(err: &BollardError) -> ExecError { mod tests { use super::*; + #[test] + fn selinux_label_opt_out_is_per_container_not_a_runtime_default() { + let ic = ContainerConfig { + image_ref: "example/ic".to_owned(), + platform: "linux/amd64".to_owned(), + name: "ic".to_owned(), + args: vec!["customize".to_owned()], + binds: vec!["/build:/build".to_owned()], + privileged: true, + disable_selinux_label: false, + cell_slug: "cell".to_owned(), + log_source: LogSource::ImageCustomizer, + log_file: None, + }; + let normal = container_docker_config(ic.clone()); + let host = normal.host_config.as_ref().unwrap(); + assert!(host.security_opt.is_none()); + assert_eq!(host.privileged, Some(true)); + assert!( + serde_json::to_value(&normal).unwrap()["HostConfig"] + .get("SecurityOpt") + .is_none() + ); + + let janitor = container_docker_config(ContainerConfig { + privileged: false, + disable_selinux_label: true, + log_source: LogSource::Janitor, + ..ic + }); + let host = janitor.host_config.as_ref().unwrap(); + assert_eq!( + host.security_opt.as_deref(), + Some(&["label=disable".to_owned()][..]) + ); + assert_eq!(host.privileged, Some(false)); + let payload = serde_json::to_value(&janitor).unwrap(); + assert_eq!( + payload["HostConfig"]["SecurityOpt"], + serde_json::json!(["label=disable"]) + ); + assert_eq!(payload["HostConfig"]["Privileged"], false); + } + #[test] fn parse_daemon_info_detects_docker_rootless_and_userns() { let options = vec!["name=seccomp".to_owned(), "name=rootless".to_owned()]; diff --git a/tools/tailor/crates/tailor-exec/src/executor.rs b/tools/tailor/crates/tailor-exec/src/executor.rs index 6875d0c76c..c4316b2b66 100644 --- a/tools/tailor/crates/tailor-exec/src/executor.rs +++ b/tools/tailor/crates/tailor-exec/src/executor.rs @@ -329,6 +329,7 @@ impl IcExecutor { args, binds: arg_builder::container_binds(cell, context, extra_rw)?, privileged: context.runtime.privileged, + disable_selinux_label: false, cell_slug: cell.slug.as_ref().to_owned(), log_source: tailor_core::LogSource::ImageCustomizer, log_file: log_file.cloned(), diff --git a/tools/tailor/crates/tailor-exec/src/janitor.rs b/tools/tailor/crates/tailor-exec/src/janitor.rs index ff98673343..56f54d1709 100644 --- a/tools/tailor/crates/tailor-exec/src/janitor.rs +++ b/tools/tailor/crates/tailor-exec/src/janitor.rs @@ -118,6 +118,7 @@ async fn run_janitor( args, binds, privileged: false, + disable_selinux_label: true, cell_slug: String::new(), log_source: LogSource::Janitor, log_file: None, @@ -152,6 +153,96 @@ fn path_arg(path: &Path) -> String { mod tests { use super::*; + use std::sync::Mutex; + + use tailor_core::{ContainerResult, DaemonInfo, LocalImage}; + + #[derive(Default)] + struct RecordingRuntime { + configs: Mutex>, + } + + impl ContainerRuntime for RecordingRuntime { + async fn pull_image(&self, _reference: &str) -> Result<(), ExecError> { + Ok(()) + } + + async fn inspect_image(&self, _reference: &str) -> Result, ExecError> { + Ok(None) + } + + async fn create_and_run( + &self, + config: ContainerConfig, + _cancel: CancellationToken, + ) -> Result { + self.configs.lock().unwrap().push(config); + Ok(ContainerResult { + exit_code: 0, + logs: String::new(), + failure_dump: None, + }) + } + + async fn daemon_info(&self) -> Result { + Ok(DaemonInfo::default()) + } + + async fn export_container( + &self, + _image_ref: &str, + _platform: &str, + _pull: bool, + _dest_dir: &Path, + _cancel: CancellationToken, + ) -> Result<(), ExecError> { + Ok(()) + } + } + + #[tokio::test] + async fn janitor_requests_disable_selinux_labels_without_privilege_or_broad_mounts() { + let dir = tempfile::Builder::new() + .prefix("tailor-janitor-") + .tempdir_in(std::env::current_dir().unwrap()) + .unwrap(); + let target = dir.path().join("tools-dir"); + std::fs::create_dir(&target).unwrap(); + let runtime = RecordingRuntime::default(); + let config = RuntimeConfig { + janitor_image: "example/janitor@sha256:abc".to_owned(), + ..RuntimeConfig::default() + }; + + chown_paths( + &runtime, + std::slice::from_ref(&target), + &config, + CancellationToken::new(), + ) + .await + .unwrap(); + remove_paths( + &runtime, + std::slice::from_ref(&target), + &config, + CancellationToken::new(), + ) + .await + .unwrap(); + + let configs = runtime.configs.lock().unwrap(); + assert_eq!(configs.len(), 2); + for request in configs.iter() { + assert!(request.disable_selinux_label); + assert!(!request.privileged); + assert_eq!(request.log_source, LogSource::Janitor); + } + assert_eq!(configs[0].binds, vec![identity_bind(&target)]); + assert_eq!(configs[1].binds, vec![identity_bind(dir.path())]); + assert!(target.exists(), "mock janitor must not delete host files"); + } + #[test] fn host_platform_matches_the_running_arch() { let platform = host_platform(); diff --git a/tools/tailor/docs/explanation/threat-model.md b/tools/tailor/docs/explanation/threat-model.md index 4a2bfe7fb0..c3f7842d7f 100644 --- a/tools/tailor/docs/explanation/threat-model.md +++ b/tools/tailor/docs/explanation/threat-model.md @@ -55,7 +55,19 @@ load-bearing — treat a regression in them as a serious bug: `crates/tailor-core/src/atomic.rs`. - **Scoped privileged cleanup.** The janitor only chowns/removes the specific paths a build produced (the build directory's children, the image cache, the - per-cell log), never arbitrary locations. + per-cell log), never arbitrary locations. On SELinux hosts, janitor containers + request `SecurityOpt: ["label=disable"]` so cleanup can write to host-labeled + output paths without recursively relabeling them. This opts the janitor out + of SELinux container separation for its lifetime; it remains unprivileged + (`Privileged: false`) and receives only the guarded target/parent binds. + Image Customizer containers do not request this opt-out. Run builds with a + trusted janitor image and trusted output paths; filesystem permissions, + user namespaces, and the daemon's own restrictions still apply. Path guards + do not prove cleanup targets are inactive: IC may leave live loop devices + backed by build scratch or writable mounts under it. Tailor does not currently + check for these before janitor removal; do not clean affected paths until + their liveness has been verified outside tailor. Artifact modification times + do not establish that a mount is inactive. ## Supply-chain pinning From a9c4e3fcebda621a18bb2a807fe7e37967f620d5 Mon Sep 17 00:00:00 2001 From: Paco Date: Tue, 13 Oct 2026 17:06:08 -0700 Subject: [PATCH 2/3] tailor: clarify janitor SELinux boundary Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../tailor/crates/tailor-exec/src/janitor.rs | 10 ++------ tools/tailor/docs/explanation/threat-model.md | 23 +++++++------------ 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/tools/tailor/crates/tailor-exec/src/janitor.rs b/tools/tailor/crates/tailor-exec/src/janitor.rs index 56f54d1709..75ac3fa97e 100644 --- a/tools/tailor/crates/tailor-exec/src/janitor.rs +++ b/tools/tailor/crates/tailor-exec/src/janitor.rs @@ -69,18 +69,12 @@ pub(crate) async fn remove_paths( } let mut args = vec![RM_BINARY.to_owned(), "-rf".to_owned(), "--".to_owned()]; args.extend(existing.iter().map(|path| path_arg(path))); - // Bind each target's PARENT, not the target itself. If we identity-bound the target, it would be - // an active mountpoint inside the janitor container: `rm -rf` clears its contents but cannot - // `rmdir` a busy mountpoint, so the top directory survives and rm exits non-zero (EBUSY) — the - // recurring "cannot remove '': Device or resource busy" that failed every successful build. - // With the parent bound, the target is an ordinary child directory that rm removes top-and-all. + // Binding the target makes it a mountpoint that `rm` cannot remove (EBUSY). let binds = removal_binds(&existing)?; run_janitor(runtime, config, args, binds, cancel).await } -/// Bind sources for a removal: each distinct target **parent**, guarded so the janitor never binds -/// `/`, an ancestor of the working directory, or another unsafe directory. The target itself is then -/// an ordinary child under its bound parent (see [`remove_paths`]). +/// Bind each target's parent for removal; refuse binding `/`. fn removal_binds(paths: &[PathBuf]) -> Result, ExecError> { let mut parents = std::collections::BTreeSet::new(); for path in paths { diff --git a/tools/tailor/docs/explanation/threat-model.md b/tools/tailor/docs/explanation/threat-model.md index c3f7842d7f..5a926e5430 100644 --- a/tools/tailor/docs/explanation/threat-model.md +++ b/tools/tailor/docs/explanation/threat-model.md @@ -53,21 +53,14 @@ load-bearing — treat a regression in them as a serious bug: artifact are written to a temporary file and renamed into place, so an interrupted build cannot leave torn state a later run would trust. See `crates/tailor-core/src/atomic.rs`. -- **Scoped privileged cleanup.** The janitor only chowns/removes the specific - paths a build produced (the build directory's children, the image cache, the - per-cell log), never arbitrary locations. On SELinux hosts, janitor containers - request `SecurityOpt: ["label=disable"]` so cleanup can write to host-labeled - output paths without recursively relabeling them. This opts the janitor out - of SELinux container separation for its lifetime; it remains unprivileged - (`Privileged: false`) and receives only the guarded target/parent binds. - Image Customizer containers do not request this opt-out. Run builds with a - trusted janitor image and trusted output paths; filesystem permissions, - user namespaces, and the daemon's own restrictions still apply. Path guards - do not prove cleanup targets are inactive: IC may leave live loop devices - backed by build scratch or writable mounts under it. Tailor does not currently - check for these before janitor removal; do not clean affected paths until - their liveness has been verified outside tailor. Artifact modification times - do not establish that a mount is inactive. +- **Scoped cleanup.** The janitor binds a target for chown or its parent for + removal; removal refuses a parent resolving to `/`. A bound parent can still + contain unrelated files. On SELinux hosts, janitors request `label=disable` + without relabeling host files; IC is unchanged and janitors remain + `Privileged: false`. With a rootful daemon, container root can override + ordinary file permissions within these writable binds; use a trusted janitor + image. Tailor does not check for live loop devices or mounts before removal; + do not clean live build trees. ## Supply-chain pinning From e4e474c7b53cf5bb5c247d973db6433805835aa0 Mon Sep 17 00:00:00 2001 From: Paco Date: Wed, 14 Oct 2026 12:15:50 -0700 Subject: [PATCH 3/3] docs(tailor): record measured janitor SELinux type Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/tailor/docs/explanation/threat-model.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/tailor/docs/explanation/threat-model.md b/tools/tailor/docs/explanation/threat-model.md index 5a926e5430..0491375648 100644 --- a/tools/tailor/docs/explanation/threat-model.md +++ b/tools/tailor/docs/explanation/threat-model.md @@ -57,7 +57,9 @@ load-bearing — treat a regression in them as a serious bug: removal; removal refuses a parent resolving to `/`. A bound parent can still contain unrelated files. On SELinux hosts, janitors request `label=disable` without relabeling host files; IC is unchanged and janitors remain - `Privileged: false`. With a rootful daemon, container root can override + `Privileged: false`. A Fedora 43/Docker 29.8.2 probe with the janitor image + ran as `system_u:system_r:spc_t:s0`, not `unconfined_t`; other engines may + assign a different type. With a rootful daemon, container root can override ordinary file permissions within these writable binds; use a trusted janitor image. Tailor does not check for live loop devices or mounts before removal; do not clean live build trees.