Re: [HACKERS] Additional role attributes && superuser review

[Noah Misch]

On Wed, Feb 03, 2016 at 01:44:28PM -0500, Robert Haas wrote:
> On Thu, Jan 28, 2016 at 4:37 PM, Stephen Frost  wrote:
> > pg_monitor
> >
> >   Allows roles granted more information from pg_stat_activity.  Can't be
> >   just a regular non-default-role right as we don't, currently, have a
> >   way to say "filter out the values of certain columns on certain rows,
> >   but not on others."
> >
> >   (There's a question here though- for the privileges which will be
> >   directly GRANT'able, should we GRANT those to pg_monitor, or have
> >   pg_monitor only provide unfiltered access to pg_stat_activity, or..?
> >   Further, if it's only for pg_stat_activity, should we name it
> >   something else?)
> 
> I endorse this proposed role.  I'd have it just grant access to
> pg_stat_activity but keep the name pg_monitor so that it could apply
> to other similar things in the future, but there might be other good
> alternatives too.

-1 for adding any pg_monitor role.  That name belongs to a role allowing, from
inception, more than just full pg_stat_activity access.  The last, vigorous
effort to define its scope failed; there may not exist one notion of
"monitoring" operations qualified to claim this name.  At least, the community
lacks the ability to design it in the foreseeable future.

> > pg_signal_backend
> >
> >   Allows roles to signal other backend processes beyond those backends
> >   which are owned by a user they are a role member of.  Can't be a
> >   regular non-default-role right as we don't, currently, have any way to
> >   GRANT rights to send signals only to backends you own or are a member
> >   of.
> 
> I also endorse this.

+1.  This role has a clear scope and a name denoting that scope, so it's a
sound way to introduce built-in role infrastructure.  I like the concept of
built-in roles.  One can inherit them via user-defined roles and use NOINHERIT
to regulate that.  One can grant WITH ADMIN OPTION, something role options
like REPLICATION don't offer.  It feels more scalable than defining role
options, and it's a technique non-core code can imitate.

I haven't decided whether it's a problem that members of built-in roles can
create objects owned by those roles.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Thu, Jan 28, 2016 at 4:37 PM, Stephen Frost  wrote:
> pg_monitor
>
>   Allows roles granted more information from pg_stat_activity.  Can't be
>   just a regular non-default-role right as we don't, currently, have a
>   way to say "filter out the values of certain columns on certain rows,
>   but not on others."
>
>   (There's a question here though- for the privileges which will be
>   directly GRANT'able, should we GRANT those to pg_monitor, or have
>   pg_monitor only provide unfiltered access to pg_stat_activity, or..?
>   Further, if it's only for pg_stat_activity, should we name it
>   something else?)

I endorse this proposed role.  I'd have it just grant access to
pg_stat_activity but keep the name pg_monitor so that it could apply
to other similar things in the future, but there might be other good
alternatives too.

> pg_signal_backend
>
>   Allows roles to signal other backend processes beyond those backends
>   which are owned by a user they are a role member of.  Can't be a
>   regular non-default-role right as we don't, currently, have any way to
>   GRANT rights to send signals only to backends you own or are a member
>   of.

I also endorse this.

> pg_replication
>
>   Allows roles to use the various replication functions.  Can't be a
>   regular non-default-role right as the REPLICATION role attribute
>   allows access to those functions and the GRANT system has no way of
>   saying "allow access to these functions if they have role attribute
>   X."
>
> Make sense, as these are cases where we can't simply write GRANT
> statements and get the same result, but we don't need (or at least,
> shouldn't have without supporting GRANT on catalog objects and agreement
> on what they're intended for):

This seems like it could be reshuffled so that it can be done with
GRANT.  Therefore, I don't endorse this.

> pg_backup
>
>   pg_start_backup(), pg_stop_backup(), pg_switch_xlog(), and
>   pg_create_restore_point() will all be managed by the normal GRANT
>   system and therefore we don't need a default role for those use-cases.

Agreed.

> pg_file_settings
>
>   pg_file_settings() function and pg_file_settings view will be managed
>   by the normal GRANT system and therefore we don't need a default role
>   for those use-cases.

Agreed.

> pg_replay
>
>   pg_xlog_replay_pause(), and pg_xlog_replay_resume() will be managed
>   by the normal GRANT system and therefore we don't need a default role
>   for those use-cases.

Agreed.

> pg_rotate_logfile
>
>   pg_rotate_logfile() will be managed by the normal GRANT system and
>   therefore we don't need a default role for that use-cases.

Agreed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Sun, Jan 31, 2016 at 7:55 AM, Michael Paquier
 wrote:
> On Sun, Jan 31, 2016 at 5:32 AM, Craig Ringer  wrote:
>> On 29 January 2016 at 22:41, Stephen Frost  wrote:
>>>
>>> Michael,
>>>
>>> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost 
>>> > wrote:
>>> > > * Robert Haas (robertmh...@gmail.com) wrote:
>>> > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
>>> > wrote:
>>> > >> > Personally, I don't have any particular issue having both, but the
>>> > >> > desire was stated that it would be better to have the regular
>>> > >> > GRANT EXECUTE ON catalog_func() working before we consider having
>>> > >> > default roles for same.  That moves the goal posts awful far
>>> > >> > though, if
>>> > >> > we're to stick with that and consider how we might extend the GRANT
>>> > >> > system in the future.
>>> > >>
>>> > >> I don't think it moves the goal posts all that far.  Convincing
>>> > >> pg_dump to dump grants on system functions shouldn't be a crazy large
>>> > >> patch.
>>> > >
>>> > > I wasn't clear as to what I was referring to here.  I've already
>>> > > written
>>> > > a patch to pg_dump to support grants on system objects and agree that
>>> > > it's at least reasonable.
>>> >
>>> > Is it already posted somewhere? I don't recall seeing it. Robert and
>>> > Noah
>>> > have a point that this would be useful for users who would like to dump
>>> > GRANT/REVOKE rights on system functions & all, using a new option in
>>> > pg_dumpall, say --with-system-acl or --with-system-privileges.
>>>
>>> Multiple versions were posted on this thread.  I don't fault you for not
>>> finding it, this thread is a bit long in the tooth.  The one I'm
>>> currently working from is
>>>
>>
>> It strikes me that this thread would possibly benefit from a wiki page
>> outlining the permissions, overall concepts, etc, as it's getting awfully
>> hard to follow.
>
> +1. This has proved to be very beneficial for UPSERT.

I am marking this patch as returned with feedback per the current
status of this thread.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Craig Ringer]

On 29 January 2016 at 22:41, Stephen Frost  wrote:

> Michael,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
> > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost 
> wrote:
> > > * Robert Haas (robertmh...@gmail.com) wrote:
> > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
> > wrote:
> > >> > Personally, I don't have any particular issue having both, but the
> > >> > desire was stated that it would be better to have the regular
> > >> > GRANT EXECUTE ON catalog_func() working before we consider having
> > >> > default roles for same.  That moves the goal posts awful far
> though, if
> > >> > we're to stick with that and consider how we might extend the GRANT
> > >> > system in the future.
> > >>
> > >> I don't think it moves the goal posts all that far.  Convincing
> > >> pg_dump to dump grants on system functions shouldn't be a crazy large
> > >> patch.
> > >
> > > I wasn't clear as to what I was referring to here.  I've already
> written
> > > a patch to pg_dump to support grants on system objects and agree that
> > > it's at least reasonable.
> >
> > Is it already posted somewhere? I don't recall seeing it. Robert and Noah
> > have a point that this would be useful for users who would like to dump
> > GRANT/REVOKE rights on system functions & all, using a new option in
> > pg_dumpall, say --with-system-acl or --with-system-privileges.
>
> Multiple versions were posted on this thread.  I don't fault you for not
> finding it, this thread is a bit long in the tooth.  The one I'm
> currently working from is
>
>
It strikes me that this thread would possibly benefit from a wiki page
outlining the permissions, overall concepts, etc, as it's getting awfully
hard to follow.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Sun, Jan 31, 2016 at 5:32 AM, Craig Ringer  wrote:
> On 29 January 2016 at 22:41, Stephen Frost  wrote:
>>
>> Michael,
>>
>> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost 
>> > wrote:
>> > > * Robert Haas (robertmh...@gmail.com) wrote:
>> > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
>> > wrote:
>> > >> > Personally, I don't have any particular issue having both, but the
>> > >> > desire was stated that it would be better to have the regular
>> > >> > GRANT EXECUTE ON catalog_func() working before we consider having
>> > >> > default roles for same.  That moves the goal posts awful far
>> > >> > though, if
>> > >> > we're to stick with that and consider how we might extend the GRANT
>> > >> > system in the future.
>> > >>
>> > >> I don't think it moves the goal posts all that far.  Convincing
>> > >> pg_dump to dump grants on system functions shouldn't be a crazy large
>> > >> patch.
>> > >
>> > > I wasn't clear as to what I was referring to here.  I've already
>> > > written
>> > > a patch to pg_dump to support grants on system objects and agree that
>> > > it's at least reasonable.
>> >
>> > Is it already posted somewhere? I don't recall seeing it. Robert and
>> > Noah
>> > have a point that this would be useful for users who would like to dump
>> > GRANT/REVOKE rights on system functions & all, using a new option in
>> > pg_dumpall, say --with-system-acl or --with-system-privileges.
>>
>> Multiple versions were posted on this thread.  I don't fault you for not
>> finding it, this thread is a bit long in the tooth.  The one I'm
>> currently working from is
>>
>
> It strikes me that this thread would possibly benefit from a wiki page
> outlining the permissions, overall concepts, etc, as it's getting awfully
> hard to follow.

+1. This has proved to be very beneficial for UPSERT.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
> wrote:
> >> > Personally, I don't have any particular issue having both, but the
> >> > desire was stated that it would be better to have the regular
> >> > GRANT EXECUTE ON catalog_func() working before we consider having
> >> > default roles for same.  That moves the goal posts awful far though, if
> >> > we're to stick with that and consider how we might extend the GRANT
> >> > system in the future.
> >>
> >> I don't think it moves the goal posts all that far.  Convincing
> >> pg_dump to dump grants on system functions shouldn't be a crazy large
> >> patch.
> >
> > I wasn't clear as to what I was referring to here.  I've already written
> > a patch to pg_dump to support grants on system objects and agree that
> > it's at least reasonable.
> 
> Is it already posted somewhere? I don't recall seeing it. Robert and Noah
> have a point that this would be useful for users who would like to dump
> GRANT/REVOKE rights on system functions & all, using a new option in
> pg_dumpall, say --with-system-acl or --with-system-privileges. 

Multiple versions were posted on this thread.  I don't fault you for not
finding it, this thread is a bit long in the tooth.  The one I'm
currently working from is:

http://www.postgresql.org/message-id/attachment/38049/catalog_function_acls_v4.patch

I'm going to split up the pg_dump changes and the backend changes, as
they can logically go in independently (though without both, we're not
moving the needle very far with regards to what administrators can do).

> If at least
> the three of you are agreeing here I think that we should try to move at
> least toward this goal first. That seems a largely doable goal for 9.6. For
> the set of default roles, there is clearly no clear consensus regarding
> what each role should do or not, and under which limitation it should
> operate.

I'm trying to work towards a consensus on the default roles, hence the
questions and discussion posed in the email you replied to.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Fri, Jan 29, 2016 at 11:41 PM, Stephen Frost  wrote:
> Michael,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost  wrote:
>> > * Robert Haas (robertmh...@gmail.com) wrote:
>> >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
>> wrote:
>> >> > Personally, I don't have any particular issue having both, but the
>> >> > desire was stated that it would be better to have the regular
>> >> > GRANT EXECUTE ON catalog_func() working before we consider having
>> >> > default roles for same.  That moves the goal posts awful far though, if
>> >> > we're to stick with that and consider how we might extend the GRANT
>> >> > system in the future.
>> >>
>> >> I don't think it moves the goal posts all that far.  Convincing
>> >> pg_dump to dump grants on system functions shouldn't be a crazy large
>> >> patch.
>> >
>> > I wasn't clear as to what I was referring to here.  I've already written
>> > a patch to pg_dump to support grants on system objects and agree that
>> > it's at least reasonable.
>>
>> Is it already posted somewhere? I don't recall seeing it. Robert and Noah
>> have a point that this would be useful for users who would like to dump
>> GRANT/REVOKE rights on system functions & all, using a new option in
>> pg_dumpall, say --with-system-acl or --with-system-privileges.
>
> Multiple versions were posted on this thread.  I don't fault you for not
> finding it, this thread is a bit long in the tooth.  The one I'm
> currently working from is:
>
> http://www.postgresql.org/message-id/attachment/38049/catalog_function_acls_v4.patch
>
> I'm going to split up the pg_dump changes and the backend changes, as
> they can logically go in independently (though without both, we're not
> moving the needle very far with regards to what administrators can do).

OK. Looks like a good way to move forward to me.

>> If at least
>> the three of you are agreeing here I think that we should try to move at
>> least toward this goal first. That seems a largely doable goal for 9.6. For
>> the set of default roles, there is clearly no clear consensus regarding
>> what each role should do or not, and under which limitation it should
>> operate.
>
> I'm trying to work towards a consensus on the default roles, hence the
> questions and discussion posed in the email you replied to.

So the current CF entry should be marked as returned with feedback
perhaps? What do you think? Once patches are ready for the default
roles in backend and for pg_dump, we could then work on reviewing them
separately.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Jan 17, 2016 at 6:58 PM, Stephen Frost  wrote:
> > I'm not against that idea, though I continue to feel that there are
> > common sets of privileges which backup tools could leverage.
> >
> > The other issue that I'm running into, again, while considering how to
> > move back to ACL-based permissions for these objects is that we can't
> > grant out the actual permissions which currently exist.  That means we
> > either need to break backwards compatibility, which would be pretty
> > ugly, in my view, or come up with new functions and then users will have
> > to know which functions to use when.
> >
> > As I don't think we really want to break backwards compatibility or
> > remove existing functionality, the only approach which is going to make
> > sense is to add additional functions in some cases.  In particular, we
> > will need alternate versions of pg_terminate_backend and
> > pg_cancel_backend.  One thought I had was to make that
> > 'pg_signal_backend', but that sounds like we'd allow any signal sent by
> > a user with that right, which seems a bit much to me...
> 
> So, this seems like a case where a built-in role would be
> well-justified.  I don't really believe in built-in roles as a way of
> bundling related permissions; I know you do, but I don't.  I'd rather
> see the individual function permissions granted individually.  But
> here you are talking about a variable level of access to the same
> function, depending on role.  And for that it seems to me that a
> built-in role has a lot more to recommend it in that case than it does
> in the other.  If you have been granted pg_whack, you can signal any
> process on the system; otherwise just your own.  Those checks are
> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
> substitute.

If we extend this into the future then it seems to potentially fall
afoul of Noah's concern that we're going to end up with two different
ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
of pg_stat_activity, where values are shown or hidden based on who the
current role is.  The policy system only supports whole-row, today, but
the question has already come up, both on the lists and off, of support
for hiding individual column values for certain rows, exactly as we do
today for pg_stat_activity.  Once we reach that point, we'll have a way
to GRANT out these rights and a default role which just has them.

Personally, I don't have any particular issue having both, but the
desire was stated that it would be better to have the regular
GRANT EXECUTE ON catalog_func() working before we consider having
default roles for same.  That moves the goal posts awful far though, if
we're to stick with that and consider how we might extend the GRANT
system in the future.

