allowing for control over SET ROLE
Hi,
There are two ways in which a role can exercise the privileges of some
other role which has been granted to it. First, it can implicitly
inherit the privileges of the granted role. Second, it can assume the
identity of the granted role using the SET ROLE command. It is
possible to control the former behavior, but not the latter. In v15
and prior release, we had a role-level [NO]INHERIT property which
controlled whether a role automatically inherited the privileges of
any role granted to it. This was all-or-nothing. Beginning in
e3ce2de09d814f8770b2e3b3c152b7671bcdb83f, the inheritance behavior of
role-grants can be overridden for individual grants, so that some
grants are inherited and others are not. However, there is no similar
facility for controlling whether a role can SET ROLE to some other
role of which it is a member. At present, if role A is a member of
role B, then A can SET ROLE B, and that's it.
In some circumstances, it may be desirable to control this behavior.
For example, if we GRANT pg_read_all_settings TO seer, we do want the
seer to be able to read all the settings, else we would not have
granted the role. But we might not want the seer to be able to do
this:
You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
SET
rhaas=> create table artifact (a int);
CREATE TABLE
rhaas=> \d
List of relations
Schema | Name | Type | Owner
--------+----------+-------+----------------------
public | artifact | table | pg_read_all_settings
(1 row)
I have attached a rather small patch which makes it possible to
control this behavior:
You are now connected to database "rhaas" as user "rhaas".
rhaas=# grant pg_read_all_settings to seer with set false;
GRANT ROLE
rhaas=# \c - seer
You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
ERROR: permission denied to set role "pg_read_all_settings"
I think that this behavior is generally useful, and not just for the
predefined roles that we ship as part of PostgreSQL. I don't think
it's too hard to imagine someone wanting to use some locally created
role as a container for privileges but not wanting the users who
possess this role to run around creating new objects owned by it. To
some extent that can be controlled by making sure the role in question
doesn't have any excess privileges, but that's not really sufficient:
all you need is one schema anywhere in the system that grants CREATE
to PUBLIC. You could avoid creating such a schema, which might be a
good idea for other reasons anyway, but it feels like approaching the
problem from the wrong end. What you really want is to allow the users
to inherit the privileges of the role but not use SET ROLE to become
that role, so that's what this patch lets you do.
There's one other kind of case in which this sort of thing might be
somewhat useful, although it's more dubious. Suppose you have an
oncall group where you regularly add and remove members according to
who is on call. Naturally, you have an on-call bot which performs this
task automatically. The on-call bot has the ability to manage
memberships in the oncall group, but should not have the ability to
access any of its privileges, either by inheritance or via SET ROLE.
This patch KIND OF lets you accomplish this:
rhaas=# create role oncall;
CREATE ROLE
rhaas=# create role oncallbot login;
CREATE ROLE
rhaas=# grant oncall to oncallbot with inherit false, set false, admin true;
GRANT ROLE
rhaas=# create role anna;
CREATE ROLE
rhaas=# create role eliza;
CREATE ROLE
rhaas=# \c - oncallbot
You are now connected to database "rhaas" as user "oncallbot".
rhaas=> grant oncall to anna;
GRANT ROLE
rhaas=> revoke oncall from anna;
REVOKE ROLE
rhaas=> grant oncall to eliza;
GRANT ROLE
rhaas=> set role oncall;
ERROR: permission denied to set role "oncall"
The problem here is that if a nasty evil hacker takes over the
oncallbot role, nothing whatsoever prevents them from executing "grant
oncall to oncallbot with set true" after which they can then "SET ROLE
oncall" using the privileges they just granted themselves. And even if
under some theory we blocked that, they could still maliciously grant
the sought-after on-call privileges to some other role i.e. "grant
oncall to accomplice". It's fundamentally difficult to allow people to
administer a set of privileges without giving them the ability to
usurp those privileges, and I wouldn't like to pretend that this patch
is in any way sufficient to accomplish such a thing. Nevertheless, I
think there's some chance it might be useful to someone building such
a system, in combination with other safeguards. Or maybe not: this
isn't the main reason I'm interested in this, and it's just an added
benefit if it turns out that someone can do something like this with
it.
In order to apply this patch, we'd need to reach a conclusion about
the matters mentioned in
/messages/by-id/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
-- and thinking about this patch might shed some light on what we'd
want to do over there.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Add-a-SET-option-to-the-GRANT-command.patchapplication/octet-stream; name=v1-0001-Add-a-SET-option-to-the-GRANT-command.patchDownload
From daf3bc2c18c456e9ca7c8a122b3c5d34f550243a Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 26 Aug 2022 15:17:41 -0400
Subject: [PATCH v1] Add a SET option to the GRANT command.
Similar to how the INHERIT option controls whether or not the
permissions of the granted role are automatically available to the
grantee, the new SET permission controls whether or not the grantee
may use the SET ROLE command to assume the privileges of the granted
role.
---
doc/src/sgml/catalogs.sgml | 11 ++++
doc/src/sgml/func.sgml | 9 ++--
doc/src/sgml/ref/grant.sgml | 32 +++++++----
doc/src/sgml/ref/revoke.sgml | 8 +--
doc/src/sgml/ref/set_role.sgml | 9 ++--
doc/src/sgml/user-manag.sgml | 20 +++++--
src/backend/commands/user.c | 44 +++++++++++++--
src/backend/commands/variable.c | 2 +-
src/backend/utils/adt/acl.c | 68 +++++++++++++++++++-----
src/bin/pg_dump/pg_dumpall.c | 23 +++++---
src/include/catalog/pg_auth_members.h | 1 +
src/include/utils/acl.h | 1 +
src/test/regress/expected/privileges.out | 9 ++++
src/test/regress/sql/privileges.sql | 9 ++++
14 files changed, 199 insertions(+), 47 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 00f833d210..9ed2b020b7 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1727,6 +1727,17 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
granted role
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>set_option</structfield> <type>bool</type>
+ </para>
+ <para>
+ True if the member can
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ to the granted role
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f87afefeae..e7e5eca7e5 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24070,11 +24070,14 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
<para>
Does user have privilege for role?
Allowable privilege types are
- <literal>MEMBER</literal> and <literal>USAGE</literal>.
+ <literal>MEMBER</literal>, <literal>USAGE</literal>,
+ and <literal>SET</literal>.
<literal>MEMBER</literal> denotes direct or indirect membership in
- the role (that is, the right to do <command>SET ROLE</command>), while
+ the role without regard to what specific privileges may be conferred.
<literal>USAGE</literal> denotes whether the privileges of the role
- are immediately available without doing <command>SET ROLE</command>.
+ are immediately available without doing <command>SET ROLE</command>,
+ while <literal>SET</literal> denotes whether it is possible to change
+ to the role using the <literal>SET ROLE</literal> command.
This function does not allow the special case of
setting <parameter>user</parameter> to <literal>public</literal>,
because the PUBLIC pseudo-role can never be a member of real roles.
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index dea19cd348..d5911ff942 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -98,7 +98,7 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replaceable class="parameter">role_specification</replaceable> [, ...]
- [ WITH { ADMIN | INHERIT } { OPTION | TRUE | FALSE } ]
+ [ WITH { ADMIN | INHERIT | SET } { OPTION | TRUE | FALSE } ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
<phrase>where <replaceable class="parameter">role_specification</replaceable> can be:</phrase>
@@ -250,17 +250,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
<para>
This variant of the <command>GRANT</command> command grants membership
in a role to one or more other roles. Membership in a role is significant
- because it conveys the privileges granted to a role to each of its
- members.
+ because it potentially allows access to the privileges granted to a role
+ to each of its members, and potentially also the ability to make changes
+ to the role itself. However, the actualy permisions conferred depend on
+ the options associated with the grant.
</para>
<para>
- The effect of membership in a role can be modified by specifying the
- <literal>ADMIN</literal> or <literal>INHERIT</literal> option, each
- of which can be set to either <literal>TRUE</literal> or
- <literal>FALSE</literal>. The keyword <literal>OPTION</literal> is accepted
- as a synonym for <literal>TRUE</literal>, so that
- <literal>WITH ADMIN OPTION</literal>
+ Each of the options described below can be set to either
+ <literal>TRUE</literal> or <literal>FALSE</literal>. The keyword
+ <literal>OPTION</literal> is accepted as a synonym for
+ <literal>TRUE</literal>, so that <literal>WITH ADMIN OPTION</literal>
is a synonym for <literal>WITH ADMIN TRUE</literal>.
</para>
@@ -272,7 +272,8 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
OPTION</literal> on itself. Database superusers can grant or revoke
membership in any role to anyone. Roles having
<literal>CREATEROLE</literal> privilege can grant or revoke membership
- in any role that is not a superuser.
+ in any role that is not a superuser. This option defaults to
+ <literal>FALSE</literal>.
</para>
<para>
@@ -287,6 +288,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
See <link linkend="sql-createrole"><command>CREATE ROLE</command></link>.
</para>
+ <para>
+ The <literal>SET</literal> option, if it is set to
+ <literal>TRUE</literal>, allows the member to change to the granted
+ role using the
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ command. If a role is an indirect member of another role, it can use
+ <literal>SET ROLE</literal> to change to that role only if there is a
+ chain of grants each of which has <literal>SET TRUE</literal>.
+ This option defaults to <literal>TRUE</literal>.
+ </para>
+
<para>
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
having been done by the specified role. A user can only attribute a grant
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 4fd4bfb3d7..2db66bbf37 100644
--- a/doc/src/sgml/ref/revoke.sgml
+++ b/doc/src/sgml/ref/revoke.sgml
@@ -125,7 +125,7 @@ REVOKE [ GRANT OPTION FOR ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
-REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
+REVOKE [ { ADMIN | INHERIT | SET } OPTION FOR ]
<replaceable class="parameter">role_name</replaceable> [, ...] FROM <replaceable class="parameter">role_specification</replaceable> [, ...]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
@@ -209,9 +209,9 @@ REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
<para>
Just as <literal>ADMIN OPTION</literal> can be removed from an existing
- role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>.
- This is equivalent to setting the value of that option to
- <literal>FALSE</literal>.
+ role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>
+ or <literal>SET OPTION</literal>. This is equivalent to setting the value
+ of the corresponding option to <literal>FALSE</literal>.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index deecfe4120..13bad1bf66 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -77,14 +77,17 @@ RESET ROLE
effectively drops all the privileges except for those which the target role
directly possesses or inherits. On the other hand, if the session user role
has been granted memberships <literal>WITH INHERIT FALSE</literal>, the
- privileges of the granted roles can't be accessed by default. However, the
+ privileges of the granted roles can't be accessed by default. However, if
+ the role was granted <literal>WITH SET TRUE</literal>, the
session user can use <command>SET ROLE</command> to drop the privileges
assigned directly to the session user and instead acquire the privileges
- available to the named role.
+ available to the named role. If the role was granted <literal>WITH INHERIT
+ FALSE, SET FALSE</literal> then the privileges of that role cannot be
+ exercised either with or without <literal>SET ROLE</literal>.
</para>
<para>
- In particular, when a superuser chooses to <command>SET ROLE</command> to a
+ Note that when a superuser chooses to <command>SET ROLE</command> to a
non-superuser role, they lose their superuser privileges.
</para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index fc836d5748..601fff3e6b 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -354,7 +354,8 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
<para>
The members of a group role can use the privileges of the role in two
- ways. First, every member of a group can explicitly do
+ ways. First, member roles that have been granted membership with the
+ <literal>SET</literal> option can do
<link linkend="sql-set-role"><command>SET ROLE</command></link> to
temporarily <quote>become</quote> the group role. In this state, the
database session has access to the privileges of the group role rather
@@ -369,13 +370,16 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
CREATE ROLE joe LOGIN;
CREATE ROLE admin;
CREATE ROLE wheel;
+CREATE ROLE island;
GRANT admin TO joe WITH INHERIT TRUE;
GRANT wheel TO admin WITH INHERIT FALSE;
+GRANT island TO joe WITH INHERIT TRUE, SET FALSE;
</programlisting>
Immediately after connecting as role <literal>joe</literal>, a database
session will have use of privileges granted directly to <literal>joe</literal>
- plus any privileges granted to <literal>admin</literal>, because <literal>joe</literal>
- <quote>inherits</quote> <literal>admin</literal>'s privileges. However, privileges
+ plus any privileges granted to <literal>admin</literal> and
+ <literal>island</literal>, because <literal>joe</literal>
+ <quote>inherits</quote> those privileges. However, privileges
granted to <literal>wheel</literal> are not available, because even though
<literal>joe</literal> is indirectly a member of <literal>wheel</literal>, the
membership is via <literal>admin</literal> which was granted using
@@ -384,7 +388,8 @@ GRANT wheel TO admin WITH INHERIT FALSE;
SET ROLE admin;
</programlisting>
the session would have use of only those privileges granted to
- <literal>admin</literal>, and not those granted to <literal>joe</literal>. After:
+ <literal>admin</literal>, and not those granted to <literal>joe</literal> or
+ <literal>island</literal>. After:
<programlisting>
SET ROLE wheel;
</programlisting>
@@ -402,9 +407,14 @@ RESET ROLE;
<note>
<para>
The <command>SET ROLE</command> command always allows selecting any role
- that the original login role is directly or indirectly a member of.
+ that the original login role is directly or indirectly a member of,
+ provided that there is a chain of membership grants each of which has
+ <literal>SET TRUE</literal> (which is the default).
Thus, in the above example, it is not necessary to become
<literal>admin</literal> before becoming <literal>wheel</literal>.
+ On the other hand, it is not possible to become <literal>island</literal>
+ at all; <literal>joe</literal> can only access those privileges via
+ inheritance.
</para>
</note>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 265a48af7e..61456d6723 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -51,8 +51,8 @@
* RRG_REMOVE_ADMIN_OPTION indicates a grant that would need to have
* admin_option set to false by the operation.
*
- * RRG_REMOVE_INHERIT_OPTION indicates a grant that would need to have
- * inherit_option set to false by the operation.
+ * Similarly, RRG_REMOVE_INHERIT_OPTION and RRG_REMOVE_SET_OPTION indicate
+ * grants that would need to have the corresponding options set to false.
*
* RRG_DELETE_GRANT indicates a grant that would need to be removed entirely
* by the operation.
@@ -62,6 +62,7 @@ typedef enum
RRG_NOOP,
RRG_REMOVE_ADMIN_OPTION,
RRG_REMOVE_INHERIT_OPTION,
+ RRG_REMOVE_SET_OPTION,
RRG_DELETE_GRANT
} RevokeRoleGrantAction;
@@ -73,10 +74,12 @@ typedef struct
unsigned specified;
bool admin;
bool inherit;
+ bool set;
} GrantRoleOptions;
#define GRANT_ROLE_SPECIFIED_ADMIN 0x0001
#define GRANT_ROLE_SPECIFIED_INHERIT 0x0002
+#define GRANT_ROLE_SPECIFIED_SET 0x0004
/* GUC parameter */
int Password_encryption = PASSWORD_TYPE_SCRAM_SHA_256;
@@ -1389,6 +1392,12 @@ GrantRole(ParseState *pstate, GrantRoleStmt *stmt)
if (parse_bool(optval, &popt.inherit))
continue;
}
+ else if (strcmp(opt->defname, "set") == 0)
+ {
+ popt.specified |= GRANT_ROLE_SPECIFIED_SET;
+ if (parse_bool(optval, &popt.set))
+ continue;
+ }
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
@@ -1776,6 +1785,16 @@ AddRoleMems(const char *rolename, Oid roleid,
at_least_one_change = true;
}
+ if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0
+ && authmem_form->set_option != popt->set)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ at_least_one_change = true;
+ }
+
if (!at_least_one_change)
{
ereport(NOTICE,
@@ -1798,9 +1817,15 @@ AddRoleMems(const char *rolename, Oid roleid,
Oid objectId;
Oid *newmembers = palloc(sizeof(Oid));
- /* Set admin option if user set it to true, otherwise not. */
+ /*
+ * The values for these options can be taken directly from 'popt'.
+ * Either they were specified, or the defaults as set by
+ * InitGrantRoleOptions are correct.
+ */
new_record[Anum_pg_auth_members_admin_option - 1] =
BoolGetDatum(popt->admin);
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
/*
* If the user specified a value for the inherit option, use
@@ -1989,6 +2014,13 @@ DelRoleMems(const char *rolename, Oid roleid,
new_record_repl[Anum_pg_auth_members_inherit_option - 1] =
true;
}
+ else if (actions[i] == RRG_REMOVE_SET_OPTION)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(false);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ }
else
elog(ERROR, "unknown role revoke action");
@@ -2182,6 +2214,11 @@ plan_single_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
*/
actions[i] = RRG_REMOVE_INHERIT_OPTION;
}
+ else if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0)
+ {
+ /* Here too, no need to recurse. */
+ actions[i] = RRG_REMOVE_SET_OPTION;
+ }
else
{
bool revoke_admin_option_only;
@@ -2331,4 +2368,5 @@ InitGrantRoleOptions(GrantRoleOptions *popt)
popt->specified = 0;
popt->admin = false;
popt->inherit = false;
+ popt->set = true;
}
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index e5ddcda0b4..f907b59746 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -881,7 +881,7 @@ check_role(char **newval, void **extra, GucSource source)
* leader's state.
*/
if (!InitializingParallelWorker &&
- !is_member_of_role(GetSessionUserId(), roleid))
+ !member_can_set_role(GetSessionUserId(), roleid))
{
if (source == PGC_S_TEST)
{
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index fd71a9b13e..355ba98cc8 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -61,16 +61,17 @@ typedef struct
*
* Each element of cached_roles is an OID list of constituent roles for the
* corresponding element of cached_role (always including the cached_role
- * itself). One cache has ROLERECURSE_PRIVS semantics, and the other has
- * ROLERECURSE_MEMBERS semantics.
+ * itself). There's a separate cache for each RoleRecurseType, with the
+ * corresponding semantics.
*/
enum RoleRecurseType
{
- ROLERECURSE_PRIVS = 0, /* recurse through inheritable grants */
- ROLERECURSE_MEMBERS = 1 /* recurse unconditionally */
+ ROLERECURSE_MEMBERS = 0, /* recurse unconditionally */
+ ROLERECURSE_PRIVS = 1, /* recurse through inheritable grants */
+ ROLERECURSE_SETROLE = 2 /* recurse through grants with set_option */
};
-static Oid cached_role[] = {InvalidOid, InvalidOid};
-static List *cached_roles[] = {NIL, NIL};
+static Oid cached_role[] = {InvalidOid, InvalidOid, InvalidOid};
+static List *cached_roles[] = {NIL, NIL, NIL};
static uint32 cached_db_hash;
@@ -4685,10 +4686,13 @@ convert_role_priv_string(text *priv_type_text)
static const priv_map role_priv_map[] = {
{"USAGE", ACL_USAGE},
{"MEMBER", ACL_CREATE},
+ {"SET", ACL_SET},
{"USAGE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"USAGE WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{NULL, 0}
};
@@ -4717,6 +4721,11 @@ pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode)
if (has_privs_of_role(roleid, role_oid))
return ACLCHECK_OK;
}
+ if (mode & ACL_SET)
+ {
+ if (member_can_set_role(roleid, role_oid))
+ return ACLCHECK_OK;
+ }
return ACLCHECK_NO_PRIV;
}
@@ -4765,15 +4774,17 @@ RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
}
/* Force membership caches to be recomputed on next use */
- cached_role[ROLERECURSE_PRIVS] = InvalidOid;
cached_role[ROLERECURSE_MEMBERS] = InvalidOid;
+ cached_role[ROLERECURSE_PRIVS] = InvalidOid;
+ cached_role[ROLERECURSE_SETROLE] = InvalidOid;
}
/*
* Get a list of roles that the specified roleid is a member of
*
- * Type ROLERECURSE_PRIVS recurses only through inheritable grants,
- * while ROLERECURSE_MEMBERS recurses through all grants.
+ * Type ROLERECURSE_MEMBERS recurses through all grants; ROLERECURSE_PRIVS
+ * recurses only through inheritable grants; and ROLERECURSE_SETROLe recurses
+ * only through grants with set_option.
*
* Since indirect membership testing is relatively expensive, we cache
* a list of memberships. Hence, the result is only guaranteed good until
@@ -4864,6 +4875,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
if (type == ROLERECURSE_PRIVS && !form->inherit_option)
continue;
+ /* If we're supposed to ignore non-SET grants, do so. */
+ if (type == ROLERECURSE_SETROLE && !form->set_option)
+ continue;
+
/*
* Even though there shouldn't be any loops in the membership
* graph, we must test for having already seen this role. It is
@@ -4903,9 +4918,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
/*
* Does member have the privileges of role (directly or indirectly)?
*
- * This is defined not to recurse through grants that are not inherited;
- * in such cases, membership implies the ability to do SET ROLE, but
- * the privileges are not available until you've done so.
+ * This is defined not to recurse through grants that are not inherited,
+ * and only inherited grants confer the associated privileges automatically.
+ *
+ * See also member_can_set_role, below.
*/
bool
has_privs_of_role(Oid member, Oid role)
@@ -4927,6 +4943,34 @@ has_privs_of_role(Oid member, Oid role)
role);
}
+/*
+ * Can member use SET ROLE to this role?
+ *
+ * There must be a chain of grants from 'member' to 'role' each of which
+ * permits SET ROLE; that is, each of which has set_option = true.
+ *
+ * It doesn't matter whether the grants are inheritable. That's a separate
+ * question; see has_privs_of_role.
+ */
+bool
+member_can_set_role(Oid member, Oid role)
+{
+ /* Fast path for simple case */
+ if (member == role)
+ return true;
+
+ /* Superusers have every privilege, so can always SET ROLE */
+ if (superuser_arg(member))
+ return true;
+
+ /*
+ * Find all the roles that member can access via SET ROLE, including
+ * multi-level recursion, then see if target role is any one of them.
+ */
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_SETROLE,
+ InvalidOid, NULL),
+ role);
+}
/*
* Is member a member of role (directly or indirectly)?
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index d665b257c9..75c09596c4 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -953,8 +953,9 @@ dumpRoleMembership(PGconn *conn)
end,
total;
bool dump_grantors;
- bool dump_inherit_option;
+ bool dump_grant_options;
int i_inherit_option;
+ int i_set_option;
/*
* Previous versions of PostgreSQL didn't used to track the grantor very
@@ -966,10 +967,10 @@ dumpRoleMembership(PGconn *conn)
dump_grantors = (PQserverVersion(conn) >= 160000);
/*
- * Previous versions of PostgreSQL also did not have a grant-level
+ * Previous versions of PostgreSQL also did not have grant-level options.
* INHERIT option.
*/
- dump_inherit_option = (server_version >= 160000);
+ dump_grant_options = (server_version >= 160000);
/* Generate and execute query. */
printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
@@ -977,8 +978,8 @@ dumpRoleMembership(PGconn *conn)
"ug.oid AS grantorid, "
"ug.rolname AS grantor, "
"a.admin_option");
- if (dump_inherit_option)
- appendPQExpBuffer(buf, ", a.inherit_option");
+ if (dump_grant_options)
+ appendPQExpBuffer(buf, ", a.inherit_option, a.set_option");
appendPQExpBuffer(buf, " FROM pg_auth_members a "
"LEFT JOIN %s ur on ur.oid = a.roleid "
"LEFT JOIN %s um on um.oid = a.member "
@@ -987,6 +988,7 @@ dumpRoleMembership(PGconn *conn)
"ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog);
res = executeQuery(conn, buf->data);
i_inherit_option = PQfnumber(res, "inherit_option");
+ i_set_option = PQfnumber(res, "set_option");
if (PQntuples(res) > 0)
fprintf(OPF, "--\n-- Role memberships\n--\n\n");
@@ -1057,6 +1059,7 @@ dumpRoleMembership(PGconn *conn)
char *admin_option;
char *grantorid;
char *grantor;
+ char *set_option = "true";
bool found;
/* If we already did this grant, don't do it again. */
@@ -1067,6 +1070,8 @@ dumpRoleMembership(PGconn *conn)
grantorid = PQgetvalue(res, i, 2);
grantor = PQgetvalue(res, i, 3);
admin_option = PQgetvalue(res, i, 4);
+ if (dump_grant_options)
+ set_option = PQgetvalue(res, i, i_set_option);
/*
* If we're not dumping grantors or if the grantor is the
@@ -1096,7 +1101,7 @@ dumpRoleMembership(PGconn *conn)
fprintf(OPF, " TO %s", fmtId(member));
if (*admin_option == 't')
appendPQExpBufferStr(optbuf, "ADMIN OPTION");
- if (dump_inherit_option)
+ if (dump_grant_options)
{
char *inherit_option;
@@ -1107,6 +1112,12 @@ dumpRoleMembership(PGconn *conn)
*inherit_option == 't' ?
"TRUE" : "FALSE");
}
+ if (*set_option != 't')
+ {
+ if (optbuf->data[0] != '\0')
+ appendPQExpBufferStr(optbuf, ", ");
+ appendPQExpBuffer(optbuf, "SET FALSE");
+ }
if (optbuf->data[0] != '\0')
fprintf(OPF, " WITH %s", optbuf->data);
if (dump_grantors)
diff --git a/src/include/catalog/pg_auth_members.h b/src/include/catalog/pg_auth_members.h
index 3ee6ae5f6a..b145fce1ed 100644
--- a/src/include/catalog/pg_auth_members.h
+++ b/src/include/catalog/pg_auth_members.h
@@ -35,6 +35,7 @@ CATALOG(pg_auth_members,1261,AuthMemRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_
Oid grantor BKI_LOOKUP(pg_authid); /* who granted the membership */
bool admin_option; /* granted with admin option? */
bool inherit_option; /* exercise privileges without SET ROLE? */
+ bool set_option; /* use SET ROLE to the target role? */
} FormData_pg_auth_members;
/* ----------------
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 3d6411197c..48de5d5c01 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,6 +209,7 @@ extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId,
extern int aclmembers(const Acl *acl, Oid **roleids);
extern bool has_privs_of_role(Oid member, Oid role);
+extern bool member_can_set_role(Oid member, Oid role);
extern bool is_member_of_role(Oid member, Oid role);
extern bool is_member_of_role_nosuper(Oid member, Oid role);
extern bool is_admin_of_role(Oid member, Oid role);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index bd3453ee91..309a15abdc 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -132,6 +132,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+ERROR: permission denied to set role "pg_read_all_settings"
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+ERROR: permission denied to set role "pg_read_all_stats"
+RESET ROLE;
+RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
DROP USER regress_priv_user10;
DROP USER regress_priv_user9;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 4ad366470d..7a169f5860 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -114,6 +114,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
+RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+RESET ROLE;
+
RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
--
2.24.3 (Apple Git-128)
On Wed, Aug 31, 2022 at 08:56:31AM -0400, Robert Haas wrote:
In some circumstances, it may be desirable to control this behavior.
For example, if we GRANT pg_read_all_settings TO seer, we do want the
seer to be able to read all the settings, else we would not have
granted the role. But we might not want the seer to be able to do
this:You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
SET
rhaas=> create table artifact (a int);
CREATE TABLE
rhaas=> \d
List of relations
Schema | Name | Type | Owner
--------+----------+-------+----------------------
public | artifact | table | pg_read_all_settings
(1 row)
+1
The problem here is that if a nasty evil hacker takes over the
oncallbot role, nothing whatsoever prevents them from executing "grant
oncall to oncallbot with set true" after which they can then "SET ROLE
oncall" using the privileges they just granted themselves. And even if
under some theory we blocked that, they could still maliciously grant
the sought-after on-call privileges to some other role i.e. "grant
oncall to accomplice". It's fundamentally difficult to allow people to
administer a set of privileges without giving them the ability to
usurp those privileges, and I wouldn't like to pretend that this patch
is in any way sufficient to accomplish such a thing. Nevertheless, I
think there's some chance it might be useful to someone building such
a system, in combination with other safeguards. Or maybe not: this
isn't the main reason I'm interested in this, and it's just an added
benefit if it turns out that someone can do something like this with
it.
Yeah, if you have ADMIN for a role, you would effectively have SET. I'm
tempted to suggest that ADMIN roles should be restricted from granting SET
unless they have it themselves. However, that seems like it'd create a
weird discrepancy. If you have ADMIN but not INHERIT or SET, you'd still
be able to grant membership with or without INHERIT, but you wouldn't be
able to grant SET. In the end, I guess I agree with you that it's
"fundamentally difficult to allow people to administer a set of privileges
without giving them the ability to usurp those privileges..."
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Robert Haas:
Beginning in
e3ce2de09d814f8770b2e3b3c152b7671bcdb83f, the inheritance behavior of
role-grants can be overridden for individual grants, so that some
grants are inherited and others are not.
That's a great thing to have!
However, there is no similar
facility for controlling whether a role can SET ROLE to some other
role of which it is a member. At present, if role A is a member of
role B, then A can SET ROLE B, and that's it.In some circumstances, it may be desirable to control this behavior.
+1
rhaas=# grant oncall to oncallbot with inherit false, set false, admin true;
Looking at the syntax here, I'm not sure whether adding more WITH
options is the best way to do this. From a user perspective WITH SET
TRUE looks more like a privilege granted on how to use this database
object (role). Something like this would be more consistent with the
other GRANT variants:
GRANT SET ON ROLE oncall TO oncallbot WITH GRANT OPTION;
This is obviously not exactly the same as the command above, because
oncallbot would be able to use SET ROLE directly. But as discussed, this
is more cosmetic anyway, because they could GRANT it to themselves.
The full syntax could look like this:
GRANT { INHERIT | SET | ALL [ PRIVILEGES ] }
ON ROLE role_name [, ...]
TO role_specification [, ...] WITH GRANT OPTION
[ GRANTED BY role_specification ]
With this new syntax, the existing
GRANT role_name TO role_specification [WITH ADMIN OPTION];
would be the same as
GRANT ALL ON role_name TO role_specification [WITH GRANT OPTION];
This would slightly change the way INHERIT works: As a privilege, it
would not override the member's role INHERIT attribute, but would
control whether that attribute is applied. This means:
- INHERIT attribute + INHERIT granted -> inheritance (same)
- INHERIT attribute + INHERIT not granted -> no inheritance (different!)
- NOINHERIT attribute + INHERIT not granted -> no inheritance (same)
- NOINHERIT attribute + INHERIT granted -> no inheritance (different!)
This would allow us to do the following:
GRANT INHERIT ON ROLE pg_read_all_settings TO seer_bot WITH GRANT OPTION;
seer_bot would now be able to GRANT pg_read_all_settings to other users,
too - but without the ability to use or grant SET ROLE to anyone. As
long as seer_bot has the NOINHERIT attribute set, they wouldn't use that
privilege, though - which might be desired for the bot.
Similary, it would be possible for the oncallbot in the example above to
be able to grant SET ROLE only - and not INHERIT.
I realize that there has been a lot of discussion about roles and
privileges in the past year. I have tried to follow those discussions,
but it's likely that I missed some good arguments against my proposal above.
Best
Wolfgang
On Fri, Sep 2, 2022 at 3:20 AM Wolfgang Walther <walther@technowledgy.de> wrote:
The full syntax could look like this:
GRANT { INHERIT | SET | ALL [ PRIVILEGES ] }
ON ROLE role_name [, ...]
TO role_specification [, ...] WITH GRANT OPTION
[ GRANTED BY role_specification ]With this new syntax, the existing
GRANT role_name TO role_specification [WITH ADMIN OPTION];
would be the same as
GRANT ALL ON role_name TO role_specification [WITH GRANT OPTION];
This would be a pretty significant rework. Right now, there's only one
ADMIN OPTION on a role, and you either have it or you don't. Changing
things around so that you can have each individual privilege with or
without grant option would be a fair amount of work. I don't think
it's completely crazy, but I'm not very sold on the idea, either,
because giving somebody *either* the ability to grant INHERIT option
*or* the ability to grant SET option is largely equivalent from a
security point of view. Either way, the grantees will be able to
access the privileges of the role in some fashion. This is different
from table privileges, where SELECT and INSERT are clearly distinct
rights that do not overlap, and thus separating the ability to
administer one of those things from the ability to administer the
other one has more utility.
The situation might look different in the future if we added more role
options and if each of those were clearly severable rights. For
instance, if we had a DROP option on a role grant that conferred the
right to drop the role, that would be distinct from SET and INHERIT
and it might make sense to allow someone to administer SET and/or
INHERIT but not DROP. However, I don't have any current plans to add
such an option, and TBH I find it a little hard to come up with a
compelling list of things that would be worth adding as separate
permissions here. There are a bunch of things that one role can do to
another using ALTER ROLE, and right now you have to be SUPERUSER or
have CREATEROLE to do that stuff. In
theory, you could turn that into a big list of individual rights so
that you can e.g. GRANT CHANGE PASSWORD ON role1 TO role2 WITH GRANT
OPTION.
However, I really don't see a lot of utility in slicing things up at
that level of granularity. There isn't in my view a lot of use case
for giving a user the right to change some other user's password but
not giving them the right to set the connection limit for that same
other user -- and there's even less use case for giving some user the
ability to grant one of those rights but not the other.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, 2022-08-31 at 08:56 -0400, Robert Haas wrote:
In some circumstances, it may be desirable to control this behavior.
For example, if we GRANT pg_read_all_settings TO seer, we do want the
seer to be able to read all the settings, else we would not have
granted the role. But we might not want the seer to be able to do
this:You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
SET
rhaas=> create table artifact (a int);
CREATE TABLE
rhaas=> \d
List of relations
Schema | Name | Type | Owner
--------+----------+-------+----------------------
public | artifact | table | pg_read_all_settings
(1 row)
Interesting case.
I have attached a rather small patch which makes it possible to
control this behavior:You are now connected to database "rhaas" as user "rhaas".
rhaas=# grant pg_read_all_settings to seer with set false;
GRANT ROLE
You've defined this in terms of the mechanics -- allow SET ROLE or not
-- but I assume you intend it as a security feature to allow/forbid
some capabilities.
Is this only about the capability to create objects owned by a role
you're a member of? Or are there other implications?
Regards,
Jeff Davis
On Sat, Sep 3, 2022 at 3:46 PM Jeff Davis <pgsql@j-davis.com> wrote:
You are now connected to database "rhaas" as user "rhaas".
rhaas=# grant pg_read_all_settings to seer with set false;
GRANT ROLEYou've defined this in terms of the mechanics -- allow SET ROLE or not
-- but I assume you intend it as a security feature to allow/forbid
some capabilities.Is this only about the capability to create objects owned by a role
you're a member of? Or are there other implications?
I think there are some other implications, but I don't think they're
anything super-dramatic. For example, you could create a group that's
just for purposes of pg_hba.conf matching and make the grants both SET
FALSE and INHERIT FALSE, with the idea that the members shouldn't have
any access to the role; it's just there for grouping purposes. I
mentioned one other possible scenario, with oncallbot, in the original
post.
I'm not sure whether thinking about this in terms of security
capabilities is the most helpful way to view it. My view was, as you
say, more mechanical. I think sometimes you grant somebody a role and
you want them to be able to use SET ROLE to assume the privileges of
the target role, and sometimes you don't. I think that primarily
depends on the reason why you made the grant. In the case of a
predefined role, you're almost certainly granting membership so that
the privileges of the predefined role can be inherited. In other
cases, you may be doing it so that the member can SET ROLE to the
target role, or you may be doing it so that the member can administer
the role (because you give them ADMIN OPTION), or you may even be
doing it for pg_hba.conf matching.
And because of this, I think it follows that there may be some
capabilities conferred by role membership that you don't really want
to convey in particular cases, so I think it makes sense to have a way
to avoid conveying the ones that aren't necessary for the grant to
fulfill its purpose. I'm not exactly sure how far that gets you in
terms of building a system that is more secure than what you could
build otherwise, but it feels like a useful capability regardless.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, 2022-09-06 at 10:42 -0400, Robert Haas wrote:
I think there are some other implications, but I don't think they're
anything super-dramatic. For example, you could create a group that's
just for purposes of pg_hba.conf matching and make the grants both
SET
FALSE and INHERIT FALSE, with the idea that the members shouldn't
have
any access to the role; it's just there for grouping purposes. I
mentioned one other possible scenario, with oncallbot, in the
original
post.
Interesting. All of those seem like worthwhile use cases to me.
I'm not sure whether thinking about this in terms of security
capabilities is the most helpful way to view it. My view was, as you
say, more mechanical. I think sometimes you grant somebody a role and
you want them to be able to use SET ROLE to assume the privileges of
the target role, and sometimes you don't.
By denying the ability to "SET ROLE pg_read_all_settings", I assumed
that we'd deny the ability to create objects owned by that
pg_read_all_settings. But on closer inspection:
grant all privileges on schema public to public;
create user u1;
grant pg_read_all_settings to u1 with set false;
\c - u1
create table foo(i int);
set role pg_read_all_settings;
ERROR: permission denied to set role "pg_read_all_settings"
alter table foo owner to pg_read_all_settings;
\d
List of relations
Schema | Name | Type | Owner
--------+------+-------+----------------------
public | foo | table | pg_read_all_settings
(1 row)
Users will reasonably interpret any feature of GRANT to be a security
feature that allows or prevents certain users from causing certain
outcomes. But here, I was initially fooled, and the outcome is still
possible.
So I believe we do need to think in terms of what capabilities we are
really restricting with this feature rather than solely the mechanics.
Regards,
Jeff Davis
On Tue, Sep 6, 2022 at 2:45 PM Jeff Davis <pgsql@j-davis.com> wrote:
I'm not sure whether thinking about this in terms of security
capabilities is the most helpful way to view it. My view was, as you
say, more mechanical. I think sometimes you grant somebody a role and
you want them to be able to use SET ROLE to assume the privileges of
the target role, and sometimes you don't.By denying the ability to "SET ROLE pg_read_all_settings", I assumed
that we'd deny the ability to create objects owned by that
pg_read_all_settings. But on closer inspection:grant all privileges on schema public to public;
create user u1;
grant pg_read_all_settings to u1 with set false;
\c - u1
create table foo(i int);
set role pg_read_all_settings;
ERROR: permission denied to set role "pg_read_all_settings"
alter table foo owner to pg_read_all_settings;
\d
List of relations
Schema | Name | Type | Owner
--------+------+-------+----------------------
public | foo | table | pg_read_all_settings
(1 row)
Yeah. Please note this paragraph in my original post:
"In order to apply this patch, we'd need to reach a conclusion about
the matters mentioned in
/messages/by-id/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
-- and thinking about this patch might shed some light on what we'd
want to do over there."
I hadn't quite gotten around to updating that thread based on posting
this, but this scenario was indeed on my mind.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 31.08.22 14:56, Robert Haas wrote:
In some circumstances, it may be desirable to control this behavior.
For example, if we GRANT pg_read_all_settings TO seer, we do want the
seer to be able to read all the settings, else we would not have
granted the role. But we might not want the seer to be able to do
this:You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
SET
rhaas=> create table artifact (a int);
CREATE TABLE
rhaas=> \d
List of relations
Schema | Name | Type | Owner
--------+----------+-------+----------------------
public | artifact | table | pg_read_all_settings
(1 row)
I think this is because we have (erroneously) make SET ROLE to be the
same as SET SESSION AUTHORIZATION. If those two were separate (i.e.,
there is a current user and a separate current role, as in the SQL
standard), then this would be more straightforward.
I don't know if it's possible to untangle that at this point.
On Mon, Sep 12, 2022 at 11:41 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
I think this is because we have (erroneously) make SET ROLE to be the
same as SET SESSION AUTHORIZATION. If those two were separate (i.e.,
there is a current user and a separate current role, as in the SQL
standard), then this would be more straightforward.I don't know if it's possible to untangle that at this point.
I think that it already works as you describe:
rhaas=# create role foo;
CREATE ROLE
rhaas=# create role bar;
CREATE ROLE
rhaas=# grant bar to foo;
GRANT ROLE
rhaas=# set session authorization foo;
SET
rhaas=> set role bar;
SET
rhaas=> select current_user;
current_user
--------------
bar
(1 row)
rhaas=> select session_user;
session_user
--------------
foo
(1 row)
There may well be problems here, but this example shows that the
current_user and session_user concepts are different in PostgreSQL.
It's also true that the privileges required to execute the commands
are different: SET SESSION AUTHORIZATION requires that the session
user is a superuser, and SET ROLE requires that the identity
established via SET SESSION AUTHORIZATION has the target role granted
to it.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Aug 31, 2022 at 8:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
In order to apply this patch, we'd need to reach a conclusion about
the matters mentioned in
/messages/by-id/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
-- and thinking about this patch might shed some light on what we'd
want to do over there.
That thread has not reached an entirely satisfying conclusion.
However, the behavior that was deemed outright buggy over there has
been fixed. The remaining question is what to do about commands that
allow you to give objects to other users (like ALTER <whatever> ..
OWNER TO) or commands that allow you to create objects owned by other
users (like CREATE DATABASE ... OWNER). I have, in this version,
adopted the proposal by Wolfgang Walther on the other thread to make
this controlled by the new SET option. This essentially takes the view
that the ability to create objects owned by another user is not
precisely a privilege, and is thus not inherited just because the
INHERIT option is set on the GRANT, but it is something you can do if
you could SET ROLE to that role, so we make it dependent on the SET
option. This logic is certainly debatable, but it does have the
practical advantage of making INHERIT TRUE, SET FALSE a useful
combination of settings for predefined roles. It's also 100%
backward-compatible, whereas if we made the behavior dependent on the
INHERIT option, users could potentially notice behavior changes after
upgrading to v16.
So I do like this behavior ... but it's definitely arguable whether
it's the best thing. At any rate, here's an updated patch that
implements it, and to which I've also added a test case.
Review appreciated.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-Add-a-SET-option-to-the-GRANT-command.patchapplication/octet-stream; name=v2-0001-Add-a-SET-option-to-the-GRANT-command.patchDownload
From 3939879b4dd58303fa0563cfd9b59eee88a2e381 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 30 Sep 2022 16:30:56 -0400
Subject: [PATCH v2] Add a SET option to the GRANT command.
Similar to how the INHERIT option controls whether or not the
permissions of the granted role are automatically available to the
grantee, the new SET permission controls whether or not the grantee
may use the SET ROLE command to assume the privileges of the granted
role.
In addition, the new SET permission controls whether or not it
is possible to transfer ownership of objects to the target role
or to create new objects owned by the target role using commands
such as CREATE DATABASE .. OWNER. We could alternatively have made
this controlled by the INHERIT option, or allow it when either
option is given. An advantage of this approach is that if you
are granted a predefined role with INHERIT TRUE, SET FALSE, you
can't go and create objects owned by that role.
The underlying theory here is that the ability to create objects
as a target role is not a privilege per se, and thus does not
depend on whether you inherit the target role's privileges. However,
it's surely something you could do anyway if you could SET ROLE
to the target role, and thus making it contingent on whether you
have that ability is reasonable.
Discussion: http://postgr.es/m/CA+Tgmob+zDSRS6JXYrgq0NWdzCXuTNzT5eK54Dn2hhgt17nm8A@mail.gmail.com
---
doc/src/sgml/catalogs.sgml | 11 ++
doc/src/sgml/func.sgml | 9 +-
doc/src/sgml/ref/grant.sgml | 32 ++++--
doc/src/sgml/ref/revoke.sgml | 8 +-
doc/src/sgml/ref/set_role.sgml | 9 +-
doc/src/sgml/user-manag.sgml | 20 +++-
src/backend/commands/alter.c | 2 +-
src/backend/commands/dbcommands.c | 4 +-
src/backend/commands/foreigncmds.c | 2 +-
src/backend/commands/publicationcmds.c | 2 +-
src/backend/commands/schemacmds.c | 4 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/typecmds.c | 2 +-
src/backend/commands/user.c | 44 +++++++-
src/backend/commands/variable.c | 2 +-
src/backend/utils/adt/acl.c | 106 +++++++++++++++-----
src/bin/pg_dump/pg_dumpall.c | 23 +++--
src/include/catalog/pg_auth_members.h | 1 +
src/include/utils/acl.h | 3 +-
src/test/regress/expected/alter_generic.out | 38 +++----
src/test/regress/expected/foreign_data.out | 2 +-
src/test/regress/expected/privileges.out | 41 ++++++++
src/test/regress/sql/privileges.sql | 39 +++++++
23 files changed, 315 insertions(+), 91 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 00f833d210..9ed2b020b7 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1727,6 +1727,17 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
granted role
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>set_option</structfield> <type>bool</type>
+ </para>
+ <para>
+ True if the member can
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ to the granted role
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e82077292c..c9b8811f63 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23036,11 +23036,14 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
<para>
Does user have privilege for role?
Allowable privilege types are
- <literal>MEMBER</literal> and <literal>USAGE</literal>.
+ <literal>MEMBER</literal>, <literal>USAGE</literal>,
+ and <literal>SET</literal>.
<literal>MEMBER</literal> denotes direct or indirect membership in
- the role (that is, the right to do <command>SET ROLE</command>), while
+ the role without regard to what specific privileges may be conferred.
<literal>USAGE</literal> denotes whether the privileges of the role
- are immediately available without doing <command>SET ROLE</command>.
+ are immediately available without doing <command>SET ROLE</command>,
+ while <literal>SET</literal> denotes whether it is possible to change
+ to the role using the <literal>SET ROLE</literal> command.
This function does not allow the special case of
setting <parameter>user</parameter> to <literal>public</literal>,
because the PUBLIC pseudo-role can never be a member of real roles.
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index dea19cd348..d5911ff942 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -98,7 +98,7 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replaceable class="parameter">role_specification</replaceable> [, ...]
- [ WITH { ADMIN | INHERIT } { OPTION | TRUE | FALSE } ]
+ [ WITH { ADMIN | INHERIT | SET } { OPTION | TRUE | FALSE } ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
<phrase>where <replaceable class="parameter">role_specification</replaceable> can be:</phrase>
@@ -250,17 +250,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
<para>
This variant of the <command>GRANT</command> command grants membership
in a role to one or more other roles. Membership in a role is significant
- because it conveys the privileges granted to a role to each of its
- members.
+ because it potentially allows access to the privileges granted to a role
+ to each of its members, and potentially also the ability to make changes
+ to the role itself. However, the actualy permisions conferred depend on
+ the options associated with the grant.
</para>
<para>
- The effect of membership in a role can be modified by specifying the
- <literal>ADMIN</literal> or <literal>INHERIT</literal> option, each
- of which can be set to either <literal>TRUE</literal> or
- <literal>FALSE</literal>. The keyword <literal>OPTION</literal> is accepted
- as a synonym for <literal>TRUE</literal>, so that
- <literal>WITH ADMIN OPTION</literal>
+ Each of the options described below can be set to either
+ <literal>TRUE</literal> or <literal>FALSE</literal>. The keyword
+ <literal>OPTION</literal> is accepted as a synonym for
+ <literal>TRUE</literal>, so that <literal>WITH ADMIN OPTION</literal>
is a synonym for <literal>WITH ADMIN TRUE</literal>.
</para>
@@ -272,7 +272,8 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
OPTION</literal> on itself. Database superusers can grant or revoke
membership in any role to anyone. Roles having
<literal>CREATEROLE</literal> privilege can grant or revoke membership
- in any role that is not a superuser.
+ in any role that is not a superuser. This option defaults to
+ <literal>FALSE</literal>.
</para>
<para>
@@ -287,6 +288,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
See <link linkend="sql-createrole"><command>CREATE ROLE</command></link>.
</para>
+ <para>
+ The <literal>SET</literal> option, if it is set to
+ <literal>TRUE</literal>, allows the member to change to the granted
+ role using the
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ command. If a role is an indirect member of another role, it can use
+ <literal>SET ROLE</literal> to change to that role only if there is a
+ chain of grants each of which has <literal>SET TRUE</literal>.
+ This option defaults to <literal>TRUE</literal>.
+ </para>
+
<para>
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
having been done by the specified role. A user can only attribute a grant
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 4fd4bfb3d7..2db66bbf37 100644
--- a/doc/src/sgml/ref/revoke.sgml
+++ b/doc/src/sgml/ref/revoke.sgml
@@ -125,7 +125,7 @@ REVOKE [ GRANT OPTION FOR ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
-REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
+REVOKE [ { ADMIN | INHERIT | SET } OPTION FOR ]
<replaceable class="parameter">role_name</replaceable> [, ...] FROM <replaceable class="parameter">role_specification</replaceable> [, ...]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
@@ -209,9 +209,9 @@ REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
<para>
Just as <literal>ADMIN OPTION</literal> can be removed from an existing
- role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>.
- This is equivalent to setting the value of that option to
- <literal>FALSE</literal>.
+ role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>
+ or <literal>SET OPTION</literal>. This is equivalent to setting the value
+ of the corresponding option to <literal>FALSE</literal>.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index deecfe4120..13bad1bf66 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -77,14 +77,17 @@ RESET ROLE
effectively drops all the privileges except for those which the target role
directly possesses or inherits. On the other hand, if the session user role
has been granted memberships <literal>WITH INHERIT FALSE</literal>, the
- privileges of the granted roles can't be accessed by default. However, the
+ privileges of the granted roles can't be accessed by default. However, if
+ the role was granted <literal>WITH SET TRUE</literal>, the
session user can use <command>SET ROLE</command> to drop the privileges
assigned directly to the session user and instead acquire the privileges
- available to the named role.
+ available to the named role. If the role was granted <literal>WITH INHERIT
+ FALSE, SET FALSE</literal> then the privileges of that role cannot be
+ exercised either with or without <literal>SET ROLE</literal>.
</para>
<para>
- In particular, when a superuser chooses to <command>SET ROLE</command> to a
+ Note that when a superuser chooses to <command>SET ROLE</command> to a
non-superuser role, they lose their superuser privileges.
</para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index fc836d5748..601fff3e6b 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -354,7 +354,8 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
<para>
The members of a group role can use the privileges of the role in two
- ways. First, every member of a group can explicitly do
+ ways. First, member roles that have been granted membership with the
+ <literal>SET</literal> option can do
<link linkend="sql-set-role"><command>SET ROLE</command></link> to
temporarily <quote>become</quote> the group role. In this state, the
database session has access to the privileges of the group role rather
@@ -369,13 +370,16 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
CREATE ROLE joe LOGIN;
CREATE ROLE admin;
CREATE ROLE wheel;
+CREATE ROLE island;
GRANT admin TO joe WITH INHERIT TRUE;
GRANT wheel TO admin WITH INHERIT FALSE;
+GRANT island TO joe WITH INHERIT TRUE, SET FALSE;
</programlisting>
Immediately after connecting as role <literal>joe</literal>, a database
session will have use of privileges granted directly to <literal>joe</literal>
- plus any privileges granted to <literal>admin</literal>, because <literal>joe</literal>
- <quote>inherits</quote> <literal>admin</literal>'s privileges. However, privileges
+ plus any privileges granted to <literal>admin</literal> and
+ <literal>island</literal>, because <literal>joe</literal>
+ <quote>inherits</quote> those privileges. However, privileges
granted to <literal>wheel</literal> are not available, because even though
<literal>joe</literal> is indirectly a member of <literal>wheel</literal>, the
membership is via <literal>admin</literal> which was granted using
@@ -384,7 +388,8 @@ GRANT wheel TO admin WITH INHERIT FALSE;
SET ROLE admin;
</programlisting>
the session would have use of only those privileges granted to
- <literal>admin</literal>, and not those granted to <literal>joe</literal>. After:
+ <literal>admin</literal>, and not those granted to <literal>joe</literal> or
+ <literal>island</literal>. After:
<programlisting>
SET ROLE wheel;
</programlisting>
@@ -402,9 +407,14 @@ RESET ROLE;
<note>
<para>
The <command>SET ROLE</command> command always allows selecting any role
- that the original login role is directly or indirectly a member of.
+ that the original login role is directly or indirectly a member of,
+ provided that there is a chain of membership grants each of which has
+ <literal>SET TRUE</literal> (which is the default).
Thus, in the above example, it is not necessary to become
<literal>admin</literal> before becoming <literal>wheel</literal>.
+ On the other hand, it is not possible to become <literal>island</literal>
+ at all; <literal>joe</literal> can only access those privileges via
+ inheritance.
</para>
</note>
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 55219bb097..eca0436a44 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -999,7 +999,7 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
objname);
}
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), new_ownerId);
+ check_can_set_role(GetUserId(), new_ownerId);
/* New owner must have CREATE privilege on namespace */
if (OidIsValid(namespaceId))
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 96b46cbc02..7cc41eca69 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -941,7 +941,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to create database")));
- check_is_member_of_role(GetUserId(), datdba);
+ check_can_set_role(GetUserId(), datdba);
/*
* Lookup database (template) to be cloned, and obtain share lock on it.
@@ -2495,7 +2495,7 @@ AlterDatabaseOwner(const char *dbname, Oid newOwnerId)
dbname);
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/*
* must have createdb rights
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 91f4dd30de..64e01c5841 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -363,7 +363,7 @@ AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
NameStr(form->srvname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have USAGE privilege on foreign-data wrapper */
aclresult = pg_foreign_data_wrapper_aclcheck(form->srvfdw, newOwnerId, ACL_USAGE);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 8514ebfe91..622135d867 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1910,7 +1910,7 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
NameStr(form->pubname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on database */
aclresult = pg_database_aclcheck(MyDatabaseId, newOwnerId, ACL_CREATE);
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 1346104973..46c103bbb4 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -96,7 +96,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
aclcheck_error(aclresult, OBJECT_DATABASE,
get_database_name(MyDatabaseId));
- check_is_member_of_role(saved_uid, owner_uid);
+ check_can_set_role(saved_uid, owner_uid);
/* Additional check to protect reserved schema names */
if (!allowSystemTableMods && IsReservedName(schemaName))
@@ -369,7 +369,7 @@ AlterSchemaOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
NameStr(nspForm->nspname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/*
* must have create-schema rights
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d8a75d23c..627d8f2f20 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13801,7 +13801,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
RelationGetRelationName(target_rel));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(namespaceOid, newOwnerId,
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 33b64fd279..539de56283 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3743,7 +3743,7 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
aclcheck_error_type(ACLCHECK_NOT_OWNER, typTup->oid);
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(typTup->typnamespace,
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 04a18d4a42..e3584250fb 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -51,8 +51,8 @@
* RRG_REMOVE_ADMIN_OPTION indicates a grant that would need to have
* admin_option set to false by the operation.
*
- * RRG_REMOVE_INHERIT_OPTION indicates a grant that would need to have
- * inherit_option set to false by the operation.
+ * Similarly, RRG_REMOVE_INHERIT_OPTION and RRG_REMOVE_SET_OPTION indicate
+ * grants that would need to have the corresponding options set to false.
*
* RRG_DELETE_GRANT indicates a grant that would need to be removed entirely
* by the operation.
@@ -62,6 +62,7 @@ typedef enum
RRG_NOOP,
RRG_REMOVE_ADMIN_OPTION,
RRG_REMOVE_INHERIT_OPTION,
+ RRG_REMOVE_SET_OPTION,
RRG_DELETE_GRANT
} RevokeRoleGrantAction;
@@ -73,10 +74,12 @@ typedef struct
unsigned specified;
bool admin;
bool inherit;
+ bool set;
} GrantRoleOptions;
#define GRANT_ROLE_SPECIFIED_ADMIN 0x0001
#define GRANT_ROLE_SPECIFIED_INHERIT 0x0002
+#define GRANT_ROLE_SPECIFIED_SET 0x0004
/* GUC parameter */
int Password_encryption = PASSWORD_TYPE_SCRAM_SHA_256;
@@ -1389,6 +1392,12 @@ GrantRole(ParseState *pstate, GrantRoleStmt *stmt)
if (parse_bool(optval, &popt.inherit))
continue;
}
+ else if (strcmp(opt->defname, "set") == 0)
+ {
+ popt.specified |= GRANT_ROLE_SPECIFIED_SET;
+ if (parse_bool(optval, &popt.set))
+ continue;
+ }
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
@@ -1776,6 +1785,16 @@ AddRoleMems(const char *rolename, Oid roleid,
at_least_one_change = true;
}
+ if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0
+ && authmem_form->set_option != popt->set)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ at_least_one_change = true;
+ }
+
if (!at_least_one_change)
{
ereport(NOTICE,
@@ -1798,9 +1817,15 @@ AddRoleMems(const char *rolename, Oid roleid,
Oid objectId;
Oid *newmembers = palloc(sizeof(Oid));
- /* Set admin option if user set it to true, otherwise not. */
+ /*
+ * The values for these options can be taken directly from 'popt'.
+ * Either they were specified, or the defaults as set by
+ * InitGrantRoleOptions are correct.
+ */
new_record[Anum_pg_auth_members_admin_option - 1] =
BoolGetDatum(popt->admin);
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
/*
* If the user specified a value for the inherit option, use
@@ -1989,6 +2014,13 @@ DelRoleMems(const char *rolename, Oid roleid,
new_record_repl[Anum_pg_auth_members_inherit_option - 1] =
true;
}
+ else if (actions[i] == RRG_REMOVE_SET_OPTION)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(false);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ }
else
elog(ERROR, "unknown role revoke action");
@@ -2182,6 +2214,11 @@ plan_single_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
*/
actions[i] = RRG_REMOVE_INHERIT_OPTION;
}
+ else if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0)
+ {
+ /* Here too, no need to recurse. */
+ actions[i] = RRG_REMOVE_SET_OPTION;
+ }
else
{
bool revoke_admin_option_only;
@@ -2331,4 +2368,5 @@ InitGrantRoleOptions(GrantRoleOptions *popt)
popt->specified = 0;
popt->admin = false;
popt->inherit = false;
+ popt->set = true;
}
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c795cb7a29..ee060f7254 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -939,7 +939,7 @@ check_role(char **newval, void **extra, GucSource source)
* leader's state.
*/
if (!InitializingParallelWorker &&
- !is_member_of_role(GetSessionUserId(), roleid))
+ !member_can_set_role(GetSessionUserId(), roleid))
{
if (source == PGC_S_TEST)
{
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 4fac402e5b..a7ffbe4f19 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -61,16 +61,17 @@ typedef struct
*
* Each element of cached_roles is an OID list of constituent roles for the
* corresponding element of cached_role (always including the cached_role
- * itself). One cache has ROLERECURSE_PRIVS semantics, and the other has
- * ROLERECURSE_MEMBERS semantics.
+ * itself). There's a separate cache for each RoleRecurseType, with the
+ * corresponding semantics.
*/
enum RoleRecurseType
{
- ROLERECURSE_PRIVS = 0, /* recurse through inheritable grants */
- ROLERECURSE_MEMBERS = 1 /* recurse unconditionally */
+ ROLERECURSE_MEMBERS = 0, /* recurse unconditionally */
+ ROLERECURSE_PRIVS = 1, /* recurse through inheritable grants */
+ ROLERECURSE_SETROLE = 2 /* recurse through grants with set_option */
};
-static Oid cached_role[] = {InvalidOid, InvalidOid};
-static List *cached_roles[] = {NIL, NIL};
+static Oid cached_role[] = {InvalidOid, InvalidOid, InvalidOid};
+static List *cached_roles[] = {NIL, NIL, NIL};
static uint32 cached_db_hash;
@@ -4685,10 +4686,13 @@ convert_role_priv_string(text *priv_type_text)
static const priv_map role_priv_map[] = {
{"USAGE", ACL_USAGE},
{"MEMBER", ACL_CREATE},
+ {"SET", ACL_SET},
{"USAGE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"USAGE WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{NULL, 0}
};
@@ -4717,6 +4721,11 @@ pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode)
if (has_privs_of_role(roleid, role_oid))
return ACLCHECK_OK;
}
+ if (mode & ACL_SET)
+ {
+ if (member_can_set_role(roleid, role_oid))
+ return ACLCHECK_OK;
+ }
return ACLCHECK_NO_PRIV;
}
@@ -4765,15 +4774,17 @@ RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
}
/* Force membership caches to be recomputed on next use */
- cached_role[ROLERECURSE_PRIVS] = InvalidOid;
cached_role[ROLERECURSE_MEMBERS] = InvalidOid;
+ cached_role[ROLERECURSE_PRIVS] = InvalidOid;
+ cached_role[ROLERECURSE_SETROLE] = InvalidOid;
}
/*
* Get a list of roles that the specified roleid is a member of
*
- * Type ROLERECURSE_PRIVS recurses only through inheritable grants,
- * while ROLERECURSE_MEMBERS recurses through all grants.
+ * Type ROLERECURSE_MEMBERS recurses through all grants; ROLERECURSE_PRIVS
+ * recurses only through inheritable grants; and ROLERECURSE_SETROLe recurses
+ * only through grants with set_option.
*
* Since indirect membership testing is relatively expensive, we cache
* a list of memberships. Hence, the result is only guaranteed good until
@@ -4864,6 +4875,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
if (type == ROLERECURSE_PRIVS && !form->inherit_option)
continue;
+ /* If we're supposed to ignore non-SET grants, do so. */
+ if (type == ROLERECURSE_SETROLE && !form->set_option)
+ continue;
+
/*
* Even though there shouldn't be any loops in the membership
* graph, we must test for having already seen this role. It is
@@ -4903,9 +4918,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
/*
* Does member have the privileges of role (directly or indirectly)?
*
- * This is defined not to recurse through grants that are not inherited;
- * in such cases, membership implies the ability to do SET ROLE, but
- * the privileges are not available until you've done so.
+ * This is defined not to recurse through grants that are not inherited,
+ * and only inherited grants confer the associated privileges automatically.
+ *
+ * See also member_can_set_role, below.
*/
bool
has_privs_of_role(Oid member, Oid role)
@@ -4927,48 +4943,86 @@ has_privs_of_role(Oid member, Oid role)
role);
}
-
/*
- * Is member a member of role (directly or indirectly)?
+ * Can member use SET ROLE to this role?
*
- * This is defined to recurse through grants whether they are inherited or not.
+ * There must be a chain of grants from 'member' to 'role' each of which
+ * permits SET ROLE; that is, each of which has set_option = true.
*
- * Do not use this for privilege checking, instead use has_privs_of_role()
+ * It doesn't matter whether the grants are inheritable. That's a separate
+ * question; see has_privs_of_role.
+ *
+ * This function should be used to determine whether the session user can
+ * use SET ROLE to become the target user. We also use it to determine whether
+ * the session user can change an existing object to be owned by the target
+ * user, or create new objects owned by the target user.
*/
bool
-is_member_of_role(Oid member, Oid role)
+member_can_set_role(Oid member, Oid role)
{
/* Fast path for simple case */
if (member == role)
return true;
- /* Superusers have every privilege, so are part of every role */
+ /* Superusers have every privilege, so can always SET ROLE */
if (superuser_arg(member))
return true;
/*
- * Find all the roles that member is a member of, including multi-level
- * recursion, then see if target role is any one of them.
+ * Find all the roles that member can access via SET ROLE, including
+ * multi-level recursion, then see if target role is any one of them.
*/
- return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_SETROLE,
InvalidOid, NULL),
role);
}
/*
- * check_is_member_of_role
- * is_member_of_role with a standard permission-violation error if not
+ * Permission violation eror unless able to SET ROLE to target role.
*/
void
-check_is_member_of_role(Oid member, Oid role)
+check_can_set_role(Oid member, Oid role)
{
- if (!is_member_of_role(member, role))
+ if (!member_can_set_role(member, role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be member of role \"%s\"",
+ errmsg("must be able to SET ROLE \"%s\"",
GetUserNameFromId(role, false))));
}
+/*
+ * Is member a member of role (directly or indirectly)?
+ *
+ * This is defined to recurse through grants whether they are inherited or not.
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role().
+ * Don't use it for determining whether it's possible to SET ROLE to some
+ * other role; for that, use member_can_set_role(). And don't use it for
+ * determining whether it's OK to create an object owned by some other role:
+ * use member_can_set_role() for that, too.
+ *
+ * In short, calling this function is the wrong thing to do nearly everywhere.
+ */
+bool
+is_member_of_role(Oid member, Oid role)
+{
+ /* Fast path for simple case */
+ if (member == role)
+ return true;
+
+ /* Superusers have every privilege, so are part of every role */
+ if (superuser_arg(member))
+ return true;
+
+ /*
+ * Find all the roles that member is a member of, including multi-level
+ * recursion, then see if target role is any one of them.
+ */
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+ InvalidOid, NULL),
+ role);
+}
+
/*
* Is member a member of role, not considering superuserness?
*
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 083012ca39..76a186b639 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -955,8 +955,9 @@ dumpRoleMembership(PGconn *conn)
end,
total;
bool dump_grantors;
- bool dump_inherit_option;
+ bool dump_grant_options;
int i_inherit_option;
+ int i_set_option;
/*
* Previous versions of PostgreSQL didn't used to track the grantor very
@@ -968,10 +969,10 @@ dumpRoleMembership(PGconn *conn)
dump_grantors = (PQserverVersion(conn) >= 160000);
/*
- * Previous versions of PostgreSQL also did not have a grant-level
+ * Previous versions of PostgreSQL also did not have grant-level options.
* INHERIT option.
*/
- dump_inherit_option = (server_version >= 160000);
+ dump_grant_options = (server_version >= 160000);
/* Generate and execute query. */
printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
@@ -979,8 +980,8 @@ dumpRoleMembership(PGconn *conn)
"ug.oid AS grantorid, "
"ug.rolname AS grantor, "
"a.admin_option");
- if (dump_inherit_option)
- appendPQExpBufferStr(buf, ", a.inherit_option");
+ if (dump_grant_options)
+ appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option");
appendPQExpBuffer(buf, " FROM pg_auth_members a "
"LEFT JOIN %s ur on ur.oid = a.roleid "
"LEFT JOIN %s um on um.oid = a.member "
@@ -989,6 +990,7 @@ dumpRoleMembership(PGconn *conn)
"ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog);
res = executeQuery(conn, buf->data);
i_inherit_option = PQfnumber(res, "inherit_option");
+ i_set_option = PQfnumber(res, "set_option");
if (PQntuples(res) > 0)
fprintf(OPF, "--\n-- Role memberships\n--\n\n");
@@ -1059,6 +1061,7 @@ dumpRoleMembership(PGconn *conn)
char *admin_option;
char *grantorid;
char *grantor;
+ char *set_option = "true";
bool found;
/* If we already did this grant, don't do it again. */
@@ -1069,6 +1072,8 @@ dumpRoleMembership(PGconn *conn)
grantorid = PQgetvalue(res, i, 2);
grantor = PQgetvalue(res, i, 3);
admin_option = PQgetvalue(res, i, 4);
+ if (dump_grant_options)
+ set_option = PQgetvalue(res, i, i_set_option);
/*
* If we're not dumping grantors or if the grantor is the
@@ -1098,7 +1103,7 @@ dumpRoleMembership(PGconn *conn)
fprintf(OPF, " TO %s", fmtId(member));
if (*admin_option == 't')
appendPQExpBufferStr(optbuf, "ADMIN OPTION");
- if (dump_inherit_option)
+ if (dump_grant_options)
{
char *inherit_option;
@@ -1109,6 +1114,12 @@ dumpRoleMembership(PGconn *conn)
*inherit_option == 't' ?
"TRUE" : "FALSE");
}
+ if (*set_option != 't')
+ {
+ if (optbuf->data[0] != '\0')
+ appendPQExpBufferStr(optbuf, ", ");
+ appendPQExpBuffer(optbuf, "SET FALSE");
+ }
if (optbuf->data[0] != '\0')
fprintf(OPF, " WITH %s", optbuf->data);
if (dump_grantors)
diff --git a/src/include/catalog/pg_auth_members.h b/src/include/catalog/pg_auth_members.h
index 3ee6ae5f6a..b145fce1ed 100644
--- a/src/include/catalog/pg_auth_members.h
+++ b/src/include/catalog/pg_auth_members.h
@@ -35,6 +35,7 @@ CATALOG(pg_auth_members,1261,AuthMemRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_
Oid grantor BKI_LOOKUP(pg_authid); /* who granted the membership */
bool admin_option; /* granted with admin option? */
bool inherit_option; /* exercise privileges without SET ROLE? */
+ bool set_option; /* use SET ROLE to the target role? */
} FormData_pg_auth_members;
/* ----------------
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 9a4df3a5da..05bfda0530 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,11 +209,12 @@ extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId,
extern int aclmembers(const Acl *acl, Oid **roleids);
extern bool has_privs_of_role(Oid member, Oid role);
+extern bool member_can_set_role(Oid member, Oid role);
+extern void check_can_set_role(Oid member, Oid role);
extern bool is_member_of_role(Oid member, Oid role);
extern bool is_member_of_role_nosuper(Oid member, Oid role);
extern bool is_admin_of_role(Oid member, Oid role);
extern Oid select_best_admin(Oid member, Oid role);
-extern void check_is_member_of_role(Oid member, Oid role);
extern Oid get_role_oid(const char *rolname, bool missing_ok);
extern Oid get_role_oid_or_public(const char *rolname);
extern Oid get_rolespec_oid(const RoleSpec *role, bool missing_ok);
diff --git a/src/test/regress/expected/alter_generic.out b/src/test/regress/expected/alter_generic.out
index 54d3fe5764..ae54cb254f 100644
--- a/src/test/regress/expected/alter_generic.out
+++ b/src/test/regress/expected/alter_generic.out
@@ -46,7 +46,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func2; -- failed (name conflict)
ERROR: function alt_func2(integer) already exists in schema "alt_nsp1"
ALTER FUNCTION alt_func1(int) RENAME TO alt_func3; -- OK
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user3; -- OK
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp1; -- OK, already there
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2; -- OK
@@ -54,7 +54,7 @@ ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg2; -- failed (name conflict)
ERROR: function alt_agg2(integer) already exists in schema "alt_nsp1"
ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg3; -- OK
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user3; -- OK
ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -74,7 +74,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func4; -- OK
ALTER FUNCTION alt_func3(int) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of function alt_func3
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER FUNCTION alt_func3(int) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of function alt_func3
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2; -- failed (name conflicts)
@@ -85,7 +85,7 @@ ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg4; -- OK
ALTER AGGREGATE alt_agg3(int) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of function alt_agg3
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER AGGREGATE alt_agg3(int) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of function alt_agg3
ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -122,7 +122,7 @@ ALTER CONVERSION alt_conv1 RENAME TO alt_conv2; -- failed (name conflict)
ERROR: conversion "alt_conv2" already exists in schema "alt_nsp1"
ALTER CONVERSION alt_conv1 RENAME TO alt_conv3; -- OK
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user3; -- OK
ALTER CONVERSION alt_conv2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -134,7 +134,7 @@ ALTER CONVERSION alt_conv1 RENAME TO alt_conv4; -- OK
ALTER CONVERSION alt_conv3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of conversion alt_conv3
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER CONVERSION alt_conv3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of conversion alt_conv3
ALTER CONVERSION alt_conv2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -196,7 +196,7 @@ ALTER LANGUAGE alt_lang1 RENAME TO alt_lang3; -- OK
ALTER LANGUAGE alt_lang2 OWNER TO regress_alter_generic_user3; -- failed (not owner)
ERROR: must be owner of language alt_lang2
ALTER LANGUAGE alt_lang3 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER LANGUAGE alt_lang3 OWNER TO regress_alter_generic_user3; -- OK
RESET SESSION AUTHORIZATION;
SELECT lanname, a.rolname
@@ -216,7 +216,7 @@ SET SESSION AUTHORIZATION regress_alter_generic_user1;
CREATE OPERATOR @-@ ( leftarg = int4, rightarg = int4, procedure = int4mi );
CREATE OPERATOR @+@ ( leftarg = int4, rightarg = int4, procedure = int4pl );
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR @-@(int4, int4) SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -224,7 +224,7 @@ CREATE OPERATOR @-@ ( leftarg = int4, rightarg = int4, procedure = int4mi );
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator @+@
ALTER OPERATOR @-@(int4, int4) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR @+@(int4, int4) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator @+@
-- can't test this: the error message includes the raw oid of namespace
@@ -259,14 +259,14 @@ ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf2; -- failed (name c
ERROR: operator family "alt_opf2" for access method "hash" already exists in schema "alt_nsp1"
ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf3; -- OK
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR FAMILY alt_opf2 USING hash SET SCHEMA alt_nsp2; -- OK
ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc2; -- failed (name conflict)
ERROR: operator class "alt_opc2" for access method "hash" already exists in schema "alt_nsp1"
ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc3; -- OK
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR CLASS alt_opc2 USING hash SET SCHEMA alt_nsp2; -- OK
RESET SESSION AUTHORIZATION;
@@ -285,7 +285,7 @@ ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf4; -- OK
ALTER OPERATOR FAMILY alt_opf3 USING hash OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator family alt_opf3
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR FAMILY alt_opf3 USING hash SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator family alt_opf3
ALTER OPERATOR FAMILY alt_opf2 USING hash SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -296,7 +296,7 @@ ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc4; -- OK
ALTER OPERATOR CLASS alt_opc3 USING hash OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator class alt_opc3
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR CLASS alt_opc3 USING hash SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator class alt_opc3
ALTER OPERATOR CLASS alt_opc2 USING hash SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -531,7 +531,7 @@ ALTER STATISTICS alt_stat1 RENAME TO alt_stat2; -- failed (name conflict)
ERROR: statistics object "alt_stat2" already exists in schema "alt_nsp1"
ALTER STATISTICS alt_stat1 RENAME TO alt_stat3; -- OK
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user3; -- OK
ALTER STATISTICS alt_stat2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -544,7 +544,7 @@ ALTER STATISTICS alt_stat1 RENAME TO alt_stat4; -- OK
ALTER STATISTICS alt_stat3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of statistics object alt_stat3
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER STATISTICS alt_stat3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of statistics object alt_stat3
ALTER STATISTICS alt_stat2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -573,7 +573,7 @@ ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict2; -- failed (na
ERROR: text search dictionary "alt_ts_dict2" already exists in schema "alt_nsp1"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict3; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user3; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -585,7 +585,7 @@ ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict4; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of text search dictionary alt_ts_dict3
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of text search dictionary alt_ts_dict3
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -614,7 +614,7 @@ ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf2; -- failed
ERROR: text search configuration "alt_ts_conf2" already exists in schema "alt_nsp1"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf3; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user3; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -626,7 +626,7 @@ ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf4; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of text search configuration alt_ts_conf3
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of text search configuration alt_ts_conf3
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 SET SCHEMA alt_nsp2; -- failed (name conflict)
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 9d7610b948..a480b9481c 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -442,7 +442,7 @@ ERROR: invalid option "foo"
ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host);
SET ROLE regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR
-ERROR: must be member of role "regress_test_indirect"
+ERROR: must be able to SET ROLE "regress_test_indirect"
RESET ROLE;
GRANT regress_test_indirect TO regress_test_role;
SET ROLE regress_test_role;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index bd3453ee91..a497db94a8 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -132,6 +132,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+ERROR: permission denied to set role "pg_read_all_settings"
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+ERROR: permission denied to set role "pg_read_all_stats"
+RESET ROLE;
+RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
DROP USER regress_priv_user10;
DROP USER regress_priv_user9;
@@ -2809,3 +2818,35 @@ DROP ROLE regress_group;
DROP ROLE regress_group_direct_manager;
DROP ROLE regress_group_indirect_manager;
DROP ROLE regress_group_member;
+-- test SET and INHERIT options with object ownership changes
+CREATE ROLE regress_roleoption_protagonist;
+CREATE ROLE regress_roleoption_donor;
+CREATE ROLE regress_roleoption_recipient;
+CREATE SCHEMA regress_roleoption;
+GRANT CREATE, USAGE ON SCHEMA regress_roleoption TO PUBLIC;
+GRANT regress_roleoption_donor TO regress_roleoption_protagonist WITH INHERIT TRUE, SET FALSE;
+GRANT regress_roleoption_recipient TO regress_roleoption_protagonist WITH INHERIT FALSE, SET TRUE;
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+CREATE TABLE regress_roleoption.t1 (a int);
+CREATE TABLE regress_roleoption.t2 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_donor;
+CREATE TABLE regress_roleoption.t3 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_recipient;
+CREATE TABLE regress_roleoption.t4 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+ALTER TABLE regress_roleoption.t1 OWNER TO regress_roleoption_donor; -- fails, can't be come donor
+ERROR: must be able to SET ROLE "regress_roleoption_donor"
+ALTER TABLE regress_roleoption.t2 OWNER TO regress_roleoption_recipient; -- works
+ALTER TABLE regress_roleoption.t3 OWNER TO regress_roleoption_protagonist; -- works
+ALTER TABLE regress_roleoption.t4 OWNER TO regress_roleoption_protagonist; -- fails, we don't inherit from recipient
+ERROR: must be owner of table t4
+RESET SESSION AUTHORIZATION;
+DROP TABLE regress_roleoption.t1;
+DROP TABLE regress_roleoption.t2;
+DROP TABLE regress_roleoption.t3;
+DROP TABLE regress_roleoption.t4;
+DROP SCHEMA regress_roleoption;
+DROP ROLE regress_roleoption_recipient;
+DROP ROLE regress_roleoption_donor;
+DROP ROLE regress_roleoption_donor;
+ERROR: role "regress_roleoption_donor" does not exist
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 4ad366470d..daecf0ec64 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -114,6 +114,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
+RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+RESET ROLE;
+
RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
@@ -1813,3 +1822,33 @@ DROP ROLE regress_group;
DROP ROLE regress_group_direct_manager;
DROP ROLE regress_group_indirect_manager;
DROP ROLE regress_group_member;
+
+-- test SET and INHERIT options with object ownership changes
+CREATE ROLE regress_roleoption_protagonist;
+CREATE ROLE regress_roleoption_donor;
+CREATE ROLE regress_roleoption_recipient;
+CREATE SCHEMA regress_roleoption;
+GRANT CREATE, USAGE ON SCHEMA regress_roleoption TO PUBLIC;
+GRANT regress_roleoption_donor TO regress_roleoption_protagonist WITH INHERIT TRUE, SET FALSE;
+GRANT regress_roleoption_recipient TO regress_roleoption_protagonist WITH INHERIT FALSE, SET TRUE;
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+CREATE TABLE regress_roleoption.t1 (a int);
+CREATE TABLE regress_roleoption.t2 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_donor;
+CREATE TABLE regress_roleoption.t3 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_recipient;
+CREATE TABLE regress_roleoption.t4 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+ALTER TABLE regress_roleoption.t1 OWNER TO regress_roleoption_donor; -- fails, can't be come donor
+ALTER TABLE regress_roleoption.t2 OWNER TO regress_roleoption_recipient; -- works
+ALTER TABLE regress_roleoption.t3 OWNER TO regress_roleoption_protagonist; -- works
+ALTER TABLE regress_roleoption.t4 OWNER TO regress_roleoption_protagonist; -- fails, we don't inherit from recipient
+RESET SESSION AUTHORIZATION;
+DROP TABLE regress_roleoption.t1;
+DROP TABLE regress_roleoption.t2;
+DROP TABLE regress_roleoption.t3;
+DROP TABLE regress_roleoption.t4;
+DROP SCHEMA regress_roleoption;
+DROP ROLE regress_roleoption_recipient;
+DROP ROLE regress_roleoption_donor;
+DROP ROLE regress_roleoption_donor;
--
2.24.3 (Apple Git-128)
On Fri, Sep 30, 2022 at 04:34:32PM -0400, Robert Haas wrote:
That thread has not reached an entirely satisfying conclusion.
However, the behavior that was deemed outright buggy over there has
been fixed. The remaining question is what to do about commands that
allow you to give objects to other users (like ALTER <whatever> ..
OWNER TO) or commands that allow you to create objects owned by other
users (like CREATE DATABASE ... OWNER). I have, in this version,
adopted the proposal by Wolfgang Walther on the other thread to make
this controlled by the new SET option. This essentially takes the view
that the ability to create objects owned by another user is not
precisely a privilege, and is thus not inherited just because the
INHERIT option is set on the GRANT, but it is something you can do if
you could SET ROLE to that role, so we make it dependent on the SET
option. This logic is certainly debatable, but it does have the
practical advantage of making INHERIT TRUE, SET FALSE a useful
combination of settings for predefined roles. It's also 100%
backward-compatible, whereas if we made the behavior dependent on the
INHERIT option, users could potentially notice behavior changes after
upgrading to v16.
I'm not sure about tying the ownership stuff to this new SET privilege.
While you noted some practical advantages, I'd expect users to find it kind
of surprising. Also, for predefined roles, I think you need to be careful
about distributing ADMIN, as anyone with ADMIN on a predefined role can
just GRANT SET to work around the restrictions. I don't have a better
idea, though, so perhaps neither of these things is a deal-breaker. I was
tempted to suggest using ADMIN instead of SET for the ownership stuff, but
that wouldn't be backward-compatible, and you'd still be able to work
around it to some extent with SET (e.g., SET ROLE followed by CREATE
DATABASE).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Greetings,
* Nathan Bossart (nathandbossart@gmail.com) wrote:
On Fri, Sep 30, 2022 at 04:34:32PM -0400, Robert Haas wrote:
That thread has not reached an entirely satisfying conclusion.
However, the behavior that was deemed outright buggy over there has
been fixed. The remaining question is what to do about commands that
allow you to give objects to other users (like ALTER <whatever> ..
OWNER TO) or commands that allow you to create objects owned by other
users (like CREATE DATABASE ... OWNER). I have, in this version,
adopted the proposal by Wolfgang Walther on the other thread to make
this controlled by the new SET option. This essentially takes the view
that the ability to create objects owned by another user is not
precisely a privilege, and is thus not inherited just because the
INHERIT option is set on the GRANT, but it is something you can do if
you could SET ROLE to that role, so we make it dependent on the SET
option. This logic is certainly debatable, but it does have the
practical advantage of making INHERIT TRUE, SET FALSE a useful
combination of settings for predefined roles. It's also 100%
backward-compatible, whereas if we made the behavior dependent on the
INHERIT option, users could potentially notice behavior changes after
upgrading to v16.I'm not sure about tying the ownership stuff to this new SET privilege.
While you noted some practical advantages, I'd expect users to find it kind
of surprising. Also, for predefined roles, I think you need to be careful
about distributing ADMIN, as anyone with ADMIN on a predefined role can
just GRANT SET to work around the restrictions. I don't have a better
idea, though, so perhaps neither of these things is a deal-breaker. I was
tempted to suggest using ADMIN instead of SET for the ownership stuff, but
that wouldn't be backward-compatible, and you'd still be able to work
around it to some extent with SET (e.g., SET ROLE followed by CREATE
DATABASE).
As we work through splitting up the privileges and managing them in a
more fine-grained way, it seems clear that we'll need to have a similar
split for ADMIN rights on roles- that is, we'll need to be able to
say "role X is allowed to GRANT INHERIT for role Y to other roles, but
not SET".
I'm still half-tempted to say that predefined roles should just be dealt
with as a special case.. but if we split ADMIN in the manner as
described above then maybe we could get away with not having to, but it
would depend a great deal of people actually reading the documentation
and I'm concerned that's a bit too much to ask in this case.
That is- the first person who is likely to GRANT out ADMIN rights in a
predefined role is going to be a superuser. To avoid breaking backwards
compatibility, GRANT'ing of ADMIN needs to GRANT all the partial-ADMIN
rights that exist, or at least exist today, which includes both SET and
INHERIT. Unless we put some kind of special case for predefined roles
where we throw an error or at least a warning when a superuser
(presumably) inadvertantly does a simple GRANT ADMIN for $predefined
role, we're going to end up in the situation where folks can SET ROLE to
a predefined role and do things that they really shouldn't be allowed
to.
We could, of course, very clearly document that the way to GRANT ADMIN
rights for a predefined role is to always make sure to *only* GRANT
ADMIN/INHERIT, but again I worry that it simply wouldn't be followed in
many cases. Perhaps we could arrange for the bootstrap superuser to
only be GRANT'd ADMIN/INHERIT for predefined roles and then not have an
explicit cut-out for superuser doing a GRANT on predefined roles or
perhaps having such be protected under allow_system_table_mods under the
general consideration that modifying of predefined roles isn't something
that folks should be doing post-initdb.
Just a few thoughts on this, not sure any of these ideas are great but
perhaps this helps move us forward.
Thanks,
Stephen
On Wed, Oct 12, 2022 at 4:59 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
I'm not sure about tying the ownership stuff to this new SET privilege.
While you noted some practical advantages, I'd expect users to find it kind
of surprising. Also, for predefined roles, I think you need to be careful
about distributing ADMIN, as anyone with ADMIN on a predefined role can
just GRANT SET to work around the restrictions. I don't have a better
idea, though, so perhaps neither of these things is a deal-breaker.
Right, I think if you give ADMIN away to someone, that's it: they can
grant that role to whoever they want in whatever mode they want,
including themselves. That seems more or less intentional to me,
though. Giving someone ADMIN OPTION on a role is basically making them
an administrator of that role, and then it is not surprising that they
can access its privileges.
I agree with your other caveat about it being potentially surprising,
but I think it's not worse than a lot of other somewhat surprising
things that we handle by documenting them. And I don't have a better
idea either.
I was
tempted to suggest using ADMIN instead of SET for the ownership stuff, but
that wouldn't be backward-compatible, and you'd still be able to work
around it to some extent with SET (e.g., SET ROLE followed by CREATE
DATABASE).
I think that would be way worse. Giving ADMIN OPTION on a role is like
making someone the owner of the object, whereas giving someone INHERIT
or SET on a role is just a privilege to use the object.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Sun, Oct 16, 2022 at 12:34 PM Stephen Frost <sfrost@snowman.net> wrote:
As we work through splitting up the privileges and managing them in a
more fine-grained way, it seems clear that we'll need to have a similar
split for ADMIN rights on roles- that is, we'll need to be able to
say "role X is allowed to GRANT INHERIT for role Y to other roles, but
not SET".
I don't think this is clear at all, actually. I see very little
advantage in splitting up ADMIN OPTION this way. I did think about
this, because it would be more consistent with what we do for table
privileges, but INHERIT and SET overlap enough from a permissions
point of view that there doesn't seem to be a lot of value in it. Now,
if we invent a bunch more per-grant options, things might look
different, but in my opinion that has dubious value. Right now, all
role privileges other than the ones that are controlled by ADMIN
OPTION, INHERIT, and what I'm proposing to make controlled by SET, are
gated by CREATEROLE or by SUPERUSER. The list looks something like
this: change the INHERIT flag on a role, change the CREATEROLE flag on
a role, change the CREATEDB flag on a role, change the connection
limit for a role, change the VALID UNTIL time for a role, change the
password for a role other than your own, drop the role.
And that's a pretty obscure list of things. I do think we need better
ways to control who can do those things, but I don't think making them
all role privileges and then on top of that giving them all separate
admin options is the right way to go. It's slicing things incredibly
finely to give alice the right to grant to some other user the right
to set only the VALID UNTIL time on role bob, but not the right to
modify role bob in any other way or the right to confer the ability to
set VALID UNTIL for any other user. I can't believe we want to go
there. It's not worth the permissions bits, and even if we had
infinite privilege bits available, it's not worth the complexity from
a user perspective. Maybe you have some less-obscure list of things
that you think should be grantable privileges on roles?
Another thing to consider is that, since ADMIN OPTION is, as I
understand it, part of the SQL specification, I think it would move us
further from the SQL specification. I think we will be better off
thinking of ADMIN OPTION on a role as roughly equivalent to being the
owner of that role, which is an indivisible privilege, rather than
thinking of it as equivalent to GRANT OPTION on each of N rights,
which could then be subdivided.
I'm still half-tempted to say that predefined roles should just be dealt
with as a special case.. but if we split ADMIN in the manner as
described above then maybe we could get away with not having to, but it
would depend a great deal of people actually reading the documentation
and I'm concerned that's a bit too much to ask in this case.
I don't think any splitting of ADMIN would be required to solve the
predefined roles problem. Doesn't the patch I proposed do that?
--
Robert Haas
EDB: http://www.enterprisedb.com
Bump.
Discussion has trailed off here, but I still don't see that we have a
better way forward here than what I proposed on September 30th. Two
people have commented. Nathan said that he wasn't sure this was best
(neither am I) but that he didn't have a better idea either (neither
do I). Stephen proposed decomposing ADMIN OPTION, which is not my
preference, but even if it turns out that we want to pursue that
approach, I do not think it would make sense to bundle that into this
patch, because there isn't enough overlap between that change and this
change to justify that treatment.
If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon. Otherwise, I am going to press
forward with committing this. If we do not, we will continue to have
no way of restricting of SET ROLE, and we will continue to have no way
of preventing the creation of objects owned by predefined roles by
users who have been granted those roles. As far as I am aware, no one
is opposed to those goals, and in fact I think everyone who has
commented thinks that it would be good to do something. If a better
idea than what I've implemented comes along, I'm happy to defer to it,
but I think this is one of those cases in which there probably isn't
any totally satisfying solution, and yet doing nothing is not a
superior alternative.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Nov 15, 2022 at 12:07:06PM -0500, Robert Haas wrote:
If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon. Otherwise, I am going to press
forward with committing this.
I don't think I have any further thoughts about the approach, so I won't
balk if you proceed with this change. It might be worth starting a new
thread to discuss whether to treat predefined roles as a special case, but
IMO that needn't hold up this patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, 2022-11-15 at 12:07 -0500, Robert Haas wrote:
If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon.
Did you have some thoughts on:
/messages/by-id/a41d606daaaa03b629c2ef0ed274ae3b04a2c266.camel@j-davis.com
Regards,
Jeff Davis
On Tue, Nov 15, 2022 at 7:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
On Tue, 2022-11-15 at 12:07 -0500, Robert Haas wrote:
If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon.Did you have some thoughts on:
/messages/by-id/a41d606daaaa03b629c2ef0ed274ae3b04a2c266.camel@j-davis.com
I mean, I think what we were discussing there could be done, but it's
not the approach I like best. That's partly because that was just a
back-of-the-envelope sketch of an idea, not a real proposal for
something with a clear implementation path. But I think the bigger
reason is that, in my opinion, this proposal is more generally useful,
because it takes no position on why you wish to disallow SET ROLE. You
can just disallow it in some cases and allow it in others, and that's
fine. That proposal targets a specific use case, which may make it a
better solution to that particular problem, but it makes it unworkable
as a solution to any other problem, I believe.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
But I think the bigger
reason is that, in my opinion, this proposal is more generally
useful,
because it takes no position on why you wish to disallow SET ROLE.
You
can just disallow it in some cases and allow it in others, and that's
fine.
I agree that the it's more flexible in the sense that it does what it
does, and administrators can use it if it's useful for them. That means
we don't need to understand the actual goals as well; but it also means
that it's harder to determine the consequences if we tweak the behavior
(or any related behavior) later.
I'll admit that I don't have an example of a likely problem here,
though.
That proposal targets a specific use case, which may make it a
better solution to that particular problem, but it makes it
unworkable
as a solution to any other problem, I believe.
Yeah, that's the flip side: "virtual" roles (for lack of a better name)
are a more narrow fix for the problem as I understand it; but it might
leave related problems unfixed. You and Stephen[2]/messages/by-id/YzIAGCrxoXibAKOD@tamriel.snowman.net both seemed to
consider this approach, and I happened to like it, so I wanted to make
sure that it wasn't dismissed too quickly.
But I'm fine if you'd like to move on with the SET ROLE privilege
instead, as long as we believe it grants a stable set of capabilities
(and conversely, that if the SET ROLE privilege is revoked, that it
revokes a stable set of capabilities).
[2]: /messages/by-id/YzIAGCrxoXibAKOD@tamriel.snowman.net
/messages/by-id/YzIAGCrxoXibAKOD@tamriel.snowman.net
--
Jeff Davis
PostgreSQL Contributor Team - AWS
On Thu, Nov 17, 2022 at 7:24 PM Jeff Davis <pgsql@j-davis.com> wrote:
But I'm fine if you'd like to move on with the SET ROLE privilege
instead, as long as we believe it grants a stable set of capabilities
(and conversely, that if the SET ROLE privilege is revoked, that it
revokes a stable set of capabilities).
OK.
Here's a rebased v3 to see what cfbot thinks.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v3-0001-Add-a-SET-option-to-the-GRANT-command.patchapplication/octet-stream; name=v3-0001-Add-a-SET-option-to-the-GRANT-command.patchDownload
From 3d14e171e9e2236139e8976f3309a588bcc8683b Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 18 Nov 2022 12:32:50 -0500
Subject: [PATCH v3] Add a SET option to the GRANT command.
Similar to how the INHERIT option controls whether or not the
permissions of the granted role are automatically available to the
grantee, the new SET permission controls whether or not the grantee
may use the SET ROLE command to assume the privileges of the granted
role.
In addition, the new SET permission controls whether or not it
is possible to transfer ownership of objects to the target role
or to create new objects owned by the target role using commands
such as CREATE DATABASE .. OWNER. We could alternatively have made
this controlled by the INHERIT option, or allow it when either
option is given. An advantage of this approach is that if you
are granted a predefined role with INHERIT TRUE, SET FALSE, you
can't go and create objects owned by that role.
The underlying theory here is that the ability to create objects
as a target role is not a privilege per se, and thus does not
depend on whether you inherit the target role's privileges. However,
it's surely something you could do anyway if you could SET ROLE
to the target role, and thus making it contingent on whether you
have that ability is reasonable.
Design review by Nathan Bossat, Wolfgang Walther, Jeff Davis,
Peter Eisentraut, and Stephen Frost.
Discussion: http://postgr.es/m/CA+Tgmob+zDSRS6JXYrgq0NWdzCXuTNzT5eK54Dn2hhgt17nm8A@mail.gmail.com
---
doc/src/sgml/catalogs.sgml | 11 ++
doc/src/sgml/func.sgml | 9 +-
doc/src/sgml/ref/grant.sgml | 32 ++++--
doc/src/sgml/ref/revoke.sgml | 8 +-
doc/src/sgml/ref/set_role.sgml | 9 +-
doc/src/sgml/user-manag.sgml | 20 +++-
src/backend/commands/alter.c | 2 +-
src/backend/commands/dbcommands.c | 4 +-
src/backend/commands/foreigncmds.c | 2 +-
src/backend/commands/publicationcmds.c | 2 +-
src/backend/commands/schemacmds.c | 4 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/typecmds.c | 2 +-
src/backend/commands/user.c | 44 +++++++-
src/backend/commands/variable.c | 2 +-
src/backend/utils/adt/acl.c | 106 +++++++++++++++-----
src/bin/pg_dump/pg_dumpall.c | 23 +++--
src/include/catalog/pg_auth_members.h | 1 +
src/include/utils/acl.h | 3 +-
src/test/regress/expected/alter_generic.out | 38 +++----
src/test/regress/expected/foreign_data.out | 2 +-
src/test/regress/expected/privileges.out | 41 ++++++++
src/test/regress/sql/privileges.sql | 39 +++++++
23 files changed, 315 insertions(+), 91 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 00f833d210..9ed2b020b7 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1727,6 +1727,17 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
granted role
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>set_option</structfield> <type>bool</type>
+ </para>
+ <para>
+ True if the member can
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ to the granted role
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6e0425cb3d..82fba48d5f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23033,11 +23033,14 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
<para>
Does user have privilege for role?
Allowable privilege types are
- <literal>MEMBER</literal> and <literal>USAGE</literal>.
+ <literal>MEMBER</literal>, <literal>USAGE</literal>,
+ and <literal>SET</literal>.
<literal>MEMBER</literal> denotes direct or indirect membership in
- the role (that is, the right to do <command>SET ROLE</command>), while
+ the role without regard to what specific privileges may be conferred.
<literal>USAGE</literal> denotes whether the privileges of the role
- are immediately available without doing <command>SET ROLE</command>.
+ are immediately available without doing <command>SET ROLE</command>,
+ while <literal>SET</literal> denotes whether it is possible to change
+ to the role using the <literal>SET ROLE</literal> command.
This function does not allow the special case of
setting <parameter>user</parameter> to <literal>public</literal>,
because the PUBLIC pseudo-role can never be a member of real roles.
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index dea19cd348..d5911ff942 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -98,7 +98,7 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replaceable class="parameter">role_specification</replaceable> [, ...]
- [ WITH { ADMIN | INHERIT } { OPTION | TRUE | FALSE } ]
+ [ WITH { ADMIN | INHERIT | SET } { OPTION | TRUE | FALSE } ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
<phrase>where <replaceable class="parameter">role_specification</replaceable> can be:</phrase>
@@ -250,17 +250,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
<para>
This variant of the <command>GRANT</command> command grants membership
in a role to one or more other roles. Membership in a role is significant
- because it conveys the privileges granted to a role to each of its
- members.
+ because it potentially allows access to the privileges granted to a role
+ to each of its members, and potentially also the ability to make changes
+ to the role itself. However, the actualy permisions conferred depend on
+ the options associated with the grant.
</para>
<para>
- The effect of membership in a role can be modified by specifying the
- <literal>ADMIN</literal> or <literal>INHERIT</literal> option, each
- of which can be set to either <literal>TRUE</literal> or
- <literal>FALSE</literal>. The keyword <literal>OPTION</literal> is accepted
- as a synonym for <literal>TRUE</literal>, so that
- <literal>WITH ADMIN OPTION</literal>
+ Each of the options described below can be set to either
+ <literal>TRUE</literal> or <literal>FALSE</literal>. The keyword
+ <literal>OPTION</literal> is accepted as a synonym for
+ <literal>TRUE</literal>, so that <literal>WITH ADMIN OPTION</literal>
is a synonym for <literal>WITH ADMIN TRUE</literal>.
</para>
@@ -272,7 +272,8 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
OPTION</literal> on itself. Database superusers can grant or revoke
membership in any role to anyone. Roles having
<literal>CREATEROLE</literal> privilege can grant or revoke membership
- in any role that is not a superuser.
+ in any role that is not a superuser. This option defaults to
+ <literal>FALSE</literal>.
</para>
<para>
@@ -287,6 +288,17 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
See <link linkend="sql-createrole"><command>CREATE ROLE</command></link>.
</para>
+ <para>
+ The <literal>SET</literal> option, if it is set to
+ <literal>TRUE</literal>, allows the member to change to the granted
+ role using the
+ <link linkend="sql-set-role"><command>SET ROLE</command></link>
+ command. If a role is an indirect member of another role, it can use
+ <literal>SET ROLE</literal> to change to that role only if there is a
+ chain of grants each of which has <literal>SET TRUE</literal>.
+ This option defaults to <literal>TRUE</literal>.
+ </para>
+
<para>
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
having been done by the specified role. A user can only attribute a grant
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 4fd4bfb3d7..2db66bbf37 100644
--- a/doc/src/sgml/ref/revoke.sgml
+++ b/doc/src/sgml/ref/revoke.sgml
@@ -125,7 +125,7 @@ REVOKE [ GRANT OPTION FOR ]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
-REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
+REVOKE [ { ADMIN | INHERIT | SET } OPTION FOR ]
<replaceable class="parameter">role_name</replaceable> [, ...] FROM <replaceable class="parameter">role_specification</replaceable> [, ...]
[ GRANTED BY <replaceable class="parameter">role_specification</replaceable> ]
[ CASCADE | RESTRICT ]
@@ -209,9 +209,9 @@ REVOKE [ { ADMIN | INHERIT } OPTION FOR ]
<para>
Just as <literal>ADMIN OPTION</literal> can be removed from an existing
- role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>.
- This is equivalent to setting the value of that option to
- <literal>FALSE</literal>.
+ role grant, it is also possible to revoke <literal>INHERIT OPTION</literal>
+ or <literal>SET OPTION</literal>. This is equivalent to setting the value
+ of the corresponding option to <literal>FALSE</literal>.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index deecfe4120..13bad1bf66 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -77,14 +77,17 @@ RESET ROLE
effectively drops all the privileges except for those which the target role
directly possesses or inherits. On the other hand, if the session user role
has been granted memberships <literal>WITH INHERIT FALSE</literal>, the
- privileges of the granted roles can't be accessed by default. However, the
+ privileges of the granted roles can't be accessed by default. However, if
+ the role was granted <literal>WITH SET TRUE</literal>, the
session user can use <command>SET ROLE</command> to drop the privileges
assigned directly to the session user and instead acquire the privileges
- available to the named role.
+ available to the named role. If the role was granted <literal>WITH INHERIT
+ FALSE, SET FALSE</literal> then the privileges of that role cannot be
+ exercised either with or without <literal>SET ROLE</literal>.
</para>
<para>
- In particular, when a superuser chooses to <command>SET ROLE</command> to a
+ Note that when a superuser chooses to <command>SET ROLE</command> to a
non-superuser role, they lose their superuser privileges.
</para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index fc836d5748..601fff3e6b 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -354,7 +354,8 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
<para>
The members of a group role can use the privileges of the role in two
- ways. First, every member of a group can explicitly do
+ ways. First, member roles that have been granted membership with the
+ <literal>SET</literal> option can do
<link linkend="sql-set-role"><command>SET ROLE</command></link> to
temporarily <quote>become</quote> the group role. In this state, the
database session has access to the privileges of the group role rather
@@ -369,13 +370,16 @@ REVOKE <replaceable>group_role</replaceable> FROM <replaceable>role1</replaceabl
CREATE ROLE joe LOGIN;
CREATE ROLE admin;
CREATE ROLE wheel;
+CREATE ROLE island;
GRANT admin TO joe WITH INHERIT TRUE;
GRANT wheel TO admin WITH INHERIT FALSE;
+GRANT island TO joe WITH INHERIT TRUE, SET FALSE;
</programlisting>
Immediately after connecting as role <literal>joe</literal>, a database
session will have use of privileges granted directly to <literal>joe</literal>
- plus any privileges granted to <literal>admin</literal>, because <literal>joe</literal>
- <quote>inherits</quote> <literal>admin</literal>'s privileges. However, privileges
+ plus any privileges granted to <literal>admin</literal> and
+ <literal>island</literal>, because <literal>joe</literal>
+ <quote>inherits</quote> those privileges. However, privileges
granted to <literal>wheel</literal> are not available, because even though
<literal>joe</literal> is indirectly a member of <literal>wheel</literal>, the
membership is via <literal>admin</literal> which was granted using
@@ -384,7 +388,8 @@ GRANT wheel TO admin WITH INHERIT FALSE;
SET ROLE admin;
</programlisting>
the session would have use of only those privileges granted to
- <literal>admin</literal>, and not those granted to <literal>joe</literal>. After:
+ <literal>admin</literal>, and not those granted to <literal>joe</literal> or
+ <literal>island</literal>. After:
<programlisting>
SET ROLE wheel;
</programlisting>
@@ -402,9 +407,14 @@ RESET ROLE;
<note>
<para>
The <command>SET ROLE</command> command always allows selecting any role
- that the original login role is directly or indirectly a member of.
+ that the original login role is directly or indirectly a member of,
+ provided that there is a chain of membership grants each of which has
+ <literal>SET TRUE</literal> (which is the default).
Thus, in the above example, it is not necessary to become
<literal>admin</literal> before becoming <literal>wheel</literal>.
+ On the other hand, it is not possible to become <literal>island</literal>
+ at all; <literal>joe</literal> can only access those privileges via
+ inheritance.
</para>
</note>
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index b2089d785b..10b6fe19a2 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -999,7 +999,7 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
objname);
}
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), new_ownerId);
+ check_can_set_role(GetUserId(), new_ownerId);
/* New owner must have CREATE privilege on namespace */
if (OidIsValid(namespaceId))
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index a67ea86619..6eb8742718 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -941,7 +941,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to create database")));
- check_is_member_of_role(GetUserId(), datdba);
+ check_can_set_role(GetUserId(), datdba);
/*
* Lookup database (template) to be cloned, and obtain share lock on it.
@@ -2495,7 +2495,7 @@ AlterDatabaseOwner(const char *dbname, Oid newOwnerId)
dbname);
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/*
* must have createdb rights
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 55b0be9e1d..28b5c75f11 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -363,7 +363,7 @@ AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
NameStr(form->srvname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have USAGE privilege on foreign-data wrapper */
aclresult = object_aclcheck(ForeignDataWrapperRelationId, form->srvfdw, newOwnerId, ACL_USAGE);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 940655b9be..20fa72c5c8 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1911,7 +1911,7 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
NameStr(form->pubname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on database */
aclresult = object_aclcheck(DatabaseRelationId, MyDatabaseId, newOwnerId, ACL_CREATE);
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index b03f07a232..12cbfba7d0 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -97,7 +97,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
aclcheck_error(aclresult, OBJECT_DATABASE,
get_database_name(MyDatabaseId));
- check_is_member_of_role(saved_uid, owner_uid);
+ check_can_set_role(saved_uid, owner_uid);
/* Additional check to protect reserved schema names */
if (!allowSystemTableMods && IsReservedName(schemaName))
@@ -370,7 +370,7 @@ AlterSchemaOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
NameStr(nspForm->nspname));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/*
* must have create-schema rights
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f006807852..845208d662 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13833,7 +13833,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
RelationGetRelationName(target_rel));
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = object_aclcheck(NamespaceRelationId, namespaceOid, newOwnerId,
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index ecc8b3f44c..7770a86bee 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3745,7 +3745,7 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
aclcheck_error_type(ACLCHECK_NOT_OWNER, typTup->oid);
/* Must be able to become new owner */
- check_is_member_of_role(GetUserId(), newOwnerId);
+ check_can_set_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = object_aclcheck(NamespaceRelationId, typTup->typnamespace,
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 2369cc600c..8b6543edee 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -51,8 +51,8 @@
* RRG_REMOVE_ADMIN_OPTION indicates a grant that would need to have
* admin_option set to false by the operation.
*
- * RRG_REMOVE_INHERIT_OPTION indicates a grant that would need to have
- * inherit_option set to false by the operation.
+ * Similarly, RRG_REMOVE_INHERIT_OPTION and RRG_REMOVE_SET_OPTION indicate
+ * grants that would need to have the corresponding options set to false.
*
* RRG_DELETE_GRANT indicates a grant that would need to be removed entirely
* by the operation.
@@ -62,6 +62,7 @@ typedef enum
RRG_NOOP,
RRG_REMOVE_ADMIN_OPTION,
RRG_REMOVE_INHERIT_OPTION,
+ RRG_REMOVE_SET_OPTION,
RRG_DELETE_GRANT
} RevokeRoleGrantAction;
@@ -73,10 +74,12 @@ typedef struct
unsigned specified;
bool admin;
bool inherit;
+ bool set;
} GrantRoleOptions;
#define GRANT_ROLE_SPECIFIED_ADMIN 0x0001
#define GRANT_ROLE_SPECIFIED_INHERIT 0x0002
+#define GRANT_ROLE_SPECIFIED_SET 0x0004
/* GUC parameter */
int Password_encryption = PASSWORD_TYPE_SCRAM_SHA_256;
@@ -1389,6 +1392,12 @@ GrantRole(ParseState *pstate, GrantRoleStmt *stmt)
if (parse_bool(optval, &popt.inherit))
continue;
}
+ else if (strcmp(opt->defname, "set") == 0)
+ {
+ popt.specified |= GRANT_ROLE_SPECIFIED_SET;
+ if (parse_bool(optval, &popt.set))
+ continue;
+ }
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
@@ -1776,6 +1785,16 @@ AddRoleMems(const char *rolename, Oid roleid,
at_least_one_change = true;
}
+ if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0
+ && authmem_form->set_option != popt->set)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ at_least_one_change = true;
+ }
+
if (!at_least_one_change)
{
ereport(NOTICE,
@@ -1798,9 +1817,15 @@ AddRoleMems(const char *rolename, Oid roleid,
Oid objectId;
Oid *newmembers = palloc(sizeof(Oid));
- /* Set admin option if user set it to true, otherwise not. */
+ /*
+ * The values for these options can be taken directly from 'popt'.
+ * Either they were specified, or the defaults as set by
+ * InitGrantRoleOptions are correct.
+ */
new_record[Anum_pg_auth_members_admin_option - 1] =
BoolGetDatum(popt->admin);
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(popt->set);
/*
* If the user specified a value for the inherit option, use
@@ -1989,6 +2014,13 @@ DelRoleMems(const char *rolename, Oid roleid,
new_record_repl[Anum_pg_auth_members_inherit_option - 1] =
true;
}
+ else if (actions[i] == RRG_REMOVE_SET_OPTION)
+ {
+ new_record[Anum_pg_auth_members_set_option - 1] =
+ BoolGetDatum(false);
+ new_record_repl[Anum_pg_auth_members_set_option - 1] =
+ true;
+ }
else
elog(ERROR, "unknown role revoke action");
@@ -2182,6 +2214,11 @@ plan_single_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
*/
actions[i] = RRG_REMOVE_INHERIT_OPTION;
}
+ else if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0)
+ {
+ /* Here too, no need to recurse. */
+ actions[i] = RRG_REMOVE_SET_OPTION;
+ }
else
{
bool revoke_admin_option_only;
@@ -2331,4 +2368,5 @@ InitGrantRoleOptions(GrantRoleOptions *popt)
popt->specified = 0;
popt->admin = false;
popt->inherit = false;
+ popt->set = true;
}
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 791bac6715..00d8d54d82 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -939,7 +939,7 @@ check_role(char **newval, void **extra, GucSource source)
* leader's state.
*/
if (!InitializingParallelWorker &&
- !is_member_of_role(GetSessionUserId(), roleid))
+ !member_can_set_role(GetSessionUserId(), roleid))
{
if (source == PGC_S_TEST)
{
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 8bdb9461b7..d4d68f9724 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -67,16 +67,17 @@ typedef struct
*
* Each element of cached_roles is an OID list of constituent roles for the
* corresponding element of cached_role (always including the cached_role
- * itself). One cache has ROLERECURSE_PRIVS semantics, and the other has
- * ROLERECURSE_MEMBERS semantics.
+ * itself). There's a separate cache for each RoleRecurseType, with the
+ * corresponding semantics.
*/
enum RoleRecurseType
{
- ROLERECURSE_PRIVS = 0, /* recurse through inheritable grants */
- ROLERECURSE_MEMBERS = 1 /* recurse unconditionally */
+ ROLERECURSE_MEMBERS = 0, /* recurse unconditionally */
+ ROLERECURSE_PRIVS = 1, /* recurse through inheritable grants */
+ ROLERECURSE_SETROLE = 2 /* recurse through grants with set_option */
};
-static Oid cached_role[] = {InvalidOid, InvalidOid};
-static List *cached_roles[] = {NIL, NIL};
+static Oid cached_role[] = {InvalidOid, InvalidOid, InvalidOid};
+static List *cached_roles[] = {NIL, NIL, NIL};
static uint32 cached_db_hash;
@@ -4691,10 +4692,13 @@ convert_role_priv_string(text *priv_type_text)
static const priv_map role_priv_map[] = {
{"USAGE", ACL_USAGE},
{"MEMBER", ACL_CREATE},
+ {"SET", ACL_SET},
{"USAGE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"USAGE WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{"MEMBER WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+ {"SET WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
{NULL, 0}
};
@@ -4723,6 +4727,11 @@ pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode)
if (has_privs_of_role(roleid, role_oid))
return ACLCHECK_OK;
}
+ if (mode & ACL_SET)
+ {
+ if (member_can_set_role(roleid, role_oid))
+ return ACLCHECK_OK;
+ }
return ACLCHECK_NO_PRIV;
}
@@ -4771,15 +4780,17 @@ RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
}
/* Force membership caches to be recomputed on next use */
- cached_role[ROLERECURSE_PRIVS] = InvalidOid;
cached_role[ROLERECURSE_MEMBERS] = InvalidOid;
+ cached_role[ROLERECURSE_PRIVS] = InvalidOid;
+ cached_role[ROLERECURSE_SETROLE] = InvalidOid;
}
/*
* Get a list of roles that the specified roleid is a member of
*
- * Type ROLERECURSE_PRIVS recurses only through inheritable grants,
- * while ROLERECURSE_MEMBERS recurses through all grants.
+ * Type ROLERECURSE_MEMBERS recurses through all grants; ROLERECURSE_PRIVS
+ * recurses only through inheritable grants; and ROLERECURSE_SETROLe recurses
+ * only through grants with set_option.
*
* Since indirect membership testing is relatively expensive, we cache
* a list of memberships. Hence, the result is only guaranteed good until
@@ -4870,6 +4881,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
if (type == ROLERECURSE_PRIVS && !form->inherit_option)
continue;
+ /* If we're supposed to ignore non-SET grants, do so. */
+ if (type == ROLERECURSE_SETROLE && !form->set_option)
+ continue;
+
/*
* Even though there shouldn't be any loops in the membership
* graph, we must test for having already seen this role. It is
@@ -4909,9 +4924,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
/*
* Does member have the privileges of role (directly or indirectly)?
*
- * This is defined not to recurse through grants that are not inherited;
- * in such cases, membership implies the ability to do SET ROLE, but
- * the privileges are not available until you've done so.
+ * This is defined not to recurse through grants that are not inherited,
+ * and only inherited grants confer the associated privileges automatically.
+ *
+ * See also member_can_set_role, below.
*/
bool
has_privs_of_role(Oid member, Oid role)
@@ -4933,48 +4949,86 @@ has_privs_of_role(Oid member, Oid role)
role);
}
-
/*
- * Is member a member of role (directly or indirectly)?
+ * Can member use SET ROLE to this role?
*
- * This is defined to recurse through grants whether they are inherited or not.
+ * There must be a chain of grants from 'member' to 'role' each of which
+ * permits SET ROLE; that is, each of which has set_option = true.
*
- * Do not use this for privilege checking, instead use has_privs_of_role()
+ * It doesn't matter whether the grants are inheritable. That's a separate
+ * question; see has_privs_of_role.
+ *
+ * This function should be used to determine whether the session user can
+ * use SET ROLE to become the target user. We also use it to determine whether
+ * the session user can change an existing object to be owned by the target
+ * user, or create new objects owned by the target user.
*/
bool
-is_member_of_role(Oid member, Oid role)
+member_can_set_role(Oid member, Oid role)
{
/* Fast path for simple case */
if (member == role)
return true;
- /* Superusers have every privilege, so are part of every role */
+ /* Superusers have every privilege, so can always SET ROLE */
if (superuser_arg(member))
return true;
/*
- * Find all the roles that member is a member of, including multi-level
- * recursion, then see if target role is any one of them.
+ * Find all the roles that member can access via SET ROLE, including
+ * multi-level recursion, then see if target role is any one of them.
*/
- return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_SETROLE,
InvalidOid, NULL),
role);
}
/*
- * check_is_member_of_role
- * is_member_of_role with a standard permission-violation error if not
+ * Permission violation eror unless able to SET ROLE to target role.
*/
void
-check_is_member_of_role(Oid member, Oid role)
+check_can_set_role(Oid member, Oid role)
{
- if (!is_member_of_role(member, role))
+ if (!member_can_set_role(member, role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be member of role \"%s\"",
+ errmsg("must be able to SET ROLE \"%s\"",
GetUserNameFromId(role, false))));
}
+/*
+ * Is member a member of role (directly or indirectly)?
+ *
+ * This is defined to recurse through grants whether they are inherited or not.
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role().
+ * Don't use it for determining whether it's possible to SET ROLE to some
+ * other role; for that, use member_can_set_role(). And don't use it for
+ * determining whether it's OK to create an object owned by some other role:
+ * use member_can_set_role() for that, too.
+ *
+ * In short, calling this function is the wrong thing to do nearly everywhere.
+ */
+bool
+is_member_of_role(Oid member, Oid role)
+{
+ /* Fast path for simple case */
+ if (member == role)
+ return true;
+
+ /* Superusers have every privilege, so are part of every role */
+ if (superuser_arg(member))
+ return true;
+
+ /*
+ * Find all the roles that member is a member of, including multi-level
+ * recursion, then see if target role is any one of them.
+ */
+ return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+ InvalidOid, NULL),
+ role);
+}
+
/*
* Is member a member of role, not considering superuserness?
*
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 083012ca39..76a186b639 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -955,8 +955,9 @@ dumpRoleMembership(PGconn *conn)
end,
total;
bool dump_grantors;
- bool dump_inherit_option;
+ bool dump_grant_options;
int i_inherit_option;
+ int i_set_option;
/*
* Previous versions of PostgreSQL didn't used to track the grantor very
@@ -968,10 +969,10 @@ dumpRoleMembership(PGconn *conn)
dump_grantors = (PQserverVersion(conn) >= 160000);
/*
- * Previous versions of PostgreSQL also did not have a grant-level
+ * Previous versions of PostgreSQL also did not have grant-level options.
* INHERIT option.
*/
- dump_inherit_option = (server_version >= 160000);
+ dump_grant_options = (server_version >= 160000);
/* Generate and execute query. */
printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
@@ -979,8 +980,8 @@ dumpRoleMembership(PGconn *conn)
"ug.oid AS grantorid, "
"ug.rolname AS grantor, "
"a.admin_option");
- if (dump_inherit_option)
- appendPQExpBufferStr(buf, ", a.inherit_option");
+ if (dump_grant_options)
+ appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option");
appendPQExpBuffer(buf, " FROM pg_auth_members a "
"LEFT JOIN %s ur on ur.oid = a.roleid "
"LEFT JOIN %s um on um.oid = a.member "
@@ -989,6 +990,7 @@ dumpRoleMembership(PGconn *conn)
"ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog);
res = executeQuery(conn, buf->data);
i_inherit_option = PQfnumber(res, "inherit_option");
+ i_set_option = PQfnumber(res, "set_option");
if (PQntuples(res) > 0)
fprintf(OPF, "--\n-- Role memberships\n--\n\n");
@@ -1059,6 +1061,7 @@ dumpRoleMembership(PGconn *conn)
char *admin_option;
char *grantorid;
char *grantor;
+ char *set_option = "true";
bool found;
/* If we already did this grant, don't do it again. */
@@ -1069,6 +1072,8 @@ dumpRoleMembership(PGconn *conn)
grantorid = PQgetvalue(res, i, 2);
grantor = PQgetvalue(res, i, 3);
admin_option = PQgetvalue(res, i, 4);
+ if (dump_grant_options)
+ set_option = PQgetvalue(res, i, i_set_option);
/*
* If we're not dumping grantors or if the grantor is the
@@ -1098,7 +1103,7 @@ dumpRoleMembership(PGconn *conn)
fprintf(OPF, " TO %s", fmtId(member));
if (*admin_option == 't')
appendPQExpBufferStr(optbuf, "ADMIN OPTION");
- if (dump_inherit_option)
+ if (dump_grant_options)
{
char *inherit_option;
@@ -1109,6 +1114,12 @@ dumpRoleMembership(PGconn *conn)
*inherit_option == 't' ?
"TRUE" : "FALSE");
}
+ if (*set_option != 't')
+ {
+ if (optbuf->data[0] != '\0')
+ appendPQExpBufferStr(optbuf, ", ");
+ appendPQExpBuffer(optbuf, "SET FALSE");
+ }
if (optbuf->data[0] != '\0')
fprintf(OPF, " WITH %s", optbuf->data);
if (dump_grantors)
diff --git a/src/include/catalog/pg_auth_members.h b/src/include/catalog/pg_auth_members.h
index 3ee6ae5f6a..b145fce1ed 100644
--- a/src/include/catalog/pg_auth_members.h
+++ b/src/include/catalog/pg_auth_members.h
@@ -35,6 +35,7 @@ CATALOG(pg_auth_members,1261,AuthMemRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_
Oid grantor BKI_LOOKUP(pg_authid); /* who granted the membership */
bool admin_option; /* granted with admin option? */
bool inherit_option; /* exercise privileges without SET ROLE? */
+ bool set_option; /* use SET ROLE to the target role? */
} FormData_pg_auth_members;
/* ----------------
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 35b3d8dd88..afbfdccf53 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,11 +209,12 @@ extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId,
extern int aclmembers(const Acl *acl, Oid **roleids);
extern bool has_privs_of_role(Oid member, Oid role);
+extern bool member_can_set_role(Oid member, Oid role);
+extern void check_can_set_role(Oid member, Oid role);
extern bool is_member_of_role(Oid member, Oid role);
extern bool is_member_of_role_nosuper(Oid member, Oid role);
extern bool is_admin_of_role(Oid member, Oid role);
extern Oid select_best_admin(Oid member, Oid role);
-extern void check_is_member_of_role(Oid member, Oid role);
extern Oid get_role_oid(const char *rolname, bool missing_ok);
extern Oid get_role_oid_or_public(const char *rolname);
extern Oid get_rolespec_oid(const RoleSpec *role, bool missing_ok);
diff --git a/src/test/regress/expected/alter_generic.out b/src/test/regress/expected/alter_generic.out
index 54d3fe5764..ae54cb254f 100644
--- a/src/test/regress/expected/alter_generic.out
+++ b/src/test/regress/expected/alter_generic.out
@@ -46,7 +46,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func2; -- failed (name conflict)
ERROR: function alt_func2(integer) already exists in schema "alt_nsp1"
ALTER FUNCTION alt_func1(int) RENAME TO alt_func3; -- OK
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user3; -- OK
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp1; -- OK, already there
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2; -- OK
@@ -54,7 +54,7 @@ ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg2; -- failed (name conflict)
ERROR: function alt_agg2(integer) already exists in schema "alt_nsp1"
ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg3; -- OK
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user3; -- OK
ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -74,7 +74,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func4; -- OK
ALTER FUNCTION alt_func3(int) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of function alt_func3
ALTER FUNCTION alt_func2(int) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER FUNCTION alt_func3(int) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of function alt_func3
ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2; -- failed (name conflicts)
@@ -85,7 +85,7 @@ ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg4; -- OK
ALTER AGGREGATE alt_agg3(int) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of function alt_agg3
ALTER AGGREGATE alt_agg2(int) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER AGGREGATE alt_agg3(int) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of function alt_agg3
ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -122,7 +122,7 @@ ALTER CONVERSION alt_conv1 RENAME TO alt_conv2; -- failed (name conflict)
ERROR: conversion "alt_conv2" already exists in schema "alt_nsp1"
ALTER CONVERSION alt_conv1 RENAME TO alt_conv3; -- OK
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user3; -- OK
ALTER CONVERSION alt_conv2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -134,7 +134,7 @@ ALTER CONVERSION alt_conv1 RENAME TO alt_conv4; -- OK
ALTER CONVERSION alt_conv3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of conversion alt_conv3
ALTER CONVERSION alt_conv2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER CONVERSION alt_conv3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of conversion alt_conv3
ALTER CONVERSION alt_conv2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -196,7 +196,7 @@ ALTER LANGUAGE alt_lang1 RENAME TO alt_lang3; -- OK
ALTER LANGUAGE alt_lang2 OWNER TO regress_alter_generic_user3; -- failed (not owner)
ERROR: must be owner of language alt_lang2
ALTER LANGUAGE alt_lang3 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER LANGUAGE alt_lang3 OWNER TO regress_alter_generic_user3; -- OK
RESET SESSION AUTHORIZATION;
SELECT lanname, a.rolname
@@ -216,7 +216,7 @@ SET SESSION AUTHORIZATION regress_alter_generic_user1;
CREATE OPERATOR @-@ ( leftarg = int4, rightarg = int4, procedure = int4mi );
CREATE OPERATOR @+@ ( leftarg = int4, rightarg = int4, procedure = int4pl );
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR @-@(int4, int4) SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -224,7 +224,7 @@ CREATE OPERATOR @-@ ( leftarg = int4, rightarg = int4, procedure = int4mi );
ALTER OPERATOR @+@(int4, int4) OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator @+@
ALTER OPERATOR @-@(int4, int4) OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR @+@(int4, int4) SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator @+@
-- can't test this: the error message includes the raw oid of namespace
@@ -259,14 +259,14 @@ ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf2; -- failed (name c
ERROR: operator family "alt_opf2" for access method "hash" already exists in schema "alt_nsp1"
ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf3; -- OK
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR FAMILY alt_opf2 USING hash SET SCHEMA alt_nsp2; -- OK
ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc2; -- failed (name conflict)
ERROR: operator class "alt_opc2" for access method "hash" already exists in schema "alt_nsp1"
ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc3; -- OK
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user3; -- OK
ALTER OPERATOR CLASS alt_opc2 USING hash SET SCHEMA alt_nsp2; -- OK
RESET SESSION AUTHORIZATION;
@@ -285,7 +285,7 @@ ALTER OPERATOR FAMILY alt_opf1 USING hash RENAME TO alt_opf4; -- OK
ALTER OPERATOR FAMILY alt_opf3 USING hash OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator family alt_opf3
ALTER OPERATOR FAMILY alt_opf2 USING hash OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR FAMILY alt_opf3 USING hash SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator family alt_opf3
ALTER OPERATOR FAMILY alt_opf2 USING hash SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -296,7 +296,7 @@ ALTER OPERATOR CLASS alt_opc1 USING hash RENAME TO alt_opc4; -- OK
ALTER OPERATOR CLASS alt_opc3 USING hash OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of operator class alt_opc3
ALTER OPERATOR CLASS alt_opc2 USING hash OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER OPERATOR CLASS alt_opc3 USING hash SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of operator class alt_opc3
ALTER OPERATOR CLASS alt_opc2 USING hash SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -531,7 +531,7 @@ ALTER STATISTICS alt_stat1 RENAME TO alt_stat2; -- failed (name conflict)
ERROR: statistics object "alt_stat2" already exists in schema "alt_nsp1"
ALTER STATISTICS alt_stat1 RENAME TO alt_stat3; -- OK
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user3; -- OK
ALTER STATISTICS alt_stat2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -544,7 +544,7 @@ ALTER STATISTICS alt_stat1 RENAME TO alt_stat4; -- OK
ALTER STATISTICS alt_stat3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of statistics object alt_stat3
ALTER STATISTICS alt_stat2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER STATISTICS alt_stat3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of statistics object alt_stat3
ALTER STATISTICS alt_stat2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -573,7 +573,7 @@ ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict2; -- failed (na
ERROR: text search dictionary "alt_ts_dict2" already exists in schema "alt_nsp1"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict3; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user3; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -585,7 +585,7 @@ ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 RENAME TO alt_ts_dict4; -- OK
ALTER TEXT SEARCH DICTIONARY alt_ts_dict3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of text search dictionary alt_ts_dict3
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER TEXT SEARCH DICTIONARY alt_ts_dict3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of text search dictionary alt_ts_dict3
ALTER TEXT SEARCH DICTIONARY alt_ts_dict2 SET SCHEMA alt_nsp2; -- failed (name conflict)
@@ -614,7 +614,7 @@ ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf2; -- failed
ERROR: text search configuration "alt_ts_conf2" already exists in schema "alt_nsp1"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf3; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user2; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user2"
+ERROR: must be able to SET ROLE "regress_alter_generic_user2"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user3; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 SET SCHEMA alt_nsp2; -- OK
SET SESSION AUTHORIZATION regress_alter_generic_user2;
@@ -626,7 +626,7 @@ ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 RENAME TO alt_ts_conf4; -- OK
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf3 OWNER TO regress_alter_generic_user2; -- failed (not owner)
ERROR: must be owner of text search configuration alt_ts_conf3
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 OWNER TO regress_alter_generic_user3; -- failed (no role membership)
-ERROR: must be member of role "regress_alter_generic_user3"
+ERROR: must be able to SET ROLE "regress_alter_generic_user3"
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf3 SET SCHEMA alt_nsp2; -- failed (not owner)
ERROR: must be owner of text search configuration alt_ts_conf3
ALTER TEXT SEARCH CONFIGURATION alt_ts_conf2 SET SCHEMA alt_nsp2; -- failed (name conflict)
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 47bf56adbf..5b30ee49f3 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -442,7 +442,7 @@ ERROR: invalid option "foo"
ALTER SERVER s8 OPTIONS (connect_timeout '30', SET dbname 'db1', DROP host);
SET ROLE regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_indirect; -- ERROR
-ERROR: must be member of role "regress_test_indirect"
+ERROR: must be able to SET ROLE "regress_test_indirect"
RESET ROLE;
GRANT regress_test_indirect TO regress_test_role;
SET ROLE regress_test_role;
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index bd3453ee91..a497db94a8 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -132,6 +132,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+ERROR: permission denied to set role "pg_read_all_settings"
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+ERROR: permission denied to set role "pg_read_all_stats"
+RESET ROLE;
+RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
DROP USER regress_priv_user10;
DROP USER regress_priv_user9;
@@ -2809,3 +2818,35 @@ DROP ROLE regress_group;
DROP ROLE regress_group_direct_manager;
DROP ROLE regress_group_indirect_manager;
DROP ROLE regress_group_member;
+-- test SET and INHERIT options with object ownership changes
+CREATE ROLE regress_roleoption_protagonist;
+CREATE ROLE regress_roleoption_donor;
+CREATE ROLE regress_roleoption_recipient;
+CREATE SCHEMA regress_roleoption;
+GRANT CREATE, USAGE ON SCHEMA regress_roleoption TO PUBLIC;
+GRANT regress_roleoption_donor TO regress_roleoption_protagonist WITH INHERIT TRUE, SET FALSE;
+GRANT regress_roleoption_recipient TO regress_roleoption_protagonist WITH INHERIT FALSE, SET TRUE;
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+CREATE TABLE regress_roleoption.t1 (a int);
+CREATE TABLE regress_roleoption.t2 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_donor;
+CREATE TABLE regress_roleoption.t3 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_recipient;
+CREATE TABLE regress_roleoption.t4 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+ALTER TABLE regress_roleoption.t1 OWNER TO regress_roleoption_donor; -- fails, can't be come donor
+ERROR: must be able to SET ROLE "regress_roleoption_donor"
+ALTER TABLE regress_roleoption.t2 OWNER TO regress_roleoption_recipient; -- works
+ALTER TABLE regress_roleoption.t3 OWNER TO regress_roleoption_protagonist; -- works
+ALTER TABLE regress_roleoption.t4 OWNER TO regress_roleoption_protagonist; -- fails, we don't inherit from recipient
+ERROR: must be owner of table t4
+RESET SESSION AUTHORIZATION;
+DROP TABLE regress_roleoption.t1;
+DROP TABLE regress_roleoption.t2;
+DROP TABLE regress_roleoption.t3;
+DROP TABLE regress_roleoption.t4;
+DROP SCHEMA regress_roleoption;
+DROP ROLE regress_roleoption_recipient;
+DROP ROLE regress_roleoption_donor;
+DROP ROLE regress_roleoption_donor;
+ERROR: role "regress_roleoption_donor" does not exist
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 4ad366470d..daecf0ec64 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -114,6 +114,15 @@ SET SESSION AUTHORIZATION regress_priv_user8;
SET ROLE pg_read_all_settings;
RESET ROLE;
+RESET SESSION AUTHORIZATION;
+REVOKE SET OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+GRANT pg_read_all_stats TO regress_priv_user8 WITH SET FALSE;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings; -- fail, no SET option any more
+SET ROLE pg_read_all_stats; -- fail, granted without SET option
+RESET ROLE;
+
RESET SESSION AUTHORIZATION;
REVOKE pg_read_all_settings FROM regress_priv_user8;
@@ -1813,3 +1822,33 @@ DROP ROLE regress_group;
DROP ROLE regress_group_direct_manager;
DROP ROLE regress_group_indirect_manager;
DROP ROLE regress_group_member;
+
+-- test SET and INHERIT options with object ownership changes
+CREATE ROLE regress_roleoption_protagonist;
+CREATE ROLE regress_roleoption_donor;
+CREATE ROLE regress_roleoption_recipient;
+CREATE SCHEMA regress_roleoption;
+GRANT CREATE, USAGE ON SCHEMA regress_roleoption TO PUBLIC;
+GRANT regress_roleoption_donor TO regress_roleoption_protagonist WITH INHERIT TRUE, SET FALSE;
+GRANT regress_roleoption_recipient TO regress_roleoption_protagonist WITH INHERIT FALSE, SET TRUE;
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+CREATE TABLE regress_roleoption.t1 (a int);
+CREATE TABLE regress_roleoption.t2 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_donor;
+CREATE TABLE regress_roleoption.t3 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_recipient;
+CREATE TABLE regress_roleoption.t4 (a int);
+SET SESSION AUTHORIZATION regress_roleoption_protagonist;
+ALTER TABLE regress_roleoption.t1 OWNER TO regress_roleoption_donor; -- fails, can't be come donor
+ALTER TABLE regress_roleoption.t2 OWNER TO regress_roleoption_recipient; -- works
+ALTER TABLE regress_roleoption.t3 OWNER TO regress_roleoption_protagonist; -- works
+ALTER TABLE regress_roleoption.t4 OWNER TO regress_roleoption_protagonist; -- fails, we don't inherit from recipient
+RESET SESSION AUTHORIZATION;
+DROP TABLE regress_roleoption.t1;
+DROP TABLE regress_roleoption.t2;
+DROP TABLE regress_roleoption.t3;
+DROP TABLE regress_roleoption.t4;
+DROP SCHEMA regress_roleoption;
+DROP ROLE regress_roleoption_recipient;
+DROP ROLE regress_roleoption_donor;
+DROP ROLE regress_roleoption_donor;
--
2.24.3 (Apple Git-128)
On 2022-Nov-18, Robert Haas wrote:
On Thu, Nov 17, 2022 at 7:24 PM Jeff Davis <pgsql@j-davis.com> wrote:
But I'm fine if you'd like to move on with the SET ROLE privilege
instead, as long as we believe it grants a stable set of capabilities
(and conversely, that if the SET ROLE privilege is revoked, that it
revokes a stable set of capabilities).OK.
Here's a rebased v3 to see what cfbot thinks.
I think this hunk in dumpRoleMembership() leaves an unwanted line
behind.
/*
- * Previous versions of PostgreSQL also did not have a grant-level
+ * Previous versions of PostgreSQL also did not have grant-level options.
* INHERIT option.
*/
(I was just looking at the doc part of this patch.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
On Fri, Nov 18, 2022 at 12:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
Here's a rebased v3 to see what cfbot thinks.
cfbot is happy, so committed.
--
Robert Haas
EDB: http://www.enterprisedb.com
Op 18-11-2022 om 19:43 schreef Robert Haas:
On Fri, Nov 18, 2022 at 12:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
Here's a rebased v3 to see what cfbot thinks.
cfbot is happy, so committed.
In grant.sgml,
'actualy permisions'
looks a bit unorthodox.
On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
In grant.sgml,
'actualy permisions'
looks a bit unorthodox.
Fixed that, and the other mistake Álvaro spotted, and also bumped
catversion because I forgot that earlier.
--
Robert Haas
EDB: http://www.enterprisedb.com
Op 18-11-2022 om 22:19 schreef Robert Haas:
On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
In grant.sgml,
'actualy permisions'
looks a bit unorthodox.
Fixed that, and the other mistake Álvaro spotted, and also bumped
catversion because I forgot that earlier.
Sorry to be nagging but
'permisions' should be
'permissions'
as well.
And as I'm nagging anyway: I also wondered whether the word order could
improve:
- Word order as it stands:
However, the actual permissions conferred depend on the options
associated with the grant.
-- maybe better:
However, the permissions actually conferred depend on the options
associated with the grant.
But I'm not sure.
Thanks,
Erik
Thanks,
Erik
On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
Fixed that, and the other mistake Álvaro spotted, and also bumped
catversion because I forgot that earlier.
I was looking at this code yesterday, to see today that psql's
completion should be completed with this new clause, similary to ADMIN
and INHERIT.
--
Michael
Attachments:
pgsql-set-clause.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 13014f074f..d7f90b657a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3773,7 +3773,7 @@ psql_completion(const char *text, int start, int end)
*/
/* Complete GRANT/REVOKE with a list of roles and privileges */
else if (TailMatches("GRANT|REVOKE") ||
- TailMatches("REVOKE", "ADMIN|GRANT|INHERIT", "OPTION", "FOR"))
+ TailMatches("REVOKE", "ADMIN|GRANT|INHERIT|SET", "OPTION", "FOR"))
{
/*
* With ALTER DEFAULT PRIVILEGES, restrict completion to grantable
@@ -3791,10 +3791,11 @@ psql_completion(const char *text, int start, int end)
Privilege_options_of_grant_and_revoke,
"GRANT OPTION FOR",
"ADMIN OPTION FOR",
- "INHERIT OPTION FOR");
+ "INHERIT OPTION FOR",
+ "SET OPTION FOR");
else if (TailMatches("REVOKE", "GRANT", "OPTION", "FOR"))
COMPLETE_WITH(Privilege_options_of_grant_and_revoke);
- else if (TailMatches("REVOKE", "ADMIN|INHERIT", "OPTION", "FOR"))
+ else if (TailMatches("REVOKE", "ADMIN|INHERIT|SET", "OPTION", "FOR"))
COMPLETE_WITH_QUERY(Query_for_list_of_roles);
}
@@ -3802,10 +3803,12 @@ psql_completion(const char *text, int start, int end)
TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER"))
COMPLETE_WITH("SYSTEM");
- else if (TailMatches("GRANT|REVOKE", "SET") ||
- TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET") ||
+ else if (TailMatches("REVOKE", "SET"))
+ COMPLETE_WITH("ON PARAMETER", "OPTION FOR");
+ else if (TailMatches("GRANT", "SET") ||
TailMatches("GRANT|REVOKE", "ALTER", "SYSTEM") ||
- TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM"))
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM") ||
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET"))
COMPLETE_WITH("ON PARAMETER");
else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "PARAMETER") ||
@@ -3941,14 +3944,16 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("GRANT") && TailMatches("TO", MatchAny))
COMPLETE_WITH("WITH ADMIN",
"WITH INHERIT",
+ "WITH SET",
"WITH GRANT OPTION",
"GRANTED BY");
else if (HeadMatches("GRANT") && TailMatches("TO", MatchAny, "WITH"))
COMPLETE_WITH("ADMIN",
"INHERIT",
+ "SET",
"GRANT OPTION");
else if (HeadMatches("GRANT") &&
- (TailMatches("TO", MatchAny, "WITH", "ADMIN|INHERIT")))
+ (TailMatches("TO", MatchAny, "WITH", "ADMIN|INHERIT|SET")))
COMPLETE_WITH("OPTION", "TRUE", "FALSE");
else if (HeadMatches("GRANT") && TailMatches("TO", MatchAny, "WITH", MatchAny, "OPTION"))
COMPLETE_WITH("GRANTED BY");
On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
In grant.sgml,
'actualy permisions'
looks a bit unorthodox.
Fixed that, and the other mistake �lvaro spotted, and also bumped
catversion because I forgot that earlier.
I think Erik was trying to report that both words were misspelled. I
added to my typos to be fixed in batch if you want to wait.
On Sat, Nov 19, 2022 at 1:00 AM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
Fixed that, and the other mistake Álvaro spotted, and also bumped
catversion because I forgot that earlier.I was looking at this code yesterday, to see today that psql's
completion should be completed with this new clause, similary to ADMIN
and INHERIT.
Seems like a good idea but I'm not sure about this hunk:
TailMatches("GRANT|REVOKE", "ALTER", "SYSTEM") ||
- TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM"))
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM") ||
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET"))
That might be a correct change for other reasons, but it doesn't seem
related to this patch. The rest looks good.
--
Robert Haas
EDB: http://www.enterprisedb.com
идея и её реализация насчёт Параллельного чтения - как вам? Мне
показалось, интересно и полезно. Но это, думаю, одноразовая акция.
Времени и сил на это довольно много ухлопал, хотя вроде дело нехитрое
:) Стоило?
15.11.2022 20:07, Robert Haas пишет:
Show quoted text
Bump.
Discussion has trailed off here, but I still don't see that we have a
better way forward here than what I proposed on September 30th. Two
people have commented. Nathan said that he wasn't sure this was best
(neither am I) but that he didn't have a better idea either (neither
do I). Stephen proposed decomposing ADMIN OPTION, which is not my
preference, but even if it turns out that we want to pursue that
approach, I do not think it would make sense to bundle that into this
patch, because there isn't enough overlap between that change and this
change to justify that treatment.If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon. Otherwise, I am going to press
forward with committing this. If we do not, we will continue to have
no way of restricting of SET ROLE, and we will continue to have no way
of preventing the creation of objects owned by predefined roles by
users who have been granted those roles. As far as I am aware, no one
is opposed to those goals, and in fact I think everyone who has
commented thinks that it would be good to do something. If a better
idea than what I've implemented comes along, I'm happy to defer to it,
but I think this is one of those cases in which there probably isn't
any totally satisfying solution, and yet doing nothing is not a
superior alternative.Thanks,
On Mon, Nov 21, 2022 at 10:45:53AM -0500, Robert Haas wrote:
Seems like a good idea but I'm not sure about this hunk:
TailMatches("GRANT|REVOKE", "ALTER", "SYSTEM") || - TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM")) + TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM") || + TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET"))That might be a correct change for other reasons, but it doesn't seem
related to this patch. The rest looks good.
(Forgot to press "Send" a few days ago..)
Hmm, right, I see your point. I have just moved that to reorder the
terms alphabetically, but moving the check on REVOKE GRANT OPTION FOR
SET is not mandatory. I have moved it back in its previous
position, leading to less noise in the diffs, and applied the rest as
of 9d0cf57.
Thanks!
--
Michael
On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
But I think the bigger reason is that, in my opinion, this proposal is
more generally useful, because it takes no position on why you wish to
disallow SET ROLE. You can just disallow it in some cases and allow it in
others, and that's fine.
In this commit 3d14e17, the documentation takes the above "no position". The
implementation does not, in that WITH SET FALSE has undocumented ability to
block ALTER ... OWNER TO, not just SET ROLE. Leaving that undocumented feels
weird to me, but documenting it would take the position that WITH SET FALSE is
relevant to the security objective of preventing object creation like the
example in the original post of this thread. How do you weigh those
documentation trade-offs?
I agree that the it's more flexible in the sense that it does what it
does, and administrators can use it if it's useful for them. That means
we don't need to understand the actual goals as well; but it also means
that it's harder to determine the consequences if we tweak the behavior
(or any related behavior) later.
I have similar concerns. For the original post's security objective, the role
must also own no objects of certain types. Otherwise, WITH SET FALSE members
can use operations like CREATE OR REPLACE FUNCTION or CREATE INDEX to escalate
to full role privileges:
create user unpriv;
grant pg_maintain to unpriv with set false;
create schema maint authorization pg_maintain
create table t (c int);
create or replace function maint.f() returns int language sql as 'select 1';
alter function maint.f() owner to pg_maintain;
set session authorization unpriv;
create or replace function maint.f() returns int language sql security definer as 'select 1';
create index on maint.t(c);
On Sat, Dec 31, 2022 at 1:16 AM Noah Misch <noah@leadboat.com> wrote:
On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
But I think the bigger reason is that, in my opinion, this proposal is
more generally useful, because it takes no position on why you wish to
disallow SET ROLE. You can just disallow it in some cases and allow it in
others, and that's fine.In this commit 3d14e17, the documentation takes the above "no position". The
implementation does not, in that WITH SET FALSE has undocumented ability to
block ALTER ... OWNER TO, not just SET ROLE. Leaving that undocumented feels
weird to me, but documenting it would take the position that WITH SET FALSE is
relevant to the security objective of preventing object creation like the
example in the original post of this thread. How do you weigh those
documentation trade-offs?
In general, I favor trying to make the documentation clearer and more
complete. Intentionally leaving things undocumented doesn't seem like
the right course of action to me. That said, the pre-existing
documentation in this area is so incomplete that it's sometimes hard
to figure out where to add new information - and it made no mention of
the privileges required for ALTER .. OWNER TO. I didn't immediately
know where to add that, so did nothing. Maybe I should have tried
harder, though.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Jan 03, 2023 at 02:43:10PM -0500, Robert Haas wrote:
On Sat, Dec 31, 2022 at 1:16 AM Noah Misch <noah@leadboat.com> wrote:
On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
But I think the bigger reason is that, in my opinion, this proposal is
more generally useful, because it takes no position on why you wish to
disallow SET ROLE. You can just disallow it in some cases and allow it in
others, and that's fine.In this commit 3d14e17, the documentation takes the above "no position". The
implementation does not, in that WITH SET FALSE has undocumented ability to
block ALTER ... OWNER TO, not just SET ROLE. Leaving that undocumented feels
weird to me, but documenting it would take the position that WITH SET FALSE is
relevant to the security objective of preventing object creation like the
example in the original post of this thread. How do you weigh those
documentation trade-offs?In general, I favor trying to make the documentation clearer and more
complete. Intentionally leaving things undocumented doesn't seem like
the right course of action to me.
For what it's worth, I like to leave many things undocumented, but not this.
That said, the pre-existing
documentation in this area is so incomplete that it's sometimes hard
to figure out where to add new information - and it made no mention of
the privileges required for ALTER .. OWNER TO. I didn't immediately
know where to add that, so did nothing.
I'd start with locations where the patch already added documentation. In the
absence of documentation otherwise, a reasonable person could think WITH SET
controls just SET ROLE. The documentation of WITH SET is a good place to list
what else you opted for it to control. If the documentation can explain the
set of principles that would be used to decide whether WITH SET should govern
another thing in the future, that would provide extra value.
On Tue, Jan 3, 2023 at 5:03 PM Noah Misch <noah@leadboat.com> wrote:
I'd start with locations where the patch already added documentation. In the
absence of documentation otherwise, a reasonable person could think WITH SET
controls just SET ROLE. The documentation of WITH SET is a good place to list
what else you opted for it to control. If the documentation can explain the
set of principles that would be used to decide whether WITH SET should govern
another thing in the future, that would provide extra value.
From the point of view of the code, we currently have four different
functions that make inquiries about role membership:
has_privs_of_role, is_member_of_role, is_member_of_role_nosuper, and
member_can_set_role.
I spent a while looking at how has_privs_of_role() is used. Basically,
there are three main patterns. First, in some places, you must have
the privileges of a certain role (typically, either a predefined role
or the role that owns some object) or the operation will fail with an
error indicating that you don't have sufficient permissions. Second,
there are places where having the privileges of a certain role exempts
you from some other permissions check; if you have neither, you'll get
an error. An example is that having the permissions of
pg_read_all_data substitutes for a select privilege. And third, there
are cases where you definitely won't get an error, but the behavior
will vary depending on whether you have the privileges of some role.
For instance, you can see more data in pg_stat_replication,
pg_stat_wal_receiver, and other stats views if you have
pg_read_all_stats. The GUC values reported in EXPLAIN output will
exclude superuser-only values unless you have pg_read_all_settings. It
looks like some maintenance commands like CLUSTER and VACUUM
completely skip over, or just warn about, cases where permission is
lacking. And weirdest of all, having the privileges of a role means
that the RLS policies applied to that role also apply to you. That's
odd because it makes permissions not strictly additive.
member_can_set_role() controls (a) whether you can SET ROLE to some
other role, (b) whether you can alter the owner of an existing object
to that role, and (c) whether you can create an object owned by some
other user in cases where the CREATE command has an option for that,
like CREATE DATABASE ... OWNER.
is_member_of_role_nosuper() is used to prevent creation of role
membership loops, and for pg_hba.conf matching.
The only remaining call to is_member_of_role() is in
pg_role_aclcheck(), which just supports the SQL-callable
pg_has_role(). has_privs_of_role() and member_can_set_role() are used
here, too.
How much of this should we document, do you think? If we're going to
go into the details, I sort of feel like it would be good to somehow
contrast what is attached to membership with what is attached to the
INHERIT option or the SET option. I think it would be slightly
surprising not to mention the way that RLS rules are triggered by
privilege inheritance yet include the fact that the SET option affects
ALTER ... OWNER TO, but maybe I've got the wrong idea. An exhaustive
concordance of what depends on what might be too much, or maybe it
isn't, but it's probably good if the level of detail is pretty
uniform.
Your thoughts?
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Jan 04, 2023 at 03:56:34PM -0500, Robert Haas wrote:
On Tue, Jan 3, 2023 at 5:03 PM Noah Misch <noah@leadboat.com> wrote:
I'd start with locations where the patch already added documentation. In the
absence of documentation otherwise, a reasonable person could think WITH SET
controls just SET ROLE. The documentation of WITH SET is a good place to list
what else you opted for it to control. If the documentation can explain the
set of principles that would be used to decide whether WITH SET should govern
another thing in the future, that would provide extra value.From the point of view of the code, we currently have four different
functions that make inquiries about role membership:
has_privs_of_role, is_member_of_role, is_member_of_role_nosuper, and
member_can_set_role.I spent a while looking at how has_privs_of_role() is used. Basically,
there are three main patterns. First, in some places, you must have
the privileges of a certain role (typically, either a predefined role
or the role that owns some object) or the operation will fail with an
error indicating that you don't have sufficient permissions. Second,
there are places where having the privileges of a certain role exempts
you from some other permissions check; if you have neither, you'll get
an error. An example is that having the permissions of
pg_read_all_data substitutes for a select privilege. And third, there
are cases where you definitely won't get an error, but the behavior
will vary depending on whether you have the privileges of some role.
For instance, you can see more data in pg_stat_replication,
pg_stat_wal_receiver, and other stats views if you have
pg_read_all_stats. The GUC values reported in EXPLAIN output will
exclude superuser-only values unless you have pg_read_all_settings. It
looks like some maintenance commands like CLUSTER and VACUUM
completely skip over, or just warn about, cases where permission is
lacking. And weirdest of all, having the privileges of a role means
that the RLS policies applied to that role also apply to you. That's
odd because it makes permissions not strictly additive.member_can_set_role() controls (a) whether you can SET ROLE to some
other role, (b) whether you can alter the owner of an existing object
to that role, and (c) whether you can create an object owned by some
other user in cases where the CREATE command has an option for that,
like CREATE DATABASE ... OWNER.is_member_of_role_nosuper() is used to prevent creation of role
membership loops, and for pg_hba.conf matching.The only remaining call to is_member_of_role() is in
pg_role_aclcheck(), which just supports the SQL-callable
pg_has_role(). has_privs_of_role() and member_can_set_role() are used
here, too.How much of this should we document, do you think?
Rough thoughts:
Do document:
- For pg_read_all_stats, something like s/Read all pg_stat_/See all rows of all pg_stat_/
- At CREATE POLICY and/or similar places, explain the semantics used to judge
the applicability of role_name to a given query.
Don't document:
- Mechanism for preventing membership loops.
Already documented adequately:
- "First, in some places, you must have the privileges of a certain role" is
documented through language like "You must own the table".
- pg_read_all_data
- EXPLAIN. I'm not seeing any setting that's both GUC_SUPERUSER_ONLY and
GUC_EXPLAIN.
- SQL-level pg_has_role().
Unsure:
- At INHERIT, cover the not-strictly-additive RLS consequences.
If we're going to
go into the details, I sort of feel like it would be good to somehow
contrast what is attached to membership with what is attached to the
INHERIT option or the SET option.
Works for me.
I think it would be slightly
surprising not to mention the way that RLS rules are triggered by
privilege inheritance yet include the fact that the SET option affects
ALTER ... OWNER TO, but maybe I've got the wrong idea.
The CREATE POLICY syntax and docs show the role_name parameter, though they
don't detail how exactly the server determines whether a given role applies at
a given moment. The docs are silent on the SET / OWNER TO connection. Hence,
I think the doc gap around SET / OWNER TO is more acute than the doc gap
around this RLS behavior.
Thanks,
nm
On Fri, 2022-12-30 at 22:16 -0800, Noah Misch wrote:
create user unpriv;
grant pg_maintain to unpriv with set false;
create schema maint authorization pg_maintain
create table t (c int);
create or replace function maint.f() returns int language sql as
'select 1';
alter function maint.f() owner to pg_maintain;
set session authorization unpriv;
create or replace function maint.f() returns int language sql
security definer as 'select 1';
create index on maint.t(c);
I dug into this case, as well as some mirror-image risks associated
with SECURITY INVOKER. This goes on a bit of a tangent and I'm sure I'm
retreading what others already know.
The risks of SECURITY DEFINER are about ownership: by owning something
with code attached, you're responsible to make sure the code is safe
and can only be run by the right users. Additionally, there are a
number of ways someone might come to own some code other than by
defining it themselves. Robert addressed the SET ROLE, CREATE ... OWNER
and the OWNER TO paths; but that still leaves the replace-function path
and the create index paths that you illustrated. As I said earlier I'm
not 100% satisfied with SET ROLE as a privilege; but I'm more
comfortable that it has a defined scope: the SET ROLE privilege should
control paths that can "gift" code to that user.
The risks of SECURITY INVOKER are more serious. It inherently means
that one user is writing code, and another is executing it. And in the
SQL world of triggers, views, expression indexes and logical
replication, the invoker often doesn't know what they are invoking.
There are search path risks, risks associated with resolving the right
function/operator/cast, risks of concurrent DDL (i.e. changing a
function definition right before a superuser executes it), etc. It
severely limits the kinds of trust models you can use in logical
replication. And SECURITY INVOKER weirdly inverts the trust
relationship of a GRANT: if A grants to B, then B must *completely*
trust A in order to exercise that new privilege because A can inject
arbitrary SECURITY INVOKER code in front of the object.
UNIX basically operates on a SECURITY INVOKER model, so I guess that
means that it can work. But then again, grepping a file doesn't execute
arbitrary code from inside that file (though there are bugs
sometimes... see [1]https://lcamtuf.blogspot.com/2014/10/psa-dont-run-strings-on-untrusted-files.html). It just seems like the wrong model for SQL.
[ Aside: that probably explains why the SQL spec defaults to SECURITY
DEFINER. ]
Brainstorming, I think we can do more to mitigate the risks of SECURITY
INVOKER:
* If running a command that would invoke a SECURITY INVOKER function
that is not owned by superuser or a member of the invoker's role, throw
an error instead. We could control this with a GUC for compatibility.
* Have SECURITY PUBLIC which executes with minimal privileges, which
would be good for convenience functions that might be used in an index
expression or view.
* Another idea is to separate out read privileges -- a SECURITY INVOKER
that is read-only is sounds less dangerous (though not without some
risk).
* Prevent extension scripts from running SECURITY INVOKER functions.
[1]: https://lcamtuf.blogspot.com/2014/10/psa-dont-run-strings-on-untrusted-files.html
https://lcamtuf.blogspot.com/2014/10/psa-dont-run-strings-on-untrusted-files.html
--
Jeff Davis
PostgreSQL Contributor Team - AWS
On Sat, Jan 7, 2023 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
The docs are silent on the SET / OWNER TO connection. Hence,
Reviewing the documentation again today, I realized that the
documentation describes the rules for changing the ownership of an
object in a whole bunch of places which this patch failed to update.
Here's a patch to update all of the places I found.
I suspect that these changes will mean that we don't also need to
adjust the discussion of the SET option itself, but let me know what
you think.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Update-ALTER-OWNER-documentation-for-GRANT-.-WITH.patchapplication/octet-stream; name=v1-0001-Update-ALTER-OWNER-documentation-for-GRANT-.-WITH.patchDownload
From 034ced75c426cf627e1d61557a9485317e44f389 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Tue, 10 Jan 2023 11:00:54 -0500
Subject: [PATCH v1] Update ALTER OWNER documentation for GRANT ... WITH SET
OPTION.
---
doc/src/sgml/ddl.sgml | 4 ++--
doc/src/sgml/ref/alter_aggregate.sgml | 7 ++++---
doc/src/sgml/ref/alter_conversion.sgml | 7 ++++---
doc/src/sgml/ref/alter_database.sgml | 4 ++--
doc/src/sgml/ref/alter_domain.sgml | 6 +++---
doc/src/sgml/ref/alter_foreign_table.sgml | 6 +++---
doc/src/sgml/ref/alter_function.sgml | 6 +++---
doc/src/sgml/ref/alter_large_object.sgml | 5 +++--
doc/src/sgml/ref/alter_materialized_view.sgml | 7 ++++---
doc/src/sgml/ref/alter_opclass.sgml | 7 ++++---
doc/src/sgml/ref/alter_operator.sgml | 7 ++++---
doc/src/sgml/ref/alter_procedure.sgml | 7 ++++---
doc/src/sgml/ref/alter_publication.sgml | 10 ++++++----
doc/src/sgml/ref/alter_schema.sgml | 4 ++--
doc/src/sgml/ref/alter_sequence.sgml | 7 ++++---
doc/src/sgml/ref/alter_server.sgml | 4 ++--
doc/src/sgml/ref/alter_statistics.sgml | 7 ++++---
doc/src/sgml/ref/alter_subscription.sgml | 4 ++--
doc/src/sgml/ref/alter_table.sgml | 7 ++++---
doc/src/sgml/ref/alter_tablespace.sgml | 4 ++--
doc/src/sgml/ref/alter_type.sgml | 7 ++++---
doc/src/sgml/ref/alter_view.sgml | 7 ++++---
22 files changed, 74 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index d91a781479..22e43a4977 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1714,8 +1714,8 @@ ALTER TABLE products RENAME TO items;
ALTER TABLE <replaceable>table_name</replaceable> OWNER TO <replaceable>new_owner</replaceable>;
</programlisting>
Superusers can always do this; ordinary roles can only do it if they are
- both the current owner of the object (or a member of the owning role) and
- a member of the new owning role.
+ both the current owner of the object (or inherit the privileges of the
+ owning role) and able to <literal>SET ROLE</literal> to the new owning role.
</para>
<para>
diff --git a/doc/src/sgml/ref/alter_aggregate.sgml b/doc/src/sgml/ref/alter_aggregate.sgml
index aee10a5ca2..d0a39ba7b5 100644
--- a/doc/src/sgml/ref/alter_aggregate.sgml
+++ b/doc/src/sgml/ref/alter_aggregate.sgml
@@ -46,9 +46,10 @@ ALTER AGGREGATE <replaceable>name</replaceable> ( <replaceable>aggregate_signatu
You must own the aggregate function to use <command>ALTER AGGREGATE</command>.
To change the schema of an aggregate function, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the aggregate function's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the aggregate function's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating
the aggregate function. However, a superuser can alter ownership of any
aggregate function anyway.)
diff --git a/doc/src/sgml/ref/alter_conversion.sgml b/doc/src/sgml/ref/alter_conversion.sgml
index a128f20f3e..5c7cc978ee 100644
--- a/doc/src/sgml/ref/alter_conversion.sgml
+++ b/doc/src/sgml/ref/alter_conversion.sgml
@@ -37,9 +37,10 @@ ALTER CONVERSION <replaceable>name</replaceable> SET SCHEMA <replaceable>new_sch
<para>
You must own the conversion to use <command>ALTER CONVERSION</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the conversion's schema. (These restrictions enforce that altering the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the conversion's schema.
+ (These restrictions enforce that altering the
owner doesn't do anything you couldn't do by dropping and recreating the
conversion. However, a superuser can alter ownership of any conversion
anyway.)
diff --git a/doc/src/sgml/ref/alter_database.sgml b/doc/src/sgml/ref/alter_database.sgml
index 181e9d3620..83a369e511 100644
--- a/doc/src/sgml/ref/alter_database.sgml
+++ b/doc/src/sgml/ref/alter_database.sgml
@@ -68,8 +68,8 @@ ALTER DATABASE <replaceable class="parameter">name</replaceable> RESET ALL
<para>
The third form changes the owner of the database.
- To alter the owner, you must own the database and also be a direct or
- indirect member of the new owning role, and you must have the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and you must have the
<literal>CREATEDB</literal> privilege.
(Note that superusers have all these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_domain.sgml b/doc/src/sgml/ref/alter_domain.sgml
index 2db5372513..f6704d7557 100644
--- a/doc/src/sgml/ref/alter_domain.sgml
+++ b/doc/src/sgml/ref/alter_domain.sgml
@@ -157,9 +157,9 @@ ALTER DOMAIN <replaceable class="parameter">name</replaceable>
You must own the domain to use <command>ALTER DOMAIN</command>.
To change the schema of a domain, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the domain's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal> privilege
+ on the domain's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the domain.
However, a superuser can alter ownership of any domain anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_foreign_table.sgml b/doc/src/sgml/ref/alter_foreign_table.sgml
index d056dc1bb1..0f4191713a 100644
--- a/doc/src/sgml/ref/alter_foreign_table.sgml
+++ b/doc/src/sgml/ref/alter_foreign_table.sgml
@@ -320,9 +320,9 @@ ALTER FOREIGN TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceab
You must own the table to use <command>ALTER FOREIGN TABLE</command>.
To change the schema of a foreign table, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the table's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal> privilege
+ on the table's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the table.
However, a superuser can alter ownership of any table anyway.)
To add a column or alter a column type, you must also
diff --git a/doc/src/sgml/ref/alter_function.sgml b/doc/src/sgml/ref/alter_function.sgml
index 2e8e1162d8..8193b17f25 100644
--- a/doc/src/sgml/ref/alter_function.sgml
+++ b/doc/src/sgml/ref/alter_function.sgml
@@ -60,9 +60,9 @@ ALTER FUNCTION <replaceable>name</replaceable> [ ( [ [ <replaceable class="param
<para>
You must own the function to use <command>ALTER FUNCTION</command>.
To change a function's schema, you must also have <literal>CREATE</literal>
- privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
+ privilege on the new schema. To alter the owner, you must be able to
+ <literal>SET ROLE</literal> to the new owning role, and that role must
+ have <literal>CREATE</literal> privilege on
the function's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the function.
However, a superuser can alter ownership of any function anyway.)
diff --git a/doc/src/sgml/ref/alter_large_object.sgml b/doc/src/sgml/ref/alter_large_object.sgml
index 17ea1491ba..f427006f80 100644
--- a/doc/src/sgml/ref/alter_large_object.sgml
+++ b/doc/src/sgml/ref/alter_large_object.sgml
@@ -35,8 +35,9 @@ ALTER LARGE OBJECT <replaceable class="parameter">large_object_oid</replaceable>
<para>
You must own the large object to use <command>ALTER LARGE OBJECT</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role. (However, a superuser can alter any large object anyway.)
+ To alter the owner, you must also be able to <literal>SET ROLE</literal> to
+ the new owning role.
+ (However, a superuser can alter any large object anyway.)
Currently, the only functionality is to assign a new owner, so both
restrictions always apply.
</para>
diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml
index 040ae53f98..da7ed04597 100644
--- a/doc/src/sgml/ref/alter_materialized_view.sgml
+++ b/doc/src/sgml/ref/alter_materialized_view.sgml
@@ -63,9 +63,10 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE <replaceable class="parameter">name</r
You must own the materialized view to use <command>ALTER MATERIALIZED
VIEW</command>. To change a materialized view's schema, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the materialized view's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the materialized view's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating the
materialized view. However, a superuser can alter ownership of any view
anyway.)
diff --git a/doc/src/sgml/ref/alter_opclass.sgml b/doc/src/sgml/ref/alter_opclass.sgml
index b1db459b11..231597d629 100644
--- a/doc/src/sgml/ref/alter_opclass.sgml
+++ b/doc/src/sgml/ref/alter_opclass.sgml
@@ -42,9 +42,10 @@ ALTER OPERATOR CLASS <replaceable>name</replaceable> USING <replaceable class="p
<para>
You must own the operator class to use <command>ALTER OPERATOR CLASS</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the operator class's schema. (These restrictions enforce that altering the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the operator class's schema.
+ (These restrictions enforce that altering the
owner doesn't do anything you couldn't do by dropping and recreating the
operator class. However, a superuser can alter ownership of any operator
class anyway.)
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index ad90c137f1..a4a1af564f 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -44,9 +44,10 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
<para>
You must own the operator to use <command>ALTER OPERATOR</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the operator's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the operator's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the operator.
However, a superuser can alter ownership of any operator anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_procedure.sgml b/doc/src/sgml/ref/alter_procedure.sgml
index 20a623885f..a4737a3439 100644
--- a/doc/src/sgml/ref/alter_procedure.sgml
+++ b/doc/src/sgml/ref/alter_procedure.sgml
@@ -54,9 +54,10 @@ ALTER PROCEDURE <replaceable>name</replaceable> [ ( [ [ <replaceable class="para
You must own the procedure to use <command>ALTER PROCEDURE</command>.
To change a procedure's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the procedure's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the procedure's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the procedure.
However, a superuser can alter ownership of any procedure anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index c84b11f47a..cd20868bca 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -75,10 +75,12 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
Adding a table to a publication additionally requires owning that table.
The <literal>ADD TABLES IN SCHEMA</literal> and
<literal>SET TABLES IN SCHEMA</literal> to a publication requires the
- invoking user to be a superuser. To alter the owner, you must also be a
- direct or indirect member of the new owning role. The new owner must have
- <literal>CREATE</literal> privilege on the database. Also, the new owner
- of a <literal>FOR ALL TABLES</literal> or <literal>FOR TABLES IN SCHEMA</literal>
+ invoking user to be a superuser.
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the database.
+ Also, the new owner of a <literal>FOR ALL TABLES</literal> or
+ <literal>FOR TABLES IN SCHEMA</literal>
publication must be a superuser. However, a superuser can
change the ownership of a publication regardless of these restrictions.
</para>
diff --git a/doc/src/sgml/ref/alter_schema.sgml b/doc/src/sgml/ref/alter_schema.sgml
index 04624c5a5e..b8ace0561f 100644
--- a/doc/src/sgml/ref/alter_schema.sgml
+++ b/doc/src/sgml/ref/alter_schema.sgml
@@ -37,8 +37,8 @@ ALTER SCHEMA <replaceable>name</replaceable> OWNER TO { <replaceable>new_owner</
You must own the schema to use <command>ALTER SCHEMA</command>.
To rename a schema you must also have the
<literal>CREATE</literal> privilege for the database.
- To alter the owner, you must also be a direct or
- indirect member of the new owning role, and you must have the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have the
<literal>CREATE</literal> privilege for the database.
(Note that superusers have all these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml
index 148085d4f2..7be36cf466 100644
--- a/doc/src/sgml/ref/alter_sequence.sgml
+++ b/doc/src/sgml/ref/alter_sequence.sgml
@@ -51,9 +51,10 @@ ALTER SEQUENCE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> S
You must own the sequence to use <command>ALTER SEQUENCE</command>.
To change a sequence's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the sequence's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the sequence's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the sequence.
However, a superuser can alter ownership of any sequence anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_server.sgml b/doc/src/sgml/ref/alter_server.sgml
index 186f38b5f8..467bf85589 100644
--- a/doc/src/sgml/ref/alter_server.sgml
+++ b/doc/src/sgml/ref/alter_server.sgml
@@ -40,8 +40,8 @@ ALTER SERVER <replaceable class="parameter">name</replaceable> RENAME TO <replac
<para>
To alter the server you must be the owner of the server.
- Additionally to alter the owner, you must own the server and also
- be a direct or indirect member of the new owning role, and you must
+ Additionally to alter the owner, you must be able to
+ <literal>SET ROLE</literal> to the new owning role, and you must
have <literal>USAGE</literal> privilege on the server's foreign-data
wrapper. (Note that superusers satisfy all these criteria
automatically.)
diff --git a/doc/src/sgml/ref/alter_statistics.sgml b/doc/src/sgml/ref/alter_statistics.sgml
index ce6cdf2bb1..73cc9e830d 100644
--- a/doc/src/sgml/ref/alter_statistics.sgml
+++ b/doc/src/sgml/ref/alter_statistics.sgml
@@ -43,9 +43,10 @@ ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTIC
You must own the statistics object to use <command>ALTER STATISTICS</command>.
To change a statistics object's schema, you must also
have <literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the statistics object's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the statistics object's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating
the statistics object. However, a superuser can alter ownership of any
statistics object anyway.)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 1e8d72062b..ad93553a1d 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -46,8 +46,8 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
You must own the subscription to use <command>ALTER SUBSCRIPTION</command>.
- To alter the owner, you must also be a direct or indirect member of the
- new owning role. The new owner has to be a superuser.
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role. The new owner has to be a superuser.
(Currently, all subscription owners must be superusers, so the owner checks
will be bypassed in practice. But this might change in the future.)
</para>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 794e886f96..9aaa32a782 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1106,9 +1106,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
To add the table as a new child of a parent table, you must own the parent
table as well. Also, to attach a table as a new partition of the table,
you must own the table being attached.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the table's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the table's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the table.
However, a superuser can alter ownership of any table anyway.)
To add a column or alter a column type or use the <literal>OF</literal>
diff --git a/doc/src/sgml/ref/alter_tablespace.sgml b/doc/src/sgml/ref/alter_tablespace.sgml
index 6de80746d5..6ec863400d 100644
--- a/doc/src/sgml/ref/alter_tablespace.sgml
+++ b/doc/src/sgml/ref/alter_tablespace.sgml
@@ -38,8 +38,8 @@ ALTER TABLESPACE <replaceable>name</replaceable> RESET ( <replaceable class="par
<para>
You must own the tablespace to change the definition of a tablespace.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role.
+ To alter the owner, you must also be able to <literal>SET ROLE</literal>
+ to the new owning role.
(Note that superusers have these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 146065144f..025a3ee48f 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -246,9 +246,10 @@ ALTER TYPE <replaceable class="parameter">name</replaceable> SET ( <replaceable
You must own the type to use <command>ALTER TYPE</command>.
To change the schema of a type, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the type's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the type's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the type.
However, a superuser can alter ownership of any type anyway.)
To add an attribute or alter an attribute type, you must also
diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml
index 8bdc90a5a1..afbb3d02c7 100644
--- a/doc/src/sgml/ref/alter_view.sgml
+++ b/doc/src/sgml/ref/alter_view.sgml
@@ -45,9 +45,10 @@ ALTER VIEW [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RESET
You must own the view to use <command>ALTER VIEW</command>.
To change a view's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the view's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the view's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the view.
However, a superuser can alter ownership of any view anyway.)
</para>
--
2.37.1 (Apple Git-137.1)
On Tue, Jan 10, 2023 at 2:28 AM Jeff Davis <pgsql@j-davis.com> wrote:
The risks of SECURITY INVOKER are more serious. It inherently means
that one user is writing code, and another is executing it. And in the
SQL world of triggers, views, expression indexes and logical
replication, the invoker often doesn't know what they are invoking.
There are search path risks, risks associated with resolving the right
function/operator/cast, risks of concurrent DDL (i.e. changing a
function definition right before a superuser executes it), etc. It
severely limits the kinds of trust models you can use in logical
replication. And SECURITY INVOKER weirdly inverts the trust
relationship of a GRANT: if A grants to B, then B must *completely*
trust A in order to exercise that new privilege because A can inject
arbitrary SECURITY INVOKER code in front of the object.
Yes. I think it's extremely difficult to operate a PostgreSQL database
with mutually untrusting users. If the high-privilege users don't
trust the regular users, they must also make very little use of the
database and only in carefully circumscribed ways. If not, the whole
security model unravels really fast. It would certainly be nice to do
better here.
UNIX basically operates on a SECURITY INVOKER model, so I guess that
means that it can work. But then again, grepping a file doesn't execute
arbitrary code from inside that file (though there are bugs
sometimes... see [1]). It just seems like the wrong model for SQL.
I often think about the UNIX model to better understand the problems
we have in PostgreSQL. I don't think that there's any real theoretical
difference between the cases, but there are practical differences
nearly all of which are unfavorable to PostgreSQL. For example, when
you log into your UNIX account, you have a home directory which is
pre-created. Your path is likely configured to contain only root-owned
directories and perhaps directories within your home directory that
are controlled by you, and the permissions on the root-owned
directories are locked down tight. That's because people figured out
in the 1970s and 1980s that if other people could write executable
code into a path you were likely to search, your account was probably
going to get compromised.
Now, in PostgreSQL, the equivalent of a home directory is a user
schema. We set things up to search those by default if they exist, but
we don't create them by default. We also put the public schema in the
default search path and, up until very recently, it was writeable by
default. In practice, many users probably put back write permission on
that schema, partly because if they don't, unprivileged users can't
create database objects anywhere at all. The practical effect of this
is that, when you log into a UNIX system, you're strongly encouraged
to access only things that are owned by you or root, and any new stuff
you create will be in a location where nobody but you is likely to
touch it. On the other hand, when you log into a PostgreSQL system,
you're set up by default to access objects created by other
unprivileged users and you may have nowhere to put your own objects
where those users won't also be accessing your stuff.
So the risks, which in theory are all very similar, are in practice
far greater in the PostgreSQL context, basically because our default
setup is about 40 years behind the times in terms of implementing best
practices. At least we've locked down write permission on pg_catalog.
I think some early UNIX systems didn't even do that, or not well. But
that's about the end of the good things that I have to say about what
we're doing in this area.
To be fair, I think many security people also consider it wise to
assume that a local unprivileged UNIX user can probably find a way to
escalate to root. There are a lot of setuid binaries on a
normally-configured UNIX system, and you only need to find one of them
that has an exploitable vulnerability. Those are the equivalent of
SECURITY DEFINER privileges, and I don't think we ship any of those in
a default configuration. In that regard, we're perhaps better-secured
than UNIX. Unfortunately, I think it is probably still wise to assume
that an unprivileged PostgreSQL user can find some way of getting
superuser if they want -- not only because of Trojan horse attacks
based on leaving security-invoker functions or procedures or operators
lying around, but also because I strongly suspect there are more
escalate-to-superuser bugs in the code than we've found yet. Those
we've not found, or have found but have not fixed, may still be known
to bad actors.
[ Aside: that probably explains why the SQL spec defaults to SECURITY
DEFINER. ]
I doubt that SECURITY DEFINER is safer in general than SECURITY
INVOKER. That'd be the equivalent of having binaries installed setuid
by default, which would be insane. I think it is right to regard
SECURITY DEFINER as the bigger threat by far. The reason it doesn't
always seem that way with PostgreSQL, at least in my view, is because
we make it so atrociously easy to accidentally invoke executable code
somewhere. If you start by assuming that you're probably going to
execute some random other user's code by accident, well then in that
world yes you would prefer to at least have it be running as them, not
you. But that's not really safe anyway. Sure, if the code runs as
them, they can't so easily usurp your privileges, but they can still
log everything you do, or make it fail, or make it take forever. Those
things are less serious than outright account takeover, but nobody
stands up a web site and hopes that it only gets DDOS'd rather than
vandalized. What you want is for it to stay up.
Brainstorming, I think we can do more to mitigate the risks of SECURITY
INVOKER:* If running a command that would invoke a SECURITY INVOKER function
that is not owned by superuser or a member of the invoker's role, throw
an error instead. We could control this with a GUC for compatibility.* Have SECURITY PUBLIC which executes with minimal privileges, which
would be good for convenience functions that might be used in an index
expression or view.* Another idea is to separate out read privileges -- a SECURITY INVOKER
that is read-only is sounds less dangerous (though not without some
risk).* Prevent extension scripts from running SECURITY INVOKER functions.
It might be best to repost some of these ideas on a new thread with a
relevant subject line, but I agree that there's some potential here.
Your first idea reminds me a lot of the proposal Tom made in
/messages/by-id/19327.1533748538@sss.pgh.pa.us
-- except that his mechanism is more general, since you can say whose
code you trust and whose code you don't trust. Noah had a competing
version of that patch, too. But we never settled on an approach. I
still think something like this would be a good idea, and the fact
that you've apparently-independently come up with a similar notion
just reinforces that.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, 2023-01-10 at 11:45 -0500, Robert Haas wrote:
So the risks, which in theory are all very similar, are in practice
far greater in the PostgreSQL context, basically because our default
setup is about 40 years behind the times in terms of implementing
best
practices.
I agree that huge improvements could be made with improvements to best
practices/defaults.
But there are some differences that are harder to fix that way. In
postgres, one can attach arbitrary code to pretty much anything, so you
need to trust everything you touch. There is no safe postgres
equivalent to grepping an untrusted file.
It might be best to repost some of these ideas on a new thread with a
relevant subject line, but I agree that there's some potential here.
Your first idea reminds me a lot of the proposal Tom made in
/messages/by-id/19327.1533748538@sss.pgh.pa.us
-- except that his mechanism is more general, since you can say whose
code you trust and whose code you don't trust. Noah had a competing
version of that patch, too. But we never settled on an approach. I
still think something like this would be a good idea, and the fact
that you've apparently-independently come up with a similar notion
just reinforces that.
Will do, thank you for the reference.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
On Tue, Jan 10, 2023 at 11:06:52AM -0500, Robert Haas wrote:
On Sat, Jan 7, 2023 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
The docs are silent on the SET / OWNER TO connection. Hence,
Reviewing the documentation again today, I realized that the
documentation describes the rules for changing the ownership of an
object in a whole bunch of places which this patch failed to update.
Here's a patch to update all of the places I found.
A "git grep 'direct or indirect mem'" found a few more:
doc/src/sgml/ref/alter_collation.sgml:42: To alter the owner, you must also be a direct or indirect member of the new
doc/src/sgml/ref/create_database.sgml:92: role, you must be a direct or indirect member of that role,
doc/src/sgml/ref/create_schema.sgml:92: owned by another role, you must be a direct or indirect member of
I wondered if the new recurring phrase "must be able to SET ROLE" should be
more specific, e.g. one of "must have
{permission,authorization,authority,right} to SET ROLE". But then I stopped
wondering and figured "be able to" is sufficient.
I suspect that these changes will mean that we don't also need to
adjust the discussion of the SET option itself, but let me know what
you think.
I still think docs for the SET option itself should give a sense of the
diversity of things it's intended to control. It could be simple. A bunch of
the sites you're modifying are near text like "These restrictions enforce that
altering the owner doesn't do anything you couldn't do by dropping and
recreating the aggregate function." Perhaps the main SET doc could say
something about how it restricts other things that would yield equivalent
outcomes. (Incidentally, DROP is another case of something one likely doesn't
want the WITH SET FALSE member using. I think that reinforces a point I wrote
upthread. To achieve the original post's security objective, the role must
own no objects whatsoever.)
On Wed, Jan 11, 2023 at 10:16 AM Noah Misch <noah@leadboat.com> wrote:
A "git grep 'direct or indirect mem'" found a few more:
doc/src/sgml/ref/alter_collation.sgml:42: To alter the owner, you must also be a direct or indirect member of the new
doc/src/sgml/ref/create_database.sgml:92: role, you must be a direct or indirect member of that role,
doc/src/sgml/ref/create_schema.sgml:92: owned by another role, you must be a direct or indirect member of
Ah, thanks.
I wondered if the new recurring phrase "must be able to SET ROLE" should be
more specific, e.g. one of "must have
{permission,authorization,authority,right} to SET ROLE". But then I stopped
wondering and figured "be able to" is sufficient.
I think so, too. Note the wording of the error message in check_can_set_role().
I still think docs for the SET option itself should give a sense of the
diversity of things it's intended to control. It could be simple. A bunch of
the sites you're modifying are near text like "These restrictions enforce that
altering the owner doesn't do anything you couldn't do by dropping and
recreating the aggregate function." Perhaps the main SET doc could say
something about how it restricts other things that would yield equivalent
outcomes. (Incidentally, DROP is another case of something one likely doesn't
want the WITH SET FALSE member using. I think that reinforces a point I wrote
upthread. To achieve the original post's security objective, the role must
own no objects whatsoever.)
I spent a while on this. The attached is as well I was able to figure
out how to do. What do you think?
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-More-documentation-update-for-GRANT-.-WITH-SET-OP.patchapplication/octet-stream; name=v2-0001-More-documentation-update-for-GRANT-.-WITH-SET-OP.patchDownload
From b9d40a6cd6e198e635d177a6e47db5c62cbccdc9 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 11 Jan 2023 14:57:30 -0500
Subject: [PATCH v2] More documentation update for GRANT ... WITH SET OPTION.
Update the reference pages for various ALTER commands that
mentioned that you must be a member of role that will be the
new owner to instead say that you must be able to SET ROLE
to the new owner. Update ddl.sgml's generate statement on this
topic along similar lines.
Likewise, update CREATE SCHEMA and CREATE DATABASE, which
have options to specify who will own the new objects, to say
that you must be able to SET ROLE to the role that will own
them.
Finally, update the documentation for the GRANT statement
itself with some general principles about how the SET option
works and how it can be used.
---
doc/src/sgml/ddl.sgml | 4 ++--
doc/src/sgml/ref/alter_aggregate.sgml | 7 ++++---
doc/src/sgml/ref/alter_collation.sgml | 7 ++++---
doc/src/sgml/ref/alter_conversion.sgml | 7 ++++---
doc/src/sgml/ref/alter_database.sgml | 4 ++--
doc/src/sgml/ref/alter_domain.sgml | 6 +++---
doc/src/sgml/ref/alter_foreign_table.sgml | 6 +++---
doc/src/sgml/ref/alter_function.sgml | 6 +++---
doc/src/sgml/ref/alter_large_object.sgml | 5 +++--
doc/src/sgml/ref/alter_materialized_view.sgml | 7 ++++---
doc/src/sgml/ref/alter_opclass.sgml | 7 ++++---
doc/src/sgml/ref/alter_operator.sgml | 7 ++++---
doc/src/sgml/ref/alter_procedure.sgml | 7 ++++---
doc/src/sgml/ref/alter_publication.sgml | 10 ++++++----
doc/src/sgml/ref/alter_schema.sgml | 4 ++--
doc/src/sgml/ref/alter_sequence.sgml | 7 ++++---
doc/src/sgml/ref/alter_server.sgml | 4 ++--
doc/src/sgml/ref/alter_statistics.sgml | 7 ++++---
doc/src/sgml/ref/alter_subscription.sgml | 4 ++--
doc/src/sgml/ref/alter_table.sgml | 7 ++++---
doc/src/sgml/ref/alter_tablespace.sgml | 4 ++--
doc/src/sgml/ref/alter_type.sgml | 7 ++++---
doc/src/sgml/ref/alter_view.sgml | 7 ++++---
doc/src/sgml/ref/create_database.sgml | 4 ++--
doc/src/sgml/ref/create_schema.sgml | 4 ++--
doc/src/sgml/ref/grant.sgml | 14 ++++++++++++++
26 files changed, 96 insertions(+), 67 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 4b219435d4..0213db103a 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1714,8 +1714,8 @@ ALTER TABLE products RENAME TO items;
ALTER TABLE <replaceable>table_name</replaceable> OWNER TO <replaceable>new_owner</replaceable>;
</programlisting>
Superusers can always do this; ordinary roles can only do it if they are
- both the current owner of the object (or a member of the owning role) and
- a member of the new owning role.
+ both the current owner of the object (or inherit the privileges of the
+ owning role) and able to <literal>SET ROLE</literal> to the new owning role.
</para>
<para>
diff --git a/doc/src/sgml/ref/alter_aggregate.sgml b/doc/src/sgml/ref/alter_aggregate.sgml
index aee10a5ca2..d0a39ba7b5 100644
--- a/doc/src/sgml/ref/alter_aggregate.sgml
+++ b/doc/src/sgml/ref/alter_aggregate.sgml
@@ -46,9 +46,10 @@ ALTER AGGREGATE <replaceable>name</replaceable> ( <replaceable>aggregate_signatu
You must own the aggregate function to use <command>ALTER AGGREGATE</command>.
To change the schema of an aggregate function, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the aggregate function's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the aggregate function's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating
the aggregate function. However, a superuser can alter ownership of any
aggregate function anyway.)
diff --git a/doc/src/sgml/ref/alter_collation.sgml b/doc/src/sgml/ref/alter_collation.sgml
index a8c831d728..a40a31442a 100644
--- a/doc/src/sgml/ref/alter_collation.sgml
+++ b/doc/src/sgml/ref/alter_collation.sgml
@@ -39,9 +39,10 @@ ALTER COLLATION <replaceable>name</replaceable> SET SCHEMA <replaceable>new_sche
<para>
You must own the collation to use <command>ALTER COLLATION</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the collation's schema. (These restrictions enforce that altering the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the collation's schema.
+ (These restrictions enforce that altering the
owner doesn't do anything you couldn't do by dropping and recreating the
collation. However, a superuser can alter ownership of any collation
anyway.)
diff --git a/doc/src/sgml/ref/alter_conversion.sgml b/doc/src/sgml/ref/alter_conversion.sgml
index a128f20f3e..5c7cc978ee 100644
--- a/doc/src/sgml/ref/alter_conversion.sgml
+++ b/doc/src/sgml/ref/alter_conversion.sgml
@@ -37,9 +37,10 @@ ALTER CONVERSION <replaceable>name</replaceable> SET SCHEMA <replaceable>new_sch
<para>
You must own the conversion to use <command>ALTER CONVERSION</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the conversion's schema. (These restrictions enforce that altering the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the conversion's schema.
+ (These restrictions enforce that altering the
owner doesn't do anything you couldn't do by dropping and recreating the
conversion. However, a superuser can alter ownership of any conversion
anyway.)
diff --git a/doc/src/sgml/ref/alter_database.sgml b/doc/src/sgml/ref/alter_database.sgml
index 0962f32e13..5144e1f4ea 100644
--- a/doc/src/sgml/ref/alter_database.sgml
+++ b/doc/src/sgml/ref/alter_database.sgml
@@ -68,8 +68,8 @@ ALTER DATABASE <replaceable class="parameter">name</replaceable> RESET ALL
<para>
The third form changes the owner of the database.
- To alter the owner, you must own the database and also be a direct or
- indirect member of the new owning role, and you must have the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and you must have the
<literal>CREATEDB</literal> privilege.
(Note that superusers have all these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_domain.sgml b/doc/src/sgml/ref/alter_domain.sgml
index 2db5372513..f6704d7557 100644
--- a/doc/src/sgml/ref/alter_domain.sgml
+++ b/doc/src/sgml/ref/alter_domain.sgml
@@ -157,9 +157,9 @@ ALTER DOMAIN <replaceable class="parameter">name</replaceable>
You must own the domain to use <command>ALTER DOMAIN</command>.
To change the schema of a domain, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the domain's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal> privilege
+ on the domain's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the domain.
However, a superuser can alter ownership of any domain anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_foreign_table.sgml b/doc/src/sgml/ref/alter_foreign_table.sgml
index d056dc1bb1..0f4191713a 100644
--- a/doc/src/sgml/ref/alter_foreign_table.sgml
+++ b/doc/src/sgml/ref/alter_foreign_table.sgml
@@ -320,9 +320,9 @@ ALTER FOREIGN TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceab
You must own the table to use <command>ALTER FOREIGN TABLE</command>.
To change the schema of a foreign table, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the table's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal> privilege
+ on the table's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the table.
However, a superuser can alter ownership of any table anyway.)
To add a column or alter a column type, you must also
diff --git a/doc/src/sgml/ref/alter_function.sgml b/doc/src/sgml/ref/alter_function.sgml
index 2e8e1162d8..8193b17f25 100644
--- a/doc/src/sgml/ref/alter_function.sgml
+++ b/doc/src/sgml/ref/alter_function.sgml
@@ -60,9 +60,9 @@ ALTER FUNCTION <replaceable>name</replaceable> [ ( [ [ <replaceable class="param
<para>
You must own the function to use <command>ALTER FUNCTION</command>.
To change a function's schema, you must also have <literal>CREATE</literal>
- privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
+ privilege on the new schema. To alter the owner, you must be able to
+ <literal>SET ROLE</literal> to the new owning role, and that role must
+ have <literal>CREATE</literal> privilege on
the function's schema. (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the function.
However, a superuser can alter ownership of any function anyway.)
diff --git a/doc/src/sgml/ref/alter_large_object.sgml b/doc/src/sgml/ref/alter_large_object.sgml
index 17ea1491ba..f427006f80 100644
--- a/doc/src/sgml/ref/alter_large_object.sgml
+++ b/doc/src/sgml/ref/alter_large_object.sgml
@@ -35,8 +35,9 @@ ALTER LARGE OBJECT <replaceable class="parameter">large_object_oid</replaceable>
<para>
You must own the large object to use <command>ALTER LARGE OBJECT</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role. (However, a superuser can alter any large object anyway.)
+ To alter the owner, you must also be able to <literal>SET ROLE</literal> to
+ the new owning role.
+ (However, a superuser can alter any large object anyway.)
Currently, the only functionality is to assign a new owner, so both
restrictions always apply.
</para>
diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml
index 040ae53f98..da7ed04597 100644
--- a/doc/src/sgml/ref/alter_materialized_view.sgml
+++ b/doc/src/sgml/ref/alter_materialized_view.sgml
@@ -63,9 +63,10 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE <replaceable class="parameter">name</r
You must own the materialized view to use <command>ALTER MATERIALIZED
VIEW</command>. To change a materialized view's schema, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the materialized view's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the materialized view's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating the
materialized view. However, a superuser can alter ownership of any view
anyway.)
diff --git a/doc/src/sgml/ref/alter_opclass.sgml b/doc/src/sgml/ref/alter_opclass.sgml
index b1db459b11..231597d629 100644
--- a/doc/src/sgml/ref/alter_opclass.sgml
+++ b/doc/src/sgml/ref/alter_opclass.sgml
@@ -42,9 +42,10 @@ ALTER OPERATOR CLASS <replaceable>name</replaceable> USING <replaceable class="p
<para>
You must own the operator class to use <command>ALTER OPERATOR CLASS</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the operator class's schema. (These restrictions enforce that altering the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the operator class's schema.
+ (These restrictions enforce that altering the
owner doesn't do anything you couldn't do by dropping and recreating the
operator class. However, a superuser can alter ownership of any operator
class anyway.)
diff --git a/doc/src/sgml/ref/alter_operator.sgml b/doc/src/sgml/ref/alter_operator.sgml
index ad90c137f1..a4a1af564f 100644
--- a/doc/src/sgml/ref/alter_operator.sgml
+++ b/doc/src/sgml/ref/alter_operator.sgml
@@ -44,9 +44,10 @@ ALTER OPERATOR <replaceable>name</replaceable> ( { <replaceable>left_type</repla
<para>
You must own the operator to use <command>ALTER OPERATOR</command>.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the operator's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the operator's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the operator.
However, a superuser can alter ownership of any operator anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_procedure.sgml b/doc/src/sgml/ref/alter_procedure.sgml
index 20a623885f..a4737a3439 100644
--- a/doc/src/sgml/ref/alter_procedure.sgml
+++ b/doc/src/sgml/ref/alter_procedure.sgml
@@ -54,9 +54,10 @@ ALTER PROCEDURE <replaceable>name</replaceable> [ ( [ [ <replaceable class="para
You must own the procedure to use <command>ALTER PROCEDURE</command>.
To change a procedure's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the procedure's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the procedure's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the procedure.
However, a superuser can alter ownership of any procedure anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index c84b11f47a..cd20868bca 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -75,10 +75,12 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
Adding a table to a publication additionally requires owning that table.
The <literal>ADD TABLES IN SCHEMA</literal> and
<literal>SET TABLES IN SCHEMA</literal> to a publication requires the
- invoking user to be a superuser. To alter the owner, you must also be a
- direct or indirect member of the new owning role. The new owner must have
- <literal>CREATE</literal> privilege on the database. Also, the new owner
- of a <literal>FOR ALL TABLES</literal> or <literal>FOR TABLES IN SCHEMA</literal>
+ invoking user to be a superuser.
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the database.
+ Also, the new owner of a <literal>FOR ALL TABLES</literal> or
+ <literal>FOR TABLES IN SCHEMA</literal>
publication must be a superuser. However, a superuser can
change the ownership of a publication regardless of these restrictions.
</para>
diff --git a/doc/src/sgml/ref/alter_schema.sgml b/doc/src/sgml/ref/alter_schema.sgml
index 04624c5a5e..b8ace0561f 100644
--- a/doc/src/sgml/ref/alter_schema.sgml
+++ b/doc/src/sgml/ref/alter_schema.sgml
@@ -37,8 +37,8 @@ ALTER SCHEMA <replaceable>name</replaceable> OWNER TO { <replaceable>new_owner</
You must own the schema to use <command>ALTER SCHEMA</command>.
To rename a schema you must also have the
<literal>CREATE</literal> privilege for the database.
- To alter the owner, you must also be a direct or
- indirect member of the new owning role, and you must have the
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have the
<literal>CREATE</literal> privilege for the database.
(Note that superusers have all these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml
index 148085d4f2..7be36cf466 100644
--- a/doc/src/sgml/ref/alter_sequence.sgml
+++ b/doc/src/sgml/ref/alter_sequence.sgml
@@ -51,9 +51,10 @@ ALTER SEQUENCE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> S
You must own the sequence to use <command>ALTER SEQUENCE</command>.
To change a sequence's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the sequence's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the sequence's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the sequence.
However, a superuser can alter ownership of any sequence anyway.)
</para>
diff --git a/doc/src/sgml/ref/alter_server.sgml b/doc/src/sgml/ref/alter_server.sgml
index 186f38b5f8..467bf85589 100644
--- a/doc/src/sgml/ref/alter_server.sgml
+++ b/doc/src/sgml/ref/alter_server.sgml
@@ -40,8 +40,8 @@ ALTER SERVER <replaceable class="parameter">name</replaceable> RENAME TO <replac
<para>
To alter the server you must be the owner of the server.
- Additionally to alter the owner, you must own the server and also
- be a direct or indirect member of the new owning role, and you must
+ Additionally to alter the owner, you must be able to
+ <literal>SET ROLE</literal> to the new owning role, and you must
have <literal>USAGE</literal> privilege on the server's foreign-data
wrapper. (Note that superusers satisfy all these criteria
automatically.)
diff --git a/doc/src/sgml/ref/alter_statistics.sgml b/doc/src/sgml/ref/alter_statistics.sgml
index ce6cdf2bb1..73cc9e830d 100644
--- a/doc/src/sgml/ref/alter_statistics.sgml
+++ b/doc/src/sgml/ref/alter_statistics.sgml
@@ -43,9 +43,10 @@ ALTER STATISTICS <replaceable class="parameter">name</replaceable> SET STATISTIC
You must own the statistics object to use <command>ALTER STATISTICS</command>.
To change a statistics object's schema, you must also
have <literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the statistics object's schema. (These restrictions enforce that altering
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the statistics object's schema.
+ (These restrictions enforce that altering
the owner doesn't do anything you couldn't do by dropping and recreating
the statistics object. However, a superuser can alter ownership of any
statistics object anyway.)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 1e8d72062b..ad93553a1d 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -46,8 +46,8 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
You must own the subscription to use <command>ALTER SUBSCRIPTION</command>.
- To alter the owner, you must also be a direct or indirect member of the
- new owning role. The new owner has to be a superuser.
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role. The new owner has to be a superuser.
(Currently, all subscription owners must be superusers, so the owner checks
will be bypassed in practice. But this might change in the future.)
</para>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 794e886f96..9aaa32a782 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1106,9 +1106,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
To add the table as a new child of a parent table, you must own the parent
table as well. Also, to attach a table as a new partition of the table,
you must own the table being attached.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the table's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the table's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the table.
However, a superuser can alter ownership of any table anyway.)
To add a column or alter a column type or use the <literal>OF</literal>
diff --git a/doc/src/sgml/ref/alter_tablespace.sgml b/doc/src/sgml/ref/alter_tablespace.sgml
index 6de80746d5..6ec863400d 100644
--- a/doc/src/sgml/ref/alter_tablespace.sgml
+++ b/doc/src/sgml/ref/alter_tablespace.sgml
@@ -38,8 +38,8 @@ ALTER TABLESPACE <replaceable>name</replaceable> RESET ( <replaceable class="par
<para>
You must own the tablespace to change the definition of a tablespace.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role.
+ To alter the owner, you must also be able to <literal>SET ROLE</literal>
+ to the new owning role.
(Note that superusers have these privileges automatically.)
</para>
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 146065144f..025a3ee48f 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -246,9 +246,10 @@ ALTER TYPE <replaceable class="parameter">name</replaceable> SET ( <replaceable
You must own the type to use <command>ALTER TYPE</command>.
To change the schema of a type, you must also have
<literal>CREATE</literal> privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the type's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the type's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the type.
However, a superuser can alter ownership of any type anyway.)
To add an attribute or alter an attribute type, you must also
diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml
index 8bdc90a5a1..afbb3d02c7 100644
--- a/doc/src/sgml/ref/alter_view.sgml
+++ b/doc/src/sgml/ref/alter_view.sgml
@@ -45,9 +45,10 @@ ALTER VIEW [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RESET
You must own the view to use <command>ALTER VIEW</command>.
To change a view's schema, you must also have <literal>CREATE</literal>
privilege on the new schema.
- To alter the owner, you must also be a direct or indirect member of the new
- owning role, and that role must have <literal>CREATE</literal> privilege on
- the view's schema. (These restrictions enforce that altering the owner
+ To alter the owner, you must be able to <literal>SET ROLE</literal> to the
+ new owning role, and that role must have <literal>CREATE</literal>
+ privilege on the view's schema.
+ (These restrictions enforce that altering the owner
doesn't do anything you couldn't do by dropping and recreating the view.
However, a superuser can alter ownership of any view anyway.)
</para>
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index 2f034e2859..f3df2def86 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -89,8 +89,8 @@ CREATE DATABASE <replaceable class="parameter">name</replaceable>
The role name of the user who will own the new database,
or <literal>DEFAULT</literal> to use the default (namely, the
user executing the command). To create a database owned by another
- role, you must be a direct or indirect member of that role,
- or be a superuser.
+ role, you must must be able to <literal>SET ROLE</literal> to that
+ role.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_schema.sgml b/doc/src/sgml/ref/create_schema.sgml
index 3c2dddb163..04b0c28731 100644
--- a/doc/src/sgml/ref/create_schema.sgml
+++ b/doc/src/sgml/ref/create_schema.sgml
@@ -89,8 +89,8 @@ CREATE SCHEMA IF NOT EXISTS AUTHORIZATION <replaceable class="parameter">role_sp
<para>
The role name of the user who will own the new schema. If omitted,
defaults to the user executing the command. To create a schema
- owned by another role, you must be a direct or indirect member of
- that role, or be a superuser.
+ owned by another role, you must must be able to
+ <literal>SET ROLE</literal> to that role.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 85f5f42ea6..35bf0332c8 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
This option defaults to <literal>TRUE</literal>.
</para>
+ <para>
+ To create an object owned by another role or give ownership of an existing
+ object to another role, you must have the ability to <literal>SET
+ ROLE</literal> to that role; otherwise, commands such as <literal>ALTER
+ ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal>
+ will fail. However, a user who inherits the privileges of a role but does
+ not have the ability to <literal>SET ROLE</literal> to that role may be
+ able to obtain full access to the role by manipulating existing objects
+ owned by that role (e.g. they could redefine an existing function to act
+ as a Trojan horse). Therefore, if a role's privileges are to be inherited
+ but should not be accessible via <literal>SET ROLE</literal>, it should not
+ own any SQL objects.
+ </para>
+
<para>
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
having been done by the specified role. A user can only attribute a grant
--
2.37.1 (Apple Git-137.1)
On Wed, Jan 11, 2023 at 03:13:29PM -0500, Robert Haas wrote:
On Wed, Jan 11, 2023 at 10:16 AM Noah Misch <noah@leadboat.com> wrote:
I still think docs for the SET option itself should give a sense of the
diversity of things it's intended to control. It could be simple. A bunch of
the sites you're modifying are near text like "These restrictions enforce that
altering the owner doesn't do anything you couldn't do by dropping and
recreating the aggregate function." Perhaps the main SET doc could say
something about how it restricts other things that would yield equivalent
outcomes. (Incidentally, DROP is another case of something one likely doesn't
want the WITH SET FALSE member using. I think that reinforces a point I wrote
upthread. To achieve the original post's security objective, the role must
own no objects whatsoever.)I spent a while on this. The attached is as well I was able to figure
out how to do. What do you think?
I think this is good to go modulo one or two things:
Subject: [PATCH v2] More documentation update for GRANT ... WITH SET OPTION.
Update the reference pages for various ALTER commands that
mentioned that you must be a member of role that will be the
new owner to instead say that you must be able to SET ROLE
to the new owner. Update ddl.sgml's generate statement on this
s/generate/general/
--- a/doc/src/sgml/ref/grant.sgml +++ b/doc/src/sgml/ref/grant.sgml @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace This option defaults to <literal>TRUE</literal>. </para>+ <para> + To create an object owned by another role or give ownership of an existing + object to another role, you must have the ability to <literal>SET + ROLE</literal> to that role; otherwise, commands such as <literal>ALTER + ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal> + will fail. However, a user who inherits the privileges of a role but does + not have the ability to <literal>SET ROLE</literal> to that role may be + able to obtain full access to the role by manipulating existing objects + owned by that role (e.g. they could redefine an existing function to act + as a Trojan horse). Therefore, if a role's privileges are to be inherited + but should not be accessible via <literal>SET ROLE</literal>, it should not + own any SQL objects. + </para>
I recommend deleting the phrase "are to be inherited but" as superfluous. The
earlier sentence's mention will still be there. WITH SET FALSE + NOINHERIT is
a combination folks should not use or should use only when the role has no
known privileges.
On Thu, Jan 12, 2023 at 12:09 AM Noah Misch <noah@leadboat.com> wrote:
I think this is good to go modulo one or two things:
Subject: [PATCH v2] More documentation update for GRANT ... WITH SET OPTION.
Update the reference pages for various ALTER commands that
mentioned that you must be a member of role that will be the
new owner to instead say that you must be able to SET ROLE
to the new owner. Update ddl.sgml's generate statement on thiss/generate/general/
Oops, yes.
--- a/doc/src/sgml/ref/grant.sgml +++ b/doc/src/sgml/ref/grant.sgml @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace This option defaults to <literal>TRUE</literal>. </para>+ <para> + To create an object owned by another role or give ownership of an existing + object to another role, you must have the ability to <literal>SET + ROLE</literal> to that role; otherwise, commands such as <literal>ALTER + ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal> + will fail. However, a user who inherits the privileges of a role but does + not have the ability to <literal>SET ROLE</literal> to that role may be + able to obtain full access to the role by manipulating existing objects + owned by that role (e.g. they could redefine an existing function to act + as a Trojan horse). Therefore, if a role's privileges are to be inherited + but should not be accessible via <literal>SET ROLE</literal>, it should not + own any SQL objects. + </para>I recommend deleting the phrase "are to be inherited but" as superfluous. The
earlier sentence's mention will still be there. WITH SET FALSE + NOINHERIT is
a combination folks should not use or should use only when the role has no
known privileges.
I don't think I agree with this suggestion. If the privileges aren't
going to be inherited, it doesn't matter whether the role owns SQL
objects or not. And I think that there are two notable use cases for
SET FALSE + NOINHERIT (or SET FALSE + INHERIT FALSE). First, the a
grant with SET FALSE, INHERIT FALSE, ADMIN TRUE gives you the ability
to administer a role without inheriting its privileges or being able
to SET ROLE to it. You could grant yourself those abilities if you
want, but you don't have them straight off. In fact, CREATE ROLE
issued by a non-superuser creates such a grant implicitly as of
cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb. Second, SET FALSE, INHERIT
FALSE could be used to set up groups for pg_hba.conf matching without
conferring privileges.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Jan 12, 2023 at 10:21:32AM -0500, Robert Haas wrote:
On Thu, Jan 12, 2023 at 12:09 AM Noah Misch <noah@leadboat.com> wrote:
--- a/doc/src/sgml/ref/grant.sgml +++ b/doc/src/sgml/ref/grant.sgml @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace This option defaults to <literal>TRUE</literal>. </para>+ <para> + To create an object owned by another role or give ownership of an existing + object to another role, you must have the ability to <literal>SET + ROLE</literal> to that role; otherwise, commands such as <literal>ALTER + ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal> + will fail. However, a user who inherits the privileges of a role but does + not have the ability to <literal>SET ROLE</literal> to that role may be + able to obtain full access to the role by manipulating existing objects + owned by that role (e.g. they could redefine an existing function to act + as a Trojan horse). Therefore, if a role's privileges are to be inherited + but should not be accessible via <literal>SET ROLE</literal>, it should not + own any SQL objects. + </para>I recommend deleting the phrase "are to be inherited but" as superfluous. The
earlier sentence's mention will still be there. WITH SET FALSE + NOINHERIT is
a combination folks should not use or should use only when the role has no
known privileges.I don't think I agree with this suggestion. If the privileges aren't
going to be inherited, it doesn't matter whether the role owns SQL
objects or not. And I think that there are two notable use cases for
SET FALSE + NOINHERIT (or SET FALSE + INHERIT FALSE). First, the a
grant with SET FALSE, INHERIT FALSE, ADMIN TRUE gives you the ability
to administer a role without inheriting its privileges or being able
to SET ROLE to it. You could grant yourself those abilities if you
want, but you don't have them straight off. In fact, CREATE ROLE
issued by a non-superuser creates such a grant implicitly as of
cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb.
That is a valid use case, but Trojan horse matters don't apply there.
Second, SET FALSE, INHERIT
FALSE could be used to set up groups for pg_hba.conf matching without
conferring privileges.
That is factual, but doing this and having that role own objects shouldn't be
considered a best practice. It's a bit like using the address of a function
as an enum value. Instead of role own_some_objects_and_control_hba, the best
practice would be to have two roles, own_some_objects / control_hba.
Since the text is superfluous but not wrong, I won't insist.
On Fri, Jan 13, 2023 at 2:17 AM Noah Misch <noah@leadboat.com> wrote:
Since the text is superfluous but not wrong, I won't insist.
OK, committed as I had it, then.
To me, the text isn't superfluous, because otherwise the connection to
what has been said in the previous sentence seems tenuous, which
impacts understandability. We'll see what other people think, I guess.
Perhaps there's some altogether better way to talk about this.
--
Robert Haas
EDB: http://www.enterprisedb.com