From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: CREATEROLE users vs. role properties
Date: Wed, 18 Jan 2023 15:17:49 -0800
Message-ID: <20230118231749.GA3790025@nathanxps13> (raw)
In-Reply-To: <CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com>
References: <CA+TgmoYwTFTQR0+qe+_asEvv2wqQL+0JCyexgVy2Ct31+rCb7w@mail.gmail.com>
<CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com>
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@gmail.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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, robertmhaas@gmail.com
Subject: Re: CREATEROLE users vs. role properties
In-Reply-To: <20230118231749.GA3790025@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox