Re: [HACKERS] pg_monitor role

All,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Wed, Feb 22, 2017 at 1:47 PM, Dave Page <dpage@pgadmin.org> wrote:
> > On Tue, Feb 21, 2017 at 5:40 PM, Masahiko Sawada <sawada.mshk@gmail.com>
> > wrote:
> > > On Mon, Feb 20, 2017 at 8:48 PM, Dave Page <dpage@pgadmin.org> wrote:
> > >> Further to the patch I just submitted
> > >> (https://www.postgresql.org/message-id/CA%2BOCxow-X%
> > 3DD2fWdKy%2BHP%2BvQ1LtrgbsYQ%3DCshzZBqyFT5jOYrFw%40mail.gmail.com)
> > >> I'd like to propose the addition of a default role, pg_monitor.
> > >>
> > >> The intent is to make it easy for users to setup a role for fully
> > >> monitoring their servers, without requiring superuser level privileges
> > >> which is a problem for many users working within strict security
> > >> policies.
> > >>
> > >> At present, functions or system config info that divulge any
> > >> installation path related info typically require superuser privileges.
> > >> This makes monitoring for unexpected changes in configuration or
> > >> filesystem level monitoring (e.g. checking for large numbers of WAL
> > >> files or log file info) impossible for non-privileged roles.
> > >>
> > >> A similar example is the restriction on the pg_stat_activity.query
> > >> column, which prevents non-superusers seeing any query strings other
> > >> than their own.
> > >>
> > >> Using ACLs is a problem for a number of reasons:
> > >>
> > >> - Users often don't like their database schemas to be modified
> > >> (cluttered with GRANTs).
> > >> - ACL modifications would potentially have to be made in every
> > >> database in a cluster.
> > >> - Using a pre-defined role minimises the setup that different tools
> > >> would have to require.
> > >> - Not all functionality has an ACL (e.g. SHOW)
> > >>
> > >> Other DBMSs solve this problem in a similar way.
> > >>
> > >> Initially I would propose that permission be granted to the role to:
> > >>
> > >> - Execute pg_ls_logdir() and pg_ls_waldir()
> > >> - Read pg_stat_activity, including the query column for all queries.
> > >> - Allow "SELECT pg_tablespace_size('pg_global')"
> > >> - Read all GUCs
> > >>
> > >
> > > Thank you for working on this.
> >
> > You're welcome.
> >
> > > What about granting to the role to read other statistic views such as
> > > pg_stat_replication and pg_stat_wal_receiver? Since these informations
> > > can only be seen by superuser the for example monitoring and
> > > clustering tool seems to have the same concern.
> >
> > Yes, good point.
>
> I think basically pg_stat_* should be readable by this role.

Agreed.  I would explicitly point out that we do *not* want to include
'pg_statistic' in this as that would include actual data from the
tables.

> > > And what about the diagnostic tools such as pageinspect and pgstattuple?
> >
> > I think external/contrib modules should not be included. To install
> > them you need admin privileges anyway, so you can easily grant
> > whatever usage privileges you want at that time.
>
> I'll start by saying "why not cover contrib"?

+1.

> Then I'll say *absolutely* not pageinspect. That is a diagnostics tool and
> not a monitoring tool. And also, if you give me pageinspect I will happily
> open up your pg_authid and hack your database. This needs to be superuser
> only.

Agreed.

> pgstattuple can be discussed. It doesn't leak anything dangerous. But it
> does have views that are quite expensive.

For my 2c, I think pgstattuple should be included.  It wouldn't be
difficult to just have a GRANT at the end of the extension creation
script to provide the appropriate rights to pg_monitor (or whatever).

> There's also pg_stat_statements, which seems lik eit should be included?
> Any security issues with that one would be the same as with
> pg_stat_activity.

Agreed.

I do see two issues to be addressed with such a role:

#1- We shouldn't just shove everything into one role.  Where
functionality can't be GRANT'd independently of the role, we should have
another default role.  For example, "Read all GUCs" is not something
that can currently be GRANT'd.  I'm sure there are cases where $admin
wants a given role to be able to read all GUCs, but not execute
pg_ls_logdir(), for example.  If we start writing code that refers
explicitly to pg_monitor then we will end up in an "all-or-nothing" kind
of situation (not unlike the superuser case) instead of allowing users a
fine-grained set of options.

That isn't to say that we shouldn't have a pg_monitor role, I'd really
like to have one, actually, but that role should only have rights which
can be GRANT'd to it (either by GRANT'ing other default roles to it, or
by GRANT'ing regular object-level ACLs to it).  What I'm getting at is
that we should have a 'pg_read_all_gucs' default role for the right and
then GRANT that role to pg_monitor.

#2- We need to define very carefully, up-front, how we will deal with
new privileges/capabilities/features down the road.  A very specific
default role like 'pg_read_all_gucs' is quite clear about what's allowed
by it and I don't think we'd get any push-back from adding new GUCs that
such a default role could read, but some new view pg_stat_X view that
would be really useful to monitoring tools might also allow more access
than the pg_monitor has or that some admins would be comfortable with-
how do we handle such a case?  I see a few options:
 - Define up-front that pg_monitor has rights on all pg_stat_X views,which then requires we provide a definition and
clarityon what"pg_stat_X" *is* and provides.  We can then later add such views andGRANT access to them to pg_monitor. 
 - Create new versions of pg_monitor in the future that cover everincreasing sets of privileges
("pg_monitor_with_pg_stat_X"or"pg_monitor_v11" for PG11 or something). 
 - Do not create our own pg_monitor but instead providedocumentation/scripts for users to create their own "monitor"
roles.

It seems at least unlikely that we'll never have another pg_stat_X view
that we want to give pg_monitor access to, so I don't really see "Fix
what pg_monitor can read forever based on what's in PG10" as being a
solution.

Thanks!

Stephen