Обсуждение: [PATCH] Conflation of member/privs for predefined roles

Generally if a role is granted membership to another role with NOINHERIT
they must use SET ROLE to access the privileges of that role, however
with predefined roles the membership and privilege is conflated, as
demonstrated by:

CREATE ROLE readrole;
CREATE ROLE role2 NOINHERIT;
CREATE ROLE brindle LOGIN;

GRANT role2 TO brindle;
CREATE TABLE foo(i INT);
GRANT readrole TO role2;
GRANT ALL ON TABLE foo TO readrole;

GRANT pg_read_all_stats,pg_read_all_settings,pg_read_server_files,pg_write_server_files,pg_execute_server_program TO
role2;

Log in as brindle:

postgres=> select current_user;
 current_user
--------------
 brindle
(1 row)

postgres=> SELECT * FROM foo;
ERROR:  permission denied for table foo

postgres=> SELECT DISTINCT query FROM pg_stat_activity;
                    query
----------------------------------------------

 SELECT DISTINCT query FROM pg_stat_activity;
(2 rows)

postgres=> SET ROLE readrole;
SET
postgres=> SELECT * FROM foo;
 i
---
(0 rows)

After this patch:

postgres=> SELECT DISTINCT query FROM pg_stat_activity;
          query
--------------------------
 <insufficient privilege>
(1 row)

postgres=> SET ROLE pg_read_all_stats;
SET
postgres=> SELECT DISTINCT query FROM pg_stat_activity;
                    query
----------------------------------------------

 SELECT DISTINCT query FROM pg_stat_activity;
(2 rows)

postgres=> SHOW config_file;
ERROR:  must be superuser or have privileges of pg_read_all_settings to examine "config_file"
postgres=> SET ROLE pg_read_all_settings;
SET
postgres=> SHOW config_file;
            config_file
-----------------------------------
 /var/lib/pgsql/15/postgresql.conf
(1 row)

With inheritance it works as expected:

ALTER ROLE role2 INHERIT;

postgres=> SELECT current_user;
 current_user
--------------
 brindle
(1 row)

postgres=> SHOW config_file;
            config_file
-----------------------------------
 /var/lib/pgsql/15/postgresql.conf
(1 row)

