Issue 337: the store's reader could read the controller's signing key and every secret the store holds

Opened and diagnosed: pg_read_all_data on the login the store seat's query runs as. Says what the store can and cannot tell about whether the key was read, why it should be rotated once the fix is taken, and that no rotation mechanism exists yet. The fix is mesh-catalog PR 143.
2026-10-08 21:46:01 +02:00
parent ebffbaa17e
commit 6c297214ad
2 changed files with 202 additions and 0 deletions
@@ -0,0 +1,74 @@
+---
+status: located
+opened: 2026-10-08
+located-in: [mesh-catalog modules/postgres (cmd/postgres-provider/client.go, the reader's grant)]
+fixed-by:
+amended-design:
+replay-none: The hole is a grant on a store, not a behaviour of the core a lab replay can lay over a commit. The module's own tests hold it against a real throwaway server, and the one that uses only what the module had before the fix reads a fixture signing key on the commit before it and is refused on the fix.
+---
+
+# 337. The store's reader can read the controller's signing key
+
+**Severity: critical.** It touches the core's trust: the key every node-engine believes.
+
+## Symptom
+
+On 2026-10-08 the store seat's `query` verb (`mesh-store.query`), called through the mesh MCP server,
+answered as the login `mesh_store_reader`. Asked about privileges only, never about a value, the
+`identity` database answered:
+
+- the login is a member of `pg_read_all_data`;
+- it may `SELECT` the column `private` of `signing_key`, the controller's Ed25519 private key, which signs
+  every declaration a node-engine verifies and then applies as root (the identity context's migration
+  `0001-signing-key.sql`);
+- it may `SELECT` the column `private` of `authority`, the table meant for the mesh's certificate
+  authority (it holds no row today).
+
+The same login may read, in `inventory`, the sealed values and operator keys of `secret` and
+`module_secret`, `operator_key`, `bus_membership.sealed`, `bus_user.password_hash` and
+`enrolment_token.secret`; in `licences`, the sealed tokens of `licence_holder` and `refresh_grant`; and in
+every consumer's database, whatever it holds. In the identity provider's database it may read 20 columns
+whose names say they hold a secret, across 8 tables; in the forge's, 29 across 15. Those counts come from
+the catalogue (`has_column_privilege` over column names), not from reading a row.
+
+## Why it is an issue
+
+[ADR 0159](../../02-DECISIONS/0159-a-tool-call-names-the-machine-and-a-holder-serves-its-seats-verbs.md)
+gave the store seat a `query` verb: one read-only statement against one database.
+[Issue 193](../193-the-store-seats-read-only-query-is-read-only-by-convention/00-report.md) made it read-only
+by its login instead of by text around the statement, and chose `pg_read_all_data` for that login. The
+role kept the verb from writing and let it read **everything**, keys included. Nothing in the design said
+what the verb may read, so nothing refused it.
+
+[ADR 0004](../../02-DECISIONS/0004-a-node-and-how-it-joins.md) rests a node's trust in the mesh on that key:
+"each declaration is verified by its signature, every time", so that a compromised broker cannot forge
+one. Whoever holds the private half can sign a declaration every node-engine accepts. The key was one
+`select` away from every caller allowed to call `mesh-store.query`, and that includes every agent
+session on a machine running the mesh MCP server.
+
+## Impact
+
+- **Anyone who could call the verb could take the key.** Forging a declaration also needs a way to put it
+  on a node's subject on the bus, so the key alone is not a node; but it removes the one check
+  ADR 0004 put between the bus and root on every node.
+- **Every consumer's secrets were readable the same way**: the forge's, the identity provider's, the
+  cloud's, the mail server's. A module's database was meant to be reachable by its own login alone
+  ([ADR 0048](../../02-DECISIONS/0048-a-provider-creates-the-credential-the-mesh-minted.md)).
+- **Whether anyone did read the key is not known.** The diagnosis says what the store can tell and
+  what it cannot.
+
+## How it is checked
+
+- **In the module's tests, against a real throwaway server** (`MESH_POSTGRES_LIVE`): a reader set up as an
+  install before the fix left it (`pg_read_all_data` and a table granted by hand) asks for a fixture
+  `signing_key.private` through `ReadOnlyQuery`. On the commit before the fix it reads it. On the fix it
+  is refused, and the reader is no longer a member of `pg_read_all_data`.
+- **The probe:** across every database the server holds, no column whose name says it holds a secret
+  (private, secret, seal, token, password, credential, salt, a key) may be read by the reader after a
+  reconcile. The same probe runs in the reader's own session before every statement, and a statement is
+  refused while it finds one.
+- **Live, after the fix is taken:** `has_column_privilege('mesh_store_reader', 'signing_key', 'private',
+  'SELECT')` in `identity` answers false, `pg_has_role('mesh_store_reader', 'pg_read_all_data', 'member')`
+  answers false, and `postgres_reader_grants` lists no database with an exposed column.
+
+The diagnosis is in [01-diagnosis.md](01-diagnosis.md).
@@ -0,0 +1,128 @@
+# 337 — Diagnosis
+
+## 2026-10-08
+
+**The records first.** `records_search` for `mesh_store_reader` found nothing. For `pg_read_all_data` it
+found [issue 193](../193-the-store-seats-read-only-query-is-read-only-by-convention/00-report.md), whose fix
+chose the role: "Run the verb as a role that can only read, granted `pg_read_all_data`". Issue 193 was
+about writing. Nobody asked what the role lets the verb read. A search for "signing key" found
+[ADR 0004](../../02-DECISIONS/0004-a-node-and-how-it-joins.md) and nothing on rotating it.
+
+**Where the grant is.** The `postgres` module's provider (mesh-catalog `modules/postgres`, the holder of
+`mesh-store`) makes the reader in `ReaderStatements`, `cmd/postgres-provider/client.go`:
+`GRANT pg_read_all_data TO mesh_store_reader`. It ran once per process, on the first query. Both
+`postgres_query` and the seat's `query` verb call `ReadOnlyQuery`, which connects as the reader to
+whichever database the caller names. The role gives `SELECT` on every table, view and sequence of every
+database, and `USAGE` on every schema. So every column the store holds was one statement away.
+
+**Confirmed live, by privileges only.** Through `mesh-store.query` against `identity`:
+`current_user` is `mesh_store_reader`, `pg_has_role('pg_read_all_data', 'member')` is true, and
+`has_column_privilege` is true for `signing_key.private` and `authority.private`. `authority` holds no row
+(`count(*)` is 0), so the mesh's certificate authority key was not exposed through this table. The
+signing key table holds one active key. Against `inventory`, `licences` and two consumers' databases the
+same check was run over column names, with the results the report gives. No statement selected a key or
+a secret value.
+
+**Ruled out.**
+
+- *The verb is limited to some databases.* It is not. `ReadOnlyQuery` takes any database name, and the
+  role covers every database.
+- *The seat's `invokes` grant limits who can call it.* It limits who can call it, not what the call can
+  read. The mesh MCP server's account on a machine may call it, and every agent session on that machine
+  goes through that account.
+- *The controller's databases are protected by their own logins* ([ADR 0008](../../02-DECISIONS/0008-a-context-owns-its-store.md)
+  as the identity context states it: "reached with a credential no other context holds"). They are, for
+  logging in as their owners. A predefined role that reads all data goes around ownership.
+
+### Did anyone read the key?
+
+What the store can say, read without printing any value:
+
+- **No statement log.** `log_statement` is `none`, `log_min_duration_statement` is `-1`, and
+  `log_connections` is `off`. `pg_stat_statements` is not installed in `identity`.
+- **The table's scan counts.** Since the server last started (2026-10-03, 22:53 UTC), `signing_key` was
+  read by sequential scan 7 times, the last at 01:02:40 UTC on 2026-10-08. `node_key` and `authority`
+  were scanned 7 times each, with exactly the same last time. I think those 7 are the nightly dump, which
+  reads every table of the database at once, because three tables with the same count and the same
+  second fit that and nothing else. The controller reads the key by index (1,725 index scans), so its own
+  reads are not among the 7.
+- **What the counts cannot rule out.** A statement that reads the key with a condition on `id` would be an
+  index scan, indistinguishable from the controller's. Anything before the 2026-10-03 start is not in
+  these counts. Before issue 193's fix (2026-10-02) the verb ran as the server's administrator, which
+  could read the key as well. And the tool runner keeps no record of a call's arguments that this
+  diagnosis could search.
+
+**So whether the key was read is not known, and cannot be known from the store.** There is no sign of a
+read beyond the nightly dump since 2026-10-03. Before that there is no record at all.
+
+### Should the signing key be rotated?
+
+**Yes, after the fix is live, and through a mechanism that does not exist yet.**
+
+- *Why rotate.* The key was readable by every caller of the verb since the verb existed. Its exposure
+  cannot be excluded, and a forged declaration is applied as root on every node that receives it.
+- *Why after the fix.* A new key made before the fix is taken would be exactly as readable as the old one.
+- *How a rotation is done in this mesh today.* It is not. The controller's `rotate` verb replaces a pair
+  credential or a module's own secret, not the signing key. The identity context keeps a `retired`
+  timestamp on `signing_key` and signs with whichever key is not retired, read per signature, so the
+  controller side could change keys with one retirement and one new key. But a node-engine holds one
+  signing key, the one its enrolment token carried, and refuses a declaration signed by any other. So
+  changing the key today means re-enrolling every node with a new token. ADR 0004 names the missing
+  piece: "Rotating the signing identity is a fleet-wide operation with an overlapping rollover". That
+  is a node-engine that accepts a successor key announced in a declaration signed by the current one,
+  and a controller that retires the old key only once every node has acknowledged the new one. That
+  needs a decision before anyone builds it.
+- **Nothing was rotated in this diagnosis.**
+
+### The fix
+
+mesh-catalog pull request 143, in the `postgres` module:
+
+1. **The reader holds no role.** `pg_read_all_data` is no longer granted. Every role the reader is a
+   member of is revoked on the first query and on every reconcile pass, so an existing install loses it
+   without anybody acting.
+2. **Deny by default, per column.** A consumer declares what the reader may read in its contribution to
+   `postgres-database`, under `readable`: `[schema.]table` or `[schema.]table(column, …)`. A database no
+   contribution names, the controller's own among them, gives the reader nothing.
+3. **A secret is never granted.** A column or table whose name says it holds a secret is refused even
+   when declared, and the refusal is reported.
+4. **Reconciled every minute, on every database.** Grants the reader holds that are not declared are
+   revoked, and declared ones that are missing are granted. `postgres_reader_grants` answers the last
+   pass, by name only.
+5. **Fails closed where it is used.** Before a caller's statement runs, the reader's own session lists
+   the secret-named columns it can read. If there are any, for example from a grant to `PUBLIC` or one
+   made by hand, the statement is refused and the columns are named. A statement naming one of the
+   controller's known secret tables, or a name that reads as a key or a secret, is refused before it is
+   sent. That check is a second layer: a name built from pieces inside a function call gets past it, and
+   the privileges still refuse the read.
+
+**Why the declaration is in the contribution, and the controller learns nothing.** Contribution values
+are passed through to the provider as written. The controller's module check does not read them, and the
+existing `extensions` value already works this way. `readable` is a list, not an object, because the
+controller tells a contribution's single shape from its several-instance shape by whether every value is
+an object. The lenient read of an unknown manifest field
+([ADR 0262](../../02-DECISIONS/0262-a-preference-has-a-default-in-its-manifest-and-the-operators-own-value-has-none.md))
+is not involved either: the key sits inside `contributes`, whose values are read as they are. A
+catalogue test parses every manifest's `readable`, so a malformed one is found at review.
+
+**What it costs.** Until modules declare `readable`, the verb reads nothing in any database, and none
+does yet. Diagnosis that read the controller's `inventory` through the verb loses that. The controller is
+not a consumer of the provision, so it has no contribution in which to declare what of its own is
+readable. Giving it one is a follow-up, and so is each module's own declaration.
+
+### Still exposed until the fix is taken
+
+The module's upgrade policy is `record`: a person takes each build after a backup. Until that build is
+taken on the control node, the reader keeps `pg_read_all_data`, and the signing key and every secret
+above remain one statement away. Merging the pull request does not close the hole.
+
+### Follow-ups
+
+- **A decision on rotating the signing key**, with the overlapping rollover ADR 0004 asks for. Then the
+  rotation itself.
+- **What the controller lets the reader read** of `inventory`, `identity` and `licences`. None of
+  `identity`'s tables should be readable.
+- **The consumers' declarations**, module by module, each reviewed for columns whose names hide a secret.
+  The name check catches what is named for what it is, and a view can rename a column.
+- **The lesson for the knowledge base**: a role that makes a login read-only is not a role that makes
+  it harmless. `pg_read_all_data` is a superuser's reach without the writes.