Re: CREATEROLE and role ownership hierarchies
> On Dec 21, 2021, at 5:11 PM, Shinya Kato > wrote: > > I fixed the patches because they cannot be applied to HEAD. Thank you. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger wrote: > > > > > On Dec 21, 2021, at 5:11 PM, Shinya Kato > > wrote: > > > > I fixed the patches because they cannot be applied to HEAD. > > Thank you. I reviewed and tested these and they LGTM. FYI the rebased v3 patches upthread are raw diffs so git am won't apply them. I can add myself to the CF as a reviewer if it is helpful.
Re: CREATEROLE and role ownership hierarchies
On 10/20/21, 11:46 AM, "Mark Dilger" wrote: > The purpose of these patches is to fix the CREATEROLE escalation > attack vector misfeature. (Not everyone will see CREATEROLE that > way, but the perceived value of the patch set likely depends on how > much you see CREATEROLE in that light.) Regarding the "attack vector misfeature" comment, I remember being surprised when I first learned how much roles with CREATEROLE can do. When I describe CREATEROLE to others, I am sure to emphasize the note in the docs about such roles being "almost-superuser" roles. CREATEROLE is a rather big hammer at the moment, so I certainly think there is value in reducing its almost-superuser-ness. I mentioned this in the other thread [0] already, but the first thing that comes to mind when I look at these patches is how upgrades might work. Will we just make the bootstrap superuser the owner for all roles when you first upgrade to v15? Also, are we just going to strip the current CREATEROLE roles of much of their powers? Maybe it's worth keeping a legacy CREATEROLE role attribute for upgraded clusters that could eventually be removed down the road. I'd also like to bring up my note about allowing users to transfer role ownership. When I tested the patches earlier, REASSIGN OWNED BY was failing with an "unexpected classid" ERROR. Besides REASSIGN OWNED BY, perhaps there should be another mechanism for transferring ownership on a role-by-role basis (i.e., ALTER ROLE OWNER TO). I haven't looked at this new patch set too closely, so my apologies if this has already been added. Nathan [0] https://postgr.es/m/53C7DF4C-8463-4647-9DFD-779B5E1861C4%40amazon.com
Re: CREATEROLE and role ownership hierarchies
> On Oct 21, 2021, at 4:04 PM, Bossart, Nathan wrote: > > Regarding the "attack vector misfeature" comment, I remember being > surprised when I first learned how much roles with CREATEROLE can do. > When I describe CREATEROLE to others, I am sure to emphasize the note > in the docs about such roles being "almost-superuser" roles. > CREATEROLE is a rather big hammer at the moment, so I certainly think > there is value in reducing its almost-superuser-ness. It is hard to know how many people are using CREATEROLE currently. There isn't much reason to give it out, since if you care enough about security to not give out superuser, you probably care too much about security to give away CREATEROLE. > I mentioned this in the other thread [0] already, but the first thing > that comes to mind when I look at these patches is how upgrades might > work. Will we just make the bootstrap superuser the owner for all > roles when you first upgrade to v15? Yes, that's the idea. After upgrade, all roles will form a tree, with the bootstrap superuser at the root of the tree. The initial tree structure isn't very interesting, with all other roles directly owned by it, but from there the superuser can rearrange the tree, and after that non-superuser roles can manage whatever subtree of roles they are the root of. > Also, are we just going to strip > the current CREATEROLE roles of much of their powers? Maybe it's > worth keeping a legacy CREATEROLE role attribute for upgraded clusters > that could eventually be removed down the road. The patch as written drastically reduces the power of the CREATEROLE attribute, in a non-backwards compatible way. I wondered if there would be complaints about that. If so, we could instead leave CREATEROLE alone, and create some other privileged role for the same thing, but it does start to look funny having a CREATEROLE privilege bit and also a privileged role named, perhaps, pg_can_create_roles. > I'd also like to bring up my note about allowing users to transfer > role ownership. When I tested the patches earlier, REASSIGN OWNED BY > was failing with an "unexpected classid" ERROR. Besides REASSIGN > OWNED BY, perhaps there should be another mechanism for transferring > ownership on a role-by-role basis (i.e., ALTER ROLE OWNER TO). I > haven't looked at this new patch set too closely, so my apologies if > this has already been added. Yes, I completely agree with you on that. Both REASSIGN OWNED BY and ALTER ROLE OWNER TO should work. I'll take a look at the patches and repost with any adjustments that I find necessary to make those work. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On 2021-10-21 03:40, Mark Dilger wrote: These patches have been split off the now deprecated monolithic "Delegating superuser tasks to new security roles" thread at [1]. The purpose of these patches is to fix the CREATEROLE escalation attack vector misfeature. (Not everyone will see CREATEROLE that way, but the perceived value of the patch set likely depends on how much you see CREATEROLE in that light.) Hi! Thank you for the patch. I too think that CREATEROLE escalation attack is problem. I have three comments. 1. Is there a function to check the owner of a role, it would be nice to be able to check with \du or pg_roles view. 2. Is it correct that REPLICATION/BYPASSRLS can be granted even if you are not a super user, but have CREATEROLE and REPLICATION/BYPASSRLS? 3. I think it would be better to have an "DROP ROLE [ IF EXISTS ] name [, ...] [CASCADE | RESTRICT]" like "DROP TABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]". What do you think? -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
> On Oct 25, 2021, at 10:09 PM, Shinya Kato > wrote: > > On 2021-10-21 03:40, Mark Dilger wrote: >> These patches have been split off the now deprecated monolithic >> "Delegating superuser tasks to new security roles" thread at [1]. >> The purpose of these patches is to fix the CREATEROLE escalation >> attack vector misfeature. (Not everyone will see CREATEROLE that way, >> but the perceived value of the patch set likely depends on how much >> you see CREATEROLE in that light.) > > Hi! Thank you for the patch. > I too think that CREATEROLE escalation attack is problem. > > I have three comments. > 1. Is there a function to check the owner of a role, it would be nice to be > able to check with \du or pg_roles view. No, but that is a good idea. > 2. Is it correct that REPLICATION/BYPASSRLS can be granted even if you are > not a super user, but have CREATEROLE and REPLICATION/BYPASSRLS? It is intentional, yes. Whether it is correct is up for debate, but I think it is. > 3. I think it would be better to have an "DROP ROLE [ IF EXISTS ] name [, > ...] [CASCADE | RESTRICT]" like "DROP TABLE [ IF EXISTS ] name [, ...] [ > CASCADE | RESTRICT ]". What do you think? I agree it would be nice to have, but roles are cluster-global and there are technical difficulties in cascading into multiple databases to drop all objects owned by the role. There was also a debate [1] about whether we would even want such behavior, leading to no real conclusion regarding how or if such a command should be implemented. The current solution is to run REASSIGN OWNED in each database where the role owns objects before running DROP ROLE. At that point, the CASCADE option (not implemented) won't be needed. Of course, I need to post the next revision of this patch set addressing the deficiencies that Nathan pointed out upthread to make that work. [1] https://www.postgresql.org/message-id/flat/20211005025746.GN20998%40tamriel.snowman.net — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On 10/21/21 19:21, Mark Dilger wrote: >> Also, are we just going to strip >> the current CREATEROLE roles of much of their powers? Maybe it's >> worth keeping a legacy CREATEROLE role attribute for upgraded clusters >> that could eventually be removed down the road. > The patch as written drastically reduces the power of the CREATEROLE > attribute, in a non-backwards compatible way. I wondered if there would be > complaints about that. If so, we could instead leave CREATEROLE alone, and > create some other privileged role for the same thing, but it does start to > look funny having a CREATEROLE privilege bit and also a privileged role > named, perhaps, pg_can_create_roles. Give that CREATEROLE currently just about amounts to being a superuser, maybe there should be a pg_upgrade option to convert CREATEROLE to SUPERUSER. I don't want to perpetuate the misfeature though, so let's just bring it to an end. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On 2021-10-28 07:21, Mark Dilger wrote:
On Oct 25, 2021, at 10:09 PM, Shinya Kato
wrote:
Hi! Thank you for the patch.
I too think that CREATEROLE escalation attack is problem.
I have three comments.
1. Is there a function to check the owner of a role, it would be nice
to be able to check with \du or pg_roles view.
No, but that is a good idea.
These two ideas are implemented in v2. Both \du and pg_roles show the
owner information.
Thank you. It seems good to me.
By the way, I got the following execution result.
I was able to add the membership of a role with a different owner.
In brief, "a" was able to change the membership of owner "shinya".
Is this the correct behavior?
---
postgres=# CREATE ROLE a LOGIN;
CREATE ROLE
postgres=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
postgres=# CREATE ROLE b;
CREATE ROLE
postgres=# \du a
List of roles
Role name | Owner | Attributes | Member of
---+++-
a | shinya || {pg_execute_server_program}
postgres=# \du b
List of roles
Role name | Owner | Attributes | Member of
---++--+---
b | shinya | Cannot login | {}
postgres=# \c - a
You are now connected to database "postgres" as user "a".
postgres=> GRANT pg_execute_server_program TO b;
GRANT ROLE
postgres=> \du b
List of roles
Role name | Owner | Attributes | Member of
---++--+-
b | shinya | Cannot login | {pg_execute_server_program}
---
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
> On Oct 27, 2021, at 7:32 PM, Shinya Kato
> wrote:
>
> I was able to add the membership of a role with a different owner.
> In brief, "a" was able to change the membership of owner "shinya".
> Is this the correct behavior?
I believe it is required for backwards compatibility. In a green field, we
might consider doing things differently.
The only intentional backward compatibility break in this patch set is the the
behavior of CREATEROLE. The general hope is that such a compatibility break
will help far more than it hurts, as CREATEROLE does not appear to be a well
adopted feature. I would expect that breaking the behavior of the WITH ADMIN
OPTION feature would cause a lot more pain.
Trying your example on both the unpatched and the patched sources, things
appear to work as they should:
UNPATCHED
--
mark.dilger=# CREATE ROLE a LOGIN;
CREATE ROLE
mark.dilger=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
mark.dilger=# CREATE ROLE b;
CREATE ROLE
mark.dilger=# \du+ a
List of roles
Role name | Attributes | Member of | Description
---++-+-
a || {pg_execute_server_program} |
mark.dilger=# \du+ b
List of roles
Role name | Attributes | Member of | Description
---+--+---+-
b | Cannot login | {}|
mark.dilger=# \c - a
You are now connected to database "mark.dilger" as user "a".
mark.dilger=> GRANT pg_execute_server_program TO b;
GRANT ROLE
mark.dilger=> \du+ b
List of roles
Role name | Attributes | Member of | Description
---+--+-+-
b | Cannot login | {pg_execute_server_program} |
mark.dilger=> \du+ "mark.dilger"
List of roles
Role name | Attributes |
Member of | Description
-++---+-
mark.dilger | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
|
PATCHED:
---
mark.dilger=# CREATE ROLE a LOGIN;
CREATE ROLE
mark.dilger=# GRANT pg_execute_server_program TO a WITH ADMIN OPTION;
GRANT ROLE
mark.dilger=# CREATE ROLE b;
CREATE ROLE
mark.dilger=# \du+ a
List of roles
Role name |Owner| Attributes | Member of |
Description
---+-++-+-
a | mark.dilger || {pg_execute_server_program} |
mark.dilger=# \du+ b
List of roles
Role name |Owner| Attributes | Member of | Description
---+-+--+---+-
b | mark.dilger | Cannot login | {}|
mark.dilger=# \c - a
You are now connected to database "mark.dilger" as user "a".
mark.dilger=> GRANT pg_execute_server_program TO b;
GRANT ROLE
mark.dilger=> \du+ b
List of roles
Role name |Owner| Attributes | Member of |
Description
---+-+--+-+-
b | mark.dilger | Cannot login | {pg_execute_server_program} |
mark.dilger=> \du+ "mark.dilger"
List of roles
Role name |Owner| Attributes
| Member of | Description
-+-++---+-
mark.dilger | mark.dilger | Superuser, Create role, Create DB, Replication,
Bypass RLS | {}|
You should notice that the owner of role "b" is the superuser "mark.dilger",
and that owner's attributes are unchanged. But your point that role "a" can
change the attributes of role "mark.dilger" is correct, as shown here:
mark.dilger=> GRANT pg_execute_server_program TO "mark.dilger";
GRANT ROLE
mark.dilger=> \du+ "mark.dilger"
List of roles
Role name |Owner| Attributes
| Member of | Description
-+-++-+-
mark.dilger | mark.dilger | Superuser, Create role, Create DB, Replication,
Bypass RLS | {pg_execute_server_program} |
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
Mark Dilger writes: > The only intentional backward compatibility break in this patch set is the > the behavior of CREATEROLE. The general hope is that such a compatibility > break will help far more than it hurts, as CREATEROLE does not appear to be a > well adopted feature. I would expect that breaking the behavior of the WITH > ADMIN OPTION feature would cause a lot more pain. Even more to the point, WITH ADMIN OPTION is defined by the SQL standard. The only way you get to mess with that is if you can convince people we mis-implemented the standard. regards, tom lane
Re: CREATEROLE and role ownership hierarchies
On 2021-10-29 01:14, Tom Lane wrote: Mark Dilger writes: The only intentional backward compatibility break in this patch set is the the behavior of CREATEROLE. The general hope is that such a compatibility break will help far more than it hurts, as CREATEROLE does not appear to be a well adopted feature. I would expect that breaking the behavior of the WITH ADMIN OPTION feature would cause a lot more pain. Even more to the point, WITH ADMIN OPTION is defined by the SQL standard. The only way you get to mess with that is if you can convince people we mis-implemented the standard. Thank you for the detailed explanation. I now understand what you said. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
On 2021-10-28 07:21, Mark Dilger wrote:
On Oct 25, 2021, at 10:09 PM, Shinya Kato
wrote:
Hi! Thank you for the patch.
I too think that CREATEROLE escalation attack is problem.
I have three comments.
1. Is there a function to check the owner of a role, it would be nice
to be able to check with \du or pg_roles view.
No, but that is a good idea.
These two ideas are implemented in v2. Both \du and pg_roles show the
owner information.
The current solution is to run REASSIGN OWNED in each database where
the role owns objects before running DROP ROLE. At that point, the
CASCADE option (not implemented) won't be needed. Of course, I need
to post the next revision of this patch set addressing the
deficiencies that Nathan pointed out upthread to make that work.
REASSIGN OWNED and ALTER ROLE..OWNER TO now work in v2.
When ALTER ROLE with the privilege of REPLICATION, only the superuser is
checked.
Therefore, we have a strange situation where we can create a role but
not change it.
---
postgres=> SELECT current_user;
current_user
--
test
(1 row)
postgres=> \du test
List of roles
Role name | Owner |Attributes| Member of
---++--+---
test | shinya | Create role, Replication | {}
postgres=> CREATE ROLE test2 REPLICATION;
CREATE ROLE
postgres=> ALTER ROLE test2 NOREPLICATION;
2021-11-04 14:24:02.687 JST [2615016] ERROR: must be superuser to alter
replication roles or change replication attribute
2021-11-04 14:24:02.687 JST [2615016] STATEMENT: ALTER ROLE test2
NOREPLICATION;
ERROR: must be superuser to alter replication roles or change
replication attribute
---
Wouldn't it be better to check if the role has CREATEROLE and
REPLICATION?
The same is true for BYPASSRLS.
By the way, is this thread registered to CommitFest?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
The cfbot is testing the last patch posted to this thread which is the remove-self-own patch which was already committed. I gather that there's still (at least one) patch under discussion. Could I suggest reposting the last version of the main patch, perhaps rebasing it. That way the cfbot would at least continue to test for conflicts.
Re: CREATEROLE and role ownership hierarchies
On Fri, Apr 1, 2022 at 10:46 AM Greg Stark wrote: > The cfbot is testing the last patch posted to this thread which is the > remove-self-own patch which was already committed. I gather that > there's still (at least one) patch under discussion. > > Could I suggest reposting the last version of the main patch, perhaps > rebasing it. That way the cfbot would at least continue to test for > conflicts. We should move this patch to the next CF or maybe even mark it returned with feedback. We're not going to get anything else done here for v15, and I'm not sure whether what we do beyond that will take this form or not. -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On 12/23/21 16:06, Joshua Brindle wrote: > On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger > wrote: >> >> >>> On Dec 21, 2021, at 5:11 PM, Shinya Kato >>> wrote: >>> >>> I fixed the patches because they cannot be applied to HEAD. >> Thank you. > I reviewed and tested these and they LGTM. FYI the rebased v3 patches > upthread are raw diffs so git am won't apply them. That's not at all unusual. I normally apply patches just using patch -p 1 < $patchfile > I can add myself to > the CF as a reviewer if it is helpful. Please do. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 3, 2022 at 5:08 PM Andrew Dunstan wrote:
>
>
> On 12/23/21 16:06, Joshua Brindle wrote:
> > On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger
> > wrote:
> >>
> >>
> >>> On Dec 21, 2021, at 5:11 PM, Shinya Kato
> >>> wrote:
> >>>
> >>> I fixed the patches because they cannot be applied to HEAD.
> >> Thank you.
> > I reviewed and tested these and they LGTM. FYI the rebased v3 patches
> > upthread are raw diffs so git am won't apply them.
>
>
> That's not at all unusual. I normally apply patches just using
>
>patch -p 1 < $patchfile
>
> > I can add myself to
> > the CF as a reviewer if it is helpful.
>
>
> Please do.
I just ran across this and I don't know if it is intended behavior or
not, can you tell me why this happens?
postgres=> \du+
List of roles
Role name | Owner | Attributes
| Member of | Description
---+--++---+-
brindle | brindle | Password valid until 2022-01-05 00:00:00-05
| {}|
joshua| postgres | Create role
| {}|
postgres | postgres | Superuser, Create role, Create DB,
Replication, Bypass RLS | {}|
postgres=> \password
Enter new password for user "brindle":
Enter it again:
ERROR: role "brindle" with OID 16384 owns itself
Re: CREATEROLE and role ownership hierarchies
> On Jan 4, 2022, at 6:35 AM, Joshua Brindle > wrote: > > I just ran across this and I don't know if it is intended behavior or > not > postgres=> \password > Enter new password for user "brindle": > Enter it again: > ERROR: role "brindle" with OID 16384 owns itself No, that looks like a bug. Thanks for reviewing! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
> On Jan 4, 2022, at 9:07 AM, Mark Dilger wrote: > > No, that looks like a bug. I was able to reproduce that using REASSIGN OWNED BY to cause a user to own itself. Is that how you did it, or is there yet another way to get into that state? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On Tue, Jan 4, 2022 at 3:39 PM Mark Dilger wrote: > > > > > On Jan 4, 2022, at 9:07 AM, Mark Dilger > > wrote: > > > > No, that looks like a bug. > > I was able to reproduce that using REASSIGN OWNED BY to cause a user to own > itself. Is that how you did it, or is there yet another way to get into that > state? I did: ALTER ROLE brindle OWNER TO brindle;
Re: CREATEROLE and role ownership hierarchies
On Wed, Jan 5, 2022 at 7:05 PM Mark Dilger wrote: > > On Jan 4, 2022, at 12:47 PM, Joshua Brindle > > wrote: > > > >> I was able to reproduce that using REASSIGN OWNED BY to cause a user to > >> own itself. Is that how you did it, or is there yet another way to get > >> into that state? > > > > I did: > > ALTER ROLE brindle OWNER TO brindle; > > Ok, thanks. I have rebased, fixed both REASSIGN OWNED BY and ALTER ROLE .. > OWNER TO cases, and added regression coverage for them. > > The last patch set to contain significant changes was v2, with v3 just being > a rebase. Relative to those sets: > > 0001 -- rebased. > 0002 -- rebased; extend AlterRoleOwner_internal to disallow making a role its > own immediate owner. > 0003 -- rebased; extend AlterRoleOwner_internal to disallow cycles in the > role ownership graph. > 0004 -- rebased. > 0005 -- new; removes the broken pg_auth_members.grantor field. > LGTM +1
Re: CREATEROLE and role ownership hierarchies
On 1/5/22 19:05, Mark Dilger wrote:
>
>> On Jan 4, 2022, at 12:47 PM, Joshua Brindle
>> wrote:
>>
>>> I was able to reproduce that using REASSIGN OWNED BY to cause a user to own
>>> itself. Is that how you did it, or is there yet another way to get into
>>> that state?
>> I did:
>> ALTER ROLE brindle OWNER TO brindle;
> Ok, thanks. I have rebased, fixed both REASSIGN OWNED BY and ALTER ROLE ..
> OWNER TO cases, and added regression coverage for them.
>
> The last patch set to contain significant changes was v2, with v3 just being
> a rebase. Relative to those sets:
>
> 0001 -- rebased.
> 0002 -- rebased; extend AlterRoleOwner_internal to disallow making a role its
> own immediate owner.
> 0003 -- rebased; extend AlterRoleOwner_internal to disallow cycles in the
> role ownership graph.
> 0004 -- rebased.
> 0005 -- new; removes the broken pg_auth_members.grantor field.
In general this looks good. Some nitpicks:
+/*
+ * Ownership check for a role (specified by OID)
+ */
+bool
+pg_role_ownercheck(Oid role_oid, Oid roleid)
This is a bit confusing. Let's rename these params so it's clear which
is the owner and which the owned role.
+ * Note: In versions prior to PostgreSQL version 15, roles did not have
owners
+ * per se; instead we used this test in places where an ownership-like
+ * permissions test was needed for a role.
No need to talk about what we used to do. People who want to know can
look back at older branches.
+bool
+has_rolinherit_privilege(Oid roleid)
+{
This and similar functions should have header comments.
+ /* Owners of roles have every privilge the owned role has */
s/privlge/privilege/
+CREATE ROLE regress_role_1 CREATEDB CREATEROLE REPLICATION BYPASSRLS;
I don't really like this business of just numbering large numbers of
roles in the tests. Let's give them more meaningful names.
+ Role owners can change any of these settings on roles they own except
I would say "on roles they directly or indirectly own", here and
similarly in one or two other places.
...
I will probably do one or two more passes over the patches, but as I say
in general they look fairly good.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
Greetings,
* Mark Dilger (mark.dil...@enterprisedb.com) wrote:
> > On Jan 4, 2022, at 12:47 PM, Joshua Brindle
> > wrote:
> >
> >> I was able to reproduce that using REASSIGN OWNED BY to cause a user to
> >> own itself. Is that how you did it, or is there yet another way to get
> >> into that state?
> >
> > I did:
> > ALTER ROLE brindle OWNER TO brindle;
>
> Ok, thanks. I have rebased, fixed both REASSIGN OWNED BY and ALTER ROLE ..
> OWNER TO cases, and added regression coverage for them.
>
> The last patch set to contain significant changes was v2, with v3 just being
> a rebase. Relative to those sets:
>
> 0001 -- rebased.
> 0002 -- rebased; extend AlterRoleOwner_internal to disallow making a role its
> own immediate owner.
> 0003 -- rebased; extend AlterRoleOwner_internal to disallow cycles in the
> role ownership graph.
> 0004 -- rebased.
> 0005 -- new; removes the broken pg_auth_members.grantor field.
> Subject: [PATCH v4 1/5] Add tests of the CREATEROLE attribute.
No particular issue with this one.
> Subject: [PATCH v4 2/5] Add owners to roles
>
> All roles now have owners. By default, roles belong to the role
> that created them, and initdb-time roles are owned by POSTGRES.
... database superuser, not 'POSTGRES'.
> +++ b/src/backend/catalog/aclchk.c
> @@ -5430,6 +5434,57 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid
> roleid)
> return has_privs_of_role(roleid, ownerId);
> }
>
> +/*
> + * Ownership check for a role (specified by OID)
> + */
> +bool
> +pg_role_ownercheck(Oid role_oid, Oid roleid)
> +{
> + HeapTuple tuple;
> + Form_pg_authid authform;
> + Oid owner_oid;
> +
> + /* Superusers bypass all permission checking. */
> + if (superuser_arg(roleid))
> + return true;
> +
> + /* Otherwise, look up the owner of the role */
> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid));
> + if (!HeapTupleIsValid(tuple))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("role with OID %u does not exist",
> + role_oid)));
> + authform = (Form_pg_authid) GETSTRUCT(tuple);
> + owner_oid = authform->rolowner;
> +
> + /*
> + * Roles must necessarily have owners. Even the bootstrap user has an
> + * owner. (It owns itself). Other roles must form a proper tree.
> + */
> + if (!OidIsValid(owner_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u has invalid
> owner",
> + authform->rolname.data,
> authform->oid)));
> + if (authform->oid != BOOTSTRAP_SUPERUSERID &&
> + authform->rolowner == authform->oid)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owns itself",
> + authform->rolname.data,
> authform->oid)));
> + if (authform->oid == BOOTSTRAP_SUPERUSERID &&
> + authform->rolowner != BOOTSTRAP_SUPERUSERID)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owned by role
> with OID %u",
> + authform->rolname.data,
> authform->oid,
> + authform->rolowner)));
> + ReleaseSysCache(tuple);
> +
> + return (owner_oid == roleid);
> +}
Do we really need all of these checks on every call of this function..?
Also, there isn't much point in including the role OID twice in the last
error message, is there? Unless things have gotten quite odd, it's
goint to be the same value both times as we just proved to ourselves
that it is, in fact, the same value (and that it's not the
BOOTSTRAP_SUPERUSERID).
This function also doesn't actually do any kind of checking to see if
the role ownership forms a proper tree, so it seems a bit odd to have
the comment talking about that here where it's doing other checks.
> +++ b/src/backend/commands/user.c
> @@ -77,6 +79,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> Datum new_record[Natts_pg_authid];
> boolnew_record_nulls[Natts_pg_authid];
> Oid roleid;
> + Oid owner_uid;
> + Oid saved_uid;
> + int save_sec_context;
Seems a bit odd to introduce 'uid' into this file, which hasn't got any
such anywhere in it, and I'm not entirely sure that any of these are
actually needed..?
> @@ -108,6 +113,16 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> DefElem*dvalidUntil = NULL;
> DefEl
Re: CREATEROLE and role ownership hierarchies
On 1/22/22 16:20, Stephen Frost wrote: >> Subject: [PATCH v4 1/5] Add tests of the CREATEROLE attribute. > No particular issue with this one. > > I'm going to commit this piece forthwith so we get it out of the way. That will presumably make the cfbot unhappy until Mark submits a new patch set. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On Sat, Jan 22, 2022 at 4:20 PM Stephen Frost wrote: > Whoah, really? No, I don't agree with this, it's throwing away the > entire concept around inheritance of role rights and how you can have > roles which you can get the privileges of by doing a SET ROLE to them > but you don't automatically have those rights. I see it differently. In my opinion, what that does is make the patch actually useful instead of largely a waste of time. If you are a service provider, you want to give your customers a super-user-like experience without actually making them superuser. You don't want to actually make them superuser, because then they could do things like change archive_command or install plperlu and shell out to the OS account, which you don't want. But you do want them to be able to administer objects within the database just as a superuser could. And a superuser has privileges over objects they own and objects belonging to other users automatically, without needing to SET ROLE. Imagine what happens if we adopt your proposal here. Everybody now has to understand the behavior of a regular account, the behavior of a superuser account, and the behavior of this third type of account which is sort of like a superuser but requires a lot more SET ROLE commands. And also every tool. So for example pg_dump and restore isn't going to work, not even on the set of objects this elevated-privilege user can access. pgAdmin isn't going to understand that it needs to insert a bunch of extra SET ROLE commands to administer objects. Ditto literally every other tool anyone has ever written to administer PostgreSQL. And for all of that pain, we get exactly zero extra security. -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On 1/24/22 15:33, Robert Haas wrote: > On Sat, Jan 22, 2022 at 4:20 PM Stephen Frost wrote: >> Whoah, really? No, I don't agree with this, it's throwing away the >> entire concept around inheritance of role rights and how you can have >> roles which you can get the privileges of by doing a SET ROLE to them >> but you don't automatically have those rights. > I see it differently. In my opinion, what that does is make the patch > actually useful instead of largely a waste of time. If you are a > service provider, you want to give your customers a super-user-like > experience without actually making them superuser. You don't want to > actually make them superuser, because then they could do things like > change archive_command or install plperlu and shell out to the OS > account, which you don't want. But you do want them to be able to > administer objects within the database just as a superuser could. And > a superuser has privileges over objects they own and objects belonging > to other users automatically, without needing to SET ROLE. > +many I encountered such issues on a cloud provider several years ago, and blogged about the difficulties, which would have been solved very nicely and cleanly by this proposal. It was when I understood properly how this proposal worked, precisely as Robert states, that I became more enthusiastic about it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
Greetings, On Mon, Jan 24, 2022 at 15:33 Robert Haas wrote: > On Sat, Jan 22, 2022 at 4:20 PM Stephen Frost wrote: > > Whoah, really? No, I don't agree with this, it's throwing away the > > entire concept around inheritance of role rights and how you can have > > roles which you can get the privileges of by doing a SET ROLE to them > > but you don't automatically have those rights. > > I see it differently. In my opinion, what that does is make the patch > actually useful instead of largely a waste of time. The idea behind this patch is to enable creation and dropping of roles, which isn’t possible now without being effectively a superuser. Forcing owners to also implicitly have all rights of the roles they create is orthogonal to that and an unnecessary change. If you are a > service provider, you want to give your customers a super-user-like > experience without actually making them superuser. You don't want to > actually make them superuser, because then they could do things like > change archive_command or install plperlu and shell out to the OS > account, which you don't want. But you do want them to be able to > administer objects within the database just as a superuser could. And > a superuser has privileges over objects they own and objects belonging > to other users automatically, without needing to SET ROLE. I am not saying that we would explicitly set all cases to be noninherit or that we would even change the default away from what it is today, only that we should use the existing role system and it’s concept of inherit-vs-noninherit rather than throwing all of that away. Everybody now has > to understand the behavior of a regular account, the behavior of a > superuser account, and the behavior of this third type of account > which is sort of like a superuser but requires a lot more SET ROLE > commands. Inherit vs. noninherit roles is not a new concept, it has existed since the role system was implemented. Further, that system does not require a lot of SET ROLE commands unless and until an admin sets up a non-inherit role. At that time, however, it’s expected that the rights of a role which has inherit set to false are not automatically allowed for the role to which it was GRANT’d. That’s how roles have always worked since they were introduced. And also every tool. So for example pg_dump and restore > isn't going to work, not even on the set of objects this > elevated-privilege user can access. pgAdmin isn't going to understand > that it needs to insert a bunch of extra SET ROLE commands to > administer objects. Ditto literally every other tool anyone has ever > written to administer PostgreSQL. And for all of that pain, we get > exactly zero extra security. We have an inherit system today and pg_dump works just fine, as far as I’m aware, and it does, indeed, issue SET ROLE at various points. Perhaps you could explain with PG today what the issue is that is caused? Or what issue pgAdmin has with PG’s existing role inherit system? Further, being able to require a SET ROLE before running a given operation is certainly a benefit in much the same way that having a user have to sudo before running an operation is. Thanks, Stephen >
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 24, 2022 at 4:23 PM Stephen Frost wrote: > The idea behind this patch is to enable creation and dropping of roles, which > isn’t possible now without being effectively a superuser. > > Forcing owners to also implicitly have all rights of the roles they create is > orthogonal to that and an unnecessary change. I just took a look at the first email on this thread and it says this: >>> These patches have been split off the now deprecated monolithic "Delegating >>> superuser tasks to new security roles" thread at [1]. Therefore I think it is pretty clear that the goals of this patch set include being able to delegate superuser tasks to new security roles. And having those tasks be delegated but *work randomly differently* is much less useful. > I am not saying that we would explicitly set all cases to be noninherit or > that we would even change the default away from what it is today, only that > we should use the existing role system and it’s concept of > inherit-vs-noninherit rather than throwing all of that away. INHERIT vs. NOINHERIT is documented to control the behavior of role *membership*. This patch is introducing a new concept of role *ownership*. It's not self-evident that what applies to one case should apply to the other. > Further, being able to require a SET ROLE before running a given operation is > certainly a benefit in much the same way that having a user have to sudo > before running an operation is. That's a reasonable point of view, but having things work similarly to what happens for a superuser is ALSO a very big benefit. In my opinion, in fact, it is a far larger benefit. -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
Greetings, On Mon, Jan 24, 2022 at 16:42 Robert Haas wrote: > On Mon, Jan 24, 2022 at 4:23 PM Stephen Frost wrote: > > The idea behind this patch is to enable creation and dropping of roles, > which isn’t possible now without being effectively a superuser. > > > > Forcing owners to also implicitly have all rights of the roles they > create is orthogonal to that and an unnecessary change. > > I just took a look at the first email on this thread and it says this: > > >>> These patches have been split off the now deprecated monolithic > "Delegating superuser tasks to new security roles" thread at [1]. > > Therefore I think it is pretty clear that the goals of this patch set > include being able to delegate superuser tasks to new security roles. > And having those tasks be delegated but *work randomly differently* is > much less useful. Being able to create and drop users is, in fact, effectively a superuser-only task today. We could throw out the entire idea of role ownership, in fact, as being entirely unnecessary when talking about that specific task. > I am not saying that we would explicitly set all cases to be noninherit > or that we would even change the default away from what it is today, only > that we should use the existing role system and it’s concept of > inherit-vs-noninherit rather than throwing all of that away. > > INHERIT vs. NOINHERIT is documented to control the behavior of role > *membership*. This patch is introducing a new concept of role > *ownership*. It's not self-evident that what applies to one case > should apply to the other. This is an argument to drop the role ownership concept, as I view it. Privileges are driven by membership today and inventing some new independent way to do that is increasing confusion, not improving things. I disagree that adding role ownership should necessarily change how the regular GRANT privilege system works or throw away basic concepts of that system which have been in place for decades. Increasing the number of independent ways to answer the question of “what users have what rights on object X” is an active bad thing. Anything that cares about object access will now also have to address role ownership to answer that question, while if we don’t include this one change then they don’t need to directly have any concern for ownership because regular object privileges still work the same way they did before. > Further, being able to require a SET ROLE before running a given > operation is certainly a benefit in much the same way that having a user > have to sudo before running an operation is. > > That's a reasonable point of view, but having things work similarly to > what happens for a superuser is ALSO a very big benefit. In my > opinion, in fact, it is a far larger benefit. Superuser is a problem specifically because it gives people access to do absolutely anything, both for security and safety concerns. Disallowing a way to curtail that same risk when it comes to role ownership invites exactly those same problems. I appreciate that there’s an edge between the ownership system being proposed and the existing role membership system, but we’d be much better off trying to minimize the amount that they end up overlapping- role ownership should be about managing roles. To push back on the original “tenant” argument, consider that one of the bigger issues in cloud computing today is exactly the problem that the cloud managers can potentially gain access to the sensitive data of their tenants and that’s not generally viewed as a positive thing. This change would make it so that every landlord can go and SELECT from the tables of their tenants without so much as a by-your-leave. The tenants likely don’t like that idea, and almost as likely the landlords in many cases aren’t thrilled with it either. Should the landlords be able to DROP the tenant due to the tenant not paying their bill? Of course, and that should then eliminate the tenant’s tables and other objects which take up resources, but that’s not the same thing as saying that a landlord should be able to unlock a tenant’s old phone that they left behind (and yeah, maybe the analogy falls apart a bit there, but the point I’m trying to get at is that it’s not as simple as it’s being made out to be here and we should think about these things and not just implicitly grant all access to the owner because that’s an easy thing to do- and is exactly what viewing owners as “mini superusers” does and leads to many of the same issues we already have with superusers). Thanks, Stephen >
Re: CREATEROLE and role ownership hierarchies
> On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > Being able to create and drop users is, in fact, effectively a superuser-only > task today. We could throw out the entire idea of role ownership, in fact, > as being entirely unnecessary when talking about that specific task. Wow, that's totally contrary to how I see this patch. The heart and soul of this patch is to fix the fact that CREATEROLE is currently overpowered. Everything else is gravy. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
> On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > Superuser is a problem specifically because it gives people access to do > absolutely anything, both for security and safety concerns. Disallowing a way > to curtail that same risk when it comes to role ownership invites exactly > those same problems. Before the patch, users with CREATEROLE can do mischief. After the patch, users with CREATEROLE can do mischief. The difference is that the mischief that can be done after the patch is a proper subset of the mischief that can be done before the patch. (Counter-examples highly welcome.) Specifically, I claim that before the patch, non-superuser "bob" with CREATEROLE can interfere with *any* non-superuser. After the patch, non-superuser "bob" with CREATEROLE can interfere with *some* non-superusers; specifically, with non-superusers he created himself, or which have had ownership transferred to him. Restricting the scope of bob's mischief is a huge win, in my view. The argument about whether owners should always implicitly inherit privileges from roles they own is a bit orthogonal to my point about mischief-making. Do we at least agree on the mischief-abatement aspect of this patch set? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On 2022/01/25 8:18, Mark Dilger wrote: On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: Superuser is a problem specifically because it gives people access to do absolutely anything, both for security and safety concerns. Disallowing a way to curtail that same risk when it comes to role ownership invites exactly those same problems. Before the patch, users with CREATEROLE can do mischief. After the patch, users with CREATEROLE can do mischief. The difference is that the mischief that can be done after the patch is a proper subset of the mischief that can be done before the patch. (Counter-examples highly welcome.) Specifically, I claim that before the patch, non-superuser "bob" with CREATEROLE can interfere with *any* non-superuser. After the patch, non-superuser "bob" with CREATEROLE can interfere with *some* non-superusers; specifically, with non-superusers he created himself, or which have had ownership transferred to him. Restricting the scope of bob's mischief is a huge win, in my view. +1 One of "mischiefs" I'm thinking problematic is that users with CREATEROLE can give any predefined role that they don't have, to other users including themselves. For example, users with CREATEROLE can give pg_execute_server_program to themselves and run any OS commands by COPY PROGRAM. This would be an issue when providing something like PostgreSQL cloud service that wants to prevent end users from running OS commands but allow them to create/drop roles. Does the proposed patch fix also this issue? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: CREATEROLE and role ownership hierarchies
> On Jan 24, 2022, at 10:55 PM, Fujii Masao wrote: > > +1 > > One of "mischiefs" I'm thinking problematic is that users with CREATEROLE can > give any predefined role that they don't have, to other users including > themselves. For example, users with CREATEROLE can give > pg_execute_server_program to themselves and run any OS commands by COPY > PROGRAM. This would be an issue when providing something like PostgreSQL > cloud service that wants to prevent end users from running OS commands but > allow them to create/drop roles. Does the proposed patch fix also this issue? Yes, the patch restricts CREATEROLE privilege from granting any privilege they themselves lack. There is a regression test in the patch set which demonstrates this. See src/test/regress/expected/create_role.out. The diffs from v6-0004-Restrict-power-granted-via-CREATEROLE.patch are quoted here for ease of viewing: --- ok, having CREATEROLE is enough to create roles in privileged roles +-- fail, having CREATEROLE is not enough to create roles in privileged roles CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data; +ERROR: must have admin option on role "pg_read_all_data" CREATE ROLE regress_write_all_data IN ROLE pg_write_all_data; +ERROR: must have admin option on role "pg_write_all_data" CREATE ROLE regress_monitor IN ROLE pg_monitor; +ERROR: must have admin option on role "pg_monitor" CREATE ROLE regress_read_all_settings IN ROLE pg_read_all_settings; +ERROR: must have admin option on role "pg_read_all_settings" CREATE ROLE regress_read_all_stats IN ROLE pg_read_all_stats; +ERROR: must have admin option on role "pg_read_all_stats" CREATE ROLE regress_stat_scan_tables IN ROLE pg_stat_scan_tables; +ERROR: must have admin option on role "pg_stat_scan_tables" CREATE ROLE regress_read_server_files IN ROLE pg_read_server_files; +ERROR: must have admin option on role "pg_read_server_files" CREATE ROLE regress_write_server_files IN ROLE pg_write_server_files; +ERROR: must have admin option on role "pg_write_server_files" CREATE ROLE regress_execute_server_program IN ROLE pg_execute_server_program; +ERROR: must have admin option on role "pg_execute_server_program" CREATE ROLE regress_signal_backend IN ROLE pg_signal_backend; +ERROR: must have admin option on role "pg_signal_backend" — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
> On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > To push back on the original “tenant” argument, consider that one of the > bigger issues in cloud computing today is exactly the problem that the cloud > managers can potentially gain access to the sensitive data of their tenants > and that’s not generally viewed as a positive thing. +1. This is a real problem. I have been viewing this problem as separate from the one which role ownership is intended to fix. Do you have a suggestion about how to tackle the problems together with less work than tackling them separately? > This change would make it so that every landlord can go and SELECT from the > tables of their tenants without so much as a by-your-leave. I would expect that is already true. A user with CREATEROLE can do almost everything. This patch closes some CREATEROLE related security problems, but not this one you mention. > The tenants likely don’t like that idea +1 > , and almost as likely the landlords in many cases aren’t thrilled with it > either. +1 > Should the landlords be able to DROP the tenant due to the tenant not paying > their bill? Of course, and that should then eliminate the tenant’s tables > and other objects which take up resources, but that’s not the same thing as > saying that a landlord should be able to unlock a tenant’s old phone that > they left behind (and yeah, maybe the analogy falls apart a bit there, but > the point I’m trying to get at is that it’s not as simple as it’s being made > out to be here and we should think about these things and not just implicitly > grant all access to the owner because that’s an easy thing to do- and is > exactly what viewing owners as “mini superusers” does and leads to many of > the same issues we already have with superusers). This is a pretty interesting argument. I don't believe it will work to do as you say unconditionally, as there is still a need to have CREATEROLE users who have privileges on their created roles' objects, even if for no other purpose than to be able to REASSIGN OWNED BY those objects before dropping roles. But maybe there is also a need to have CREATEROLE users who lack that privilege? Would that be a privilege bit akin to (but not the same as!) the INHERIT privilege? Should I redesign for something like that? I like that the current patch restricts CREATEROLE users from granting privileges they themselves lack. Would such a new privilege bit work the same way? Imagine that you, "stephen", have CREATEROLE but not this new bit, and you create me, "mark" as a tenant with CREATEROLE. Can you give me the bit? Or does the fact that you lack the bit mean you can't give it to me, either? Other suggestions? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 24, 2022 at 5:21 PM Stephen Frost wrote: > This is an argument to drop the role ownership concept, as I view it. > Privileges are driven by membership today and inventing some new independent > way to do that is increasing confusion, not improving things. I disagree > that adding role ownership should necessarily change how the regular GRANT > privilege system works or throw away basic concepts of that system which have > been in place for decades. Increasing the number of independent ways to > answer the question of “what users have what rights on object X” is an active > bad thing. Anything that cares about object access will now also have to > address role ownership to answer that question, while if we don’t include > this one change then they don’t need to directly have any concern for > ownership because regular object privileges still work the same way they did > before. It really feels to me like you just keep moving the goalposts. We started out with a conversation where Mark said he'd like to be able to grant permissions on GUCs to non-superusers.[1] You argued repeatedly that we really needed to do something about CREATEROLE [2,3,4]. Mark argued that this was an unrelated problem[5] but you argued that unless it were addressed, users would still be able to break out of the sandbox[6] which must mean either the OS user, or at least PostgreSQL users other than the ones they were supposed to be able to control. That led *directly* to the patch at hand, which solves the problem by inventing the notion of role ownership, so that you can distinguish the roles you can administer from the ones you drop. You are now proposing that we get rid of that concept, a concept that was added four months ago[7] as a direct response to your previous feedback. It's completely unfair to make an argument that results in the addition of a complex piece of machinery to a body of work that was initially on an only marginally related topic and then turn around and argue, quite close to the end of the release cycle, for the removal of that exact same mechanism. And your argument about whether the privileges should be able to be exercised without SET ROLE is also just completely baffling to me given the previous conversation. It seems 100% clear from the previous discussion that we were talking about service provider environments and trying to deliver a good user experience to "lead tenants" in such environments. Regardless of the technical details of how INHERIT or anything else work, an actual superuser would not be subject to a restriction similar to the one you're talking about, so arguing that it ought to be present here for some technical reason is placing technicalities ahead of what seemed at the time to be a shared goal. There's a perfectly good argument to be made that the superuser role should not work the way it does, but it's too late to relitigate that. And I can't imagine why any service provider would find any value in a new role that requires all of the extra push-ups you're trying to impose on it. I just can't shake the feeling that you're trying to redesign this patch out of (a) getting committed and (b) solving any of the problems it intends to solve, problems with which you largely seemed to agree. I assume that is not actually your intention, but I can't think of anything you'd be doing differently here if it were. [1] https://www.postgresql.org/message-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC%40enterprisedb.com [2] https://www.postgresql.org/message-id/20210726200542.GX20766%40tamriel.snowman.net [3] https://www.postgresql.org/message-id/20210726205433.GA20766%40tamriel.snowman.net [4] https://www.postgresql.org/message-id/20210823181351.GB17906%40tamriel.snowman.net [5] https://www.postgresql.org/message-id/92AA9A52-A644-42FE-B699-8ECAEE12E635%40enterprisedb.com [6] https://www.postgresql.org/message-id/20210823195130.GF17906%40tamriel.snowman.net [7] https://www.postgresql.org/message-id/67BB2F92-704B-415C-8D47-149327CA8F4B%40enterprisedb.com -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > Being able to create and drop users is, in fact, effectively a > > superuser-only task today. We could throw out the entire idea of role > > ownership, in fact, as being entirely unnecessary when talking about that > > specific task. > > Wow, that's totally contrary to how I see this patch. The heart and soul of > this patch is to fix the fact that CREATEROLE is currently overpowered. > Everything else is gravy. 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. 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. 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. * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > > > Superuser is a problem specifically because it gives people access to do > > absolutely anything, both for security and safety concerns. Disallowing a > > way to curtail that same risk when it comes to role ownership invites > > exactly those same problems. > > Before the patch, users with CREATEROLE can do mischief. After the patch, > users with CREATEROLE can do mischief. The difference is that the mischief > that can be done after the patch is a proper subset of the mischief that can > be done before the patch. (Counter-examples highly welcome.) > > Specifically, I claim that before the patch, non-superuser "bob" with > CREATEROLE can interfere with *any* non-superuser. After the patch, > non-superuser "bob" with CREATEROLE can interfere with *some* non-superusers; > specifically, with non-superusers he created himself, or which have had > ownership transferred to him. > > Restricting the scope of bob's mischief is a huge win, in my view. > > The argument about whether owners should always implicitly inherit privileges > from roles they own is a bit orthogonal to my point about mischief-making. > Do we at least agree on the mischief-abatement aspect of this patch set? I don't know how many bites at this particular apple we're going to get, but I doubt folks are going to be happy if we change our minds every release. Further, I suspect we'll be better off going too far in the direction of 'mischief reduction' than not far enough. If we restrict things too far then we can provide ways to add those things back, but it's harder to remove things we didn't take away. This particular case is even an oddity on that spectrum though- CREATEROLE users, today, don't have access to all the objects created by roles which they create. Yes, they can get such access if they go through some additional hoops, but that could then be caught by someone auditing the logs, a consideration that I don't think we appreciate enough today. * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 24, 2022, at 2:21 PM, Stephen Frost wrote: > > > > To push back on the original “tenant” argument, consider that one of the > > bigger issues in cloud computing today is exactly the problem that the > > cloud managers can potentially gain access to the sensitive data of their > > tenants and that’s not generally viewed as a positive thing. > > +1. This is a real problem. I have been viewing this problem as separate > from the one which role ownership is intended to fix. Do you have a > suggestion about how to tackle the problems together with less work than > tackling them separately? I don't know about less work or not, but in this particular case I was asking for a few lines to be removed from the patch. I can believe that doing so would create some issues in terms of the use-cases that you want to solve with this and if we agree on those being sensible cases to address then we'd need to implement something to address those, though it's also possibly not the case and maybe removing those few lines doesn't impact anything beyond then allowing owners to not automatically inherit the rights of the roles they own if they don't wish to. Instead of talking about those cases concretely though, it seems like we've shifted to abstractly talking about ownership and landlords. Maybe some of that is helpful, but it seems to increasingly be an area that's causing more division than helping to move forward towards a mutually agreeable result. > > This change would make it so that every landlord can go and SELECT from > > the tables of th
Re: CREATEROLE and role ownership hierarchies
> On Jan 25, 2022, at 12:44 PM, Stephen Frost wrote: > > As I mentioned in the patch review, having a particular bit set doesn't > necessarily mean you should be able to pass it on- the existing object > GRANT system distinguishes those two and it seems like we should too. > In other words, I'm saying that we should be able to explicitly say just > what privileges a CREATEROLE user is able to grant to some other role > rather than basing it on what that user themselves has. I like the way you are thinking, but I'm not sure I agree with the facts you are asserting. I agree that "CREATE ROLE.. ROLE .." differs from "CREATE ROLE .. ADMIN ..", and "GRANT..WITH GRANT OPTION" differs from "GRANT..", but those only cover privileges tracked in an aclitem array. The privileges CREATEDB, CREATEROLE, REPLICATION, and BYPASSRLS don't work that way. There isn't a with/without grant option distinction for them. So I'm forced to say that a role without those privileges must not give them away. I'd be happier if we could get rid of all privileges of that kind, leaving only those that can be granted with/without grant option, tracked in an aclitem, and use that to determine if the user creating the role can give them away. But that's a bigger redesign of the system. Just touching how CREATEROLE works entails backwards compatibility problems. I'd hate to try to change all these other things; we'd be breaking a lot more, and features that appear more commonly used. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
> On Jan 25, 2022, at 12:44 PM, Stephen Frost 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. > 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" -- 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. v8-0001-Adding-admin-options-for-role-attributes.patch.WIP Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
Hi, On Sat, Jan 29, 2022 at 09:58:38PM -0800, Mark Dilger wrote: > > On Jan 25, 2022, at 12:44 PM, Stephen Frost 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 discu
Re: CREATEROLE and role ownership hierarchies
> On Jan 30, 2022, at 2:38 PM, Michael Banck wrote:
>
> Hi,
Your review is greatly appreciated!
>> 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
> 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!
> Some light review of the patch (I haven't read all the previous ones, so
> please excuse me if I rehash old discussions):
Not a problem.
> Spaces vs. tabs here...
>
> ... and here, is that intentional?
> I think typdefs usually go at the top of the file, not at line 5441...
> I feel this function comment needs revision...
> Hrm, maybe also mention ...
All good comments, but I'm not doing code cleanup on this WIP patch just yet.
Forgive me.
> Shouldn't this (and the following) be "must have admin option on
> CREATEROLE"?
Yes, there may be other places where I failed to replace the verbiage "grant
option" with "admin option". Earlier drafts of the patch were using that
language. I wouldn't mind review comments on which language people thinks is
better.
>
>> @@ -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?
Part of the problem with the grammar introduced in this patch is that you are
not normally required to mention attributes like VALID UNTIL, but if you want
to change whether the created role gets WITH ADMIN OPTION, you have to. That
leaves the problem of what to do if you *only* want to specify the ADMIN part.
The grammar needs some sort of "dummy" value that intentionally has no effect,
but sets up for the WITH/WITHOUT ADMIN OPTION clause. I think I left a few
bits of cruft around like that. But what I'd really like to know is if people
think this sort of thing is even headed in the right direction? Are there
problems with SQL spec compliance? Does it just feel icky? I don't have any
pride-of-ownership in the grammar this WIP patch introduces. I just needed
something to put out there for people to attack/improve.
>> +
>> +/* 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)?
If it is the officially correct language, I arrived at it by accident. I
didn't take any time to wordsmith those error messages. Improvements welcome!
> Also, I think those role options are usually capit
Re: CREATEROLE and role ownership hierarchies
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.ba...@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 previou
Re: CREATEROLE and role ownership hierarchies
Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 25, 2022, at 12:44 PM, Stephen Frost 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. Alright. > > 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. Sure. > > 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. Yeah, we do need to have a way to determine who is allowed to drop roles and role ownership seems like it's one possible approach to that. > 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; Right, this was one of the approaches that I was thinking could work for managing role attributes and it's very similar to roles and the admin option for them. As I suggested at least once, another possible approach could be to have login users not be able to create roles but for them to be able to SET ROLE to a role which is able to create roles, and then, using your prior method, only allow the attributes which that role has to be able to be given to other roles. That essentially makes a role be a proxy for the per-attribute admin options. There's pros and cons for each approach and so I'm curious as to which you feel is the better approach? I get the feeling that you're more inclined to go with the approach of having an admin option for each role attribute (having written this WIP patch) but I'm not sure if that is because you contempltaed both and felt this was better for some reason or more because I wasn't explaining the other approach very well, or if there was some other reason. > -- 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" I would say not just privileged roles, but any roles that the user doesn't have admin rights on. > 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. Given the relative size of the changes we're talking about regarding CREATEROLE, I don't really think we need to stress about backwards compatibility too much. Thanks, Stephen signature.asc Description: PGP signature
Re: CREATEROLE and role ownership hierarchies
> On Jan 31, 2022, at 12:43 AM, Michael Banck wrote: > 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. Right. Perhaps some background on this patch series will help. The patch versions before v8 were creating an owner-owned relationship between the creator and the createe, and a lot of privileges were dependent on that ownership. Stephen objected that we were creating parallel tracks on which the privilege system was running; things like belonging to a role or having admin on a role were partially conflated with owning a role. He also objected that the pre-v8 patch sets allowed a creator role with the CREATEROLE privilege to give away any privilege the creator had, rather than needing to have GRANT or ADMIN option on the privilege being given. The v8-WIP patch is not a complete replacement for the pre-v8 patches. It's just a balloon I'm floating to try out candidate solutions to some of Stephen's objections. In the long run, I want the solution to Stephen's objections to not create problems for anybody who liked the way the pre-v8 patches worked (Robert, Andrew, and to some extent me.) In this WIP patch, for a creator to give *anything* away to a createe, the creator must have GRANT or ADMIN on the thing being given. That includes attributes like BYPASSRLS, CREATEDB, LOGIN, etc., and also ADMIN on any role the createe is granted into. I tried to structure things for backwards compatibility, considering which things roles with CREATEROLE could give away historically. It turns out they can give away most everything, but not SUPERUSER, BYPASSRLS, or REPLICATION. So I structured the default privileges for CREATEROLE to match. But I'm uncertain that design is any good, and your comments below suggest that you find it pretty hard to use. Part of the problem with trying to be backwards compatible is that we must break compatibility anyway, to address the problem that historically having CREATEROLE meant you effectively had ADMIN on all non-superuser roles. That's got to change. So in part I'm asking pgsql-hackers if partial backwards compatibility is worth the bother. If we don't go with backwards compatibility, then CREATEROLE would only allow you to create a new role, but not to give that role LOGIN, nor CREATEDB, etc. You'd need to also have admin option on those things. To create a role that can give those things away, you'd need to run something like: CREATE ROLE michael CREATEROLE WITH ADMIN OPTION-- can further give away "createrole" CREATEDB WITH ADMIN OPTION-- can further give away "createdb" LOGIN WITH ADMIN OPTION-- can further give away "login" NOREPLICATION WITHOUT ADMIN OPTION-- this would be implied anyway NOBYPASSRLS WITHOUT ADMIN OPTION-- this would be implied anyway CONNECTION LIMIT WITH ADMIN OPTION-- can specify connection limits PASSWORD WITH ADMIN OPTION-- can specify passwords VALID UNTIL WITH ADMIN OPTION-- can specify expiration (I'm on the fence about the phrase "WITH ADMIN OPTION" vs. the phrase "WITH GRANT OPTION".) Even then, when "michael" creates new roles, if he wants to be able to further administer those roles, he needs to remember to give himself ADMIN membership in that role at creation time. After the role is created, if he doesn't have ADMIN, he can't give it to himself. So, at create time, he needs to remember to do this: SET ROLE michael; CREATE ROLE mark ADMIN michael; But that's still a bit strange, because "ADMIN michael" means that michael can grant other roles membership in "mark", not that michael can, for example, change mark's password. If we don't want CREATEROLE to imply that you can mess around with arbitrary roles (rather than only roles that you created or have been transferred control over) then we need the concept of role ownership. This patch doesn't go that far, so for now, only superusers can do those things. Assuming some form of this patch is acceptable, the v9 series will resurrect some of the pre-v7 logic for role ownership and say that the owner can do those things. >>> 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 opt
Re: CREATEROLE and role ownership hierarchies
> On Jan 31, 2022, at 8:53 AM, Stephen Frost wrote: > > Yeah, we do need to have a way to determine who is allowed to drop > roles and role ownership seems like it's one possible approach to that. Which other ways are on the table? Having ADMIN on a role doesn't allow you to do that, but maybe it could? What else? >> 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; > > Right, this was one of the approaches that I was thinking could work for > managing role attributes and it's very similar to roles and the admin > option for them. As I suggested at least once, another possible > approach could be to have login users not be able to create roles but > for them to be able to SET ROLE to a role which is able to create roles, > and then, using your prior method, only allow the attributes which that > role has to be able to be given to other roles. I'm not sure how that works. If I have a group named "administrators" which as multiple attributes like BYPASSRLS and such, and user "stephen" is a member of "administrators", then stephen can not only give away bypassrls to new users but also has it himself. How is that an improvement? (I mean this as a question, not as criticism.) > That essentially makes > a role be a proxy for the per-attribute admin options. There's pros and > cons for each approach and so I'm curious as to which you feel is the > better approach? I get the feeling that you're more inclined to go with > the approach of having an admin option for each role attribute (having > written this WIP patch) but I'm not sure if that is because you > contempltaed both and felt this was better for some reason or more > because I wasn't explaining the other approach very well, or if there > was some other reason. I need more explanation of the other option you are contemplating. My apologies if I'm being thick-headed. >> -- 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" > > I would say not just privileged roles, but any roles that the user > doesn't have admin rights on. Yes, that's how it works. But this portion of the test is only checking the interaction between CREATEROLE and built-in privileged roles, hence the comment. >> 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. > > Given the relative size of the changes we're talking about regarding > CREATEROLE, I don't really think we need to stress about backwards > compatibility too much. Yeah, I'm leaning pretty strongly that way, too. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 31, 2022, at 8:53 AM, Stephen Frost wrote: > > Yeah, we do need to have a way to determine who is allowed to drop > > roles and role ownership seems like it's one possible approach to that. > > Which other ways are on the table? Having ADMIN on a role doesn't allow you > to do that, but maybe it could? What else? Supporting that through ADMIN is one option, another would be a 'DROPROLE' attribute, though we'd want a way to curtail that from being able to be used for just any role and that does lead down a path similar to ownership or just generally the concept that some roles have certain rights over certain other roles (whether you create them or not...). I do think there's a lot of value in being able to segregate certain rights- consider that you may want a role that's able to create other roles, perhaps grant them into some set of roles, can lock those roles (prevent them from logging in, maybe do a password reset, something like that), but which *isn't* able to drop those roles (and all their objects) as that's dangerous and mistakes can certainly happen, or be able to become that role because the creating role simply doesn't have any need to be able to do that (or desire to in many cases, as we discussed in the landlord-vs-tenant sub-thread). Naturally, you'd want *some* role to be able to drop that role (and one that doesn't have full superuser access) but that might be a role that's not able to create new roles or take over accounts. Separation of concerns and powers and all of that is what we want to be going for here, more generically, which is why I was opposed to the blanket "owners have all rights of all roles they own" implementation. That approach doesn't support the ability to have a relatively unprivileged role that's able to create other roles, which seems like a pretty important use-case for us to be considering. The terminology seems to also be driving us in a certain direction and I don't know that it's necessarily a good one. That is- the term 'owner' implies certain things and maybe that's where some of the objection to my argument that owners shouldn't necessarily have all rights of the roles they 'own' comes from (ok- I'll also put out there for general consideration that since we're talking about roles, and login roles are generally associated with people, that maybe 'owner' isn't a great term to use for this anyway ...). I feel like the 'owner' concept came from the way we have table owners and function owners and database owners today rather than from a starting point of what do we wish to specifically enable. Perhaps instead of starting from the 'owner' concept, we start from the question about the kinds of things we want roles to be able to do and perhaps that will help inform the terminology. - Create new roles - Drop an existing role - Drop objects which belong to a role - Lock existing roles - Change/reset the PW of existing roles - Give roles to other roles - Revoke access to some roles from other roles - Give select role attributes to a role - Revoke role attributes from a role - Traditional role-based access control (group memberships, SET ROLE) Certain of the above are already covered by the existing role membership system and with the admin option, though there's definitely an argument to be made as to if that is as capable as we'd like it to be (there's no way to, today at least, GRANT *just* the admin option, for example, and maybe that's something that it would actually be sensible to support). Perhaps there is a need to have a user who has all of the above capabilities and maybe that would be an 'owner' or 'manager', but as I tried to illustrate above, there's definitely use-cases for giving a role only some of the above capabilities rather than all of them together at once. > >> 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 > >>CREATEROL
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 31, 2022 at 1:50 PM Stephen Frost wrote: > > Greetings, > > * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > > On Jan 31, 2022, at 8:53 AM, Stephen Frost wrote: > > > Yeah, we do need to have a way to determine who is allowed to drop > > > roles and role ownership seems like it's one possible approach to that. > > > > Which other ways are on the table? Having ADMIN on a role doesn't allow > > you to do that, but maybe it could? What else? > > Supporting that through ADMIN is one option, another would be a > 'DROPROLE' attribute, though we'd want a way to curtail that from being > able to be used for just any role and that does lead down a path similar > to ownership or just generally the concept that some roles have certain > rights over certain other roles (whether you create them or not...). > > I do think there's a lot of value in being able to segregate certain > rights- consider that you may want a role that's able to create other > roles, perhaps grant them into some set of roles, can lock those roles > (prevent them from logging in, maybe do a password reset, something like > that), but which *isn't* able to drop those roles (and all their > objects) as that's dangerous and mistakes can certainly happen, or be > able to become that role because the creating role simply doesn't have > any need to be able to do that (or desire to in many cases, as we > discussed in the landlord-vs-tenant sub-thread). > This is precisely the use case I am trying to accomplish with this patchset, roughly: - An automated bot that creates users and adds them to the employees role - Bot cannot access any employee (or other roles) table data - Bot cannot become any employee - Bot can disable the login of any employee Yes there are attack surfaces around the fringes of login, etc but those can be mitigated with certificate authentication. My pg_hba would require any role in the employees role to use cert auth. This would adequately mitigate many threats while greatly enhancing user management. > Naturally, you'd want *some* role to be able to drop that role (and one > that doesn't have full superuser access) but that might be a role that's > not able to create new roles or take over accounts. I suspect some kind of web backend to handle manual user pruning. I don't expect Bot to automatically drop users because mistakes can happen, and disabling the login ability seems like an adequate tradeoff. > Separation of concerns and powers and all of that is what we want to be > going for here, more generically, which is why I was opposed to the > blanket "owners have all rights of all roles they own" implementation. > That approach doesn't support the ability to have a relatively > unprivileged role that's able to create other roles, which seems like a > pretty important use-case for us to be considering. Agreed. > The terminology seems to also be driving us in a certain direction and I > don't know that it's necessarily a good one. That is- the term 'owner' > implies certain things and maybe that's where some of the objection to > my argument that owners shouldn't necessarily have all rights of the > roles they 'own' comes from (ok- I'll also put out there for general > consideration that since we're talking about roles, and login roles are > generally associated with people, that maybe 'owner' isn't a great term > to use for this anyway ...). I feel like the 'owner' concept came from > the way we have table owners and function owners and database owners > today rather than from a starting point of what do we wish to > specifically enable. > > Perhaps instead of starting from the 'owner' concept, we start from the > question about the kinds of things we want roles to be able to do and > perhaps that will help inform the terminology. > > - Create new roles > - Drop an existing role > - Drop objects which belong to a role > - Lock existing roles > - Change/reset the PW of existing roles > - Give roles to other roles > - Revoke access to some roles from other roles > - Give select role attributes to a role > - Revoke role attributes from a role > - Traditional role-based access control (group memberships, SET ROLE) > > Certain of the above are already covered by the existing role membership > system and with the admin option, though there's definitely an argument > to be made as to if that is as capable as we'd like it to be (there's no > way to, today at least, GRANT *just* the admin option, for example, and > maybe that's something that it would actually be sensible to support). > > Perhaps there is a need to have a user who has all of the above > capabilities and maybe that would be an 'owner' or 'manager', but as I > tried to illustrate above, there's definitely use-cases for giving > a role only some of the above capabilities rather than all of them > together at once. > > > >> The main idea here is that having CREATEROLE doesn't give you ADMIN on > > >> roles, nor on role attributes. For role
Re: CREATEROLE and role ownership hierarchies
Hi, Am Montag, dem 31.01.2022 um 09:18 -0800 schrieb Mark Dilger: > On Jan 31, 2022, at 12:43 AM, Michael Banck < > michael.ba...@credativ.de> wrote: > 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. Right. Perhaps some background on this patch series will help. [...] Thanks a lot! If we don't go with backwards compatibility, then CREATEROLE would only allow you to create a new role, but not to give that role LOGIN, nor CREATEDB, etc. You'd need to also have admin option on those things. To create a role that can give those things away, you'd need to run something like: CREATE ROLE michael CREATEROLE WITH ADMIN OPTION -- can further give away "createrole" CREATEDB WITH ADMIN OPTION -- can further give away "createdb" LOGIN WITH ADMIN OPTION -- can further give away "login" NOREPLICATION WITHOUT ADMIN OPTION -- this would be implied anyway NOBYPASSRLS WITHOUT ADMIN OPTION -- this would be implied anyway CONNECTION LIMIT WITH ADMIN OPTION -- can specify connection limits PASSWORD WITH ADMIN OPTION -- can specify passwords VALID UNTIL WITH ADMIN OPTION -- can specify expiration Those last three don't work for me: postgres=# CREATE ROLE admin3 VALID UNTIL WITH ADMIN OPTION; ERROR: syntax error at or near "WITH" postgres=# CREATE ROLE admin3 PASSWORD WITH ADMIN OPTION; ERROR: syntax error at or near "WITH" postgres=# CREATE ROLE admin3 CONNECTION LIMIT WITH ADMIN OPTION; ERROR: syntax error at or near "WITH" > (I'm on the fence about the phrase "WITH ADMIN OPTION" vs. the phrase > "WITH GRANT OPTION".) > > Even then, when "michael" creates new roles, if he wants to be able > to further administer those roles, he needs to remember to give > himself ADMIN membership in that role at creation time. After the > role is created, if he doesn't have ADMIN, he can't give it to > himself. So, at create time, he needs to remember to do this: > > SET ROLE michael; > CREATE ROLE mark ADMIN michael; What would happen if ADMIN was implicit if michael is a non-superuser and there's no ADMIN in the CREATE ROLE statement? It would be backwards-compatible, one could still let somebody else be ADMIN, but ISTM a CREATEROLE role could no longer admin a role already existing previously/it didn't create/got assigned admin for (e.g. the predefined roles). I.e. (responding what you wrote much further below), the CREATEROLE role would no longer be ADMIN for all roles, just automatically for the ones it created. > But that's still a bit strange, because "ADMIN michael" means that > michael can grant other roles membership in "mark", not that michael > can, for example, change mark's password. Yeah, changing a password is one of the important tasks of a delegated role admin, if no superusers are around. > If we don't want CREATEROLE to imply that you can mess around with > arbitrary roles (rather than only roles that you created or have been > transferred control over) then we need the concept of role > ownership. This patch doesn't go that far, so for now, only > superusers can do those things. Assuming some form of this patch is > acceptable, the v9 series will resurrect some of the pre-v7 logic for > role ownership and say that the owner can do those things. > > 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; > > Yes, those it's more likely admin2 would have been created with these > privileges to begin with, if the creator intended admin2 to do such > things. Right, maybe people just have to adjust to the new way. It still feels strange that whatever you do at role creation time is more meaningful than when altering a role. > > > 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=> > > I don't really have this worked out yet. That's mostly because I'm > planning to fix it with role ownership, but perhaps there is a better > way? Well see above, maybe the patch is just broken/unfinished with respect to PASSWORD and the others? It works for REPLICATION e.g.: postgres=# ALTER ROLE admin2 REPLICATION WITH ADMIN OPTION; ALTER
Re: CREATEROLE and role ownership hierarchies
> On Jan 31, 2022, at 10:50 AM, Stephen Frost wrote: > > Supporting that through ADMIN is one option, another would be a > 'DROPROLE' attribute, though we'd want a way to curtail that from being > able to be used for just any role and that does lead down a path similar > to ownership or just generally the concept that some roles have certain > rights over certain other roles (whether you create them or not...). I've been operating under the assumption that I have a lot more freedom to create new features than to change how existing features behave, for two reasons: backwards compatibility and sql-spec compliance. Changing how having ADMIN on a role works seems problematic for both those reasons. My family got me socks for Christmas, not what I actually wanted, a copy of the SQL-spec. So I'm somewhat guessing here. But I believe we'd have problems if we "fixed" the part where a role can revoke ADMIN from others on themselves. Whatever we have, whether we call it "ownership", it can't be something a role can unilaterally revoke. As for a 'DROPROLE' attribute, I don't think that gets us anywhere. You don't seem to think so, either. So that leaves us with "ownership", perhaps by another word? I only chose that word because it's what we use elsewhere, but if we want to call it "managementship" and "manager" or whatever, that's fine. I'm not to the point of debating the terminology just yet. I'm still trying to get the behavior nailed down. > I do think there's a lot of value in being able to segregate certain > rights- consider that you may want a role that's able to create other > roles, perhaps grant them into some set of roles, can lock those roles > (prevent them from logging in, maybe do a password reset, something like > that), but which *isn't* able to drop those roles (and all their > objects) as that's dangerous and mistakes can certainly happen, or be > able to become that role because the creating role simply doesn't have > any need to be able to do that (or desire to in many cases, as we > discussed in the landlord-vs-tenant sub-thread). I'm totally on the same page. Your argument upthread about wanting any malfeasance on the part of a service provider showing up in the audit logs was compelling. Even for those things the "owner"/"manager" has the rights to do, we might want to make them choose to do it explicitly and not merely do it by accident. > Naturally, you'd want *some* role to be able to drop that role (and one > that doesn't have full superuser access) but that might be a role that's > not able to create new roles or take over accounts. I think it's important to go beyond the idea of a role attribute here. It's not that role "bob" can drop roles. It's that "bob" can drop *specific* roles, and for that, there has to be some kind of dependency tracked between "bob" and those other roles. I'm calling that "ownership". I think that language isn't just arbitrary, but actually helpful (technically, not politically) because REASSIGN OWNED should treat this kind of relationship exactly the same as it treats ownership of schemas, tables, functions, etc. > Separation of concerns and powers and all of that is what we want to be > going for here, more generically, which is why I was opposed to the > blanket "owners have all rights of all roles they own" implementation. I'm hoping to bring back, in v9, the idea of ownership/managership. The real sticking point here is that we (Robert, Andrew, I, and possibly others) want to be able to drop in a non-superuser-creator-role into existing systems that use superuser for role management. We'd like it to be as transparent a switch as possible. With a superuser creating a role, that superuser can come back and muck with the role afterward, and the role can't revoke the superuser's right to do so. It's not enough that a non-superuser-creator-role (henceforth, "manager") can grant itself ADMIN on the created role. It also needs to be able to set passwords, transfer object ownerships to/from the role, grant the role into other roles or other roles into it, etc. All of that has to be sandboxed such that the "manager" can't touch stuff outside the manager's sandbox, but within the sandbox, it shouldn't make any practical difference that the manager isn't actually a superuser. I think what I had in v7 was almost right. I'm hoping that we just need to adjust things like the idea that managers always have implicit membership in and ADMIN on roles they manage. I think that needs to be optional, and the audit logs could show if the manager granted themselves such things, as it might violate policy and be a red flag in the audit log. > That approach doesn't support the ability to have a relatively > unprivileged role that's able to create other roles, which seems like a > pretty important use-case for us to be considering. I think we have that ability. It's just that the creator role isn't "rel
Re: CREATEROLE and role ownership hierarchies
On 1/31/22 12:18, Mark Dilger wrote: > >> On Jan 31, 2022, at 12:43 AM, Michael Banck >> wrote: >> 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. > Right. > > Perhaps some background on this patch series will help. The patch versions > before v8 were creating an owner-owned relationship between the creator and > the createe, and a lot of privileges were dependent on that ownership. > Stephen objected that we were creating parallel tracks on which the privilege > system was running; things like belonging to a role or having admin on a role > were partially conflated with owning a role. He also objected that the > pre-v8 patch sets allowed a creator role with the CREATEROLE privilege to > give away any privilege the creator had, rather than needing to have GRANT or > ADMIN option on the privilege being given. > > The v8-WIP patch is not a complete replacement for the pre-v8 patches. It's > just a balloon I'm floating to try out candidate solutions to some of > Stephen's objections. In the long run, I want the solution to Stephen's > objections to not create problems for anybody who liked the way the pre-v8 > patches worked (Robert, Andrew, and to some extent me.) > > In this WIP patch, for a creator to give *anything* away to a createe, the > creator must have GRANT or ADMIN on the thing being given. That includes > attributes like BYPASSRLS, CREATEDB, LOGIN, etc., and also ADMIN on any role > the createe is granted into. > > I tried to structure things for backwards compatibility, considering which > things roles with CREATEROLE could give away historically. It turns out they > can give away most everything, but not SUPERUSER, BYPASSRLS, or REPLICATION. > So I structured the default privileges for CREATEROLE to match. But I'm > uncertain that design is any good, and your comments below suggest that you > find it pretty hard to use. > > Part of the problem with trying to be backwards compatible is that we must > break compatibility anyway, to address the problem that historically having > CREATEROLE meant you effectively had ADMIN on all non-superuser roles. > That's got to change. So in part I'm asking pgsql-hackers if partial > backwards compatibility is worth the bother. > > If we don't go with backwards compatibility, then CREATEROLE would only allow > you to create a new role, but not to give that role LOGIN, nor CREATEDB, etc. > You'd need to also have admin option on those things. To create a role that > can give those things away, you'd need to run something like: > > CREATE ROLE michael > CREATEROLE WITH ADMIN OPTION-- can further give away "createrole" > CREATEDB WITH ADMIN OPTION-- can further give away "createdb" > LOGIN WITH ADMIN OPTION-- can further give away "login" > NOREPLICATION WITHOUT ADMIN OPTION-- this would be implied anyway > NOBYPASSRLS WITHOUT ADMIN OPTION-- this would be implied anyway > CONNECTION LIMIT WITH ADMIN OPTION-- can specify connection limits > PASSWORD WITH ADMIN OPTION-- can specify passwords > VALID UNTIL WITH ADMIN OPTION-- can specify expiration > > (I'm on the fence about the phrase "WITH ADMIN OPTION" vs. the phrase "WITH > GRANT OPTION".) > > Even then, when "michael" creates new roles, if he wants to be able to > further administer those roles, he needs to remember to give himself ADMIN > membership in that role at creation time. After the role is created, if he > doesn't have ADMIN, he can't give it to himself. So, at create time, he > needs to remember to do this: > > SET ROLE michael; > CREATE ROLE mark ADMIN michael; > > But that's still a bit strange, because "ADMIN michael" means that michael > can grant other roles membership in "mark", not that michael can, for > example, change mark's password. If we don't want CREATEROLE to imply that > you can mess around with arbitrary roles (rather than only roles that you > created or have been transferred control over) then we need the concept of > role ownership. This patch doesn't go that far, so for now, only superusers > can do those things. Assuming some form of this patch is acceptable, the v9 > series will resurrect some of the pre-v7 logic for role ownership and say > that the owner can do those things. > This seems complicated. Maybe the previous proposal was too simple, but simplicity has some virtues. It seemed to me that more complex rules could possibly have been implemented for those who really needed them by using SECURITY DEFINER functions. The whole 'NOFOO WITH ADMIN OPTION' thing seems to me a bit like a POLA violation. Nevertheless I can probably live with it as long as it's *really* well documented. Even so I suspect it would be too complex for many, and they will just continue to use superusers to create and manage roles if possible.
Re: CREATEROLE and role ownership hierarchies
> On Feb 1, 2022, at 1:10 PM, Andrew Dunstan wrote: > > The whole 'NOFOO WITH ADMIN OPTION' > thing seems to me a bit like a POLA violation. Nevertheless I can > probably live with it as long as it's *really* well documented. Even so > I suspect it would be too complex for many, and they will just continue > to use superusers to create and manage roles if possible. I agree with the sentiment, but it might help to distinguish between surprising behavior vs. surprising grammar. In existing postgresql releases, having CREATEROLE means you can give away most attributes, including ones you yourself don't have (createdb, login). So we already have the concept of NOFOO WITH ADMIN OPTION, we just don't call it that. In pre-v8 patches on this thread, I got rid of that; you *must* have the attribute to give it away. But maybe that was too restrictive, and we need a way to specify, attribute by attribute, how this works. Is this just a problem of surprising grammar? Is it surprising behavior? If the latter, I'm inclined to give up this WIP as having been a bad move. If the former, I'll try to propose some less objectionable grammar. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On 2/1/22 17:27, Mark Dilger wrote: > >> On Feb 1, 2022, at 1:10 PM, Andrew Dunstan wrote: >> >> The whole 'NOFOO WITH ADMIN OPTION' >> thing seems to me a bit like a POLA violation. Nevertheless I can >> probably live with it as long as it's *really* well documented. Even so >> I suspect it would be too complex for many, and they will just continue >> to use superusers to create and manage roles if possible. > I agree with the sentiment, but it might help to distinguish between > surprising behavior vs. surprising grammar. > > In existing postgresql releases, having CREATEROLE means you can give away > most attributes, including ones you yourself don't have (createdb, login). > So we already have the concept of NOFOO WITH ADMIN OPTION, we just don't call > it that. In pre-v8 patches on this thread, I got rid of that; you *must* > have the attribute to give it away. But maybe that was too restrictive, and > we need a way to specify, attribute by attribute, how this works. Is this > just a problem of surprising grammar? Is it surprising behavior? If the > latter, I'm inclined to give up this WIP as having been a bad move. If the > former, I'll try to propose some less objectionable grammar. > > Certainly the grammar would need to be better. But I'm not sure any grammar that expresses what is supported here is not going to be confusing, because the underlying scheme seems complex. But I'm persuadable. I'd like to hear from others on the subject. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
On Tue, Feb 1, 2022 at 6:38 PM Andrew Dunstan wrote: > > In existing postgresql releases, having CREATEROLE means you can give away > > most attributes, including ones you yourself don't have (createdb, login). > > So we already have the concept of NOFOO WITH ADMIN OPTION, we just don't > > call it that. In pre-v8 patches on this thread, I got rid of that; you > > *must* have the attribute to give it away. But maybe that was too > > restrictive, and we need a way to specify, attribute by attribute, how this > > works. Is this just a problem of surprising grammar? Is it surprising > > behavior? If the latter, I'm inclined to give up this WIP as having been a > > bad move. If the former, I'll try to propose some less objectionable > > grammar. > > > > Certainly the grammar would need to be better. But I'm not sure any > grammar that expresses what is supported here is not going to be > confusing, because the underlying scheme seems complex. But I'm > persuadable. I'd like to hear from others on the subject. Well, we've been moving more and more in the direction of using predefined roles to manage access. The things that are basically Boolean flags on the role are mostly legacy stuff. So my tentative opinion (and I'm susceptible to being persuaded that I'm wrong here) is that putting a lot of work into fleshing out that infrastructure does not necessarily make a ton of sense. Are we ever going to add even one more flag that works that way? Also, any account that can create roles is a pretty high-privilege account. Maybe it's superuser, or maybe not, but it's certainly powerful. In my opinion, that makes fine distinctions here less important. Is there really an argument for saying "well, we're going to let you bypass RLS, but we're not going to let you give that privilege to others"? It seems contrived to think of restricting a role that is powerful enough to create whole new accounts in such a way. I'm not saying that someone couldn't have a use case for it, but I think it'd be a pretty thin use case. In short, I think it makes tons of sense to say that CREATEROLE lets you give to others those role flags which you have, but not the ones you lack. However, to me, it feels like overengineering to distinguish between things you have and can give away, things you have and can't give away, and things you don't even have. -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Jan 31, 2022, at 10:50 AM, Stephen Frost wrote: > > Supporting that through ADMIN is one option, another would be a > > 'DROPROLE' attribute, though we'd want a way to curtail that from being > > able to be used for just any role and that does lead down a path similar > > to ownership or just generally the concept that some roles have certain > > rights over certain other roles (whether you create them or not...). > > I've been operating under the assumption that I have a lot more freedom to > create new features than to change how existing features behave, for two > reasons: backwards compatibility and sql-spec compliance. I agree that those are concerns that need to be considered, though I'm more concerned about the SQL compliance and less about backwards compatibility in this case. For one thing, I'm afraid that we're not as compliant as we really should be and that should really drive us to make change here anyway, to get closer to what the spec calls for. > Changing how having ADMIN on a role works seems problematic for both those > reasons. My family got me socks for Christmas, not what I actually wanted, a > copy of the SQL-spec. So I'm somewhat guessing here. But I believe we'd > have problems if we "fixed" the part where a role can revoke ADMIN from > others on themselves. Whatever we have, whether we call it "ownership", it > can't be something a role can unilaterally revoke. > > As for a 'DROPROLE' attribute, I don't think that gets us anywhere. You > don't seem to think so, either. So that leaves us with "ownership", perhaps > by another word? I only chose that word because it's what we use elsewhere, > but if we want to call it "managementship" and "manager" or whatever, that's > fine. I'm not to the point of debating the terminology just yet. I'm still > trying to get the behavior nailed down. Yeah, didn't mean to imply that those were great ideas or that I was particularly advocating for them, but just to bring up some other ideas to try and get more thought going into this. > > I do think there's a lot of value in being able to segregate certain > > rights- consider that you may want a role that's able to create other > > roles, perhaps grant them into some set of roles, can lock those roles > > (prevent them from logging in, maybe do a password reset, something like > > that), but which *isn't* able to drop those roles (and all their > > objects) as that's dangerous and mistakes can certainly happen, or be > > able to become that role because the creating role simply doesn't have > > any need to be able to do that (or desire to in many cases, as we > > discussed in the landlord-vs-tenant sub-thread). > > I'm totally on the same page. Your argument upthread about wanting any > malfeasance on the part of a service provider showing up in the audit logs > was compelling. Even for those things the "owner"/"manager" has the rights > to do, we might want to make them choose to do it explicitly and not merely > do it by accident. Glad to hear that. > > Naturally, you'd want *some* role to be able to drop that role (and one > > that doesn't have full superuser access) but that might be a role that's > > not able to create new roles or take over accounts. > > I think it's important to go beyond the idea of a role attribute here. It's > not that role "bob" can drop roles. It's that "bob" can drop *specific* > roles, and for that, there has to be some kind of dependency tracked between > "bob" and those other roles. I'm calling that "ownership". I think that > language isn't just arbitrary, but actually helpful (technically, not > politically) because REASSIGN OWNED should treat this kind of relationship > exactly the same as it treats ownership of schemas, tables, functions, etc. I agree that role attributes isn't a good approach and that we should be moving away from it. I'm less sure that the existance of REASSIGN OWNED for schemas and tables and such should be the driver for what this capability of one role being able to drop another role needs to be called. > > Separation of concerns and powers and all of that is what we want to be > > going for here, more generically, which is why I was opposed to the > > blanket "owners have all rights of all roles they own" implementation. > > I'm hoping to bring back, in v9, the idea of ownership/managership. The real > sticking point here is that we (Robert, Andrew, I, and possibly others) want > to be able to drop in a non-superuser-creator-role into existing systems that > use superuser for role management. We'd like it to be as transparent a > switch as possible. That description itself really makes me wonder about the sense of what was proposed. Specifically "existing systems that use superuser for role management" doesn't make me picture a system where this manager role has any need to run SELECT statements against the tabl
Re: CREATEROLE and role ownership hierarchies
> On Feb 2, 2022, at 11:52 AM, Stephen Frost wrote: > > The question that we need to solve is how to give > users the ability to choose what roles have which of the privileges that > we've outlined above and agreed should be separable. Ok, there are really two different things going on here, and the conversation keeps conflating them. Maybe I'm wrong, but I think the conflation of these things is the primary problem preventing us from finishing up the design. Thing 1: The superuser needs to be able to create roles who can create other roles. Let's call them "creators". Not every organization will want the same level of privilege to be given to a creator, or even that all creators have equal levels of privilege. So when the superuser creates a creator, the superuser needs to be able to configure what exactly what that creator can do. This includes which attributes the creator can give to new roles. It *might* include whether the creator maintains a dependency link with the created role, called "ownership" or somesuch. It *might* include whether the creator can create roles into which the creator is granted membership/administership. But there really isn't any reason that these things should be all-or-nothing. Maybe one creator maintains a dependency link with created roles, and that dependency link entails some privileges. Maybe other creators do not maintain such a link. It seems like superuser can define a creator in many different ways, as long as we nail down what those ways are, and what they mean. Thing 2: The creator needs to be able to specify which attributes and role memberships are set up with for roles the creator creates. To the extent that the creator has been granted the privilege to create yet more creators, this recurses to Thing 1. But not all creators will have that ability. I think the conversation gets off topic and disagreement abounds when Thing 1 is assumed to be hardcoded, leaving just the details of Thing 2 to be discussed. It's perfectly reasonable (in my mind) that Robert, acting as superuser, may want to create a creator who acts like a superuser over the sandbox, while at the same time Stephen, acting as superuser, may want to create a creator who acts as a low privileged bot that only adds and removes roles, but cannot read their tables, SET ROLE to them, etc. I don't see any reason that Robert and Stephen can't both get what they want. We just have to make Thing 1 flexible enough. Do you agree at least with this much? If so, I think we can hammer out what to do about Thing 1 and get something committed in time for postgres 15. If not, then I'm probably going to stop working on this until next year, because at this point, we don't have enough time to finish. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: CREATEROLE and role ownership hierarchies
On Wed, Feb 2, 2022 at 3:23 PM Mark Dilger wrote: > It's perfectly reasonable (in my mind) that Robert, acting as superuser, may > want to create a creator who acts like a superuser over the sandbox, while at > the same time Stephen, acting as superuser, may want to create a creator who > acts as a low privileged bot that only adds and removes roles, but cannot > read their tables, SET ROLE to them, etc. > > I don't see any reason that Robert and Stephen can't both get what they want. > We just have to make Thing 1 flexible enough. Hmm, that would be fine with me. I don't mind a bit if other people get what they want, as long as I can get what I want, too! In fact, I'd prefer it if other people also get what they want... That having been said, I have some reservations if it involves tightly coupling new features that we're trying to add to existing things that may or may not be that well designed, like the role-level INHERIT flag, or WITH ADMIN OPTION, or the not-properly maintained pg_auth_members.grantor column, or even the SQL standard. I'm not saying we should ignore any of those things and I don't think that we should ... but at the same time, we can't whether the feature does what people want it to do, either. If we do, this whole thing is really a complete waste of time. If a patch achieves infinitely large amounts of backward compatibility, standards compliance, and conformity with existing design but doesn't do the right stuff, forget it! -- Robert Haas EDB: http://www.enterprisedb.com
Re: CREATEROLE and role ownership hierarchies
I'm chiming in a little late here, but as someone who worked on a system to basically work around the lack of unprivileged CREATE ROLE for a cloud provider (I worked on the Heroku Data team for several years), I thought it might be useful to offer my perspective. This is, of course, not the only use case, but maybe it's useful to have something concrete. As a caveat, I don't know how current this still is (I no longer work there, though the docs [1] seem to still describe the same system), or if there are better ways to achieve the goals of a service provider. Broadly, the general use case is something like what Robert has sketched out in his e-mails. Heroku took care of setting up the database, archiving, replication, and any other system-level config. It would then keep the superuser credentials private, create a database, and a user that owned that database and had all the permissions we could grant it without compromising the integrity of the system. (We did not want customers to break their databases, both to ensure a better user experience and to avoid getting paged.) Initially, this meant customers got just the one database user because of CREATE ROLE's limitations. To work around that, at some point, we added an API that would CREATE ROLE for you, accessible through a dashboard and the Heroku CLI. This ran CREATE ROLE (or DROP ROLE) for you, but otherwise it largely let you configure the resulting roles as you pleased (using the original role we create for you). We wanted to avoid reinventing the wheel as much as possible, and the customer database (including the role configuration) was mostly a black box for us (we did manage some predefined permissions configurations through our dashboard, but the Postgres catalogs were the source of truth for that). Thinking about how this would fit into a potential non-superuser CREATE ROLE world, the sandbox superuser model discussed above covers this pretty well, though I share some of Robert's concerns around how this fits into existing systems. Hope this is useful feedback. Thanks for working on this! [1]: https://devcenter.heroku.com/articles/heroku-postgresql-credentials
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 31, 2022 at 1:57 PM Joshua Brindle wrote: > This is precisely the use case I am trying to accomplish with this > patchset, roughly: > > - An automated bot that creates users and adds them to the employees role > - Bot cannot access any employee (or other roles) table data > - Bot cannot become any employee > - Bot can disable the login of any employee > > Yes there are attack surfaces around the fringes of login, etc but > those can be mitigated with certificate authentication. My pg_hba > would require any role in the employees role to use cert auth. > > This would adequately mitigate many threats while greatly enhancing > user management. So, where do we go from here? I've been thinking about this comment a bit. On the one hand, I have some reservations about the phrase "the use case I am trying to accomplish with this patchset," because in the end, this is not your patch set. It's not reasonable to complain that a patch someone else wrote doesn't solve your problem; of course everyone writes patches to solve their own problems, or those of their employer, not other people's problems. And that's as it should be, else we will have few contributors. On the other hand, to the extent that this patch set makes things worse for a reasonable use case which you have in mind, that's an entirely legitimate complaint. After a bit of testing, it seems to me that as things stand today, things are nearly perfect for the use case that you have in mind. I would be interested to know whether you agree. If I set up an account and give it CREATEROLE, it can create users, and it can put them into the employees role, but it can't SET ROLE to any of those accounts. It can also ALTER ROLE ... NOLOGIN on any of those accounts. The only gap I see is that there are certain role-based flags which the CREATEROLE account cannot set: SUPERUSER, BYPASSRLS, REPLICATION. You might prefer a system where your bot account had the option to grant those privileges also, and I think that's a reasonable thing to want. However, I *also* think it's reasonable to want an account that can create roles but can't give to those roles membership in roles that it does not itself possess. Likewise, I think it's reasonable to want an account that can only drop roles which that account itself created. These kinds of requirements stem from a different use case than what you are talking about here, but they seem like fine things to want, and as far as I know we have pretty broad agreement that they are reasonable. It seems extremely difficult to make a convincing argument that this is not a thing which anyone should want to block: rhaas=> create role bob role pg_execute_server_program; CREATE ROLE Honestly, that seems like a big yikes from here. How is it OK to block "create role bob superuser" yet allow that command? I'm inclined to think that's just broken. Even if the role were pg_write_all_data rather than pg_execute_server_program, it's still a heck of a lot of power to be handing out, and I don't see how anyone could make a serious argument that we shouldn't have an option to restrict that. Let me separate the two features that I just mentioned and talk about them individually: 1. Don't allow a CREATEROLE user to give out membership in groups which that user does not possess. Leaving aside the details of any previously-proposed patches and just speaking theoretically, how can this be implemented? I can think of a few ideas. We could (1A) just change CREATEROLE to work that way, but IIUC that would break the use case you outline here, so I guess that's off the table unless I am misunderstanding the situation. We could also (1B) add a second role attribute with a different name, like, err, CREATEROLEWEAKLY, that behaves in that way, leaving the existing one untouched. But we could also take it a lot further, because someone might want to let an account hand out a set of privileges which corresponds neither to the privileges of that account nor to the full set of available privileges. That leads to another idea: (1C) implement an in-database system that lets you specify which privileges an account has, and, separately, which ones it can assign to others. I am skeptical of that idea because it seems really, really complicated, not only from an implementation standpoint but even just from a user-experience standpoint. Suppose user 'userbot' has rights to grant a suitable set of groups to the new users that it creates -- but then someone creates a new group. Should that also be added to the things 'userbot' can grant or not? What if we have 'userbot1' through 'userbot6' and each of them can grant a different set of roles? I wouldn't mind (1D) providing a hook that allows the system administrator to install a loadable module that can enforce any rules it likes, but it seems way too complicated to me to expose all of this configuration as SQL, especially because for what I want to do, either (1A) or (1B) is adequate, and (1B) is a LOT sim
Re: CREATEROLE and role ownership hierarchies
On Thu, Feb 17, 2022 at 12:40 PM Robert Haas wrote: > > On Mon, Jan 31, 2022 at 1:57 PM Joshua Brindle > wrote: > > This is precisely the use case I am trying to accomplish with this > > patchset, roughly: > > > > - An automated bot that creates users and adds them to the employees role > > - Bot cannot access any employee (or other roles) table data > > - Bot cannot become any employee > > - Bot can disable the login of any employee > > > > Yes there are attack surfaces around the fringes of login, etc but > > those can be mitigated with certificate authentication. My pg_hba > > would require any role in the employees role to use cert auth. > > > > This would adequately mitigate many threats while greatly enhancing > > user management. > > So, where do we go from here? > > I've been thinking about this comment a bit. On the one hand, I have > some reservations about the phrase "the use case I am trying to > accomplish with this patchset," because in the end, this is not your > patch set. It's not reasonable to complain that a patch someone else > wrote doesn't solve your problem; of course everyone writes patches to > solve their own problems, or those of their employer, not other > people's problems. And that's as it should be, else we will have few > contributors. On the other hand, to the extent that this patch set > makes things worse for a reasonable use case which you have in mind, > that's an entirely legitimate complaint. Yes, absolutely. It is my understanding that generally a community consensus is attempted, I was throwing my (and Crunchy's) use case out there as a possible goal, and I have spent time reviewing and testing the patch, so I think that is fair. Obviously I am not in the position to stipulate hard requirements. > After a bit of testing, it seems to me that as things stand today, > things are nearly perfect for the use case that you have in mind. I > would be interested to know whether you agree. If I set up an account > and give it CREATEROLE, it can create users, and it can put them into > the employees role, but it can't SET ROLE to any of those accounts. It > can also ALTER ROLE ... NOLOGIN on any of those accounts. The only gap > I see is that there are certain role-based flags which the CREATEROLE > account cannot set: SUPERUSER, BYPASSRLS, REPLICATION. You might > prefer a system where your bot account had the option to grant those > privileges also, and I think that's a reasonable thing to want. I believe the only issue in the existing patchset was that membership was required in employees was required for the Bot, but I can apply the current patchset and test it out more in a bit. > However, I *also* think it's reasonable to want an account that can > create roles but can't give to those roles membership in roles that it > does not itself possess. Likewise, I think it's reasonable to want an > account that can only drop roles which that account itself created. > These kinds of requirements stem from a different use case than what > you are talking about here, but they seem like fine things to want, > and as far as I know we have pretty broad agreement that they are > reasonable. It seems extremely difficult to make a convincing argument > that this is not a thing which anyone should want to block: > > rhaas=> create role bob role pg_execute_server_program; > CREATE ROLE > > Honestly, that seems like a big yikes from here. How is it OK to block > "create role bob superuser" yet allow that command? I'm inclined to > think that's just broken. Even if the role were pg_write_all_data > rather than pg_execute_server_program, it's still a heck of a lot of > power to be handing out, and I don't see how anyone could make a > serious argument that we shouldn't have an option to restrict that. Yes, agreed 100%. To be clear, I do not want Bot in the above use case to be able to add any role other than employees to new roles it creates. So we are in complete agreement there, the only difference is that I do not want Bot to be able to become those roles (or use any access granted via those roles), it's only job is to manage roles, not look at data. > Let me separate the two features that I just mentioned and talk about > them individually: > > 1. Don't allow a CREATEROLE user to give out membership in groups > which that user does not possess. Leaving aside the details of any > previously-proposed patches and just speaking theoretically, how can > this be implemented? I can think of a few ideas. We could (1A) just > change CREATEROLE to work that way, but IIUC that would break the use > case you outline here, so I guess that's off the table unless I am > misunderstanding the situation. We could also (1B) add a second role > attribute with a different name, like, err, CREATEROLEWEAKLY, that > behaves in that way, leaving the existing one untouched. But we could > also take it a lot further, because someone might want to let an > account hand out a set of privileges which correspon
Re: CREATEROLE and role ownership hierarchies
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > 1. Don't allow a CREATEROLE user to give out membership in groups > which that user does not possess. Leaving aside the details of any > previously-proposed patches and just speaking theoretically, how can > this be implemented? I can think of a few ideas. We could (1A) just > change CREATEROLE to work that way, but IIUC that would break the use > case you outline here, so I guess that's off the table unless I am > misunderstanding the situation. We could also (1B) add a second role > attribute with a different name, like, err, CREATEROLEWEAKLY, that > behaves in that way, leaving the existing one untouched. But we could > also take it a lot further, because someone might want to let an > account hand out a set of privileges which corresponds neither to the > privileges of that account nor to the full set of available > privileges. That leads to another idea: (1C) implement an in-database > system that lets you specify which privileges an account has, and, > separately, which ones it can assign to others. I am skeptical of that > idea because it seems really, really complicated, not only from an > implementation standpoint but even just from a user-experience > standpoint. Suppose user 'userbot' has rights to grant a suitable set > of groups to the new users that it creates -- but then someone creates > a new group. Should that also be added to the things 'userbot' can > grant or not? What if we have 'userbot1' through 'userbot6' and each > of them can grant a different set of roles? I wouldn't mind (1D) > providing a hook that allows the system administrator to install a > loadable module that can enforce any rules it likes, but it seems way > too complicated to me to expose all of this configuration as SQL, > especially because for what I want to do, either (1A) or (1B) is > adequate, and (1B) is a LOT simpler than (1C). It also caters to what > I believe to be a common thing to want, without prejudice to the > possibility that other people want other things. I'm generally in support of changing CREATEROLE to only allow roles that the role with CREATEROLE is an admin of to be allowed as part of the command (throwing an error in other cases). That doesn't solve other use-cases which would certainly be nice to solve but it would at least reduce the shock people have when they discover how CREATEROLE actually works (that is, the way we document it to work, but that's ultimately not what people expect). If that's all this was about then that would be one thing, but folks are interested in doing more here and that's good because there's a lot here that could be (and I'd say should be..) done. I'm not a fan of 1B. In general, I'm in support of 1C but I don't feel that absolutely everything must be done for 1C right from the start- rather, I would argue that we'd be better off building a way for 1C to be improved upon in the future, akin to our existing privilege system where we've added things like the ability to GRANT TRUNCATE rights which didn't originally exist. I don't think 1D is a reasonable way to accomplish that though, particularly as this involves storing information about roles which needs to be cleaned up if those roles are removed or modified. I also don't really agree with the statement that this ends up being too complicated for SQL. > 2. Only allow a CREATEROLE user to drop users which that account > created, and not just any role that isn't a superuser. Again leaving > aside previous proposals, this cannot be implemented without providing > some means by which we know which CREATEROLE user created which other > user. I believe there are a variety of words we could use to describe > that linkage, and I don't deeply care which ones we pick, although I > have my own preferences. We could speak of the CREATEROLE user being > the owner, manager, or administrator of the created role. We could > speak of a new kind of object, a TENANT, of which the CREATEROLE user > is the administrator and to which the created user is linked. I > proposed this previously and it's still my favorite idea. There are no > doubt other options as well. But it's axiomatic that we cannot > restrict the rights of a CREATEROLE user to drop other roles to a > subset of roles without having some way to define which subset is at > issue. I don't think it's a great plan to limit who is allowed to DROP roles to be just those that a given role created. I also don't like the idea of introducing a single field for owner/manager/tenant/whatever to the role system- instead we should add other ways that roles can be associated to each other by extending the existing system that we have for that, which is role membership. Role membership today is pretty limited but I don't see any reason why we couldn't improve on that in a way that's flexible and allows us to define new associations in the future. The biggest difference between a 'tenant' or such as proposed vs. a ro
Re: CREATEROLE and role ownership hierarchies
[ Been away, catching up on email. ] On Tue, Feb 22, 2022 at 10:54 AM Joshua Brindle wrote: > Yes, absolutely. It is my understanding that generally a community > consensus is attempted, I was throwing my (and Crunchy's) use case out > there as a possible goal, and I have spent time reviewing and testing > the patch, so I think that is fair. Obviously I am not in the position > to stipulate hard requirements. I agree with all of that -- and thanks for writing back. > if 1A worked for admins, or members I think it may work (i.e., Bot is > admin of employees but not a member of employees and therefore can > manage employees but not become them or read their tables) > > For example, today this works (in master): > > postgres=# CREATE USER creator password 'a'; > CREATE ROLE > postgres=# CREATE ROLE employees ADMIN creator NOLOGIN; > CREATE ROLE > > as creator: > postgres=> CREATE USER joshua IN ROLE employees PASSWORD 'a'; > ERROR: permission denied to create role > > as superuser: > postgres=# CREATE USER joshua LOGIN PASSWORD 'a'; > CREATE ROLE > > as creator: > postgres=> GRANT employees TO joshua; > GRANT ROLE > postgres=> SET ROLE joshua; > ERROR: permission denied to set role "joshua" > postgres=> SET ROLE employees; > SET > > So ADMIN of a role can add membership, but not create, and > unfortunately can SET ROLE to employees. > > Can ADMIN mean "can create and drop roles with membership of this role > but not implicitly be a member of the role"? I foresee big problems trying to go in this direction. According to the documentation, "the ADMIN clause is like ROLE, but the named roles are added to the new role WITH ADMIN OPTION, giving them the right to grant membership in this role to others." And for me, the name "WITH ADMIN OPTION" is a huge red flag. You grant membership in a role, and you may grant that membership with the admin option, or without the admin option, but either way you are granting membership. And to me that is just built into the phraseology. You may be able to buy the car that you want with or without the all-wheel drive option, and you may even be able to upgrade a car purchased without that option to have it later, but you can't buy all-wheel drive in the abstract without an association to some particular car. That's what it means for it to be an option. Now, I think there is a good argument to be made that in this case the fact that the administration privileges are an option associated with membership is artificial. I expect we can all agree that it is conceptually easy to understand the idea of being able to administer a role and the idea of having that role's privileges as two separate concepts, neither dependent upon the other, and certainly the SQL syntax could be written in a way that makes that very natural. But as it is, what is the equivalent of GRANT employees TO bot WITH ADMIN OPTION when you want to convey only administration rights and not membership? GRANT employees TO bot WITH ADMIN OPTION BUT WITHOUT THE UNDERLYING MEMBERSHIP TO WHICH ADMIN IS AN OPTION? Maybe that sounds sarcastic, but to me it seems like a genuinely serious problem. People construct a mental model of how stuff works based to a significant degree on the structure of the syntax, and I really don't see an obvious way of extending the grammar in a way that is actually going to make sense to people. > The current (v8) patch conflates membership and admin: > > postgres=# CREATE USER user_creator CREATEROLE WITHOUT ADMIN OPTION > PASSWORD 'a'; > CREATE ROLE > postgres=# CREATE ROLE employees ADMIN user_creator NOLOGIN; > CREATE ROLE > > (Note I never GRANTED employees to user_creator): I think you did, because even right now without the patch "ADMIN whatever" is documented to mean membership with admin option. > > So that leads to these questions: (2A) Do you care about restricting > > which roles the userbot can drop? (2B) If yes, do you endorse > > restricting the ability of roles to revoke themselves from other > > roles? > > 2A, yes > 2B, yes, and IIUC this already exists: > postgres=> select current_user; > current_user > -- > joshua > (1 row) > > postgres=> REVOKE employees FROM joshua; > ERROR: must have admin option on role "employees" No, because as Stephen correctly points out, you've got that REVOKE command backwards. > > I think that we don't have any great problems here, at least as far as > > this very specific issue is concerned, if either the answer to (2A) is > > no or the answer to (2B) is yes. However, if the answer to (2A) is yes > > and the answer to (2B) is no, there are difficulties. Evidently in > > that case we need some new kind of thing that behaves mostly likes a > > group of roles but isn't actually a group of roles -- and that thing > > needs to prohibit self-revocation. Given what I've written above, you > > may be able to guess my preferred solution: let's call it a TENANT. > > Then, my pseudo-super-user can have permission to (i) create roles
Re: CREATEROLE and role ownership hierarchies
On Mon, Feb 28, 2022 at 2:09 PM Stephen Frost wrote: > I'm generally in support of changing CREATEROLE to only allow roles that > the role with CREATEROLE is an admin of to be allowed as part of the > command (throwing an error in other cases). That doesn't solve other > use-cases which would certainly be nice to solve but it would at least > reduce the shock people have when they discover how CREATEROLE actually > works (that is, the way we document it to work, but that's ultimately > not what people expect). So I'm 100% good with that because it does exactly what I want, but my understanding of the situation is that it breaks the userbot case that Joshua is talking about. Right now, with stock PostgreSQL in any released version, his userbot can have CREATEROLE and give out roles that it doesn't itself possess. If we restrict CREATEROLE in the way described here, that's no longer possible. Now it's possible that you and/or he would take the position that we're still coming out ahead despite that functional regression, because as I now understand after reading Joshua's latest email, he doesn't want the userbot to be able to grant ANY role, just the 'employees' role - and today he can't get that. So in a modified universe where we restrict the privileges of CREATEROLE, then on the one hand he GAINS the ability to have a userbot that can grant some roles but not others, but on the other hand, he's forced to give the userbot the roles he wants it to be able to hand out. Is that better overall or worse? To really give him EXACTLY what he wants, we need a way of specifying administration without membership. See my last reply to the thread for my concerns about that. > I don't think it's a great plan to limit who is allowed to DROP roles to > be just those that a given role created. I also don't like the idea of > introducing a single field for owner/manager/tenant/whatever to the role > system- instead we should add other ways that roles can be associated to > each other by extending the existing system that we have for that, which > is role membership. Role membership today is pretty limited but I don't > see any reason why we couldn't improve on that in a way that's flexible > and allows us to define new associations in the future. The biggest > difference between a 'tenant' or such as proposed vs. a role association > is in where the information is tracked and what exactly it means. > Saying "I want a owner" or such is easy because it's basically punting > on the complciated bit of asking the question: what does that *mean* > when it comes to what rights that includes vs. doesn't? What if I only > want some of those rights to be given away but not all of them? We have > that system for tables/schemas/etc, and it hasn't been great as we've > seen through the various requests to add things like GRANT TRUNCATE. Well, there's no accounting for taste, but I guess I see this pretty much opposite to the way you do. I think GRANT TRUNCATE is nice and simple and clear. It does one thing and it's easy to understand what that thing is, and it has very few surprising or undocumented side effects. On the other hand, role membership is a mess, and it's not at all clear how to sort that mess out. I guess I agree with you that it would be nice if it could be done, but the list of problems is pretty substantial. Like, membership implies the right to SET ROLE, and also the right to implicitly exercise the privileges of the role, and you've complained about that fuzziness. And ADMIN OPTION implies membership, and you don't like that either. And elsewhere it's been raised that nobody would expect to have a table end up owned by 'pg_execute_server_programs', or a user logged in directly as 'employees' rather than as some particular employee, but all that stuff can happen, and some of it can't even be effectively prevented with good configuration. 'toe' can be a member of 'foot' while, which makes sense to everybody, and at the same time, 'foot' can be a member of 'toe', which doesn't make any sense at all. And because both directions are possible even experienced PostgreSQL users and hackers get confused, as demonstrated by Joshua's having just got the revoke-from-role case backwards. Of those four problems, the last two are clearly the result of conflating users with groups - and really also with capabilities - and having a unified role concept that encompasses all of those things. I think we would be better off if we had not done that, both in the sense that I think the system would be less confusing to understand, and also in the sense that we would likely have fewer security bugs. And similarly I agree with you that it would be better if the right to administer a role were clearly separated from membership in a role, and if the right to use the privileges of a role were separated from the ability to SET ROLE to it. However, unlike you, I see the whole 'role membership' concept as the problem, not the solution. W
