Re: Monitoring roles patch - Mailing list pgsql-hackers

Hi

On Tue, Mar 28, 2017 at 2:22 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Dave,
>
> * Dave Page (dpage@pgadmin.org) wrote:
>> OK, so before I start hacking again, here's a proposal based on my
>> understanding of folks comments, and so open questions. If I can get
>> agreement and answers, I'll be able to break out vi again without
>> (hopefully) too many more revisions:
>>
>> pg_read_all_stats: Will have C-coded access to pg_stats views and
>> pg_*_size that are currently hard-coded to superuser
>
> Not quite sure what you mean here by 'C-coded access to pg_stats
> *views*', but I'm guessing that isn't exactly what you meant since I'm
> sure we aren't going to change how view permissions are done in this
> patch.  I take it you mean access to the functions under the views?  If
> so, I believe this is correct.

Right, sorry I wasn't clear.

>> pg_read_all_settings: Will have C-coded access to read GUCs that are
>> currently hard-coded to the superuser
>
> Right.
>
>> pg_monitor: Will include pg_read_all_stats and pg_read_all_settings,
>> and all explicitly GRANTable rights, e.g. in contrib modules.
>
> Right.
>
>> Patch to be rebased on Simon's updated version.
>
> Right.
>
>> Questions:
>>
>> - pg_stat_statements has a hard-coded superuser check. Shall I remove
>> that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor?
>
> pg_stat_statements shouldn't have ever had that superuser check and it
> shouldn't have ever used '==' for the user check, it should have been
> using 'has_privs_of_role()' from the start, which means that the
> superuser check isn't necessary.
>
> I don't think we should remove that check, nor should we REVOKE EXECUTE
> from public for the function.  We *should* add a hard-coded role check
> to allow another role which isn't a member of the role whose query it is
> to view the query.  That shouldn't be pg_monitor, of course (as
> discussed).  I don't think pg_read_all_stats or pg_read_all_settings
> really covers this case either- this is more like pg_read_all_queries
> and should also be used for pg_stat_activity.

OK, so essentially what I did, except s/pg_read_all_stats/pg_read_all_queries ?

> That would then be granted to pg_monitor.
>
>> - pgrowlocks has hard-coded access to superuser and users with SELECT
>> on the table being examined. How should this be handled?
>
> I don't see any hard-coded superuser check?  There is a
> pg_class_aclcheck() for SELECT rights on the table.  I like the idea of
> having another way to grant access to run this function on a table
> besides giving SELECT rights on the entire table to the user.  This
> would fall under the mandate of the role described in your next bullet,
> in my view.
>
>> - Stephen suggested a separate role for functions that can lock
>> tables. Is this still desired, or shall we just grant access to
>> pg_monitor (I think the latter is fine)?
>
> Right, I was thinking something like pg_stat_all_tables or
> pg_stat_scan_tables or similar.  We would add that (perhaps also with a
> SELECT check like pgrowlocks has) for the other functions like
> pgstattuple and pg_freespacemap and pg_visibility.

So pgstattuple, pg_sfreespacemap, pg_visibility and pgrowlocks to be
allowed access from members of pg_stat_scan_tables, which in turn is
granted to pg_monitor?

Thanks!

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

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