Обсуждение: [PATCH v2] use has_privs_for_role for predefined roles

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
> Attached is an updated version of the patch to replace
> is_member_of_role with has_privs_for_role for predefined roles. It
> does not remove is_member_of_role from acl.h but it does add a warning
> not to use that function for privilege checking.
>
> Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

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

On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> <joshua.brindle@crunchydata.com> wrote:
> > Attached is an updated version of the patch to replace
> > is_member_of_role with has_privs_for_role for predefined roles. It
> > does not remove is_member_of_role from acl.h but it does add a warning
> > not to use that function for privilege checking.
> >
> > Please consider this for the upcoming commitfest.
>
> I am not sure I understand what the advantage of this is supposed to be.

Pre-defined roles currently do not operate the same way other roles do
with respect to inheritance. The patchfile has an explanation and
examples, I wasn't sure if that needed to be repeated in the email or
not.

On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
>
> On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> > <joshua.brindle@crunchydata.com> wrote:
> > > Attached is an updated version of the patch to replace
> > > is_member_of_role with has_privs_for_role for predefined roles. It
> > > does not remove is_member_of_role from acl.h but it does add a warning
> > > not to use that function for privilege checking.
> > >
> > > Please consider this for the upcoming commitfest.
> >
> > I am not sure I understand what the advantage of this is supposed to be.
>
> Pre-defined roles currently do not operate the same way other roles do
> with respect to inheritance. The patchfile has an explanation and
> examples, I wasn't sure if that needed to be repeated in the email or
> not.

And FWIW several predefined role patches on the list currently
correctly use has_privs_for_role rather than is_memver_of_role so this
brings the older roles to be consistent with the newer ones being
proposed.

On 10/27/21, 2:19 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:
> I am not sure I understand what the advantage of this is supposed to be.

There are a couple of other related threads with some additional
context:

        https://www.postgresql.org/message-id/flat/20211026224731.115337-1-joshua.brindle%40crunchydata.com

https://www.postgresql.org/message-id/flat/CAGB%2BVh4enxvLBM_BJweWEO12Q0ySLMBWK9iOLaM7e%3DV1Y0YadA%40mail.gmail.com

Nathan

Greetings,

* Joshua Brindle (joshua.brindle@crunchydata.com) wrote:
> On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> > > <joshua.brindle@crunchydata.com> wrote:
> > > > Attached is an updated version of the patch to replace
> > > > is_member_of_role with has_privs_for_role for predefined roles. It
> > > > does not remove is_member_of_role from acl.h but it does add a warning
> > > > not to use that function for privilege checking.
> > > >
> > > > Please consider this for the upcoming commitfest.
> > >
> > > I am not sure I understand what the advantage of this is supposed to be.
> >
> > Pre-defined roles currently do not operate the same way other roles do
> > with respect to inheritance. The patchfile has an explanation and
> > examples, I wasn't sure if that needed to be repeated in the email or
> > not.
>
> And FWIW several predefined role patches on the list currently
> correctly use has_privs_for_role rather than is_memver_of_role so this
> brings the older roles to be consistent with the newer ones being
> proposed.

Right, we really should be consistent here and we're not and that's not
a good thing.  Further, if you're logged in as an unprivileged role and
expect to not see things that you shouldn't, then it's not good for that
to end up happening because you've been GRANT'd a more privileged, but
noinherit, role that was given some predefined roles.  This doesn't end
up being an actual security issue as you could just SET ROLE to that
more privileged role, so it's not that you couldn't see that data, just
that you should have had to SET ROLE to do so.

Reviewing the actual patch, it generally looks good to me except that
you missed updating this comment:

Superusers or members of pg_read_all_stats members are allowed

Thanks,

Stephen

On Mon, Nov 8, 2021 at 3:44 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Joshua Brindle (joshua.brindle@crunchydata.com) wrote:
> > On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> > > > <joshua.brindle@crunchydata.com> wrote:
> > > > > Attached is an updated version of the patch to replace
> > > > > is_member_of_role with has_privs_for_role for predefined roles. It
> > > > > does not remove is_member_of_role from acl.h but it does add a warning
> > > > > not to use that function for privilege checking.
> > > > >
> > > > > Please consider this for the upcoming commitfest.
> > > >
> > > > I am not sure I understand what the advantage of this is supposed to be.
> > >
> > > Pre-defined roles currently do not operate the same way other roles do
> > > with respect to inheritance. The patchfile has an explanation and
> > > examples, I wasn't sure if that needed to be repeated in the email or
> > > not.
> >
> > And FWIW several predefined role patches on the list currently
> > correctly use has_privs_for_role rather than is_memver_of_role so this
> > brings the older roles to be consistent with the newer ones being
> > proposed.
>
> Right, we really should be consistent here and we're not and that's not
> a good thing.  Further, if you're logged in as an unprivileged role and
> expect to not see things that you shouldn't, then it's not good for that
> to end up happening because you've been GRANT'd a more privileged, but
> noinherit, role that was given some predefined roles.  This doesn't end
> up being an actual security issue as you could just SET ROLE to that
> more privileged role, so it's not that you couldn't see that data, just
> that you should have had to SET ROLE to do so.
>
> Reviewing the actual patch, it generally looks good to me except that
> you missed updating this comment:
>
> Superusers or members of pg_read_all_stats members are allowed

Thanks for the review, attached is an update with that comment fixed
and also sgml documentation changes that I missed earlier.

On 11/8/21, 2:19 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> Thanks for the review, attached is an update with that comment fixed
> and also sgml documentation changes that I missed earlier.

I think there are a number of documentation changes that are still
missing.  I did a quick scan and saw the "is member of" language in
func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml,
pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml.

<para>
 By default, the <structname>pg_shmem_allocations</structname> view can be
-   read only by superusers or members of the <literal>pg_read_all_stats</literal>
-   role.
+   read only by superusers or roles with privilges of the
+   <literal>pg_read_all_stats</literal> role.
</para>
</sect1>

nitpick: "privileges" is misspelled.

Nathan

On Wed, Nov 10, 2021 at 12:45 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 11/8/21, 2:19 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> > Thanks for the review, attached is an update with that comment fixed
> > and also sgml documentation changes that I missed earlier.
>
> I think there are a number of documentation changes that are still
> missing.  I did a quick scan and saw the "is member of" language in
> func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml,
> pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml.

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

> <para>
>  By default, the <structname>pg_shmem_allocations</structname> view can be
> -   read only by superusers or members of the <literal>pg_read_all_stats</literal>
> -   role.
> +   read only by superusers or roles with privilges of the
> +   <literal>pg_read_all_stats</literal> role.
> </para>
> </sect1>
>
> nitpick: "privileges" is misspelled.

Fixed, thanks for reviewing.

On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> All of these and also adminpack.sgml updated. I think that is all of
> them but docs broken across lines and irregular wording makes it
> difficult.

LGTM.  I've marked this as ready-for-committer.

Nathan

"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
>> All of these and also adminpack.sgml updated. I think that is all of
>> them but docs broken across lines and irregular wording makes it
>> difficult.

> LGTM.  I've marked this as ready-for-committer.

This needs another rebase --- it's trying to adjust
file_fdw/output/file_fdw.source, which no longer exists
(fix the regular file_fdw.out, instead).

            regards, tom lane

On Tue, Jan 4, 2022 at 3:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "Bossart, Nathan" <bossartn@amazon.com> writes:
> > On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
> >> All of these and also adminpack.sgml updated. I think that is all of
> >> them but docs broken across lines and irregular wording makes it
> >> difficult.
>
> > LGTM.  I've marked this as ready-for-committer.
>
> This needs another rebase --- it's trying to adjust
> file_fdw/output/file_fdw.source, which no longer exists
> (fix the regular file_fdw.out, instead).
>
>                         regards, tom lane

Attached, thanks

On 1/4/22 16:51, Joshua Brindle wrote:
> On Tue, Jan 4, 2022 at 3:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> > On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
>> >> All of these and also adminpack.sgml updated. I think that is all of
>> >> them but docs broken across lines and irregular wording makes it
>> >> difficult.
>>
>> > LGTM.  I've marked this as ready-for-committer.
>>
>> This needs another rebase --- it's trying to adjust
>> file_fdw/output/file_fdw.source, which no longer exists
>> (fix the regular file_fdw.out, instead).
>>
>>                         regards, tom lane
> 
> Attached, thanks

I'd like to pick this patch up and see it through to commit/push. 
Presumably that will include back-patching to all supported pg versions.

Before I go through the effort to back-patch, does anyone want to argue 
that this should *not* be back-patched?

Thanks,

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:
> I'd like to pick this patch up and see it through to commit/push. 
> Presumably that will include back-patching to all supported pg versions.
> Before I go through the effort to back-patch, does anyone want to argue 
> that this should *not* be back-patched?

Hm, I'm -0.5 or so.  I think changing security-related behaviors
in a stable branch is a hard sell unless you are closing a security
hole.  This is a fine improvement for HEAD but I'm inclined to
leave the back branches alone.

            regards, tom lane

On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Joe Conway <mail@joeconway.com> writes:
> > I'd like to pick this patch up and see it through to commit/push.
> > Presumably that will include back-patching to all supported pg versions.
> > Before I go through the effort to back-patch, does anyone want to argue
> > that this should *not* be back-patched?
>
> Hm, I'm -0.5 or so.  I think changing security-related behaviors
> in a stable branch is a hard sell unless you are closing a security
> hole.  This is a fine improvement for HEAD but I'm inclined to
> leave the back branches alone.

I think the threshold to back-patch a clear behavior change is pretty
high, so I do not think it should be back-patched.

I am also not convinced that a sufficient argument has been made for
changing this in master. This isn't the only thread where NOINHERIT
has come up lately, and I have to admit that I'm not a fan. Let's
suppose that I have two users, let's say sunita and sri. In the real
world, Sunita is Sri's manager and needs to be able to perform actions
as Sri when Sri is out of the office, but it isn't desirable for
Sunita to have Sri's privileges in all situations all the time. So we
mark role sunita as NOINHERIT and grant sri to sunita. Then it turns
out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her
pg_execute_server_program. Now, if she can't exercise this privilege
without setting role to the prefined role, that's bad, isn't it? I
mean, we want her to be able to copy between *her* tables and various
shell commands, not the tables owned by pg_execute_server_program, of
which there are presumably none.

It seems to me that the INHERIT role flag isn't very well-considered.
Inheritance, or the lack of it, ought to be decided separately for
each inherited role. However, that would be a major architectural
change. But in the absence of that, it seems clearly better for
predefined roles to disregard INHERIT and just always grant the rights
they are intended to give. Because if we don't do that, then we end up
with people having to SET ROLE to the predefined role and perform
actions directly as that role, which seems like it can't be what we
want. I almost feel like we ought to be looking for ways of preventing
people from doing SET ROLE to a predefined role altogether, not
encouraging them to do it.

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

On 2/7/22 10:35, Robert Haas wrote:
> On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Joe Conway <mail@joeconway.com> writes:
>> > I'd like to pick this patch up and see it through to commit/push.
>> > Presumably that will include back-patching to all supported pg versions.
>> > Before I go through the effort to back-patch, does anyone want to argue
>> > that this should *not* be back-patched?
>>
>> Hm, I'm -0.5 or so.  I think changing security-related behaviors
>> in a stable branch is a hard sell unless you are closing a security
>> hole.  This is a fine improvement for HEAD but I'm inclined to
>> leave the back branches alone.
> 
> I think the threshold to back-patch a clear behavior change is pretty
> high, so I do not think it should be back-patched.

ok

> I am also not convinced that a sufficient argument has been made for
> changing this in master. This isn't the only thread where NOINHERIT
> has come up lately, and I have to admit that I'm not a fan. Let's
> suppose that I have two users, let's say sunita and sri. In the real
> world, Sunita is Sri's manager and needs to be able to perform actions
> as Sri when Sri is out of the office, but it isn't desirable for
> Sunita to have Sri's privileges in all situations all the time. So we
> mark role sunita as NOINHERIT and grant sri to sunita. Then it turns
> out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her
> pg_execute_server_program. Now, if she can't exercise this privilege
> without setting role to the prefined role, that's bad, isn't it? I
> mean, we want her to be able to copy between *her* tables and various
> shell commands, not the tables owned by pg_execute_server_program, of
> which there are presumably none.

Easily worked around with one additional level of role:

8<---------------------------------------
nmx=# create user sunita;
CREATE ROLE
nmx=# create user sri superuser;
CREATE ROLE
nmx=# create user sri_alt noinherit;
CREATE ROLE
nmx=# grant sri to sri_alt;
GRANT ROLE
nmx=# grant sri_alt to sunita;
GRANT ROLE
nmx=# grant pg_execute_server_program to sunita;
GRANT ROLE
nmx=# set session authorization sri;
SET
nmx=# create table foo(id int);
CREATE TABLE
nmx=# insert into foo values(42);
INSERT 0 1
nmx=# set session authorization sunita;
SET
nmx=> select * from foo;
ERROR:  permission denied for table foo
nmx=> set role sri;
SET
nmx=# select * from foo;
  id
----
  42
(1 row)

nmx=# reset role;
RESET
nmx=> select current_user;
  current_user
--------------
  sunita
(1 row)

nmx=> create temp table sfoo(f1 text);
CREATE TABLE
nmx=> copy sfoo(f1) from program 'id';
COPY 1
nmx=> select f1 from sfoo;
                                            f1 

----------------------------------------------------------------------------------------
  uid=1001(postgres) gid=1001(postgres) 
groups=1001(postgres),108(ssl-cert),1002(pgconf)
(1 row)
8<---------------------------------------

> It seems to me that the INHERIT role flag isn't very well-considered.
> Inheritance, or the lack of it, ought to be decided separately for
> each inherited role. However, that would be a major architectural
> change.

Agreed -- that would be useful.

> But in the absence of that, it seems clearly better for predefined
> roles to disregard INHERIT and just always grant the rights they are
> intended to give. Because if we don't do that, then we end up with
> people having to SET ROLE to the predefined role and perform actions
> directly as that role, which seems like it can't be what we want. I
> almost feel like we ought to be looking for ways of preventing people
> from doing SET ROLE to a predefined role altogether, not encouraging
> them to do it.
I disagree with this though.

It is confusing and IMHO dangerous that the predefined roles currently 
work differently than regular roles eith respect to privilege inheritance.

In fact, I would extend that argument to the pseudo-role PUBLIC.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
> Easily worked around with one additional level of role:

Interesting.

> > But in the absence of that, it seems clearly better for predefined
> > roles to disregard INHERIT and just always grant the rights they are
> > intended to give. Because if we don't do that, then we end up with
> > people having to SET ROLE to the predefined role and perform actions
> > directly as that role, which seems like it can't be what we want. I
> > almost feel like we ought to be looking for ways of preventing people
> > from doing SET ROLE to a predefined role altogether, not encouraging
> > them to do it.
> I disagree with this though.
>
> It is confusing and IMHO dangerous that the predefined roles currently
> work differently than regular roles eith respect to privilege inheritance.

I feel like that's kind of a conclusory statement, as opposed to
making an argument. I mean that this tells me something about how you
feel, but it doesn't really help me understand why you feel that way.

I suppose one argument in favor of your position is that if it
happened to be sri who was granted a predefined role, sunita would
inherit the rest of sr's privileges only with SET ROLE, but the
predefined role either way (IIUC, which I might not). If that's so,
then I guess I see the point, but I'm still sort of inclined to think
we're just trading one set of problems in for a different set. I just
have such a hard time imaging anyone using NOINHERIT in anger and
being happy with the result....

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

On Mon, Feb 7, 2022 at 12:09 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
> > Easily worked around with one additional level of role:
>
> Interesting.
>
> > > But in the absence of that, it seems clearly better for predefined
> > > roles to disregard INHERIT and just always grant the rights they are
> > > intended to give. Because if we don't do that, then we end up with
> > > people having to SET ROLE to the predefined role and perform actions
> > > directly as that role, which seems like it can't be what we want. I
> > > almost feel like we ought to be looking for ways of preventing people
> > > from doing SET ROLE to a predefined role altogether, not encouraging
> > > them to do it.
> > I disagree with this though.
> >
> > It is confusing and IMHO dangerous that the predefined roles currently
> > work differently than regular roles eith respect to privilege inheritance.
>
> I feel like that's kind of a conclusory statement, as opposed to
> making an argument. I mean that this tells me something about how you
> feel, but it doesn't really help me understand why you feel that way.
>
> I suppose one argument in favor of your position is that if it
> happened to be sri who was granted a predefined role, sunita would
> inherit the rest of sr's privileges only with SET ROLE, but the
> predefined role either way (IIUC, which I might not). If that's so,
> then I guess I see the point, but I'm still sort of inclined to think
> we're just trading one set of problems in for a different set. I just
> have such a hard time imaging anyone using NOINHERIT in anger and
> being happy with the result....
>

IMO this is inarguably a plain bug. The inheritance system works one
way for pre-defined roles and another way for other roles - and the
difference isn't even documented.

The question is whether there is a security issue warranting back
patching, which is a bit of a tossup I think. According to git history
it's always worked this way, and the possible breakage of pre-existing
clusters seems maybe not worth it.

On 2/7/22 12:09, Robert Haas wrote:
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
>> It is confusing and IMHO dangerous that the predefined roles currently
>> work differently than regular roles eith respect to privilege inheritance.
> 
> I feel like that's kind of a conclusory statement, as opposed to
> making an argument. I mean that this tells me something about how you
> feel, but it doesn't really help me understand why you feel that way.


The argument is that we call these things "predefined roles", but they 
do not behave the same way normal "roles" behave.

Someone not intimately familiar with that fact could easily make bad 
assumptions, and therefore wind up with misconfigured security settings. 
In other words, it violates the principle of least astonishment (POLA).

As Joshua said nearby, it simply jumps out at me as a bug.

-------
After more thought, perhaps the real problem is that these things should 
not have been called "predefined roles" at all. I know, the horse has 
already left the barn on that, but in any case...

They are (to me at least) similar in concept to Linux capabilities in 
that they allow roles other than superusers to do a certain subset of 
the things historically reserved for superusers through a special 
mechanism (hardcoded) rather than through the normal privilege system 
(GRANTS/ACLs).

As an example, the predefined role pg_read_all_settings allows a 
non-superuser to read GUC normally reserved for superuser access only.

If I create a new user "bob" with the default INHERIT attribute, and I 
grant postgres to bob, bob must SET ROLE to postgres in order to access 
the capability to read superuser settings.

This is similar to bob's access to the default superuser privilege to 
read data in someone else's table (must SET ROLE to access that capability).

But it is different from bob's access to inherited privileges which are 
GRANTed:
8<--------------------------
psql nmx
psql (15devel)
Type "help" for help.

nmx=# create user bob;
CREATE ROLE

nmx=# grant postgres to bob;
GRANT ROLE

nmx=# \q
8<--------------------------
-and-
8<--------------------------
psql -U bob nmx
psql (15devel)
Type "help" for help.

nmx=> select current_user;
  current_user
--------------
  bob
(1 row)