What got me thinking along these lines was a question posed by Bruce
(Bruce, feel free to chime in if I've misunderstood) to me at SCALE
where we were chatting a bit about this, which was- how could we extend
GRANT to support the permissions that we actually wish
pg_terminate_backend() and pg_cancel_backend() to have, instead of using
a default role?  I didn't think too much on it at the time as it strikes
me as a pretty narrow use-case and requiring quite a bit of complexity
to get right, but there again, I'd certainly view a system where the
user is allowed to have pg_start_backup() rights but not
pg_stop_backup() as being quite a small use-case also, yet that's the
direction we're largely going in with this discussion.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost  wrote:
>> So, this seems like a case where a built-in role would be
>> well-justified.  I don't really believe in built-in roles as a way of
>> bundling related permissions; I know you do, but I don't.  I'd rather
>> see the individual function permissions granted individually.  But
>> here you are talking about a variable level of access to the same
>> function, depending on role.  And for that it seems to me that a
>> built-in role has a lot more to recommend it in that case than it does
>> in the other.  If you have been granted pg_whack, you can signal any
>> process on the system; otherwise just your own.  Those checks are
>> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
>> substitute.
>
> If we extend this into the future then it seems to potentially fall
> afoul of Noah's concern that we're going to end up with two different
> ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
> of pg_stat_activity, where values are shown or hidden based on who the
> current role is.  The policy system only supports whole-row, today, but
> the question has already come up, both on the lists and off, of support
> for hiding individual column values for certain rows, exactly as we do
> today for pg_stat_activity.  Once we reach that point, we'll have a way
> to GRANT out these rights and a default role which just has them.

Well, I'm not saying that predefined rolls are the *only* way to solve
a problem like this, but I think they're one way and I don't clearly
see that something else is better.  However, my precise point is that
we should *not* have predefined rolls that precisely duplicate the
result of GRANT EXECUTE X ON Y.  Having predefined rolls that change
the behavior of the system in other ways is a different thing.  So I
don't see how this falls afoul of Noah's concern.  (If it does,
perhaps he can clarify.)

> Personally, I don't have any particular issue having both, but the
> desire was stated that it would be better to have the regular
> GRANT EXECUTE ON catalog_func() working before we consider having
> default roles for same.  That moves the goal posts awful far though, if
> we're to stick with that and consider how we might extend the GRANT
> system in the future.

I don't think it moves the goal posts all that far.  Convincing
pg_dump to dump grants on system functions shouldn't be a crazy large
patch.

> What got me thinking along these lines was a question posed by Bruce
> (Bruce, feel free to chime in if I've misunderstood) to me at SCALE
> where we were chatting a bit about this, which was- how could we extend
> GRANT to support the permissions that we actually wish
> pg_terminate_backend() and pg_cancel_backend() to have, instead of using
> a default role?  I didn't think too much on it at the time as it strikes
> me as a pretty narrow use-case and requiring quite a bit of complexity
> to get right, but there again, I'd certainly view a system where the
> user is allowed to have pg_start_backup() rights but not
> pg_stop_backup() as being quite a small use-case also, yet that's the
> direction we're largely going in with this discussion.

Well, sure, in that largely artificial example it's not hard to say
ha, ha, silly, but the actual examples we looked at upthread were much
less obviously silly.  There was plenty of room for argument about the
precise contours of each predefined role.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost  wrote:
> >> So, this seems like a case where a built-in role would be
> >> well-justified.  I don't really believe in built-in roles as a way of
> >> bundling related permissions; I know you do, but I don't.  I'd rather
> >> see the individual function permissions granted individually.  But
> >> here you are talking about a variable level of access to the same
> >> function, depending on role.  And for that it seems to me that a
> >> built-in role has a lot more to recommend it in that case than it does
> >> in the other.  If you have been granted pg_whack, you can signal any
> >> process on the system; otherwise just your own.  Those checks are
> >> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
> >> substitute.
> >
> > If we extend this into the future then it seems to potentially fall
> > afoul of Noah's concern that we're going to end up with two different
> > ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
> > of pg_stat_activity, where values are shown or hidden based on who the
> > current role is.  The policy system only supports whole-row, today, but
> > the question has already come up, both on the lists and off, of support
> > for hiding individual column values for certain rows, exactly as we do
> > today for pg_stat_activity.  Once we reach that point, we'll have a way
> > to GRANT out these rights and a default role which just has them.
> 
> Well, I'm not saying that predefined rolls are the *only* way to solve
> a problem like this, but I think they're one way and I don't clearly
> see that something else is better.  However, my precise point is that
> we should *not* have predefined rolls that precisely duplicate the
> result of GRANT EXECUTE X ON Y.  Having predefined rolls that change
> the behavior of the system in other ways is a different thing.  So I
> don't see how this falls afoul of Noah's concern.  (If it does,
> perhaps he can clarify.)

Apologies if it seems like what I'm getting at is obtuse but I'm trying
to generalize this, to provide guidance on how to handle the larger set
of privileges.

If I'm following correctly, having default roles for cases where the
role is specifically for something more than 'GRANT EXECUTE X ON Y' (or
a set of such commands..?) makes sense.  Going back to the list of
roles, that would mean that default roles:

pg_monitor

  Allows roles granted more information from pg_stat_activity.  Can't be
  just a regular non-default-role right as we don't, currently, have a
  way to say "filter out the values of certain columns on certain rows,
  but not on others."

  (There's a question here though- for the privileges which will be
  directly GRANT'able, should we GRANT those to pg_monitor, or have
  pg_monitor only provide unfiltered access to pg_stat_activity, or..?
  Further, if it's only for pg_stat_activity, should we name it
  something else?)

pg_signal_backend

  Allows roles to signal other backend processes beyond those backends
  which are owned by a user they are a role member of.  Can't be a
  regular non-default-role right as we don't, currently, have any way to
  GRANT rights to send signals only to backends you own or are a member
  of.

pg_replication

  Allows roles to use the various replication functions.  Can't be a
  regular non-default-role right as the REPLICATION role attribute
  allows access to those functions and the GRANT system has no way of
  saying "allow access to these functions if they have role attribute
  X."

Make sense, as these are cases where we can't simply write GRANT
statements and get the same result, but we don't need (or at least,
shouldn't have without supporting GRANT on catalog objects and agreement
on what they're intended for):

pg_backup

  pg_start_backup(), pg_stop_backup(), pg_switch_xlog(), and
  pg_create_restore_point() will all be managed by the normal GRANT
  system and therefore we don't need a default role for those use-cases.

pg_file_settings

  pg_file_settings() function and pg_file_settings view will be managed
  by the normal GRANT system and therefore we don't need a default role
  for those use-cases.

pg_replay

  pg_xlog_replay_pause(), and pg_xlog_replay_resume() will be managed
  by the normal GRANT system and therefore we don't need a default role
  for those use-cases.

pg_rotate_logfile

  pg_rotate_logfile() will be managed by the normal GRANT system and
  therefore we don't need a default role for that use-cases.

> > Personally, I don't have any particular issue having both, but the
> > desire was stated that it would be better to have the regular
> > GRANT EXECUTE ON catalog_func() working before we consider having
> > default roles for same.  That moves the goal posts awful far though, if
> > we're to stick with that and consider how we might extend the GRANT
> > system in the future.
> 
> I don't think it moves 

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost 
wrote:
>> > Personally, I don't have any particular issue having both, but the
>> > desire was stated that it would be better to have the regular
>> > GRANT EXECUTE ON catalog_func() working before we consider having
>> > default roles for same.  That moves the goal posts awful far though, if
>> > we're to stick with that and consider how we might extend the GRANT
>> > system in the future.
>>
>> I don't think it moves the goal posts all that far.  Convincing
>> pg_dump to dump grants on system functions shouldn't be a crazy large
>> patch.
>
> I wasn't clear as to what I was referring to here.  I've already written
> a patch to pg_dump to support grants on system objects and agree that
> it's at least reasonable.

Is it already posted somewhere? I don't recall seeing it. Robert and Noah
have a point that this would be useful for users who would like to dump
GRANT/REVOKE rights on system functions & all, using a new option in
pg_dumpall, say --with-system-acl or --with-system-privileges. If at least
the three of you are agreeing here I think that we should try to move at
least toward this goal first. That seems a largely doable goal for 9.6. For
the set of default roles, there is clearly no clear consensus regarding
what each role should do or not, and under which limitation it should
operate.
-- 
Michael

Re: [HACKERS] Additional role attributes && superuser review

[David Steele]

On 1/17/16 9:10 PM, Stephen Frost wrote:
> but if it's possible to do a backup without
> being a superuser and with only read access to the data directory, I
> would expect every backup soltuion to view that as a feature which they
> want to support, as there are environments which will find it desirable,
> at a minimum, and even some which will require it.

This would certainly be a desirable feature for pgBackRest.  The fewer
processes running with the ability to alter the cluster the better.
Even if they are well-tested it's still one less thing to worry about.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Sun, Jan 17, 2016 at 6:58 PM, Stephen Frost  wrote:
> I'm not against that idea, though I continue to feel that there are
> common sets of privileges which backup tools could leverage.
>
> The other issue that I'm running into, again, while considering how to
> move back to ACL-based permissions for these objects is that we can't
> grant out the actual permissions which currently exist.  That means we
> either need to break backwards compatibility, which would be pretty
> ugly, in my view, or come up with new functions and then users will have
> to know which functions to use when.
>
> As I don't think we really want to break backwards compatibility or
> remove existing functionality, the only approach which is going to make
> sense is to add additional functions in some cases.  In particular, we
> will need alternate versions of pg_terminate_backend and
> pg_cancel_backend.  One thought I had was to make that
> 'pg_signal_backend', but that sounds like we'd allow any signal sent by
> a user with that right, which seems a bit much to me...

So, this seems like a case where a built-in role would be
well-justified.  I don't really believe in built-in roles as a way of
bundling related permissions; I know you do, but I don't.  I'd rather
see the individual function permissions granted individually.  But
here you are talking about a variable level of access to the same
function, depending on role.  And for that it seems to me that a
built-in role has a lot more to recommend it in that case than it does
in the other.  If you have been granted pg_whack, you can signal any
process on the system; otherwise just your own.  Those checks are
internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
substitute.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Wed, Jan  6, 2016 at 12:29:14PM -0500, Robert Haas wrote:
> The point is that with the GRANT EXECUTE ON FUNCTION proposal, authors
> of monitoring tools enjoy various really noteworthy advantages.  They
> can have monitoring roles which have *exactly* the privileges that
> their tool needs, not whatever set of permissions (larger or smaller)
> the core project has decide the pg_monitor role should have.  They can
> have optional features requiring extra permissions and those extra
> permissions can be granted in precisely those shops where those extra
> features are in use.  They can deploy a new versions of their
> monitoring tool that requires an extra privilege on an existing
> PostgreSQL release without requiring any core modifications, which
> shaves years of time off the deployment schedule and avoids
> contentious arguments with the lovable folks who populate this mailing
> list.  That sounds *terrific* to me compared to the alternative you
> are proposing.

I assume backup tools would either document the functions they want
access to via SQL commands, or supply a script.  I assume they would
create a non-login role (group) with the desired permissions, and then
have users inherit from that.  They would also need to be able to allow
upgrades where they would (conditionally?) add the role and then
add/revoke permissions as needed, e.g. they might not need all
permissions they needed in a previous release, or they might need new
ones.

That all seems very straight-forward to me.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Mon, Jan  4, 2016 at 12:55:16PM -0500, Stephen Frost wrote:
> > I'd like to be able to include, in both of those, a simple set of
> > instructions for granting the necessary rights to the user who is
> > running those processes.  A set of rights which an administrator can go
> > look up and easily read and understand the result of those grants.  For
> > example:
> > 
> ...
> > pgbackrest:
> > 
> >   To run pgbackrest as a non-superuser and not the 'postgres' system
> >   user, grant the pg_backup role to the backrest user and ensure the
> >   backrest system user has read access to the database files (eg: by
> >   having the system user be a member of the 'postgres' group):
> --
> 
> Just to clarify, the 'postgres' OS user group cannot read the data
> directory, e.g.
> 
>   drwx-- 19 postgres staff 4096 Jan 17 12:19 data/
>   ^^^group
> 
> I assume we don't want to change that.

This is going to be distribution dependent, unfortunately.  On
Debian-based distributions, the group is 'postgres' and it'd be
perfectly reasonable to allow that group to read the data directory.

I don't recall offhand if that means we'd have to make changes to allow
that, but, for my 2c, I don't see why we wouldn't allow it to be an
option.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Sun, Jan 17, 2016 at 01:49:19PM -0500, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > > > pgbackrest:
> > > > 
> > > >   To run pgbackrest as a non-superuser and not the 'postgres' system
> > > >   user, grant the pg_backup role to the backrest user and ensure the
> > > >   backrest system user has read access to the database files (eg: by
> > > >   having the system user be a member of the 'postgres' group):
> > > --
> > > 
> > > Just to clarify, the 'postgres' OS user group cannot read the data
> > > directory, e.g.
> > > 
> > >   drwx-- 19 postgres staff 4096 Jan 17 12:19 data/
> > >   ^^^group
> > > 
> > > I assume we don't want to change that.
> > 
> > This is going to be distribution dependent, unfortunately.  On
> > Debian-based distributions, the group is 'postgres' and it'd be
> > perfectly reasonable to allow that group to read the data directory.
> 
> Well, while the group name would be OS-dependent, the lack of any group
> permisions in not OS-dependent and is forced by initdb:
> 
> umask(S_IRWXG | S_IRWXO);
> 
> create_data_directory();

Right, we also check in the backend on startup for certain permissions.
I don't recall offhand if that's forced to 700 or if we allow 750.

> > I don't recall offhand if that means we'd have to make changes to allow
> > that, but, for my 2c, I don't see why we wouldn't allow it to be an
> > option.
> 
> OK, that would be an initdb change then.

It would need to be optional, so distributions and users could choose
which makes sense for their systems.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Mon, Jan  4, 2016 at 12:55:16PM -0500, Stephen Frost wrote:
> I'd like to be able to include, in both of those, a simple set of
> instructions for granting the necessary rights to the user who is
> running those processes.  A set of rights which an administrator can go
> look up and easily read and understand the result of those grants.  For
> example:
> 
...
> pgbackrest:
> 
>   To run pgbackrest as a non-superuser and not the 'postgres' system
>   user, grant the pg_backup role to the backrest user and ensure the
>   backrest system user has read access to the database files (eg: by
>   having the system user be a member of the 'postgres' group):
--

Just to clarify, the 'postgres' OS user group cannot read the data
directory, e.g.

drwx-- 19 postgres staff 4096 Jan 17 12:19 data/
^^^group

I assume we don't want to change that.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Sun, Jan 17, 2016 at 01:49:19PM -0500, Stephen Frost wrote:
> * Bruce Momjian (br...@momjian.us) wrote:
> > > pgbackrest:
> > > 
> > >   To run pgbackrest as a non-superuser and not the 'postgres' system
> > >   user, grant the pg_backup role to the backrest user and ensure the
> > >   backrest system user has read access to the database files (eg: by
> > >   having the system user be a member of the 'postgres' group):
> > --
> > 
> > Just to clarify, the 'postgres' OS user group cannot read the data
> > directory, e.g.
> > 
> > drwx-- 19 postgres staff 4096 Jan 17 12:19 data/
> > ^^^group
> > 
> > I assume we don't want to change that.
> 
> This is going to be distribution dependent, unfortunately.  On
> Debian-based distributions, the group is 'postgres' and it'd be
> perfectly reasonable to allow that group to read the data directory.

Well, while the group name would be OS-dependent, the lack of any group
permisions in not OS-dependent and is forced by initdb:

umask(S_IRWXG | S_IRWXO);

create_data_directory();

> I don't recall offhand if that means we'd have to make changes to allow
> that, but, for my 2c, I don't see why we wouldn't allow it to be an
> option.

OK, that would be an initdb change then.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Sun, Jan 17, 2016 at 01:57:22PM -0500, Stephen Frost wrote:
> Right, we also check in the backend on startup for certain permissions.
> I don't recall offhand if that's forced to 700 or if we allow 750.
> 
> > > I don't recall offhand if that means we'd have to make changes to allow
> > > that, but, for my 2c, I don't see why we wouldn't allow it to be an
> > > option.
> > 
> > OK, that would be an initdb change then.
> 
> It would need to be optional, so distributions and users could choose
> which makes sense for their systems.

While the group owner of the directory is a distributions question, the
permissions are usually a backup-method-specific requirement.  I can see
us creating an SQL function that opens up group permissions on the data
directory for specific backup tools that need it, then granting
permissions on that function to the backup role.   This is another
example where different backup tools need different permissions.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Sun, Jan 17, 2016 at 06:58:25PM -0500, Stephen Frost wrote:
> I'm not against that idea, though I continue to feel that there are
> common sets of privileges which backup tools could leverage.
> 
> The other issue that I'm running into, again, while considering how to
> move back to ACL-based permissions for these objects is that we can't
> grant out the actual permissions which currently exist.  That means we

Is that because many of them are complex, e.g. you can kill only your
own sessions?

> either need to break backwards compatibility, which would be pretty
> ugly, in my view, or come up with new functions and then users will have
> to know which functions to use when.
> 
> As I don't think we really want to break backwards compatibility or
> remove existing functionality, the only approach which is going to make
> sense is to add additional functions in some cases.  In particular, we
> will need alternate versions of pg_terminate_backend and
> pg_cancel_backend.  One thought I had was to make that

Like these?  Could we define own-user-type rights?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Wed, Jan  6, 2016 at 12:29:14PM -0500, Robert Haas wrote:
> > The point is that with the GRANT EXECUTE ON FUNCTION proposal, authors
> > of monitoring tools enjoy various really noteworthy advantages.  They
> > can have monitoring roles which have *exactly* the privileges that
> > their tool needs, not whatever set of permissions (larger or smaller)
> > the core project has decide the pg_monitor role should have.  They can
> > have optional features requiring extra permissions and those extra
> > permissions can be granted in precisely those shops where those extra
> > features are in use.  They can deploy a new versions of their
> > monitoring tool that requires an extra privilege on an existing
> > PostgreSQL release without requiring any core modifications, which
> > shaves years of time off the deployment schedule and avoids
> > contentious arguments with the lovable folks who populate this mailing
> > list.  That sounds *terrific* to me compared to the alternative you
> > are proposing.
> 
> I assume backup tools would either document the functions they want
> access to via SQL commands, or supply a script.  I assume they would
> create a non-login role (group) with the desired permissions, and then
> have users inherit from that.  They would also need to be able to allow
> upgrades where they would (conditionally?) add the role and then
> add/revoke permissions as needed, e.g. they might not need all
> permissions they needed in a previous release, or they might need new
> ones.
> 
> That all seems very straight-forward to me.

I'm not against that idea, though I continue to feel that there are
common sets of privileges which backup tools could leverage.

The other issue that I'm running into, again, while considering how to
move back to ACL-based permissions for these objects is that we can't
grant out the actual permissions which currently exist.  That means we
either need to break backwards compatibility, which would be pretty
ugly, in my view, or come up with new functions and then users will have
to know which functions to use when.

As I don't think we really want to break backwards compatibility or
remove existing functionality, the only approach which is going to make
sense is to add additional functions in some cases.  In particular, we
will need alternate versions of pg_terminate_backend and
pg_cancel_backend.  One thought I had was to make that
'pg_signal_backend', but that sounds like we'd allow any signal sent by
a user with that right, which seems a bit much to me...

Perhaps that's what I'll do though, barring other suggestions.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Bruce,

* Bruce Momjian (br...@momjian.us) wrote:
> On Sun, Jan 17, 2016 at 01:57:22PM -0500, Stephen Frost wrote:
> > Right, we also check in the backend on startup for certain permissions.
> > I don't recall offhand if that's forced to 700 or if we allow 750.
> > 
> > > > I don't recall offhand if that means we'd have to make changes to allow
> > > > that, but, for my 2c, I don't see why we wouldn't allow it to be an
> > > > option.
> > > 
> > > OK, that would be an initdb change then.
> > 
> > It would need to be optional, so distributions and users could choose
> > which makes sense for their systems.
> 
> While the group owner of the directory is a distributions question, the
> permissions are usually a backup-method-specific requirement.  I can see
> us creating an SQL function that opens up group permissions on the data
> directory for specific backup tools that need it, then granting
> permissions on that function to the backup role.   This is another
> example where different backup tools need different permissions.

I don't believe we can really consider group ownership and group
permissions independently.  They really go hand-in-hand.  On
RedHat-based system, where the group is set as 'staff', you probably
don't want group permissions to be allowed.  On Debian-based systems,
where there is a dedicated 'postgres' group, group permissions are fine
to allow.

Group ownership and permissions aren't a backup-method-specific
requirement either, in my view.  I'm happy to chat with Marco (who has
said he would be weighing in on this thread when he is able to)
regarding barman, and whomever would be appropriate for BART (perhaps
you could let me know..?), but if it's possible to do a backup without
being a superuser and with only read access to the data directory, I
would expect every backup soltuion to view that as a feature which they
want to support, as there are environments which will find it desirable,
at a minimum, and even some which will require it.

Lastly, I'm pretty sure this would have to be a postgresql.conf option
as we check the permissions on the data directory on startup.  I don't
see how having an SQL function would work there as I certainly wouldn't
want to have the permissions changing on a running system.  That strikes
me as being risky without any real benefit.  Either it's safe and
acceptable to allow those rights to the group, or it isn't.  A temproary
grant really isn't helping with anything that I can see, surely there
are numerous ways to exploit such a time-based grant.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Sun, Jan 17, 2016 at 06:58:25PM -0500, Stephen Frost wrote:
> > I'm not against that idea, though I continue to feel that there are
> > common sets of privileges which backup tools could leverage.
> > 
> > The other issue that I'm running into, again, while considering how to
> > move back to ACL-based permissions for these objects is that we can't
> > grant out the actual permissions which currently exist.  That means we
> 
> Is that because many of them are complex, e.g. you can kill only your
> own sessions?

Right.

> > either need to break backwards compatibility, which would be pretty
> > ugly, in my view, or come up with new functions and then users will have
> > to know which functions to use when.
> > 
> > As I don't think we really want to break backwards compatibility or
> > remove existing functionality, the only approach which is going to make
> > sense is to add additional functions in some cases.  In particular, we
> > will need alternate versions of pg_terminate_backend and
> > pg_cancel_backend.  One thought I had was to make that
> 
> Like these?  Could we define own-user-type rights?

Interesting idea but I don't really see that being general enough that
we would want to burn a GRANT bit for it...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Sun, Jan 17, 2016 at 09:10:23PM -0500, Stephen Frost wrote:
> > While the group owner of the directory is a distributions question, the
> > permissions are usually a backup-method-specific requirement.  I can see
> > us creating an SQL function that opens up group permissions on the data
> > directory for specific backup tools that need it, then granting
> > permissions on that function to the backup role.   This is another
> > example where different backup tools need different permissions.
> 
> I don't believe we can really consider group ownership and group
> permissions independently.  They really go hand-in-hand.  On
> RedHat-based system, where the group is set as 'staff', you probably
> don't want group permissions to be allowed.  On Debian-based systems,
> where there is a dedicated 'postgres' group, group permissions are fine
> to allow.

Yes, I can see that as problematic.  Seems it would have to be something
done by the administrator from the command-line.

> Group ownership and permissions aren't a backup-method-specific
> requirement either, in my view.  I'm happy to chat with Marco (who has
> said he would be weighing in on this thread when he is able to)
> regarding barman, and whomever would be appropriate for BART (perhaps
> you could let me know..?), but if it's possible to do a backup without
> being a superuser and with only read access to the data directory, I
> would expect every backup soltuion to view that as a feature which they
> want to support, as there are environments which will find it desirable,
> at a minimum, and even some which will require it.

pg_dump doesn't need to read the PGDATA directory, and I thought this
permission was to be used by pg_dump users as well.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Bruce Momjian]

On Sun, Jan 17, 2016 at 09:23:14PM -0500, Stephen Frost wrote:
> > > Group ownership and permissions aren't a backup-method-specific
> > > requirement either, in my view.  I'm happy to chat with Marco (who has
> > > said he would be weighing in on this thread when he is able to)
> > > regarding barman, and whomever would be appropriate for BART (perhaps
> > > you could let me know..?), but if it's possible to do a backup without
> > > being a superuser and with only read access to the data directory, I
> > > would expect every backup soltuion to view that as a feature which they
> > > want to support, as there are environments which will find it desirable,
> > > at a minimum, and even some which will require it.
> > 
> > pg_dump doesn't need to read the PGDATA directory, and I thought this
> > permission was to be used by pg_dump users as well.
> 
> No.  That has been a source of confusion, though I'm not quite sure how
> or why, beyond the general assumption that anything 'backup' must
> include 'pg_dump' (I don't generally consider that to be the case,
> myself, but it seems others do...).

I think the source of that is that many people have asked for
backup-only uses, and I thought running pg_dump or pg_dumpall was one of
those cases.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Sun, Jan 17, 2016 at 09:23:14PM -0500, Stephen Frost wrote:
> > > > Group ownership and permissions aren't a backup-method-specific
> > > > requirement either, in my view.  I'm happy to chat with Marco (who has
> > > > said he would be weighing in on this thread when he is able to)
> > > > regarding barman, and whomever would be appropriate for BART (perhaps
> > > > you could let me know..?), but if it's possible to do a backup without
> > > > being a superuser and with only read access to the data directory, I
> > > > would expect every backup soltuion to view that as a feature which they
> > > > want to support, as there are environments which will find it desirable,
> > > > at a minimum, and even some which will require it.
> > > 
> > > pg_dump doesn't need to read the PGDATA directory, and I thought this
> > > permission was to be used by pg_dump users as well.
> > 
> > No.  That has been a source of confusion, though I'm not quite sure how
> > or why, beyond the general assumption that anything 'backup' must
> > include 'pg_dump' (I don't generally consider that to be the case,
> > myself, but it seems others do...).
> 
> I think the source of that is that many people have asked for
> backup-only uses, and I thought running pg_dump or pg_dumpall was one of
> those cases.

I'd want that to be a seperate permission which would really be
something along the lines of "allowed to read everything through a DB
connection", which is what pg_dump/pg_dumpall need.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Robert, Noah,

I just wanted to start off by saying thank you for taking the time read
and comment with your thoughts on this concept.  I was a bit frustrated
about it feeling rather late, but appreciate the comments which have
been made as they've certainly been constructive.

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jan 4, 2016 at 4:56 PM, Stephen Frost  wrote:
> >> First, it's not really going to matter to users very much whether the
> >> command to enable one of these features is a single GRANT command or a
> >> short sequence of GRANT commands executed one after another.  So even
> >> if we don't have roles that bundle together multiple permissions, you
> >> will still be able to do this; you just might need more than one
> >> command.  Probably, I'm guessing, not very many commands, but more
> >> than one.
> >
> > I disagree.  I find that it does make a difference to users to be using
> > a well documented and clear approach over one which involves fiddling
> > with the individual pieces to get everything to work, and if a new
> > function comes along that is useful for backup users, that would have to
> > also be granted, even if it would clearly be useful to a backup role.
> 
> How is that a fair way to characterize the discussion here?  Just
> because you have to execute several SQL commands instead of one
> doesn't turn a "well-documented and clear approach" into something
> that involves "fiddling with individual pieces".  Cutting and pasting
> a sequence of 3 or 4 SQL commands into a psql window is not a lot
> harder than copying and pasting a single one, and does not turn a good
> approach into a shambles.

I was looking at it from a perspective of what we have currently vs.
what the future state with default roles would be.  That's not an
entirely fair characterization.  If we supported ACLs on catalog objects
via pg_dump, then we could add documentation along the lines of "backups
generally need access to functions X, Y, Z, here's an example of how to
create such a role: blah, blah."

Of course, that documentation would likely have to be repeated in the
various backup tools, though it's possible they could tailor those, if
there was something different about their particular tool.

> >> Second, I think it's really unlikely that you're going to maintain
> >> perfect alignment between your predefined roles and the permissions
> >> that third-party tools need over the course of multiple releases.  I
> >> think the chances of that working out are just about zero.  Sure, you
> >> can make the initial set of permissions granted to pg_backup match
> >> exactly what pgbackrest needs, but it's a good bet that one of the six
> >> or eight widely-used backup tools uses something that's not included
> >> in that set.
> >
> > There may be something I've missed with the proposed pg_backup role, but
> > I don't believe you're correct that there isn't a set of privileges
> > which all of those backup tools need and which could be provided through
> > the pg_backup role.
> 
> Well, there's certainly some set of privileges that will make them all
> work.  But it might include more than some of them want.  If you done
> any analysis of this, I have not seen it posted to this thread.

I can certainly work up a formal analysis and submit it for
consideration.

> >> And even if not, it doesn't require very much
> >> imagination to suppose that some tool, maybe pgbackrest or maybe
> >> something else, that comes along over the next few releases will
> >> require some extra permission.  When that happens, will we include add
> >> that permission to the pg_backup role, or not?
> >
> > As I pointed out previously, we've already been doing this with the
> > replication role attribute and I don't recall any complaining about it.
> 
> 1. This doesn't really seem like the same thing.  You're introducing
> something quite new here: these are not role attributes that apply
> only to the role itself, but inheritable role attributes.

This approach started out by adding role attributes to handle these
kinds of rights, but in discussion with Tom and Magnus, iirc (no, I
don't have the specific links or threads, though I have asked Magnus to
take a look at this thread, as he has time), the idea of default roles
seemed better specifically because they would then be inheritable and
granting access could also be delegated.

> 2. I believe that the discussion about what the replication role
> should and should not allow involved considerably more people than
> have discussed any of the specific roles you propose to add here.

I didn't intend to dispute that, but...

> 3. It was clear from the outset that the replication role's basic
> purpose was to be sufficient privilege for a streaming standby and no
> more.  The remit of these roles is a lot less clear, at least to me.

I've certainly intended the intention of these roles to be clear and
documented.  The point I was trying to address above is 

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Wed, Jan 6, 2016 at 11:13 AM, Stephen Frost  wrote:
> I just wanted to start off by saying thank you for taking the time read
> and comment with your thoughts on this concept.  I was a bit frustrated
> about it feeling rather late, but appreciate the comments which have
> been made as they've certainly been constructive.

Thanks.

>> Well, there's certainly some set of privileges that will make them all
>> work.  But it might include more than some of them want.  If you done
>> any analysis of this, I have not seen it posted to this thread.
>
> I can certainly work up a formal analysis and submit it for
> consideration.

I would be in favor of that.

>> 1. This doesn't really seem like the same thing.  You're introducing
>> something quite new here: these are not role attributes that apply
>> only to the role itself, but inheritable role attributes.
>
> This approach started out by adding role attributes to handle these
> kinds of rights, but in discussion with Tom and Magnus, iirc (no, I
> don't have the specific links or threads, though I have asked Magnus to
> take a look at this thread, as he has time), the idea of default roles
> seemed better specifically because they would then be inheritable and
> granting access could also be delegated.

I agree that predefined roles are better than additional role
attributes.  I don't agree that predefined roles are better than GRANT
EXECUTE ON FUNCTION pg_catalog.blah().  I think full support for GRANT
EXECUTE ON FUNCTION pg_catalog.blah() is undeniably useful and will be
a very clear improvement over what we have now.  I think predefined
roles are a reasonable thing for somebody to want, but I don't think
it's nearly as clear-cut, and I'm very much unconvinced that we know
that the ones you're proposing are the right ones.

>> 3. It was clear from the outset that the replication role's basic
>> purpose was to be sufficient privilege for a streaming standby and no
>> more.  The remit of these roles is a lot less clear, at least to me.
>
> I've certainly intended the intention of these roles to be clear and
> documented.  The point I was trying to address above is that we
> certainly appear fine to add additional privileges as new capabilities
> are added to existing role attributes (the entire logical replication
> system was added after the replication role attribute).

Mmmph.  I think we got lucky there as much as anything.  replication
already allows sufficient privilege to extract all data in the
database, so allowing it to also request a logical change stream isn't
really a privilege escalation.  That's not going to be true for what
you are proposing here.

>> > Adding pg_dump dump and restore support for catalog ACLs implies that
>> > we're supporting ACLs on all catalog objects- including tables.
>>
>> Not to me it doesn't.  I think we could support it just for functions,
>> and have it continue to be as weird as it is currently for other types
>> of objects until somebody gets around to straightening that out.  If
>> we want to support it for more object types, great, but I don't think
>> that's a hard requirement.
>
> Alright, that would certainly simplify things if we're talking about
> only functions.  The only concern which I have there is if there are any
> non-function cases that we'll end up coming across, and I'm still a bit
> nervous about the "old pg_dump / new database" restore concern, but
> perhaps that's an acceptable issue.

I wouldn't worry about that a bit.  We recommend that users always
pg_dump using the newest version of pg_dump, even if the dumped server
is older.  That advice will be entirely satisfactory in this case
also.

> Based on Noah's response (which I respond to below), we seem to still
> be debating the whole concept of default roles.  I'm happy to provide
> detailed analysis if we're able to agree on the concept.

I think you need to provide detailed analysis SO THAT we can agree on
the concept.

>> Oh, sure: they are not backup tools specifically.  But anything that
>> might need elevated privileges deserves consideration here: what sort
>> of subdivision of the superuser role would make that need go away?
>
> The general approach which I've been using for the default roles is that
> they should grant rights which aren't able to be used to trivially make
> oneself a superuser.

That's a good principle, but not a sufficient guide.

>> Stop.  Even if PostgreSQL introduces pg_monitor, check_postgres will do 
>> itself
>> a favor by not using it.  The moment the two projects' sense of monitoring
>> privileges falls out of lockstep, benefits from using pg_monitor go negative.
>> check_postgres should instead furnish a script that creates a role holding
>> required privileges and/or SECURITY DEFINER helpers.  If check_postgres 
>> starts
>> using an object, say pgstattuple, it will wish to use it in all versions.
>> Since PostgreSQL will change pg_monitor privileges in major releases only,
>> 

Re: [HACKERS] Additional role attributes && superuser review

[Noah Misch]

On Mon, Jan 04, 2016 at 12:55:16PM -0500, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:
> > On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:

