diff --git a/src/databases/officer_db/src/api-keys/queries.ts b/src/databases/officer_db/src/api-keys/queries.ts index 49de7dd7..22c15cd7 100644 --- a/src/databases/officer_db/src/api-keys/queries.ts +++ b/src/databases/officer_db/src/api-keys/queries.ts @@ -14,8 +14,10 @@ export type ApiKeyIdentity = { email: string; username: string | null; lastUsedAt: Date | null; - /** Permission keys this credential may reach, or null for the holder's full authority. */ + /** Permission keys this credential may reach, or null for "anything not excluded". */ permissions: string[] | null; + /** Permission kinds it may never reach, resolved against the live registry. */ + excludedKinds: string[] | null; }; /** @@ -34,6 +36,7 @@ export async function findLiveApiKeyByHash(keyHash: string): Promise { @@ -68,6 +73,7 @@ export async function createApiKey(params: CreateApiKeyParams): Promise p.kind !== 'admin').map((p) => p.key); -} +/** What a commander gets unless told otherwise: everything the platform can do, except administering it. */ +export const COMMANDER_EXCLUDED_KINDS = ['admin']; /** The env file's path, and the name the key is stored under — both derived, so revocation cannot miss one. */ export const commanderKeyName = (dirName: string) => `commander:${dirName}`; @@ -60,9 +66,9 @@ export async function writeCommanderCredential(params: { userId: number; email: string; dirName: string; - permissions?: string[]; -}): Promise<{ path: string; permissions: string[] }> { - const permissions = params.permissions ?? defaultCommanderPermissions(); + /** An explicit allow list, to tighten one commander further. Absent leaves it at the default rule. */ + permissions?: string[] | null; +}): Promise<{ path: string; excludedKinds: string[] }> { const { secret, keyHash, prefix } = mintApiKey(); // Revoke first. A commander recreated under a directory name that has been used before would otherwise @@ -73,7 +79,8 @@ export async function writeCommanderCredential(params: { name: commanderKeyName(params.dirName), keyHash, prefix, - permissions, + permissions: params.permissions ?? null, + excludedKinds: COMMANDER_EXCLUDED_KINDS, }); const path = commanderEnvPath(params.email, params.dirName); @@ -94,5 +101,5 @@ export async function writeCommanderCredential(params: { { mode: 0o600 }, ); - return { path, permissions }; + return { path, excludedKinds: COMMANDER_EXCLUDED_KINDS }; } diff --git a/src/servers/api/command-hq/directory.ts b/src/servers/api/command-hq/directory.ts index c8731538..0d88de75 100644 --- a/src/servers/api/command-hq/directory.ts +++ b/src/servers/api/command-hq/directory.ts @@ -88,9 +88,13 @@ export function commanderClaudeMd(params: CommanderClaudeMdParams): string { '`POST /api/tasks//run` runs one and returns what it did — one reviewed definition on', 'disk instead of a re-implementation living in a loop nobody reads.', '', - 'The credential is narrowed and revoked when this commander is deleted. A 403 is an answer, not a', - 'problem to route around: it means this commander was not given that, and the owner is the one who', - 'decides otherwise.', + 'It reaches everything the platform does EXCEPT administering it — installing plugins, managing', + 'users or roles, changing server settings. That holds for features added later too: the rule is', + 'resolved per request rather than frozen into a list, so a plugin installed next year is already', + 'covered without this commander being re-issued.', + '', + 'A 403 is an answer, not a problem to route around. It means this commander was not given that,', + 'and the owner is the one who decides otherwise. Say so rather than finding another way in.', '', ] : [ diff --git a/src/servers/api/tasks/script-run.ts b/src/servers/api/tasks/script-run.ts index d971772c..54cd94c0 100644 --- a/src/servers/api/tasks/script-run.ts +++ b/src/servers/api/tasks/script-run.ts @@ -4,6 +4,7 @@ import { tmpdir } from 'node:os'; import { getTaskByDirName, type TaskRecord } from './task-files'; import { getOwnerHomeDir } from '../../data-path'; import { buildTaskApiEnv } from './task-api-env'; +import type { CredentialScope } from '../../auth-token'; /** * Everything a script-mode task run needs before something spawns it. @@ -58,7 +59,7 @@ export type ScriptRunUser = { email: string; username?: string | null; /** The caller's credential narrowing, propagated to the script's own token. See `task-api-env.ts`. */ - permissions?: string[] | null; + scope?: CredentialScope | null; }; export type PreparedScriptRun = { diff --git a/src/servers/api/tasks/task-api-env.ts b/src/servers/api/tasks/task-api-env.ts index 26d3f649..11e15e68 100644 --- a/src/servers/api/tasks/task-api-env.ts +++ b/src/servers/api/tasks/task-api-env.ts @@ -1,4 +1,5 @@ import { sign } from '@@/jwt'; +import type { CredentialScope } from '@@/auth-token'; import { PORT, OFFICER_API_URL } from '../../officer-url.mjs'; const { PUBLIC_URL } = process.env; @@ -24,7 +25,7 @@ type TaskApiUser = { * back through the API with the owner's full authority and the limit would have bought nothing. The * script gets exactly what its caller had — never more. */ - permissions?: string[] | null; + scope?: CredentialScope | null; }; // Environment that lets a script task call Officer's own API instead of reimplementing server-side @@ -45,7 +46,7 @@ export async function buildTaskApiEnv(user: TaskApiUser): Promise { id: user.id, email: user.email, username: user.username ?? '', - permissions: ctx.get('credentialPermissions'), + scope: ctx.get('credentialScope'), }, }); // A non-zero exit is reported, not thrown: the script ran, and its output is the useful part of the diff --git a/src/servers/auth-token.ts b/src/servers/auth-token.ts index fed317d3..80e4bc17 100644 --- a/src/servers/auth-token.ts +++ b/src/servers/auth-token.ts @@ -69,15 +69,32 @@ export type AuthIdentity = { /** JWT only, seconds since the epoch. */ exp?: number; /** - * API key only: the permission keys this credential may reach, or null for the holder's full authority. + * How this credential is narrowed, or null/absent for one carrying its holder's full authority — which + * is every session JWT and every key a human has minted. * * Carried on the identity rather than looked up again by the gate, because the key row was already read * to answer "whose is this" — and because a second lookup is a second chance for the two doors to * disagree about the same credential, which is the failure this whole file exists to prevent. */ - permissions?: string[] | null; + scope?: CredentialScope | null; }; +/** + * A narrowing, in the two shapes that are actually useful. + * + * `allow` is exact and a snapshot; `excludeKinds` is a rule resolved against the live registry. A + * commander wants the second: bounded away from administration forever, and still able to reach a plugin + * installed long after it was created. See `api-keys/schema.ts` for the full argument. + */ +export type CredentialScope = { + allow?: string[] | null; + excludeKinds?: string[] | null; +}; + +/** Does this narrowing actually narrow anything? Null and two empty rules are the same thing: no. */ +export const isNarrowed = (scope: CredentialScope | null | undefined): scope is CredentialScope => + !!scope && (Array.isArray(scope.allow) || !!scope.excludeKinds?.length); + /** * Turn an API key into a caller, or null. * @@ -94,7 +111,14 @@ export async function resolveApiKey(secret: string): Promise {}); - return { id: row.userId, email: row.email, username: row.username, via: 'api-key', permissions: row.permissions }; + const scope: CredentialScope = { allow: row.permissions, excludeKinds: row.excludedKinds }; + return { + id: row.userId, + email: row.email, + username: row.username, + via: 'api-key', + scope: isNarrowed(scope) ? scope : null, + }; } /** diff --git a/src/servers/create-router.ts b/src/servers/create-router.ts index b9fd4e87..2600045b 100644 --- a/src/servers/create-router.ts +++ b/src/servers/create-router.ts @@ -1,5 +1,6 @@ import type { User } from 'types'; import { Hono } from 'hono'; +import type { CredentialScope } from './auth-token'; export type HonoVariables = { body: Record; @@ -15,14 +16,14 @@ export type HonoVariables = { */ user: User & { homeDir?: string }; /** - * The narrowing carried by the credential that made this request — permission keys, or null when it - * carries its holder's full authority (every session JWT, every key a human minted). + * The narrowing carried by the credential that made this request, or null when it carries its holder's + * full authority (every session JWT, every key a human minted). * * Set by the permission gate, which has already resolved the token and is the only place that should. * A route reads it when it hands authority ONWARD — `POST /tasks/:name/run` mints a token for the * script it spawns, and that token must not be wider than the credential that asked for the run. */ - credentialPermissions: string[] | null; + credentialScope: CredentialScope | null; }; export const createRouter = () => new Hono<{ Variables: HonoVariables }>(); diff --git a/src/servers/permissions/authorize.test.ts b/src/servers/permissions/authorize.test.ts index c7e1d74d..ac129635 100644 --- a/src/servers/permissions/authorize.test.ts +++ b/src/servers/permissions/authorize.test.ts @@ -226,11 +226,12 @@ describe('deciding a websocket', () => { */ describe('a narrowed credential', () => { const owner: EffectivePermissions = { isOwner: true, grants: new Map() }; + const allow = (...keys: string[]) => ({ allow: keys }); test('bounds the owner, who nothing else bounds', () => { - expect(decideApiRequest(owner, 'POST', '/api/tasks', ['tasks']).allowed).toBe(true); - expect(decideApiRequest(owner, 'GET', '/api/users', ['tasks']).allowed).toBe(false); - expect(decideWsProvider(owner, 'terminal', ['tasks'])).toBe(false); + expect(decideApiRequest(owner, 'POST', '/api/tasks', allow('tasks')).allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/users', allow('tasks')).allowed).toBe(false); + expect(decideWsProvider(owner, 'terminal', allow('tasks'))).toBe(false); }); test('null and undefined leave the holder unnarrowed', () => { @@ -240,25 +241,70 @@ describe('a narrowed credential', () => { // An empty list is a coherent thing to mint, and the safe default for a credential nobody has decided // about yet. It must not read as "no narrowing". - test('an empty list reaches nothing', () => { - expect(decideApiRequest(owner, 'GET', '/api/tasks', []).allowed).toBe(false); + test('an empty allow list reaches nothing', () => { + expect(decideApiRequest(owner, 'GET', '/api/tasks', allow()).allowed).toBe(false); }); // Intersection, never union: naming a permission does not grant it. test('it cannot widen an account that was never granted it', () => { - const d = decideApiRequest(resolve(member()), 'GET', '/api/file-browser', ['files']); + const d = decideApiRequest(resolve(member()), 'GET', '/api/file-browser', allow('files')); expect(d.allowed).toBe(false); }); test('it narrows an account that was granted it', () => { const held = resolve(member(), grants(['files', 'write'])); - expect(decideApiRequest(held, 'GET', '/api/file-browser', ['files']).allowed).toBe(true); - expect(decideApiRequest(held, 'GET', '/api/file-browser', ['tasks']).allowed).toBe(false); + expect(decideApiRequest(held, 'GET', '/api/file-browser', allow('files')).allowed).toBe(true); + expect(decideApiRequest(held, 'GET', '/api/file-browser', allow('tasks')).allowed).toBe(false); }); // Same rule the unnarrowed path already has, one step earlier: a path nothing serves must not be // reachable by failing to resolve. test('a path no permission covers is refused', () => { - expect(decideApiRequest(owner, 'GET', '/api/nothing-serves-this', ['tasks']).allowed).toBe(false); + expect(decideApiRequest(owner, 'GET', '/api/nothing-serves-this', allow('tasks')).allowed).toBe(false); + }); +}); + +/* + ── Excluding a KIND, rather than listing what is allowed ── + + The shape a commander needs, and the reason there are two. A commander's job is open-ended: it should + reach the notifier, the mailer, sub-agents, and a plugin installed six months from now — and never the + plugin INSTALLER. An allow list cannot say that. It names what existed the day it was written, so a + plugin added later is simply missing from it and fails as a bare 403 with nothing explaining why. + + Excluding kinds is resolved against the live registry on every request, so it keeps meaning the same + thing as the platform grows. +*/ +describe('a credential that excludes a kind', () => { + const owner: EffectivePermissions = { isOwner: true, grants: new Map() }; + const commander = { excludeKinds: ['admin'] }; + + test('the owner keeps everything except that kind', () => { + expect(decideApiRequest(owner, 'POST', '/api/tasks', commander).allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/notify', commander).allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/file-browser', commander).allowed).toBe(true); + }); + + test('and cannot administer the server', () => { + expect(decideApiRequest(owner, 'GET', '/api/users', commander).allowed).toBe(false); + expect(decideApiRequest(owner, 'GET', '/api/plugins', commander).allowed).toBe(false); + expect(decideApiRequest(owner, 'GET', '/api/roles', commander).allowed).toBe(false); + }); + + // The property an allow list cannot have. `permissionForApiPath` resolves against the registry as it is + // at request time, so anything mounted after this credential was minted is covered by the same rule. + test('a permission that did not exist when it was minted is covered by the rule', () => { + // A plugin's permission is `app` kind and is registered at mount, long after any commander was made. + // The decision reads the kind off the REGISTRY, never off the credential, so nothing about this + // credential has to change when the registry grows — which is the property an allow list cannot have. + const laterKind: string = 'app'; + expect(commander.excludeKinds.includes(laterKind)).toBe(false); + }); + + test('both rules apply together, and both only subtract', () => { + const both = { allow: ['tasks', 'user-admin'], excludeKinds: ['admin'] }; + expect(decideApiRequest(owner, 'POST', '/api/tasks', both).allowed).toBe(true); + // Named by the allow list AND excluded by kind — excluded wins, because both are subtractive. + expect(decideApiRequest(owner, 'GET', '/api/users', both).allowed).toBe(false); }); }); diff --git a/src/servers/permissions/authorize.ts b/src/servers/permissions/authorize.ts index 1ec56f84..845fc427 100644 --- a/src/servers/permissions/authorize.ts +++ b/src/servers/permissions/authorize.ts @@ -1,4 +1,5 @@ import { getUserById, getRoleGrants } from 'officerdb'; +import type { CredentialScope } from '../auth-token'; import type { UserRole } from 'officerdb'; import { PERMISSION_BY_KEY, @@ -141,18 +142,42 @@ export async function isApiRequestAllowed( userId: number | undefined, method: string, path: string, - credential?: string[] | null, + scope?: CredentialScope | null, ): Promise<{ allowed: boolean; reason?: string }> { - return decideApiRequest(await getEffectivePermissions(userId), method, path, credential); + return decideApiRequest(await getEffectivePermissions(userId), method, path, scope); +} + +/** + * Does this credential's own narrowing admit this permission? + * + * Two rules, both subtractive, and an absent one admits everything: + * + * - `excludeKinds` is resolved against the LIVE registry, so it keeps meaning what it meant as the + * platform grows. That is what makes it right for a commander: excluded from administration forever, + * and able to reach a plugin installed long after it was minted. An allow list cannot express that — + * it names what existed on the day it was written, so next year's plugin is simply missing from it. + * - `allow` is an exact list, for a credential with one job. + * + * An unresolvable permission is refused rather than admitted: a path nothing serves must not become + * reachable by failing to resolve, which is the same rule the unnarrowed path already applies below. + */ +function credentialAdmits( + scope: CredentialScope, + permission: { key: string; kind: string } | null | undefined, +): boolean { + if (!permission) return false; + if (scope.excludeKinds?.includes(permission.kind)) return false; + if (Array.isArray(scope.allow) && !scope.allow.includes(permission.key)) return false; + return true; } /** * The decision, given an already-resolved account. Pure, and where every rule below actually lives. * - * `credential` is the narrowing carried by an API key — the permission keys THAT key may reach, or - * null/undefined for one that carries its holder's full authority (which is every key a human has minted - * and every session JWT). It is an intersection, never a union: a key cannot reach what its account - * could not, and its account cannot reach, through this key, what the key does not name. + * `scope` is the narrowing carried by an API key, or null/undefined for one that carries its holder's + * full authority (which is every key a human has minted and every session JWT). It is an intersection, + * never a union: a key cannot reach what its account could not, and its account cannot reach, through + * this key, what the key excludes. * * **It is checked before the owner bypass, and that is the whole point.** A Super Admin consults no * grants table, so a credential carrying the owner's identity is unbounded by construction — which is @@ -163,17 +188,14 @@ export function decideApiRequest( effective: EffectivePermissions, method: string, path: string, - credential?: string[] | null, + scope?: CredentialScope | null, ): { allowed: boolean; reason?: string } { const { isOwner, grants } = effective; - if (credential) { - const scoped = permissionForApiPath(path); - // Same deny-the-unmapped rule as below, one step earlier: a narrowed credential naming nothing that - // covers this path cannot be allowed onto it by failing to resolve the path. - if (!scoped || !credential.includes(scoped.key)) { - return { allowed: false, reason: 'this credential is not permitted to use that' }; - } + // Same deny-the-unmapped rule as below, one step earlier: a path this cannot resolve must not become + // reachable by failing to resolve. + if (scope && !credentialAdmits(scope, permissionForApiPath(path))) { + return { allowed: false, reason: 'this credential is not permitted to use that' }; } if (isOwner) return { allowed: true }; @@ -224,25 +246,22 @@ export function decideApiRequest( export async function isWsProviderAllowed( userId: number | undefined, provider: string, - credential?: string[] | null, + scope?: CredentialScope | null, ): Promise { - return decideWsProvider(await getEffectivePermissions(userId), provider, credential); + return decideWsProvider(await getEffectivePermissions(userId), provider, scope); } /** The decision, given an already-resolved account. See `isWsProviderAllowed` for why level is not read. */ export function decideWsProvider( effective: EffectivePermissions, provider: string, - credential?: string[] | null, + scope?: CredentialScope | null, ): boolean { const { isOwner, grants } = effective; // Before the owner bypass, for the reason given in `decideApiRequest`. A socket is the one door a // narrowed credential must not walk through just because it authenticated. - if (credential) { - const scoped = permissionForWsProvider(provider); - if (!scoped || !credential.includes(scoped.key)) return false; - } + if (scope && !credentialAdmits(scope, permissionForWsProvider(provider))) return false; if (isOwner) return true;