Signed-off-by: Joshua Brindle <joshua.brindle@crunchydata.com>
---
 src/backend/commands/copy.c           | 12 ++++++------
 src/backend/replication/walreceiver.c |  8 ++++----
 src/backend/replication/walsender.c   |  8 ++++----
 src/backend/utils/adt/dbsize.c        |  8 ++++----
 src/backend/utils/adt/genfile.c       |  6 +++---
 src/backend/utils/adt/pgstatfuncs.c   |  2 +-
 src/backend/utils/misc/guc.c          | 20 ++++++++++----------
 7 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 53f48531419..e26ff42fd82 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -80,26 +80,26 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
     {
         if (stmt->is_program)
         {
-            if (!is_member_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
+            if (!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM))
                 ereport(ERROR,
                         (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("must be superuser or a member of the pg_execute_server_program role to COPY to or
froman external program"),
 
+                         errmsg("must be superuser or have privileges of the pg_execute_server_program role to COPY to
orfrom an external program"),
 
                          errhint("Anyone can COPY to stdout or from stdin. "
                                  "psql's \\copy command also works for anyone.")));
         }
         else
         {
-            if (is_from && !is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
+            if (is_from && !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
                 ereport(ERROR,
                         (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("must be superuser or a member of the pg_read_server_files role to COPY from a
file"),
+                         errmsg("must be superuser or have privileges of the pg_read_server_files role to COPY from a
file"),
                          errhint("Anyone can COPY to stdout or from stdin. "
                                  "psql's \\copy command also works for anyone.")));
 
-            if (!is_from && !is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
+            if (!is_from && !has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
                 ereport(ERROR,
                         (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("must be superuser or a member of the pg_write_server_files role to COPY to a file"),
+                         errmsg("must be superuser or have privileges of the pg_write_server_files role to COPY to a
file"),
                          errhint("Anyone can COPY to stdout or from stdin. "
                                  "psql's \\copy command also works for anyone.")));
         }
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index b90e5ca98ea..c8ddb6fc323 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1392,12 +1392,12 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
     /* Fetch values */
     values[0] = Int32GetDatum(pid);
 
-    if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
+    if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
     {
         /*
-         * Only superusers and members of pg_read_all_stats can see details.
-         * Other users only get the pid value to know whether it is a WAL
-         * receiver, but no details.
+         * Only superusers and roles with privileges of pg_read_all_stats
+         * can see details. Other users only get the pid value to know whether
+         * it is a WAL receiver, but no details.
          */
         MemSet(&nulls[1], true, sizeof(bool) * (tupdesc->natts - 1));
     }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index d9ab6d6de24..4daf1581bc6 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -3486,12 +3486,12 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
         memset(nulls, 0, sizeof(nulls));
         values[0] = Int32GetDatum(pid);
 
-        if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
+        if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
         {
             /*
-             * Only superusers and members of pg_read_all_stats can see
-             * details. Other users only get the pid value to know it's a
-             * walsender, but no details.
+             * Only superusers and roles with privileges of pg_read_all_stats
+             * can see details. Other users only get the pid value to know
+             * it's a walsender, but no details.
              */
             MemSet(&nulls[1], true, PG_STAT_GET_WAL_SENDERS_COLS - 1);
         }
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index d5a7fb13f3c..95a5d34fdf1 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -112,12 +112,12 @@ calculate_database_size(Oid dbOid)
     AclResult    aclresult;
 
     /*
-     * User must have connect privilege for target database or be a member of
+     * User must have connect privilege for target database or have privilegs of
      * pg_read_all_stats
      */
     aclresult = pg_database_aclcheck(dbOid, GetUserId(), ACL_CONNECT);
     if (aclresult != ACLCHECK_OK &&
-        !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
+        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
     {
         aclcheck_error(aclresult, OBJECT_DATABASE,
                        get_database_name(dbOid));
@@ -196,12 +196,12 @@ calculate_tablespace_size(Oid tblspcOid)
     AclResult    aclresult;
 
     /*
-     * User must be a member of pg_read_all_stats or have CREATE privilege for
+     * User must have privileges of pg_read_all_stats or have CREATE privilege for
      * target tablespace, either explicitly granted or implicitly because it
      * is default for current database.
      */
     if (tblspcOid != MyDatabaseTableSpace &&
-        !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
+        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
     {
         aclresult = pg_tablespace_aclcheck(tblspcOid, GetUserId(), ACL_CREATE);
         if (aclresult != ACLCHECK_OK)
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index c436d9318b6..f87f77093a6 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -58,11 +58,11 @@ convert_and_check_filename(text *arg)
     canonicalize_path(filename);    /* filename can change length here */
 
     /*
-     * Members of the 'pg_read_server_files' role are allowed to access any
-     * files on the server as the PG user, so no need to do any further checks
+     * Roles with privleges of the 'pg_read_server_files' role are allowed to access
+     * any files on the server as the PG user, so no need to do any further checks
      * here.
      */
-    if (is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
+    if (has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES))
         return filename;
 
     /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ff5aedc99cb..56762a7d98d 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -34,7 +34,7 @@
 
 #define UINT32_ACCESS_ONCE(var)         ((uint32)(*((volatile uint32 *)&(var))))
 
-#define HAS_PGSTAT_PERMISSIONS(role)     (is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS) ||
has_privs_of_role(GetUserId(),role))
 
+#define HAS_PGSTAT_PERMISSIONS(role)     (has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS) ||
has_privs_of_role(GetUserId(),role))
 
 
 Datum
 pg_stat_get_numscans(PG_FUNCTION_ARGS)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e91d5a3cfda..e400bfdea13 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8154,10 +8154,10 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
         return NULL;
     if (restrict_privileged &&
         (record->flags & GUC_SUPERUSER_ONLY) &&
-        !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
         ereport(ERROR,
                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"",
+                 errmsg("must be superuser have privileges of pg_read_all_settings to examine \"%s\"",
                         name)));
 
     switch (record->vartype)
@@ -8201,10 +8201,10 @@ GetConfigOptionResetString(const char *name)
     record = find_option(name, false, false, ERROR);
     Assert(record != NULL);
     if ((record->flags & GUC_SUPERUSER_ONLY) &&
-        !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
         ereport(ERROR,
                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"",
+                 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
                         name)));
 
     switch (record->vartype)
@@ -9448,7 +9448,7 @@ ShowAllGUCConfig(DestReceiver *dest)
 
         if ((conf->flags & GUC_NO_SHOW_ALL) ||
             ((conf->flags & GUC_SUPERUSER_ONLY) &&
-             !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+             !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
             continue;
 
         /* assign to the values array */
@@ -9515,7 +9515,7 @@ get_explain_guc_options(int *num)
         /* return only options visible to the current user */
         if ((conf->flags & GUC_NO_SHOW_ALL) ||
             ((conf->flags & GUC_SUPERUSER_ONLY) &&
-             !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+             !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
             continue;
 
         /* return only options that are different from their boot values */
@@ -9597,10 +9597,10 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
     }
 
     if ((record->flags & GUC_SUPERUSER_ONLY) &&
-        !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
         ereport(ERROR,
                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"",
+                 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
                         name)));
 
     if (varname)
@@ -9628,7 +9628,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
     {
         if ((conf->flags & GUC_NO_SHOW_ALL) ||
             ((conf->flags & GUC_SUPERUSER_ONLY) &&
-             !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+             !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
             *noshow = true;
         else
             *noshow = false;
@@ -9823,7 +9823,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
      * insufficiently-privileged users.
      */
     if (conf->source == PGC_S_FILE &&
-        is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+        has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
     {
         values[14] = conf->sourcefile;
         snprintf(buffer, sizeof(buffer), "%d", conf->sourceline);
-- 
2.31.1

On 10/26/21, 3:50 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> Generally if a role is granted membership to another role with NOINHERIT
> they must use SET ROLE to access the privileges of that role, however
> with predefined roles the membership and privilege is conflated, as
> demonstrated by:

I think it makes sense that INHERIT/NOINHERIT should be respected for
the predefined roles.  I went through some of the old threads and
commits for predefined roles, and I didn't find any mention of
inheritance, so there might not be a strong reason it was done this
way.

I saw a few places in the docs that will likely need to be updated as
well.  For example, pg_freespacemap has this note:

        By default use is restricted to superusers and members of the pg_stat_scan_tables role.

And I found at least one test (rolenames.sql) that fails due to the
new ERROR message.

Nathan

On Wed, Oct 27, 2021 at 1:20 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/26/21, 3:50 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> > Generally if a role is granted membership to another role with NOINHERIT
> > they must use SET ROLE to access the privileges of that role, however
> > with predefined roles the membership and privilege is conflated, as
> > demonstrated by:
>
> I think it makes sense that INHERIT/NOINHERIT should be respected for
> the predefined roles.  I went through some of the old threads and
> commits for predefined roles, and I didn't find any mention of
> inheritance, so there might not be a strong reason it was done this
> way.

Thank you for looking into this. We believe this was a mistake and I
have a follow-up patch to remove is_member_of_role() from the header
to avoid it going forward.

At least one new pre-defined role patch (pg_maintenance) was recently
submitted using has_privs_of_role() so it seems like there is a need
for consistency regardless.

> I saw a few places in the docs that will likely need to be updated as
> well.  For example, pg_freespacemap has this note:
>
>         By default use is restricted to superusers and members of the pg_stat_scan_tables role.
>
> And I found at least one test (rolenames.sql) that fails due to the
> new ERROR message.

I'm new to contributing here but I've been told that the string
changes get taken care of later, is that not true?

On 10/27/21, 10:28 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> I'm new to contributing here but I've been told that the string
> changes get taken care of later, is that not true?

I will sometimes leave out tests and docs until I get buy-in on the
approach.  But for serious consideration, I think the patch has to be
more-or-less complete.

Nathan

On Wed, Oct 27, 2021 at 1:34 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/27/21, 10:28 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> > I'm new to contributing here but I've been told that the string
> > changes get taken care of later, is that not true?
>
> I will sometimes leave out tests and docs until I get buy-in on the
> approach.  But for serious consideration, I think the patch has to be
> more-or-less complete.
>

Thanks, I'll fix those and resubmit both patches in a single email
since one depends on the other.