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)."), } }