Let mdb_admin manage resource groups

[Alena0704]

Let mdb_admin manage resource groups
Add gp_toolkit.pg_create_mdb_admin_role() (gp_toolkit 1.7) to provision the
role with its fixed OID.

Allow members of mdb_admin — not just superusers — to CREATE/ALTER/DROP
resource groups and run pg_resgroup_move_query(), so a cloud admin can tune
their own CPU/memory limits.

Based on pg-sharding/cpg 7b8c912. Some tests are adapted from
open-gpdb/gpdb 3ac99962ad2.

Co-authored-by: reshkereshke@double.cloud
Co-authored-by: Andrey Borodin x4mmm@yandex-team.ru

Fixes #ISSUE_Number

What does this PR do?

Type of Change

• Bug fix (non-breaking change)
• New feature (non-breaking change)
• Breaking change (fix or feature with breaking changes)
• Documentation update

Breaking Changes

Test Plan

• Unit tests added/updated
• Integration tests added/updated
• Passed make installcheck
• Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

• Followed contribution guide
• Added/updated documentation
• Reviewed code for security implications
• This PR contains AI-assisted code generation
• Requested review from cloudberry committers

Additional Context

CI Skip Instructions

---

[Alena0704]

I rewrote the patch completely because a new bootstrap catalog entry bumps CATALOG_VERSION_NO and incompatible with minor gpupgrade - I adapted the commit from open-gpdb 3ac99962ad2 and added my tests.

[my-ship-it]

This PR's permission check looks up the role by name, not by a fixed OID:

 role = get_role_oid("mdb_admin", true); /* look up the role by the name "mdb_admin" */
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR, ...);

This means: members of any role that happens to be named mdb_admin can manage resource groups. The code doesn't care who created that role or when.

Where the hole is: This assumption overlooks CREATEROLE users. Suppose a DBA has granted some user CREATEROLE (quite common on self-hosted clusters,
e.g. to let someone manage ordinary user accounts):

 -- Executed as an ordinary CREATEROLE user:
CREATE ROLE mdb_admin; -- CREATEROLE allows creating any non-superuser role ✓
GRANT mdb_admin TO myself; -- Under PG14 semantics, a CREATEROLE user can grant
-- any non-superuser role to anyone, including themselves ✓
ALTER RESOURCE GROUP default_group SET concurrency 1; -- now passes the permission check!

Three SQL statements, and a user who was only supposed to "manage accounts" has self-granted resource group administration rights.

[Alena0704]

This PR's permission check looks up the role by name, not by a fixed OID:

 role = get_role_oid("mdb_admin", true); /* look up the role by the name "mdb_admin" */
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR, ...);

This means: members of any role that happens to be named mdb_admin can manage resource groups. The code doesn't care who created that role or when.

Where the hole is: This assumption overlooks CREATEROLE users. Suppose a DBA has granted some user CREATEROLE (quite common on self-hosted clusters, e.g. to let someone manage ordinary user accounts):

 -- Executed as an ordinary CREATEROLE user:
CREATE ROLE mdb_admin; -- CREATEROLE allows creating any non-superuser role ✓
GRANT mdb_admin TO myself; -- Under PG14 semantics, a CREATEROLE user can grant
-- any non-superuser role to anyone, including themselves ✓
ALTER RESOURCE GROUP default_group SET concurrency 1; -- now passes the permission check!

Three SQL statements, and a user who was only supposed to "manage accounts" has self-granted resource group administration rights.

Agree, I have fixed it by checking that it is included in admin and system group.

[reshke]

Where the hole is: This assumption overlooks CREATEROLE users. Suppose a DBA has granted some user CREATEROLE (quite common on self-hosted clusters, e.g. to let someone manage ordinary user accounts):

 -- Executed as an ordinary CREATEROLE user:
CREATE ROLE mdb_admin; -- CREATEROLE allows creating any non-superuser role ✓
GRANT mdb_admin TO myself; -- Under PG14 semantics, a CREATEROLE user can grant
-- any non-superuser role to anyone, including themselves ✓
ALTER RESOURCE GROUP default_group SET concurrency 1; -- now passes the permission check!

Three SQL statements, and a user who was only supposed to "manage accounts" has self-granted resource group administration rights.

for our use in Cloud we just do not consider this as privilege escalation, because we grant this role to cloud users(non-superuser) by request (you just click in UI like that this role to this role)

Anyway issue looks valid in general use

[reshke]

This PR's permission check looks up the role by name, not by a fixed OID:

 role = get_role_oid("mdb_admin", true); /* look up the role by the name "mdb_admin" */
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR, ...);

