Re: Role Attribute Bitmask Catalog Representation

Adam,

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)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