Read access for pg_monitor to pg_replication_origin_status view

Hi,

While working on some monitoring tasks I realised that the pg_monitor
role doesn't have access to the pg_replication_origin_status.

Are there any strong thoughts on not giving pg_monitor access to this
view, or is it just something that nobody asked for yet? I can't find
any reason for pg_monitor not to have access to it.

Further looking into this, I can see that the requirement of a
superuser to access/monify the replication origins is hardcoded in
backend/replication/logical/origin.c, so it's not a question of
GRANTing access to the pg_monitor user.

```
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),
errmsg("cannot query or manipulate replication origin
when max_replication_slots = 0")));

if (!recoveryOK && RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
errmsg("cannot manipulate replication origins during
recovery")));

}
```

I believe we could skip the superuser() check for cases like
pg_replication_origin_session_progress() and
pg_replication_origin_progress().

Once option could be to add a third bool argument check_superuser to
replorigin_check_prerequisites() and have it set to false for the
functions which a none superuser could execute.

Patch attached

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, May 29, 2020 at 05:39:31PM -0300, Martín Marqués wrote:

I believe we could skip the superuser() check for cases like
pg_replication_origin_session_progress() and
pg_replication_origin_progress().

Once option could be to add a third bool argument check_superuser to
replorigin_check_prerequisites() and have it set to false for the
functions which a none superuser could execute.

Wouldn't it be just better to remove this hardcoded superuser check
and replace it with equivalent ACLs by default? The trick is to make
sure that any function calling replorigin_check_prerequisites() has
its execution correctly revoked from public. See for example
e79350fe.
--
Michael

Hi Michael,

Wouldn't it be just better to remove this hardcoded superuser check
and replace it with equivalent ACLs by default? The trick is to make
sure that any function calling replorigin_check_prerequisites() has
its execution correctly revoked from public. See for example
e79350fe.

Looking at that, it seems a better solution. Let me wrap up a new
patch, likely later today or early tomorrow as it's Sunday ;-)

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

El dom., 31 de may. de 2020 a la(s) 12:13, Martín Marqués
(martin@2ndquadrant.com) escribió:

Hi Michael,

Wouldn't it be just better to remove this hardcoded superuser check
and replace it with equivalent ACLs by default? The trick is to make
sure that any function calling replorigin_check_prerequisites() has
its execution correctly revoked from public. See for example
e79350fe.

Looking at that, it seems a better solution. Let me wrap up a new
patch, likely later today or early tomorrow as it's Sunday ;-)

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--

Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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;

Regards,

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jun 01, 2020 at 03:38:07PM -0300, Martín Marqués wrote:

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.

+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;

FWIW, I think that removing a hardcoded superuser() restriction and
assigning new rights to system roles are two different things, so it
would be better to split the logic into two patches to ease the
review.

I can also see the following in func.sgml:
Use of functions for replication origin is restricted to
superusers.

But that's not right as one can use GRANT to leverage the ACLs of
those functions with your patch. I have a suggestion for this part,
as follows:
"Use of functions for replication origin is only allowed to the
superuser by default, but may be allowed to other users by using the
GRANT command."

Also, you may want to add this patch to the next commit fest:
https://commitfest.postgresql.org/28/
--
Michael

Hi.

At Mon, 1 Jun 2020 21:41:13 -0300, Martín Marqués <martin@2ndquadrant.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

Hi,

