Re: CREATEROLE and role ownership hierarchies - Mailing list pgsql-hackers

Hi,

On Sat, Jan 29, 2022 at 09:58:38PM -0800, Mark Dilger wrote:
> > On Jan 25, 2022, at 12:44 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > I agree that CREATEROLE is overpowered and that the goal of this should
> > be to provide a way for roles to be created and dropped that doesn't
> > give the user who has that power everything that CREATEROLE currently
> > does.
> 
> I'm attaching a patch that attempts to fix CREATEROLE without any
> connection to role ownership.

Sounds like a useful way forward.
 
> >  The point I was making is that the concept of role ownership
> > isn't intrinsically linked to that and is, therefore, as you say, gravy.
> 
> I agree, they aren't intrinsically linked, though the solution to one
> might interact in some ways with the solution to the other.
> 
> > That isn't to say that I'm entirely against the role ownership idea but
> > I'd want it to be focused on the goal of providing ways of creating and
> > dropping users and otherwise performing that kind of administration and
> > that doesn't require the specific change to make owners be members of
> > all roles they own and automatically have all privileges of those roles
> > all the time.
> 
> The attached WIP patch attempts to solve most of the CREATEROLE
> problems but not the problem of which role who can drop which other
> role.  That will likely require an ownership concept.
> 
> The main idea here is that having CREATEROLE doesn't give you ADMIN on
> roles, nor on role attributes.  For role attributes, the syntax has
> been extended.  An excerpt from the patch's regression test
> illustrates some of that concept:
> 
> -- ok, superuser can create a role that can create login replication users, but
> -- cannot itself login, nor perform replication
> CREATE ROLE regress_role_repladmin
>     CREATEROLE WITHOUT ADMIN OPTION     -- can create roles, but cannot give it away
>     NOCREATEDB WITHOUT ADMIN OPTION     -- cannot create db, nor give it away
>     NOLOGIN WITH ADMIN OPTION           -- cannot log in, but can give it away
>     NOREPLICATION WITH ADMIN OPTION     -- cannot replicate, but can give it away
>     NOBYPASSRLS WITHOUT ADMIN OPTION;   -- cannot bypassrls, nor give it away
> 
> -- ok, superuser can create a role with CREATEROLE but restrict give-aways
> CREATE ROLE regress_role_minoradmin
>     NOSUPERUSER                         -- WITHOUT ADMIN OPTION is implied
>     CREATEROLE WITHOUT ADMIN OPTION
>     NOCREATEDB WITHOUT ADMIN OPTION
>     NOLOGIN WITHOUT ADMIN OPTION
>     NOREPLICATION                       -- WITHOUT ADMIN OPTION is implied
>     NOBYPASSRLS                         -- WITHOUT ADMIN OPTION is implied
>     NOINHERIT WITHOUT ADMIN OPTION
>     CONNECTION LIMIT NONE WITHOUT ADMIN OPTION
>     VALID ALWAYS WITHOUT ADMIN OPTION
>     PASSWORD NULL WITHOUT ADMIN OPTION;
> 
> -- fail, having CREATEROLE is not enough to create roles in privileged roles
> SET SESSION AUTHORIZATION regress_role_minoradmin;
> CREATE ROLE regress_nosuch_read_all_data IN ROLE pg_read_all_data;
> ERROR:  must have admin option on role "pg_read_all_data"

Great.
 
> -- fail, cannot change attributes without ADMIN for them
> SET SESSION AUTHORIZATION regress_role_minoradmin;
> ALTER ROLE regress_role_login LOGIN;
> ERROR:  must have admin on login to change login attribute
>
> ALTER ROLE regress_role_login NOLOGIN;
> ERROR:  must have admin on login to change login attribute
> 
> Whether "WITH ADMIN OPTION" or "WITHOUT ADMIN OPTION" is implied
> hinges on whether the role is given CREATEROLE.  That hackery is
> necessary to preserve backwards compatibility.  If we don't care about
> compatibility, I could change the patch to make "WITHOUT ADMIN OPTION"
> implied for all attributes when not specified.
> 
> I'd appreciate feedback on the direction this patch is going.
 
One thing I noticed (and which will likely make DBAs grumpy) is that it
seems being able to create users (as opposed to non-login roles/groups)
depends on when you get the CREATEROLE attribute (on role creation or
later), viz:

