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

Hi,

Am Sonntag, dem 30.01.2022 um 17:11 -0800 schrieb Mark Dilger:
> > On Jan 30, 2022, at 2:38 PM, Michael Banck <   
> > michael.banck@credativ.de> wrote:
> > > The attached WIP patch attempts to solve most of the CREATEROLE
> 
> I'm mostly looking for whether the general approach in this Work In
> Progress patch is acceptable, so I was a bit sloppy with whitespace
> and such....

Ok, sure. I think this topic is hugely important and as I read the
patch anyway, I added some comments, but yeah, we need to figure out
the fundamentals first.
> 

> > 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. 
> 
> It's intentional, but part of what I wanted review comments about. 
> The issue is that historically:
> 
>   CREATE USER michael CREATEROLE
> 
> meant that you could go on to do things like create users with LOGIN
> privilege.  I could take that away, which would be a backwards
> compatibility break, or I can do the weird thing this patch does.  Or
> I could have your
> 
>   ALTER ROLE admin2 CREATEROLE;
> 
> also grant the other privileges like LOGIN unless you explicitly say
> otherwise with a bunch of explicit WITHOUT ADMIN OPTION clauses. 
> Finding out which of those this is preferred was a big part of why I
> put this up for review.  Thanks for calling it out in under 24 hours!

Ok, so what I would have needed to do in the above in order to have
"admin2" and "admin" be the same as far as creating login users is (I
believe):

ALTER ROLE admin2 CREATEROLE LOGIN WITH ADMIN OPTION;

I think if possible, it would be nice to just have this part as default
if possible. I.e. CREATEROLE and HASLOGIN are historically so much
intertwined that I think the above should be implicit (again, if that
is possible); I don't care and/or haven't made up my mind about any of
the other options so far...

Ok, so now that I had another look, I see we are going down Pandora's
box: For any of the other things a role admin would like to do (change
password, change conn limit), one would have to go with this weird
disconnect between CREATE USER admin CREATEROLE and ALTER USER admin2
CREATEROLE [massive list of WITH ADMIN OPTION], and then I'm not sure
where we stop.

By the way, is there now even a way to add admpassword to a role after
it got created?

postgres=# SET ROLE admin2;
SET
postgres=> \password test
Enter new password for user "test": 
Enter it again: 
ERROR:  must have admin on password to change password attribute
postgres=> RESET ROLE;
RESET
postgres=# ALTER ROLE admin2 PASSWORD WITH ADMIN OPTION;
ERROR:  syntax error at or near "WITH"
UPDATE pg_authid SET admpassword = 't' WHERE rolname = 'admin2';
UPDATE 1
postgres=# SET ROLE admin2;
SET
postgres=> \password test
Enter new password for user "test": 
Enter it again: 
postgres=> 

However, the next thing is:

postgres=# SET ROLE admin;
SET
postgres=> CREATE GROUP testgroup;
CREATE ROLE
postgres=> GRANT testgroup TO test;
ERROR:  must have admin option on role "testgroup"

First off, what does "admin option" mean on a role?

I then tried this:

postgres=# CREATE USER admin3 CREATEROLE WITH ADMIN OPTION;
CREATE ROLE
postgres=# SET ROLE admin3;
SET
postgres=> CREATE USER test3;
CREATE ROLE
postgres=> CREATE GROUP testgroup3;
CREATE ROLE
postgres=> GRANT testgroup3 TO test3;
ERROR:  must have admin option on role "testgroup3"

So I created both user and group, I have the CREATEROLE priv (with or
without admin option), but I still can't assign the group. Is that
(tracking who created a role and letting the creator do more thing) the
part that got chopped away in your last patch in order to find a common
ground?

Is there now any way non-Superusers can assign groups to other users? I
feel this (next to creating users/groups) is the primary thing those
CREATEROLE admins are supposed to do/where doing up to now.


Again, sorry if this was all discussed previously, I only skimmed this
thread.

Two more comments regarding the code:

> > > 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...
> 
> Yes, that's also a big part of what people might object to.  I think
> it's a reasonable objection, but I don't know where else to put the
> information, given the lack of an aclitem[]?

Yeah, it crossed my mind that an array might not be bad. In any case,
if we can fix CREATEROLE for good, a couple of extra columns in
pg_authid might be a small price to pay.

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).
> 
> Oh, totally agree.  I had that blank there during development because
> the "rol..." and "adm..." all started to blur together.

The way the adm* privs are now somewhere in the middle of the rol*
privs also looks weird for the end-user and there does not seems to be
some greater scheme behind it:

postgres=# SELECT * FROM pg_authid WHERE rolname = 'admin' \gx 
-[ RECORD 1 ]--+------
oid            | 16385
rolname        | admin
rolsuper       | f
rolinherit     | t
rolcreaterole  | t
rolcreatedb    | f
rolcanlogin    | t
rolreplication | f
rolbypassrls   | f
adminherit     | t
admcreaterole  | t
admcreatedb    | t
admcanlogin    | t
admreplication | f
admbypassrls   | f
admconnlimit   | t
admpassword    | t
admvaliduntil  | t
rolconnlimit   | -1
rolpassword    | 
rolvaliduntil  | 


Michael

-- 
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
E-Mail: 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