nmx=> show stats_temp_directory;
ERROR:  must be superuser or have privileges of pg_read_all_settings to 
examine "stats_temp_directory"
nmx=> set role postgres;
SET
nmx=# show stats_temp_directory;
  stats_temp_directory
----------------------
  pg_stat_tmp
(1 row)

nmx=# select current_user;
  current_user
--------------
  postgres
(1 row)

nmx=# select * from foo;
  id
----
  42
(1 row)

nmx=# reset role;
RESET
nmx=> select current_user;
  current_user
--------------
  bob
(1 row)

nmx=> select * from foo;
ERROR:  permission denied for table foo
nmx=> set role postgres;
SET
nmx=# grant select on table foo to postgres;
GRANT
nmx=# reset role;
RESET
nmx=> select current_user;
  current_user
--------------
  bob
(1 row)

nmx=> select * from foo;
  id
----
  42
(1 row)
8<--------------------------

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Feb 8, 2022 at 6:59 AM Joe Conway <mail@joeconway.com> wrote:
> This is similar to bob's access to the default superuser privilege to
> read data in someone else's table (must SET ROLE to access that capability).
>
> But it is different from bob's access to inherited privileges which are
> GRANTed:

Yeah. I think right here you've put your finger on what's been bugging
me about this: it's similar to one thing, and it's different from
another. To you and Joshua and Stephen, it seems 100% obvious that
these roles should work like grants of other roles. But I think of
them as capabilities derived from the superuser account, and so I'm
sort of tempted to think that they should work the way the superuser
bit does. And that's why I don't think the fact that they work the
other way is "just a bug" -- it's one of two possible ways that
someone could think that it ought to work based on how other things in
the system actually do work.

I'm not hard stuck on the idea that the current behavior is right, but
I don't think that we can really say that we've made things fully
consistent unless we make things like SUPERUSER and BYPASSRLS work the
same way that you want to make predefined roles work. And probably do
something about the INHERIT flag too because the current situation
seems like a hot mess.

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

On Tue, Feb 8, 2022 at 8:46 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Feb 8, 2022 at 6:59 AM Joe Conway <mail@joeconway.com> wrote:
> > This is similar to bob's access to the default superuser privilege to
> > read data in someone else's table (must SET ROLE to access that capability).
> >
> > But it is different from bob's access to inherited privileges which are
> > GRANTed:
>
> Yeah. I think right here you've put your finger on what's been bugging
> me about this: it's similar to one thing, and it's different from
> another. To you and Joshua and Stephen, it seems 100% obvious that
> these roles should work like grants of other roles. But I think of
> them as capabilities derived from the superuser account, and so I'm
> sort of tempted to think that they should work the way the superuser
> bit does. And that's why I don't think the fact that they work the
> other way is "just a bug" -- it's one of two possible ways that
> someone could think that it ought to work based on how other things in
> the system actually do work.
>
> I'm not hard stuck on the idea that the current behavior is right, but
> I don't think that we can really say that we've made things fully
> consistent unless we make things like SUPERUSER and BYPASSRLS work the
> same way that you want to make predefined roles work. And probably do
> something about the INHERIT flag too because the current situation
> seems like a hot mess.

I think hot mess is an apt description of the current situation, for
example consider that:

src/backend/catalog/aclchk.c
3931: has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA))
3943: has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA))
4279: (has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA) ||
4280: has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA)))

src/backend/storage/ipc/signalfuncs.c
82: if (!has_privs_of_role(GetUserId(), proc->roleId) &&
83: !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))

src/backend/storage/ipc/procarray.c
3843: if (!has_privs_of_role(GetUserId(), proc->roleId) &&
3844: !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))

src/backend/tcop/utility.c
943: if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINTER))

4 predefined roles currently use has_privs_of_role in master.

Further, pg_monitor, as an SQL-only predefined role, also behaves
consistently with the INHERIT rules that other roles do.

In order for SQL-only predefined roles to ignore INHERIT we would need
to hardcode bypasses for them, which IMO seems like the worst possible
solution to the current inconsistency.

On Tue, Feb 8, 2022 at 10:00 AM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
> 4 predefined roles currently use has_privs_of_role in master.
>
> Further, pg_monitor, as an SQL-only predefined role, also behaves
> consistently with the INHERIT rules that other roles do.
>
> In order for SQL-only predefined roles to ignore INHERIT we would need
> to hardcode bypasses for them, which IMO seems like the worst possible
> solution to the current inconsistency.

I agree we need to make the situation consistent. But if you think
there's exactly one problem here and this patch fixes it, I
emphatically disagree.

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

On 2/8/22 10:07, Robert Haas wrote:
> On Tue, Feb 8, 2022 at 10:00 AM Joshua Brindle
> <joshua.brindle@crunchydata.com> wrote:
>> 4 predefined roles currently use has_privs_of_role in master.
>>
>> Further, pg_monitor, as an SQL-only predefined role, also behaves
>> consistently with the INHERIT rules that other roles do.
>>
>> In order for SQL-only predefined roles to ignore INHERIT we would need
>> to hardcode bypasses for them, which IMO seems like the worst possible
>> solution to the current inconsistency.
> 
> I agree we need to make the situation consistent. But if you think
> there's exactly one problem here and this patch fixes it, I
> emphatically disagree.


If we were to start all over again with this feature my vote would be to 
do things differently than we have done. I would not have called them 
predefined roles, and I would have used attributes of roles (e.g. make 
rolsuper into a bitmap rather than a boolean) rather than role 
membership to implement them. But I didn't find time to participate in 
the original discussion or review/write the code, so I have little room 
to complain.

However since we did call these things predefined roles, and used role 
membership to implement them, I think they ought to behave both 
self-consistently as a group, and like other real roles.

That is what this patch does unless I am missing something.

I guess an alternative is to discuss a "proper fix", but it seems to me 
that would be a version 16 thing given how late we are in this 
development cycle and how invasive it is likely to be. And doing nothing 
for pg15 is not a very satisfying proposition :-/

Joe


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Feb 8, 2022 at 7:38 PM Joe Conway <mail@joeconway.com> wrote:
> If we were to start all over again with this feature my vote would be to
> do things differently than we have done. I would not have called them
> predefined roles, and I would have used attributes of roles (e.g. make
> rolsuper into a bitmap rather than a boolean) rather than role
> membership to implement them. But I didn't find time to participate in
> the original discussion or review/write the code, so I have little room
> to complain.

Yep, fair. I kind of like the predefined role concept myself. I find
it sort of elegant, mostly because I think it scales better than a
bitmask, which can run out of bits surprisingly rapidly. But opinions
can vary, of course.

> However since we did call these things predefined roles, and used role
> membership to implement them, I think they ought to behave both
> self-consistently as a group, and like other real roles.
>
> That is what this patch does unless I am missing something.

Yes, I see that.

> I guess an alternative is to discuss a "proper fix", but it seems to me
> that would be a version 16 thing given how late we are in this
> development cycle and how invasive it is likely to be. And doing nothing
> for pg15 is not a very satisfying proposition :-/

True. The fact that we don't have consistency among existing
predefined roles is IMHO the best argument for this patch. That surely
can't be right.

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

On Tue, Feb 08, 2022 at 10:54:50PM -0500, Robert Haas wrote:
> On Tue, Feb 8, 2022 at 7:38 PM Joe Conway <mail@joeconway.com> wrote:
>> If we were to start all over again with this feature my vote would be to
>> do things differently than we have done. I would not have called them
>> predefined roles, and I would have used attributes of roles (e.g. make
>> rolsuper into a bitmap rather than a boolean) rather than role
>> membership to implement them. But I didn't find time to participate in
>> the original discussion or review/write the code, so I have little room
>> to complain.
> 
> Yep, fair. I kind of like the predefined role concept myself. I find
> it sort of elegant, mostly because I think it scales better than a
> bitmask, which can run out of bits surprisingly rapidly. But opinions
> can vary, of course.

I do wonder if users find the differences between predefined roles and role
attributes confusing.  INHERIT doesn't govern role attributes, but it will
govern predefined roles when this patch is applied.  Maybe the role
attribute system should eventually be deprecated in favor of using
predefined roles for everything.  Or perhaps the predefined roles should be
converted to role attributes.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Feb 9, 2022 at 1:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I do wonder if users find the differences between predefined roles and role
> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> govern predefined roles when this patch is applied.  Maybe the role
> attribute system should eventually be deprecated in favor of using
> predefined roles for everything.  Or perhaps the predefined roles should be
> converted to role attributes.

I couldn't agree more. Apparently it's even confusing to developers,
because otherwise (1) we wouldn't have the problem the patch proposes
to fix in the first place and (2) I would have immediately been
convinced of the value of the patch once it showed up. Since those
things didn't happen, this is apparently confusing to (1) whoever
wrote the code that this patch fixes and (2) me.

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

On 2/9/22 13:13, Nathan Bossart wrote:
> On Tue, Feb 08, 2022 at 10:54:50PM -0500, Robert Haas wrote:
>> On Tue, Feb 8, 2022 at 7:38 PM Joe Conway <mail@joeconway.com> wrote:
>>> If we were to start all over again with this feature my vote would be to
>>> do things differently than we have done. I would not have called them
>>> predefined roles, and I would have used attributes of roles (e.g. make
>>> rolsuper into a bitmap rather than a boolean) rather than role
>>> membership to implement them. But I didn't find time to participate in
>>> the original discussion or review/write the code, so I have little room
>>> to complain.
>> 
>> Yep, fair. I kind of like the predefined role concept myself. I find
>> it sort of elegant, mostly because I think it scales better than a
>> bitmask, which can run out of bits surprisingly rapidly. But opinions
>> can vary, of course.
> 
> I do wonder if users find the differences between predefined roles and role
> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> govern predefined roles when this patch is applied.  Maybe the role
> attribute system should eventually be deprecated in favor of using
> predefined roles for everything.  Or perhaps the predefined roles should be
> converted to role attributes.

Yep, I was suggesting that the latter would have been preferable to me 
while Robert seemed to prefer the former. Honestly I could be happy with 
either of those solutions, but as I alluded to that is probably a 
discussion for the next development cycle since I don't see us doing 
that big a change in this one.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, Feb 9, 2022 at 3:58 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Feb 9, 2022 at 1:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > I do wonder if users find the differences between predefined roles and role
> > attributes confusing.  INHERIT doesn't govern role attributes, but it will
> > govern predefined roles when this patch is applied.  Maybe the role
> > attribute system should eventually be deprecated in favor of using
> > predefined roles for everything.  Or perhaps the predefined roles should be
> > converted to role attributes.
>
> I couldn't agree more. Apparently it's even confusing to developers,
> because otherwise (1) we wouldn't have the problem the patch proposes
> to fix in the first place and (2) I would have immediately been
> convinced of the value of the patch once it showed up. Since those
> things didn't happen, this is apparently confusing to (1) whoever
> wrote the code that this patch fixes and (2) me.
>

My original patch removed is_member_of to address #1 above, but that
was rejected[1]. There is now a warning in the header beside it to
hopefully dissuade improper usage going forward.

1. https://www.postgresql.org/message-id/254275.1635357633%40sss.pgh.pa.us

On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> On 2/9/22 13:13, Nathan Bossart wrote:
>> I do wonder if users find the differences between predefined roles and role
>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>> govern predefined roles when this patch is applied.  Maybe the role
>> attribute system should eventually be deprecated in favor of using
>> predefined roles for everything.  Or perhaps the predefined roles should be
>> converted to role attributes.
> 
> Yep, I was suggesting that the latter would have been preferable to me while
> Robert seemed to prefer the former. Honestly I could be happy with either of
> those solutions, but as I alluded to that is probably a discussion for the
> next development cycle since I don't see us doing that big a change in this
> one.

I agree.  I still think Joshua's proposed patch is a worthwhile improvement
for v15.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 2/10/22 14:28, Nathan Bossart wrote:
> On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> On 2/9/22 13:13, Nathan Bossart wrote:
>>> I do wonder if users find the differences between predefined roles and role
>>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>>> govern predefined roles when this patch is applied.  Maybe the role
>>> attribute system should eventually be deprecated in favor of using
>>> predefined roles for everything.  Or perhaps the predefined roles should be
>>> converted to role attributes.
>> 
>> Yep, I was suggesting that the latter would have been preferable to me while
>> Robert seemed to prefer the former. Honestly I could be happy with either of
>> those solutions, but as I alluded to that is probably a discussion for the
>> next development cycle since I don't see us doing that big a change in this
>> one.
> 
> I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> for v15.

+1

I am planning to get into it in detail this weekend. So far I have 
really just ensured it merges cleanly and passes make world.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
>
> On 2/10/22 14:28, Nathan Bossart wrote:
> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> >> On 2/9/22 13:13, Nathan Bossart wrote:
> >>> I do wonder if users find the differences between predefined roles and role
> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> >>> govern predefined roles when this patch is applied.  Maybe the role
> >>> attribute system should eventually be deprecated in favor of using
> >>> predefined roles for everything.  Or perhaps the predefined roles should be
> >>> converted to role attributes.
> >>
> >> Yep, I was suggesting that the latter would have been preferable to me while
> >> Robert seemed to prefer the former. Honestly I could be happy with either of
> >> those solutions, but as I alluded to that is probably a discussion for the
> >> next development cycle since I don't see us doing that big a change in this
> >> one.
> >
> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> > for v15.
>
> +1
>
> I am planning to get into it in detail this weekend. So far I have
> really just ensured it merges cleanly and passes make world.
>

Rebased patch to apply to master attached.

On 3/3/22 11:26, Joshua Brindle wrote:
> On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
>>
>> On 2/10/22 14:28, Nathan Bossart wrote:
>> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> >> On 2/9/22 13:13, Nathan Bossart wrote:
>> >>> I do wonder if users find the differences between predefined roles and role
>> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>> >>> govern predefined roles when this patch is applied.  Maybe the role
>> >>> attribute system should eventually be deprecated in favor of using
>> >>> predefined roles for everything.  Or perhaps the predefined roles should be
>> >>> converted to role attributes.
>> >>
>> >> Yep, I was suggesting that the latter would have been preferable to me while
>> >> Robert seemed to prefer the former. Honestly I could be happy with either of
>> >> those solutions, but as I alluded to that is probably a discussion for the
>> >> next development cycle since I don't see us doing that big a change in this
>> >> one.
>> >
>> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
>> > for v15.
>>
>> +1
>>
>> I am planning to get into it in detail this weekend. So far I have
>> really just ensured it merges cleanly and passes make world.
> 
> Rebased patch to apply to master attached.

Well longer than I planned, but finally took a closer look.

I made one minor editorial fix to Joshua's patch, rebased to current 
master, and added two missing call sites that presumably are related to 
recent commits for pg_basebackup.

On that last note, I did not find basebackup_to_shell.required_role 
documented at all, and did not attempt to fix that.

When this thread petered out, it seemed that Robert was at least neutral 
on the patch, and everyone else was +1 on applying it to master for pg15.

As such, if there are any other issues, complaints, etc., please speak 
real soon now...

Thanks,

Joe

On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com> wrote:
>
> On 3/3/22 11:26, Joshua Brindle wrote:
> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
> >>
> >> On 2/10/22 14:28, Nathan Bossart wrote:
> >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> >> >> On 2/9/22 13:13, Nathan Bossart wrote:
> >> >>> I do wonder if users find the differences between predefined roles and role
> >> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> >> >>> govern predefined roles when this patch is applied.  Maybe the role
> >> >>> attribute system should eventually be deprecated in favor of using
> >> >>> predefined roles for everything.  Or perhaps the predefined roles should be
> >> >>> converted to role attributes.
> >> >>
> >> >> Yep, I was suggesting that the latter would have been preferable to me while
> >> >> Robert seemed to prefer the former. Honestly I could be happy with either of
> >> >> those solutions, but as I alluded to that is probably a discussion for the
> >> >> next development cycle since I don't see us doing that big a change in this
> >> >> one.
> >> >
> >> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> >> > for v15.
> >>
> >> +1
> >>
> >> I am planning to get into it in detail this weekend. So far I have
> >> really just ensured it merges cleanly and passes make world.
> >
> > Rebased patch to apply to master attached.
>
> Well longer than I planned, but finally took a closer look.
>
> I made one minor editorial fix to Joshua's patch, rebased to current
> master, and added two missing call sites that presumably are related to
> recent commits for pg_basebackup.

FWIW I pinged Stephen when I saw the basebackup changes included
is_member_of and he didn't think they necessarily needed to be changed
since they aren't accessible to human and you can't SET ROLE on a
replication connection in order to access the role where inheritance
was blocked.

> On that last note, I did not find basebackup_to_shell.required_role
> documented at all, and did not attempt to fix that.
>
> When this thread petered out, it seemed that Robert was at least neutral
> on the patch, and everyone else was +1 on applying it to master for pg15.
>
> As such, if there are any other issues, complaints, etc., please speak
> real soon now...
>
> Thanks,
>
> Joe

On 3/20/22 12:31, Joshua Brindle wrote:
> On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com> wrote:
>>
>> On 3/3/22 11:26, Joshua Brindle wrote:
>> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
>> >>
>> >> On 2/10/22 14:28, Nathan Bossart wrote:
>> >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> >> >> On 2/9/22 13:13, Nathan Bossart wrote:
>> >> >>> I do wonder if users find the differences between predefined roles and role
>> >> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>> >> >>> govern predefined roles when this patch is applied.  Maybe the role
>> >> >>> attribute system should eventually be deprecated in favor of using
>> >> >>> predefined roles for everything.  Or perhaps the predefined roles should be
>> >> >>> converted to role attributes.
>> >> >>
>> >> >> Yep, I was suggesting that the latter would have been preferable to me while
>> >> >> Robert seemed to prefer the former. Honestly I could be happy with either of
>> >> >> those solutions, but as I alluded to that is probably a discussion for the
>> >> >> next development cycle since I don't see us doing that big a change in this
>> >> >> one.
>> >> >
>> >> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
>> >> > for v15.
>> >>
>> >> +1
>> >>
>> >> I am planning to get into it in detail this weekend. So far I have
>> >> really just ensured it merges cleanly and passes make world.
>> >
>> > Rebased patch to apply to master attached.
>>
>> Well longer than I planned, but finally took a closer look.
>>
>> I made one minor editorial fix to Joshua's patch, rebased to current
>> master, and added two missing call sites that presumably are related to
>> recent commits for pg_basebackup.
> 
> FWIW I pinged Stephen when I saw the basebackup changes included
> is_member_of and he didn't think they necessarily needed to be changed
> since they aren't accessible to human and you can't SET ROLE on a
> replication connection in order to access the role where inheritance
> was blocked.

Maybe worth a discussion, but it seems strange to me to treat them 
differently when the whole purpose of this patch is to make things 
consistent ¯\_(ツ)_/¯

Joe

On Sun, Mar 20, 2022 at 12:34 PM Joe Conway <mail@joeconway.com> wrote:
>
> On 3/20/22 12:31, Joshua Brindle wrote:
> > On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com> wrote:
> >>
> >> On 3/3/22 11:26, Joshua Brindle wrote:
> >> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
> >> >>
> >> >> On 2/10/22 14:28, Nathan Bossart wrote:
> >> >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> >> >> >> On 2/9/22 13:13, Nathan Bossart wrote:
> >> >> >>> I do wonder if users find the differences between predefined roles and role
> >> >> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> >> >> >>> govern predefined roles when this patch is applied.  Maybe the role
> >> >> >>> attribute system should eventually be deprecated in favor of using
> >> >> >>> predefined roles for everything.  Or perhaps the predefined roles should be
> >> >> >>> converted to role attributes.
> >> >> >>
> >> >> >> Yep, I was suggesting that the latter would have been preferable to me while
> >> >> >> Robert seemed to prefer the former. Honestly I could be happy with either of
> >> >> >> those solutions, but as I alluded to that is probably a discussion for the
> >> >> >> next development cycle since I don't see us doing that big a change in this
> >> >> >> one.
> >> >> >
> >> >> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> >> >> > for v15.
> >> >>
> >> >> +1
> >> >>
> >> >> I am planning to get into it in detail this weekend. So far I have
> >> >> really just ensured it merges cleanly and passes make world.
> >> >
> >> > Rebased patch to apply to master attached.
> >>
> >> Well longer than I planned, but finally took a closer look.
> >>
> >> I made one minor editorial fix to Joshua's patch, rebased to current
> >> master, and added two missing call sites that presumably are related to
> >> recent commits for pg_basebackup.
> >
> > FWIW I pinged Stephen when I saw the basebackup changes included
> > is_member_of and he didn't think they necessarily needed to be changed
> > since they aren't accessible to human and you can't SET ROLE on a
> > replication connection in order to access the role where inheritance
> > was blocked.
>
> Maybe worth a discussion, but it seems strange to me to treat them
> differently when the whole purpose of this patch is to make things
> consistent ¯\_(ツ)_/¯
>

I don't have a strong opinion either way, just noting that it's a
possible exception area due to how it is used.

Thanks for committing this.

On 3/20/22 12:38, Stephen Frost wrote:
> Greetings,
> 
> On Sun, Mar 20, 2022 at 18:31 Joshua Brindle 
> <joshua.brindle@crunchydata.com <mailto:joshua.brindle@crunchydata.com>> 
> wrote:
> 
>     On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com
>     <mailto:mail@joeconway.com>> wrote:
>      >
>      > On 3/3/22 11:26, Joshua Brindle wrote:
>      > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com
>     <mailto:mail@joeconway.com>> wrote:
>      > >>
>      > >> On 2/10/22 14:28, Nathan Bossart wrote:
>      > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>      > >> >> On 2/9/22 13:13, Nathan Bossart wrote:
>      > >> >>> I do wonder if users find the differences between
>     predefined roles and role
>      > >> >>> attributes confusing.  INHERIT doesn't govern role
>     attributes, but it will
>      > >> >>> govern predefined roles when this patch is applied.  Maybe
>     the role
>      > >> >>> attribute system should eventually be deprecated in favor
>     of using
>      > >> >>> predefined roles for everything.  Or perhaps the
>     predefined roles should be
>      > >> >>> converted to role attributes.
>      > >> >>
>      > >> >> Yep, I was suggesting that the latter would have been
>     preferable to me while
>      > >> >> Robert seemed to prefer the former. Honestly I could be
>     happy with either of
>      > >> >> those solutions, but as I alluded to that is probably a
>     discussion for the
>      > >> >> next development cycle since I don't see us doing that big
>     a change in this
>      > >> >> one.
>      > >> >
>      > >> > I agree.  I still think Joshua's proposed patch is a
>     worthwhile improvement
>      > >> > for v15.
>      > >>
>      > >> +1
>      > >>
>      > >> I am planning to get into it in detail this weekend. So far I have
>      > >> really just ensured it merges cleanly and passes make world.
>      > >
>      > > Rebased patch to apply to master attached.
>      >
>      > Well longer than I planned, but finally took a closer look.
>      >
>      > I made one minor editorial fix to Joshua's patch, rebased to current
>      > master, and added two missing call sites that presumably are
>     related to
>      > recent commits for pg_basebackup.
> 
>     FWIW I pinged Stephen when I saw the basebackup changes included
>     is_member_of and he didn't think they necessarily needed to be changed
>     since they aren't accessible to human and you can't SET ROLE on a
>     replication connection in order to access the role where inheritance
>     was blocked.
> 
> Yeah, though that should really be very clearly explained in comments 
> around that code as anything that uses is_member should really be viewed 
> as an exception.  I also wouldn’t be against using has_privs there 
> anyway and saying that you shouldn’t be trying to connect as one role on 
> a replication connection and using the privs of another when you don’t 
> automatically inherit those rights, but on the assumption that the 
> committer intended is_member there because SET ROLE isn’t something one 
> does on replication connections, I’m alright with it being as is.


Robert -- any opinion on this? If I am not mistaken it is code that you 
are actively working on.

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 3/21/22 16:15, Joe Conway wrote:
> On 3/20/22 12:38, Stephen Frost wrote:
>> Greetings,
>> 
>> On Sun, Mar 20, 2022 at 18:31 Joshua Brindle 
>> <joshua.brindle@crunchydata.com <mailto:joshua.brindle@crunchydata.com>> 
>> wrote:
>> 
>>     On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com
>>     <mailto:mail@joeconway.com>> wrote:
>>      >
>>      > On 3/3/22 11:26, Joshua Brindle wrote:
>>      > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com
>>     <mailto:mail@joeconway.com>> wrote:
>>      > >>
>>      > >> On 2/10/22 14:28, Nathan Bossart wrote:
>>      > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>>      > >> >> On 2/9/22 13:13, Nathan Bossart wrote:
>>      > >> >>> I do wonder if users find the differences between
>>     predefined roles and role
>>      > >> >>> attributes confusing.  INHERIT doesn't govern role
>>     attributes, but it will
>>      > >> >>> govern predefined roles when this patch is applied.  Maybe
>>     the role
>>      > >> >>> attribute system should eventually be deprecated in favor
>>     of using
>>      > >> >>> predefined roles for everything.  Or perhaps the
>>     predefined roles should be
>>      > >> >>> converted to role attributes.
>>      > >> >>
>>      > >> >> Yep, I was suggesting that the latter would have been
>>     preferable to me while
>>      > >> >> Robert seemed to prefer the former. Honestly I could be
>>     happy with either of
>>      > >> >> those solutions, but as I alluded to that is probably a
>>     discussion for the
>>      > >> >> next development cycle since I don't see us doing that big
>>     a change in this
>>      > >> >> one.
>>      > >> >
>>      > >> > I agree.  I still think Joshua's proposed patch is a
>>     worthwhile improvement
>>      > >> > for v15.
>>      > >>
>>      > >> +1
>>      > >>
>>      > >> I am planning to get into it in detail this weekend. So far I have
>>      > >> really just ensured it merges cleanly and passes make world.
>>      > >
>>      > > Rebased patch to apply to master attached.
>>      >
>>      > Well longer than I planned, but finally took a closer look.
>>      >
>>      > I made one minor editorial fix to Joshua's patch, rebased to current
>>      > master, and added two missing call sites that presumably are
>>     related to
>>      > recent commits for pg_basebackup.
>> 
>>     FWIW I pinged Stephen when I saw the basebackup changes included
>>     is_member_of and he didn't think they necessarily needed to be changed
>>     since they aren't accessible to human and you can't SET ROLE on a
>>     replication connection in order to access the role where inheritance
>>     was blocked.
>> 
>> Yeah, though that should really be very clearly explained in comments 
>> around that code as anything that uses is_member should really be viewed 
>> as an exception.  I also wouldn’t be against using has_privs there 
>> anyway and saying that you shouldn’t be trying to connect as one role on 
>> a replication connection and using the privs of another when you don’t 
>> automatically inherit those rights, but on the assumption that the 
>> committer intended is_member there because SET ROLE isn’t something one 
>> does on replication connections, I’m alright with it being as is.
> 
> 
> Robert -- any opinion on this? If I am not mistaken it is code that you
> are actively working on.

Lacking any feedback from Robert, I removed my changes related to 
basebackup and pushed Joshua's patch with one minor editorial fix. What 
to do with the basebackup changes can go on the open items list for pg15 
I guess.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Mon, Mar 21, 2022 at 4:15 PM Joe Conway <mail@joeconway.com> wrote:
> Robert -- any opinion on this? If I am not mistaken it is code that you
> are actively working on.

Woops, I only just saw this. I don't mind if you want to change the
calls to is_member_of_role() in basebackup_server.c and
basebackup_to_shell.c to has_privs_of_role(). However, it's not clear
to me why it's different than the calls we have in other places, like
calculate_database_size() and the relatively widely-used
check_is_member_of_role(). As long as we have a bunch of different
practices in different parts of the code base I can't see people
getting this right consistently ... leaving aside any possible
disagreement about which way is "right".

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

On 3/28/22 15:56, Robert Haas wrote:
> On Mon, Mar 21, 2022 at 4:15 PM Joe Conway <mail@joeconway.com> wrote:
>> Robert -- any opinion on this? If I am not mistaken it is code that you
>> are actively working on.
> 
> Woops, I only just saw this. I don't mind if you want to change the
> calls to is_member_of_role() in basebackup_server.c and
> basebackup_to_shell.c to has_privs_of_role().

No worries -- I will take care of that shortly.

> However, it's not clear to me why it's different than the calls we
> have in other places, like calculate_database_size() and the
> relatively widely-used check_is_member_of_role().

I will have to go refresh my memory, but when I looked at those sites 
closely it all made sense to me.

I think most if not all of them were checking for the ability to switch 
to the other role, not actually checking for privileges by virtue of 
belonging to that role.

> As long as we have a bunch of different practices in different parts
> of the code base I can't see people getting this right consistently
> ... leaving aside any possible disagreement about which way is
> "right".
When I take the next pass I can consider whether additional comments 
will help and report back.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 3/28/22 15:56, Robert Haas wrote:
> On Mon, Mar 21, 2022 at 4:15 PM Joe Conway <mail@joeconway.com> wrote:
>> Robert -- any opinion on this? If I am not mistaken it is code that you
>> are actively working on.
> 
> Woops, I only just saw this. I don't mind if you want to change the
> calls to is_member_of_role() in basebackup_server.c and
> basebackup_to_shell.c to has_privs_of_role().

Done as originally posted.

> On that last note, I did not find basebackup_to_shell.required_role 
> documented at all, and did not attempt to fix that.

I saw that Robert added documentation and it already reads correctly I 
believe, except possibly an unrelated issue:

8<--------------
      <para>
       A role which replication whose privileges users are required to 
possess
       in order to make use of the <literal>shell</literal> backup target.
       If this is not set, any replication user may make use of the
       <literal>shell</literal> backup target.
      </para>
8<--------------

Robert, should that actually be:
  s/role which replication/role with replication/
?

Joe

On Sat, Apr 2, 2022 at 1:32 PM Joe Conway <mail@joeconway.com> wrote:
> I saw that Robert added documentation and it already reads correctly I
> believe, except possibly an unrelated issue:
>
> 8<--------------
>       <para>
>        A role which replication whose privileges users are required to
> possess
>        in order to make use of the <literal>shell</literal> backup target.
>        If this is not set, any replication user may make use of the
>        <literal>shell</literal> backup target.
>       </para>
> 8<--------------
>
> Robert, should that actually be:
>   s/role which replication/role with replication/

Oh, good catch. Actually shouldn't "with replication" just be deleted?

Or maybe it needs to be reworded more, for clarity: "Replication users
must possess the privileges of this role in order to ..."

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

On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
> > It seems to me that the INHERIT role flag isn't very well-considered.
> > Inheritance, or the lack of it, ought to be decided separately for
> > each inherited role. However, that would be a major architectural
> > change.
>
> Agreed -- that would be useful.

Is this a kind of change people would support? Here's a quick sketch:

1. Extend the GRANT role_name TO role_name [ WITH ADMIN OPTION ] with
a new, optional clause, something like WITH NO INHERIT or WITH
NOINHERIT or WITHOUT INHERIT.
2. Remove the INHERIT | NOINHERIT flag from CREATE ROLE and ALTER ROLE.
3. Replace pg_authid.rolinherit with pg_auth_members.inherit. Any
place where we would have considered rolinherit, instead consider the
inherit flag for the particular pg_auth_members entry at issue.
4. When dumping from an old version, dump all grants to NOINHERIT
roles as non-inheritable grants.

The idea here is that, today, a role must either inherit privileges
from all roles of which it is a member or none of them. With this
proposal, you could make a role inherit some of those privileges but
not others. I think it's not difficult to see how that might be
useful: for example, user A could be granted non-login role X with
inheritance and login role B without inheritance. That would allow
user A to exercise the privileges of a member of group X at all times,
and the privileges of user B only with an explicit SET ROLE operation.
That way, A is empowered to act for B when necessary, but won't
accidentally do so.

It's been proposed elsewhere by Stephen and others that we ought to
have the ability to grant ADMIN OPTION on a role without granting
membership in that role. There's some overlap between these two
proposals. If your concern is just about accident prevention, you will
be happy to use this instead of that. If you want real access
restrictions, you will want that, not this. I think that's OK. Doing
this first doesn't mean we can't do that later. What about the
reverse? Could we add ADMIN-without-membership first, and then decide
whether to do this later? Maybe so, but I have come to feel that
NOINHERIT is such an ugly wart that we'll be happier the sooner we
kill it. It seems to make every discussion that we have about any
other potential change in this area more difficult, and I venture that
ADMIN-without-membership wouldn't turn out to be an exception.

Based on previous discussions I think that, long term, we're probably
headed toward a future where role grants have a bunch of different
flags, each of which controls a single behavior: whether you can
implicitly use the privileges of the role, whether you can access them
via SET ROLE, whether you can grant the role to others, or revoke it
from them, etc. I don't know exactly what the final list will look
like, and hopefully it won't be so long that it makes us all wish we
were dead, but there doesn't seem to be any possibility that implicit
inheritance of permissions won't be in that list. I spent a few
minutes considering whether I ought to instead propose that we just
nuke NOINHERIT from orbit and replace it with absolutely nothing, and
concluded that such a proposal had no hope of attracting consensus.
Perhaps the idea of replacing it with that is more powerful and at
least IMHO cleaner will.

Thoughts?

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

Robert Haas <robertmhaas@gmail.com> writes:
> Is this a kind of change people would support? Here's a quick sketch:

> 1. Extend the GRANT role_name TO role_name [ WITH ADMIN OPTION ] with
> a new, optional clause, something like WITH NO INHERIT or WITH
> NOINHERIT or WITHOUT INHERIT.
> 2. Remove the INHERIT | NOINHERIT flag from CREATE ROLE and ALTER ROLE.
> 3. Replace pg_authid.rolinherit with pg_auth_members.inherit. Any
> place where we would have considered rolinherit, instead consider the
> inherit flag for the particular pg_auth_members entry at issue.
> 4. When dumping from an old version, dump all grants to NOINHERIT
> roles as non-inheritable grants.

Point 2 would cause every existing pg_dumpall script to fail, which
seems like kind of a large gotcha.  Less unpleasant alternatives
could include

* Continue to accept the syntax, but ignore it, maybe with a WARNING
for the alternative that doesn't correspond to the new behavior.

* Keep pg_authid.rolinherit, and have it act as supplying the default
behavior for subsequent GRANTs to that role.

Perhaps there are other ways.

            regards, tom lane

On Thu, Jun 2, 2022 at 1:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Point 2 would cause every existing pg_dumpall script to fail, which
> seems like kind of a large gotcha.  Less unpleasant alternatives
> could include
>
> * Continue to accept the syntax, but ignore it, maybe with a WARNING
> for the alternative that doesn't correspond to the new behavior.
>
> * Keep pg_authid.rolinherit, and have it act as supplying the default
> behavior for subsequent GRANTs to that role.

Of those two alternatives, I would certainly prefer the first, because
the second doesn't actually get rid of the ugly wart. It just adds a
non-ugly thing that we have to maintain along with the ugly thing,
apparently in perpetuity. If we do the first of these, we can probably
remove the obsolete syntax at some point in the distant future, and in
the meantime, we don't have to figure out how it's supposed to
interact with existing features or new ones, since the actual feature
is already removed.

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

On Thu, Jun 02, 2022 at 12:26:48PM -0400, Robert Haas wrote:
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
>> > It seems to me that the INHERIT role flag isn't very well-considered.
>> > Inheritance, or the lack of it, ought to be decided separately for
>> > each inherited role. However, that would be a major architectural
>> > change.
>>
>> Agreed -- that would be useful.
> 
> Is this a kind of change people would support? Here's a quick sketch:

Yes.

> The idea here is that, today, a role must either inherit privileges
> from all roles of which it is a member or none of them. With this
> proposal, you could make a role inherit some of those privileges but
> not others. I think it's not difficult to see how that might be
> useful: for example, user A could be granted non-login role X with
> inheritance and login role B without inheritance. That would allow
> user A to exercise the privileges of a member of group X at all times,
> and the privileges of user B only with an explicit SET ROLE operation.
> That way, A is empowered to act for B when necessary, but won't
> accidentally do so.

I think we should also consider replacing role attributes with predefined
roles.  I'm not sure that this proposal totally prepares us for such a
change, given role attributes apply only to the specific role for which
they are set and aren't inherited.  ISTM in order to support that, we'd
need even more enhanced functionality.  For example, if I want 'robert' to
be a superuser, and I want 'joe' to inherit the privileges of 'robert' but
not 'pg_superuser', you'd need some way to specify inheriting only certain
privileges possessed by an intermediate role.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jun 2, 2022 at 2:07 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I think we should also consider replacing role attributes with predefined
> roles.  I'm not sure that this proposal totally prepares us for such a
> change, given role attributes apply only to the specific role for which
> they are set and aren't inherited.  ISTM in order to support that, we'd
> need even more enhanced functionality.  For example, if I want 'robert' to
> be a superuser, and I want 'joe' to inherit the privileges of 'robert' but
> not 'pg_superuser', you'd need some way to specify inheriting only certain
> privileges possessed by an intermediate role.

I guess we could think about adding something like an ONLY clause,
like GRANT ONLY robert TO joe. I feel a little bit uncomfortable about
that, though, because it assumes that robert is a superuser but his
own privileges are distinguishable from those of the superuser. Are
they really? If I can assume robert's identity, I can presumably
Trojan my way into the superuser account pretty easily. I'll just
define a little trigger on one of his tables. I don't really see a way
where we can ever make it safe to grant a non-superuser membership in
a superuser role.

But even if there is a way, I think that is a separate patch from what
I'm proposing here. [NO]INHERIT only has to do with what privileges
you can exercise without SET ROLE. To solve the problem you're talking
about here, you'd need a way to control what privileges are conferred
in any manner, which is related, but different.

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