This means: members of any role that happens to be named mdb_admin can manage resource groups. The code doesn't care who created that role or when.

Yep, exactly :) We only care about rolsuper or not, if mdb-admin patch does not allow to gain superuser priviledge - we consider it safe :)

[my-ship-it]

This PR's permission check looks up the role by name, not by a fixed OID:

 role = get_role_oid("mdb_admin", true); /* look up the role by the name "mdb_admin" */
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR, ...);

This means: members of any role that happens to be named mdb_admin can manage resource groups. The code doesn't care who created that role or when.

Yep, exactly :) We only care about rolsuper or not, if mdb-admin patch does not allow to gain superuser priviledge - we consider it safe :)

Thanks for the iterations on this. I agree with the constraint driving the current design — a
new bootstrap entry in pg_authid.dat bumps CATALOG_VERSION_NO and breaks minor-version upgrades,
so a runtime lookup is the right shape for the stable branch.

How about Proposal: use a reserved-prefix role name

Rename the role to e.g. pg_manage_resgroup and keep the same runtime get_role_oid() lookup. The
point is that the pg_ namespace is already reserved for roles:

• CreateRole() rejects any pg_-prefixed name via IsReservedName() (src/backend/commands/user.c),
  so a CREATEROLE user cannot forge it;
• RenameRole() rejects renaming any role to a pg_ name, so it cannot be smuggled in via ALTER
  ROLE ... RENAME either;

One caveat: DROP is not name-protected

[Alena0704]

This PR's permission check looks up the role by name, not by a fixed OID:

 role = get_role_oid("mdb_admin", true); /* look up the role by the name "mdb_admin" */
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR, ...);

This means: members of any role that happens to be named mdb_admin can manage resource groups. The code doesn't care who created that role or when.

Yep, exactly :) We only care about rolsuper or not, if mdb-admin patch does not allow to gain superuser priviledge - we consider it safe :)

Thanks for the iterations on this. I agree with the constraint driving the current design — a new bootstrap entry in pg_authid.dat bumps CATALOG_VERSION_NO and breaks minor-version upgrades, so a runtime lookup is the right shape for the stable branch.

How about Proposal: use a reserved-prefix role name

Rename the role to e.g. pg_manage_resgroup and keep the same runtime get_role_oid() lookup. The point is that the pg_ namespace is already reserved for roles:

• CreateRole() rejects any pg_-prefixed name via IsReservedName() (src/backend/commands/user.c),
  so a CREATEROLE user cannot forge it;
• RenameRole() rejects renaming any role to a pg_ name, so it cannot be smuggled in via ALTER
  ROLE ... RENAME either;

One caveat: DROP is not name-protected

We can't use a pg_ name because creating it requires changes in pg_authid.dat (a bootstrap role), which bumps CATALOG_VERSION_NO and breaks in-place minor upgrades.

postgres=# CREATE ROLE pg_manage_resgroup;
ERROR: role name "pg_manage_resgroup" is reserved
DETAIL: Role names starting with "pg_" are reserved.

I added in the last commit a check in user.c that restricts CREATE/ALTER/RENAME/DROP/GRANT/REVOKE of the mdb_admin and mdb_superuser roles to superusers only, so an ordinary CREATEROLE user cannot hijack the by-name privilege gate (create, rename, reset the password of, or self-grant the role) and thereby escalate privileges.

CREATE ROLE rg_attacker CREATEROLE;
SET ROLE rg_attacker;
GRANT mdb_admin TO rg_attacker; -- self-grant membership
DROP ROLE mdb_admin; -- drop the gate
ALTER ROLE mdb_admin RENAME TO rg_attacker_admin; -- move the real role aside
ALTER ROLE mdb_admin PASSWORD 'hijack'; -- take it over via password reset
CREATE ROLE mdb_superuser; -- create a protected role
RESET ROLE;
DROP ROLE rg_attacker;

CREATE ROLE
SET
ERROR: must be superuser to grant membership in role "mdb_admin"
ERROR: must be superuser to drop role "mdb_admin"
ERROR: must be superuser to rename role "mdb_admin"
ERROR: must be superuser to alter role "mdb_admin"
ERROR: must be superuser to create role "mdb_superuser"
RESET
DROP ROLE

[Alena0704]

Reworked the patch following the pg_aux commit - moved the mdb_admin role into a contrib extension, pg_aux_catalog: it's created via pg_create_mdb_admin_role() (fixed OID 8067, superuser-only). Resource-group operations are gated on mdb_admin membership, identified by that fixed OID.

Because the role is created at a single fixed OID and only by a superuser, it cannot be re-created or taken over by another user - a CREATEROLE user can neither establish mdb_admin nor recreate it at that OID:

