should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?

On 10/7/21, 10:42 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

In a typical production environment, the user (not necessarily a
superuser) sometimes wants to analyze the memory usage via
pg_backend_memory_contexts view or pg_log_backend_memory_contexts
function which are accessible to only superusers. Isn't it better to
allow non-superusers with an appropriate predefined role (I'm thinking
of pg_monitor) to access them?

It looks like this was discussed previously [0]/messages/by-id/a99bdd0e-7271-8176-f700-2553a51d4a27@oss.nttdata.com. From the description
of pg_monitor [1]https://www.postgresql.org/docs/devel/predefined-roles.html, I think it's definitely arguable that this view and
function should be accessible by roles that are members of pg_monitor.

The pg_monitor, pg_read_all_settings, pg_read_all_stats and
pg_stat_scan_tables roles are intended to allow administrators
to easily configure a role for the purpose of monitoring the
database server. They grant a set of common privileges
allowing the role to read various useful configuration
settings, statistics and other system information normally
restricted to superusers.

AFAICT the current permissions were chosen as a safe default, but
maybe it can be revisited. The view and function appear to only
reveal high level information about the memory contexts in use (e.g.,
name, size, amount used), so I'm not seeing any obvious reason why
they should remain superuser-only. pg_log_backend_memory_contexts()
directly affects the server log, which might be a bit beyond what
pg_monitor should be able to do. My currently thinking is that we
should give pg_monitor access to pg_backend_memory_contexts (and maybe
even pg_shmem_allocations). However, one interesting thing I see is
that there is no mention of any predefined roles in system_views.sql.
Instead, the convention seems to be to add hard-coded checks for
predefined roles in the backing functions. I don't know if that's a
hard and fast rule, but I do see that predefined roles are given
special privileges in system_functions.sql.

Nathan

[0]: /messages/by-id/a99bdd0e-7271-8176-f700-2553a51d4a27@oss.nttdata.com
[1]: https://www.postgresql.org/docs/devel/predefined-roles.html

On Fri, Oct 8, 2021 at 12:27 AM Bossart, Nathan <bossartn@amazon.com> wrote:

On 10/7/21, 10:42 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

In a typical production environment, the user (not necessarily a
superuser) sometimes wants to analyze the memory usage via
pg_backend_memory_contexts view or pg_log_backend_memory_contexts
function which are accessible to only superusers. Isn't it better to
allow non-superusers with an appropriate predefined role (I'm thinking
of pg_monitor) to access them?

It looks like this was discussed previously [0]. From the description
of pg_monitor [1], I think it's definitely arguable that this view and
function should be accessible by roles that are members of pg_monitor.

The pg_monitor, pg_read_all_settings, pg_read_all_stats and
pg_stat_scan_tables roles are intended to allow administrators
to easily configure a role for the purpose of monitoring the
database server. They grant a set of common privileges
allowing the role to read various useful configuration
settings, statistics and other system information normally
restricted to superusers.

Hm.

AFAICT the current permissions were chosen as a safe default, but
maybe it can be revisited. The view and function appear to only
reveal high level information about the memory contexts in use (e.g.,
name, size, amount used), so I'm not seeing any obvious reason why
they should remain superuser-only. pg_log_backend_memory_contexts()
directly affects the server log, which might be a bit beyond what
pg_monitor should be able to do. My currently thinking is that we
should give pg_monitor access to pg_backend_memory_contexts (and maybe
even pg_shmem_allocations).

pg_shmem_allocations is also a good candidate.

However, one interesting thing I see is
that there is no mention of any predefined roles in system_views.sql.
Instead, the convention seems to be to add hard-coded checks for
predefined roles in the backing functions. I don't know if that's a
hard and fast rule, but I do see that predefined roles are given
special privileges in system_functions.sql.

There are two things: 1) We revoke the permissions for nonsuperuser in
system_views.sql with the below
REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
REVOKE ALL ON pg_backend_memory_contexts FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;

2) We don't revoke any permissions in the system_views.sql, but we
have the following kind checks in the underlying function:
/*
* Only superusers or members of pg_monitor can <<see the details>>.
*/
if (!superuser() || !is_member_of_role(GetUserId(), ROLE_PG_MONITOR))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be a superuser or a member of the pg_monitor role to
<<see the details>>")));

