From 4f44bf1eb5538c6466849c4c336dde936b16e2c0 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 21:43:15 +0200 Subject: [PATCH 1/4] Give the store's reader only the columns a module declares readable, never a secret, so a query cannot read the controller's signing key (hq issue 337) mesh_store_reader held pg_read_all_data, so every caller of mesh-store.query and postgres_query could read every column the store holds: the signing key nodes trust, sealed secrets, password hashes. The reader now holds no role; each minute, on every database, its memberships are revoked and its grants brought to what the consumer's contribution lists under readable, minus any column or table whose name says it holds a secret. Before a statement runs, the reader's own session is asked which secret columns it can read and the statement is refused if any; a statement naming a known secret table is refused before it is sent. --- modules/postgres/README.md | 28 +- .../postgres/cmd/postgres-provider/client.go | 81 ++- .../cmd/postgres-provider/client_test.go | 10 +- .../cmd/postgres-provider/live_reader_test.go | 115 ++++ .../postgres/cmd/postgres-provider/main.go | 3 + .../cmd/postgres-provider/readable.go | 573 ++++++++++++++++++ .../postgres-provider/readable_live_test.go | 113 ++++ .../cmd/postgres-provider/readable_test.go | 296 +++++++++ .../postgres/cmd/postgres-provider/tools.go | 15 +- 9 files changed, 1215 insertions(+), 19 deletions(-) create mode 100644 modules/postgres/cmd/postgres-provider/live_reader_test.go create mode 100644 modules/postgres/cmd/postgres-provider/readable.go create mode 100644 modules/postgres/cmd/postgres-provider/readable_live_test.go create mode 100644 modules/postgres/cmd/postgres-provider/readable_test.go diff --git a/modules/postgres/README.md b/modules/postgres/README.md index ff38e6b5..c5dbcef8 100644 --- a/modules/postgres/README.md +++ b/modules/postgres/README.md @@ -47,9 +47,26 @@ for, so one removed by hand is installed again. - **`postgres_retire_database` renames, it does not drop**: the database becomes `_deleted_` and its owner is locked. Removing the data is a person's act, by hand. - **A caller's statement never runs as the superuser.** `postgres_query` and the seat's `query` verb - run as `mesh_store_reader` — `pg_read_all_data` and nothing else, every session read-only by the - server's own setting, a 60 s statement timeout — with the statement sent as given (issue 193). - Without the reader's password (`own-secrets.reader`) the call is refused. + run as `mesh_store_reader` — every session read-only by the server's own setting, a 60 s statement + timeout — with the statement sent as given (issue 193). Without the reader's password + (`own-secrets.reader`) the call is refused. +- **The reader reads only what a module declares readable, and never a secret** (issue 337). It holds + no role — until issue 337 it held `pg_read_all_data`, which let any caller of the seat's `query` read + the controller's signing key — and is granted per column, in each consumer's database, what that + consumer's contribution lists: + + `"contributes": {"postgres-database": {"name": "letta", "readable": ["agents", "steps(id, created_at)"]}}` + + An entry is `[schema.]table` (every column it has, at each pass) or `[schema.]table(column, …)`. A + database no contribution names — the controller's own among them — gives the reader nothing. A column + or table whose name says it holds a secret (private, secret, sealed, token, password, credential, salt, + a key) is never granted, declared or not, and the refusal is reported. Every minute, on every + database, the reader's role memberships are revoked and its grants brought to exactly that, so an + existing install is narrowed without anybody acting and a narrowed declaration takes effect at the + next pass. Before a caller's statement runs, the reader's own session is asked which secret-named + columns it can read (a grant to `PUBLIC` or by hand would show there); if any, the statement is + refused and the columns named. A statement that names a known secret table of the controller's, or a + secret-sounding name, is refused before it is sent — a second layer, not the guard. ## Tools @@ -59,6 +76,7 @@ for, so one removed by hand is installed again. | `postgres_query` `{database, sql}` | one read-only statement, as the reader; rows keyed by column, `NULL` as null | | `postgres_retire_database` `{database, confirm}` | renames a database aside and locks its owner; `confirm` repeats the name | | `mesh-store.databases`, `mesh-store.query` | the store seat's verbs: the same listing and read-only query | +| `postgres_reader_grants` | what the reader may read in each database as the last pass left it: readable columns, refused secrets, declared but missing, revoked, and any secret it can still read; names only | | `provisioner_retirement` | what is held, what waits for a person, what was rejected, every retired consumer and set-aside database with when, why and size | | `provisioner_retire_approve`, `provisioner_retire_reject` `{consumers, why, by}` | a person's answer to a set waiting; through the controller's `retire approve|reject` | | `provisioner_delete` `{consumer, confirm, why, by}` | drops one retired consumer's database and role, or one set-aside database; through the controller's `cleanup delete` | @@ -76,4 +94,6 @@ has one, byte for byte the same as keycloak's. also runs `live_test.go` against a real, throwaway one (see the file for a `pgvector/pgvector` container): the extension installed and a second pass a no-op, the reader unable to write, a retired login locked out with its data kept and listed retired, enabled again as it was, and a deletion -dropping only its own database and role. +dropping only its own database and role. `live_reader_test.go` and `readable_live_test.go` need no extension: an install's +reader as issue 337 found it reading no secret after its first query, and the reconcile granting exactly +what is declared, refusing secrets, taking back what is no longer declared, and finding a grant to `PUBLIC`. diff --git a/modules/postgres/cmd/postgres-provider/client.go b/modules/postgres/cmd/postgres-provider/client.go index c6954239..2d31a6fc 100644 --- a/modules/postgres/cmd/postgres-provider/client.go +++ b/modules/postgres/cmd/postgres-provider/client.go @@ -24,9 +24,10 @@ import ( "github.com/jackc/pgx/v5/pgconn" ) -// Reader is the login a caller's statement runs as (novox/hq issue 193). It may read every table and -// change nothing: `pg_read_all_data` and no other grant, and every transaction it opens is read-only -// by the server's own setting. A statement cannot climb out of a login the way it can out of a +// Reader is the login a caller's statement runs as (novox/hq issue 193). It changes nothing — every +// transaction it opens is read-only by the server's own setting — and it reads only the columns a +// consuming module declares readable, never one that holds a secret (readable.go, novox/hq issue 337). +// It holds no role: until issue 337 it held `pg_read_all_data`, and with it the controller's signing key. A statement cannot climb out of a login the way it can out of a // transaction wrapped around it as text: `COMMIT; DROP …` ended the old wrapper and ran the rest as // the superuser, and even one read-only statement as a superuser can run a program on the server. const Reader = "mesh_store_reader" @@ -103,6 +104,9 @@ type Client struct { readerMu sync.Mutex readerReady bool + + // reader is what the last reconcile of the reader's grants found (readable.go). + reader readerState } // NewClient is a client over a dialer; nil dials the server for real. @@ -195,16 +199,20 @@ func (c *Client) as(ctx context.Context, l Login, sql string) (Result, error) { if err != nil { return Result{}, err } + return lastAnswer(results), nil +} + +// lastAnswer is the last statement that answered rows, or the last statement: what psql -c printed last. +func lastAnswer(results []Result) Result { if len(results) == 0 { - return Result{}, nil + return Result{} } - // The last statement that answered rows, or the last statement: what psql -c printed last. for i := len(results) - 1; i >= 0; i-- { if len(results[i].Fields) > 0 { - return results[i], nil + return results[i] } } - return results[len(results)-1], nil + return results[len(results)-1] } // Query runs SQL as the admin against a database ("postgres" when empty). @@ -454,7 +462,9 @@ func (c *Client) ListDatabases(ctx context.Context) ([]Database, error) { } // ReaderStatements are the DDL that make the read-only login, every attribute stated so an existing -// role someone widened is narrowed again. +// role someone widened is narrowed again. It is granted no role: what it reads is granted per column +// (readable.go), and every role it holds is revoked (ensureReaderRole) — `pg_read_all_data` first, +// which every install before novox/hq issue 337 gave it. func ReaderStatements(exists bool, password string) []string { verb := "CREATE" if exists { @@ -463,7 +473,7 @@ func ReaderStatements(exists bool, password string) []string { return []string{ fmt.Sprintf("%s ROLE %s WITH LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION "+ "NOBYPASSRLS INHERIT PASSWORD %s VALID UNTIL 'infinity'", verb, Ident(Reader), Literal(password)), - "GRANT pg_read_all_data TO " + Ident(Reader), + "REVOKE pg_read_all_data FROM " + Ident(Reader), "ALTER ROLE " + Ident(Reader) + " SET default_transaction_read_only = on", "ALTER ROLE " + Ident(Reader) + " SET statement_timeout = '60s'", } @@ -483,6 +493,19 @@ func (c *Client) EnsureReader(ctx context.Context) error { if c.readerReady { return nil } + if err := c.ensureReaderRole(ctx); err != nil { + return err + } + c.readerReady = true + return nil +} + +// ensureReaderRole makes or narrows the reader's role and revokes every role it is a member of. Asked +// by every reconcile pass, so an install that granted it a role is narrowed without a restart. +func (c *Client) ensureReaderRole(ctx context.Context) error { + if c.conn.ReaderPassword == "" { + return errReaderMissing + } has, err := c.exists(ctx, "SELECT 1 FROM pg_roles WHERE rolname = "+Literal(Reader)) if err != nil { return err @@ -492,7 +515,18 @@ func (c *Client) EnsureReader(ctx context.Context) error { return err } } - c.readerReady = true + r, err := c.Query(ctx, "", "SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON r.oid = m.roleid "+ + "JOIN pg_roles u ON u.oid = m.member WHERE u.rolname = "+Literal(Reader)) + if err != nil { + return err + } + for _, row := range r.Rows { + if role := cell(row, 0); role != "" { + if _, err := c.Query(ctx, "", "REVOKE "+Ident(role)+" FROM "+Ident(Reader)); err != nil { + return err + } + } + } return nil } @@ -504,14 +538,39 @@ var firstWord = regexp.MustCompile(`^\s*([A-Za-z]+)`) // **Read-only by the login, not by text around the statement.** The statement is sent as it was // given, as the reader, whose role can write nothing and whose session the server makes read-only. // Never as the admin: without the reader's password the call is refused. +// +// **It reads only what was declared readable, and fails closed** (novox/hq issue 337). A statement +// naming a known secret table or a secret-sounding name is refused before it is sent; then, in the +// reader's own session, the reader is asked which secret columns it can read, and if it can read any +// the statement is refused, naming them. func (c *Client) ReadOnlyQuery(ctx context.Context, database, sql string) (Result, error) { + if why := refusedStatement(database, sql); why != "" { + return Result{}, errors.New(why) + } if err := c.EnsureReader(ctx); err != nil { return Result{}, err } - r, err := c.as(ctx, Login{Database: database, User: Reader, Password: c.conn.ReaderPassword, Options: readerOptions}, sql) + s, err := c.dial(ctx, Login{Database: database, User: Reader, Password: c.conn.ReaderPassword, Options: readerOptions}) if err != nil { return Result{}, err } + defer s.Close() + probe, err := s.Run(ctx, ExposureProbe) + if err != nil { + return Result{}, fmt.Errorf("asking what the reader can read, before the statement: %w", err) + } + if len(probe) > 0 { + if exposed := Exposed(probe[len(probe)-1]); len(exposed) > 0 { + return Result{}, fmt.Errorf("refused: in %s the store's reader can read %s, whose names say they hold secrets; "+ + "nothing is run as it until it cannot (novox/hq issue 337 — a grant to PUBLIC or by hand, or a "+ + "declaration the provider has not yet narrowed)", database, strings.Join(exposed, ", ")) + } + } + results, err := s.Run(ctx, sql) + if err != nil { + return Result{}, err + } + r := lastAnswer(results) size := 0 for _, row := range r.Rows { for _, v := range row { diff --git a/modules/postgres/cmd/postgres-provider/client_test.go b/modules/postgres/cmd/postgres-provider/client_test.go index c9925108..57e45ddb 100644 --- a/modules/postgres/cmd/postgres-provider/client_test.go +++ b/modules/postgres/cmd/postgres-provider/client_test.go @@ -221,12 +221,18 @@ func TestTheReaderIsMadeAsTheAdminWithEveryAttributeOnce(t *testing.T) { } } } - if creates != 1 || readerCalls != 2 { + // Each query is two statements as the reader: what it can read that is secret, then the caller's. + if creates != 1 || readerCalls != 4 { t.Fatalf("made %d times, %d reader calls", creates, readerCalls) } - if !has(ddl, `^GRANT pg_read_all_data TO "`+Reader+`"$`) || !has(ddl, `default_transaction_read_only = on`) { + if !has(ddl, `default_transaction_read_only = on`) { t.Fatal(strings.Join(ddl, "\n")) } + // novox/hq issue 337: the reader reads only what is declared, so it is never given the role that + // reads everything — and an install that gave it that role has it taken back. + if has(ddl, `GRANT pg_read_all_data`) || !has(ddl, `^REVOKE pg_read_all_data FROM "`+Reader+`"$`) { + t.Fatal("the reader is given, or keeps, pg_read_all_data:\n" + strings.Join(ddl, "\n")) + } } func TestWithoutTheReadersPasswordNothingRunsAsTheAdmin(t *testing.T) { diff --git a/modules/postgres/cmd/postgres-provider/live_reader_test.go b/modules/postgres/cmd/postgres-provider/live_reader_test.go new file mode 100644 index 00000000..4c07bd86 --- /dev/null +++ b/modules/postgres/cmd/postgres-provider/live_reader_test.go @@ -0,0 +1,115 @@ +package main + +// Against a real server, when one is named (MESH_POSTGRES_LIVE, as live_test.go) — skipped otherwise. +// +// novox/hq issue 337: an install made before it gave the store's reader `pg_read_all_data`, and with it +// every secret the store holds. This proves, through nothing but the query the store seat serves, that +// such an install's reader reads no secret any more: the first query takes the role back, and a secret +// the reader can still read (a grant by hand) refuses the query rather than answering it. It uses only +// what the module had before the fix, so it fails on the commit before it. + +import ( + "net/url" + "os" + "strings" + "testing" +) + +const liveStore = "mesh_test_store" + +func liveClient(t *testing.T) *Client { + t.Helper() + raw := os.Getenv("MESH_POSTGRES_LIVE") + if raw == "" { + t.Skip("MESH_POSTGRES_LIVE names no server") + } + u, _ := url.Parse(raw) + pw, _ := u.User.Password() + env := map[string]string{"MESH_PROVISION_POSTGRES": raw, "MESH_POSTGRES_PASSWORD": pw, "MESH_POSTGRES_READER_PASSWORD": "reader-pw"} + c, err := ClientFromEnv(func(k string) string { return env[k] }) + if err != nil { + t.Fatal(err) + } + return c +} + +// freshStore makes a database holding a key beside a plain table, and a reader as an install before +// issue 337 left it: granted pg_read_all_data, and a table granted to it by hand. +func freshStore(t *testing.T, c *Client) { + t.Helper() + for _, db := range []string{liveStore, "identity"} { + if _, err := c.Query(ctx, "", "DROP DATABASE IF EXISTS "+Ident(db)+" WITH (FORCE)"); err != nil { + t.Fatal(err) + } + if _, err := c.Query(ctx, "", "CREATE DATABASE "+Ident(db)); err != nil { + t.Fatal(err) + } + } + setup := []string{ + "CREATE TABLE vault (id int, name text, private bytea)", + "INSERT INTO vault VALUES (1, 'the mesh', 'not to be read')", + "CREATE TABLE notes (id int, body text)", + "INSERT INTO notes VALUES (1, 'plain')", + "CREATE SCHEMA audit", + "CREATE TABLE audit.events (at timestamptz default now(), what text)", + "INSERT INTO audit.events (what) VALUES ('started')", + } + for _, sql := range setup { + if _, err := c.Query(ctx, liveStore, sql); err != nil { + t.Fatal(sql, err) + } + } + if _, err := c.Query(ctx, "identity", "CREATE TABLE signing_key (id int, public bytea, private bytea); INSERT INTO signing_key VALUES (1, 'p', 'k')"); err != nil { + t.Fatal(err) + } + has, err := c.exists(ctx, "SELECT 1 FROM pg_roles WHERE rolname = "+Literal(Reader)) + if err != nil { + t.Fatal(err) + } + verb := "CREATE" + if has { + verb = "ALTER" + } + for _, sql := range []string{ + verb + " ROLE " + Ident(Reader) + " LOGIN PASSWORD 'reader-pw'", + "GRANT pg_read_all_data TO " + Ident(Reader), + } { + if _, err := c.Query(ctx, "", sql); err != nil { + t.Fatal(sql, err) + } + } + if _, err := c.Query(ctx, liveStore, "GRANT SELECT ON vault TO "+Ident(Reader)); err != nil { + t.Fatal(err) + } +} + +func dropStore(c *Client) { + for _, db := range []string{liveStore, "identity"} { + _, _ = c.Query(ctx, "", "DROP DATABASE IF EXISTS "+Ident(db)+" WITH (FORCE)") + } +} + +func TestLiveAnInstallsReaderReadsNoSecret(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + + // The signing key, by name, in the controller's database: refused. + if r, err := c.ReadOnlyQuery(ctx, "identity", "SELECT private FROM signing_key"); err == nil { + t.Fatalf("the reader read the signing key: %v", r.Maps()) + } + // A plain column of a table holding a key, granted to the reader by hand: refused while the reader + // can read the key beside it, and the key named. + r, err := c.ReadOnlyQuery(ctx, liveStore, "SELECT name FROM vault") + if err == nil { + t.Fatalf("answered while the reader can read vault.private: %v", r.Maps()) + } + if !strings.Contains(err.Error(), "public.vault.private") { + t.Fatal(err) + } + // The role that read everything is taken back by the first query. + r, err = c.Query(ctx, "", "SELECT pg_has_role("+Literal(Reader)+", 'pg_read_all_data', 'MEMBER') AS reads_all") + if err != nil || cell(r.Rows[0], 0) != "f" { + t.Fatalf("the reader still holds pg_read_all_data: %v %v", r.Rows, err) + } +} diff --git a/modules/postgres/cmd/postgres-provider/main.go b/modules/postgres/cmd/postgres-provider/main.go index 9ab6f218..af26205c 100644 --- a/modules/postgres/cmd/postgres-provider/main.go +++ b/modules/postgres/cmd/postgres-provider/main.go @@ -53,6 +53,9 @@ func main() { go h.Run(context.Background()) } go audit() + // The store's reader is narrowed to what modules declare readable, at once and every minute, whether + // or not the provisioner runs: without a contributions file it is given nothing (novox/hq issue 337). + go pg.RunReader(context.Background(), os.Getenv("MESH_RECEIVES"), time.Minute, say) // The retirement tools beside postgres's own: what is retired here, approving or rejecting what waits // for a person, and deleting a retired consumer (novox/hq ADR 0230). if err := stdio.Serve("", append(Tools(pg), RetirementTools(h)...)); err != nil { diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go new file mode 100644 index 00000000..787e1687 --- /dev/null +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -0,0 +1,573 @@ +package main + +// What the store's reader may read (novox/hq issue 337). +// +// The reader (Reader) is the login every caller's statement runs as: the store seat's `query` verb and +// postgres_query. Until issue 337 it was granted `pg_read_all_data`, so it could read every column of +// every database the store holds — the controller's signing key and its sealed secrets among them — and +// anybody who could call the verb could read them through it. +// +// **Deny by default.** The reader holds no role and no grant but what this file gives it, and it gives +// only what a consuming module declares readable in its contribution to `postgres-database`: +// +// "contributes": { "postgres-database": { "name": "letta", "readable": ["agents", "steps(id, created_at)"] } } +// +// An entry is `[schema.]table` — every column it has at each pass — or `[schema.]table(col, col)` — those +// columns only, spelled as the catalogue spells them. A database no contribution names (the controller's +// own among them) gives the reader nothing. A list is used rather than an object so the controller's +// reading of `contributes` cannot mistake it for the several-instance shape. +// +// **A column or table whose name says it holds a secret is never granted, declared or not** +// (SecretName): private keys, sealed values, tokens, passwords, credentials, salts. The refusal is said +// in the reader's report (postgres_reader_grants), so a module that meant it learns why. +// +// **Reconciled every pass, on every database, not only made once.** The reader's role memberships are +// revoked (pg_read_all_data included, which is what an existing install holds), and in every database +// every grant the reader holds that is not declared is revoked, and every declared one that is missing +// is granted. A narrowed declaration narrows the reader at the next pass. +// +// **Checked where it is used.** Before a caller's statement runs, the reader's own session is asked +// which columns it can read whose name says secret; if any, the statement is refused and the columns +// named (ExposureProbe). That catches what the reconcile does not own — a grant to PUBLIC, a grant made +// by hand — and fails closed. And a statement that names a known secret table or a secret-sounding name +// is refused before it is sent (refusedStatement): defence in depth, not the guard — the privileges are. + +import ( + "context" + "encoding/json" + "fmt" + "os" + "regexp" + "sort" + "strings" + "sync" + "time" +) + +// ReadableTable is one entry of a contribution's `readable`. +type ReadableTable struct { + Schema string + Table string + Columns []string // nil: every column the table has +} + +func (r ReadableTable) String() string { + s := r.Schema + "." + r.Table + if r.Columns != nil { + s += "(" + strings.Join(r.Columns, ", ") + ")" + } + return s +} + +var ( + readableEntry = regexp.MustCompile(`^(?:([A-Za-z_][A-Za-z0-9_$]*)\.)?([A-Za-z_][A-Za-z0-9_$]*)\s*(?:\(([^()]*)\))?$`) + identifier = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_$]*$`) +) + +// ParseReadable reads a contribution's `readable`: absent is nothing readable. Anything malformed refuses +// the whole list, so a typo never grants more than was meant. +func ParseReadable(values map[string]any) ([]ReadableTable, error) { + raw, ok := values["readable"] + if !ok || raw == nil { + return nil, nil + } + list, ok := raw.([]any) + if !ok { + return nil, fmt.Errorf("readable must be a list of tables, as \"table\" or \"table(column, column)\", not %T", raw) + } + var out []ReadableTable + seen := map[string]bool{} + for _, v := range list { + entry, ok := v.(string) + if !ok { + return nil, fmt.Errorf("readable: %v is not a table name", v) + } + m := readableEntry.FindStringSubmatch(strings.TrimSpace(entry)) + if m == nil { + return nil, fmt.Errorf("readable: %q is not \"[schema.]table\" or \"[schema.]table(column, …)\"", entry) + } + t := ReadableTable{Schema: firstOf(m[1], "public"), Table: m[2]} + if strings.Contains(entry, "(") { + t.Columns = []string{} + for _, col := range strings.Split(m[3], ",") { + col = strings.TrimSpace(col) + if !identifier.MatchString(col) { + return nil, fmt.Errorf("readable: %q names a column %q that is not a plain name", entry, col) + } + t.Columns = append(t.Columns, col) + } + } + key := t.Schema + "." + t.Table + if seen[key] { + return nil, fmt.Errorf("readable: %s is named twice", key) + } + seen[key] = true + out = append(out, t) + } + return out, nil +} + +// secretName is what a column or table is called when it holds a secret, or something a secret is +// made from: a private key, a sealed value, a token, a password or its hash, a credential, a salt, a +// one-time-password seed, or a key of any kind. Wide on purpose — a public key refused is a small cost; +// a private one granted is the whole mesh. +var secretName = regexp.MustCompile(`(?i)(private|secret|seal|token|passw|pwd|credential|salt|cookie|nonce|` + + `api_?key|(^|_)(otp|totp|mfa|key|keys|rands)(_|$))`) + +// SecretName says whether a column or table name says it holds a secret. +func SecretName(name string) bool { return secretName.MatchString(name) } + +// knownSecretTables are tables whose secret is not in their columns' names — the mesh's own, in the +// controller's databases. A statement naming one is refused before it is sent. +var knownSecretTables = map[string][]string{ + "identity": {"signing_key", "authority", "node_key"}, + "inventory": {"secret", "module_secret", "operator_key", "bus_membership", "bus_user", "enrolment_token"}, + "licences": {"licence_holder", "licence_manager"}, +} + +// statementWord is what the statement guard reads: every word of the statement, literals included, so +// a name in a string handed to a function is read as well. +var statementWord = regexp.MustCompile(`[A-Za-z_][A-Za-z0-9_$]*`) + +// guardName is narrower than secretName, because it is read over a caller's whole statement, where +// `key` and `token` are ordinary words (jsonb_object_keys). +var guardName = regexp.MustCompile(`(?i)(private|secret|sealed|passw|credential|api_?key|signing_key|` + + `sealing_key|overlay_key|operator_key|refresh_token|access_token|authtoken|pg_authid|pg_shadow|pg_user_mapping)`) + +// refusedStatement says why a caller's statement is refused before it is sent, or "". +func refusedStatement(database, sql string) string { + known := map[string]bool{} + for _, t := range knownSecretTables[database] { + known[t] = true + } + for _, w := range statementWord.FindAllString(sql, -1) { + lw := strings.ToLower(w) + if known[lw] { + return fmt.Sprintf("the statement names %s.%s, which holds the mesh's keys or secrets; the store's reader is "+ + "never given it (novox/hq issue 337)", database, lw) + } + if guardName.MatchString(lw) { + return fmt.Sprintf("the statement names %q, which reads as a key, secret or credential; the store's reader is "+ + "never given one, and statements naming one are refused (novox/hq issue 337)", w) + } + } + return "" +} + +// ---- the probe -------------------------------------------------------------------------------- + +// ExposureProbe lists every column the session's own login may SELECT outside the system schemas — by +// table grant, column grant, PUBLIC or a role it holds. Exposed filters it to the secret ones. +const ExposureProbe = `SELECT n.nspname, c.relname, a.attname FROM pg_catalog.pg_attribute a ` + + `JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace ` + + `WHERE c.relkind IN ('r','v','m','p','f') AND a.attnum > 0 AND NOT a.attisdropped ` + + `AND n.nspname NOT IN ('pg_catalog','information_schema') AND n.nspname NOT LIKE 'pg\_toast%' ` + + `AND pg_catalog.has_column_privilege(c.oid, a.attnum, 'SELECT') ORDER BY 1, 2, 3` + +// Exposed is the probe's rows that name a secret: "schema.table.column". +func Exposed(r Result) []string { + var out []string + for _, row := range r.Rows { + if len(row) < 3 || row[0] == nil || row[1] == nil || row[2] == nil { + continue + } + if SecretName(*row[1]) || SecretName(*row[2]) { + out = append(out, *row[0]+"."+*row[1]+"."+*row[2]) + } + } + return out +} + +// ---- the plan --------------------------------------------------------------------------------- + +// column is one column of one table. +type column struct{ Schema, Table, Name string } + +type table struct{ Schema, Table string } + +// held is what the reader holds in one database, read from the catalogue as the admin. +type held struct { + // Tables are those the reader holds any table-level privilege on, with their kind ('S': sequence). + Tables map[table]string + // Columns are the column privileges it holds, each column's privilege types. + Columns map[column][]string +} + +// DatabaseReport is what the reader may read in one database, and what was refused or missing. +type DatabaseReport struct { + Database string `json:"database"` + Declared bool `json:"declared"` + Readable []string `json:"readable,omitempty"` + Refused []string `json:"refused,omitempty"` + Missing []string `json:"missing,omitempty"` + Revoked []string `json:"revoked,omitempty"` + Exposed []string `json:"exposed,omitempty"` + Error string `json:"error,omitempty"` +} + +// planDatabase is the DDL that brings the reader's grants in one database to exactly what is declared, +// minus every secret: revoke what is held and not wanted, grant what is wanted and not held. +// existing is every column of every declared table that the database has. Pure; the tests' subject. +func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[string]bool) ([]string, DatabaseReport) { + var rep DatabaseReport + byTable := map[table][]string{} + for _, c := range existing { + t := table{c.Schema, c.Table} + byTable[t] = append(byTable[t], c.Name) + } + want := map[column]bool{} + for _, d := range decl { + t := table{d.Schema, d.Table} + cols, ok := byTable[t] + if !ok { + rep.Missing = append(rep.Missing, d.String()) + continue + } + if SecretName(d.Table) { + rep.Refused = append(rep.Refused, d.Schema+"."+d.Table+" (its name says it holds secrets)") + continue + } + named := cols + if d.Columns != nil { + // A declared column the table lacks is said and left out: granting it would fail the + // database's whole change. + named = nil + has := map[string]bool{} + for _, c := range cols { + has[c] = true + } + for _, c := range d.Columns { + if has[c] { + named = append(named, c) + } else { + rep.Missing = append(rep.Missing, d.Schema+"."+d.Table+"."+c) + } + } + } + for _, c := range named { + if SecretName(c) { + rep.Refused = append(rep.Refused, d.Schema+"."+d.Table+"."+c+" (its name says it holds a secret)") + continue + } + want[column{d.Schema, d.Table, c}] = true + } + } + // Every table-level privilege is revoked: the reader is only ever given columns. REVOKE on a table + // takes its column privileges with it, so those are granted again below if wanted. + var stmts []string + cleared := map[table]bool{} + for _, t := range sortedTables(h.Tables) { + kind := "TABLE" + if h.Tables[t] == "S" { + kind = "SEQUENCE" + } + stmts = append(stmts, fmt.Sprintf("REVOKE ALL ON %s %s.%s FROM %s", kind, Ident(t.Schema), Ident(t.Table), Ident(Reader))) + rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+" (every privilege on the table)") + cleared[t] = true + } + have := map[column]bool{} + unwanted := map[table][]string{} + for c, privs := range h.Columns { + if cleared[table{c.Schema, c.Table}] { + continue + } + onlySelect := len(privs) == 1 && privs[0] == "SELECT" + if want[c] && onlySelect { + have[c] = true + continue + } + // Not wanted, or held with more than SELECT: revoked, and granted SELECT again below if wanted. + t := table{c.Schema, c.Table} + unwanted[t] = append(unwanted[t], c.Name) + } + for _, t := range sortedKeys(unwanted) { + cols := unwanted[t] + sort.Strings(cols) + stmts = append(stmts, fmt.Sprintf("REVOKE ALL (%s) ON %s.%s FROM %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) + for _, c := range cols { + rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+"."+c) + } + } + grant := map[table][]string{} + schemas := map[string]bool{} + for c := range want { + rep.Readable = append(rep.Readable, c.Schema+"."+c.Table+"."+c.Name) + if !have[c] { + t := table{c.Schema, c.Table} + grant[t] = append(grant[t], c.Name) + schemas[c.Schema] = true + } + } + sort.Strings(rep.Readable) + for _, s := range sortedSet(schemas) { + if noUsage[s] { + stmts = append(stmts, fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", Ident(s), Ident(Reader))) + } + } + for _, t := range sortedKeys(grant) { + cols := grant[t] + sort.Strings(cols) + stmts = append(stmts, fmt.Sprintf("GRANT SELECT (%s) ON %s.%s TO %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) + } + return stmts, rep +} + +func idents(names []string) string { + out := make([]string, len(names)) + for i, n := range names { + out[i] = Ident(n) + } + return strings.Join(out, ", ") +} + +func sortedTables(m map[table]string) []table { + out := make([]table, 0, len(m)) + for t := range m { + out = append(out, t) + } + sort.Slice(out, func(i, j int) bool { return out[i].Schema+"."+out[i].Table < out[j].Schema+"."+out[j].Table }) + return out +} + +func sortedKeys(m map[table][]string) []table { + out := make([]table, 0, len(m)) + for t := range m { + out = append(out, t) + } + sort.Slice(out, func(i, j int) bool { return out[i].Schema+"."+out[i].Table < out[j].Schema+"."+out[j].Table }) + return out +} + +func sortedSet(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for s := range m { + out = append(out, s) + } + sort.Strings(out) + return out +} + +// ---- the reconcile ------------------------------------------------------------------------------ + +// Declarations are what each consumer's database lets the reader read, by database; a database whose +// declaration was malformed is in Malformed and is given nothing. +type Declarations struct { + Tables map[string][]ReadableTable + Malformed map[string]string +} + +// DeclarationsFrom reads the contributions file the mesh delivers (MESH_RECEIVES): each consumer's +// database is its login (`as`), and its `readable` is in its values. Unreadable is no declarations: +// the reader is then given nothing, and the reason is answered. +func DeclarationsFrom(path string) (Declarations, error) { + d := Declarations{Tables: map[string][]ReadableTable{}, Malformed: map[string]string{}} + if path == "" { + return d, fmt.Errorf("no contributions file is named (MESH_RECEIVES); the reader is given nothing") + } + raw, err := os.ReadFile(path) + if err != nil { + return d, fmt.Errorf("contributions file unreadable (%s), the reader is given nothing: %w", path, err) + } + var doc struct { + Given []struct { + As string `json:"as"` + Values map[string]any `json:"values"` + } `json:"given"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + return d, fmt.Errorf("contributions file is not JSON (%s), the reader is given nothing: %w", path, err) + } + for _, g := range doc.Given { + if g.As == "" { + continue + } + tables, err := ParseReadable(g.Values) + if err != nil { + d.Malformed[g.As] = err.Error() + continue + } + if len(tables) > 0 { + d.Tables[g.As] = tables + } + } + return d, nil +} + +// ReaderReport is the last reconcile's answer, for postgres_reader_grants. +type ReaderReport struct { + At time.Time `json:"at"` + Problem string `json:"problem,omitempty"` + Databases []DatabaseReport `json:"databases"` +} + +type readerState struct { + mu sync.Mutex + last ReaderReport +} + +// ReconcileReader brings the reader to exactly what is declared, in every database the server holds. +// Each database is its own: one that fails is said in its report and the rest go on. +func (c *Client) ReconcileReader(ctx context.Context, d Declarations, problem string) ReaderReport { + rep := ReaderReport{At: time.Now().UTC(), Problem: problem} + if err := c.ensureReaderRole(ctx); err != nil { + rep.Problem = strings.TrimSpace(rep.Problem + "; the reader's role: " + err.Error()) + c.keep(rep) + return rep + } + r, err := c.Query(ctx, "", "SELECT datname FROM pg_database WHERE datallowconn AND NOT datistemplate ORDER BY datname") + if err != nil { + rep.Problem = strings.TrimSpace(rep.Problem + "; listing the databases: " + err.Error()) + c.keep(rep) + return rep + } + present := map[string]bool{} + for _, row := range r.Rows { + if len(row) == 0 || row[0] == nil { + continue + } + db := *row[0] + present[db] = true + dr := c.reconcileDatabase(ctx, db, d.Tables[db]) + if why, ok := d.Malformed[db]; ok { + dr.Error = strings.TrimSpace(dr.Error + " its readable declaration is refused, nothing granted: " + why) + } + rep.Databases = append(rep.Databases, dr) + } + for db := range d.Tables { + if !present[db] { + rep.Databases = append(rep.Databases, DatabaseReport{Database: db, Declared: true, Error: "declared readable, but the server holds no such database"}) + } + } + sort.Slice(rep.Databases, func(i, j int) bool { return rep.Databases[i].Database < rep.Databases[j].Database }) + c.keep(rep) + return rep +} + +func (c *Client) keep(rep ReaderReport) { + c.reader.mu.Lock() + c.reader.last = rep + c.reader.mu.Unlock() +} + +// LastReaderReport is what the last reconcile found. +func (c *Client) LastReaderReport() ReaderReport { + c.reader.mu.Lock() + defer c.reader.mu.Unlock() + return c.reader.last +} + +func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []ReadableTable) DatabaseReport { + fail := func(err error) DatabaseReport { + return DatabaseReport{Database: db, Declared: decl != nil, Error: err.Error()} + } + h := held{Tables: map[table]string{}, Columns: map[column][]string{}} + readerOID := "(SELECT oid FROM pg_catalog.pg_roles WHERE rolname = " + Literal(Reader) + ")" + r, err := c.Query(ctx, db, "SELECT n.nspname, c.relname, c.relkind::text FROM pg_catalog.pg_class c "+ + "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace CROSS JOIN LATERAL pg_catalog.aclexplode(c.relacl) x "+ + "WHERE x.grantee = "+readerOID+" GROUP BY 1, 2, 3") + if err != nil { + return fail(err) + } + for _, row := range r.Rows { + h.Tables[table{cell(row, 0), cell(row, 1)}] = cell(row, 2) + } + r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname, x.privilege_type FROM pg_catalog.pg_attribute a "+ + "JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "CROSS JOIN LATERAL pg_catalog.aclexplode(a.attacl) x WHERE x.grantee = "+readerOID) + if err != nil { + return fail(err) + } + for _, row := range r.Rows { + k := column{cell(row, 0), cell(row, 1), cell(row, 2)} + h.Columns[k] = append(h.Columns[k], cell(row, 3)) + } + var existing []column + noUsage := map[string]bool{} + if len(decl) > 0 { + var pairs []string + for _, d := range decl { + pairs = append(pairs, "("+Literal(d.Schema)+", "+Literal(d.Table)+")") + } + r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname FROM pg_catalog.pg_attribute a "+ + "JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "WHERE c.relkind IN ('r','v','m','p','f') AND a.attnum > 0 AND NOT a.attisdropped "+ + "AND (n.nspname, c.relname) IN ("+strings.Join(pairs, ", ")+") ORDER BY a.attnum") + if err != nil { + return fail(err) + } + for _, row := range r.Rows { + existing = append(existing, column{cell(row, 0), cell(row, 1), cell(row, 2)}) + } + r, err = c.Query(ctx, db, "SELECT nspname FROM pg_catalog.pg_namespace WHERE NOT pg_catalog.has_schema_privilege("+ + Literal(Reader)+", oid, 'USAGE')") + if err != nil { + return fail(err) + } + for _, row := range r.Rows { + noUsage[cell(row, 0)] = true + } + } + stmts, rep := planDatabase(decl, existing, h, noUsage) + rep.Database, rep.Declared = db, decl != nil + if len(stmts) > 0 { + // One string: the simple protocol runs it as one implicit transaction, all or nothing. + if _, err := c.Query(ctx, db, strings.Join(stmts, "; ")); err != nil { + rep.Error = "changing the reader's grants: " + err.Error() + return rep + } + } + // Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a + // grant made by hand) is found here, said, and makes the reader's statements against it refused. + probe := strings.Replace(ExposureProbe, "pg_catalog.has_column_privilege(c.oid", "pg_catalog.has_column_privilege("+Literal(Reader)+", c.oid", 1) + if r, err = c.Query(ctx, db, probe); err != nil { + rep.Error = "probing what the reader can read: " + err.Error() + return rep + } + rep.Exposed = Exposed(r) + return rep +} + +// RunReader reconciles the reader at once and then every `every`, until ctx ends, reading the +// declarations from the contributions file each time. A change, a refusal and an exposure are said. +func (c *Client) RunReader(ctx context.Context, receives string, every time.Duration, say func(string, ...any)) { + lastSaid := "" + for { + d, err := DeclarationsFrom(receives) + problem := "" + if err != nil { + problem = err.Error() + } + rep := c.ReconcileReader(ctx, d, problem) + var lines []string + if rep.Problem != "" { + lines = append(lines, "reader: "+rep.Problem) + } + for _, db := range rep.Databases { + if len(db.Exposed) > 0 { + lines = append(lines, fmt.Sprintf("reader: EXPOSED in %s — the reader can read %s; its statements there are refused until it cannot", + db.Database, strings.Join(db.Exposed, ", "))) + } + if db.Error != "" { + lines = append(lines, fmt.Sprintf("reader: %s: %s", db.Database, db.Error)) + } + if len(db.Refused) > 0 { + lines = append(lines, fmt.Sprintf("reader: %s: not granted, %s", db.Database, strings.Join(db.Refused, "; "))) + } + if len(db.Revoked) > 0 { + say("reader: %s: revoked %s", db.Database, strings.Join(db.Revoked, ", ")) + } + } + // The same standing is said once, not every pass. + if now := strings.Join(lines, "\n"); now != lastSaid { + for _, l := range lines { + say("%s", l) + } + lastSaid = now + } + select { + case <-ctx.Done(): + return + case <-time.After(every): + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_live_test.go b/modules/postgres/cmd/postgres-provider/readable_live_test.go new file mode 100644 index 00000000..8db18b45 --- /dev/null +++ b/modules/postgres/cmd/postgres-provider/readable_live_test.go @@ -0,0 +1,113 @@ +package main + +// Against a real server (MESH_POSTGRES_LIVE), novox/hq issue 337: the reconcile gives the reader exactly +// what is declared and no secret, takes back what is no longer declared, and a grant it does not own +// that exposes a secret is found and refuses queries there. + +import ( + "strings" + "testing" +) + +func columnReadable(t *testing.T, c *Client, db, tbl, col string) bool { + t.Helper() + r, err := c.Query(ctx, db, "SELECT has_column_privilege("+Literal(Reader)+", "+Literal(tbl)+", "+Literal(col)+", 'SELECT')") + if err != nil { + t.Fatal(err) + } + return cell(r.Rows[0], 0) == "t" +} + +// secretsReadable is the probe every database is held to: the columns the reader can read whose names +// say they hold a secret, across every database the server holds. +func secretsReadable(t *testing.T, c *Client) []string { + t.Helper() + dbs, err := c.ListDatabases(ctx) + if err != nil { + t.Fatal(err) + } + probe := strings.Replace(ExposureProbe, "pg_catalog.has_column_privilege(c.oid", "pg_catalog.has_column_privilege("+Literal(Reader)+", c.oid", 1) + var out []string + for _, db := range dbs { + r, err := c.Query(ctx, db.Name, probe) + if err != nil { + t.Fatal(db.Name, err) + } + for _, e := range Exposed(r) { + out = append(out, db.Name+": "+e) + } + } + return out +} + +func TestLiveTheReaderReadsWhatIsDeclaredAndNoSecret(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + + if got := secretsReadable(t, c); len(got) == 0 { + t.Fatal("the install before the fix was not reproduced: no secret readable") + } + decl := Declarations{Tables: map[string][]ReadableTable{liveStore: { + {Schema: "public", Table: "notes"}, + {Schema: "public", Table: "vault"}, // every column: private is still refused, by its name + {Schema: "audit", Table: "events"}, + }}} + rep := c.ReconcileReader(ctx, decl, "") + if rep.Problem != "" { + t.Fatal(rep.Problem) + } + if got := secretsReadable(t, c); len(got) != 0 { + t.Fatalf("after the reconcile the reader can read %v", got) + } + for _, want := range [][3]string{{liveStore, "notes", "body"}, {liveStore, "vault", "name"}, {liveStore, "audit.events", "what"}} { + if !columnReadable(t, c, want[0], want[1], want[2]) { + t.Errorf("%v is declared and not readable", want) + } + } + if columnReadable(t, c, liveStore, "vault", "private") || columnReadable(t, c, "identity", "signing_key", "public") { + t.Fatal("a secret, or an undeclared database, is readable") + } + r, err := c.ReadOnlyQuery(ctx, liveStore, "SELECT name FROM vault") + if err != nil || len(r.Rows) != 1 { + t.Fatal("a declared column cannot be read:", r, err) + } + if _, err := c.ReadOnlyQuery(ctx, liveStore, "SELECT * FROM vault"); err == nil { + t.Fatal("SELECT * read past the refused column") + } + if _, err := c.ReadOnlyQuery(ctx, "identity", "SELECT id FROM signing_key"); err == nil { + t.Fatal("the signing key's table answered") + } + // The same declaration again changes nothing. + again := c.ReconcileReader(ctx, decl, "") + for _, db := range again.Databases { + if len(db.Revoked) > 0 || db.Error != "" { + t.Fatalf("a second pass changed %s: %+v", db.Database, db) + } + } + + // Narrowed: notes is no longer declared, and is taken back. + decl.Tables[liveStore] = []ReadableTable{{Schema: "public", Table: "vault", Columns: []string{"name"}}} + c.ReconcileReader(ctx, decl, "") + if columnReadable(t, c, liveStore, "notes", "body") || columnReadable(t, c, liveStore, "audit.events", "what") { + t.Fatal("a table no longer declared is still readable") + } + + // A grant the provider does not own exposes a secret: found, said, and queries there refused. + if _, err := c.Query(ctx, liveStore, "GRANT SELECT ON vault TO PUBLIC"); err != nil { + t.Fatal(err) + } + rep = c.ReconcileReader(ctx, decl, "") + var exposed []string + for _, db := range rep.Databases { + if db.Database == liveStore { + exposed = db.Exposed + } + } + if strings.Join(exposed, ",") != "public.vault.private" { + t.Fatalf("exposure not found: %+v", rep.Databases) + } + if _, err := c.ReadOnlyQuery(ctx, liveStore, "SELECT name FROM vault"); err == nil || !strings.Contains(err.Error(), "public.vault.private") { + t.Fatal("a query ran while the reader can read a secret:", err) + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_test.go b/modules/postgres/cmd/postgres-provider/readable_test.go new file mode 100644 index 00000000..ebebbd6b --- /dev/null +++ b/modules/postgres/cmd/postgres-provider/readable_test.go @@ -0,0 +1,296 @@ +package main + +// The store's reader reads only what a module declares readable, never a secret (novox/hq issue 337). +// What the server does with the grants is proven against a real one (live_reader_test.go); here, that +// the module asks for the right ones. + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestReadableFromAContribution(t *testing.T) { + got, err := ParseReadable(map[string]any{"name": "x", "readable": []any{"agents", "audit.events", "steps( id , created_at )"}}) + if err != nil { + t.Fatal(err) + } + want := []string{"public.agents", "audit.events", "public.steps(id, created_at)"} + if len(got) != len(want) { + t.Fatalf("%v", got) + } + for i := range want { + if got[i].String() != want[i] { + t.Fatalf("%d: %s, want %s", i, got[i], want[i]) + } + } + if got[0].Columns != nil { + t.Fatal("a bare table is every column, not none") + } + if none, err := ParseReadable(map[string]any{"name": "x"}); err != nil || none != nil { + t.Fatal("absent is nothing readable:", none, err) + } + for _, bad := range []any{ + "agents", // not a list + []any{"agents; drop table x"}, // not a name + []any{"steps(id, created_at"}, // unbalanced + []any{"steps(id, \"x\")"}, // a quoted column + []any{"agents", "public.agents"}, // twice + []any{42}, // not a string + []any{"a.b.c"}, // three parts + } { + if _, err := ParseReadable(map[string]any{"readable": bad}); err == nil { + t.Errorf("%v was accepted", bad) + } + } +} + +func TestSecretNames(t *testing.T) { + for _, secret := range []string{"private", "signing_key", "sealed", "operator_sealed", "module_secret", "secret", + "operator_key", "node_key", "overlay_key", "sealing_key", "password_hash", "passwd", "salt", "rands", "token", + "refresh_token", "secret_data", "credential_data", "api_key", "apikey", "totp_secret", "otp", "key", "client_secret"} { + if !SecretName(secret) { + t.Errorf("%s is not taken for a secret", secret) + } + } + for _, plain := range []string{"name", "created", "commit_hash", "status", "node", "keyboard", "monkey", "footprint", + "description", "id", "retired", "public"} { + if SecretName(plain) { + t.Errorf("%s is taken for a secret", plain) + } + } +} + +func TestTheReaderIsGivenOnlyWhatIsDeclaredAndNeverASecret(t *testing.T) { + decl, _ := ParseReadable(map[string]any{"readable": []any{"node", "signing_key", "build(id, sealed, nope)", "gone"}}) + existing := []column{ + {"public", "node", "name"}, {"public", "node", "overlay_key"}, {"public", "node", "role"}, + {"public", "signing_key", "public"}, {"public", "signing_key", "private"}, + {"public", "build", "id"}, {"public", "build", "sealed"}, {"public", "build", "log"}, + } + stmts, rep := planDatabase(decl, existing, held{Tables: map[table]string{}, Columns: map[column][]string{}}, map[string]bool{}) + sql := strings.Join(stmts, "\n") + if !strings.Contains(sql, `GRANT SELECT ("name", "role") ON "public"."node" TO "mesh_store_reader"`) || + !strings.Contains(sql, `GRANT SELECT ("id") ON "public"."build" TO "mesh_store_reader"`) { + t.Fatal(sql) + } + for _, never := range []string{"overlay_key", "private", "sealed", "signing_key", `"log"`} { + if strings.Contains(sql, never) { + t.Fatalf("%s granted:\n%s", never, sql) + } + } + if len(rep.Refused) != 3 || len(rep.Missing) != 2 { + t.Fatalf("refused %v, missing %v", rep.Refused, rep.Missing) + } + if strings.Join(rep.Readable, ",") != "public.build.id,public.node.name,public.node.role" { + t.Fatal(rep.Readable) + } +} + +func TestWhatTheReaderHoldsAndIsNotDeclaredIsRevoked(t *testing.T) { + // An install before issue 337, or a grant by hand: a table-level grant, a column no longer + // declared, and a declared column held with more than SELECT. + h := held{ + Tables: map[table]string{{"public", "secret"}: "r", {"public", "seq"}: "S"}, + Columns: map[column][]string{ + {"public", "node", "name"}: {"SELECT"}, + {"public", "node", "role"}: {"SELECT", "UPDATE"}, + {"public", "node", "old"}: {"SELECT"}, + }, + } + decl, _ := ParseReadable(map[string]any{"readable": []any{"node(name, role)"}}) + existing := []column{{"public", "node", "name"}, {"public", "node", "role"}, {"public", "node", "old"}} + stmts, rep := planDatabase(decl, existing, h, map[string]bool{}) + want := []string{ + `REVOKE ALL ON TABLE "public"."secret" FROM "mesh_store_reader"`, + `REVOKE ALL ON SEQUENCE "public"."seq" FROM "mesh_store_reader"`, + `REVOKE ALL ("old", "role") ON "public"."node" FROM "mesh_store_reader"`, + `GRANT SELECT ("role") ON "public"."node" TO "mesh_store_reader"`, + } + if strings.Join(stmts, "\n") != strings.Join(want, "\n") { + t.Fatalf("got\n%s\nwant\n%s", strings.Join(stmts, "\n"), strings.Join(want, "\n")) + } + if len(rep.Revoked) != 4 { + t.Fatal(rep.Revoked) + } + // Undeclared: everything held goes, and nothing is granted. + stmts, _ = planDatabase(nil, nil, h, map[string]bool{}) + for _, s := range stmts { + if strings.HasPrefix(s, "GRANT") { + t.Fatal(s) + } + } + if len(stmts) != 3 { + t.Fatal(stmts) + } + // Held exactly as declared: nothing to do. + exact := held{Tables: map[table]string{}, Columns: map[column][]string{{"public", "node", "name"}: {"SELECT"}}} + decl, _ = ParseReadable(map[string]any{"readable": []any{"node(name)"}}) + if stmts, _ := planDatabase(decl, existing, exact, map[string]bool{}); len(stmts) != 0 { + t.Fatal(stmts) + } +} + +func TestASchemaTheReaderCannotUseIsGrantedUsage(t *testing.T) { + decl, _ := ParseReadable(map[string]any{"readable": []any{"audit.events"}}) + stmts, _ := planDatabase(decl, []column{{"audit", "events", "at"}}, held{Tables: map[table]string{}, Columns: map[column][]string{}}, + map[string]bool{"audit": true}) + if len(stmts) != 2 || stmts[0] != `GRANT USAGE ON SCHEMA "audit" TO "mesh_store_reader"` { + t.Fatal(stmts) + } +} + +func TestAStatementNamingASecretIsRefusedBeforeItIsSent(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + for _, c := range []struct{ db, sql string }{ + {"identity", "select private from signing_key"}, + {"identity", "SELECT * FROM Authority"}, + {"identity", `select * from "signing_key"`}, + {"inventory", "select count(*) from module_secret"}, + {"inventory", "select * from bus_membership"}, + {"inventory", "select query_to_xml('select * from secret', true, true, '')"}, + {"anything", "select operator_sealed from t"}, + {"anything", "select client_secret from t"}, + {"anything", "select * from pg_authid"}, + } { + f, cl := newFake(conn) + _, err := cl.ReadOnlyQuery(ctx, c.db, c.sql) + if err == nil || !strings.Contains(err.Error(), "issue 337") { + t.Errorf("%s in %s: %v", c.sql, c.db, err) + } + if len(f.calls) != 0 { + t.Errorf("%s in %s was sent: %v", c.sql, c.db, f.calls) + } + } + // Ordinary words pass: a table called secret elsewhere is the privileges' to refuse, not the words'. + for _, ok := range []string{"select jsonb_object_keys(x) from t", "select name, created from node", "select token_count from usage"} { + if why := refusedStatement("inventory", ok); why != "" { + t.Errorf("%s: %s", ok, why) + } + } +} + +func TestAQueryIsRefusedWhileTheReaderCanReadASecret(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + f, c := newFake(conn) + f.answer(`has_column_privilege`, []string{"nspname", "relname", "attname"}, + []string{"public", "users", "name"}, []string{"public", "users", "password_hash"}) + f.answer(`^SELECT name FROM users`, []string{"name"}, []string{"a"}) + _, err := c.ReadOnlyQuery(ctx, "app", "SELECT name FROM users") + if err == nil || !strings.Contains(err.Error(), "public.users.password_hash") || strings.Contains(err.Error(), "users.name") { + t.Fatal(err) + } + for _, k := range f.calls { + if k.SQL == "SELECT name FROM users" { + t.Fatal("the statement ran") + } + } + // The probe runs as the reader, in the database asked about. + var probed bool + for _, k := range f.calls { + if strings.Contains(k.SQL, "has_column_privilege") && k.User == Reader && k.Database == "app" { + probed = true + } + } + if !probed { + t.Fatal("not probed as the reader") + } +} + +func TestDeclarationsFromTheContributionsFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "mesh.json") + doc := `{"requirement":"postgres-database","given":[ + {"as":"letta","secret":"s","values":{"name":"letta","readable":["agents"]}}, + {"as":"gitea","secret":"s","values":{"name":"gitea"}}, + {"as":"bad","secret":"s","values":{"readable":"agents"}}]}` + if err := os.WriteFile(path, []byte(doc), 0o600); err != nil { + t.Fatal(err) + } + d, err := DeclarationsFrom(path) + if err != nil { + t.Fatal(err) + } + if len(d.Tables) != 1 || d.Tables["letta"][0].Table != "agents" || d.Malformed["bad"] == "" { + t.Fatalf("%+v", d) + } + if _, err := DeclarationsFrom(filepath.Join(dir, "absent")); err == nil { + t.Fatal("an unreadable file is said") + } +} + +func TestReconcileRevokesTheRolesTheReaderHoldsAndGrantsPerDatabase(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + f, c := newFake(conn) + f.answer(`FROM pg_auth_members`, []string{"rolname"}, []string{"pg_read_all_data"}) + f.answer(`SELECT datname FROM pg_database WHERE datallowconn`, []string{"datname"}, []string{"identity"}, []string{"letta"}) + f.answer(`aclexplode\(c\.relacl\)`, []string{"nspname", "relname", "relkind"}) + f.answer(`aclexplode\(a\.attacl\)`, []string{"nspname", "relname", "attname", "privilege_type"}) + f.answer(`IN \(\('public', 'agents'\)\)`, []string{"nspname", "relname", "attname"}, + []string{"public", "agents", "id"}, []string{"public", "agents", "api_key"}) + f.answer(`has_schema_privilege`, []string{"nspname"}) + rep := c.ReconcileReader(ctx, Declarations{Tables: map[string][]ReadableTable{"letta": {{Schema: "public", Table: "agents"}}}}, "") + if rep.Problem != "" || len(rep.Databases) != 2 { + t.Fatalf("%+v", rep) + } + var revoked, granted bool + for _, k := range f.calls { + if k.SQL == `REVOKE "pg_read_all_data" FROM "mesh_store_reader"` && k.Database == "postgres" { + revoked = true + } + if k.SQL == `GRANT SELECT ("id") ON "public"."agents" TO "mesh_store_reader"` && k.Database == "letta" { + granted = true + } + if strings.Contains(k.SQL, "GRANT") && k.Database == "identity" { + t.Fatal("granted in an undeclared database:", k.SQL) + } + if strings.Contains(k.SQL, "GRANT") && strings.Contains(k.SQL, "api_key") { + t.Fatal("granted a secret:", k.SQL) + } + } + if !revoked || !granted { + t.Fatalf("revoked %v, granted %v: %v", revoked, granted, f.calls) + } + if last := c.LastReaderReport(); len(last.Databases) != 2 || last.Databases[1].Refused == nil { + t.Fatalf("%+v", last) + } +} + +// Every module in the catalogue that declares what the reader may read declares it in a shape the +// provider reads: a typo is found here, not as a refusal on a live store. +func TestTheCataloguesReadableDeclarationsParse(t *testing.T) { + manifests, _ := filepath.Glob("../../../*/module.json") + if len(manifests) == 0 { + t.Skip("not in the catalogue") + } + for _, path := range manifests { + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var m struct { + Contributes map[string]json.RawMessage `json:"contributes"` + } + if json.Unmarshal(raw, &m) != nil { + continue + } + var values map[string]any + if json.Unmarshal(m.Contributes["postgres-database"], &values) != nil { + continue + } + decl, err := ParseReadable(values) + if err != nil { + t.Errorf("%s: %v", path, err) + } + for _, d := range decl { + if SecretName(d.Table) { + t.Errorf("%s declares %s readable, whose name says it holds secrets; it would never be granted", path, d) + } + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/tools.go b/modules/postgres/cmd/postgres-provider/tools.go index 901759ac..1b7c9f83 100644 --- a/modules/postgres/cmd/postgres-provider/tools.go +++ b/modules/postgres/cmd/postgres-provider/tools.go @@ -65,7 +65,17 @@ func Tools(pg *Client) []stdio.Tool { return []stdio.Tool{ listTool(pg, "postgres_list_databases", "List the databases on the postgres server, with their on-disk size."), queryTool(pg, "postgres_query", "postgres_query", - "Run a read-only SQL query against a named database, as a login that can read every table and change nothing."), + "Run a read-only SQL query against a named database, as a login that changes nothing and reads only the "+ + "columns the database's module declares readable — never a key, secret, token or password (hq issue 337). "+ + "postgres_reader_grants says what it may read."), + { + Name: "postgres_reader_grants", + Description: "What the store's reader — the login every query runs as — may read in each database, as the last " + + "pass made it: the columns each module declares readable, what was refused because its name says it holds a " + + "secret, what was declared but is missing, what was revoked, and any secret column it can still read " + + "(exposed; queries there are then refused). Names only, never values. (r)", + Run: func(map[string]any) (any, error) { return pg.LastReaderReport(), nil }, + }, { Name: "postgres_retire_database", Description: "Take one database out of service on purpose: rename it to _deleted_ and lock its owner's login. " + @@ -93,6 +103,7 @@ func Tools(pg *Client) []stdio.Tool { }, }, listTool(pg, Seat+".databases", "Every database the store holds, with its on-disk size."), - queryTool(pg, Seat+".query", "query", "One read-only statement against one database the store holds."), + queryTool(pg, Seat+".query", "query", "One read-only statement against one database the store holds, reading only what that database's module "+ + "declares readable and never a key, secret, token or password (hq issue 337)."), } } From fb07d552a050b57548817543578605d78ec39452 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 21:44:36 +0200 Subject: [PATCH 2/4] Name the licences store's sealed tables as they are called, so a statement naming one is refused --- modules/postgres/cmd/postgres-provider/readable.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go index 787e1687..2f04cd96 100644 --- a/modules/postgres/cmd/postgres-provider/readable.go +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -122,7 +122,7 @@ func SecretName(name string) bool { return secretName.MatchString(name) } var knownSecretTables = map[string][]string{ "identity": {"signing_key", "authority", "node_key"}, "inventory": {"secret", "module_secret", "operator_key", "bus_membership", "bus_user", "enrolment_token"}, - "licences": {"licence_holder", "licence_manager"}, + "licences": {"licence_holder", "refresh_grant"}, } // statementWord is what the statement guard reads: every word of the statement, literals included, so From 8bf50f75557ae3cad1b9f5143ab7f76318367cfd Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 22:01:43 +0200 Subject: [PATCH 3/4] Revoke the reader's grants as their own grantors and read them back, alter its role only on drift, and report definer functions and views (review of hq issue 337) A superuser's REVOKE acts as the table owner, so a grant to the reader made by a role holding GRANT OPTION survived it while the report said revoked. Each grant is now revoked under SET ROLE to its grantor, the grants are read back, and anything still held is reported as a failure. The role was altered every minute with its password in the statement, which a server logging DDL would record and which could collide with a first query; it is now altered only when an attribute, a setting or the password has drifted, one ensure at a time. SECURITY DEFINER functions and views the reader may use reach data without a grant on it, so every pass lists them. The README says the name filter is a backstop and that the read-only session default can be turned off by a statement. --- modules/postgres/README.md | 28 +- .../postgres/cmd/postgres-provider/client.go | 68 ++++- .../cmd/postgres-provider/readable.go | 270 +++++++++++++----- .../postgres-provider/readable_live_test.go | 70 +++++ .../cmd/postgres-provider/readable_test.go | 84 +++++- .../postgres/cmd/postgres-provider/tools.go | 3 +- 6 files changed, 431 insertions(+), 92 deletions(-) diff --git a/modules/postgres/README.md b/modules/postgres/README.md index c5dbcef8..7593f125 100644 --- a/modules/postgres/README.md +++ b/modules/postgres/README.md @@ -47,8 +47,10 @@ for, so one removed by hand is installed again. - **`postgres_retire_database` renames, it does not drop**: the database becomes `_deleted_` and its owner is locked. Removing the data is a person's act, by hand. - **A caller's statement never runs as the superuser.** `postgres_query` and the seat's `query` verb - run as `mesh_store_reader` — every session read-only by the server's own setting, a 60 s statement - timeout — with the statement sent as given (issue 193). Without the reader's password + run as `mesh_store_reader` — every session read-only by default, a 60 s statement timeout — with the + statement sent as given (issue 193). The default is only a default: a statement may + `SET transaction_read_only = off`. What stops the reader writing is that it holds no grant to write + (and, beyond that, only the `TEMP` privilege PostgreSQL gives every login, for temporary tables). Without the reader's password (`own-secrets.reader`) the call is refused. - **The reader reads only what a module declares readable, and never a secret** (issue 337). It holds no role — until issue 337 it held `pg_read_all_data`, which let any caller of the seat's `query` read @@ -60,13 +62,25 @@ for, so one removed by hand is installed again. An entry is `[schema.]table` (every column it has, at each pass) or `[schema.]table(column, …)`. A database no contribution names — the controller's own among them — gives the reader nothing. A column or table whose name says it holds a secret (private, secret, sealed, token, password, credential, salt, - a key) is never granted, declared or not, and the refusal is reported. Every minute, on every - database, the reader's role memberships are revoked and its grants brought to exactly that, so an - existing install is narrowed without anybody acting and a narrowed declaration takes effect at the - next pass. Before a caller's statement runs, the reader's own session is asked which secret-named + a key) is never granted, declared or not, and the refusal is reported. **That name check is a + backstop, not the guard:** a module that declares a table whose secret sits under a plain column name + (`value`, `data`, `blob`) has declared that secret readable, and the review of its declaration is + what stands in the way. Declare columns by name where a table mixes the two. + Every minute, on every database, the reader's role memberships are revoked and its grants brought to + exactly that — each grant revoked as its own grantor, since a superuser's revoke acts as the table's + owner and leaves a grant another role made — and then read back: anything the reader still holds that + is not declared is reported as a failure, never as revoked. An existing install is narrowed without + anybody acting, and a narrowed declaration takes effect at the next pass. The role itself is altered + only when an attribute, a setting or its password has drifted, so the password is not sent every + minute. Before a caller's statement runs, the reader's own session is asked which secret-named columns it can read (a grant to `PUBLIC` or by hand would show there); if any, the statement is refused and the columns named. A statement that names a known secret table of the controller's, or a secret-sounding name, is refused before it is sent — a second layer, not the guard. +- **What no grant shows.** A `SECURITY DEFINER` function the reader may execute (PUBLIC may execute a new + function unless revoked) runs with its owner's rights, and a view the reader may read reads with its + owner's. Neither needs a grant on the table behind it, so neither the reconcile nor the probe can see + what it returns. Each pass lists both for every database, in `postgres_reader_grants` and once in the + log; they are reported, not refused. ## Tools @@ -76,7 +90,7 @@ for, so one removed by hand is installed again. | `postgres_query` `{database, sql}` | one read-only statement, as the reader; rows keyed by column, `NULL` as null | | `postgres_retire_database` `{database, confirm}` | renames a database aside and locks its owner; `confirm` repeats the name | | `mesh-store.databases`, `mesh-store.query` | the store seat's verbs: the same listing and read-only query | -| `postgres_reader_grants` | what the reader may read in each database as the last pass left it: readable columns, refused secrets, declared but missing, revoked, and any secret it can still read; names only | +| `postgres_reader_grants` | what the reader may read in each database as the last pass left it: readable columns, refused secrets, declared but missing, revoked, any secret it can still read, and the definer functions and views it may use; names only | | `provisioner_retirement` | what is held, what waits for a person, what was rejected, every retired consumer and set-aside database with when, why and size | | `provisioner_retire_approve`, `provisioner_retire_reject` `{consumers, why, by}` | a person's answer to a set waiting; through the controller's `retire approve|reject` | | `provisioner_delete` `{consumer, confirm, why, by}` | drops one retired consumer's database and role, or one set-aside database; through the controller's `cleanup delete` | diff --git a/modules/postgres/cmd/postgres-provider/client.go b/modules/postgres/cmd/postgres-provider/client.go index 2d31a6fc..895e57eb 100644 --- a/modules/postgres/cmd/postgres-provider/client.go +++ b/modules/postgres/cmd/postgres-provider/client.go @@ -24,10 +24,12 @@ import ( "github.com/jackc/pgx/v5/pgconn" ) -// Reader is the login a caller's statement runs as (novox/hq issue 193). It changes nothing — every -// transaction it opens is read-only by the server's own setting — and it reads only the columns a -// consuming module declares readable, never one that holds a secret (readable.go, novox/hq issue 337). -// It holds no role: until issue 337 it held `pg_read_all_data`, and with it the controller's signing key. A statement cannot climb out of a login the way it can out of a +// Reader is the login a caller's statement runs as (novox/hq issue 193). It changes nothing because it +// holds no grant to write: its sessions are read-only by default, but a statement may turn that off +// (SET transaction_read_only = off), so the default is a convenience and the missing grants are the +// guard. It reads only the columns a consuming module declares readable, never one that holds a +// secret (readable.go, novox/hq issue 337). It holds no role: until issue 337 it held +// `pg_read_all_data`, and with it the controller's signing key. A statement cannot climb out of a login the way it can out of a // transaction wrapped around it as text: `COMMIT; DROP …` ended the old wrapper and ran the rest as // the superuser, and even one read-only statement as a superuser can run a program on the server. const Reader = "mesh_store_reader" @@ -104,6 +106,7 @@ type Client struct { readerMu sync.Mutex readerReady bool + roleMu sync.Mutex // reader is what the last reconcile of the reader's grants found (readable.go). reader readerState @@ -506,13 +509,21 @@ func (c *Client) ensureReaderRole(ctx context.Context) error { if c.conn.ReaderPassword == "" { return errReaderMissing } - has, err := c.exists(ctx, "SELECT 1 FROM pg_roles WHERE rolname = "+Literal(Reader)) + // One at a time: the reconcile loop and a first query may both ask, and two ALTER ROLEs at once + // fail with "tuple concurrently updated". + c.roleMu.Lock() + defer c.roleMu.Unlock() + has, drifted, err := c.readerDrifted(ctx) if err != nil { return err } - for _, sql := range ReaderStatements(has, c.conn.ReaderPassword) { - if _, err := c.Query(ctx, "", sql); err != nil { - return err + // The role is altered only when it has drifted: a statement carrying the password is sent as + // seldom as possible, since a server logging DDL would write it down. + if !has || drifted { + for _, sql := range ReaderStatements(has, c.conn.ReaderPassword) { + if _, err := c.Query(ctx, "", sql); err != nil { + return err + } } } r, err := c.Query(ctx, "", "SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON r.oid = m.roleid "+ @@ -530,6 +541,47 @@ func (c *Client) ensureReaderRole(ctx context.Context) error { return nil } +// readerAttributes is what the reader's role must say, in the order readerDrifted asks for them: +// not a superuser, cannot make databases or roles, replicate or bypass row security; inherits; logs in; +// its password never expires. +var readerAttributes = []string{"f", "f", "f", "f", "f", "t", "t", "t"} + +// readerDrifted says whether the reader's role exists, and whether it differs from what +// ReaderStatements makes: an attribute, a session setting, or a password that no longer logs in. +func (c *Client) readerDrifted(ctx context.Context) (exists, drifted bool, err error) { + r, err := c.Query(ctx, "", "SELECT rolsuper, rolcreatedb, rolcreaterole, rolreplication, rolbypassrls, rolinherit, rolcanlogin, "+ + "COALESCE(rolvaliduntil = 'infinity', false), COALESCE(array_to_string(rolconfig, ','), '') FROM pg_roles WHERE rolname = "+Literal(Reader)) + if err != nil { + return false, false, err + } + if len(r.Rows) == 0 { + return false, false, nil + } + row := r.Rows[0] + for i, want := range readerAttributes { + if cell(row, i) != want { + return true, true, nil + } + } + config := map[string]bool{} + for _, kv := range strings.Split(cell(row, len(readerAttributes)), ",") { + config[kv] = true + } + if !config["default_transaction_read_only=on"] || !config["statement_timeout=60s"] { + return true, true, nil + } + // The password: asked by logging in with it, which is the only way to know without reading a hash. + s, err := c.dial(ctx, Login{Database: "postgres", User: Reader, Password: c.conn.ReaderPassword, Options: readerOptions}) + if err != nil { + if IsLost(err) { + return true, true, nil + } + return true, false, err + } + s.Close() + return true, false, nil +} + var firstWord = regexp.MustCompile(`^\s*([A-Za-z]+)`) // ReadOnlyQuery runs a caller's statement against a named database as the read-only login, for the diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go index 2f04cd96..aa284f1f 100644 --- a/modules/postgres/cmd/postgres-provider/readable.go +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -185,14 +185,26 @@ type column struct{ Schema, Table, Name string } type table struct{ Schema, Table string } -// held is what the reader holds in one database, read from the catalogue as the admin. -type held struct { - // Tables are those the reader holds any table-level privilege on, with their kind ('S': sequence). - Tables map[table]string - // Columns are the column privileges it holds, each column's privilege types. - Columns map[column][]string +// grant is one privilege the reader holds, and who granted it. +type grant struct{ Privilege, Grantor string } + +// tableGrants is a table the reader holds table-level privileges on: its kind ('S': sequence) and every +// privilege with its grantor. +type tableGrants struct { + Kind string + Grants []grant } +// held is what the reader holds in one database, read from the catalogue as the admin, with grantors: +// a grant is only taken back by its own grantor. A superuser's REVOKE acts as the table's owner, so a +// grant made by a role holding GRANT OPTION survives it (the review of novox/hq issue 337). +type held struct { + Tables map[table]tableGrants + Columns map[column][]grant +} + +func newHeld() held { return held{Tables: map[table]tableGrants{}, Columns: map[column][]grant{}} } + // DatabaseReport is what the reader may read in one database, and what was refused or missing. type DatabaseReport struct { Database string `json:"database"` @@ -202,14 +214,16 @@ type DatabaseReport struct { Missing []string `json:"missing,omitempty"` Revoked []string `json:"revoked,omitempty"` Exposed []string `json:"exposed,omitempty"` + // Definers and Views reach data without a grant on it: a SECURITY DEFINER function the reader may + // execute runs with its owner's rights, and a view it may read reads with its owner's. Reported, not + // refused: the name check cannot see what either returns. + Definers []string `json:"definers,omitempty"` + Views []string `json:"views,omitempty"` Error string `json:"error,omitempty"` } -// planDatabase is the DDL that brings the reader's grants in one database to exactly what is declared, -// minus every secret: revoke what is held and not wanted, grant what is wanted and not held. -// existing is every column of every declared table that the database has. Pure; the tests' subject. -func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[string]bool) ([]string, DatabaseReport) { - var rep DatabaseReport +// wantedColumns is every declared column that exists and is not a secret, with what was refused or missing. +func wantedColumns(decl []ReadableTable, existing []column, rep *DatabaseReport) map[column]bool { byTable := map[table][]string{} for _, c := range existing { t := table{c.Schema, c.Table} @@ -252,51 +266,102 @@ func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[s want[column{d.Schema, d.Table, c}] = true } } - // Every table-level privilege is revoked: the reader is only ever given columns. REVOKE on a table - // takes its column privileges with it, so those are granted again below if wanted. - var stmts []string - cleared := map[table]bool{} + return want +} + +// unwanted is what the reader holds beyond the wanted columns' SELECT: every table-level privilege, +// and every column privilege that is not SELECT on a wanted column. Each as "schema.table[.column]". +func unwanted(h held, want map[column]bool) []string { + var out []string + for t := range h.Tables { + out = append(out, t.Schema+"."+t.Table) + } + for c, grants := range h.Columns { + for _, g := range grants { + if !want[c] || g.Privilege != "SELECT" { + out = append(out, c.Schema+"."+c.Table+"."+c.Name) + break + } + } + } + sort.Strings(out) + return out +} + +// planDatabase is the DDL that brings the reader's grants in one database to exactly what is declared, +// minus every secret: revoke what is held and not wanted — as each grant's own grantor — and grant what +// is wanted and not held. existing is every column of every declared table that the database has. +// Pure; the tests' subject. +func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[string]bool) ([]string, DatabaseReport) { + var rep DatabaseReport + want := wantedColumns(decl, existing, &rep) + + // Revokes, grouped by grantor: SET ROLE to it, so the REVOKE is that grantor's and takes its grant. + byGrantor := map[string][]string{} + touched := map[table]bool{} for _, t := range sortedTables(h.Tables) { kind := "TABLE" - if h.Tables[t] == "S" { + if h.Tables[t].Kind == "S" { kind = "SEQUENCE" } - stmts = append(stmts, fmt.Sprintf("REVOKE ALL ON %s %s.%s FROM %s", kind, Ident(t.Schema), Ident(t.Table), Ident(Reader))) + for _, g := range grantorsOf(h.Tables[t].Grants) { + byGrantor[g] = append(byGrantor[g], fmt.Sprintf("REVOKE ALL ON %s %s.%s FROM %s", kind, Ident(t.Schema), Ident(t.Table), Ident(Reader))) + } rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+" (every privilege on the table)") - cleared[t] = true + touched[t] = true } - have := map[column]bool{} - unwanted := map[table][]string{} - for c, privs := range h.Columns { - if cleared[table{c.Schema, c.Table}] { + type colKey struct { + t table + grantor string + } + cols := map[colKey][]string{} + for c, grants := range h.Columns { + bad := false + for _, g := range grants { + if !want[c] || g.Privilege != "SELECT" { + bad = true + } + } + if !bad { continue } - onlySelect := len(privs) == 1 && privs[0] == "SELECT" - if want[c] && onlySelect { - have[c] = true - continue - } - // Not wanted, or held with more than SELECT: revoked, and granted SELECT again below if wanted. + // Not wanted, or held with more than SELECT: every grantor's grant of it is taken back, and + // SELECT granted again below if it is wanted. t := table{c.Schema, c.Table} - unwanted[t] = append(unwanted[t], c.Name) - } - for _, t := range sortedKeys(unwanted) { - cols := unwanted[t] - sort.Strings(cols) - stmts = append(stmts, fmt.Sprintf("REVOKE ALL (%s) ON %s.%s FROM %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) - for _, c := range cols { - rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+"."+c) + for _, g := range grantorsOf(grants) { + k := colKey{t, g} + cols[k] = append(cols[k], c.Name) } + touched[t] = true + rep.Revoked = append(rep.Revoked, c.Schema+"."+c.Table+"."+c.Name) } - grant := map[table][]string{} + for k, names := range cols { + sort.Strings(names) + byGrantor[k.grantor] = append(byGrantor[k.grantor], + fmt.Sprintf("REVOKE ALL (%s) ON %s.%s FROM %s", idents(names), Ident(k.t.Schema), Ident(k.t.Table), Ident(Reader))) + } + var stmts []string + for _, g := range sortedGrantors(byGrantor) { + revokes := byGrantor[g] + sort.Strings(revokes) + stmts = append(stmts, "SET ROLE "+Ident(g)) + stmts = append(stmts, revokes...) + stmts = append(stmts, "RESET ROLE") + } + sort.Strings(rep.Revoked) + + // A wanted column is granted unless it is held with SELECT alone on a table nothing was revoked on; + // granting SELECT that is held already changes nothing. + grantCols := map[table][]string{} schemas := map[string]bool{} for c := range want { rep.Readable = append(rep.Readable, c.Schema+"."+c.Table+"."+c.Name) - if !have[c] { - t := table{c.Schema, c.Table} - grant[t] = append(grant[t], c.Name) - schemas[c.Schema] = true + t := table{c.Schema, c.Table} + if len(h.Columns[c]) > 0 && !touched[t] { + continue } + grantCols[t] = append(grantCols[t], c.Name) + schemas[c.Schema] = true } sort.Strings(rep.Readable) for _, s := range sortedSet(schemas) { @@ -304,14 +369,36 @@ func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[s stmts = append(stmts, fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", Ident(s), Ident(Reader))) } } - for _, t := range sortedKeys(grant) { - cols := grant[t] - sort.Strings(cols) - stmts = append(stmts, fmt.Sprintf("GRANT SELECT (%s) ON %s.%s TO %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) + for _, t := range sortedKeys(grantCols) { + names := grantCols[t] + sort.Strings(names) + stmts = append(stmts, fmt.Sprintf("GRANT SELECT (%s) ON %s.%s TO %s", idents(names), Ident(t.Schema), Ident(t.Table), Ident(Reader))) } return stmts, rep } +func grantorsOf(grants []grant) []string { + seen := map[string]bool{} + var out []string + for _, g := range grants { + if !seen[g.Grantor] { + seen[g.Grantor] = true + out = append(out, g.Grantor) + } + } + sort.Strings(out) + return out +} + +func sortedGrantors(m map[string][]string) []string { + out := make([]string, 0, len(m)) + for g := range m { + out = append(out, g) + } + sort.Strings(out) + return out +} + func idents(names []string) string { out := make([]string, len(names)) for i, n := range names { @@ -320,7 +407,7 @@ func idents(names []string) string { return strings.Join(out, ", ") } -func sortedTables(m map[table]string) []table { +func sortedTables(m map[table]tableGrants) []table { out := make([]table, 0, len(m)) for t := range m { out = append(out, t) @@ -456,31 +543,47 @@ func (c *Client) LastReaderReport() ReaderReport { return c.reader.last } +// readHeld is every privilege the reader holds in one database, table-level and column-level, each +// with its grantor. +func (c *Client) readHeld(ctx context.Context, db string) (held, error) { + h := newHeld() + readerOID := "(SELECT oid FROM pg_catalog.pg_roles WHERE rolname = " + Literal(Reader) + ")" + r, err := c.Query(ctx, db, "SELECT n.nspname, c.relname, c.relkind::text, x.privilege_type, pg_catalog.pg_get_userbyid(x.grantor) "+ + "FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "CROSS JOIN LATERAL pg_catalog.aclexplode(c.relacl) x WHERE x.grantee = "+readerOID) + if err != nil { + return h, err + } + for _, row := range r.Rows { + t := table{cell(row, 0), cell(row, 1)} + tg := h.Tables[t] + tg.Kind = cell(row, 2) + tg.Grants = append(tg.Grants, grant{cell(row, 3), cell(row, 4)}) + h.Tables[t] = tg + } + r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname, x.privilege_type, pg_catalog.pg_get_userbyid(x.grantor) "+ + "FROM pg_catalog.pg_attribute a JOIN pg_catalog.pg_class c ON c.oid = a.attrelid "+ + "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "CROSS JOIN LATERAL pg_catalog.aclexplode(a.attacl) x WHERE x.grantee = "+readerOID) + if err != nil { + return h, err + } + for _, row := range r.Rows { + k := column{cell(row, 0), cell(row, 1), cell(row, 2)} + h.Columns[k] = append(h.Columns[k], grant{cell(row, 3), cell(row, 4)}) + } + return h, nil +} + func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []ReadableTable) DatabaseReport { fail := func(err error) DatabaseReport { return DatabaseReport{Database: db, Declared: decl != nil, Error: err.Error()} } - h := held{Tables: map[table]string{}, Columns: map[column][]string{}} - readerOID := "(SELECT oid FROM pg_catalog.pg_roles WHERE rolname = " + Literal(Reader) + ")" - r, err := c.Query(ctx, db, "SELECT n.nspname, c.relname, c.relkind::text FROM pg_catalog.pg_class c "+ - "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace CROSS JOIN LATERAL pg_catalog.aclexplode(c.relacl) x "+ - "WHERE x.grantee = "+readerOID+" GROUP BY 1, 2, 3") + h, err := c.readHeld(ctx, db) if err != nil { return fail(err) } - for _, row := range r.Rows { - h.Tables[table{cell(row, 0), cell(row, 1)}] = cell(row, 2) - } - r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname, x.privilege_type FROM pg_catalog.pg_attribute a "+ - "JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ - "CROSS JOIN LATERAL pg_catalog.aclexplode(a.attacl) x WHERE x.grantee = "+readerOID) - if err != nil { - return fail(err) - } - for _, row := range r.Rows { - k := column{cell(row, 0), cell(row, 1), cell(row, 2)} - h.Columns[k] = append(h.Columns[k], cell(row, 3)) - } + var r Result var existing []column noUsage := map[string]bool{} if len(decl) > 0 { @@ -515,6 +618,16 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab rep.Error = "changing the reader's grants: " + err.Error() return rep } + // Read back: what the reader still holds beyond what is wanted is a failure, said as one — + // never reported as revoked while it stands. + after, err := c.readHeld(ctx, db) + if err != nil { + rep.Error = "reading the reader's grants back: " + err.Error() + return rep + } + if left := unwanted(after, wantedColumns(decl, existing, &DatabaseReport{})); len(left) > 0 { + rep.Error = "STILL HELD after revoking: the reader can still use " + strings.Join(left, ", ") + } } // Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a // grant made by hand) is found here, said, and makes the reader's statements against it refused. @@ -524,6 +637,27 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab return rep } rep.Exposed = Exposed(r) + // What reaches data without a grant on it, which the probe above cannot see: SECURITY DEFINER + // functions the reader may execute (PUBLIC has EXECUTE on a new function), and views it may read. + who := Literal(Reader) + if r, err = c.Query(ctx, db, "SELECT n.nspname || '.' || p.proname || '(' || pg_catalog.pg_get_function_identity_arguments(p.oid) || ')' "+ + "FROM pg_catalog.pg_proc p JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace WHERE p.prosecdef "+ + "AND n.nspname NOT IN ('pg_catalog','information_schema') AND pg_catalog.has_function_privilege("+who+", p.oid, 'EXECUTE') ORDER BY 1"); err != nil { + rep.Error = "listing the definer functions the reader may run: " + err.Error() + return rep + } + for _, row := range r.Rows { + rep.Definers = append(rep.Definers, cell(row, 0)) + } + if r, err = c.Query(ctx, db, "SELECT n.nspname || '.' || c.relname FROM pg_catalog.pg_class c "+ + "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relkind IN ('v','m') "+ + "AND n.nspname NOT IN ('pg_catalog','information_schema') AND pg_catalog.has_any_column_privilege("+who+", c.oid, 'SELECT') ORDER BY 1"); err != nil { + rep.Error = "listing the views the reader may read: " + err.Error() + return rep + } + for _, row := range r.Rows { + rep.Views = append(rep.Views, cell(row, 0)) + } return rep } @@ -550,6 +684,14 @@ func (c *Client) RunReader(ctx context.Context, receives string, every time.Dura if db.Error != "" { lines = append(lines, fmt.Sprintf("reader: %s: %s", db.Database, db.Error)) } + if len(db.Definers) > 0 { + lines = append(lines, fmt.Sprintf("reader: %s: the reader may run SECURITY DEFINER function(s) %s, which read with their "+ + "owner's rights; what they return is not checked", db.Database, strings.Join(db.Definers, ", "))) + } + if len(db.Views) > 0 { + lines = append(lines, fmt.Sprintf("reader: %s: the reader may read view(s) %s, which read with their owner's rights", + db.Database, strings.Join(db.Views, ", "))) + } if len(db.Refused) > 0 { lines = append(lines, fmt.Sprintf("reader: %s: not granted, %s", db.Database, strings.Join(db.Refused, "; "))) } diff --git a/modules/postgres/cmd/postgres-provider/readable_live_test.go b/modules/postgres/cmd/postgres-provider/readable_live_test.go index 8db18b45..ebef9714 100644 --- a/modules/postgres/cmd/postgres-provider/readable_live_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_live_test.go @@ -111,3 +111,73 @@ func TestLiveTheReaderReadsWhatIsDeclaredAndNoSecret(t *testing.T) { t.Fatal("a query ran while the reader can read a secret:", err) } } + +// A grant to the reader made by a role holding GRANT OPTION — not the owner — is taken back as that +// role, and what still stands after a pass is a failure, not "revoked". SECURITY DEFINER functions and +// views the reader may use are reported. +func TestLiveAGrantByAnotherGrantorIsTakenBackAndDefinersAreSaid(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + for _, sql := range []string{ + "DROP ROLE IF EXISTS mesh_test_owner", "DROP ROLE IF EXISTS mesh_test_middle", + } { + if _, err := c.Query(ctx, "", sql); err != nil { + t.Fatal(sql, err) + } + } + defer func() { + _, _ = c.Query(ctx, "", "DROP DATABASE IF EXISTS "+Ident(liveStore)+" WITH (FORCE)") + _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_middle") + _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_owner") + }() + for _, sql := range []string{ + "CREATE ROLE mesh_test_owner", "CREATE ROLE mesh_test_middle", + } { + if _, err := c.Query(ctx, "", sql); err != nil { + t.Fatal(sql, err) + } + } + for _, sql := range []string{ + "ALTER TABLE notes OWNER TO mesh_test_owner", + "GRANT SELECT ON notes TO mesh_test_middle WITH GRANT OPTION", + "SET ROLE mesh_test_middle; GRANT SELECT ON notes TO " + Ident(Reader) + "; GRANT SELECT (body) ON notes TO " + Ident(Reader) + "; RESET ROLE", + "CREATE FUNCTION peek() RETURNS bytea LANGUAGE sql SECURITY DEFINER AS 'SELECT private FROM vault LIMIT 1'", + "CREATE VIEW plain_names AS SELECT name FROM vault", + "GRANT SELECT ON plain_names TO " + Ident(Reader), + } { + if _, err := c.Query(ctx, liveStore, sql); err != nil { + t.Fatal(sql, err) + } + } + var mine DatabaseReport + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore { + mine = db + } + } + if mine.Error != "" { + t.Fatal("left standing:", mine.Error) + } + if columnReadable(t, c, liveStore, "notes", "body") { + t.Fatal("the other grantor's grant survived") + } + if strings.Join(mine.Definers, ",") != "public.peek()" || strings.Join(mine.Views, ",") != "" { + t.Fatalf("definers %v, views %v", mine.Definers, mine.Views) + } + // A second pass finds nothing to take back. + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && (len(db.Revoked) > 0 || db.Error != "") { + t.Fatalf("%+v", db) + } + } + // A view granted to PUBLIC is said. + if _, err := c.Query(ctx, liveStore, "GRANT SELECT ON plain_names TO PUBLIC"); err != nil { + t.Fatal(err) + } + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && strings.Join(db.Views, ",") != "public.plain_names" { + t.Fatalf("views %v", db.Views) + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_test.go b/modules/postgres/cmd/postgres-provider/readable_test.go index ebebbd6b..28f81095 100644 --- a/modules/postgres/cmd/postgres-provider/readable_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_test.go @@ -70,7 +70,7 @@ func TestTheReaderIsGivenOnlyWhatIsDeclaredAndNeverASecret(t *testing.T) { {"public", "signing_key", "public"}, {"public", "signing_key", "private"}, {"public", "build", "id"}, {"public", "build", "sealed"}, {"public", "build", "log"}, } - stmts, rep := planDatabase(decl, existing, held{Tables: map[table]string{}, Columns: map[column][]string{}}, map[string]bool{}) + stmts, rep := planDatabase(decl, existing, newHeld(), map[string]bool{}) sql := strings.Join(stmts, "\n") if !strings.Contains(sql, `GRANT SELECT ("name", "role") ON "public"."node" TO "mesh_store_reader"`) || !strings.Contains(sql, `GRANT SELECT ("id") ON "public"."build" TO "mesh_store_reader"`) { @@ -92,22 +92,28 @@ func TestTheReaderIsGivenOnlyWhatIsDeclaredAndNeverASecret(t *testing.T) { func TestWhatTheReaderHoldsAndIsNotDeclaredIsRevoked(t *testing.T) { // An install before issue 337, or a grant by hand: a table-level grant, a column no longer // declared, and a declared column held with more than SELECT. + sel := func(by string) grant { return grant{"SELECT", by} } h := held{ - Tables: map[table]string{{"public", "secret"}: "r", {"public", "seq"}: "S"}, - Columns: map[column][]string{ - {"public", "node", "name"}: {"SELECT"}, - {"public", "node", "role"}: {"SELECT", "UPDATE"}, - {"public", "node", "old"}: {"SELECT"}, + Tables: map[table]tableGrants{ + {"public", "secret"}: {Kind: "r", Grants: []grant{sel("postgres")}}, + {"public", "seq"}: {Kind: "S", Grants: []grant{{"USAGE", "postgres"}}}, + }, + Columns: map[column][]grant{ + {"public", "node", "name"}: {sel("postgres")}, + {"public", "node", "role"}: {sel("postgres"), {"UPDATE", "postgres"}}, + {"public", "node", "old"}: {sel("postgres")}, }, } decl, _ := ParseReadable(map[string]any{"readable": []any{"node(name, role)"}}) existing := []column{{"public", "node", "name"}, {"public", "node", "role"}, {"public", "node", "old"}} stmts, rep := planDatabase(decl, existing, h, map[string]bool{}) want := []string{ - `REVOKE ALL ON TABLE "public"."secret" FROM "mesh_store_reader"`, - `REVOKE ALL ON SEQUENCE "public"."seq" FROM "mesh_store_reader"`, + `SET ROLE "postgres"`, `REVOKE ALL ("old", "role") ON "public"."node" FROM "mesh_store_reader"`, - `GRANT SELECT ("role") ON "public"."node" TO "mesh_store_reader"`, + `REVOKE ALL ON SEQUENCE "public"."seq" FROM "mesh_store_reader"`, + `REVOKE ALL ON TABLE "public"."secret" FROM "mesh_store_reader"`, + `RESET ROLE`, + `GRANT SELECT ("name", "role") ON "public"."node" TO "mesh_store_reader"`, } if strings.Join(stmts, "\n") != strings.Join(want, "\n") { t.Fatalf("got\n%s\nwant\n%s", strings.Join(stmts, "\n"), strings.Join(want, "\n")) @@ -122,20 +128,74 @@ func TestWhatTheReaderHoldsAndIsNotDeclaredIsRevoked(t *testing.T) { t.Fatal(s) } } - if len(stmts) != 3 { + if len(stmts) != 5 { t.Fatal(stmts) } // Held exactly as declared: nothing to do. - exact := held{Tables: map[table]string{}, Columns: map[column][]string{{"public", "node", "name"}: {"SELECT"}}} + exact := held{Tables: map[table]tableGrants{}, Columns: map[column][]grant{{"public", "node", "name"}: {sel("postgres")}}} decl, _ = ParseReadable(map[string]any{"readable": []any{"node(name)"}}) if stmts, _ := planDatabase(decl, existing, exact, map[string]bool{}); len(stmts) != 0 { t.Fatal(stmts) } } +// A superuser's REVOKE acts as the table's owner, so a grant to the reader made by another role +// holding GRANT OPTION survives it. Each grant is revoked as its own grantor. +func TestAGrantIsRevokedAsItsOwnGrantor(t *testing.T) { + h := held{ + Tables: map[table]tableGrants{{"public", "vault"}: {Kind: "r", Grants: []grant{{"SELECT", "middle"}, {"SELECT", "owner1"}}}}, + Columns: map[column][]grant{{"public", "vault", "name"}: {{"SELECT", "middle"}}}, + } + stmts, _ := planDatabase(nil, nil, h, map[string]bool{}) + want := []string{ + `SET ROLE "middle"`, + `REVOKE ALL ("name") ON "public"."vault" FROM "mesh_store_reader"`, + `REVOKE ALL ON TABLE "public"."vault" FROM "mesh_store_reader"`, + `RESET ROLE`, + `SET ROLE "owner1"`, + `REVOKE ALL ON TABLE "public"."vault" FROM "mesh_store_reader"`, + `RESET ROLE`, + } + if strings.Join(stmts, "\n") != strings.Join(want, "\n") { + t.Fatalf("got\n%s\nwant\n%s", strings.Join(stmts, "\n"), strings.Join(want, "\n")) + } + if left := unwanted(h, nil); strings.Join(left, ",") != "public.vault,public.vault.name" { + t.Fatal(left) + } +} + +// The role is altered only when it drifted: the statement carries the password. +func TestTheReadersRoleIsAlteredOnlyWhenItDrifted(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + for _, c := range []struct { + row []string + alter bool + }{ + {[]string{"f", "f", "f", "f", "f", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}, false}, + {[]string{"f", "f", "f", "f", "t", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}, true}, + {[]string{"f", "f", "f", "f", "f", "t", "t", "t", "statement_timeout=60s"}, true}, + } { + f, cl := newFake(conn) + f.answer(`^SELECT rolsuper`, []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"}, c.row) + if err := cl.ensureReaderRole(ctx); err != nil { + t.Fatal(err) + } + altered := false + for _, k := range f.calls { + if strings.HasPrefix(k.SQL, "ALTER ROLE") && strings.Contains(k.SQL, "PASSWORD") { + altered = true + } + } + if altered != c.alter { + t.Errorf("%v: altered %v", c.row, altered) + } + } +} + func TestASchemaTheReaderCannotUseIsGrantedUsage(t *testing.T) { decl, _ := ParseReadable(map[string]any{"readable": []any{"audit.events"}}) - stmts, _ := planDatabase(decl, []column{{"audit", "events", "at"}}, held{Tables: map[table]string{}, Columns: map[column][]string{}}, + stmts, _ := planDatabase(decl, []column{{"audit", "events", "at"}}, newHeld(), map[string]bool{"audit": true}) if len(stmts) != 2 || stmts[0] != `GRANT USAGE ON SCHEMA "audit" TO "mesh_store_reader"` { t.Fatal(stmts) diff --git a/modules/postgres/cmd/postgres-provider/tools.go b/modules/postgres/cmd/postgres-provider/tools.go index 1b7c9f83..9f2e3095 100644 --- a/modules/postgres/cmd/postgres-provider/tools.go +++ b/modules/postgres/cmd/postgres-provider/tools.go @@ -73,7 +73,8 @@ func Tools(pg *Client) []stdio.Tool { Description: "What the store's reader — the login every query runs as — may read in each database, as the last " + "pass made it: the columns each module declares readable, what was refused because its name says it holds a " + "secret, what was declared but is missing, what was revoked, and any secret column it can still read " + - "(exposed; queries there are then refused). Names only, never values. (r)", + "(exposed; queries there are then refused), and the SECURITY DEFINER functions and views it may use, which read " + + "with their owner's rights and which no grant shows. Names only, never values. (r)", Run: func(map[string]any) (any, error) { return pg.LastReaderReport(), nil }, }, { From e0e97a96c4011e755a15baec1a5fda6f324d0126 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 22:11:06 +0200 Subject: [PATCH 4/4] Report as revoked only what the read-back shows gone, and take back what the reader passed on (review of hq issue 337) A reader given a privilege WITH GRANT OPTION that passed it on made the plain revoke fail with dependent privileges exist; the pass still listed the table as revoked and logged it every minute. The pass now retries such a change with CASCADE, which reaches only grants that came from the reader, reads the grants back, reports as revoked only what is gone, and says what is still held and why when anything is. --- modules/postgres/README.md | 7 +- .../cmd/postgres-provider/readable.go | 64 +++++++++++++++++-- .../postgres-provider/readable_live_test.go | 48 ++++++++++++++ .../cmd/postgres-provider/readable_test.go | 24 +++++++ 4 files changed, 134 insertions(+), 9 deletions(-) diff --git a/modules/postgres/README.md b/modules/postgres/README.md index 7593f125..10a9ad2e 100644 --- a/modules/postgres/README.md +++ b/modules/postgres/README.md @@ -68,8 +68,11 @@ for, so one removed by hand is installed again. what stands in the way. Declare columns by name where a table mixes the two. Every minute, on every database, the reader's role memberships are revoked and its grants brought to exactly that — each grant revoked as its own grantor, since a superuser's revoke acts as the table's - owner and leaves a grant another role made — and then read back: anything the reader still holds that - is not declared is reported as a failure, never as revoked. An existing install is narrowed without + owner and leaves a grant another role made — and then read back: only what the read-back shows gone + is reported as revoked, and anything the reader still holds that is not declared is reported as a + failure, with the reason. A reader that was given a privilege with grant option and passed it on makes + a plain revoke fail ("dependent privileges exist"); the pass then revokes with CASCADE, which reaches + only the grants that came from the reader. An existing install is narrowed without anybody acting, and a narrowed declaration takes effect at the next pass. The role itself is altered only when an attribute, a setting or its password has drifted, so the password is not sent every minute. Before a caller's statement runs, the reader's own session is asked which secret-named diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go index aa284f1f..41d241d5 100644 --- a/modules/postgres/cmd/postgres-provider/readable.go +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -35,6 +35,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "os" "regexp" @@ -42,6 +43,8 @@ import ( "strings" "sync" "time" + + "github.com/jackc/pgx/v5/pgconn" ) // ReadableTable is one entry of a contribution's `readable`. @@ -543,6 +546,40 @@ func (c *Client) LastReaderReport() ReaderReport { return c.reader.last } +// dependentPrivileges says whether the server refused a revoke because grants depend on it +// (dependent_privileges_exist): the reader passed on a privilege it holds with grant option. +func dependentPrivileges(err error) bool { + var pg *pgconn.PgError + return errors.As(err, &pg) && pg.Code == "2BP01" +} + +// cascaded is the change with every REVOKE made CASCADE. +func cascaded(stmts []string) []string { + out := make([]string, len(stmts)) + for i, s := range stmts { + if strings.HasPrefix(s, "REVOKE ") { + s += " CASCADE" + } + out[i] = s + } + return out +} + +// gone is what was held before and is not held now. +func gone(before, left []string) []string { + still := map[string]bool{} + for _, l := range left { + still[l] = true + } + var out []string + for _, b := range before { + if !still[b] { + out = append(out, b) + } + } + return out +} + // readHeld is every privilege the reader holds in one database, table-level and column-level, each // with its grantor. func (c *Client) readHeld(ctx context.Context, db string) (held, error) { @@ -612,21 +649,34 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab } stmts, rep := planDatabase(decl, existing, h, noUsage) rep.Database, rep.Declared = db, decl != nil + // What was revoked is what the read-back shows gone, never what was asked for. + rep.Revoked = nil if len(stmts) > 0 { + want := wantedColumns(decl, existing, &DatabaseReport{}) + before := unwanted(h, want) // One string: the simple protocol runs it as one implicit transaction, all or nothing. - if _, err := c.Query(ctx, db, strings.Join(stmts, "; ")); err != nil { - rep.Error = "changing the reader's grants: " + err.Error() - return rep + _, changeErr := c.Query(ctx, db, strings.Join(stmts, "; ")) + if changeErr != nil && dependentPrivileges(changeErr) { + // The reader holds a grant option and passed the privilege on: a plain revoke refuses + // while those grants stand. CASCADE takes them too — they all came from the reader, since + // only grants depending on the reader's grant option depend on what is revoked here. + _, changeErr = c.Query(ctx, db, strings.Join(cascaded(stmts), "; ")) } - // Read back: what the reader still holds beyond what is wanted is a failure, said as one — - // never reported as revoked while it stands. after, err := c.readHeld(ctx, db) if err != nil { rep.Error = "reading the reader's grants back: " + err.Error() return rep } - if left := unwanted(after, wantedColumns(decl, existing, &DatabaseReport{})); len(left) > 0 { - rep.Error = "STILL HELD after revoking: the reader can still use " + strings.Join(left, ", ") + left := unwanted(after, want) + rep.Revoked = gone(before, left) + if len(left) > 0 { + why := "the revoke ran and they stand" + if changeErr != nil { + why = "changing the reader's grants failed: " + changeErr.Error() + } + rep.Error = "STILL HELD: the reader can still use " + strings.Join(left, ", ") + " (" + why + ")" + } else if changeErr != nil { + rep.Error = "changing the reader's grants: " + changeErr.Error() } } // Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a diff --git a/modules/postgres/cmd/postgres-provider/readable_live_test.go b/modules/postgres/cmd/postgres-provider/readable_live_test.go index ebef9714..de746e5f 100644 --- a/modules/postgres/cmd/postgres-provider/readable_live_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_live_test.go @@ -181,3 +181,51 @@ func TestLiveAGrantByAnotherGrantorIsTakenBackAndDefinersAreSaid(t *testing.T) { } } } + +// The reader was given a table WITH GRANT OPTION and passed it on. A plain revoke then fails with +// "dependent privileges exist" and changes nothing; the pass must take back the reader's grant and +// what the reader passed on (CASCADE reaches only grants that came from the reader), report as +// revoked only what the read-back shows gone, and say what is still held and why if anything is. +func TestLiveAGrantTheReaderPassedOnIsTakenBack(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + defer func() { _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward") }() + if _, err := c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward; CREATE ROLE mesh_test_onward"); err != nil { + t.Fatal(err) + } + for _, sql := range []string{ + "REVOKE ALL ON vault FROM " + Ident(Reader), + "GRANT SELECT ON notes TO " + Ident(Reader) + " WITH GRANT OPTION", + "SET ROLE " + Ident(Reader) + "; GRANT SELECT ON notes TO mesh_test_onward; RESET ROLE", + } { + if _, err := c.Query(ctx, liveStore, sql); err != nil { + t.Fatal(sql, err) + } + } + var mine DatabaseReport + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore { + mine = db + } + } + if mine.Error != "" { + t.Fatal("left standing:", mine.Error) + } + if columnReadable(t, c, liveStore, "notes", "body") { + t.Fatalf("the reader still reads notes, and the pass said revoked %v", mine.Revoked) + } + if strings.Join(mine.Revoked, ",") != "public.notes" { + t.Fatalf("revoked %v", mine.Revoked) + } + r, err := c.Query(ctx, liveStore, "SELECT has_table_privilege('mesh_test_onward', 'notes', 'SELECT')") + if err != nil || cell(r.Rows[0], 0) != "f" { + t.Fatal("what the reader passed on survived:", r.Rows, err) + } + // The next pass finds nothing held, and says nothing revoked. + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && (len(db.Revoked) > 0 || db.Error != "") { + t.Fatalf("%+v", db) + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_test.go b/modules/postgres/cmd/postgres-provider/readable_test.go index 28f81095..aa5ea0fb 100644 --- a/modules/postgres/cmd/postgres-provider/readable_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_test.go @@ -6,6 +6,7 @@ package main import ( "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -354,3 +355,26 @@ func TestTheCataloguesReadableDeclarationsParse(t *testing.T) { } } } + +// A change that fails reports nothing revoked, and says what is still held and why — every pass, not +// "revoked" every pass. +func TestAFailedChangeSaysWhatIsStillHeldAndRevokesNothing(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + f, c := newFake(conn) + f.answer(`^SELECT rolsuper`, []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"}, + []string{"f", "f", "f", "f", "f", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}) + f.answer(`SELECT datname FROM pg_database WHERE datallowconn`, []string{"datname"}, []string{"app"}) + f.answer(`aclexplode\(c\.relacl\)`, []string{"nspname", "relname", "relkind", "privilege_type", "grantor"}, + []string{"public", "vault", "r", "SELECT", "owner1"}) + f.answer(`aclexplode\(a\.attacl\)`, []string{"nspname", "relname", "attname", "privilege_type", "grantor"}) + f.fail(`^SET ROLE`, errors.New("server busy")) + rep := c.ReconcileReader(ctx, Declarations{}, "") + db := rep.Databases[0] + if len(db.Revoked) != 0 { + t.Fatalf("said revoked: %v", db.Revoked) + } + if !strings.Contains(db.Error, "STILL HELD") || !strings.Contains(db.Error, "public.vault") || !strings.Contains(db.Error, "server busy") { + t.Fatal(db.Error) + } +}