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.
2026-10-08 22:01:43 +02:00
parent fb07d552a0
commit a643e28083
7 changed files with 431 additions and 92 deletions
@@ -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, "; ")))
 			}