On Thu, Jun 02, 2022 at 03:37:34PM -0400, Robert Haas wrote:
> On Thu, Jun 2, 2022 at 2:07 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I think we should also consider replacing role attributes with predefined
>> roles.  I'm not sure that this proposal totally prepares us for such a
>> change, given role attributes apply only to the specific role for which
>> they are set and aren't inherited.  ISTM in order to support that, we'd
>> need even more enhanced functionality.  For example, if I want 'robert' to
>> be a superuser, and I want 'joe' to inherit the privileges of 'robert' but
>> not 'pg_superuser', you'd need some way to specify inheriting only certain
>> privileges possessed by an intermediate role.
> 
> I guess we could think about adding something like an ONLY clause,
> like GRANT ONLY robert TO joe. I feel a little bit uncomfortable about
> that, though, because it assumes that robert is a superuser but his
> own privileges are distinguishable from those of the superuser. Are
> they really? If I can assume robert's identity, I can presumably
> Trojan my way into the superuser account pretty easily. I'll just
> define a little trigger on one of his tables. I don't really see a way
> where we can ever make it safe to grant a non-superuser membership in
> a superuser role.

I was primarily looking at this from the angle of preserving current
behavior when upgrading from a version with role attributes to a version
without them.  If it's alright that a role with privileges of a superuser
role begins being treated like a superuser after an upgrade, then we
probably don't need something like GRANT ONLY.  I bet that's how a lot of
people expect role attributes to work, anyway.  I'm sure I did at some
point.

> But even if there is a way, I think that is a separate patch from what
> I'm proposing here. [NO]INHERIT only has to do with what privileges
> you can exercise without SET ROLE. To solve the problem you're talking
> about here, you'd need a way to control what privileges are conferred
> in any manner, which is related, but different.

I agree that the role-attribute-to-predefined-role stuff needs its own
thread.  I just think it's worth designing this stuff with that in mind.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 2022-06-02 Th 14:06, Robert Haas wrote:
> On Thu, Jun 2, 2022 at 1:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Point 2 would cause every existing pg_dumpall script to fail, which
>> seems like kind of a large gotcha.  Less unpleasant alternatives
>> could include
>>
>> * Continue to accept the syntax, but ignore it, maybe with a WARNING
>> for the alternative that doesn't correspond to the new behavior.
>>
>> * Keep pg_authid.rolinherit, and have it act as supplying the default
>> behavior for subsequent GRANTs to that role.
> Of those two alternatives, I would certainly prefer the first, because
> the second doesn't actually get rid of the ugly wart. It just adds a
> non-ugly thing that we have to maintain along with the ugly thing,
> apparently in perpetuity. If we do the first of these, we can probably
> remove the obsolete syntax at some point in the distant future, and in
> the meantime, we don't have to figure out how it's supposed to
> interact with existing features or new ones, since the actual feature
> is already removed.
>

+1


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Jun 2, 2022 at 12:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 1. Extend the GRANT role_name TO role_name [ WITH ADMIN OPTION ] with
> a new, optional clause, something like WITH NO INHERIT or WITH
> NOINHERIT or WITHOUT INHERIT.

I just realized that, with ADMIN OPTION, you can change your mind
after the initial grant, and we probably would want to allow the same
kind of thing here. The existing syntax is:

1. GRANT foo TO bar [WITH ADMIN OPTION];
2. REVOKE foo FROM bar;
3. REVOKE ADMIN OPTION FOR foo FROM bar;

To grant the admin option later, you just use (1) again and this time
include WITH ADMIN OPTION. To revoke it without removing the grant
entirely, you use (3). This could easily be generalized to any number
of options all of which are Boolean properties and all of which have a
default value of false, but INHERIT is a Boolean property with a
default value of true, and calling the property NOINHERIT to dodge
that issue seems dumb. I'd like to find a way to extend the syntax
that can accommodate not only INHERIT as an option, but any other
options we might want to add in the future, and maybe even leave room
for non-Boolean properties, just in case. The question of which
options we think it valuable to add is separate from what the syntax
ought to be, so for syntax discussion purposes let's suppose we want
to add three new options: FRUIT, which can be strawberry, banana, or
kiwi; CHOCOLATE, a Boolean whose default value is true; and SARDINES,
another Boolean whose default value is false. Then:

GRANT foo TO bar WITH FRUIT STRAWBERRY;
GRANT foo TO bar WITH CHOCOLATE FALSE;
GRANT foo TO bar WITH SARDINES TRUE;
GRANT foo TO bar WITH SARDINES; -- same as previous
GRANT foo TO bar WITH SARDINES OPTION; -- also same as previous
GRANT foo TO bar WITH FRUIT KIWI, SARDINES; -- multiple options
GRANT foo TO bar WITH CHOCOLATE OPTION, SARDINES OPTION; -- dubious combination

In other words, you write a comma-separated list of option names. Each
option name can be followed by an option value or by the word OPTION.
If it is followed by the word OPTION or by nothing, the option value
is taken to be true. This would mean that, going forward, any of the
following would work: (a) GRANT foo TO bar WITH ADMIN OPTION, (b)
GRANT foo TO BAR WITH ADMIN, (c) GRANT foo TO BAR WITH ADMIN TRUE.

To revoke a grant entirely, you just say REVOKE foo FROM bar, as now.
To change an option for an existing grant, you can re-execute the
grant statement with a different WITH clause. Any options that are
explicitly mentioned will be changed to have the associated values;
unmentioned options will retain their existing values. If you want to
change the value of a Boolean option to false, you have a second
option, which is to write "REVOKE option_name OPTION FOR foo FROM
bar," which means exactly the same thing as "GRANT foo TO bar WITH
option_name FALSE".

In terms of what options to offer, the most obvious idea is to just
add INHERIT as a Boolean option which is true by default. We could go
further and also add a SET option, with the idea that INHERIT OPTION
controls whether you can exercise the privileges of the role without
SET ROLE, and SET OPTION controls whether you can switch to that role
using the SET ROLE command. Those two things together would give us a
way to get to the admin-without-membership concept that we have
previously discussed: GRANT foo TO BAR WITH ADMIN TRUE, INHERIT FALSE,
SET FALSE sounds like it should do the trick.

I briefly considered suggesting that the way to set a Boolean-valued
option to false ought to be to write "NO option_name" rather than
"option_name FALSE", since it reads more naturally, but I proposed
this instead because it's more like what we do for other options lists
(cf. EXPLAIN, VACUUM, COPY).

Thoughts?

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:
> > > It seems to me that the INHERIT role flag isn't very well-considered.
> > > Inheritance, or the lack of it, ought to be decided separately for
> > > each inherited role. However, that would be a major architectural
> > > change.
> >
> > Agreed -- that would be useful.
>
> Is this a kind of change people would support? Here's a quick sketch:

On the whole, moving things to pg_auth_members when they're about
relationships between roles makes more sense to me too (ISTR mentioning
that somewhere or at least thinking about it but not sure where and it
doesn't really matter).

> It's been proposed elsewhere by Stephen and others that we ought to
> have the ability to grant ADMIN OPTION on a role without granting
> membership in that role. There's some overlap between these two
> proposals. If your concern is just about accident prevention, you will
> be happy to use this instead of that. If you want real access
> restrictions, you will want that, not this. I think that's OK. Doing
> this first doesn't mean we can't do that later. What about the
> reverse? Could we add ADMIN-without-membership first, and then decide
> whether to do this later? Maybe so, but I have come to feel that
> NOINHERIT is such an ugly wart that we'll be happier the sooner we
> kill it. It seems to make every discussion that we have about any
> other potential change in this area more difficult, and I venture that
> ADMIN-without-membership wouldn't turn out to be an exception.

Being able to segregate the control over privileges from the control
over objects is definitely helpful in some environments.  I don't see
any strong reason that one must happen before the other and am happy to
defer to whomever has cycles to work on these things to sort that out.

> Based on previous discussions I think that, long term, we're probably
> headed toward a future where role grants have a bunch of different
> flags, each of which controls a single behavior: whether you can
> implicitly use the privileges of the role, whether you can access them
> via SET ROLE, whether you can grant the role to others, or revoke it
> from them, etc. I don't know exactly what the final list will look
> like, and hopefully it won't be so long that it makes us all wish we
> were dead, but there doesn't seem to be any possibility that implicit
> inheritance of permissions won't be in that list. I spent a few
> minutes considering whether I ought to instead propose that we just
> nuke NOINHERIT from orbit and replace it with absolutely nothing, and
> concluded that such a proposal had no hope of attracting consensus.

I agree that a future where there's more granularity in terms of role
grants would be a better place than where we are now.  I'd be -1 on just
removing 'NOINHERIT', to no one's surprise, I'm sure.

> Perhaps the idea of replacing it with that is more powerful and at
> least IMHO cleaner will.

-EPARSEFAIL (tho I'm guessing you were intending to say that replacing
the role-attribute noinherit with something more powerful would be a
generally agreeable way forward, which I would generally support)

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Jun 2, 2022 at 12:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > 1. Extend the GRANT role_name TO role_name [ WITH ADMIN OPTION ] with
> > a new, optional clause, something like WITH NO INHERIT or WITH
> > NOINHERIT or WITHOUT INHERIT.
>
> I just realized that, with ADMIN OPTION, you can change your mind
> after the initial grant, and we probably would want to allow the same
> kind of thing here. The existing syntax is:

Yes.

> 1. GRANT foo TO bar [WITH ADMIN OPTION];
> 2. REVOKE foo FROM bar;
> 3. REVOKE ADMIN OPTION FOR foo FROM bar;
>
> To grant the admin option later, you just use (1) again and this time
> include WITH ADMIN OPTION. To revoke it without removing the grant
> entirely, you use (3). This could easily be generalized to any number
> of options all of which are Boolean properties and all of which have a
> default value of false, but INHERIT is a Boolean property with a
> default value of true, and calling the property NOINHERIT to dodge
> that issue seems dumb. I'd like to find a way to extend the syntax
> that can accommodate not only INHERIT as an option, but any other
> options we might want to add in the future, and maybe even leave room
> for non-Boolean properties, just in case. The question of which
> options we think it valuable to add is separate from what the syntax
> ought to be, so for syntax discussion purposes let's suppose we want
> to add three new options: FRUIT, which can be strawberry, banana, or
> kiwi; CHOCOLATE, a Boolean whose default value is true; and SARDINES,
> another Boolean whose default value is false. Then:
>
> GRANT foo TO bar WITH FRUIT STRAWBERRY;
> GRANT foo TO bar WITH CHOCOLATE FALSE;
> GRANT foo TO bar WITH SARDINES TRUE;
> GRANT foo TO bar WITH SARDINES; -- same as previous
> GRANT foo TO bar WITH SARDINES OPTION; -- also same as previous
> GRANT foo TO bar WITH FRUIT KIWI, SARDINES; -- multiple options
> GRANT foo TO bar WITH CHOCOLATE OPTION, SARDINES OPTION; -- dubious combination
>
> In other words, you write a comma-separated list of option names. Each
> option name can be followed by an option value or by the word OPTION.
> If it is followed by the word OPTION or by nothing, the option value
> is taken to be true. This would mean that, going forward, any of the
> following would work: (a) GRANT foo TO bar WITH ADMIN OPTION, (b)
> GRANT foo TO BAR WITH ADMIN, (c) GRANT foo TO BAR WITH ADMIN TRUE.

Seems generally reasonable... though I'm thinking that we should make
OPTION only be accepted for ADMIN as TRUE rather than having it
magically work for all the other things because ... why would we?  What
if we decided later that we wanted a FRUIT OPTION (to use your example
of FRUIT being an enum).  I don't feel very strongly on that point
though (but what if you wrote FRUIT OPTION and FRUIT isn't able to just
be TRUE?  Would that blow up, or?).

> To revoke a grant entirely, you just say REVOKE foo FROM bar, as now.
> To change an option for an existing grant, you can re-execute the
> grant statement with a different WITH clause. Any options that are
> explicitly mentioned will be changed to have the associated values;
> unmentioned options will retain their existing values. If you want to
> change the value of a Boolean option to false, you have a second
> option, which is to write "REVOKE option_name OPTION FOR foo FROM
> bar," which means exactly the same thing as "GRANT foo TO bar WITH
> option_name FALSE".

I'm a bit concerned about this because, iiuc, it would mean:

GRANT foo TO bar WITH FRUIT KIWI, SARDINES;
GRANT foo TO bar WITH FRUIT STRAWBERRY;

would mean that the GRANT of FRUIT would then *only* have STRAWBERRY,
right?  The first GRANT in this example would essentially be entirely
undone by the second GRANT.  Having GRANT remove things would be new, I
believe.  An alternative would be to have GRANT continue to always be
'additive' and require using REVOKE to remove them, ala:

REVOKE FRUIT OPTION FOR foo FROM bar;
GRANT foo TO bar WITH FRUIT STRAWBERRY;

Another tricky item to consider here is the "implicitly includes role
membership" part of the GRANT.  That is, the spec explicitly says:

GRANT foo TO bar WITH ADMIN OPTION;

Means that 'foo' is grant'd to 'bar' (plus the admin option thing is
done).  In your proposal, does:

GRANT foo TO bar WITH FRUIT STRAWBERRY;

mean that 'foo' is grant'd to 'bar' too?  Seems to be closest to current
usage and the spec's ideas on these things.  I'm thinking that could be
dealt with by having a MEMBERSHIP option (which would be a separate
column in pg_auth_members and default would be 'true') but otherwise
using exactly what you have here, eg:

GRANT foo TO bar;
GRANT foo TO bar WITH MEMBERSHIP; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP TRUE; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP OPTION; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP FALSE; -- no-op
GRANT foo TO bar WITH MEMBERSHIP FALSE, FRUIT STRAWBERRY; -- only fruit
GRANT foo TO bar WITH FRUIT STRAWBERRY; -- with membership and fruit

Note also that the spec defines a WITH HIERARCHY OPTION too (but it's
explicitly defined for table privileges not roles, but still part of the
overall GRANT syntax) and after those WITH's has a GRANTED BY syntax
(for both roles and object privileges, which are actually distinct
things in the spec), just when thinking about what will/won't work in
the grammar.

> In terms of what options to offer, the most obvious idea is to just
> add INHERIT as a Boolean option which is true by default. We could go
> further and also add a SET option, with the idea that INHERIT OPTION
> controls whether you can exercise the privileges of the role without
> SET ROLE, and SET OPTION controls whether you can switch to that role
> using the SET ROLE command. Those two things together would give us a
> way to get to the admin-without-membership concept that we have
> previously discussed: GRANT foo TO BAR WITH ADMIN TRUE, INHERIT FALSE,
> SET FALSE sounds like it should do the trick.

Guess this is pretty close to what I just suggested above, but using
"SET" for that strikes me as quite generic while MEMBERSHIP is clearer.

Not really sure how I feel about what the results of:

GRANT foo TO bar;
GRANT foo TO bar WITH MEMBERSHIP FALSE;

should be with this but I think I'm leaning towards the "GRANT isn't for
removing stuff" side.  A GRANT is for giving things, REVOKE is for
taking things away, meaning that the second statement above would,
again, be a no-op.

> I briefly considered suggesting that the way to set a Boolean-valued
> option to false ought to be to write "NO option_name" rather than
> "option_name FALSE", since it reads more naturally, but I proposed
> this instead because it's more like what we do for other options lists
> (cf. EXPLAIN, VACUUM, COPY).

The spec has WITH ADMIN OPTION and WITH HIERARCHY OPTION, so going in
that general direction seems a bit better.  Also, as mentioned, GRANT
doesn't really do 'subtractive' things, so having a 'NO' in there seems
to go against the grain.

Thanks!

Stephen