postgres=# CREATE USER admin CREATEROLE;
CREATE ROLE
postgres=# SET ROLE admin;
SET
postgres=> CREATE USER testuser; -- this works
CREATE ROLE
postgres=> RESET ROLE;
RESET
postgres=# CREATE USER admin2;
CREATE ROLE
postgres=# ALTER ROLE admin2 CREATEROLE; -- we get CREATEROLE after the fact
ALTER ROLE
postgres=# SET ROLE admin2;
SET
postgres=> CREATE USER testuser2; -- bam
ERROR:  must have grant option on LOGIN privilege to create login users
postgres=# SELECT rolname, admcreaterole, admcanlogin FROM pg_authid
WHERE rolname LIKE 'admin%';
 rolname | admcreaterole | admcanlogin 
---------+---------------+-------------
 admin   | t             | t
 admin2  | f             | f
(2 rows)

Is that intentional? If it is, I think it would be nice if this could be
changed, unless I'm missing some serious security concerns or so. 

Some light review of the patch (I haven't read all the previous ones, so
please excuse me if I rehash old discussions):

> From 82d235b39b32ca0cd0b94d47a54ee6806645a365 Mon Sep 17 00:00:00 2001
> From: Mark Dilger <mark.dilger@enterprisedb.com>
> Date: Fri, 28 Jan 2022 07:57:57 -0800
> Subject: [PATCH v8] Adding admin options for role attributes
> 
> When creating roles, attributes such as BYPASSRLS can be optionally
> specified WITH ADMIN OPTION or WITHOUT ADMIN OPTION.  If these
> optional clauses are unspecified, they all default to WITHOUT
> unless the role being created is given CREATEROLE, in which case
> they default to WITHOUT for SUPERUSER, REPLICATION, and BYPASSRLS
> and true for all others.  This preserves backwards compatible
> behavior.
> 
> The CREATEROLE attribute no longer makes up for lacking the ADMIN
> option on a role.  The creator of a role only has the ADMIN-like
> right to grant other roles into the new role during the creation
> statement itself.  After that, the creator may only do so if the
> creator has ADMIN on the role.  Note that creators may add
> themselves to the list of ADMINs on the new role during creation
> time.
> 
> SUPERUSER can still only be granted by superusers.
> ---
>  doc/src/sgml/ref/create_role.sgml         |  50 ++--
>  src/backend/catalog/aclchk.c              | 179 ++++++++++++--
>  src/backend/commands/user.c               | 278 +++++++++++++++++-----
>  src/backend/parser/gram.y                 | 161 ++++++++++---
>  src/include/catalog/pg_authid.dat         |  52 +++-
>  src/include/catalog/pg_authid.h           |  10 +
>  src/include/nodes/nodes.h                 |   1 +
>  src/include/nodes/parsenodes.h            |  11 +-
>  src/include/utils/acl.h                   |  12 +
>  src/test/regress/expected/create_role.out | 188 ++++++++++++++-
>  src/test/regress/expected/privileges.out  |   4 +
>  src/test/regress/sql/create_role.sql      | 153 +++++++++++-
>  src/test/regress/sql/privileges.sql       |   3 +
>  src/tools/pgindent/typedefs.list          |   1 +
>  14 files changed, 936 insertions(+), 167 deletions(-)
> 
> diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
> index b6a4ea1f72..7163779e0a 100644
> --- a/doc/src/sgml/ref/create_role.sgml
> +++ b/doc/src/sgml/ref/create_role.sgml
> @@ -26,15 +26,22 @@ CREATE ROLE <replaceable class="parameter">name</replaceable> [ [ WITH ] <replac
>  <phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
>  
>        SUPERUSER | NOSUPERUSER
> -    | CREATEDB | NOCREATEDB
> -    | CREATEROLE | NOCREATEROLE
> -    | INHERIT | NOINHERIT
> -    | LOGIN | NOLOGIN
> -    | REPLICATION | NOREPLICATION
> -    | BYPASSRLS | NOBYPASSRLS
> -    | CONNECTION LIMIT <replaceable class="parameter">connlimit</replaceable>
> -    | [ ENCRYPTED ] PASSWORD '<replaceable class="parameter">password</replaceable>' | PASSWORD NULL
> -    | VALID UNTIL '<replaceable class="parameter">timestamp</replaceable>'
> +    | INHERIT [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOINHERIT [ { WITH | WITHOUT } GRANT OPTION ]

Spaces vs. tabs here...

> +    | CREATEDB [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOCREATEDB [ { WITH | WITHOUT } GRANT OPTION ]
> +    | CREATEROLE [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOCREATEROLE [ { WITH | WITHOUT } GRANT OPTION ]
> +    | LOGIN [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOLOGIN [ { WITH | WITHOUT } GRANT OPTION ]
> +    | REPLICATION [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOREPLICATION [ { WITH | WITHOUT } GRANT OPTION ]
> +    | BYPASSRLS [ { WITH | WITHOUT } GRANT OPTION ]
> +    | NOBYPASSRLS [ { WITH | WITHOUT } GRANT OPTION ]
> +    | CONNECTION LIMIT [ <replaceable class="parameter">connlimit</replaceable> | NONE ] [ { WITH | WITHOUT } GRANT
OPTION]
 
> +    | [ ENCRYPTED ] PASSWORD '<replaceable class="parameter">password</replaceable>' [ { WITH | WITHOUT } GRANT
OPTION]
 
> +    | PASSWORD NULL [ { WITH | WITHOUT } GRANT OPTION ]

... and here, is that intentional?

> @@ -356,6 +363,18 @@ in sync when changing the above synopsis!
>     <link linkend="sql-revoke"><command>REVOKE</command></link>.
>    </para>
>  
> +  <para>
> +   Some parameters allow the <literal>WITH ADMIN OPTION</literal> or
> +   <literal>WITHOUT ADMIN OPTION</literal> clause to be specified.  For roles
> +   with the <literal>CREATEROLE</literal> attribute, these clauses govern
> +   whether new roles may be created with the attribute.  If not given, for
> +   reasons of backwards compatibility, <literal>WITHOUT ADMIN OPTION</literal>
> +   is the default for <literal>REPLICATION</literal> and
> +   <literal>BYPASSRLS</literal>, but <literal>WITH ADMIN OPTION</literal> is
> +   the default for <literal>CREATEDB</literal>, <literal>CREATEROLE</literal>,
> +   and <literal>LOGIN</literal>.
> +  </para>
> +
>    <para>
>     The <literal>VALID UNTIL</literal> clause defines an expiration time for a
>     password only, not for the role per se.  In
> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> index 1dd03a8e51..c66f545f36 100644
> --- a/src/backend/catalog/aclchk.c
> +++ b/src/backend/catalog/aclchk.c
> @@ -5430,6 +5430,91 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid)
>      return has_privs_of_role(roleid, ownerId);
>  }
>  
> +typedef enum ROLPRIV

I think typdefs usually go at the top of the file, not at line 5441...

> +{
> +    CREATEROLE,
> +    CREATEDB,
> +    CANLOGIN,
> +    REPLICATION,
> +    BYPASSRLS,
> +    INHERIT,
> +    CONNLIMIT,
> +    VALIDUNTIL,
> +    PASSWORD
> +} ROLPRIV;
> +

[...]

>  /*
>   * Check whether specified role has CREATEROLE privilege (or is a superuser)
>   *

I feel this function comment needs revision; we now have a dozen similar
functions that all do the same, but only the first one
(has_createrole_privilege) is being explained.

I guess the comment overall is still applicable, so as a minimum maybe
just change the CREATEROLE above for a generic "has some privilege", and
add a space in order to make it clear this applies to all of the
following functions.

Hrm, maybe also mention why there may_admin_*_privilege for all
privileges, but has_*_privilege only for some.

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> index f9d3c1246b..501613a840 100644
> --- a/src/backend/commands/user.c
> +++ b/src/backend/commands/user.c
> @@ -255,27 +305,36 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                       errmsg("must be superuser to create superusers")));
>      }
> -    else if (isreplication)
> -    {
> -        if (!superuser())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("must be superuser to create replication users")));
> -    }
> -    else if (bypassrls)
> -    {
> -        if (!superuser())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("must be superuser to create bypassrls users")));
> -    }
> -    else
> -    {
> -        if (!have_createrole_privilege())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("permission denied to create role")));
> -    }
> +
> +    if (createrole && !may_admin_createrole_privilege(GetUserId()))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                 errmsg("must have grant option on CREATEROLE privilege to create createrole users")));

Shouldn't this (and the following) be "must have admin option on
CREATEROLE"?

> @@ -311,7 +370,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>                          stmt->role)));
>  
>      /* Convert validuntil to internal form */
> -    if (validUntil)
> +    if (validUntil && strcmp(validUntil, "always") != 0)

This (there are other similar hunks further down) looks like an
independent patch/feature?

> @@ -637,32 +727,57 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
>                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>                       errmsg("must be superuser to alter superuser roles or change superuser attribute")));
>      }
> -    else if (authform->rolreplication || disreplication)
> -    {
> -        if (!superuser())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("must be superuser to alter replication roles or change replication attribute")));
> -    }
> -    else if (dbypassRLS)
> -    {
> -        if (!superuser())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("must be superuser to change bypassrls attribute")));
> -    }
> -    else if (!have_createrole_privilege())
> -    {
> -        /* check the rest */
> -        if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
> -            drolemembers || dvalidUntil || !dpassword || roleid != GetUserId())
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("permission denied")));
> -    }
> +
> +    /* To mess with replication roles, must have admin on REPLICATION */
> +    if ((authform->rolreplication || disreplication) &&
> +        !may_admin_replication_privilege(GetUserId()))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                 errmsg("must have admin on replication to alter replication roles or change replication
attribute")));

"have admin" sounds a bit weird, but I understand the error message is
too long already to spell out "must have admin option"? Or am I mistaken
and "admin" is what it's actually called (same for the ones below)?

Also, I think those role options are usually capitalized like
REPLICATION in other error messages.

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index b5966712ce..7503d3ead6 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -1131,67 +1140,111 @@ AlterOptRoleElem:
[...]

> +            | VALID ALWAYS opt_admin_spec
> +                {
> +                    RoleElem *n = makeNode(RoleElem);
> +                    n->elem = makeDefElem("validUntil", (Node *)makeString("always"), @1);
> +                    n->admin_spec = $3;
> +                    $$ = (Node *)n;

This one is from another patch as well I think.

>                  }
>          /*    Supported but not documented for roles, for use by ALTER GROUP. */
> -            | USER role_list
> +            | USER role_list opt_admin_spec
>                  {
> -                    $$ = makeDefElem("rolemembers", (Node *)$2, @1);
> +                    RoleElem *n = makeNode(RoleElem);
> +                    n->elem = makeDefElem("rolemembers", (Node *)$2, @1);
> +                    n->admin_spec = $3;
> +                    $$ = (Node *)n;
>                  }
> -            | IDENT
> +            | IDENT opt_admin_spec
>                  {
>                      /*
>                       * We handle identifiers that aren't parser keywords with
>                       * the following special-case codes, to avoid bloating the
>                       * size of the main parser.
>                       */
> +                    RoleElem *n = makeNode(RoleElem);
> +
> +                    /*
> +                     * Record whether the user specified WITH GRANT OPTION.

WITH ADMIN OPTION rather?

> +                     * Note that for some privileges this is always implied,
> +                     * such as SUPERUSER, but we don't reflect that here.
> +                     */
> +                    n->admin_spec = $2;
> +

> diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
> index 6c28119fa1..4829a6dbd2 100644
> --- a/src/include/catalog/pg_authid.dat
> +++ b/src/include/catalog/pg_authid.dat
> @@ -22,67 +22,93 @@
>  { oid => '10', oid_symbol => 'BOOTSTRAP_SUPERUSERID',
>    rolname => 'POSTGRES', rolsuper => 't', rolinherit => 't',
>    rolcreaterole => 't', rolcreatedb => 't', rolcanlogin => 't',
> -  rolreplication => 't', rolbypassrls => 't', rolconnlimit => '-1',
> +  rolreplication => 't', rolbypassrls => 't', adminherit => 't', admcreaterole => 't',
> +  admcreatedb => 't', admcanlogin => 't', admreplication => 't', admbypassrls => 't',
> +  admconnlimit => 't', admpassword => 't', admvaliduntil => 't', rolconnlimit => '-1',
>    rolpassword => '_null_', rolvaliduntil => '_null_' },

Those sure are a couple of new columns in pg_authid, but oh well...

> diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
> index 4b65e39a1f..4acdcaa685 100644
> --- a/src/include/catalog/pg_authid.h
> +++ b/src/include/catalog/pg_authid.h
> @@ -39,6 +39,16 @@ CATALOG(pg_authid,1260,AuthIdRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_OID(284
>      bool        rolcanlogin;    /* allowed to log in as session user? */
>      bool        rolreplication; /* role used for streaming replication */
>      bool        rolbypassrls;    /* bypasses row-level security? */
> +
> +    bool        adminherit;        /* allowed to administer inherit? */
> +    bool        admcreaterole;    /* allowed to administer createrole? */
> +    bool        admcreatedb;    /* allowed to administer createdb?? */
> +    bool        admcanlogin;    /* allowed to administer login? */
> +    bool        admreplication; /* allowed to administer replication? */
> +    bool        admbypassrls;    /* allowed to administer bypassesrls? */
> +    bool        admconnlimit;    /* allowed to administer connlimit? */
> +    bool        admpassword;    /* allowed to administer password? */
> +    bool        admvaliduntil;    /* allowed to administer validuntil? */
>      int32        rolconnlimit;    /* max connections allowed (-1=no limit) */

It's cosmetic, but the space between rolbypassrls and adminherit is
maybe not needed, and I'd put rolconnlimit first (even though it has a
different type).


Michael

-- 
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz