Re: pg_monitor role

Dave,

* Dave Page (dpage(at)pgadmin(dot)org) wrote:
> On Wed, Feb 22, 2017 at 4:52 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> > > 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.
>
> I'm not convinced we should include it, for the reason I gave above.
> However, I don't feel that strongly.
>
> What modules should be included?

On a quick review of all of the modules, excluding those that are just
testing or examples or which can already be used by non-superusers by
default, and excluding those which can be used to trivially gain
superuser access (adminpack and pageinspect), I came up with:

pg_buffercache
pg_freespacemap
pgrowlocks
pg_stat_statements
pgstattuple
pg_visibility

Reviewing this list, they all seem like things a monitoring user could
have a use for and none of them allow direct access to table data from
what I could tell on a quick review. Obviously, a more detailed review
of each should be done to make sure I didn't miss something.

One interesting thing that comes up from this list is that there's a
number of things which are "look at something about a row" or "look at
something about a block" (pg_freespacemap, pgrowlocks, pgstattuple,
pg_visibility all fall into those, and to some extent pg_buffercache
too). I'm tempted to suggest that we have a role which covers that
theme (and is then GRANT'd to pg_monitor).

> >> pgstattuple can be discussed. It doesn't leak anything dangerous. But it
> >> does have views that are quite expensive.
>
> I don't think expense should be a concern. It's not like a regular
> user cannot run something expensive already, so why stop a user
> specifically setup to monitor something?

I tend to agree with this.

> > 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.
>
> I'm fine with having pg_read_all_gucs - it's a trivial change. I
> wouldn't want us to go too far and end up with separate roles for
> everything under the sun though.

I agree with you there- having too many default roles would lead to
things getting messy, without there really being a need for it. Users
can always create their own roles for the specific set of capabilities
that they want to provide. The main thing I want to avoid is having a
situation where a user *can't* create a role that has only a subset of
what "pg_monitor" has because there's some code somewhere that
explicitly allows the "pg_monitor" role to do something.

> > #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 clarity on what
> > "pg_stat_X" *is* and provides. We can then later add such views and
> > GRANT access to them to pg_monitor.
> >
> > - Create new versions of pg_monitor in the future that cover ever
> > increasing sets of privileges ("pg_monitor_with_pg_stat_X" or
> > "pg_monitor_v11" for PG11 or something).
>
> I prefer the first option. In my experience, users don't much care
> about the rights their monitoring user has, as long as it's not full
> superuser. The only case where I think there are legitimate concerns
> are where you can read arbitrary data (I do not consider query strings
> to be in that class for the record). That said, if we ever do add
> something like that then there's nothing stopping us from explicitly
> documenting that it's excluded from pg_monitor for that reason, and if
> desired the user can grant on it as needed.
>
> Using a scheme like that would also mean that the user is more likely
> to need to manually update the role their monitoring system uses
> following an upgrade.

I prefer the first option too, but that means you have work to do to
define what "pg_stat_X" means/covers in a way that, hopefully, future
hackers will remember and adhere to. ;)

I'd rather we try to avoid having exceptions as it can lead to confusion
and adds complexity that security folks would almost certainly prefer to
not have to deal with.

> > 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.
>
> No, but similarly I don't see any reason why we cannot add new views
> by default, and exclude by exception when there's a compelling reason.

I'd rather simply name whatever that exception is 'pg_somethingelse'.
There would have to be a really, really good reason to have a
'pg_stat_X' that isn't something that pg_monitor can have access to.

Now, perhaps what that means is that we should really name these things
something else or have aliases to them that is what pg_monitor is given,
so we get away from the issue that "statistics" stuff might have
something sensitive included that we don't want pg_monitor to have
access to (eg: pg_mon_statements, pg_mon_activity, pg_mon_whatever ...).

I guess that ends up depending on what we can come up with to define
what "pg_stat_X" is and if everyone can more-or-less agree to that (and,
subsequently, agree that pg_monitor should have access to all of that).

Thanks!

Stephen