postgres=# CREATE ROLE rg_attacker CREATEROLE;
CREATE ROLE
postgres=# SET ROLE rg_attacker;
SET
postgres=> ALTER ROLE mdb_admin RENAME TO rg_attacker_admin;
ERROR: role "mdb_admin" does not exist
postgres=> ALTER ROLE mdb_admin PASSWORD 'hijack';
ERROR: role "mdb_admin" does not exist

Tests were moved into the extension as well.

[my-ship-it]

Reworked the patch following the pg_aux commit - moved the mdb_admin role into a contrib extension, pg_aux_catalog: it's created via pg_create_mdb_admin_role() (fixed OID 8067, superuser-only). Resource-group operations are gated on mdb_admin membership, identified by that fixed OID.

Because the role is created at a single fixed OID and only by a superuser, it cannot be re-created or taken over by another user - a CREATEROLE user can neither establish mdb_admin nor recreate it at that OID:

postgres=# CREATE ROLE rg_attacker CREATEROLE;
CREATE ROLE
postgres=# SET ROLE rg_attacker;
SET
postgres=> ALTER ROLE mdb_admin RENAME TO rg_attacker_admin;
ERROR: role "mdb_admin" does not exist
postgres=> ALTER ROLE mdb_admin PASSWORD 'hijack';
ERROR: role "mdb_admin" does not exist

Tests were moved into the extension as well.

Thanks for the rework — moving to a fixed-OID role provisioned through contrib/pg_aux_catalog is the right call,
and I think it closes the escalation path we were discussing. A few notes:

Security model — LGTM. The original concern (a CREATEROLE user forging a role named mdb_admin and
self-granting it) is now moot, because the gate checks membership in OID 8067 rather than the name:

• A plain CREATE ROLE mdb_admin gets an ordinary OID (≥ FirstNormalObjectId), never 8067, so a forged role is inert.
• next_aux_pg_authid_oid is only set inside the superuser-only function, and 8067 sits in the reserved range that normal OID allocation never hands out.
• I confirmed is_member_of_role() short-circuits for superusers (src/backend/utils/adt/acl.c), so when the extension isn't installed (OID 8067 absent) the behavior cleanly degrades to the previous superuser-only semantics, and superusers never lose access. Nice fallback.

[Alena0704]

Reworked the patch following the pg_aux commit - moved the mdb_admin role into a contrib extension, pg_aux_catalog: it's created via pg_create_mdb_admin_role() (fixed OID 8067, superuser-only). Resource-group operations are gated on mdb_admin membership, identified by that fixed OID.
Because the role is created at a single fixed OID and only by a superuser, it cannot be re-created or taken over by another user - a CREATEROLE user can neither establish mdb_admin nor recreate it at that OID:

postgres=# CREATE ROLE rg_attacker CREATEROLE;
CREATE ROLE
postgres=# SET ROLE rg_attacker;
SET
postgres=> ALTER ROLE mdb_admin RENAME TO rg_attacker_admin;
ERROR: role "mdb_admin" does not exist
postgres=> ALTER ROLE mdb_admin PASSWORD 'hijack';
ERROR: role "mdb_admin" does not exist

Tests were moved into the extension as well.

Thanks for the rework — moving to a fixed-OID role provisioned through contrib/pg_aux_catalog is the right call, and I think it closes the escalation path we were discussing. A few notes:

Security model — LGTM. The original concern (a CREATEROLE user forging a role named mdb_admin and self-granting it) is now moot, because the gate checks membership in OID 8067 rather than the name:

• A plain CREATE ROLE mdb_admin gets an ordinary OID (≥ FirstNormalObjectId), never 8067, so a forged role is inert.
• next_aux_pg_authid_oid is only set inside the superuser-only function, and 8067 sits in the reserved range that normal OID allocation never hands out.
• I confirmed is_member_of_role() short-circuits for superusers (src/backend/utils/adt/acl.c), so when the extension isn't installed (OID 8067 absent) the behavior cleanly degrades to the previous superuser-only semantics, and superusers never lose access. Nice fallback.

Thanks! Yes, that's the intent - the check is on membership in OID 8067, and only a superuser can create the role, so a plain CREATE ROLE mdb_admin is harmless. It's all in the current commit (ae95005).

[leborchuk]

LGTM, checked on internal Cloudberry clusters

[fairyfar]

It seems unnecessary to add the new extension pg_aux_catalog.
Perhaps the newly added interface SQL can be added to gp_toolkit.

[Alena0704]

It seems unnecessary to add the new extension pg_aux_catalog. Perhaps the newly added interface SQL can be added to gp_toolkit.

Agree. I moved it there and updated commit message