$ 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`.

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.

I'll upload new patches shortly following Michael's suggestions.

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Greetings,

* Martín Marqués (martin@2ndquadrant.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

Hi Stephen,

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.

Good, that's more or less what I had in mind.

Here goes v2 of the patch, now there are 4 files (I could have
squashed the docs with the code changes, but hey, that'll be easy to
merge if needed :-) )

I did some fiddling to Michaels doc proposal, but it says basically the same.

Not 100% happy with the change to user-manag.sgml, but ok enough to send.

I also added an entry to the commitfest so we can track this there as well.

Regards,

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

At Tue, 2 Jun 2020 13:13:18 -0300, Martín Marqués <martin@2ndquadrant.com> wrote in

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.

Good, that's more or less what I had in mind.

Here goes v2 of the patch, now there are 4 files (I could have
squashed the docs with the code changes, but hey, that'll be easy to
merge if needed :-) )

I did some fiddling to Michaels doc proposal, but it says basically the same.

Not 100% happy with the change to user-manag.sgml, but ok enough to send.

I also added an entry to the commitfest so we can track this there as well.

0001:

Looks good to me. REVOKE is performed on all functions that called
replorigin_check_prerequisites.

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.

Even if we also granted execution of the function to the specific
role, anyone who wants to grant a user for the view also needs to
grant execution of the function.

To avoid such an inconvenience, as I mentioned upthread, the view and
the function should be granted to public and the function should just
mute the output all the rows, or some columns in each row. That can be
done the same way with pg_stat_get_activity(), as Stephen said.

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.

0004:

Looks fine by me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Hi, Martin.

At Wed, 3 Jun 2020 13:32:28 -0300, Martín Marqués <martin@2ndquadrant.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

Hi Kyotaro-san,

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.

Yes, I agree that it makes the revoking/granting easier to read if
it's grouped by objects, or groups of objects.

Done.

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. :-)

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.

I did some re-writing of the doc, which is pretty close to what you
proposed above.

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.

Regards,

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, 4 Jun 2020 at 21:17, Martín Marqués <martin@2ndquadrant.com> wrote:

Hi Kyotaro-san,

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.

Yes, I agree that it makes the revoking/granting easier to read if
it's grouped by objects, or groups of objects.

Done.

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. :-)

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.

I did some re-writing of the doc, which is pretty close to what you
proposed above.

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've looked at these patches and have one question:

REVOKE ALL ON pg_replication_origin_status FROM public;

+GRANT SELECT ON pg_replication_origin_status TO pg_read_all_stats;

+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;

I thought that this patch has pg_replication_origin_status view behave
like other pg_stat_* views in terms of privileges but it's slightly
different. For instance, since we grant all privileges on
pg_stat_replication to public by default, the only user who either is
a member of pg_read_all_stats or is superuser can see all values but
other users not having such privileges also can access that view and
see the part of statistics. On the other hand, with this patch, we
allow only user who either is a member of pg_read_all_stats or is
superuser to access pg_replication_origin_status view. Other users
cannot even access to that view. Is there any reason why we grant
select privilege to only pg_read_all_stats? I wonder if we can have
pg_replication_origin_status accessible by public and filter some
column data in pg_show_replication_origin_status() that we don't want
to show to users who neither a member of pg_read_all_stats nor
superuser.

There is a typo in 0001 patch:

+--
+-- Permision to execute Replication Origin functions should be
revoked from public
+--

s/Permision/Permission/

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

At Mon, 8 Jun 2020 16:21:45 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

I've looked at these patches and have one question:

REVOKE ALL ON pg_replication_origin_status FROM public;

+GRANT SELECT ON pg_replication_origin_status TO pg_read_all_stats;

+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;

I thought that this patch has pg_replication_origin_status view behave
like other pg_stat_* views in terms of privileges but it's slightly
different. For instance, since we grant all privileges on
pg_stat_replication to public by default, the only user who either is
a member of pg_read_all_stats or is superuser can see all values but
other users not having such privileges also can access that view and
see the part of statistics. On the other hand, with this patch, we
allow only user who either is a member of pg_read_all_stats or is
superuser to access pg_replication_origin_status view. Other users
cannot even access to that view. Is there any reason why we grant
select privilege to only pg_read_all_stats? I wonder if we can have
pg_replication_origin_status accessible by public and filter some
column data in pg_show_replication_origin_status() that we don't want
to show to users who neither a member of pg_read_all_stats nor
superuser.

Yeah, I agree to this (and wrote something like that before).

On the other hand Martín seems to just want to allow other users to
see it while preserving the current behavior. I also understand that
thought.

There is a typo in 0001 patch:

+--
+-- Permision to execute Replication Origin functions should be
revoked from public
+--

s/Permision/Permission/

Mmm. Right.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jun 08, 2020 at 05:44:56PM +0900, Kyotaro Horiguchi wrote:

Mmm. Right.

Yep. I bumped on that myself. I am not sure about 0002 and 0004 yet,
and IMO they are not mandatory pieces, but from what I can see in the
set 0001 and 0003 can just be squashed together to remove those
superuser checks, and no spots within the twelve functions calling
replorigin_check_prerequisites() are missing a REVOKE. So something
like the attached could just happen first, no? If the rights of
pg_read_all_stats need to be extended, it would always be possible to
do so once the attached is done with a custom script.

Also, why don't we use this occation to do the same thing for the
functions working on replication slots? While we are looking at this
area, we may as well just do it. Here is the set of functions that
would be involved:
- pg_create_physical_replication_slot
- pg_create_logical_replication_slot
- pg_replication_slot_advance
- pg_drop_replication_slot
- pg_copy_logical_replication_slot (3 functions)
- pg_copy_physical_replication_slot (2 functions)
--
Michael

On Tue, 9 Jun 2020 at 15:11, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Jun 08, 2020 at 05:44:56PM +0900, Kyotaro Horiguchi wrote:

Mmm. Right.

Yep. I bumped on that myself. I am not sure about 0002 and 0004 yet,
and IMO they are not mandatory pieces, but from what I can see in the
set 0001 and 0003 can just be squashed together to remove those
superuser checks, and no spots within the twelve functions calling
replorigin_check_prerequisites() are missing a REVOKE. So something
like the attached could just happen first, no? If the rights of
pg_read_all_stats need to be extended, it would always be possible to
do so once the attached is done with a custom script.

One thing I'm concerned with this change is that we will end up
needing to grant both execute on pg_show_replication_origin_status()
and select on pg_replication_origin_status view when we want a
non-super user to access pg_replication_origin_status. It’s unlikely
that the user can grant both privileges at once as
pg_show_replication_origin_status() is not documented.

Also, why don't we use this occation to do the same thing for the
functions working on replication slots? While we are looking at this
area, we may as well just do it. Here is the set of functions that
would be involved:
- pg_create_physical_replication_slot
- pg_create_logical_replication_slot
- pg_replication_slot_advance
- pg_drop_replication_slot
- pg_copy_logical_replication_slot (3 functions)
- pg_copy_physical_replication_slot (2 functions)

A user having a replication privilege already is able to execute these
functions. Do you mean to ease it so that a user also executes them
without replication privilege?

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services