> I'm approaching this largely from a 3rd-party application perspective.
> There are two examples off-hand which I'm considering:
> 
> check_postgres
> pgbackrest
> 
> I'd like to be able to include, in both of those, a simple set of
> instructions for granting the necessary rights to the user who is
> running those processes.  A set of rights which an administrator can go
> look up and easily read and understand the result of those grants.  For
> example:
> 
> check_postgres:
> 
>   Most check_postgres sub-commands can be run without superuser
>   privileges by granting the pg_monitor role to the monitoring user:
> 
>   GRANT pg_monitor TO monitor;
> 
>   For information regarding the pg_monitor role, see:
>   http://www.postgresql.org/docs/current/static/roles/database-roles.html

Stop.  Even if PostgreSQL introduces pg_monitor, check_postgres will do itself
a favor by not using it.  The moment the two projects' sense of monitoring
privileges falls out of lockstep, benefits from using pg_monitor go negative.
check_postgres should instead furnish a script that creates a role holding
required privileges and/or SECURITY DEFINER helpers.  If check_postgres starts
using an object, say pgstattuple, it will wish to use it in all versions.
Since PostgreSQL will change pg_monitor privileges in major releases only,
check_postgres would wait 6-18 months to use a privilege in just one of five
supported versions.  If PostgreSQL hackers ever disagree with check_postgres
hackers about whether a privilege belongs in the pg_monitor role, then
check_postgres will wish it had never used pg_monitor.  For a sample
controversy, some monitoring tool may well call pg_read_file, but that's
arguably too much power to be giving _every_ monitoring tool.

By the way, the pg_monitor role you were ready to commit does not cover
today's check_postgres needs.  Among restricted objects, check_postgres uses
at least pg_ls_dir, pg_stats, pg_settings, and pg_stat_activity.  Having said
that, it remains premature to design predefined roles.  It would be a waste to
mobilize such a design effort with GRANT's limitation clouding the issue.

> and, to reiterate what I said above, I'd rather have one abstraction for
> these kinds of permissions instead of a mish-mash of instructions.  The
> difference I can imagine being between:
> 
> For backups and monitoring, you can use default roles:
> 
> GRANT pg_backup,pg_monitor to new_admin;
> 
> but for other regular privileges such as rotating logfiles, or sending
> signals to other processes, you have to explicitly GRANT permissions:
> 
> GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO new_admin;
> GRANT EXECUTE ON FUNCTION pg_signal_backend() TO new_admin;

I don't mind having those two ways to transfer privilege.  If I have to settle
for one or the other, I pick the latter.

> > > Further, I'm not convinced that adding support for dumping ACLs or, in
> > > general, encouraging users to define their own ACLs on catalog objects
> > > is a good idea.  We certainly have no mechanism in place today for those
> > > ACLs to be respected by SysCache and encouraging their use when we won't
> > > actually respect them is likely to be confusing.
> > 
> > What's this problem with syscache?  It sounds important.
> 
> CREATE TABLE and DROP TABLE aren't going to care one bit if you have
> access to pg_class or not.  The same goes for basically everything else.
> 
> If we really want to support ACLs on the catalog, we'd have to either
> caveat that none of the internal lookups will respect them or revamp
> SysCache and any other direct catalog access to do permission checks
> first, which I don't think we really want to do.

Oh, that.  Having internal lookups ignore the ACLs is more good than bad, and
users have little cause to expect something different.  You don't need INSERT
on pg_attribute to add a column, so why expect lack of SELECT on pg_attribute
to prevent dropping one?  I might document it like so:

While GRANT can modify the privileges of a system catalog table, that
affects only queries that address the catalog as an SQL table.  Internal,
system access to the same underlying data will proceed normally.  For
example, "REVOKE SELECT ON pg_proc FROM PUBLIC" does not preclude calling
functions or even preclude passing them to pg_get_functiondef.  It does
block queries that name pg_proc in a FROM clause.

> This entire discussion of privileges-on-catalog-objects should really
> also consider the ongoing discussion about providing policies for the
> catalog via RLS.  If we start pg_dump'ing the ACLs of catalog objects
> then we'd, presumably, also want to pg_dump out any policies defined
> against catalog objects.

I would have no qualms supporting ACL changes while not supporting added
policies, indexes, 

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Dec 31, 2015 at 4:26 PM, Noah Misch  wrote:
> > The proposed pg_replication role introduces abstraction that could, as you
> > hope, spare a DBA from studying sets of functions to grant together.  The
> > pg_rotate_logfile role, however, does not shield the DBA from complexity.
> > Being narrowly tied to a specific function, it's just a suboptimal spelling 
> > of
> > GRANT.  The gap in GRANT has distorted the design for these predefined 
> > roles.
> > I do not anticipate a sound design discussion about specific predefined 
> > roles
> > so long as the state of GRANT clouds the matter.
> 
> As the patch stands, the system roles that are just close to synonyms
> of GRANT/REVOKE are the following:
> - pg_file_settings, which works just on the system view
> pg_file_settings and the function pg_show_all_file_settings().
> - pg_rotate_logfile as mentioned already.

The above are fine, I believe.

> - pg_signal_backend, which is just checked once in pg_signal_backend

This is a bit more complicated.  pg_signal_backend() isn't directly
exposed, pg_cancel_backend() and pg_termiante_backend() are.  I'm not
saying that doesn't mean we should, necessairly, keep the
pg_signal_backend role, just that it's more than just a single function.

Further, pg_terminate_backend and pg_cancel_backend are callable by
anyone today.  We'd have to invent new functions or change user-visible
behavior in an unfortunate way- we don't want *anyone* to be able to
call those functions on *anyone* unless they've been specifically
granted that access.  Today, you can cancel and/or terminate your own
backends and superusers can cancel and/or termiante anyone's.  The point
of pg_signal_backend was to allow non-superusers to cancel and/or
terminate any non-superuser-started backends.  With the default role
approach, we can provide exactly the intended semantics and not break
backwards compatibility for those functions.

> Based on those concerns, it looks clear that they should be shaved off
> from the patch.

I'd like to hear back regarding the summary email that I sent to Noah
and the follow-up I sent to Robert, as they have time to reply, of
course.  It's certainly easy enough to remove those default roles if
that's the consensus though.

> >> > To summarize, I think the right next step is to resume designing pg_dump
> >> > support for system object ACLs.  I looked over your other two patches 
> >> > and will
> >> > unshelve those reviews when their time comes.
> >>
> >> To be clear, I don't believe the two patches are particularly involved
> >> with each other and don't feel that one needs to wait for the other.
> >
> > Patch 2/3 could stand without patch 3/3, but not vice-versa.  It's patch 2/3
> > that makes pg_dumpall skip ^pg_ roles, and that must be in place no later 
> > than
> > the first patch that adds a predefined ^pg_ role.
> 
> I am a bit confused by this statement, and I disagree with Stephen's
> point as well. It seems to me that 2/3 exists *because* 3/3 is here.
> Why would we want to restrict the role names that can be used if we
> are not going to introduce some system roles that are created at
> bootstrap?

My comment was, evidently, not anywhere near clear enough.  The two
patches I was referring to were the pg_dump-support-for-catalog-ACLs and
the default-roles patches.  Apologies for the confusion there.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Dec 29, 2015 at 5:35 AM, Stephen Frost  wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> >> > Updated patch attached.  I'll give it another good look and then commit
> >> > it, barring objections.
> >>
> >> This thread and its satellite[1] have worked their way through a few 
> >> designs.
> >> At first, it was adding role attributes, alongside existing attributes like
> >> REPLICATION and BYPASSRLS.  It switched[2] to making pg_dump preserve ACLs 
> >> on
> >> system objects.  Built-in roles joined[3] the pg_dump work to offer 
> >> predefined
> >> collections of ACL grants.  Finally, it dropped[4] the pg_dump side and
> >> hard-coded the roles into the features they govern.
> >
> > Correct, after quite a bit of discussion and the conclusion that, while
> > pg_dump support for dumping ACLs might be interesting, it was quite a
> > bit more complex an approach than this use-case justified.
> 
> Hmm.  I don't think I agree with that conclusion.  Who were the
> participants in that discussion, and how many people spoke in favor
> from moving on from that proposal - which I rather liked - to what
> you've got now?  Do you have links to the relevant portion of the
> relevant thread?

I'm not sure it's entirely relevant now- I've outlined the reasoning in
my email to Noah as a, hopefully, pretty comprehensive summary.  If that
doesn't sway your minds then it seems unlikely that a reference to a
thread from 6 months or a year ago would.  Further, as happens, other
discussions were in person where I discussed the ideas with other
hackers at conferences.  I got generally positive responses to the idea
of default roles with specific sets of rights, which is the path that
I've been on since.  As with most decisions, there was not a formal vote
over the proposals for me to reference.  I do specifically recall the
opinion that sets of privileges probably make more sense than granting
out individual ones, from Tom, if I'm remembering correctly.

In any case, I can work on the pg_dump support for catalog ACLs if there
is agreement now on that being the direction to go in.  If there's
agreement that we are happy with the idea of default roles also then I
can strip out those few which are only one-permission and which
therefore wouldn't be necessary, if we had ACL support on catalog
objects, quite easily.

> I think it's not a very good thing if we add roles that allow, say,
> execution of exactly one SQL function.  The
> dump-grants-on-system-objects proposal would accomplish the same thing
> in a much more flexible way, and with less catalog clutter.

For my 2c, I don't see a default role or two as creating terribly much
clutter.  Changing all of our code that currently has internal checks to
rely on the external checks and adjusting the permissions on the
individual functions will be a fair bit of churn though.

> More
> broadly, I'm not very convinced that even the roles which allow for
> rights on multiple objects are going to meet with general approval.

There's been rather little oposition to the idea and support when I've
discussed it.  Of course, that was before it got to the point where I
was planning to commit it.  Perhaps there will be once it's actually in,
or maybe not until it's in the wild.  In any case, I continue to feel,
as others have, that we can make adjustments moving forward.

> There has been enough discussion of which roles should be created and
> which things should be included in each one, and the overall tenor of
> that discussion seems to be that different people have different
> ideas.

Michael had a question about pg_switch_xlog, but he appeared to
reconsider that position after the subsequent discussion, which put us
back to essentially the same proposal that I started with, I believe.  I
don't recall terribly much other discussion or concern about what roles
should be created or what should be included in each one, though I'd be
happy to hear your thoughts on what you'd like to see.

I certainly don't like the idea of punting on all of this to the user to
figure out, even if it does meet the specific requirements our clients
have asked for, it strikes me that we can do better.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Mon, Jan 4, 2016 at 12:55 PM, Stephen Frost  wrote:
> I'd like to be able to include, in both of those, a simple set of
> instructions for granting the necessary rights to the user who is
> running those processes.  A set of rights which an administrator can go
> look up and easily read and understand the result of those grants.  For
> example:
>
> check_postgres:
>
>   Most check_postgres sub-commands can be run without superuser
>   privileges by granting the pg_monitor role to the monitoring user:
>
>   GRANT pg_monitor TO monitor;
>
>   For information regarding the pg_monitor role, see:
>   http://www.postgresql.org/docs/current/static/roles/database-roles.html
>
> pgbackrest:
>
>   To run pgbackrest as a non-superuser and not the 'postgres' system
>   user, grant the pg_backup role to the backrest user and ensure the
>   backrest system user has read access to the database files (eg: by
>   having the system user be a member of the 'postgres' group):
>
>   GRANT pg_backup to backrest;
>
>   For information regarding the pg_backup role, see:
>   http://www.postgresql.org/docs/current/static/roles/database-roles.html

So I have two comments on this.

First, it's not really going to matter to users very much whether the
command to enable one of these features is a single GRANT command or a
short sequence of GRANT commands executed one after another.  So even
if we don't have roles that bundle together multiple permissions, you
will still be able to do this; you just might need more than one
command.  Probably, I'm guessing, not very many commands, but more
than one.

Second, I think it's really unlikely that you're going to maintain
perfect alignment between your predefined roles and the permissions
that third-party tools need over the course of multiple releases.  I
think the chances of that working out are just about zero.  Sure, you
can make the initial set of permissions granted to pg_backup match
exactly what pgbackrest needs, but it's a good bet that one of the six
or eight widely-used backup tools uses something that's not included
in that set.  And even if not, it doesn't require very much
imagination to suppose that some tool, maybe pgbackrest or maybe
something else, that comes along over the next few releases will
require some extra permission.  When that happens, will we include add
that permission to the pg_backup role, or not?  If we do, then we'll
be giving excess permissions to all the other backup tools that don't
need that new right, and maybe surprising users who upgrade without
realizing that some of their roles now have new rights.  If we don't,
then that tool won't be able to work without some additional
twiddling.  I just find it incredibly unlikely that every monitoring
tool, or every backup tool, or every admin tool will require exactly
the same set of permissions.

> I can see similar bits of documentation being included in pgAdmin or
> other tools.

...and pgAdmin is a particularly good example.  The permissions that
pgAdmin requires depend on what you want to do with it, and it does a
lot of things, and it might do more or fewer things in the future.
You can't even fairly assume that everyone wants to give the same
permissions to pgAdmin, let alone that some competing tool will
require the same set.

>> Being narrowly tied to a specific function, it's just a suboptimal spelling 
>> of
>> GRANT.  The gap in GRANT has distorted the design for these predefined roles.
>> I do not anticipate a sound design discussion about specific predefined roles
>> so long as the state of GRANT clouds the matter.
>
> I'm loathe to encourage any direct modification of catalog objects,
> even if it's just ACLs.  I've seen too many cases, as I imagine others
> have, of users destroying their databases or running into unexpected
> results when modifying the catalog.  The catalog modifications supported
> should be explicitly provided through other means rather than direct
> commands against the catalog objects.  I see the default roles approach
> as being similar to having:
>
> ALTER DATABASE db WITH CONNECTION LIMIT 5;
>
> instead of suggesting users issue:
>
> UPDATE DATABASE SET datconnlimit = 5 WHERE datname = 'db';

This doesn't make any sense to me.  Nobody was proposing issuing an
UPDATE against pg_database directly (and it would have to be
pg_database, not just database as you wrote here).  We're talking
about whether the user is going to GRANT pg_rotate_logfile TO ... or
GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO ... which is not the
same sort of thing at all.

>> What's this problem with syscache?  It sounds important.
>
> CREATE TABLE and DROP TABLE aren't going to care one bit if you have
> access to pg_class or not.  The same goes for basically everything else.
>
> If we really want to support ACLs on the catalog, we'd have to either
> caveat that none of the internal lookups will respect them or revamp
> SysCache and any other direct catalog access to do 

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Mon, Jan 4, 2016 at 4:56 PM, Stephen Frost  wrote:
>> First, it's not really going to matter to users very much whether the
>> command to enable one of these features is a single GRANT command or a
>> short sequence of GRANT commands executed one after another.  So even
>> if we don't have roles that bundle together multiple permissions, you
>> will still be able to do this; you just might need more than one
>> command.  Probably, I'm guessing, not very many commands, but more
>> than one.
>
> I disagree.  I find that it does make a difference to users to be using
> a well documented and clear approach over one which involves fiddling
> with the individual pieces to get everything to work, and if a new
> function comes along that is useful for backup users, that would have to
> also be granted, even if it would clearly be useful to a backup role.

How is that a fair way to characterize the discussion here?  Just
because you have to execute several SQL commands instead of one
doesn't turn a "well-documented and clear approach" into something
that involves "fiddling with individual pieces".  Cutting and pasting
a sequence of 3 or 4 SQL commands into a psql window is not a lot
harder than copying and pasting a single one, and does not turn a good
approach into a shambles.

>> Second, I think it's really unlikely that you're going to maintain
>> perfect alignment between your predefined roles and the permissions
>> that third-party tools need over the course of multiple releases.  I
>> think the chances of that working out are just about zero.  Sure, you
>> can make the initial set of permissions granted to pg_backup match
>> exactly what pgbackrest needs, but it's a good bet that one of the six
>> or eight widely-used backup tools uses something that's not included
>> in that set.
>
> There may be something I've missed with the proposed pg_backup role, but
> I don't believe you're correct that there isn't a set of privileges
> which all of those backup tools need and which could be provided through
> the pg_backup role.

Well, there's certainly some set of privileges that will make them all
work.  But it might include more than some of them want.  If you done
any analysis of this, I have not seen it posted to this thread.

>> And even if not, it doesn't require very much
>> imagination to suppose that some tool, maybe pgbackrest or maybe
>> something else, that comes along over the next few releases will
>> require some extra permission.  When that happens, will we include add
>> that permission to the pg_backup role, or not?
>
> As I pointed out previously, we've already been doing this with the
> replication role attribute and I don't recall any complaining about it.

1. This doesn't really seem like the same thing.  You're introducing
something quite new here: these are not role attributes that apply
only to the role itself, but inheritable role attributes.

2. I believe that the discussion about what the replication role
should and should not allow involved considerably more people than
have discussed any of the specific roles you propose to add here.

3. It was clear from the outset that the replication role's basic
purpose was to be sufficient privilege for a streaming standby and no
more.  The remit of these roles is a lot less clear, at least to me.

> I wasn't suggesting that we give everyone the same privileges to some
> default 'pgAdmin' role but rather that providing an abstraction of the
> set of privileges possible against the catalog objects, into sets that
> make sense together, is a useful simplification for users and that it'd
> be a better approach than asking users to figure out what sets make
> sense on their own.

I have no objection to that *in theory*.  What's not clear to me is
that the way that you have broken it up actually meets the bona fide
needs of actual tools in a useful way.

> Adding pg_dump dump and restore support for catalog ACLs implies that
> we're supporting ACLs on all catalog objects- including tables.

Not to me it doesn't.  I think we could support it just for functions,
and have it continue to be as weird as it is currently for other types
of objects until somebody gets around to straightening that out.  If
we want to support it for more object types, great, but I don't think
that's a hard requirement.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jan 4, 2016 at 12:55 PM, Stephen Frost  wrote:
> > I'd like to be able to include, in both of those, a simple set of
> > instructions for granting the necessary rights to the user who is
> > running those processes.  A set of rights which an administrator can go
> > look up and easily read and understand the result of those grants.  For
> > example:
> >
> > check_postgres:
> >
> >   Most check_postgres sub-commands can be run without superuser
> >   privileges by granting the pg_monitor role to the monitoring user:
> >
> >   GRANT pg_monitor TO monitor;
> >
> >   For information regarding the pg_monitor role, see:
> >   http://www.postgresql.org/docs/current/static/roles/database-roles.html
> >
> > pgbackrest:
> >
> >   To run pgbackrest as a non-superuser and not the 'postgres' system
> >   user, grant the pg_backup role to the backrest user and ensure the
> >   backrest system user has read access to the database files (eg: by
> >   having the system user be a member of the 'postgres' group):
> >
> >   GRANT pg_backup to backrest;
> >
> >   For information regarding the pg_backup role, see:
> >   http://www.postgresql.org/docs/current/static/roles/database-roles.html
> 
> So I have two comments on this.
> 
> First, it's not really going to matter to users very much whether the
> command to enable one of these features is a single GRANT command or a
> short sequence of GRANT commands executed one after another.  So even
> if we don't have roles that bundle together multiple permissions, you
> will still be able to do this; you just might need more than one
> command.  Probably, I'm guessing, not very many commands, but more
> than one.

I disagree.  I find that it does make a difference to users to be using
a well documented and clear approach over one which involves fiddling
with the individual pieces to get everything to work, and if a new
function comes along that is useful for backup users, that would have to
also be granted, even if it would clearly be useful to a backup role.

> Second, I think it's really unlikely that you're going to maintain
> perfect alignment between your predefined roles and the permissions
> that third-party tools need over the course of multiple releases.  I
> think the chances of that working out are just about zero.  Sure, you
> can make the initial set of permissions granted to pg_backup match
> exactly what pgbackrest needs, but it's a good bet that one of the six
> or eight widely-used backup tools uses something that's not included
> in that set.

There may be something I've missed with the proposed pg_backup role, but
I don't believe you're correct that there isn't a set of privileges
which all of those backup tools need and which could be provided through
the pg_backup role.

> And even if not, it doesn't require very much
> imagination to suppose that some tool, maybe pgbackrest or maybe
> something else, that comes along over the next few releases will
> require some extra permission.  When that happens, will we include add
> that permission to the pg_backup role, or not?  If we do, then we'll
> be giving excess permissions to all the other backup tools that don't
> need that new right, and maybe surprising users who upgrade without
> realizing that some of their roles now have new rights.  If we don't,
> then that tool won't be able to work without some additional
> twiddling.  I just find it incredibly unlikely that every monitoring
> tool, or every backup tool, or every admin tool will require exactly
> the same set of permissions.

As I pointed out previously, we've already been doing this with the
replication role attribute and I don't recall any complaining about it.

This discussion started out with the idea of adding more role
attributes to address this need to break out superuser rights, as we
have done in the past, and then moved to discussing default roles
instead because it's a better solution for abstracting permissions as
roles are more easily grantable, delegation of role granting can be
done, and role membership works with inheritance.

The arguments you raise here would apply nearly the same to the
replication role attribute, but in practice, we don't seem to have any
question regarding how that's handled, nor have we gotten complaints
about it.  I expect the same would be true with default roles and don't
see any particular reason to fear otherwise.

> > I can see similar bits of documentation being included in pgAdmin or
> > other tools.
> 
> ...and pgAdmin is a particularly good example.  The permissions that
> pgAdmin requires depend on what you want to do with it, and it does a
> lot of things, and it might do more or fewer things in the future.
> You can't even fairly assume that everyone wants to give the same
> permissions to pgAdmin, let alone that some competing tool will
> require the same set.

I wasn't suggesting that we give everyone the same privileges to 

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Mon, Jan 4, 2016 at 5:22 PM, Stephen Frost  wrote:
>> So, is this another case where the support is all in off-list fora and
>> thus invisible, or can you point to specific on-list discussions where
>> it was supported, and to the opinions offered in support?  I don't
>> really remember many opinions that were any more positive than "I
>> wouldn't be strongly opposed to this" or "If we're going to do this
>> then we ought to do it in X way".  I'm happy to be corrected if I'm
>> misrepresenting the record, but I'd characterize the overall reaction
>> to this proposal as tepid: nobody hated it, but nobody really loved it
>> either, and a bunch of mild concerns were offered.
>
> I agree that this has largely been the on-list reaction.  To be fair,
> it's been largely the off-list reaction also, which I've expressly
> tried to seek out, as mentioned above.  I'm not asking anyone to love
> it, I'm not entirely convinced it's lovable myself, but I do feel it's
> useful and worth making an effort for.

I think the question of whether the specific proposals on the table
are in fact useful is one that deserves more study.  I am not
convinced of that.  I believe something like this could be useful, but
I don't see a lot of evidence that the particular roles you're arguing
for actually are.

> I'd love to have folks from other companies involved in these
> discussions.  I'll even reach out explicitly to seek their comment, as
> I've done with other hackers at conferences, and try to get them to
> voice their opinions here.

Great, thanks.

>> What really bothers me about this thread is that these predefined
>> roles are intended to be useful for third-party tools, but the people
>> who maintain those third-party tools have said basically nothing.
>
> For my 2c, I believe that to be, by-and-large, because they don't want
> to get their hopes up until they see something actually get committed.
> Following long and deep threads such as these are quite a committment.

Yep.

>> I
>> don't recall, for example, Dave Page weighing in on what pgAdmin
>> needs, or anybody commenting on to what degree any of these proposals
>> would meet the needs of Slony or pgBouncer or pgPool or any backup
>> tool (other than perhaps pgbackrest, which I assume your proposals
>> cater to) or any monitoring tool.  Like, we've heard zip.  Either
>> those people don't know this thread exists, or they can't understand
>> it, or they think it's so boring that they can't be bothered to write
>> in and say whether this is useful or not.  I'd have a lot more
>> confidence that we are making a good decision if some of those people
>> would show up and say "I have reviewed this proposal and it looks {
>> great | terrible | mediocre } for $TOOL because $REASON".
>
> We *have* heard complaints from people, multiple times on various lists,
> that they'd like to set up check_postgres, Nagios, $MONITORINGTOOL, with
> a role that *isn't* a superuser.

True.  But we should verify that this proposal actually meets those
needs, not just assume it does.

> I'll ask Greg S-M if he would have
> time to weigh in on this though, check_postgres was specifically one of
> the tools which I was looking at when considering the pg_monitor role.

OK, that sounds like a good idea.

> I'm not sure about the references you use above to Slony or pgBouncer or
> pgPool as those aren't backup tools, to my mind..  I would expect barman
> and other backup tools to also use pg_start/stop_backup and
> pg_switch_xlog.  I'm not sure that there's a way to cater to one backup
> role when it comes to how filesystem-level backups are handled in PG,
> but perhaps I've missed something there that barman uses and which isn't
> included currently.

Oh, sure: they are not backup tools specifically.  But anything that
might need elevated privileges deserves consideration here: what sort
of subdivision of the superuser role would make that need go away?

> Of course, my reviewing barman or other tools wouldn't have the same
> support as Simon weighing in, so I'll try and pursue that avenue as
> well.

Cool.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Mon, Jan 4, 2016 at 3:07 PM, Stephen Frost  wrote:
> I'm not sure it's entirely relevant now- I've outlined the reasoning in
> my email to Noah as a, hopefully, pretty comprehensive summary.  If that
> doesn't sway your minds then it seems unlikely that a reference to a
> thread from 6 months or a year ago would.  Further, as happens, other
> discussions were in person where I discussed the ideas with other
> hackers at conferences.  I got generally positive responses to the idea
> of default roles with specific sets of rights, which is the path that
> I've been on since.  As with most decisions, there was not a formal vote
> over the proposals for me to reference.  I do specifically recall the
> opinion that sets of privileges probably make more sense than granting
> out individual ones, from Tom, if I'm remembering correctly.

To be honest, that's not really my point.  You have cited off-list
discussions you've had over and over as a reason why you proceeded
down some particular path.  That is fine up to a point - I discuss
lots of things with people off-list, too - but a consensus that a
particular design is acceptable for commit has to mean the consensus
on this mailing list, and nothing else.  In seven years of reading
this mailing list, I can't remember a single other person on this
mailing list saying "I'm going to commit this because I talked to a
bunch of unspecified people at conferences I attended and they liked
it", but you've used essentially that rationale a couple of times now.

> For my 2c, I don't see a default role or two as creating terribly much
> clutter.

I don't believe any version of this proposal has involved only one or
two such roles.  They've all had at least four or five AFAICS.  Now
maybe that's still OK, but 4 or 5 > 1 or 2, and the number is only
likely to go up in the future.

> Changing all of our code that currently has internal checks to
> rely on the external checks and adjusting the permissions on the
> individual functions will be a fair bit of churn though.

I'm not sure it'll really be that bad, but if you have some evidence
that I'm wrong I'd like to hear it.

>> More
>> broadly, I'm not very convinced that even the roles which allow for
>> rights on multiple objects are going to meet with general approval.
>
> There's been rather little oposition to the idea and support when I've
> discussed it.  Of course, that was before it got to the point where I
> was planning to commit it.  Perhaps there will be once it's actually in,
> or maybe not until it's in the wild.  In any case, I continue to feel,
> as others have, that we can make adjustments moving forward.

So, is this another case where the support is all in off-list fora and
thus invisible, or can you point to specific on-list discussions where
it was supported, and to the opinions offered in support?  I don't
really remember many opinions that were any more positive than "I
wouldn't be strongly opposed to this" or "If we're going to do this
then we ought to do it in X way".  I'm happy to be corrected if I'm
misrepresenting the record, but I'd characterize the overall reaction
to this proposal as tepid: nobody hated it, but nobody really loved it
either, and a bunch of mild concerns were offered.

>> There has been enough discussion of which roles should be created and
>> which things should be included in each one, and the overall tenor of
>> that discussion seems to be that different people have different
>> ideas.
>
> Michael had a question about pg_switch_xlog, but he appeared to
> reconsider that position after the subsequent discussion, which put us
> back to essentially the same proposal that I started with, I believe.  I
> don't recall terribly much other discussion or concern about what roles
> should be created or what should be included in each one, though I'd be
> happy to hear your thoughts on what you'd like to see.

Honestly, my vote is for creating only those predefined roles that are
clearly endorsed by three people with different employers, which I
currently believe to be true of none of the proposed roles.  It's not
even that I suspect you or anyone of ballot-stuffing; it's just that
people who work at different companies are likely to work with
different tools and those tools may have different requirements.  I
mean, people at 2ndQuadrant probably mostly use repmgr; people at
Crunchy probably like pgbackrest; people at OmniTI probably use
PITRtools; and EnterpriseDB employees are more likely than average to
be familiar with BART.  If several of those people come together and
say they all agree that predefined role X is perfect for the needs of
all of their respective tools, I'd consider that a really good sign
that we've hit on something that is of general utility.  Otherwise,
I'd just the authors of each tool specify the GRANT EXECUTE ON
FUNCTION lines necessary for their own tool and call it good.  I think
that's almost as convenient and a lot more flexible.

What 

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jan 4, 2016 at 3:07 PM, Stephen Frost  wrote:
> > I'm not sure it's entirely relevant now- I've outlined the reasoning in
> > my email to Noah as a, hopefully, pretty comprehensive summary.  If that
> > doesn't sway your minds then it seems unlikely that a reference to a
> > thread from 6 months or a year ago would.  Further, as happens, other
> > discussions were in person where I discussed the ideas with other
> > hackers at conferences.  I got generally positive responses to the idea
> > of default roles with specific sets of rights, which is the path that
> > I've been on since.  As with most decisions, there was not a formal vote
> > over the proposals for me to reference.  I do specifically recall the
> > opinion that sets of privileges probably make more sense than granting
> > out individual ones, from Tom, if I'm remembering correctly.
> 
> To be honest, that's not really my point.  You have cited off-list
> discussions you've had over and over as a reason why you proceeded
> down some particular path.  That is fine up to a point - I discuss
> lots of things with people off-list, too - but a consensus that a
> particular design is acceptable for commit has to mean the consensus
> on this mailing list, and nothing else.  In seven years of reading
> this mailing list, I can't remember a single other person on this
> mailing list saying "I'm going to commit this because I talked to a
> bunch of unspecified people at conferences I attended and they liked
> it", but you've used essentially that rationale a couple of times now.

I've found consensus among folks on the lists for far more of my commits
than I've cited off-list discussions for.  Further, consensus on these
lists is largely in the quiet, which is why I go out of my way to
attempt to engage parties who may be interested when there *isn't* much
discussion on the lists.  I'm trying to do better than simply assuming
consensus based on no feedback to the contrary.  Had I assumed minimal
response meant consensus for this patch, as is typically the norm, this
patch would have been committed six months ago.  Instead, I tried to
engage people at conferences to ensure that there really was consensus
on the approach.

> > For my 2c, I don't see a default role or two as creating terribly much
> > clutter.
> 
> I don't believe any version of this proposal has involved only one or
> two such roles.  They've all had at least four or five AFAICS.  Now
> maybe that's still OK, but 4 or 5 > 1 or 2, and the number is only
> likely to go up in the future.

The 'role or two' was under the expectation that we'd still have default
roles for the more complicated cases (pg_backup, et al) and that we
would only be removing the default role of pg_switch_xlog and
pg_file_settings (the only two which an individual GRANT could replace).

> > Changing all of our code that currently has internal checks to
> > rely on the external checks and adjusting the permissions on the
> > individual functions will be a fair bit of churn though.
> 
> I'm not sure it'll really be that bad, but if you have some evidence
> that I'm wrong I'd like to hear it.

I implemented the discussed pg_dump support for ACLs and looked at the
changes required.  I may not be remembering it entirely, but it's not
that I've not looked at it.

> >> More
> >> broadly, I'm not very convinced that even the roles which allow for
> >> rights on multiple objects are going to meet with general approval.
> >
> > There's been rather little oposition to the idea and support when I've
> > discussed it.  Of course, that was before it got to the point where I
> > was planning to commit it.  Perhaps there will be once it's actually in,
> > or maybe not until it's in the wild.  In any case, I continue to feel,
> > as others have, that we can make adjustments moving forward.
> 
> So, is this another case where the support is all in off-list fora and
> thus invisible, or can you point to specific on-list discussions where
> it was supported, and to the opinions offered in support?  I don't
> really remember many opinions that were any more positive than "I
> wouldn't be strongly opposed to this" or "If we're going to do this
> then we ought to do it in X way".  I'm happy to be corrected if I'm
> misrepresenting the record, but I'd characterize the overall reaction
> to this proposal as tepid: nobody hated it, but nobody really loved it
> either, and a bunch of mild concerns were offered.

I agree that this has largely been the on-list reaction.  To be fair,
it's been largely the off-list reaction also, which I've expressly
tried to seek out, as mentioned above.  I'm not asking anyone to love
it, I'm not entirely convinced it's lovable myself, but I do feel it's
useful and worth making an effort for.

> >> There has been enough discussion of which roles should be created and
> >> which things should be included in each one, and the overall tenor of

Re: [HACKERS] Additional role attributes && superuser review

[Alvaro Herrera]

Based on the feedback here, I have returned this patch to Needs Review
status.  (Waiting on Author would be fairer actually, since we are
waiting for an updated version.)

As far as I can make it from Noah and Robert's comments, what we would
like to see here is a way for pg_dump to output nondefault grants to
system objects; that would solve part of the need here where this patch
proposes default roles for one or two system objects.  Another part of
this patch, which could presumably be committed independently, is the
addition of default roles that are related to larger sets of system
objects.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Noah,

* Noah Misch (n...@leadboat.com) wrote:
> On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> 
> > The one argument which you've put forth for adding the complexity of
> > dumping catalog ACLs is that we might reduce the number of default
> > roles provided to the user.
> 
> Right.  If "GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" worked as
> well as it works on user-defined functions, the community would not choose to
> add a pg_rotate_logfile role holding just that one permission.

I understand that's your position, but I disagree with your conclusion.

If we're going to provide default roles, which I continue to feel is a
good approach, then I would suggest we use them as an abstraction for
the permissions which we view as senible sets of grantable rights.  I
dislike the idea of having default roles and then making an exception
for single-permission cases.

I'm approaching this largely from a 3rd-party application perspective.
There are two examples off-hand which I'm considering:

check_postgres
pgbackrest

I'd like to be able to include, in both of those, a simple set of
instructions for granting the necessary rights to the user who is
running those processes.  A set of rights which an administrator can go
look up and easily read and understand the result of those grants.  For
example:

check_postgres:

  Most check_postgres sub-commands can be run without superuser
  privileges by granting the pg_monitor role to the monitoring user:

  GRANT pg_monitor TO monitor;

  For information regarding the pg_monitor role, see:
  http://www.postgresql.org/docs/current/static/roles/database-roles.html

pgbackrest:

  To run pgbackrest as a non-superuser and not the 'postgres' system
  user, grant the pg_backup role to the backrest user and ensure the
  backrest system user has read access to the database files (eg: by
  having the system user be a member of the 'postgres' group):

  GRANT pg_backup to backrest;

  For information regarding the pg_backup role, see:
  http://www.postgresql.org/docs/current/static/roles/database-roles.html


I can see similar bits of documentation being included in pgAdmin or
other tools.  For the pg_rotate_logfile permission, specifically, we
were asked by a client about that permission with the use case being a
logrotate-type of tool, which only has access to the log files but needs
to be able to perform a rotation.  This particular client is pretty tech
savvy and I don't think they'd have a problem using GRANT EXECUTE if
that was the only option, but I can see a similar use-case with
logrotate or pgAdmin or even for regular non-superuser admins using psql
and, to reiterate what I said above, I'd rather have one abstraction for
these kinds of permissions instead of a mish-mash of instructions.  The
difference I can imagine being between:

For backups and monitoring, you can use default roles:

GRANT pg_backup,pg_monitor to new_admin;

but for other regular privileges such as rotating logfiles, or sending
signals to other processes, you have to explicitly GRANT permissions:

GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO new_admin;
GRANT EXECUTE ON FUNCTION pg_signal_backend() TO new_admin;

> > I disagree that we would.  Having a single
> > set of default roles which provide a sensible breakdown of permissions
> > is a better approach than asking every administrator and application
> > developer who is building tools on top of PG to try and work through
> > what makes sense themselves, even if that means we have a default role
> > with a small, or even only an individual, capability.
> 
> The proposed pg_replication role introduces abstraction that could, as you
> hope, spare a DBA from studying sets of functions to grant together.  The
> pg_rotate_logfile role, however, does not shield the DBA from complexity.

I disagree with the above statement.  Having to understand only one
level of abstraction (the default roles) does reduce the complexity and
means that the DBA does not have to work out if the specifc GRANT
requested by the end user would result in some other access or if there
are any unexpected issues to encounter with issuing GRANTs directly on
catalog objects- something we don't currently support, so such concern
is certainly reasonable.

> Being narrowly tied to a specific function, it's just a suboptimal spelling of
> GRANT.  The gap in GRANT has distorted the design for these predefined roles.
> I do not anticipate a sound design discussion about specific predefined roles
> so long as the state of GRANT clouds the matter.

I'm loathe to encourage any direct modification of catalog objects,
even if it's just ACLs.  I've seen too many cases, as I imagine others
have, of users destroying their databases or running into unexpected
results when modifying the catalog.  The catalog modifications supported
should be explicitly provided through other means rather than direct
commands against the 

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Dec 29, 2015 at 11:55 PM, Stephen Frost  wrote:
> > I could go either way on that, really.  I don't find namespace to be
> > confusing when used in that way, but I'll change it since others do.
> 
> It seems to me that the way patch does it is fine..

Alright, fair enough.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Thu, Dec 31, 2015 at 4:26 PM, Noah Misch  wrote:
> On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:
>> * Noah Misch (n...@leadboat.com) wrote:
>> I disagree that we would.  Having a single
>> set of default roles which provide a sensible breakdown of permissions
>> is a better approach than asking every administrator and application
>> developer who is building tools on top of PG to try and work through
>> what makes sense themselves, even if that means we have a default role
>> with a small, or even only an individual, capability.
>
> The proposed pg_replication role introduces abstraction that could, as you
> hope, spare a DBA from studying sets of functions to grant together.  The
> pg_rotate_logfile role, however, does not shield the DBA from complexity.
> Being narrowly tied to a specific function, it's just a suboptimal spelling of
> GRANT.  The gap in GRANT has distorted the design for these predefined roles.
> I do not anticipate a sound design discussion about specific predefined roles
> so long as the state of GRANT clouds the matter.

As the patch stands, the system roles that are just close to synonyms
of GRANT/REVOKE are the following:
- pg_file_settings, which works just on the system view
pg_file_settings and the function pg_show_all_file_settings().
- pg_rotate_logfile as mentioned already.
- pg_signal_backend, which is just checked once in pg_signal_backend
Based on those concerns, it looks clear that they should be shaved off
from the patch.

>> > To summarize, I think the right next step is to resume designing pg_dump
>> > support for system object ACLs.  I looked over your other two patches and 
>> > will
>> > unshelve those reviews when their time comes.
>>
>> To be clear, I don't believe the two patches are particularly involved
>> with each other and don't feel that one needs to wait for the other.
>
> Patch 2/3 could stand without patch 3/3, but not vice-versa.  It's patch 2/3
> that makes pg_dumpall skip ^pg_ roles, and that must be in place no later than
> the first patch that adds a predefined ^pg_ role.

I am a bit confused by this statement, and I disagree with Stephen's
point as well. It seems to me that 2/3 exists *because* 3/3 is here.
Why would we want to restrict the role names that can be used if we
are not going to introduce some system roles that are created at
bootstrap?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Noah Misch]

On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote:
> * Noah Misch (n...@leadboat.com) wrote:

> The one argument which you've put forth for adding the complexity of
> dumping catalog ACLs is that we might reduce the number of default
> roles provided to the user.

Right.  If "GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" worked as
well as it works on user-defined functions, the community would not choose to
add a pg_rotate_logfile role holding just that one permission.

> I disagree that we would.  Having a single
> set of default roles which provide a sensible breakdown of permissions
> is a better approach than asking every administrator and application
> developer who is building tools on top of PG to try and work through
> what makes sense themselves, even if that means we have a default role
> with a small, or even only an individual, capability.

The proposed pg_replication role introduces abstraction that could, as you
hope, spare a DBA from studying sets of functions to grant together.  The
pg_rotate_logfile role, however, does not shield the DBA from complexity.
Being narrowly tied to a specific function, it's just a suboptimal spelling of
GRANT.  The gap in GRANT has distorted the design for these predefined roles.
I do not anticipate a sound design discussion about specific predefined roles
so long as the state of GRANT clouds the matter.

> > To summarize, I think the right next step is to resume designing pg_dump
> > support for system object ACLs.  I looked over your other two patches and 
> > will
> > unshelve those reviews when their time comes.
> 
> To be clear, I don't believe the two patches are particularly involved
> with each other and don't feel that one needs to wait for the other.

Patch 2/3 could stand without patch 3/3, but not vice-versa.  It's patch 2/3
that makes pg_dumpall skip ^pg_ roles, and that must be in place no later than
the first patch that adds a predefined ^pg_ role.

> Further, I'm not convinced that adding support for dumping ACLs or, in
> general, encouraging users to define their own ACLs on catalog objects
> is a good idea.  We certainly have no mechanism in place today for those
> ACLs to be respected by SysCache and encouraging their use when we won't
> actually respect them is likely to be confusing.

What's this problem with syscache?  It sounds important.

nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Tue, Dec 29, 2015 at 5:35 AM, Stephen Frost  wrote:
> * Noah Misch (n...@leadboat.com) wrote:
>> > Updated patch attached.  I'll give it another good look and then commit
>> > it, barring objections.
>>
>> This thread and its satellite[1] have worked their way through a few designs.
>> At first, it was adding role attributes, alongside existing attributes like
>> REPLICATION and BYPASSRLS.  It switched[2] to making pg_dump preserve ACLs on
>> system objects.  Built-in roles joined[3] the pg_dump work to offer 
>> predefined
>> collections of ACL grants.  Finally, it dropped[4] the pg_dump side and
>> hard-coded the roles into the features they govern.
>
> Correct, after quite a bit of discussion and the conclusion that, while
> pg_dump support for dumping ACLs might be interesting, it was quite a
> bit more complex an approach than this use-case justified.

Hmm.  I don't think I agree with that conclusion.  Who were the
participants in that discussion, and how many people spoke in favor
from moving on from that proposal - which I rather liked - to what
you've got now?  Do you have links to the relevant portion of the
relevant thread?

I think it's not a very good thing if we add roles that allow, say,
execution of exactly one SQL function.  The
dump-grants-on-system-objects proposal would accomplish the same thing
in a much more flexible way, and with less catalog clutter.  More
broadly, I'm not very convinced that even the roles which allow for
rights on multiple objects are going to meet with general approval.
There has been enough discussion of which roles should be created and
which things should be included in each one, and the overall tenor of
that discussion seems to be that different people have different
ideas.  Under those circumstances, it seems very dubious to proceed
with this.  Michael seems to think that we can go ahead and start
changing things and sort out whatever is broken later, but that
doesn't sound like a very good plan to me.  We can change the
internals of a bad implementation later and replace it with a good
implementation, but changing user-visible behavior once people have
started relying on it is a lot harder.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Thu, Dec 31, 2015 at 1:50 AM, Robert Haas  wrote:
> Under those circumstances, it seems very dubious to proceed
> with this.  Michael seems to think that we can go ahead and start
> changing things and sort out whatever is broken later, but that
> doesn't sound like a very good plan to me.

I meant [snip]tuning those roles during this development cycle.[/snip]
But I'll just withdraw as there are enough concerns, from two
committers on top of it. My point was just to move on with this patch
in the direction where the overall consensus seemed to be at the point
I begun participating in this thread.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Tue, Dec 29, 2015 at 11:55 PM, Stephen Frost  wrote:
> I could go either way on that, really.  I don't find namespace to be
> confusing when used in that way, but I'll change it since others do.

It seems to me that the way patch does it is fine..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2015/12/23 7:23, Stephen Frost wrote:
> > Updated patch attached.  I'll give it another good look and then commit
> > it, barring objections.
> 
> Just a minor nitpick about a code comment -
> 
>  /*
> + * Check that the user is not trying to create a role in the reserved
> + * "pg_" namespace.
> + */
> +if (IsReservedName(stmt->role))
> 
> The wording may be slightly confusing, especially saying "... in ...
> namespace". ISTM, "namespace" is fairly extensively used around the code
> to mean something like "a schema's namespace".
> 
> Could perhaps be reworded as:
> 
>  /*
> + * Check that the user is not trying to create a role with reserved
> + * name, ie, one starting with "pg_".
> 
> If OK, there seems to be one more place further down in the patch with
> similar wording.

I could go either way on that, really.  I don't find namespace to be
confusing when used in that way, but I'll change it since others do.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Noah,

* Noah Misch (n...@leadboat.com) wrote:
> > Updated patch attached.  I'll give it another good look and then commit
> > it, barring objections.
> 
> This thread and its satellite[1] have worked their way through a few designs.
> At first, it was adding role attributes, alongside existing attributes like
> REPLICATION and BYPASSRLS.  It switched[2] to making pg_dump preserve ACLs on
> system objects.  Built-in roles joined[3] the pg_dump work to offer predefined
> collections of ACL grants.  Finally, it dropped[4] the pg_dump side and
> hard-coded the roles into the features they govern.

Correct, after quite a bit of discussion and the conclusion that, while
pg_dump support for dumping ACLs might be interesting, it was quite a
bit more complex an approach than this use-case justified.  Further,
adding support to pg_dump for dumping ACLs could be done independently
of default roles.

The one argument which you've put forth for adding the complexity of
dumping catalog ACLs is that we might reduce the number of default
roles provided to the user.  I disagree that we would.  Having a single
set of default roles which provide a sensible breakdown of permissions
is a better approach than asking every administrator and application
developer who is building tools on top of PG to try and work through
what makes sense themselves, even if that means we have a default role
with a small, or even only an individual, capability.

I also disagree that we won't be able to adjust the privileges granted
to a role in the future.  We have certainly made adjustments to what a
'replication' role is able to do, which has largely been in the 'more
capabilities' direction that you opine concern over.

> To summarize, I think the right next step is to resume designing pg_dump
> support for system object ACLs.  I looked over your other two patches and will
> unshelve those reviews when their time comes.

To be clear, I don't believe the two patches are particularly involved
with each other and don't feel that one needs to wait for the other.

Further, I'm not convinced that adding support for dumping ACLs or, in
general, encouraging users to define their own ACLs on catalog objects
is a good idea.  We certainly have no mechanism in place today for those
ACLs to be respected by SysCache and encouraging their use when we won't
actually respect them is likely to be confusing.  I had thought
differently at one point but my position changed during the discussion
when I realized the complexity and potential confusion it could cause
and considered that against the simplicity and relatively low cost of
having default roles.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Noah Misch]

On Tue, Dec 22, 2015 at 05:23:47PM -0500, Stephen Frost wrote:
> > >> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost  
> > >> wrote:
> > >>> Updated and rebased patch attached which takes the 'pg_switch_xlog'
> > >>> default role back out, leaving us with:
> > >>>
> > >>> pg_monitor - View privileged info
> > >>> pg_backup - start/stop backups, switch xlog, create restore points
> > >>> pg_replay - Pause/resume xlog replay on replicas
> > >>> pg_replication - Create/destroy/etc replication slots
> > >>> pg_rotate_logfile - Request logfile rotation
> > >>> pg_signal_backend - Signal other backends (cancel query/terminate)
> > >>> pg_file_settings - View configuration settings in all config files

> Updated patch attached.  I'll give it another good look and then commit
> it, barring objections.

This thread and its satellite[1] have worked their way through a few designs.
At first, it was adding role attributes, alongside existing attributes like
REPLICATION and BYPASSRLS.  It switched[2] to making pg_dump preserve ACLs on
system objects.  Built-in roles joined[3] the pg_dump work to offer predefined
collections of ACL grants.  Finally, it dropped[4] the pg_dump side and
hard-coded the roles into the features they govern.

I find pg_dump support for system object ACLs to be the best of the goals
pursued here.  [5] proposed a good division of pg_dump+roles into multiple
patches, and the pg_dump support for system object ACLs belongs as the first
of the series.  There's nothing to like about today's behavior of allowing the
GRANT but omitting it from dumps, and attacking the problem at that layer will
provide considerable admin freedom.  Starting there is important, because it
will change the roles design.  I doubt we would add role pg_rotate_logfile if
"GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" were fully usable;
likewise for other roles that would carry a single ACL.  Contrary to comments
upthread, we won't be particularly free to redefine the scope of built-in
roles later.  Removing privileges from a role will be an ordinary
compatibility break, and adding privileges will be quite sensitive.

Your first patch, to catalogs.sgml, stands alone.  May as well get that out of
the way.  To answer one question you asked a few times, reserving the pg_ role
prefix is fine.  (Whether any particular pg_foo role proposal sinks or floats
is a separate question.)

To summarize, I think the right next step is to resume designing pg_dump
support for system object ACLs.  I looked over your other two patches and will
unshelve those reviews when their time comes.

Thanks,
nm

[1] 
http://www.postgresql.org/message-id/flat/20150508042928.gp30...@tamriel.snowman.net
[2] 
http://www.postgresql.org/message-id/20150402045311.gw3...@tamriel.snowman.net
[3] 
http://www.postgresql.org/message-id/20150429144722.gy30...@tamriel.snowman.net
[4] 
http://www.postgresql.org/message-id/20150508042928.gp30...@tamriel.snowman.net
[5] 
http://www.postgresql.org/message-id/CA+TgmobH4tdccajn7VmPT-1RqBdzLYcAz5jUz4bJ=rkqs_g...@mail.gmail.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Amit Langote]

Hi,

On 2015/12/23 7:23, Stephen Frost wrote:
> Updated patch attached.  I'll give it another good look and then commit
> it, barring objections.

Just a minor nitpick about a code comment -

 /*
+ * Check that the user is not trying to create a role in the reserved
+ * "pg_" namespace.
+ */
+if (IsReservedName(stmt->role))

