From 4f44bf1eb5538c6466849c4c336dde936b16e2c0 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 21:43:15 +0200 Subject: [PATCH] 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)."), } }