I think we can remove the below revoke statements from
system_views.sql and place the checks shown at (2) in the underlying
functions pg_get_shmem_allocations, pg_get_backend_memory_contexts,
also in pg_log_backend_memory_contexts.

REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
REVOKE ALL ON pg_backend_memory_contexts FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;

Thoughts?

Regards,
Bharath Rupireddy.

On 10/8/21, 12:01 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I think we can remove the below revoke statements from
system_views.sql and place the checks shown at (2) in the underlying
functions pg_get_shmem_allocations, pg_get_backend_memory_contexts,
also in pg_log_backend_memory_contexts.

REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
REVOKE ALL ON pg_backend_memory_contexts FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;

Thoughts?

This approach would add a restriction that a role must have SUPERUSER
or be a member of pg_monitor to use the views/functions. I think
there is value in allowing any role to use them (if granted the proper
privileges). In any case, users may already depend on being able to
do that.

Instead, I think we should just grant privileges to pg_monitor. I've
attached a (basically untested) patch to demonstrate what I'm
thinking.

Nathan

Greetings,

* Bossart, Nathan (bossartn@amazon.com) wrote:

On 10/8/21, 12:01 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I think we can remove the below revoke statements from
system_views.sql and place the checks shown at (2) in the underlying
functions pg_get_shmem_allocations, pg_get_backend_memory_contexts,
also in pg_log_backend_memory_contexts.

REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
REVOKE ALL ON pg_backend_memory_contexts FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;

Thoughts?

This approach would add a restriction that a role must have SUPERUSER
or be a member of pg_monitor to use the views/functions. I think
there is value in allowing any role to use them (if granted the proper
privileges). In any case, users may already depend on being able to
do that.

Instead, I think we should just grant privileges to pg_monitor. I've
attached a (basically untested) patch to demonstrate what I'm
thinking.

I'm not necessarily against this, but I will point out that we've stayed
away, so far, from explicitly GRANT'ing privileges to pg_monitor itself,
intending that to be a role which just combines privileges of certain
other predefined roles together.

I would think that these would fall under "pg_read_all_stats", in
particular, which is explicitly documented as: Read all pg_stat_* views
and use various statistics related extensions, even those normally
visible only to superusers.

(the last bit being particularly relevant in this case)

Thanks,

Stephen

On Sat, Oct 9, 2021 at 12:45 AM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* Bossart, Nathan (bossartn@amazon.com) wrote:

On 10/8/21, 12:01 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I think we can remove the below revoke statements from
system_views.sql and place the checks shown at (2) in the underlying
functions pg_get_shmem_allocations, pg_get_backend_memory_contexts,
also in pg_log_backend_memory_contexts.

REVOKE ALL ON pg_shmem_allocations FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_shmem_allocations() FROM PUBLIC;
REVOKE ALL ON pg_backend_memory_contexts FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;

Thoughts?

This approach would add a restriction that a role must have SUPERUSER
or be a member of pg_monitor to use the views/functions. I think
there is value in allowing any role to use them (if granted the proper
privileges). In any case, users may already depend on being able to
do that.

Instead, I think we should just grant privileges to pg_monitor. I've
attached a (basically untested) patch to demonstrate what I'm
thinking.

I'm not necessarily against this, but I will point out that we've stayed
away, so far, from explicitly GRANT'ing privileges to pg_monitor itself,
intending that to be a role which just combines privileges of certain
other predefined roles together.

I would think that these would fall under "pg_read_all_stats", in
particular, which is explicitly documented as: Read all pg_stat_* views
and use various statistics related extensions, even those normally
visible only to superusers.

(the last bit being particularly relevant in this case)

+1. I will prepare the patch with the pg_read_all_stats role.

Regards,
Bharath Rupireddy.

On Sat, Oct 9, 2021 at 3:56 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I would think that these would fall under "pg_read_all_stats", in
particular, which is explicitly documented as: Read all pg_stat_* views
and use various statistics related extensions, even those normally
visible only to superusers.

(the last bit being particularly relevant in this case)

+1. I will prepare the patch with the pg_read_all_stats role.

Here's the v1, please review it further.

I've also made a CF entry - https://commitfest.postgresql.org/35/3352/

Regards,
Bharath Rupireddy.

On 10/9/21, 2:12 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Here's the v1, please review it further.

Thanks for the patch.