The wording may be slightly confusing, especially saying "... in ...
namespace". ISTM, "namespace" is fairly extensively used around the code
to mean something like "a schema's namespace".

Could perhaps be reworded as:

 /*
+ * Check that the user is not trying to create a role with reserved
+ * name, ie, one starting with "pg_".

If OK, there seems to be one more place further down in the patch with
similar wording.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Dec 22, 2015 at 2:54 PM, Amit Langote
>  wrote:
> > On 2015/12/22 14:05, Michael Paquier wrote:
> >> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost  wrote:
> >>> Updated and rebased patch attached which takes the 'pg_switch_xlog'
> >>> default role back out, leaving us with:
> >>>
> >>> pg_monitor - View privileged info
> >>> pg_backup - start/stop backups, switch xlog, create restore points
> >>> pg_replay - Pause/resume xlog replay on replicas
> >>> pg_replication - Create/destroy/etc replication slots
> >>> pg_rotate_logfile - Request logfile rotation
> >>> pg_signal_backend - Signal other backends (cancel query/terminate)
> >>> pg_file_settings - View configuration settings in all config files
> >>
> >> Thanks. This looks fine to me. I just have one single comment:
> >>
> >> +   Request logfile rotation.
> >> s/logfile/transaction log file/
> >
> > Looks correct as is. Or maybe "server's log file" as in:
> >
> > 9.26.2. Server Signaling Functions
> >
> > pg_rotate_logfile(): Rotate server's log file
> 
> You're right, this is not a WAL segment, just a normal log file. Your
> phrasing is better.

Works for me.

Updated patch attached.  I'll give it another good look and then commit
it, barring objections.

Thanks!

Stephen
From f493051be96514c0f0e178ef74d6d824f702a7c2 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 18 Nov 2015 11:50:57 -0500
Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog

Add a note to the system catalog section pointing out that while
modifying the permissions on catalog tables is possible, it's
unlikely to have the desired effect.
---
 doc/src/sgml/catalogs.sgml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 97ef618..3b7768c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -21,6 +21,17 @@
particularly esoteric operations, such as adding index access methods.
   
 
+  
+   
+Changing the permissions on objects in the system catalogs, while
+possible, is unlikely to have the desired effect as the internal
+lookup functions use a cache and do not check the permissions nor
+policies of tables in the system catalog.  Further, permission
+changes to objects in the system catalogs are not preserved by
+pg_dump or across upgrades.
+   
+  
+
  
   Overview
 
-- 
2.5.0


From dece838e3ad74549c6f07dc878c8637dc2db674d Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 30 Sep 2015 07:04:55 -0400
Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles

This will prevent users from creating roles which begin with "pg_" and
will check for those roles before allowing an upgrade using pg_upgrade.

This will allow for default roles to be provided at initdb time.
---
 doc/src/sgml/ref/psql-ref.sgml  |  8 +--
 src/backend/catalog/catalog.c   |  5 ++--
 src/backend/commands/user.c | 40 
 src/backend/utils/adt/acl.c | 41 +
 src/bin/pg_dump/pg_dumpall.c|  2 ++
 src/bin/pg_upgrade/check.c  | 40 ++--
 src/bin/psql/command.c  |  4 ++--
 src/bin/psql/describe.c |  5 +++-
 src/bin/psql/describe.h |  2 +-
 src/bin/psql/help.c |  4 ++--
 src/include/utils/acl.h |  1 +
 src/test/regress/expected/rolenames.out | 18 +++
 src/test/regress/sql/rolenames.sql  |  8 +++
 13 files changed, 166 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 6d0cb3d..76bb642 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1365,13 +1365,15 @@ testdb=
 
 
   
-\dg[+] [ pattern ]
+\dg[S+] [ pattern ]
 
 
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
 \du.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
 If the form \dg+ is used, additional information
@@ -1525,13 +1527,15 @@ testdb=
   
 
   
-\du[+] [ pattern ]
+\du[S+] [ pattern ]
 
 
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
 \dg.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern 

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost  wrote:
> Updated and rebased patch attached which takes the 'pg_switch_xlog'
> default role back out, leaving us with:
>
> pg_monitor - View privileged info
> pg_backup - start/stop backups, switch xlog, create restore points
> pg_replay - Pause/resume xlog replay on replicas
> pg_replication - Create/destroy/etc replication slots
> pg_rotate_logfile - Request logfile rotation
> pg_signal_backend - Signal other backends (cancel query/terminate)
> pg_file_settings - View configuration settings in all config files
>
> Michael, another review would be great, if you don't mind.  I'm going to
> be going through it also more closely since it sounds like we've got
> consensus on at least this initial set of default roles and rights.  If
> all looks good then I'll commit it.

Thanks. This looks fine to me. I just have one single comment:

+   Request logfile rotation.
s/logfile/transaction log file/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Tue, Dec 22, 2015 at 2:54 PM, Amit Langote
 wrote:
> On 2015/12/22 14:05, Michael Paquier wrote:
>> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost  wrote:
>>> Updated and rebased patch attached which takes the 'pg_switch_xlog'
>>> default role back out, leaving us with:
>>>
>>> pg_monitor - View privileged info
>>> pg_backup - start/stop backups, switch xlog, create restore points
>>> pg_replay - Pause/resume xlog replay on replicas
>>> pg_replication - Create/destroy/etc replication slots
>>> pg_rotate_logfile - Request logfile rotation
>>> pg_signal_backend - Signal other backends (cancel query/terminate)
>>> pg_file_settings - View configuration settings in all config files
>>
>> Thanks. This looks fine to me. I just have one single comment:
>>
>> +   Request logfile rotation.
>> s/logfile/transaction log file/
>
> Looks correct as is. Or maybe "server's log file" as in:
>
> 9.26.2. Server Signaling Functions
>
> pg_rotate_logfile(): Rotate server's log file

You're right, this is not a WAL segment, just a normal log file. Your
phrasing is better.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Amit Langote]

On 2015/12/22 14:05, Michael Paquier wrote:
> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost  wrote:
>> Updated and rebased patch attached which takes the 'pg_switch_xlog'
>> default role back out, leaving us with:
>>
>> pg_monitor - View privileged info
>> pg_backup - start/stop backups, switch xlog, create restore points
>> pg_replay - Pause/resume xlog replay on replicas
>> pg_replication - Create/destroy/etc replication slots
>> pg_rotate_logfile - Request logfile rotation
>> pg_signal_backend - Signal other backends (cancel query/terminate)
>> pg_file_settings - View configuration settings in all config files
> 
> Thanks. This looks fine to me. I just have one single comment:
> 
> +   Request logfile rotation.
> s/logfile/transaction log file/

Looks correct as is. Or maybe "server's log file" as in:

9.26.2. Server Signaling Functions

pg_rotate_logfile(): Rotate server's log file

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Michael, all,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> OK, let's do so then by having this one fall under pg_backup. Let's
> not be my grunting concerns be an obstacle for this patch, and we
> could still change it afterwards in this release beta cycle anyway
> based on user feedback.

Updated and rebased patch attached which takes the 'pg_switch_xlog'
default role back out, leaving us with:

pg_monitor - View privileged info
pg_backup - start/stop backups, switch xlog, create restore points
pg_replay - Pause/resume xlog replay on replicas
pg_replication - Create/destroy/etc replication slots
pg_rotate_logfile - Request logfile rotation
pg_signal_backend - Signal other backends (cancel query/terminate)
pg_file_settings - View configuration settings in all config files

Michael, another review would be great, if you don't mind.  I'm going to
be going through it also more closely since it sounds like we've got
consensus on at least this initial set of default roles and rights.  If
all looks good then I'll commit it.

Thanks!

Stephen
From 59bda4266a96976547e0aa44874ad716bf3dbdc9 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 18 Nov 2015 11:50:57 -0500
Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog

Add a note to the system catalog section pointing out that while
modifying the permissions on catalog tables is possible, it's
unlikely to have the desired effect.
---
 doc/src/sgml/catalogs.sgml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 97ef618..3b7768c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -21,6 +21,17 @@
particularly esoteric operations, such as adding index access methods.
   
 
+  
+   
+Changing the permissions on objects in the system catalogs, while
+possible, is unlikely to have the desired effect as the internal
+lookup functions use a cache and do not check the permissions nor
+policies of tables in the system catalog.  Further, permission
+changes to objects in the system catalogs are not preserved by
+pg_dump or across upgrades.
+   
+  
+
  
   Overview
 
-- 
2.5.0


From a659b7ecd220c0671d3cc272b3774318bce97567 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 30 Sep 2015 07:04:55 -0400
Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles

This will prevent users from creating roles which begin with "pg_" and
will check for those roles before allowing an upgrade using pg_upgrade.

This will allow for default roles to be provided at initdb time.
---
 doc/src/sgml/ref/psql-ref.sgml  |  8 +--
 src/backend/catalog/catalog.c   |  5 ++--
 src/backend/commands/user.c | 40 
 src/backend/utils/adt/acl.c | 41 +
 src/bin/pg_dump/pg_dumpall.c|  2 ++
 src/bin/pg_upgrade/check.c  | 40 ++--
 src/bin/psql/command.c  |  4 ++--
 src/bin/psql/describe.c |  5 +++-
 src/bin/psql/describe.h |  2 +-
 src/bin/psql/help.c |  4 ++--
 src/include/utils/acl.h |  1 +
 src/test/regress/expected/rolenames.out | 18 +++
 src/test/regress/sql/rolenames.sql  |  8 +++
 13 files changed, 166 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 6d0cb3d..76bb642 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1365,13 +1365,15 @@ testdb=
 
 
   
-\dg[+] [ pattern ]
+\dg[S+] [ pattern ]
 
 
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
 \du.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
 If the form \dg+ is used, additional information
@@ -1525,13 +1527,15 @@ testdb=
   
 
   
-\du[+] [ pattern ]
+\du[S+] [ pattern ]
 
 
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
 \dg.)
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
 If the form \du+ is used, additional information
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 81ccebf..184aa7d 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -184,8 +184,9 @@ IsToastNamespace(Oid namespaceId)

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Tue, Dec 1, 2015 at 9:18 AM, Michael Paquier
 wrote:
> On Tue, Dec 1, 2015 at 3:32 AM, Stephen Frost  wrote:
>> * Robert Haas (robertmh...@gmail.com) wrote:
>>> On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost  wrote:
>>> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
>>> >> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> >> >> It seems weird to not have a dedicated role for pg_switch_xlog.
>>> >> >
>>> >> > I didn't add a pg_switch_xlog default role in this patch series, but
>>> >> > would be happy to do so if that's the consensus.  It's quite easy to 
>>> >> > do.
>>> >>
>>> >> Agreed. I am not actually getting why that's part of the backup
>>> >> actually. That would be more related to archiving, both being
>>> >> unrelated concepts. But at this point I guess that's mainly a
>>> >> philosophical split.
>>> >
>>> > As David notes, they're actually quite related.  Note that in our
>>> > documentation pg_switch_xlog() is listed in the "Backup Control
>>> > Functions" table.
>>> >
>>> > I can think of a use-case for a user who can call pg_switch_xlog, but
>>> > not pg_start_backup()/pg_stop_backup(), but I have to admit that it
>>> > seems rather limited and I'm on the fence about it being a worthwhile
>>> > distinction.
>>>
>>> Sounds too narrow to me.  Are we going to have a separate predefined
>>> role for every security-restricted function to which someone might
>>> want to grant access?  That seems over the top to me.
>>
>> I certainly don't want to go down to that level and was, as seen above,
>> unsure about having pg_switch_xlog() as a differentiated privilege.
>> Michael, do you still see that as a useful independent capability?
>
> OK, let's do so then by having this one fall under pg_backup. Let's
> not be my grunting concerns be an obstacle for this patch, and we
> could still change it afterwards in this release beta cycle anyway
> based on user feedback.

Three weeks later...
This thread has not moved a iota. Stephen, are you planning to work
more on this patch? It seems that we found a consensus. If nothing
happens, I am afraid that the destiny of this patch will be to be
returned with feedback, it is the 5th CF where this entry is
registered.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Dec 1, 2015 at 9:18 AM, Michael Paquier
>  wrote:
> > OK, let's do so then by having this one fall under pg_backup. Let's
> > not be my grunting concerns be an obstacle for this patch, and we
> > could still change it afterwards in this release beta cycle anyway
> > based on user feedback.
> 
> Three weeks later...
> This thread has not moved a iota. Stephen, are you planning to work
> more on this patch? It seems that we found a consensus. If nothing
> happens, I am afraid that the destiny of this patch will be to be
> returned with feedback, it is the 5th CF where this entry is
> registered.

Ok, seems you're right that we've got consensus on it.  I'll post an
updated patch later today which I'll plan to commit.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost  wrote:
> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
> >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
> >> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
> >> >> It seems weird to not have a dedicated role for pg_switch_xlog.
> >> >
> >> > I didn't add a pg_switch_xlog default role in this patch series, but
> >> > would be happy to do so if that's the consensus.  It's quite easy to do.
> >>
> >> Agreed. I am not actually getting why that's part of the backup
> >> actually. That would be more related to archiving, both being
> >> unrelated concepts. But at this point I guess that's mainly a
> >> philosophical split.
> >
> > As David notes, they're actually quite related.  Note that in our
> > documentation pg_switch_xlog() is listed in the "Backup Control
> > Functions" table.
> >
> > I can think of a use-case for a user who can call pg_switch_xlog, but
> > not pg_start_backup()/pg_stop_backup(), but I have to admit that it
> > seems rather limited and I'm on the fence about it being a worthwhile
> > distinction.
> 
> Sounds too narrow to me.  Are we going to have a separate predefined
> role for every security-restricted function to which someone might
> want to grant access?  That seems over the top to me.

I certainly don't want to go down to that level and was, as seen above,
unsure about having pg_switch_xlog() as a differentiated privilege.
Michael, do you still see that as a useful independent capability?

> I don't think we should make it our goal to completely eliminate the
> use of SECURITY DEFINER functions for privilege delegation.  Of
> course, being able to grant privileges directly is nicer, because then
> the client code doesn't have to know about it.  But I think it's OK,
> even good, if the predefined roles cater to the common cases, and the
> less common cases aren't handled quite as elegantly.

Agreed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Robert Haas]

On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
>> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> >> It seems weird to not have a dedicated role for pg_switch_xlog.
>> >
>> > I didn't add a pg_switch_xlog default role in this patch series, but
>> > would be happy to do so if that's the consensus.  It's quite easy to do.
>>
>> Agreed. I am not actually getting why that's part of the backup
>> actually. That would be more related to archiving, both being
>> unrelated concepts. But at this point I guess that's mainly a
>> philosophical split.
>
> As David notes, they're actually quite related.  Note that in our
> documentation pg_switch_xlog() is listed in the "Backup Control
> Functions" table.
>
> I can think of a use-case for a user who can call pg_switch_xlog, but
> not pg_start_backup()/pg_stop_backup(), but I have to admit that it
> seems rather limited and I'm on the fence about it being a worthwhile
> distinction.

Sounds too narrow to me.  Are we going to have a separate predefined
role for every security-restricted function to which someone might
want to grant access?  That seems over the top to me.

I don't think we should make it our goal to completely eliminate the
use of SECURITY DEFINER functions for privilege delegation.  Of
course, being able to grant privileges directly is nicer, because then
the client code doesn't have to know about it.  But I think it's OK,
even good, if the predefined roles cater to the common cases, and the
less common cases aren't handled quite as elegantly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Alvaro Herrera]

Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:

> > > I can think of a use-case for a user who can call pg_switch_xlog, but
> > > not pg_start_backup()/pg_stop_backup(), but I have to admit that it
> > > seems rather limited and I'm on the fence about it being a worthwhile
> > > distinction.
> > 
> > Sounds too narrow to me.  Are we going to have a separate predefined
> > role for every security-restricted function to which someone might
> > want to grant access?  That seems over the top to me.
> 
> I certainly don't want to go down to that level and was, as seen above,
> unsure about having pg_switch_xlog() as a differentiated privilege.
> Michael, do you still see that as a useful independent capability?

Hmm, Robert's argument seems reasonable -- we can continue to offer
access to individual elements by granting execute on a security-definer
function owned by predefined user pg_backup.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Tue, Dec 1, 2015 at 3:32 AM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost  wrote:
>> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
>> >> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> >> >> It seems weird to not have a dedicated role for pg_switch_xlog.
>> >> >
>> >> > I didn't add a pg_switch_xlog default role in this patch series, but
>> >> > would be happy to do so if that's the consensus.  It's quite easy to do.
>> >>
>> >> Agreed. I am not actually getting why that's part of the backup
>> >> actually. That would be more related to archiving, both being
>> >> unrelated concepts. But at this point I guess that's mainly a
>> >> philosophical split.
>> >
>> > As David notes, they're actually quite related.  Note that in our
>> > documentation pg_switch_xlog() is listed in the "Backup Control
>> > Functions" table.
>> >
>> > I can think of a use-case for a user who can call pg_switch_xlog, but
>> > not pg_start_backup()/pg_stop_backup(), but I have to admit that it
>> > seems rather limited and I'm on the fence about it being a worthwhile
>> > distinction.
>>
>> Sounds too narrow to me.  Are we going to have a separate predefined
>> role for every security-restricted function to which someone might
>> want to grant access?  That seems over the top to me.
>
> I certainly don't want to go down to that level and was, as seen above,
> unsure about having pg_switch_xlog() as a differentiated privilege.
> Michael, do you still see that as a useful independent capability?

OK, let's do so then by having this one fall under pg_backup. Let's
not be my grunting concerns be an obstacle for this patch, and we
could still change it afterwards in this release beta cycle anyway
based on user feedback.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Alvaro Herrera]

Stephen Frost wrote:

> Even so, in the interest of having more fine-grained permission
> controls, I've gone ahead and added a pg_switch_xlog default role.
> Note that this means that pg_switch_xlog() can be called by both
> pg_switch_xlog roles and pg_backup roles.  I'd be very much against
> removing the ability to call pg_switch_xlog from the pg_backup role as
> that really is a capability which is needed by users running backups and
> it'd just add unnecessary complexity to require users setting up backup
> tools to grant two different roles to get the backup to work.

Isn't it simpler to grant pg_switch_xlog to pg_backup in the default
config?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

On Tuesday, November 24, 2015, Alvaro Herrera 
wrote:

> Stephen Frost wrote:
>
> > Even so, in the interest of having more fine-grained permission
> > controls, I've gone ahead and added a pg_switch_xlog default role.
> > Note that this means that pg_switch_xlog() can be called by both
> > pg_switch_xlog roles and pg_backup roles.  I'd be very much against
> > removing the ability to call pg_switch_xlog from the pg_backup role as
> > that really is a capability which is needed by users running backups and
> > it'd just add unnecessary complexity to require users setting up backup
> > tools to grant two different roles to get the backup to work.
>
> Isn't it simpler to grant pg_switch_xlog to pg_backup in the default
> config?
>

I'm not against it, but it would imply a set of data lines for
pg_auth_members, which we don't have today. We can't easily directly GRANT
the role due to the restrictions put in place to prevent regular users from
changing the system roles.  On the other hand, we could change the check to
only apply when we aren't in bootstrap mode.

Thanks!

Stephen

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost  wrote:
> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
> > Even so, in the interest of having more fine-grained permission
> > controls, I've gone ahead and added a pg_switch_xlog default role.
> > Note that this means that pg_switch_xlog() can be called by both
> > pg_switch_xlog roles and pg_backup roles.  I'd be very much against
> > removing the ability to call pg_switch_xlog from the pg_backup role as
> > that really is a capability which is needed by users running backups and
> > it'd just add unnecessary complexity to require users setting up backup
> > tools to grant two different roles to get the backup to work.
> 
> There is going to be many opinions regarding the granularity of this
> control, each one of us having a different opinion at the end. I don't
> think this should be a stopper for this patch, hence I am fine with the
> judgement you think is good. We could still more finely tune those default
> roles later in the dev cycle of 9.6 (10.0?).

Agreed.

> Thanks, this looks good to me.

Great.

> I guess that's better than nothing.

Agreed.

> I don't think you mean to refer to the switch of segments files here. Same
> comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location
> and pg_last_xlog_replay_location.

Urgh.  Got a bit ahead of myself there, apologies.  I've updated all of
these and a number of other minor typos and incorrect comments along the
way.

> +   ereport(ERROR,
> +   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +errmsg("must be superuser or member of
> pg_file_settings to see all config file settings")));
> Should avoid abbreviations => "all configuration file settings".

Fixed.

>
> -\dg[+] [  linkend="APP-PSQL-patterns"> class="parameter">pattern ]
> +\dgS[+] [  linkend="APP-PSQL-patterns"> class="parameter">pattern ]
>  
> I'm picky here, but that should be "\dg[S+]". Same for \du[S+].

Fixed

Updated patch attached.

Thanks!

Stephen
From 37b4627352287328ad28cd167620f51c26a68f04 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 18 Nov 2015 11:50:57 -0500
Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog

Add a note to the system catalog section pointing out that while
modifying the permissions on catalog tables is possible, it's
unlikely to have the desired effect.
---
 doc/src/sgml/catalogs.sgml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 97ef618..3b7768c 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -21,6 +21,17 @@
particularly esoteric operations, such as adding index access methods.
   
 
+  
+   
+Changing the permissions on objects in the system catalogs, while
+possible, is unlikely to have the desired effect as the internal
+lookup functions use a cache and do not check the permissions nor
+policies of tables in the system catalog.  Further, permission
+changes to objects in the system catalogs are not preserved by
+pg_dump or across upgrades.
+   
+  
+
  
   Overview
 
-- 
2.5.0


From 408a8ee807edbe875ff9fd860ebfd6c859dcffb8 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 30 Sep 2015 07:04:55 -0400
Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles

This will prevent users from creating roles which begin with "pg_" and
will check for those roles before allowing an upgrade using pg_upgrade.

This will allow for default roles to be provided at initdb time.
---
 doc/src/sgml/ref/psql-ref.sgml  |  8 +--
 src/backend/catalog/catalog.c   |  5 ++--
 src/backend/commands/user.c | 40 
 src/backend/utils/adt/acl.c | 41 +
 src/bin/pg_dump/pg_dumpall.c|  2 ++
 src/bin/pg_upgrade/check.c  | 40 ++--
 src/bin/psql/command.c  |  4 ++--
 src/bin/psql/describe.c |  5 +++-
 src/bin/psql/describe.h |  2 +-
 src/bin/psql/help.c |  4 ++--
 src/include/utils/acl.h |  1 +
 src/test/regress/expected/rolenames.out | 18 +++
 src/test/regress/sql/rolenames.sql  |  8 +++
 13 files changed, 166 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5899bb4..e0de107 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1336,13 +1336,15 @@ testdb=
 
 
   
-\dg[+] [ pattern ]
+\dg[S+] [ pattern ]
 
 
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command 

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
> Even so, in the interest of having more fine-grained permission
> controls, I've gone ahead and added a pg_switch_xlog default role.
> Note that this means that pg_switch_xlog() can be called by both
> pg_switch_xlog roles and pg_backup roles.  I'd be very much against
> removing the ability to call pg_switch_xlog from the pg_backup role as
> that really is a capability which is needed by users running backups and
> it'd just add unnecessary complexity to require users setting up backup
> tools to grant two different roles to get the backup to work.

There is going to be many opinions regarding the granularity of this
control, each one of us having a different opinion at the end. I don't
think this should be a stopper for this patch, hence I am fine with the
judgement you think is good. We could still more finely tune those default
roles later in the dev cycle of 9.6 (10.0?).

>> For the code paths where a backend would be actually running,
>> you could use the following:
>> SET client_min_messages TO 'error';
>> -- This should return "1" and not an ERROR nor a WARNING if PID does not
exist.
>> select count(pg_terminate_backend(0));
>> But that's ugly depending on your taste.
>
> I really dislike that.
>
>> Do you think that it makes sense to add tests as well in the second
>> patch to check that restrictions pg_* are in place? Except that I
>> don't have much more to say about patches 1 and 2 which are rather
>> straight-forward.
>
> Ah, yes.  I've now moved those hunks to the second patch where they
> really should have been.
>
>> Regarding patch 3, the documentation of psql should mention the new
>> subommands \dgS and \dgS+. That's a one-liner.
>
> Ah, right.  I've updated the psql SGML documentation for \duS and \dgS.
> The '\h' output had already been updated.  Was there something else
> which you meant above that I've missed?  Note that these fixes went into
> the second patch.

Thanks, this looks good to me.

>> +GRANT pg_backup TO regressuser1;
>> +SET SESSION AUTHORIZATION regressuser1;
>> +SELECT pg_start_backup('abc'); -- fail
>> +ERROR:  WAL level not sufficient for making an online backup
>> +HINT:  wal_level must be set to "archive", "hot_standby", or
>> "logical" at server start.
>> make install would wal on a server with something else than wal_level
>> = minimal. Just checking that errors kick correctly looks fine to me
>> here.
>
> I've removed those checks as they could get annoying on the buildfarm or
> for people doing make installcheck, as you say, but I'm not really
> thrilled that we're only testing the failure paths.

I guess that's better than nothing.

>> +GRANT pg_backup TO pg_monitor; -- error
>> +ERROR:  role "pg_monitor" is reserved
>> +DETAIL:  Roles starting with "pg_" are reserved.
>> +GRANT testrol0 TO pg_backup; -- error
>> +ERROR:  role "pg_backup" is reserved
>> We would gain with verbose error messages here, like, "cannot GRANT
>> somerole to a system role", and "cannot GRANT system role to
>> somerole".
>
> Alright, I've changed these to have better detail messages.


@@ -183,6 +190,13 @@ pg_current_xlog_location(PG_FUNCTION_ARGS)
 {
XLogRecPtr  current_recptr;

+   if (!has_privs_of_role(GetUserId(), DEFAULT_ROLE_BACKUPID) &&
+   !has_privs_of_role(GetUserId(), DEFAULT_ROLE_MONITORID) &&
+   !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SWITCH_XLOGID))
+   ereport(ERROR,
+   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+errmsg("must be superuser or member of
pg_monitor, pg_backup, or pg_switch_xlog to switch transaction log
files")));

I don't think you mean to refer to the switch of segments files here. Same
comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location
and pg_last_xlog_replay_location.

+   ereport(ERROR,
+   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+errmsg("must be superuser or member of
pg_file_settings to see all config file settings")));
Should avoid abbreviations => "all configuration file settings".

   
-\dg[+] [ pattern ]
+\dgS[+] [ pattern ]
 
I'm picky here, but that should be "\dg[S+]". Same for \du[S+].

The rest looks fine.
Regards,
-- 
Michael

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
> > * Michael Paquier (michael.paqu...@gmail.com) wrote:
> >> It seems weird to not have a dedicated role for pg_switch_xlog.
> >
> > I didn't add a pg_switch_xlog default role in this patch series, but
> > would be happy to do so if that's the consensus.  It's quite easy to do.
> 
> Agreed. I am not actually getting why that's part of the backup
> actually. That would be more related to archiving, both being
> unrelated concepts. But at this point I guess that's mainly a
> philosophical split.

As David notes, they're actually quite related.  Note that in our
documentation pg_switch_xlog() is listed in the "Backup Control
Functions" table.

I can think of a use-case for a user who can call pg_switch_xlog, but
not pg_start_backup()/pg_stop_backup(), but I have to admit that it
seems rather limited and I'm on the fence about it being a worthwhile
distinction.

Even so, in the interest of having more fine-grained permission
controls, I've gone ahead and added a pg_switch_xlog default role.
Note that this means that pg_switch_xlog() can be called by both
pg_switch_xlog roles and pg_backup roles.  I'd be very much against
removing the ability to call pg_switch_xlog from the pg_backup role as
that really is a capability which is needed by users running backups and
it'd just add unnecessary complexity to require users setting up backup
tools to grant two different roles to get the backup to work.

> > I'm a bit confused- this is a separate change.  Note that the previous
> > patch was actually a git patchset which had two patches- one to do the
> > reservation and a second to add the default roles.  The attached
> > patchset is actually three patches:
> > 1- Update to catalog documentation regarding permissions
> > 2- Reserve 'pg_' role namespace
> > 3- Add default roles
> 
> I see, that's why I got confused. I just downloaded your file and
> applied it blindly with git apply or patch (don't recall which). Other
> folks tend to publish that as a separate set of files generated by
> git-format-patch.

The file was generated using 'git format-patch', but sent to one file
instead of multiple.  'git am' understands that format and will add the
independent commits to the current branch.  Note that the git
documentation (see 'man git-format-patch' and 'man git-apply', at least
on Debian-based systems) specifically recommends using 'git am' to
create commits from patches generated by 'git format-patch'.

The workflow you're describing would require saving off each patch,
doing a 'patch' or 'git apply' run for each, then committing the changes
with your own commit message and only then would you have the entire
series.  That doesn't seem like a better approach.

> >> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location,
> >> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a
> >> restriction category like pg_monitor? I recall of course that we discussed
> >> that some months ago and that a lot of people were reluctant to harden this
> >> area to not break any existing monitoring tool, still this patch may be the
> >> occasion to restrict a bit those functions, particularly on servers where
> >> wal_compression is enabled.
> >
> > For my 2c, I believe they should.  I've not modified them in that way in
> > this patchset, but I can certainly do so if others agree.
> 
> My vote goes into hardening them a bit this way.

I've made those changes in this patch set.  Note that these functions
can now only be called by roles which are members of pg_monitor,
pg_backup, or pg_switch_xlog (or superuser, of course).

> > I've added regression tests for the default roles where it was
> > relatively straight-forward to do so.  I don't see a trivial way to test
> > that the pg_signal_backend role works though, as an example.
> 
> It is at least possible to check that the error code paths are
> working. 

Perhaps, but...

> For the code paths where a backend would be actually running,
> you could use the following:
> SET client_min_messages TO 'error';
> -- This should return "1" and not an ERROR nor a WARNING if PID does not 
> exist.
> select count(pg_terminate_backend(0));
> But that's ugly depending on your taste.

I really dislike that.

> Do you think that it makes sense to add tests as well in the second
> patch to check that restrictions pg_* are in place? Except that I
> don't have much more to say about patches 1 and 2 which are rather
> straight-forward.

Ah, yes.  I've now moved those hunks to the second patch where they
really should have been.

> Regarding patch 3, the documentation of psql should mention the new
> subommands \dgS and \dgS+. That's a one-liner.

Ah, right.  I've updated the psql SGML documentation for \duS and \dgS.
The '\h' output had already been updated.  Was there something else
which you meant above that I've missed?  Note that these fixes went into
the 

Re: [HACKERS] Additional role attributes && superuser review

[David Steele]

On 11/19/15 2:13 AM, Michael Paquier wrote:
> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
>> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>>> It seems weird to not have a dedicated role for pg_switch_xlog.
>>
>> I didn't add a pg_switch_xlog default role in this patch series, but
>> would be happy to do so if that's the consensus.  It's quite easy to do.
> 
> Agreed. I am not actually getting why that's part of the backup
> actually. That would be more related to archiving, both being
> unrelated concepts. But at this point I guess that's mainly a
> philosophical split.

I wouldn't say that backup and archiving are unrelated since backups
aren't valid without the proper set of WAL segments.

When configuring archiving/backup I use pg_switch_xlog() to verify that
WAL segments are getting to the archive.

I also use pg_switch_xlog() after pg_create_restore_point() to force the
WAL segment containing the restore point record to the archive.
Otherwise it might not be possible (or at least not easy) to recover to
the restore point in case of a problem.  The required WAL segment is
likely to get pushed to the archive before it is needed but I would
rather not bet on that.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Wed, Nov 18, 2015 at 10:06 PM, Michael Paquier  wrote:

>
>
> On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost  wrote:
> > * Heikki Linnakangas (hlinn...@iki.fi) wrote:
> >> I agree with Robert's earlier point that this needs to be split into
> >> multiple patches, which can then be reviewed and discussed
> >> separately. Pending that, I'm going to mark this as "Waiting on
> >> author" in the commitfest.
> >
> > Attached is an initial split which divides up reserving the role names
> > from actually adding the initial set of default roles.
>
> I have begun looking at this patch, and here are some comments after
> screening it.
>
> Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c
> (see attached for the rebase. none of the comments mentioning issues are
> fixed by it).
>
> =# grant pg_replay to pg_backup  ;
> GRANT ROLE
> =# \du pg_backup
>  List of roles
>  Role name |  Attributes  |  Member of
> ---+--+-
>  pg_backup | Cannot login | {pg_replay}
> Perhaps we should restrict granting a default role to another default role?
>
> +  
> +   Also note that changing the permissions on objects in the system
> +   catalogs, while possible, is unlikely to have the desired effect as
> +   the internal lookup functions use a cache and do not check the
> +   permissions nor policies of tables in the system catalog.  Further,
> +   permission changes to objects in the system catalogs are not
> +   preserved by pg_dump or across upgrades.
> +  
> This bit could be perhaps applied as a separate patch.
>
> +  
> +   pg_backup
> +   Start and stop backups, switch xlogs, and create restore
> points.
> +  
> s/switch xlogs/switch to a new transaction log file/
> It seems weird to not have a dedicated role for pg_switch_xlog.
>
> In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and
> pg_xlog_replay_resume still mention that those functions are restricted
> only to superusers. The documentation needs an update. It would be good to
> double-check if there are similar mistakes for the other functions.
>
> +   pg_montior
> Typo, pg_monitor.
>
> +   Pause and resume xlog replay on replicas.
> Those descriptions should be complete sentences or not? Both styles are
> mixed in user-manag.sgml.
>
> +  
> +   pg_replication
> +   Create, destroy, and work with replication slots.
> +  
> pg_replication_slots would be more adapted?
>
> +  
> +   pg_file_settings
> +   Allowed to view config settings from all config
> files
> +  
> I would say "configuration files" instead. An abbreviation is not adapted.
>
> +   pg_admin
> +   
> +Granted pg_backup, pg_monitor, pg_reply, pg_replication,
> +pg_rotate_logfile, pg_signal_backend and pg_file_settings roles.
> +   
> Typo, s/pg_reply/pg_replay and I think that there should be 
> markups around those role names. I am actually not convinced that we
> actually need it.
>
> +   if (IsReservedName(stmt->role))
> +   ereport(ERROR,
> +   (errcode(ERRCODE_RESERVED_NAME),
> +errmsg("role name \"%s\" is reserved",
> +stmt->role),
> +errdetail("Role names starting with
> \"pg_\" are reserved.")));
> Perhaps this could be a separate change? It seems that restricting roles
> with pg_ on the cluster is not a bad restriction in any case. You may want
> to add regression tests to trigger those errors as well.
>
> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location,
> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a
> restriction category like pg_monitor? I recall of course that we discussed
> that some months ago and that a lot of people were reluctant to harden this
> area to not break any existing monitoring tool, still this patch may be the
> occasion to restrict a bit those functions, particularly on servers where
> wal_compression is enabled.
>
> It would be nice to have regression tests as well to check that role
> restrictions are applied where they should.
>

Bonus:
-static void
-check_permissions(void)
-{
-   if (!superuser() && !has_rolreplication(GetUserId()))
-   ereport(ERROR,
-   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser or replication
role to use replication slots";
-}
I would have let check_permissions and directly done the checks on
has_rolreplication and has_privs_of_role in it, the same code being
duplicated three times.
-- 
Michael

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost  wrote:
> * Heikki Linnakangas (hlinn...@iki.fi) wrote:
>> I agree with Robert's earlier point that this needs to be split into
>> multiple patches, which can then be reviewed and discussed
>> separately. Pending that, I'm going to mark this as "Waiting on
>> author" in the commitfest.
>
> Attached is an initial split which divides up reserving the role names
> from actually adding the initial set of default roles.

I have begun looking at this patch, and here are some comments after
screening it.

Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c
(see attached for the rebase. none of the comments mentioning issues are
fixed by it).

=# grant pg_replay to pg_backup  ;
GRANT ROLE
=# \du pg_backup
 List of roles
 Role name |  Attributes  |  Member of
---+--+-
 pg_backup | Cannot login | {pg_replay}
Perhaps we should restrict granting a default role to another default role?

+  
+   Also note that changing the permissions on objects in the system
+   catalogs, while possible, is unlikely to have the desired effect as
+   the internal lookup functions use a cache and do not check the
+   permissions nor policies of tables in the system catalog.  Further,
+   permission changes to objects in the system catalogs are not
+   preserved by pg_dump or across upgrades.
+  
This bit could be perhaps applied as a separate patch.

+  
+   pg_backup
+   Start and stop backups, switch xlogs, and create restore
points.
+  
s/switch xlogs/switch to a new transaction log file/
It seems weird to not have a dedicated role for pg_switch_xlog.

In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and
pg_xlog_replay_resume still mention that those functions are restricted
only to superusers. The documentation needs an update. It would be good to
double-check if there are similar mistakes for the other functions.

+   pg_montior
Typo, pg_monitor.

+   Pause and resume xlog replay on replicas.
Those descriptions should be complete sentences or not? Both styles are
mixed in user-manag.sgml.

+  
+   pg_replication
+   Create, destroy, and work with replication slots.
+  
pg_replication_slots would be more adapted?

+  
+   pg_file_settings
+   Allowed to view config settings from all config files
+  
I would say "configuration files" instead. An abbreviation is not adapted.

+   pg_admin
+   
+Granted pg_backup, pg_monitor, pg_reply, pg_replication,
+pg_rotate_logfile, pg_signal_backend and pg_file_settings roles.
+   
Typo, s/pg_reply/pg_replay and I think that there should be 
markups around those role names. I am actually not convinced that we
actually need it.

+   if (IsReservedName(stmt->role))
+   ereport(ERROR,
+   (errcode(ERRCODE_RESERVED_NAME),
+errmsg("role name \"%s\" is reserved",
+stmt->role),
+errdetail("Role names starting with
\"pg_\" are reserved.")));
Perhaps this could be a separate change? It seems that restricting roles
with pg_ on the cluster is not a bad restriction in any case. You may want
to add regression tests to trigger those errors as well.

Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location,
pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a
restriction category like pg_monitor? I recall of course that we discussed
that some months ago and that a lot of people were reluctant to harden this
area to not break any existing monitoring tool, still this patch may be the
occasion to restrict a bit those functions, particularly on servers where
wal_compression is enabled.

It would be nice to have regression tests as well to check that role
restrictions are applied where they should.
Regards,
-- 
Michael
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
index 212fd1d..79a7f86 100644
--- a/contrib/test_decoding/expected/permissions.out
+++ b/contrib/test_decoding/expected/permissions.out
@@ -54,13 +54,13 @@ RESET ROLE;
 -- plain user *can't* can control replication
 SET ROLE lr_normal;
 SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR:  must be superuser or replication role to use replication slots
+ERROR:  must be superuser or member of pg_replication to use replication slots
 INSERT INTO lr_test VALUES('lr_superuser_init');
 ERROR:  permission denied for relation lr_test
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-ERROR:  must be superuser or replication role to use replication slots
+ERROR:  must be superuser or member of pg_replication to use replication slots
 SELECT 

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

Michael,

Thanks for the review!

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c
> (see attached for the rebase. none of the comments mentioning issues are
> fixed by it).

Done (did it a bit differently from what you had, to hopefully avoid
future OID conflicts and also to allow us a bit of room to add
additional default roles later, if we choose, in nearby OID space).

> =# grant pg_replay to pg_backup  ;
> GRANT ROLE
> =# \du pg_backup
>  List of roles
>  Role name |  Attributes  |  Member of
> ---+--+-
>  pg_backup | Cannot login | {pg_replay}
> Perhaps we should restrict granting a default role to another default role?

Done (in the second patch in the series, the one reserving 'pg_').

> +  
> +   Also note that changing the permissions on objects in the system
> +   catalogs, while possible, is unlikely to have the desired effect as
> +   the internal lookup functions use a cache and do not check the
> +   permissions nor policies of tables in the system catalog.  Further,
> +   permission changes to objects in the system catalogs are not
> +   preserved by pg_dump or across upgrades.
> +  
> This bit could be perhaps applied as a separate patch.

Done as a separate patch (first one in the series).

> +  
> +   pg_backup
> +   Start and stop backups, switch xlogs, and create restore
> points.
> +  
> s/switch xlogs/switch to a new transaction log file/

Fixed.

> It seems weird to not have a dedicated role for pg_switch_xlog.

I didn't add a pg_switch_xlog default role in this patch series, but
would be happy to do so if that's the consensus.  It's quite easy to do.

> In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and
> pg_xlog_replay_resume still mention that those functions are restricted
> only to superusers. The documentation needs an update. It would be good to
> double-check if there are similar mistakes for the other functions.

Fixed.  Also did a review and found a number of other places which
required updating, so those have also been fixed.

> +   pg_montior
> Typo, pg_monitor.

Fixed.

> +   Pause and resume xlog replay on replicas.
> Those descriptions should be complete sentences or not? Both styles are
> mixed in user-manag.sgml.

I didn't take any action on this.

> +  
> +   pg_replication
> +   Create, destroy, and work with replication slots.
> +  
> pg_replication_slots would be more adapted?

Perhaps...  I didn't make this change, but if others agree that adding
"_slots" would help, I'd be happy to do so.  Personally, I'd like to
keep these names shorter, if possible, but I don't want it to be
confusing either.

> +  
> +   pg_file_settings
> +   Allowed to view config settings from all config files
> +  
> I would say "configuration files" instead. An abbreviation is not adapted.

Done.

> +   pg_admin
> +   
> +Granted pg_backup, pg_monitor, pg_reply, pg_replication,
> +pg_rotate_logfile, pg_signal_backend and pg_file_settings roles.
> +   
> Typo, s/pg_reply/pg_replay and I think that there should be 
> markups around those role names. I am actually not convinced that we
> actually need it.

I added  markups around the role names, where used.

I'm guessing you were referring to pg_admin with your comment that you
were "not convinced that we actually need it."  After thinking about
it for a bit, I tend to agree.  A user could certainly create their own
role which combines these all together if they wanted to (or do any
other combinations, based on their particular needs).  I've gone ahead
and removed pg_admin from the set of default roles.

> +   if (IsReservedName(stmt->role))
> +   ereport(ERROR,
> +   (errcode(ERRCODE_RESERVED_NAME),
> +errmsg("role name \"%s\" is reserved",
> +stmt->role),
> +errdetail("Role names starting with
> \"pg_\" are reserved.")));
> Perhaps this could be a separate change? It seems that restricting roles
> with pg_ on the cluster is not a bad restriction in any case. You may want
> to add regression tests to trigger those errors as well.

I'm a bit confused- this is a separate change.  Note that the previous
patch was actually a git patchset which had two patches- one to do the
reservation and a second to add the default roles.  The attached
patchset is actually three patches:

1- Update to catalog documentation regarding permissions
2- Reserve 'pg_' role namespace
3- Add default roles

> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location,
> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a
> restriction category like pg_monitor? I recall of course that we discussed
> that some months ago and that a lot of people were reluctant 

Re: [HACKERS] Additional role attributes && superuser review

[Michael Paquier]

On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> It seems weird to not have a dedicated role for pg_switch_xlog.
>
> I didn't add a pg_switch_xlog default role in this patch series, but
> would be happy to do so if that's the consensus.  It's quite easy to do.

Agreed. I am not actually getting why that's part of the backup
actually. That would be more related to archiving, both being
unrelated concepts. But at this point I guess that's mainly a
philosophical split.

> I'm guessing you were referring to pg_admin with your comment that you
> were "not convinced that we actually need it."  After thinking about
> it for a bit, I tend to agree.  A user could certainly create their own
> role which combines these all together if they wanted to (or do any
> other combinations, based on their particular needs).  I've gone ahead
> and removed pg_admin from the set of default roles.

Yeah that 's what I meant. Thanks, I should have been more precise
with my words.

>> +   if (IsReservedName(stmt->role))
>> +   ereport(ERROR,
>> +   (errcode(ERRCODE_RESERVED_NAME),
>> +errmsg("role name \"%s\" is reserved",
>> +stmt->role),
>> +errdetail("Role names starting with
>> \"pg_\" are reserved.")));
>> Perhaps this could be a separate change? It seems that restricting roles
>> with pg_ on the cluster is not a bad restriction in any case. You may want
>> to add regression tests to trigger those errors as well.
>
> I'm a bit confused- this is a separate change.  Note that the previous
> patch was actually a git patchset which had two patches- one to do the
> reservation and a second to add the default roles.  The attached
> patchset is actually three patches:
> 1- Update to catalog documentation regarding permissions
> 2- Reserve 'pg_' role namespace
> 3- Add default roles

I see, that's why I got confused. I just downloaded your file and
applied it blindly with git apply or patch (don't recall which). Other
folks tend to publish that as a separate set of files generated by
git-format-patch.

>> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location,
>> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a
>> restriction category like pg_monitor? I recall of course that we discussed
>> that some months ago and that a lot of people were reluctant to harden this
>> area to not break any existing monitoring tool, still this patch may be the
>> occasion to restrict a bit those functions, particularly on servers where
>> wal_compression is enabled.
>
> For my 2c, I believe they should.  I've not modified them in that way in
> this patchset, but I can certainly do so if others agree.

My vote goes into hardening them a bit this way.

> I've added regression tests for the default roles where it was
> relatively straight-forward to do so.  I don't see a trivial way to test
> that the pg_signal_backend role works though, as an example.

It is at least possible to check that the error code paths are
working. For the code paths where a backend would be actually running,
you could use the following:
SET client_min_messages TO 'error';
-- This should return "1" and not an ERROR nor a WARNING if PID does not exist.
select count(pg_terminate_backend(0));
But that's ugly depending on your taste.

>> Bonus:
>> -static void
>> -check_permissions(void)
>> -{
>> -   if (!superuser() && !has_rolreplication(GetUserId()))
>> -   ereport(ERROR,
>> -   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> -(errmsg("must be superuser or replication
>> role to use replication slots";
>> -}
>> I would have let check_permissions and directly done the checks on
>> has_rolreplication and has_privs_of_role in it, the same code being
>> duplicated three times.
>
> For my 2c, I dislike the notion of "check_permissions()" functions being
> added throughout the codebase as I'm afraid it'd get confusing, which is
> why I got rid of it.  I'm much happier seeing the actual permissions
> check as it should be happening early on in the primary function which
> is being called into.  If there is really a push to go back to having a
> 'check_permissions()' like function, I'd argue that we should make the
> function name more descriptive of what's actually being done and have
> them be distinct from each other.  As I recall, I discussed this change
> with Andres and he didn't feel particularly strongly about this one way
> or the other, therefore, I've not made this change for this patchset.

Fine for me. Usually people point out such code duplications are bad
things, and here we just have a static routine being used for a set of
functions interacting with the same kind of object, here a replication
slot. But I'm fine either way.

Do you think that it 

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> I agree with Robert's earlier point that this needs to be split into
> multiple patches, which can then be reviewed and discussed
> separately. Pending that, I'm going to mark this as "Waiting on
> author" in the commitfest.

Attached is an initial split which divides up reserving the role names
from actually adding the initial set of default roles.

Thanks!

Stephen
From 593ae64bf22bc343d7a5824d65c1224a77091710 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 30 Sep 2015 07:04:55 -0400
Subject: [PATCH 1/2] Reserve the "pg_" namespace for roles

This will prevent users from creating roles which being with "pg_" and
will check for those roles before allowing an upgrade using pg_upgrade.

This will allow for default roles to be provided at initdb time.
---
 src/backend/catalog/catalog.c |  5 +++--
 src/backend/commands/user.c   | 32 
 src/bin/pg_dump/pg_dumpall.c  |  2 ++
 src/bin/pg_upgrade/check.c| 40 ++--
 src/bin/psql/command.c|  4 ++--
 src/bin/psql/describe.c   |  5 -
 src/bin/psql/describe.h   |  2 +-
 src/bin/psql/help.c   |  4 ++--
 8 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 81ccebf..184aa7d 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -184,8 +184,9 @@ IsToastNamespace(Oid namespaceId)
  *		True iff name starts with the pg_ prefix.
  *
  *		For some classes of objects, the prefix pg_ is reserved for
- *		system objects only.  As of 8.0, this is only true for
- *		schema and tablespace names.
+ *		system objects only.  As of 8.0, this was only true for
+ *		schema and tablespace names.  With 9.6, this is also true
+ *		for roles.
  */
 bool
 IsReservedName(const char *name)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 295e0b0..fde8d15 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -17,6 +17,7 @@
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "catalog/binary_upgrade.h"
+#include "catalog/catalog.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
 #include "catalog/objectaccess.h"
