Re: Read access for pg_monitor to pg_replication_origin_status view

Hi, Martin.

At Wed, 3 Jun 2020 13:32:28 -0300, Martín Marqués <martin(at)2ndquadrant(dot)com> wrote in
> Hi Kyotaro-san,
>
> Thank you for taking the time to review my patches. Would you like to
> set yourself as a reviewer in the commit entry here?
> https://commitfest.postgresql.org/28/2577/

Done.

> > 0002:
> >
> > It is forgetting to grant pg_read_all_stats to execute
> > pg_show_replication_origin_status. As the result pg_read_all_stats
> > gets error on executing the function, not on doing select on the view.
>
> Seems I was testing on a cluster where I had already been digging, so
> pg_real_all_stats had execute privileges on
> pg_show_replication_origin_status (I had manually granted that) and
> didn't notice because I forgot to drop the cluster and initialize
> again.
>
> Thanks for the pointer here!

Sorry for not mentioning it at that time, but about the following diff:

+GRANT SELECT ON pg_replication_origin_status TO pg_read_all_stats;

system_views.sql already has a REVOKE command on the view. We should
put the above just below the REVOKE command.

I'm not sure where to put the GRANT on
pg_show_replication_origin_status(), but maybe it also should be at
the same place.

> > 0003:
> >
> > It seems to be a take-after of adminpack's documentation, but a
> > superuser is not the only one on PostgreSQL. The something like the
> > description in 27.2.2 Viewing Statistics looks more suitable.
> >
> > > Superusers and members of the built-in role pg_read_all_stats (see
> > > also Section 21.5) can see all the information about all sessions.
> >
> > Section 21.5 is already saying as follows.
> >
> > > pg_monitor
> > > Read/execute various monitoring views and functions. This role is a
> > > member of pg_read_all_settings, pg_read_all_stats and
> > > pg_stat_scan_tables.
>
> I'm not sure if I got this right, but I added some more text to point
> out that the pg_read_all_stats role can also access one specific
> function. I personally think it's a bit too detailed, and if we wanted
> to add details it should be formatted differently, which would require
> a more invasive patch (would prefer leaving that out, as it might even
> mean moving parts which are not part of this patch).
>
> In any case, I hope the change fits what you've kindly pointed out.

I forgot to mention it at that time, but the function
pg_show_replication_origin_status is a function to back up
system-views, like pg_stat_get_activity(), pg_show_all_file_settings()
and so on. Such functions are not documented since users don't need to
call them. pg_show_replication_origin_status is not documented for the
same readon. Thus we don't need to mention the function.

In the previous comment, one point I meant is that the "to the
superuser" should be "to superusers", because a PostgreSQL server
(cluster) can define multiple superusers. Another is that "permitted
to other users by using the GRANT command." might be obscure for
users. In this regard I found a more specific description in the same
file:

Computes the total disk space used by the database with the specified
name or OID. To use this function, you must
have <literal>CONNECT</literal> privilege on the specified database
(which is granted by default) or be a member of
the <literal>pg_read_all_stats</literal> role.

So, as the result it would be like the following: (Note that, as you
know, I'm not good at this kind of task..)

Use of functions for replication origin is restricted to superusers.
Use for these functions may be permitted to other users by granting
<literal>EXECUTE<literal> privilege on the functions.

And in regard to the view, granting privileges on both the view and
function to individual user is not practical so we should mention only
granting pg_read_all_stats to users, like the attached patch.

> > 0004:
> >
> > Looks fine by me.
>
> Here goes v3 of the patch

By the way, the attachements of your mail are out-of-order. I'm not
sure that that does something bad, though.

--
Kyotaro Horiguchi
NTT Open Source Software Center