[ trimming various comments that broadly make sense to me and which
don't seem to require further comment in this moment ]

On Mon, Jun 6, 2022 at 7:21 PM Stephen Frost <sfrost@snowman.net> wrote:
> > To revoke a grant entirely, you just say REVOKE foo FROM bar, as now.
> > To change an option for an existing grant, you can re-execute the
> > grant statement with a different WITH clause. Any options that are
> > explicitly mentioned will be changed to have the associated values;
> > unmentioned options will retain their existing values. If you want to
> > change the value of a Boolean option to false, you have a second
> > option, which is to write "REVOKE option_name OPTION FOR foo FROM
> > bar," which means exactly the same thing as "GRANT foo TO bar WITH
> > option_name FALSE".
>
> I'm a bit concerned about this because, iiuc, it would mean:
>
> GRANT foo TO bar WITH FRUIT KIWI, SARDINES;
> GRANT foo TO bar WITH FRUIT STRAWBERRY;
>
> would mean that the GRANT of FRUIT would then *only* have STRAWBERRY,
> right?

I think that you are misunderstanding what kind of option I intended
FRUIT to be. Here, I was imagining FRUIT as a property that every
grant has. Any given grant is either strawberry, or it's kiwi, or it's
banana. It cannot be more than one of those things, nor can it be none
of those things. It follows that if you execute GRANT without
specifying a FRUIT, there's some default - hopefully banana, but
that's a matter of taste. Later, you can change the fruit associated
with a grant, but you cannot remove it, because there's no such thing
as a fruitless grant. Imagine that the catalog representation is a
"char" that is either 's', 'k', or 'b'.

Now you could certainly question whether it's a good idea for us to
have an option that works like this. I don't really know. For a while
I thought that it might make sense to propose something like ACCESS {
EXPLICIT | IMPLICIT | NONE }, where ACCESS IMPLICIT would mean that
the grantee implicitly has the permissions of the role, ACCESS
EXPLICIT means that they do not implicitly have those permissions but
can access them via SET ROLE, and ACCESS NONE means that they can't
even do that. The default would I suppose be ACCESS IMPLICIT but you
could change it to one of the other two. However, I then thought that
it made more sense to keep it as two separate Booleans because
actually all four combinations are sensible: you could want to have a
setup where you're allowed to implicitly access the permissions of the
role but you CANNOT SET ROLE to it. For instance, this might make
sense for a predefined role, so that you don't end up with tables
owned by pg_monitor or whatever.

Anyway, if the hypothetical FRUIT property works as I describe here -
there's always a single value - then the second GRANT leaves the
SARDINES property set, but changes the FRUIT property from strawberry
to kiwi. Since the property is single-valued, you cannot add a second
fruit, nor can you remove the fruit altogether, because those just
aren't sensible ideas with an option of this kind. As alonger example,
it's like the FORMAT property of EXPLAIN: it always has to be TEXT or
XML or JSON. You can choose not to explicitly specify the option, but
then you get a default. Your EXPLAIN output always has to have some
format.

> In your proposal, does:
>
> GRANT foo TO bar WITH FRUIT STRAWBERRY;
>
> mean that 'foo' is grant'd to 'bar' too?  Seems to be closest to current
> usage and the spec's ideas on these things.  I'm thinking that could be
> dealt with by having a MEMBERSHIP option (which would be a separate
> column in pg_auth_members and default would be 'true') but otherwise
> using exactly what you have here, eg:

Currently, GRANT always creates an entry in pg_auth_members, or
modifies an existing one, or does nothing because the one that's there
is the same as the one it would have created. I think we should stick
with that idea.

That's why I proposed the name SET, not MEMBERSHIP. You would still
get a catalog entry in pg_auth_members, so you are still a member in
some loose sense even if your grant has INHERIT FALSE and SET FALSE,
but in such a case the fact that you are a member isn't really doing
anything for you in terms of getting you access to privileges because
you're neither allowed to exercise them implicitly nor SET ROLE to the
role.

I find that idea - that GRANT always grants membership but membership
by itself doesn't really do anything for you unless you've got some
other options enabled somewhere - more appealing than the design you
seem to have in mind, which seems to me that membership is the same
thing as the ability to SET ROLE and thus, if the ability to SET ROLE
has not been granted, you have a grant that didn't confirm membership
in any sense. I'm not saying we couldn't make that work, but I think
it's awkward to make that work. Among other problems, what happens
with the actual catalog representation? You could for example still
create a role in pg_auth_members and then have a Boolean column
membership = false, but that's a bit odd. Or you could add a new
catalog or you could rename the existing catalog, but that's more
complicated for not much benefit. I think there's some fuzziness at
the semantic level with this kind of thing too: if I do a GRANT with
MEMBERSHIP FALSE, what exactly is it that I am granting? I like the
conceptual simplicity of being able to say that a GRANT always confers
membership, but membership does not intrinsically include the ability
to SET ROLE -- that's a Boolean property of membership, not membership
itself.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Jun 6, 2022 at 7:21 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > To revoke a grant entirely, you just say REVOKE foo FROM bar, as now.
> > > To change an option for an existing grant, you can re-execute the
> > > grant statement with a different WITH clause. Any options that are
> > > explicitly mentioned will be changed to have the associated values;
> > > unmentioned options will retain their existing values. If you want to
> > > change the value of a Boolean option to false, you have a second
> > > option, which is to write "REVOKE option_name OPTION FOR foo FROM
> > > bar," which means exactly the same thing as "GRANT foo TO bar WITH
> > > option_name FALSE".
> >
> > I'm a bit concerned about this because, iiuc, it would mean:
> >
> > GRANT foo TO bar WITH FRUIT KIWI, SARDINES;
> > GRANT foo TO bar WITH FRUIT STRAWBERRY;
> >
> > would mean that the GRANT of FRUIT would then *only* have STRAWBERRY,
> > right?
>
> I think that you are misunderstanding what kind of option I intended
> FRUIT to be. Here, I was imagining FRUIT as a property that every
> grant has. Any given grant is either strawberry, or it's kiwi, or it's
> banana. It cannot be more than one of those things, nor can it be none
> of those things. It follows that if you execute GRANT without
> specifying a FRUIT, there's some default - hopefully banana, but
> that's a matter of taste. Later, you can change the fruit associated
> with a grant, but you cannot remove it, because there's no such thing
> as a fruitless grant. Imagine that the catalog representation is a
> "char" that is either 's', 'k', or 'b'.

Ah, yeah, if it's always single-value then that seems reasonable to me
too.  If we ever get to wanting to support multiple choices for a given
option then we could possibly require they be provided as an ARRAY or
using ()'s or something else, but we probably don't need to try and sort
that today.

> > In your proposal, does:
> >
> > GRANT foo TO bar WITH FRUIT STRAWBERRY;
> >
> > mean that 'foo' is grant'd to 'bar' too?  Seems to be closest to current
> > usage and the spec's ideas on these things.  I'm thinking that could be
> > dealt with by having a MEMBERSHIP option (which would be a separate
> > column in pg_auth_members and default would be 'true') but otherwise
> > using exactly what you have here, eg:
>
> Currently, GRANT always creates an entry in pg_auth_members, or
> modifies an existing one, or does nothing because the one that's there
> is the same as the one it would have created. I think we should stick
> with that idea.

Alright.

> That's why I proposed the name SET, not MEMBERSHIP. You would still
> get a catalog entry in pg_auth_members, so you are still a member in
> some loose sense even if your grant has INHERIT FALSE and SET FALSE,
> but in such a case the fact that you are a member isn't really doing
> anything for you in terms of getting you access to privileges because
> you're neither allowed to exercise them implicitly nor SET ROLE to the
> role.

Naming things is hard. :)  I'm still not a fan of calling that option
'SET' and 'membership' feels like how it's typically described today
when someone has the rights of a group (implicitly or explicitly).  As
for what to call "has a pg_auth_members row but no actual access", maybe
'associated'?

That does lead me down a bit of a rabbit hole because every role in the
entire system could be considered 'associated' with every other one and
if the case of "no pg_auth_members row" is identical to the case of
"pg_auth_members row with everything off/default" then it feels a bit
odd to have an entry for it- and is there any way to get rid of that
entry?

All that said ... we have a similar thing with GRANT today when it comes
to privileges on objects in that we go from NULL to owner-all+whatever,
and while it's a bit odd, it works well enough.

> I find that idea - that GRANT always grants membership but membership
> by itself doesn't really do anything for you unless you've got some
> other options enabled somewhere - more appealing than the design you
> seem to have in mind, which seems to me that membership is the same
> thing as the ability to SET ROLE and thus, if the ability to SET ROLE
> has not been granted, you have a grant that didn't confirm membership
> in any sense. I'm not saying we couldn't make that work, but I think
> it's awkward to make that work. Among other problems, what happens
> with the actual catalog representation? You could for example still
> create a role in pg_auth_members and then have a Boolean column
> membership = false, but that's a bit odd. Or you could add a new
> catalog or you could rename the existing catalog, but that's more
> complicated for not much benefit. I think there's some fuzziness at
> the semantic level with this kind of thing too: if I do a GRANT with
> MEMBERSHIP FALSE, what exactly is it that I am granting? I like the
> conceptual simplicity of being able to say that a GRANT always confers
> membership, but membership does not intrinsically include the ability
> to SET ROLE -- that's a Boolean property of membership, not membership
> itself.

I agree with having the ability to have the SET ROLE privilege be
distinct and able to be given, or not.  I don't think we need a new
catalog either, my thought was more along the lines of just renaming
what you proposed as being 'SET' to be 'MEMBERSHIP' while mostly keeping
the rest the same, but I did want to ask the question that didn't get
answered above:

> > In your proposal, does:
> >
> > GRANT foo TO bar WITH FRUIT STRAWBERRY;
> >
> > mean that 'foo' is grant'd to 'bar' too?

That is, regardless of how we track these things in the catalog or such,
we have to respect that:

GRANT foo TO bar;

is a SQL-defined thing that says that a 'role authorization descriptor'
is created.  SET ROLE then checks if a role authorization descriptor
exists or not matching the current role to the new role and if it does
then the current role is changed to the new role.  What I was really
trying to get at above is that:

GRANT foo TO bar WITH $anything-other-than-SET-false;

should probably also create a 'role authorization descriptor' that SET
ROLE will pick up on.  In other words, the 'SET' thing, or if we call
that something else, should exist as a distinct column in
pg_auth_members, but the default value of it should be 'true', with the
ability for it to be turned to false either at GRANT time or with a
REVOKE.

Thanks,

Stephen

On Wed, Jun 8, 2022 at 10:16 AM Stephen Frost <sfrost@snowman.net> wrote:
> > That's why I proposed the name SET, not MEMBERSHIP. You would still
> > get a catalog entry in pg_auth_members, so you are still a member in
> > some loose sense even if your grant has INHERIT FALSE and SET FALSE,
> > but in such a case the fact that you are a member isn't really doing
> > anything for you in terms of getting you access to privileges because
> > you're neither allowed to exercise them implicitly nor SET ROLE to the
> > role.
>
> Naming things is hard. :)  I'm still not a fan of calling that option
> 'SET' and 'membership' feels like how it's typically described today
> when someone has the rights of a group (implicitly or explicitly).  As
> for what to call "has a pg_auth_members row but no actual access", maybe
> 'associated'?

What I want here is two Boolean flags, one of which controls whether
you implicitly have the privileges of the granted role and the other
of which controls whether you can access those privileges via SET
ROLE. In each case, I want TRUE to be the state where you have or can
get the privileges and FALSE the state where you can't. I'm not
especially stuck on the names INHERIT and SET for those things,
although in my opinion INHERIT is pretty good and SET is where there
is, perhaps, more likely to be room for improvement. However, I don't
think ASSOCIATED is an improvement because it reverses the sense: now,
TRUE means you don't have the privileges (because you're only
associated, not actually a member, I guess) and FALSE means you do
(because you are not merely associated, but are a member). Yick. That
seems super-unintuitive.

Other alternatives to SET:

