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 }, }, {