Skip to content

Let mdb_admin manage resource groups #1804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Alena0704 wants to merge 1 commit into
base: REL_2_STABLE
Choose a base branch
from
Open

Let mdb_admin manage resource groups #1804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 39 additions & 18 deletions src/backend/commands/resgroupcmds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "commands/resgroupcmds.h"
#include "miscadmin.h"
#include "nodes/pg_list.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datetime.h"
#include "utils/fmgroids.h"
Expand Down Expand Up @@ -102,12 +103,14 @@ CreateResourceGroup(CreateResourceGroupStmt *stmt)
ResGroupCaps caps;
int nResGroups;
MemoryContext oldContext;
Oid role;

/* Permission check - only superuser can create groups. */
if (!superuser())
/* Permission check - only superuser or mdb_admin can create groups. */
role = get_role_oid("mdb_admin", true);
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create resource groups")));
errmsg("must be mdb_admin to create resource groups")));

/*
* Check for an illegal name ('none' is used to signify no group in ALTER ROLE).
Expand Down Expand Up @@ -268,12 +271,14 @@ DropResourceGroup(DropResourceGroupStmt *stmt)
SysScanDesc sscan;
Oid groupid;
ResourceGroupCallbackContext *callbackCtx;
Oid role;

/* Permission check - only superuser can drop resource groups. */
if (!superuser())
/* Permission check - only superuser or mdb_admin can drop resource groups. */
role = get_role_oid("mdb_admin", true);
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to drop resource groups")));
errmsg("must be mdb_admin to drop resource groups")));

/*
* Check the pg_resgroup relation to be certain the resource group already
Expand Down Expand Up @@ -302,6 +307,13 @@ DropResourceGroup(DropResourceGroupStmt *stmt)
*/
groupid = ((Form_pg_resgroup) GETSTRUCT(tuple))->oid;

/* Permission check - only superuser can drop the admin/system resource groups. */
if (!superuser() && (groupid == ADMINRESGROUP_OID || groupid == SYSTEMRESGROUP_OID))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to drop resource group \"%s\"",
stmt->name)));

/* cannot DROP default resource groups */
if (groupid == DEFAULTRESGROUP_OID
|| groupid == ADMINRESGROUP_OID
Expand Down Expand Up @@ -374,12 +386,27 @@ AlterResourceGroup(AlterResourceGroupStmt *stmt)
char *io_limit = NULL;
ResourceGroupCallbackContext *callbackCtx;
MemoryContext oldContext;
Oid role;

/* Permission check - only mdb_admin can alter resource groups. */
role = get_role_oid("mdb_admin", true);
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be mdb_admin to alter resource groups")));

/*
* Check the pg_resgroup relation to be certain the resource group already
* exists.
*/
groupid = get_resgroup_oid(stmt->name, false);

/* Permission check - only superuser can alter resource groups. */
if (!superuser())
/* Permission check - only superuser can alter the admin/system resource groups. */
if (!superuser() && (groupid == ADMINRESGROUP_OID || groupid == SYSTEMRESGROUP_OID))
ereport(ERROR,
Comment thread
Alena0704 marked this conversation as resolved.
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to alter resource groups")));
errmsg("must be superuser to alter resource group \"%s\"",
stmt->name)));

/* Currently we only support to ALTER one limit at one time */
Assert(list_length(stmt->options) == 1);
Expand All @@ -406,12 +433,6 @@ AlterResourceGroup(AlterResourceGroupStmt *stmt)
checkResgroupCapLimit(limitType, value);
}

/*
* Check the pg_resgroup relation to be certain the resource group already
* exists.
*/
groupid = get_resgroup_oid(stmt->name, false);

if (limitType == RESGROUP_LIMIT_TYPE_CONCURRENCY &&
value == 0 &&
groupid == ADMINRESGROUP_OID)
Expand Down Expand Up @@ -500,7 +521,7 @@ AlterResourceGroup(AlterResourceGroupStmt *stmt)
RESGROUP_DEFAULT_CPU_WEIGHT, "");

updateResgroupCapabilityEntry(pg_resgroupcapability_rel,
groupid, RESGROUP_LIMIT_TYPE_CPUSET,
groupid, RESGROUP_LIMIT_TYPE_CPUSET,
0, caps.cpuset);
}
else if (limitType == RESGROUP_LIMIT_TYPE_CPU)
Expand Down Expand Up @@ -1007,7 +1028,7 @@ parseStmtOptions(CreateResourceGroupStmt *stmt, ResGroupCaps *caps)
else
mask |= 1 << type;

if (type == RESGROUP_LIMIT_TYPE_CPUSET)
if (type == RESGROUP_LIMIT_TYPE_CPUSET)
{
const char *cpuset = defGetString(defel);
strlcpy(caps->cpuset, cpuset, sizeof(caps->cpuset));
Expand Down Expand Up @@ -1611,7 +1632,7 @@ checkCpuSetByRole(const char *cpuset)
* ex:
* cpuset = "1;4"
* then we should assign '1' to corrdinator and '4' to segment
*
*
* cpuset = "1"
* assign '1' to both coordinator and segment
*/
Expand Down
103 changes: 102 additions & 1 deletion src/backend/commands/user.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,42 @@ have_createrole_privilege(void)
return has_createrole_privilege(GetUserId());
}

/*
* is_mdb_protected_rolename
*
* mdb_admin / mdb_superuser are privilege roles resolved by name at runtime
* (see acl.c). Since the gate is keyed on the name, only superusers may manage
* these roles.
*/
static bool
is_mdb_protected_rolename(const char *rolename)
{
return rolename != NULL &&
(strcmp(rolename, "mdb_admin") == 0 ||
strcmp(rolename, "mdb_superuser") == 0);
}


