Date: Wed, 18 Jan 2023 15:17:49 -0800
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
Message-ID: <20230118231749.GA3790025@nathanxps13>
References: 
 <CA+TgmoYwTFTQR0+qe+_asEvv2wqQL+0JCyexgVy2Ct31+rCb7w@mail.gmail.com>
 <CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230118231749.GA3790025%40nathanxps13>
Precedence: bulk

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