@@ -312,6 +313,17 @@ CreateRole(CreateRoleStmt *stmt)
 	}
 
 	/*
+	 * Check that the user is not trying to create a role is the reserved
+	 * "pg_" namespace.
+	 */
+	if (IsReservedName(stmt->role))
+		ereport(ERROR,
+(errcode(ERRCODE_RESERVED_NAME),
+ errmsg("role name \"%s\" is reserved",
+	 stmt->role),
+ errdetail("Role names starting with \"pg_\" are reserved.")));
+
+	/*
 	 * Check the pg_authid relation to be certain the role doesn't already
 	 * exist.
 	 */
@@ -1117,6 +1129,7 @@ RenameRole(const char *oldname, const char *newname)
 	int			i;
 	Oid			roleid;
 	ObjectAddress address;
+	Form_pg_authid authform;
 
 	rel = heap_open(AuthIdRelationId, RowExclusiveLock);
 	dsc = RelationGetDescr(rel);
@@ -1136,6 +1149,7 @@ RenameRole(const char *oldname, const char *newname)
 	 */
 
 	roleid = HeapTupleGetOid(oldtuple);
+	authform = (Form_pg_authid) GETSTRUCT(oldtuple);
 
 	if (roleid == GetSessionUserId())
 		ereport(ERROR,
@@ -1146,6 +1160,24 @@ RenameRole(const char *oldname, const char *newname)
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg("current user cannot be renamed")));
 
