From d0511929ccd94d04875639c35d8d51fc25fa51f0 Mon Sep 17 00:00:00 2001 From: Ivan Liang Date: Sun, 6 Sep 2026 14:36:16 -0400 Subject: [PATCH 1/2] fix: exec the OCI runtime, and always apply readonly networking mounts call_oci_runtime spawned the runtime as a child and waited on it, leaving the interceptor in the process tree for the lifetime of every runtime call. containerd's shim invokes the runtime through go-runc, which builds commands with Go's exec.CommandContext, and a cancelled or timed-out call delivers SIGKILL to the direct child only -- so the interceptor died and the real runtime underneath it was orphaned while the shim believed the call had been cancelled. For a `runc delete` that leaves the task undeleted and its containerd-shim-runc-v2 running indefinitely, holding roughly 5 MiB until the host reboots. A 2 GiB cork worker was measured carrying 66 such shims against 5 running containers. Exec'ing means the runtime inherits our pid and we leave the process tree entirely. It also fixes a runtime killed by a signal being reported as exit code -1, which process::exit truncates to 255, hiding signal death from callers that distinguish the two. This is necessary but not sufficient on its own, and the documentation now says so. Docker generates its own wrapper script for any runtime whose runtimeArgs is non-empty, and that generated wrapper does not exec: #!/bin/sh /usr/local/bin/oci-interceptor --flags $@ It re-inserts exactly the layer this change removes, so an operator following the old README example got the fixed binary and still leaked shims. The only complete daemon.json example no longer uses runtimeArgs, and the README documents the wrapper-script alternative with "$@" quoted -- Docker's generated wrapper uses a bare $@, which word-splits and glob-expands. It also stops presenting the CI integration job as a reference deployment: that job uses runtimeArgs because the per-runtime flag assertions need it, which means CI never exercises the shape a real host should use. Separately, /etc/hosts, /etc/hostname and /etc/resolv.conf are now made read-only unconditionally rather than only under --oi-readonly-networking-mounts. Docker bind-mounts these from outside the container's writable layer, so where XFS project quotas restrict that layer they are an escape hatch for filling the host volume, which on a shared host affects every other container. Note this is not a barrier between containers -- each gets its own copy, so tampering only affects the container doing it, and a process with code execution bypasses these files anyway via HOSTALIASES, RES_OPTIONS, LD_PRELOAD or by querying a resolver directly. There is no use case here for writable networking mounts, so there is no opt-out; --oi-readonly-networking-mounts is retained as an accepted no-op so a daemon.json still passing it keeps working, which keeps the rollout order independent. The mounts themselves are left in place: Docker points a container on a user-defined network at its embedded DNS resolver by writing `nameserver 127.0.0.11` into the bind-mounted /etc/resolv.conf, so removing that mount would break sibling-container name resolution (see #71). Mount options are normalised rather than appended to: any existing ro or rw is dropped and ro re-appended last, so a mount that arrives with no options at all, or with a rw following a ro, still ends up read-only. The kernel honours the last of a conflicting pair, so appending without normalising would have left both cases writable. modify_networking_mounts now reports whether it changed anything, so the spec is only rewritten to disk when a mount was actually modified. tests/runtime_invocation.rs drives a stub runtime and covers the process-tree behaviour and the paths around it: that the runtime inherits the interceptor's pid, that signalling the interceptor does not orphan it, that a runtime which cannot be exec'd is reported as such, that exit codes survive the exec, and that a signal-killed runtime is not reported as exit 255. All but the exit-code test fail against the previous implementation, which reports unix_wait_status(65280) for a SIGTERM'd runtime. --- README.md | 52 ++++++-- src/main.rs | 63 ++++++--- src/networking_mounts.rs | 110 +++++++++++++--- tests/integration.rs | 8 +- tests/runtime_invocation.rs | 246 ++++++++++++++++++++++++++++++++++++ 5 files changed, 430 insertions(+), 49 deletions(-) create mode 100644 tests/runtime_invocation.rs diff --git a/README.md b/README.md index 8cc3df7..993310c 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ Options: --oi-runtime-path Path to OCI runtime. [default: runc] --oi-readonly-networking-mounts - Mount networking files as readonly + Deprecated, and now a no-op: networking mounts are always readonly --oi-write-debug-output Write debug output --oi-debug-output-dir @@ -70,10 +70,7 @@ option, as it defaults to `runc`, the default runtime bundled with Docker. "default-runtime": "oci-interceptor", "runtimes": { "oci-interceptor": { - "path": "/usr/local/bin/oci-interceptor", - "runtimeArgs": [ - "--oi-readonly-networking-mounts" - ] + "path": "/usr/local/bin/oci-interceptor" } } } @@ -82,7 +79,40 @@ The Docker daemon must be restarted (`systemctl restart docker.service`) in orde Note that if you set `oci-interceptor` as the default runtime, you can still bypass it for a specific container by specifying `docker run --runtime=runc`. -While it is not possible to override `runtimeArgs` with a `docker run` option, you could specify multiple interceptor "runtimes" (with different flags) and switch between them using `docker run --runtime=`. +#### Passing flags: use a wrapper script, not `runtimeArgs` + +**Do not put the interceptor's flags in `runtimeArgs`.** Docker generates its own wrapper +script for any runtime whose `runtimeArgs` is non-empty, and that generated wrapper does +not `exec`: + +```sh +#!/bin/sh +/usr/local/bin/oci-interceptor --flags $@ +``` + +It therefore stays in the process tree between the containerd shim and the real runtime. +containerd invokes the runtime through go-runc, which builds commands with Go's +`exec.CommandContext`; a cancelled or timed-out call delivers SIGKILL to the direct child +only — that shell — orphaning `runc` beneath it. For a `runc delete` the task is then never +deleted and its `containerd-shim-runc-v2` never shuts down, leaking roughly 5 MiB per +occurrence until the host is rebooted. That defeats the point of the interceptor exec'ing +the runtime at all. + +If you have no flags to pass, you need no wrapper at all: name the binary itself as `path`, +as in the example above. Otherwise put the flags in a wrapper script of your own that +`exec`s, and name that script as `path`: + +```sh +#!/bin/sh +exec /usr/local/bin/oci-interceptor --oi-env HTTP_PROXY=http://proxy.example:3128 "$@" +``` + +Keep `"$@"` quoted — Docker's generated wrapper uses a bare `$@`, which word-splits and +glob-expands its arguments. An absent or empty `runtimeArgs` generates no wrapper. + +You can still run several interceptor "runtimes" with different flags and switch between +them using `docker run --runtime=`; each one needs its own wrapper script, named by +its own `path`. ## Supported Customizations @@ -100,12 +130,14 @@ users to fill the host storage volume. This can usually only be circumvented by manually creating read-only bind mounts over these paths (in which case Docker can no longer manage the container's DNS configuration) or by making the entire rootfs read-only (which severely constrains the workloads possible inside the container). -To avoid this issue, specify the `--oi-readonly-networking-mounts` flag. This modifies these mounts to be read-only, preventing writes from inside the container. +These mounts are always modified to be read-only, preventing writes from inside the container. The mounts themselves are left in place: Docker points a container on a user-defined network at its embedded DNS resolver by writing `nameserver 127.0.0.11` into the bind-mounted `/etc/resolv.conf`, so removing the mount would break resolution of sibling containers by name. Only write access is taken away. + +The `--oi-readonly-networking-mounts` flag is retained as an accepted no-op so that an existing `daemon.json` passing it keeps working; it no longer has any effect. #### Related issues - Workaround for [moby#13152](https://github.com/moby/moby/issues/41991), [moby#41991](https://github.com/moby/moby/issues/41991) (without custom bind mounts or making entire rootfs readonly) -- Optionally reverts [moby#5129](https://github.com/moby/moby/pull/5129) +- Reverts [moby#5129](https://github.com/moby/moby/pull/5129) ### Overriding environment variables @@ -127,7 +159,7 @@ Unit tests run with `cargo test`. End-to-end integration tests live in `tests/in To run the integration tests locally on a Linux host with Docker: 1. Build and install the binary: `cargo build --release && sudo install -m 0755 target/release/oci-interceptor /usr/local/bin/oci-interceptor` -2. Configure `/etc/docker/daemon.json` with the named runtimes listed in the module docs of [tests/integration.rs](tests/integration.rs). The CI workflow (`.github/workflows/CI.yml`, `integration` job) is the canonical reference for the required daemon.json. +2. Configure `/etc/docker/daemon.json` with the named runtimes listed in the module docs of [tests/integration.rs](tests/integration.rs). The CI workflow (`.github/workflows/CI.yml`, `integration` job) shows the exact set the suite expects. Note that it is a test harness, not a reference deployment: it uses `runtimeArgs` because that is the only native per-runtime flag mechanism and the flag assertions need it. For a real host, follow [Passing flags](#passing-flags-use-a-wrapper-script-not-runtimeargs) instead. 3. `sudo systemctl restart docker` 4. `OCI_INTERCEPTOR_INTEGRATION=1 cargo test --test integration -- --test-threads=1` @@ -135,7 +167,7 @@ CI runs the same suite on every push and pull request, which provides a regressi ### Debug output -Specify the `--oi-write-debug-output` flag to write original, parsed, and modified container configs to the directory specified as `--oi-debug-output-dir` (default `/var/log/oci-interceptor`). +Specify the `--oi-write-debug-output` flag to write original, parsed, and modified container configs to the directory specified as `--oi-debug-output-dir` (default `/var/log/oci-interceptor`). As with every other flag, pass these from an exec'ing wrapper script rather than `runtimeArgs` — see [Passing flags](#passing-flags-use-a-wrapper-script-not-runtimeargs). The resulting files will be named: - `_original.json` (the original config) diff --git a/src/main.rs b/src/main.rs index 819bbc8..e01a22d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,7 +8,7 @@ use clap::{ use env_vars::{EnvVar, EnvVarOverride, modify_env_vars, parse_env_var}; use networking_mounts::modify_networking_mounts; use oci_spec::runtime::Spec; -use std::{fs, io::Write, path::PathBuf, process}; +use std::{fs, io::Write, os::unix::process::CommandExt, path::PathBuf, process}; fn main() -> Result<()> { let matches = clap::Command::new(crate_name!()) @@ -26,10 +26,13 @@ fn main() -> Result<()> { .help("Path to OCI runtime."), ) .arg( + // Retained as an accepted no-op so a daemon.json still passing it keeps + // working: rejecting it would fail every container launch on any host + // upgraded before its runtime config was updated. Arg::new("readonly-networking-mounts") .long("oi-readonly-networking-mounts") .action(ArgAction::SetTrue) - .help("Mount networking files as readonly"), + .help("Deprecated, and now a no-op: networking mounts are always readonly"), ) .arg( Arg::new("write-debug-output") @@ -137,11 +140,20 @@ fn main() -> Result<()> { serde_json::to_writer_pretty(&parsed_file, &spec)?; } - // Make any enabled modifications - if matches.get_flag("readonly-networking-mounts") { - modify_networking_mounts(&mut spec); - spec_modified = true; - } + // Make any enabled modifications. + // + // Networking mounts are always made read-only. Docker bind-mounts these three + // files from outside the container's writable layer, so where XFS project quotas + // restrict that layer they are an escape hatch for filling the host volume -- see + // "Read-only networking mounts" in the README. On a shared host that affects every + // other container, which is what makes it worth enforcing unconditionally. + // + // Note this is not a barrier between containers: each gets its own copy of these + // files, so tampering only ever affects the container doing it, and a process with + // code execution can bypass them anyway (HOSTALIASES, RES_OPTIONS, LD_PRELOAD, or + // simply querying a resolver directly). Treat it as quota enforcement, with + // in-container tamper resistance as a side effect. + spec_modified |= modify_networking_mounts(&mut spec); if !env_var_overrides.is_empty() { modify_env_vars(&mut spec, env_var_overrides); spec_modified = true; @@ -176,7 +188,8 @@ fn main() -> Result<()> { .write_all(format!("{} {}\n", runtime_path, runtime_options.join(" ")).as_bytes())?; runtime_calls.flush()?; } - std::process::exit(call_oci_runtime(runtime_path, runtime_options)?); + // On success this replaces the current process, so nothing below runs. + Err(call_oci_runtime(runtime_path, runtime_options)) } /// Extracts the container bundle path from the trailing runtime options, if present. @@ -194,19 +207,29 @@ fn get_bundle_path(options: &mut [String]) -> Option { None } -/// Calls the actual OCI runtime, passing along any runtime options. -fn call_oci_runtime(runtime_path: &str, options: Vec) -> Result { - let mut child = process::Command::new(runtime_path) +/// Replaces this process with the actual OCI runtime, passing along any runtime options. +/// +/// This execs rather than spawning a child and waiting on it, so that the runtime inherits +/// our PID and we leave the process tree entirely. A wrapper that spawns and waits is +/// wrong for an OCI runtime in two ways: +/// +/// - containerd's shim invokes the runtime through go-runc, which builds commands with +/// Go's `exec.CommandContext`. On a cancelled or timed-out call that delivers SIGKILL to +/// the direct child only. With a wrapper in between, the wrapper dies and the real +/// runtime is orphaned, while the shim believes the call was cancelled. For a +/// `runc delete` that leaves the task undeleted and its shim never shut down. +/// - A runtime killed by a signal was reported to the caller as exit code -1, which +/// `process::exit` truncates to 255. Callers that distinguish signal death from a 255 +/// exit status saw the wrong thing. Exec'ing reports the real wait status. +/// +/// Returns only on failure to exec. +fn call_oci_runtime(runtime_path: &str, options: Vec) -> anyhow::Error { + let err = process::Command::new(runtime_path) .args(options.as_slice()) - .spawn() - .with_context(|| "Failed to execute underlying OCI runtime")?; - let status = child - .wait() - .with_context(|| "Failed to wait on OCI runtime process")?; - match status.code() { - Some(code) => Ok(code), - None => Ok(-1), // child process was killed by a signal - } + .exec(); + anyhow::Error::new(err).context(format!( + "Failed to execute underlying OCI runtime: {runtime_path}" + )) } #[cfg(test)] diff --git a/src/networking_mounts.rs b/src/networking_mounts.rs index 78bce6a..fcc8165 100644 --- a/src/networking_mounts.rs +++ b/src/networking_mounts.rs @@ -7,24 +7,104 @@ use oci_spec::runtime::Spec; /// - /etc/resolv.conf /// /// in the container config, making them read-only. -pub(crate) fn modify_networking_mounts(spec: &mut Spec) { - if let Some(mounts) = spec.mounts() { - let mut mounts = mounts.clone(); - for mount in mounts.iter_mut() { - match mount.destination().to_str() { - Some("/etc/hosts") | Some("/etc/hostname") | Some("/etc/resolv.conf") => { - if let Some(options) = mount.options() - && !options.contains(&"ro".into()) - { - let mut options = options.clone(); - options.push("ro".into()); - mount.set_options(Some(options)); - } +/// +/// The mounts themselves are left in place. Docker points a container on a user-defined +/// network at its embedded DNS resolver by writing `nameserver 127.0.0.11` into the +/// bind-mounted /etc/resolv.conf, so removing that mount would break resolution of +/// sibling containers by name. Only write access is taken away. +/// +/// Returns whether any mount was actually changed. +pub(crate) fn modify_networking_mounts(spec: &mut Spec) -> bool { + let Some(mounts) = spec.mounts() else { + return false; + }; + let mut mounts = mounts.clone(); + let mut changed = false; + for mount in mounts.iter_mut() { + match mount.destination().to_str() { + Some("/etc/hosts") | Some("/etc/hostname") | Some("/etc/resolv.conf") => { + // Absent options are still writable, and the kernel takes the last of a + // conflicting pair, so an existing "ro" earlier in the list does not + // survive a later "rw". Drop both and re-append "ro" so it always wins. + let current = mount.options().clone().unwrap_or_default(); + let mut wanted: Vec = current + .iter() + .filter(|option| *option != "ro" && *option != "rw") + .cloned() + .collect(); + wanted.push(String::from("ro")); + if wanted != current { + mount.set_options(Some(wanted)); + changed = true; } - Some(_) => {} - None => {} } + _ => {} } + } + if changed { spec.set_mounts(Some(mounts)); } + changed +} + +#[cfg(test)] +mod tests { + use super::modify_networking_mounts; + use oci_spec::runtime::Spec; + + fn spec_with(mounts: &str) -> Spec { + serde_json::from_str(&format!("{{\"ociVersion\":\"1.0.0\",\"mounts\":{mounts}}}")) + .expect("test spec should parse") + } + + fn options_of(spec: &Spec, destination: &str) -> Vec { + spec.mounts() + .as_ref() + .expect("spec should have mounts") + .iter() + .find(|mount| mount.destination().to_str() == Some(destination)) + .expect("mount should be present") + .options() + .clone() + .unwrap_or_default() + } + + #[test] + fn a_mount_without_options_still_becomes_readonly() { + let mut spec = spec_with("[{\"destination\":\"/etc/hosts\"}]"); + assert!(modify_networking_mounts(&mut spec)); + assert_eq!(options_of(&spec, "/etc/hosts"), vec!["ro"]); + } + + #[test] + fn a_writable_mount_keeps_its_other_options() { + let mut spec = + spec_with("[{\"destination\":\"/etc/hosts\",\"options\":[\"rbind\",\"rw\"]}]"); + assert!(modify_networking_mounts(&mut spec)); + assert_eq!(options_of(&spec, "/etc/hosts"), vec!["rbind", "ro"]); + } + + #[test] + fn a_later_rw_does_not_survive_an_earlier_ro() { + // The kernel takes the last of a conflicting pair, so this mount was writable. + let mut spec = + spec_with("[{\"destination\":\"/etc/resolv.conf\",\"options\":[\"ro\",\"rw\"]}]"); + assert!(modify_networking_mounts(&mut spec)); + assert_eq!(options_of(&spec, "/etc/resolv.conf"), vec!["ro"]); + } + + #[test] + fn an_already_readonly_mount_reports_no_change() { + let mut spec = + spec_with("[{\"destination\":\"/etc/hostname\",\"options\":[\"rbind\",\"ro\"]}]"); + assert!(!modify_networking_mounts(&mut spec)); + assert_eq!(options_of(&spec, "/etc/hostname"), vec!["rbind", "ro"]); + } + + #[test] + fn unrelated_mounts_are_left_writable() { + let mut spec = spec_with("[{\"destination\":\"/tmp\",\"options\":[\"rw\"]}]"); + assert!(!modify_networking_mounts(&mut spec)); + assert_eq!(options_of(&spec, "/tmp"), vec!["rw"]); + } } diff --git a/tests/integration.rs b/tests/integration.rs index c0a9c93..143c76f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -90,16 +90,16 @@ fn passthrough_propagates_nonzero_exit_code() { } #[test] -fn networking_mounts_writable_without_flag() { - if !check_enabled("networking_mounts_writable_without_flag") { +fn networking_mounts_readonly_without_any_flag() { + if !check_enabled("networking_mounts_readonly_without_any_flag") { return; } for target in ["/etc/hosts", "/etc/hostname", "/etc/resolv.conf"] { let opts = mount_options("oi-default", target); let opts: Vec<&str> = opts.split(',').collect(); assert!( - opts.contains(&"rw"), - "{target} expected to be rw by default, got: {opts:?}" + opts.contains(&"ro"), + "{target} expected to be ro with no flags at all, got: {opts:?}" ); } } diff --git a/tests/runtime_invocation.rs b/tests/runtime_invocation.rs new file mode 100644 index 0000000..b4c51dd --- /dev/null +++ b/tests/runtime_invocation.rs @@ -0,0 +1,246 @@ +//! Tests for how oci-interceptor hands off to the underlying OCI runtime. +//! +//! These use a stub runtime script rather than runc, so they need neither Docker nor a +//! real runtime and always run as part of `cargo test`. +//! +//! The property under test is that the interceptor *execs* the runtime rather than +//! spawning it as a child and waiting. containerd's shim invokes the runtime through +//! go-runc, which builds commands with Go's `exec.CommandContext`; a cancelled call +//! delivers SIGKILL to the direct child only. If the interceptor were still in the +//! process tree at that point, the signal would kill the interceptor and orphan the real +//! runtime underneath it. + +#![cfg(unix)] + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::os::unix::process::ExitStatusExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::thread::sleep; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + +const BIN: &str = env!("CARGO_BIN_EXE_oci-interceptor"); + +fn unique_dir(prefix: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("oi-{prefix}-{nanos}-{n}")); + fs::create_dir_all(&dir).expect("failed to create temp dir"); + dir +} + +/// Writes a stub runtime that records its own PID and then blocks, so the test can +/// inspect the process tree while a runtime call is in flight. +fn write_stub_runtime(dir: &Path, pid_file: &Path) -> PathBuf { + let path = dir.join("stub-runtime"); + fs::write( + &path, + format!( + "#!/bin/sh\necho $$ > {}\nexec sleep 30\n", + pid_file.display() + ), + ) + .expect("failed to write stub runtime"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)) + .expect("failed to chmod stub runtime"); + path +} + +fn wait_for_pid_file(pid_file: &Path) -> u32 { + let deadline = Instant::now() + Duration::from_secs(10); + while Instant::now() < deadline { + if let Ok(contents) = fs::read_to_string(pid_file) + && let Ok(pid) = contents.trim().parse::() + { + return pid; + } + sleep(Duration::from_millis(20)); + } + panic!( + "stub runtime never recorded its PID at {}", + pid_file.display() + ); +} + +fn process_alive(pid: u32) -> bool { + // `kill -0` rather than a /proc lookup: this module is cfg(unix), and on a Unix + // without procfs the path test reports every process as dead, which would turn the + // orphan assertion below into a green test that checks nothing. Panic rather than + // defaulting to false, for the same reason. + Command::new("kill") + .args(["-0", &pid.to_string()]) + // kill(1) writes "No such process" to stderr for a dead pid, which is the normal + // result here and only clutters the test output. + .stderr(Stdio::null()) + .status() + .expect("failed to run kill -0") + .success() +} + +/// The interceptor must exec the runtime, leaving the runtime with the interceptor's own +/// PID. If it spawned and waited instead, the runtime would report a different PID. +#[test] +fn execs_runtime_in_place_rather_than_spawning_a_child() { + let dir = unique_dir("exec"); + let pid_file = dir.join("runtime.pid"); + let stub = write_stub_runtime(&dir, &pid_file); + + let mut child = Command::new(BIN) + .arg("--oi-runtime-path") + .arg(&stub) + .args(["state", "some-container-id"]) + .spawn() + .expect("failed to invoke oci-interceptor"); + + let runtime_pid = wait_for_pid_file(&pid_file); + let interceptor_pid = child.id(); + + let _ = child.kill(); + let _ = child.wait(); + // When this assertion is about to fail the runtime is a separate process, so killing + // the interceptor leaves it behind. Clean up before reporting. + if runtime_pid != interceptor_pid { + force_kill(runtime_pid); + } + let _ = fs::remove_dir_all(&dir); + + assert_eq!( + runtime_pid, interceptor_pid, + "runtime ran as PID {runtime_pid} but the interceptor was PID {interceptor_pid}; \ + the interceptor spawned the runtime as a child instead of exec'ing it, which \ + orphans the runtime when the caller signals the interceptor" + ); +} + +/// Signalling the interceptor must take the runtime down with it. A spawn-and-wait +/// wrapper leaves the runtime running after the wrapper is killed. +#[test] +fn signalling_the_interceptor_does_not_orphan_the_runtime() { + let dir = unique_dir("orphan"); + let pid_file = dir.join("runtime.pid"); + let stub = write_stub_runtime(&dir, &pid_file); + + let mut child = Command::new(BIN) + .arg("--oi-runtime-path") + .arg(&stub) + .args(["delete", "some-container-id"]) + .spawn() + .expect("failed to invoke oci-interceptor"); + + let runtime_pid = wait_for_pid_file(&pid_file); + + // SIGKILL to the process the caller started, exactly as go-runc's + // exec.CommandContext does when a runtime call is cancelled. + child.kill().expect("failed to kill oci-interceptor"); + child.wait().expect("failed to reap oci-interceptor"); + + // Give the kernel a moment to tear the process down before checking. + let deadline = Instant::now() + Duration::from_secs(5); + while process_alive(runtime_pid) && Instant::now() < deadline { + sleep(Duration::from_millis(20)); + } + let orphaned = process_alive(runtime_pid); + + if orphaned { + force_kill(runtime_pid); + } + let _ = fs::remove_dir_all(&dir); + + assert!( + !orphaned, + "runtime PID {runtime_pid} survived the interceptor being killed; a cancelled \ + runtime call would leave an orphaned runc behind, and its shim never shut down" + ); +} + +/// Best-effort SIGKILL so a failing test does not leak the stub runtime. +fn force_kill(pid: u32) { + let _ = Command::new("kill").arg("-9").arg(pid.to_string()).status(); +} + +/// Writes a stub runtime that exits immediately in a given way. +fn write_stub(dir: &Path, name: &str, body: &str) -> PathBuf { + let path = dir.join(name); + fs::write( + &path, + format!( + "#!/bin/sh +{body} +" + ), + ) + .expect("failed to write stub"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("failed to chmod stub"); + path +} + +/// A runtime that cannot be exec'd must surface as a non-zero exit naming the path, rather +/// than being mistaken for a runtime that ran and failed. +#[test] +fn failure_to_exec_the_runtime_is_reported() { + let out = Command::new(BIN) + .arg("--oi-runtime-path") + .arg("/nonexistent/runc") + .args(["state", "some-container-id"]) + .output() + .expect("failed to invoke oci-interceptor"); + + assert!(!out.status.success(), "expected a non-zero exit"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("/nonexistent/runc"), + "stderr should name the runtime it could not execute, got: {stderr}" + ); +} + +/// A runtime killed by a signal must be reported as signal death. Spawning and waiting +/// reported it as exit code -1, which process::exit truncates to 255 — indistinguishable +/// from a runtime that genuinely exited 255. +#[test] +fn a_signal_killed_runtime_is_not_reported_as_exit_255() { + let dir = unique_dir("signal"); + let stub = write_stub(&dir, "suicidal-runtime", "kill -TERM $$"); + + let status = Command::new(BIN) + .arg("--oi-runtime-path") + .arg(&stub) + .args(["state", "some-container-id"]) + .status() + .expect("failed to invoke oci-interceptor"); + + let _ = fs::remove_dir_all(&dir); + assert_eq!( + status.signal(), + Some(15), + "expected death by SIGTERM, got {status:?}" + ); + assert_eq!( + status.code(), + None, + "a signal-killed runtime must not present as an ordinary exit code" + ); +} + +/// Exit codes must survive the exec. Covered by the integration suite too, but that is +/// gated behind OCI_INTERCEPTOR_INTEGRATION and silently passes without Docker. +#[test] +fn runtime_exit_code_propagates() { + let dir = unique_dir("exitcode"); + let stub = write_stub(&dir, "exit42-runtime", "exit 42"); + + let status = Command::new(BIN) + .arg("--oi-runtime-path") + .arg(&stub) + .args(["state", "some-container-id"]) + .status() + .expect("failed to invoke oci-interceptor"); + + let _ = fs::remove_dir_all(&dir); + assert_eq!(status.code(), Some(42)); +} From 37b85f3f07fdd2f421f3091fd40c5d7b8f0496e0 Mon Sep 17 00:00:00 2001 From: Ivan Liang Date: Sun, 6 Sep 2026 14:36:21 -0400 Subject: [PATCH 2/2] release: v0.3.0 --- CHANGELOG.md | 8 ++++++++ Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d76778..366cf2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## v0.3.0 + +- The OCI runtime is now `exec`'d rather than spawned as a child process. containerd's shim invokes the runtime through go-runc, which builds commands with Go's `exec.CommandContext`; a cancelled or timed-out call delivers SIGKILL to the direct child only, so the interceptor died and the real runtime was orphaned beneath it. For a `runc delete` that left the task undeleted and its `containerd-shim-runc-v2` resident indefinitely, at roughly 5 MiB each until the host was rebooted. +- A runtime killed by a signal is now reported as signal death. Previously it was mapped to exit code -1, which `process::exit` truncates to 255 — indistinguishable from a runtime that genuinely exited 255. +- **Deployment change required for the above to take effect.** Docker generates its own non-`exec` `/bin/sh` wrapper for any runtime whose `runtimeArgs` is non-empty, which re-inserts exactly the process layer this release removes. Pass the interceptor's flags from a wrapper script of your own that `exec`s, name that script as the runtime's `path`, and leave `runtimeArgs` unset. See "Passing flags" in the README. +- Networking mounts (`/etc/hosts`, `/etc/hostname`, `/etc/resolv.conf`) are now always made read-only. This is a behaviour change for anyone who did not previously pass `--oi-readonly-networking-mounts`, and there is no opt-out. The flag is retained as an accepted no-op so an existing `daemon.json` that passes it keeps working. The mounts themselves are left in place, so Docker's embedded DNS resolver still works for sibling-container name resolution. Mount options are normalised rather than appended to: a mount that arrives with no options at all, or with a `rw` following a `ro`, now ends up read-only too, because `ro` is always placed last and that is the flag the kernel honours. +- The container spec is only written back to disk when a modification actually changed it. + ## v0.2.3 - Switched CI from `dtolnay/rust-toolchain` to `actions-rust-lang/setup-rust-toolchain`. CI builds now compile with `RUSTFLAGS=-D warnings` and surface rustc/rustfmt warnings as PR annotations; release builds are exempt so a new stable lint cannot break a tagged release. Dependabot now tracks GitHub Actions pins. diff --git a/Cargo.lock b/Cargo.lock index f3514e0..00bca4b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -236,7 +236,7 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "oci-interceptor" -version = "0.2.3" +version = "0.3.0" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index f95d8db..478096c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ license = "MIT OR Apache-2.0" name = "oci-interceptor" publish = false repository = "https://github.com/picoCTF/oci-interceptor" -version = "0.2.3" +version = "0.3.0" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html