From e1856d30ec9a4a37d29bc37a2ce3bd06086cc3b6 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 5 Jun 2026 01:33:51 +0000 Subject: [PATCH] feat(worker): drop per-job podman to non-root when worker is root (protect /proc env) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the docker worker runs as root (the default for compose/helm workers), start the per-job podman via `runuser` dropped to a non-root uid so it runs rootless. This keeps the worker's secrets in /proc//environ (DATABASE_URL etc.) root-owned and therefore unreadable by a `docker run --pid=host` container, and makes a container escape land unprivileged — without the fragile namespace confinement (no unshare/mask/pid-ns, which conflicted with podman system service). A non-root worker can't drop further (podman runs rootless as itself; a --pid=host container shares its uid), so run the docker worker as root for the /proc protection. The per-job podman + its containers are spawned in a new process group so teardown and the storage-cap monitor kill the whole tree (podman is a runuser grandchild). This does NOT confine the container's filesystem view — a `# docker` script can still bind-mount worker-visible paths — so docker-capable workers remain a trusted-tenant capability (documented). Reverses the earlier `user: 1000` advice: keep the worker root; windmill drops podman itself. e2e verified (bare-metal, worker as root): stock docker job runs (dropped rootless podman); a uid-1000 --pid=host container reading the root worker's /proc/environ -> 'Permission denied' (DATABASE_URL/secret protected). Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 16 +++-- backend/Cargo.lock | 1 + backend/windmill-worker/Cargo.toml | 1 + backend/windmill-worker/src/bash_executor.rs | 67 ++++++++++++++------ docker-compose.yml | 18 +++--- 5 files changed, 70 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 7c2429ee14..39312bbb83 100644 --- a/README.md +++ b/README.md @@ -228,13 +228,15 @@ Go to http://localhost - default credentials: `admin@windmill.dev` / `changeme` > all your default workers use a `*-full` image, route docker jobs to a dedicated > `*-full` group with `WORKER_TAGS=docker` rather than relying on the default tag. > -> **Security:** the per-job podman daemon runs outside the worker's nsjail sandbox (only -> its socket is exposed to the script), so it does **not** extend nsjail's filesystem -> isolation to containers — a `# docker` script can bind-mount worker-visible paths (e.g. -> other concurrent job dirs, caches) via the Docker API. Rootless uid-mapping limits this -> to the unprivileged worker user (a big improvement over privileged dind), but treat -> docker-capable workers as a trusted-tenant capability and prefer dedicated/per-workspace -> docker workers on shared multi-tenant fleets. +> **Security:** keep the docker worker running as **root** (the default — don't set a +> non-root `user`): windmill drops the per-job podman to a non-root uid itself, so +> containers are rootless **and** the worker's own secrets in `/proc` (`DATABASE_URL` +> etc.) stay root-owned — unreadable by a `docker run --pid=host` container. What this +> does **not** do is confine the container's *filesystem* view: the daemon runs outside +> the job's nsjail sandbox, so a `# docker` script can still `docker run -v ` +> to reach other concurrent job dirs / caches. So treat docker-capable workers as a +> **trusted-tenant** capability (a big improvement over privileged dind, but not a full +> sandbox) and prefer dedicated/per-workspace docker workers on shared multi-tenant fleets. **Using an external database**: Set `DATABASE_URL` in `.env` to point to your managed Postgres (AWS RDS, GCP Cloud SQL, Azure, Neon, etc.) and set db replicas to 0. diff --git a/backend/Cargo.lock b/backend/Cargo.lock index c9e09a4963..99d0bbc35c 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15620,6 +15620,7 @@ dependencies = [ "itertools 0.14.0", "jsonwebtoken 8.3.0", "lazy_static", + "libc", "libffi-sys", "libloading", "mappable-rc", diff --git a/backend/windmill-worker/Cargo.toml b/backend/windmill-worker/Cargo.toml index 2e75ac3632..b9edbef66f 100644 --- a/backend/windmill-worker/Cargo.toml +++ b/backend/windmill-worker/Cargo.toml @@ -115,6 +115,7 @@ pem = { workspace = true, optional = true } rsa = { workspace = true, optional = true } urlencoding.workspace = true nix.workspace = true +libc = "0.2" bytes.workspace = true reqwest.workspace = true reqwest-middleware.workspace = true diff --git a/backend/windmill-worker/src/bash_executor.rs b/backend/windmill-worker/src/bash_executor.rs index c95df02993..1ffb1360e8 100644 --- a/backend/windmill-worker/src/bash_executor.rs +++ b/backend/windmill-worker/src/bash_executor.rs @@ -497,6 +497,9 @@ fn teardown_per_job_podman(dir: String, service: Option) { .stderr(std::process::Stdio::null()) .status(); if let Some(mut child) = service { + // Kill the whole process group: when the worker is root the daemon runs as a + // `runuser` grandchild, so killing only the direct child would leak it. + unsafe { libc::kill(-(child.id() as i32), libc::SIGKILL) }; let _ = child.kill(); let _ = child.wait(); } @@ -608,10 +611,8 @@ fn spawn_docker_storage_monitor( ) .await; if let Some(pid) = service_pid { - let _ = std::process::Command::new("kill") - .arg("-9") - .arg(pid.to_string()) - .status(); + // Kill the process group (podman may be a runuser grandchild). + unsafe { libc::kill(-(pid as i32), libc::SIGKILL) }; } let _ = std::process::Command::new("podman") .args([ @@ -637,16 +638,17 @@ fn spawn_docker_storage_monitor( // config (CONTAINERS_CONF etc.) but overrides storage so it is job-scoped. A // background monitor enforces a soft size cap on the image store. // -// SECURITY: the daemon runs OUTSIDE the nsjail (only its socket is mounted in), so -// it does NOT extend nsjail's filesystem isolation to docker containers. A `# docker` +// SECURITY: the daemon runs OUTSIDE the nsjail (only its socket is mounted in), so it +// does NOT extend nsjail's filesystem isolation to docker containers. A `# docker` // script controls this daemon over the Docker API and can bind-mount any path the -// worker user can read (e.g. `docker run -v /tmp/windmill/...`), reaching other -// concurrent job dirs / caches that nsjail deliberately hid. Rootless uid-mapping -// caps the blast radius to the unprivileged worker user (no root-owned secrets), and -// this is a large improvement over the privileged dind it replaces — but the -// per-job-podman path is NOT a full filesystem sandbox the way pure nsjail is. Treat -// docker-capable workers as a trusted-tenant capability; on shared multi-tenant -// fleets prefer per-workspace/dedicated docker workers so this stays intra-tenant. +// daemon user can read (e.g. `docker run -v /tmp/windmill/...`), reaching other +// concurrent job dirs / caches that nsjail deliberately hid — so the per-job-podman +// path is NOT a full filesystem sandbox like pure nsjail, and docker-capable workers +// stay a TRUSTED-TENANT capability (prefer dedicated/per-workspace docker workers on +// shared fleets). What IS protected, when the worker runs as root (the default) and +// the daemon is dropped to a non-root uid here: the worker's secrets in +// `/proc//environ` (DATABASE_URL etc.) stay root-owned and unreadable by the +// uid-1000 container even via `docker run --pid=host`, and an escape is unprivileged. #[cfg(feature = "dind")] async fn start_per_job_podman( job_dir: &str, @@ -654,14 +656,39 @@ async fn start_per_job_podman( workspace_id: &str, conn: &Connection, ) -> Result { + use std::os::unix::process::CommandExt; let dir = format!("{job_dir}/podman"); - tokio::fs::create_dir_all(&dir).await.map_err(to_anyhow)?; + let xdg = format!("{dir}/xdg"); + tokio::fs::create_dir_all(&xdg).await.map_err(to_anyhow)?; let host_sock = format!("{dir}/podman.sock"); let _ = tokio::fs::remove_file(&host_sock).await; let storage = format!("{dir}/storage"); let runroot = format!("{dir}/runroot"); - let service = std::process::Command::new("podman") - .args([ + let sock_url = format!("unix://{host_sock}"); + + // If the worker is root (the default for docker workers), drop the per-job podman + // to a non-root uid via `runuser` so it runs ROOTLESS. This keeps the worker's + // secrets in `/proc//environ` (DATABASE_URL etc.) root-owned and therefore + // unreadable by the uid-1000 container even via `docker run --pid=host`, and makes + // a container escape land as an unprivileged user. NB: it does NOT confine the + // daemon's filesystem view — a script can still `docker run -v ` to + // reach other job dirs / caches, so docker-capable workers remain trusted-tenant. + // A non-root worker can't drop further (it runs podman rootless as itself, and a + // `--pid=host` container shares its uid) — run the docker worker as root for the + // /proc env protection. `process_group(0)` lets teardown/monitor kill the whole + // tree (with `runuser`, podman is a grandchild, not the direct child). + let mut cmd = if unsafe { libc::geteuid() } == 0 { + let launch = format!( + "set -e\nchown -R 1000:1000 '{dir}'\nexec runuser -u \"$(id -nu 1000)\" -- env \ + HOME='{dir}' XDG_RUNTIME_DIR='{xdg}' podman --root '{storage}' --runroot '{runroot}' \ + system service --time=0 '{sock_url}'\n" + ); + let mut c = std::process::Command::new("bash"); + c.args(["-c", &launch]); + c + } else { + let mut c = std::process::Command::new("podman"); + c.args([ "--root", &storage, "--runroot", @@ -669,8 +696,12 @@ async fn start_per_job_podman( "system", "service", "--time=0", - &format!("unix://{host_sock}"), - ]) + &sock_url, + ]); + c + }; + let service = cmd + .process_group(0) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .spawn() diff --git a/docker-compose.yml b/docker-compose.yml index ec426db8c0..efbc9a29f0 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -65,13 +65,16 @@ services: # host has it): # devices: # - /dev/fuse - # Optional hardening: run rootless (a container escape lands as an unprivileged - # user) by uncommenting `user` below and `- HOME=/tmp` under environment — the - # named cache volumes must then be writable by uid 1000. To use an external/host - # Docker daemon instead (legacy): set DOCKER_HOST or mount /var/run/docker.sock. - # Cap per-job image storage via the `docker_image_storage_size_mb` instance - # setting (default 8GB; the worker aborts a job whose images exceed it). - # user: "1000:1000" + # Keep this worker running as ROOT (the default — do NOT set `user: 1000`): + # windmill itself drops the per-job podman to a non-root uid, so containers are + # rootless AND the worker's own secrets in /proc (DATABASE_URL etc.) stay + # root-owned, unreadable by a `docker run --pid=host` container. (NB: this does + # not confine the container's filesystem view — a `# docker` script can still + # `docker run -v` worker-visible paths, so treat docker workers as trusted-tenant + # / dedicate them on shared fleets.) Cap per-job image storage via the + # `docker_image_storage_size_mb` instance setting (default 8GB). To use an + # external/host Docker daemon instead (legacy): set DOCKER_HOST or mount + # /var/run/docker.sock. pull_policy: always deploy: replicas: 3 @@ -88,7 +91,6 @@ services: - DATABASE_URL=${DATABASE_URL} - MODE=worker - WORKER_GROUP=default - # - HOME=/tmp # required when running as non-root (see `user` above) - FAVOR_UNSHARE_PID=true depends_on: db: