Thread: fixing CREATEROLE

The CREATEROLE permission is in a very bad spot right now. The biggest
problem that I know about is that it allows you to trivially access
the OS user account under which PostgreSQL is running, which is
expected behavior for a superuser but simply wrong behavior for any
other user. This is because CREATEROLE conveys powerful capabilities
not only to create roles but also to manipulate them in various ways,
including granting any non-superuser role in the system to any new or
existing user, including themselves. Since v11, the roles that can be
granted include pg_execute_server_program and pg_write_server_files
which are trivially exploitable. Perhaps this should have been treated
as an urgent security issue and a fix back-patched, although it is not
clear to me exactly what such a fix would look like. Since we haven't
done that, I went looking for a way to improve things in a principled
way going forward, taking advantage also of recent master-only work to
improve various aspects of the role grant system.

Here, I feel it important to point out that I think the current system
would be broken even if we didn't have predefined roles that are
trivially exploitable to obtain OS user access. We would still lack
any way to restrict the scope of the CREATEROLE privilege. Sure, the
privilege doesn't extend to superusers, but that's not really good
enough. Consider:

rhaas=# create role alice createrole;
CREATE ROLE
rhaas=# create role bob password 'known_only_to_bob';
CREATE ROLE
rhaas=# set session authorization alice;
SET
rhaas=> alter role bob password 'known_to_alice';
ALTER ROLE

Assuming that some form of password authentication is supported, alice
is basically empowered to break into any non-superuser account on the
system and assume all of its privileges. That's really not cool: it's
OK, I think, to give a non-superuser the right to change somebody
else's passwords, but it should be possible to limit it in some way,
e.g. to the users that alice creates. Also, while the ability to make
this sort of change seems to be the clear intention of the code, it's
not documented on the CREATE ROLE page. The problems with
pg_execute_server_program et. al. are not documented either; all it
says is that you should "regard roles that have the CREATEROLE
privilege as almost-superuser-roles," which seems to me to be
understating the extent of the problem.

I have drafted a few patches to try to improve the situation. It seems
to me that the root of any fix in this area must be to change the rule
that CREATEROLE can administer any role whatsoever. Instead, I propose
to change things so that you can only administer roles for which you
have ADMIN OPTION. Administering a role here includes changing the
password for a role, renaming a role, dropping a role, setting the
comment or security label on a role, or granting membership in that
role to another role, whether at role creation time or later. All of
these options are treated in essentially two places in the code, so it
makes sense to handle them all in a symmetric way. One problem with
this proposal is that, if we did exactly this much, then a CREATEROLE
user wouldn't be able to administer the roles which they themselves
had just created. That seems like it would be restricting the
privileges of CREATEROLE users too much.

To fix that, I propose when a non-superuser creates a role, the role
be implicitly granted back to the creator WITH ADMIN OPTION. This
arguably doesn't add any fundamentally new capability because the
CREATEROLE user could do something like "CREATE ROLE some_new_role
ADMIN myself" anyway, but that's awkward to remember and doing it
automatically seems a lot more convenient. However, there's a little
bit of trickiness here, too. Granting the new role back to the creator
might, depending on whether the INHERIT or SET flags are true or false
for the new grant, allow the CREATEROLE user to inherit the privileges
of, or set role to, the target role, which under current rules would
not be allowed. We can minimize behavior changes from the status quo
by setting up the new, implicit grant with SET FALSE, INHERIT FALSE.

However, that might not be what everyone wants. It's definitely not
what *I* want. I want a way for non-superusers to create new roles and
automatically inherit the privileges of those roles just as a
superuser automatically inherits everyone's privileges. I just don't
want the users who can do this to also be able to break out to the OS
as if they were superusers when they're not actually supposed to be.
However, it's clear from previous discussion that other people do NOT
want that, so I propose to make it configurable. Thus, this patch
series also proposes to add INHERITCREATEDROLES and SETCREATEDROLES
properties to roles. These have no meaning if the role is not marked
CREATEROLE. If it is, then they control the properties of the implicit
grant that happens when a CREATEROLE user who is not a superuser
creates a role. If INHERITCREATEDROLES is set, then the implicit grant
back to the creator is WITH INHERIT TRUE, else it's WITH INHERIT
FALSE; likewise, SETCREATEDROLES affects whether the implicit grant is
WITH SET TRUE or WITH SET FALSE.

I'm curious to hear what other people think of these proposals, but
let me first say what I think about them. First, I think it's clear
that we need to do something, because things right now are pretty
badly broken and in a way that affects security. Although these
patches are not back-patchable, they at least promise to improve
things as older versions go out of use. Second, it's possible that we
should look for back-patchable fixes here, but I can't really see that
we're going to come up with anything much better than just telling
people not to use this feature against older releases, because
back-patching catalog changes or dramatic behavior changes seems like
a non-starter. In other words, I think this is going to be a
master-only fix. Third, someone could well have a better or just
different idea how to fix the problems in this area than what I'm
proposing here. This is the best that I've been able to come up with
so far, but that's not to say it's free of problems or that no
improvements are possible.

Finally, I think that whatever we do about the code, the documentation
needs quite a bit of work, because the code is doing a lot of stuff
that is security-critical and entirely non-obvious from the
documentation. I have not in this version of these patches included
any documentation changes and the regression test changes that I have
included are quite minimal. That all needs to be fixed up before there
could be any thought of moving forward with these patches. However, I
thought it best to get rough patches and an outline of the proposed
direction on the table first, before doing a lot of work refining
things.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas:
> It seems
> to me that the root of any fix in this area must be to change the rule
> that CREATEROLE can administer any role whatsoever.

Agreed.

> Instead, I propose
> to change things so that you can only administer roles for which you
> have ADMIN OPTION. [...] > I'm curious to hear what other people think of these proposals, [...]
> Third, someone could well have a better or just
> different idea how to fix the problems in this area than what I'm
> proposing here.

Once you can restrict CREATEROLE to only manage "your own" (no matter 
how that is defined, e.g. via ADMIN or through some "ownership" concept) 
roles, the possibility to "namespace" those roles somehow will become a 
lot more important. For example in a multi-tenant setup in the same 
cluster, where each tenant has their own database and admin user with a 
restricted CREATEROLE privilege, it will very quickly be at least quite 
annoying to have conflicts with other tenants' role names. I'm not sure 
whether it could even be a serious problem, because I should still be 
able to GRANT my own roles to other roles from other tenants - and that 
could affect matching of +group records in pg_hba.conf?

My suggestion to $subject and the namespace problem would be to 
introduce database-specific roles, i.e. add a database column to 
pg_authid. Having this column set to 0 will make the role a cluster-wide 
role ("cluster role") just as currently the case. But having a database 
oid set will make the role exist in the context of that database only 
("database role"). Then, the following principles should be enforced:

- database roles can not share the same name with a cluster role.
- database roles can have the same name as database roles in other 
databases.
- database roles can not be members of database roles in **other** 
databases.
- database roles with CREATEROLE can only create or alter database roles 
in their own database, but not roles in other databases and also not 
cluster roles.
- database roles with CREATEROLE can GRANT all database roles in the 
same database, but only those cluster roles they have ADMIN privilege on.
- database roles with CREATEROLE can not set SUPERUSER.

To be able to create database roles with a cluster role, there needs to 
be some syntax, e.g. something like

CREATE ROLE name IN DATABASE dbname ...

A database role with CREATEROLE should not need to use that syntax, 
though - every CREATE ROLE should be IN DATABASE anyway.

With database roles, it would be possible to hand out CREATEROLE without 
the ability to grant SUPERUSER or any of the built-in roles. It would be 
much more useful on top of that, too. Not only is the namespace problem 
mentioned above solved, but it would also be possible to let pg_dump 
dump a whole database, including the database roles and their 
memberships. This would allow dumping (and restoring) a single 
tenant/application including the relevant roles and privileges - without 
dumping all roles in the cluster. Plus, it's backwards compatible 
because without creating database roles, everything stays exactly the same.

Best,

Wolfgang

On Tue, Nov 22, 2022 at 3:02 AM <walther@technowledgy.de> wrote:
> My suggestion to $subject and the namespace problem would be to
> introduce database-specific roles, i.e. add a database column to
> pg_authid. Having this column set to 0 will make the role a cluster-wide
> role ("cluster role") just as currently the case. But having a database
> oid set will make the role exist in the context of that database only
> ("database role"). Then, the following principles should be enforced:
>
> - database roles can not share the same name with a cluster role.
> - database roles can have the same name as database roles in other
> databases.
> - database roles can not be members of database roles in **other**
> databases.
> - database roles with CREATEROLE can only create or alter database roles
> in their own database, but not roles in other databases and also not
> cluster roles.
> - database roles with CREATEROLE can GRANT all database roles in the
> same database, but only those cluster roles they have ADMIN privilege on.
> - database roles with CREATEROLE can not set SUPERUSER.
>
> To be able to create database roles with a cluster role, there needs to
> be some syntax, e.g. something like
>
> CREATE ROLE name IN DATABASE dbname ...

I have three comments on this:

1. It's a good idea and might make for some interesting followup work.

2. There are some serious implementation challenges because the
constraints on duplicate object names must be something which can be
enforced by unique constraints on the relevant catalogs. Off-hand, I
don't see how to do that. It would be easy to make the cluster roles
all have unique names, and it would be easy to make the database roles
have unique names within each database, but I have no idea how you
would keep a database role from having the same name as a cluster
role. For anyone to try to implement this, we'd need to have a
solution to that problem.

3. I don't want to sidetrack this thread into talking about possible
future features or followup work. There is enough to do just getting
consensus on the design ideas that I proposed without addressing the
question of what else we might do later. I do not think there is any
reasonable argument that we can't clean up the CREATEROLE mess without
also implementing database-specific roles, and I have no intention of
including that in this patch series. Whether I or someone else might
work on it in the future is a question we can leave for another day.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas:
> 2. There are some serious implementation challenges because the
> constraints on duplicate object names must be something which can be
> enforced by unique constraints on the relevant catalogs. Off-hand, I
> don't see how to do that. It would be easy to make the cluster roles
> all have unique names, and it would be easy to make the database roles
> have unique names within each database, but I have no idea how you
> would keep a database role from having the same name as a cluster
> role. For anyone to try to implement this, we'd need to have a
> solution to that problem.

For each database created, create a partial unique index:

CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0, 
<database_oid>);

Is that possible on catalogs?

Best,

Wolfgang

walther@technowledgy.de writes:
> Robert Haas:
>> 2. There are some serious implementation challenges because the
>> constraints on duplicate object names must be something which can be
>> enforced by unique constraints on the relevant catalogs. Off-hand, I
>> don't see how to do that.

> For each database created, create a partial unique index:
> CREATE UNIQUE INDEX ... ON pg_authid (rolname) WHERE roldatabase IN (0, 
> <database_oid>);
> Is that possible on catalogs?

No, we don't support partial indexes on catalogs, and I don't think
we want to change that.  Partial indexes would require expression
evaluations occurring at very inopportune times.

Also, we don't support creating shared indexes post-initdb.
The code has hard-wired lists of which relations are shared,
besides which there's no way to update other databases' pg_class.