- BECOME (can you become that user?)
- ASSUME (can you assume that roles's privileges?)
- EXPLICIT (perhaps also renaming the INHERIT permission to IMPLICIT)

Honestly the main thing I don't like about SET is that it sounds like
it ought to be the action verb in the command, rather than just an
option name. But we've already crossed that Rubicon by deciding that
you can GRANT SET on a GUC, and what's the difference between that and
granting a role WITH SET? I'm actually a bit afraid that if we get
creative here it will sound inconsistent.

> That does lead me down a bit of a rabbit hole because every role in the
> entire system could be considered 'associated' with every other one and
> if the case of "no pg_auth_members row" is identical to the case of
> "pg_auth_members row with everything off/default" then it feels a bit
> odd to have an entry for it- and is there any way to get rid of that
> entry?

"everything off" and "everything default" are two very different
things. If you just do "GRANT foo TO bar" without setting any options,
then you get all the options set to their default values, and that's
going to generate a pg_auth_members entry that we certainly can't
omit. However, let's say that you then alter that grant by removing
every single privilege bit, one by one:

GRANT foo TO bar; -- normal grant
GRANT foo TO bar WITH INHERIT OFF; -- implicit inheritance of privileges removed
GRANT foo TO bar WITH SET OFF; -- explicit SET ROLE privilege now also removed
GRANT foo TO bar WITH JOIE_DE_VIVRE OFF; -- now there's nothing left

One can certainly make an argument that the last GRANT ought to just
go ahead and nuke the pg_auth_members entry entirely, and I'm willing
to do that if that's what most people want. However, I think it might
be better to leave well enough alone. In my proposal, that last GRANT
command is just adjusting the options of the existing grant, not
removing it. True, it is a pretty worthless grant, lacking even joie
de vivre, but it is a grant all the same, and it can still be dumped
and restored just fine. With this change, that last GRANT command
becomes, in effect, a REVOKE, which is a little odd. Right now, the
pg_auth_members row has no "payload data" and perhaps it never will,
but if we were storing, I don't know, the number of times the grant
had ever been used, or when it got created, or whatever, that
information would be lost when that row is deleted, and that seems
like something that the user might want to have happen only when they
explicitly asked for it.

I don't think this is a big deal either way. I've designed systems for
other things both ways in the past, and my general experience has been
that the implicit-removal behavior tends to turn out to be more
annoying than you initially think that it will be, but if people want
that, it should be possible.

> > > In your proposal, does:
> > >
> > > GRANT foo TO bar WITH FRUIT STRAWBERRY;
> > >
> > > mean that 'foo' is grant'd to 'bar' too?
>
> That is, regardless of how we track these things in the catalog or such,
> we have to respect that:
>
> GRANT foo TO bar;
>
> is a SQL-defined thing that says that a 'role authorization descriptor'
> is created.  SET ROLE then checks if a role authorization descriptor
> exists or not matching the current role to the new role and if it does
> then the current role is changed to the new role.  What I was really
> trying to get at above is that:
>
> GRANT foo TO bar WITH $anything-other-than-SET-false;
>
> should probably also create a 'role authorization descriptor' that SET
> ROLE will pick up on.  In other words, the 'SET' thing, or if we call
> that something else, should exist as a distinct column in
> pg_auth_members, but the default value of it should be 'true', with the
> ability for it to be turned to false either at GRANT time or with a
> REVOKE.

I think we're on the same page here. I have a strong desire not to get
caught up in a fruitless argument about SQL specification compliance,
but I believe everything after "in other words" matches my plan.

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

On 02.06.22 18:26, Robert Haas wrote:
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway<mail@joeconway.com>  wrote:
>>> It seems to me that the INHERIT role flag isn't very well-considered.
>>> Inheritance, or the lack of it, ought to be decided separately for
>>> each inherited role. However, that would be a major architectural
>>> change.
>> Agreed -- that would be useful.
> Is this a kind of change people would support?

I think this would create a conflict with what role-based access control 
normally means (outside of PostgreSQL specifically).  A role is a 
collection of privileges that you dynamically enable (e.g., with SET 
ROLE).  That corresponds to the NOINHERIT behavior.  It's also what the 
SQL standard specifies (which in term references other standards for 
RBAC).  The INHERIT behavior basically emulates the groups that used to 
exist in PostgreSQL.

There are also possibly other properties you could derive from that 
distinction.  For example, you could  argue that something like 
pg_hba.conf should have group matching but not (noinherit-)role 
matching, since those roles are not actually activated all the time.

There are also more advanced concepts like roles that are mutually 
exclusive so that you can't activate them both at once.

Now, what PostgreSQL currently implements is a bit of a mess that's a 
somewhat incompatible subset of all that.  But you can basically get 
those standard behaviors somehow.  So I don't think we should break this 
and go into a totally opposite direction.

On Fri, Jun 10, 2022 at 4:36 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I think this would create a conflict with what role-based access control
> normally means (outside of PostgreSQL specifically).  A role is a
> collection of privileges that you dynamically enable (e.g., with SET
> ROLE).  That corresponds to the NOINHERIT behavior.  It's also what the
> SQL standard specifies (which in term references other standards for
> RBAC).  The INHERIT behavior basically emulates the groups that used to
> exist in PostgreSQL.

I don't think this creates any more of a conflict than we've already
got. In fact, I'd go so far as to say it resolves a problem that we
currently have. As far as I can see, we are stuck with a situation
where we have to support both the INHERIT behavior and the NOINHERIT
behavior. Removing either one would be a pretty major compatibility
break. And even if some people were willing to endorse that, it seems
clear from previous discussions that there are people who like the
NOINHERIT behavior and would object to its removal, and other people
(like me!) who like the INHERIT behavior and would object to removing
that. If you think it's feasible to get rid of either of these
behaviors, I'd be interested in hearing your thoughts on that, but to
me it looks like we are stuck with supporting both. From my point of
view, the question is how to make the best of that situation.

Consider a user who in general prefers the NOINHERIT behavior but also
wants to use predefined roles. Perhaps user 'peter' is to be granted
both 'paul' and 'pg_execute_server_programs'. If role 'peter' is set
to INHERIT, Peter will be sad, because his love for NOINHERIT probably
means that he doesn't want to exercise Paul's privileges
automatically. However, he needs to inherit the privileges of
'pg_execute_server_programs' or they are of no use to him. Peter
presumably wants to use COPY TO/FROM program to put data into a table
owned by 'peter', not a table owned by 'pg_execute_server_programs'.
If so, being able to SET ROLE to 'pg_execute_server_programs' is of no
use to him at all, but inheriting the privilege is useful. What is
really needed here is to have the grant of 'paul' be NOINHERIT and the
grant of 'pg_execute_server_programs' be INHERIT, but there's no way
to do that presently. My proposal fixes that problem.

If you don't agree with that analysis, I'd like to know which part of
it seems wrong to you. To me, it seems like an open and shut case: a
grant of a predefined role, or of a group, should have the INHERIT
behavior. A grant of a login role could plausibly have either one,
depending on user preferences. As long as the INHERIT property is a
property of the role to which the permission is granted, you have
trouble as soon as you try to do one grant that should have INHERIT
behavior and another that should have NOINHERIT behavior targeting the
same role.

> There are also possibly other properties you could derive from that
> distinction.  For example, you could  argue that something like
> pg_hba.conf should have group matching but not (noinherit-)role
> matching, since those roles are not actually activated all the time.
>
> There are also more advanced concepts like roles that are mutually
> exclusive so that you can't activate them both at once.

I don't see how my proposal makes any of that any harder than it is
today. Or any easier, either. Seems pretty much orthogonal.

> Now, what PostgreSQL currently implements is a bit of a mess that's a
> somewhat incompatible subset of all that.  But you can basically get
> those standard behaviors somehow.  So I don't think we should break this
> and go into a totally opposite direction.

I don't think I'm proposing to break anything or go in a totally
opposite direction from anything, and to be honest I'm kind of
confused as to why you think that what I'm proposing would have that
effect. As far as I can see, the changes that I'm proposing are
upward-compatible and would permit easy migration to new releases via
either pg_dump or pg_upgrade with no behavior changes at all. Some
syntax would be a bit different on the new releases and that would
unlock some new options we don't currently have, but there's no
behavior that you can get today which you wouldn't be able to get any
more under this proposal.

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

On Mon, Jun 13, 2022 at 2:42 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Agreed.  Moving the inherit flag to the many-to-many join relation provides flexibility, while representing the
presentbehavior is trivial - every row for a given member role has the same value for said flag. 

Precisely.

> One seemingly missing feature is to specify for a role that its privileges cannot be inherited.  In this case
associationswhere it is the group role mustn't be flagged inherit.  Symmetrically, "inherit only" seems like a
plausibleoption for pre-defined group roles. 

Yeah, I was kind of wondering about that, although I hadn't thought so
much of making it mandatory as having some kind of way of setting the
default. One could do either, but I think that can be left for a
future patch that builds on what I am proposing here. No sense trying
to do too many things all at once.

> I agree that granting membership makes the pg_auth_members record appear and revoking membership makes it disappear.

Great.

> I dislike having GRANT do stuff when membership is already established.
>
> ALTER MEMBER role IN group ALTER [SET | ASSUME] [TO | =] [TRUE | FALSE]

I thought about this, too. We could definitely do something like that.
I imagine it would be called ALTER GRANT rather than ALTER MEMBER, but
other than that I don't see an issue. However, there's existing
precedent for the way I proposed it: if you repeat the same GRANT
command but write WITH ADMIN OPTION only the second time, it modifies
the existing grant and adds the admin option to it. If you repeat it
verbatim the second time, it instead gives you a warning that your
command was redundant. That to me establishes the precedent that the
way you modify the options associated with a GRANT is to issue a new
GRANT command. I don't find changing that existing behavior very
appealing, even if we might not pick the same thing if we were doing
it over. We could add something else alongside that to provide another
way of accomplishing the same thing, but that seemed more complicated
for not much benefit.

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

On 13.06.22 20:00, Robert Haas wrote:
> I don't think this creates any more of a conflict than we've already
> got. In fact, I'd go so far as to say it resolves a problem that we
> currently have. As far as I can see, we are stuck with a situation
> where we have to support both the INHERIT behavior and the NOINHERIT
> behavior. Removing either one would be a pretty major compatibility
> break. And even if some people were willing to endorse that, it seems
> clear from previous discussions that there are people who like the
> NOINHERIT behavior and would object to its removal, and other people
> (like me!) who like the INHERIT behavior and would object to removing
> that. If you think it's feasible to get rid of either of these
> behaviors, I'd be interested in hearing your thoughts on that, but to
> me it looks like we are stuck with supporting both. From my point of
> view, the question is how to make the best of that situation.

I think we want to keep both.

> Consider a user who in general prefers the NOINHERIT behavior but also
> wants to use predefined roles. Perhaps user 'peter' is to be granted
> both 'paul' and 'pg_execute_server_programs'. If role 'peter' is set
> to INHERIT, Peter will be sad, because his love for NOINHERIT probably
> means that he doesn't want to exercise Paul's privileges
> automatically. However, he needs to inherit the privileges of
> 'pg_execute_server_programs' or they are of no use to him. Peter
> presumably wants to use COPY TO/FROM program to put data into a table
> owned by 'peter', not a table owned by 'pg_execute_server_programs'.
> If so, being able to SET ROLE to 'pg_execute_server_programs' is of no
> use to him at all, but inheriting the privilege is useful.

That's because our implementation of SET ROLE is bogus.  We should have 
a SET ROLE that is separate from SET SESSION AUTHORIZATION, where the 
current user can keep their current user-ness and additionally enable 
(non-inherited) roles.

> I don't think I'm proposing to break anything or go in a totally
> opposite direction from anything, and to be honest I'm kind of
> confused as to why you think that what I'm proposing would have that
> effect. As far as I can see, the changes that I'm proposing are
> upward-compatible and would permit easy migration to new releases via
> either pg_dump or pg_upgrade with no behavior changes at all. Some
> syntax would be a bit different on the new releases and that would
> unlock some new options we don't currently have, but there's no
> behavior that you can get today which you wouldn't be able to get any
> more under this proposal.

I'm mainly concerned that (AAIU), you propose to remove the current 
INHERIT/NOINHERIT attribute of roles.  I wouldn't like that.  If you 
want a feature that allows overriding that per-grant, maybe that's okay.

On Wed, Jun 15, 2022 at 5:23 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> > Consider a user who in general prefers the NOINHERIT behavior but also
> > wants to use predefined roles. Perhaps user 'peter' is to be granted
> > both 'paul' and 'pg_execute_server_programs'. If role 'peter' is set
> > to INHERIT, Peter will be sad, because his love for NOINHERIT probably
> > means that he doesn't want to exercise Paul's privileges
> > automatically. However, he needs to inherit the privileges of
> > 'pg_execute_server_programs' or they are of no use to him. Peter
> > presumably wants to use COPY TO/FROM program to put data into a table
> > owned by 'peter', not a table owned by 'pg_execute_server_programs'.
> > If so, being able to SET ROLE to 'pg_execute_server_programs' is of no
> > use to him at all, but inheriting the privilege is useful.
>
> That's because our implementation of SET ROLE is bogus.  We should have
> a SET ROLE that is separate from SET SESSION AUTHORIZATION, where the
> current user can keep their current user-ness and additionally enable
> (non-inherited) roles.

It would help me to have a better description of what you think the
behavior ought to be. I've always thought there was something funny
about SET ROLE and SET SESSION AUTHORIZATION, because it seems like
they are too similar to each other. But it would surprise me if SET
ROLE added additional privileges to my session while leaving the old
ones intact, too, much as I'd be surprised if SET work_mem = '8MB'
followed by SET work_mem = '1GB' somehow left both values partly in
effect at the same time. It feels to me like SET is describing an
action that changes the session state, rather than adding to it.

> I'm mainly concerned that (AAIU), you propose to remove the current
> INHERIT/NOINHERIT attribute of roles.  I wouldn't like that.  If you
> want a feature that allows overriding that per-grant, maybe that's okay.

Yeah, I want to remove it and replace it with something more
fine-grained. I don't yet understand why that's a problem for anything
you want to do.

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

On Thu, Jun 2, 2022 at 1:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Point 2 would cause every existing pg_dumpall script to fail, which
> seems like kind of a large gotcha.  Less unpleasant alternatives
> could include
>
> * Continue to accept the syntax, but ignore it, maybe with a WARNING
> for the alternative that doesn't correspond to the new behavior.
>
> * Keep pg_authid.rolinherit, and have it act as supplying the default
> behavior for subsequent GRANTs to that role.

Here's a rather small patch that does it the first way.

I know that Peter Eisentraut didn't like the idea of removing the
role-level option, but I don't know why, so I don't know whether doing
this the second way instead would address his objection.

I suppose if we did it the second way, we could make the syntax GRANT
granted_role TO recipient_role WITH INHERIT { TRUE | FALSE | DEFAULT
}, and DEFAULT would copy the current value of the rolinherit
property, so that changing the rolinherit property later would not
affect previous grants. The reverse is also possible: with the same
syntax, the rolinherit column could be changed from bool to "char",
storing t/f/d, and 'd' could mean the value of the rolinherit property
at time of use; thus, changing rolinherit would affect previous grants
performed using WITH INHERIT DEFAULT but not those that specified WITH
INHERIT TRUE or WITH INHERIT FALSE.

In some sense, this whole scheme seems backwards to me: wouldn't it be
more natural if the default were based on the role being granted
rather than the role to which it is granted? If I decide to GRANT
some_predefined_role TO some_user, it seems like I am really likely to
want the INHERIT behavior, whereas if I GRANT some_other_user TO
some_user, it could go either way depending on the use case. But this
may be water under the bridge: introducing a way to set the default
that is backwards from the way that the current role-level property
works may be too confusing to be worthy of any consideration at all.

Thoughts?

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

On Wed, Jun 22, 2022 at 04:30:36PM -0400, Robert Haas wrote:
> On Thu, Jun 2, 2022 at 1:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Point 2 would cause every existing pg_dumpall script to fail, which
>> seems like kind of a large gotcha.  Less unpleasant alternatives
>> could include
>>
>> * Continue to accept the syntax, but ignore it, maybe with a WARNING
>> for the alternative that doesn't correspond to the new behavior.
>>
>> * Keep pg_authid.rolinherit, and have it act as supplying the default
>> behavior for subsequent GRANTs to that role.
> 
> Here's a rather small patch that does it the first way.

I've been thinking about whether we ought to WARNING or ERROR with the
deprecated syntax.  WARNING has the advantage of not breaking existing
scripts, but users might not catch that NOINHERIT roles will effectively
become INHERIT roles, which IMO is just a different type of breakage.
However, I don't think I can defend ERROR-ing and breaking all existing
pg_dumpall scripts completely, so perhaps the best we can do is emit a
WARNING until we feel comfortable removing the option completely 5-10 years
from now.

I'm guessing we'll also need a new pg_dumpall option for generating pre-v16
style role commands versus the v16+ style ones.  When run on v16 and later,
you'd have to require the latter option, as you won't always be able to
convert grant-level inheritance options into role-level options.  However,
you can always do the reverse.  I'm thinking that by default, pg_dumpall
would output the style of commands for the current server version.
pg_upgrade would make use of this option when upgrading from <v16 to v16
and above.  Is this close to what you are thinking?

> I suppose if we did it the second way, we could make the syntax GRANT
> granted_role TO recipient_role WITH INHERIT { TRUE | FALSE | DEFAULT
> }, and DEFAULT would copy the current value of the rolinherit
> property, so that changing the rolinherit property later would not
> affect previous grants. The reverse is also possible: with the same
> syntax, the rolinherit column could be changed from bool to "char",
> storing t/f/d, and 'd' could mean the value of the rolinherit property
> at time of use; thus, changing rolinherit would affect previous grants
> performed using WITH INHERIT DEFAULT but not those that specified WITH
> INHERIT TRUE or WITH INHERIT FALSE.

Yeah, something like this might be a nice way to sidestep the issue.  I was
imagining something more like your second option, but instead of continuing
to allow grant-level options to take effect when rolinherit was true or
false, I was thinking we would ignore them or even disallow them.  By
disallowing grant-level options when a role-level option was set, we might
be able to avoid the confusion about what takes effect when.  That being
said, the syntax for this sort of thing might not be the cleanest.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jun 29, 2022 at 7:19 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > Here's a rather small patch that does it the first way.
>
> I've been thinking about whether we ought to WARNING or ERROR with the
> deprecated syntax.  WARNING has the advantage of not breaking existing
> scripts, but users might not catch that NOINHERIT roles will effectively
> become INHERIT roles, which IMO is just a different type of breakage.
> However, I don't think I can defend ERROR-ing and breaking all existing
> pg_dumpall scripts completely, so perhaps the best we can do is emit a
> WARNING until we feel comfortable removing the option completely 5-10 years
> from now.

Yeah, I think if we're going to ERROR we should just remove the syntax
support entirely. A syntax error by any other name is still a syntax
error. If we're going to go with a WARNING there's some value in that
to allow existing dumps to be sorta-restored on new versions, but this
is also moot if we don't end up deprecating this syntax after all.

> I'm guessing we'll also need a new pg_dumpall option for generating pre-v16
> style role commands versus the v16+ style ones.  When run on v16 and later,
> you'd have to require the latter option, as you won't always be able to
> convert grant-level inheritance options into role-level options.  However,
> you can always do the reverse.  I'm thinking that by default, pg_dumpall
> would output the style of commands for the current server version.
> pg_upgrade would make use of this option when upgrading from <v16 to v16
> and above.  Is this close to what you are thinking?

I don't see why we need an option. pg_dump's charter is to produce
output suitable for the server version that matches the pg_dump
version. Existing pg_dump versions will do the right thing for the
server versions they support, and in the v16, we can just straight up
change the behavior to produce the syntax that v16 wants.

> > I suppose if we did it the second way, we could make the syntax GRANT
> > granted_role TO recipient_role WITH INHERIT { TRUE | FALSE | DEFAULT
> > }, and DEFAULT would copy the current value of the rolinherit
> > property, so that changing the rolinherit property later would not
> > affect previous grants. The reverse is also possible: with the same
> > syntax, the rolinherit column could be changed from bool to "char",
> > storing t/f/d, and 'd' could mean the value of the rolinherit property
> > at time of use; thus, changing rolinherit would affect previous grants
> > performed using WITH INHERIT DEFAULT but not those that specified WITH
> > INHERIT TRUE or WITH INHERIT FALSE.
>
> Yeah, something like this might be a nice way to sidestep the issue.  I was
> imagining something more like your second option, but instead of continuing
> to allow grant-level options to take effect when rolinherit was true or
> false, I was thinking we would ignore them or even disallow them.  By
> disallowing grant-level options when a role-level option was set, we might
> be able to avoid the confusion about what takes effect when.  That being
> said, the syntax for this sort of thing might not be the cleanest.

I don't think that it's a good idea to disallow grant-level options
when a role-level option is set, for two reasons. First, it would
necessitate restricting the ability to ALTER the role-level option;
otherwise no invariant could be maintained. Second, I'm interested in
this feature because I want to be able to perform a role grant that
will have a well-defined behavior without knowing what role-level
options are set. I thought initially that I wouldn't be able to
accomplish my goals if we kept the role-level options around, but now
I think that's not true. However, I think I need the grant-level
option to work regardless of how the role-level option is set. The
problem with the role-level option is essentially that you can't
reason about what a new grant will do.

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

On Thu, Jun 30, 2022 at 09:42:11AM -0400, Robert Haas wrote:
> On Wed, Jun 29, 2022 at 7:19 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I'm guessing we'll also need a new pg_dumpall option for generating pre-v16
>> style role commands versus the v16+ style ones.  When run on v16 and later,
>> you'd have to require the latter option, as you won't always be able to
>> convert grant-level inheritance options into role-level options.  However,
>> you can always do the reverse.  I'm thinking that by default, pg_dumpall
>> would output the style of commands for the current server version.
>> pg_upgrade would make use of this option when upgrading from <v16 to v16
>> and above.  Is this close to what you are thinking?
> 
> I don't see why we need an option. pg_dump's charter is to produce
> output suitable for the server version that matches the pg_dump
> version. Existing pg_dump versions will do the right thing for the
> server versions they support, and in the v16, we can just straight up
> change the behavior to produce the syntax that v16 wants.

Got it.  Makes sense.

>> > I suppose if we did it the second way, we could make the syntax GRANT
>> > granted_role TO recipient_role WITH INHERIT { TRUE | FALSE | DEFAULT
>> > }, and DEFAULT would copy the current value of the rolinherit
>> > property, so that changing the rolinherit property later would not
>> > affect previous grants. The reverse is also possible: with the same
>> > syntax, the rolinherit column could be changed from bool to "char",
>> > storing t/f/d, and 'd' could mean the value of the rolinherit property
>> > at time of use; thus, changing rolinherit would affect previous grants
>> > performed using WITH INHERIT DEFAULT but not those that specified WITH
>> > INHERIT TRUE or WITH INHERIT FALSE.
>>
>> Yeah, something like this might be a nice way to sidestep the issue.  I was
>> imagining something more like your second option, but instead of continuing
>> to allow grant-level options to take effect when rolinherit was true or
>> false, I was thinking we would ignore them or even disallow them.  By
>> disallowing grant-level options when a role-level option was set, we might
>> be able to avoid the confusion about what takes effect when.  That being
>> said, the syntax for this sort of thing might not be the cleanest.
> 
> I don't think that it's a good idea to disallow grant-level options
> when a role-level option is set, for two reasons. First, it would
> necessitate restricting the ability to ALTER the role-level option;
> otherwise no invariant could be maintained. Second, I'm interested in
> this feature because I want to be able to perform a role grant that
> will have a well-defined behavior without knowing what role-level
> options are set. I thought initially that I wouldn't be able to
> accomplish my goals if we kept the role-level options around, but now
> I think that's not true. However, I think I need the grant-level
> option to work regardless of how the role-level option is set. The
> problem with the role-level option is essentially that you can't
> reason about what a new grant will do.

IIUC you are suggesting that we'd leave rolinherit in pg_authid alone, but
we'd add the ability to specify a grant-level option that would always take
precedence.  The default (WITH INHERIT DEFAULT) would cause things to work
exactly as they do today (i.e., use rolinherit).  Does this sound right?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jun 30, 2022 at 7:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> IIUC you are suggesting that we'd leave rolinherit in pg_authid alone, but
> we'd add the ability to specify a grant-level option that would always take
> precedence.  The default (WITH INHERIT DEFAULT) would cause things to work
> exactly as they do today (i.e., use rolinherit).  Does this sound right?

Yeah, that could be an alternative to the patch I proposed previously.
What do you (and others) think of that idea?

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

On Thu, Jun 30, 2022 at 10:21:53PM -0400, Robert Haas wrote:
> On Thu, Jun 30, 2022 at 7:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> IIUC you are suggesting that we'd leave rolinherit in pg_authid alone, but
>> we'd add the ability to specify a grant-level option that would always take
>> precedence.  The default (WITH INHERIT DEFAULT) would cause things to work
>> exactly as they do today (i.e., use rolinherit).  Does this sound right?
> 
> Yeah, that could be an alternative to the patch I proposed previously.
> What do you (and others) think of that idea?

I like it.  If rolinherit is left in place, existing pg_dumpall scripts
will continue to work, and folks can continue to use the role-level option
exactly as they do today.  However, we'd be adding the ability to use a
grant-level option if desired, and it would be relatively easy to reason
about (i.e., the grant-level option always takes precedence over the
role-level option).  Also, AFAICT this strategy still provides the full set
of behavior that would be possible if only the grant-level option existed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 6/30/22 22:58, Nathan Bossart wrote:
> On Thu, Jun 30, 2022 at 10:21:53PM -0400, Robert Haas wrote:
>> On Thu, Jun 30, 2022 at 7:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>>> IIUC you are suggesting that we'd leave rolinherit in pg_authid alone, but
>>> we'd add the ability to specify a grant-level option that would always take
>>> precedence.  The default (WITH INHERIT DEFAULT) would cause things to work
>>> exactly as they do today (i.e., use rolinherit).  Does this sound right?
>> 
>> Yeah, that could be an alternative to the patch I proposed previously.
>> What do you (and others) think of that idea?
> 
> I like it.  If rolinherit is left in place, existing pg_dumpall scripts
> will continue to work, and folks can continue to use the role-level option
> exactly as they do today.  However, we'd be adding the ability to use a
> grant-level option if desired, and it would be relatively easy to reason
> about (i.e., the grant-level option always takes precedence over the
> role-level option).  Also, AFAICT this strategy still provides the full set
> of behavior that would be possible if only the grant-level option existed.

Would this allow for an explicit REVOKE to override a default INHERIT 
along a specific path?

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

On Fri, Jul 1, 2022 at 6:17 AM Joe Conway <mail@joeconway.com> wrote:
> Would this allow for an explicit REVOKE to override a default INHERIT
> along a specific path?

Can you give an example?

If you mean that A is granted to B which is granted to C which is
granted to D and you now want NOINHERIT behavior for the B->C link in
the chain, this would allow that. You could modify the existing grant
by saying either "REVOKE INHERIT OPTION FOR B FROM C" or "GRANT B TO C
WITH INHERIT FALSE".

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

On 7/1/22 07:48, Robert Haas wrote:
> On Fri, Jul 1, 2022 at 6:17 AM Joe Conway <mail@joeconway.com> wrote:
>> Would this allow for an explicit REVOKE to override a default INHERIT
>> along a specific path?
> 
> Can you give an example?
> 
> If you mean that A is granted to B which is granted to C which is
> granted to D and you now want NOINHERIT behavior for the B->C link in
> the chain, this would allow that. You could modify the existing grant
> by saying either "REVOKE INHERIT OPTION FOR B FROM C" or "GRANT B TO C
> WITH INHERIT FALSE".

Hmm, maybe I am misunderstanding something, but what I mean is something 
like:

8<----------------
CREATE TABLE t1(f1 int);
CREATE TABLE t2(f1 int);

CREATE USER A; --defaults to INHERIT
CREATE USER B;
CREATE USER C;

GRANT select ON TABLE t1 TO B;
GRANT select ON TABLE t2 TO C;

GRANT B TO A;
GRANT C TO A;

SET SESSION AUTHORIZATION A;

-- works
SELECT * FROM t1;
-- works
SELECT * FROM t2;

RESET SESSION AUTHORIZATION;
REVOKE INHERIT OPTION FOR C FROM A;
SET SESSION AUTHORIZATION A;

-- works
SELECT * FROM t1;
-- fails
SELECT * FROM t2;
8<----------------

So now A has implicit inherited privs for t1 but not for t2.

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

On Fri, Jul 1, 2022 at 8:22 AM Joe Conway <mail@joeconway.com> wrote:
> Hmm, maybe I am misunderstanding something, but what I mean is something
> like:
>
> 8<----------------
> CREATE TABLE t1(f1 int);
> CREATE TABLE t2(f1 int);
>
> CREATE USER A; --defaults to INHERIT
> CREATE USER B;
> CREATE USER C;
>
> GRANT select ON TABLE t1 TO B;
> GRANT select ON TABLE t2 TO C;
>
> GRANT B TO A;
> GRANT C TO A;
>
> SET SESSION AUTHORIZATION A;
>
> -- works
> SELECT * FROM t1;
> -- works
> SELECT * FROM t2;
>
> RESET SESSION AUTHORIZATION;
> REVOKE INHERIT OPTION FOR C FROM A;
> SET SESSION AUTHORIZATION A;
>
> -- works
> SELECT * FROM t1;
> -- fails
> SELECT * FROM t2;
> 8<----------------
>
> So now A has implicit inherited privs for t1 but not for t2.

Yeah, I anticipate that this would work in the way that you postulate here.

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

On Fri, Jul 1, 2022 at 9:05 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > So now A has implicit inherited privs for t1 but not for t2.
>
> Yeah, I anticipate that this would work in the way that you postulate here.

And here is a patch that makes it work that way. In this version, you
can GRANT foo TO bar WITH INHERIT { TRUE | FALSE | DEFAULT }, and
DEFAULT means that role-level [NO]INHERIT property is controlling.
Otherwise, the grant-level option overrides the role-level option.

I have some hesitations about this approach. On the positive side, I
believe it's fully backward compatible, and that's something to like.
On the negative side, I've always felt that the role-level properties
- [NO]INHERIT, [NO]CREATEROLE, [NO]SUPERUSER, [NO]CREATEDB were hacky
kludges, and I'd be happier they all went away in favor of more
principled ways of controlling those behaviors. I think that the
previous design moves us in the direction of being able to eventually
remove [NO]INHERIT, whereas this, if anything, entrenches it.

It might even encourage people to add *more* role-level Boolean flags.
For instance, say we add another grant-level property that controls
whether or not you can SET ROLE to the granted role. Well, it somebody
going to then say that, for symmetry with CREATE ROLE .. [NO]INHERIT
we need CREATE ROLE .. [NO]SETROLE? I think such additions would be
actively harmful, not only because they add complexity, but also
because I think they are fundamentally the wrong way to think about
these kinds of properties.

To me, setting a default for whether or not role grants have INHERIT
default by default is a bit like setting a default for the size of
your future credit card transactions. "If I use my credit card and
don't say how much I want to charge, make it $6.82!" This is obviously
nonsense, at least in normal scenarios, because the amount of a credit
card transaction depends fundamentally on the purpose of the credit
card transaction, and you cannot know what the right amount to charge
in future transactions will be unless you already know the purpose of
those transactions. So here: we probably cannot say anything about the
purpose of a future GRANT based on the identity of the role that is
receiving the privileges. You could have a special purpose role that
receives many grants but always with the same general purpose, just as
you could have a credit card holder that only ever buys things that
cost $6.82, and either case, a default could I guess be useful. But
those seem to be corner cases, and even then the benefit of being able
to set the default seems to be modest.

Thoughts?

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

On Fri, Jul 01, 2022 at 02:59:53PM -0400, Robert Haas wrote:
> And here is a patch that makes it work that way. In this version, you
> can GRANT foo TO bar WITH INHERIT { TRUE | FALSE | DEFAULT }, and
> DEFAULT means that role-level [NO]INHERIT property is controlling.
> Otherwise, the grant-level option overrides the role-level option.

I haven't spent a ton of time reviewing the patch yet, but I did read
through it and thought it looked pretty good.

> I have some hesitations about this approach. On the positive side, I
> believe it's fully backward compatible, and that's something to like.
> On the negative side, I've always felt that the role-level properties
> - [NO]INHERIT, [NO]CREATEROLE, [NO]SUPERUSER, [NO]CREATEDB were hacky
> kludges, and I'd be happier they all went away in favor of more
> principled ways of controlling those behaviors. I think that the
> previous design moves us in the direction of being able to eventually
> remove [NO]INHERIT, whereas this, if anything, entrenches it.

+1.  As mentioned upthread [0], I think attributes like CREATEROLE,
SUPERUSER, and CREATEDB could be replaced with predefined roles.  However,
since role attributes aren't inherited, that would result in a behavior
change.  I'm curious what you have in mind.  It might be worth spinning off
a new thread for this sooner than later.

> It might even encourage people to add *more* role-level Boolean flags.
> For instance, say we add another grant-level property that controls
> whether or not you can SET ROLE to the granted role. Well, it somebody
> going to then say that, for symmetry with CREATE ROLE .. [NO]INHERIT
> we need CREATE ROLE .. [NO]SETROLE? I think such additions would be
> actively harmful, not only because they add complexity, but also
> because I think they are fundamentally the wrong way to think about
> these kinds of properties.

Perhaps there should just be a policy against adding new role-level
options.  Instead, new functionality should be provided via predefined
roles or grant-level options.  There is some precedent for this.  For
example, VACUUM has several options that are only usable via the
parenthesized syntax, and the unparenthesized syntax is marked deprecated
in the documentation.  (Speaking of which, is it finally time to remove the
unparenthesized syntax or add WARNINGs when it is used?)  For [NO]INHERIT
and WITH INHERIT DEFAULT, presumably we could do something similar.  Down
the road, those would be removed in favor of only using grant-level
options.

[0] https://postgr.es/m/20220602180755.GA2402942%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jul 1, 2022 at 5:12 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > I have some hesitations about this approach. On the positive side, I
> > believe it's fully backward compatible, and that's something to like.
> > On the negative side, I've always felt that the role-level properties
> > - [NO]INHERIT, [NO]CREATEROLE, [NO]SUPERUSER, [NO]CREATEDB were hacky
> > kludges, and I'd be happier they all went away in favor of more
> > principled ways of controlling those behaviors. I think that the
> > previous design moves us in the direction of being able to eventually
> > remove [NO]INHERIT, whereas this, if anything, entrenches it.
>
> +1.  As mentioned upthread [0], I think attributes like CREATEROLE,
> SUPERUSER, and CREATEDB could be replaced with predefined roles.  However,
> since role attributes aren't inherited, that would result in a behavior
> change.  I'm curious what you have in mind.  It might be worth spinning off
> a new thread for this sooner than later.

I don't think there is a whole lot of point in replacing role-level
flags with predefined roles that work exactly the same way. Maybe
there is some point, but I'm not excited about it. The problem with
these settings in my opinion is that they are too monolithic. Either
you can create any role with basically any privileges or you can
create no roles at all. Either you are a superuser and can bypass all
permissions checks or you are not and can't bypass any permissions
checks.

> unparenthesized syntax or add WARNINGs when it is used?)  For [NO]INHERIT
> and WITH INHERIT DEFAULT, presumably we could do something similar.  Down
> the road, those would be removed in favor of only using grant-level
> options.

