From 5e90c3902894788219c3428c742ee99e4e2a0ae7 Mon Sep 17 00:00:00 2001 From: Martin Anker Have Date: Mon, 3 Aug 2026 14:19:50 +0200 Subject: [PATCH 1/2] fix: don't let foreign owned relations block database reconciliation Relations created by extensions are owned by the admin user as CREATE EXTENSION is executed on the admin connection. As extensions are installed into the service schema, the following reconciliation failed with 'permission denied for table hypopg_list_indexes' because GRANT ... ON ALL TABLES IN SCHEMA aborts if a single relation in the schema is owned by another role and the grantor holds no privileges on it. This blocked all further reconciliation of the database, ie. no password updates and no new read/readwrite grants. Grants are now applied per relation, filtered on pg_has_role(current_user, relowner, 'USAGE'), so foreign owned relations are skipped while relations owned by the service user, or by a role it is a member of, are still granted. The latter keeps shared databases working. REVOKE ALL ON ALL TABLES IN SCHEMA public in revokeAllOnPublic had the same latent issue and got the same treatment. --- pkg/postgres/database.go | 67 +++++++++++++- pkg/postgres/database_test.go | 166 ++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+), 3 deletions(-) diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 96d2cee..b37300b 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -293,11 +293,15 @@ func revokeAllOnPublic(log logr.Logger, serviceConnection *sql.DB, serviceCreden log.V(1).Info(fmt.Sprintf("Revoke ALL on role PUBLIC for database '%s'", serviceCredentials.Name)) err := execAsf(serviceConnection, serviceCredentials.User, ` REVOKE ALL ON DATABASE %s from PUBLIC; - REVOKE ALL ON SCHEMA public from PUBLIC; - REVOKE ALL ON ALL TABLES IN SCHEMA public from PUBLIC;`, serviceCredentials.Name) + REVOKE ALL ON SCHEMA public from PUBLIC;`, serviceCredentials.Name) if err != nil { return fmt.Errorf("revoke all for role PUBLIC on database '%s': %w, as %s", serviceCredentials.Name, err, serviceCredentials.User) } + + err = revokeAllOnExistingTablesFromPublicAs(serviceConnection, "public", serviceCredentials.User) + if err != nil { + return fmt.Errorf("revoke all on existing tables in schema public for role PUBLIC on database '%s': %w, as %s", serviceCredentials.Name, err, serviceCredentials.User) + } return nil } @@ -361,13 +365,59 @@ func setDefaultPrivilegesAs(db *sql.DB, schema, role, privileges, actor string) if err != nil { return fmt.Errorf("grant %s privileges on existing schema: %w, as %s", privileges, err, actor) } - err = execAsf(db, actor, fmt.Sprintf("GRANT %s ON ALL TABLES IN SCHEMA %s TO %s", privileges, schema, role)) + err = grantOnExistingTablesAs(db, schema, role, privileges, actor) if err != nil { return fmt.Errorf("grant %s privileges on existing tables: %w, as %s", privileges, err, actor) } return nil } +// grantOnExistingTablesAs grants privileges on the existing relations in schema +// that actor is allowed to grant on. +func grantOnExistingTablesAs(db *sql.DB, schema, role, privileges, actor string) error { + return forEachOwnedRelationAs(db, schema, actor, fmt.Sprintf("GRANT %s ON TABLE %%s TO %s", privileges, role)) +} + +// revokeAllOnExistingTablesFromPublicAs revokes all privileges from the role +// PUBLIC on the existing relations in schema that actor is allowed to revoke +// on. +func revokeAllOnExistingTablesFromPublicAs(db *sql.DB, schema, actor string) error { + return forEachOwnedRelationAs(db, schema, actor, "REVOKE ALL ON TABLE %s FROM PUBLIC") +} + +// forEachOwnedRelationAs executes statement for every relation in schema that +// actor is allowed to grant and revoke privileges on, ie. relations owned by +// actor or by a role that actor is a member of. statement is a PostgreSQL +// format() template with a single %s placeholder for the relation name. +// +// This intentionally does not use the GRANT/REVOKE ... ON ALL TABLES IN SCHEMA +// statements as they fail hard with "permission denied for table x" if just a +// single relation in the schema is owned by another role and actor holds no +// privileges on it. That happens for relations created by extensions, as +// extensions are installed by the admin user, and would block all further +// reconciliation of the database. +func forEachOwnedRelationAs(db *sql.DB, schema, actor, statement string) error { + query := fmt.Sprintf(` + DO $$ + DECLARE + relation regclass; + BEGIN + FOR relation IN + SELECT c.oid::regclass + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = %s + AND c.relkind IN ('r', 'p', 'v', 'm', 'f') + AND pg_has_role(current_user, c.relowner, 'USAGE') + LOOP + EXECUTE format(%s, relation); + END LOOP; + END + $$;`, pq.QuoteLiteral(schema), pq.QuoteLiteral(statement)) + + return execAs(db, actor, query) +} + // execf executes a formatted query on db. func execf(db *sql.DB, query string, args ...interface{}) error { _, err := db.Exec(fmt.Sprintf(query, args...)) @@ -377,6 +427,17 @@ func execf(db *sql.DB, query string, args ...interface{}) error { return nil } +// execAs executes query on db as given role. Contrary to execAsf the query is +// not treated as a format string. +func execAs(db *sql.DB, role string, query string) error { + fullQuery := prependSetRole(query, role) + _, err := db.Exec(fullQuery) + if err != nil { + return fmt.Errorf("unable to execute query '%s'. %w", fullQuery, err) + } + return nil +} + // execf executes a formatted query on db as given role. func execAsf(db *sql.DB, role string, query string, args ...interface{}) error { err := execf(db, prependSetRole(query, role), args...) diff --git a/pkg/postgres/database_test.go b/pkg/postgres/database_test.go index d0c0c3c..a141270 100644 --- a/pkg/postgres/database_test.go +++ b/pkg/postgres/database_test.go @@ -833,6 +833,172 @@ func TestDatabase_mixedOwnershipOnSharedDatabase(t *testing.T) { assert.Equal(t, []string{"value-from-new-user", "value-from-shared-user"}, developerNonOwnedRows, "nonowned rows not as expected") } +// TestDatabase_foreignOwnedRelationInServiceSchema verifies that a relation in +// the service schema owned by another role, eg. a view created by an extension +// installed by the admin user, does not block reconciliation. Such relations +// are skipped when granting privileges on existing tables while relations owned +// by the service user are still granted. +func TestDatabase_foreignOwnedRelationInServiceSchema(t *testing.T) { + postgresqlHost := test.Integration(t) + log := test.SetLogger(t) + managerRole := "postgres_role_name" + + db, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: "postgres", + User: "iam_creator", + Password: "iam_creator", + }) + require.NoError(t, err, "connect to database failed") + defer db.Close() + + require.NoError(t, createManagerRole(log, db, managerRole), "create manager role failed") + + name := fmt.Sprintf("test_%d", time.Now().UnixNano()) + password := "test" + adminCredentials := postgres.Credentials{ + User: "iam_creator", + Password: "iam_creator", + } + serviceCredentials := postgres.Credentials{ + Name: name, + User: name, + Password: password, + } + + err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil) + require.NoError(t, err, "first Database call failed") + + // connect as the admin user and create a relation in the service schema + // owned by the admin user without any privileges granted to the service user. + // This is the state an installed extension leaves behind. + adminConn, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: name, + User: "iam_creator", + Password: "iam_creator", + }) + require.NoError(t, err, "connect as admin to service database failed") + defer adminConn.Close() + + dbExec(t, adminConn, `CREATE TABLE %s.extension_owned (title varchar(40) NOT NULL)`, name) + dbExec(t, adminConn, `CREATE VIEW %s.extension_owned_view AS SELECT * FROM %[1]s.extension_owned`, name) + + // connect as the service user and create an owned table that should still get + // privileges granted + serviceConn, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: name, + User: name, + Password: password, + }) + require.NoError(t, err, "connect as service user to service database failed") + defer serviceConn.Close() + + dbExec(t, serviceConn, `CREATE TABLE %s.owned (title varchar(40) NOT NULL)`, name) + + // reconcile again. This used to fail with + // 'pq: permission denied for table extension_owned' + err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil) + require.NoError(t, err, "second Database call failed") + + assert.True(t, + tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_read", name), fmt.Sprintf("%s.owned", name), "SELECT"), + "read role should have SELECT on the service owned table", + ) + assert.True(t, + tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_readwrite", name), fmt.Sprintf("%s.owned", name), "INSERT"), + "readwrite role should have INSERT on the service owned table", + ) + assert.False(t, + tableHasPrivilege(t, adminConn, fmt.Sprintf("%s_read", name), fmt.Sprintf("%s.extension_owned", name), "SELECT"), + "read role should not have SELECT on the foreign owned table", + ) +} + +func tableHasPrivilege(t *testing.T, db *sql.DB, role, table, privilege string) bool { + t.Helper() + var hasPrivilege bool + err := db.QueryRow("SELECT has_table_privilege($1, $2, $3)", role, table, privilege).Scan(&hasPrivilege) + require.NoError(t, err, "query table privilege failed") + return hasPrivilege +} + +// TestDatabase_foreignOwnedRelationInPublicSchema verifies that a relation in +// the public schema owned by another role, eg. an extension installed manually +// without an explicit schema, does not block reconciliation. Such relations are +// skipped when revoking privileges from PUBLIC while relations owned by the +// service user are still revoked. +func TestDatabase_foreignOwnedRelationInPublicSchema(t *testing.T) { + postgresqlHost := test.Integration(t) + log := test.SetLogger(t) + managerRole := "postgres_role_name" + + db, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: "postgres", + User: "iam_creator", + Password: "iam_creator", + }) + require.NoError(t, err, "connect to database failed") + defer db.Close() + + require.NoError(t, createManagerRole(log, db, managerRole), "create manager role failed") + + name := fmt.Sprintf("test_%d", time.Now().UnixNano()) + password := "test" + adminCredentials := postgres.Credentials{ + User: "iam_creator", + Password: "iam_creator", + } + serviceCredentials := postgres.Credentials{ + Name: name, + User: name, + Password: password, + } + + err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil) + require.NoError(t, err, "first Database call failed") + + // create a relation in the public schema owned by the admin user without any + // privileges granted to the service user. This is the state a manually + // installed extension leaves behind, as public is the default schema. + adminConn, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: name, + User: "iam_creator", + Password: "iam_creator", + }) + require.NoError(t, err, "connect as admin to service database failed") + defer adminConn.Close() + + dbExec(t, adminConn, `CREATE TABLE public.extension_owned (title varchar(40) NOT NULL)`) + + // create a table in public owned by the service user with privileges granted + // to PUBLIC. These privileges are expected to be revoked on reconcile. + serviceConn, err := postgres.Connect(postgres.ConnectionString{ + Host: postgresqlHost, + Database: name, + User: name, + Password: password, + }) + require.NoError(t, err, "connect as service user to service database failed") + defer serviceConn.Close() + + dbExec(t, serviceConn, `CREATE TABLE public.owned (title varchar(40) NOT NULL)`) + dbExec(t, serviceConn, `GRANT SELECT ON public.owned TO PUBLIC`) + + // reconcile again. This used to fail with + // 'pq: permission denied for table extension_owned' + err = postgres.Database(log, postgresqlHost, adminCredentials, serviceCredentials, managerRole, nil) + require.NoError(t, err, "second Database call failed") + + assert.False(t, + tableHasPrivilege(t, adminConn, "public", "public.owned", "SELECT"), + "PUBLIC should not have SELECT on the service owned table in schema public", + ) +} + func TestDatabase_idempotency(t *testing.T) { postgresqlHost := test.Integration(t) log := test.SetLogger(t) From d41cbb1fd7dabed7bcdd741a3bc910214d4ae917 Mon Sep 17 00:00:00 2001 From: Martin Anker Have Date: Tue, 4 Aug 2026 13:30:02 +0200 Subject: [PATCH 2/2] refactor: clarify granting role --- pkg/postgres/database.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index b37300b..aca9642 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -385,10 +385,11 @@ func revokeAllOnExistingTablesFromPublicAs(db *sql.DB, schema, actor string) err return forEachOwnedRelationAs(db, schema, actor, "REVOKE ALL ON TABLE %s FROM PUBLIC") } -// forEachOwnedRelationAs executes statement for every relation in schema that -// actor is allowed to grant and revoke privileges on, ie. relations owned by -// actor or by a role that actor is a member of. statement is a PostgreSQL -// format() template with a single %s placeholder for the relation name. +// forEachOwnedRelationAs executes statement as grantingRole for every relation +// in schema that grantingRole is allowed to grant and revoke privileges on, +// ie. relations owned by grantingRole or by a role that grantingRole is a +// member of. statement is a PostgreSQL format() template with a single %s +// placeholder for the relation name. // // This intentionally does not use the GRANT/REVOKE ... ON ALL TABLES IN SCHEMA // statements as they fail hard with "permission denied for table x" if just a @@ -396,7 +397,7 @@ func revokeAllOnExistingTablesFromPublicAs(db *sql.DB, schema, actor string) err // privileges on it. That happens for relations created by extensions, as // extensions are installed by the admin user, and would block all further // reconciliation of the database. -func forEachOwnedRelationAs(db *sql.DB, schema, actor, statement string) error { +func forEachOwnedRelationAs(db *sql.DB, schema, grantingRole, statement string) error { query := fmt.Sprintf(` DO $$ DECLARE @@ -415,7 +416,7 @@ func forEachOwnedRelationAs(db *sql.DB, schema, actor, statement string) error { END $$;`, pq.QuoteLiteral(schema), pq.QuoteLiteral(statement)) - return execAs(db, actor, query) + return execAs(db, grantingRole, query) } // execf executes a formatted query on db.