Even without that, the idea of a shared catalog ending up with 10000
indexes after you create 10000 databases (requiring 10^8 pg_class
entries across the whole cluster) seems ... unattractive.

            regards, tom lane

Tom Lane:
> No, we don't support partial indexes on catalogs, and I don't think
> we want to change that.  Partial indexes would require expression
> evaluations occurring at very inopportune times.

I see. Is that the same for indexes *on* an expression? Or would those 
be ok?

With a custom operator, an EXCLUDE constraint on the ROW(reldatabase, 
relname) expression could work. The operator would compare:
- (0, name1) and (0, name2) as name1 == name2
- (db1, name1) and (0, name2) as name1 == name2
- (0, name1) and (db2, name2) as name1 == name2
- (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2

or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2.

Now, you are going to tell me that EXCLUDE constraints are not supported 
on catalogs either, right? ;)

Best,

Wolfgang

Wolfgang Walther:
> Tom Lane:
>> No, we don't support partial indexes on catalogs, and I don't think
>> we want to change that.  Partial indexes would require expression
>> evaluations occurring at very inopportune times.
> 
> I see. Is that the same for indexes *on* an expression? Or would those 
> be ok?
> 
> With a custom operator, an EXCLUDE constraint on the ROW(reldatabase, 
> relname) expression could work. The operator would compare:
> - (0, name1) and (0, name2) as name1 == name2
> - (db1, name1) and (0, name2) as name1 == name2
> - (0, name1) and (db2, name2) as name1 == name2
> - (db1, name1) and (db2, name2) as db1 == db2 && name1 == name2
> 
> or just (db1 == 0 || db2 == 0 || db1 == db2) && name1 == name2.

Does it even need to be on the expression? I don't think so. It would be 
enough to just make it compare relname WITH = and reldatabase WITH the 
custom operator (db1 == 0 || db2 == 0 || db1 == db2), right?

Best,

Wolfgang

walther@technowledgy.de writes:
>> No, we don't support partial indexes on catalogs, and I don't think
>> we want to change that.  Partial indexes would require expression
>> evaluations occurring at very inopportune times.

> I see. Is that the same for indexes *on* an expression? Or would those 
> be ok?

Right, we don't support those on catalogs either.

> Now, you are going to tell me that EXCLUDE constraints are not supported 
> on catalogs either, right? ;)

Nor those.

            regards, tom lane

On 11/21/22 15:39, Robert Haas wrote:
> I'm curious to hear what other people think of these proposals, but
> let me first say what I think about them. First, I think it's clear
> that we need to do something, because things right now are pretty
> badly broken and in a way that affects security. Although these
> patches are not back-patchable, they at least promise to improve
> things as older versions go out of use.

+1

> Second, it's possible that we should look for back-patchable fixes
> here, but I can't really see that we're going to come up with
> anything much better than just telling people not to use this feature
> against older releases, because back-patching catalog changes or
> dramatic behavior changes seems like a non-starter. In other words, I
> think this is going to be a master-only fix.

Yep, seems highly likely

> Third, someone could well have a better or just different idea how to
> fix the problems in this area than what I'm proposing here. This is
> the best that I've been able to come up with so far, but that's not
> to say it's free of problems or that no improvements are possible.

On quick inspection I like what you have proposed and no significantly 
"better" ideas jump to mind. I will try to think on it though.

> Finally, I think that whatever we do about the code, the documentation
> needs quite a bit of work, because the code is doing a lot of stuff
> that is security-critical and entirely non-obvious from the
> documentation. I have not in this version of these patches included
> any documentation changes and the regression test changes that I have
> included are quite minimal. That all needs to be fixed up before there
> could be any thought of moving forward with these patches. However, I
> thought it best to get rough patches and an outline of the proposed
> direction on the table first, before doing a lot of work refining
> things.

I have looked at, and even done some doc improvements in this area in 
the past, and concluded that it is simply hard to describe it in a 
clear, straightforward way.

There are multiple competing concepts (privs on objects, attributes of 
roles, membership, when things are inherited versus not, settings bound 
to roles, etc). I don't know what to do about it, but yeah, fixing the 
documentation would be a noble goal.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

> On Nov 21, 2022, at 12:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> I have drafted a few patches to try to improve the situation.

The 0001 and 0002 patches appear to be uncontroversial refactorings.  Patch 0003 looks on-point and a move in the right
direction. The commit message in that patch is well written.  Patch 0004 feels like something that won't get committed.
The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems clunky. 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 22, 2022 at 3:01 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On Nov 21, 2022, at 12:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > I have drafted a few patches to try to improve the situation.
>
> The 0001 and 0002 patches appear to be uncontroversial refactorings.  Patch 0003 looks on-point and a move in the
rightdirection.  The commit message in that patch is well written.
 

Thanks.

> Patch 0004 feels like something that won't get committed.  The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems
clunky.

I think role properties are kind of clunky in general, the way we've
implemented them in PostgreSQL, but I don't really see why these are
worse than anything else. I think we need some way to control the
behavior, and I don't really see a reasonable place to put it other
than a per-role property. And if we're going to do that then they
might as well look like the other properties that we've already got.

Do you have a better idea?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Nov 22, 2022, at 2:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> Patch 0004 feels like something that won't get committed.  The INHERITCREATEDROLES and SETCREATEDROLES in 0004 seems
clunky.
>
> I think role properties are kind of clunky in general, the way we've
> implemented them in PostgreSQL, but I don't really see why these are
> worse than anything else. I think we need some way to control the
> behavior, and I don't really see a reasonable place to put it other
> than a per-role property. And if we're going to do that then they
> might as well look like the other properties that we've already got.
>
> Do you have a better idea?

Whatever behavior is to happen in the CREATE ROLE statement should be spelled out in that statement.  "CREATE ROLE bob
WITHINHERIT false WITH SET false" doesn't seem too unwieldy, and has the merit that it can be read and understood
withoutreference to hidden parameters.  Forcing this to be explicit should be safer if these statements ultimately make
theirway into dump/restore scripts, or into logical replication. 

That's not to say that I wouldn't rather that it always work one way or always the other.  It's just to say that I
don'twant it to work differently based on some poorly advertised property of the role executing the command. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 22, 2022 at 5:48 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Whatever behavior is to happen in the CREATE ROLE statement should be spelled out in that statement.  "CREATE ROLE
bobWITH INHERIT false WITH SET false" doesn't seem too unwieldy, and has the merit that it can be read and understood
withoutreference to hidden parameters.  Forcing this to be explicit should be safer if these statements ultimately make
theirway into dump/restore scripts, or into logical replication. 
>
> That's not to say that I wouldn't rather that it always work one way or always the other.  It's just to say that I
don'twant it to work differently based on some poorly advertised property of the role executing the command. 

That seems rather pejorative. If we stipulate from the outset that the
property is poorly advertised, then obviously anything at all
depending on it is not going to seem like a very good idea. But why
should we assume that it will be poorly-advertised? I clearly said
that the documentation needs a bunch of work, and that I planned to
work on it. As an initial matter, the documentation is where we
advertise new features, so I think you ought to take it on faith that
this will be well-advertised, unless you think that I'm completely
hopeless at writing documentation or something.

On the actual issue, I think that one key question is who should
control what happens when a role is created. Is that the superuser's
decision, or the CREATEROLE user's decision? I think it's better for
it to be the superuser's decision. Consider first the use case where
you want to set up a user who "feels like a superuser" i.e. inherits
the privileges of users they create. You don't want them to have to
specify anything when they create a role for that to happen. You just
want it to happen. So you want to set up their account so that it will
happen automatically, not throw the complexity back on them. In the
reverse scenario where you don't want the privileges inherited, I
think it's a little less clear, possibly just because I haven't
thought about that scenario as much, but I think it's very reasonable
here too to want the superuser to set up a configuration for the
CREATEROLE user that does what the superuser wants, rather than what
the CREATEROLE user wants.

Even aside from the question of who controls what, I think it is far
better from a usability perspective to have ways of setting up good
defaults. That is why we have the default_tablespace GUC, for example.
We could have made the CREATE TABLE command always use the database's
default tablespace, or we could have made it always use the main
tablespace. Then it would not be dependent on (poorly advertised?)
settings elsewhere. But it would also be really inconvenient, because
if someone is creating a lot of tables and wants them all to end up in
the same place, they don't want to have to specify the name of that
tablespace each time. They want to set a default and have that get
used by each command.

There's another, subtler consideration here, too. Since
ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who
can validly be recorded as the grantor of a particular role grant,
just as we have always done for other types of grants. The grants have
to form a tree, with each grant having a grantor that was themselves
granted ADMIN OPTION by someone else, until eventually you get back to
the bootstrap superuser who is the source of all privileges. Thus,
today, when a CREATEROLE user grants membership in a role, the grantor
is recorded as the bootstrap superuser, because they might not
actually possess ADMIN OPTION on the role at all, and so we can't
necessarily record them as the grantor. But this patch changes that,
which I think is a significant improvement. The implicit grant that is
created when CREATE ROLE is issued has the bootstrap superuser as
grantor, because there is no other legal option, but then any further
grants performed by the CREATE ROLE user rely on that user having that
grant, and thus record the OID of the CREATEROLE user as the grantor,
not the bootstrap superuser.

That, in turn, has a number of rather nice consequences. It means in
particular that the CREATEROLE user can't remove the implicit grant,
nor can they alter it. They are, after all, not the grantor, who is
the bootstrap superuser, nor do they any longer have the authority to
act as the bootstrap superuser. Thus, if you have two or more
CREATEROLE users running around doing stuff, you can tell who did
what. Every role that those users created is linked back to the
creating role in a way that the creator can't alter. A CREATEROLE user
is unable to contrive a situation where they no longer control a role
that they created. That also means that the superuser, if desired, can
revoke all membership grants performed by any particular CREATEROLE
user by revoking the implicit grants with CASCADE.

But since this implicit grant has, and must have, the bootstrap
superuser as grantor, it is also only reasonable that superusers get
to determine what options are used when creating that grant, rather
than leaving that up to the CREATEROLE user.

--
Robert Haas
EDB: http://www.enterprisedb.com

> On Nov 23, 2022, at 9:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> That's not to say that I wouldn't rather that it always work one way or always the other.  It's just to say that I
don'twant it to work differently based on some poorly advertised property of the role executing the command. 
>
> That seems rather pejorative. If we stipulate from the outset that the
> property is poorly advertised, then obviously anything at all
> depending on it is not going to seem like a very good idea. But why
> should we assume that it will be poorly-advertised? I clearly said
> that the documentation needs a bunch of work, and that I planned to
> work on it. As an initial matter, the documentation is where we
> advertise new features, so I think you ought to take it on faith that
> this will be well-advertised, unless you think that I'm completely
> hopeless at writing documentation or something.

Oh, I don't mean that it will be poorly documented.  I mean that the way the command is written won't advertise what it
isgoing to do.  That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and recreate
therole, only to discover that a property you weren't thinking about, and which you are accustomed to being set the
oppositeway, is set such that you can't drop the role you just created.  I think if you're going to create-and-disown
something,you should have to say so, to make sure you mean it. 

