Re: Read access for pg_monitor to pg_replication_origin_status view

Greetings,

* Martín Marqués (martin(at)2ndquadrant(dot)com) wrote:
> > > $ git diff
> > > diff --git a/src/backend/catalog/system_views.sql
> > > b/src/backend/catalog/system_views.sql
> > > index c16061f8f00..97ee72a9cfc 100644
> > > --- a/src/backend/catalog/system_views.sql
> > > +++ b/src/backend/catalog/system_views.sql
> > > @@ -1494,9 +1494,6 @@ GRANT EXECUTE ON FUNCTION
> > > pg_ls_archive_statusdir() TO pg_monitor;
> > > GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
> > > GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
> > >
> > > -GRANT EXECUTE ON FUNCTION pg_replication_origin_progress(text,
> > > boolean) TO pg_monitor;
> > > -GRANT EXECUTE ON FUNCTION
> > > pg_replication_origin_session_progress(boolean) TO pg_monitor;
> > > -
> > > GRANT pg_read_all_settings TO pg_monitor;
> > > GRANT pg_read_all_stats TO pg_monitor;
> > > GRANT pg_stat_scan_tables TO pg_monitor;
> >
> >
> > Agreed on this part. The two functions aren't needed to be granted.
> >
> > But, pg_show_replication_origin_status() should be allowed
> > pg_read_all_stats, not pg_monitor. pg_monitor is just a union role of
> > actual privileges.
>
> I placed that GRANT on purpose to `pg_monitor`, separated from the
> `pg_read_all_stats` role, because it doesn't match the description for
> that role.
>
> ```
> Read all pg_stat_* views and use various statistics related
> extensions, even those normally visible only to superusers.
> ```
>
> I have no problem adding it to this ROLE, but we'd have to amend the
> doc for default-roles to reflect that SELECT for this view is also
> granted to `pg_read_all_stats`.

I agree in general that pg_monitor shouldn't have privileges granted
directly to it. If this needs a new default role, that's an option, but
it seems like it'd make sense to be part of pg_read_all_stats to me, so
amending the docs looks reasonable from here.

> > Another issue would be how to control output of
> > pg_show_replication_origin_status(). Most of functions that needs
> > pg_read_all_stats privileges are filtering sensitive columns in each
> > row, instead of hiding the existence of rows. Maybe the view
> > pg_replication_origin_status should show only local_id and hide other
> > columns from non-pg_read_all_stats users.
>
> I think that the output from `pg_show_replication_origin_status()`
> doesn't expose any data that `pg_read_all_stats` or `pg_monitor`
> shouldn't be able to read. Removing or obfuscating `external_id`
> and/or `remote_lsn` would make the view somehow pointless, in
> particular for monitoring and diagnostic tools.

Yeah, pg_read_all_stats is a rather privileged role when it comes to
reading data, consider that it can see basically everything in
pg_stat_activity, for example.

Thanks,

Stephen