| From: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> | 
|---|---|
| To: | martin(at)2ndquadrant(dot)com | 
| Cc: | michael(at)paquier(dot)xyz, pgsql-hackers(at)postgresql(dot)org | 
| Subject: | Re: Read access for pg_monitor to pg_replication_origin_status view | 
| Date: | 2020-06-02 05:01:02 | 
| Message-ID: | 20200602.140102.383813767800521555.horikyota.ntt@gmail.com | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
Hi.
At Mon, 1 Jun 2020 21:41:13 -0300, Martín Marqués <martin(at)2ndquadrant(dot)com> wrote in 
> Hi,
> 
> > Took me a bit longer than expected, but here is a new version, now
> > with the idea of just removing the superuser() check and REVOKEing
> > execution of the functions from public. At the end I grant permission
> > to functions and the pg_replication_origin_status view.
> 
> > I wonder now if I needed to GRANT execution of the functions. A grant
> > on the view should be enough.
> 
> > I'll think about it.
> 
> Yeah, those `GRANT EXECUTE` for the 2 functions should go, as the view
> which is what we want to `SELECT` from has the appropriate ACL set.
> 
> $ 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.
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.
-- 
Kyotaro Horiguchi
NTT Open Source Software Center
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Fujii Masao | 2020-06-02 05:23:50 | Re: SIGSEGV from START_REPLICATION 0/XXXXXXX in XLogSendPhysical () at walsender.c:2762 | 
| Previous Message | Amit Kapila | 2020-06-02 04:52:26 | Re: Small doc improvement about spilled txn tracking |