This consideration differs from the default schema or default tablespace settings.  If I fat-finger the creation of a
table,regardless of where it gets placed, I'm still the owner of the table, and I can still drop and recreate the table
tofix my mistake. 

Why not make this be a permissions issue, rather than a default behavior issue?  Instead of a single CREATEROLE
privilege,grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and
therebylimit which forms of the command they may execute.  That way, the semantics of the command do not depend on some
propertyexternal to the command.  Users (and older scripts) will expect the traditional syntax to behave consistent
withhow CREATE  ROLE has worked in the past.  The behaviors can be specified explicitly. 

> On the actual issue, I think that one key question is who should
> control what happens when a role is created. Is that the superuser's
> decision, or the CREATEROLE user's decision? I think it's better for
> it to be the superuser's decision. Consider first the use case where
> you want to set up a user who "feels like a superuser" i.e. inherits
> the privileges of users they create. You don't want them to have to
> specify anything when they create a role for that to happen. You just
> want it to happen. So you want to set up their account so that it will
> happen automatically, not throw the complexity back on them. In the
> reverse scenario where you don't want the privileges inherited, I
> think it's a little less clear, possibly just because I haven't
> thought about that scenario as much, but I think it's very reasonable
> here too to want the superuser to set up a configuration for the
> CREATEROLE user that does what the superuser wants, rather than what
> the CREATEROLE user wants.
>
> Even aside from the question of who controls what, I think it is far
> better from a usability perspective to have ways of setting up good
> defaults. That is why we have the default_tablespace GUC, for example.
> We could have made the CREATE TABLE command always use the database's
> default tablespace, or we could have made it always use the main
> tablespace. Then it would not be dependent on (poorly advertised?)
> settings elsewhere. But it would also be really inconvenient, because
> if someone is creating a lot of tables and wants them all to end up in
> the same place, they don't want to have to specify the name of that
> tablespace each time. They want to set a default and have that get
> used by each command.
>
> There's another, subtler consideration here, too. Since
> ce6b672e4455820a0348214be0da1a024c3f619f, there are constraints on who
> can validly be recorded as the grantor of a particular role grant,
> just as we have always done for other types of grants. The grants have
> to form a tree, with each grant having a grantor that was themselves
> granted ADMIN OPTION by someone else, until eventually you get back to
> the bootstrap superuser who is the source of all privileges. Thus,
> today, when a CREATEROLE user grants membership in a role, the grantor
> is recorded as the bootstrap superuser, because they might not
> actually possess ADMIN OPTION on the role at all, and so we can't
> necessarily record them as the grantor. But this patch changes that,
> which I think is a significant improvement. The implicit grant that is
> created when CREATE ROLE is issued has the bootstrap superuser as
> grantor, because there is no other legal option, but then any further
> grants performed by the CREATE ROLE user rely on that user having that
> grant, and thus record the OID of the CREATEROLE user as the grantor,
> not the bootstrap superuser.
>
> That, in turn, has a number of rather nice consequences. It means in
> particular that the CREATEROLE user can't remove the implicit grant,
> nor can they alter it. They are, after all, not the grantor, who is
> the bootstrap superuser, nor do they any longer have the authority to
> act as the bootstrap superuser. Thus, if you have two or more
> CREATEROLE users running around doing stuff, you can tell who did
> what. Every role that those users created is linked back to the
> creating role in a way that the creator can't alter. A CREATEROLE user
> is unable to contrive a situation where they no longer control a role
> that they created. That also means that the superuser, if desired, can
> revoke all membership grants performed by any particular CREATEROLE
> user by revoking the implicit grants with CASCADE.
>
> But since this implicit grant has, and must have, the bootstrap
> superuser as grantor, it is also only reasonable that superusers get
> to determine what options are used when creating that grant, rather
> than leaving that up to the CREATEROLE user.

Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a
setting?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Oh, I don't mean that it will be poorly documented.  I mean that the way the command is written won't advertise what
itis going to do.  That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and
recreatethe role, only to discover that a property you weren't thinking about, and which you are accustomed to being
setthe opposite way, is set such that you can't drop the role you just created. 

That doesn't ever happen. No matter how the properties are set, you
end up with ADMIN OPTION on the newly-created role and can drop it.
The flags control things like whether you can select from the newly
created role's tables even if you otherwise lack permissions on them
(INHERIT), and whether you can SET ROLE to it (SET). You can always
administer it, i.e. grant rights on it to others, change its password,
drop it.

> I think if you're going to create-and-disown something, you should have to say so, to make sure you mean it.

Reasonable, but not relevant, since that isn't what's happening.

> Why not make this be a permissions issue, rather than a default behavior issue?  Instead of a single CREATEROLE
privilege,grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and
therebylimit which forms of the command they may execute.  That way, the semantics of the command do not depend on some
propertyexternal to the command.  Users (and older scripts) will expect the traditional syntax to behave consistent
withhow CREATE  ROLE has worked in the past.  The behaviors can be specified explicitly. 

Perhaps if we get the confusion above cleared up you won't be as
concerned about this, but let me just say that this patch is
absolutely breaking backward compatibility. I don't feel bad about
that, either. I think it's a good thing in this case, because the
current behavior is abjectly broken and horrible. What we've been
doing for the last several years is shipping a product that has a
built-in exploit that a clever 10-year-old could use to escalate
privileges from CREATEROLE to SUPERUSER. We should not be OK with
that, and we should be OK with changing the behavior however much is
required to fix it. I'm personally of the opinion that this patch set
does a rather clever job minimizing that blast radius, but that might
be my own bias as the patch author. Regardless, I don't think there's
any reasonable argument for maintaining the current behavior. I don't
entirely follow exactly what you have in mind in the sentence above,
but if it involves keeping the current CREATEROLE behavior around in
any form, -1 from me.

> > But since this implicit grant has, and must have, the bootstrap
> > superuser as grantor, it is also only reasonable that superusers get
> > to determine what options are used when creating that grant, rather
> > than leaving that up to the CREATEROLE user.
>
> Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a
setting?

Well, we certainly don't HAVE to add those new role-level properties;
that's why they are in a separate patch. But I think they add a lot of
useful functionality for a pretty minimal amount of extra code
complexity.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>> Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a
setting?

> Well, we certainly don't HAVE to add those new role-level properties;
> that's why they are in a separate patch. But I think they add a lot of
> useful functionality for a pretty minimal amount of extra code
> complexity.

I haven't thought about these issues hard enough to form an overall
opinion (though I agree that making CREATEROLE less tantamount
to superuser would be an improvement).  However, I share Mark's
discomfort about making these commands act differently depending on
context.  We learned long ago that allowing GUCs to affect query
semantics was a bad idea.  Basing query semantics on properties
of the issuing role (beyond success-or-permissions-failure) doesn't
seem a whole lot different from that.  It still means that
applications can't just issue command X and expect Y to happen;
they have to inquire about context in order to find out that Z might
happen instead.  That's bad in any case, but it seems especially bad
for security-critical behaviors.

            regards, tom lane

On Wed, Nov 23, 2022 at 1:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I haven't thought about these issues hard enough to form an overall
> opinion (though I agree that making CREATEROLE less tantamount
> to superuser would be an improvement).  However, I share Mark's
> discomfort about making these commands act differently depending on
> context.  We learned long ago that allowing GUCs to affect query
> semantics was a bad idea.  Basing query semantics on properties
> of the issuing role (beyond success-or-permissions-failure) doesn't
> seem a whole lot different from that.  It still means that
> applications can't just issue command X and expect Y to happen;
> they have to inquire about context in order to find out that Z might
> happen instead.  That's bad in any case, but it seems especially bad
> for security-critical behaviors.

I'm not sure that this behavior qualifies as security-critical. If
INHERITCREATEDROLES and SETCREATEDROLES are both true, then the grant
has INHERIT TRUE and SET TRUE and there are no more rights to be
gained. If not, the createrole user can do something like GRANT
new_role TO my_own_account WITH INHERIT TRUE, SET TRUE. Even if we
somehow disallowed that, they could gain access to the privilege of
the created role in a bunch of other ways, such as granting the rights
to someone else, or just changing the password and using the new
password to log into the account.

When I started working in this area, I thought non-inherited grants
were pretty useless, because you can so easily work around it.
However, other people did not agree. From what I can gather, I think
the reason why people like non-inherited grants is that they prevent
mistakes. A user who has ADMIN OPTION on another role but does not
inherit its privileges can break into that account and do whatever
they want, but they cannot ACCIDENTALLY perform an operation that
makes use of that user's privileges. They will have to SET ROLE, or
GRANT themselves something, and those actions can be logged and
audited if desired. Because of the potential for that sort of logging
and auditing, you can certainly make an argument that this is a
security-critical behavior, but it's not that clear cut, because it's
making assumptions about the behavior of other software, and of human
beings. Strictly speaking, looking just at PostgreSQL, these options
don't affect security.

On the more general question of configurability, I tend to agree that
it's not great to have the behavior of commands depend too much on
context, especially for security-critical things. A particularly toxic
example IMHO is search_path, which I think is an absolute disaster in
terms of security that I believe we will never be able to fully fix.
Yet there are plenty of examples of configurability that no one finds
problematic. No one agitates against the idea that a database can have
a default tablespace, or that you can ALTER USER or ALTER DATABASE to
configure an setting on a user-specific or database-specific setting,
even a security-critical one like search_path, or one that affects
query behavior like work_mem. No one is outraged that a data type has
a default btree operator class that is used for indexes unless you
specify another one explicitly. What people mostly complain about IME
is stuff like standard_conforming_strings, or bytea_output, or
datestyle. Often, when proposal come up on pgsql-hackers and get shot
down on these grounds, the issue is that they would essentially make
it impossible to write SQL that will run portably on PostgreSQL.
Instead, you'd need to write your application to issue different SQL
depending on the value of settings on the local system. That's un-fun
at best, and impossible at worst, as in the case of extension scripts,
whose content has to be static when you ship the thing.

But it's not exactly clear to me what the broader organizing principle
is here, or ought to be. I think it would be ridiculous to propose --
and I assume that you are not proposing -- that no command should have
behavior that in any way depends on what SQL commands have been
executed previously. Taken to a silly extreme, that would imply that
CREATE TABLE ought to be removed, because the behavior of SELECT *
FROM something will otherwise depend on whether someone has previously
issued CREATE TABLE something. Obviously that is a stupid argument.
But on the other hand, it would also be ridiculous to propose the
reverse, that it's fine to add arbitrary settings that affect the
behavior of any command whatsoever in arbitrary ways. Simon's proposal
to add a GUC that would make vacuum request a background vacuum rather
than performing one in the foreground is a good example of a proposal
that did not sit well with either of us.

