Обсуждение: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?

Hi,

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?

Thoughts?

Regards,
Bharath Rupireddy.

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.

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]
https://www.postgresql.org/message-id/flat/a99bdd0e-7271-8176-f700-2553a51d4a27%40oss.nttdata.com#0f79f7cf6a6c3b3e3ccb4570870b3bd4
[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

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

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.

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

On Wed, Oct 13, 2021 at 7:45 PM Jeff Davis <pgsql@j-davis.com> wrote:
> 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?

According to https://www.postgresql.org/docs/current/sql-createview.html
it always works like that: "Access to tables referenced in the view is
determined by permissions of the view owner. In some cases, this can
be used to provide secure but restricted access to the underlying
tables."

Hmm, unless that rule is only being applied for *tables* and not for
*functions*? I guess that could be true, but if so, it sure seems
inconsistent.

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

Greetings,

* Isaac Morland (isaac.morland@gmail.com) wrote:
> On Thu, 14 Oct 2021 at 09:11, Robert Haas <robertmhaas@gmail.com> wrote:
> > According to https://www.postgresql.org/docs/current/sql-createview.html
> > it always works like that: "Access to tables referenced in the view is
> > determined by permissions of the view owner. In some cases, this can
> > be used to provide secure but restricted access to the underlying
> > tables."
> >
> > Hmm, unless that rule is only being applied for *tables* and not for
> > *functions*? I guess that could be true, but if so, it sure seems
> > inconsistent.

I'm not sure that it's really inconsistent- if you want the function to
run as someone else, define it as SECURITY DEFINER and it will.  If the
function is defined as SECURITY INVOKER then it'll run with the
privileges of the user invoking the function- which can be pretty handy
if, say, the function references CURRENT_USER.  Note that RLS policies
work in the same way.

> Yes, I think this has come up before. It seems obvious to me that a view
> should execute entirely in the context of its owner. I should be able to
> use functions to define view columns without requiring that access to those
> functions be handed out to users of the view.

I don't know that it's all that obvious, particularly when you consider
that the function owner has the option of having the function run as the
invoker of the function or as the owner of the function.

> I feel this might relate to the discussion of triggers, which I claim
> should execute in the context of the table owner (or maybe the trigger
> owner, if that were a separate concept). There are lots of triggers one
> might want to write that cannot be written because they execute in the
> context of the user of the table; my recollection is that it is harder to
> find examples of non-malware triggers that depend on executing in the
> context of the user of the table.

Triggers can call security definer functions, so I'm not quite sure I
understand what the issue here is.

Thanks,

Stephen

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> 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?

Erm, surely the function should be documented...

Other than that, grouping of privileges is generally done using roles.
We could possibly create a predefined role to assist with this but I
don't think it's a huge issue for users to do that themselves.,
particularly since they're likely to grant other accesses to that role
too.  In some instances, it might make sense to grant such access to
other predefined roles too (pg_monitor or the other ones), of course.

I don't think we really want to be doing privilege checks with one role
(view owner) for who is allowed to run the function, and then actually
running the function with some other role when it's a security invoker
function..

Thanks,

Stephen

On Thu, 2021-10-14 at 13:43 -0400, Stephen Frost wrote:
> I'm not sure that it's really inconsistent- if you want the function
> to
> run as someone else, define it as SECURITY DEFINER and it will.

There are two issues:

1. Does having permissions to read a view give the reader the ability
to execute the function as a part of reading the view?

Here it seems like we should allow the user to execute the function
that's a part of the view. If it's doing something that performs
another permission check, then it could fail, but at least they'd be
able to execute it. That seems consistent with the ability to read
tables as a part of reading the view.

2. If the function is executed, is it SECURITY INVOKER or SECURITY
DEFINER?

I think here the answer is SECURITY INVOKER. SECURITY DEFINER doesn't
even really make sense, because the definer might not be the owner of
the view. Maybe we need a concept where the function is executed as
neither the invoker or the definer, but as the owner of the view (or
something else), which sounds appealing, but sounds more like a new
feature.

Regards,
    Jeff Davis

On Thu, Oct 14, 2021 at 1:43 PM Stephen Frost <sfrost@snowman.net> wrote:
> I'm not sure that it's really inconsistent- if you want the function to
> run as someone else, define it as SECURITY DEFINER and it will.  If the
> function is defined as SECURITY INVOKER then it'll run with the
> privileges of the user invoking the function- which can be pretty handy
> if, say, the function references CURRENT_USER.

That presumes that (1) the user who owns the view also owns the
function and (2) the user who created the view and the function wants
to permit people who query the view to call the function with any
arguments, rather than only those arguments that would be passed by
querying the view. Neither of those things is necessarily true.

I am not really sure that we can get away with changing this, since it
is long-established behavior. At least, if we do, we are going to have
to warn people to watch out for backward-compatibility issues, some of
which may not be things breaking functionally but rather having a
different security profile. But, in a green field, I don't know why
it's sane to suppose that if you query a view, the things in the view
behave partly as if the user querying the view were running them, and
partly as if the user owning the view were one of them. It seems much
more logical for it to be one or the other.

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

On Thu, 2021-10-14 at 14:22 -0400, Robert Haas wrote:
> I am not really sure that we can get away with changing this, since
> it
> is long-established behavior. At least, if we do, we are going to
> have
> to warn people to watch out for backward-compatibility issues, some
> of
> which may not be things breaking functionally but rather having a
> different security profile. But, in a green field, I don't know why
> it's sane to suppose that if you query a view, the things in the view
> behave partly as if the user querying the view were running them, and
> partly as if the user owning the view were one of them. It seems much
> more logical for it to be one or the other.

How do you feel about at least allowing the functions to execute (and
if it's SECURITY INVOKER, possibly encountering a permissions failure
during execution)?

There are of course security implications with any change like that,
but it seems like a fairly minor one unless I'm missing something. Why
would an admin give someone the privileges to read a view if it will
always fail due to lack of execute privilege?

Regards,
    Jeff Davis

On Thu, Oct 14, 2021 at 3:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
> How do you feel about at least allowing the functions to execute (and
> if it's SECURITY INVOKER, possibly encountering a permissions failure
> during execution)?

I think we'd at least need to check that the view owner has execute
permission on the function. I'm not sure whether there are any other
gotchas.

> There are of course security implications with any change like that,
> but it seems like a fairly minor one unless I'm missing something. Why
> would an admin give someone the privileges to read a view if it will
> always fail due to lack of execute privilege?

An excellent question.

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

On Fri, 2021-10-15 at 09:08 -0400, Robert Haas wrote:
> I think we'd at least need to check that the view owner has execute
> permission on the function. I'm not sure whether there are any other
> gotchas.

Right, like we do for tables in a view now.

The alternative is not very appealing: that we have to document a lot
of currently-undocumented internal functions like
pg_get_backend_memory_contexts(), pg_lock_status(), etc., so that users
can grant fine-grained permissions.

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Fri, 2021-10-15 at 09:08 -0400, Robert Haas wrote:
> > I think we'd at least need to check that the view owner has execute
> > permission on the function. I'm not sure whether there are any other
> > gotchas.
>
> Right, like we do for tables in a view now.
>
> The alternative is not very appealing: that we have to document a lot
> of currently-undocumented internal functions like
> pg_get_backend_memory_contexts(), pg_lock_status(), etc., so that users
> can grant fine-grained permissions.

Being undocumented and being an 'internal function' aren't quite the
same thing..  pg_lock_status() is available for users to call and even
has a description which they can review with \dfS+ and is "view system
lock information", not to mention that it calls GetLockStatusData which
is explicitly documented as "for use in a user-level reporting
function".

I do recongize that it's not in the formal documentation currently,
though I'm not quite sure I understand why that's the case when we
document things like pg_stat_get_activity().  While I appreciate that it
isn't really addressing the complaint you have that it'd be nice if we
made things simpler for administrators by making it so they don't have
to GRANT access to both the view and the function, and I can see how
that would be nice, it seems like we should probably be documenting
these functions too and I don't know that it's correct to characterize
them as 'internal'.  I can't say that I know exactly where the line is
between being a user-level function and an 'internal' function is, but
being used in a view that's created for users to query seems to me to
make it closer to user-level than, say, aclitemin.

Thanks,

Stephen

On Fri, 2021-10-15 at 13:52 -0400, Stephen Frost wrote:
> While I appreciate that
> it
> isn't really addressing the complaint you have that it'd be nice if
> we
> made things simpler for administrators by making it so they don't
> have
> to GRANT access to both the view and the function, and I can see how
> that would be nice, it seems like we should probably be documenting
> these functions too and I don't know that it's correct to
> characterize
> them as 'internal'.

I'm content with that explanation.

It would be nice if there was some kind of improvement here, but I
won't push too hard for it if there are security concerns.

Regards,
    Jeff Davis

On Fri, Oct 15, 2021 at 11:53 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2021-10-15 at 13:52 -0400, Stephen Frost wrote:
> > While I appreciate that
> > it
> > isn't really addressing the complaint you have that it'd be nice if
> > we
> > made things simpler for administrators by making it so they don't
> > have
> > to GRANT access to both the view and the function, and I can see how
> > that would be nice, it seems like we should probably be documenting
> > these functions too and I don't know that it's correct to
> > characterize
> > them as 'internal'.
>
> I'm content with that explanation.
>
> It would be nice if there was some kind of improvement here, but I
> won't push too hard for it if there are security concerns.

I tried to go through the discussion that happened upthread, following
is what I could grasp:
1) Documenting internal functions that are being used by some of the
views in system_views.sql: These functions have entries in the pg_proc
catalog and users are not restricted from using them. I agree that the
same permissions should be applied for the views and those functions.
If at all, others agree to document them, it should be discussed
separately and not in this thread as there are lots of functions.
Personally, I'm against documenting them all.
2)  Removal of superuser() checks in all (if possible) or some of the
functions as suggested in [1]: actually the list of functions having
superuser() checks is huge and I'm not sure all agree on this. It
should be discussed separately and not in this thread.

