Re: Read access for pg_monitor to pg_replication_origin_status view

Hello, Martín.

Thanks for the new version.

At Thu, 4 Jun 2020 09:17:18 -0300, Martín Marqués <martin@2ndquadrant.com> wrote in
> > 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.
>
> Yes, I agree that it makes the revoking/granting easier to read if
> it's grouped by objects, or groups of objects.
>
> Done.

0002 looks fine to me.

> > 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:
>
> OK, now I understand what you were saying. :-)

I'm happy to hear that:)

> > 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.
>
> I did some re-writing of the doc, which is pretty close to what you
> proposed above.

(0003) Unfortunately, the closing tag of EXECUTE is missing prefixing
'/'.

I see many nearby occurrences of "This function is restricted to
superusers by default, but other users can be granted EXECUTE to run
the function".  I'm not sure, but it might be better to use the same
expression, but I don't insist on that. It's not changed in the
attached.

> > By the way, the attachements of your mail are out-of-order. I'm not
> > sure that that does something bad, though.
>
> That's likely Gmail giving them random order when you attach multiple
> files all at once.
>
> New patches attached.

- I'm fine with the direction of this patch. Works as expected, that
  is, makes no changes of behavior for replication origin functions,
  and pg_read_all_stats can read the pg_replication_origin_status
  view.

- The patches cleanly applied to the current HEAD and can be compiled
  with a minor fix (fixed in the attached v5).

- The patches should be merged but I'll left that for committer.

- The commit titles are too long. Each of them should be split up into
  a brief title and a description. But I think committers would rewrite
  them for the final patch to commit so I don't think we don't need to
  rewrite them right now.

I'll wait for a couple of days for comments from others or opinions
before moving this to Ready for Committer.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center
From 857be52f29cf244b7c217bedf87bd4507e49a388 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 <martin.marques@2ndquadrant.com>
Date: Tue, 2 Jun 2020 10:44:42 -0300
Subject: [PATCH v5 1/4] Access to pg_replication_origin_status view has
 restricted access only for superusers due to it using
 pg_show_replication_origin_status(), and all replication origin functions
 requiring superuser rights.

This patch removes the check for superuser priviledges when executing
replication origin functions, which is hardcoded, and instead rely on
ACL checks.

This patch will also revoke execution of such functions from PUBLIC
---
 src/backend/catalog/system_views.sql     | 16 ++++++++++++++++
 src/backend/replication/logical/origin.c |  5 -----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 56420bbc9d..6658f0c2eb 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1469,6 +1469,22 @@ REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
 
+--
+-- Permision to execute Replication Origin functions should be revoked from public
+--
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_advance(text, pg_lsn) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_create(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_drop(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_oid(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_progress(text, boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_session_is_setup() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_session_progress(boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_session_reset() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_session_setup(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_xact_reset() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_replication_origin_xact_setup(pg_lsn, timestamp with time zone) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_show_replication_origin_status() FROM public;
+
 --
 -- We also set up some things as accessible to standard roles.
 --
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 1ca4479605..bc50106070 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -182,11 +182,6 @@ static ReplicationState *session_replication_state = NULL;
 static void
 replorigin_check_prerequisites(bool check_slots, bool recoveryOK)
 {
-    if (!superuser())
-        ereport(ERROR,
-                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 errmsg("only superusers can query or manipulate replication origins")));
-
     if (check_slots && max_replication_slots == 0)
         ereport(ERROR,
                 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-- 
2.18.2

From 328f5199b040d022754f7182ff3803d5b16bd57e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 <martin.marques@2ndquadrant.com>
Date: Thu, 4 Jun 2020 08:44:20 -0300
Subject: [PATCH v5 2/4] We want the monitoring role `pg_read_all_stats` to be
 able to SELECT from `pg_replication_origin_status`. We also need to give this
 role EXECUTE privileges for `pg_show_replication_origin_status()`.

---
 src/backend/catalog/system_views.sql | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 6658f0c2eb..9949b4316f 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1123,6 +1123,9 @@ CREATE VIEW pg_replication_origin_status AS
 
 REVOKE ALL ON pg_replication_origin_status FROM public;
 
+GRANT SELECT ON pg_replication_origin_status TO pg_read_all_stats;
+
+
 -- All columns of pg_subscription except subconninfo are readable.
 REVOKE ALL ON pg_subscription FROM public;
 GRANT SELECT (subdbid, subname, subowner, subenabled, subslotname, subpublications)
@@ -1485,6 +1488,8 @@ REVOKE EXECUTE ON FUNCTION pg_replication_origin_xact_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_replication_origin_xact_setup(pg_lsn, timestamp with time zone) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_show_replication_origin_status() FROM public;
 
+GRANT EXECUTE ON FUNCTION pg_show_replication_origin_status() TO pg_read_all_stats;
+
 --
 -- We also set up some things as accessible to standard roles.
 --
-- 
2.18.2

From 252a56f952ab16b8e59095c53012942661128103 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 <martin.marques@2ndquadrant.com>
Date: Thu, 4 Jun 2020 09:04:36 -0300
Subject: [PATCH v5 3/4] Change replication origin function documenation to
 reflect that now none superusers could be granted execution of these
 functions.

---
 doc/src/sgml/func.sgml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index a8d57f4e39..7a3b4cdc43 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24614,7 +24614,10 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
     <xref linkend="streaming-replication-slots"/>, and
     <xref linkend="replication-origins"/>
     for information about the underlying features.
-    Use of functions for replication origin is restricted to superusers.
+    Use of functions for replication origin is by default allowed only
+    to superusers.
+    To use these functions, you must have <literal>EXECUTE</literal>
+    privilege on them.
     Use of functions for replication slots is restricted to superusers
     and users having <literal>REPLICATION</literal> privilege.
    </para>
-- 
2.18.2

From 10c48d3319a86b172c7a795ae4a61159a431b12d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
 <martin.marques@2ndquadrant.com>
Date: Thu, 4 Jun 2020 09:05:39 -0300
Subject: [PATCH v5 4/4] Apply changes to the documentation to reflect that now
 pg_read_all_stats a lso has SELECT privileges on
 pg_replication_origin_status.

---
 doc/src/sgml/user-manag.sgml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 829decd883..88b38bc15d 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -524,8 +524,8 @@ DROP ROLE doomed_role;
       </row>
       <row>
        <entry>pg_read_all_stats</entry>
-       <entry>Read all pg_stat_* views and use various statistics related extensions,
-       even those normally visible only to superusers.</entry>
+       <entry>Read all pg_stat_* and pg_replication_origin_status views, and use various statistics
+       related extensions, even those normally visible only to superusers.</entry>
       </row>
       <row>
        <entry>pg_stat_scan_tables</entry>
-- 
2.18.2