[PATCH] remove is_member_of_role() from header, add can_set_role()
As a follow-on to Conflation of member/privs for predefined roles,
this removes is_member_of_role from the header to dissuade it's use
for privilege checking. Since SET ROLE must use membership rather than
privileges a new, explicitly named can_set_role() function is
exported.
is_member_of_role_nosuper() still exists for the following purposes:
- membership loop checking in user.c
- membership matching for pg_hba.conf in hba.c
Other uses of is_member_of_role_nosuper() should be avoided.
Attachments:
0001-unexport-is_member_of_role-add-can_set_role.patchapplication/octet-stream; name=0001-unexport-is_member_of_role-add-can_set_role.patchDownload
From c5eaa67a3d487327fd373f19f94c8d18783da08c Mon Sep 17 00:00:00 2001
From: Joshua Brindle <joshua.brindle@crunchydata.com>
Date: Wed, 27 Oct 2021 08:14:14 -0700
Subject: [PATCH] unexport is_member_of_role(), add can_set_role()
is_member_of_role should not be used for privilege checking, so
to avoid that remove it from the acl header. The only remaining
usage is for SET ROLE in variable.c where membership is the
legitimate check, so a new exported explicetly named
can_set_role() is added in it's place.
Signed-off-by: Joshua Brindle <joshua.brindle@crunchydata.com>
---
src/backend/commands/variable.c | 2 +-
src/backend/utils/adt/acl.c | 17 +++++++++++++++++
src/include/utils/acl.h | 2 +-
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 0c85679420c..04f56e42872 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))
+ !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 67f8b29434a..d638eb35e3c 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -72,6 +72,7 @@ static List *cached_roles[] = {NIL, NIL};
static uint32 cached_db_hash;
+static bool is_member_of_role(Oid member, Oid role);
static const char *getid(const char *s, char *n);
static void putid(char *p, const char *s);
static Acl *allocacl(int n);
@@ -4859,11 +4860,25 @@ has_privs_of_role(Oid member, Oid role)
role);
}
+/*
+ * can_set_role
+ *
+ * Identical to is_member_of_role but exported for the sole use by check_role()
+ * for checking SET ROLE
+ *
+ */
+bool
+can_set_role(Oid member, Oid role)
+{
+ return is_member_of_role(member, role);
+}
/*
* Is member a member of role (directly or indirectly)?
*
* This is defined to recurse through roles regardless of rolinherit.
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role()
*/
bool
is_member_of_role(Oid member, Oid role)
@@ -4904,6 +4919,8 @@ check_is_member_of_role(Oid member, Oid role)
*
* This is identical to is_member_of_role except we ignore superuser
* status.
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role()
*/
bool
is_member_of_role_nosuper(Oid member, Oid role)
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index af771c901d1..bdbbc43b697 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -206,7 +206,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 is_member_of_role(Oid member, Oid role);
+extern bool can_set_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 void check_is_member_of_role(Oid member, Oid role);
--
2.31.1
On Oct 27, 2021, at 9:26 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
As a follow-on to Conflation of member/privs for predefined roles,
this removes is_member_of_role from the header to dissuade it's use
for privilege checking. Since SET ROLE must use membership rather than
privileges a new, explicitly named can_set_role() function is
exported.is_member_of_role_nosuper() still exists for the following purposes:
- membership loop checking in user.c
- membership matching for pg_hba.conf in hba.cOther uses of is_member_of_role_nosuper() should be avoided.
<0001-unexport-is_member_of_role-add-can_set_role.patch>
I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment:
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role()be added to the header for is_member_of_role() without needing the new wrapper function?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
On Oct 27, 2021, at 9:26 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
As a follow-on to Conflation of member/privs for predefined roles,
this removes is_member_of_role from the header to dissuade it's use
for privilege checking. Since SET ROLE must use membership rather than
privileges a new, explicitly named can_set_role() function is
exported.is_member_of_role_nosuper() still exists for the following purposes:
- membership loop checking in user.c
- membership matching for pg_hba.conf in hba.cOther uses of is_member_of_role_nosuper() should be avoided.
<0001-unexport-is_member_of_role-add-can_set_role.patch>I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment:
+ * + * Do not use this for privilege checking, instead use has_privs_of_role()be added to the header for is_member_of_role() without needing the new wrapper function?
It could be, but the intent is to dissuade it from being used, so
getting rid of it and making an explicit version that has a sole use
seemed useful.
It's possible that it's being used inappropriately out-of-tree so this
would also prevent that.
On 10/27/21, 10:22 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment:
+ * + * Do not use this for privilege checking, instead use has_privs_of_role()be added to the header for is_member_of_role() without needing the new wrapper function?
It could be, but the intent is to dissuade it from being used, so
getting rid of it and making an explicit version that has a sole use
seemed useful.It's possible that it's being used inappropriately out-of-tree so this
would also prevent that.
I think a comment about the intended usage is sufficient. However,
renaming the function or replacing it with a wrapper might break
extensions and encourage the authors to reevaluate. That could be a
good thing.
Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 10/27/21, 10:22 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment:
I think a comment about the intended usage is sufficient.
I agree with the position that a better function header comment is
sufficient. However, if we're to rename it, please not "can_set_role".
That's bad in at least two ways:
* it's quite unclear what "set" means in this context ... maybe the
function is testing whether you're allowed to alter properties of
the role, or do "ALTER ROLE ... SET parameter = value"? It only makes
sense if you already know that the specific command SET ROLE is being
thought of.
* it's unclear which argument is which end of the relationship.
Something like "can_set_role_to()" would help with the second problem,
but I'm not sure it does much for the first one.
On the whole I think the existing name is fine.
regards, tom lane