But I don't know on what basis exactly we put a proposal like this in
one category rather than the other. I'm not sure I can really
articulate the general principle in a sensible way. For me, this
clearly falls into the "good" category: it's configuration that you
put into the database that makes things happen the way you want, not a
behavior-changing setting that comes along and ruins somebody's day.
But if someone else feels otherwise, I'm not sure I can defend that
view in a really rigorous way, because I'm not really sure what the
litmus test is, or should be. I think the best that I can do is to say
that if we *don't* add those options but *do* adopt the rest of the
patch set, we will have to make a decision about what behavior
everyone is going to get, and no matter what we decide, some people
are not going to be really unhappy with the result. I would like to
find a way to avoid that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Nov 23, 2022, at 11:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> For me, this
> clearly falls into the "good" category: it's configuration that you
> put into the database that makes things happen the way you want, not a
> behavior-changing setting that comes along and ruins somebody's day.

I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE to Alice with particular settings, those
settingswould limit the things that Alice could do when creating role Bob, specifically limiting how much she could
administer/inherit/setrole Bob thereafter.  Apparently, your proposal only configures what happens by default, and
Alicecan work around that if she wants to.  But if that's the case, did I misunderstand upthread that these are
propertiesthe superuser specifies about Alice?  Can Alice just set these properties about herself, so she gets the
behaviorshe wants?  I'm confused now about who controls these settings. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 23, 2022 at 2:28 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On Nov 23, 2022, at 11:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > For me, this
> > clearly falls into the "good" category: it's configuration that you
> > put into the database that makes things happen the way you want, not a
> > behavior-changing setting that comes along and ruins somebody's day.
>
> I had incorrectly imagined that if the bootstrap superuser granted CREATEROLE to Alice with particular settings,
thosesettings would limit the things that Alice could do when creating role Bob, specifically limiting how much she
couldadminister/inherit/set role Bob thereafter.  Apparently, your proposal only configures what happens by default,
andAlice can work around that if she wants to. 

Right.

> But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice?
CanAlice just set these properties about herself, so she gets the behavior she wants?  I'm confused now about who
controlsthese settings. 

