Re: pg_monitor role

Hi

On Wed, Feb 22, 2017 at 4:52 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > > 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.

Right.

>> > > 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?

>> 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.

+1

>> 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?

> 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.

OK.

> 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.

> 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.

OK.

> #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.

> - Do not create our own pg_monitor but instead provide
> documentation/scripts for users to create their own "monitor" roles.

The whole point here is to minimise the requirements on the user, and
have a good set of default 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.

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.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company