diff --git a/CHANGELOG.md b/CHANGELOG.md index e279fc5f..6a664903 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,13 @@ Registering an OpenID Connect provider needs every host in its discovery documen issuer also needs `oauth2.googleapis.com` and `openidconnect.googleapis.com`. Registration is refused with the untrusted host named. +A Bot id may now contain only letters, digits, hyphen and underscore, and must start with a letter or +digit. The same rule container and volume names have always followed. A deployment whose +`COMPUTER_BOT_ID` breaks it refuses to start and says so, rather than answering 400 to everything. + +`AUDIT_RETENTION_DAYS` is new and unset, which keeps the audit trail forever, as before. Set it to a +whole number of days to have old rows removed. + Sessions survive and nobody signs in again. ### Added @@ -86,6 +93,55 @@ Sessions survive and nobody signs in again. is unavailable never blocks a sign-in. ### Fixed +- **A boundary rule applied on one server out of N.** The policy is read from memory on every action, + which is right, but memory was only ever filled at boot. An administrator's new deny rule was + enforced by whichever process served the request and roughly one action in N went through it, while + the admin screen reported success because the row really was saved and the audit trail agreed + because it records the boundary each process started with. Both honest, and both describing + something other than what the fleet was enforcing. A write now announces on Postgres in the same + transaction and every server re-reads, including on reconnect, so a server that was down when the + rule changed catches up rather than waiting for a restart. Reset travels the same way. +- **A ref resolved on one replica and nowhere else.** The gateway turns the opaque ref in a click into + the element it points at, and that mapping lived in a `Map` in the process that took the snapshot. + On any other replica the ref resolved to nothing, so a deny rule written about the element did not + match and the click went through, recorded as allowed with no rule. It is in Postgres now, keyed on + the generation the computer stamped, so a ref from a superseded page still resolves to nothing. +- **Anybody signed in could act as anybody's Bot.** The Bot id travels in the path and the acting + routes checked only that somebody was signed in, so a signed-in person could drive another person's + private Bot, reset its browser, read its screen and fire its granted tools. Every route under a Bot + id now asks the store the same question the roster already asks, and a Bot that does not exist and + one belonging to somebody else answer identically. +- **The computer fleet listing was open to any signed-in person.** It ignores its `:botId` and returns + every Bot's machine, so it told anybody who could reach it every Bot id in the deployment and + whether each was running, private coworkers included. Administrator-only now. +- **A Bot id could name a directory outside the profiles volume.** The id arrives as a URL segment or + a header, was joined onto a filesystem path, and `reset` deletes that path recursively as root, so + `../../tmp/something` deleted it. Refused at the request boundary and again where the path is built. +- **A mistyped deny rule permitted instead of refusing.** A rule that parsed and evaluated but + answered with something other than true or false was neither a match nor an error, so + `deny: ["Submit order"]` — what somebody writes who reads the list as labels — let the action + through with nothing logged, while the rule sat on the Boundaries page looking as though it were in + force. Any non-boolean answer is now a broken rule and takes the existing fail-closed path. +- **Rotating a Bot's key left the old one live.** Editing a key wrote a new vault row and repointed + the Bot at it, leaving the previous credential decryptable and still valid with nothing listing it, + so rotation did not do the one thing rotation is for. Deleting a Bot left its key live too. Both + revoke now. +- **Nothing recorded what changed about a Bot.** Ten mutating routes wrote one audit row between them + and there was no event type for any of the other nine. A Bot's endpoint is where conversation + content is sent, so "who pointed this Bot at that host, and when" is the first question in an + incident and could not be answered. Eight event types and eight rows now, recording what changed and + never a value. +- **The people list and the channel list grew without bound.** Both were read in full on every render, + and reading one person ran the whole people aggregate over the deployment twice per role change. + Both are paged now, and the people screen searches on the server so somebody can be found without + walking pages. +- **A computer accumulated one browser per Bot, forever.** Nothing closed an idle one, so a deployment + where every employee has a Bot trends toward a resident Chromium per employee in one container until + it is killed for memory. There is a cap and an idle timeout, and closing one costs only a relaunch + because the profile is on disk. +- **The audit screen's filters were sequential scans.** It filters by event type, by who did it and by + what it was done to, and the only index was on the timestamp, over what becomes the largest table in + the deployment. Each filter leads its own index now. - **A deployment with no identity provider came up open by default.** Covered under Changed above, and listed here too because it is the one on this list that was reachable from the internet. - **Registering a company's identity provider was owned by whoever registered it.** Better Auth @@ -156,6 +212,18 @@ Sessions survive and nobody signs in again. ### Changed +- **A retention policy for the audit trail.** `AUDIT_RETENTION_DAYS` removes rows older than the + window it names, swept hourly by whichever server holds an advisory lock. Unset by default, because + deleting somebody's audit trail because a default said so is the worse of the two failures. The + trail stays append-only: the database permits a delete only when the transaction declares a + retention window and only for rows already outside it, so removing recent rows is still impossible + and an `UPDATE` still is under every condition. +- **`allowed_groups` is documented as a declaration, not a control.** The tenant package writes it and + nothing reads it on any access path, and `users.groups` is written by nothing either, so both halves + of the rule are waiting on group membership arriving from the identity provider. Channel access is + membership alone. The columns stay, because they are the right shape for the rule they are named + for. Thanks to [@NathanTarbert](https://github.com/CopilotKit/OpenBot/pull/92) and + [@andreolf](https://github.com/CopilotKit/OpenBot/issues/82). - **Running with no sign-in takes a flag and nothing else.** It used to be locked with `NODE_ENV=production`, which is exactly backwards: `NODE_ENV` is unset unless somebody sets it, so a container on a VM with a hand-written env file and no identity provider served every visitor on diff --git a/server/src/computer/policy-listener.ts b/server/src/computer/policy-listener.ts index 01dba02a..89b1f19b 100644 --- a/server/src/computer/policy-listener.ts +++ b/server/src/computer/policy-listener.ts @@ -25,12 +25,12 @@ export async function startPolicyListener( ): Promise { const connection = postgres(databaseUrl, { max: 1 }); - await connection.listen(ACTION_POLICY_TOPIC, () => { - /* - * The payload is ignored on purpose. It says the boundary moved, not what it moved to: a rule - * list can outgrow NOTIFY's 8000-byte cap, and a server enforcing whatever fitted in a payload - * would be a subtler version of the bug this fixes. The row is the record; this re-reads it. - */ + /* + * The payload is ignored on purpose. It says the boundary moved, not what it moved to: a rule list + * can outgrow NOTIFY's 8000-byte cap, and a server enforcing whatever fitted in a payload would be + * a subtler version of the bug this fixes. The row is the record; this re-reads it. + */ + const reread = () => { void store.refresh().catch((error) => { console.error( JSON.stringify({ @@ -40,7 +40,21 @@ export async function startPolicyListener( }), ); }); - }); + }; + + /* + * Also on reconnect, which is the case a live notification cannot cover. + * + * A NOTIFY reaches whoever is listening at the time. A replica that was restarting, or whose + * connection had dropped, is not, so it misses the announcement and goes on enforcing the rules it + * read at boot until something restarts it again. That is the original bug wearing a smaller hat, + * and it is worse for being intermittent. + * + * `onlisten` fires whenever the driver establishes or re-establishes the subscription, so the row + * is re-read at exactly the moments this server could have missed something. Thanks to + * @NathanTarbert, whose #94 caught this. + */ + await connection.listen(ACTION_POLICY_TOPIC, reread, reread); return { stop: async () => { diff --git a/server/src/computer/policy-store.ts b/server/src/computer/policy-store.ts index 94111bef..f8abc93e 100644 --- a/server/src/computer/policy-store.ts +++ b/server/src/computer/policy-store.ts @@ -90,33 +90,38 @@ export function createPolicyStore( // Written before it is enforced. If the write fails this throws and the caller reports a // failure, which is the honest outcome: an administrator who is told a rule was saved must // not be enforcing a rule that will disappear at the next restart. - await database - .insert(actionPolicy) - .values({ - id: CURRENT, - mode: next.mode, - deny: next.deny, - allow: next.allow, - updatedBy: by ?? null, - updatedAt: new Date(), - }) - .onConflictDoUpdate({ - target: actionPolicy.id, - set: { + // + // The announcement goes in the same transaction, so it is delivered on commit: a write that + // rolls back announces nothing, and there is no window where the row has changed and the + // other servers have not been told. Same shape as `channels/routes.ts`. + await database.transaction(async (transaction) => { + await transaction + .insert(actionPolicy) + .values({ + id: CURRENT, mode: next.mode, deny: next.deny, allow: next.allow, updatedBy: by ?? null, updatedAt: new Date(), - }, - }); + }) + .onConflictDoUpdate({ + target: actionPolicy.id, + set: { + mode: next.mode, + deny: next.deny, + allow: next.allow, + updatedBy: by ?? null, + updatedAt: new Date(), + }, + }); + + // Every server hears it, including this one, which re-reads and arrives at what it + // already has. + await announce(transaction); + }); } current = next; - - // Announced after the row is written, so a server that re-reads on this signal finds the new - // rule rather than the old one. Every server hears it, including this one, which re-reads and - // arrives at what it already has. - if (database) await announce(database); }, reset: async () => { @@ -124,10 +129,14 @@ export function createPolicyStore( // this deployment has no boundary of its own again, and changing what configuration says then // changes what it enforces, which is what an operator expects of a reset. if (database) { - await database.delete(actionPolicy).where(eq(actionPolicy.id, CURRENT)); + await database.transaction(async (transaction) => { + await transaction + .delete(actionPolicy) + .where(eq(actionPolicy.id, CURRENT)); + await announce(transaction); + }); } current = clone(configured); - if (database) await announce(database); }, load: async () => { @@ -176,7 +185,7 @@ export function createPolicyStore( * announcement costs the other servers their update until their next restart, which is worth a loud * log and is not worth failing a write an administrator has been told succeeded. */ -async function announce(database: Database): Promise { +async function announce(database: Pick): Promise { try { await database.execute(sql`select pg_notify(${ACTION_POLICY_TOPIC}, '')`); } catch (error) { diff --git a/server/tests/policy-fanout.integration.test.ts b/server/tests/policy-fanout.integration.test.ts index c85e1183..95b33c2a 100644 --- a/server/tests/policy-fanout.integration.test.ts +++ b/server/tests/policy-fanout.integration.test.ts @@ -130,7 +130,76 @@ describe("a rule added on one server", () => { }); }); +describe("a server that was not listening when it changed", () => { + test("catches up when its subscription comes back", async () => { + /* + * A NOTIFY reaches whoever is listening at the time. A replica that was restarting, or whose + * connection had dropped, is not, so it misses the announcement and goes on enforcing what it + * read at boot until something restarts it again. That is the original bug wearing a smaller hat + * and it is worse for being intermittent: the fleet disagrees with itself and nothing says so. + * + * Simulated the way it happens: the rule changes while this server has no subscription, and then + * the subscription is established. `onlisten` fires on every establish, including reconnects, so + * the row is re-read at exactly the moments a notification could have been missed. + * + * Thanks to @NathanTarbert, whose #94 caught this. + */ + const wroteIt = createPolicyStore(DEFAULT_ACTION_POLICY, database); + const wasDown = createPolicyStore(DEFAULT_ACTION_POLICY, database); + await wasDown.load(); + expect(wasDown.get().deny).toEqual([]); + + // Changed while nothing on this server is listening. + await wroteIt.set({ mode: "enforce", deny: [RULE], allow: ["true"] }); + expect(wasDown.get().deny).toEqual([]); + + const listener = await startPolicyListener(databaseUrl, wasDown); + try { + await until(() => wasDown.get().deny.length > 0); + expect(wasDown.get().deny).toEqual([RULE]); + } finally { + await listener.stop(); + } + }); +}); + describe("the announcement itself", () => { + test("a write that rolls back announces nothing", async () => { + /* + * The announcement is in the same transaction as the write, so it is delivered on commit. A + * failed write that had already announced would have every other server re-read a row that never + * changed, which is harmless, and a committed write that had not announced yet leaves them stale, + * which is not. Same shape as channel activity. + */ + const listening = createPolicyStore(DEFAULT_ACTION_POLICY, database); + await listening.load(); + const listener = await startPolicyListener(databaseUrl, listening); + + try { + // A store whose write fails after the insert. The transaction rolls back, taking the + // announcement with it. + const failing = new Proxy(database, { + get: (target, property, receiver) => + property === "transaction" + ? async () => { + throw new Error("the write rolled back"); + } + : Reflect.get(target, property, receiver), + }); + const store = createPolicyStore(DEFAULT_ACTION_POLICY, failing); + + await expect( + store.set({ mode: "enforce", deny: [RULE], allow: ["true"] }), + ).rejects.toThrow(); + + // Nothing was saved, so nothing should have been announced and nobody should have moved. + await new Promise((resolve) => setTimeout(resolve, 200)); + expect(listening.get().deny).toEqual([]); + } finally { + await listener.stop(); + } + }); + test("a saved rule is not lost when the announcement fails", async () => { // The row is the record and this process is already enforcing it. Failing the write here would // tell an administrator their rule was rejected when it is saved and live, which is worse than