Grant minimum required permissions for non-superuser usage

[ardentperf]

Version 1.5.2 of pg_repack allowed non-superusers to repack their own tables by updating privilege checks in several backend functions (PR #431). This capability is important for environments where platform teams manage postgres installation and upgrades, but non-superusers directly manage their own schemas and tables including repacks. However version 1.5.2 did not grant the actual privileges to run pg_repack to non-superusers. This change adds the missing grants and allows table owners to use pg_repack with --no-superuser-check. This also adds a successful non-superuser test case, which was missing before.

Importantly, table ownership permissions still prevent users from accessing intermediate repack data and log tables of other users. (cf https://github.com/ardentperf/pg_repack_isolation/blob/main/test_multiuser_isolation.log )

[za-arthur]

Thank you @ardentperf,

Would it make sense to grant permissions only to pg_maintain role instead of broad public? Although this would work only for Postgres 17+ I don't think it makes sense to grant access to everybody.

Moreover we could allow in pg_repack to repack to those who has pg_maintain privileges, in addition to owners.

[za-arthur]

There is a relevant PR #451

[harinath001]

@za-arthur @ardentperf

this patch #451 offers a more secure and maintainable solution by leveraging PostgreSQL's native trusted extension mechanism (available in PostgreSQL 13+).

the approach uses the @extowner@ placeholder to grant permissions exclusively to the extension owner, allowing controlled delegation without exposing repack capabilities to all users by default. it better aligns with PostgreSQL's security model for trusted extensions, where the extension owner (who has explicit CREATE privileges on the database) becomes the natural administrator of the extension and schema.

Also, we don't need to modify any test cases as the patch is backward compatible.

let me know if this looks good, appreciate any reviews on the PR and waiting for inclusion of the patch in future releases.

Thanks.

[ardentperf]

@za-arthur pg_repack is effectively a substitute for vacuum full with less downtime. This is the biggest use case for pg_repack. I think the most sensible thing here is that it should operate with the same permission model - if someone has privileges to do a vacuum full then they should have privs to do repack. Is there a specific threat we're protecting against by blocking pg_repack access from people who can do a vacuum full?

Would it make sense to grant permissions only to pg_maintain role instead of broad public? Although this would work only for Postgres 17+ I don't think it makes sense to grant access to everybody.

Version 1.5.2 of pg_repack allowed non-superusers to repack their own tables (but not other people's tables) by updating privilege checks in several backend functions. Importantly, table ownership permissions still prevent users from accessing intermediate repack data and log tables of other users. This proposed PR does not grant access to public to successfully repack arbitrary tables.

The third regression test verifies that the nosuper user cannot repack a table that isn't owned by nosuper. This being said, it wouldn't hurt for someone to double-check my work here and have a second set of eyes confirm that there aren't any accidental corner cases allowing a non-superuser to successfully repack tables that couldn't already be the target of a vacuum full. I don't think we need a regression test with two different non-superusers, as the third test should provide sufficient coverage for now? Are there other privilege model situations worth adding regression tests for? Should we add more comments to the regression tests explaining why we intentionally want the third test to fail and prove the security model works?

[za-arthur]

@harinath001 #451 won't work since it grants privileges to a user who installed the extension, which is a superuser currently, because pg_repack isn't marked as trusted extension. @ardentperf 's idea is to allow owners of tables to execute pg_repack and #451 doesn't help with that.

pg_repack is effectively a substitute for vacuum full with less downtime. This is the biggest use case for pg_repack. I think the most sensible thing here is that it should operate with the same permission model - if someone has privileges to do a vacuum full then they should have privs to do repack.

@ardentperf still they are implemented differently and might require different permission model. The least difference from the user point of view is the additional repack schema.

Is there a specific threat we're protecting against by blocking pg_repack access from people who can do a vacuum full?

There are might be environments and users who might need to have more tight and strict permissions. And if we merge this PR they might need to execute additional REVOKE/GRANT script to narrow down access to repack schema. Which is essentially is a bit similar to the current approach when you need to execute additional GRANT script if you want to allow non-superuser access. And I suppose we need to choose what default is "better", permissive or restrictive. Current Postgres default is to restrict permissions (at least CREATE on public schema) of public role which aligns with pg_repack's defaults.

I think another approach to address the issue might be to allow to create temporary objects on a same schema where is the original table. This at least doesn't require granting CREATE permissions on the repack schema. Which might create another issues like naming conflicts and cleanup if repack failed.

[harinath001]

@harinath001 #451 won't work since it grants privileges to a user who installed the extension, which is a superuser currently, because pg_repack isn't marked as trusted extension. @ardentperf 's idea is to allow owners of tables to execute pg_repack and #451 doesn't help with that.

@za-arthur the intention of the patch is to make the change backward compatible (and not to modify the default value of trusted flag ), hence if the trusted flag is marked as false (which is by default ), then only superusers can run pg_repack, but given if the database admins want to change the flag to true in extension control file, then non-superusers should be able to run pg_repack.

BTW, I also tested the patch by modifying the trusted flag to true and running pg_repack with flag --no-superuser-check and it worked.

As Postgres is giving more freedom for admins to modify the control file of extension, for example introduction of extension_control_path in pg-18 Ref link , making pg_repack work for non-superusers if admin permits, will be super useful.

[ardentperf]

@za-arthur thanks, you're right - I missed the fact that vacuum full works without having CREATE privileges on any schema, but pg_repack requires granting this. (USAGE is required for vacuum full also.) Postgres 14 and older granted CREATE on the public schema to all users, but starting with Postgres 15 only USAGE is granted to public.

docker run -e POSTGRES_PASSWORD=jeremy -p 5432:5432 postgres

postgres=# CREATE ROLE nosuper LOGIN;
CREATE SCHEMA app_schema;
CREATE TABLE app_schema.data_table (id int);
ALTER TABLE app_schema.data_table OWNER TO nosuper;
GRANT USAGE ON SCHEMA app_schema TO nosuper;
CREATE ROLE
CREATE SCHEMA
CREATE TABLE
ALTER TABLE
GRANT

postgres=# SET ROLE nosuper;
SET

postgres=> VACUUM FULL app_schema.data_table;    -- succeeds
VACUUM

postgres=> CREATE TABLE app_schema.new_table (id int);  -- fails
ERROR:  permission denied for schema app_schema
LINE 1: CREATE TABLE app_schema.new_table (id int);
                     ^

[ardentperf]

Thinking about this some more - it seems to me that it's ok to grant USAGE, EXECUTE ON ALL FUNCTIONS, and SELECT ON ALL TABLES to public for the repack schema.

The only question I see is around CREATE on the schema.

The background of revoking CREATE from the public schema starting with postgres v15 was that it was initially driven by search_path concerns and CVE-2018-1058. The search_path concerns don't carry over to the repack schema, which isn't included in the search path by default (unlike public).

However, I do see another consideration - since the default behavior now is that newly created users can't create tables anywhere by default, if pg_repack has a different default behavior then users might just start putting all kind of stuff in the repack schema as a workaround to admins not granting them the right privs after creating the new roles. I think it would be a good idea for pg_repack to match the default behavior of postgres for newly create roles, and not grant default privs to create tables.

This does mean that users will need to manually grant the CREATE privilege on repack for normal users to be able to repack their own tables.

I see the pg_maintain role as a bit more privileged because it actually grants people the ability to vacuum tables they don't own. And this introduces another wrinkle: if pg_repack creates intermediate objects in the same schema as the target table, then someone with pg_maintain now needs CREATE on those user schemas to repack tables they don't own - but they don't need the user schema CREATE privilege to vacuum those same tables.

For this reason I'd actually propose that the best solution here is keeping intermediate objects in the repack schema, granting the CREATE privilege to the pg_maintain, and documenting that regular users (without pg_maintain) will need CREATE on the repack schema to repack their own tables. We should also document the reasoning for this: because starting in pg15 the default behavior of postgres is not to allow CREATE in public by default for new users, and we want to match that default behavior for the repack schema so that unprivileged users don't start putting all kind of stuff in repack as a workaround to admins not granting them the right privs.

something else to consider is whether we still need a --no-superuser-check command line argument at all?

i'm going to go ahead and make an initial update to this PR to switch the CREATE grant to pg_maintain. I think the other three privs can be granted to public - and this makes it easier for users since they only need a single grant to allow unprivileged users to run repack on their own tables. i think the pg_maintain role may not exist before pg14 but the regression tests go back to 9.6 so i'll do some testing and see what happens on older versions

---

on a side note - i found that pg_repack is compatible with https://github.com/nektos/act to locally run the full test matrix from https://github.com/reorg/pg_repack/actions which can test all major versions of PG back to 9.5 in parallel. I found it useful and a bit quicker than running the tests locally or in my personal github for this many different major versions.

act -j test --network bridge 2>&1 | tee act_test_maintain.log

or

act -j test --network bridge --matrix pg:17 --matrix pg:14 --matrix pg:10 2>&1 | tee act_test_maintain.log

(need the bridge network to avoid listening port conflicts)

[ardentperf]

granting privs to pg_maintain does not allow running pg_repack on tables owner by others, but VACUUM FULL does work. because of the other privs needed, i don't think we can make pg_repack work seamlessly with pg_maintain

│[make installcheck/PostgreSQL 17]   | +-- Test with a user who is a member of pg_maintain
│[make installcheck/PostgreSQL 17]   | +-- pg_maintain members should be able to repack any table without additional grants
│[make installcheck/PostgreSQL 17]   | +VACUUM FULL tbl_cluster;
│[make installcheck/PostgreSQL 17]   | +-- => OK (member of pg_maintain can use --no-superuser-check on any table)
│[make installcheck/PostgreSQL 17]   | +\! pg_repack --dbname=contrib_regression --table=tbl_cluster --username=maintain_user --no-superuser-check
│[make installcheck/PostgreSQL 17]   | +INFO: repacking table "public.tbl_cluster"
│[make installcheck/PostgreSQL 17]   | +ERROR: query failed: ERROR:  permission denied for table tbl_cluster
│[make installcheck/PostgreSQL 17]   | +DETAIL: query was: CREATE TRIGGER repack_trigger AFTER INSERT OR DELETE OR UPDATE ON public.tbl_cluster FOR EACH ROW EXECUTE PROCEDURE repack.repack_trigger(',")', 'col1')
│[make installcheck/PostgreSQL 17]   | +ERROR: query failed: ERROR:  must be owner of table tbl_cluster
│[make installcheck/PostgreSQL 17]   | +DETAIL: query was: SELECT repack.repack_drop($1, $2)

[ardentperf]

i don't see any path to allowing users with pg_maintain to repack tables they don't own because of privilege requirements like CREATE TRIGGER on the target table, which aren't included in pg_maintain.

I think another approach to address the issue might be to allow to create temporary objects on a same schema where is the original table. This at least doesn't require granting CREATE permissions on the repack schema. Which might create another issues like naming conflicts and cleanup if repack failed.

After ruling out pg_maintain, I now think this is a good idea. Because intermediate objects are hidden in the repack schema I think users easily forget about cleanup after failures - it might actually be a benefit if it's a little easier for users to notice the intermediate objects?

[za-arthur]

@ardentperf thanks for checking pg_maintain privileges!

The background of revoking CREATE from the public schema starting with postgres v15 was that it was initially driven by search_path concerns and GHSA-wj3f-f94q-2r98. The search_path concerns don't carry over to the repack schema, which isn't included in the search path by default (unlike public).

Indeed, that is the reason public doesn't have CREATE privileges on the public schema by default. I thought it might be good to stick with although pg_repack hardcodes search_path in a few places: here, here and here

pg_repack has a different default behavior then users might just start putting all kind of stuff in the repack schema as a workaround to admins not granting them the right privs after creating the new roles. I think it would be a good idea for pg_repack to match the default behavior of postgres for newly create roles, and not grant default privs to create tables.

Good point! That is definitely is a good reason to stick with the default Postgres behavior.

After ruling out pg_maintain, I now think this is a good idea. Because intermediate objects are hidden in the repack schema I think users easily forget about cleanup after failures - it might actually be a benefit if it's a little easier for users to notice the intermediate objects?

Yeah, I think I could try this option. We would need to add an additional option, like --user-original-schema. There is a risk of having naming conflicts. pg_repack has tables log_<uid> and table_<uid> and a type pk_<uid> which doesn't look very unique and a user might not understand what are these tables for, since they are out of the repack schema now and their names are not very descriptive. We could just add repack_ into the name of tables and the type:

public.repack_log_<uid>
public.repack_table_<uid>
public.repack_pk_<uid>

This could add a little bit of understanding for a user what are these tables for. And we would need to acquire ACCESS SHARE lock on these tables to restrict accidental dropping by a user.

[za-arthur]

something else to consider is whether we still need a --no-superuser-check command line argument at all?

I originally left it for backwards compatibility since if we remove it pg_repack will start failing. But we had a couple of release since then now and we definitely should consider removing it.

[ardentperf]

something else to consider is whether we still need a --no-superuser-check command line argument at all?

I originally left it for backwards compatibility since if we remove it pg_repack will start failing. But we had a couple of release since then now and we definitely should consider removing it.

Alternatively, could make it a no-op argument that doesn't do anything but leave it around so that we don't break scripts. Either option could work here