Re: Role Attribute Bitmask Catalog Representation - Mailing list pgsql-hackers

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:
> I have attached an updated patch that incorporates the feedback and
> recommendations provided.

Thanks.  Comments below.

> diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
> --- 56,62 ----
>
>       backupidstr = text_to_cstring(backupid);
>
> !     if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>           ereport(ERROR,
>                   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>              errmsg("must be superuser or replication role to run a backup")));

The point of has_role_attribute() was to avoid the need to explicitly
say "!superuser()" everywhere.  The idea with check_role_attribute() is
that we want to present the user (in places like pg_roles) with the
values that are *actually* set.

In other words, the above should just be:

if (!has_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))

> --- 84,90 ----
>   {
>       XLogRecPtr    stoppoint;
>
> !     if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>           ereport(ERROR,
>                   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>            (errmsg("must be superuser or replication role to run a backup"))));

Ditto here.

> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> --- 5031,5081 ----
>   }
>
>   /*
> !  * has_role_attribute
> !  *   Check if the role has the specified role has a specific role attribute.
> !  *   This function will always return true for roles with superuser privileges
> !  *   unless the attribute being checked is CATUPDATE.
>    *
> !  * roleid - the oid of the role to check.
> !  * attribute - the attribute to check.
>    */
>   bool
> ! has_role_attribute(Oid roleid, RoleAttr attribute)
>   {
> !     /*
> !      * Superusers bypass all permission checking except in the case of CATUPDATE.
> !      */
> !     if (!(attribute & ROLE_ATTR_CATUPDATE) && superuser_arg(roleid))
>           return true;
>
> !     return check_role_attribute(roleid, attribute);
>   }
>
> + /*
> +  * check_role_attribute
> +  *   Check if the role with the specified id has been assigned a specific
> +  *   role attribute.  This function does not allow any superuser bypass.

I don't think we need to say that it doesn't "allow" superuser bypass.
Instead, I'd comment that has_role_attribute() should be used for
permissions checks while check_role_attribute is for checking what the
value in the rolattr bitmap is and isn't for doing permissions checks
directly.

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> *************** CreateRole(CreateRoleStmt *stmt)
> *** 82,93 ****
>       bool        encrypt_password = Password_encryption; /* encrypt password? */
>       char        encrypted_password[MD5_PASSWD_LEN + 1];
>       bool        issuper = false;    /* Make the user a superuser? */
> !     bool        inherit = true; /* Auto inherit privileges? */
>       bool        createrole = false;        /* Can this user create roles? */
>       bool        createdb = false;        /* Can the user create databases? */
>       bool        canlogin = false;        /* Can this user login? */
>       bool        isreplication = false;    /* Is this a replication role? */
>       bool        bypassrls = false;        /* Is this a row security enabled role? */
>       int            connlimit = -1; /* maximum connections allowed */
>       List       *addroleto = NIL;    /* roles to make this a member of */
>       List       *rolemembers = NIL;        /* roles to be members of this role */
> --- 74,86 ----
>       bool        encrypt_password = Password_encryption; /* encrypt password? */
>       char        encrypted_password[MD5_PASSWD_LEN + 1];
>       bool        issuper = false;    /* Make the user a superuser? */
> !     bool        inherit = true;    /* Auto inherit privileges? */
>       bool        createrole = false;        /* Can this user create roles? */
>       bool        createdb = false;        /* Can the user create databases? */
>       bool        canlogin = false;        /* Can this user login? */
>       bool        isreplication = false;    /* Is this a replication role? */
>       bool        bypassrls = false;        /* Is this a row security enabled role? */
> +     RoleAttr    attributes = ROLE_ATTR_NONE;    /* role attributes, initialized to none. */
>       int            connlimit = -1; /* maximum connections allowed */
>       List       *addroleto = NIL;    /* roles to make this a member of */
>       List       *rolemembers = NIL;        /* roles to be members of this role */

Please clean up the whitespace changes- there's no need for the
'inherit' line to change..

> diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
> *************** XLogRead(char *buf, TimeLineID tli, XLog
> *** 205,211 ****
>   static void
>   check_permissions(void)
>   {
> !     if (!superuser() && !has_rolreplication(GetUserId()))
>           ereport(ERROR,
>                   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                    (errmsg("must be superuser or replication role to use replication slots"))));
> --- 207,213 ----
>   static void
>   check_permissions(void)
>   {
> !     if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>           ereport(ERROR,
>                   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                    (errmsg("must be superuser or replication role to use replication slots"))));

Another case which should be using has_role_attribute()

> diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
> --- 17,34 ----
>   #include "miscadmin.h"
>
>   #include "access/htup_details.h"
> + #include "catalog/pg_authid.h"
>   #include "replication/slot.h"
>   #include "replication/logical.h"
>   #include "replication/logicalfuncs.h"
> + #include "utils/acl.h"
>   #include "utils/builtins.h"
>   #include "utils/pg_lsn.h"
>
>   static void
>   check_permissions(void)
>   {
> !     if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>           ereport(ERROR,
>                   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                    (errmsg("must be superuser or replication role to use replication slots"))));

Also here.

> diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
> *************** pg_role_aclcheck(Oid role_oid, Oid rolei
> *** 4602,4607 ****
> --- 4603,4773 ----
>       return ACLCHECK_NO_PRIV;
>   }
>
> + /*
> +  * pg_has_role_attribute_id_attr

I'm trying to figure out what the point of the trailing "_attr" in the
function name is..?  Doesn't seem necessary to have that for these
functions and it'd look a bit cleaner without it, imv.

> diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
> new file mode 100644
> index c348034..be0e1cc
> *** a/src/backend/utils/init/postinit.c
> --- b/src/backend/utils/init/postinit.c
> *************** InitPostgres(const char *in_dbname, Oid
> *** 762,768 ****
>       {
>           Assert(!bootstrap);
>
> !         if (!superuser() && !has_rolreplication(GetUserId()))
>               ereport(FATAL,
>                       (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                        errmsg("must be superuser or replication role to start walsender")));
> --- 762,768 ----
>       {
>           Assert(!bootstrap);
>
> !         if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>               ereport(FATAL,
>                       (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                        errmsg("must be superuser or replication role to start walsender")));

Also here.

> ! #define ROLE_ATTR_ALL          255 /* or (1 << N_ROLE_ATTRIBUTES) - 1 */

I'd say "equals" or something rather than "or" since that kind of
implies that it's an laternative, but we can't do that as discussed in
the comment (which I like).

> ! /* role attribute check routines */
> ! extern bool has_role_attribute(Oid roleid, RoleAttr attribute);
> ! extern bool check_role_attribute(Oid roleid, RoleAttr attribute);

With all the 'has_role_attribute(GetUserId(), ROLEATTR_BLAH)' cases, I'd
suggest doing the same as 'superuser()' and also provide a simpler
version: 'have_role_attribute(ROLEATTR_BLAH)' which handles doing the
GetUserId() itself.  That'd simplify quite a few of the above calls.

I'm pretty happy with the rest of it.
Thanks!
    Stephen