From: | Nathan Bossart <nathandbossart(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: CREATEROLE users vs. role properties |
Date: | 2023-01-18 23:17:49 |
Message-ID: | 20230118231749.GA3790025@nathanxps13 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Jan 18, 2023 at 12:15:33PM -0500, Robert Haas wrote:
> On Mon, Jan 16, 2023 at 2:29 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> 1. It's still possible for a CREATEROLE user to hand out role
>> attributes that they don't possess. The new prohibitions in
>> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
>> from handing out membership in a role on which they lack sufficient
>> permissions, but they don't prevent a CREATEROLE user who lacks
>> CREATEDB from creating a new user who does have CREATEDB. I think we
>> should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
>> the same rule that we now use for role memberships: you've got to have
>> the property in order to give it to someone else. In the case of
>> CREATEDB, this would tighten the current rules, which allow you to
>> give out CREATEDB without having it. In the case of REPLICATION and
>> BYPASSRLS, this would liberalize the current rules: right now, a
>> CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
>> even if they possess those attributes.
>>
>> This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
>> properties. It seems possible to me that someone might want to set up
>> a CREATEROLE user who can't make more such users, and this proposal
>> doesn't manufacture any way of doing that. It also doesn't let you
>> constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
>> for some other user. I think that's OK. It might be nice to have ways
>> of imposing such restrictions at some point in the future, but it is
>> not very obvious what to do about such cases and, importantly, I don't
>> think there's any security impact from failing to address those cases.
>> If a CREATEROLE user without CREATEDB can create a new role that does
>> have CREATEDB, that's a privilege escalation. If they can hand out
>> CREATEROLE, that isn't: they already have it.
>
> Here is a patch implementing the above proposal. Since this is fairly
> closely related to already-committed work, I would like to get this
> into v16. That way, all the changes to how CREATEROLE works will go
> into a single release, which seems less confusing for users. It is
> also fairly clear to me that this is an improvement over the status
> quo. Sometimes things that seem clear to me turn out to be false, so
> if this change seems like a problem to you, please let me know.
This seems like a clear improvement to me. However, as the attribute
system becomes more sophisticated, I think we ought to improve the error
messages in user.c. IMHO messages like "permission denied" could be
greatly improved with some added context.
For example, if I want to change a role's password, I need both CREATEROLE
and ADMIN OPTION on the role, but the error message only mentions
CREATEROLE.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole password 'test';
ERROR: must have CREATEROLE privilege to change another user's password
Similarly, if I want to allow a role to grant REPLICATION to another role,
I have to give it CREATEROLE, REPLICATION, and membership with ADMIN
OPTION. If the role is missing CREATEROLE or membership with ADMIN OPTION,
it'll only ever see a "permission denied" error.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# alter role createrole with replication;
ALTER ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# grant otherrole to createrole;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: permission denied
postgres=> reset role;
RESET
postgres=# grant otherrole to createrole with admin option;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ALTER ROLE
If it has both CREATEROLE and membership with ADMIN OPTION (but not
REPLICATION), it'll see a more helpful message.
postgres=# create role createrole with createrole;
CREATE ROLE
postgres=# create role otherrole;
CREATE ROLE
postgres=# grant otherrole to createrole with admin option;
GRANT ROLE
postgres=# set role createrole;
SET
postgres=> alter role otherrole with replication;
ERROR: must have replication privilege to change replication attribute
This probably shouldn't block your patch, but I think it's worth doing in
v16 since there are other changes in this area. I'm happy to help.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2023-01-18 23:28:19 | Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation |
Previous Message | Tom Lane | 2023-01-18 22:55:46 | Re: Rethinking the implementation of ts_headline() |