Because they are role-level properties, they can be set by whoever has
ADMIN OPTION on the role. That always includes every superuser, and it
never includes Alice herself (except if she's a superuser). It could
include other users depending on the system configuration. For
example, under this proposal, the superuser might create alice and set
her account to CREATEROLE, configuring the INHERITCREATEDROLES and
SETCREATEDROLES properties on Alice's account according to preference.
Then, alice might create another user, say bob, and make him
CREATEROLE as well. In such a case, either the superuser or alice
could set these properties for role bob, because alice enjoys ADMIN
OPTION on role bob.

Somewhat to one side of the question you were asking, but related to
the above, I believe there is an opportunity, and perhaps a need, to
modify the scope of CREATEROLE in terms of what role-level options a
CREATEROLE user can set. For instance, if a CREATEROLE user doesn't
have CREATEDB, they can still create users and give them that
privilege, even with these patches, and likewise these two new
properties. This patch is only concerned about which roles you can
manipulate, not what role-level properties you can set. Somebody might
feel that's a serious problem, and they might even feel that this
patch set ought to something about it. In my view, the issues are
somewhat severable. I don't think you can do anything as evil by
setting role-level properties (except for SUPERUSER, of course) as
what you can do by granting predefined roles, so I don't find
restricting those capabilities to be as urgent as doing something to
restrict role grants.

Also, and this definitely plays into it too, I think there's some
debate about what the model ought to look like there. For instance,
you could simply stipulate that you can't give what you don't have,
but that would mean that every  CREATEROLE user can create additional
CREATEROLE users, and I suspect some people might like to restrict
that. We could add a new CREATECREATEROLE property to decide whether a
user can make CREATEROLE users, but by that argument we'd also need a
new CREATECREATECREATEROLE property to decide whether a role can make
CREATECREATEROLE users, and then it just recurses indefinitely from
there. Similarly for CREATEDB. Also, what if anything should you
restrict about how the new INHERITCREATEDROLES and SETCREATEDROLES
properties should be set? You could argue that they ought to be
superuser-only (because the implicit grant is performed by the
bootstrap superuser) or that it's fine for them to be set by a
CREATEROLE user with ADMIN OPTION (because it's not all that
security-critical how they get set) or maybe even that a user ought to
be able to set those properties on his or her own role.

I'm not very certain about any of that stuff; I don't have a clear
mental model of how it should work, or even what exact problem we're
trying to solve. To me, the patches that I posted make sense as far as
they go, but I'm not under the illusion that they solve all the
problems in this area, or even that I understand what all of the
problems are.

--
Robert Haas
EDB: http://www.enterprisedb.com

> On Nov 23, 2022, at 12:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> But if that's the case, did I misunderstand upthread that these are properties the superuser specifies about Alice?
CanAlice just set these properties about herself, so she gets the behavior she wants?  I'm confused now about who
controlsthese settings. 
>
> Because they are role-level properties, they can be set by whoever has
> ADMIN OPTION on the role. That always includes every superuser, and it
> never includes Alice herself (except if she's a superuser). It could
> include other users depending on the system configuration. For
> example, under this proposal, the superuser might create alice and set
> her account to CREATEROLE, configuring the INHERITCREATEDROLES and
> SETCREATEDROLES properties on Alice's account according to preference.
> Then, alice might create another user, say bob, and make him
> CREATEROLE as well. In such a case, either the superuser or alice
> could set these properties for role bob, because alice enjoys ADMIN
> OPTION on role bob.

Ok, so the critical part of this proposal is that auditing tools can tell when Alice circumvents these settings.
Withoutthat bit, the whole thing is inane.  Why make Alice jump through hoops that you are explicitly allowing her to
jumpthrough?  Apparently the answer is that you can point a high speed camera at the hoops. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 23, 2022 at 2:28 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>> I had incorrectly imagined that if the bootstrap superuser granted
>> CREATEROLE to Alice with particular settings, those settings would
>> limit the things that Alice could do when creating role Bob,
>> specifically limiting how much she could administer/inherit/set role
>> Bob thereafter.  Apparently, your proposal only configures what happens
>> by default, and Alice can work around that if she wants to.

> Right.

Okay ...

>> But if that's the case, did I misunderstand upthread that these are
>> properties the superuser specifies about Alice?  Can Alice just set
>> these properties about herself, so she gets the behavior she wants?
>> I'm confused now about who controls these settings.

> Because they are role-level properties, they can be set by whoever has
> ADMIN OPTION on the role. That always includes every superuser, and it
> never includes Alice herself (except if she's a superuser).

That is just bizarre.  Alice can do X, and she can do Y, but she
can't control a flag that says which of those happens by default?
How is that sane (disregarding the question of whether the existence
of the flag is a good idea, which I'm now even less sold on)?

            regards, tom lane

On Wed, Nov 23, 2022 at 3:11 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Ok, so the critical part of this proposal is that auditing tools can tell when Alice circumvents these settings.
Withoutthat bit, the whole thing is inane.  Why make Alice jump through hoops that you are explicitly allowing her to
jumpthrough?  Apparently the answer is that you can point a high speed camera at the hoops. 

Well put.

Also, it's a bit like 'su', right? Typically you don't just log in as
root and do everything a root, even if you have access to root
privileges. You log in as 'mdilger' or whatever and then when you want
to exercise elevated privileges you use 'su' or  'sudo' or something.
Similarly here you can make an argument that it's a lot cleaner to
give Alice the potential to access all of these privileges than to
make her have them all the time.

But on the flip side, one big advantage of having 'alice' have the
privileges all the time is that, for example, she can probably restore
a database dump that might otherwise be restorable only with superuser
privileges. As long as she has been granted all the relevant roles
with INHERIT TRUE, SET TRUE, the kinds of locutions that pg_dump spits
out should pretty much work fine, whereas if Alice is firewalled from
the privileges of the roles she manages, that is not going to work
well at all. To me, that is a pretty huge advantage, and it's a major
reason why I initially thought that alice should just categorically,
always inherit the privileges of the roles she creates.

But having been burned^Wenlightened by previous community discussion,
I can now see both sides of the argument, which is why I am now
proposing to let people pick the behavior they happen to want.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Nov 23, 2022 at 3:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Because they are role-level properties, they can be set by whoever has
> > ADMIN OPTION on the role. That always includes every superuser, and it
> > never includes Alice herself (except if she's a superuser).
>
> That is just bizarre.  Alice can do X, and she can do Y, but she
> can't control a flag that says which of those happens by default?
> How is that sane (disregarding the question of whether the existence
> of the flag is a good idea, which I'm now even less sold on)?

Look, I admitted later in that same email that I don't really know
what the rules for setting role-level properties ought to be. If you
have an idea, I'd love to hear it, but I'd rather if you didn't just
label things into which I have put quite a bit of work as insane
without giving any constructive feedback, especially if you haven't
yet fully understood the proposal.

Your description of the behavior here is not quite accurate.
Regardless of how the flags are set, alice, as a CREATEROLE user, can
gain access to all the privileges of the target role, and she can
arrange to have a grant of permissions on that role with INHERIT TRUE
and SET TRUE. However, there's a difference between the case where (a)
INHERITCREATEDROLE and SETCREATEDROLE are set, and alice gets the
permissions of the role by default and the one where (b)
NOINHERITCREATEDROLE and NOSETCREATEDROLE are set, and therefore alice
gets the permissions only if she does GRANT created_role TO ALICE WITH
INHERIT TRUE, SET TRUE. In the former case, there is only one grant,
and it has grantor=bootstrap_superuser/admin_option=true/inherit_option=true/set_option=true.
In the latter case there are two, one with
grantor=bootstrap_supeuser/admin_option=true/set_option=false/inherit_option=false
and a second with
grantor=alice/admin_option=false/set_option=true/inherit_option=true.
That is pretty nearly equivalent, but it is not the same, and it will
not, for example, be dumped in the same way. Furthermore, it's not
equivalent in the other direction at all. If the superuser gives alice
INHERITCREATEDROLES and SETCREATEDROLES, she can't renounce those
permissions in the patch as written. All of which is to say that I
don't think your characterization of this as "Alice can do X, and she
can do Y, but she can't control a flag that says which of those
happens by default?" is really correct. It's subtler than that.

But having said that, I could certainly change the patches so that any
user, or maybe just a createrole user since it's otherwise irrelevant,
can flip the INHERITCREATEDROLE and SETCREATEDROLE bits on their own
account. There would be no harm in that from a security or auditing
perspective AFAICS. It would, however, make the handling of those
flags different from the handling of most other role-level properties.
That is in fact why things ended up the way that they did: I just made
the new role-level properties which I added work like most of the
existing ones. I don't think that's insane at all. I even think it
might be the right decision. But it's certainly arguable. If you think
it should work differently, make an argument for that. What I would
particularly like to hear in such an argument, though, is a theory
that goes beyond those two particular properties and addresses what
ought to be done with all the other ones, especially CREATEDB and
CREATEROLE. If we can't come up with such a grand unifying theory but
are confident we know what to do about this case, so be it, but we
shouldn't make an idiosyncratic rule for this case without at least
thinking about the overall picture.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Nov 23, 2022 at 3:59 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> I haven't yet formed a complete thought here but is there any reason we cannot convert the permission-like attributes
topredefined roles?
 
>
> pg_login
> pg_replication
> pg_bypassrls
> pg_createdb
> pg_createrole
> pg_haspassword (password and valid until)
> pg_hasconnlimit
>
> Presently, attributes are never inherited, but having that be controlled via the INHERIT property of the grant seems
desirable.

I think that something like this might be possible, but I'm not
convinced that it's a good idea. I've always felt that the role-level
properties seemed kind of like warts, but in studying these issues
recently, I've come to the conclusion that in some ways that's just a
visual impression. The syntax LOOKS outdated and clunky, whereas
granting someone a predefined role feels clean and modern. But the
reality is that the predefined roles system is full of really
unpleasant warts. For example, in talking through the now-committed
patch to allow control over SET ROLE, we had some fairly extensive
discussion of the fact that there was previously no way to avoid
having a user who has been granted the pg_read_all_stats predefined
role to create objects owned by pg_read_all_stats, or to alter
existing objects. That's really pretty grotty. We now have a way to
prevent that, but perhaps we should have something even better. I'm
also not really sure that's the only problem here, but maybe it is.

Either way, I'm not quite sure what the benefit of converting these
things to predefined roles is. I think actually the strongest argument
would be to do this for the superuser property! Make the bootstrap
superuser the only real superuser, and anyone else who wants to be a
superuser has to inherit that from that role. It's really unclear to
me why inheriting a lot of permissions is allowable, but inheriting
all of them is not allowable. Doing it for something like
pg_hasconnlimit seems pretty unappealing by contrast, because that's
an integer-valued property, not a Boolean, and it's not at all clear
to me why that should be inherited or what the semantics ought to be.
Really, I'm not that tempted to try to rejigger this kind of stuff
around because it seems like a lot of work for not a whole lot of
benefit. I think there's a perfectly reasonable case for setting some
things on a per-role basis that are actually per-role and not
inherited. A password is a fine example of that. You should never
inherit someone else's password. Whether we've chosen the right set of
things to treat as per-role properties rather than predefined roles is
very much debatable, though, as are a number of other aspects of the
role system.

For instance, I'm pretty well unconvinced that merging users and
groups into a uniformed thing called roles was a good idea. I think it
makes all of this machinery a LOT harder to understand, which may be
part of the reason why this area doesn't seem to have had much TLC in
quite a long time. But I think it's too late to revisit that decision,
and I also think it's too late to revisit the question of having
predefined roles at all. For better or for worse, that's what we did,
and what remains now is to find a way to make the best of it in light
of those decisions.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> But having said that, I could certainly change the patches so that any
> user, or maybe just a createrole user since it's otherwise irrelevant,
> can flip the INHERITCREATEDROLE and SETCREATEDROLE bits on their own
> account. There would be no harm in that from a security or auditing
> perspective AFAICS. It would, however, make the handling of those
> flags different from the handling of most other role-level properties.
> That is in fact why things ended up the way that they did: I just made
> the new role-level properties which I added work like most of the
> existing ones.

To be clear, I'm not saying that I know a better answer.  But the fact
that these end up so different from other role-property bits seems to
me to suggest that making them role-property bits is the wrong thing.
They aren't privileges in any usual sense of the word --- if they
were, allowing Alice to flip her own bits would obviously be silly.
But all the existing role-property bits, with the exception of
rolinherit, certainly are in the nature of privileges.

> What I would
> particularly like to hear in such an argument, though, is a theory
> that goes beyond those two particular properties and addresses what
> ought to be done with all the other ones, especially CREATEDB and
> CREATEROLE.

CREATEDB and CREATEROLE don't particularly bother me.  We've talked before
about replacing them with memberships in predefined roles, and that would
be fine.  But the reason nobody's got around to that (IMNSHO) is that it
won't really add much.  The thing that I think is a big wart is
rolinherit.  I don't know quite what to do about it.  But these two new
proposed bits seem to be much the same kind of wart, so I'd rather not
invent them, at least not in the form of role properties.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wed, Nov 23, 2022 at 2:18 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> Either way, I'm not quite sure what the benefit of converting these
>> things to predefined roles is.

> Specifically, you gain inheritance/set and "admin option" for free.

Right: the practical issue with CREATEROLE/CREATEDB is that you need
some mechanism for managing who can grant those privileges.  The
current answer isn't very flexible, which has been complained of
repeatedly.  If they become predefined roles then we get a lot of
already-built-out infrastructure to solve that, instead of having to
write even more single-purpose logic.  I think it's a sensible future
path, but said lack of flexibility hasn't yet spurred anyone to do it.

            regards, tom lane

On Wed, Nov 23, 2022 at 4:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> To be clear, I'm not saying that I know a better answer.  But the fact
> that these end up so different from other role-property bits seems to
> me to suggest that making them role-property bits is the wrong thing.
> They aren't privileges in any usual sense of the word --- if they
> were, allowing Alice to flip her own bits would obviously be silly.
> But all the existing role-property bits, with the exception of
> rolinherit, certainly are in the nature of privileges.

I think that's somewhat true, but I don't completely agree. I don't
think that INHERIT, LOGIN, CONNECTION LIMIT, PASSWORD, or VALID UNTIL
are privileges either. I think they're just properties. I would put
these in the same category: properties, not privileges. I think that
SUPERUSER, CREATEDB, CREATEROLE, REPLICATION, and BYPASSRLS are
privileges.

> CREATEDB and CREATEROLE don't particularly bother me.  We've talked before
> about replacing them with memberships in predefined roles, and that would
> be fine.  But the reason nobody's got around to that (IMNSHO) is that it
> won't really add much.

I agree, although I'm not sure that means that we don't need to do
anything about them as we evolve the system.

> The thing that I think is a big wart is
> rolinherit.  I don't know quite what to do about it.

One option is to nuke it from orbit. Now that you can set that
property on a per-grant basis, the per-role basis serves only to set
the default. I think that's of dubious value, and arguably backwards,
because ISTM that in a lot of cases whether you want a role grant to
be inherited will depend on the nature of the role being granted
rather than the role to which it is being granted. The setting we have
works the other way around, and I can never keep in my head what the
use case for that is. I think there must be one, though, because Peter
Eisentraut seemed to like having it around. I don't understand why,
but I respect Peter. :-)

> But these two new
> proposed bits seem to be much the same kind of wart, so I'd rather not
> invent them, at least not in the form of role properties.

I have to admit that when I realized that was the natural place to put
them to make the patch work, my first reaction internally was "well,
that can't possibly be right, role properties suck!". But I didn't and
still don't see where else to put them that makes any sense at all, so
I eventually decided that my initial reaction was misguided. So I
can't really blame you for not liking it either, and would be happy if
we could come up with something else that feels better. I just don't
know what it is: at least as of this moment in time, I believe these
naturally ARE properties of the role, and therefore I'm inclined to
view my initial reluctance to implement it that way as a reflex rather
than a well-considered opinion. That is, the CREATE ROLE syntax is
clunky, and it controls some things that are properties and others
that are permissions, but they're not inherited like regular
permissions, so it stinks, and thus adding things to it also feels
stinky. But if the existing command weren't such a mess I'm not sure
adding this stuff to it would feel bad either.

That might be the wrong view. As I say, I'm open to other ideas, and
it's possible there's some nicer way to do it that I just don't see
right now.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas:
> I have to admit that when I realized that was the natural place to put
> them to make the patch work, my first reaction internally was "well,
> that can't possibly be right, role properties suck!". But I didn't and
> still don't see where else to put them that makes any sense at all, so
> I eventually decided that my initial reaction was misguided. So I
> can't really blame you for not liking it either, and would be happy if
> we could come up with something else that feels better. I just don't
> know what it is: at least as of this moment in time, I believe these
> naturally ARE properties of the role [...]
> 
> That might be the wrong view. As I say, I'm open to other ideas, and
> it's possible there's some nicer way to do it that I just don't see
> right now.

INHERITCREATEDROLES and SETCREATEDROLES behave much like DEFAULT 
PRIVILEGES. What about something like:

ALTER DEFAULT PRIVILEGES FOR alice
GRANT TO alice WITH INHERIT FALSE, SET TRUE, ADMIN TRUE

The "abbreviated grant" is very much abbreviated, because the original 
syntax GRANT a TO b is already quite short to begin with, i.e. there is 
no ON ROLE or something like that in it.

The initial DEFAULT privilege would be INHERIT FALSE, SET FALSE, ADMIN 
TRUE, I guess?

Best,

Wolfgang

On Thu, Nov 24, 2022 at 2:41 AM <walther@technowledgy.de> wrote:
> INHERITCREATEDROLES and SETCREATEDROLES behave much like DEFAULT
> PRIVILEGES. What about something like:
>
> ALTER DEFAULT PRIVILEGES FOR alice
> GRANT TO alice WITH INHERIT FALSE, SET TRUE, ADMIN TRUE
>
> The "abbreviated grant" is very much abbreviated, because the original
> syntax GRANT a TO b is already quite short to begin with, i.e. there is
> no ON ROLE or something like that in it.
>
> The initial DEFAULT privilege would be INHERIT FALSE, SET FALSE, ADMIN
> TRUE, I guess?

I don't know if changing the syntax from A to B is really getting us
anywhere. I generally agree that the ALTER DEFAULT PRIVILEGES syntax
looks nicer than the CREATE/ALTER ROLE syntax, but I'm not sure that's
a sufficient reason to move the control over this behavior to ALTER
DEFAULT PRIVILEGES. One thing to consider is that, as I've designed
this, whether or not ADMIN is included in the grant is non-negotiable.
I am, at least at present, inclined to think that was the right call,
partly because Mark Dilger expressed a lot of concern about the
CREATEROLE user losing control over the role they'd just created, and
allowing ADMIN to be turned off would have exactly that effect. Plus a
grant with INHERIT FALSE, SET FALSE, ADMIN FALSE would end up being
almost identical to no great at all, which seems pointless. Basically,
without ADMIN, the implicit GRANT fails to accomplish its intended
purpose, so I don't like having that as a possibility.

The other thing that's a little weird about the syntax which you
propose is that it's not obviously related to CREATE ROLE. The intent
of the patch as implemented is to allow control over only the implicit
GRANT that is created when a new role is created, not all grants that
might be created by or to a particular user. Setting defaults for all
grants doesn't seem like a particularly good idea to me, but it's
definitely a different idea than what the patch proposes to do.

I did spend some time thinking about trying to tie this to the
CREATEROLE syntax itself. For example, instead of CREATE ROLE alice
CREATEROLE INHERITCREATEDROLES SETCREATEDROLES you could write CREATE
ROLE alice CREATEROLE WITH (INHERIT TRUE, SET TRUE) or something like
this. That would avoid introducing new, lengthy keywords that are just
concatenations of other English words, a kind of syntax that doesn't
look particularly nice to me and probably is less friendly to
non-English speakers as well. I didn't do it that way because the
parser support would be more complex, but I could. CREATEROLE would
have to become a keyword again, but that's not a catastrophe.

Another idea would be to break the CREATEROLE stuff off from CREATE
ROLE entirely and put it all into GRANT. You could imagine syntax like
GRANT CREATEROLE (or CREATE ROLE?) TO role_specification WITH (INHERIT
TRUE/FALSE, SET TRUE/FALSE). There are a few potential issues with
this direction. One, if we did this, then CREATEROLE probably ought to
become inheritable, because that's the way grants work in general, and
this likely shouldn't be an exception, but this would be a behavior
change. However, if it is the consensus that such a behavior change
would be an improvement, that might be OK. Two, I wonder what we'd do
about the GRANTED BY role_specification clause. We could leave it out,
but that would be asymmetric with other GRANT commands. We could also
support it and record that information and make this work more like
other cases, including, I suppose, the possibility of dependent
grants. We'd have to think about what that means exactly. If you
revoke CREATEROLE from someone who has granted CREATEROLE to others, I
suppose that's a clear dependent grant and needs to be recursively
revoked. But what about the implicit grants that were created because
the person had CREATEROLE? Are those also dependent grants? And what
about the roles themselves? Should revoking CREATEROLE drop the roles
that the user in question created? That gets complicated, because
those roles might own objects. That's scary, because you might not
expect revoking a role permission to result in tables getting dropped.
It's also problematic, because those tables might be in some other
database where they are inaccessible to the current session. All in
all I'm inclined to think that recursing to the roles themselves is a
bad plan, but it's debatable.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas:
> I don't know if changing the syntax from A to B is really getting us
> anywhere. I generally agree that the ALTER DEFAULT PRIVILEGES syntax
> looks nicer than the CREATE/ALTER ROLE syntax, but I'm not sure that's
> a sufficient reason to move the control over this behavior to ALTER
> DEFAULT PRIVILEGES.

The list of role attributes can currently be roughly divided into the 
following categories:
- Settings with role-specific values: CONNECTION LIMIT, PASSWORD, VALID 
UNTIL. It's hard to imagine storing them anywhere else, because they 
need to have a different value for each role. Those are not just "flags" 
like all the other attributes.
- Two special attributes in INHERIT and BYPASSRLS regarding 
security/privileges. Those were invented because there was no other 
syntax to do the same thing. Those could be interpreted as privileges to 
do something, too - but lacking the ability to do that explicit. There 
is no SET BYPASSRLS ON/OFF or SET INHERIT ON/OFF. Of course the INHERIT 
case is now a bit different, because there is the inherit grant option 
you introduced.
- Cluster-wide privileges: SUPERUSER, CREATEDB, CREATEROLE, LOGIN, 
REPLICATION. Those can't be granted on some kind of object, because 
there is no such global object. You could imagine inventing some kind of 
global CLUSTER object and do something like GRANT SUPERUSER ON CLUSTER 
TO alice; instead. Turning those into role attributes was the choice 
made instead. Most likely it would have been only a syntactic difference 
anyway: Even if there was something like GRANT .. ON CLUSTER, you'd most 
likely implement that as... storing those grants as role attributes.

Your patch is introducing a new category of role attributes - those that 
are affecting default behavior. But there is already a way to express 
this right now, and that's ALTER DEFAULT PRIVILEGES in this case. Imho, 
the question asked should not be "why change from syntax A to B?" but 
rather: Why introduce a new category of role attributes, when there is a 
way to express the same concept already? I can't see any compelling 
reason for that, yet.

Or not "yet", but rather "anymore". When I understood and remember 
correctly, you implemented it in a way that a user could not change 
those new attributes on their own role. This is in fact different to how 
ALTER DEFAULT PRIVILEGES works, so you could have made an argument that 
this was better expressed as role attributes. But I think this was asked 
and agreed on to act differently, so that the user can change this 
default behavior of what happens when they create a role for themselves. 
And now this reason is gone - there is no reason NOT to implement it as 
DEFAULT PRIVILEGES.

> One thing to consider is that, as I've designed
> this, whether or not ADMIN is included in the grant is non-negotiable.
> I am, at least at present, inclined to think that was the right call,
> partly because Mark Dilger expressed a lot of concern about the
> CREATEROLE user losing control over the role they'd just created, and
> allowing ADMIN to be turned off would have exactly that effect. Plus a
> grant with INHERIT FALSE, SET FALSE, ADMIN FALSE would end up being
> almost identical to no great at all, which seems pointless. Basically,
> without ADMIN, the implicit GRANT fails to accomplish its intended
> purpose, so I don't like having that as a possibility.

With how you implemented it right now, is it possible to do the following?

CREATE ROLE alice;
REVOKE ADMIN OPTION FOR alice FROM CURRENT_USER;

If the answer is yes, then there is no reason to allow a user to set a 
shortcut for SET and INHERIT, but not for ADMIN.

If the answer is no, then you could just not allow specifying the ADMIN 
option in the ALTER DEFAULT PRIVILEGES statement and always force it to 
be TRUE.


> The other thing that's a little weird about the syntax which you
> propose is that it's not obviously related to CREATE ROLE. The intent
> of the patch as implemented is to allow control over only the implicit
> GRANT that is created when a new role is created, not all grants that
> might be created by or to a particular user. Setting defaults for all
> grants doesn't seem like a particularly good idea to me, but it's
> definitely a different idea than what the patch proposes to do.

Before I proposed that I was confused for a moment about this, too - but 
it turns out to be wrong. ALTER DEFAULT PRIVILEGES in general works as:

When object A is created, issue a GRANT ON A automatically.

In my proposal, the "object" is not the GRANT of that role. It's the 
role itself. So the default privileges express what should happen when 
the role is created. The default privileges would NOT affect a regular 
GRANT role TO role_spec command. They only run that command when a role 
is created.

> I did spend some time thinking about trying to tie this to the
> CREATEROLE syntax itself. For example, instead of CREATE ROLE alice
> CREATEROLE INHERITCREATEDROLES SETCREATEDROLES you could write CREATE
> ROLE alice CREATEROLE WITH (INHERIT TRUE, SET TRUE) or something like
> this. That would avoid introducing new, lengthy keywords that are just
> concatenations of other English words, a kind of syntax that doesn't
> look particularly nice to me and probably is less friendly to
> non-English speakers as well. I didn't do it that way because the
> parser support would be more complex, but I could. CREATEROLE would
> have to become a keyword again, but that's not a catastrophe.

I agree, this would not have been any better.

> Another idea would be to break the CREATEROLE stuff off from CREATE
> ROLE entirely and put it all into GRANT. You could imagine syntax like
> GRANT CREATEROLE (or CREATE ROLE?) TO role_specification WITH (INHERIT
> TRUE/FALSE, SET TRUE/FALSE). There are a few potential issues with
> this direction. One, if we did this, then CREATEROLE probably ought to
> become inheritable, because that's the way grants work in general, and
> this likely shouldn't be an exception, but this would be a behavior
> change. However, if it is the consensus that such a behavior change
> would be an improvement, that might be OK. Two, I wonder what we'd do
> about the GRANTED BY role_specification clause. We could leave it out,
> but that would be asymmetric with other GRANT commands. We could also
> support it and record that information and make this work more like
> other cases, including, I suppose, the possibility of dependent
> grants. We'd have to think about what that means exactly. If you
> revoke CREATEROLE from someone who has granted CREATEROLE to others, I
> suppose that's a clear dependent grant and needs to be recursively
> revoked. But what about the implicit grants that were created because
> the person had CREATEROLE? Are those also dependent grants? And what
> about the roles themselves? Should revoking CREATEROLE drop the roles
> that the user in question created? That gets complicated, because
> those roles might own objects. That's scary, because you might not
> expect revoking a role permission to result in tables getting dropped.
> It's also problematic, because those tables might be in some other
> database where they are inaccessible to the current session. All in
> all I'm inclined to think that recursing to the roles themselves is a
> bad plan, but it's debatable.

I'm not sure how that relates to the role attributes vs. default 
privileges discussion. Those seem to be orthogonal to the question of 
how to treat the CREATEROLE privilege itself. Right now, it's a role 
attribute. I proposed "database roles" and making CREATEROLE a privilege 
on the database level. David Johnston proposed to use a pg_createrole 
built-in role instead. Your proposal here is to invent a CREATEROLE 
privilege that can be granted, which is very similar to what I wrote 
above about "GRANT CREATEROLE ON CLUSTER". Side note: Without the ON 
CLUSTER, there'd be no target object in your GRANT statement and as such 
CREATEROLE should be treated as a role name - so I'm not sure your
proposal actually works. In any case: All those proposals change the 
semantics of how this whole CREATEROLE "privilege" works in terms of 
inheritance etc. However, those proposals all don't really change the 
way you'll want to treat the ADMIN option on the role, I think, and can 
all be made to create that implicit GRANT WITH ADMIN, when you create 
the role. And once you do that, the question of how that GRANT looks by 
default comes up - so in all those scenarios, we could talk about role 
attributes vs. default privileges. Or we could just decide not to, 
because is it really that hard to just issue a GRANT statement 
immediately after CREATE ROLE, when you want to have SET or INHERIT 
options on that role?

The answer to that question was "yes it is too hard" a while back and as 
such DEFAULT PRIVILEGES were introduced.

Best,

Wolfgang

On Mon, Nov 28, 2022 at 1:56 PM <walther@technowledgy.de> wrote:
> And now this reason is gone - there is no reason NOT to implement it as
> DEFAULT PRIVILEGES.

I think there is, and it's this, which you wrote further down:

> In my proposal, the "object" is not the GRANT of that role. It's the
> role itself. So the default privileges express what should happen when
> the role is created. The default privileges would NOT affect a regular
> GRANT role TO role_spec command. They only run that command when a role
> is created.

I agree that this is what you are proposing, but it is not what your
proposed syntax says. Your proposed syntax simply says ALTER DEFAULT
PRIVILEGES .. GRANT. Users who read that are going to think it
controls the default behavior for all grants, because that's what the
syntax says. If the proposed syntax mentioned CREATE ROLE someplace,
maybe that would have some potential. A proposal to make a command
that controls CREATE ROLE and only CREATE ROLE and mentions neither
CREATE nor ROLE anywhere in the syntax is never going to be
acceptable.

> With how you implemented it right now, is it possible to do the following?
>
> CREATE ROLE alice;
> REVOKE ADMIN OPTION FOR alice FROM CURRENT_USER;
>
> If the answer is yes, then there is no reason to allow a user to set a
> shortcut for SET and INHERIT, but not for ADMIN.
>
> If the answer is no, then you could just not allow specifying the ADMIN
> option in the ALTER DEFAULT PRIVILEGES statement and always force it to
> be TRUE.

It's no. Well, OK, you can do it, but it doesn't revoke anything,
because you can only revoke your own grant, not the bootstrap
superuser's grant.

> attributes vs. default privileges. Or we could just decide not to,
> because is it really that hard to just issue a GRANT statement
> immediately after CREATE ROLE, when you want to have SET or INHERIT
> options on that role?

It's not difficult in the sense that climbing Mount Everest is
difficult, but it makes the user experience as a CREATEROLE
non-superuser quite noticeably different from being a superuser.
Having a way to paper over such differences is, in my opinion, an
important usability feature.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

David G. Johnston:
> A quick tally of the thread so far:
> 
> No Defaults needed: David J., Mark?, Tom?
> Defaults needed - attached to role directly: Robert
> Defaults needed - defined within Default Privileges: Walther?

s/Walther/Wolfgang

> The capability itself seems orthogonal to the rest of the patch to track 
> these details better.  I think we can "Fix CREATEROLE" without any 
> feature regarding optional default behaviors and would suggest this 
> patch be so limited and that another thread be started for discussion of 
> (assuming a default specifying mechanism is wanted overall) how it 
> should look.  Let's not let a usability debate distract us from fixing a 
> real problem.

+1

I didn't argue for whether defaults are needed in this case or not. I 
just said that ADP is better for defaults than role attributes are. Or 
the other way around: I think role attributes are not a good way to 
express those.

Personally, I'm in the No Defaults needed camp, too.

Best,

Wolfgang

Robert Haas:
>> In my proposal, the "object" is not the GRANT of that role. It's the
>> role itself. So the default privileges express what should happen when
>> the role is created. The default privileges would NOT affect a regular
>> GRANT role TO role_spec command. They only run that command when a role
>> is created.
> 
> I agree that this is what you are proposing, but it is not what your
> proposed syntax says. Your proposed syntax simply says ALTER DEFAULT
> PRIVILEGES .. GRANT. Users who read that are going to think it
> controls the default behavior for all grants, because that's what the
> syntax says. If the proposed syntax mentioned CREATE ROLE someplace,
> maybe that would have some potential. A proposal to make a command
> that controls CREATE ROLE and only CREATE ROLE and mentions neither
> CREATE nor ROLE anywhere in the syntax is never going to be
> acceptable.

Yes, I agree - the abbreviated GRANT syntax is confusing/misleading in 
that case. Consistent with the other syntaxes, but easily confused 
nonetheless.

> It's no. Well, OK, you can do it, but it doesn't revoke anything,
> because you can only revoke your own grant, not the bootstrap
> superuser's grant.

Ah, I see. I didn't get that difference regarding the bootstrap 
superuser, so far.

So in that sense, the ADP GRANT would be an additional GRANT issued by 
the user that created the role in addition to the bootstrap superuser's 
grant. You can't revoke the bootstrap superuser's grant - but you can't 
modify it either. And there is no need to add SET or INHERIT to the 
boostrap superuser's grant, because you can grant the role yourself 
again, with those options.

I think it would be very strange to have a default for that bootstrap 
superuser's grant. Or rather: A different default than the minimum 
required - and that's just ADMIN, not SET, not INHERIT. When you have 
the minimum, you can always choose to grant SET and INHERIT later on 
yourself - and revoke it, too! But when the SET and INHERIT are on the 
boostrap superuser's grant - then there is no way for you to revoke SET 
or INHERIT on that grant anymore later.

Why should the superuser, who gave you CREATEROLE, insist on you having 
SET or INHERIT forever and disallow to revoke it from yourself?

Best,

Wolfgang

> On Nov 28, 2022, at 11:34 AM, David G. Johnston <david.g.johnston@gmail.com> wrote:
>
> No Defaults needed: David J., Mark?, Tom?

As Robert has the patch organized, I think defaults are needed, but I see that as a strike against the patch.

> Defaults needed - attached to role directly: Robert
> Defaults needed - defined within Default Privileges: Walther?
> The capability itself seems orthogonal to the rest of the patch to track these details better.  I think we can "Fix
CREATEROLE"without any feature regarding optional default behaviors and would suggest this patch be so limited and that
anotherthread be started for discussion of (assuming a default specifying mechanism is wanted overall) how it should
look. Let's not let a usability debate distract us from fixing a real problem. 

In Robert's initial email, he wrote, "It seems to me that the root of any fix in this area must be to change the rule
thatCREATEROLE can administer any role whatsoever." 

The obvious way to fix that is to revoke that rule and instead automatically grant ADMIN OPTION to a creator over any
rolethey create.  That's problematic, though, because as things stand, ADMIN OPTION is granted to somebody by granting
themmembership in the administered role WITH ADMIN OPTION, so membership in the role and administration of the role are
conflated.

Robert's patch tries to deal with the (possibly unwanted) role membership by setting up defaults to mitigate the
effects,but that is more confusing to me than just de-conflating role membership from role administration, and giving
rolecreators administration over roles they create, without in so doing giving them role membership.  I don't recall
enoughdetails about how hard it is to de-conflate role membership from role administration, and maybe that's a
non-starterfor reasons I don't recall at the moment.  I expect Robert has already contemplated that idea and instead
proposedthis patch for good reasons.  Robert? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Mark Dilger:
> Robert's patch tries to deal with the (possibly unwanted) role membership by setting up defaults to mitigate the
effects,but that is more confusing to me than just de-conflating role membership from role administration, and giving
rolecreators administration over roles they create, without in so doing giving them role membership.  I don't recall
enoughdetails about how hard it is to de-conflate role membership from role administration, and maybe that's a
non-starterfor reasons I don't recall at the moment.
 

Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That 
should allow role administration, without actually granting membership 
in that role, yet, right?

Best,

Wolfgang

On Mon, Nov 28, 2022 at 3:02 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Robert's patch tries to deal with the (possibly unwanted) role membership by setting up defaults to mitigate the
effects,but that is more confusing to me than just de-conflating role membership from role administration, and giving
rolecreators administration over roles they create, without in so doing giving them role membership.  I don't recall
enoughdetails about how hard it is to de-conflate role membership from role administration, and maybe that's a
non-starterfor reasons I don't recall at the moment.  I expect Robert has already contemplated that idea and instead
proposedthis patch for good reasons.  Robert? 

"De-conflating role membership from role administration" isn't really
a specific proposal that someone can go out and implement. You have to
make some decision about *how* you are going to separate those
concepts. And that's what I did: I made INHERIT and SET into
grant-level options. That allows you to give someone access to the
privileges of a role without the ability to administer it (at least
one of INHERIT and SET true, and ADMIN false) or the ability to
administer a role without having any direct access to its privileges
(INHERIT FALSE, SET FALSE, ADMIN TRUE). I don't see that we can, or
need to, separate things any more than that.

You can argue that a grant with INHERIT FALSE, SET FALSE, ADMIN TRUE
still grants membership, and I think formally that's true, but I also
think it's just picking something to bicker about. The need isn't to
separate membership per se from administration. It's to separate
privilege inheritance and the ability to SET ROLE from role
administration. And I've done that.

I strongly disagree with the idea that the ability for users to
control defaults here isn't needed. You can set a default tablespace
for your database, and a default tablespace for your session, and a
default tablespace for new partitions of an existing partition table.
You can set default privileges for every type of object you can
create, and a default search path to find objects in the database. You
can set defaults for all of your connection parameters to the database
using environment variables, and the default data directory for
commands that need one. You can set defaults for all of your psql
settings in ~/.psqlrc. You can set defaults for the character sets,
locales and collations of new databases.  You can set the default
version of an extension in the control file, so that the user doesn't
have to specify a version. And so on and so on. There's absolutely
scads of things for which it is useful to be able to set defaults and
for which we give people the ability to set defaults, and I don't
think anyone is making a real argument for why that isn't also true
here. The argument that has been made is essentially that you could
get by without it, but that's true of *every* default. Yet we keep
adding the ability to set defaults for new things, and to set the
defaults for existing things in new ways, and there's a very good
reason for that: it's extremely convenient. And that's true here, too.

--
Robert Haas
EDB: http://www.enterprisedb.com

> On Nov 28, 2022, at 12:08 PM, walther@technowledgy.de wrote:
>
> Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That should allow role administration, without
actuallygranting membership in that role, yet, right? 

Can you clarify what you mean here?  Are you inventing a new syntax?

+GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE;
+ERROR:  syntax error at or near "SET"
+LINE 1: GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE...


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Nov 28, 2022, at 12:33 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>> Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That should allow role administration, without
actuallygranting membership in that role, yet, right? 
>
> Can you clarify what you mean here?  Are you inventing a new syntax?

Nevermind.  After reading Robert's email, it's clear enough what you mean here.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 28, 2022 at 4:19 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> That's fine, but are you saying this patch is incapable (or simply undesirable) of having the parts about handling
defaultsseparated out from the parts that define how the system works with a given set of permissions; and the one
implementationdetail of having the bootstrap superuser automatically grant admin to any roles a createuser role
creates?If you and others feel strongly about defaults I'm sure that the suggested other thread focused on that will
getattention and be committed in a timely manner.  But the system will work, and not be broken, if that got stalled,
andit could be added in later. 

The topics are so closely intertwined that I don't believe that trying
to have separate discussions will be useful or productive. There's no
hope of anybody understanding 0004 or having an educated opinion about
it without first understanding the earlier patches, and there's no
requirement that someone has to review 0004, or like it, just because
they review or like 0001-0003.

But so far nobody has actually reviewed anything, and all that's
happened is people have complained about 0004 for reasons which in my
opinion are pretty nebulous and largely ignore the factors that caused
it to exist in the first place. We had about 400 emails during the
last release cycle arguing about a whole bunch of topics related to
user management, and it became absolutely crystal clear in that
discussion that Stephen Frost and David Steele wanted to have roles
that could create other roles but not immediately be able to access
their privileges. Mark and I, on the other hand, wanted to have roles
that could create other roles WITH immediate access to their
privileges. That argument was probably the main thing that derailed
that entire patch set, which represented months of work by Mark. Now,
I have come up with a competing patch set that for the price of 100
lines of code and a couple of slightly ugly option names can do either
thing. So Stephen and David and any like-minded users can have what
they want, and Mark and I and any like-minded users can have what we
want. And the result is that I've got like five people, some of whom
particulated in those discussions, showing up to say "hey, we don't
need the ability to set defaults." Well, if that's the case, then why
did we have hundreds and hundreds of emails within the last 12 months
arguing about which way it should work?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Nov 28, 2022 at 4:55 PM Robert Haas <robertmhaas@gmail.com> wrote:
> But so far nobody has actually reviewed anything, ...

Actually this isn't true. Mark did review. Thanks, Mark.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Nov 28, 2022 at 6:32 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Desirable follow-on patches include:
>
> 1) Automatically install an additional membership grant, with the CREATEROLE user as the grantor, specifying INHERIT
ORSET as TRUE (I personally favor attaching these to ALTER ROLE, modifiable only by oneself)
 

Hmm, that's an interesting alternative to what I actually implemented.
Some people might like it better, because it puts the behavior fully
under the control of the CREATEROLE user, which a number of you seem
to favor.

I suppose if we did it that way, it could even be a GUC, like
create_role_automatic_grant_options.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Mark Dilger:
>> Isn't this just GRANT .. WITH SET FALSE, INHERIT FALSE, ADMIN TRUE? That should allow role administration, without
actuallygranting membership in that role, yet, right?
 
> 
> Can you clarify what you mean here?  Are you inventing a new syntax?
> 
> +GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE;
> +ERROR:  syntax error at or near "SET"
> +LINE 1: GRANT bob TO alice WITH SET FALSE, INHERIT FALSE, ADMIN TRUE...

This is valid syntax on latest master.

Best,

Wolfgang

Robert Haas:
> And the result is that I've got like five people, some of whom
> particulated in those discussions, showing up to say "hey, we don't
> need the ability to set defaults." Well, if that's the case, then why
> did we have hundreds and hundreds of emails within the last 12 months
> arguing about which way it should work?

For me: "Needed" as in "required". I don't think we *require* defaults 
to make this useful, just as David said as well. Personally, I don't 
need defaults either, at least I didn't have a use-case for it, yet. I'm 
not objecting to introduce defaults, but I do object to *how* they were 
introduced in your patch set, so far. It just wasn't consistent with the 
other stuff that already exists.

Best,

Wolfgang

Robert Haas:
>> 1) Automatically install an additional membership grant, with the CREATEROLE user as the grantor, specifying INHERIT
ORSET as TRUE (I personally favor attaching these to ALTER ROLE, modifiable only by oneself)
 
> 
> Hmm, that's an interesting alternative to what I actually implemented.
> Some people might like it better, because it puts the behavior fully
> under the control of the CREATEROLE user, which a number of you seem
> to favor.

+1

> I suppose if we did it that way, it could even be a GUC, like
> create_role_automatic_grant_options.

I don't think using GUCs for that is any better. ALTER DEFAULT 
PRIVILEGES is the correct way to do it. The only argument against it 
was, so far, that it's easy to confuse with default options for newly 
created role grants, due to the abbreviated grant syntax.

I propose a slightly different syntax instead:

ALTER DEFAULT PRIVILEGES GRANT CREATED ROLE TO role_specification WITH ...;

This, together with the proposal above regarding the grantor, should be 
consistent.

Is there any other argument to be made against ADP?

Note, that ADP allows much more than just creating a grant for the 
CREATEROLE user, which would be the case if the default GRANT was made 
TO the_create_role_user. But it could be made towards *other* users as 
well, so you could do something like this:

CREATE ROLE alice CREATEROLE;
CREATE ROLE bob;

ALTER DEFAULT PRIVILEGES FOR alice GRANT CREATED ROLE TO bob WITH SET 
TRUE, INHERIT FALSE;

This is much more flexible than role attributes or GUCs.

Best,

Wolfgang

On Tue, Nov 29, 2022 at 2:32 AM <walther@technowledgy.de> wrote:
> I propose a slightly different syntax instead:
>
> ALTER DEFAULT PRIVILEGES GRANT CREATED ROLE TO role_specification WITH ...;
>
> This, together with the proposal above regarding the grantor, should be
> consistent.

I think that is more powerful than what I proposed but less fit for
purpose. If alice is a CREATEROLE user and issues CREATE ROLE bob, my
proposal allows alice to automatically obtain access to bob's
privileges. Your proposal would allow that, but it would also allow
alice to automatically confer bob's privileges on some third user, say
charlie. Maybe that's useful to somebody, I don't know.

But one significant disadvantage of this is that every CREATEROLE user
must have their own configuration. If we have CREATE ROLE users alice,
dave, and ellen, then allice needs to execute ALTER DEFAULT PRIVILEGES
GRANT CREATED ROLE TO alice WITH ...; dave needs to do the same thing
with dave instead of alice; and ellen needs to do the same thing with
ellen instead of alice. There's no way to apply a system-wide
configuration that applies nicely to all CREATEROLE users.

A GUC would of course allow that, because it could be set in
postgresql.conf and then overridden for particular databases, users,
or sessions.

David claims that "these aren't privileges, they are memberships." I
don't entirely agree with that, because I think that we're basically
using memberships as a pseudonym for privileges where roles are
concerned. However, it is true that there's no precedent for referring
to role grants using the keyword PRIVILEGES at the SQL level, and the
fact that the underlying works in somewhat similar ways doesn't
necessarily mean that it's OK to conflate the two concepts at the SQL
level.

So I'm still not very sold on this idea.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Nov 28, 2022 at 8:33 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Hmm, that's an interesting alternative to what I actually implemented.
> Some people might like it better, because it puts the behavior fully
> under the control of the CREATEROLE user, which a number of you seem
> to favor.

Here's an updated patch set.

0001 adds more precise and extensive documentation for the current
(broken) state of affairs. I propose to back-patch this to all
supported branches. It also removes a <tip> suggesting that you should
use a CREATEDB & CREATEROLE role instead of a superuser, because that
is pretty pointless as things stand, and is too simplistic for the new
system that I'm proposing to put in place, too.

0002 and 0003 are refactoring, unchanged from v1.

0004 is the core fix to CREATEROLE. It has been updated from the
previous version with documentation and some bug fixes.

0005 adopts David's suggestion: instead of giving the superuser a way
to control the options on the implicit grant, give CREATEROLE users a
way to grant newly-created roles to themselves automatically. I made
this a GUC, which means that the person setting up the system could
configure a default in postgresql.conf, but a user who doesn't prefer
that default can also override it using ALTER ROLE .. SET or ~/.psqlrc
or whatever. This is simpler than what I had before, doesn't involve a
catalog change, makes it clear that the behavior is not
security-critical, and puts the decision fully in the hands of the
CREATEROLE user rather than being partly controlled by that user and
partly by the superuser. Hopefully that's an improvement.

Comments?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Reading 0001:

+        However, <literal>CREATEROLE</literal> does not convey the ability to
+        create <literal>SUPERUSER</literal> roles, nor does it convey any
+        power over <literal>SUPERUSER</literal> roles that already exist.
+        Furthermore, <literal>CREATEROLE</literal> does not convey the power
+        to create <literal>REPLICATION</literal> users, nor the ability to
+        grant or revoke the <literal>REPLICATION</literal> privilege, nor the
+        ability to the role properties of such users.

"... nor the ability to the role properties ..."
I think a verb is missing here.

The contents looks good to me other than that problem, and I agree to
backpatch it.


Why did you choose to use two dots for ellipses in some command
<literal>s rather than three?  I know I've made that choice too on
occassion, but there aren't many such cases and maybe we should put a
stop to it (or a period) before it spreads too much.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Thu, Dec 22, 2022 at 9:14 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> The contents looks good to me other than that problem, and I agree to
> backpatch it.

Cool. Thanks for the review.

> Why did you choose to use two dots for ellipses in some command
> <literal>s rather than three?  I know I've made that choice too on
> occassion, but there aren't many such cases and maybe we should put a
> stop to it (or a period) before it spreads too much.

Honestly, I wasn't aware that we had some other convention for it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Dec 23, 2022 at 4:55 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Dec 22, 2022 at 9:14 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > The contents looks good to me other than that problem, and I agree to
> > backpatch it.
>
> Cool. Thanks for the review.
>
> > Why did you choose to use two dots for ellipses in some command
> > <literal>s rather than three?  I know I've made that choice too on
> > occassion, but there aren't many such cases and maybe we should put a
> > stop to it (or a period) before it spreads too much.
>
> Honestly, I wasn't aware that we had some other convention for it.

Committed and back-patched 0001 with fixes for the issues that you pointed out.

Here's a trivial rebase of the rest of the patch set.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jan 3, 2023 at 3:11 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Committed and back-patched 0001 with fixes for the issues that you pointed out.
>
> Here's a trivial rebase of the rest of the patch set.

I committed 0001 and 0002 after improving the commit messages a bit.
Here's the remaining two patches back. I've done a bit more polishing
of these as well, specifically in terms of fleshing out the regression
tests. I'd like to move forward with these soon, if nobody's too
vehemently opposed to that.

Previous feedback, especially from Tom but also others, was that the
role-level properties the final patch was creating were not good. Now
it doesn't create any new role-level properties, and in fact it has
nothing to say about role-level properties in any way. That might not
be the right thing. Right now, if you have CREATEROLE, you can create
new roles with any combination of attributes you like, except that you
cannot set the SUPERUSER, REPLICATION, or BYPASSRLS properties. While
I think it makes sense that a CREATEROLE user can't hand out SUPERUSER
or REPLICATION privileges, it is really not obvious to me why a
CREATEROLE user shouldn't be permitted to hand out BYPASSRLS, at least
if they have it themselves, and right now there's no way to allow
that. On the other hand, I think that some superusers might want to
restrict a CREATEROLE user's ability to hand out CREATEROLE or
CREATEDB to the users they create, and right now there's no way to
prohibit that.

I don't have a great idea about what a system for handling this
problem ought to look like. In a vacuum, I think it would be
reasonable to change CREATEROLE to only allow CREATEDB, BYPASSRLS, and
similar to be given to new users if the creating user possesses them,
but that approach does not work for CREATEROLE, because if you didn't
have that, you couldn't create any new users at all. It's also pretty
weird for, say, CONNECTION LIMIT. I doubt that there's any connection
between the CONNECTION LIMIT of the CREATEROLE user and the values
that they ought to be able to set for users that they create. Probably
you just want to allow setting CONNECTION LIMIT for downstream users,
or not. Or maybe it's not even worth worrying about -- I think there
might be a decent argument that limiting the ability to set CONNECTION
LIMIT just isn't interesting.

If someone else has a good idea what we ought to do about this part of
the problem, I'd be interested to hear it. Absent such a good idea --
or if that good idea is more work to implement that can be done in the
near term -- I think it would be OK to ship as much as I've done here
and revisit the topic at some later point when we've had a chance to
absorb user feedback.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Jan 5, 2023 at 2:53 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jan 3, 2023 at 3:11 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > Committed and back-patched 0001 with fixes for the issues that you pointed out.
> >
> > Here's a trivial rebase of the rest of the patch set.
>
> I committed 0001 and 0002 after improving the commit messages a bit.
> Here's the remaining two patches back. I've done a bit more polishing
> of these as well, specifically in terms of fleshing out the regression
> tests. I'd like to move forward with these soon, if nobody's too
> vehemently opposed to that.

Done now.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 10 Jan 2023 at 23:16, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 5, 2023 at 2:53 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Jan 3, 2023 at 3:11 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > Committed and back-patched 0001 with fixes for the issues that you pointed out.
> > >
> > > Here's a trivial rebase of the rest of the patch set.
> >
> > I committed 0001 and 0002 after improving the commit messages a bit.
> > Here's the remaining two patches back. I've done a bit more polishing
> > of these as well, specifically in terms of fleshing out the regression
> > tests. I'd like to move forward with these soon, if nobody's too
> > vehemently opposed to that.
>
> Done now.

I'm not sure if any work is left here, if there is nothing more to do,
can we close this?

Regards,
Vignesh

On Sat, Jan 14, 2023 at 2:26 AM vignesh C <vignesh21@gmail.com> wrote:
> I'm not sure if any work is left here, if there is nothing more to do,
> can we close this?

There's a discussion on another thread about some follow-up
documentation adjustments, but feel free to close the CF entry for
this patch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Sun, 15 Jan 2023 at 06:02, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Jan 14, 2023 at 2:26 AM vignesh C <vignesh21@gmail.com> wrote:
> > I'm not sure if any work is left here, if there is nothing more to do,
> > can we close this?
>
> There's a discussion on another thread about some follow-up
> documentation adjustments, but feel free to close the CF entry for
> this patch.

Thanks, I have marked the CF entry as committed.

Regards,
Vignesh