Issue 337 after review: say control-node, drop the store's operational settings, and match the fix's revoke-by-grantor and read-back
mesh/merge-gate pass: the change touches no module of the mesh's graph
mesh/repo-check pass: its merge-check.sh passed
mesh/delivery superseded: a newer head of the same pull request

Adds what the fix cannot see (definer functions and views; the name check
as a backstop) and, as a follow-up, that the reader's read-only session is a
default a statement can turn off, so the missing write grants are the guard.
This commit is contained in:
jochen
2026-10-08 22:02:30 +02:00
parent 6c297214ad
commit b17839a7db
@@ -38,22 +38,21 @@ a secret value.
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.
- **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 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
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 2026-10-03. Before that there is no record at all.
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?
@@ -86,9 +85,13 @@ mesh-catalog pull request 143, in the `postgres` module:
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.
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, never as revoked. 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
@@ -110,10 +113,20 @@ does yet. Diagnosis that read the controller's `inventory` through the verb lose
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
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
@@ -124,5 +137,13 @@ above remain one statement away. Merging the pull request does not close the hol
`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.