I would like to confine this thread to allowing non-superusers with a
predefined role (earlier suggestion was to use pg_read_all_stats) to
access views pg_backend_memory_contexts and pg_shmem_allocations and
functions pg_get_backend_memory_contexts and pg_get_shmem_allocations.
Attaching the previous v2 patch here for further review and thoughts.

[1] - https://www.postgresql.org/message-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw%40mail.gmail.com

Regards,
Bharath Rupireddy.

On 10/20/21, 11:44 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> I would like to confine this thread to allowing non-superusers with a
> predefined role (earlier suggestion was to use pg_read_all_stats) to
> access views pg_backend_memory_contexts and pg_shmem_allocations and
> functions pg_get_backend_memory_contexts and pg_get_shmem_allocations.
> Attaching the previous v2 patch here for further review and thoughts.

I took a look at the new patch.  The changes to system_views.sql look
good to me.  Let's be sure to update doc/src/sgml/catalogs.sgml as
well.

-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());

nitpick: Do we need to remove the "* FROM" here?  This seems like an
unrelated change.

+-- test to check privileges of system views pg_shmem_allocations,
+-- pg_backend_memory_contexts and function pg_log_backend_memory_contexts.

I think the comment needs to be updated to remove the reference to
pg_log_backend_memory_contexts.  It doesn't appear to be tested here.