+	/*
+	 * Check that the user is not trying to rename a system role and
+	 * not trying to rename a role into the reserved "pg_" namespace.
+	 */
+	if (IsReservedName(NameStr(authform->rolname)))
+		ereport(ERROR,
+(errcode(ERRCODE_RESERVED_NAME),
+ errmsg("role name \"%s\" is reserved",
+	 NameStr(authform->rolname)),
+ errdetail("Role names starting with \"pg_\" are reserved.")));
+
+	if (IsReservedName(newname))
+		ereport(ERROR,
+(errcode(ERRCODE_RESERVED_NAME),
+ errmsg("role name \"%s\" is reserved",
+	 newname),
+ errdetail("Role names starting with \"pg_\" are reserved.")));
+
 	/* make sure the new name doesn't exist */
 	if (SearchSysCacheExists1(AUTHNAME, CStringGetDatum(newname)))
 		ereport(ERROR,
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3461335..addabd0 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -673,6 +673,7 @@ dumpRoles(PGconn *conn)
 			 "pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment, "
 		  "rolname = current_user AS is_current_user "
 		  "FROM pg_authid "
+		  "WHERE rolname !~ '^pg_' "
 		  "ORDER BY 2");
 	else if (server_version >= 90100)
 		printfPQExpBuffer(buf,
@@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn)
 	   "LEFT JOIN pg_authid ur on ur.oid = a.roleid "
 	   "LEFT JOIN pg_authid um on um.oid = a.member "
 	   "LEFT JOIN pg_authid ug on ug.oid = a.grantor "
+	   "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')"
 	   

Re: [HACKERS] Additional role attributes superuser review

[Michael Paquier]

On Sat, Jul 11, 2015 at 6:06 AM, Heikki Linnakangas wrote:
 On 05/08/2015 07:35 AM, Stephen Frost wrote:
 In consideration of the fact that you can't create schemas which start
 with pg_ and therefore the default search_path wouldn't work for that
 user, and that we also reserve pg_ for tablespaces, I'm not inclined
 to worry too much about this case.  Further, if we accept this argument,
 then we simply can't ever provide additional default or system roles,
 ever.  That'd be a pretty narrow corner to have painted ourselves into.


 Well, you could still provide them through some other mechanism, like
 require typing SYSTEM ROLE pg_backup any time you mean that magic role.
 But I agree, reserving pg_* is much better. I wish we had done it when we
 invented roles (6.5?), so there would be no risk that you would upgrade from
 a system that already has a pg_foo role. But I think it'd still be OK.

 I agree with Robert's earlier point that this needs to be split into
 multiple patches, which can then be reviewed and discussed separately.
 Pending that, I'm going to mark this as Waiting on author in the
 commitfest.

... And now marked as returned with feedback.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Heikki Linnakangas]

On 05/08/2015 07:35 AM, Stephen Frost wrote:

Gavin,

* Gavin Flower (gavinflo...@archidevsys.co.nz) wrote:

What if I had a company with several subsidiaries using the same
database, and want to prefix roles and other things with the
subsidiary's initials? (I am not saying this would be a good
architecture!!!)


If you admit that it's not a good solution then I'm not quite sure how
much we really want to worry about it. :)


For example if one subsidiary was called 'Perfect Gentleman', so I
would want roles prefixed by 'pg_' and would be annoyed if I
couldn't!


You might try creating a schema for that user..  You'll hopefully find
it difficult to do. :)

In consideration of the fact that you can't create schemas which start
with pg_ and therefore the default search_path wouldn't work for that
user, and that we also reserve pg_ for tablespaces, I'm not inclined
to worry too much about this case.  Further, if we accept this argument,
then we simply can't ever provide additional default or system roles,
ever.  That'd be a pretty narrow corner to have painted ourselves into.


Well, you could still provide them through some other mechanism, like 
require typing SYSTEM ROLE pg_backup any time you mean that magic 
role. But I agree, reserving pg_* is much better. I wish we had done it 
when we invented roles (6.5?), so there would be no risk that you would 
upgrade from a system that already has a pg_foo role. But I think it'd 
still be OK.


I agree with Robert's earlier point that this needs to be split into 
multiple patches, which can then be reviewed and discussed separately. 
Pending that, I'm going to mark this as Waiting on author in the 
commitfest.


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Gavin,

* Gavin Flower (gavinflo...@archidevsys.co.nz) wrote:
 What if I had a company with several subsidiaries using the same
 database, and want to prefix roles and other things with the
 subsidiary's initials? (I am not saying this would be a good
 architecture!!!)

If you admit that it's not a good solution then I'm not quite sure how
much we really want to worry about it. :)

 For example if one subsidiary was called 'Perfect Gentleman', so I
 would want roles prefixed by 'pg_' and would be annoyed if I
 couldn't!

You might try creating a schema for that user..  You'll hopefully find
it difficult to do. :)

In consideration of the fact that you can't create schemas which start
with pg_ and therefore the default search_path wouldn't work for that
user, and that we also reserve pg_ for tablespaces, I'm not inclined
to worry too much about this case.  Further, if we accept this argument,
then we simply can't ever provide additional default or system roles,
ever.  That'd be a pretty narrow corner to have painted ourselves into.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Robert Haas]

On Wed, Apr 29, 2015 at 10:47 AM, Stephen Frost sfr...@snowman.net wrote:
 Here is the latest revision of this patch.

I think this patch is too big and does too many things.  It should be
broken up into small patches which can be discussed and validated
independently.  The fact that your commit message is incredibly long
is a sign that there's too much going on here, and that message
doesn't even cover all of it.

It seems to me that the core change here is really to pg_dump.  You're
adding the ability for pg_dump to dump and restore privileges on
objects in pg_dump.  That capability is a complete - and useful -
feature in its own right, with no other changes.

Then the next thing you've got here is a series of patches that change
the rights to execute various utility functions.  Some of those, like
the changes to pg_stop_backup(), are pretty much a slam-dunk, because
they don't really change user-visible behavior; they just give the DBA
more options.  Others, like the changes to replication permissions,
are likely to be more controversial.  You should split the stuff
that's a slam-dunk apart from the stuff that makes policy decisions,
and plan to commit the former changes first, and the latter changes
only if and when there is very clear agreement on the specific
policies to be changed.

Finally, you've got the idea of making pg_ a reserved prefix for
roles, adding some predefined roles, and giving them some predefined
privileges.  That should be yet another patch.

I think that if you commit this the way you have it today, everybody
will go, oh, look, Stephen committed something, but it looks
complicated, I won't pay attention.  And then, six months from now
when we're in beta, or maybe after final, people will start looking at
this and realizing that there are parts of it they hate, but it will
be hard to fix at that point.  Breaking it out will hopefully allow
more discussion on the individual features of in here, of which there
are probably at least four.  It will also make it easier to revert
part of it rather than all of it if that turns out to be needed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Alvaro Herrera]

Robert Haas wrote:

 I think that if you commit this the way you have it today, everybody
 will go, oh, look, Stephen committed something, but it looks
 complicated, I won't pay attention.

Yeah, that sucks.

 Finally, you've got the idea of making pg_ a reserved prefix for
 roles, adding some predefined roles, and giving them some predefined
 privileges.  That should be yet another patch.

On this part I have a bit of a problem -- the prefix is not really
reserved, is it.  I mean, evidently it's still possible to create roles
with the pg_ prefix ... otherwise, how come the new lines to
system_views.sql that create the predefined roles work in the first
place?  I think if we're going to reserve role names, we should reserve
them for real: CREATE ROLE should flat out reject creation of such
roles, and the default ones should be created during bootstrap.

IMO anyway.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Robert, all,

* Stephen Frost (sfr...@snowman.net) wrote:
 * Stephen Frost (sfr...@snowman.net) wrote:
  * Robert Haas (robertmh...@gmail.com) wrote:
   The tricky part of this seems to me to be the pg_dump changes.  The
   new catalog flag seems a little sketchy to me; wouldn't it be better
   to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
   DUMP_NONE?
  
  I agree that the pg_dump changes are a very big part of this change.
  I'll look at using an enum and see if that would work.
 
 I've implemented this approach and there are things which I like about
 it and things which I don't.  I'd love to hear your thoughts.  As
 mentioned previously, this patch does not break the pg_stat_activity or
 pg_stat_replication view APIs.  Similairly, the functions which directly
 feed those views return the same results they did previously, there are
 just additional functions now which provide everything, and view on top
 of those, for users who are GRANT'd access to them.

Here is the latest revision of this patch.

The big change here is the addition of default roles.  This approach has
been brought up a few times and Magnus recently mentioned it to me
again.  Having the default roles greatly reduced the impact of this
change on the test_deparse regression test, which was quite nice.

Updates are included for pg_upgrade and pg_dumpall to handle roles which
start with pg_ specially as we are now claiming those as System
defined roles (similar to how we claim schemas starting with pg_ are
system defined, etc).  These new default roles are in line with the
previously discussed role attributes, but have the advantage that they
act just like normal roles and work inside of the normal permissions
system.  They are:

pg_backup - Start and stop backups, switch xlogs, create restore points.
pg_monitor - View privileged system info (user activity, replica lag)
pg_replay - Control XLOG replay on replica (pause, resume)
pg_replication - Create, destroy, work with replication slots

pg_admin - All of the above, plus rotate logfiles and signal backends

Feedback on all of this would be great.  One interesting idea is that,
with these defined default roles, we could rip out the majority of the
changes to pg_dump and declare that users have to use only the roles we
provide to manage access to those functions (or risk any changes they
make to the ACLs of system objects disappearing across upgrades or
pg_dump/restore's, which is what happens today anyway).  I'm a bit on
the fence about it myself; it'd certainly reduce the risk of this change
but it would also limit users to only being able to operate at the
pre-defined levels we've set, but then again, the same was going to be
true with the role attributes-based approach and I don't recall any
complaints during that discussion.

Thoughts?  Feedback on this would be most welcome; it's been a long time
incubating and I'd really like to get this capability in and close it
out of the current commitfest.  I'm certainly of the opinion that it
will be a welcome step forward for quite a few of our users as the
discussion about how to create non-superuser roles for certain
operations (a monitor role, in particular, but also backup and replay)
has come up quite a bit, both on the lists and directly from clients.

Thanks!

Stephen
From 488df9c567fdd0a56afa084a8f22f8b8a2412bd7 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Thu, 19 Mar 2015 14:49:26 -0400
Subject: [PATCH] Use GRANT for access to privileged functions

Historically, we have hard-coded the permissions for privileged
functions in C code (eg: if (superuser()) then allow X, else don't).
Unfortunately, that's pretty limiting and means that functions which are
useful for roles that should not be superusers (eg: monitoring, backup
management) require that the user calling them be a superuser.  This
leads to far more uses of superuser roles than is ideal.

Thankfully, we have a very handy and complex privilege system for
managing who has access to what already built into PG.  This is the
GRANT system which has existed since very near the beginning of PG.

This provides a set of system functions which are not able to be
executed by all users by default and allows administrators to grant
access to those functions for the users (eg: monitoring or other roles)
where they feel it is appropriate.

Further, create a set of pre-defined roles (which start with pg_)
for administrators to use to grant bulk access with.  This greatly
simplifies the granting of monitor, backup, and similar privileges.
pg_upgrade and pg_dumpall are updated to treat roles starting with
pg_ in much the same way as the default role is handled, and
pg_upgrade has been taught to complain if it finds any roles starting
with pg_ in a 9.4 or older system.  Having pre-defined roles also
allows 3rd-party utilities (eg: check_postgres.pl) to depend on these
roles and the access they provide in their documentation for 

Re: [HACKERS] Additional role attributes superuser review

[Gavin Flower]

On 30/04/15 12:20, Alvaro Herrera wrote:

Robert Haas wrote:


I think that if you commit this the way you have it today, everybody
will go, oh, look, Stephen committed something, but it looks
complicated, I won't pay attention.

Yeah, that sucks.


Finally, you've got the idea of making pg_ a reserved prefix for
roles, adding some predefined roles, and giving them some predefined
privileges.  That should be yet another patch.

On this part I have a bit of a problem -- the prefix is not really
reserved, is it.  I mean, evidently it's still possible to create roles
with the pg_ prefix ... otherwise, how come the new lines to
system_views.sql that create the predefined roles work in the first
place?  I think if we're going to reserve role names, we should reserve
them for real: CREATE ROLE should flat out reject creation of such
roles, and the default ones should be created during bootstrap.

IMO anyway.

What if I had a company with several subsidiaries using the same 
database, and want to prefix roles and other things with the 
subsidiary's initials? (I am not saying this would be a good 
architecture!!!)


For example if one subsidiary was called 'Perfect Gentleman', so I would 
want roles prefixed by 'pg_' and would be annoyed if I couldn't!



Cheers,
Gavin


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Robert Haas]

On Wed, Apr 29, 2015 at 8:20 PM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
 Finally, you've got the idea of making pg_ a reserved prefix for
 roles, adding some predefined roles, and giving them some predefined
 privileges.  That should be yet another patch.

 On this part I have a bit of a problem -- the prefix is not really
 reserved, is it.  I mean, evidently it's still possible to create roles
 with the pg_ prefix ... otherwise, how come the new lines to
 system_views.sql that create the predefined roles work in the first
 place?  I think if we're going to reserve role names, we should reserve
 them for real: CREATE ROLE should flat out reject creation of such
 roles, and the default ones should be created during bootstrap.

 IMO anyway.

This is exactly what I mean about needing separate discussion for
separate parts of the patch.  There's so much different stuff in there
right now that objections like this won't necessarily come out until
it's far too late to change things around.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Robert,

* Stephen Frost (sfr...@snowman.net) wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
  On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost sfr...@snowman.net wrote:
   Clearly, further testing and documentation is required and I'll be
   getting to that over the next couple of days, but it's pretty darn late
   and I'm currently getting libpq undefined reference errors, probably
   because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)
  
   Thoughts?
  
  The tricky part of this seems to me to be the pg_dump changes.  The
  new catalog flag seems a little sketchy to me; wouldn't it be better
  to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
  DUMP_NONE?
 
 I agree that the pg_dump changes are a very big part of this change.
 I'll look at using an enum and see if that would work.

I've implemented this approach and there are things which I like about
it and things which I don't.  I'd love to hear your thoughts.  As
mentioned previously, this patch does not break the pg_stat_activity or
pg_stat_replication view APIs.  Similairly, the functions which directly
feed those views return the same results they did previously, there are
just additional functions now which provide everything, and view on top
of those, for users who are GRANT'd access to them.

This does change the API of a few functions which previously allowed
roles with the replication attribute to call them.  We could provide
additional functions to provide both paths but I don't believe it's
really necessary.  Indeed, I feel it's better for administrators to
explicit grant access to those functions instead.

Note that this doesn't use an enum but a bit field for which components
of a given object should be dumped out.  While I like that in general,
it meant a lot of changes and I'll be the first to admit that I've not
tested every possible pg_dump option permutation to make sure that the
correct results are returned.  I plan to do that in the coming weeks and
will address any issues found.

Is this, more-or-less, what you were thinking of?  I looked at removing
the relatively grotty options (dataOnly, aclsSkip, etc) and it didn't
appear trivial to use the bitmask instead.  I can look into that further
though, as I do feel that it'd be good if we could reduce our dependence
on those options in favor of the bitmask approach.

Thoughts?

Thanks!

Stephen
From dd682d4d9dc7f25ae38bb5fc5ed5082c5071 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Thu, 19 Mar 2015 14:49:26 -0400
Subject: [PATCH] Use GRANT for access to privileged functions

Historically, we have hard-coded the permissions for privileged
functions in C code (eg: if (superuser()) then allow X, else don't).
Unfortunately, that's pretty limiting and means that functions which are
useful for monitoring require that the user calling them be a superuser.
That's a problem because, generally speaking, monitoring systems should
not have more access than they need and certainly should not have write
access to a system.

Thankfully, we have a very handy and complex privilege system for
managing who has access to what already built into PG.  This is the
GRANT system which has existed since very near the beginning of PG.
Therefore, provide a set of system functions which are not able to be
executed by all users by default and allow administrators to grant
access to those functions for the users (eg: monitoring or other roles)
where they feel it is appropriate.

We avoid breaking existing APIs for the system views by providing
backwards compatible functions which continue to filter in the C code
based on the user's credentials, where that is possible.

For a few functions (eg: pg_logical_slot_* and friends), it makes more
sense to break compatibility as they are relatively new and require
admins to GRANT access to those functions explicitly on upgrade (this
should be noted in the release notes).  Other functions, which allowed
access based on role attribute 'replication', may need to have GRANTs
applied to them, but note that replication connections do not go through
the normal GRANT system and therefore tools such as pg_basebackup will
not be impacted by this change.  In general, this change requires
administrators to be more explicit about which roles have access to
these functions.

Last, but certainly not least, this changes pg_dump to include ACLs for
the pg_catalog schema.  This is necessary as administrators are now
expected and encouraged to set privileges on functions and perhaps views
differently from their defaults and we need to preserve those settings.
---
 contrib/test_decoding/expected/permissions.out |   17 +-
 contrib/test_decoding/sql/permissions.sql  |9 +
 src/backend/access/transam/xlogfuncs.c |   30 -
 src/backend/catalog/system_views.sql   |   72 ++
 src/backend/replication/logical/logicalfuncs.c |   11 -
 src/backend/replication/slotfuncs.c|   15 -
 

Re: [HACKERS] Additional role attributes superuser review

[Robert Haas]

On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost sfr...@snowman.net wrote:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  REVOKE'ing access *without* removing the permissions checks would defeat
  the intent of these changes, which is to allow an administrator to grant
  the ability for a certain set of users to cancel and/or terminate
  backends started by other users, without also granting those users
  superuser rights.

 I see: we have two different use-cases and no way for GRANT/REVOKE
 to manage both cases using permissions on a single object.  Carry
 on then.

 Alright, after going about this three or four different ways, I've
 settled on the approach proposed in the attached patch.  Most of it is
 pretty straight-forward: drop the hard-coded check in the function
 itself and REVOKE EXECUTE on the function from PUBLIC.  The interesting
 bits are those pieces which are more complex than the all-or-nothing
 cases.

 This adds a few new SQL-level functions which return unfiltered results,
 if you're allowed to call them based on the GRANT system (and EXECUTE
 privileges for them are REVOKE'd from PUBLIC, of course).  These are:
 pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and
 pg_signal_backend (which is similar but not the same- that allows you to
 send signals to other backends as a regular user, if you are allowed to
 call that function and the other backend wasn't started by a superuser).

 There are two new views added, which simply sit on top of the new
 functions: pg_stat_activity_all and pg_stat_replication_all.

 Minimizing the amount of code duplication wasn't too hard with the
 pg_stat_get_wal_senders case; as it was already returning a tuplestore
 and so I simply moved most of the logic into a helper function which
 handled populating the tuplestore and then each call path was relatively
 small and mostly boilerplate.  pg_stat_get_activity required a bit more
 in the way of changes, but they were essentially to modify it to return
 a tuplestore like pg_stat_get_wal_senders, and then add in the extra
 function and boilerplate for the non-filtered path.

 I had considered (and spent a good bit of time implementing...) a number
 of alternatives, mostly around trying to do more at the SQL level when
 it came to filtering, but that got very ugly very quickly.  There's no
 simple way to find out what was the effective role of the caller of
 this function from inside of a security definer function (which would
 be required for the filtering, as it would need to be able to call the
 function underneath).  This is necessary, of course, because functions
 in views run as the caller of the view, not as the view owner (that's
 useful in some cases, less useful in others).  I looked at exposing the
 information about the effective role which was calling a function, but
 that got pretty ugly too.  The GUC system has a stack, but we don't
 actually update the 'role' GUC when we are in security definer
 functions.  There's the notion of an outer role ID, but that doesn't
 account for intermediate security definer functions and so, while it's
 fine for what it does, it wasn't really helpful in this case.

 I do still think it'd be nice to provide that information and perhaps we
 can do so with fmgr_security_definer(), but it's beyond what's really
 needed here and I don't think it'd end up being particularly cleaner to
 do it all in SQL now that I've refactored pg_stat_get_activity.

 I'd further like to see the ability to declare on either a function call
 by function call basis (inside a view defintion), of even at the view
 level (as that would allow me to build up multiple views with different
 parameters) who to run this function as, where the options would be
 view owner or view querier, very similar to our SECURITY DEFINER vs.
 SECURITY INVOKER options for functions today.  This is interesting
 because, more and more, we're building functions which are actually
 returning data sets, not individual values, and we want to filter those
 sets sometimes and not other times.  In any case, those are really
 thoughts for the future and get away from what this is all about, which
 is reducing the need for monitoring and/or regular admins to have the
 superuser bit when they don't really need it.

 Clearly, further testing and documentation is required and I'll be
 getting to that over the next couple of days, but it's pretty darn late
 and I'm currently getting libpq undefined reference errors, probably
 because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)

 Thoughts?

The tricky part of this seems to me to be the pg_dump changes.  The
new catalog flag seems a little sketchy to me; wouldn't it be better
to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
DUMP_NONE?

Is this creating a user-visible API break, where pg_stat_activity and
pg_stat_replication now always show only the publicly-visible
information, and privileged users 

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost sfr...@snowman.net wrote:
  Clearly, further testing and documentation is required and I'll be
  getting to that over the next couple of days, but it's pretty darn late
  and I'm currently getting libpq undefined reference errors, probably
  because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)
 
  Thoughts?
 
 The tricky part of this seems to me to be the pg_dump changes.  The
 new catalog flag seems a little sketchy to me; wouldn't it be better
 to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
 DUMP_NONE?

I agree that the pg_dump changes are a very big part of this change.
I'll look at using an enum and see if that would work.

 Is this creating a user-visible API break, where pg_stat_activity and
 pg_stat_replication now always show only the publicly-visible
 information, and privileged users must explicitly decide to query the
 _all versions?  If so, -1 from me on that part of this.

Thanks for bringing it up.  No, the existing API is exactly the same for
the existing views- the only difference is that now there are _all
versions which provide unfiltered data (but you have to have permission
to call the _all() functions underneath, or you get a permission denied
error).

Where this does have an API change is with the simpler functions that
used to do if (superuser() || replication_role()) and now depend on
the GRANT system instead.  We can't (today, at least) say:

GRANT EXECUTE ON function_whatever() TO replication_roles;

And have that be kept current as that role attribute is modified.  I've
not done it yet, but we could certainly have pg_dump dump out GRANTs for
the *current* users which have that role attribute and give those users
access to the functions via the normal permissions system.  I'm not
really sure that's a good idea though- it might be better to have a
clean break here (and one which is clearly in the more secure/explicit
direction) and tell admins in the release notes to GRANT EXECUTE on the
functions for the roles they want to give permission to.  We could also
duplicate the functions and have the existing ones remain as-is and have
the new ones just depend on the GRANT system, but I don't particularly
like that since I'm afraid we'd end up in the same boat we're in now
with pg_user and friends.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  REVOKE'ing access *without* removing the permissions checks would defeat
  the intent of these changes, which is to allow an administrator to grant
  the ability for a certain set of users to cancel and/or terminate
  backends started by other users, without also granting those users
  superuser rights.
 
 I see: we have two different use-cases and no way for GRANT/REVOKE
 to manage both cases using permissions on a single object.  Carry
 on then.

Alright, after going about this three or four different ways, I've
settled on the approach proposed in the attached patch.  Most of it is
pretty straight-forward: drop the hard-coded check in the function
itself and REVOKE EXECUTE on the function from PUBLIC.  The interesting
bits are those pieces which are more complex than the all-or-nothing
cases.

