diff --git a/TODO.md b/TODO.md index cadbb3df..fab26198 100644 --- a/TODO.md +++ b/TODO.md @@ -402,6 +402,30 @@ outside the example's new README: ## Waiting on the owner to run it +- [ ] **Rotate the Anthropic proxy secret on the production host.** The file modes are fixed in code + (`state.ts` writes 0600, `secureRuntimeSecrets()` sweeps every boot) and were repaired on the dev box + — but `data/sidecar/claude-state.json` was world-readable for the whole life of the install, so any + account that read it still holds a key to the owner's Claude Max subscription. **A chmod closes the + door; it does not change the lock.** + + On the production host — which is the one with six accounts and real members — this matters and the + dev box does not. + + ```bash + pm2 restart officer # runs the sweep; check the log for what it closed + # then rotate: + # stop officer-anthropic-proxy, clear "proxySecret" in data/sidecar/claude-state.json, start it + pm2 restart officer-anthropic-proxy officer-claude-code + ``` + + **Both sidecars, together.** `officer-claude-code` caches the secret in `process.env.ANTHROPIC_API_KEY` + (`ensureAnthropicEnv` returns early once it is set), so rotating the proxy alone makes every chat turn + 401 until the agent sidecar is also restarted. + + Worth checking while there: `ls -la $OFFICER_ROOT/data/sidecar/` on production, and whether any member + account has a shell history or a `~/.claude/settings.json` naming `ANTHROPIC_BASE_URL` — that would be + evidence someone actually used it, rather than merely could have. + - [ ] **Plan mode is built and has never executed.** Landed `1b811dfa` on 2026-08-17, typechecks, unit tested at the translation layer — and not one line of it has run on a machine. The owner was away when it was finished and asked to be reminded, which is what this entry is. diff --git a/docs/per-user-linux-accounts.md b/docs/per-user-linux-accounts.md index 98382367..3f8c7fa1 100644 --- a/docs/per-user-linux-accounts.md +++ b/docs/per-user-linux-accounts.md @@ -81,6 +81,47 @@ prerequisite that gets skipped. The same applies to `permissions/` (775 today) and to the repo checkout itself: a member can read the platform source. That is acceptable — it is not secret — but anything credential-shaped inside it is not. +### The one this list missed: the Anthropic proxy secret + +Found 2026-08-17, while verifying the claim that members do not share the owner's Claude Max subscription. +**They do not** — `spawnClaudeAsMember` runs their `claude` through `setpriv --reset-env` with a six-name +allowlist, so neither `ANTHROPIC_BASE_URL` nor `ANTHROPIC_API_KEY` can cross into it and their turns go +straight to Anthropic on their own OAuth. That part holds up. + +But the isolation could be walked around: + +| path | mode | consequence | +| -------------------------------- | ------- | ------------------------------------- | +| `data/sidecar/` | 775 | listable by anyone | +| `data/sidecar/claude-state.json` | **664** | **world-readable — holds the secret** | + +`officer-anthropic-proxy` listens on `127.0.0.1`, accepts that string as `x-api-key`, swaps it for the +owner's Max OAuth bearer and forwards to `api.anthropic.com`. Every local account can reach loopback, so +the file IS the subscription. Confirmed by reading it as `nobody` — an account with no relationship to the +platform at all — and by observing the proxy 401 without the key, so the secret is the only control. + +Nothing had ever set a mode: `mkdirSync`/`Bun.write` at Ubuntu's default umask of 002. The same umask +reasoning was already written down two sections above for `.env` and in `os-user.ts` for the owner's home; +this file was simply on nobody's list. + +Fixed in two places, deliberately: + +- **`state.ts` sets 0600/0700 on every write** — the authoritative fix, at the point that creates the file. + On every write rather than at creation, because `writeFileSync` leaves an existing file's mode alone. +- **`secureRuntimeSecrets()` sweeps at every boot** (`server.tsx`) — because the proxy is a separate PM2 + process that can recreate its file long after Officer started, and because every existing install has the + old mode until something repairs it. + +It **repairs** rather than refusing to boot, which is the opposite of `assertSecretsClosed` one section +above. The line is who created the file: `.env` is the installer's and Officer must not guess at it, while +this is Officer's own output at the machine's umask — a wrong mode there is Officer's bug, and bricking the +platform over it would punish the user for our mistake. + +**A chmod does not undo exposure.** Any account that read the secret while it was open still holds it, and +it does not expire. Rotating it means clearing `proxySecret` and restarting `officer-anthropic-proxy` — +and then `officer-claude-code`, which caches the value in `process.env.ANTHROPIC_API_KEY` and will 401 on +every turn until it re-reads. Restart both or neither. + ## The mechanism, and the trap in it ### `Bun.spawn` silently ignores `uid` and `gid` diff --git a/src/server.tsx b/src/server.tsx index 730f3aab..e5455ab7 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -8,7 +8,7 @@ import { refreshPluginMounts, snapshotPlugins } from './servers/plugins/mount'; import { generatePluginsModule, rebuildFrontend, BUILD_DIR, SHELL_FILE } from './servers/plugins/generate'; import { assertInstallLayout } from './servers/data-path'; import { PORT } from './servers/officer-url.mjs'; -import { assertSecretsClosed } from './servers/os-user'; +import { assertSecretsClosed, secureRuntimeSecrets } from './servers/os-user'; import { resolveHomeDir } from './servers/user-home'; import { resolveAuthToken } from './servers/auth-token'; import { isWsProviderAllowed } from './servers/permissions/authorize'; @@ -167,6 +167,20 @@ assertInstallLayout(); // JWT_SECRET is worse than no isolation, because the model looks intact. await assertSecretsClosed(process.cwd()); +// Then close the ones the platform writes ITSELF, which is a repair rather than a refusal — see +// `secureRuntimeSecrets`. It runs on every boot because `officer-anthropic-proxy` is a separate PM2 +// process that can recreate its state file at the machine's umask hours after this one started, and +// because the file it protects is the owner's Claude Max subscription in string form. +const secured = await secureRuntimeSecrets(); +if (secured.repaired.length) { + console.warn( + `[boot] closed ${secured.repaired.length} world-readable platform secret(s): ${secured.repaired.join(', ')}\n` + + ' If this names sidecar/claude-state.json, the proxy secret was readable by every account on\n' + + ' this machine and should be ROTATED, not just closed — the mode is fixed, the string is not.', + ); +} +for (const path of secured.failed) console.error(`[boot] could not close ${path} — check its owner and mode`); + // Mount whatever is installed, BEFORE serve(). // // Without this a restart silently unmounts every plugin: `honoServer` is built with none at import, diff --git a/src/servers/os-user.test.ts b/src/servers/os-user.test.ts index 4a6f7329..21893172 100644 --- a/src/servers/os-user.test.ts +++ b/src/servers/os-user.test.ts @@ -1,8 +1,8 @@ import { describe, expect, test } from 'bun:test'; -import { chmod, mkdtemp, writeFile } from 'node:fs/promises'; +import { chmod, mkdir, mkdtemp, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { findReadableSecrets, osUserNameFor, runAsArgv } from './os-user'; +import { findReadableSecrets, osUserNameFor, runAsArgv, secureRuntimeSecrets } from './os-user'; // The tests that matter here are the two that prove the MECHANISM rather than the plumbing: that Bun // ignores `uid`, and that `setpriv` does not. Everything else in os-user.ts touches the passwd database @@ -167,3 +167,79 @@ describe('findReadableSecrets', () => { expect(await findReadableSecrets(dir)).toEqual([]); }); }); + +/* + The proxy secret IS the owner's Claude Max subscription. + + `officer-anthropic-proxy` swaps a matching `x-api-key` for the owner's OAuth bearer and forwards to + Anthropic, on a loopback port every local account can reach. So these assertions are not about tidy file + modes — they are the difference between a member's turns being billed to their own account (which + `spawn-as-member.ts` goes to considerable lengths to ensure) and a member being able to help themselves + to the owner's. + + It shipped world-readable: default umask 002, no mode ever set, verified by reading the live secret as + `nobody` on 2026-08-17. +*/ +describe('secureRuntimeSecrets', () => { + async function fixture(mode: number) { + const root = await mkdtemp(join(tmpdir(), 'officer-data-')); + const dir = join(root, 'sidecar'); + await mkdir(dir, { recursive: true }); + const file = join(dir, 'claude-state.json'); + await writeFile(file, JSON.stringify({ proxySecret: 'sk-ant-api03-test', claudeSessions: {} })); + await chmod(file, mode); + await chmod(dir, 0o755); + return { root, dir, file }; + } + + test('closes a world-readable proxy secret and its directory', async () => { + const { root, dir, file } = await fixture(0o664); + + const result = await secureRuntimeSecrets(root); + + expect(result.failed).toEqual([]); + expect(result.repaired).toContain(file); + expect(result.repaired).toContain(dir); + // The mode is the assertion, not the return value: a function that reports success while leaving the + // file readable is the exact failure this exists to prevent. + expect((await stat(file)).mode & 0o777).toBe(0o600); + expect((await stat(dir)).mode & 0o777).toBe(0o700); + }); + + test('group-readable counts too — a member’s supplementary groups are not ours to predict', async () => { + const { root, file } = await fixture(0o640); + const result = await secureRuntimeSecrets(root); + expect(result.repaired).toContain(file); + expect((await stat(file)).mode & 0o777).toBe(0o600); + }); + + test('is idempotent: a second boot repairs nothing and reports nothing', async () => { + const { root } = await fixture(0o664); + await secureRuntimeSecrets(root); + + // Not merely "no error" — an empty list, so the log on every subsequent restart stays silent and the + // warning keeps meaning something when it does appear. + expect(await secureRuntimeSecrets(root)).toEqual({ repaired: [], failed: [] }); + }); + + test('covers the per-account copies, not just the proxy’s own', async () => { + const { root } = await fixture(0o600); + const memberDir = join(root, 'someone@example.com', 'sidecar'); + await mkdir(memberDir, { recursive: true }); + const memberFile = join(memberDir, 'claude-state.json'); + await writeFile(memberFile, '{}'); + await chmod(memberFile, 0o664); + + const result = await secureRuntimeSecrets(root); + + // These hold no live secret today, but they map session keys to transcript uuids for a NAMED account, + // which is not something one member should be able to enumerate about another. + expect(result.repaired).toContain(memberFile); + expect((await stat(memberFile)).mode & 0o777).toBe(0o600); + }); + + test('an install with nothing written yet is not an error', async () => { + const root = await mkdtemp(join(tmpdir(), 'officer-data-')); + expect(await secureRuntimeSecrets(root)).toEqual({ repaired: [], failed: [] }); + }); +}); diff --git a/src/servers/os-user.ts b/src/servers/os-user.ts index d58b8d62..c4b74805 100644 --- a/src/servers/os-user.ts +++ b/src/servers/os-user.ts @@ -596,6 +596,80 @@ export async function findReadableSecrets(projectDir: string): Promise return bad; } +/** + * Close the secrets the PLATFORM ITSELF writes, on every boot. + * + * ── Why this repairs where `assertSecretsClosed` refuses ── + * + * The two functions cover the same hazard and take opposite actions, and the line between them is who + * created the file. `.env` and the secret store are the INSTALLER's: Officer cannot know what mode they + * were meant to have or whether it may touch them, so it names them and refuses to start. Everything here + * is Officer's own output, written by its own sidecars at whatever umask the machine happens to have — so + * a wrong mode is Officer's bug, and refusing to boot over it would be punishing the user for our mistake. + * Fixing it silently and saying so in the log is the honest response. + * + * ── What this is protecting ── + * + * `sidecar/claude-state.json` holds `proxySecret`, which `officer-anthropic-proxy` accepts as `x-api-key` + * and exchanges for the owner's Claude Max OAuth bearer. The proxy is on 127.0.0.1, which every local + * account can reach, so that string IS the owner's subscription. Found world-readable on 2026-08-17 and + * confirmed by reading it as `nobody` — every per-member protection in `spawn-as-member.ts` was intact and + * simply walked around. + * + * The per-user copies (`/sidecar/`) carry no live secret today but map session keys to transcript + * uuids for named accounts, which is not something one member should be able to enumerate about another. + * They are covered by the same sweep rather than a second one, because "platform state, service user only" + * is one rule. + * + * ── Why it repeats on every boot ── + * + * `state.ts` now sets these modes at every write, which is the authoritative fix. This exists anyway for + * two reasons: the proxy is a separate PM2 process that can recreate its file long after Officer booted, + * and every install that already exists has the old mode right now. A sweep is cheap and idempotent; the + * alternative is a fix that only reaches machines installed after it. + * + * Never throws — a chmod that fails is reported and the platform starts. The one thing worse than a + * world-readable secret is a platform that will not boot and cannot be used to fix it. + */ +export async function secureRuntimeSecrets(dataPath: string = DATA_PATH): Promise<{ + repaired: string[]; + failed: string[]; +}> { + const repaired: string[] = []; + const failed: string[] = []; + + // Every `sidecar/` under DATA_PATH: the proxy's own, plus one per account that has run a turn. + const dirs = [join(dataPath, 'sidecar')]; + try { + for (const entry of await readdir(dataPath, { withFileTypes: true })) { + if (entry.isDirectory() && entry.name.includes('@')) dirs.push(join(dataPath, entry.name, 'sidecar')); + } + } catch { + // No DATA_PATH yet — a first boot. Nothing written means nothing exposed. + } + + const targets: { path: string; mode: number }[] = []; + for (const dir of dirs) { + targets.push({ path: dir, mode: 0o700 }); + targets.push({ path: join(dir, 'claude-state.json'), mode: 0o600 }); + } + + for (const { path, mode } of targets) { + if (!existsSync(path)) continue; + try { + const info = await stat(path); + // Only touch what is actually open, so a repeat boot logs nothing and the list stays meaningful. + if (!(info.mode & 0o077)) continue; + await chmod(path, mode); + repaired.push(path); + } catch { + failed.push(path); + } + } + + return { repaired, failed }; +} + /** * Refuse to boot while a secret in the project tree is readable by other accounts on this machine. * diff --git a/src/servers/sidecar/claude/state.ts b/src/servers/sidecar/claude/state.ts index d9952673..9c7b9856 100644 --- a/src/servers/sidecar/claude/state.ts +++ b/src/servers/sidecar/claude/state.ts @@ -1,5 +1,5 @@ import { join } from 'node:path'; -import { mkdirSync, existsSync, readFileSync, writeFileSync, unlinkSync } from 'node:fs'; +import { mkdirSync, existsSync, readFileSync, writeFileSync, unlinkSync, chmodSync } from 'node:fs'; import { DATA_PATH } from '../../data-path'; /** @@ -47,9 +47,52 @@ export function initPaths(email?: string): void { // Default to proxy paths initPaths(); +/* + This file holds `proxySecret`, and that secret is the ONLY thing standing between a local account and the + owner's Claude Max subscription. + + `officer-anthropic-proxy` listens on 127.0.0.1, swaps an `x-api-key` matching this value for the owner's + OAuth bearer, and forwards to api.anthropic.com. Any account on the box can reach a loopback port, so + whoever can read this file can spend the owner's subscription — deliberately, and with no trace tying it + to them. + + The platform's whole per-member design is built so this CANNOT happen by accident: `spawnClaudeAsMember` + runs a member's `claude` through `setpriv --reset-env` with a six-name allowlist, so `ANTHROPIC_BASE_URL` + and `ANTHROPIC_API_KEY` cannot cross into it and their turns go straight to Anthropic on their own OAuth. + None of that helps if the key is world-readable on disk — the isolation is intact and simply walked + around. + + It WAS world-readable. Ubuntu's default umask is 002, `mkdirSync`/`writeFileSync` take it, and nothing + here ever set a mode: verified 2026-08-17 by reading the live secret as `nobody`, an account with no + relationship to the platform at all. The same umask reasoning is already written down for the owner's + home in `os-user.ts` — this file was simply not on anyone's list. + + 0600 on the file and 0700 on the directory. Applied on every write and not only at creation, because + `writeFileSync` leaves an EXISTING file's mode alone, so creating it correctly would have fixed nothing + on the installs that already have one. +*/ +const SECRET_FILE_MODE = 0o600; +const SECRET_DIR_MODE = 0o700; + function ensureDir() { if (!existsSync(stateDir)) { - mkdirSync(stateDir, { recursive: true }); + mkdirSync(stateDir, { recursive: true, mode: SECRET_DIR_MODE }); + } + // Unconditional, for the directories that already exist at 0775. + try { + chmodSync(stateDir, SECRET_DIR_MODE); + } catch { + // Not ours to chmod. The boot sweep reports it; failing here would take the sidecar down over a + // permission bit on a directory it may not own. + } +} + +/** Clamp the state file to 0600. Safe to call when it does not exist yet. */ +function secureStateFile(): void { + try { + if (existsSync(stateFile)) chmodSync(stateFile, SECRET_FILE_MODE); + } catch { + // Same reasoning as ensureDir. } } @@ -99,6 +142,9 @@ function migrateSessions(raw: Record, ownerUserId?: number): Re export async function saveState(): Promise { ensureDir(); await Bun.write(stateFile, JSON.stringify(currentState, null, 2)); + // After the write, never before: `Bun.write` creates at the process umask, and this is the only moment + // the file is guaranteed to exist. + secureStateFile(); } export function getState(): PersistedState {