/*
* may_manage_mdb_role_membership
*
* Membership in mdb_admin / mdb_superuser is the gate that confers their
* privileges (see acl.c), so it must not be self-granted by an arbitrary
* CREATEROLE holder. Permit it only for superusers (the control plane) and
* for members of mdb_superuser, who are trusted to delegate the gate.
*/
static bool
may_manage_mdb_role_membership(void)
{
Oid mdb_superuser_roleoid;

if (superuser())
return true;

mdb_superuser_roleoid = get_role_oid("mdb_superuser", true);
return OidIsValid(mdb_superuser_roleoid) &&
is_member_of_role(GetUserId(), mdb_superuser_roleoid);
}

/*
* CREATE ROLE
Expand Down Expand Up @@ -505,6 +541,16 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
stmt->role),
errdetail("Role names starting with \"pg_\" are reserved.")));

/*
* Only superusers may create the mdb_admin / mdb_superuser privilege roles,
* so a CREATEROLE holder cannot create and own the gate.
*/
if (is_mdb_protected_rolename(stmt->role) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create role \"%s\"",
stmt->role)));

/*
* If built with appropriate switch, whine when regression-testing
* conventions for role names are violated.
Expand Down Expand Up @@ -1283,6 +1329,15 @@ AlterRole(AlterRoleStmt *stmt)

bWas_super = ((Form_pg_authid) GETSTRUCT(tuple))->rolsuper;

/*
* Only superusers may alter the mdb_admin / mdb_superuser privilege roles
* (e.g. reset their password), so a CREATEROLE holder cannot take them over.
*/
if (is_mdb_protected_rolename(rolename) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to alter role \"%s\"", rolename)));

if (authform->rolsuper || issuper >= 0)
{
if (!superuser())
Expand Down Expand Up @@ -2087,6 +2142,15 @@ DropRole(DropRoleStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to drop superusers")));

/*
* Likewise, only superusers may drop the mdb_admin / mdb_superuser
* privilege roles, so the gate cannot be removed and re-created.
*/
if (is_mdb_protected_rolename(role) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to drop role \"%s\"", role)));

/* DROP hook for the role being removed */
InvokeObjectDropHook(AuthIdRelationId, roleid, 0);

Expand Down Expand Up @@ -2182,7 +2246,7 @@ DropRole(DropRoleStmt *stmt)
*/
DeleteSharedComments(roleid, AuthIdRelationId);
DeleteSharedSecurityLabel(roleid, AuthIdRelationId);

/*
* Delete any tag description and associated dependencies.
*/
Expand Down Expand Up @@ -2330,6 +2394,18 @@ RenameRole(const char *oldname, const char *newname)
errmsg("permission denied to rename role")));
}

/*
* Only superusers may rename the mdb_admin / mdb_superuser privilege roles,
* in either direction, so a CREATEROLE holder cannot move the real role out
* of the way or claim the protected name for a role they control.
*/
if ((is_mdb_protected_rolename(oldname) ||
is_mdb_protected_rolename(newname)) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to rename role \"%s\"",
is_mdb_protected_rolename(oldname) ? oldname : newname)));

/* OK, construct the modified tuple */
for (i = 0; i < Natts_pg_authid; i++)
repl_repl[i] = false;
Expand Down Expand Up @@ -2598,6 +2674,18 @@ AddRoleMems(const char *rolename, Oid roleid,
}
else
{
/*
* Membership in the mdb_admin / mdb_superuser privilege roles may only
* be granted by a superuser (the control plane) or a member of
* mdb_superuser; otherwise any CREATEROLE holder could self-grant the
* gate.
*/
if (is_mdb_protected_rolename(rolename) && !may_manage_mdb_role_membership())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or a member of mdb_superuser to grant membership in role \"%s\"",
rolename)));

if (!have_createrole_privilege() &&
!is_admin_of_role(grantorId, roleid))
ereport(ERROR,
Expand Down Expand Up @@ -3069,6 +3157,19 @@ DelRoleMems(const char *rolename, Oid roleid,
}
else
{

/*
* Membership in the mdb_admin / mdb_superuser privilege roles may only
* be revoked by a superuser (the control plane) or a member of
* mdb_superuser; otherwise a CREATEROLE holder could lock the legitimate
* admin out of the gate.
*/
if (is_mdb_protected_rolename(rolename) && !may_manage_mdb_role_membership())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or a member of mdb_superuser to revoke membership in role \"%s\"",
rolename)));

if (!have_createrole_privilege() &&
!is_admin_of_role(GetUserId(), roleid))
ereport(ERROR,
Expand Down
7 changes: 5 additions & 2 deletions src/backend/utils/resgroup/resgroup_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "cdb/cdbvars.h"
#include "commands/resgroupcmds.h"
#include "storage/procarray.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/datetime.h"
#include "utils/resgroup.h"
Expand Down Expand Up @@ -458,16 +459,18 @@ pg_resgroup_move_query(PG_FUNCTION_ARGS)
int sessionId;
Oid groupId;
const char *groupName;
Oid role;

if (!IsResGroupEnabled())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("resource group is not enabled"))));

if (!superuser())
role = get_role_oid("mdb_admin", true);
if (!is_member_of_role(GetUserId(), role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser to move query"))));
(errmsg("must be mdb_admin to move query"))));

if (Gp_role == GP_ROLE_DISPATCH)
{
Expand Down
Loading