I think it'd be hard to do that if WITH INHERIT DEFAULT is actually
state stored in the catalog. Maybe I should revise this again so that
the catalog column is just true or false, and the role-level property
only sets the default for future grants. That might be more like what
Tom had in mind originally.

More clear sketch: Remove WITH INHERIT DEFAULT and just have WITH
INHERIT { TRUE | FALSE }. If neither is specified, the default for a
new GRANT is set based on the role-level property. I want more control
than that.

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

On Sat, Jul 02, 2022 at 08:45:50AM -0400, Robert Haas wrote:
> I don't think there is a whole lot of point in replacing role-level
> flags with predefined roles that work exactly the same way. Maybe
> there is some point, but I'm not excited about it. The problem with
> these settings in my opinion is that they are too monolithic. Either
> you can create any role with basically any privileges or you can
> create no roles at all. Either you are a superuser and can bypass all
> permissions checks or you are not and can't bypass any permissions
> checks.

Okay.  I see.

>> unparenthesized syntax or add WARNINGs when it is used?)  For [NO]INHERIT
>> and WITH INHERIT DEFAULT, presumably we could do something similar.  Down
>> the road, those would be removed in favor of only using grant-level
>> options.
> 
> I think it'd be hard to do that if WITH INHERIT DEFAULT is actually
> state stored in the catalog. Maybe I should revise this again so that
> the catalog column is just true or false, and the role-level property
> only sets the default for future grants. That might be more like what
> Tom had in mind originally.

I was thinking that when DEFAULT was removed, pg_dump would just need to
generate WITH INHERIT TRUE/FALSE based on the value of rolinherit for older
versions.  Using the role-level property as the default for future grants
seems a viable strategy, although it would break backward compatibility.
For example, if I create a NOINHERIT role, grant a bunch of roles to it,
and then change it to INHERIT, the role won't begin inheriting the
privileges of the roles it is a member of.  Right now, it does.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sat, Jul 2, 2022 at 6:16 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I was thinking that when DEFAULT was removed, pg_dump would just need to
> generate WITH INHERIT TRUE/FALSE based on the value of rolinherit for older
> versions.  Using the role-level property as the default for future grants
> seems a viable strategy, although it would break backward compatibility.
> For example, if I create a NOINHERIT role, grant a bunch of roles to it,
> and then change it to INHERIT, the role won't begin inheriting the
> privileges of the roles it is a member of.  Right now, it does.

I think the idea you propose here is interesting, because I think it
proves that committing v2 or something like it doesn't really lock us
into the role-level property any more than we already are, which at
least makes me feel slightly less bad about that option. However, if
there's implacable opposition to any compatibility break at any point,
then maybe this plan would never actually be implemented in practice.
And if there's not, maybe we can be bolder now.

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

On Sat, Jul 02, 2022 at 11:04:28PM -0400, Robert Haas wrote:
> On Sat, Jul 2, 2022 at 6:16 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I was thinking that when DEFAULT was removed, pg_dump would just need to
>> generate WITH INHERIT TRUE/FALSE based on the value of rolinherit for older
>> versions.  Using the role-level property as the default for future grants
>> seems a viable strategy, although it would break backward compatibility.
>> For example, if I create a NOINHERIT role, grant a bunch of roles to it,
>> and then change it to INHERIT, the role won't begin inheriting the
>> privileges of the roles it is a member of.  Right now, it does.
> 
> I think the idea you propose here is interesting, because I think it
> proves that committing v2 or something like it doesn't really lock us
> into the role-level property any more than we already are, which at
> least makes me feel slightly less bad about that option. However, if
> there's implacable opposition to any compatibility break at any point,
> then maybe this plan would never actually be implemented in practice.
> And if there's not, maybe we can be bolder now.

If by "bolder" you mean "mark [NO]INHERIT as deprecated-and-to-be-removed
and begin emitting WARNINGs when it and WITH INHERIT DEFAULT are used," I
think it's worth consideration.  I suspect it will be hard to sell removing
[NO]INHERIT in v16 because it would introduce a compatibility break without
giving users much time to migrate.  I could be wrong, though.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sun, Jul 3, 2022 at 1:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> If by "bolder" you mean "mark [NO]INHERIT as deprecated-and-to-be-removed
> and begin emitting WARNINGs when it and WITH INHERIT DEFAULT are used," I
> think it's worth consideration.  I suspect it will be hard to sell removing
> [NO]INHERIT in v16 because it would introduce a compatibility break without
> giving users much time to migrate.  I could be wrong, though.

It's a fair point. But, if our goal for v16 is to do something that
could lead to an eventual deprecation of [NO]INHERIT, I still think
removing WITH INHERIT DEFAULT from the patch set is probably a good
idea. Perhaps then we could document that we recommend using the
grant-level option rather than setting NOINHERIT on the role, and if
users were to follow that advice, eventually no one would be relying
on the role-level property anymore. It might be optimistic to assume
that users will read the documentation, let alone follow it, but it's
something. Now, that does mean accepting a compatibility break now, in
that flipping the role-level setting would no longer affect existing
grants, but it's less of a compatibility break than I proposed
originally, so maybe it's acceptable.

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

On Tue, Jul 5, 2022 at 8:04 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Jul 3, 2022 at 1:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > If by "bolder" you mean "mark [NO]INHERIT as deprecated-and-to-be-removed
> > and begin emitting WARNINGs when it and WITH INHERIT DEFAULT are used," I
> > think it's worth consideration.  I suspect it will be hard to sell removing
> > [NO]INHERIT in v16 because it would introduce a compatibility break without
> > giving users much time to migrate.  I could be wrong, though.
>
> It's a fair point. But, if our goal for v16 is to do something that
> could lead to an eventual deprecation of [NO]INHERIT, I still think
> removing WITH INHERIT DEFAULT from the patch set is probably a good
> idea.

So here is an updated patch with that change.

For those who may not have read the entire thread, the current patch
does not actually remove the role-level option as the subject line
suggests, but rather makes it set the default for future grants as
suggested by Tom in
http://postgr.es/m/1066202.1654190251@sss.pgh.pa.us

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

On Fri, Jul 08, 2022 at 03:56:56PM -0400, Robert Haas wrote:
> For those who may not have read the entire thread, the current patch
> does not actually remove the role-level option as the subject line
> suggests, but rather makes it set the default for future grants as
> suggested by Tom in
> http://postgr.es/m/1066202.1654190251@sss.pgh.pa.us

I think this is an interesting approach, as it seems to move things closer
to the end goal (i.e., removing [NO]INHERIT), but it also introduces a
pretty significant compatibility break.  With this change, you cannot keep
using [NO]INHERIT like you do today.  You will also need to individually
update each GRANT.  The advantage of using [NO]INHERIT as the fallback
value in the absence of a grant-level option is that users don't need to
adjust behavior right away.  They can essentially ignore the new
grant-level options for now, although presumably they'd need to adjust at
some point.

IMO we should either retain compatibility for existing scripts for now, or
we should remove [NO]INHERIT completely in v16.  I'm worried that keeping
[NO]INHERIT around while changing its behavior somewhat subtly is only
going to create confusion.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jul 8, 2022 at 5:02 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I think this is an interesting approach, as it seems to move things closer
> to the end goal (i.e., removing [NO]INHERIT), but it also introduces a
> pretty significant compatibility break.  With this change, you cannot keep
> using [NO]INHERIT like you do today.  You will also need to individually
> update each GRANT.

True. But it may not be very common for people to ALTER ROLE
[NO]INHERIT after having previously used GRANT, so I'm not sure that
it would be a big problem in practice.

> The advantage of using [NO]INHERIT as the fallback
> value in the absence of a grant-level option is that users don't need to
> adjust behavior right away.  They can essentially ignore the new
> grant-level options for now, although presumably they'd need to adjust at
> some point.

Also true.

> IMO we should either retain compatibility for existing scripts for now, or
> we should remove [NO]INHERIT completely in v16.  I'm worried that keeping
> [NO]INHERIT around while changing its behavior somewhat subtly is only
> going to create confusion.

Could be. It's just a little hard to know what to do here, because
we've gotten opinions from only a few people, and they're all
different. I've now produced patches for what I believe to be all
three of the viable alternatives, and none of them have met with
universal acclaim:

v1: hard compatibility break, NOINHERIT no-op w/WARNING
v2: WITH INHERIT { TRUE | FALSE | DEFAULT }, DEFAULT => use rolinherit
that is current at time of use
v3: WITH INHERIT { TRUE | FALSE }, if unspecified => set grant-level
Boolean to rolinherit that is current at time of GRANT

Nobody seems desperately opposed to the basic idea of adding a
grant-level option, so probably it's OK to proceed with one of these.
Which one, though, is a bit of a puzzler.

Anyone else want to offer an opinion?

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

On Mon, Jul 11, 2022 at 12:48 PM tushar <tushar.ahuja@enterprisedb.com> wrote:
> One scenario where the syntax is created in pg_dumpall is wrong
>
> postgres=# create user u1;
> postgres=# create group g1 with user u1;
> postgres=# grant g1 to u1 with admin option, inherit false;
>
> Perform pg_dumpall
>
> GRANT g1 TO u1 WITH ADMIN OPTION WITH INHERIT FALSE GRANTED BY edb;

Oops. Here is a rebased version of v3 which aims to fix this bug.

It seems that I can replace the previous changes to src/backend/nodes
with nothing at all in view of Peter's commit to automatically
generate node support functions. Nice.

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

On 7/11/22 11:01 PM, Robert Haas wrote:
> Oops. Here is a rebased version of v3 which aims to fix this bug.
Thanks, Issue seems to be fixed with this patch.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 7/11/22 11:01 PM, Robert Haas wrote:
> Oops. Here is a rebased version of v3 which aims to fix this bug.
I found one issue where pg_upgrade is failing

PG v14.4 , create these below objects

create user u1 with superuser;
create user u3;
create group g2 with user  u1;

now try to perform pg_upgrade from v16(w/patch), it is failing with 
these messages

[edb@centos7tushar bin]$ tail -10 
dc2/pg_upgrade_output.d/20220714T195919.494/log/pg_upgrade_utility.log
--
--
CREATE ROLE "u3";
CREATE ROLE
ALTER ROLE "u3" WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN 
NOREPLICATION NOBYPASSRLS;
ALTER ROLE
GRANT "g2" TO "u1" WITH  GRANTED BY "edb";
psql:dc2/pg_upgrade_output.d/20220714T195919.494/dump/pg_upgrade_dump_globals.sql:47: 
ERROR:  syntax error at or near "BY"
LINE 1: GRANT "g2" TO "u1" WITH  GRANTED BY "edb";
                                          ^
This issue is not reproducible on PG v16 (without patch).

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Jul 14, 2022 at 10:53 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> GRANT "g2" TO "u1" WITH  GRANTED BY "edb";

Another good catch. Here is v5 with a fix for that problem.

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

On 7/19/22 12:56 AM, Robert Haas wrote:
> Another good catch. Here is v5 with a fix for that problem.
Thanks, the issue is fixed now.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 7/19/22 12:56 AM, Robert Haas wrote:
> Another good catch. Here is v5 with a fix for that problem.
Here is one scenario in which I have NOT granted (inherit false) 
explicitly but still revoke
command is changing the current state

postgres=# create group foo;
CREATE ROLE
postgres=# create user bar in group foo;
CREATE ROLE
postgres=# revoke inherit option for foo from bar;
REVOKE ROLE

[edb@centos7tushar bin]$ ./pg_dumpall > /tmp/a11

[edb@centos7tushar bin]$ cat /tmp/a11 |grep 'inherit false' -i
GRANT foo TO bar WITH INHERIT FALSE GRANTED BY edb;

I think this revoke command should be ignored and inherit option should 
remain 'TRUE'
as it was before?

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Jul 28, 2022 at 10:16 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> On 7/19/22 12:56 AM, Robert Haas wrote:
> > Another good catch. Here is v5 with a fix for that problem.
> Here is one scenario in which I have NOT granted (inherit false)
> explicitly but still revoke
> command is changing the current state
>
> postgres=# create group foo;
> CREATE ROLE
> postgres=# create user bar in group foo;
> CREATE ROLE
> postgres=# revoke inherit option for foo from bar;
> REVOKE ROLE
>
> [edb@centos7tushar bin]$ ./pg_dumpall > /tmp/a11
>
> [edb@centos7tushar bin]$ cat /tmp/a11 |grep 'inherit false' -i
> GRANT foo TO bar WITH INHERIT FALSE GRANTED BY edb;
>
> I think this revoke command should be ignored and inherit option should
> remain 'TRUE'
> as it was before?

No, it seems to me that's behaving as intended. REVOKE BLAH OPTION ...
is intended to be a way of switching an option off.

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

On 7/28/22 8:03 PM, Robert Haas wrote:
> No, it seems to me that's behaving as intended. REVOKE BLAH OPTION ...
> is intended to be a way of switching an option off.
Ok, Thanks, Robert. I tested with a couple of more scenarios like 
pg_upgrade/pg_dumpall /grant/revoke .. with admin option/inherit
and things look good to me.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Jul 28, 2022 at 12:32 PM tushar <tushar.ahuja@enterprisedb.com> wrote:
> On 7/28/22 8:03 PM, Robert Haas wrote:
> > No, it seems to me that's behaving as intended. REVOKE BLAH OPTION ...
> > is intended to be a way of switching an option off.
> Ok, Thanks, Robert. I tested with a couple of more scenarios like
> pg_upgrade/pg_dumpall /grant/revoke .. with admin option/inherit
> and things look good to me.