+SELECT name, ident, parent, level, total_bytes >= free_bytes
+  FROM pg_backend_memory_contexts WHERE level = 0; -- permission denied error
+SELECT COUNT(*) >= 0 AS ok FROM pg_shmem_allocations; -- permission denied error

Since we're really just checking the basic permissions, could we just
do the "count(*) >= 0" check for both views?

Nathan

On Fri, Oct 22, 2021 at 3:15 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/20/21, 11:44 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> > I would like to confine this thread to allowing non-superusers with a
> > predefined role (earlier suggestion was to use pg_read_all_stats) to
> > access views pg_backend_memory_contexts and pg_shmem_allocations and
> > functions pg_get_backend_memory_contexts and pg_get_shmem_allocations.
> > Attaching the previous v2 patch here for further review and thoughts.
>
> I took a look at the new patch.  The changes to system_views.sql look
> good to me.

Thanks for reviewing.

> Let's be sure to update doc/src/sgml/catalogs.sgml as
> well.

Added.

> -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
> +SELECT pg_log_backend_memory_contexts(pg_backend_pid());
>
> nitpick: Do we need to remove the "* FROM" here?  This seems like an
> unrelated change.

Yes it's not mandatory, while we are on this I thought we could
combine them, I've also specified this in the commit message. IMO, we
can leave it to the committer.

> +-- test to check privileges of system views pg_shmem_allocations,
> +-- pg_backend_memory_contexts and function pg_log_backend_memory_contexts.
>
> I think the comment needs to be updated to remove the reference to
> pg_log_backend_memory_contexts.  It doesn't appear to be tested here.

Removed.

> +SELECT name, ident, parent, level, total_bytes >= free_bytes
> +  FROM pg_backend_memory_contexts WHERE level = 0; -- permission denied error
> +SELECT COUNT(*) >= 0 AS ok FROM pg_shmem_allocations; -- permission denied error
>
> Since we're really just checking the basic permissions, could we just
> do the "count(*) >= 0" check for both views?

Done.

Here's v3 for further review.

Regards,
Bharath Rupireddy.

On 10/21/21, 6:51 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> Here's v3 for further review.

I've marked this as ready-for-committer.  The only other feedback I
would offer is nitpicking at the test code to clean it up a little
bit, but I don't think it is necessary to block on that.

Nathan

On Wed, 2021-10-13 at 11:43 +0530, Bharath Rupireddy wrote:
> Here comes the v2 patch. Note that I've retained superuser() check in
> the pg_log_backend_memory_contexts(). Please review it.

FYI: I submitted a separate patch here to allow pg_signal_backend to
execute pg_log_backend_memory_contexts():


https://www.postgresql.org/message-id/flat/e5cf6684d17c8d1ef4904ae248605ccd6da03e72.camel@j-davis.com

So we can consider that patch separately.

Regards,
    Jeff Davis

On Fri, Oct 22, 2021 at 10:28 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/21/21, 6:51 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> > Here's v3 for further review.
>
> I've marked this as ready-for-committer.  The only other feedback I
> would offer is nitpicking at the test code to clean it up a little
> bit, but I don't think it is necessary to block on that.

I forgot to change the CATALOG_VERSION_NO (it can be set to right
value while committing the patch), I did it in the v4 attached here,
otherwise the patch remains same as v3.

Regards,
Bharath Rupireddy.