From faf28332d521c409d737533042922117cb105b1c Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Tue, 22 Sep 2026 21:07:46 -0400 Subject: [PATCH 1/5] feat!: listen on Unix socket only and bring TypeScript to parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Access control in this proxy is built on socket ownership: a caller is authorised because it can connect(2) to a Unix socket whose filesystem permissions the operator controls. The TCP listener had no equivalent — no peer uid/gid, so anything able to reach the port was implicitly trusted. --listen-tcp defaulted to 127.0.0.1:2375 and the compose setup bound 0.0.0.0:2375, so the weaker path was the one exercised by default. The outbound direction already enforced this rule, rejecting tcp:// for --docker-host on exactly these grounds. The inbound listener never got the same treatment. Removes --listen-tcp from all three implementations and adds Unix socket listening to TypeScript, which was TCP-only and had no --listen-socket at all, in violation of the equal-peers rule. TypeScript now supports the same fd://3 socket activation and stale socket cleanup as Go and Rust. A bind failure is now fatal rather than logged. With a single listener a running-but-unbound process is never useful; previously the error was logged and the process stayed up serving nothing. Migrates the integration suites from tcp://proxy:2375 to a shared-volume Unix socket. The helpers move from wget to curl --unix-socket, since busybox wget cannot address a Unix socket, and normalise connection failures to a synthetic 000 so that a dead proxy reports a failed assertion instead of aborting the run under set -e. In docker-compose.sock.yml the proxies now create their own listening sockets in the shared volume as non-root, which raced the fixture's chmod: depends_on service_started only means the container process exists. The fixture now chmods before its slow apk fetch and exposes a healthcheck, and the proxies gate on service_healthy. BREAKING CHANGE: --listen-tcp is removed. Deployments passing it, or connecting via DOCKER_HOST=tcp://, must switch to a mounted Unix socket. The flag is removed outright rather than deprecated, so Go and Rust exit on the unknown argument instead of silently listening somewhere the caller does not expect. --- Makefile | 7 +-- README.md | 25 ++++++---- deploy/docker-compose.sock.yml | 31 +++++++----- deploy/docker-compose.yml | 15 ++++-- deploy/test-sock.sh | 58 +++++++++++++++------- deploy/test.sh | 59 +++++++++++----------- go/main.go | 42 +++++++--------- rs/src/main.rs | 90 +++++++++------------------------- ts/src/flags.test.ts | 43 ++++++++++++---- ts/src/flags.ts | 50 ++++++++++++++----- ts/src/index.ts | 37 ++++++++++++-- 11 files changed, 265 insertions(+), 192 deletions(-) diff --git a/Makefile b/Makefile index 1a52189..a103575 100644 --- a/Makefile +++ b/Makefile @@ -107,9 +107,10 @@ test-integration-rs: test-integration-ts: $(MAKE) test-integration IMPL=ts -# Unix-socket provisioning tests. The proxy only connects to the Docker -# daemon over a Unix socket — TCP would bypass user/group socket ownership, -# which is the security model this target exercises. Not run in CI (uses +# Unix-socket provisioning tests. The proxy both listens and connects over +# Unix sockets only — TCP would bypass user/group socket ownership, which is +# the security model this target exercises: proxy-granted is in the socket's +# group and succeeds, proxy-denied is not and gets 403. Not run in CI (uses # group-restricted socket setup); run locally per IMPL. test-integration-sock: IMPL=$(IMPL) docker compose -f deploy/docker-compose.sock.yml down --remove-orphans -v 2>/dev/null; \ diff --git a/README.md b/README.md index 73ce79c..73c0305 100644 --- a/README.md +++ b/README.md @@ -212,20 +212,26 @@ docker pull attacker/malware:latest # denied: image not in allowlist | Flag | Default | Description | |------|---------|-------------| -| `--listen-socket` | `/var/run/docker-socket-policy.sock` | Unix socket (or `fd://3` for systemd). **Go/Rust only** | -| `--listen-tcp` | `127.0.0.1:2375` | TCP listen address | +| `--listen-socket` | `/var/run/docker-socket-policy.sock` | Unix socket to listen on (or `fd://3` for systemd) | | `--docker-host` | `/var/run/docker.sock` | Docker daemon socket path (Unix socket only) | | `--config-dir` | `/etc/docker-socket-policy/services` | Policy config directory | | `--log-file` | `/var/log/docker-socket-policy.log` | Audit log path | | `--readonly` | `false` | Enable read-only mode | -> **Unix socket security boundary**: Go and Rust support `--listen-socket` for -> binding to a Unix socket, enabling socket-level access control via file -> permissions and Unix groups. TypeScript does not implement `--listen-socket` -> and listens on TCP only (`--listen-tcp`). All three implementations connect -> to the Docker daemon over Unix sockets exclusively; they reject `tcp://` and -> `http://` schemes for `--docker-host`. TCP connections would bypass socket -> ownership-based access control, breaking the security model. +> **Unix socket security boundary**: the proxy listens on a Unix socket only, +> in all three implementations. Access control is the file permissions and Unix +> group on that socket — a caller is authorised because it can `connect(2)` to +> it. A TCP listener carries no peer identity, so anything able to reach the +> port would be implicitly trusted; there is no `--listen-tcp`. +> +> The same rule applies outbound: all three implementations connect to the +> Docker daemon over Unix sockets exclusively and reject `tcp://` and `http://` +> schemes for `--docker-host`. +> +> To grant a service access, place its container user in the group that owns +> the socket and bind-mount the socket in. To revoke it, remove the group +> membership. If the proxy cannot reach the daemon socket because of its own +> group permissions, requests surface as `403`. ### Systemd Socket Activation @@ -235,7 +241,6 @@ docker pull attacker/malware:latest # denied: image not in allowlist ListenStream=/var/run/docker-socket-policy.sock SocketMode=0660 SocketGroup=builders -ListenStream=127.0.0.1:2375 ``` **`docker-socket-policy.service`**: diff --git a/deploy/docker-compose.sock.yml b/deploy/docker-compose.sock.yml index fadf433..7c84262 100644 --- a/deploy/docker-compose.sock.yml +++ b/deploy/docker-compose.sock.yml @@ -10,6 +10,11 @@ services: - sh - -c - | + # chmod first, before the slow apk fetch: the proxies bind their own + # listening sockets in this directory as non-root, and anything they + # attempt before this runs fails with EACCES. The access control under + # test is the mode on docker.sock below, not the mode on the directory. + chmod 0777 /sock apk add --no-cache socat addgroup -g 2001 dockertest socat UNIX-LISTEN:/sock/docker.sock,unlink-early,fork,group=dockertest,perm=660 \ @@ -17,6 +22,13 @@ services: volumes: - /var/run/docker.sock:/var/run/docker.sock - sock-data:/sock + # Gates the proxies below. service_started would only mean "the container + # process exists", which was racing the chmod and the socat bind above. + healthcheck: + test: ["CMD", "sh", "-c", "test -S /sock/docker.sock"] + interval: 1s + timeout: 2s + retries: 60 proxy-granted: build: @@ -26,16 +38,13 @@ services: user: 65532:2001 depends_on: sock-perms: - condition: service_started - ports: - - "12376:2375" + condition: service_healthy volumes: - ./config:/etc/docker-socket-policy/services:ro - sock-data:/sock command: - --docker-host=/sock/docker.sock - - --listen-tcp=0.0.0.0:2375 - - --listen-socket=/tmp/docker-socket-policy.sock + - --listen-socket=/sock/granted.sock - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -47,16 +56,13 @@ services: user: 65532:3001 depends_on: sock-perms: - condition: service_started - ports: - - "12377:2375" + condition: service_healthy volumes: - ./config:/etc/docker-socket-policy/services:ro - sock-data:/sock command: - --docker-host=/sock/docker.sock - - --listen-tcp=0.0.0.0:2375 - - --listen-socket=/tmp/docker-socket-policy.sock + - --listen-socket=/sock/denied.sock - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -66,8 +72,9 @@ services: - proxy-granted - proxy-denied environment: - PROXY_GRANTED: http://proxy-granted:2375 - PROXY_DENIED: http://proxy-denied:2375 + PROXY_GRANTED_SOCK: /sock/granted.sock + PROXY_DENIED_SOCK: /sock/denied.sock volumes: - ./test-sock.sh:/test-sock.sh:ro + - sock-data:/sock entrypoint: ["/bin/sh", "/test-sock.sh"] diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 9dc9392..f2b5555 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -6,15 +6,13 @@ services: context: ../${IMPL:-go} dockerfile: Dockerfile user: "0:0" - ports: - - "12375:2375" volumes: - /var/run/docker.sock:/var/run/docker.sock - ./config:/etc/docker-socket-policy/services:ro + - proxy-sock:/sock command: - --docker-host=/var/run/docker.sock - - --listen-tcp=0.0.0.0:2375 - - --listen-socket=/tmp/docker-socket-policy.sock + - --listen-socket=/sock/proxy.sock - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -23,7 +21,14 @@ services: depends_on: - proxy environment: - DOCKER_HOST: tcp://proxy:2375 + PROXY_SOCK: /sock/proxy.sock volumes: - ./test.sh:/test.sh:ro + - proxy-sock:/sock entrypoint: ["/bin/sh", "/test.sh"] + +# The proxy is reachable only over this shared Unix socket: there is no +# published port, because filesystem ownership on the socket is the +# access-control boundary the proxy enforces. +volumes: + proxy-sock: diff --git a/deploy/test-sock.sh b/deploy/test-sock.sh index cacdd0a..e96012f 100755 --- a/deploy/test-sock.sh +++ b/deploy/test-sock.sh @@ -8,14 +8,35 @@ set -e PASS=0 FAIL=0 -GRANTED="${PROXY_GRANTED:-http://proxy-granted:2375}" -DENIED="${PROXY_DENIED:-http://proxy-denied:2375}" +# Each proxy listens on its own Unix socket in the shared volume. The host part +# of the URL is ignored when curl is given --unix-socket, but must still parse. +GRANTED_SOCK="${PROXY_GRANTED_SOCK:-/sock/granted.sock}" +DENIED_SOCK="${PROXY_DENIED_SOCK:-/sock/denied.sock}" +URL="http://localhost" + +# busybox wget cannot speak to a Unix socket, so the helpers below need curl. +if ! command -v curl >/dev/null 2>&1; then + apk add --no-cache curl >/dev/null 2>&1 || { + echo "ERROR: curl is required to talk to the proxies over Unix sockets" + exit 1 + } +fi + +# Both helpers take the target socket as their first argument, since this +# suite talks to two proxies with deliberately different socket permissions. +# curl exits non-zero when it cannot connect at all, which under `set -e` would +# abort the run instead of reporting a failed assertion, so connection failures +# are normalised to the synthetic status 000. This matters more here than in +# test.sh: the wait loops below poll sockets that do not exist yet. get_status() { - wget -qO /dev/null -S "$1" 2>&1 | grep -o 'HTTP/[0-9.]* [0-9]*' | tail -1 | awk '{print $2}' + out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$1" "$2" 2>/dev/null) || out="000" + echo "$out" } post_json() { - wget -qO /dev/null -S --post-data="$1" --header="Content-Type: application/json" "$2" 2>&1 | grep -o 'HTTP/[0-9.]* [0-9]*' | tail -1 | awk '{print $2}' + out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$1" \ + -X POST -H "Content-Type: application/json" -d "$2" "$3" 2>/dev/null) || out="000" + echo "$out" } check() { @@ -40,10 +61,10 @@ echo "" # ─── Wait for proxies to be ready ────────────────────── # proxy-granted needs the socat socket to be ready; poll _ping until 200 -echo "Waiting for proxy-granted at $GRANTED..." +echo "Waiting for proxy-granted at $GRANTED_SOCK..." i=0 while [ $i -lt 30 ]; do - S=$(get_status "$GRANTED/_ping") + S=$(get_status "$GRANTED_SOCK" "$URL/_ping") if [ "$S" = "200" ]; then echo "proxy-granted ready." break @@ -57,12 +78,13 @@ if [ $i -eq 30 ]; then echo "WARNING: proxy-granted not ready after 30s" fi -# proxy-denied: just verify TCP port is open (will always return 403 on requests) -host="${DENIED#http://}" -echo "Waiting for proxy-denied at $DENIED..." +# proxy-denied: it listens fine but cannot reach the Docker socket, so it can +# never return 200. Wait for the listening socket itself to accept a request, +# whatever status that request comes back with. +echo "Waiting for proxy-denied at $DENIED_SOCK..." i=0 while [ $i -lt 15 ]; do - if nc -z "${host%:*}" "${host#*:}" 2>/dev/null; then + if [ "$(get_status "$DENIED_SOCK" "$URL/_ping")" != "000" ]; then echo "proxy-denied ready." break fi @@ -80,17 +102,17 @@ echo "" echo "--- proxy-granted (GID 2001, has group access) ---" -S=$(get_status "$GRANTED/_ping") +S=$(get_status "$GRANTED_SOCK" "$URL/_ping") check "GET /_ping -> 200" "200" "$S" -S=$(get_status "$GRANTED/version") +S=$(get_status "$GRANTED_SOCK" "$URL/version") check "GET /version -> 200" "200" "$S" -S=$(get_status "$GRANTED/containers/json") +S=$(get_status "$GRANTED_SOCK" "$URL/containers/json") check "GET /containers/json -> 200" "200" "$S" # Allowed image create passes through to Docker (daemon returns 404, not 403) -S=$(post_json '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/config.yml"]}' "$GRANTED/containers/create") +S=$(post_json "$GRANTED_SOCK" '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/config.yml"]}' "$URL/containers/create") if [ "$S" = "201" ] || [ "$S" = "404" ]; then echo " PASS: create container -> $S (not 403)" PASS=$((PASS+1)) @@ -106,16 +128,16 @@ echo "--- proxy-denied (GID 3001, no group access) ---" # The proxy starts and listens, but cannot connect to the Docker socket. # Permission denied on the Unix socket returns 403 Forbidden. -S=$(get_status "$DENIED/_ping") +S=$(get_status "$DENIED_SOCK" "$URL/_ping") check "GET /_ping -> 403 (permission denied on socket)" "403" "$S" -S=$(get_status "$DENIED/version") +S=$(get_status "$DENIED_SOCK" "$URL/version") check "GET /version -> 403" "403" "$S" -S=$(get_status "$DENIED/containers/json") +S=$(get_status "$DENIED_SOCK" "$URL/containers/json") check "GET /containers/json -> 403" "403" "$S" -S=$(post_json '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/config.yml"]}' "$DENIED/containers/create") +S=$(post_json "$DENIED_SOCK" '{"Image":"chainsafe/lodestar:beacon","Cmd":["--rcConfig","/data/config.yml"]}' "$URL/containers/create") check "POST /containers/create -> 403" "403" "$S" # ─── Summary ────────────────────────────────────────── diff --git a/deploy/test.sh b/deploy/test.sh index 510c658..1483608 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -8,33 +8,26 @@ set -e PASS=0 FAIL=0 -PROXY="${DOCKER_HOST:-tcp://proxy:2375}" -# Strip tcp:// prefix for HTTP clients -case "$PROXY" in - tcp://*) PROXY="http://${PROXY#tcp://}" ;; - unix://*) echo "ERROR: unix:// DOCKER_HOST not supported for HTTP tests"; exit 1 ;; -esac - -# Wait for proxy to become available -echo "Waiting for proxy at ${PROXY}..." -i=0 -while [ $i -lt 30 ]; do - if wget -qO- "$PROXY/_ping" >/dev/null 2>&1; then - echo "Proxy ready." - break - fi - printf "." - sleep 1 - i=$((i + 1)) -done -if [ $i -eq 30 ]; then - echo "" - echo "ERROR: Proxy failed to respond within 30s" - exit 1 + +# The proxy listens on a Unix socket only, shared with this container through +# a volume. The host part of the URL is ignored when curl is given +# --unix-socket, but it still has to be present for the URL to parse. +PROXY_SOCK="${PROXY_SOCK:-/sock/proxy.sock}" +PROXY="http://localhost" + +# busybox wget cannot speak to a Unix socket, so the helpers below need curl. +if ! command -v curl >/dev/null 2>&1; then + apk add --no-cache curl >/dev/null 2>&1 || { + echo "ERROR: curl is required to talk to the proxy over a Unix socket" + exit 1 + } fi -echo "" + +# Wait for the proxy to create and serve its socket. +echo "Waiting for proxy at ${PROXY_SOCK}..." +i=0 while [ $i -lt 30 ]; do - if wget -qO- "$PROXY/_ping" >/dev/null 2>&1; then + if curl -sf --unix-socket "$PROXY_SOCK" "$PROXY/_ping" >/dev/null 2>&1; then echo "Proxy ready." break fi @@ -49,15 +42,23 @@ if [ $i -eq 30 ]; then fi echo "" -# Helpers: extract HTTP status code from wget --server-response output +# Helpers: report the HTTP status code of a request sent over the Unix socket. +# curl exits non-zero when it cannot connect at all, which under `set -e` would +# abort the run instead of reporting a failed assertion, so connection failures +# are normalised to the synthetic status 000. get_status() { - wget -qO /dev/null -S "$1" 2>&1 | grep -o 'HTTP/[0-9.]* [0-9]*' | tail -1 | awk '{print $2}' + out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" "$1" 2>/dev/null) || out="000" + echo "$out" } post_json() { - wget -qO /dev/null -S --post-data="$1" --header="Content-Type: application/json" "$2" 2>&1 | grep -o 'HTTP/[0-9.]* [0-9]*' | tail -1 | awk '{print $2}' + out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "$1" "$2" 2>/dev/null) || out="000" + echo "$out" } post_empty() { - wget -qO /dev/null -S --post-data="" --header="Content-Type: application/json" "$1" 2>&1 | grep -o 'HTTP/[0-9.]* [0-9]*' | tail -1 | awk '{print $2}' + out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "" "$1" 2>/dev/null) || out="000" + echo "$out" } check() { diff --git a/go/main.go b/go/main.go index fd48649..07a231c 100644 --- a/go/main.go +++ b/go/main.go @@ -22,8 +22,6 @@ var Version = "dev" func main() { listenSocket := flag.String("listen-socket", "/var/run/docker-socket-policy.sock", "Unix socket to listen on (or fd://3 for systemd socket activation)") - listenTCP := flag.String("listen-tcp", "127.0.0.1:2375", - "TCP address to listen on") dockerHost := flag.String("docker-host", "/var/run/docker.sock", "Docker daemon socket path") configDir := flag.String("config-dir", "/etc/docker-socket-policy/services", @@ -56,8 +54,13 @@ func main() { transport := proxy.NewTransport(*dockerHost) handler := proxy.NewHandler(router, chain, auditLog, transport) - go startListener(ctx, "unix", *listenSocket, handler) - startListener(ctx, "tcp", *listenTCP, handler) + listener, err := unixListener(*listenSocket) + if err != nil { + slog.Error("failed to start listener", "addr", *listenSocket, "error", err) + os.Exit(1) + } + + go serve(ctx, listener, *listenSocket, handler) <-ctx.Done() slog.Info("shutting down...") @@ -69,25 +72,18 @@ func main() { slog.Info("shutdown complete") } -func startListener(ctx context.Context, network, addr string, handler http.Handler) { - var listener net.Listener - var err error - - if network == "unix" { - if addr == "fd://3" { - listener, err = net.FileListener(os.NewFile(3, "socket")) - } else { - _ = os.Remove(addr) - listener, err = net.Listen("unix", addr) - } - } else { - listener, err = net.Listen("tcp", addr) - } - if err != nil { - slog.Error("failed to start listener", "network", network, "addr", addr, "error", err) - return +// unixListener binds the proxy's only listening socket. Listening is Unix-socket +// only by design: peer credentials and filesystem ownership on the socket are the +// access-control boundary, and a TCP listener would have neither. +func unixListener(addr string) (net.Listener, error) { + if addr == "fd://3" { + return net.FileListener(os.NewFile(3, "socket")) } + _ = os.Remove(addr) + return net.Listen("unix", addr) +} +func serve(ctx context.Context, listener net.Listener, addr string, handler http.Handler) { server := &http.Server{Handler: handler} go func() { <-ctx.Done() @@ -96,8 +92,8 @@ func startListener(ctx context.Context, network, addr string, handler http.Handl server.Shutdown(shutdownCtx) }() - slog.Info("listening", "network", network, "addr", addr) + slog.Info("listening", "network", "unix", "addr", addr) if err := server.Serve(listener); err != nil && err != http.ErrServerClosed { - slog.Error("server error", "network", network, "addr", addr, "error", err) + slog.Error("server error", "network", "unix", "addr", addr, "error", err) } } diff --git a/rs/src/main.rs b/rs/src/main.rs index a314af3..76d9527 100644 --- a/rs/src/main.rs +++ b/rs/src/main.rs @@ -37,9 +37,6 @@ struct Cli { #[arg(long, default_value = "/var/run/docker-socket-policy.sock")] listen_socket: String, - #[arg(long, default_value = "127.0.0.1:2375")] - listen_tcp: String, - #[arg(long, default_value = "/var/run/docker.sock")] docker_host: String, @@ -76,13 +73,12 @@ async fn main() -> Result<(), Box> { let transport: Box = Box::new(transport::UnixSocketTransport::new(&cli.docker_host)); let handler = Arc::new(handler::Handler::new(router, chain, audit, transport)); - // A broadcast channel lets every listener task shut down independently - // when a signal arrives, without one listener's loop owning the others. - // Both receivers are created BEFORE the signal tasks spawn: a broadcast - // send with zero receivers is silently dropped, so subscribing later - // would open a window where an early signal is lost. + // A broadcast channel lets the listener task shut down independently when a + // signal arrives, without the signal handlers owning the listener's loop. + // The receiver is created BEFORE the signal tasks spawn: a broadcast send + // with zero receivers is silently dropped, so subscribing later would open + // a window where an early signal is lost. let (shutdown_tx, unix_shutdown_rx) = broadcast::channel::<()>(1); - let tcp_shutdown_rx = shutdown_tx.subscribe(); { let tx = shutdown_tx.clone(); @@ -104,10 +100,20 @@ async fn main() -> Result<(), Box> { }); } - let unix_handle = spawn_unix_listener(handler.clone(), cli.listen_socket.clone(), unix_shutdown_rx); - let tcp_handle = spawn_tcp_listener(handler.clone(), cli.listen_tcp.clone(), tcp_shutdown_rx); - - let _ = tokio::join!(unix_handle, tcp_handle); + // Bind before spawning so a bind failure is fatal: the Unix socket is the + // process's only listener, so a running-but-unbound proxy is never useful. + let listener = bind_unix_listener(&cli.listen_socket).map_err(|e| { + tracing::error!("failed to bind unix socket {}: {}", cli.listen_socket, e); + e + })?; + let unix_handle = spawn_unix_listener( + handler.clone(), + listener, + cli.listen_socket.clone(), + unix_shutdown_rx, + ); + + let _ = unix_handle.await; tracing::info!("shutdown complete"); Ok(()) @@ -115,6 +121,10 @@ async fn main() -> Result<(), Box> { /// Binds the Unix socket listener for `--listen-socket`. /// +/// This is the proxy's only listener by design: peer credentials and filesystem +/// ownership on the socket are the access-control boundary, and a TCP listener +/// would have neither. +/// /// `fd://3` selects systemd socket activation (the socket is already bound /// and listening; we just adopt the fd). Any other value is treated as a /// filesystem path: a stale socket file left over from a previous run is @@ -151,17 +161,11 @@ fn unix_listener_from_raw_fd(fd: RawFd) -> io::Result fn spawn_unix_listener( handler: Arc, + listener: tokio::net::UnixListener, addr: String, mut shutdown_rx: broadcast::Receiver<()>, ) -> JoinHandle<()> { tokio::spawn(async move { - let listener = match bind_unix_listener(&addr) { - Ok(l) => l, - Err(e) => { - tracing::error!("failed to bind unix socket {}: {}", addr, e); - return; - } - }; tracing::info!("listening on unix socket {}", addr); loop { @@ -189,52 +193,6 @@ fn spawn_unix_listener( }) } -fn spawn_tcp_listener( - handler: Arc, - addr: String, - mut shutdown_rx: broadcast::Receiver<()>, -) -> JoinHandle<()> { - tokio::spawn(async move { - let socket_addr: std::net::SocketAddr = match addr.parse() { - Ok(a) => a, - Err(e) => { - tracing::error!("invalid TCP listen address {}: {}", addr, e); - return; - } - }; - let listener = match tokio::net::TcpListener::bind(socket_addr).await { - Ok(l) => l, - Err(e) => { - tracing::error!("failed to bind TCP {}: {}", socket_addr, e); - return; - } - }; - tracing::info!("listening on TCP {}", socket_addr); - - loop { - tokio::select! { - result = listener.accept() => { - match result { - Ok((stream, _)) => { - tokio::spawn(serve_connection(handler.clone(), stream)); - } - Err(e) => { - // Back off briefly: persistent accept errors - // (e.g. EMFILE) would otherwise busy-loop. - tracing::warn!("accept error on TCP socket: {}", e); - tokio::time::sleep(ACCEPT_ERROR_BACKOFF).await; - } - } - } - _ = shutdown_rx.recv() => { - tracing::info!("TCP listener shutting down"); - break; - } - } - } - }) -} - async fn serve_connection(handler: Arc, stream: S) where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, diff --git a/ts/src/flags.test.ts b/ts/src/flags.test.ts index 921160a..20ef0a3 100644 --- a/ts/src/flags.test.ts +++ b/ts/src/flags.test.ts @@ -1,6 +1,6 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; -import { getFlag, hasFlag, parseHostPort, parseSocketPath } from "./flags.js"; +import { getFlag, hasFlag, parseListenSocket, parseSocketPath } from "./flags.js"; describe("flags", () => { describe("getFlag", () => { @@ -30,7 +30,7 @@ describe("flags", () => { it("does not match a different flag's value", () => { assert.equal( - getFlag(["--listen-tcp", "0.0.0.0:2375"], "--config-dir", "d"), + getFlag(["--listen-socket", "/run/dsp.sock"], "--config-dir", "d"), "d", ); }); @@ -58,18 +58,41 @@ describe("flags", () => { }); }); - describe("parseHostPort", () => { - it("parses host:port", () => { - assert.deepEqual(parseHostPort("0.0.0.0:2375"), { host: "0.0.0.0", port: 2375 }); - assert.deepEqual(parseHostPort("127.0.0.1:2375"), { host: "127.0.0.1", port: 2375 }); + describe("parseListenSocket", () => { + it("accepts a plain unix socket path", () => { + assert.deepEqual(parseListenSocket("/var/run/docker-socket-policy.sock"), { + kind: "path", + path: "/var/run/docker-socket-policy.sock", + }); + }); + + it("accepts fd://3 for systemd socket activation", () => { + assert.deepEqual(parseListenSocket("fd://3"), { kind: "fd", fd: 3 }); }); - it("defaults to all interfaces for a bare port", () => { - assert.deepEqual(parseHostPort("2375"), { host: "0.0.0.0", port: 2375 }); + it("rejects socket activation on any fd other than 3", () => { + const result = parseListenSocket("fd://4"); + assert.equal(result.kind, "error"); + assert.match(result.kind === "error" ? result.message : "", /only supports fd:\/\/3/); }); - it("accepts a custom default host for bare port", () => { - assert.deepEqual(parseHostPort("2375", "127.0.0.1"), { host: "127.0.0.1", port: 2375 }); + it("rejects TCP and HTTP listen addresses", () => { + for (const input of [ + "tcp://0.0.0.0:2375", + "http://0.0.0.0:2375", + "https://0.0.0.0:2375", + "unix:///var/run/docker-socket-policy.sock", + ]) { + const result = parseListenSocket(input); + assert.equal(result.kind, "error", `expected ${input} to be rejected`); + assert.match(result.kind === "error" ? result.message : "", /Unix socket paths/); + } + }); + + it("rejects empty values", () => { + const result = parseListenSocket(""); + assert.equal(result.kind, "error"); + assert.match(result.kind === "error" ? result.message : "", /must not be empty/); }); }); diff --git a/ts/src/flags.ts b/ts/src/flags.ts index d16930d..aa0b984 100644 --- a/ts/src/flags.ts +++ b/ts/src/flags.ts @@ -19,18 +19,46 @@ export function hasFlag(args: string[], name: string): boolean { return args.includes(name) || args.some((a) => a.startsWith(prefix)); } -// Parses a "host:port" listen address (the form Go's net.Listen and Rust's -// bind accept) into the (host, port) pair that Node's http.Server.listen -// requires. A bare port string defaults to binding all interfaces. -export function parseHostPort( - input: string, - defaultHost = "0.0.0.0", -): { host: string; port: number } { - const colon = input.lastIndexOf(":"); - if (colon === -1) { - return { host: defaultHost, port: parseInt(input, 10) }; +// Raw fd systemd passes for the first socket under socket activation +// (sd_listen_fds convention: fds start at 3). +export const SYSTEMD_SOCKET_FD = 3; + +// Where the proxy should listen. The proxy listens on a Unix socket only: +// filesystem ownership on that socket is the access-control boundary, and a +// TCP listener would carry no peer identity at all. +export type ListenTarget = + | { kind: "fd"; fd: number } + | { kind: "path"; path: string } + | { kind: "error"; message: string }; + +// Parses --listen-socket. "fd://3" selects systemd socket activation (the +// socket is already bound; we adopt the fd). Any other value is a filesystem +// path. Mirrors bindUnixListener in Go and bind_unix_listener in Rust. +export function parseListenSocket(input: string): ListenTarget { + if (input === `fd://${SYSTEMD_SOCKET_FD}`) { + return { kind: "fd", fd: SYSTEMD_SOCKET_FD }; + } + if (input.startsWith("fd://")) { + return { + kind: "error", + message: `--listen-socket only supports fd://${SYSTEMD_SOCKET_FD} for socket activation, got: ${input}`, + }; + } + if ( + input.startsWith("tcp://") || + input.startsWith("http://") || + input.startsWith("https://") || + input.startsWith("unix://") + ) { + return { + kind: "error", + message: `--listen-socket only supports Unix socket paths, got: ${input}`, + }; + } + if (input.length === 0) { + return { kind: "error", message: "--listen-socket must not be empty" }; } - return { host: input.slice(0, colon), port: parseInt(input.slice(colon + 1), 10) }; + return { kind: "path", path: input }; } // Validates a Docker daemon address supplied via --docker-host. Only Unix diff --git a/ts/src/index.ts b/ts/src/index.ts index 80901a7..5961c34 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -1,11 +1,12 @@ import { createServer } from "node:http"; +import { unlinkSync } from "node:fs"; import { AuditLogger } from "./audit.js"; import { Chain } from "./middleware.js"; import { Manager } from "./policy.js"; import { Router } from "./proxy.js"; import { Handler } from "./handler.js"; import { Transport } from "./transport.js"; -import { getFlag, hasFlag, parseHostPort, parseSocketPath } from "./flags.js"; +import { getFlag, hasFlag, parseListenSocket, parseSocketPath } from "./flags.js"; const args = process.argv.slice(2); @@ -15,8 +16,12 @@ if (socketPathError) { console.error(socketPathError); process.exit(2); } -const listenTCP = getFlag(args, "--listen-tcp", "127.0.0.1:2375"); -const { host: listenHost, port: listenPort } = parseHostPort(listenTCP, "127.0.0.1"); +const listenSocket = getFlag(args, "--listen-socket", "/var/run/docker-socket-policy.sock"); +const listenTarget = parseListenSocket(listenSocket); +if (listenTarget.kind === "error") { + console.error(listenTarget.message); + process.exit(2); +} const configDir = getFlag(args, "--config-dir", "/etc/docker-socket-policy/services"); const logFile = getFlag(args, "--log-file", "/var/log/docker-socket-policy.log"); const readonly = hasFlag(args, "--readonly"); @@ -40,10 +45,32 @@ const server = createServer((req, res) => { }); }); -server.listen(listenPort, listenHost, () => { - console.log(`listening on ${listenHost}:${listenPort}`); +// A bind failure leaves the process with no listener at all, so it is fatal +// rather than merely logged. +server.on("error", (err) => { + console.error(`failed to bind ${listenSocket}: ${err.message}`); + process.exit(1); }); +if (listenTarget.kind === "fd") { + // systemd socket activation: the socket is already bound and listening, + // so we adopt the fd rather than binding a path ourselves. + server.listen({ fd: listenTarget.fd }, () => { + console.log(`listening on socket-activated fd ${listenTarget.fd}`); + }); +} else { + // Remove a stale socket file left by a previous run before binding, + // matching the Go and Rust implementations. + try { + unlinkSync(listenTarget.path); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err; + } + server.listen(listenTarget.path, () => { + console.log(`listening on unix socket ${listenTarget.path}`); + }); +} + function shutdown(signal: string) { console.log(`received ${signal}, shutting down...`); server.close(() => { From 5223db63ad337a3cfc6620911cda3e4b548eb4ea Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Tue, 22 Sep 2026 21:16:29 -0400 Subject: [PATCH 2/5] docs: correct the scope of the socket boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note added earlier in this branch implied the listening socket gives per-service granularity: "to grant a service access, place its container user in the group". It does not. The proxy performs no caller authentication — policy is selected from the Image field of the request body (router.go GetByImage, mirrored in the Quint spec), and there is no peer-credential check in any of the three implementations. Every caller of one socket therefore shares one trust domain and can act under any policy in that proxy's config dir by naming its image. States that explicitly and points at one-proxy-per-service for real isolation. --- README.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 73c0305..50a4bd3 100644 --- a/README.md +++ b/README.md @@ -228,10 +228,19 @@ docker pull attacker/malware:latest # denied: image not in allowlist > Docker daemon over Unix sockets exclusively and reject `tcp://` and `http://` > schemes for `--docker-host`. > -> To grant a service access, place its container user in the group that owns -> the socket and bind-mount the socket in. To revoke it, remove the group +> To grant access, place the caller's container user in the group that owns the +> listening socket and bind-mount that socket in; to revoke it, remove the group > membership. If the proxy cannot reach the daemon socket because of its own > group permissions, requests surface as `403`. +> +> **What the socket does not give you is per-service isolation.** The proxy +> performs no caller authentication: it selects a policy from the `Image` field +> of the request body, not from the identity of the connection. Every caller of +> one socket therefore shares one trust domain, and can act under any policy in +> that proxy's `--config-dir` by naming that policy's image. Treat the socket as +> a boundary around the whole proxy, not around a single service. To isolate +> services from one another, run a proxy instance per service, each with its own +> socket and a `--config-dir` containing only that service's policy. ### Systemd Socket Activation From f15c0fc71f9f6022ad38418f76d57c835e50d7e3 Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Wed, 23 Sep 2026 11:40:39 -0400 Subject: [PATCH 3/5] fix: validate listen socket and reject unknown flags across all three MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the TCP removal found several ways the Unix-socket-only claim did not actually hold. Socket activation did not check what it was handed. net.FileListener returns whatever the fd is, so a unit with ListenStream=127.0.0.1:2375 made Go serve plain TCP while logging "listening network=unix" — reinstating the listener this change exists to remove. Verified by planting a TCP listener at fd 3: listener type=*net.TCPListener addr=127.0.0.1:52701 network=tcp Node behaved the same way; Rust escaped only by an accident of address decoding in mio. All three now confirm the fd is an AF_UNIX socket and exit non-zero otherwise. --listen-socket was otherwise unvalidated in Go and Rust. "tcp://..." bound a file named "tcp:/0.0.0.0:2375", and on Linux a leading "@" put Go in the abstract namespace, where a socket has no inode, no mode and no owner and any process in the namespace can connect. Validation now matches TypeScript's and lives in all three, with the bare "0.0.0.0:2375" migration mistake rejected rather than silently bound. TypeScript silently ignored unknown flags, so --listen-tcp=0.0.0.0:2375 was a no-op there while Go and Rust exited 2. It now rejects unrecognised arguments, which is the difference between a loud migration failure and a caller believing TCP is still served. --docker-host was only validated in TypeScript, making the README's claim that all three reject tcp:// false. Ported to Go and Rust. Stale socket cleanup removed whatever was at the path; os.Remove also rmdir's empty directories, so a mistyped path could destroy data. All three now refuse anything that is not a socket. TypeScript's bind-error handler was registered permanently, but Node also emits "error" on accept failures, so any caller could kill the proxy with EMFILE while it reported "failed to bind". Scoped to the bind. Tests: Go's main package had none; it now has 7 covering validation, the non-socket refusal and the fd type check. Rust gains 4, TypeScript 10. The integration suite gains a guard asserting nothing answers on TCP 2375 — every other assertion goes over the Unix socket and would pass just as happily with a TCP listener alongside. curl is baked into deploy/Dockerfile.test rather than apk-added per run, since docker:28-cli does not ship it and installing at runtime put the Alpine CDN on the critical path of every run. Requests are bounded with --max-time, without which a hung proxy would hang CI rather than fail it, and the status fallback no longer discards a real code on a mid-response error. test-sock.sh now exits on readiness failure instead of warning and reporting eight misleading assertion failures. Refs #38. Socket mode/ownership is tracked separately in #40. --- deploy/Dockerfile.test | 11 ++ deploy/docker-compose.sock.yml | 22 ++- deploy/docker-compose.yml | 7 +- deploy/test-sock.sh | 39 ++++-- deploy/test.sh | 49 +++++-- go/main.go | 106 ++++++++++++++- go/main_test.go | 236 +++++++++++++++++++++++++++++++++ rs/src/main.rs | 206 ++++++++++++++++++++++++++-- ts/src/flags.test.ts | 83 +++++++++++- ts/src/flags.ts | 49 +++++++ ts/src/index.ts | 70 ++++++++-- 11 files changed, 816 insertions(+), 62 deletions(-) create mode 100644 deploy/Dockerfile.test create mode 100644 go/main_test.go diff --git a/deploy/Dockerfile.test b/deploy/Dockerfile.test new file mode 100644 index 0000000..db07044 --- /dev/null +++ b/deploy/Dockerfile.test @@ -0,0 +1,11 @@ +# Test runner for the integration suites. +# +# The proxy listens on a Unix socket only, and busybox wget cannot address one, +# so the suites need curl. docker:28-cli does not ship it (the upstream image +# installs only ca-certificates, openssh-client and git), so installing it at +# runtime would put the Alpine CDN on the critical path of every test run and +# of `make release-verify`. Baking it into a cached layer keeps the suites +# runnable offline once built. +FROM docker:28-cli + +RUN apk add --no-cache curl diff --git a/deploy/docker-compose.sock.yml b/deploy/docker-compose.sock.yml index 7c84262..6bb64c9 100644 --- a/deploy/docker-compose.sock.yml +++ b/deploy/docker-compose.sock.yml @@ -14,7 +14,12 @@ services: # listening sockets in this directory as non-root, and anything they # attempt before this runs fails with EACCES. The access control under # test is the mode on docker.sock below, not the mode on the directory. - chmod 0777 /sock + # + # Sticky bit: without it, proxy-denied — the container this suite + # asserts has no access — could unlink docker.sock or granted.sock and + # bind its own in their place, which would be a bypass of the very + # control being demonstrated, and a bad pattern to copy. + chmod 1777 /sock apk add --no-cache socat addgroup -g 2001 dockertest socat UNIX-LISTEN:/sock/docker.sock,unlink-early,fork,group=dockertest,perm=660 \ @@ -24,11 +29,20 @@ services: - sock-data:/sock # Gates the proxies below. service_started would only mean "the container # process exists", which was racing the chmod and the socat bind above. + # + # Checks the mode as well as existence: socat applies group and perm after + # bind(2), so there is a window where the socket exists at the default + # umask and proxy-granted would start into an EACCES it should not get. healthcheck: - test: ["CMD", "sh", "-c", "test -S /sock/docker.sock"] + test: + - CMD + - sh + - -c + - test -S /sock/docker.sock && [ "$$(stat -c '%a %G' /sock/docker.sock)" = "660 dockertest" ] interval: 1s timeout: 2s retries: 60 + start_period: 30s proxy-granted: build: @@ -67,7 +81,9 @@ services: - --log-file=/tmp/docker-socket-policy.log test: - image: docker:28-cli + build: + context: . + dockerfile: Dockerfile.test depends_on: - proxy-granted - proxy-denied diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index f2b5555..17fbc2b 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -17,11 +17,16 @@ services: - --log-file=/tmp/docker-socket-policy.log test: - image: docker:28-cli + build: + context: . + dockerfile: Dockerfile.test depends_on: - proxy environment: PROXY_SOCK: /sock/proxy.sock + # Hostname the proxy would have been reachable on when it still had a + # TCP listener, so the suite can assert that it no longer is. + PROXY_HOST: proxy volumes: - ./test.sh:/test.sh:ro - proxy-sock:/sock diff --git a/deploy/test-sock.sh b/deploy/test-sock.sh index e96012f..aed5989 100755 --- a/deploy/test-sock.sh +++ b/deploy/test-sock.sh @@ -16,27 +16,32 @@ DENIED_SOCK="${PROXY_DENIED_SOCK:-/sock/denied.sock}" URL="http://localhost" # busybox wget cannot speak to a Unix socket, so the helpers below need curl. +# It is baked into Dockerfile.test rather than installed here, so a run does +# not depend on the Alpine CDN being reachable. if ! command -v curl >/dev/null 2>&1; then - apk add --no-cache curl >/dev/null 2>&1 || { - echo "ERROR: curl is required to talk to the proxies over Unix sockets" - exit 1 - } + echo "ERROR: curl is missing from the test image (see deploy/Dockerfile.test)" + exit 1 fi +# Bound every request; curl has no default overall timeout. +TIMEOUT="--max-time 10 --connect-timeout 2" + # Both helpers take the target socket as their first argument, since this # suite talks to two proxies with deliberately different socket permissions. # curl exits non-zero when it cannot connect at all, which under `set -e` would # abort the run instead of reporting a failed assertion, so connection failures # are normalised to the synthetic status 000. This matters more here than in # test.sh: the wait loops below poll sockets that do not exist yet. +# curl still prints %{http_code} when it exits non-zero, so the fallback only +# applies when it produced nothing at all. get_status() { - out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$1" "$2" 2>/dev/null) || out="000" - echo "$out" + out=$(curl -s -o /dev/null -w '%{http_code}' $TIMEOUT --unix-socket "$1" "$2" 2>/dev/null || true) + echo "${out:-000}" } post_json() { - out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$1" \ - -X POST -H "Content-Type: application/json" -d "$2" "$3" 2>/dev/null) || out="000" - echo "$out" + out=$(curl -s -o /dev/null -w '%{http_code}' $TIMEOUT --unix-socket "$1" \ + -X POST -H "Content-Type: application/json" -d "$2" "$3" 2>/dev/null || true) + echo "${out:-000}" } check() { @@ -75,7 +80,14 @@ while [ $i -lt 30 ]; do done if [ $i -eq 30 ]; then echo "" - echo "WARNING: proxy-granted not ready after 30s" + # Fail here rather than letting every assertion come back 000, which reads + # as a policy bug when the real cause is that the proxy never started. + if [ ! -S "$GRANTED_SOCK" ]; then + echo "ERROR: proxy-granted never created $GRANTED_SOCK — it failed to bind" + else + echo "ERROR: proxy-granted is listening but not answering after 30s" + fi + exit 1 fi # proxy-denied: it listens fine but cannot reach the Docker socket, so it can @@ -94,7 +106,12 @@ while [ $i -lt 15 ]; do done if [ $i -eq 15 ]; then echo "" - echo "WARNING: proxy-denied not responding after 15s" + if [ ! -S "$DENIED_SOCK" ]; then + echo "ERROR: proxy-denied never created $DENIED_SOCK — it failed to bind" + else + echo "ERROR: proxy-denied is listening but not answering after 15s" + fi + exit 1 fi echo "" diff --git a/deploy/test.sh b/deploy/test.sh index 1483608..447cadc 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -16,18 +16,23 @@ PROXY_SOCK="${PROXY_SOCK:-/sock/proxy.sock}" PROXY="http://localhost" # busybox wget cannot speak to a Unix socket, so the helpers below need curl. +# It is baked into Dockerfile.test rather than installed here, so a run does +# not depend on the Alpine CDN being reachable. if ! command -v curl >/dev/null 2>&1; then - apk add --no-cache curl >/dev/null 2>&1 || { - echo "ERROR: curl is required to talk to the proxy over a Unix socket" - exit 1 - } + echo "ERROR: curl is missing from the test image (see deploy/Dockerfile.test)" + exit 1 fi +# Bound every request. curl has no default overall timeout, so without this a +# proxy that accepts the connection and then never answers would hang the run +# instead of failing it — and the readiness counter below would never advance. +TIMEOUT="--max-time 10 --connect-timeout 2" + # Wait for the proxy to create and serve its socket. echo "Waiting for proxy at ${PROXY_SOCK}..." i=0 while [ $i -lt 30 ]; do - if curl -sf --unix-socket "$PROXY_SOCK" "$PROXY/_ping" >/dev/null 2>&1; then + if curl -sf $TIMEOUT --unix-socket "$PROXY_SOCK" "$PROXY/_ping" >/dev/null 2>&1; then echo "Proxy ready." break fi @@ -46,19 +51,22 @@ echo "" # curl exits non-zero when it cannot connect at all, which under `set -e` would # abort the run instead of reporting a failed assertion, so connection failures # are normalised to the synthetic status 000. +# curl still prints %{http_code} when it exits non-zero, so the fallback only +# applies when it produced nothing at all. Overwriting unconditionally would +# discard a real status on a mid-response error and report it as 000. get_status() { - out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" "$1" 2>/dev/null) || out="000" - echo "$out" + out=$(curl -s -o /dev/null -w '%{http_code}' $TIMEOUT --unix-socket "$PROXY_SOCK" "$1" 2>/dev/null || true) + echo "${out:-000}" } post_json() { - out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" \ - -X POST -H "Content-Type: application/json" -d "$1" "$2" 2>/dev/null) || out="000" - echo "$out" + out=$(curl -s -o /dev/null -w '%{http_code}' $TIMEOUT --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "$1" "$2" 2>/dev/null || true) + echo "${out:-000}" } post_empty() { - out=$(curl -s -o /dev/null -w '%{http_code}' --unix-socket "$PROXY_SOCK" \ - -X POST -H "Content-Type: application/json" -d "" "$1" 2>/dev/null) || out="000" - echo "$out" + out=$(curl -s -o /dev/null -w '%{http_code}' $TIMEOUT --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "" "$1" 2>/dev/null || true) + echo "${out:-000}" } check() { @@ -80,6 +88,21 @@ echo " docker-socket-policy integration tests" echo "============================================" echo "" +# ─── Transport: Unix socket only ──────────────────────────────── + +# Regression guard. Every other assertion in this file goes over the Unix +# socket and would pass just as happily if the proxy were also serving TCP, +# so without this nothing here would notice a TCP listener coming back. +echo "--- Transport ---" + +if curl -s -o /dev/null --max-time 3 --connect-timeout 2 "http://${PROXY_HOST:-proxy}:2375/_ping" 2>/dev/null; then + echo " FAIL: proxy answered on TCP 2375 (it must listen on a Unix socket only)" + FAIL=$((FAIL+1)) +else + echo " PASS: no TCP listener on 2375" + PASS=$((PASS+1)) +fi + # ─── Read-only endpoints (always allowed) ─────────────────────── echo "--- Read-only endpoints ---" diff --git a/go/main.go b/go/main.go index 07a231c..3684b0c 100644 --- a/go/main.go +++ b/go/main.go @@ -2,12 +2,16 @@ package main import ( "context" + "errors" "flag" + "fmt" + "io/fs" "log/slog" "net" "net/http" "os" "os/signal" + "strings" "syscall" "time" @@ -32,6 +36,15 @@ func main() { "Enable read-only mode (deny all POST/PUT/DELETE)") flag.Parse() + if err := validateListenSocket(*listenSocket); err != nil { + slog.Error(err.Error()) + os.Exit(2) + } + if err := validateDockerHost(*dockerHost); err != nil { + slog.Error(err.Error()) + os.Exit(2) + } + ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer cancel() @@ -72,14 +85,97 @@ func main() { slog.Info("shutdown complete") } +// systemdSocketFD is the first fd systemd passes under socket activation +// (sd_listen_fds convention: fds start at 3). +const systemdSocketFD = 3 + +// validateListenSocket rejects --listen-socket values that would not produce a +// filesystem-visible Unix socket. Without this, several of them bind something +// surprising rather than failing: "tcp://0.0.0.0:2375" becomes a file named +// "tcp:/0.0.0.0:2375", and on Linux a leading "@" puts Go in the abstract +// namespace, where the socket has no inode, no mode and no owner and every +// process in the network namespace can connect to it unconditionally. +func validateListenSocket(addr string) error { + switch { + case addr == "": + return errors.New("--listen-socket must not be empty") + case addr == fmt.Sprintf("fd://%d", systemdSocketFD): + return nil + case strings.HasPrefix(addr, "fd://"): + return fmt.Errorf("--listen-socket only supports fd://%d for socket activation, got: %s", + systemdSocketFD, addr) + case strings.HasPrefix(addr, "tcp://"), + strings.HasPrefix(addr, "http://"), + strings.HasPrefix(addr, "https://"), + strings.HasPrefix(addr, "unix://"): + return fmt.Errorf("--listen-socket only supports Unix socket paths, got: %s", addr) + case strings.HasPrefix(addr, "@"), strings.HasPrefix(addr, "\x00"): + return fmt.Errorf("--listen-socket must be a filesystem path; abstract sockets have no "+ + "permissions and would be reachable by any process, got: %s", addr) + case !strings.HasPrefix(addr, "/"): + return fmt.Errorf("--listen-socket must be an absolute path, got: %s", addr) + } + return nil +} + +// validateDockerHost rejects non-Unix Docker daemon addresses. Connecting to the +// daemon over TCP would bypass the user/group ownership on the daemon socket, +// which is what constrains the proxy's own access. +func validateDockerHost(addr string) error { + if addr == "" { + return errors.New("--docker-host must not be empty") + } + for _, scheme := range []string{"tcp://", "http://", "https://", "unix://"} { + if strings.HasPrefix(addr, scheme) { + return fmt.Errorf("--docker-host only supports Unix socket paths, got: %s", addr) + } + } + return nil +} + +// listenerFromFile adopts an already-bound socket, as passed by systemd. +// +// Split out from unixListener so the type check can be exercised in tests +// against an arbitrary fd rather than only against the real fd 3, mirroring +// Rust's unix_listener_from_raw_fd. +func listenerFromFile(f *os.File) (net.Listener, error) { + l, err := net.FileListener(f) + if err != nil { + return nil, err + } + // net.FileListener returns whatever the fd actually is. A unit with + // ListenStream=127.0.0.1:2375 hands back a TCP socket, and serving it + // would silently reinstate the TCP listener this proxy does not have. + if _, ok := l.(*net.UnixListener); !ok { + l.Close() + return nil, fmt.Errorf("fd %d is a %T, not a Unix socket: set ListenStream to a "+ + "filesystem path in the .socket unit", systemdSocketFD, l) + } + return l, nil +} + // unixListener binds the proxy's only listening socket. Listening is Unix-socket -// only by design: peer credentials and filesystem ownership on the socket are the -// access-control boundary, and a TCP listener would have neither. +// only by design: filesystem ownership on the socket is the access-control +// boundary, and a TCP listener would have none. func unixListener(addr string) (net.Listener, error) { - if addr == "fd://3" { - return net.FileListener(os.NewFile(3, "socket")) + if addr == fmt.Sprintf("fd://%d", systemdSocketFD) { + return listenerFromFile(os.NewFile(systemdSocketFD, "socket")) + } + + // Remove a stale socket from a previous run, but only a socket: os.Remove + // also unlinks regular files and rmdir's empty directories, so ignoring its + // error would let a mistyped path silently delete an operator's data. + if info, err := os.Lstat(addr); err == nil { + if info.Mode()&fs.ModeSocket == 0 { + return nil, fmt.Errorf("refusing to remove %s: not a socket (mode %s)", addr, info.Mode()) + } + if err := os.Remove(addr); err != nil { + return nil, fmt.Errorf("removing stale socket %s: %w", addr, err) + } + } else if !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("checking %s: %w", addr, err) } - _ = os.Remove(addr) + return net.Listen("unix", addr) } diff --git a/go/main_test.go b/go/main_test.go new file mode 100644 index 0000000..7ac7881 --- /dev/null +++ b/go/main_test.go @@ -0,0 +1,236 @@ +package main + +import ( + "net" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestValidateListenSocket(t *testing.T) { + tests := []struct { + name string + addr string + wantErr string // substring; empty means the value must be accepted + }{ + {"absolute path", "/var/run/docker-socket-policy.sock", ""}, + {"systemd activation", "fd://3", ""}, + {"empty", "", "must not be empty"}, + {"other fd", "fd://4", "only supports fd://3"}, + {"fd zero", "fd://0", "only supports fd://3"}, + {"fd garbage", "fd://abc", "only supports fd://3"}, + // The flag this proxy deliberately no longer has. Someone migrating + // from --listen-tcp is likely to carry the value across. + {"tcp scheme", "tcp://0.0.0.0:2375", "only supports Unix socket paths"}, + {"http scheme", "http://0.0.0.0:2375", "only supports Unix socket paths"}, + {"https scheme", "https://0.0.0.0:2375", "only supports Unix socket paths"}, + {"unix scheme", "unix:///var/run/d.sock", "only supports Unix socket paths"}, + // Go's net package maps a leading "@" to the Linux abstract namespace, + // where the socket has no inode, no mode and no owner. + {"abstract at-sign", "@dsp", "abstract sockets have no permissions"}, + {"abstract nul", "\x00dsp", "abstract sockets have no permissions"}, + // The likeliest --listen-tcp migration mistake: drop the scheme, keep + // the address. Binding it would create a file named "0.0.0.0:2375". + {"bare host port", "0.0.0.0:2375", "must be an absolute path"}, + {"relative path", "dsp.sock", "must be an absolute path"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateListenSocket(tt.addr) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("validateListenSocket(%q) = %v, want nil", tt.addr, err) + } + return + } + if err == nil { + t.Fatalf("validateListenSocket(%q) = nil, want error containing %q", tt.addr, tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("validateListenSocket(%q) = %q, want it to contain %q", tt.addr, err, tt.wantErr) + } + }) + } +} + +func TestValidateDockerHost(t *testing.T) { + tests := []struct { + name string + addr string + wantErr string + }{ + {"unix path", "/var/run/docker.sock", ""}, + {"empty", "", "must not be empty"}, + // Reaching the daemon over TCP would bypass the user/group ownership + // on the daemon socket, which is what constrains the proxy itself. + {"tcp scheme", "tcp://dind:2375", "only supports Unix socket paths"}, + {"http scheme", "http://dind:2375", "only supports Unix socket paths"}, + {"https scheme", "https://dind:2375", "only supports Unix socket paths"}, + {"unix scheme", "unix:///var/run/docker.sock", "only supports Unix socket paths"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateDockerHost(tt.addr) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("validateDockerHost(%q) = %v, want nil", tt.addr, err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("validateDockerHost(%q) = %v, want error containing %q", tt.addr, err, tt.wantErr) + } + }) + } +} + +// shortTempDir returns a temp dir under /tmp rather than t.TempDir(). +// sun_path is capped at 104 bytes on macOS and 108 on Linux, and the +// per-test paths t.TempDir() produces on macOS exceed that, so binding +// inside one fails with EINVAL regardless of the code under test. +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("/tmp", "dsp-test-") + if err != nil { + t.Fatalf("creating temp dir: %v", err) + } + t.Cleanup(func() { os.RemoveAll(dir) }) + return dir +} + +func TestUnixListenerBindsFreshPath(t *testing.T) { + path := filepath.Join(shortTempDir(t), "fresh.sock") + + l, err := unixListener(path) + if err != nil { + t.Fatalf("unixListener(%q) = %v, want nil", path, err) + } + defer l.Close() + + if _, ok := l.(*net.UnixListener); !ok { + t.Fatalf("unixListener returned %T, want *net.UnixListener", l) + } + if info, err := os.Lstat(path); err != nil { + t.Fatalf("socket not created: %v", err) + } else if info.Mode()&os.ModeSocket == 0 { + t.Fatalf("created %s, want a socket", info.Mode()) + } +} + +func TestUnixListenerReplacesStaleSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stale.sock") + + // Leave a real socket behind, as an unclean shutdown would. + first, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("seeding stale socket: %v", err) + } + first.Close() + // net.Listen's listener unlinks on Close, so recreate the stale entry. + stale, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("re-seeding stale socket: %v", err) + } + _ = stale + + l, err := unixListener(path) + if err != nil { + t.Fatalf("unixListener over stale socket = %v, want nil", err) + } + l.Close() +} + +func TestUnixListenerRefusesToDeleteNonSocket(t *testing.T) { + // A mistyped --listen-socket must not silently destroy data. os.Remove + // would happily unlink a regular file and rmdir an empty directory. + t.Run("regular file", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "important.txt") + if err := os.WriteFile(path, []byte("important data"), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := unixListener(path); err == nil { + t.Fatal("unixListener over a regular file = nil, want error") + } else if !strings.Contains(err.Error(), "not a socket") { + t.Fatalf("error = %q, want it to mention 'not a socket'", err) + } + + if got, err := os.ReadFile(path); err != nil { + t.Fatalf("file was destroyed: %v", err) + } else if string(got) != "important data" { + t.Fatalf("file contents = %q, want them untouched", got) + } + }) + + t.Run("directory", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "adir") + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatal(err) + } + + if _, err := unixListener(path); err == nil { + t.Fatal("unixListener over a directory = nil, want error") + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("directory was removed: %v", err) + } + }) +} + +// TestListenerFromFileRejectsTCPSocket is the regression guard for the hole +// that motivated validating socket activation at all: net.FileListener returns +// whatever the fd actually is, so a .socket unit with +// ListenStream=127.0.0.1:2375 would otherwise reinstate a TCP listener while +// the proxy logged "listening network=unix". +func TestListenerFromFileRejectsTCPSocket(t *testing.T) { + tcp, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("creating TCP listener: %v", err) + } + defer tcp.Close() + + f, err := tcp.(*net.TCPListener).File() + if err != nil { + t.Fatalf("extracting TCP fd: %v", err) + } + defer f.Close() + + l, err := listenerFromFile(f) + if err == nil { + l.Close() + t.Fatal("listenerFromFile accepted a TCP socket, want an error") + } + if !strings.Contains(err.Error(), "not a Unix socket") { + t.Fatalf("error = %q, want it to mention 'not a Unix socket'", err) + } +} + +// TestListenerFromFileAcceptsUnixSocket is the positive half: genuine socket +// activation must still work. +func TestListenerFromFileAcceptsUnixSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "activated.sock") + unix, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("creating Unix listener: %v", err) + } + defer unix.Close() + + f, err := unix.(*net.UnixListener).File() + if err != nil { + t.Fatalf("extracting Unix fd: %v", err) + } + defer f.Close() + + l, err := listenerFromFile(f) + if err != nil { + t.Fatalf("listenerFromFile on a Unix socket = %v, want nil", err) + } + defer l.Close() + + if _, ok := l.(*net.UnixListener); !ok { + t.Fatalf("listenerFromFile returned %T, want *net.UnixListener", l) + } +} diff --git a/rs/src/main.rs b/rs/src/main.rs index 76d9527..6bdd251 100644 --- a/rs/src/main.rs +++ b/rs/src/main.rs @@ -1,6 +1,5 @@ -// Only the `Cli` struct fields legitimately go unread if a listener is -// disabled by config; broader dead_code cleanup across policy/proxy/ -// middleware (pre-existing, unrelated to this fix) is tracked separately. +// Broader dead_code cleanup across policy/proxy/middleware (pre-existing, +// unrelated to this change) is tracked separately. #![allow(dead_code)] mod audit; @@ -15,6 +14,7 @@ use hyper::body::Incoming as IncomingBody; use hyper::Request; use hyper_util::rt::TokioIo; use std::io; +use std::os::unix::fs::FileTypeExt; use std::os::unix::io::{FromRawFd, RawFd}; use std::os::unix::net::UnixListener as StdUnixListener; use std::sync::Arc; @@ -58,6 +58,15 @@ async fn main() -> Result<(), Box> { let cli = Cli::parse(); + if let Err(msg) = validate_listen_socket(&cli.listen_socket) { + tracing::error!("{}", msg); + std::process::exit(2); + } + if let Err(msg) = validate_docker_host(&cli.docker_host) { + tracing::error!("{}", msg); + std::process::exit(2); + } + let policy_manager = policy::Manager::new(&cli.config_dir)?; tracing::info!("loaded {} policies", policy_manager.list().len()); @@ -127,21 +136,87 @@ async fn main() -> Result<(), Box> { /// /// `fd://3` selects systemd socket activation (the socket is already bound /// and listening; we just adopt the fd). Any other value is treated as a -/// filesystem path: a stale socket file left over from a previous run is -/// removed before binding, matching the Go implementation. +/// filesystem path: a stale socket left over from a previous run is removed +/// before binding, matching the Go implementation. fn bind_unix_listener(addr: &str) -> io::Result { if addr == "fd://3" { return unix_listener_from_raw_fd(SYSTEMD_SOCKET_FD); } - if let Err(e) = std::fs::remove_file(addr) { - if e.kind() != io::ErrorKind::NotFound { - return Err(e); + // Remove a stale socket, but only a socket: blindly removing would let a + // mistyped path silently delete an operator's file, so anything that is + // not a socket is an error rather than something to clear out of the way. + match std::fs::symlink_metadata(addr) { + Ok(meta) => { + if !meta.file_type().is_socket() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("refusing to remove {}: not a socket ({:?})", addr, meta.file_type()), + )); + } + std::fs::remove_file(addr)?; } + Err(e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => return Err(e), } tokio::net::UnixListener::bind(addr) } +/// Rejects `--listen-socket` values that would not produce a filesystem-visible +/// Unix socket. Several of them otherwise bind something surprising rather than +/// failing: `tcp://0.0.0.0:2375` becomes a file named `tcp:/0.0.0.0:2375`. +/// Mirrors `validateListenSocket` in Go and `parseListenSocket` in TypeScript. +fn validate_listen_socket(addr: &str) -> Result<(), String> { + let activation = format!("fd://{}", SYSTEMD_SOCKET_FD); + if addr.is_empty() { + Err("--listen-socket must not be empty".to_string()) + } else if addr == activation { + Ok(()) + } else if addr.starts_with("fd://") { + Err(format!( + "--listen-socket only supports {} for socket activation, got: {}", + activation, addr + )) + } else if ["tcp://", "http://", "https://", "unix://"] + .iter() + .any(|s| addr.starts_with(s)) + { + Err(format!( + "--listen-socket only supports Unix socket paths, got: {}", + addr + )) + } else if addr.starts_with('@') || addr.starts_with('\0') { + Err(format!( + "--listen-socket must be a filesystem path; abstract sockets have no permissions \ + and would be reachable by any process, got: {}", + addr + )) + } else if !addr.starts_with('/') { + Err(format!("--listen-socket must be an absolute path, got: {}", addr)) + } else { + Ok(()) + } +} + +/// Rejects non-Unix Docker daemon addresses. Connecting to the daemon over TCP +/// would bypass the user/group ownership on the daemon socket, which is what +/// constrains the proxy's own access. +fn validate_docker_host(addr: &str) -> Result<(), String> { + if addr.is_empty() { + return Err("--docker-host must not be empty".to_string()); + } + if ["tcp://", "http://", "https://", "unix://"] + .iter() + .any(|s| addr.starts_with(s)) + { + return Err(format!( + "--docker-host only supports Unix socket paths, got: {}", + addr + )); + } + Ok(()) +} + /// Wraps an existing raw fd as a Tokio `UnixListener`. /// /// Split out from [`bind_unix_listener`] so the fd-adoption mechanics @@ -155,6 +230,22 @@ fn unix_listener_from_raw_fd(fd: RawFd) -> io::Result // surfaces as an `io::Error` from the syscalls below (or from `accept`), // never as undefined behavior. let std_listener = unsafe { StdUnixListener::from_raw_fd(fd) }; + + // Confirm the fd really is an AF_UNIX socket rather than relying on a later + // accept to fail. A unit with ListenStream=127.0.0.1:2375 hands us a TCP + // socket, and treating it as a Unix socket would either serve plain TCP or + // wedge the accept loop warning at every retry. + std_listener.local_addr().map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "fd {} is not a Unix socket ({}): set ListenStream to a filesystem path \ + in the .socket unit", + fd, e + ), + ) + })?; + std_listener.set_nonblocking(true)?; tokio::net::UnixListener::from_std(std_listener) } @@ -221,21 +312,95 @@ mod tests { } #[tokio::test] - async fn test_bind_unix_listener_removes_stale_socket_file() { + async fn test_bind_unix_listener_removes_stale_socket() { let path = unique_socket_path(); - // Simulate a stale socket file left behind by a previous run. - std::fs::write(&path, b"stale").unwrap(); + // A real socket left behind by an unclean shutdown. Dropping the + // listener does not unlink it, so the file outlives the process. + let stale = StdUnixListener::bind(&path).unwrap(); + drop(stale); + assert!(path.exists(), "precondition: stale socket should still be on disk"); let result = bind_unix_listener(path.to_str().unwrap()); assert!( result.is_ok(), - "expected stale socket file to be removed and bind to succeed: {:?}", + "expected stale socket to be removed and bind to succeed: {:?}", result.err() ); std::fs::remove_file(&path).ok(); } + #[tokio::test] + async fn test_bind_unix_listener_refuses_to_delete_non_socket() { + // A mistyped --listen-socket must not silently destroy data. + let path = unique_socket_path(); + std::fs::write(&path, b"important data").unwrap(); + + let result = bind_unix_listener(path.to_str().unwrap()); + assert!(result.is_err(), "expected a regular file to be refused, not deleted"); + assert!( + result.unwrap_err().to_string().contains("not a socket"), + "error should explain that the path is not a socket" + ); + assert_eq!( + std::fs::read(&path).unwrap(), + b"important data", + "the file must be left untouched" + ); + + std::fs::remove_file(&path).ok(); + } + + #[test] + fn test_validate_listen_socket() { + // Accepted. + for addr in ["/var/run/docker-socket-policy.sock", "fd://3"] { + assert!(validate_listen_socket(addr).is_ok(), "{} should be accepted", addr); + } + + // Rejected, with the reason that should be reported. + let cases = [ + ("", "must not be empty"), + ("fd://4", "only supports fd://3"), + ("fd://0", "only supports fd://3"), + ("fd://abc", "only supports fd://3"), + ("tcp://0.0.0.0:2375", "only supports Unix socket paths"), + ("http://0.0.0.0:2375", "only supports Unix socket paths"), + ("https://0.0.0.0:2375", "only supports Unix socket paths"), + ("unix:///var/run/d.sock", "only supports Unix socket paths"), + ("@dsp", "abstract sockets have no permissions"), + ("\0dsp", "abstract sockets have no permissions"), + // The likeliest --listen-tcp migration mistake: drop the scheme, + // keep the address. Binding it would create a file called + // "0.0.0.0:2375" and report success while being unreachable. + ("0.0.0.0:2375", "must be an absolute path"), + ("dsp.sock", "must be an absolute path"), + ]; + for (addr, want) in cases { + let err = validate_listen_socket(addr) + .expect_err(&format!("{:?} should be rejected", addr)); + assert!(err.contains(want), "error for {:?} was {:?}, want it to contain {:?}", addr, err, want); + } + } + + #[test] + fn test_validate_docker_host() { + assert!(validate_docker_host("/var/run/docker.sock").is_ok()); + + let cases = [ + ("", "must not be empty"), + ("tcp://dind:2375", "only supports Unix socket paths"), + ("http://dind:2375", "only supports Unix socket paths"), + ("https://dind:2375", "only supports Unix socket paths"), + ("unix:///var/run/docker.sock", "only supports Unix socket paths"), + ]; + for (addr, want) in cases { + let err = validate_docker_host(addr) + .expect_err(&format!("{:?} should be rejected", addr)); + assert!(err.contains(want), "error for {:?} was {:?}", addr, err); + } + } + #[tokio::test] async fn test_bind_unix_listener_binds_fresh_path() { let path = unique_socket_path(); @@ -258,4 +423,21 @@ mod tests { std::fs::remove_file(&path).ok(); } + + /// Regression guard for socket activation handing back the wrong socket + /// family: a unit with ListenStream=127.0.0.1:2375 would otherwise wedge + /// the accept loop instead of failing, while the process looked healthy. + #[tokio::test] + async fn test_unix_listener_from_raw_fd_rejects_tcp_socket() { + let tcp = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let fd = tcp.into_raw_fd(); + + let err = unix_listener_from_raw_fd(fd) + .expect_err("expected a TCP socket at the activation fd to be rejected"); + assert!( + err.to_string().contains("not a Unix socket"), + "error should explain the fd is not a Unix socket, got: {}", + err + ); + } } diff --git a/ts/src/flags.test.ts b/ts/src/flags.test.ts index 20ef0a3..9b2b5c9 100644 --- a/ts/src/flags.test.ts +++ b/ts/src/flags.test.ts @@ -1,6 +1,12 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; -import { getFlag, hasFlag, parseListenSocket, parseSocketPath } from "./flags.js"; +import { + getFlag, + hasFlag, + parseListenSocket, + parseSocketPath, + validateFlags, +} from "./flags.js"; describe("flags", () => { describe("getFlag", () => { @@ -58,6 +64,50 @@ describe("flags", () => { }); }); + describe("validateFlags", () => { + const VALUE = ["--listen-socket", "--docker-host", "--config-dir", "--log-file"]; + const BOOL = ["--readonly"]; + const check = (args: string[]) => validateFlags(args, VALUE, BOOL); + + it("accepts known flags in both space and equals form", () => { + assert.equal(check(["--config-dir", "/x", "--log-file=/y"]), null); + assert.equal(check([]), null); + }); + + it("accepts a boolean flag without swallowing the next argument", () => { + // --readonly takes no value, so --config-dir must still be parsed. + assert.equal(check(["--readonly", "--config-dir", "/x"]), null); + }); + + it("rejects --listen-tcp, which this proxy no longer has", () => { + // The whole point: silently ignoring it would leave a caller believing + // the proxy is listening on TCP when it is not. + assert.match(check(["--listen-tcp=0.0.0.0:2375"]) ?? "", /unrecognised flag: --listen-tcp/); + assert.match(check(["--listen-tcp", "0.0.0.0:2375"]) ?? "", /unrecognised flag: --listen-tcp/); + }); + + it("rejects unknown and misspelled flags", () => { + assert.match(check(["--bogus"]) ?? "", /unrecognised flag: --bogus/); + assert.match(check(["--read-only"]) ?? "", /unrecognised flag: --read-only/); + assert.match(check(["-readonly"]) ?? "", /unrecognised flag: -readonly/); + }); + + it("rejects stray positional arguments", () => { + assert.match(check(["oops"]) ?? "", /unexpected argument: oops/); + assert.match(check(["--readonly", "oops"]) ?? "", /unexpected argument: oops/); + }); + + it("rejects a value flag with no value", () => { + assert.match(check(["--config-dir"]) ?? "", /flag needs an argument: --config-dir/); + }); + + it("does not mistake a flag-like value for a flag", () => { + // "--config-dir --readonly" consumes --readonly as the value; odd, but + // it matches Go's flag package, and the point is that it is not an error. + assert.equal(check(["--config-dir", "--readonly"]), null); + }); + }); + describe("parseListenSocket", () => { it("accepts a plain unix socket path", () => { assert.deepEqual(parseListenSocket("/var/run/docker-socket-policy.sock"), { @@ -71,9 +121,34 @@ describe("flags", () => { }); it("rejects socket activation on any fd other than 3", () => { - const result = parseListenSocket("fd://4"); - assert.equal(result.kind, "error"); - assert.match(result.kind === "error" ? result.message : "", /only supports fd:\/\/3/); + for (const input of ["fd://4", "fd://0", "fd://", "fd://3x", "fd://abc"]) { + const result = parseListenSocket(input); + assert.ok(result.kind === "error", `expected ${input} to be rejected`); + assert.match(result.message, /only supports fd:\/\/3/); + } + }); + + it("rejects a host:port left over from --listen-tcp", () => { + // The likeliest migration mistake: dropping the scheme but keeping the + // address. Accepting it would create a file named "0.0.0.0:2375" and + // report success while being unreachable. + const result = parseListenSocket("0.0.0.0:2375"); + assert.ok(result.kind === "error"); + assert.match(result.message, /absolute path/); + }); + + it("rejects abstract-namespace sockets, which have no permissions", () => { + for (const input of ["@dsp", "\0dsp"]) { + const result = parseListenSocket(input); + assert.ok(result.kind === "error", `expected ${input} to be rejected`); + assert.match(result.message, /abstract sockets have no permissions/); + } + }); + + it("rejects relative paths", () => { + const result = parseListenSocket("dsp.sock"); + assert.ok(result.kind === "error"); + assert.match(result.message, /absolute path/); }); it("rejects TCP and HTTP listen addresses", () => { diff --git a/ts/src/flags.ts b/ts/src/flags.ts index aa0b984..f3338ca 100644 --- a/ts/src/flags.ts +++ b/ts/src/flags.ts @@ -19,6 +19,41 @@ export function hasFlag(args: string[], name: string): boolean { return args.includes(name) || args.some((a) => a.startsWith(prefix)); } +// Rejects anything not recognised. Go's flag package and Rust's clap both exit +// non-zero on an unrecognised argument; without this the hand-rolled parser +// above would silently ignore one. That matters most for flags this proxy +// deliberately no longer has: `--listen-tcp=0.0.0.0:2375` must be a loud +// failure, not a no-op that leaves the caller assuming it took effect. +// +// `valueFlags` take an argument (in either `--name value` or `--name=value` +// form); `boolFlags` do not, and must not swallow the following argument. +// Returns an error message, or null when every argument is recognised. +export function validateFlags( + args: string[], + valueFlags: string[], + boolFlags: string[], +): string | null { + const known = [...valueFlags, ...boolFlags]; + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + const hasEquals = arg.includes("="); + const name = hasEquals ? arg.slice(0, arg.indexOf("=")) : arg; + + if (!known.includes(name)) { + if (arg.startsWith("-")) { + return `unrecognised flag: ${name}\nsupported flags: ${known.join(", ")}`; + } + return `unexpected argument: ${arg}`; + } + if (valueFlags.includes(name)) { + if (hasEquals) continue; + if (i + 1 >= args.length) return `flag needs an argument: ${name}`; + i++; // consume the value so it is not mistaken for an argument + } + } + return null; +} + // Raw fd systemd passes for the first socket under socket activation // (sd_listen_fds convention: fds start at 3). export const SYSTEMD_SOCKET_FD = 3; @@ -55,6 +90,20 @@ export function parseListenSocket(input: string): ListenTarget { message: `--listen-socket only supports Unix socket paths, got: ${input}`, }; } + if (input.startsWith("@") || input.startsWith("\0")) { + return { + kind: "error", + message: + `--listen-socket must be a filesystem path; abstract sockets have no permissions ` + + `and would be reachable by any process, got: ${input}`, + }; + } + if (input.length > 0 && !input.startsWith("/")) { + return { + kind: "error", + message: `--listen-socket must be an absolute path, got: ${input}`, + }; + } if (input.length === 0) { return { kind: "error", message: "--listen-socket must not be empty" }; } diff --git a/ts/src/index.ts b/ts/src/index.ts index 5961c34..47163d7 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -1,15 +1,30 @@ import { createServer } from "node:http"; -import { unlinkSync } from "node:fs"; +import { lstatSync, unlinkSync } from "node:fs"; import { AuditLogger } from "./audit.js"; import { Chain } from "./middleware.js"; import { Manager } from "./policy.js"; import { Router } from "./proxy.js"; import { Handler } from "./handler.js"; import { Transport } from "./transport.js"; -import { getFlag, hasFlag, parseListenSocket, parseSocketPath } from "./flags.js"; +import { + getFlag, + hasFlag, + parseListenSocket, + parseSocketPath, + validateFlags, +} from "./flags.js"; const args = process.argv.slice(2); +const VALUE_FLAGS = ["--listen-socket", "--docker-host", "--config-dir", "--log-file"]; +const BOOL_FLAGS = ["--readonly"]; + +const flagError = validateFlags(args, VALUE_FLAGS, BOOL_FLAGS); +if (flagError) { + console.error(flagError); + process.exit(2); +} + const dockerHost = getFlag(args, "--docker-host", "/var/run/docker.sock"); const socketPathError = parseSocketPath(dockerHost); if (socketPathError) { @@ -45,29 +60,58 @@ const server = createServer((req, res) => { }); }); -// A bind failure leaves the process with no listener at all, so it is fatal -// rather than merely logged. -server.on("error", (err) => { +// A bind failure leaves the process with no listener at all, so it is fatal. +// This handler is scoped to the bind: net.Server also emits "error" for accept +// failures (EMFILE and friends), and exiting on those would let any caller kill +// the proxy — Go retries them and Rust backs off, so exiting would be a +// TypeScript-only availability regression. +const onBindError = (err: NodeJS.ErrnoException) => { console.error(`failed to bind ${listenSocket}: ${err.message}`); process.exit(1); +}; +server.once("error", onBindError); +server.once("listening", () => { + server.off("error", onBindError); + server.on("error", (err) => console.error(`server error: ${err.message}`)); }); if (listenTarget.kind === "fd") { // systemd socket activation: the socket is already bound and listening, // so we adopt the fd rather than binding a path ourselves. - server.listen({ fd: listenTarget.fd }, () => { - console.log(`listening on socket-activated fd ${listenTarget.fd}`); + const fd = listenTarget.fd; + server.listen({ fd }, () => { + // Node hands back whatever the fd actually is. A unit with + // ListenStream=127.0.0.1:2375 yields a TCP server, which would silently + // reinstate the TCP listener this proxy does not have. address() returns + // a string for a Unix socket and an object for TCP. + if (typeof server.address() !== "string") { + console.error( + `fd ${fd} is not a Unix socket: set ListenStream to a filesystem path ` + + `in the .socket unit`, + ); + process.exit(1); + } + console.log(`listening on socket-activated fd ${fd}`); }); } else { - // Remove a stale socket file left by a previous run before binding, - // matching the Go and Rust implementations. + // Remove a stale socket left by a previous run, but only a socket: blindly + // unlinking would let a mistyped path silently delete an operator's file. + const path = listenTarget.path; try { - unlinkSync(listenTarget.path); + if (!lstatSync(path).isSocket()) { + console.error(`refusing to remove ${path}: not a socket`); + process.exit(1); + } + unlinkSync(path); } catch (err) { - if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err; + const e = err as NodeJS.ErrnoException; + if (e.code !== "ENOENT") { + console.error(`failed to remove stale socket ${path}: ${e.message}`); + process.exit(1); + } } - server.listen(listenTarget.path, () => { - console.log(`listening on unix socket ${listenTarget.path}`); + server.listen(path, () => { + console.log(`listening on unix socket ${path}`); }); } From bc0ec9866a611bc744cfab1b71370d4e83e312e8 Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Wed, 23 Sep 2026 11:59:56 -0400 Subject: [PATCH 4/5] test: assert connection-refused on the TCP probe, not any curl failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard added in f15c0fc treated any non-zero curl exit as proof that no TCP listener exists. That also passes when the hostname fails to resolve (exit 6), which proves nothing — the same class of test-passing- for-the-wrong-reason the guard was written to catch. Now requires exit 7 specifically, so the host must have resolved and actively refused the connection, and reports anything else as inconclusive rather than silently counting it as a pass. --- deploy/test.sh | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/deploy/test.sh b/deploy/test.sh index 447cadc..bd10d69 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -95,13 +95,29 @@ echo "" # so without this nothing here would notice a TCP listener coming back. echo "--- Transport ---" -if curl -s -o /dev/null --max-time 3 --connect-timeout 2 "http://${PROXY_HOST:-proxy}:2375/_ping" 2>/dev/null; then - echo " FAIL: proxy answered on TCP 2375 (it must listen on a Unix socket only)" - FAIL=$((FAIL+1)) -else - echo " PASS: no TCP listener on 2375" - PASS=$((PASS+1)) -fi +# Assert the specific failure rather than "curl failed somehow": exit 7 is +# connection refused, which proves the host resolved and nothing accepted on +# 2375. Treating any non-zero exit as success would also pass when the name +# does not resolve (exit 6), which proves nothing at all. +curl -s -o /dev/null --max-time 3 --connect-timeout 2 \ + "http://${PROXY_HOST:-proxy}:2375/_ping" 2>/dev/null +rc=$? +case "$rc" in + 0) + echo " FAIL: proxy answered on TCP 2375 (it must listen on a Unix socket only)" + FAIL=$((FAIL+1)) + ;; + 7) + echo " PASS: no TCP listener on 2375 (connection refused)" + PASS=$((PASS+1)) + ;; + *) + echo " FAIL: inconclusive TCP probe of ${PROXY_HOST:-proxy}:2375 (curl exit $rc);" + echo " expected 7 (connection refused). Exit 6 means the name did not" + echo " resolve, so this assertion would prove nothing." + FAIL=$((FAIL+1)) + ;; +esac # ─── Read-only endpoints (always allowed) ─────────────────────── From 2e90e7f98ab76a17c30482ff64c85e430256cdda Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Wed, 23 Sep 2026 12:14:08 -0400 Subject: [PATCH 5/5] fix(test): stop set -e aborting the TCP probe before its exit code is read bc0ec98 replaced a protected `if curl ...; then` with a bare curl followed by `rc=$?`. Under set -e the bare failing command terminates the script immediately, so the probe never reported and the integration job died right after printing '--- Transport ---'. The same trap this suite's status helpers were already written to avoid. Guarded with `|| rc=$?`, which keeps the expected failure from aborting the run. Verified both forms directly: the guarded one reaches rc=7, the bare one never reaches the next line. --- deploy/test.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/deploy/test.sh b/deploy/test.sh index bd10d69..0f5ad65 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -99,9 +99,11 @@ echo "--- Transport ---" # connection refused, which proves the host resolved and nothing accepted on # 2375. Treating any non-zero exit as success would also pass when the name # does not resolve (exit 6), which proves nothing at all. +# `|| rc=$?` keeps set -e from aborting here: this curl is expected to fail, +# and a bare failing command would terminate the script before rc is read. +rc=0 curl -s -o /dev/null --max-time 3 --connect-timeout 2 \ - "http://${PROXY_HOST:-proxy}:2375/_ping" 2>/dev/null -rc=$? + "http://${PROXY_HOST:-proxy}:2375/_ping" 2>/dev/null || rc=$? case "$rc" in 0) echo " FAIL: proxy answered on TCP 2375 (it must listen on a Unix socket only)"