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..50a4bd3 100644 --- a/README.md +++ b/README.md @@ -212,20 +212,35 @@ 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 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 @@ -235,7 +250,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/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 fadf433..6bb64c9 100644 --- a/deploy/docker-compose.sock.yml +++ b/deploy/docker-compose.sock.yml @@ -10,6 +10,16 @@ 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. + # + # 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 \ @@ -17,6 +27,22 @@ 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. + # + # 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 && [ "$$(stat -c '%a %G' /sock/docker.sock)" = "660 dockertest" ] + interval: 1s + timeout: 2s + retries: 60 + start_period: 30s proxy-granted: build: @@ -26,16 +52,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,27 +70,27 @@ 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 test: - image: docker:28-cli + build: + context: . + dockerfile: Dockerfile.test depends_on: - 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..17fbc2b 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -6,24 +6,34 @@ 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 test: - image: docker:28-cli + build: + context: . + dockerfile: Dockerfile.test depends_on: - proxy environment: - DOCKER_HOST: tcp://proxy:2375 + 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 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..aed5989 100755 --- a/deploy/test-sock.sh +++ b/deploy/test-sock.sh @@ -8,14 +8,40 @@ 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. +# 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 + 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() { - 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}' $TIMEOUT --unix-socket "$1" "$2" 2>/dev/null || true) + echo "${out:-000}" } 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}' $TIMEOUT --unix-socket "$1" \ + -X POST -H "Content-Type: application/json" -d "$2" "$3" 2>/dev/null || true) + echo "${out:-000}" } check() { @@ -40,10 +66,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 @@ -54,15 +80,23 @@ 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: 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 @@ -72,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 "" @@ -80,17 +119,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 +145,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..0f5ad65 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -8,33 +8,31 @@ 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" +# 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. +# 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 + echo "ERROR: curl is missing from the test image (see deploy/Dockerfile.test)" exit 1 fi -echo "" + +# 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 wget -qO- "$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 @@ -49,15 +47,26 @@ 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. +# 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() { - 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}' $TIMEOUT --unix-socket "$PROXY_SOCK" "$1" 2>/dev/null || true) + echo "${out:-000}" } 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}' $TIMEOUT --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "$1" "$2" 2>/dev/null || true) + echo "${out:-000}" } 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}' $TIMEOUT --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: application/json" -d "" "$1" 2>/dev/null || true) + echo "${out:-000}" } check() { @@ -79,6 +88,39 @@ 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 ---" + +# 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. +# `|| 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=$? +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) ─────────────────────── echo "--- Read-only endpoints ---" diff --git a/go/main.go b/go/main.go index fd48649..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" @@ -22,8 +26,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", @@ -34,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() @@ -56,8 +67,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 +85,101 @@ func main() { slog.Info("shutdown complete") } -func startListener(ctx context.Context, network, addr string, handler http.Handler) { - var listener net.Listener - var err error +// 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 +} - 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) +// 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) } - } else { - listener, err = net.Listen("tcp", 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 { - slog.Error("failed to start listener", "network", network, "addr", addr, "error", err) - return + 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: 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 == 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) + } + + 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 +188,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/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 a314af3..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; @@ -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, @@ -61,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()); @@ -76,13 +82,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 +109,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,23 +130,93 @@ 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 -/// 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 @@ -145,23 +230,33 @@ 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) } 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 +284,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, @@ -263,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(); @@ -300,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 921160a..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, parseHostPort, parseSocketPath } from "./flags.js"; +import { + getFlag, + hasFlag, + parseListenSocket, + parseSocketPath, + validateFlags, +} from "./flags.js"; describe("flags", () => { describe("getFlag", () => { @@ -30,7 +36,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 +64,110 @@ 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("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"), { + 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("rejects socket activation on any fd other than 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("defaults to all interfaces for a bare port", () => { - assert.deepEqual(parseHostPort("2375"), { host: "0.0.0.0", port: 2375 }); + 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("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 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", () => { + 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..f3338ca 100644 --- a/ts/src/flags.ts +++ b/ts/src/flags.ts @@ -19,18 +19,95 @@ 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) }; - } - return { host: input.slice(0, colon), port: parseInt(input.slice(colon + 1), 10) }; +// 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; + +// 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.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" }; + } + 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..47163d7 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -1,22 +1,42 @@ import { createServer } from "node:http"; +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, parseHostPort, 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) { 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 +60,61 @@ 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. +// 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. + 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 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 { + if (!lstatSync(path).isSocket()) { + console.error(`refusing to remove ${path}: not a socket`); + process.exit(1); + } + unlinkSync(path); + } catch (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(path, () => { + console.log(`listening on unix socket ${path}`); + }); +} + function shutdown(signal: string) { console.log(`received ${signal}, shutting down...`); server.close(() => {