From 5d9fd0e9530fb67aed56aeb0367334bb97ef63b2 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Sun, 20 Sep 2026 01:43:48 +0000 Subject: [PATCH] fix(container-logs): let the grant serve a real Docker client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The container-logs@1 proxy has shipped since 0.11.0-rc.1 with no provider to exercise it. Packaging one (Dozzle, try-hola/apps#160) and installing it on a VM found three ways the envelope refused the clients it exists for. All three were shape, not policy — nothing here widens what the grant discloses. 1. HEAD was refused. Docker's own client pings with `HEAD /_ping` before anything else, so a GET-only allowlist rejected every standard client on its first call, and the client reported it as "Could not connect to any Docker Engine" — not as a refusal. HEAD is now allowed wherever GET is: it returns headers and no body, so it reveals strictly less than the GET it mirrors. 2. GET /info was refused outright. A client on Docker's SDK calls it to decide an engine is really there and exits without it. It is now rebuilt from an allowlist, the same way inspect is: what identifies and sizes the engine survives (name, version, OS, arch, cpu/memory, container counts) and everything describing how the host is configured does not — RegistryConfig, Labels, Plugins, DockerRootDir, SecurityOptions, Swarm, and above all HttpProxy/HttpsProxy, which routinely embed credentials. 3. The redacted inspect omitted HostConfig, Mounts and NetworkSettings entirely. A real daemon always returns them, so clients walk them without checking — Dozzle segfaults on HostConfig.PortBindings. Dropping the field denied the client, not the data. They are now present but empty: no host port map, no bind sources, no network topology, and the same nothing on the wire, in a shape an SDK client can parse. Verified on a disposable VM against the real app: before, dozzle crash-looped on "Could not connect"; after, it reports "Connected to Docker", serves, and reads a neighbouring app's logs with sh.hola.app labels intact. The envelope still holds — stats, archive, images, secrets, volumes, POST restart, POST exec and DELETE all answer 403, and the neighbour's inspect carries no Env. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vck5KSX2CLxhohx14nb5Sh --- .../src/__tests__/lib/docker-proxy.test.ts | 157 +++++++++++++++++- packages/server/src/lib/docker-proxy.ts | 68 +++++++- 2 files changed, 213 insertions(+), 12 deletions(-) diff --git a/packages/server/src/__tests__/lib/docker-proxy.test.ts b/packages/server/src/__tests__/lib/docker-proxy.test.ts index 5470db6e..6ae5c39b 100644 --- a/packages/server/src/__tests__/lib/docker-proxy.test.ts +++ b/packages/server/src/__tests__/lib/docker-proxy.test.ts @@ -8,7 +8,7 @@ import { mkdtemp, rm } from 'fs/promises'; import { tmpdir } from 'os'; import { join } from 'path'; -import { decide, redactInspect, startDockerProxy } from '../../lib/docker-proxy'; +import { decide, redactInspect, startDockerProxy, redactInfo } from '../../lib/docker-proxy'; import type { DockerProxyHandle } from '../../lib/docker-proxy'; describe('decide', () => { @@ -74,11 +74,17 @@ describe('redactInspect', () => { expect(config.Hostname).toBe('abc123'); }); - test('drops Config.Env, Config.Cmd, Config.Entrypoint, HostConfig, Mounts, NetworkSettings', () => { + test('empties HostConfig, Mounts and NetworkSettings without dropping them', () => { + // Emptied rather than removed: a real daemon always returns these, so + // clients walk them unchecked (Dozzle segfaults on a missing HostConfig). + // The content is what the grant withholds, not the shape. const redacted = redactInspect(FULL_INSPECT) as Record; - expect(redacted.HostConfig).toBeUndefined(); - expect(redacted.Mounts).toBeUndefined(); - expect(redacted.NetworkSettings).toBeUndefined(); + expect(redacted.HostConfig).toEqual({ PortBindings: {} }); + expect(redacted.Mounts).toEqual([]); + expect(redacted.NetworkSettings).toEqual({ Networks: {} }); + const serialized = JSON.stringify(redacted); + expect(serialized).not.toContain('172.18.0.5'); + expect(serialized).not.toContain('/var/run/docker.sock'); const config = redacted.Config as Record; expect(config.Env).toBeUndefined(); expect(config.Cmd).toBeUndefined(); @@ -92,6 +98,7 @@ describe('redactInspect', () => { expect(redactInspect({})).toEqual({ Id: undefined, Name: undefined, Created: undefined, State: undefined, Image: undefined, Config: { Tty: undefined, Labels: undefined, Image: undefined, Hostname: undefined }, + HostConfig: { PortBindings: {} }, Mounts: [], NetworkSettings: { Networks: {} }, }); }); }); @@ -178,15 +185,15 @@ describe('startDockerProxy (integration, fake Docker API on a temp unix socket)' expect(await res.json()).toEqual([{ Id: 'c1', Names: ['/app'] }]); }); - test('GET /v1.45/containers/{id}/json is redacted (no Env, no HostConfig, no Mounts)', async () => { + test('GET /v1.45/containers/{id}/json is redacted (no Env, empty HostConfig and Mounts)', async () => { const res = await fetch(proxyUrl('/v1.45/containers/c1/json')); expect(res.status).toBe(200); const body = await res.json(); expect(body.Config.Tty).toBe(true); expect(body.Config.Labels).toEqual({ app: 'x' }); expect(body.Config.Env).toBeUndefined(); - expect(body.HostConfig).toBeUndefined(); - expect(body.Mounts).toBeUndefined(); + expect(body.HostConfig).toEqual({ PortBindings: {} }); + expect(body.Mounts).toEqual([]); }); test('GET /containers/{id}/logs streams bytes identical', async () => { @@ -233,3 +240,137 @@ describe('startDockerProxy (integration, fake Docker API on a temp unix socket)' IDLE_GAP_MS + 15_000, ); }); + +// --------------------------------------------------------------------------- +// What a real Docker client actually sends (found by running Dozzle, the first +// container-logs@1 provider, against this proxy on a VM). +describe('serving a standard Docker client', () => { + test('HEAD /_ping is allowed — every Docker client pings with HEAD first', () => { + // A GET-only allowlist refuses the very first call any client makes, and + // the client reports it as "no Docker engine" rather than as a refusal. + expect(decide('HEAD', '/_ping')).toEqual({ allow: true, kind: 'passthrough' }); + expect(decide('HEAD', '/v1.52/_ping')).toEqual({ allow: true, kind: 'passthrough' }); + }); + + test('HEAD is allowed wherever GET is — it reveals strictly less', () => { + expect(decide('HEAD', '/containers/json')).toEqual({ allow: true, kind: 'passthrough' }); + expect(decide('HEAD', '/containers/abc123/json')).toEqual({ allow: true, kind: 'inspect' }); + }); + + test('HEAD does not open anything GET cannot reach', () => { + expect(decide('HEAD', '/containers/abc123/archive')).toEqual({ allow: false }); + expect(decide('HEAD', '/secrets')).toEqual({ allow: false }); + }); + + test('every mutating verb is still refused', () => { + for (const verb of ['POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS']) { + expect(decide(verb, '/containers/abc123/restart')).toEqual({ allow: false }); + expect(decide(verb, '/containers/json')).toEqual({ allow: false }); + } + }); + + test('GET /info is allowed, versioned or not', () => { + expect(decide('GET', '/info')).toEqual({ allow: true, kind: 'info' }); + expect(decide('GET', '/v1.52/info')).toEqual({ allow: true, kind: 'info' }); + }); +}); + +describe('redactInfo', () => { + const raw = { + ID: 'ABCD:EFGH', + Name: 'hola-vm-102', + ServerVersion: '27.3.1', + OSType: 'linux', + Architecture: 'x86_64', + NCPU: 4, + MemTotal: 6221225472, + Containers: 9, + ContainersRunning: 8, + ContainersPaused: 0, + ContainersStopped: 1, + Images: 12, + // Everything below must not survive. + HttpProxy: 'http://user:hunter2@proxy.internal:3128', + HttpsProxy: 'https://user:hunter2@proxy.internal:3128', + RegistryConfig: { IndexConfigs: { 'docker.io': {} } }, + Labels: ['tier=prod'], + Plugins: { Volume: ['local'] }, + DockerRootDir: '/var/lib/docker', + SecurityOptions: ['name=apparmor'], + Swarm: { NodeID: 'xyz', LocalNodeState: 'active' }, + KernelVersion: '6.8.0-45-generic', + OperatingSystem: 'Ubuntu 24.04.1 LTS', + }; + + test('keeps what identifies and sizes the engine', () => { + const out = redactInfo(raw) as Record; + expect(out.Name).toBe('hola-vm-102'); + expect(out.ServerVersion).toBe('27.3.1'); + expect(out.OSType).toBe('linux'); + expect(out.NCPU).toBe(4); + expect(out.ContainersRunning).toBe(8); + // Kept so a client can tell Docker from Podman — Dozzle reads it for exactly + // that, and it says no more than OSType already does. + expect(out.OperatingSystem).toBe('Ubuntu 24.04.1 LTS'); + }); + + test('drops the proxy URLs, which routinely carry credentials', () => { + const out = JSON.stringify(redactInfo(raw)); + expect(out).not.toContain('hunter2'); + expect(out).not.toContain('HttpProxy'); + }); + + test('drops host configuration the grant has no business exposing', () => { + const out = redactInfo(raw) as Record; + for (const k of ['RegistryConfig', 'Labels', 'Plugins', 'DockerRootDir', 'SecurityOptions', 'Swarm', 'KernelVersion']) { + expect(out[k]).toBeUndefined(); + } + }); + + test('a non-object body passes through untouched', () => { + expect(redactInfo(null)).toBeNull(); + expect(redactInfo('nope')).toBe('nope'); + }); +}); + +describe('redactInspect keeps the response shape a real client expects', () => { + const raw = { + Id: 'abc123', + Name: '/hola-app-1', + Created: '2026-09-20T00:00:00Z', + State: { Status: 'running' }, + Image: 'sha256:deadbeef', + Config: { Tty: false, Labels: { 'sh.hola.app': 'calibre-web' }, Image: 'app:1', Hostname: 'h', Env: ['SECRET=hunter2'] }, + HostConfig: { PortBindings: { '8080/tcp': [{ HostPort: '8080' }] }, Binds: ['/etc/passwd:/x'], Privileged: true }, + Mounts: [{ Source: '/srv/hola/apps/x', Destination: '/data' }], + NetworkSettings: { Networks: { hola: { IPAddress: '172.18.0.5' } } }, + }; + + test('structural fields are present but empty — clients walk them without nil checks', () => { + // Dozzle segfaults on HostConfig.PortBindings when HostConfig is absent; + // anything on Docker's SDK assumes the same shape. Dropping the field denies + // the client, not the data. + const out = redactInspect(raw) as Record; + expect(out.HostConfig).toEqual({ PortBindings: {} }); + expect(out.Mounts).toEqual([]); + expect(out.NetworkSettings).toEqual({ Networks: {} }); + }); + + test('and they disclose nothing', () => { + const out = JSON.stringify(redactInspect(raw)); + expect(out).not.toContain('8080'); // no host port map + expect(out).not.toContain('/etc/passwd'); // no bind sources + expect(out).not.toContain('172.18.0.5'); // no network topology + expect(out).not.toContain('Privileged'); + expect(out).not.toContain('hunter2'); // env still gone + }); + + test('what a log collector needs still comes through', () => { + const out = redactInspect(raw) as Record; + expect(out.Id).toBe('abc123'); + expect(out.State).toEqual({ Status: 'running' }); + const cfg = out.Config as Record; + expect((cfg.Labels as Record)['sh.hola.app']).toBe('calibre-web'); + expect(cfg.Tty).toBe(false); + }); +}); diff --git a/packages/server/src/lib/docker-proxy.ts b/packages/server/src/lib/docker-proxy.ts index 31c5a7c5..1d7a6c3f 100644 --- a/packages/server/src/lib/docker-proxy.ts +++ b/packages/server/src/lib/docker-proxy.ts @@ -19,7 +19,7 @@ /** What the proxy does with a request it allows. */ export type ProxyDecision = - | { allow: true; kind: 'passthrough' | 'inspect' | 'stream' } + | { allow: true; kind: 'passthrough' | 'inspect' | 'info' | 'stream' } | { allow: false }; const VERSION_PREFIX_RE = /^\/v\d+(?:\.\d+)*(?=\/|$)/; @@ -30,16 +30,24 @@ const LOGS_RE = /^\/containers\/[^/]+\/logs$/; * Decide whether a request is permitted by the container-logs grant, and how * to handle it. An optional `/vN.NN` API-version prefix is stripped before * matching (accepted and forwarded unchanged) — Docker clients routinely pin - * one. Only `GET` is ever allowed: every other verb can mutate or destroy - * state, which the grant never permits. + * one. + * + * `GET` and `HEAD` are allowed; every other verb can mutate or destroy state, + * which the grant never permits. HEAD earns its place by being strictly less + * revealing than the GET of the same path — it returns headers and no body — + * and by being unavoidable: Docker's own client pings with `HEAD /_ping` + * before anything else, so a GET-only allowlist refuses every standard client + * on its first call and looks to the caller like no engine at all. */ export function decide(method: string, path: string): ProxyDecision { - if (method.toUpperCase() !== 'GET') return { allow: false }; + const verb = method.toUpperCase(); + if (verb !== 'GET' && verb !== 'HEAD') return { allow: false }; const withoutVersion = path.replace(VERSION_PREFIX_RE, '') || '/'; const pathname = withoutVersion.split('?')[0] ?? withoutVersion; if (pathname === '/_ping' || pathname === '/version') return { allow: true, kind: 'passthrough' }; + if (pathname === '/info') return { allow: true, kind: 'info' }; if (pathname === '/containers/json') return { allow: true, kind: 'passthrough' }; if (pathname === '/events') return { allow: true, kind: 'stream' }; if (INSPECT_RE.test(pathname)) return { allow: true, kind: 'inspect' }; @@ -55,6 +63,41 @@ export function decide(method: string, path: string): ProxyDecision { * carries secrets (env) or grants more than "read logs, know what exists" * (host config, mounts, network internals). */ +/** + * Rebuild `GET /info` from an allowlist, the same way inspect is. + * + * A client on Docker's official SDK calls this to decide an engine is really + * there — Dozzle reports "Could not connect to any Docker Engine" and exits + * without it — so refusing it outright means the grant cannot serve the + * clients it exists for. The raw response is far too generous though: it + * carries `RegistryConfig`, `Labels`, `Plugins`, `DockerRootDir`, + * `SecurityOptions`, Swarm membership and, worst of all, `HttpProxy` / + * `HttpsProxy`, which routinely embed credentials. + * + * What survives is what identifies the engine and sizes it: enough for a + * collector to label its connection and show a host, and nothing that + * describes how the host is configured or what it can reach. + */ +export function redactInfo(body: unknown): unknown { + if (!body || typeof body !== 'object') return body; + const b = body as Record; + return { + ID: b.ID, + Name: b.Name, + ServerVersion: b.ServerVersion, + OSType: b.OSType, + OperatingSystem: b.OperatingSystem, + Architecture: b.Architecture, + NCPU: b.NCPU, + MemTotal: b.MemTotal, + Containers: b.Containers, + ContainersRunning: b.ContainersRunning, + ContainersPaused: b.ContainersPaused, + ContainersStopped: b.ContainersStopped, + Images: b.Images, + }; +} + export function redactInspect(body: unknown): unknown { if (!body || typeof body !== 'object') return body; const b = body as Record; @@ -73,6 +116,16 @@ export function redactInspect(body: unknown): unknown { Image: config.Image, Hostname: config.Hostname, }, + // Present but empty, rather than absent. A real daemon always returns these, + // so a client walks them without checking — Dozzle segfaults on + // `HostConfig.PortBindings` when HostConfig is missing, and it is not alone: + // anything built on Docker's SDK assumes the shape. Dropping the field + // therefore doesn't deny the data, it denies the client. Empty containers + // keep the response shape-compatible while disclosing nothing: no host port + // map, no bind sources, no network topology. The grant is about logs. + HostConfig: { PortBindings: {} }, + Mounts: [], + NetworkSettings: { Networks: {} }, }; } @@ -136,6 +189,13 @@ export async function startDockerProxy(opts: { }); } + if (decision.kind === 'info') { + const raw = await upstream.json().catch(() => undefined); + return new Response(JSON.stringify(redactInfo(raw)), { + status: upstream.status, + headers: { 'content-type': 'application/json' }, + }); + } if (decision.kind === 'inspect') { let body: unknown; try {