Merge pull request 'Issue 337: the store's reader can read the controller's signing key (critical)' (#230) from issues/337-the-stores-reader-can-read-the-controllers-signing-key into main
This commit was merged in pull request #230.
This commit is contained in:
@@ -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,152 @@
|
||||
# 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 record of statements.** The store logs neither statements nor connections, and keeps no
|
||||
statistics of statements in `identity`.
|
||||
- **The table's scan counts.** Since the server last started, `signing_key` was read by sequential scan
|
||||
as many times as `node_key` and `authority`, each with the same last time. I think those scans 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, so its own
|
||||
reads are not among them.
|
||||
- **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 server's last 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 the server's last start. 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.** Each grant the reader holds that is not declared is
|
||||
revoked as the role that granted it, because a superuser's revoke acts as the table's owner and
|
||||
leaves a grant made by another role holding grant option. The grants are then read back, and anything
|
||||
the reader still holds that is not declared is reported as a failure with its reason, and only what
|
||||
the read-back shows gone is reported as revoked. If the reader was given a privilege with grant
|
||||
option and passed it on, a plain revoke fails, and the pass revokes with `CASCADE`, which reaches only
|
||||
the grants that came from the reader. Declared
|
||||
columns that are missing are granted. The reader's role is altered only when an attribute, a setting
|
||||
or its password has drifted, so its password is not sent every minute. `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.
|
||||
|
||||
### What the fix cannot see
|
||||
|
||||
A `SECURITY DEFINER` function the reader may execute runs with its owner's rights, and PUBLIC may
|
||||
execute a new function unless that is revoked. A view the reader may read reads with its owner's rights.
|
||||
Either can return a secret without any grant to the reader on the table behind it, so neither the
|
||||
reconcile nor the probe sees it. Each pass lists both, for every database, in `postgres_reader_grants`
|
||||
and once in the module's log; they are reported, not refused. The name check is likewise a backstop: a
|
||||
module that declares a table whose secret sits under a plain column name has declared that secret
|
||||
readable, and the review of its declaration is what stands in the way.
|
||||
|
||||
### 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 reader's read-only session is a default, not a guard.** Since issue 193 the reader's sessions
|
||||
start read-only, and issue 193 calls that "read-only by its role and its session". A statement can
|
||||
`SET transaction_read_only = off`, and the reader is then stopped only by holding no grant to write
|
||||
(it keeps the `TEMP` privilege every login has, so it can make temporary tables). The guard is the
|
||||
missing grants. Issue 193's wording should say so, and a test should hold that a write after turning
|
||||
the setting off is still refused.
|
||||
- **Definer functions and views** that a pass lists: each one found is reviewed, and its `EXECUTE` or
|
||||
`SELECT` revoked from PUBLIC where it exposes a secret.
|
||||
- **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.
|
||||
Reference in New Issue
Block a user