From e5b8a4c098ad6add39626a14475148872cd687e0 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Tue, 10 Jan 2023 12:44:49 -0500 Subject: [PATCH] Add new GUC createrole_self_grant. Can be set to the empty string, or to either or both of "set" or "inherit". If set to a non-empty value, a non-superuser who creates a role (necessarily by relying up the CREATEROLE privilege) will grant that role back to themselves with the specified options. This isn't a security feature, because the grant that this feature triggers can also be performed explicitly. Instead, it's a user experience feature. A superuser would necessarily inherit the privileges of any created role and be able to access all such roles via SET ROLE; with this patch, you can configure createrole_self_grant = 'set, inherit' to provide a similar experience for a user who has CREATEROLE but not SUPERUSER. Discussion: https://postgr.es/m/CA+TgmobN59ct+Emmz6ig1Nua2Q-_o=r6DSD98KfU53kctq_kQw@mail.gmail.com --- doc/src/sgml/config.sgml | 33 +++++++ doc/src/sgml/ref/create_role.sgml | 1 + doc/src/sgml/ref/createuser.sgml | 1 + src/backend/commands/user.c | 97 ++++++++++++++++++- src/backend/utils/misc/guc_tables.c | 12 +++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/commands/user.h | 10 +- src/test/regress/expected/create_role.out | 33 +++++++ src/test/regress/sql/create_role.sql | 37 +++++++ 9 files changed, 220 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 2fec613484..77574e2d4e 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9447,6 +9447,39 @@ SET XML OPTION { DOCUMENT | CONTENT }; + + createrole_self_grant (string) + + createrole_self_grant + configuration parameter + + + + + If a user who has CREATEROLE but not + SUPERUSER creates a role, and if this + is set to a non-empty value, the newly-created role will be granted + to the creating user with the options specified. The value must be + set, inherit, or a + comma-separated list of these. + + + The purpose of this option is to allow a CREATEROLE + user who is not a superuser to automatically inherit, or automatically + gain the ability to SET ROLE to, any created users. + Since a CREATEROLE user is always implicitly granted + ADMIN OPTION on created roles, that user could + always execute a GRANT statement that would achieve + the same effect as this setting. However, it can be convenient for + usability reasons if the grant happens automatically. A superuser + automatically inherits the privileges of every role and can always + SET ROLE to any role, and this setting can be used + to produce a similar behavior for CREATEROLE users + for users which they create. + + + + diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml index 0863acbcac..7ce4e38b45 100644 --- a/doc/src/sgml/ref/create_role.sgml +++ b/doc/src/sgml/ref/create_role.sgml @@ -506,6 +506,7 @@ CREATE ROLE name [ WITH ADMIN + diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml index f91dc500a4..9a1c3d01f4 100644 --- a/doc/src/sgml/ref/createuser.sgml +++ b/doc/src/sgml/ref/createuser.sgml @@ -555,6 +555,7 @@ PostgreSQL documentation + diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 1ae2d0a66f..4d193a6f9a 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -39,6 +39,7 @@ #include "utils/fmgroids.h" #include "utils/syscache.h" #include "utils/timestamp.h" +#include "utils/varlena.h" /* * Removing a role grant - or the admin option on it - might recurse to @@ -81,8 +82,11 @@ typedef struct #define GRANT_ROLE_SPECIFIED_INHERIT 0x0002 #define GRANT_ROLE_SPECIFIED_SET 0x0004 -/* GUC parameter */ +/* GUC parameters */ int Password_encryption = PASSWORD_TYPE_SCRAM_SHA_256; +char *createrole_self_grant = ""; +bool createrole_self_grant_enabled = false; +GrantRoleOptions createrole_self_grant_options; /* Hook to check passwords in CreateRole() and AlterRole() */ check_password_hook_type check_password_hook = NULL; @@ -532,10 +536,13 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) if (!superuser()) { RoleSpec *current_role = makeNode(RoleSpec); - GrantRoleOptions poptself; + GrantRoleOptions poptself; + List *memberSpecs; + List *memberIds = list_make1_oid(currentUserId); current_role->roletype = ROLESPEC_CURRENT_ROLE; current_role->location = -1; + memberSpecs = list_make1(current_role); poptself.specified = GRANT_ROLE_SPECIFIED_ADMIN | GRANT_ROLE_SPECIFIED_INHERIT @@ -545,7 +552,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) poptself.set = false; AddRoleMems(BOOTSTRAP_SUPERUSERID, stmt->role, roleid, - list_make1(current_role), list_make1_oid(GetUserId()), + memberSpecs, memberIds, BOOTSTRAP_SUPERUSERID, &poptself); /* @@ -553,6 +560,20 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) * the additional grants will fail. */ CommandCounterIncrement(); + + /* + * Because of the implicit grant above, a CREATEROLE user who creates + * a role has the ability to grant that role back to themselves with + * the INHERIT or SET options, if they wish to inherit the role's + * privileges or be able to SET ROLE to it. The createrole_self_grant + * GUC can be used to make this happen automatically. This has no + * security implications since the same user is able to make the same + * grant using an explicit GRANT statement; it's just convenient. + */ + if (createrole_self_grant_enabled) + AddRoleMems(currentUserId, stmt->role, roleid, + memberSpecs, memberIds, + currentUserId, &createrole_self_grant_options); } /* @@ -2414,3 +2435,73 @@ InitGrantRoleOptions(GrantRoleOptions *popt) popt->inherit = false; popt->set = true; } + +/* + * GUC check_hook for createrole_self_grant + */ +bool +check_createrole_self_grant(char **newval, void **extra, GucSource source) +{ + char *rawstring; + List *elemlist; + ListCell *l; + unsigned options = 0; + unsigned *result; + + /* Need a modifiable copy of string */ + rawstring = pstrdup(*newval); + + if (!SplitIdentifierString(rawstring, ',', &elemlist)) + { + /* syntax error in list */ + GUC_check_errdetail("List syntax is invalid."); + pfree(rawstring); + list_free(elemlist); + return false; + } + + foreach(l, elemlist) + { + char *tok = (char *) lfirst(l); + + if (pg_strcasecmp(tok, "SET") == 0) + options |= GRANT_ROLE_SPECIFIED_SET; + else if (pg_strcasecmp(tok, "INHERIT") == 0) + options |= GRANT_ROLE_SPECIFIED_INHERIT; + else + { + GUC_check_errdetail("Unrecognized key word: \"%s\".", tok); + pfree(rawstring); + list_free(elemlist); + return false; + } + } + + pfree(rawstring); + list_free(elemlist); + + result = (unsigned *) guc_malloc(LOG, sizeof(unsigned)); + *result = options; + *extra = result; + + return true; +} + +/* + * GUC assign_hook for createrole_self_grant + */ +void +assign_createrole_self_grant(const char *newval, void *extra) +{ + unsigned options = * (unsigned *) extra; + + createrole_self_grant_enabled = (options != 0); + createrole_self_grant_options.specified = GRANT_ROLE_SPECIFIED_ADMIN + | GRANT_ROLE_SPECIFIED_INHERIT + | GRANT_ROLE_SPECIFIED_SET; + createrole_self_grant_options.admin = false; + createrole_self_grant_options.inherit = + (options & GRANT_ROLE_SPECIFIED_INHERIT) != 0; + createrole_self_grant_options.set = + (options & GRANT_ROLE_SPECIFIED_SET) != 0; +} diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 92545b4958..5025e80f89 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -3949,6 +3949,18 @@ struct config_string ConfigureNamesString[] = check_temp_tablespaces, assign_temp_tablespaces, NULL }, + { + {"createrole_self_grant", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Sets whether a CREATEROLE user automatically grants " + "the role to themselves, and with which options."), + NULL, + GUC_LIST_INPUT + }, + &createrole_self_grant, + "", + check_createrole_self_grant, assign_createrole_self_grant, NULL + }, + { {"dynamic_library_path", PGC_SUSET, CLIENT_CONN_OTHER, gettext_noop("Sets the path for dynamically loadable modules."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index c2ada92054..4cceda4162 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -703,6 +703,7 @@ #xmlbinary = 'base64' #xmloption = 'content' #gin_pending_list_limit = 4MB +#createrole_self_grant = '' # set and/or inherit # - Locale and Formatting - diff --git a/src/include/commands/user.h b/src/include/commands/user.h index 54c720d880..97dcb93791 100644 --- a/src/include/commands/user.h +++ b/src/include/commands/user.h @@ -15,9 +15,11 @@ #include "libpq/crypt.h" #include "nodes/parsenodes.h" #include "parser/parse_node.h" +#include "utils/guc.h" -/* GUC. Is actually of type PasswordType. */ -extern PGDLLIMPORT int Password_encryption; +/* GUCs */ +extern PGDLLIMPORT int Password_encryption; /* values from enum PasswordType */ +extern PGDLLIMPORT char *createrole_self_grant; /* Hook to check passwords in CreateRole() and AlterRole() */ typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null); @@ -34,4 +36,8 @@ extern void DropOwnedObjects(DropOwnedStmt *stmt); extern void ReassignOwnedObjects(ReassignOwnedStmt *stmt); extern List *roleSpecsToIds(List *memberNames); +extern bool check_createrole_self_grant(char **newval, void **extra, + GucSource source); +extern void assign_createrole_self_grant(const char *newval, void *extra); + #endif /* USER_H */ diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out index f5f745504c..bed3749888 100644 --- a/src/test/regress/expected/create_role.out +++ b/src/test/regress/expected/create_role.out @@ -1,6 +1,7 @@ -- ok, superuser can create users with any set of privileges CREATE ROLE regress_role_super SUPERUSER; CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS; +GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION; CREATE ROLE regress_role_normal; -- fail, only superusers can create users with these privileges SET SESSION AUTHORIZATION regress_role_admin; @@ -15,6 +16,7 @@ ERROR: must be superuser to create bypassrls users -- ok, having CREATEROLE is enough to create users with these privileges CREATE ROLE regress_createdb CREATEDB; CREATE ROLE regress_createrole CREATEROLE NOINHERIT; +GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION; CREATE ROLE regress_login LOGIN; CREATE ROLE regress_inherit INHERIT; CREATE ROLE regress_connection_limit CONNECTION LIMIT 5; @@ -83,9 +85,37 @@ ALTER VIEW tenant_view OWNER TO regress_role_admin; ERROR: must be owner of view tenant_view DROP VIEW tenant_view; ERROR: must be owner of view tenant_view +-- fail, can't create objects owned as regress_tenant +CREATE SCHEMA regress_tenant_schema AUTHORIZATION regress_tenant; +ERROR: must be able to SET ROLE "regress_tenant" -- fail, we don't inherit permissions from regress_tenant REASSIGN OWNED BY regress_tenant TO regress_createrole; ERROR: permission denied to reassign objects +-- ok, create a role with a value for createrole_self_grant +SET createrole_self_grant = 'set, inherit'; +CREATE ROLE regress_tenant2; +GRANT CREATE ON DATABASE regression TO regress_tenant2; +-- ok, regress_tenant2 can create objects within the database +SET SESSION AUTHORIZATION regress_tenant2; +CREATE TABLE tenant2_table (i integer); +REVOKE ALL PRIVILEGES ON tenant2_table FROM PUBLIC; +-- ok, because we have SET and INHERIT on regress_tenant2 +SET SESSION AUTHORIZATION regress_createrole; +CREATE SCHEMA regress_tenant2_schema AUTHORIZATION regress_tenant2; +ALTER SCHEMA regress_tenant2_schema OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_tenant2; +-- with SET but not INHERIT, we can give away objects but not take them +REVOKE INHERIT OPTION FOR regress_tenant2 FROM regress_createrole; +ALTER SCHEMA regress_tenant2_schema OWNER TO regress_tenant2; +ALTER TABLE tenant2_table OWNER TO regress_createrole; +ERROR: must be owner of table tenant2_table +-- with INHERIT but not SET, we can take objects but not give them away +GRANT regress_tenant2 TO regress_createrole WITH INHERIT TRUE, SET FALSE; +ALTER TABLE tenant2_table OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_tenant2; +ERROR: must be able to SET ROLE "regress_tenant2" +DROP TABLE tenant2_table; -- fail, CREATEROLE is not enough to create roles in privileged roles CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data; ERROR: must have admin option on role "pg_read_all_data" @@ -131,6 +161,8 @@ ERROR: role "regress_nosuch_recursive" does not exist DROP ROLE regress_nosuch_admin_recursive; ERROR: role "regress_nosuch_admin_recursive" does not exist DROP ROLE regress_plainrole; +-- must revoke privileges before dropping role +REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE; -- ok, should be able to drop non-superuser roles we created DROP ROLE regress_createdb; DROP ROLE regress_createrole; @@ -149,6 +181,7 @@ DROP ROLE regress_role_admin; ERROR: current user cannot be dropped -- ok RESET SESSION AUTHORIZATION; +REVOKE CREATE ON DATABASE regression FROM regress_role_admin CASCADE; DROP INDEX tenant_idx; DROP TABLE tenant_table; DROP VIEW tenant_view; diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql index ddc80578d9..edaed43588 100644 --- a/src/test/regress/sql/create_role.sql +++ b/src/test/regress/sql/create_role.sql @@ -1,6 +1,7 @@ -- ok, superuser can create users with any set of privileges CREATE ROLE regress_role_super SUPERUSER; CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS; +GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION; CREATE ROLE regress_role_normal; -- fail, only superusers can create users with these privileges @@ -13,6 +14,7 @@ CREATE ROLE regress_nosuch_bypassrls BYPASSRLS; -- ok, having CREATEROLE is enough to create users with these privileges CREATE ROLE regress_createdb CREATEDB; CREATE ROLE regress_createrole CREATEROLE NOINHERIT; +GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION; CREATE ROLE regress_login LOGIN; CREATE ROLE regress_inherit INHERIT; CREATE ROLE regress_connection_limit CONNECTION LIMIT 5; @@ -83,9 +85,40 @@ DROP TABLE tenant_table; ALTER VIEW tenant_view OWNER TO regress_role_admin; DROP VIEW tenant_view; +-- fail, can't create objects owned as regress_tenant +CREATE SCHEMA regress_tenant_schema AUTHORIZATION regress_tenant; + -- fail, we don't inherit permissions from regress_tenant REASSIGN OWNED BY regress_tenant TO regress_createrole; +-- ok, create a role with a value for createrole_self_grant +SET createrole_self_grant = 'set, inherit'; +CREATE ROLE regress_tenant2; +GRANT CREATE ON DATABASE regression TO regress_tenant2; + +-- ok, regress_tenant2 can create objects within the database +SET SESSION AUTHORIZATION regress_tenant2; +CREATE TABLE tenant2_table (i integer); +REVOKE ALL PRIVILEGES ON tenant2_table FROM PUBLIC; + +-- ok, because we have SET and INHERIT on regress_tenant2 +SET SESSION AUTHORIZATION regress_createrole; +CREATE SCHEMA regress_tenant2_schema AUTHORIZATION regress_tenant2; +ALTER SCHEMA regress_tenant2_schema OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_tenant2; + +-- with SET but not INHERIT, we can give away objects but not take them +REVOKE INHERIT OPTION FOR regress_tenant2 FROM regress_createrole; +ALTER SCHEMA regress_tenant2_schema OWNER TO regress_tenant2; +ALTER TABLE tenant2_table OWNER TO regress_createrole; + +-- with INHERIT but not SET, we can take objects but not give them away +GRANT regress_tenant2 TO regress_createrole WITH INHERIT TRUE, SET FALSE; +ALTER TABLE tenant2_table OWNER TO regress_createrole; +ALTER TABLE tenant2_table OWNER TO regress_tenant2; +DROP TABLE tenant2_table; + -- fail, CREATEROLE is not enough to create roles in privileged roles CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data; CREATE ROLE regress_write_all_data IN ROLE pg_write_all_data; @@ -113,6 +146,9 @@ DROP ROLE regress_nosuch_recursive; DROP ROLE regress_nosuch_admin_recursive; DROP ROLE regress_plainrole; +-- must revoke privileges before dropping role +REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE; + -- ok, should be able to drop non-superuser roles we created DROP ROLE regress_createdb; DROP ROLE regress_createrole; @@ -131,6 +167,7 @@ DROP ROLE regress_role_admin; -- ok RESET SESSION AUTHORIZATION; +REVOKE CREATE ON DATABASE regression FROM regress_role_admin CASCADE; DROP INDEX tenant_idx; DROP TABLE tenant_table; DROP VIEW tenant_view;