diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 96d2cee..aca9642 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,60 @@ 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 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 +// 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, grantingRole, 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, grantingRole, 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 +428,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)