-	/* Only allow superusers to log memory contexts. */
-	if (!superuser())
+	/*
+	 * Only superusers or members of pg_read_all_stats can log memory contexts.
+	 */
+	if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))

I personally think pg_log_backend_memory_contexts() should remain
restricted to superusers since it directly impacts the server log.
However, if we really did want to open it up to others, couldn't we
add GRANT/REVOKE statements in system_functions.sql and remove the
hard-coded superuser check? I think that provides a bit more
flexibility (e.g., permission to execute it can be granted to others
without giving them pg_read_all_stats).

Nathan

Greetings,

On Tue, Oct 12, 2021 at 20:26 Bossart, Nathan <bossartn@amazon.com> wrote:

On 10/9/21, 2:12 AM, "Bharath Rupireddy" <
bharath.rupireddyforpostgres@gmail.com> wrote:

Here's the v1, please review it further.

Thanks for the patch.

-       /* Only allow superusers to log memory contexts. */
-       if (!superuser())
+       /*
+        * Only superusers or members of pg_read_all_stats can log memory
contexts.
+        */
+       if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))

I personally think pg_log_backend_memory_contexts() should remain
restricted to superusers since it directly impacts the server log.
However, if we really did want to open it up to others, couldn't we
add GRANT/REVOKE statements in system_functions.sql and remove the
hard-coded superuser check? I think that provides a bit more
flexibility (e.g., permission to execute it can be granted to others
without giving them pg_read_all_stats).

I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

Thanks,

Stephen

On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:

I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

I don't think that this is a bad thing to remove the superuser() check
and replace it with a REVOKE FROM PUBLIC in this case, but linking the
logging of memory contexts with pg_read_all_stats does not seem right
to me.
--
Michael

On 10/12/21, 6:26 PM, "Michael Paquier" <michael@paquier.xyz> wrote:

On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:

I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

I don't think that this is a bad thing to remove the superuser() check
and replace it with a REVOKE FROM PUBLIC in this case, but linking the
logging of memory contexts with pg_read_all_stats does not seem right
to me.

+1

Nathan

On Wed, Oct 13, 2021 at 6:55 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:

I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

I don't think that this is a bad thing to remove the superuser() check
and replace it with a REVOKE FROM PUBLIC in this case,

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

And, I see there are a lot of functions in the code base that does "if
(!superuser())" check and emit "must be superuser to XXX" sort of
error.

but linking the
logging of memory contexts with pg_read_all_stats does not seem right
to me.

Agreed. The user with pg_read_all_stats can't see the server logs so
it doesn't make sense to make them call the function. I will remove
this change from the patch.

Regards,
Bharath Rupireddy.

On Wed, Oct 13, 2021 at 7:48 AM Bossart, Nathan <bossartn@amazon.com> wrote:

On 10/12/21, 6:26 PM, "Michael Paquier" <michael@paquier.xyz> wrote:

On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:

I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

I don't think that this is a bad thing to remove the superuser() check
and replace it with a REVOKE FROM PUBLIC in this case, but linking the
logging of memory contexts with pg_read_all_stats does not seem right
to me.

+1

Here comes the v2 patch. Note that I've retained superuser() check in
the pg_log_backend_memory_contexts(). Please review it.

Regards,
Bharath Rupireddy.

On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote:

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

I don't really buy this argument with the "superuser" error message.
When removing hardcoded superuser(), we just close the gap by adding
in the documentation that the function execution can be granted
afterwards. And nobody has complained about the difference in error
message AFAIK. That's about extensibility.
--
Michael

Greetings,

On Wed, Oct 13, 2021 at 03:54 Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote:

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

I don't really buy this argument with the "superuser" error message.
When removing hardcoded superuser(), we just close the gap by adding
in the documentation that the function execution can be granted
afterwards. And nobody has complained about the difference in error
message AFAIK. That's about extensibility.

Agreed.

Thanks,

Stephen

On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote:

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

I don't really buy this argument with the "superuser" error message.
When removing hardcoded superuser(), we just close the gap by adding
in the documentation that the function execution can be granted
afterwards. And nobody has complained about the difference in error
message AFAIK. That's about extensibility.

I'm not against removing superuser() check in the
pg_log_backend_memory_contexts. However, there are a lot of functions
with the "must be superuser to XXXXX" kind of error [1]. I'm worried
if someone proposes to change these as well with what we do for
pg_log_backend_memory_contexts.

