Give the store's reader only the columns a module declares readable, never a secret, so a query cannot read the controller's signing key (hq issue 337)

mesh_store_reader held pg_read_all_data, so every caller of mesh-store.query
and postgres_query could read every column the store holds: the signing key
nodes trust, sealed secrets, password hashes. The reader now holds no role;
each minute, on every database, its memberships are revoked and its grants
brought to what the consumer's contribution lists under readable, minus any
column or table whose name says it holds a secret. Before a statement runs,
the reader's own session is asked which secret columns it can read and the
statement is refused if any; a statement naming a known secret table is
refused before it is sent.

modules/postgres/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 {