This patch needed to be rebased pretty extensively after commit
ce6b672e4455820a0348214be0da1a024c3f619f. Here is a new version.

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

On 8/24/22 12:28 AM, Robert Haas wrote:
> This patch needed to be rebased pretty extensively after commit
> ce6b672e4455820a0348214be0da1a024c3f619f. Here is a new version.
Thanks, Robert, I have retested this patch with my previous scenarios
and things look good to me.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Wed, Aug 24, 2022 at 10:23 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> On 8/24/22 12:28 AM, Robert Haas wrote:
> > This patch needed to be rebased pretty extensively after commit
> > ce6b672e4455820a0348214be0da1a024c3f619f. Here is a new version.
> Thanks, Robert, I have retested this patch with my previous scenarios
> and things look good to me.

I read through this again and found a comment that needed to be
updated, so I did that, bumped catversion, and committed this.

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

On Thu, Aug 25, 2022 at 10:19 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Aug 24, 2022 at 10:23 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> > On 8/24/22 12:28 AM, Robert Haas wrote:
> > > This patch needed to be rebased pretty extensively after commit
> > > ce6b672e4455820a0348214be0da1a024c3f619f. Here is a new version.
> > Thanks, Robert, I have retested this patch with my previous scenarios
> > and things look good to me.
>
> I read through this again and found a comment that needed to be
> updated, so I did that, bumped catversion, and committed this.

Upon further review, this patch failed to update the documentation as
thoroughly as would have been desirable.

Here is a follow-up patch to correct a few oversights.

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

On Fri, Aug 26, 2022 at 02:46:59PM -0400, Robert Haas wrote:
> -   membership is via <literal>admin</literal> which has the <literal>NOINHERIT</literal>
> -   attribute.  After:
> +   membership is via <literal>admin</literal> which was granted using
> +   <literal>WITH INHERIT FALSE</literal> After:

nitpick: I believe there should be a period before "After."

Otherwise, LGTM.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sun, Aug 28, 2022 at 5:14 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> nitpick: I believe there should be a period before "After."
>
> Otherwise, LGTM.

Good catch. Thanks for the review. Committed with that correction.

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

On Mon, Aug 29, 2022 at 10:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Good catch. Thanks for the review. Committed with that correction.

Argh, I found a bug, and one that I should have caught during testing,
too. I modelled the new function select_best_grantor() on
is_admin_of_role(), but it differs in that it calls
roles_is_member_of() with ROLERECURSE_PRIVS rather than
ROLECURSE_MEMBERS. Sadly, roles_is_member_of() handles
ROLERECURSE_PRIVS by completely ignoring non-inherited grants, which
is wrong, because then calls to select_best_grantor() treat a member
of a role with INHERIT FALSE, ADMIN TRUE is if they were not an admin
at all, which is incorrect.

Here is a patch to rearrange the logic slightly and also add a test
case memorializing the intended behavior. Without this change, the
regression test included in the patch fails like this:

ERROR:  no possible grantors

...which is never supposed to happen.

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

On Mon, Aug 29, 2022 at 03:38:57PM -0400, Robert Haas wrote:
> Here is a patch to rearrange the logic slightly and also add a test
> case memorializing the intended behavior. Without this change, the
> regression test included in the patch fails like this:
> 
> ERROR:  no possible grantors
> 
> ...which is never supposed to happen.

The grantor column in the expected test output refers to "rhaas", so I
imagine the test only passes on your machines.  Should we create a new
grantor role for this test?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Aug 29, 2022 at 6:04 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Mon, Aug 29, 2022 at 03:38:57PM -0400, Robert Haas wrote:
> > Here is a patch to rearrange the logic slightly and also add a test
> > case memorializing the intended behavior. Without this change, the
> > regression test included in the patch fails like this:
> >
> > ERROR:  no possible grantors
> >
> > ...which is never supposed to happen.
>
> The grantor column in the expected test output refers to "rhaas", so I
> imagine the test only passes on your machines.  Should we create a new
> grantor role for this test?

That's the second time I've made that exact mistake *on this thread*.

I'm super good at this, I promise.

I'll figure out a fix tomorrow when I'm less tired. Thanks for catching it.

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

On Mon, Aug 29, 2022 at 10:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I'll figure out a fix tomorrow when I'm less tired. Thanks for catching it.

Second try.

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

On Tue, Aug 30, 2022 at 8:24 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Aug 29, 2022 at 10:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > I'll figure out a fix tomorrow when I'm less tired. Thanks for catching it.
>
> Second try.

Third try.

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

On Mon, Aug 29, 2022 at 03:38:57PM -0400, Robert Haas wrote:
> Argh, I found a bug, and one that I should have caught during testing,
> too. I modelled the new function select_best_grantor() on
> is_admin_of_role(), but it differs in that it calls
> roles_is_member_of() with ROLERECURSE_PRIVS rather than
> ROLECURSE_MEMBERS. Sadly, roles_is_member_of() handles
> ROLERECURSE_PRIVS by completely ignoring non-inherited grants, which
> is wrong, because then calls to select_best_grantor() treat a member
> of a role with INHERIT FALSE, ADMIN TRUE is if they were not an admin
> at all, which is incorrect.

I've been staring at this stuff for a while, and I'm still rather confused.

* You mention that you modelled the "new" function select_best_grantor() on
is_admin_of_role(), but it looks like that function was introduced in 2005.
IIUC you meant select_best_admin(), which was added much more recently.

* I don't see why we should ignore inheritance until after checking admin
option.  Prior to your commit (e3ce2de), we skipped checking for admin
option if the role had [NO]INHERIT, and AFAICT your commit aimed to retain
that behavior.  The comment above check_role_grantor() even mentions
"inheriting the privileges of a role which does have ADMIN OPTION."  Within
the function, I see the following comment:

         * Otherwise, the grantor must either have ADMIN OPTION on the role or
         * inherit the privileges of a role which does. In the former case,
         * record the grantor as the current user; in the latter, pick one of
         * the roles that is "most directly" inherited by the current role
         * (i.e. fewest "hops").

So, IIUC, the intent is for ADMIN OPTION to apply even if for non-inherited
grants, but we still choose to recurse in roles_is_member_of() based on
inheritance.  This means that you can inherit the ADMIN OPTION to some
degree, but if you have membership WITH INHERIT FALSE to a role that has
ADMIN OPTION on another role, the current role effectively does not have
ADMIN OPTION on the other role.  Is this correct?

As I'm writing this down, I think it's beginning to make more sense to me,
so this might just be a matter of under-caffeination.  But perhaps some
extra comments or documentation wouldn't be out of the question...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Aug 30, 2022 at 1:30 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I've been staring at this stuff for a while, and I'm still rather confused.
>
> * You mention that you modelled the "new" function select_best_grantor() on
> is_admin_of_role(), but it looks like that function was introduced in 2005.
> IIUC you meant select_best_admin(), which was added much more recently.

Well, a combination of the two. It's modeled after is_admin_of_role()
in the sense that it also calls roles_is_member_of() with a
non-InvalidOid third argument and a non-NULL fourth argument. All
other callers pass InvalidOid/NULL.

> * I don't see why we should ignore inheritance until after checking admin
> option.  Prior to your commit (e3ce2de), we skipped checking for admin
> option if the role had [NO]INHERIT, and AFAICT your commit aimed to retain
> that behavior.  The comment above check_role_grantor() even mentions
> "inheriting the privileges of a role which does have ADMIN OPTION."  Within
> the function, I see the following comment:
>
>                  * Otherwise, the grantor must either have ADMIN OPTION on the role or
>                  * inherit the privileges of a role which does. In the former case,
>                  * record the grantor as the current user; in the latter, pick one of
>                  * the roles that is "most directly" inherited by the current role
>                  * (i.e. fewest "hops").
>
> So, IIUC, the intent is for ADMIN OPTION to apply even if for non-inherited
> grants, but we still choose to recurse in roles_is_member_of() based on
> inheritance.  This means that you can inherit the ADMIN OPTION to some
> degree, but if you have membership WITH INHERIT FALSE to a role that has
> ADMIN OPTION on another role, the current role effectively does not have
> ADMIN OPTION on the other role.  Is this correct?

I think it's easiest if I get an example. Suppose role 'william'
inherits 'charles' and 'diana' and role 'charles' in turn inherits
from roles 'elizabeth' and 'philip'. Furthermore, 'william' has ADMIN
OPTION on 'cambridge', 'charles' on 'wales', and 'elizabeth' on 'uk'.
Now, because 'william' has ADMIN OPTION on role 'cambridge', he can
add members to that role and remove the ones he added. He can also
grant admin out option to others and later remove it (and all
dependent grants that those others have made). When he does this, he
is acting as himself, on his own authority. Because he inherits the
privileges of charles directly and of elizabeth via charles, he can
also add members to wales and uk. But if does this, he is not acting
as himself, because he himself does not possess the ADMIN OPTION on
either of those roles. Rather, he is acting on authority delegated
from some other role which does possess admin option on those roles.
Therefore, if 'william' adds a member to 'uk', the grantor MUST be
recorded as 'elizabeth', not 'william', because the grantor of record
must possess admin option on the role.

Now let's add another layer of complexity. Suppose that the 'uk' role
possesses ADMIN OPTION on some other role, let's say 'parliament'. Can
'william' use the fact that he inherits from 'elizabeth' to grant
membership in 'parliament'? That all depends on whether 'uk' was
granted to 'elizabeth' WITH INHERIT TRUE or WITH INHERIT FALSE. If it
was granted WITH INHERIT TRUE, then elizabeth freely exercises all the
powers of parliament, william freely exercises all of elizabeth's
powers, and thus william can grant membership in parliament. Such a
membership will be recorded as having been granted by uk, the role
that actually holds ADMIN OPTION on parliament. However, if elizabeth
was granted uk WITH INHERIT FALSE, then she can't mess with the
membership of parliament, and thus neither can william. At least, not
without first executing "SET ROLE uk", which in this scenario, either
could do, but perhaps the use of SET ROLE is logged, audited, and very
carefully scrutinized.

So, what does all of this mean for select_best_grantor() and its
helper function roles_is_member_of()? Well, first, we have to recurse.
william can act on his own behalf when administering cambridge, and on
behalf of charles or elizabeth when administering wales or uk
respectively, and there's no way to get that behavior without
recursing. Second, we have to stop the recursion when we reach a grant
that is not inherited. Otherwise, william might be able to act on
behalf of uk and administer membership in parliament even if uk is
granted to elizabeth WITH INHERIT FALSE. Third, and this is where it
gets tricky, we have to stop recursion *only after checking whether
we've found a valid grantor*. william is allowed to administer
membership cambridge whether or not it is granted to william WITH
INHERIT TRUE or WITH INHERIT FALSE. Either way, he possess ADMIN
OPTION on that role, and may administer membership in it. Likewise, he
can administer membership in wales or uk as charles or elizabeth,
respectively, because he definitely inherits the privileges of roles
which definitely have ADMIN OPTION on those roles.

Maybe a clearer way to look at is this: we're going to transit a
series of role-membership links going from william (who attempts some
operation) to the role in which he's trying to grant (or revoke)
membership. For the operation to succeed, we need every link but the
last in the chain to have INHERIT TRUE, and we need the last link in
the chain to have ADMIN TRUE. We don't care whether the links other
than the last one have ADMIN, and we don't care whether the last link
has INHERIT. Thus:

- william => cambridge is OK because the one and only grant involved
has ADMIN, whether or not it has INHERIT
- william => charles => wales is OK because the first hop has INHERIT
and the second hop has ADMIN
- william => charles => elizabeth => uk is OK because the first two
hops have ADMIN and the last hop has INHERIT
- william => charles => elizabeth => uk => parliament is probably not
OK because although the first two hops have ADMIN and the last has
INHERIT, the third hop probably lacks INHERIT

I hope this makes it clearer. select_best_grantor() can't completely
disregard links without INHERIT, because if it does, it can't pay any
attention to the last grant in the chain, which only needs to have
ADMIN, not INHERIT. But it must pay some attention to them, because
every earlier link in the chain does need to have INHERIT. By moving
the if-test up, the patch makes it behave just that way, or at least,
I think it does.

> As I'm writing this down, I think it's beginning to make more sense to me,
> so this might just be a matter of under-caffeination.  But perhaps some
> extra comments or documentation wouldn't be out of the question...

I do not rule that out!

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

On Tue, Aug 30, 2022 at 03:24:56PM -0400, Robert Haas wrote:
> - william => charles => elizabeth => uk is OK because the first two
> hops have ADMIN and the last hop has INHERIT

Don't you mean the first two hops have INHERIT and the last has ADMIN?

> - william => charles => elizabeth => uk => parliament is probably not
> OK because although the first two hops have ADMIN and the last has
> INHERIT, the third hop probably lacks INHERIT

Same here.  I don't mean to be pedantic, I just want to make sure I'm
thinking of this correctly.

> I hope this makes it clearer. select_best_grantor() can't completely
> disregard links without INHERIT, because if it does, it can't pay any
> attention to the last grant in the chain, which only needs to have
> ADMIN, not INHERIT. But it must pay some attention to them, because
> every earlier link in the chain does need to have INHERIT. By moving
> the if-test up, the patch makes it behave just that way, or at least,
> I think it does.

Yes, this is very helpful.  I always appreciate your detailed examples.  I
think what you are describing matches the mental model I was beginning to
form.

Okay, now to take a closer look at the patch...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Aug 30, 2022 at 08:33:46AM -0400, Robert Haas wrote:
> Third try.

Now that I have this grantor stuff fresh in my mind, this patch looks good
to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Aug 30, 2022 at 5:20 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 03:24:56PM -0400, Robert Haas wrote:
> > - william => charles => elizabeth => uk is OK because the first two
> > hops have ADMIN and the last hop has INHERIT
>
> Don't you mean the first two hops have INHERIT and the last has ADMIN?

I do.

> > - william => charles => elizabeth => uk => parliament is probably not
> > OK because although the first two hops have ADMIN and the last has
> > INHERIT, the third hop probably lacks INHERIT
>
> Same here.  I don't mean to be pedantic, I just want to make sure I'm
> thinking of this correctly.

Right. The first two hops have INHERIT and the last has ADMIN, but the
third hop probably lacks INHERIT.

> Yes, this is very helpful.  I always appreciate your detailed examples.  I
> think what you are describing matches the mental model I was beginning to
> form.

Cool.

> Okay, now to take a closer look at the patch...

Thanks.

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

On Tue, Aug 30, 2022 at 6:10 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 08:33:46AM -0400, Robert Haas wrote:
> > Third try.
>
> Now that I have this grantor stuff fresh in my mind, this patch looks good
> to me.

Thanks for reviewing. Committed.

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

Hello,

> Thanks for reviewing. Committed.

Let me return to this topic.

After looking at the changes in this patch, I have a suggestion.

The inheritance option for role memberships is important information to 
know if the role privileges will be available automatically or if a 
switch with a SET ROLE command is required. However, this information 
cannot be obtained with psql commands, specifically \du or \dg.

Previously, you could see the inherit attribute of the role (its absence 
is shown with \du). Now you have to look in the pg_auth_members system 
catalog.

My suggestion is to add information about pg_auth_members.inherit_option 
to the output of \du (\dg).

If so, we can also add information about pg_auth_members.admin_option. 
Right now this information is not available in psql command output either.

I thought about how exactly to represent these options in the output 
\du, but did not find a single solution. Considered the following choices:

1.
Add \du+ command and for each membership in the role add values of two 
options. I haven't done a patch yet, but you can imagine the changes 
like this:

CREATE ROLE alice LOGIN IN ROLE pg_read_all_data;

\du+ alice
                  List of roles
  Role name | Attributes |     Member of
-----------+------------+--------------------
  alice     |            | {pg_read_all_data(admin=f inherit=t)}

It looks long, but for \du+ it's not a problem.

2.
I assume that the default values for these options will rarely change. 
In that case, we can do without \du+ and output only the changed values 
directly in the \du command.

GRANT pg_read_all_data TO alice WITH INHERIT FALSE;

2a.
\du alice
                  List of roles
  Role name | Attributes |     Member of
-----------+------------+--------------------
  alice     |            | {pg_read_all_data(inherit=f)}

2b.
Similar to GRANT OPTION, we can use symbols instead of long text 
(inherit=f) for options. For example, for the ADMIN OPTION we can use 
"*" (again similar to GRANT OPTION), and for the missing INHERIT option 
something else, such as "-":

GRANT pg_read_all_data TO alice WITH ADMIN TRUE;
GRANT pg_write_all_data TO alice WITH INHERIT FALSE;

\du alice
                  List of roles
  Role name | Attributes |     Member of
-----------+------------+--------------------
  alice     |            | {pg_read_all_data*-,pg_write_all_data-}

But I think choices 2a and 2b are too complicated to understand. 
Especially because the two options have different default values. And 
even more. The default value for the INHERIT option depends on the value 
of the INHERIT attribute for the role.

So I like the first choice with \du+ better.

But perhaps there are other choices as well.

If it's interesting, I'm ready to open a new thread (the commitfest 
entry for this topic is now closed) and prepare a patch.

--
Pavel Luzanov

On Thu, Aug 25, 2022 at 10:19:39AM -0400, Robert Haas wrote:
> I read through this again and found a comment that needed to be
> updated, so I did that, bumped catversion, and committed this.

[commit e3ce2de]

> @@ -4735,8 +4735,8 @@ initialize_acl(void)
>  
>          /*
>           * In normal mode, set a callback on any syscache invalidation of rows
> -         * of pg_auth_members (for roles_is_member_of()), pg_authid (for
> -         * has_rolinherit()), or pg_database (for roles_is_member_of())
> +         * of pg_auth_members (for roles_is_member_of()) pg_database (for
> +         * roles_is_member_of())
>           */
>          CacheRegisterSyscacheCallback(AUTHMEMROLEMEM,
>                                        RoleMembershipCacheCallback,

I agree one could remove the "CacheRegisterSyscacheCallback(AUTHOID, ...)".
This updated the comment as though the patch were including that removal, but
AUTHOID remains.  Also, that comment needs s/pg_database/or &/.


These sites didn't change in v16 and may or may not warrant change:

doc/src/sgml/catalogs.sgml:1522:       <structfield>rolinherit</structfield> <type>bool</type>
doc/src/sgml/system-views.sgml:2585:       <structfield>rolinherit</structfield> <type>bool</type>
src/include/catalog/pg_authid.h:36:    bool        rolinherit;        /* inherit privileges from other roles? */

I likely would leave pg_authid.h as-is but change the doc/ phrases.


https://postgr.es/m/17901-93eacb513e503f43%40postgresql.org led me to notice
that v16 always inherits the implicit membership in role pg_database_owner,
with no way to override like one could in v15.  That message's test procedure
doesn't "fail" in v16.  I think that's fine, but I'm mentioning it since
pg_database_owner didn't appear upthread.