brin_page_type
brin_page_items
brin_metapage_info
brin_revmap_data
bt_page_stats_internal
bt_page_items_internal
bt_page_items_bytea
bt_metap
fsm_page_contents
gin_metapage_info
gin_page_opaque_info
and the list goes on.

Regards,
Bharath Rupireddy.

Greeting,

On Wed, Oct 13, 2021 at 04:14 Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier <michael@paquier.xyz>
wrote:

On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote:

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

I don't really buy this argument with the "superuser" error message.
When removing hardcoded superuser(), we just close the gap by adding
in the documentation that the function execution can be granted
afterwards. And nobody has complained about the difference in error
message AFAIK. That's about extensibility.

I'm not against removing superuser() check in the
pg_log_backend_memory_contexts. However, there are a lot of functions
with the "must be superuser to XXXXX" kind of error [1]. I'm worried
if someone proposes to change these as well with what we do for
pg_log_backend_memory_contexts.

brin_page_type
brin_page_items
brin_metapage_info
brin_revmap_data
bt_page_stats_internal
bt_page_items_internal
bt_page_items_bytea
bt_metap
fsm_page_contents
gin_metapage_info
gin_page_opaque_info
and the list goes on.

Yes, would generally be good to change at least some of those also, perhaps
all of them.

Not sure I see what the argument here is. We should really be trying to
move away from explicit superuser checks.

Thanks.

Stephen

On Wed, Oct 13, 2021 at 2:19 PM Stephen Frost <sfrost@snowman.net> wrote:

Greeting,

On Wed, Oct 13, 2021 at 04:14 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote:

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

I don't really buy this argument with the "superuser" error message.
When removing hardcoded superuser(), we just close the gap by adding
in the documentation that the function execution can be granted
afterwards. And nobody has complained about the difference in error
message AFAIK. That's about extensibility.

I'm not against removing superuser() check in the
pg_log_backend_memory_contexts. However, there are a lot of functions
with the "must be superuser to XXXXX" kind of error [1]. I'm worried
if someone proposes to change these as well with what we do for
pg_log_backend_memory_contexts.

brin_page_type
brin_page_items
brin_metapage_info
brin_revmap_data
bt_page_stats_internal
bt_page_items_internal
bt_page_items_bytea
bt_metap
fsm_page_contents
gin_metapage_info
gin_page_opaque_info
and the list goes on.

Yes, would generally be good to change at least some of those also, perhaps all of them.

Hm. Let's deal with it separately, if required.

Not sure I see what the argument here is. We should really be trying to move away from explicit superuser checks.

I will remove the superuser() for pg_log_backend_memory_context alone
here in the next version of patch.

Regards,
Bharath Rupireddy.

On Tue, Oct 12, 2021 at 8:33 PM Stephen Frost <sfrost@snowman.net> wrote:

Or not. I can see the argument that, because it just goes into the log, that it doesn’t make sense to grant to a predefined role, since that role wouldn’t be able to see the results even if it had access.

Yeah. I think we should really only use predefined roles where it's
not practical to have people use GRANT/REVOKE.

For instance, it makes sense to have pg_execute_server_program because
there's no particular function (or other object) to which you could
grant permissions at the SQL level to achieve the same results. And
pg_read_all_stats doesn't just allow you to run more functions: it
changes which fields those functions populate in the returned data,
and which they mask out for security reasons. So, GRANT/REVOKE
wouldn't do it in that case.

But if there's one particular function that someone may or may not
want a non-superuser to be able to execute, let's just let them do
that. It doesn't need to be tied to a predefined role, and in fact
it's more flexible if it isn't.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, 2021-10-13 at 10:03 -0400, Robert Haas wrote:

Yeah. I think we should really only use predefined roles where it's
not practical to have people use GRANT/REVOKE.

That sounds like a good rule.

A minor complaint though: to grant on pg_backend_memory_contexts, you
need two grant statements:

grant select on pg_backend_memory_contexts to foo;
grant execute on function pg_get_backend_memory_contexts() to foo;

The second is more of an internal detail, and we don't really want
users to be relying on that undocumented function. Is there a good way
to define a view kind of like a SECURITY DEFINER function so that the
superuser would only need to issue a GRANT statement on the view?

Regards,
Jeff Davis