diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c40481..4a4638d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,7 @@ jobs: node-version: "22" - run: npm install -g @informalsystems/quint - run: make typecheck + - run: make test-spec - run: make verify BACKEND=typescript go: diff --git a/AGENTS.md b/AGENTS.md index 37e9935..e69d057 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,8 +18,10 @@ deploy/ — Docker Compose + integration tests - `make build-go` / `make test-go` / `make lint-go` — Go only - `make build-rs` / `make test-rs` / `make lint-rs` — Rust only - `make build-ts` / `make test-ts` / `make lint-ts` — TypeScript only -- `make verify` — Quint spec simulation +- `make verify` — Quint spec simulation (request handling + listener) +- `make test-spec` — Quint `run` tests for `spec/listener.qnt` - `make test-integration` — Docker Compose integration tests +- `make test-integration-sock` — listening-socket integration tests (`IMPL=rs|ts` for the others) ## Architecture (same across all 3 languages) - `policy/` — Policy types + Manager (loads YAML from config dir) @@ -35,16 +37,17 @@ deploy/ — Docker Compose + integration tests - Zero external deps where possible (Go: yaml.v3, Rust: tokio/hyper/serde/clap, TS: yaml) ## Test Coverage -- Go: 74 unit tests (policy: 10, middleware: 29, proxy: 31, audit: 4) -- Rust: 112 unit tests (policy: 15, middleware: 50, proxy: 37, handler: 4, audit: 4, transport: 2) -- TypeScript: 108 unit tests (policy: 10, middleware: 37, proxy: 26, transport: 5, handler: 6, flags: 16, audit: 4) -- 26 integration tests via deploy/test.sh + docker-compose +- Go: 97 unit tests (main/listener: 23, policy: 10, middleware: 29, proxy: 31, audit: 4) +- Rust: 135 unit tests (main/listener: 23, policy: 15, middleware: 50, proxy: 37, handler: 4, audit: 4, transport: 2) +- TypeScript: 154 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 41, proxy: 26, policy: 10, handler: 6, shutdown: 5, transport: 5, audit: 4) +- Integration, per implementation: 27 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) +- Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests ## Test Conventions - Go: stdlib `testing` package, `go test ./...` - Rust: `#[cfg(test)]` inline modules, `cargo test` - TypeScript: `node:test` framework, `npm run build && node --test dist/*.test.js` -- Integration: `make test-integration` (26 test cases via Docker Compose) +- Integration: `make test-integration` (27 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/Makefile b/Makefile index a103575..69f2c88 100644 --- a/Makefile +++ b/Makefile @@ -3,9 +3,10 @@ OUTPUT_DIR ?= . VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) QUINT ?= $(shell command -v quint 2>/dev/null || echo node $$HOME/.hermes/node/lib/node_modules/@informalsystems/quint/dist/src/cli.js) SPEC ?= spec/docker_socket_policy.qnt +LISTENER_SPEC ?= spec/listener.qnt BACKEND ?= -.PHONY: build clean test lint verify typecheck validate ci-verify release-verify +.PHONY: build clean test lint verify typecheck test-spec validate ci-verify release-verify .PHONY: build-go test-go lint-go build-rs test-rs build-ts test-ts # ─── Go ────────────────────────────────────────────── @@ -65,19 +66,27 @@ clean: typecheck: $(QUINT) typecheck $(SPEC) + $(QUINT) typecheck $(LISTENER_SPEC) verify: if [ -n "$(BACKEND)" ]; then \ - $(QUINT) run --max-steps=100 --invariants allInvariants --backend $(BACKEND) $(SPEC); \ + $(QUINT) run $(SPEC) --max-steps=100 --invariants allInvariants --backend $(BACKEND) && \ + $(QUINT) run $(LISTENER_SPEC) --main=listener_locked --max-steps=30 --invariant allListenerInvariants --backend $(BACKEND); \ else \ - $(QUINT) run --max-steps=100 --invariants allInvariants $(SPEC); \ + $(QUINT) run $(SPEC) --max-steps=100 --invariants allInvariants && \ + $(QUINT) run $(LISTENER_SPEC) --main=listener_locked --max-steps=30 --invariant allListenerInvariants; \ fi +test-spec: + $(QUINT) test $(LISTENER_SPEC) --main=listener_locked + $(QUINT) test $(LISTENER_SPEC) --main=listener_unlocked + verify-ts: - $(QUINT) run --max-steps=50 --invariants allInvariants --backend typescript $(SPEC) + $(QUINT) run $(SPEC) --max-steps=50 --invariants allInvariants --backend typescript ci-verify: $(MAKE) typecheck + $(MAKE) test-spec $(MAKE) verify BACKEND=typescript $(MAKE) test-all $(MAKE) test-integration @@ -85,6 +94,7 @@ ci-verify: release-verify: $(MAKE) typecheck + $(MAKE) test-spec $(MAKE) verify BACKEND=rust $(MAKE) test-all $(MAKE) test-integration diff --git a/README.md b/README.md index dd1d706..3c2ee0f 100644 --- a/README.md +++ b/README.md @@ -123,17 +123,22 @@ make validate ### Run ```bash -./docker-socket-policy \ +sudo groupadd --system docker-socket-policy +sudo usermod -aG docker-socket-policy alice + +sudo ./docker-socket-policy \ --listen-socket=/var/run/docker-socket-policy.sock \ - --listen-socket-group=builders \ --docker-host=/var/run/docker.sock \ --config-dir=./config \ --log-file=/tmp/docker-socket-policy.log ``` -The socket is created `0660` owned by `--listen-socket-group`, so members of -that group can connect and nobody else can. Omit the flag and only the proxy's -own user can reach it. +Like `docker.sock`, the socket is always created `0660` and owned by the +`docker-socket-policy` group, so members of that group can connect and nobody +else can. Grant or revoke access with group membership alone. If the group +does not exist, the proxy warns and uses its own group instead. See the +Unix socket security boundary note under [CLI flags](#cli-flags) for the +details. ### Configure a Service @@ -217,13 +222,12 @@ docker pull attacker/malware:latest # denied: image not in allowlist | Flag | Default | Description | |------|---------|-------------| -| `--listen-socket` | `/var/run/docker-socket-policy.sock` | Unix socket to listen on (or `fd://3` for systemd) | +| `--listen-socket` | `/var/run/docker-socket-policy.sock` | Unix socket path to listen on (absolute filesystem path only) | | `--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 | -| `--listen-socket-mode` | `0660` | Octal mode for the listening socket (ignored for `fd://3`) | -| `--listen-socket-group` | *(none)* | Group name or gid owning the listening socket (ignored for `fd://3`) | +| `--listen-socket-group` | `docker-socket-policy` | Group owning the socket (default `docker-socket-policy`; `""` = the proxy's own group) | > **Unix socket security boundary**: the proxy listens on a Unix socket only, > in all three implementations. Access control is the file permissions and Unix @@ -235,25 +239,76 @@ 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 access, set `--listen-socket-group` to a group, place the caller's -> container user in that group, 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`. +> The socket works like `docker.sock`. It is always `0660`, whatever the +> ambient umask, and there is no flag to change the mode. `connect(2)` on a +> Unix socket needs **write** permission, so any other mode either locks the +> group out or opens the socket to every local uid. The group is chosen the way +> dockerd chooses the `docker` group: +> +> | `--listen-socket-group` | Group exists | Socket group | +> |---|---|---| +> | not passed | yes | `docker-socket-policy` | +> | not passed | no | the proxy's own group, with the warning `group docker-socket-policy not found, using the proxy's own group ` | +> | `=name` | yes | that group | +> | `=name` | no | none: startup fails, exit 2 | +> | `=gid` | — | that gid, used as-is (no lookup). Digits only, `0-4294967294`; a larger value fails with exit 2 | +> | `=""` | — | the proxy's own group, no warning | +> +> To grant access, create the group once and add callers to it: +> +> ```bash +> groupadd --system docker-socket-policy +> usermod -aG docker-socket-policy alice +> ``` +> +> For a container caller, give its user that group (`group_add:`) and +> bind-mount the socket in. To revoke access, remove the group membership. If +> the proxy cannot reach the daemon socket because of its own group +> permissions, requests surface as `403`. +> +> To give the socket a group other than its own, a non-root proxy must be a +> member of that group (`SupplementaryGroups=` / `group_add:`). Otherwise +> startup fails with exit 1 and the message `cannot give to group : +> the proxy's user must be a member of it`. The proxy does not fall back to +> another group, because a group that exists was chosen on purpose. +> +> The socket is bound at `0600`, then given its group, and only then widened +> to `0660`. It is never reachable by the wrong group, even for a moment. > -> The socket is created at `--listen-socket-mode` (default `0660`) regardless of -> the ambient umask. This matters: `bind(2)` applies `0777 & ~umask`, so left to -> a default umask the socket would be `0755`, and `connect(2)` on a Unix socket -> requires **write** permission — the group grant above would silently not work. -> Under `umask 0` it would be `0777`, reachable by every local uid. A -> world-writable mode is rejected at startup and there is no opt-out. +> **One instance per socket path.** At startup the proxy handles what it finds +> at the path as follows: > -> Without `--listen-socket-group` the socket is `0660` owned by the proxy's own -> user and group, so only that user can connect. The group is what makes the -> mode useful. +> - A stale socket (`connect(2)` is refused) is removed and replaced. +> - A live socket is refused with ` is in use by another process`, exit 1. +> The proxy leaves that socket untouched. +> - A socket that fails `connect(2)` in any other way (for example `EACCES`) +> is refused, and the proxy does not remove it. +> - Anything that is not a socket is refused, and the proxy does not remove it. > -> Under `fd://3` the socket belongs to systemd: use `SocketMode=` and -> `SocketGroup=` in the `.socket` unit instead, as in the example below. Both -> flags are ignored in that mode. +> The Go and Rust implementations also take an exclusive `flock` on +> `.lock` (mode `0600`) before they touch the socket. They hold it for +> the life of the process. A second instance fails with ` is in use by +> another instance (lock .lock held)`, exit 1. The kernel releases the +> lock on any exit, including `SIGKILL`, so a crash never leaves a stale lock. +> The `.lock` file stays next to the socket after shutdown. **Do not delete +> it**, especially while the proxy is running: deleting it lets a second +> instance take the socket. A `.lock` left by another uid (for example an +> earlier run as root on a persistent volume) makes startup fail with +> `opening lock …` and a permission-denied error, exit 1; delete that lock file only when +> no instance is running. +> +> *TypeScript exception:* Node has no `flock`, so the TypeScript +> implementation takes no lock and relies on the live-socket check alone. If +> two TypeScript instances start on the same path within the same few +> milliseconds, both can see the old socket as stale. The second one then +> removes the first one's new socket and binds its own, and the first keeps +> running but nothing can reach it. An instance that starts after another is +> already listening is still refused. Node also unlinks its socket path on +> close, so stopping the orphaned instance (the obvious remedy) deletes the +> surviving instance's live socket; restart the survivor afterwards. The same +> holds when a Go or Rust instance is the orphan in a race with TypeScript. +> This gap is tracked in +> [#46](https://github.com/ChainSafe/docker-socket-policy/issues/46). > > **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 @@ -264,32 +319,51 @@ docker pull attacker/malware:latest # denied: image not in allowlist > 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 +### systemd Service -**`docker-socket-policy.socket`**: -```ini -[Socket] -ListenStream=/var/run/docker-socket-policy.sock -SocketMode=0660 -SocketGroup=builders +The proxy always creates its own socket. systemd socket activation +(`fd://`) is not supported, so the proxy never listens on a socket it did not +create. Run it as a plain service: + +```bash +sudo groupadd --system docker-socket-policy # skip if it already exists +sudo useradd --system --no-create-home -g docker-socket-policy docker-socket-policy +sudo usermod -aG docker-socket-policy alice # grant a caller access ``` **`docker-socket-policy.service`**: ```ini [Service] ExecStart=/usr/local/bin/docker-socket-policy \ - --listen-socket=fd://3 \ + --listen-socket=/run/docker-socket-policy/docker-socket-policy.sock \ --docker-host=/var/run/docker.sock \ --config-dir=/etc/docker-socket-policy/services \ - --log-file=/var/log/docker-socket-policy.log + --log-file=/var/log/docker-socket-policy/audit.log User=docker-socket-policy +Group=docker-socket-policy +# Reach the Docker daemon socket. +SupplementaryGroups=docker +# A non-root proxy cannot create files in /var/run; systemd creates this +# directory for it, owned by User=/Group=. +RuntimeDirectory=docker-socket-policy +RuntimeDirectoryMode=0755 +LogsDirectory=docker-socket-policy Restart=on-failure NoNewPrivileges=true ``` +`Group=docker-socket-policy` makes that group the proxy's own group, so it +can give the socket to it. `docker-socket-policy.sock.lock` is created next +to the socket in the same directory. Callers then use +`DOCKER_HOST=unix:///run/docker-socket-policy/docker-socket-policy.sock`. + +To use a different group, pass `--listen-socket-group=` and add that +group to `SupplementaryGroups=`. Otherwise startup fails with the +"must be a member of it" error. + ## Formal Verification -This project includes a [Quint](https://quint-lang.org/) formal specification that models the security invariants as a state machine. Random-simulation verification runs 10,000 sampled traces of up to 100 steps each, checking all 9 invariants on every state transition. +This project includes a [Quint](https://quint-lang.org/) formal specification that models the security invariants as a state machine. Random-simulation verification runs 10,000 sampled traces of up to 100 steps each, checking all 9 invariants on every state transition. A second module, `spec/listener.qnt`, models listening-socket startup (group selection, existing-path checks, the single-instance lock) with 6 more invariants. The CI pipeline runs verification on every push and PR. A violation blocks the build. @@ -297,6 +371,7 @@ The CI pipeline runs verification on every push and PR. A violation blocks the b make typecheck # Quint type-check (proves type safety) make verify # Random-simulation verification (default evaluator) make verify BACKEND=rust # Same, using the faster Rust backend +make test-spec # Quint `run` tests for listener.qnt (one per design-table row) make validate # All checks: typecheck + verify + go vet + go test ``` diff --git a/deploy/config/group b/deploy/config/group new file mode 100644 index 0000000..a1a7512 --- /dev/null +++ b/deploy/config/group @@ -0,0 +1 @@ +docker-socket-policy:x:2001: diff --git a/deploy/docker-compose.sock.yml b/deploy/docker-compose.sock.yml index 48dcec9..42c597b 100644 --- a/deploy/docker-compose.sock.yml +++ b/deploy/docker-compose.sock.yml @@ -59,10 +59,28 @@ services: command: - --docker-host=/sock/docker.sock - --listen-socket=/sock/granted.sock - # Exercises the #40 flags: the socket must come out 0660 owned by this - # group regardless of the image's umask. - - --listen-socket-mode=0660 - - --listen-socket-group=2001 + - --config-dir=/etc/docker-socket-policy/services + - --log-file=/tmp/docker-socket-policy.log + + proxy-default-group: + build: + context: ../${IMPL:-go} + dockerfile: Dockerfile + # Primary GID 65532, with 2001 only as a supplementary group. The mounted + # /etc/group names 2001 docker-socket-policy, so the socket must come out + # owned by that default group rather than by the proxy's egid. + user: 65532:65532 + group_add: ["2001"] + depends_on: + sock-perms: + condition: service_healthy + volumes: + - ./config:/etc/docker-socket-policy/services:ro + - ./config/group:/etc/group:ro + - sock-data:/sock + command: + - --docker-host=/sock/docker.sock + - --listen-socket=/sock/default.sock - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -81,8 +99,6 @@ services: command: - --docker-host=/sock/docker.sock - --listen-socket=/sock/denied.sock - - --listen-socket-mode=0660 - - --listen-socket-group=3001 - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -93,9 +109,11 @@ services: depends_on: - proxy-granted - proxy-denied + - proxy-default-group environment: PROXY_GRANTED_SOCK: /sock/granted.sock PROXY_DENIED_SOCK: /sock/denied.sock + PROXY_DEFAULT_SOCK: /sock/default.sock volumes: - ./test-sock.sh:/test-sock.sh:ro - sock-data:/sock diff --git a/deploy/test-sock.sh b/deploy/test-sock.sh index 474f8d6..5204025 100755 --- a/deploy/test-sock.sh +++ b/deploy/test-sock.sh @@ -3,6 +3,8 @@ # Tests that the proxy correctly handles group-restricted Docker sockets. # proxy-granted runs with GID 2001 (in dockertest group) → should work # proxy-denied runs with GID 3001 (not in dockertest group) → should fail with 403 +# proxy-default-group runs with GID 65532 plus supplementary 2001, which the +# mounted /etc/group names docker-socket-policy → socket gets the default group set -e @@ -13,6 +15,7 @@ FAIL=0 # 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}" +DEFAULT_SOCK="${PROXY_DEFAULT_SOCK:-/sock/default.sock}" URL="http://localhost" # busybox wget cannot speak to a Unix socket, so the helpers below need curl. @@ -113,6 +116,30 @@ if [ $i -eq 15 ]; then fi exit 1 fi + +# proxy-default-group has group access to docker.sock like proxy-granted, so +# it answers 200 once ready. +echo "Waiting for proxy-default-group at $DEFAULT_SOCK..." +i=0 +while [ $i -lt 30 ]; do + S=$(get_status "$DEFAULT_SOCK" "$URL/_ping") + if [ "$S" = "200" ]; then + echo "proxy-default-group ready." + break + fi + printf "." + sleep 1 + i=$((i + 1)) +done +if [ $i -eq 30 ]; then + echo "" + if [ ! -S "$DEFAULT_SOCK" ]; then + echo "ERROR: proxy-default-group never created $DEFAULT_SOCK — it failed to bind" + else + echo "ERROR: proxy-default-group is listening but not answering after 30s" + fi + exit 1 +fi echo "" # ─── Listening socket permissions ───────────────────── @@ -127,7 +154,7 @@ MODE=$(stat -c '%a' "$GRANTED_SOCK" 2>/dev/null || echo "?") check "granted.sock mode is 660, not the umask default" "660" "$MODE" GROUP=$(stat -c '%g' "$GRANTED_SOCK" 2>/dev/null || echo "?") -check "granted.sock is owned by --listen-socket-group 2001" "2001" "$GROUP" +check "granted.sock is owned by the proxy's own group 2001 (default group absent)" "2001" "$GROUP" # The specific failure mode that removes the boundary entirely. case "$MODE" in @@ -144,6 +171,15 @@ esac MODE=$(stat -c '%a' "$DENIED_SOCK" 2>/dev/null || echo "?") check "denied.sock mode is 660" "660" "$MODE" +# proxy-default-group runs with primary gid 65532 but has docker-socket-policy +# (2001) in its mounted /etc/group and its supplementary groups, so the +# dockerd-style default applies instead of the egid fallback. +MODE=$(stat -c '%a' "$DEFAULT_SOCK" 2>/dev/null || echo "?") +check "default.sock mode is 660" "660" "$MODE" + +GROUP=$(stat -c '%g' "$DEFAULT_SOCK" 2>/dev/null || echo "?") +check "default.sock owned by docker-socket-policy (2001)" "2001" "$GROUP" + echo "" # ─── proxy-granted: should work ─────────────────────── @@ -169,6 +205,14 @@ else FAIL=$((FAIL+1)) fi +# ─── proxy-default-group: should work ───────────────── + +echo "" +echo "--- proxy-default-group (GID 65532, supplementary 2001) ---" + +S=$(get_status "$DEFAULT_SOCK" "$URL/_ping") +check "GET /_ping -> 200" "200" "$S" + # ─── proxy-denied: should return 403 ────────────────── echo "" diff --git a/go/main.go b/go/main.go index 8cbc6ea..4ba0f03 100644 --- a/go/main.go +++ b/go/main.go @@ -12,6 +12,7 @@ import ( "os" "os/signal" "os/user" + "runtime" "strconv" "strings" "syscall" @@ -27,7 +28,7 @@ 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)") + "Unix socket path 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", @@ -36,10 +37,10 @@ func main() { "Audit log file (JSON)") readonly := flag.Bool("readonly", false, "Enable read-only mode (deny all POST/PUT/DELETE)") - listenSocketMode := flag.String("listen-socket-mode", "0660", - "Octal mode for the listening socket (ignored for fd://3)") - listenSocketGroup := flag.String("listen-socket-group", "", - "Group name or gid to own the listening socket (ignored for fd://3)") + // The value is read with flagValueIfSet, never through the returned + // pointer: absent (default group) and "" (own group) must stay distinct. + flag.String("listen-socket-group", "", + `Group owning the socket (default docker-socket-policy; "" = the proxy's own group)`) flag.Parse() if err := validateListenSocket(*listenSocket); err != nil { @@ -50,15 +51,14 @@ func main() { slog.Error(err.Error()) os.Exit(2) } - socketMode, err := parseSocketMode(*listenSocketMode) + socketGID, warning, err := selectSocketGroup( + flagValueIfSet(flag.CommandLine, "listen-socket-group"), resolveGroup, os.Getegid()) if err != nil { slog.Error(err.Error()) os.Exit(2) } - socketGID, err := resolveGroup(*listenSocketGroup) - if err != nil { - slog.Error(err.Error()) - os.Exit(2) + if warning != "" { + slog.Warn(warning) } ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) @@ -83,7 +83,7 @@ func main() { transport := proxy.NewTransport(*dockerHost) handler := proxy.NewHandler(router, chain, auditLog, transport) - listener, err := unixListener(*listenSocket, socketMode, socketGID) + listener, lock, err := openListener(*listenSocket, socketGID) if err != nil { slog.Error("failed to start listener", "addr", *listenSocket, "error", err) os.Exit(1) @@ -98,13 +98,15 @@ func main() { // stall every shutdown for the full timeout: docker stop allows 10s before // SIGKILL, so the proxy would never shut down gracefully at all. <-done + // Release the lock only once the socket is closed and unlinked, so the + // next instance never finds our socket still in place. + lock.Close() slog.Info("shutdown complete") + // *os.File closes its fd from a finalizer once unreachable, which would + // drop the flock early; keep lock reachable until main returns. + runtime.KeepAlive(lock) } -// 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 @@ -115,12 +117,8 @@ 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://"), + case strings.HasPrefix(addr, "fd://"), + strings.HasPrefix(addr, "tcp://"), strings.HasPrefix(addr, "http://"), strings.HasPrefix(addr, "https://"), strings.HasPrefix(addr, "unix://"): @@ -149,31 +147,10 @@ func validateDockerHost(addr string) error { 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 -} - -// defaultListenSocketMode is the mode applied to the listening socket when -// --listen-socket-mode is not given. connect(2) on an AF_UNIX socket requires -// write permission, so 0660 is what actually grants the owning group access. -const defaultListenSocketMode = 0o660 +// socketMode is the mode applied to the listening socket. connect(2) on an +// AF_UNIX socket requires write permission, so 0660 is what actually grants the +// owning group access. +const socketMode os.FileMode = 0o660 // bindUmask is set around bind(2) so the socket is created at 0600 and is never // briefly reachable by group or world. bind() applies 0777 &^ umask, and @@ -181,17 +158,62 @@ const defaultListenSocketMode = 0o660 // window in which the socket is already listening at the ambient mode. const bindUmask = 0o177 -// resolveGroup maps --listen-socket-group to a gid. A numeric value is used +// defaultSocketGroup is the group the socket is given when +// --listen-socket-group is not passed, as dockerd defaults to "docker". +const defaultSocketGroup = "docker-socket-policy" + +// flagValueIfSet returns the value of the named flag if it was passed on the +// command line, or nil if it was not. It distinguishes an absent flag from one +// explicitly set to "". +func flagValueIfSet(fs *flag.FlagSet, name string) *string { + var value *string + fs.Visit(func(f *flag.Flag) { + if f.Name == name { + v := f.Value.String() + value = &v + } + }) + return value +} + +// selectSocketGroup picks the socket's group the way dockerd does +// (moby/daemon/listeners/listeners_linux.go). A nil flagValue means the flag +// was not passed: the default group is used if it exists, and otherwise the +// proxy falls back to its own group with a warning. An explicit group that +// does not resolve is an error. An explicit "" selects the proxy's own group. +func selectSocketGroup(flagValue *string, lookup func(string) (int, error), egid int) (int, string, error) { + if flagValue == nil { + gid, err := lookup(defaultSocketGroup) + if err != nil { + return egid, fmt.Sprintf("group %s not found, using the proxy's own group %d", + defaultSocketGroup, egid), nil + } + return gid, "", nil + } + if *flagValue == "" { + return egid, "", nil + } + gid, err := lookup(*flagValue) + if err != nil { + return -1, "", err + } + return gid, "", nil +} + +// resolveGroup maps a group name or gid to a gid. A numeric value is used // as-is so deployments without the group in /etc/group (or NSS) still work. func resolveGroup(group string) (int, error) { - if group == "" { - return -1, nil - } - if gid, err := strconv.Atoi(group); err == nil { - if gid < 0 { - return -1, fmt.Errorf("--listen-socket-group %q: negative gid", group) + if isAllDigits(group) { + // 4294967295 is chown's "don't change" sentinel, and chown keeps only + // the low 32 bits of anything larger (4294967296 would become root). + gid, err := strconv.ParseUint(group, 10, 32) + if err != nil || gid > maxSocketGid { + return -1, fmt.Errorf("--listen-socket-group %q: gid out of range (0-%d)", group, maxSocketGid) } - return gid, nil + return int(gid), nil + } + if strings.HasPrefix(group, "-") && isAllDigits(group[1:]) { + return -1, fmt.Errorf("--listen-socket-group %q: negative gid", group) } g, err := user.LookupGroup(group) if err != nil { @@ -204,25 +226,105 @@ func resolveGroup(group string) (int, error) { return gid, nil } -// parseSocketMode accepts an octal mode and rejects anything world-writable. -// A world-writable socket is connectable by every local uid, which removes the -// boundary entirely, so there is deliberately no opt-out. -func parseSocketMode(s string) (os.FileMode, error) { +const maxSocketGid = 4294967294 + +func isAllDigits(s string) bool { if s == "" { - return 0, fmt.Errorf("--listen-socket-mode must not be empty") + return false + } + for _, c := range s { + if c < '0' || c > '9' { + return false + } + } + return true +} + +// openListener takes the single-instance lock, clears the socket path and +// binds it. The returned lock must be held, and kept reachable, for the life +// of the process: dropping it would let a second instance take the path. +func openListener(addr string, gid int) (net.Listener, *os.File, error) { + lock, err := acquireInstanceLock(addr) + if err != nil { + return nil, nil, err + } + if err := prepareSocketPath(addr); err != nil { + lock.Close() + return nil, nil, err + } + l, err := unixListener(addr, gid) + if err != nil { + lock.Close() + return nil, nil, err + } + return l, lock, nil +} + +// acquireInstanceLock takes an exclusive flock on .lock, which +// replaces dockerd's pidfile. The kernel drops the lock on any exit, including +// SIGKILL, so it never goes stale, and it needs no PID check, so it also works +// across PID namespaces. The file is never truncated or unlinked: unlinking a +// lock file reopens the race it exists to close. O_NOFOLLOW stops a symlink +// planted at the lock path from redirecting O_CREAT elsewhere. +func acquireInstanceLock(socketPath string) (*os.File, error) { + lockPath := socketPath + ".lock" + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR|syscall.O_CLOEXEC|syscall.O_NOFOLLOW, 0o600) + if err != nil { + return nil, fmt.Errorf("opening lock %s: %w", lockPath, err) + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + f.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, fmt.Errorf("%s is in use by another instance (lock %s held)", socketPath, lockPath) + } + return nil, fmt.Errorf("locking %s: %w", lockPath, err) + } + return f, nil +} + +// probeTimeout bounds the connect(2) that tells a live socket from a stale one. +const probeTimeout = time.Second + +// prepareSocketPath clears the socket path for bind, following the +// existing-path table in spec/listener-design.md. Only a socket that refuses +// connections is removed. A live one belongs to another process, possibly an +// instance that takes no lock (TypeScript, or v0.2.21 and earlier), and +// replacing it would cut that process off silently. Anything that is not a +// socket is refused: os.Remove also unlinks regular files and rmdir's empty +// directories, so a mistyped path would silently delete an operator's data. +func prepareSocketPath(path string) error { + info, err := os.Lstat(path) + if errors.Is(err, fs.ErrNotExist) { + return nil } - m, err := strconv.ParseUint(s, 8, 32) if err != nil { - return 0, fmt.Errorf("--listen-socket-mode %q: not an octal mode", s) + return fmt.Errorf("checking %s: %w", path, err) + } + if info.Mode()&fs.ModeSocket == 0 { + return fmt.Errorf("refusing to remove %s: not a socket (mode %s)", path, info.Mode()) + } + + conn, err := net.DialTimeout("unix", path, probeTimeout) + if err == nil { + conn.Close() + return fmt.Errorf("%s is in use by another process", path) + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return fmt.Errorf("%s is in use by another process", path) } - if m > 0o777 { - return 0, fmt.Errorf("--listen-socket-mode %q: must be within 0777", s) + if !errors.Is(err, syscall.ECONNREFUSED) { + var errno syscall.Errno + if errors.As(err, &errno) { + return fmt.Errorf("refusing to remove %s: connect: %w", path, errno) + } + return fmt.Errorf("refusing to remove %s: %w", path, err) } - if m&0o002 != 0 { - return 0, fmt.Errorf("--listen-socket-mode %q is world-writable: every local user "+ - "could connect to the proxy, which disables the access-control boundary", s) + + if err := os.Remove(path); err != nil { + return fmt.Errorf("removing stale socket %s: %w", path, err) } - return os.FileMode(m), nil + return nil } // unixListener binds the proxy's only listening socket. Listening is Unix-socket @@ -233,27 +335,12 @@ func parseSocketMode(s string) (os.FileMode, error) { // Left to the umask the socket is 0755 by default — connect(2) needs write, so // the documented "add the caller to the socket's group" grant does not work — // and 0777 under umask 0, which lets any local uid drive the Docker API. -func unixListener(addr string, mode os.FileMode, gid int) (net.Listener, error) { - if addr == fmt.Sprintf("fd://%d", systemdSocketFD) { - // Under socket activation systemd owns the socket and applies its own - // SocketMode/SocketGroup. Re-chmod'ing it here would fight the unit. - 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) - } - +// +// main always passes the gid chosen by selectSocketGroup; a negative gid skips +// the chown and exists only for tests. +// +// The path must already be clear; openListener runs prepareSocketPath first. +func unixListener(addr string, gid int) (net.Listener, error) { // umask is process-global and not thread-safe. This runs during startup, // before any request handling, so nothing else is creating files. old := syscall.Umask(bindUmask) @@ -263,15 +350,19 @@ func unixListener(addr string, mode os.FileMode, gid int) (net.Listener, error) return nil, err } - // Widen from 0600 to the configured mode only after ownership is right, + // Widen from 0600 to socketMode only after ownership is right, // so the socket is never group-reachable by the wrong group. if gid >= 0 { if err := os.Chown(addr, -1, gid); err != nil { l.Close() + if errors.Is(err, syscall.EPERM) { + return nil, fmt.Errorf("cannot give %s to group %d: the proxy's user must be a member of it "+ + "(SupplementaryGroups= / group_add:)", addr, gid) + } return nil, fmt.Errorf("setting group on %s: %w", addr, err) } } - if err := os.Chmod(addr, mode); err != nil { + if err := os.Chmod(addr, socketMode); err != nil { l.Close() return nil, fmt.Errorf("setting mode on %s: %w", addr, err) } diff --git a/go/main_test.go b/go/main_test.go index 6a9662b..02ce30c 100644 --- a/go/main_test.go +++ b/go/main_test.go @@ -1,14 +1,20 @@ package main import ( + "bufio" "context" + "errors" + "flag" "fmt" "net" "net/http" "os" + "os/exec" "os/user" "path/filepath" + "runtime" "strings" + "sync" "syscall" "testing" "time" @@ -21,11 +27,12 @@ func TestValidateListenSocket(t *testing.T) { 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"}, + // Socket activation was removed; fd:// is just another scheme now. + {"fd 3", "fd://3", "only supports Unix socket paths"}, + {"other fd", "fd://4", "only supports Unix socket paths"}, + {"fd zero", "fd://0", "only supports Unix socket paths"}, + {"fd garbage", "fd://abc", "only supports Unix socket paths"}, // 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"}, @@ -61,6 +68,25 @@ func TestValidateListenSocket(t *testing.T) { } } +// TestListenSocketModeFlagRemoved guards against the flag creeping back: the +// socket mode is fixed, and a deployment still passing the flag must fail at +// startup rather than silently get a different mode than it asked for. +func TestListenSocketModeFlagRemoved(t *testing.T) { + bin := filepath.Join(t.TempDir(), "docker-socket-policy") + if out, err := exec.Command("go", "build", "-o", bin, ".").CombinedOutput(); err != nil { + t.Fatalf("go build: %v\n%s", err, out) + } + + out, err := exec.Command(bin, "--listen-socket-mode=0660").CombinedOutput() + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 2 { + t.Fatalf("running with --listen-socket-mode: err = %v, want exit status 2\n%s", err, out) + } + if !strings.Contains(string(out), "flag provided but not defined") { + t.Fatalf("output = %q, want it to contain %q", out, "flag provided but not defined") + } +} + func TestValidateDockerHost(t *testing.T) { tests := []struct { name string @@ -110,7 +136,7 @@ func shortTempDir(t *testing.T) string { func TestUnixListenerBindsFreshPath(t *testing.T) { path := filepath.Join(shortTempDir(t), "fresh.sock") - l, err := unixListener(path, defaultListenSocketMode, -1) + l, err := unixListener(path, -1) if err != nil { t.Fatalf("unixListener(%q) = %v, want nil", path, err) } @@ -126,119 +152,405 @@ func TestUnixListenerBindsFreshPath(t *testing.T) { } } -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) +// seedStaleSocket leaves a socket file at path with nothing listening on it, +// as an unclean shutdown would: connect(2) to it is refused. +func seedStaleSocket(t *testing.T, path string) { + t.Helper() + l, 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) + ul := l.(*net.UnixListener) + ul.SetUnlinkOnClose(false) + ul.Close() +} + +func inode(t *testing.T, path string) uint64 { + t.Helper() + info, err := os.Lstat(path) if err != nil { - t.Fatalf("re-seeding stale socket: %v", err) + t.Fatalf("stat %s: %v", path, err) } - _ = stale + return uint64(info.Sys().(*syscall.Stat_t).Ino) +} + +func TestOpenListenerReplacesStaleSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stale.sock") + seedStaleSocket(t, path) - l, err := unixListener(path, defaultListenSocketMode, -1) + l, lock, err := openListener(path, -1) if err != nil { - t.Fatalf("unixListener over stale socket = %v, want nil", err) + t.Fatalf("openListener over stale socket = %v, want nil", err) } l.Close() + lock.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) +// TestPrepareSocketPath has one case per row of the existing-path table in +// spec/listener-design.md; the subtest names match the Quint runs. +func TestPrepareSocketPath(t *testing.T) { + t.Run("pathAbsentBinds", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "absent.sock") + if err := prepareSocketPath(path); err != nil { + t.Fatalf("prepareSocketPath(absent) = %v, want nil", err) } + }) - if _, err := unixListener(path, defaultListenSocketMode, -1); 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) + t.Run("pathStaleReplaced", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stale.sock") + seedStaleSocket(t, path) + if err := prepareSocketPath(path); err != nil { + t.Fatalf("prepareSocketPath(stale) = %v, want nil", err) + } + if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stale socket still present after prepare: %v", 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("pathLiveRefused", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "live.sock") + l, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("binding live socket: %v", err) + } + defer l.Close() + before := inode(t, path) + + err = prepareSocketPath(path) + if want := path + " is in use by another process"; err == nil || err.Error() != want { + t.Fatalf("prepareSocketPath(live) = %v, want %q", err, want) + } + if after := inode(t, path); after != before { + t.Fatalf("live socket inode changed %d -> %d: it was replaced", before, after) + } + + accepted := make(chan error, 1) + go func() { + c, err := l.Accept() + if err == nil { + c.Close() + } + accepted <- err + }() + c, err := net.Dial("unix", path) + if err != nil { + t.Fatalf("live listener no longer reachable: %v", err) + } + c.Close() + if err := <-accepted; err != nil { + t.Fatalf("live listener no longer accepts: %v", err) } }) - t.Run("directory", func(t *testing.T) { - path := filepath.Join(shortTempDir(t), "adir") - if err := os.Mkdir(path, 0o755); err != nil { + // connect(2) needs write permission on the socket, so a 0000 socket + // yields EACCES: neither live nor provably stale, so it is left alone. + t.Run("pathConnectErrorRefused", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores socket permissions") + } + path := filepath.Join(shortTempDir(t), "eacces.sock") + seedStaleSocket(t, path) + if err := os.Chmod(path, 0); err != nil { t.Fatal(err) } + err := prepareSocketPath(path) + if want := "refusing to remove " + path + ": connect: permission denied"; err == nil || err.Error() != want { + t.Fatalf("prepareSocketPath(0000 socket) = %v, want %q", err, want) + } + if _, err := os.Lstat(path); err != nil { + t.Fatalf("socket was removed: %v", err) + } + }) - if _, err := unixListener(path, defaultListenSocketMode, -1); err == nil { - t.Fatal("unixListener over a directory = nil, want error") + t.Run("pathNotSocketRefused", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "file.txt") + if err := os.WriteFile(path, []byte("data"), 0o644); err != nil { + t.Fatal(err) + } + err := prepareSocketPath(path) + if want := "refusing to remove " + path + ": not a socket"; err == nil || !strings.HasPrefix(err.Error(), want) { + t.Fatalf("prepareSocketPath(regular file) = %v, want prefix %q", err, want) } if _, err := os.Stat(path); err != nil { - t.Fatalf("directory was removed: %v", err) + t.Fatalf("regular file 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") +// TestAcquireInstanceLock covers the Quint run secondInstanceLockRefused. +func TestAcquireInstanceLock(t *testing.T) { + path := filepath.Join(shortTempDir(t), "lock.sock") + + first, err := acquireInstanceLock(path) if err != nil { - t.Fatalf("creating TCP listener: %v", err) + t.Fatalf("first acquireInstanceLock = %v, want nil", err) + } + + _, err = acquireInstanceLock(path) + want := path + " is in use by another instance (lock " + path + ".lock held)" + if err == nil || err.Error() != want { + t.Fatalf("second acquireInstanceLock = %v, want %q", err, want) } - defer tcp.Close() - f, err := tcp.(*net.TCPListener).File() + info, err := os.Lstat(path + ".lock") if err != nil { - t.Fatalf("extracting TCP fd: %v", err) + t.Fatalf("stat lock file: %v", err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("lock file mode = %o, want 0600", got) } - defer f.Close() - l, err := listenerFromFile(f) - if err == nil { - l.Close() - t.Fatal("listenerFromFile accepted a TCP socket, want an error") + if err := first.Close(); err != nil { + t.Fatalf("closing first lock: %v", err) } - if !strings.Contains(err.Error(), "not a Unix socket") { - t.Fatalf("error = %q, want it to mention 'not a Unix socket'", err) + if _, err := os.Lstat(path + ".lock"); err != nil { + t.Fatalf("lock file removed on Close, want it kept: %v", err) } + + second, err := acquireInstanceLock(path) + if err != nil { + t.Fatalf("acquireInstanceLock after Close = %v, want nil", err) + } + second.Close() } -// 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) +// A symlink planted at .lock must not be followed: O_CREAT through it +// would create or lock a file of the attacker's choosing. +func TestAcquireInstanceLockRefusesSymlink(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "sym.sock") + target := filepath.Join(dir, "target") + if err := os.Symlink(target, path+".lock"); err != nil { + t.Fatal(err) + } + + if f, err := acquireInstanceLock(path); err == nil { + f.Close() + t.Fatal("acquireInstanceLock through a symlink = nil, want error") + } else if !strings.Contains(err.Error(), path+".lock") { + t.Fatalf("error = %q, want it to name %s.lock", err, path) + } + if _, err := os.Lstat(target); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("symlink target was created: %v", err) + } +} + +func TestAcquireInstanceLockUnreadable(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores file permissions") + } + path := filepath.Join(shortTempDir(t), "unreadable.sock") + if err := os.WriteFile(path+".lock", nil, 0o000); err != nil { + t.Fatal(err) + } + + if f, err := acquireInstanceLock(path); err == nil { + f.Close() + t.Fatal("acquireInstanceLock on a 0000 lock file = nil, want error") + } else if !strings.Contains(err.Error(), path+".lock") { + t.Fatalf("error = %q, want it to name %s.lock", err, path) + } +} + +// *os.File has a finalizer that closes the fd, which would drop the flock. +// The lock must hold for as long as main keeps its reference. +func TestInstanceLockSurvivesGC(t *testing.T) { + path := filepath.Join(shortTempDir(t), "gc.sock") + lock, err := acquireInstanceLock(path) if err != nil { - t.Fatalf("creating Unix listener: %v", err) + t.Fatalf("acquireInstanceLock = %v", err) + } + + runtime.GC() + runtime.GC() + + if f, err := acquireInstanceLock(path); err == nil { + f.Close() + t.Fatal("lock was released by GC while still referenced") + } + lock.Close() + runtime.KeepAlive(lock) +} + +func TestOpenListenerConcurrent(t *testing.T) { + dir := shortTempDir(t) + const racers = 8 + + type result struct { + l net.Listener + lock *os.File + err error + } + + for i := 0; i < 50; i++ { + path := filepath.Join(dir, fmt.Sprintf("c%d.sock", i)) + start := make(chan struct{}) + results := make(chan result, racers) + var wg sync.WaitGroup + for j := 0; j < racers; j++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + l, lock, err := openListener(path, -1) + results <- result{l, lock, err} + }() + } + close(start) + wg.Wait() + close(results) + + var winner *result + for r := range results { + if r.err == nil { + if winner != nil { + t.Fatalf("iteration %d: more than one openListener succeeded", i) + } + r := r + winner = &r + continue + } + if !strings.Contains(r.err.Error(), "is in use by another") { + t.Fatalf("iteration %d: loser error = %q, want an in-use error", i, r.err) + } + } + if winner == nil { + t.Fatalf("iteration %d: no openListener succeeded", i) + } + + accepted := make(chan error, 1) + go func() { + c, err := winner.l.Accept() + if err == nil { + c.Close() + } + accepted <- err + }() + c, err := net.Dial("unix", path) + if err != nil { + t.Fatalf("iteration %d: dialing winner: %v", i, err) + } + c.Close() + if err := <-accepted; err != nil { + t.Fatalf("iteration %d: winner did not accept: %v", i, err) + } + + winner.l.Close() + winner.lock.Close() } - defer unix.Close() +} - f, err := unix.(*net.UnixListener).File() +const holdLockEnv = "DSP_TEST_HOLD_LOCK" + +// TestHelperHoldLock is not a test on its own: TestLockReleasedOnSIGKILL +// re-executes the test binary to run it as a child that holds the lock. +func TestHelperHoldLock(t *testing.T) { + path := os.Getenv(holdLockEnv) + if path == "" { + t.Skip("helper process for TestLockReleasedOnSIGKILL") + } + lock, err := acquireInstanceLock(path) if err != nil { - t.Fatalf("extracting Unix fd: %v", err) + fmt.Println("error:", err) + os.Exit(1) } - defer f.Close() + defer runtime.KeepAlive(lock) + fmt.Println("ready") + select {} +} + +// TestLockReleasedOnSIGKILL covers the Quint run crashReleasesLock: the +// kernel drops the flock on any exit, so a killed instance never leaves a +// stale lock behind. +func TestLockReleasedOnSIGKILL(t *testing.T) { + path := filepath.Join(shortTempDir(t), "kill.sock") - l, err := listenerFromFile(f) + cmd := exec.Command(os.Args[0], "-test.run=^TestHelperHoldLock$") + cmd.Env = append(os.Environ(), holdLockEnv+"="+path) + stdout, err := cmd.StdoutPipe() if err != nil { - t.Fatalf("listenerFromFile on a Unix socket = %v, want nil", err) + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) } - defer l.Close() - if _, ok := l.(*net.UnixListener); !ok { - t.Fatalf("listenerFromFile returned %T, want *net.UnixListener", l) + ready := make(chan error, 1) + go func() { + scanner := bufio.NewScanner(stdout) + for scanner.Scan() { + if scanner.Text() == "ready" { + ready <- nil + return + } + } + ready <- fmt.Errorf("child exited before holding the lock: %v", scanner.Err()) + }() + select { + case err := <-ready: + if err != nil { + cmd.Process.Kill() + cmd.Wait() + t.Fatal(err) + } + case <-time.After(10 * time.Second): + cmd.Process.Kill() + cmd.Wait() + t.Fatal("child did not report ready within 10s") } + + if f, err := acquireInstanceLock(path); err == nil { + f.Close() + t.Fatal("acquired the lock while the child held it") + } + + if err := cmd.Process.Signal(syscall.SIGKILL); err != nil { + t.Fatal(err) + } + cmd.Wait() + + lock, err := acquireInstanceLock(path) + if err != nil { + t.Fatalf("acquireInstanceLock after SIGKILL = %v, want nil", err) + } + lock.Close() +} + +func TestOpenListenerRefusesToDeleteNonSocket(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 := openListener(path, -1); err == nil { + t.Fatal("openListener 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 := openListener(path, -1); err == nil { + t.Fatal("openListener over a directory = nil, want error") + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("directory was removed: %v", err) + } + }) } // unixClient returns an HTTP client that talks to a Unix socket. @@ -260,7 +572,7 @@ func unixClient(path string) *http.Client { // suites assert HTTP status codes and cannot observe process lifecycle. func TestServeReturnsPromptlyWhenIdle(t *testing.T) { path := filepath.Join(shortTempDir(t), "idle.sock") - l, err := unixListener(path, defaultListenSocketMode, -1) + l, err := unixListener(path, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -283,7 +595,7 @@ func TestServeReturnsPromptlyWhenIdle(t *testing.T) { // signal arrives must be allowed to finish. func TestServeWaitsForInFlightRequest(t *testing.T) { path := filepath.Join(shortTempDir(t), "inflight.sock") - l, err := unixListener(path, defaultListenSocketMode, -1) + l, err := unixListener(path, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -342,7 +654,7 @@ func TestServeWaitsForInFlightRequest(t *testing.T) { // closed, not merely ignored: a connection attempt after shutdown must fail. func TestServeStopsAcceptingAfterShutdown(t *testing.T) { path := filepath.Join(shortTempDir(t), "closed.sock") - l, err := unixListener(path, defaultListenSocketMode, -1) + l, err := unixListener(path, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -357,70 +669,24 @@ func TestServeStopsAcceptingAfterShutdown(t *testing.T) { } } -func TestParseSocketMode(t *testing.T) { - tests := []struct { - name string - in string - want os.FileMode - wantErr string - }{ - {"default", "0660", 0o660, ""}, - {"no leading zero", "660", 0o660, ""}, - {"owner only", "0600", 0o600, ""}, - {"group read only", "0640", 0o640, ""}, - {"empty", "", 0, "must not be empty"}, - {"not octal", "0x1ff", 0, "not an octal mode"}, - {"decimal 8 is invalid octal", "668", 0, "not an octal mode"}, - {"too wide", "1777", 0, "within 0777"}, - // connect(2) needs write, so o+w means every local uid can connect. - {"world writable", "0666", 0, "world-writable"}, - {"world writable 0777", "0777", 0, "world-writable"}, - {"world writable 0602", "0602", 0, "world-writable"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := parseSocketMode(tt.in) - if tt.wantErr == "" { - if err != nil { - t.Fatalf("parseSocketMode(%q) = %v, want nil", tt.in, err) - } - if got != tt.want { - t.Fatalf("parseSocketMode(%q) = %o, want %o", tt.in, got, tt.want) - } - return - } - if err == nil { - t.Fatalf("parseSocketMode(%q) = nil error, want %q", tt.in, tt.wantErr) - } - if !strings.Contains(err.Error(), tt.wantErr) { - t.Fatalf("parseSocketMode(%q) error = %q, want it to contain %q", tt.in, err, tt.wantErr) - } - }) - } -} - // TestUnixListenerAppliesMode is the regression test for #40: the mode used to // be whatever the umask left behind, which is 0755 by default. connect(2) // requires write permission, so the group grant the README documents silently // did not work, and under umask 0 the socket was 0777 to every local uid. func TestUnixListenerAppliesMode(t *testing.T) { - for _, mode := range []os.FileMode{0o660, 0o600, 0o640} { - t.Run(fmt.Sprintf("%o", mode), func(t *testing.T) { - path := filepath.Join(shortTempDir(t), "mode.sock") - l, err := unixListener(path, mode, -1) - if err != nil { - t.Fatalf("unixListener: %v", err) - } - defer l.Close() + path := filepath.Join(shortTempDir(t), "mode.sock") + l, err := unixListener(path, -1) + if err != nil { + t.Fatalf("unixListener: %v", err) + } + defer l.Close() - info, err := os.Lstat(path) - if err != nil { - t.Fatalf("stat: %v", err) - } - if got := info.Mode().Perm(); got != mode { - t.Fatalf("socket mode = %o, want %o", got, mode) - } - }) + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o660 { + t.Fatalf("socket mode = %o, want 0660", got) } } @@ -431,7 +697,7 @@ func TestUnixListenerIgnoresAmbientUmask(t *testing.T) { defer syscall.Umask(old) path := filepath.Join(shortTempDir(t), "umask.sock") - l, err := unixListener(path, 0o660, -1) + l, err := unixListener(path, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -450,9 +716,6 @@ func TestUnixListenerIgnoresAmbientUmask(t *testing.T) { } func TestResolveGroup(t *testing.T) { - if gid, err := resolveGroup(""); err != nil || gid != -1 { - t.Fatalf("resolveGroup(\"\") = %d, %v; want -1, nil", gid, err) - } // A numeric value is taken as a gid without consulting /etc/group, so a // container without the group defined can still be configured. if gid, err := resolveGroup("2001"); err != nil || gid != 2001 { @@ -469,3 +732,151 @@ func TestResolveGroup(t *testing.T) { } } } + +func TestResolveGroupRejectsOutOfRangeGid(t *testing.T) { + // 4294967295 is chown's "don't change" sentinel and larger values would + // be truncated to their low 32 bits (4294967296 -> 0, root). + if gid, err := resolveGroup("4294967294"); err != nil || gid != 4294967294 { + t.Fatalf("resolveGroup(\"4294967294\") = %d, %v; want 4294967294, nil", gid, err) + } + for _, v := range []string{"4294967295", "4294967296", "12345678901234567890"} { + _, err := resolveGroup(v) + want := fmt.Sprintf("--listen-socket-group %q: gid out of range (0-4294967294)", v) + if err == nil || err.Error() != want { + t.Fatalf("resolveGroup(%q) error = %v, want %q", v, err, want) + } + } + // Only a digit string is numeric; a sign makes it a (nonexistent) name. + for _, v := range []string{"+4294967296", "+5"} { + if gid, err := resolveGroup(v); err == nil { + t.Fatalf("resolveGroup(%q) = %d, nil; want an error", v, gid) + } + } + if _, err := resolveGroup("-5"); err == nil || !strings.Contains(err.Error(), "negative gid") { + t.Fatalf("resolveGroup(\"-5\") error = %v, want a negative gid error", err) + } +} + +// TestSelectSocketGroup has one case per row of the group-selection table in +// spec/listener-design.md; the subtest names match the Quint runs. +func TestSelectSocketGroup(t *testing.T) { + const egid = 65532 + known := func(groups map[string]int) func(string) (int, error) { + return func(name string) (int, error) { + if gid, ok := groups[name]; ok { + return gid, nil + } + return -1, fmt.Errorf("--listen-socket-group %q: unknown group", name) + } + } + both := known(map[string]int{"docker-socket-policy": 2001, "ops": 3001}) + + tests := []struct { + name string + flagValue *string + lookup func(string) (int, error) + wantGID int + wantWarning string + wantErr bool + }{ + {"groupDefaultPresent", nil, both, 2001, "", false}, + {"groupDefaultMissingWarns", nil, known(nil), egid, + "group docker-socket-policy not found, using the proxy's own group 65532", false}, + {"groupExplicitPresent", ptr("ops"), both, 3001, "", false}, + {"groupExplicitMissingFails", ptr("nope"), both, 0, "", true}, + {"groupEmptyUsesOwn", ptr(""), both, egid, "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gid, warning, err := selectSocketGroup(tt.flagValue, tt.lookup, egid) + if tt.wantErr { + if err == nil { + t.Fatalf("selectSocketGroup = %d, %q, nil; want an error", gid, warning) + } + return + } + if err != nil { + t.Fatalf("selectSocketGroup error = %v, want nil", err) + } + if gid != tt.wantGID { + t.Fatalf("gid = %d, want %d", gid, tt.wantGID) + } + if warning != tt.wantWarning { + t.Fatalf("warning = %q, want %q", warning, tt.wantWarning) + } + }) + } +} + +// groupFlagFromArgs parses args with a fresh flag set and reports the +// --listen-socket-group value the way main does, via flagValueIfSet. +func groupFlagFromArgs(args []string) *string { + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.String("listen-socket-group", "", "") + if err := fs.Parse(args); err != nil { + panic(err) + } + return flagValueIfSet(fs, "listen-socket-group") +} + +// An explicit empty value means "the proxy's own group" and must not be +// confused with the flag being absent, which means the default group. +func TestFlagEmptyVsAbsent(t *testing.T) { + tests := []struct { + name string + args []string + want *string + }{ + {"absent", []string{}, nil}, + {"equals empty", []string{"--listen-socket-group="}, ptr("")}, + {"separate empty", []string{"--listen-socket-group", ""}, ptr("")}, + {"named", []string{"--listen-socket-group=ops"}, ptr("ops")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := groupFlagFromArgs(tt.args) + switch { + case tt.want == nil && got != nil: + t.Fatalf("groupFlagFromArgs(%q) = %q, want nil", tt.args, *got) + case tt.want != nil && got == nil: + t.Fatalf("groupFlagFromArgs(%q) = nil, want %q", tt.args, *tt.want) + case tt.want != nil && *got != *tt.want: + t.Fatalf("groupFlagFromArgs(%q) = %q, want %q", tt.args, *got, *tt.want) + } + }) + } +} + +func ptr(s string) *string { return &s } + +// A non-root proxy that is not a member of the selected group cannot chown the +// socket to it. The error must say what to fix rather than surface a bare EPERM. +func TestUnixListenerChownEPERMNamesGroup(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can chown to any group") + } + if groups, err := os.Getgroups(); err == nil { + for _, g := range groups { + if g == 0 { + t.Skip("process is a member of gid 0, so chown to it succeeds") + } + } + } + dir := shortTempDir(t) + // BSD semantics (macOS) give a new file its directory's group, which is + // gid 0 under /tmp, and chown to the current group is always allowed. + // Give the directory our own group so the socket starts out not in gid 0. + if err := os.Chown(dir, -1, os.Getegid()); err != nil { + t.Fatalf("chown temp dir to egid: %v", err) + } + path := filepath.Join(dir, "eperm.sock") + l, err := unixListener(path, 0) + if err == nil { + l.Close() + t.Fatal("unixListener(path, 0) as non-root = nil, want EPERM") + } + if want := "the proxy's user must be a member of it"; !strings.Contains(err.Error(), want) { + t.Fatalf("error = %q, want it to contain %q", err, want) + } +} diff --git a/rs/src/main.rs b/rs/src/main.rs index e361cfc..4db85da 100644 --- a/rs/src/main.rs +++ b/rs/src/main.rs @@ -15,20 +15,23 @@ use hyper::Request; use hyper_util::rt::TokioIo; use std::io; use std::os::unix::fs::{FileTypeExt, PermissionsExt}; -use std::os::unix::io::{FromRawFd, RawFd}; -use std::os::unix::net::UnixListener as StdUnixListener; use std::sync::Arc; use tokio::io::{AsyncRead, AsyncWrite}; use tokio::sync::broadcast; use tokio::task::JoinHandle; use tracing_subscriber::EnvFilter; -/// Raw fd systemd passes for the first socket under socket activation -/// (`sd_listen_fds` convention: fds start at 3). -const SYSTEMD_SOCKET_FD: RawFd = 3; +/// Mode applied to the listening socket. connect(2) on an AF_UNIX socket +/// requires write permission, so 0660 is what actually grants the owning group +/// access. +const SOCKET_MODE: u32 = 0o660; + +/// Group given the socket when `--listen-socket-group` is not passed, as +/// dockerd does with `docker`. +const DEFAULT_SOCKET_GROUP: &str = "docker-socket-policy"; /// Pause after a failed `accept` before retrying, so persistent errors -/// (fd exhaustion, non-listening fd) don't spin the loop at 100% CPU. +/// (e.g. fd exhaustion) don't spin the loop at 100% CPU. const ACCEPT_ERROR_BACKOFF: std::time::Duration = std::time::Duration::from_millis(100); /// Set around bind(2) so the socket is created at 0600 and is never briefly @@ -60,11 +63,7 @@ struct Cli { #[arg(long, default_value_t = false)] readonly: bool, - /// Octal mode for the listening socket (ignored for fd://3). - #[arg(long, default_value = "0660")] - listen_socket_mode: String, - - /// Group name or gid to own the listening socket (ignored for fd://3). + /// Group owning the socket (default docker-socket-policy; "" = the proxy's own group) #[arg(long)] listen_socket_group: Option, } @@ -85,15 +84,15 @@ async fn main() -> Result<(), Box> { tracing::error!("{}", msg); std::process::exit(2); } - let socket_mode = match parse_socket_mode(&cli.listen_socket_mode) { - Ok(m) => m, - Err(msg) => { - tracing::error!("{}", msg); - std::process::exit(2); + // SAFETY: getegid(2) cannot fail. + let egid = unsafe { libc::getegid() }; + let socket_gid = match select_socket_group(cli.listen_socket_group.as_deref(), resolve_group, egid) { + Ok((gid, warning)) => { + if let Some(warning) = warning { + tracing::warn!("{}", warning); + } + gid } - }; - let socket_gid = match cli.listen_socket_group.as_deref().map(resolve_group).transpose() { - Ok(g) => g, Err(msg) => { tracing::error!("{}", msg); std::process::exit(2); @@ -144,8 +143,10 @@ async fn main() -> Result<(), Box> { // 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, socket_mode, socket_gid).map_err(|e| { - tracing::error!("failed to bind unix socket {}: {}", cli.listen_socket, e); + // `_lock`, not `_`: a `_` pattern drops the File at once, releasing the + // single-instance lock while the proxy is still serving. + let (listener, _lock) = open_listener(&cli.listen_socket, Some(socket_gid)).map_err(|e| { + tracing::error!("failed to start listener on {}: {}", cli.listen_socket, e); e })?; let unix_handle = spawn_unix_listener( @@ -157,45 +158,118 @@ async fn main() -> Result<(), Box> { ); let _ = unix_handle.await; + // Release the lock only once the socket is closed and unlinked, so the + // next instance never finds our socket still in place. + drop(_lock); tracing::info!("shutdown complete"); Ok(()) } +/// Takes the single-instance lock, clears the socket path and binds it. The +/// returned lock must be held for the life of the process: dropping it would +/// let a second instance take the path. +fn open_listener(addr: &str, gid: Option) -> io::Result<(tokio::net::UnixListener, std::fs::File)> { + // On an error below, `lock` is dropped on return, releasing the flock. + let lock = acquire_instance_lock(addr)?; + prepare_socket_path(addr)?; + let listener = bind_unix_listener(addr, gid)?; + Ok((listener, lock)) +} + +/// Takes an exclusive flock on `.lock`, which replaces dockerd's +/// pidfile. The kernel drops the lock on any exit, including SIGKILL, so it +/// never goes stale, and it needs no PID check, so it also works across PID +/// namespaces. The file is never truncated or unlinked: unlinking a lock file +/// reopens the race it exists to close. O_NOFOLLOW stops a symlink planted at +/// the lock path from redirecting O_CREAT elsewhere. +fn acquire_instance_lock(socket_path: &str) -> io::Result { + use std::os::unix::fs::OpenOptionsExt; + use std::os::unix::io::AsRawFd; + + let lock_path = format!("{}.lock", socket_path); + let file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC) + .open(&lock_path) + .map_err(|e| io::Error::new(e.kind(), format!("opening lock {}: {}", lock_path, e)))?; + // SAFETY: the fd is owned by `file` and open for the duration of the call. + if unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) } != 0 { + let e = io::Error::last_os_error(); + if e.raw_os_error() == Some(libc::EWOULDBLOCK) { + return Err(io::Error::new( + io::ErrorKind::AddrInUse, + format!("{} is in use by another instance (lock {} held)", socket_path, lock_path), + )); + } + return Err(io::Error::new(e.kind(), format!("locking {}: {}", lock_path, e))); + } + Ok(file) +} + +/// Bounds the connect(2) that tells a live socket from a stale one. +const PROBE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(1); + +/// Clears the socket path for bind, following the existing-path table in +/// spec/listener-design.md. Only a socket that refuses connections is removed. +/// A live one belongs to another process, possibly an instance that takes no +/// lock (TypeScript, or v0.2.21 and earlier), and replacing it would cut that +/// process off silently. Anything that is not a socket is refused, so a +/// mistyped path cannot silently delete an operator's data. +fn prepare_socket_path(path: &str) -> io::Result<()> { + let meta = match std::fs::symlink_metadata(path) { + Ok(meta) => meta, + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(io::Error::new(e.kind(), format!("checking {}: {}", path, e))), + }; + if !meta.file_type().is_socket() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("refusing to remove {}: not a socket ({:?})", path, meta.file_type()), + )); + } + + // std's connect has no timeout, so it runs on its own thread. On timeout + // the thread is left to finish on its own; the socket is live either way. + let (tx, rx) = std::sync::mpsc::channel(); + let target = path.to_string(); + std::thread::spawn(move || { + let _ = tx.send(std::os::unix::net::UnixStream::connect(target).map(drop)); + }); + let in_use = || io::Error::new(io::ErrorKind::AddrInUse, format!("{} is in use by another process", path)); + match rx.recv_timeout(PROBE_TIMEOUT) { + Ok(Ok(())) | Err(std::sync::mpsc::RecvTimeoutError::Timeout) => return Err(in_use()), + Ok(Err(e)) if e.raw_os_error() == Some(libc::ECONNREFUSED) => {} + Ok(Err(e)) => { + return Err(io::Error::new( + e.kind(), + format!("refusing to remove {}: connect: {}", path, e), + )) + } + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + return Err(io::Error::other(format!( + "refusing to remove {}: connect probe did not complete", + path + ))) + } + } + + std::fs::remove_file(path) + .map_err(|e| io::Error::new(e.kind(), format!("removing stale socket {}: {}", path, e))) +} + /// 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 left over from a previous run is removed -/// before binding, matching the Go implementation. -fn bind_unix_listener(addr: &str, mode: u32, gid: Option) -> io::Result { - if addr == "fd://3" { - // Under socket activation systemd owns the socket and applies its own - // SocketMode/SocketGroup; re-chmod'ing here would fight the unit. - return unix_listener_from_raw_fd(SYSTEMD_SOCKET_FD); - } - - // 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), - } - +/// The path must already be clear; `open_listener` runs `prepare_socket_path` +/// first. +fn bind_unix_listener(addr: &str, gid: Option) -> io::Result { // umask is process-global and not thread-safe. This runs during startup, // before any connection is served, so nothing else is creating files. // SAFETY: umask(2) cannot fail and has no preconditions. @@ -205,24 +279,70 @@ fn bind_unix_listener(addr: &str, mode: u32, gid: Option) -> io::Result, + lookup: impl Fn(&str) -> Result, + egid: u32, +) -> Result<(u32, Option), String> { + match flag { + None => match lookup(DEFAULT_SOCKET_GROUP) { + Ok(gid) => Ok((gid, None)), + Err(_) => Ok(( + egid, + Some(format!( + "group {} not found, using the proxy's own group {}", + DEFAULT_SOCKET_GROUP, egid + )), + )), + }, + Some("") => Ok((egid, None)), + Some(group) => lookup(group).map(|gid| (gid, None)), + } +} + +const MAX_SOCKET_GID: u32 = u32::MAX - 1; + /// Maps `--listen-socket-group` to a gid. A numeric value is used as-is so a /// deployment without the group in /etc/group (or NSS) can still be configured. fn resolve_group(group: &str) -> Result { - if let Ok(gid) = group.parse::() { - return Ok(gid); + if !group.is_empty() && group.bytes().all(|b| b.is_ascii_digit()) { + // u32::MAX is chown's "don't change" sentinel. + return match group.parse::() { + Ok(gid) if gid <= MAX_SOCKET_GID => Ok(gid), + _ => Err(format!( + "--listen-socket-group {:?}: gid out of range (0-{})", + group, MAX_SOCKET_GID + )), + }; } let name = std::ffi::CString::new(group) .map_err(|_| format!("--listen-socket-group {:?}: contains a NUL byte", group))?; @@ -256,44 +376,14 @@ fn resolve_group(group: &str) -> Result { Ok(grp.gr_gid) } -/// Parses an octal mode and rejects anything world-writable. connect(2) on an -/// AF_UNIX socket requires write permission, so a world-writable socket is -/// reachable by every local uid — there is deliberately no opt-out. -fn parse_socket_mode(s: &str) -> Result { - if s.is_empty() { - return Err("--listen-socket-mode must not be empty".to_string()); - } - let mode = u32::from_str_radix(s, 8) - .map_err(|_| format!("--listen-socket-mode {:?}: not an octal mode", s))?; - if mode > 0o777 { - return Err(format!("--listen-socket-mode {:?}: must be within 0777", s)); - } - if mode & 0o002 != 0 { - return Err(format!( - "--listen-socket-mode {:?} is world-writable: every local user could connect \ - to the proxy, which disables the access-control boundary", - s - )); - } - Ok(mode) -} - /// 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://"] + } else if ["fd://", "tcp://", "http://", "https://", "unix://"] .iter() .any(|s| addr.starts_with(s)) { @@ -333,39 +423,6 @@ fn validate_docker_host(addr: &str) -> Result<(), String> { Ok(()) } -/// Wraps an existing raw fd as a Tokio `UnixListener`. -/// -/// Split out from [`bind_unix_listener`] so the fd-adoption mechanics -/// (set non-blocking, hand to Tokio) can be exercised in tests against an -/// arbitrary fd, instead of only against the real systemd fd 3. -fn unix_listener_from_raw_fd(fd: RawFd) -> io::Result { - // SAFETY: `from_raw_fd` requires that we take exclusive ownership of the - // fd, which we do — nothing else in this process uses fd 3. The fd itself - // comes from user input (`--listen-socket=fd://3`) and may not actually - // be a listening AF_UNIX socket; that is not a soundness issue, as misuse - // 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, @@ -398,8 +455,7 @@ fn spawn_unix_listener( } Err(e) => { // Back off briefly: persistent accept errors - // (e.g. EMFILE, or a bad fd under socket - // activation) would otherwise busy-loop. + // (e.g. EMFILE) would otherwise busy-loop. tracing::warn!("accept error on unix socket: {}", e); tokio::time::sleep(ACCEPT_ERROR_BACKOFF).await; } @@ -435,15 +491,9 @@ fn spawn_unix_listener( /// /// Unlike Go's `net.UnixListener` and Node, tokio's `UnixListener` does not /// unlink the path when dropped, so without this a clean exit leaves the socket -/// file behind. It is recovered on the next start (`bind_unix_listener` clears +/// file behind. It is recovered on the next start (`prepare_socket_path` clears /// a stale socket), but the three implementations should behave the same. -/// -/// Under socket activation the socket belongs to systemd, which will hand the -/// same fd to the next start — unlinking it there would break the socket unit. fn unlink_listen_socket(addr: &str) { - if addr == format!("fd://{}", SYSTEMD_SOCKET_FD) { - return; - } match std::fs::remove_file(addr) { Ok(()) => {} Err(e) if e.kind() == io::ErrorKind::NotFound => {} @@ -487,65 +537,381 @@ async fn serve_connection( #[cfg(test)] mod tests { use super::*; - use std::os::unix::io::IntoRawFd; + use std::os::unix::net::UnixListener as StdUnixListener; fn unique_socket_path() -> std::path::PathBuf { std::env::temp_dir().join(format!("dsp-test-{}.sock", rand::random::())) } + /// A directory under the temp dir, removed on drop. Paths inside it stay + /// well under macOS's 104-byte sun_path limit. + struct TempDir(std::path::PathBuf); + + impl TempDir { + fn new() -> Self { + let dir = std::env::temp_dir().join(format!("dsp-{}", rand::random::())); + std::fs::create_dir(&dir).unwrap(); + // Set explicitly: bind_unix_listener's umask is process-global, so + // a bind in a parallel test can strip the directory's x bit. + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o700)).unwrap(); + TempDir(dir) + } + + fn join(&self, name: &str) -> String { + self.0.join(name).to_str().unwrap().to_string() + } + } + + impl Drop for TempDir { + fn drop(&mut self) { + std::fs::remove_dir_all(&self.0).ok(); + } + } + + /// Held by `test_lock_released_on_sigkill` from before its fork until the + /// child has closed every inherited fd, and by tests that rely on closing + /// an fd. Without it a child forked at the wrong moment keeps a copy of a + /// "closed" listener or lock alive, so a stale socket probes as live or a + /// dropped lock stays held. + static FORK_GUARD: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn fork_guard() -> std::sync::MutexGuard<'static, ()> { + FORK_GUARD.lock().unwrap_or_else(|e| e.into_inner()) + } + + /// Leaves a socket file at path with nothing listening on it, as an + /// unclean shutdown would: connect(2) to it is refused. Dropping a std + /// listener does not unlink its path. + fn seed_stale_socket(path: &str) { + let _guard = fork_guard(); + drop(StdUnixListener::bind(path).unwrap()); + } + + fn inode(path: &str) -> u64 { + use std::os::unix::fs::MetadataExt; + std::fs::symlink_metadata(path).unwrap().ino() + } + + fn is_root() -> bool { + // SAFETY: geteuid(2) cannot fail. + unsafe { libc::geteuid() == 0 } + } + #[tokio::test] - async fn test_bind_unix_listener_removes_stale_socket() { - let path = unique_socket_path(); - // 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"); + async fn test_open_listener_replaces_stale_socket() { + let dir = TempDir::new(); + let path = dir.join("stale.sock"); + seed_stale_socket(&path); + + let result = open_listener(&path, None); + assert!(result.is_ok(), "open_listener over a stale socket: {:?}", result.err()); + } + + /// A mistyped --listen-socket must not silently destroy data. + #[tokio::test] + async fn test_open_listener_refuses_to_delete_non_socket() { + let dir = TempDir::new(); - let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); + let path = dir.join("important.txt"); + std::fs::write(&path, b"important data").unwrap(); + let err = open_listener(&path, None).expect_err("regular file was not refused"); + assert!(err.to_string().contains("not a socket"), "error = {:?}", err.to_string()); + assert_eq!(std::fs::read(&path).unwrap(), b"important data", "file was modified"); + + let path = dir.join("adir"); + std::fs::create_dir(&path).unwrap(); + assert!(open_listener(&path, None).is_err(), "directory was not refused"); + assert!(std::path::Path::new(&path).is_dir(), "directory was removed"); + } + + /// One case per row of the existing-path table in + /// spec/listener-design.md; the case names match the Quint runs. + #[test] + fn test_prepare_socket_path() { + // path_absent_binds + { + let dir = TempDir::new(); + let path = dir.join("absent.sock"); + assert!(prepare_socket_path(&path).is_ok()); + } + + // path_stale_replaced + { + let dir = TempDir::new(); + let path = dir.join("stale.sock"); + seed_stale_socket(&path); + let result = prepare_socket_path(&path); + assert!(result.is_ok(), "prepare_socket_path(stale) = {:?}", result.err()); + assert!(std::fs::symlink_metadata(&path).is_err(), "stale socket still present"); + } + + // path_live_refused + { + let dir = TempDir::new(); + let path = dir.join("live.sock"); + let live = StdUnixListener::bind(&path).unwrap(); + let before = inode(&path); + + let err = prepare_socket_path(&path).expect_err("live socket was not refused"); + assert_eq!(err.to_string(), format!("{} is in use by another process", path)); + assert_eq!(inode(&path), before, "live socket was replaced"); + + // Scoped so `live` outlives the dial: the accept may take the + // probe's queued connection, and a thread owning the listener + // would then close it before the dial below. + std::thread::scope(|s| { + let accepted = s.spawn(|| live.accept().map(drop)); + std::os::unix::net::UnixStream::connect(&path).expect("live listener no longer reachable"); + accepted.join().unwrap().expect("live listener no longer accepts"); + }); + } + + // path_connect_error_refused: connect(2) needs write permission on the + // socket, so a 0000 socket yields EACCES. It is neither live nor + // provably stale, so it is left alone. + if is_root() { + eprintln!("skipping path_connect_error_refused: root ignores socket permissions"); + } else { + let dir = TempDir::new(); + let path = dir.join("eacces.sock"); + seed_stale_socket(&path); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); + let err = prepare_socket_path(&path).expect_err("0000 socket was not refused"); + let want = format!("refusing to remove {}", path); + assert!(err.to_string().starts_with(&want), "error = {:?}, want prefix {:?}", err.to_string(), want); + assert!(std::fs::symlink_metadata(&path).is_ok(), "socket was removed"); + } + + // path_not_socket_refused + { + let dir = TempDir::new(); + let path = dir.join("file.txt"); + std::fs::write(&path, b"data").unwrap(); + let err = prepare_socket_path(&path).expect_err("regular file was not refused"); + let want = format!("refusing to remove {}: not a socket", path); + assert!(err.to_string().starts_with(&want), "error = {:?}, want prefix {:?}", err.to_string(), want); + assert_eq!(std::fs::read(&path).unwrap(), b"data", "regular file was modified"); + } + } + + /// Covers the Quint run second_instance_lock_refused. + #[test] + fn test_acquire_instance_lock() { + let _guard = fork_guard(); + let dir = TempDir::new(); + let path = dir.join("lock.sock"); + let lock_path = format!("{}.lock", path); + + let first = acquire_instance_lock(&path).expect("first acquire_instance_lock"); + + let err = acquire_instance_lock(&path).expect_err("second acquisition succeeded"); + assert_eq!( + err.to_string(), + format!("{} is in use by another instance (lock {} held)", path, lock_path) + ); + + let mode = std::fs::symlink_metadata(&lock_path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "lock file mode = {:o}, want 0600", mode); + + drop(first); assert!( - result.is_ok(), - "expected stale socket to be removed and bind to succeed: {:?}", - result.err() + std::fs::symlink_metadata(&lock_path).is_ok(), + "lock file removed on drop, want it kept" ); - std::fs::remove_file(&path).ok(); + acquire_instance_lock(&path).expect("acquire_instance_lock after drop"); } - #[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(); + /// A symlink planted at .lock must not be followed: O_CREAT through + /// it would create or lock a file of the attacker's choosing. + #[test] + fn test_acquire_instance_lock_refuses_symlink() { + let dir = TempDir::new(); + let path = dir.join("sym.sock"); + let target = dir.join("target"); + std::os::unix::fs::symlink(&target, format!("{}.lock", path)).unwrap(); - let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); - assert!(result.is_err(), "expected a regular file to be refused, not deleted"); + let err = acquire_instance_lock(&path).expect_err("lock through a symlink succeeded"); assert!( - result.unwrap_err().to_string().contains("not a socket"), - "error should explain that the path is not a socket" + err.to_string().contains(&format!("{}.lock", path)), + "error = {:?}, want it to name {}.lock", + err.to_string(), + path ); - assert_eq!( - std::fs::read(&path).unwrap(), - b"important data", - "the file must be left untouched" + assert!(std::fs::symlink_metadata(&target).is_err(), "symlink target was created"); + } + + #[test] + fn test_acquire_instance_lock_unreadable() { + if is_root() { + eprintln!("skipping: root ignores file permissions"); + return; + } + let dir = TempDir::new(); + let path = dir.join("unreadable.sock"); + let lock_path = format!("{}.lock", path); + std::fs::write(&lock_path, b"").unwrap(); + std::fs::set_permissions(&lock_path, std::fs::Permissions::from_mode(0o000)).unwrap(); + + let err = acquire_instance_lock(&path).expect_err("lock on a 0000 file succeeded"); + assert!( + err.to_string().contains(&lock_path), + "error = {:?}, want it to name {}", + err.to_string(), + lock_path ); + } - std::fs::remove_file(&path).ok(); + #[test] + fn test_open_listener_concurrent() { + const RACERS: usize = 8; + let rt = tokio::runtime::Runtime::new().unwrap(); + let dir = TempDir::new(); + + for i in 0..50 { + let path = dir.join(&format!("c{}.sock", i)); + let barrier = Arc::new(std::sync::Barrier::new(RACERS)); + let racers: Vec<_> = (0..RACERS) + .map(|_| { + let (path, barrier, handle) = (path.clone(), barrier.clone(), rt.handle().clone()); + std::thread::spawn(move || { + // tokio's UnixListener registers with the runtime's reactor. + let _enter = handle.enter(); + barrier.wait(); + open_listener(&path, None) + }) + }) + .collect(); + let results: Vec<_> = racers.into_iter().map(|r| r.join().unwrap()).collect(); + + let want = format!("{} is in use by another instance (lock {}.lock held)", path, path); + let mut winner = None; + for result in results { + match result { + Ok(pair) => { + assert!(winner.is_none(), "iteration {}: more than one open_listener succeeded", i); + winner = Some(pair); + } + Err(e) => assert_eq!(e.to_string(), want, "iteration {}: loser error", i), + } + } + let (listener, lock) = winner.unwrap_or_else(|| panic!("iteration {}: no open_listener succeeded", i)); + + let _client = std::os::unix::net::UnixStream::connect(&path) + .unwrap_or_else(|e| panic!("iteration {}: dialing winner: {}", i, e)); + rt.block_on(async { + tokio::time::timeout(std::time::Duration::from_secs(5), listener.accept()) + .await + .unwrap_or_else(|_| panic!("iteration {}: winner did not accept within 5s", i)) + .unwrap_or_else(|e| panic!("iteration {}: winner accept: {}", i, e)); + }); + + drop(listener); + drop(lock); + } + } + + /// Kills and reaps the forked child even if an assertion fails first. + struct ChildGuard(libc::pid_t); + + impl Drop for ChildGuard { + fn drop(&mut self) { + if self.0 > 0 { + // SAFETY: self.0 is our own unreaped child. + unsafe { + libc::kill(self.0, libc::SIGKILL); + libc::waitpid(self.0, std::ptr::null_mut(), 0); + } + } + } + } + + /// Covers the Quint run crash_releases_lock: the kernel drops the flock on + /// any exit, so a killed instance never leaves a stale lock behind. + #[test] + fn test_lock_released_on_sigkill() { + let dir = TempDir::new(); + let path = dir.join("kill.sock"); + let lock_path = std::ffi::CString::new(format!("{}.lock", path)).unwrap(); + + let mut fds = [0 as libc::c_int; 2]; + // SAFETY: fds has room for the two descriptors pipe(2) writes. + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe: {}", io::Error::last_os_error()); + let (read_fd, write_fd) = (fds[0], fds[1]); + // SAFETY: sysconf(3) has no preconditions. + let max_fd = unsafe { libc::sysconf(libc::_SC_OPEN_MAX) }.clamp(256, 65536) as libc::c_int; + + let fork_lock = fork_guard(); + // SAFETY: the child calls only async-signal-safe functions (close, + // open, flock, write, pause, _exit) on memory prepared before fork, + // and never returns into the test harness. + let pid = unsafe { libc::fork() }; + if pid == 0 { + unsafe { + // Drop every inherited fd but the pipe, so the child cannot + // hold another test's lock or listener open. + for fd in 3..max_fd { + if fd != write_fd { + libc::close(fd); + } + } + let fd = libc::open( + lock_path.as_ptr(), + libc::O_RDWR | libc::O_CREAT | libc::O_NOFOLLOW | libc::O_CLOEXEC, + 0o600 as libc::c_uint, + ); + let ok = fd >= 0 && libc::flock(fd, libc::LOCK_EX | libc::LOCK_NB) == 0; + let byte: u8 = if ok { b'r' } else { b'e' }; + libc::write(write_fd, &byte as *const u8 as *const libc::c_void, 1); + if !ok { + libc::_exit(1); + } + loop { + libc::pause(); + } + } + } + assert!(pid > 0, "fork: {}", io::Error::last_os_error()); + let mut child = ChildGuard(pid); + // SAFETY: write_fd is ours and open. + unsafe { libc::close(write_fd) }; + + let mut pfd = libc::pollfd { fd: read_fd, events: libc::POLLIN, revents: 0 }; + // SAFETY: pfd is a valid pollfd for the duration of the call. + let ready = unsafe { libc::poll(&mut pfd, 1, 10_000) }; + let mut byte = 0u8; + // SAFETY: byte is a valid 1-byte buffer. + let n = if ready == 1 { unsafe { libc::read(read_fd, &mut byte as *mut u8 as *mut libc::c_void, 1) } } else { 0 }; + unsafe { libc::close(read_fd) }; + drop(fork_lock); + assert_eq!(ready, 1, "child did not report ready within 10s"); + assert_eq!((n, byte), (1, b'r'), "child failed to take the lock"); + + assert!(acquire_instance_lock(&path).is_err(), "acquired the lock while the child held it"); + + // SAFETY: pid is our unreaped child. + unsafe { + assert_eq!(libc::kill(pid, libc::SIGKILL), 0, "kill: {}", io::Error::last_os_error()); + assert_eq!(libc::waitpid(pid, std::ptr::null_mut(), 0), pid, "waitpid"); + } + child.0 = 0; + + acquire_instance_lock(&path).expect("acquire_instance_lock after SIGKILL"); } #[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); - } + assert!(validate_listen_socket("/var/run/docker-socket-policy.sock").is_ok()); // 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"), + // Socket activation was removed; fd:// is just another scheme now. + ("fd://3", "only supports Unix socket paths"), + ("fd://4", "only supports Unix socket paths"), + ("fd://0", "only supports Unix socket paths"), + ("fd://abc", "only supports Unix socket paths"), ("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"), @@ -565,6 +931,16 @@ mod tests { } } + /// The socket mode is fixed. A deployment still passing the flag must fail + /// at startup rather than silently get a different mode than it asked for. + #[test] + fn test_listen_socket_mode_flag_removed() { + let err = Cli::try_parse_from(["x", "--listen-socket-mode=0660"]) + .err() + .expect("--listen-socket-mode should be rejected"); + assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument); + } + #[test] fn test_validate_docker_host() { assert!(validate_docker_host("/var/run/docker.sock").is_ok()); @@ -587,42 +963,13 @@ mod tests { async fn test_bind_unix_listener_binds_fresh_path() { let path = unique_socket_path(); - let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); + let result = bind_unix_listener(path.to_str().unwrap(), None); assert!(result.is_ok(), "expected bind to a fresh path to succeed: {:?}", result.err()); assert!(path.exists(), "expected socket file to be created"); std::fs::remove_file(&path).ok(); } - #[tokio::test] - async fn test_unix_listener_from_raw_fd_wraps_existing_socket() { - let path = unique_socket_path(); - let std_listener = StdUnixListener::bind(&path).unwrap(); - let fd = std_listener.into_raw_fd(); - - let result = unix_listener_from_raw_fd(fd); - assert!(result.is_ok(), "expected wrapping an existing listening fd to succeed: {:?}", result.err()); - - 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 - ); - } - /// Transport that stalls before replying, so a request can be held /// in-flight while a shutdown signal is delivered. struct SlowTransport { @@ -676,7 +1023,7 @@ mod tests { #[tokio::test] async fn test_listener_shuts_down_promptly_when_idle() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -702,7 +1049,7 @@ mod tests { #[tokio::test] async fn test_listener_drains_in_flight_request() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -738,43 +1085,20 @@ mod tests { std::fs::remove_file(&path).ok(); } - #[test] - fn test_parse_socket_mode() { - for (input, want) in [("0660", 0o660), ("660", 0o660), ("0600", 0o600), ("0640", 0o640)] { - assert_eq!(parse_socket_mode(input), Ok(want), "{} should parse", input); - } - for (input, want) in [ - ("", "must not be empty"), - ("0x1ff", "not an octal mode"), - ("668", "not an octal mode"), - ("1777", "within 0777"), - // connect(2) needs write, so o+w means every local uid can connect. - ("0666", "world-writable"), - ("0777", "world-writable"), - ("0602", "world-writable"), - ] { - let err = parse_socket_mode(input) - .expect_err(&format!("{:?} should be rejected", input)); - assert!(err.contains(want), "error for {:?} was {:?}", input, err); - } - } - /// Regression test for #40: the mode used to be whatever the umask left /// behind, which is 0755 by default. connect(2) requires write permission, /// so the documented group grant silently did not work, and under umask 0 /// the socket was 0777 to every local uid. #[tokio::test] async fn test_bind_applies_socket_mode() { - for mode in [0o660_u32, 0o600, 0o640] { - let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), mode, None).unwrap(); + let path = unique_socket_path(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); - let got = std::fs::symlink_metadata(&path).unwrap().permissions().mode() & 0o777; - assert_eq!(got, mode, "socket mode was {:o}, want {:o}", got, mode); + let got = std::fs::symlink_metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(got, 0o660, "socket mode was {:o}, want 0660", got); - drop(listener); - std::fs::remove_file(&path).ok(); - } + drop(listener); + std::fs::remove_file(&path).ok(); } /// The ambient umask must not influence the result: that was the bug. @@ -783,7 +1107,7 @@ mod tests { // SAFETY: umask(2) cannot fail. Restored below. let previous = unsafe { libc::umask(0) }; let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); // SAFETY: as above. unsafe { libc::umask(previous) }; @@ -807,12 +1131,111 @@ mod tests { } } + #[test] + fn test_resolve_group_rejects_out_of_range_gid() { + // 4294967295 is chown's "don't change" sentinel; larger values do not + // fit a gid_t at all. + assert_eq!(resolve_group("4294967294"), Ok(4294967294)); + for v in ["4294967295", "4294967296", "12345678901234567890"] { + assert_eq!( + resolve_group(v), + Err(format!("--listen-socket-group {:?}: gid out of range (0-4294967294)", v)) + ); + } + // Only a digit string is numeric; a sign makes it a (nonexistent) name. + for v in ["+4294967296", "+5"] { + assert!(resolve_group(v).is_err(), "resolve_group({:?}) should fail", v); + } + } + + /// Mirrors the Quint group_* actions and Go's TestSelectSocketGroup. + #[test] + fn test_select_socket_group() { + const EGID: u32 = 65532; + let known = |name: &str| -> Result { + match name { + "docker-socket-policy" => Ok(2001), + "ops" => Ok(3001), + _ => Err(format!("--listen-socket-group {:?}: unknown group", name)), + } + }; + let none = |name: &str| -> Result { + Err(format!("--listen-socket-group {:?}: unknown group", name)) + }; + + // group_default_present + assert_eq!(select_socket_group(None, known, EGID), Ok((2001, None))); + // group_default_missing_warns + assert_eq!( + select_socket_group(None, none, EGID), + Ok(( + EGID, + Some("group docker-socket-policy not found, using the proxy's own group 65532".to_string()) + )) + ); + // group_explicit_present + assert_eq!(select_socket_group(Some("ops"), known, EGID), Ok((3001, None))); + // group_explicit_missing_fails + assert!(select_socket_group(Some("nope"), known, EGID).is_err()); + // group_empty_uses_own + assert_eq!(select_socket_group(Some(""), known, EGID), Ok((EGID, None))); + } + + /// An absent flag selects the default group; an explicit empty value + /// selects the proxy's own. The two must stay distinguishable. + #[test] + fn test_group_flag_empty_vs_absent() { + let parse = |args: &[&str]| { + let mut argv = vec!["x"]; + argv.extend_from_slice(args); + Cli::try_parse_from(argv).unwrap().listen_socket_group + }; + assert_eq!(parse(&[]), None); + assert_eq!(parse(&["--listen-socket-group="]), Some(String::new())); + assert_eq!(parse(&["--listen-socket-group", ""]), Some(String::new())); + } + + /// A non-root proxy that is not a member of the selected group cannot + /// chown the socket to it. The error must say what to fix. + #[tokio::test] + async fn test_bind_chown_eperm_names_group() { + // SAFETY: geteuid/getegid cannot fail. + if unsafe { libc::geteuid() } == 0 { + eprintln!("skipping: root can chown to any group"); + return; + } + // SAFETY: a zero-length query returns the group count; the second + // call fills a buffer of exactly that size. + let n = unsafe { libc::getgroups(0, std::ptr::null_mut()) }; + if n > 0 { + let mut groups = vec![0 as libc::gid_t; n as usize]; + let n = unsafe { libc::getgroups(n, groups.as_mut_ptr()) }; + if n > 0 && groups[..n as usize].contains(&0) { + eprintln!("skipping: process is a member of gid 0, so chown to it succeeds"); + return; + } + } + let dir = TempDir::new(); + // BSD semantics (macOS) give a new file its directory's group, which + // is gid 0 under /tmp, and chown to the current group is always + // allowed. Give the directory our own group so the socket does not + // start out in gid 0. + let egid = unsafe { libc::getegid() }; + std::os::unix::fs::chown(&dir.0, None, Some(egid)).unwrap(); + let path = dir.join("eperm.sock"); + + let result = bind_unix_listener(&path, Some(0)); + let err = result.expect_err("bind_unix_listener(path, Some(0)) as non-root succeeded, want EPERM"); + let want = "the proxy's user must be a member of it"; + assert!(err.to_string().contains(want), "error = {:?}, want it to contain {:?}", err.to_string(), want); + } + /// A clean shutdown must not leave the socket file on disk, matching Go /// and TypeScript. tokio does not unlink on drop, so this is explicit. #[tokio::test] async fn test_listener_unlinks_socket_on_shutdown() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -837,26 +1260,12 @@ mod tests { std::fs::remove_file(&path).ok(); } - /// Socket activation is the exception: the socket belongs to systemd and - /// must survive the process, or the unit cannot hand it to the next start. - #[test] - fn test_unlink_listen_socket_spares_socket_activation() { - let path = unique_socket_path(); - std::fs::write(&path, b"stand-in for a systemd-owned socket").unwrap(); - - unlink_listen_socket(&format!("fd://{}", SYSTEMD_SOCKET_FD)); - assert!(path.exists(), "precondition check only"); - - unlink_listen_socket(path.to_str().unwrap()); - assert!(!path.exists(), "a path-based socket should be removed"); - } - /// An idle keep-alive connection must not hold shutdown open until the /// timeout: the connection is told to close, not merely left alone. #[tokio::test] async fn test_listener_releases_idle_keepalive_connection() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( diff --git a/spec/README.md b/spec/README.md index 13e93ab..94163f9 100644 --- a/spec/README.md +++ b/spec/README.md @@ -6,7 +6,9 @@ This directory contains a [Quint](https://quint-lang.org/) formal specification | File | Purpose | |------|---------| -| `docker_socket_policy.qnt` | Single-file spec: policy types, state machine, endpoint routing table, 9 invariants (6 P0 / 3 P1), 6 attack scenario simulations | +| `docker_socket_policy.qnt` | Request-handling spec: policy types, state machine, endpoint routing table, 9 invariants (6 P0 / 3 P1), 6 attack scenario simulations | +| `listener.qnt` | Listening-socket startup: flag/group selection, existing-path checks, single-instance lock, 6 invariants, one `run` test per design-table row. Instances `listener_locked` (Go, Rust) and `listener_unlocked` (TypeScript) | +| `listener-design.md` | Design of the listening socket (dockerd parity) that `listener.qnt` models | ## How to Run @@ -25,6 +27,12 @@ quint run --max-steps=50 --invariants allInvariants --backend rust \ quint run --max-steps=50 --invariants allInvariants --backend typescript \ spec/docker_socket_policy.qnt +# Listener model: typecheck, simulate the locked model, run the table tests +quint typecheck spec/listener.qnt +quint run spec/listener.qnt --main=listener_locked --max-steps=30 --invariant allListenerInvariants +quint test spec/listener.qnt --main=listener_locked +quint test spec/listener.qnt --main=listener_unlocked + # Formal model-checking via Apalache (exhaustive, requires Java) quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy.qnt ``` @@ -50,6 +58,23 @@ quint verify --max-steps=10 --invariants allInvariants spec/docker_socket_policy | `flagsInAllowlist` | All CLI flags pass allowlist + denylist | CmdGate → `flagAllowed()` | | `routingTableComplete` | Every endpoint in the routing table has an explicit action | Explicit `endpointsTable.contains()` check | +### Listener Invariants (`listener.qnt`, checked on `listener_locked`) + +| Invariant | What It Checks | Protection in the model | +|-----------|---------------|-------------------------| +| `neverListensOnTcp` | No instance serves unless `--listen-socket` was a Unix path | `start` rejects `fd://3`, `tcp://…`, `http://…` (exit 2) | +| `groupBeforeMode` | The socket never has group bits while its group differs from the selected group | `bind` at `0600`, then `chown`, then `chmod 0660` | +| `neverWorldWritable` | The socket mode never has `o+w` | `chmod` only ever sets `0660` | +| `neverUnlinksNonSocket` | A regular file at the path is never removed | `probe` refuses a non-socket (exit 1) | +| `noLiveTakeover` | A serving instance's socket is still the one at the path | `lock` (`flock` on `.lock`) plus the `connect(2)` probe | +| `groupSelectionMatchesTable` | The selected group and warning match the design's group-selection table | `start` resolves the group moby-style; the table is written out as data | + +`allListenerInvariants` is their conjunction. `noLiveTakeover` is shown to fail on `listener_unlocked`: + +```bash +quint run spec/listener.qnt --main=listener_unlocked --max-steps=30 --invariant noLiveTakeover # violation expected +``` + ### Modeling Notes Two invariants are structurally tautological within the Quint model — they can't be falsified by any action sequence the simulator generates, so they don't get real coverage from `quint run`/`quint verify`: @@ -57,6 +82,9 @@ Two invariants are structurally tautological within the Quint model — they can - **`proxyLives`** — `proxyRunning` is set once in `init` and every action preserves it (`proxyRunning' = proxyRunning`); nothing in the model ever sets it `false`. The real guarantee ("a panic/error on one request doesn't crash the whole proxy") is enforced by language-specific mechanisms outside the model: Go's stdlib `net/http.Server` recovers per-request panics, Rust's `tokio::spawn` isolates panics per connection task, and TypeScript's request handler wraps `handle()` in a `.catch()`. These are exercised by each implementation's own test suite, not by the Quint simulation. - **`routingTableComplete`** — checks that `endpointsTable` (a fixed constant) contains a fixed list of literals declared in the same file. It documents the intended routing table but doesn't cross-check it against any of the three Router implementations; that comparison has to be done manually (or via `quint-analyzer`) against `go/internal/proxy/router.go`, `rs/src/proxy.rs`, and `ts/src/proxy.ts`. +- **`listener.qnt` checks the design, not the code.** Nothing in the model is derived from the Go, Rust or TypeScript sources. Conformance rests on each implementation's unit and integration tests, which carry the same names as the Quint `run`s (`groupDefaultPresent`, `pathStaleReplaced`, …) so every design-table row can be traced across all four. `raceWithoutLockTest` (in `listener_unlocked`) is the formal record of the TypeScript gap: Node has no `flock`, so two TypeScript instances starting together can orphan one another's socket ([#46](https://github.com/ChainSafe/docker-socket-policy/issues/46)). +- **Listener fault bias.** `step` crashes an instance on 1 in 10 draws instead of half of all steps, so random runs actually interleave live instances. Every crash stays reachable from every phase, so the reachable state space is unchanged. + ### Attack Scenarios Prevented by Invariants | Scenario | Attacker Action | Prevented By | @@ -139,4 +167,6 @@ Quint formal verification runs in CI via `.github/workflows/ci.yml` (quint job), - run: quint run --max-steps=100 --invariants allInvariants --backend typescript spec/docker_socket_policy.qnt ``` +In practice the job calls `make typecheck`, `make test-spec` and `make verify BACKEND=typescript`, which also cover `listener.qnt`. + Releases are handled by `.github/workflows/release.yml`, which auto-bumps the patch version on push to `main`, creates a draft release, builds Docker images, generates SPDX + CycloneDX SBOMs with syft, and signs them with Cosign. diff --git a/spec/listener-design.md b/spec/listener-design.md new file mode 100644 index 0000000..06fad74 --- /dev/null +++ b/spec/listener-design.md @@ -0,0 +1,285 @@ +# Listening socket: dockerd parity, Unix path only + +Date: 2026-09-29 +Status: approved 2026-09-29 +Supersedes: #29, #44 (both resolved by removing `fd://`) + +## Goal + +Operating the proxy's listening socket should work like operating +`docker.sock`: the socket is always `0660`, owned by a well-known group, and +access is granted or revoked with group membership alone +(`usermod -aG docker-socket-policy alice`). No socket configuration beyond +that. + +No code path may listen on anything but a Unix socket file the proxy created +itself. + +## Why + +- `--listen-tcp` was removed in #35, but `--listen-socket=fd://3` still + adopts whatever systemd passes, and `ListenStream=127.0.0.1:2375` hands over + a TCP socket. Keeping TCP out depends on a per-language guard, and #44 showed + one of those guards rejecting every socket, including valid ones. Removing + `fd://` removes the whole class instead of hardening it (#29). +- `--listen-socket-mode` exists only to let operators choose a mode, and every + mode but `0660` is either useless (connect needs write) or dangerous + (world-writable). dockerd has no such flag. +- Our own compose file already shows `--listen-socket-group` is redundant when + the proxy runs as `user: uid:gid`: `bind(2)` gives the socket the process's + group anyway. + +## Behaviour + +### Flags + +| Flag | Change | +|---|---| +| `--listen-socket ` | Unchanged, except `fd://…` is now rejected like any other non-path value ("only supports Unix socket paths"). | +| `--listen-socket-mode` | **Removed.** Passing it is an unknown-flag error (exit 2). | +| `--listen-socket-group` | Kept (the name has shipped in v0.2.21). Semantics below. | + +### Group selection (mirrors `moby/daemon/listeners/listeners_linux.go`) + +Default group name: `docker-socket-policy`. + +| `--listen-socket-group` | group resolves | outcome | +|---|---|---| +| not passed | yes | socket group = `docker-socket-policy` | +| not passed | no | warn `group docker-socket-policy not found, using the proxy's own group `; socket group = process egid; start | +| `=name` | yes | socket group = that group | +| `=name` | no | error, exit 2 | +| `=gid` | — | socket group = that gid, used as-is (no lookup). Digits only, `0-4294967294`; a larger value fails with exit 2 | +| `=""` | — | socket group = process egid, no warning | + +Resolution is unchanged from #40: Go `os/user.LookupGroup` / numeric, +Rust `getgrnam_r` / numeric, TypeScript parses `/etc/group` / numeric. +Only digits-only strings count as numeric; anything else (`+5`, `-5`) goes +to name lookup, except that Go rejects `-` followed by digits with a +"negative gid" error. + +The warning is emitted on every container start where the group does not +exist, exactly as dockerd does. Accepted as the cost of parity. + +If `chown` to the selected group fails with `EPERM` (a non-root proxy that +is not a member of that group), startup fails, exit 1, with a message naming +the group and saying the proxy's user must be a member of it +(`SupplementaryGroups=` / `group_add:`). It does not fall back: a group that +exists was chosen deliberately. + +### Mode + +Always `0660`. Creation order stays as #40 established it: `umask(0177)` +around `bind(2)` (socket born `0600`), then `chown`, then `chmod 0660`, so the +socket is never reachable by the wrong group and never at the ambient mode. +The world-writable rejection goes away with the flag, since nothing can +request a mode. + +### Single-instance lock (Go, Rust) + +Before touching the socket path, Go and Rust open `.lock` +(`O_CREAT|O_RDWR|O_CLOEXEC`, mode `0600`) and take +`flock(LOCK_EX|LOCK_NB)`. The fd is held for the life of the process and +the kernel releases it on any exit, including `SIGKILL`, so a crash never +leaves a stale lock behind. The lock file is **never unlinked**: unlinking +a lock file reopens the same race it exists to close. + +- `EWOULDBLOCK` → error ` is in use by another instance (lock .lock held)`, exit 1. +- Any other open or lock error → error naming it, exit 1. + +This replaces dockerd's pidfile. Unlike a pidfile, it needs no PID check, +so it also works across PID namespaces (two containers sharing a socket +volume). + +### Existing path at startup + +These checks run after the lock is taken (Go, Rust) and without a lock +(TypeScript): + +| What is at the path | outcome | +|---|---| +| nothing | bind | +| a socket, `connect(2)` → `ECONNREFUSED` | stale: unlink, bind (current behaviour) | +| a socket, `connect(2)` succeeds | **new:** error ` is in use by another process`, exit 1; the live socket is left untouched | +| a socket, any other `connect(2)` error (e.g. `EACCES`) | **new:** error naming the errno, exit 1; not unlinked | +| not a socket | error, not unlinked (current behaviour) | + +The connect check stays in Go and Rust even though they hold the lock. +It still catches an instance that doesn't take the lock: v0.2.21 and +earlier, a TypeScript instance, or an unrelated process. + +Verified on main (2026-09-29): a stale `0777` socket is recreated at `0660` +in all three, but a second instance on a live path unlinks the first +instance's socket and takes it over silently. + +### TypeScript exception (documented gap) + +Node has no `flock`: checked on Node 22, `fs.flock` is undefined and there +is no `fcntl` locking and no `O_EXLOCK` on Linux. TypeScript therefore relies +on the connect check alone. That leaves a check-then-unlink race: + +1. A connects: refused, so the socket is stale. +2. B connects: refused, so the socket is stale. +3. A unlinks and binds. A is now live. +4. B unlinks A's live socket and binds. A is left running with nothing able to reach it. + +A second instance that starts once the first is already listening is still +refused. The race needs two TypeScript instances starting within the same +few milliseconds on the same path. A Go or Rust instance starting alongside +a TypeScript one is covered only on the Go/Rust side: the TypeScript side +takes no lock. + +This is a deliberate departure from the equal-peers rule, recorded in: + +- `README.md`: a note beside the socket section saying TypeScript doesn't + take the single-instance lock, and describing the race. +- A code comment at the TypeScript bind site that points at the follow-up + issue. +- A follow-up issue, `Type: Enhancement`, TypeScript only: "TypeScript: + single-instance lock for the listening socket". It describes what closing + the gap needs: + - **Preferred:** an in-repo N-API addon (about 30 lines of C) that exposes + `flock(fd, LOCK_EX|LOCK_NB)`, with no npm dependency. It requires + `ts/Dockerfile` to compile the addon: drop `--ignore-scripts` for that + one package, or build it in an explicit step, on `stagex/pallet-nodejs` + or a StageX C-toolchain stage. `make verify-reproducible-ts` must stay + bit-for-bit reproducible. + - **Rejected alternatives** and why: an `fs-ext`-style npm addon (an + external dependency built by install scripts); calling `flock(1)` from a + shell (util-linux isn't in the image); an abstract-namespace lock socket + (Linux only, and scoped to a network namespace, so two containers + sharing a socket volume would not see each other's lock); a lock-free + `link`/`rename` protocol (it can still orphan a socket with three + instances, and a live socket briefly vanishes while it's being put back). + - **Done when:** the Quint `lockHeld = true` configuration covers + TypeScript too, and TypeScript passes the same unit and integration + concurrency tests as Go and Rust. + +### Removed + +- `fd://3` socket activation: `listenerFromFile` (Go), + `unix_listener_from_raw_fd` and `SYSTEMD_SOCKET_FD` (Rust), the `fd` listen + target (TS), the `fd://3` exemption in `unlink_listen_socket` (Rust), and + their tests. +- `--listen-socket-mode`: `parseSocketMode` / `parse_socket_mode`, + `DEFAULT_LISTEN_SOCKET_MODE`, world-writable check, and their tests. + +## Files + +- `go/main.go`, `go/main_test.go` +- `rs/src/main.rs` +- `ts/src/flags.ts`, `ts/src/flags.test.ts`, `ts/src/listen.ts`, + `ts/src/listen.test.ts`, `ts/src/index.ts` +- `deploy/docker-compose.sock.yml`, `deploy/test-sock.sh` +- `README.md`: flag table, security note, replace "Systemd Socket + Activation" with a plain `.service` using `Group=` / + `SupplementaryGroups=` and the `groupadd` / `usermod -aG` workflow +- `spec/listener.qnt` (new), `spec/README.md`, `Makefile`, + `.github/workflows/ci.yml` + +## Formal specification + +`spec/docker_socket_policy.qnt` models request handling only. The listener +gets its own module, `spec/listener.qnt`, so the existing model and its +simulation are unchanged. + +### Model + +- **Configuration:** a nondeterministic `--listen-socket` value drawn from a + Unix path, `fd://3`, `tcp://0.0.0.0:2375` and `http://…`, plus + `--listen-socket-group` (not passed, existing group, missing group, `""`). +- **Filesystem at the path:** absent, stale socket, live socket owned by + another instance, regular file. +- **Instances:** `I1`, `I2`, `I3`. Each moves through + `lock → probe → unlink → bind(0600) → chown → chmod(0660) → serving`, + **one step per transition**, so the steps of different instances can + interleave. An instance can crash at any step: the kernel releases its + lock, and its socket file stays behind as stale. +- **Constant `lockHeld: bool`:** `true` models Go and Rust; `false` models + TypeScript, where the lock step does nothing. + +### Invariants (checked with `lockHeld = true`) + +| Invariant | Statement | +|---|---| +| `neverListensOnTcp` | no instance reaches `serving` unless the configuration was a Unix path | +| `groupBeforeMode` | the socket is never group-accessible while its group differs from the selected group | +| `neverWorldWritable` | the socket's mode never has the `o+w` bit in any state | +| `neverUnlinksNonSocket` | a regular file at the path is never removed | +| `noLiveTakeover` | a serving instance's socket is never unlinked by another instance | +| `groupSelectionMatchesTable` | the selected group matches the group-selection table for every configuration | + +`allListenerInvariants` is their conjunction. + +### Tests (`quint test`) + +- One `run` per row of the group-selection and existing-path tables. Each + has a matching unit test in Go, Rust and TypeScript with the same name, so + every row can be traced across all four. +- `raceWithoutLockTest` sets `lockHeld = false` and replays the four-step + interleaving above, asserting that it ends with I1 serving on a socket + that is no longer at the path. This is the formal record of the + TypeScript gap. The follow-up issue turns it into a `lockHeld = true` + requirement for TypeScript. + +### Wiring + +- `make typecheck` type-checks both modules. +- `make verify` also runs + `quint run --invariants allListenerInvariants spec/listener.qnt`, with + `lockHeld = true`. +- New target `make test-spec` runs `quint test spec/listener.qnt`. +- The CI quint job adds `make test-spec`. CI runs no `quint test` today. +- `spec/README.md`: update the invariant count, add a listener coverage + table, and add a Modeling Note saying the model checks the design, not + the code. Conformance of the three implementations rests on their unit + and integration tests, which share names with the Quint `run`s. + +Before relying on `noLiveTakeover`, it must be shown to **fail** with +`lockHeld = false` under `quint run`, so that it demonstrably fails when the +protection is missing. The counterexample trace goes in the PR. + +## Testing + +Unit, in all three languages, same cases: + +- `fd://3` rejected as a non-path value. +- `--listen-socket-mode` rejected as unknown. +- Group table: default missing → own gid + warning; explicit missing → error; + `""` → own gid, no warning; explicit numeric → that gid. +- Existing path: stale socket replaced at `0660`; live socket refused and + left in place (same inode, first listener still answers); non-socket + refused. +- Lock (Go, Rust): a second instance fails with "in use by another + instance" while the first holds the lock, including when the socket + file has been deleted by hand. After the first instance is killed with + `SIGKILL`, a new instance starts, which shows the lock doesn't go stale. + `.lock` is `0600` and still exists after a clean shutdown. +- Concurrency (Go, Rust): start N=8 instances on the same path at once. + Exactly one serves, the rest exit 1, and the socket at the path belongs + to the one that serves. Repeat 50 times. TypeScript is not held to this + test and the gap is documented; its test is written but skipped, and + references the follow-up issue. + +Integration (`deploy/docker-compose.sock.yml`), run for Go, Rust, TS: + +- `proxy-granted` and `proxy-denied` drop both socket flags and rely on + `user: 65532:` → exercises the "default group missing" fallback. + Assertions unchanged: `660`, group `2001`, granted 200 / denied 403. +- New `proxy-default-group`: runs as `65532:65532` with `group_add: [2001]` + and a bind-mounted `/etc/group` defining `docker-socket-policy:x:2001:`, and + passes no socket flags → socket must come out `660` group `2001`. This is the + dockerd-style path. + +Every new test is checked to fail without its change. Native check on macOS +and in a Linux container: stale socket, live socket, default-group-present. + +## Delivery + +One issue (`Type: Enhancement`, `Status: Break Change`), one branch +`feat/dockerd-socket-parity`, one PR titled `feat!:` with a +`BREAKING CHANGE:` footer listing the removed flag and `fd://3`. Closes the new +issue, #29 and #44. PR #43 is independent. + +The TypeScript lock follow-up is filed at the same time. The PR links to it +without closing it. diff --git a/spec/listener.qnt b/spec/listener.qnt new file mode 100644 index 0000000..9dc2607 --- /dev/null +++ b/spec/listener.qnt @@ -0,0 +1,487 @@ +// ─── Module: listener ──────────────────────────────────────────────────── +// +// Startup of the listening socket, as designed in spec/listener-design.md. +// Each instance moves through +// idle → started → locked → probed → unlinked → bound → chowned → serving +// one step per transition, so the steps of different instances interleave. +// Any instance can crash at any step: the kernel releases its lock and its +// socket file stays at the path as a stale socket. +// +// LOCK_HELD = true models Go and Rust (flock on .lock); false models +// TypeScript, where the lock step does nothing (#46). + +module listener { + const LOCK_HELD: bool + + type Config = { + listenArg: str, + groupArg: str, + defaultGroupExists: bool + } + + // kind: "absent" | "socket" | "file"; id identifies the socket inode. + type PathObj = { kind: str, id: int } + + pure val INSTANCES = Set("I1", "I2", "I3") + + pure val LISTEN_ARGS = Set("unix", "fd3", "tcp", "http") + pure val GROUP_ARGS = Set("absent", "existing", "missing", "empty") + pure val INITIAL_PATHS = Set("absent", "stale", "live", "file") + + // Socket inodes present before any instance starts. The live one belongs + // to a process outside the model (v0.2.21, a TypeScript instance, …) and + // keeps accepting connections. + pure val STALE_ID = 0 + pure val EXTERNAL_LIVE_ID = 1 + pure val FIRST_OWN_ID = 2 + + pure val MODE_BOUND = 384 // 0600: umask(0177) around bind(2) + pure val MODE_FINAL = 432 // 0660 + + pure val DEFAULT_GROUP = "docker-socket-policy" + pure val EXPLICIT_GROUP = "explicit-group" + pure val OWN_GROUP = "egid" + pure val GROUP_ERROR = "error" + + // The group-selection table from spec/listener-design.md, one row per + // (groupArg, defaultGroupExists). `=""` ignores the default group, so it + // appears for both values. + pure val groupSelectionTable = Set( + { groupArg: "absent", defaultExists: true, group: DEFAULT_GROUP, warn: false }, + { groupArg: "absent", defaultExists: false, group: OWN_GROUP, warn: true }, + { groupArg: "existing", defaultExists: true, group: EXPLICIT_GROUP, warn: false }, + { groupArg: "existing", defaultExists: false, group: EXPLICIT_GROUP, warn: false }, + { groupArg: "missing", defaultExists: true, group: GROUP_ERROR, warn: false }, + { groupArg: "missing", defaultExists: false, group: GROUP_ERROR, warn: false }, + { groupArg: "empty", defaultExists: true, group: OWN_GROUP, warn: false }, + { groupArg: "empty", defaultExists: false, group: OWN_GROUP, warn: false }, + ) + + // Group resolution as the implementations do it (moby's listeners_linux.go + // order): explicit value first, then the default name with a fallback. + pure def resolveGroup(c: Config): (str, bool) = + if (c.groupArg == "empty") (OWN_GROUP, false) + else if (c.groupArg == "existing") (EXPLICIT_GROUP, false) + else if (c.groupArg == "missing") (GROUP_ERROR, false) + else if (c.defaultGroupExists) (DEFAULT_GROUP, false) + else (OWN_GROUP, true) + + pure def hasGroupBits(mode: int): bool = (mode / 8) % 8 != 0 + + // ─── State ────────────────────────────────────────────────────────── + + var config: Config + var initPathKind: str + var pathObj: PathObj + var sockGroup: str + var sockMode: int + var lockOwner: str + var phase: str -> str + var ownSock: str -> int + var exitCode: str -> int + var selectedGroup: str // "" until the first start resolves it + var warned: bool + var nextId: int + + // A socket accepts connections while its inode is held open by a process + // that has bound it and not exited, whether or not it is still at the path. + def isLive(id: int): bool = + id == EXTERNAL_LIVE_ID or INSTANCES.exists(j => + ownSock.get(j) == id and Set("bound", "chowned", "serving").contains(phase.get(j)) + ) + + def serving(i: str): bool = phase.get(i) == "serving" + + // ─── Init ─────────────────────────────────────────────────────────── + + action initWith(c: Config, initialPath: str): bool = all { + config' = c, + initPathKind' = if (initialPath == "file") "file" + else if (initialPath == "absent") "absent" + else "socket", + pathObj' = if (initialPath == "stale") { kind: "socket", id: STALE_ID } + else if (initialPath == "live") { kind: "socket", id: EXTERNAL_LIVE_ID } + else if (initialPath == "file") { kind: "file", id: -1 } + else { kind: "absent", id: -1 }, + sockGroup' = if (initialPath == "stale" or initialPath == "live") "other" else "", + sockMode' = if (initialPath == "stale" or initialPath == "live") MODE_BOUND else 0, + lockOwner' = "", + phase' = INSTANCES.mapBy(_ => "idle"), + ownSock' = INSTANCES.mapBy(_ => -1), + exitCode' = INSTANCES.mapBy(_ => 0), + selectedGroup' = "", + warned' = false, + nextId' = FIRST_OWN_ID, + } + + action init = { + nondet listenArg = LISTEN_ARGS.oneOf() + nondet groupArg = GROUP_ARGS.oneOf() + nondet defaultGroupExists = Set(true, false).oneOf() + nondet initialPath = INITIAL_PATHS.oneOf() + initWith({ listenArg: listenArg, groupArg: groupArg, defaultGroupExists: defaultGroupExists }, initialPath) + } + + // ─── Actions ──────────────────────────────────────────────────────── + + action exitWith(i: str, code: int): bool = all { + phase' = phase.set(i, "exited"), + exitCode' = exitCode.set(i, code), + lockOwner' = if (lockOwner == i) "" else lockOwner, + } + + // Flag parsing and group resolution. Anything but a Unix path, and an + // explicit group that doesn't resolve, is a usage error (exit 2). + action start(i: str): bool = all { + phase.get(i) == "idle", + if (config.listenArg != "unix") all { + exitWith(i, 2), + selectedGroup' = selectedGroup, + warned' = warned, + } else { + val g = resolveGroup(config) + all { + selectedGroup' = g._1, + warned' = g._2, + if (g._1 == GROUP_ERROR) exitWith(i, 2) + else all { + phase' = phase.set(i, "started"), + exitCode' = exitCode, + lockOwner' = lockOwner, + }, + } + }, + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, sockMode' = sockMode, ownSock' = ownSock, nextId' = nextId, + } + + // flock(LOCK_EX|LOCK_NB) on .lock; a pass-through without the lock. + action lock(i: str): bool = all { + phase.get(i) == "started", + if (not(LOCK_HELD)) all { + phase' = phase.set(i, "locked"), + exitCode' = exitCode, + lockOwner' = lockOwner, + } else if (lockOwner == "") all { + phase' = phase.set(i, "locked"), + exitCode' = exitCode, + lockOwner' = i, + } else exitWith(i, 1), + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, sockMode' = sockMode, ownSock' = ownSock, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + // The existing-path table: stat, then connect(2) to a socket. + action probe(i: str): bool = all { + phase.get(i) == "locked", + if (pathObj.kind == "absent") all { + phase' = phase.set(i, "probed"), exitCode' = exitCode, lockOwner' = lockOwner, + } else if (pathObj.kind == "file") exitWith(i, 1) + else if (isLive(pathObj.id)) exitWith(i, 1) + else all { + phase' = phase.set(i, "probed"), exitCode' = exitCode, lockOwner' = lockOwner, + }, + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, sockMode' = sockMode, ownSock' = ownSock, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + // unlink(2) acts on whatever is at the path now, not on what probe saw. + action unlink(i: str): bool = all { + phase.get(i) == "probed", + phase' = phase.set(i, "unlinked"), + pathObj' = { kind: "absent", id: -1 }, + sockGroup' = "", + sockMode' = 0, + config' = config, initPathKind' = initPathKind, lockOwner' = lockOwner, + ownSock' = ownSock, exitCode' = exitCode, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + // bind(2)+listen(2) under umask(0177): a fresh inode at 0600, process group. + action bind(i: str): bool = all { + phase.get(i) == "unlinked", + if (pathObj.kind != "absent") all { + exitWith(i, 1), // EADDRINUSE + pathObj' = pathObj, sockGroup' = sockGroup, sockMode' = sockMode, + ownSock' = ownSock, nextId' = nextId, + } else all { + phase' = phase.set(i, "bound"), + exitCode' = exitCode, + lockOwner' = lockOwner, + pathObj' = { kind: "socket", id: nextId }, + sockGroup' = OWN_GROUP, + sockMode' = MODE_BOUND, + ownSock' = ownSock.set(i, nextId), + nextId' = nextId + 1, + }, + config' = config, initPathKind' = initPathKind, + selectedGroup' = selectedGroup, warned' = warned, + } + + // chown(path, -1, gid) — by path, like the implementations. + action chown(i: str): bool = all { + phase.get(i) == "bound", + if (pathObj.kind != "socket") all { + exitWith(i, 1), sockGroup' = sockGroup, + } else all { + phase' = phase.set(i, "chowned"), + exitCode' = exitCode, + lockOwner' = lockOwner, + sockGroup' = selectedGroup, + }, + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockMode' = sockMode, ownSock' = ownSock, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + // chmod(path, 0660) — by path; the instance then serves. + action chmod(i: str): bool = all { + phase.get(i) == "chowned", + if (pathObj.kind != "socket") all { + exitWith(i, 1), sockMode' = sockMode, + } else all { + phase' = phase.set(i, "serving"), + exitCode' = exitCode, + lockOwner' = lockOwner, + sockMode' = MODE_FINAL, + }, + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, ownSock' = ownSock, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + // SIGKILL at any step: the kernel drops the flock; the socket file stays. + action crash(i: str): bool = all { + not(Set("idle", "exited").contains(phase.get(i))), + exitWith(i, 137), + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, sockMode' = sockMode, ownSock' = ownSock, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + action stutter: bool = all { + config' = config, initPathKind' = initPathKind, pathObj' = pathObj, + sockGroup' = sockGroup, sockMode' = sockMode, lockOwner' = lockOwner, + phase' = phase, ownSock' = ownSock, exitCode' = exitCode, + selectedGroup' = selectedGroup, warned' = warned, nextId' = nextId, + } + + action advance(i: str): bool = + any { start(i), lock(i), probe(i), unlink(i), bind(i), chown(i), chmod(i) } + + // `fault` only biases the simulator: with a plain `any { …, crash(i) }` + // half of all steps crash and random runs almost never interleave two + // live instances. Every crash is still reachable from every phase, and + // stuttering adds no states, so the reachable state space is unchanged. + pure val FAULT_ODDS = 0.to(9) + + action step = { + val active = INSTANCES.filter(i => phase.get(i) != "exited") + if (active == Set()) stutter + else { + nondet i = active.oneOf() + nondet fault = FAULT_ODDS.oneOf() + if (fault == 0 and phase.get(i) != "idle") crash(i) + else if (serving(i)) stutter + else advance(i) + } + } + + // ─── Invariants ───────────────────────────────────────────────────── + + val neverListensOnTcp: bool = + INSTANCES.forall(i => serving(i) implies config.listenArg == "unix") + + val groupBeforeMode: bool = + hasGroupBits(sockMode) implies sockGroup == selectedGroup + + val neverWorldWritable: bool = sockMode % 8 < 2 + + val neverUnlinksNonSocket: bool = + initPathKind == "file" implies pathObj.kind == "file" + + val noLiveTakeover: bool = + INSTANCES.forall(i => serving(i) implies + (pathObj.kind == "socket" and pathObj.id == ownSock.get(i))) + + val groupSelectionMatchesTable: bool = + selectedGroup == "" or groupSelectionTable.contains({ + groupArg: config.groupArg, + defaultExists: config.defaultGroupExists, + group: selectedGroup, + warn: warned, + }) + + val allListenerInvariants: bool = and { + neverListensOnTcp, + groupBeforeMode, + neverWorldWritable, + neverUnlinksNonSocket, + noLiveTakeover, + groupSelectionMatchesTable, + } + + // ─── Tests: one run per spec table row ────────────────────────────── + + pure def unixCfg(groupArg: str, defaultGroupExists: bool): Config = + { listenArg: "unix", groupArg: groupArg, defaultGroupExists: defaultGroupExists } + + action toServing(i: str): bool = + start(i).then(lock(i)).then(probe(i)).then(unlink(i)) + .then(bind(i)).then(chown(i)).then(chmod(i)) + + run groupDefaultPresentTest = + initWith(unixCfg("absent", true), "absent") + .then(toServing("I1")) + .expect(and { + serving("I1"), + sockGroup == DEFAULT_GROUP, + sockMode == MODE_FINAL, + not(warned), + allListenerInvariants, + }) + + run groupDefaultMissingWarnsTest = + initWith(unixCfg("absent", false), "absent") + .then(toServing("I1")) + .expect(and { + serving("I1"), + sockGroup == OWN_GROUP, + sockMode == MODE_FINAL, + warned, + allListenerInvariants, + }) + + run groupExplicitPresentTest = + initWith(unixCfg("existing", false), "absent") + .then(toServing("I1")) + .expect(and { + serving("I1"), + sockGroup == EXPLICIT_GROUP, + sockMode == MODE_FINAL, + not(warned), + allListenerInvariants, + }) + + run groupExplicitMissingFailsTest = + initWith(unixCfg("missing", true), "absent") + .then(start("I1")) + .expect(and { + phase.get("I1") == "exited", + exitCode.get("I1") == 2, + pathObj.kind == "absent", + allListenerInvariants, + }) + + run groupEmptyUsesOwnTest = + initWith(unixCfg("empty", true), "absent") + .then(toServing("I1")) + .expect(and { + serving("I1"), + sockGroup == OWN_GROUP, + sockMode == MODE_FINAL, + not(warned), + allListenerInvariants, + }) + + run pathAbsentBindsTest = + initWith(unixCfg("absent", true), "absent") + .then(toServing("I1")) + .expect(and { + serving("I1"), + pathObj == { kind: "socket", id: ownSock.get("I1") }, + allListenerInvariants, + }) + + run pathStaleReplacedTest = + initWith(unixCfg("absent", true), "stale") + .then(toServing("I1")) + .expect(and { + serving("I1"), + pathObj == { kind: "socket", id: ownSock.get("I1") }, + ownSock.get("I1") != STALE_ID, + sockMode == MODE_FINAL, + allListenerInvariants, + }) + + run pathLiveRefusedTest = + initWith(unixCfg("absent", true), "live") + .then(start("I1")).then(lock("I1")).then(probe("I1")) + .expect(and { + phase.get("I1") == "exited", + exitCode.get("I1") == 1, + pathObj == { kind: "socket", id: EXTERNAL_LIVE_ID }, + allListenerInvariants, + }) + + run pathNotSocketRefusedTest = + initWith(unixCfg("absent", true), "file") + .then(start("I1")).then(lock("I1")).then(probe("I1")) + .expect(and { + phase.get("I1") == "exited", + exitCode.get("I1") == 1, + pathObj.kind == "file", + allListenerInvariants, + }) +} + +// ─── Instances ────────────────────────────────────────────────────────── + +// Go and Rust: the single-instance lock is taken before the path is touched. +module listener_locked { + import listener(LOCK_HELD = true).* + + run secondInstanceLockRefusedTest = + initWith(unixCfg("absent", true), "absent") + .then(start("I1")).then(lock("I1")) + .then(start("I2")).then(lock("I2")) + .expect(and { + lockOwner == "I1", + phase.get("I1") == "locked", + phase.get("I2") == "exited", + exitCode.get("I2") == 1, + allListenerInvariants, + }) + + run crashReleasesLockTest = + initWith(unixCfg("absent", true), "absent") + .then(toServing("I1")) + .then(crash("I1")) + .expect(lockOwner == "") + .then(toServing("I2")) + .expect(and { + lockOwner == "I2", + serving("I2"), + pathObj == { kind: "socket", id: ownSock.get("I2") }, + allListenerInvariants, + }) +} + +// TypeScript: no flock in Node, so only the connect check guards the path. +module listener_unlocked { + import listener(LOCK_HELD = false).* + + // The four-step race from spec/listener-design.md §TypeScript exception: + // both probe a stale socket, I1 binds and serves, I2 unlinks I1's live + // socket and binds its own. I1 keeps serving on an inode nobody can reach. + run raceWithoutLockTest = + initWith(unixCfg("absent", true), "stale") + .then(start("I1")).then(lock("I1")) + .then(start("I2")).then(lock("I2")) + .then(probe("I1")) + .then(probe("I2")) + .then(unlink("I1")).then(bind("I1")).then(chown("I1")).then(chmod("I1")) + .expect(serving("I1") and noLiveTakeover) + .then(unlink("I2")) + .expect(serving("I1") and not(noLiveTakeover)) + .then(bind("I2")) + .expect(and { + serving("I1"), + pathObj.kind == "socket", + pathObj.id == ownSock.get("I2"), + pathObj.id != ownSock.get("I1"), + not(noLiveTakeover), + }) +} diff --git a/ts/src/flags.test.ts b/ts/src/flags.test.ts index 673c40a..a664060 100644 --- a/ts/src/flags.test.ts +++ b/ts/src/flags.test.ts @@ -4,13 +4,17 @@ import { mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { + BOOL_FLAGS, + DEFAULT_SOCKET_GROUP, getFlag, + type GroupId, + selectSocketGroup, hasFlag, parseListenSocket, - parseSocketMode, parseSocketPath, resolveGroup, validateFlags, + VALUE_FLAGS, } from "./flags.js"; describe("flags", () => { @@ -111,6 +115,15 @@ describe("flags", () => { // it matches Go's flag package, and the point is that it is not an error. assert.equal(check(["--config-dir", "--readonly"]), null); }); + + // The socket mode is fixed. A deployment still passing the flag must fail + // at startup rather than silently get a different mode than it asked for. + it("rejects --listen-socket-mode, which this proxy no longer has", () => { + assert.match( + validateFlags(["--listen-socket-mode=0660"], VALUE_FLAGS, BOOL_FLAGS) ?? "", + /unrecognised flag: --listen-socket-mode/, + ); + }); }); describe("parseListenSocket", () => { @@ -121,15 +134,12 @@ describe("flags", () => { }); }); - 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"]) { + // Socket activation was removed; fd:// is just another scheme now. + it("rejects fd:// addresses, including fd://3", () => { + for (const input of ["fd://3", "fd://4", "fd://0", "fd://", "fd://abc"]) { const result = parseListenSocket(input); assert.ok(result.kind === "error", `expected ${input} to be rejected`); - assert.match(result.message, /only supports fd:\/\/3/); + assert.match(result.message, /only supports Unix socket paths/); } }); @@ -195,43 +205,6 @@ describe("flags", () => { }); }); -describe("parseSocketMode", () => { - it("accepts octal modes with and without a leading zero", () => { - for (const [input, want] of [ - ["0660", 0o660], - ["660", 0o660], - ["0600", 0o600], - ["0640", 0o640], - ] as const) { - const got = parseSocketMode(input); - assert.deepEqual(got, { mode: want }, `${input} should parse`); - } - }); - - // connect(2) needs write permission, so o+w means every local uid can - // connect. There is deliberately no opt-out for this. - it("rejects world-writable modes", () => { - for (const input of ["0666", "0777", "0602"]) { - const got = parseSocketMode(input); - assert.ok("error" in got, `${input} should be rejected`); - assert.match(got.error, /world-writable/); - } - }); - - it("rejects malformed and out-of-range modes", () => { - for (const [input, want] of [ - ["", /must not be empty/], - ["0x1ff", /not an octal mode/], - ["668", /not an octal mode/], - ["1777", /within 0777/], - ] as const) { - const got = parseSocketMode(input); - assert.ok("error" in got, `${input} should be rejected`); - assert.match(got.error, want); - } - }); -}); - describe("resolveGroup", () => { function groupFile(contents: string): string { const p = join(mkdtempSync(join(tmpdir(), "grp-")), "group"); @@ -268,3 +241,89 @@ describe("resolveGroup", () => { assert.match(got.error, /cannot read/); }); }); + +describe("resolveGroup rejects out-of-range gid", () => { + // 4294967295 is chown's "don't change" sentinel; larger values do not fit + // a gid_t at all. + it("accepts 4294967294 and rejects anything above it", () => { + assert.deepEqual(resolveGroup("4294967294", "/nonexistent"), { gid: 4294967294 }); + for (const v of ["4294967295", "4294967296", "12345678901234567890"]) { + assert.deepEqual(resolveGroup(v, "/nonexistent"), { + error: `--listen-socket-group ${JSON.stringify(v)}: gid out of range (0-4294967294)`, + }); + } + // Only a digit string is numeric; a sign makes it a (nonexistent) name. + const f = join(mkdtempSync(join(tmpdir(), "grp-")), "group"); + writeFileSync(f, "root:x:0:\n"); + for (const v of ["+4294967296", "+5"]) { + assert.ok("error" in resolveGroup(v, f), `resolveGroup(${JSON.stringify(v)}) should fail`); + } + }); +}); + +// One case per row of the group-selection table in spec/listener-design.md; +// the test names match the Quint runs. +describe("selectSocketGroup", () => { + const egid = 65532; + const known = + (groups: Record) => + (name: string): GroupId => + name in groups + ? { gid: groups[name] } + : { error: `--listen-socket-group ${JSON.stringify(name)}: unknown group` }; + const both = known({ "docker-socket-policy": 2001, ops: 3001 }); + + it("groupDefaultPresent: flag absent, default group exists", () => { + assert.deepEqual(selectSocketGroup(undefined, both, egid), { gid: 2001 }); + }); + + it("groupDefaultMissingWarns: flag absent, default group missing", () => { + assert.deepEqual(selectSocketGroup(undefined, known({}), egid), { + gid: egid, + warning: "group docker-socket-policy not found, using the proxy's own group 65532", + }); + }); + + it("groupExplicitPresent: explicit group exists", () => { + assert.deepEqual(selectSocketGroup("ops", both, egid), { gid: 3001 }); + }); + + it("groupExplicitMissingFails: explicit group missing", () => { + const got = selectSocketGroup("nope", both, egid); + assert.ok("error" in got, `want an error, got ${JSON.stringify(got)}`); + }); + + it("groupEmptyUsesOwn: explicit empty uses the proxy's own group", () => { + assert.deepEqual(selectSocketGroup("", both, egid), { gid: egid }); + }); + + it("uses the default group name", () => { + assert.equal(DEFAULT_SOCKET_GROUP, "docker-socket-policy"); + }); +}); + +// An explicit empty value means "the proxy's own group" and must not be +// confused with the flag being absent, which means the default group. This is +// the expression index.ts uses. +describe("--listen-socket-group empty vs absent", () => { + const groupFlag = (args: string[]) => + hasFlag(args, "--listen-socket-group") + ? getFlag(args, "--listen-socket-group", "") + : undefined; + + it("absent", () => { + assert.equal(groupFlag([]), undefined); + }); + + it("equals empty", () => { + assert.equal(groupFlag(["--listen-socket-group="]), ""); + }); + + it("separate empty", () => { + assert.equal(groupFlag(["--listen-socket-group", ""]), ""); + }); + + it("named", () => { + assert.equal(groupFlag(["--listen-socket-group=ops"]), "ops"); + }); +}); diff --git a/ts/src/flags.ts b/ts/src/flags.ts index 03e8595..0be7280 100644 --- a/ts/src/flags.ts +++ b/ts/src/flags.ts @@ -21,6 +21,16 @@ export function hasFlag(args: string[], name: string): boolean { return args.includes(name) || args.some((a) => a.startsWith(prefix)); } +// The flags the proxy accepts, for validateFlags. +export const VALUE_FLAGS = [ + "--listen-socket", + "--docker-host", + "--config-dir", + "--log-file", + "--listen-socket-group", +]; +export const BOOL_FLAGS = ["--readonly"]; + // 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 @@ -56,32 +66,18 @@ export function validateFlags( 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. +// Parses --listen-socket, which must be an absolute filesystem path. Mirrors +// validateListenSocket in Go and validate_listen_socket 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("fd://") || input.startsWith("tcp://") || input.startsWith("http://") || input.startsWith("https://") || @@ -112,10 +108,10 @@ export function parseListenSocket(input: string): ListenTarget { return { kind: "path", path: input }; } -// Mode applied to the listening socket when --listen-socket-mode is omitted. -// connect(2) on an AF_UNIX socket requires write permission, so 0o660 is what -// actually grants the owning group access. -export const DEFAULT_LISTEN_SOCKET_MODE = 0o660; +// Mode applied to the listening socket. connect(2) on an AF_UNIX socket +// requires write permission, so 0o660 is what actually grants the owning group +// access. +export const SOCKET_MODE = 0o660; // Set around bind(2) so the socket is created at 0600 and is never briefly // reachable by group or world. bind() applies 0777 & ~umask, and @@ -123,33 +119,38 @@ export const DEFAULT_LISTEN_SOCKET_MODE = 0o660; // in which the socket is already listening at the ambient mode. export const BIND_UMASK = 0o177; -export type SocketMode = { mode: number } | { error: string }; +export type GroupId = { gid: number } | { error: string }; -// Parses an octal mode and rejects anything world-writable. A world-writable -// socket is connectable by every local uid, which removes the boundary -// entirely, so there is deliberately no opt-out. -export function parseSocketMode(input: string): SocketMode { - if (input.length === 0) { - return { error: "--listen-socket-mode must not be empty" }; - } - if (!/^[0-7]+$/.test(input)) { - return { error: `--listen-socket-mode ${JSON.stringify(input)}: not an octal mode` }; - } - const mode = parseInt(input, 8); - if (mode > 0o777) { - return { error: `--listen-socket-mode ${JSON.stringify(input)}: must be within 0777` }; +// The group the socket is given when --listen-socket-group is not passed. +export const DEFAULT_SOCKET_GROUP = "docker-socket-policy"; + +// Picks the socket's group the way dockerd does +// (moby/daemon/listeners/listeners_linux.go). An undefined flag means the flag +// was not passed: the default group is used if it exists, and otherwise the +// proxy falls back to its own group with a warning. An explicit group that +// does not resolve is an error. An explicit "" selects the proxy's own group. +export function selectSocketGroup( + flag: string | undefined, + lookup: (name: string) => GroupId, + egid: number, +): { gid: number; warning?: string } | { error: string } { + if (flag === undefined) { + const found = lookup(DEFAULT_SOCKET_GROUP); + if ("error" in found) { + return { + gid: egid, + warning: `group ${DEFAULT_SOCKET_GROUP} not found, using the proxy's own group ${egid}`, + }; + } + return { gid: found.gid }; } - if (mode & 0o002) { - return { - error: - `--listen-socket-mode ${JSON.stringify(input)} is world-writable: every local user ` + - `could connect to the proxy, which disables the access-control boundary`, - }; + if (flag === "") { + return { gid: egid }; } - return { mode }; + return lookup(flag); } -export type GroupId = { gid: number } | { error: string }; +const MAX_SOCKET_GID = 4294967294; // Maps --listen-socket-group to a gid. A numeric value is used as-is so a // deployment without the group defined can still be configured. @@ -160,6 +161,13 @@ export type GroupId = { gid: number } | { error: string }; // os/user.LookupGroup and Rust uses getgrnam_r, both of which do consult NSS. export function resolveGroup(input: string, groupFile = "/etc/group"): GroupId { if (/^\d+$/.test(input)) { + // 4294967295 is chown's "don't change" sentinel. BigInt keeps a long + // digit string exact, where parseInt would round it. + if (BigInt(input) > BigInt(MAX_SOCKET_GID)) { + return { + error: `--listen-socket-group ${JSON.stringify(input)}: gid out of range (0-${MAX_SOCKET_GID})`, + }; + } return { gid: parseInt(input, 10) }; } let contents: string; diff --git a/ts/src/index.ts b/ts/src/index.ts index 4e2455e..2235201 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -1,5 +1,4 @@ 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"; @@ -7,29 +6,22 @@ import { Router } from "./proxy.js"; import { Handler } from "./handler.js"; import { Transport } from "./transport.js"; import { createShutdown } from "./shutdown.js"; -import { listenOnSocket } from "./listen.js"; +import { openListener } from "./listen.js"; import { + BOOL_FLAGS, getFlag, hasFlag, parseListenSocket, - parseSocketMode, parseSocketPath, resolveGroup, + selectSocketGroup, + SOCKET_MODE, validateFlags, + VALUE_FLAGS, } from "./flags.js"; const args = process.argv.slice(2); -const VALUE_FLAGS = [ - "--listen-socket", - "--docker-host", - "--config-dir", - "--log-file", - "--listen-socket-mode", - "--listen-socket-group", -]; -const BOOL_FLAGS = ["--readonly"]; - const flagError = validateFlags(args, VALUE_FLAGS, BOOL_FLAGS); if (flagError) { console.error(flagError); @@ -48,23 +40,19 @@ if (listenTarget.kind === "error") { console.error(listenTarget.message); process.exit(2); } -const parsedMode = parseSocketMode(getFlag(args, "--listen-socket-mode", "0660")); -if ("error" in parsedMode) { - console.error(parsedMode.error); +const groupSelection = selectSocketGroup( + hasFlag(args, "--listen-socket-group") ? getFlag(args, "--listen-socket-group", "") : undefined, + resolveGroup, + process.getegid!(), +); +if ("error" in groupSelection) { + console.error(groupSelection.error); process.exit(2); } -const socketMode = parsedMode.mode; - -const groupFlag = getFlag(args, "--listen-socket-group", ""); -let socketGid: number | undefined; -if (groupFlag !== "") { - const resolved = resolveGroup(groupFlag); - if ("error" in resolved) { - console.error(resolved.error); - process.exit(2); - } - socketGid = resolved.gid; +if (groupSelection.warning) { + console.warn(groupSelection.warning); } +const socketGid = groupSelection.gid; const configDir = getFlag(args, "--config-dir", "/etc/docker-socket-policy/services"); const logFile = getFlag(args, "--log-file", "/var/log/docker-socket-policy.log"); @@ -104,53 +92,18 @@ server.once("listening", () => { 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); - } - } - listenOnSocket(server, path, socketMode, socketGid).then( - () => { - console.log( - `listening on unix socket ${path} (mode ${socketMode.toString(8).padStart(4, "0")})`, - ); - }, - (err: NodeJS.ErrnoException) => { - console.error(`failed to listen on ${path}: ${err.message}`); - process.exit(1); - }, - ); -} +const path = listenTarget.path; +openListener(server, path, socketGid).then( + () => { + console.log( + `listening on unix socket ${path} (mode ${SOCKET_MODE.toString(8).padStart(4, "0")})`, + ); + }, + (err: NodeJS.ErrnoException) => { + console.error(`failed to listen on ${path}: ${err.message}`); + process.exit(1); + }, +); const shutdown = createShutdown(server); diff --git a/ts/src/listen.test.ts b/ts/src/listen.test.ts index 7b859d5..895b4d1 100644 --- a/ts/src/listen.test.ts +++ b/ts/src/listen.test.ts @@ -1,10 +1,21 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; import { createServer } from "node:http"; -import { mkdtempSync, rmSync, statSync } from "node:fs"; +import { + chmodSync, + chownSync, + existsSync, + lstatSync, + mkdtempSync, + renameSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; +import { connect, createServer as createNetServer, type Server } from "node:net"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { listenOnSocket } from "./listen.js"; +import { listenOnSocket, openListener, prepareSocketPath } from "./listen.js"; // sun_path is capped at 104 bytes on macOS, so keep the path short. function tempSocket(): { path: string; cleanup: () => void } { @@ -23,21 +34,15 @@ describe("listenOnSocket", () => { // Regression test for #40: the mode used to be whatever the umask left // behind, which is 0755 by default. connect(2) requires write permission, so // the documented group grant silently did not work. - it("applies the requested mode", async () => { - for (const mode of [0o660, 0o600, 0o640]) { - const { path, cleanup } = tempSocket(); - const server = createServer(() => {}); - await listenOnSocket(server, path, mode); + it("applies mode 0660", async () => { + const { path, cleanup } = tempSocket(); + const server = createServer(() => {}); + await listenOnSocket(server, path); - assert.equal( - modeOf(path), - mode, - `socket mode was ${modeOf(path).toString(8)}, want ${mode.toString(8)}`, - ); + assert.equal(modeOf(path), 0o660, `socket mode was ${modeOf(path).toString(8)}, want 660`); - await new Promise((r) => server.close(r)); - cleanup(); - } + await new Promise((r) => server.close(r)); + cleanup(); }); // The ambient umask must not influence the result: that was the whole bug. @@ -46,7 +51,7 @@ describe("listenOnSocket", () => { const { path, cleanup } = tempSocket(); const server = createServer(() => {}); try { - await listenOnSocket(server, path, 0o660); + await listenOnSocket(server, path); } finally { process.umask(previous); } @@ -70,7 +75,7 @@ describe("listenOnSocket", () => { const { path, cleanup } = tempSocket(); const before = process.umask(); const server = createServer(() => {}); - await listenOnSocket(server, path, 0o660); + await listenOnSocket(server, path); assert.equal(process.umask(), before, "umask was not restored after bind"); @@ -83,7 +88,7 @@ describe("listenOnSocket", () => { const server = createServer(() => {}); await assert.rejects( - () => listenOnSocket(server, "/nonexistent-dir-xyz/s.sock", 0o660), + () => listenOnSocket(server, "/nonexistent-dir-xyz/s.sock"), /ENOENT|EACCES/, ); assert.equal(process.umask(), before, "umask was not restored after a failed bind"); @@ -92,7 +97,7 @@ describe("listenOnSocket", () => { it("is actually connectable at the mode it sets", async () => { const { path, cleanup } = tempSocket(); const server = createServer((_req, res) => res.end("ok")); - await listenOnSocket(server, path, 0o660); + await listenOnSocket(server, path); const { connect } = await import("node:net"); const reply = await new Promise((resolve, reject) => { @@ -115,3 +120,219 @@ describe("listenOnSocket", () => { cleanup(); }); }); + +// Leaves a socket file at path with nothing listening on it, as an unclean +// shutdown would: connect(2) to it is refused. Node unlinks a Unix socket when +// its server closes, so bind at a sibling path and rename the socket into +// place first; close then unlinks the (now absent) sibling path only. +async function seedStaleSocket(path: string): Promise { + const seed = path + ".seed"; + const server = createNetServer(); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(seed, () => resolve()); + }); + renameSync(seed, path); + await new Promise((r) => server.close(r)); +} + +function inode(path: string): number { + return lstatSync(path).ino; +} + +function closeServer(server: Server): Promise { + return new Promise((r) => server.close(() => r())); +} + +// One case per row of the existing-path table in spec/listener-design.md; the +// test names match the Quint runs. +describe("prepareSocketPath", () => { + it("pathAbsentBinds", async () => { + const { path, cleanup } = tempSocket(); + try { + await prepareSocketPath(path); + } finally { + cleanup(); + } + }); + + it("pathStaleReplaced", async () => { + const { path, cleanup } = tempSocket(); + try { + await seedStaleSocket(path); + assert.ok(lstatSync(path).isSocket(), "seeding did not leave a socket behind"); + await prepareSocketPath(path); + assert.equal(existsSync(path), false, "stale socket still present after prepare"); + } finally { + cleanup(); + } + }); + + it("pathLiveRefused", async () => { + const { path, cleanup } = tempSocket(); + const server = createNetServer(); + try { + await new Promise((resolve) => server.listen(path, () => resolve())); + const before = inode(path); + + await assert.rejects(prepareSocketPath(path), { + message: `${path} is in use by another process`, + }); + assert.equal(inode(path), before, "live socket inode changed: it was replaced"); + + const accepted = new Promise((resolve) => + server.once("connection", (c) => { + c.destroy(); + resolve(); + }), + ); + await new Promise((resolve, reject) => { + const c = connect(path, () => { + c.destroy(); + resolve(); + }); + c.once("error", reject); + }); + await accepted; + } finally { + await closeServer(server); + cleanup(); + } + }); + + // connect(2) needs write permission on the socket, so a 0000 socket yields + // EACCES: neither live nor provably stale, so it is left alone. + it( + "pathConnectErrorRefused", + { skip: process.geteuid?.() === 0 ? "root ignores socket permissions" : false }, + async () => { + const { path, cleanup } = tempSocket(); + try { + await seedStaleSocket(path); + chmodSync(path, 0); + await assert.rejects(prepareSocketPath(path), { + message: `refusing to remove ${path}: connect EACCES ${path}`, + }); + assert.ok(lstatSync(path).isSocket(), "socket was removed"); + } finally { + cleanup(); + } + }, + ); + + it("pathNotSocketRefused", async () => { + const { path, cleanup } = tempSocket(); + try { + writeFileSync(path, "data"); + await assert.rejects(prepareSocketPath(path), { + message: `refusing to remove ${path}: not a socket`, + }); + assert.equal(existsSync(path), true, "regular file was removed"); + } finally { + cleanup(); + } + }); +}); + +// A non-root proxy that is not a member of the selected group cannot chown the +// socket to it. The error must say what to fix rather than surface a bare EPERM. +describe("listenOnSocket chown EPERM", () => { + const skip = + process.geteuid?.() === 0 + ? "root can chown to any group" + : process.getgroups?.().includes(0) + ? "process is a member of gid 0, so chown to it succeeds" + : false; + + it("names the group", { skip }, async () => { + const { path, cleanup } = tempSocket(); + // BSD semantics (macOS) give a new file its directory's group, which is + // gid 0 under /tmp, and chown to the current group is always allowed. + // Give the directory our own group so the socket starts out not in gid 0. + chownSync(join(path, ".."), -1, process.getegid!()); + const server = createServer(() => {}); + try { + await assert.rejects(listenOnSocket(server, path, 0), { + message: + `cannot give ${path} to group 0: the proxy's user must be a member of it ` + + `(SupplementaryGroups= / group_add:)`, + }); + assert.equal(server.listening, false, "server left listening after chown failed"); + } finally { + cleanup(); + } + }); +}); + +describe("openListener", () => { + it("replaces a stale socket and applies mode 0660", async () => { + const { path, cleanup } = tempSocket(); + const server = createServer(() => {}); + try { + await seedStaleSocket(path); + chmodSync(path, 0o777); + await openListener(server, path, process.getegid!()); + assert.equal(modeOf(path), 0o660); + } finally { + await closeServer(server); + cleanup(); + } + }); + + // Node has no flock, so TypeScript takes no single-instance lock and relies + // on the connect probe alone, which races (spec/listener-design.md, + // "TypeScript exception"). This is the Go/Rust concurrency test unchanged: + // un-skipping it is the whole test for the follow-up. + it("openListener concurrent (#46)", { skip: "no flock in Node — #46" }, async () => { + const { path: base, cleanup } = tempSocket(); + const dir = join(base, ".."); + const racers = 8; + try { + for (let i = 0; i < 50; i++) { + const path = join(dir, `c${i}.sock`); + const servers = Array.from({ length: racers }, () => createNetServer()); + try { + const results = await Promise.allSettled( + servers.map((s) => openListener(s, path, process.getegid!())), + ); + + const winners = results.flatMap((r, j) => (r.status === "fulfilled" ? [j] : [])); + assert.equal( + winners.length, + 1, + `iteration ${i}: ${winners.length} openListener calls succeeded, want 1`, + ); + for (const r of results) { + if (r.status === "rejected") { + assert.match( + (r.reason as Error).message, + /is in use by another/, + `iteration ${i}: loser error, want an in-use error`, + ); + } + } + + const winner = servers[winners[0]]; + const accepted = new Promise((resolve) => + winner.once("connection", (c) => { + c.destroy(); + resolve(); + }), + ); + await new Promise((resolve, reject) => { + const c = connect(path, () => { + c.destroy(); + resolve(); + }); + c.once("error", reject); + }); + await accepted; + } finally { + await Promise.all(servers.filter((s) => s.listening).map(closeServer)); + } + } + } finally { + cleanup(); + } + }); +}); diff --git a/ts/src/listen.ts b/ts/src/listen.ts index defe57c..270bf7b 100644 --- a/ts/src/listen.ts +++ b/ts/src/listen.ts @@ -1,9 +1,11 @@ -import type { Server } from "node:http"; -import { chmodSync, chownSync } from "node:fs"; -import { BIND_UMASK } from "./flags.js"; +import type { Server } from "node:net"; +import { chmodSync, chownSync, lstatSync, unlinkSync } from "node:fs"; +import { connect } from "node:net"; +import { BIND_UMASK, SOCKET_MODE } from "./flags.js"; /** - * Binds `server` to a Unix socket path with an explicit mode and group. + * Binds `server` to a Unix socket path with mode SOCKET_MODE and an optional + * group. * * Extracted from index.ts so it can be tested: the mode of the listening * socket is invisible to the integration suite, which only observes HTTP @@ -18,11 +20,13 @@ import { BIND_UMASK } from "./flags.js"; * afterwards, because a chmod after bind leaves a window in which the socket is * already listening at the ambient mode. The window here is at 0600 instead, * which is more restrictive than any mode we would set. + * + * main always passes the gid chosen by selectSocketGroup; an undefined gid + * skips the chown and exists only for tests. */ export function listenOnSocket( server: Server, path: string, - mode: number, gid?: number, ): Promise { return new Promise((resolve, reject) => { @@ -43,10 +47,21 @@ export function listenOnSocket( // Set the group before widening the mode, so the socket is never // reachable by the wrong group. if (gid !== undefined) { - chownSync(path, -1, gid); + try { + chownSync(path, -1, gid); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EPERM") { + throw new Error( + `cannot give ${path} to group ${gid}: the proxy's user must be a member of it ` + + `(SupplementaryGroups= / group_add:)`, + ); + } + throw err; + } } - chmodSync(path, mode); + chmodSync(path, SOCKET_MODE); } catch (err) { + server.close(); reject(err); return; } @@ -54,3 +69,68 @@ export function listenOnSocket( }); }); } + +// Bounds the connect(2) that tells a live socket from a stale one. +const PROBE_TIMEOUT_MS = 1000; + +/** + * Clears the socket path for bind, following the existing-path table in + * spec/listener-design.md. Only a socket that refuses connections is removed. + * A live one belongs to another process, and replacing it would cut that + * process off silently. Anything that is not a socket is refused: blindly + * unlinking would let a mistyped path silently delete an operator's file. + */ +export async function prepareSocketPath(path: string): Promise { + let isSocket: boolean; + try { + isSocket = lstatSync(path).isSocket(); + } catch (err) { + const e = err as NodeJS.ErrnoException; + if (e.code === "ENOENT") return; + throw new Error(`checking ${path}: ${e.message}`); + } + if (!isSocket) { + throw new Error(`refusing to remove ${path}: not a socket`); + } + + await new Promise((resolve, reject) => { + const probe = connect(path); + const timer = setTimeout(() => { + probe.destroy(); + reject(new Error(`${path} is in use by another process`)); + }, PROBE_TIMEOUT_MS); + probe.once("connect", () => { + clearTimeout(timer); + probe.destroy(); + reject(new Error(`${path} is in use by another process`)); + }); + probe.once("error", (err: NodeJS.ErrnoException) => { + clearTimeout(timer); + probe.destroy(); + if (err.code === "ECONNREFUSED") { + resolve(); + } else { + reject(new Error(`refusing to remove ${path}: ${err.message}`)); + } + }); + }); + + try { + unlinkSync(path); + } catch (err) { + throw new Error(`removing stale socket ${path}: ${(err as Error).message}`); + } +} + +/** + * Clears the socket path and binds it. + * + * Unlike Go and Rust, this takes no single-instance lock: Node has no flock + * (#46, spec/listener-design.md §TypeScript exception). Two instances starting + * together can both see a stale socket refuse the probe, and the later one then + * unlinks the earlier one's freshly bound, live socket and takes the path over. + */ +export async function openListener(server: Server, path: string, gid: number): Promise { + await prepareSocketPath(path); + await listenOnSocket(server, path, gid); +}