This adds a few new SQL-level functions which return unfiltered results,
if you're allowed to call them based on the GRANT system (and EXECUTE
privileges for them are REVOKE'd from PUBLIC, of course).  These are:
pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and
pg_signal_backend (which is similar but not the same- that allows you to
send signals to other backends as a regular user, if you are allowed to
call that function and the other backend wasn't started by a superuser).

There are two new views added, which simply sit on top of the new
functions: pg_stat_activity_all and pg_stat_replication_all.

Minimizing the amount of code duplication wasn't too hard with the
pg_stat_get_wal_senders case; as it was already returning a tuplestore
and so I simply moved most of the logic into a helper function which
handled populating the tuplestore and then each call path was relatively
small and mostly boilerplate.  pg_stat_get_activity required a bit more
in the way of changes, but they were essentially to modify it to return
a tuplestore like pg_stat_get_wal_senders, and then add in the extra
function and boilerplate for the non-filtered path.

I had considered (and spent a good bit of time implementing...) a number
of alternatives, mostly around trying to do more at the SQL level when
it came to filtering, but that got very ugly very quickly.  There's no
simple way to find out what was the effective role of the caller of
this function from inside of a security definer function (which would
be required for the filtering, as it would need to be able to call the
function underneath).  This is necessary, of course, because functions
in views run as the caller of the view, not as the view owner (that's
useful in some cases, less useful in others).  I looked at exposing the
information about the effective role which was calling a function, but
that got pretty ugly too.  The GUC system has a stack, but we don't
actually update the 'role' GUC when we are in security definer
functions.  There's the notion of an outer role ID, but that doesn't
account for intermediate security definer functions and so, while it's
fine for what it does, it wasn't really helpful in this case.

I do still think it'd be nice to provide that information and perhaps we
can do so with fmgr_security_definer(), but it's beyond what's really
needed here and I don't think it'd end up being particularly cleaner to
do it all in SQL now that I've refactored pg_stat_get_activity.

I'd further like to see the ability to declare on either a function call
by function call basis (inside a view defintion), of even at the view
level (as that would allow me to build up multiple views with different
parameters) who to run this function as, where the options would be
view owner or view querier, very similar to our SECURITY DEFINER vs.
SECURITY INVOKER options for functions today.  This is interesting
because, more and more, we're building functions which are actually
returning data sets, not individual values, and we want to filter those
sets sometimes and not other times.  In any case, those are really
thoughts for the future and get away from what this is all about, which
is reducing the need for monitoring and/or regular admins to have the
superuser bit when they don't really need it.

Clearly, further testing and documentation is required and I'll be
getting to that over the next couple of days, but it's pretty darn late
and I'm currently getting libpq undefined reference errors, probably
because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)

Thoughts?

Thanks!

Stephen
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
new file mode 100644
index 212fd1d..538ebdc
*** a/contrib/test_decoding/expected/permissions.out
--- b/contrib/test_decoding/expected/permissions.out
*** SET synchronous_commit = on;
*** 4,9 
--- 4,16 
  CREATE ROLE lr_normal;
  CREATE ROLE lr_superuser SUPERUSER;
  CREATE ROLE lr_replication REPLICATION;
+ GRANT EXECUTE ON FUNCTION 

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  ... Lastly, there is the question of pg_cancel_backend and
  pg_terminate_backend.  My thinking on this is to create a new
  'pg_signal_backend' which admins could grant access to and leave the
  existing functions alone (modulo the change for has_privs_of_role as
  discussed previously).  We'd rename the current 'pg_signal_backend' to
  something else (maybe '_helper'); it's not exposed anywhere and
  therefore renaming it shouldn't cause any heartache.
 
 That seems fairly ugly.  Why would we need a new, duplicative function
 here?  (Apologies if the reasoning was spelled out upthread, I've not
 been paying much attention.)

Currently, those functions allow users to signal backends which are
owned by them, which means they can be used by anyone.  Simply
REVOKE'ing access to them would remove that capability and an admin who
then GRANT's access to the function would need to understand that
they're allowing that user the ability to cancel/terminate any backends
(except those initiated by superusers, at least if we keep that check as
discussed upthread).

If those functions just had simply superuser() checks that prevented
anyone else from using them then this wouldn't be an issue.

REVOKE'ing access *without* removing the permissions checks would defeat
the intent of these changes, which is to allow an administrator to grant
the ability for a certain set of users to cancel and/or terminate
backends started by other users, without also granting those users
superuser rights.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

All,

* Stephen Frost (sfr...@snowman.net) wrote:
 Alright, I've got an initial patch to do this for pg_start/stop_backup,
 pg_switch_xlog, and pg_create_restore_point.  The actual backend changes
 are quite small, as expected.  I'll add in the changes for the other
 functions being discussed and adapt the documentation changes from
 the earlier patch to make sense, but what I'd really appreciate are any
 comments or thoughts regarding the changes to pg_dump (which are generic
 to all of the function changes, of course).

So, I've tested this approach with extensions and binary upgrade and
things appear to all be working correctly, but there's a few issues
remaining to discuss:

The functions pg_start_backup and pg_stop_backup can currently be run by
replication roles but those roles won't have any permissions on those
functions with the new approach unless we GRANT those rights during
pg_dump and/or pg_upgrade.  Note that this isn't an issue for tools
which use the replication protocol (eg: pg_basebackup) since the
replication protocol calls do_pg_start_bacup() directly.  As such, my
first question is- do we want to worry about this?  We should certainly
mention it in the release notes but I'm not sure if we really want to do
anything else.

The second question is in regards to pg_stat_activity.  My first
suggestion for how to address that would be to have the function return
everything and then have the view perform the filtering to return only
what's shown today for users.  Admins could then grant access to the
function for whichever users they want to have access to everything.
That strikes me as the best way to avoid changing common usage while
still providing the flexibility.  Another option would be to still
invent the MONITOR role attribute and keep the rest the same.  Again,
we'd want to mention something in the release notes regarding this
change.

Lastly, there is the question of pg_cancel_backend and
pg_terminate_backend.  My thinking on this is to create a new
'pg_signal_backend' which admins could grant access to and leave the
existing functions alone (modulo the change for has_privs_of_role as
discussed previously).  We'd rename the current 'pg_signal_backend' to
something else (maybe '_helper'); it's not exposed anywhere and
therefore renaming it shouldn't cause any heartache.

For pg_create_restore_point, pg_switch_xlog, pg_xlog_replay_pause,
pg_xlog_replay_resume, pg_rotate_logfile, we can just remove the
if(!superuser()) ereport() checks and REVOKE rights on them during the
initdb process, since there isn't anything complicated happening for
those today.

One other question which I've been thinking about is if we want to try
and preserve permission changes associated with other catalog objects
(besides functions), or if we even want to change how functions are
handled regarding permission changes.  We don't currently preserve any
such changes across dump/restore or even binary upgrades and this would
be changing that.  I don't recall any complaints about not preserving
permission changes to objects in pg_catalog, but one could argue that
our lack of doing so today is a bug.

Thoughts?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 ... Lastly, there is the question of pg_cancel_backend and
 pg_terminate_backend.  My thinking on this is to create a new
 'pg_signal_backend' which admins could grant access to and leave the
 existing functions alone (modulo the change for has_privs_of_role as
 discussed previously).  We'd rename the current 'pg_signal_backend' to
 something else (maybe '_helper'); it's not exposed anywhere and
 therefore renaming it shouldn't cause any heartache.

That seems fairly ugly.  Why would we need a new, duplicative function
here?  (Apologies if the reasoning was spelled out upthread, I've not
been paying much attention.)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 That seems fairly ugly.  Why would we need a new, duplicative function
 here?  (Apologies if the reasoning was spelled out upthread, I've not
 been paying much attention.)

 Currently, those functions allow users to signal backends which are
 owned by them, which means they can be used by anyone.  Simply
 REVOKE'ing access to them would remove that capability and an admin who
 then GRANT's access to the function would need to understand that
 they're allowing that user the ability to cancel/terminate any backends
 (except those initiated by superusers, at least if we keep that check as
 discussed upthread).

 If those functions just had simply superuser() checks that prevented
 anyone else from using them then this wouldn't be an issue.

 REVOKE'ing access *without* removing the permissions checks would defeat
 the intent of these changes, which is to allow an administrator to grant
 the ability for a certain set of users to cancel and/or terminate
 backends started by other users, without also granting those users
 superuser rights.

I see: we have two different use-cases and no way for GRANT/REVOKE
to manage both cases using permissions on a single object.  Carry
on then.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Peter, all,

* Peter Eisentraut (pete...@gmx.net) wrote:
 Why are we not using roles and function execute privileges for this?

Alright, I've got an initial patch to do this for pg_start/stop_backup,
pg_switch_xlog, and pg_create_restore_point.  The actual backend changes
are quite small, as expected.  I'll add in the changes for the other
functions being discussed and adapt the documentation changes from
the earlier patch to make sense, but what I'd really appreciate are any
comments or thoughts regarding the changes to pg_dump (which are generic
to all of the function changes, of course).

I've added a notion of the catalog schema to pg_dump's internal
_namespaceinfo representation and then marked pg_catalog as being that
schema, as well as being a dumpable schema.  Throughout the
selectDumpable functions, I've made changes to only mark the objects in
the catalog as dumpable if they are functions.  I'm planning to look
into the extension and binary upgrade paths since I'm a bit worried
those may not work with this approach, but I wanted to get this out
there for at least an initial review as, if people feel this makes
things too ugly on the pg_dump side of things then we may want to
reconsider using the role attributes instead.

Thanks!

Stephen
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 2179bf7..36029d0
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
*** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,64 
  
  	backupidstr = text_to_cstring(backupid);
  
- 	if (!superuser()  !has_rolreplication(GetUserId()))
- 		ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 		   errmsg(must be superuser or replication role to run a backup)));
- 
  	startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
  
  	PG_RETURN_LSN(startpoint);
--- 54,59 
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,92 
  {
  	XLogRecPtr	stoppoint;
  
- 	if (!superuser()  !has_rolreplication(GetUserId()))
- 		ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 		 (errmsg(must be superuser or replication role to run a backup;
- 
  	stoppoint = do_pg_stop_backup(NULL, true, NULL);
  
  	PG_RETURN_LSN(stoppoint);
--- 77,82 
*** pg_switch_xlog(PG_FUNCTION_ARGS)
*** 100,110 
  {
  	XLogRecPtr	switchpoint;
  
- 	if (!superuser())
- 		ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 			 (errmsg(must be superuser to switch transaction log files;
- 
  	if (RecoveryInProgress())
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--- 90,95 
*** pg_create_restore_point(PG_FUNCTION_ARGS
*** 129,139 
  	char	   *restore_name_str;
  	XLogRecPtr	restorepoint;
  
- 	if (!superuser())
- 		ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-  (errmsg(must be superuser to create a restore point;
- 
  	if (RecoveryInProgress())
  		ereport(ERROR,
  (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--- 114,119 
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
new file mode 100644
index 2800f73..38605de
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** RETURNS interval
*** 897,899 
--- 897,907 
  LANGUAGE INTERNAL
  STRICT IMMUTABLE
  AS 'make_interval';
+ 
+ -- Revoke privileges for functions that should not be available to
+ -- all users.  Administrators are allowed to change this later, if
+ -- they wish.
+ REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean) FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_switch_xlog() FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
new file mode 100644
index fdfb431..164608a
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*** selectDumpableNamespace(NamespaceInfo *n
*** 1234,1245 
--- 1234,1255 
  	 * If specific tables are being dumped, do not dump any complete
  	 * namespaces. If specific namespaces are being dumped, dump just those
  	 * namespaces. Otherwise, dump all non-system namespaces.
+ 	 *
+ 	 * Note that we do consider dumping ACLs of functions in pg_catalog,
+ 	 * so mark that as a dumpable namespace, but further mark it as the
+ 	 * catalog namespace.
  	 */
+ 
+ 	/* Will be set when we may be dumping catalog ACLs, see below. */
+ 	nsinfo-catalog = false;
+ 
  	if (table_include_oids.head != NULL)
  		nsinfo-dobj.dump = false;
  	else if (schema_include_oids.head != NULL)
  		nsinfo-dobj.dump = simple_oid_list_member(schema_include_oids,
     nsinfo-dobj.catId.oid);
+ 	else if (strncmp(nsinfo-dobj.name, pg_catalog, 10) == 0)
+ 		nsinfo-dobj.dump = nsinfo-catalog = true;
  	else if 

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Peter Eisentraut (pete...@gmx.net) wrote:
 On 2/28/15 10:10 PM, Stephen Frost wrote:
  * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
  I have attached and updated patch for review.
  
  Thanks!  I've gone over this and made quite a few documentation and
  comment updates, but not too much else, so I'm pretty happy with how
  this is coming along.  As mentioned elsewhere, this conflicts with the
  GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling
  both this patch and that one, I'll find some way to manage. :)
  
  Updated patch attached.  Barring objections, I'll be moving forward with
  this soonish.  Would certainly appreciate any additional testing or
  review that you (or anyone!) has time to provide.
 
 Let's move this discussion to the right thread.

Agreed.

 Why are we not using roles and function execute privileges for this?

There isn't a particular reason not to, except that the existing checks
are in C code and those would need to be removed and the permission
changes done at initdb time to revoke EXECUTE from PUBLIC for these
functions.  Further, as you pointed out, we'd need to dump out the
permissions for the catalog tables and functions with this approach.  I
don't expect that to be too difficult to do though.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Peter Eisentraut]

On 2/28/15 10:10 PM, Stephen Frost wrote:
 Adam,
 
 * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
 I have attached and updated patch for review.
 
 Thanks!  I've gone over this and made quite a few documentation and
 comment updates, but not too much else, so I'm pretty happy with how
 this is coming along.  As mentioned elsewhere, this conflicts with the
 GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling
 both this patch and that one, I'll find some way to manage. :)
 
 Updated patch attached.  Barring objections, I'll be moving forward with
 this soonish.  Would certainly appreciate any additional testing or
 review that you (or anyone!) has time to provide.

Let's move this discussion to the right thread.

Why are we not using roles and function execute privileges for this?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Adam Brightwell]

Alvaro,

I thought I saw a comment about using underscore to separate words in
 privilege names, such as EXCLUSIVE_BACKUP rather than running it all
 together.  Was that idea discarded?


I'm not sure there was an actual discussion on the topic.  Though, at one
point I had proposed it as one of the forms of this attribute.  Personally,
I think it is easier to read with the underscore.  But, ultimately, I
defaulted to no underscore to remain consistent with the other attributes,
such as CREATEDB and CREATEROLE.

Thanks,
Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Re: [HACKERS] Additional role attributes superuser review

[Alvaro Herrera]

Adam Brightwell wrote:
 Alvaro,
 
  I thought I saw a comment about using underscore to separate words
  in privilege names, such as EXCLUSIVE_BACKUP rather than running it
  all together.  Was that idea discarded?
 
 I'm not sure there was an actual discussion on the topic.  Though, at one
 point I had proposed it as one of the forms of this attribute.  Personally,
 I think it is easier to read with the underscore.  But, ultimately, I
 defaulted to no underscore to remain consistent with the other attributes,
 such as CREATEDB and CREATEROLE.

If we were choosing those names nowadays, would we choose CREATEDB at
all in the first place?  I think we'd go for something more verbose,
probably CREATE_DATABASE.  (CREATEROLE is not as old as CREATEDB, but my
bet is that it was modelled after CREATEUSER without considering the
whole readability topic too much.)

Anyway it doesn't seem to me that consistency with lack of separators in
those very old names should be our guiding principle here.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 If we were choosing those names nowadays, would we choose CREATEDB at
 all in the first place?  I think we'd go for something more verbose,
 probably CREATE_DATABASE.  (CREATEROLE is not as old as CREATEDB, but my
 bet is that it was modelled after CREATEUSER without considering the
 whole readability topic too much.)
 
 Anyway it doesn't seem to me that consistency with lack of separators in
 those very old names should be our guiding principle here.

 So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP?  Or
 NO_EXCLUSIVE_BACKUP?  Or..?  If this was a green field, I think we might
 actually use spaces instead, but I'm really not sure we want to go
 through and redo everything that way at this point..  We'd end up
 breaking a lot of scripts that currently work today and I'm really not
 convinced it's better enough to justify that.

FWIW, I agree with Alvaro, and I'd go with e.g. NO_EXCLUSIVE_BACKUP.

I concur that multiple separate words would be a syntax mess, but I
see no reason not to use underscores instead.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Adam Brightwell wrote:
  I'm not sure there was an actual discussion on the topic.  Though, at one
  point I had proposed it as one of the forms of this attribute.  Personally,
  I think it is easier to read with the underscore.  But, ultimately, I
  defaulted to no underscore to remain consistent with the other attributes,
  such as CREATEDB and CREATEROLE.
 
 If we were choosing those names nowadays, would we choose CREATEDB at
 all in the first place?  I think we'd go for something more verbose,
 probably CREATE_DATABASE.  (CREATEROLE is not as old as CREATEDB, but my
 bet is that it was modelled after CREATEUSER without considering the
 whole readability topic too much.)
 
 Anyway it doesn't seem to me that consistency with lack of separators in
 those very old names should be our guiding principle here.

So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP?  Or
NO_EXCLUSIVE_BACKUP?  Or..?  If this was a green field, I think we might
actually use spaces instead, but I'm really not sure we want to go
through and redo everything that way at this point..  We'd end up
breaking a lot of scripts that currently work today and I'm really not
convinced it's better enough to justify that.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  If we were choosing those names nowadays, would we choose CREATEDB at
  all in the first place?  I think we'd go for something more verbose,
  probably CREATE_DATABASE.  (CREATEROLE is not as old as CREATEDB, but my
  bet is that it was modelled after CREATEUSER without considering the
  whole readability topic too much.)
  
  Anyway it doesn't seem to me that consistency with lack of separators in
  those very old names should be our guiding principle here.
 
  So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP?  Or
  NO_EXCLUSIVE_BACKUP?  Or..?  If this was a green field, I think we might
  actually use spaces instead, but I'm really not sure we want to go
  through and redo everything that way at this point..  We'd end up
  breaking a lot of scripts that currently work today and I'm really not
  convinced it's better enough to justify that.
 
 FWIW, I agree with Alvaro, and I'd go with e.g. NO_EXCLUSIVE_BACKUP.

Ok, works for me.  Will update (including tab completion) and send out
for review.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Alvaro Herrera]

Stephen Frost wrote:
 Alvaro,
 
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

  That being so, I would consider the idea that the NO bit is a separate
  word rather than run together with the actual privilege name.  And given
  that CREATE has all the options default to NO, there is no need to
  have these options at all in CREATE, is there?
 
 That's a good point, except that INHERIT is actually on by default, and
 LOGIN defaults to 'on' if you use CREATE USER, and 'off' if you use
 CREATE ROLE.  If they were actually all 'no' by default then this
 simplication would work but it's not and therefore I don't think we want
 to have some which are allowed at CREATE time with 'on' and some with
 'off' depending on whatever the default is.  Today, you can write a
 script to easily duplicate an existing role by just looking at what is
 on and off and using X and NOX.  This approach would require that script
 to know what's valid at CREATE time and what isn't.

All true.


  where privilege can be:
 SUPERUSER | CREATEDB | CREATEROLE
   | CREATEUSER | INHERIT | LOGIN
   | REPLICATION | EXCLUSIVE_BACKUP | ...
  and option can be:
 CONNECTION LIMIT connlimit
   | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
   | VALID UNTIL 'timestamp'
 
 With the 'NO' distinct, as discussed above, it seems like we should be
 able to support this..  I certainly like it more too.

Great.

 Certainly, and I like where you're going with this, just seems like
 there's a couple wrinkles to deal with.

Yeah.

Let's go with the NO_ prefix then ... that seems better to me than no
separator.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Let's go with the NO_ prefix then ... that seems better to me than no
 separator.

Works for me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Alvaro Herrera]

Stephen Frost wrote:

 Thanks!  I've gone over this and made quite a few documentation and
 comment updates, but not too much else, so I'm pretty happy with how
 this is coming along.  As mentioned elsewhere, this conflicts with the
 GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling
 both this patch and that one, I'll find some way to manage. :)
 
 Updated patch attached.  Barring objections, I'll be moving forward with
 this soonish.  Would certainly appreciate any additional testing or
 review that you (or anyone!) has time to provide.

I thought I saw a comment about using underscore to separate words in
privilege names, such as EXCLUSIVE_BACKUP rather than running it all
together.  Was that idea discarded?


-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost wrote:
 
  Thanks!  I've gone over this and made quite a few documentation and
  comment updates, but not too much else, so I'm pretty happy with how
  this is coming along.  As mentioned elsewhere, this conflicts with the
  GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling
  both this patch and that one, I'll find some way to manage. :)
  
  Updated patch attached.  Barring objections, I'll be moving forward with
  this soonish.  Would certainly appreciate any additional testing or
  review that you (or anyone!) has time to provide.
 
 I thought I saw a comment about using underscore to separate words in
 privilege names, such as EXCLUSIVE_BACKUP rather than running it all
 together.  Was that idea discarded?

Hrm, I'm not finding that on a quick look through the archives.  Looking
at the other role options, we don't have any with underscores but we do
have a couple of cases where we uses spaces, eg:

CONNECTION LIMIT
VALID UNTIL

So, using EXCLUSIVE BACKUP would be similar to those, but we'd then need
to have the 'NO' version as 'NO EXCLUSIVE BACKUP', I'd think, and I'm
not sure we really want to go there.

On the other hand, we do run together the 'CREATE' options (CREATEDB,
CREATEROLE), and I've not heard anyone suggesting to change those.

I guess for my part, at least, having it run together as
'EXCLUSIVEBACKUP' seems fine, but this does make me realize that I need
to go add tab-completion for these new options. :)

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Additional role attributes superuser review

[Stephen Frost]

Adam,

* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
 I have attached and updated patch for review.

Thanks!  I've gone over this and made quite a few documentation and
comment updates, but not too much else, so I'm pretty happy with how
this is coming along.  As mentioned elsewhere, this conflicts with the
GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling
both this patch and that one, I'll find some way to manage. :)

Updated patch attached.  Barring objections, I'll be moving forward with
this soonish.  Would certainly appreciate any additional testing or
review that you (or anyone!) has time to provide.

Thanks again!

Stephen
From 58efbb5ebeadca15fb2f3ada4ca5c83b7ddd5d69 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Sat, 28 Feb 2015 20:23:38 -0500
Subject: [PATCH] Add role attributes for various operations

Historically, many operations have been restricted to the superuser.
These additional role attributes allow an administrator to delegate the
right to run some of these operations out to other roles, reducing the
number of cases which strictly require superuser rights and therefore,
hopefully, reducing the number of users running as superuser in the
wild.

This patch introduces the following role attributes:

EXCLUSIVEBACKUP - allows the role to run pg_start_backup,
pg_stop_backup, pg_create_restore_point, and pg_switch_xlog.

XLOGREPLAY - allows the role to run pg_xlog_replay_pause and
pg_xlog_replay_resume.

LOGFILE - allows the role to run pg_rotate_logfile

MONITOR - allows the role to see the details of all running processes
(including both normal backends and replication backends).  The
documentation and code are also updated to use has_privs_of_role().

SIGNAL - allows the role to signal all other normal backends (except
those initiated by a superuser) with pg_cancel_backend and
pg_termiante_backend.  The documentation and code for the NOSIGNAL case
are also updated to use has_privs_of_role().

In passing, this also centralizes and standardizes the REPLICATION and
CREATEROLE support functions.

Lots of discussion, review, and commentary from Jim Nasby, Simon,
Magnus, Alvaro, and Robert.

Authored by Adam, bugs and various documentation updates from
me.
---
 doc/src/sgml/catalogs.sgml |  30 
 doc/src/sgml/func.sgml |  38 +++--
 doc/src/sgml/ref/create_role.sgml  |  81 +++
 doc/src/sgml/ref/create_user.sgml  |   6 +
 src/backend/access/transam/xlogfuncs.c |  27 ++--
 src/backend/catalog/aclchk.c   | 137 ++
 src/backend/commands/user.c| 183 ++---
 src/backend/parser/gram.y  |  20 +++
 src/backend/replication/logical/logicalfuncs.c |  15 +-
 src/backend/replication/slotfuncs.c|  25 ++--
 src/backend/replication/walsender.c|   8 +-
 src/backend/utils/adt/misc.c   |  19 ++-
 src/backend/utils/adt/pgstatfuncs.c|  54 ++--
 src/backend/utils/init/miscinit.c  |  19 ---
 src/backend/utils/init/postinit.c  |   2 +-
 src/include/catalog/pg_authid.h|  20 ++-
 src/include/miscadmin.h|   1 -
 src/include/utils/acl.h|   6 +
 18 files changed, 585 insertions(+), 106 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 515a40e..77b1090 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1454,6 +1454,36 @@
  /row
 
  row
+  entrystructfieldrolexclbackup/structfield/entry
+  entrytypebool/type/entry
+  entryRole can perform on-line exclusive backup operations/entry
+ /row
+
+ row
+  entrystructfieldrolxlogreplay/structfield/entry
+  entrytypebool/type/entry
+  entryRole can control xlog recovery replay operations/entry
+ /row
+
+ row
+  entrystructfieldrollogfile/structfield/entry
+  entrytypebool/type/entry
+  entryRole can rotate log files/entry
+ /row
+
+ row
+  entrystructfieldrolmonitor/structfield/entry
+  entrytypebool/type/entry
+  entryRole can view pg_stat_* details/entry
+ /row
+
+ row
+  entrystructfieldrolsignal/structfield/entry
+  entrytypebool/type/entry
+  entryRole can signal normal backend processes/entry
+ /row
+
+ row
   entrystructfieldrolconnlimit/structfield/entry
   entrytypeint4/type/entry
   entry
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index da2ed67..7842cfc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -16261,8 +16261,7 @@ SELECT set_config('log_statement_stats', 'off', false);
para
 The functions shown in xref
 linkend=functions-admin-signal-table send control signals to
-other server processes.  Use of these functions is usually