diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 2d9d1bd6..12e2b97e 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -8,6 +8,7 @@ * agree with any of those without them being true. */ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { existsSync } from "node:fs"; import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; @@ -15,6 +16,7 @@ import * as path from "node:path"; import { acquireLock, FileSecretStore, + readSecretFilePermissions, SecretFileLockTimeoutError, SecretFileKeyMismatchError, tightenSecretFilePermissions, @@ -450,6 +452,21 @@ describe("tightenSecretFilePermissions", () => { expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600); }); + it("readSecretFilePermissions reports the mode without changing it", async () => { + // The descriptor is rebuilt on every GET /api/config, so the function it + // calls must not chmod — once per page load, on a file another process + // may be mid-write on. The repair belongs at startup. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + await fs.chmod(filePath(), 0o644); + expect(await readSecretFilePermissions(filePath())).toEqual({ + state: "loose", + mode: 0o644, + }); + // Still 0644 — reading did not quietly repair it. + expect((await fs.stat(filePath())).mode & 0o777).toBe(0o644); + }); + it("reports absent, not a failure, for a file that doesn't exist yet", async () => { // The first-run case. Nothing to tighten and nothing to warn about — // `writeStoreFile` creates the file 0600. @@ -684,16 +701,21 @@ describe("acquireLock", () => { // unlocked and overwrite each other. First writes are exactly when two // processes are most likely to start together. const target = path.join(tmpDir, "no-such-dir", "secrets.json"); - const release = await acquireLock(target); + const release = await acquireLock(target, { pollMs: 10 }); // A real lock was taken: a second acquisition must not be handed out. let secondTaken = false; - void acquireLock(target).then((r) => { + // Held, not floated: an acquisition still running when the test ends + // would race `afterEach` removing the temp directory. + const second = acquireLock(target, { pollMs: 10 }).then((r) => { secondTaken = true; - return r(); + return r; }); await new Promise((r) => setTimeout(r, 60)); expect(secondTaken).toBe(false); await release(); + await ( + await second + )(); }); it("releases only a lock it still owns", async () => { @@ -754,25 +776,148 @@ describe("acquireLock", () => { }); try { let stolen = false; - void acquireLock(target, { + // Held and settled below rather than floated, so the waiter cannot + // still be running when `afterEach` removes the temp directory. + const waiter = acquireLock(target, { staleMs: 120, timeoutMs: 400, pollMs: 10, }) - .then((r) => { + .then(async (r) => { stolen = true; - return r(); + await r(); }) .catch(() => { // Timed out, which is the expected outcome. }); - await new Promise((r) => setTimeout(r, 500)); + await waiter; expect(stolen).toBe(false); } finally { await release(); } }); + it("does not steal a stale lock whose owner process is still alive", async () => { + // An old mtime means the holder has not heartbeated lately, and there are + // two very different reasons: it died, or it is alive but not running — + // SIGSTOPed, event-loop-blocked, or on a machine that suspended. Taking + // the lock on elapsed time alone would pull it out from under a live + // critical section, which is the lost update the lock exists to prevent. + const target = path.join(tmpDir, "suspended.json"); + const lockDir = `${target}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + // Stamp it with *this* process, which is definitionally alive, and age it + // well past stale — the shape of a suspended holder. + await fs.writeFile( + path.join(lockDir, "owner"), + JSON.stringify({ + token: "someone", + pid: process.pid, + host: os.hostname(), + }), + "utf-8", + ); + const old = new Date(Date.now() - 60_000); + await fs.utimes(lockDir, old, old); + + await expect( + acquireLock(target, { staleMs: 50, timeoutMs: 150, pollMs: 10 }), + ).rejects.toBeInstanceOf(SecretFileLockTimeoutError); + // Their lock survives the attempt. + expect(existsSync(lockDir)).toBe(true); + await fs.rm(lockDir, { recursive: true, force: true }); + }); + + it("steals a stale lock whose owner process is gone", async () => { + // The case the stealing exists for. A pid that no longer resolves is a + // dead holder, and refusing to break its lock would turn one killed + // container into permanently broken secret writes. + const target = path.join(tmpDir, "dead.json"); + const lockDir = `${target}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + await fs.writeFile( + path.join(lockDir, "owner"), + // A pid that cannot be running: `kill(0)` answers ESRCH. + JSON.stringify({ token: "gone", pid: 2 ** 31 - 1, host: os.hostname() }), + "utf-8", + ); + const old = new Date(Date.now() - 60_000); + await fs.utimes(lockDir, old, old); + + const release = await acquireLock(target, { + staleMs: 50, + timeoutMs: 2_000, + pollMs: 10, + }); + await release(); + }); + + it("steals a stale lock stamped by another machine", async () => { + // A shared filesystem: we cannot ask this host about that host's pid, so + // the mtime is all there is. Refusing would make a lock nothing can ever + // break. + const target = path.join(tmpDir, "remote.json"); + const lockDir = `${target}.lock`; + await fs.mkdir(lockDir, { recursive: true }); + await fs.writeFile( + path.join(lockDir, "owner"), + JSON.stringify({ + token: "elsewhere", + pid: process.pid, + host: "some-other-host", + }), + "utf-8", + ); + const old = new Date(Date.now() - 60_000); + await fs.utimes(lockDir, old, old); + + const release = await acquireLock(target, { + staleMs: 50, + timeoutMs: 2_000, + pollMs: 10, + }); + await release(); + }); + + it("cleans up and reports when it cannot stamp ownership", async () => { + // mkdir succeeded, the owner write did not. Returning a no-op release + // here would enter the critical section unlocked *and* leave the + // directory behind, so every later writer queues behind an orphan nobody + // can deliberately release. + const target = path.join(tmpDir, "unstampable.json"); + vi.resetModules(); + vi.doMock("node:fs/promises", async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + return { + ...actual, + default: actual, + writeFile: vi.fn((p: string, ...rest: unknown[]) => + String(p).endsWith(`${path.sep}owner`) + ? Promise.reject(new Error("ENOSPC: no space left on device")) + : (actual.writeFile as (...a: unknown[]) => Promise)( + p, + ...rest, + ), + ), + }; + }); + try { + const mod = + await import("@inspector/core/auth/node/file-secret-store.js"); + await expect(mod.acquireLock(target)).rejects.toThrow( + /Could not initialize the lock/, + ); + // And nothing is left obstructing the next writer. + expect(existsSync(`${target}.lock`)).toBe(false); + } finally { + vi.doUnmock("node:fs/promises"); + vi.resetModules(); + } + }); + it("serializes concurrent writers through two independent store instances", async () => { // Two `FileSecretStore` objects have separate in-process queues, so this // is the cross-process race in miniature: without the file lock the second diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 0a5be897..d9d9616e 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -135,6 +135,7 @@ describe("isContainer", () => { */ async function loadWithFs(opts: { dockerenv?: boolean; + containerenv?: boolean; cgroup?: string | Error; }) { vi.resetModules(); @@ -142,8 +143,11 @@ describe("isContainer", () => { const actual = await vi.importActual("node:fs"); return { ...actual, - existsSync: (p: string) => - p === "/.dockerenv" ? !!opts.dockerenv : actual.existsSync(p), + existsSync: (p: string) => { + if (p === "/.dockerenv") return !!opts.dockerenv; + if (p === "/run/.containerenv") return !!opts.containerenv; + return actual.existsSync(p); + }, readFileSync: (p: string, enc?: unknown) => { if (p === "/proc/1/cgroup") { if (opts.cgroup instanceof Error) throw opts.cgroup; @@ -173,6 +177,23 @@ describe("isContainer", () => { expect(mod.isContainer()).toBe(true); }); + it("is true when Podman's /run/.containerenv exists", async () => { + // Rootless Podman has no `/.dockerenv`, and its cgroup line often does + // not contain the literal "podman" either — this file is the signal that + // is reliably there. Missing it classified the container as a host, so a + // keychain-less box wrote a "durable" file into the ephemeral layer. + const mod = await loadWithFs({ containerenv: true }); + expect(mod.isContainer()).toBe(true); + }); + + it("is true for a libpod cgroup path", async () => { + // What Podman actually writes: `/machine.slice/libpod-.scope`. + const mod = await loadWithFs({ + cgroup: "0::/machine.slice/libpod-3f2a.scope\n", + }); + expect(mod.isContainer()).toBe(true); + }); + it("is true when a container runtime appears in /proc/1/cgroup", async () => { const mod = await loadWithFs({ cgroup: "0::/system.slice/containerd.service\n", diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 29589d36..1c0b169f 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -60,6 +60,7 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; +import * as os from "node:os"; import * as path from "node:path"; import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; import { @@ -300,6 +301,10 @@ export async function acquireLock( const lockDir = `${filePath}.lock`; const ownerFile = `${lockDir}/owner`; const token = crypto.randomBytes(16).toString("hex"); + // The stamp a would-be thief consults before taking this lock: the token + // proves ownership on release, the pid and host let a waiter ask the OS + // whether we are still alive rather than inferring it from elapsed time. + const owner: LockOwner = { token, pid: process.pid, host: os.hostname() }; const noop = async (): Promise => {}; const deadline = Date.now() + timeoutMs; @@ -317,9 +322,16 @@ export async function acquireLock( } for (;;) { + let created = false; try { await fs.mkdir(lockDir, { recursive: false }); - await fs.writeFile(ownerFile, token, "utf-8"); + created = true; + // Stamp ownership before entering the section. If this fails the lock + // exists but says nothing about who holds it, so nobody could ever + // release it deliberately and every later writer would queue behind an + // orphan until it aged out. Clean up and report, rather than proceeding + // unlocked *and* leaving the obstruction behind. + await fs.writeFile(ownerFile, JSON.stringify(owner), "utf-8"); // Prove liveness for as long as we hold it. `unref` so a held lock // never keeps the process alive on its own — the interval is a // side-channel, not work anyone is waiting for. @@ -336,8 +348,8 @@ export async function acquireLock( // Release only what we still own. A lock judged stale and stolen // now belongs to someone else, and removing it would drop them // into a race with whoever comes next. - const held = await fs.readFile(ownerFile, "utf-8"); - if (held !== token) return; + const held = parseOwner(await fs.readFile(ownerFile, "utf-8")); + if (held?.token !== token) return; await fs.rm(lockDir, { recursive: true, force: true }); } catch { // Already gone, or unreadable. Either way there is nothing to @@ -346,15 +358,17 @@ export async function acquireLock( }; } catch (err) { const code = (err as NodeJS.ErrnoException).code; + if (created) { + // We made the directory and then failed to stamp it. Ours to remove. + await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {}); + throw new SecretStoreUnavailableError( + `Could not initialize the lock for the secrets file at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } if (code !== "EEXIST") return noop; // cannot lock here at all — see above - try { - const held = (await fs.stat(lockDir)).mtimeMs; - if (Date.now() - held > staleMs) { - await fs.rm(lockDir, { recursive: true, force: true }); - continue; - } - } catch { - // The holder released it between our mkdir and our stat. Retry. + if (await canSteal(lockDir, ownerFile, staleMs)) { + await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {}); + continue; } if (Date.now() >= deadline) throw new SecretFileLockTimeoutError(filePath); @@ -363,6 +377,89 @@ export async function acquireLock( } } +/** + * May this waiter take a lock it found already held? + * + * Elapsed wall time alone does not answer that. An old mtime means the + * holder has not run its heartbeat lately, and there are two very different + * reasons for that: it died, or it is *alive but not running* — SIGSTOPed, + * starved by a blocked event loop, or on a machine that suspended. Expiring + * on time alone treats those identically and takes the lock out from under a + * live critical section, which is the lost update the lock exists to + * prevent, now arrived at through the machinery meant to prevent it. + * + * So the mtime only opens the question, and the owner stamp answers it: on + * the same host, ask the operating system whether that pid is still there. + * `kill(pid, 0)` sends no signal — it is the standard existence probe. + * `EPERM` counts as alive (the process exists; it just isn't ours), so a + * holder running as another user is never stolen from. + * + * The stamp is not always usable — a lock written by a different machine + * over a shared filesystem, or one whose owner file is missing or corrupt. + * There the mtime is all there is, and stealing on it is still right: the + * alternative is a lock nothing can ever break, which turns one dead process + * into a permanent outage. + */ +async function canSteal( + lockDir: string, + ownerFile: string, + staleMs: number, +): Promise { + let held: number; + try { + held = (await fs.stat(lockDir)).mtimeMs; + } catch { + // Released between our mkdir and our stat — retry rather than steal. + return false; + } + if (Date.now() - held <= staleMs) return false; + + let owner: LockOwner | null = null; + try { + owner = parseOwner(await fs.readFile(ownerFile, "utf-8")); + } catch { + // No stamp to consult; fall through to the time-only answer. + } + if (!owner || owner.host !== os.hostname() || typeof owner.pid !== "number") { + return true; + } + return !processIsAlive(owner.pid); +} + +/** Does this pid exist? `kill(pid, 0)` is the probe; it sends no signal. */ +function processIsAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + // EPERM means it exists and belongs to someone else — still alive. + return (err as NodeJS.ErrnoException).code === "EPERM"; + } +} + +interface LockOwner { + token: string; + pid: number; + host: string; +} + +/** Parse an owner stamp, tolerating anything that isn't one. */ +function parseOwner(raw: string): LockOwner | null { + try { + const parsed: unknown = JSON.parse(raw); + if (typeof parsed !== "object" || parsed === null) return null; + const { token, pid, host } = parsed as Partial; + if (typeof token !== "string") return null; + return { + token, + pid: typeof pid === "number" ? pid : -1, + host: String(host), + }; + } catch { + return null; + } +} + export interface FileSecretStoreOptions { /** Absolute path of the secrets file. */ filePath: string; @@ -696,9 +793,24 @@ export async function tightenSecretFilePermissions( } // Verify rather than assume. The chmod above is best-effort, but the UI and // the README both state the file is 0600 — so a file we failed to tighten - // (owned by another user, on a read-only mount) must be *reported*, not - // quietly described as protected. This is the one place that can tell the - // difference, because it is the only one that looks at the file. + // (owned by another user, on a read-only mount) must be reported, not + // quietly described as protected. + return readSecretFilePermissions(filePath); +} + +/** + * Read the file's permission state **without touching it**. + * + * The read-only half of {@link tightenSecretFilePermissions}, split out + * because the descriptor is rebuilt on every `GET /api/config` and a + * function whose job is to *describe* must not chmod as a side effect — + * once per page load, on a file another process may be mid-write on. The + * repair belongs at startup, where it happens once and its outcome is + * announced; from then on the question is only "what is the mode now". + */ +export async function readSecretFilePermissions( + filePath: string, +): Promise { try { const mode = (await fs.stat(filePath)).mode & 0o777; if (mode === 0o600) return { state: "ok" }; @@ -711,7 +823,8 @@ export async function tightenSecretFilePermissions( } /** - * Outcome of {@link tightenSecretFilePermissions}: the file is `0600` + * Outcome of {@link tightenSecretFilePermissions} and + * {@link readSecretFilePermissions}: the file is `0600` * (`ok`), does not exist yet (`absent`), or exists with a wider mode we * could not fix (`loose`) — the only case anyone needs to hear about. */ diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 637b7148..c4fba79d 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -49,6 +49,7 @@ import { } from "../secret-storage-info.js"; import { FileSecretStore, + readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; import { @@ -132,13 +133,26 @@ export function defaultSecretFilePath(): string { export function isContainer(): boolean { if (process.env.KUBERNETES_SERVICE_HOST) return true; try { - if (fsSync.existsSync("/.dockerenv")) return true; + // `/.dockerenv` is Docker's marker; `/run/.containerenv` is Podman's + // equivalent and is the only one a rootless Podman container reliably + // has. Missing it classified such a container as a host, which is the + // wrong way round to be wrong: the fallback then writes a file into the + // ephemeral writable layer and calls it durable. + if ( + fsSync.existsSync("/.dockerenv") || + fsSync.existsSync("/run/.containerenv") + ) { + return true; + } } catch { // A sandboxed / restricted filesystem. Fall through to the cgroup read. } try { const cgroup = fsSync.readFileSync("/proc/1/cgroup", "utf-8"); - return /\b(docker|containerd|kubepods|podman|lxc)\b/.test(cgroup); + // `libpod` is what Podman actually writes into cgroup paths + // (`/machine.slice/libpod-.scope`); the literal string "podman" + // often does not appear at all. + return /\b(docker|containerd|kubepods|podman|libpod|lxc)\b/.test(cgroup); } catch { // Not Linux, or /proc is not mounted. Not a container as far as we know. return false; @@ -300,8 +314,11 @@ async function describeFileStore( const plaintext = onDisk === null ? !store.encrypted : onDisk === "none"; // Re-checked here rather than captured at selection, for the same reason // the encryption state is: the mode can change under a running process, - // and this descriptor is what the browser renders as fact. - const perms = await tightenSecretFilePermissions(filePath); + // and this descriptor is what the browser renders as fact. The *read-only* + // half deliberately — describing must not chmod, least of all once per + // page load on a file another process may be mid-write on. The repair + // happens once, at selection. + const perms = await readSecretFilePermissions(filePath); return { kind, reason,