From 8bf50f75557ae3cad1b9f5143ab7f76318367cfd Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 22:01:43 +0200 Subject: [PATCH] Revoke the reader's grants as their own grantors and read them back, alter its role only on drift, and report definer functions and views (review of hq issue 337) A superuser's REVOKE acts as the table owner, so a grant to the reader made by a role holding GRANT OPTION survived it while the report said revoked. Each grant is now revoked under SET ROLE to its grantor, the grants are read back, and anything still held is reported as a failure. The role was altered every minute with its password in the statement, which a server logging DDL would record and which could collide with a first query; it is now altered only when an attribute, a setting or the password has drifted, one ensure at a time. SECURITY DEFINER functions and views the reader may use reach data without a grant on it, so every pass lists them. The README says the name filter is a backstop and that the read-only session default can be turned off by a statement. --- modules/postgres/README.md | 28 +- .../postgres/cmd/postgres-provider/client.go | 68 ++++- .../cmd/postgres-provider/readable.go | 270 +++++++++++++----- .../postgres-provider/readable_live_test.go | 70 +++++ .../cmd/postgres-provider/readable_test.go | 84 +++++- .../postgres/cmd/postgres-provider/tools.go | 3 +- 6 files changed, 431 insertions(+), 92 deletions(-) diff --git a/modules/postgres/README.md b/modules/postgres/README.md index c5dbcef8..7593f125 100644 --- a/modules/postgres/README.md +++ b/modules/postgres/README.md @@ -47,8 +47,10 @@ for, so one removed by hand is installed again. - **`postgres_retire_database` renames, it does not drop**: the database becomes `_deleted_` and its owner is locked. Removing the data is a person's act, by hand. - **A caller's statement never runs as the superuser.** `postgres_query` and the seat's `query` verb - run as `mesh_store_reader` — every session read-only by the server's own setting, a 60 s statement - timeout — with the statement sent as given (issue 193). Without the reader's password + run as `mesh_store_reader` — every session read-only by default, a 60 s statement timeout — with the + statement sent as given (issue 193). The default is only a default: a statement may + `SET transaction_read_only = off`. What stops the reader writing is that it holds no grant to write + (and, beyond that, only the `TEMP` privilege PostgreSQL gives every login, for temporary tables). Without the reader's password (`own-secrets.reader`) the call is refused. - **The reader reads only what a module declares readable, and never a secret** (issue 337). It holds no role — until issue 337 it held `pg_read_all_data`, which let any caller of the seat's `query` read @@ -60,13 +62,25 @@ for, so one removed by hand is installed again. An entry is `[schema.]table` (every column it has, at each pass) or `[schema.]table(column, …)`. A database no contribution names — the controller's own among them — gives the reader nothing. A column or table whose name says it holds a secret (private, secret, sealed, token, password, credential, salt, - a key) is never granted, declared or not, and the refusal is reported. Every minute, on every - database, the reader's role memberships are revoked and its grants brought to exactly that, so an - existing install is narrowed without anybody acting and a narrowed declaration takes effect at the - next pass. Before a caller's statement runs, the reader's own session is asked which secret-named + a key) is never granted, declared or not, and the refusal is reported. **That name check is a + backstop, not the guard:** a module that declares a table whose secret sits under a plain column name + (`value`, `data`, `blob`) has declared that secret readable, and the review of its declaration is + what stands in the way. Declare columns by name where a table mixes the two. + Every minute, on every database, the reader's role memberships are revoked and its grants brought to + exactly that — each grant revoked as its own grantor, since a superuser's revoke acts as the table's + owner and leaves a grant another role made — and then read back: anything the reader still holds that + is not declared is reported as a failure, never as revoked. An existing install is narrowed without + anybody acting, and a narrowed declaration takes effect at the next pass. The role itself is altered + only when an attribute, a setting or its password has drifted, so the password is not sent every + minute. Before a caller's statement runs, the reader's own session is asked which secret-named columns it can read (a grant to `PUBLIC` or by hand would show there); if any, the statement is refused and the columns named. A statement that names a known secret table of the controller's, or a secret-sounding name, is refused before it is sent — a second layer, not the guard. +- **What no grant shows.** A `SECURITY DEFINER` function the reader may execute (PUBLIC may execute a new + function unless revoked) runs with its owner's rights, and a view the reader may read reads with its + owner's. Neither needs a grant on the table behind it, so neither the reconcile nor the probe can see + what it returns. Each pass lists both for every database, in `postgres_reader_grants` and once in the + log; they are reported, not refused. ## Tools @@ -76,7 +90,7 @@ for, so one removed by hand is installed again. | `postgres_query` `{database, sql}` | one read-only statement, as the reader; rows keyed by column, `NULL` as null | | `postgres_retire_database` `{database, confirm}` | renames a database aside and locks its owner; `confirm` repeats the name | | `mesh-store.databases`, `mesh-store.query` | the store seat's verbs: the same listing and read-only query | -| `postgres_reader_grants` | what the reader may read in each database as the last pass left it: readable columns, refused secrets, declared but missing, revoked, and any secret it can still read; names only | +| `postgres_reader_grants` | what the reader may read in each database as the last pass left it: readable columns, refused secrets, declared but missing, revoked, any secret it can still read, and the definer functions and views it may use; names only | | `provisioner_retirement` | what is held, what waits for a person, what was rejected, every retired consumer and set-aside database with when, why and size | | `provisioner_retire_approve`, `provisioner_retire_reject` `{consumers, why, by}` | a person's answer to a set waiting; through the controller's `retire approve|reject` | | `provisioner_delete` `{consumer, confirm, why, by}` | drops one retired consumer's database and role, or one set-aside database; through the controller's `cleanup delete` | diff --git a/modules/postgres/cmd/postgres-provider/client.go b/modules/postgres/cmd/postgres-provider/client.go index 2d31a6fc..895e57eb 100644 --- a/modules/postgres/cmd/postgres-provider/client.go +++ b/modules/postgres/cmd/postgres-provider/client.go @@ -24,10 +24,12 @@ import ( "github.com/jackc/pgx/v5/pgconn" ) -// Reader is the login a caller's statement runs as (novox/hq issue 193). It changes nothing — every -// transaction it opens is read-only by the server's own setting — and it reads only the columns a -// consuming module declares readable, never one that holds a secret (readable.go, novox/hq issue 337). -// It holds no role: until issue 337 it held `pg_read_all_data`, and with it the controller's signing key. A statement cannot climb out of a login the way it can out of a +// Reader is the login a caller's statement runs as (novox/hq issue 193). It changes nothing because it +// holds no grant to write: its sessions are read-only by default, but a statement may turn that off +// (SET transaction_read_only = off), so the default is a convenience and the missing grants are the +// guard. It reads only the columns a consuming module declares readable, never one that holds a +// secret (readable.go, novox/hq issue 337). It holds no role: until issue 337 it held +// `pg_read_all_data`, and with it the controller's signing key. A statement cannot climb out of a login the way it can out of a // transaction wrapped around it as text: `COMMIT; DROP …` ended the old wrapper and ran the rest as // the superuser, and even one read-only statement as a superuser can run a program on the server. const Reader = "mesh_store_reader" @@ -104,6 +106,7 @@ type Client struct { readerMu sync.Mutex readerReady bool + roleMu sync.Mutex // reader is what the last reconcile of the reader's grants found (readable.go). reader readerState @@ -506,13 +509,21 @@ func (c *Client) ensureReaderRole(ctx context.Context) error { if c.conn.ReaderPassword == "" { return errReaderMissing } - has, err := c.exists(ctx, "SELECT 1 FROM pg_roles WHERE rolname = "+Literal(Reader)) + // One at a time: the reconcile loop and a first query may both ask, and two ALTER ROLEs at once + // fail with "tuple concurrently updated". + c.roleMu.Lock() + defer c.roleMu.Unlock() + has, drifted, err := c.readerDrifted(ctx) if err != nil { return err } - for _, sql := range ReaderStatements(has, c.conn.ReaderPassword) { - if _, err := c.Query(ctx, "", sql); err != nil { - return err + // The role is altered only when it has drifted: a statement carrying the password is sent as + // seldom as possible, since a server logging DDL would write it down. + if !has || drifted { + for _, sql := range ReaderStatements(has, c.conn.ReaderPassword) { + if _, err := c.Query(ctx, "", sql); err != nil { + return err + } } } r, err := c.Query(ctx, "", "SELECT r.rolname FROM pg_auth_members m JOIN pg_roles r ON r.oid = m.roleid "+ @@ -530,6 +541,47 @@ func (c *Client) ensureReaderRole(ctx context.Context) error { return nil } +// readerAttributes is what the reader's role must say, in the order readerDrifted asks for them: +// not a superuser, cannot make databases or roles, replicate or bypass row security; inherits; logs in; +// its password never expires. +var readerAttributes = []string{"f", "f", "f", "f", "f", "t", "t", "t"} + +// readerDrifted says whether the reader's role exists, and whether it differs from what +// ReaderStatements makes: an attribute, a session setting, or a password that no longer logs in. +func (c *Client) readerDrifted(ctx context.Context) (exists, drifted bool, err error) { + r, err := c.Query(ctx, "", "SELECT rolsuper, rolcreatedb, rolcreaterole, rolreplication, rolbypassrls, rolinherit, rolcanlogin, "+ + "COALESCE(rolvaliduntil = 'infinity', false), COALESCE(array_to_string(rolconfig, ','), '') FROM pg_roles WHERE rolname = "+Literal(Reader)) + if err != nil { + return false, false, err + } + if len(r.Rows) == 0 { + return false, false, nil + } + row := r.Rows[0] + for i, want := range readerAttributes { + if cell(row, i) != want { + return true, true, nil + } + } + config := map[string]bool{} + for _, kv := range strings.Split(cell(row, len(readerAttributes)), ",") { + config[kv] = true + } + if !config["default_transaction_read_only=on"] || !config["statement_timeout=60s"] { + return true, true, nil + } + // The password: asked by logging in with it, which is the only way to know without reading a hash. + s, err := c.dial(ctx, Login{Database: "postgres", User: Reader, Password: c.conn.ReaderPassword, Options: readerOptions}) + if err != nil { + if IsLost(err) { + return true, true, nil + } + return true, false, err + } + s.Close() + return true, false, nil +} + var firstWord = regexp.MustCompile(`^\s*([A-Za-z]+)`) // ReadOnlyQuery runs a caller's statement against a named database as the read-only login, for the diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go index 2f04cd96..aa284f1f 100644 --- a/modules/postgres/cmd/postgres-provider/readable.go +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -185,14 +185,26 @@ type column struct{ Schema, Table, Name string } type table struct{ Schema, Table string } -// held is what the reader holds in one database, read from the catalogue as the admin. -type held struct { - // Tables are those the reader holds any table-level privilege on, with their kind ('S': sequence). - Tables map[table]string - // Columns are the column privileges it holds, each column's privilege types. - Columns map[column][]string +// grant is one privilege the reader holds, and who granted it. +type grant struct{ Privilege, Grantor string } + +// tableGrants is a table the reader holds table-level privileges on: its kind ('S': sequence) and every +// privilege with its grantor. +type tableGrants struct { + Kind string + Grants []grant } +// held is what the reader holds in one database, read from the catalogue as the admin, with grantors: +// a grant is only taken back by its own grantor. A superuser's REVOKE acts as the table's owner, so a +// grant made by a role holding GRANT OPTION survives it (the review of novox/hq issue 337). +type held struct { + Tables map[table]tableGrants + Columns map[column][]grant +} + +func newHeld() held { return held{Tables: map[table]tableGrants{}, Columns: map[column][]grant{}} } + // DatabaseReport is what the reader may read in one database, and what was refused or missing. type DatabaseReport struct { Database string `json:"database"` @@ -202,14 +214,16 @@ type DatabaseReport struct { Missing []string `json:"missing,omitempty"` Revoked []string `json:"revoked,omitempty"` Exposed []string `json:"exposed,omitempty"` + // Definers and Views reach data without a grant on it: a SECURITY DEFINER function the reader may + // execute runs with its owner's rights, and a view it may read reads with its owner's. Reported, not + // refused: the name check cannot see what either returns. + Definers []string `json:"definers,omitempty"` + Views []string `json:"views,omitempty"` Error string `json:"error,omitempty"` } -// planDatabase is the DDL that brings the reader's grants in one database to exactly what is declared, -// minus every secret: revoke what is held and not wanted, grant what is wanted and not held. -// existing is every column of every declared table that the database has. Pure; the tests' subject. -func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[string]bool) ([]string, DatabaseReport) { - var rep DatabaseReport +// wantedColumns is every declared column that exists and is not a secret, with what was refused or missing. +func wantedColumns(decl []ReadableTable, existing []column, rep *DatabaseReport) map[column]bool { byTable := map[table][]string{} for _, c := range existing { t := table{c.Schema, c.Table} @@ -252,51 +266,102 @@ func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[s want[column{d.Schema, d.Table, c}] = true } } - // Every table-level privilege is revoked: the reader is only ever given columns. REVOKE on a table - // takes its column privileges with it, so those are granted again below if wanted. - var stmts []string - cleared := map[table]bool{} + return want +} + +// unwanted is what the reader holds beyond the wanted columns' SELECT: every table-level privilege, +// and every column privilege that is not SELECT on a wanted column. Each as "schema.table[.column]". +func unwanted(h held, want map[column]bool) []string { + var out []string + for t := range h.Tables { + out = append(out, t.Schema+"."+t.Table) + } + for c, grants := range h.Columns { + for _, g := range grants { + if !want[c] || g.Privilege != "SELECT" { + out = append(out, c.Schema+"."+c.Table+"."+c.Name) + break + } + } + } + sort.Strings(out) + return out +} + +// planDatabase is the DDL that brings the reader's grants in one database to exactly what is declared, +// minus every secret: revoke what is held and not wanted — as each grant's own grantor — and grant what +// is wanted and not held. existing is every column of every declared table that the database has. +// Pure; the tests' subject. +func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[string]bool) ([]string, DatabaseReport) { + var rep DatabaseReport + want := wantedColumns(decl, existing, &rep) + + // Revokes, grouped by grantor: SET ROLE to it, so the REVOKE is that grantor's and takes its grant. + byGrantor := map[string][]string{} + touched := map[table]bool{} for _, t := range sortedTables(h.Tables) { kind := "TABLE" - if h.Tables[t] == "S" { + if h.Tables[t].Kind == "S" { kind = "SEQUENCE" } - stmts = append(stmts, fmt.Sprintf("REVOKE ALL ON %s %s.%s FROM %s", kind, Ident(t.Schema), Ident(t.Table), Ident(Reader))) + for _, g := range grantorsOf(h.Tables[t].Grants) { + byGrantor[g] = append(byGrantor[g], fmt.Sprintf("REVOKE ALL ON %s %s.%s FROM %s", kind, Ident(t.Schema), Ident(t.Table), Ident(Reader))) + } rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+" (every privilege on the table)") - cleared[t] = true + touched[t] = true } - have := map[column]bool{} - unwanted := map[table][]string{} - for c, privs := range h.Columns { - if cleared[table{c.Schema, c.Table}] { + type colKey struct { + t table + grantor string + } + cols := map[colKey][]string{} + for c, grants := range h.Columns { + bad := false + for _, g := range grants { + if !want[c] || g.Privilege != "SELECT" { + bad = true + } + } + if !bad { continue } - onlySelect := len(privs) == 1 && privs[0] == "SELECT" - if want[c] && onlySelect { - have[c] = true - continue - } - // Not wanted, or held with more than SELECT: revoked, and granted SELECT again below if wanted. + // Not wanted, or held with more than SELECT: every grantor's grant of it is taken back, and + // SELECT granted again below if it is wanted. t := table{c.Schema, c.Table} - unwanted[t] = append(unwanted[t], c.Name) - } - for _, t := range sortedKeys(unwanted) { - cols := unwanted[t] - sort.Strings(cols) - stmts = append(stmts, fmt.Sprintf("REVOKE ALL (%s) ON %s.%s FROM %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) - for _, c := range cols { - rep.Revoked = append(rep.Revoked, t.Schema+"."+t.Table+"."+c) + for _, g := range grantorsOf(grants) { + k := colKey{t, g} + cols[k] = append(cols[k], c.Name) } + touched[t] = true + rep.Revoked = append(rep.Revoked, c.Schema+"."+c.Table+"."+c.Name) } - grant := map[table][]string{} + for k, names := range cols { + sort.Strings(names) + byGrantor[k.grantor] = append(byGrantor[k.grantor], + fmt.Sprintf("REVOKE ALL (%s) ON %s.%s FROM %s", idents(names), Ident(k.t.Schema), Ident(k.t.Table), Ident(Reader))) + } + var stmts []string + for _, g := range sortedGrantors(byGrantor) { + revokes := byGrantor[g] + sort.Strings(revokes) + stmts = append(stmts, "SET ROLE "+Ident(g)) + stmts = append(stmts, revokes...) + stmts = append(stmts, "RESET ROLE") + } + sort.Strings(rep.Revoked) + + // A wanted column is granted unless it is held with SELECT alone on a table nothing was revoked on; + // granting SELECT that is held already changes nothing. + grantCols := map[table][]string{} schemas := map[string]bool{} for c := range want { rep.Readable = append(rep.Readable, c.Schema+"."+c.Table+"."+c.Name) - if !have[c] { - t := table{c.Schema, c.Table} - grant[t] = append(grant[t], c.Name) - schemas[c.Schema] = true + t := table{c.Schema, c.Table} + if len(h.Columns[c]) > 0 && !touched[t] { + continue } + grantCols[t] = append(grantCols[t], c.Name) + schemas[c.Schema] = true } sort.Strings(rep.Readable) for _, s := range sortedSet(schemas) { @@ -304,14 +369,36 @@ func planDatabase(decl []ReadableTable, existing []column, h held, noUsage map[s stmts = append(stmts, fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", Ident(s), Ident(Reader))) } } - for _, t := range sortedKeys(grant) { - cols := grant[t] - sort.Strings(cols) - stmts = append(stmts, fmt.Sprintf("GRANT SELECT (%s) ON %s.%s TO %s", idents(cols), Ident(t.Schema), Ident(t.Table), Ident(Reader))) + for _, t := range sortedKeys(grantCols) { + names := grantCols[t] + sort.Strings(names) + stmts = append(stmts, fmt.Sprintf("GRANT SELECT (%s) ON %s.%s TO %s", idents(names), Ident(t.Schema), Ident(t.Table), Ident(Reader))) } return stmts, rep } +func grantorsOf(grants []grant) []string { + seen := map[string]bool{} + var out []string + for _, g := range grants { + if !seen[g.Grantor] { + seen[g.Grantor] = true + out = append(out, g.Grantor) + } + } + sort.Strings(out) + return out +} + +func sortedGrantors(m map[string][]string) []string { + out := make([]string, 0, len(m)) + for g := range m { + out = append(out, g) + } + sort.Strings(out) + return out +} + func idents(names []string) string { out := make([]string, len(names)) for i, n := range names { @@ -320,7 +407,7 @@ func idents(names []string) string { return strings.Join(out, ", ") } -func sortedTables(m map[table]string) []table { +func sortedTables(m map[table]tableGrants) []table { out := make([]table, 0, len(m)) for t := range m { out = append(out, t) @@ -456,31 +543,47 @@ func (c *Client) LastReaderReport() ReaderReport { return c.reader.last } +// readHeld is every privilege the reader holds in one database, table-level and column-level, each +// with its grantor. +func (c *Client) readHeld(ctx context.Context, db string) (held, error) { + h := newHeld() + readerOID := "(SELECT oid FROM pg_catalog.pg_roles WHERE rolname = " + Literal(Reader) + ")" + r, err := c.Query(ctx, db, "SELECT n.nspname, c.relname, c.relkind::text, x.privilege_type, pg_catalog.pg_get_userbyid(x.grantor) "+ + "FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "CROSS JOIN LATERAL pg_catalog.aclexplode(c.relacl) x WHERE x.grantee = "+readerOID) + if err != nil { + return h, err + } + for _, row := range r.Rows { + t := table{cell(row, 0), cell(row, 1)} + tg := h.Tables[t] + tg.Kind = cell(row, 2) + tg.Grants = append(tg.Grants, grant{cell(row, 3), cell(row, 4)}) + h.Tables[t] = tg + } + r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname, x.privilege_type, pg_catalog.pg_get_userbyid(x.grantor) "+ + "FROM pg_catalog.pg_attribute a JOIN pg_catalog.pg_class c ON c.oid = a.attrelid "+ + "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ + "CROSS JOIN LATERAL pg_catalog.aclexplode(a.attacl) x WHERE x.grantee = "+readerOID) + if err != nil { + return h, err + } + for _, row := range r.Rows { + k := column{cell(row, 0), cell(row, 1), cell(row, 2)} + h.Columns[k] = append(h.Columns[k], grant{cell(row, 3), cell(row, 4)}) + } + return h, nil +} + func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []ReadableTable) DatabaseReport { fail := func(err error) DatabaseReport { return DatabaseReport{Database: db, Declared: decl != nil, Error: err.Error()} } - h := held{Tables: map[table]string{}, Columns: map[column][]string{}} - readerOID := "(SELECT oid FROM pg_catalog.pg_roles WHERE rolname = " + Literal(Reader) + ")" - r, err := c.Query(ctx, db, "SELECT n.nspname, c.relname, c.relkind::text FROM pg_catalog.pg_class c "+ - "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace CROSS JOIN LATERAL pg_catalog.aclexplode(c.relacl) x "+ - "WHERE x.grantee = "+readerOID+" GROUP BY 1, 2, 3") + h, err := c.readHeld(ctx, db) if err != nil { return fail(err) } - for _, row := range r.Rows { - h.Tables[table{cell(row, 0), cell(row, 1)}] = cell(row, 2) - } - r, err = c.Query(ctx, db, "SELECT n.nspname, c.relname, a.attname, x.privilege_type FROM pg_catalog.pg_attribute a "+ - "JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace "+ - "CROSS JOIN LATERAL pg_catalog.aclexplode(a.attacl) x WHERE x.grantee = "+readerOID) - if err != nil { - return fail(err) - } - for _, row := range r.Rows { - k := column{cell(row, 0), cell(row, 1), cell(row, 2)} - h.Columns[k] = append(h.Columns[k], cell(row, 3)) - } + var r Result var existing []column noUsage := map[string]bool{} if len(decl) > 0 { @@ -515,6 +618,16 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab rep.Error = "changing the reader's grants: " + err.Error() return rep } + // Read back: what the reader still holds beyond what is wanted is a failure, said as one — + // never reported as revoked while it stands. + after, err := c.readHeld(ctx, db) + if err != nil { + rep.Error = "reading the reader's grants back: " + err.Error() + return rep + } + if left := unwanted(after, wantedColumns(decl, existing, &DatabaseReport{})); len(left) > 0 { + rep.Error = "STILL HELD after revoking: the reader can still use " + strings.Join(left, ", ") + } } // Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a // grant made by hand) is found here, said, and makes the reader's statements against it refused. @@ -524,6 +637,27 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab return rep } rep.Exposed = Exposed(r) + // What reaches data without a grant on it, which the probe above cannot see: SECURITY DEFINER + // functions the reader may execute (PUBLIC has EXECUTE on a new function), and views it may read. + who := Literal(Reader) + if r, err = c.Query(ctx, db, "SELECT n.nspname || '.' || p.proname || '(' || pg_catalog.pg_get_function_identity_arguments(p.oid) || ')' "+ + "FROM pg_catalog.pg_proc p JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace WHERE p.prosecdef "+ + "AND n.nspname NOT IN ('pg_catalog','information_schema') AND pg_catalog.has_function_privilege("+who+", p.oid, 'EXECUTE') ORDER BY 1"); err != nil { + rep.Error = "listing the definer functions the reader may run: " + err.Error() + return rep + } + for _, row := range r.Rows { + rep.Definers = append(rep.Definers, cell(row, 0)) + } + if r, err = c.Query(ctx, db, "SELECT n.nspname || '.' || c.relname FROM pg_catalog.pg_class c "+ + "JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relkind IN ('v','m') "+ + "AND n.nspname NOT IN ('pg_catalog','information_schema') AND pg_catalog.has_any_column_privilege("+who+", c.oid, 'SELECT') ORDER BY 1"); err != nil { + rep.Error = "listing the views the reader may read: " + err.Error() + return rep + } + for _, row := range r.Rows { + rep.Views = append(rep.Views, cell(row, 0)) + } return rep } @@ -550,6 +684,14 @@ func (c *Client) RunReader(ctx context.Context, receives string, every time.Dura if db.Error != "" { lines = append(lines, fmt.Sprintf("reader: %s: %s", db.Database, db.Error)) } + if len(db.Definers) > 0 { + lines = append(lines, fmt.Sprintf("reader: %s: the reader may run SECURITY DEFINER function(s) %s, which read with their "+ + "owner's rights; what they return is not checked", db.Database, strings.Join(db.Definers, ", "))) + } + if len(db.Views) > 0 { + lines = append(lines, fmt.Sprintf("reader: %s: the reader may read view(s) %s, which read with their owner's rights", + db.Database, strings.Join(db.Views, ", "))) + } if len(db.Refused) > 0 { lines = append(lines, fmt.Sprintf("reader: %s: not granted, %s", db.Database, strings.Join(db.Refused, "; "))) } diff --git a/modules/postgres/cmd/postgres-provider/readable_live_test.go b/modules/postgres/cmd/postgres-provider/readable_live_test.go index 8db18b45..ebef9714 100644 --- a/modules/postgres/cmd/postgres-provider/readable_live_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_live_test.go @@ -111,3 +111,73 @@ func TestLiveTheReaderReadsWhatIsDeclaredAndNoSecret(t *testing.T) { t.Fatal("a query ran while the reader can read a secret:", err) } } + +// A grant to the reader made by a role holding GRANT OPTION — not the owner — is taken back as that +// role, and what still stands after a pass is a failure, not "revoked". SECURITY DEFINER functions and +// views the reader may use are reported. +func TestLiveAGrantByAnotherGrantorIsTakenBackAndDefinersAreSaid(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + for _, sql := range []string{ + "DROP ROLE IF EXISTS mesh_test_owner", "DROP ROLE IF EXISTS mesh_test_middle", + } { + if _, err := c.Query(ctx, "", sql); err != nil { + t.Fatal(sql, err) + } + } + defer func() { + _, _ = c.Query(ctx, "", "DROP DATABASE IF EXISTS "+Ident(liveStore)+" WITH (FORCE)") + _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_middle") + _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_owner") + }() + for _, sql := range []string{ + "CREATE ROLE mesh_test_owner", "CREATE ROLE mesh_test_middle", + } { + if _, err := c.Query(ctx, "", sql); err != nil { + t.Fatal(sql, err) + } + } + for _, sql := range []string{ + "ALTER TABLE notes OWNER TO mesh_test_owner", + "GRANT SELECT ON notes TO mesh_test_middle WITH GRANT OPTION", + "SET ROLE mesh_test_middle; GRANT SELECT ON notes TO " + Ident(Reader) + "; GRANT SELECT (body) ON notes TO " + Ident(Reader) + "; RESET ROLE", + "CREATE FUNCTION peek() RETURNS bytea LANGUAGE sql SECURITY DEFINER AS 'SELECT private FROM vault LIMIT 1'", + "CREATE VIEW plain_names AS SELECT name FROM vault", + "GRANT SELECT ON plain_names TO " + Ident(Reader), + } { + if _, err := c.Query(ctx, liveStore, sql); err != nil { + t.Fatal(sql, err) + } + } + var mine DatabaseReport + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore { + mine = db + } + } + if mine.Error != "" { + t.Fatal("left standing:", mine.Error) + } + if columnReadable(t, c, liveStore, "notes", "body") { + t.Fatal("the other grantor's grant survived") + } + if strings.Join(mine.Definers, ",") != "public.peek()" || strings.Join(mine.Views, ",") != "" { + t.Fatalf("definers %v, views %v", mine.Definers, mine.Views) + } + // A second pass finds nothing to take back. + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && (len(db.Revoked) > 0 || db.Error != "") { + t.Fatalf("%+v", db) + } + } + // A view granted to PUBLIC is said. + if _, err := c.Query(ctx, liveStore, "GRANT SELECT ON plain_names TO PUBLIC"); err != nil { + t.Fatal(err) + } + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && strings.Join(db.Views, ",") != "public.plain_names" { + t.Fatalf("views %v", db.Views) + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_test.go b/modules/postgres/cmd/postgres-provider/readable_test.go index ebebbd6b..28f81095 100644 --- a/modules/postgres/cmd/postgres-provider/readable_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_test.go @@ -70,7 +70,7 @@ func TestTheReaderIsGivenOnlyWhatIsDeclaredAndNeverASecret(t *testing.T) { {"public", "signing_key", "public"}, {"public", "signing_key", "private"}, {"public", "build", "id"}, {"public", "build", "sealed"}, {"public", "build", "log"}, } - stmts, rep := planDatabase(decl, existing, held{Tables: map[table]string{}, Columns: map[column][]string{}}, map[string]bool{}) + stmts, rep := planDatabase(decl, existing, newHeld(), map[string]bool{}) sql := strings.Join(stmts, "\n") if !strings.Contains(sql, `GRANT SELECT ("name", "role") ON "public"."node" TO "mesh_store_reader"`) || !strings.Contains(sql, `GRANT SELECT ("id") ON "public"."build" TO "mesh_store_reader"`) { @@ -92,22 +92,28 @@ func TestTheReaderIsGivenOnlyWhatIsDeclaredAndNeverASecret(t *testing.T) { func TestWhatTheReaderHoldsAndIsNotDeclaredIsRevoked(t *testing.T) { // An install before issue 337, or a grant by hand: a table-level grant, a column no longer // declared, and a declared column held with more than SELECT. + sel := func(by string) grant { return grant{"SELECT", by} } h := held{ - Tables: map[table]string{{"public", "secret"}: "r", {"public", "seq"}: "S"}, - Columns: map[column][]string{ - {"public", "node", "name"}: {"SELECT"}, - {"public", "node", "role"}: {"SELECT", "UPDATE"}, - {"public", "node", "old"}: {"SELECT"}, + Tables: map[table]tableGrants{ + {"public", "secret"}: {Kind: "r", Grants: []grant{sel("postgres")}}, + {"public", "seq"}: {Kind: "S", Grants: []grant{{"USAGE", "postgres"}}}, + }, + Columns: map[column][]grant{ + {"public", "node", "name"}: {sel("postgres")}, + {"public", "node", "role"}: {sel("postgres"), {"UPDATE", "postgres"}}, + {"public", "node", "old"}: {sel("postgres")}, }, } decl, _ := ParseReadable(map[string]any{"readable": []any{"node(name, role)"}}) existing := []column{{"public", "node", "name"}, {"public", "node", "role"}, {"public", "node", "old"}} stmts, rep := planDatabase(decl, existing, h, map[string]bool{}) want := []string{ - `REVOKE ALL ON TABLE "public"."secret" FROM "mesh_store_reader"`, - `REVOKE ALL ON SEQUENCE "public"."seq" FROM "mesh_store_reader"`, + `SET ROLE "postgres"`, `REVOKE ALL ("old", "role") ON "public"."node" FROM "mesh_store_reader"`, - `GRANT SELECT ("role") ON "public"."node" TO "mesh_store_reader"`, + `REVOKE ALL ON SEQUENCE "public"."seq" FROM "mesh_store_reader"`, + `REVOKE ALL ON TABLE "public"."secret" FROM "mesh_store_reader"`, + `RESET ROLE`, + `GRANT SELECT ("name", "role") ON "public"."node" TO "mesh_store_reader"`, } if strings.Join(stmts, "\n") != strings.Join(want, "\n") { t.Fatalf("got\n%s\nwant\n%s", strings.Join(stmts, "\n"), strings.Join(want, "\n")) @@ -122,20 +128,74 @@ func TestWhatTheReaderHoldsAndIsNotDeclaredIsRevoked(t *testing.T) { t.Fatal(s) } } - if len(stmts) != 3 { + if len(stmts) != 5 { t.Fatal(stmts) } // Held exactly as declared: nothing to do. - exact := held{Tables: map[table]string{}, Columns: map[column][]string{{"public", "node", "name"}: {"SELECT"}}} + exact := held{Tables: map[table]tableGrants{}, Columns: map[column][]grant{{"public", "node", "name"}: {sel("postgres")}}} decl, _ = ParseReadable(map[string]any{"readable": []any{"node(name)"}}) if stmts, _ := planDatabase(decl, existing, exact, map[string]bool{}); len(stmts) != 0 { t.Fatal(stmts) } } +// A superuser's REVOKE acts as the table's owner, so a grant to the reader made by another role +// holding GRANT OPTION survives it. Each grant is revoked as its own grantor. +func TestAGrantIsRevokedAsItsOwnGrantor(t *testing.T) { + h := held{ + Tables: map[table]tableGrants{{"public", "vault"}: {Kind: "r", Grants: []grant{{"SELECT", "middle"}, {"SELECT", "owner1"}}}}, + Columns: map[column][]grant{{"public", "vault", "name"}: {{"SELECT", "middle"}}}, + } + stmts, _ := planDatabase(nil, nil, h, map[string]bool{}) + want := []string{ + `SET ROLE "middle"`, + `REVOKE ALL ("name") ON "public"."vault" FROM "mesh_store_reader"`, + `REVOKE ALL ON TABLE "public"."vault" FROM "mesh_store_reader"`, + `RESET ROLE`, + `SET ROLE "owner1"`, + `REVOKE ALL ON TABLE "public"."vault" FROM "mesh_store_reader"`, + `RESET ROLE`, + } + if strings.Join(stmts, "\n") != strings.Join(want, "\n") { + t.Fatalf("got\n%s\nwant\n%s", strings.Join(stmts, "\n"), strings.Join(want, "\n")) + } + if left := unwanted(h, nil); strings.Join(left, ",") != "public.vault,public.vault.name" { + t.Fatal(left) + } +} + +// The role is altered only when it drifted: the statement carries the password. +func TestTheReadersRoleIsAlteredOnlyWhenItDrifted(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + for _, c := range []struct { + row []string + alter bool + }{ + {[]string{"f", "f", "f", "f", "f", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}, false}, + {[]string{"f", "f", "f", "f", "t", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}, true}, + {[]string{"f", "f", "f", "f", "f", "t", "t", "t", "statement_timeout=60s"}, true}, + } { + f, cl := newFake(conn) + f.answer(`^SELECT rolsuper`, []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"}, c.row) + if err := cl.ensureReaderRole(ctx); err != nil { + t.Fatal(err) + } + altered := false + for _, k := range f.calls { + if strings.HasPrefix(k.SQL, "ALTER ROLE") && strings.Contains(k.SQL, "PASSWORD") { + altered = true + } + } + if altered != c.alter { + t.Errorf("%v: altered %v", c.row, altered) + } + } +} + func TestASchemaTheReaderCannotUseIsGrantedUsage(t *testing.T) { decl, _ := ParseReadable(map[string]any{"readable": []any{"audit.events"}}) - stmts, _ := planDatabase(decl, []column{{"audit", "events", "at"}}, held{Tables: map[table]string{}, Columns: map[column][]string{}}, + stmts, _ := planDatabase(decl, []column{{"audit", "events", "at"}}, newHeld(), map[string]bool{"audit": true}) if len(stmts) != 2 || stmts[0] != `GRANT USAGE ON SCHEMA "audit" TO "mesh_store_reader"` { t.Fatal(stmts) diff --git a/modules/postgres/cmd/postgres-provider/tools.go b/modules/postgres/cmd/postgres-provider/tools.go index 1b7c9f83..9f2e3095 100644 --- a/modules/postgres/cmd/postgres-provider/tools.go +++ b/modules/postgres/cmd/postgres-provider/tools.go @@ -73,7 +73,8 @@ func Tools(pg *Client) []stdio.Tool { Description: "What the store's reader — the login every query runs as — may read in each database, as the last " + "pass made it: the columns each module declares readable, what was refused because its name says it holds a " + "secret, what was declared but is missing, what was revoked, and any secret column it can still read " + - "(exposed; queries there are then refused). Names only, never values. (r)", + "(exposed; queries there are then refused), and the SECURITY DEFINER functions and views it may use, which read " + + "with their owner's rights and which no grant shows. Names only, never values. (r)", Run: func(map[string]any) (any, error) { return pg.LastReaderReport(), nil }, }, {