Обсуждение: Read access for pg_monitor to pg_replication_origin_status view

Hi all,

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.

Regards,

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

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

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..66679e8045 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11009,8 +11009,10 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
   <para>
    The <structname>pg_replication_origin_status</structname> view
    contains information about how far replay for a certain origin has
-   progressed.  For more on replication origins
-   see <xref linkend="replication-origins"/>.
+   progressed.  Use of this view is restricted to superusers and
+   pg_read_all_stats role.  To use this view, you must be a member of
+   the <literal>pg_read_all_stats</literal> role. For more on replication
+   origins see <xref linkend="replication-origins"/>.
   </para>
 
   <table>

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

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

On Tue, Jun 09, 2020 at 03:32:24PM +0900, Masahiko Sawada wrote:
> 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.

Not sure if that's worth worrying.  We have similar cases like that,
take for example pg_file_settings with pg_show_all_file_settings()
which requires both a SELECT ACL on pg_file_settings and an EXECUTE
ACL on pg_show_all_file_settings().  My point is that if you issue a
GRANT SELECT on the catalog view, the user can immediately see when
trying to query it that an extra execution is needed.

> 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?

Arf.  Please forget what I wrote here, the hardcoded check for
replication rights would be a problem.
--
Michael

On Tue, 9 Jun 2020 at 16:36, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jun 09, 2020 at 03:32:24PM +0900, Masahiko Sawada wrote:
> > 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.
>
> Not sure if that's worth worrying.  We have similar cases like that,
> take for example pg_file_settings with pg_show_all_file_settings()
> which requires both a SELECT ACL on pg_file_settings and an EXECUTE
> ACL on pg_show_all_file_settings().  My point is that if you issue a
> GRANT SELECT on the catalog view, the user can immediately see when
> trying to query it that an extra execution is needed.

Oh, I see. There might be room for improvement but it's a separate issue.

I agreed with the change you proposed.

Regards,

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

At Tue, 9 Jun 2020 16:35:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Tue, Jun 09, 2020 at 03:32:24PM +0900, Masahiko Sawada wrote:
> > 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.

I also concerned that, but normally all that we should do to that is
GRANTing pg_read_all_stats to the role.  I don't think there is a case
where someone wants to allow the view to a user, who should not be
allowed to see other stats views.

> Not sure if that's worth worrying.  We have similar cases like that,
> take for example pg_file_settings with pg_show_all_file_settings()
> which requires both a SELECT ACL on pg_file_settings and an EXECUTE
> ACL on pg_show_all_file_settings().  My point is that if you issue a
> GRANT SELECT on the catalog view, the user can immediately see when
> trying to query it that an extra execution is needed.

I agree to that as far as that is not the typical use case, and I
don't think that that's the typical use case.

> > 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?
> 
> Arf.  Please forget what I wrote here, the hardcoded check for
> replication rights would be a problem.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 9 Jun 2020 15:11:04 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> 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

It looks fine to me.

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

The depends on whether it is valid and safe (and useful) to reveal the
view to pg_read_all_stats. If that is the case the additional
privilege should be granted for the monitoring sake by default.  And I
think revealing the view to the role is valid and safe, and useful.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 09, 2020 at 05:07:39PM +0900, Masahiko Sawada wrote:
> Oh, I see. There might be room for improvement but it's a separate issue.
>
> I agreed with the change you proposed.

OK, thanks.  Then let's wait a couple of days to see if anybody has
any objections with the removal of the hardcoded superuser check
for those functions.
--
Michael

On Wed, Jun 10, 2020 at 12:35:49PM +0900, Michael Paquier wrote:
> On Tue, Jun 09, 2020 at 05:07:39PM +0900, Masahiko Sawada wrote:
>> I agreed with the change you proposed.
>
> OK, thanks.  Then let's wait a couple of days to see if anybody has
> any objections with the removal of the hardcoded superuser check
> for those functions.

Committed the part removing the superuser checks as of cc07264.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 10, 2020 at 12:35:49PM +0900, Michael Paquier wrote:
>> OK, thanks.  Then let's wait a couple of days to see if anybody has
>> any objections with the removal of the hardcoded superuser check
>> for those functions.

> Committed the part removing the superuser checks as of cc07264.

FWIW, I'd have included a catversion bump in this, to enforce that
the modified backend functions are used with matching pg_proc entries.
It's not terribly important at this phase of the devel cycle, but still
somebody might wonder why the regression tests are failing for them
(if they tried to skip an initdb).

            regards, tom lane

On Mon, Jun 15, 2020 at 12:44:02AM -0400, Tom Lane wrote:
> FWIW, I'd have included a catversion bump in this, to enforce that
> the modified backend functions are used with matching pg_proc entries.
> It's not terribly important at this phase of the devel cycle, but still
> somebody might wonder why the regression tests are failing for them
> (if they tried to skip an initdb).

Thanks, I am fixing that now.

(It may be effective to print a T-shirt of that at some point and give
it to people scoring the most in this area..)
--
Michael

> On 14 Jun 2020, at 05:46, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jun 10, 2020 at 12:35:49PM +0900, Michael Paquier wrote:
>> On Tue, Jun 09, 2020 at 05:07:39PM +0900, Masahiko Sawada wrote:
>>> I agreed with the change you proposed.
>>
>> OK, thanks.  Then let's wait a couple of days to see if anybody has
>> any objections with the removal of the hardcoded superuser check
>> for those functions.
>
> Committed the part removing the superuser checks as of cc07264.

AFAICT from the thread there is nothing left of this changeset to consider, so
I've marked the entry as committed in the CF app.  Feel free to update in case
I've missed something.

cheers ./daniel

On Thu, Jul 02, 2020 at 03:03:22PM +0200, Daniel Gustafsson wrote:
> AFAICT from the thread there is nothing left of this changeset to consider, so
> I've marked the entry as committed in the CF app.  Feel free to update in case
> I've missed something.

A second item discussed in this thread was if we should try to grant
more privileges to pg_read_all_stats when it comes to replication
origins, that's why I let the entry in the CF app.  Now, the thread
has stalled and what has been committed in cc07264 allows to do this
change anyway, so marking the entry as committed sounds fine to me.
Thanks!
--
Michael

Hello,

I wanted to resurface this thread.

The original intention I had with this patch I sent over a year ago
was to have the possibility for monitoring ROLEs like pg_monitor and
pg_read_all_stats to have read access for the replication origin
status. Seems the patch only got half way through (we removed the
superuser hardcoded restriction).

Too bad I didn't notice this until 14 got out, or I'd have done this
much earlier. Well, maybe it's time to do it now :)

Sending a patch to change the privileges of the on the view and
function called by the view.

The only thing I'm not sure, but can amend, is if we need tests for
this change (that would be something like switching ROLE to
pg_read_all_stats and query the pg_replication_origin_status, checking
we get the right result.

Kind regards, Martín

--
Martín Marqués
It’s not that I have something to hide,
it’s that I have nothing I want you to see