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/
> 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!
> 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.
> 0004:
>
> Looks fine by me.
Here goes v3 of the patch
--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 87c00782c691f2c6c13768cd6d96b19de69cab16 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
<martin.marq...@2ndquadrant.com>
Date: Tue, 2 Jun 2020 10:44:42 -0300
Subject: [PATCH v3 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 56420bbc9d6..6658f0c2eb2 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 981d60f135d..1f223daf21f 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.21.3
From 0d0ef6aa7e563bd3b9fe7f80ebafae9f10f3eee5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
<martin.marq...@2ndquadrant.com>
Date: Wed, 3 Jun 2020 13:28:15 -0300
Subject: [PATCH v3 4/4] Apply changes to the documentation to reflect that now
pg_read_all_stats also 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 829decd8839..88b38bc15d5 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.21.3
From 83b38cb1f15fecc7b93fcf544ae53230e9627bb5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
<martin.marq...@2ndquadrant.com>
Date: Wed, 3 Jun 2020 10:57:32 -0300
Subject: [PATCH v3 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()`.
This will help so output diagnostics and monitoring tools can be able to
gather replication origins status without requiring superuser privileges
---
src/backend/catalog/system_views.sql | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 6658f0c2eb2..f290e3ca6ea 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1497,3 +1497,6 @@ GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) 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;
+
+GRANT EXECUTE ON FUNCTION pg_show_replication_origin_status() TO pg_read_all_stats;
+GRANT SELECT ON pg_replication_origin_status TO pg_read_all_stats;
--
2.21.3
From f3c812a0c3ca06e51b592f896df61e302e29d326 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Marqu=C3=A9s?=
<martin.marq...@2ndquadrant.com>
Date: Wed, 3 Jun 2020 13:27:32 -0300
Subject: [PATCH v3 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 | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7c06afd3eaf..1e89dc80afe 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24617,7 +24617,11 @@ 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 only allowed
+ to the superuser, except for <function>pg_show_replication_origin_status</function>
+ which <literal>pg_read_all_stats</literal> can also use. Use for
+ these functions may be permitted to other users by using the GRANT
+ command.
Use of functions for replication slots is restricted to superusers
and users having <literal>REPLICATION</literal> privilege.
</para>
--
2.21.3
