Thread: Allow pg_signal_backend members to use pg_log_backend_memory_stats().

Simple patch to implement $SUBJECT attached.

pg_signal_backend seems like the appropriate predefined role, because
pg_log_backend_memory_contexts() is implemented by a sending signal.

Regards,
    Jeff Davis

On 10/23/21, 12:57 PM, "Jeff Davis" <pgsql@j-davis.com> wrote:
> pg_signal_backend seems like the appropriate predefined role, because
> pg_log_backend_memory_contexts() is implemented by a sending signal.

This seems reasonable to me.  The stated reason in the original commit
message for keeping it restricted to superusers is because of the
denial-of-service risk, but if you've got pg_signal_backend, you can
already terminate sessions.  The predefined roles documentation notes
that members of pg_signal_backend cannot signal superuser-owned
backends, but AFAICT pg_log_backend_memory_contexts() has no such
restriction at the moment.  Should we add this?

Otherwise, presumably we will need to update func.sgml and the comment
above pg_log_backend_memory_contexts() in mcxtfuncs.c.

This is unrelated to this patch, but should we also consider opening
up pg_reload_conf() and pg_rotate_logfile() to members of
pg_signal_backend?  Those are the other "server signaling functions" I
see in the docs.

Nathan

On Sat, Oct 23, 2021 at 08:42:29PM +0000, Bossart, Nathan wrote:
> Otherwise, presumably we will need to update func.sgml and the comment
> above pg_log_backend_memory_contexts() in mcxtfuncs.c.

Yes, the documentation of any SQL function whose hardcoded superuser()
check is removed needs a refresh to outline that its execution can be
GRANT-ed post-initialization, and it should also document which system
roles are able to use it.  See for instance pg_database_size(), that
mentions roles need to be a member of pg_read_all_stats.

> This is unrelated to this patch, but should we also consider opening
> up pg_reload_conf() and pg_rotate_logfile() to members of
> pg_signal_backend?  Those are the other "server signaling functions" I
> see in the docs.

Yes, there is that as well.

+CREATE ROLE testrole1 IN ROLE pg_signal_backend;
+CREATE ROLE testrole2;
Any role created in the regression test needs to be prefixed with
"regress_", or builds with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
will complain (I just add that by default to not fall into this trap
again).
--
Michael

On Sun, Oct 24, 2021 at 1:27 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
>
> Simple patch to implement $SUBJECT attached.
>
> pg_signal_backend seems like the appropriate predefined role, because
> pg_log_backend_memory_contexts() is implemented by a sending signal.

+1.

It looks like we are better off with removing explicit superuser()
checks from the functions and using normal GRANT based system, see
others agreeing on this at [1]. As we have lots of functions that are
doing explicit superuser() checks, I'm sure someday they all will have
to be moved to GRANT system. The current code is a mix - some
functions do explicit checks (I've seen many of them with the comment
at [2]) and others do it via GRANT system. I'm not saying that we
should be dealing with those here in this thread, all I'm looking for
is that we have a note of it in the postgres todo list in the wiki so
that someone interested can pick that work up. Thoughts?

Comments on the patch:
1) testrole1 and testrole2 look generic, how about
regress_mcxt_role1/2? There's no problem as they are
misc_functions.sql local, but still role names can be more readable.
+CREATE ROLE testrole1 IN ROLE pg_signal_backend;
+CREATE ROLE testrole2;
2) It seems like the privileges.sql is the right place to place the
test cases, but I'm fine with keeping all the test cases of the
function together.
3) It might be enough to do has_function_privilege, just a thought -
isn't it better if we execute the function with the test roles set in.
This will help us catch the permission denied error message in the
test output files.
4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to
the date on which the patch gets committed?
5) The following change is being handled in the patch at [3], I know
it is appropriate to have it in this patch, but please mention it in
the commit message on why we do this change. I will remove this change
from my patch at [3].
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());

[1] - https://www.postgresql.org/message-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw%40mail.gmail.com
[2] -
 * Permission checking for this function is managed through the normal
 * GRANT system.
 */
[3] - https://www.postgresql.org/message-id/CALj2ACVXk1roswqFpiCOMHrsB%2BxxW7HG536krGAzF%3DmWXh3eWQ%40mail.gmail.com

Regards,
Bharath Rupireddy.

On Sat, 2021-10-23 at 20:42 +0000, Bossart, Nathan wrote:
> The predefined roles documentation notes
> that members of pg_signal_backend cannot signal superuser-owned
> backends, but AFAICT pg_log_backend_memory_contexts() has no such
> restriction at the moment.  Should we add this?

Added, good catch.

> This is unrelated to this patch, but should we also consider opening
> up pg_reload_conf() and pg_rotate_logfile() to members of
> pg_signal_backend?  Those are the other "server signaling functions"
> I
> see in the docs.

Those are actually signalling the postmaster, not an ordinary backend.
Also, those functions are already GRANTable, so I think we should leave
them as-is.

Regards,
    Jeff Davis

On Sun, 2021-10-24 at 19:58 +0530, Bharath Rupireddy wrote:
> It looks like we are better off with removing explicit superuser()
> checks from the functions and using normal GRANT based system, see
> others agreeing on this at [1]. As we have lots of functions that are
> doing explicit superuser() checks, I'm sure someday they all will
> have
> to be moved to GRANT system.

Note that some functions have additional checks that can't be expressed
with GRANT -- see pg_cancel_backend(), for example. But I agree in
general that GRANT is the way to go most of the time.

>  The current code is a mix - some
> functions do explicit checks (I've seen many of them with the comment
> at [2]) and others do it via GRANT system. I'm not saying that we
> should be dealing with those here in this thread, all I'm looking for
> is that we have a note of it in the postgres todo list in the wiki so
> that someone interested can pick that work up. Thoughts?

It seems like there's agreement on the direction, but I don't know that
there's a good place to write it down. Probably better to just fix as
many of the functions as we can, and then when people add new ones,
they'll copy the GRANT pattern rather than the explicit superuser
check.

> Comments on the patch:
> 1) testrole1 and testrole2 look generic, how about

Michael had a similar comment. Renamed, thank you.

> 2) It seems like the privileges.sql is the right place to place the
> test cases, but I'm fine with keeping all the test cases of the
> function together.

If we add all the function privilege checks there, I think it will
overwhelm the other interesting tests happening in that file.

> 3) It might be enough to do has_function_privilege, just a thought -
> isn't it better if we execute the function with the test roles set
> in.
> This will help us catch the permission denied error message in the
> test output files.

Missed this comment. I'll tweak this before commit.

> 4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to
> the date on which the patch gets committed?

I just put it in there so that I wouldn't forget, but I'll update it at
commit time.

> 5) The following change is being handled in the patch at [3], I know
> it is appropriate to have it in this patch, but please mention it in
> the commit message on why we do this change. I will remove this
> change
> from my patch at [3].
> -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
> +SELECT pg_log_backend_memory_contexts(pg_backend_pid());

What would you like me to mention?

Regards,
    Jeff Davis

On 10/24/21, 9:51 AM, "Jeff Davis" <pgsql@j-davis.com> wrote:
> On Sat, 2021-10-23 at 20:42 +0000, Bossart, Nathan wrote:
>> The predefined roles documentation notes
>> that members of pg_signal_backend cannot signal superuser-owned
>> backends, but AFAICT pg_log_backend_memory_contexts() has no such
>> restriction at the moment.  Should we add this?
>
> Added, good catch.

The new patch looks good to me.

Nathan

At Sun, 24 Oct 2021 09:50:58 -0700, Jeff Davis <pgsql@j-davis.com> wrote in 
> On Sat, 2021-10-23 at 20:42 +0000, Bossart, Nathan wrote:
> > The predefined roles documentation notes
> > that members of pg_signal_backend cannot signal superuser-owned
> > backends, but AFAICT pg_log_backend_memory_contexts() has no such
> > restriction at the moment.  Should we add this?
> 
> Added, good catch.
> 
> > This is unrelated to this patch, but should we also consider opening
> > up pg_reload_conf() and pg_rotate_logfile() to members of
> > pg_signal_backend?  Those are the other "server signaling functions"
> > I
> > see in the docs.
> 
> Those are actually signalling the postmaster, not an ordinary backend.
> Also, those functions are already GRANTable, so I think we should leave
> them as-is.

I'm afraid that it might be wrong that all backend-signalling features
are allowed by that priviledge.  pg_signal_backends is described in
the doc as:

https://www.postgresql.org/docs/devel/predefined-roles.html

> Signal another backend to cancel a query or terminate its session.

Here, the term "signal" there seems to mean interrupting something on
that session or the session itself.  Addition to that I don't think
"terminate a session or the query on a session" and "log something on
another session" and "rotate log file" don't fall into the same
category in a severity view.

In other words, I don't think pg_signal_backends is not meant to
control "log something on another session" or "rotate log file".  It's
danger that if we allow somewone to rotate log files, that means to
allow same user to terminate another session.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, 2021-10-25 at 11:53 +0900, Kyotaro Horiguchi wrote:
> In other words, I don't think pg_signal_backends is not meant to
> control "log something on another session" or "rotate log file". 
> It's
> danger that if we allow somewone to rotate log files, that means to
> allow same user to terminate another session.

The current patch doesn't allow members of pg_signal_backend to rotate
the log file.

Do you think pg_signal_backend is the wrong group to allow usage of
pg_log_backend_memory_contexts()? Alternatively, it could simply not
GRANT anything, and leave that up to the administrator to choose who
can use it.

Regards,
    Jeff Davis

At Sun, 24 Oct 2021 20:31:37 -0700, Jeff Davis <pgsql@j-davis.com> wrote in 
> On Mon, 2021-10-25 at 11:53 +0900, Kyotaro Horiguchi wrote:
> > In other words, I don't think pg_signal_backends is not meant to
> > control "log something on another session" or "rotate log file". 
> > It's
> > danger that if we allow somewone to rotate log files, that means to
> > allow same user to terminate another session.
> 
> The current patch doesn't allow members of pg_signal_backend to rotate
> the log file.

Ah, sorry, I might have confused with some other discussion.

> Do you think pg_signal_backend is the wrong group to allow usage of
> pg_log_backend_memory_contexts()? Alternatively, it could simply not

Yes. I think it would be danger that who is allowed to dump memory
context into log files by granting pg_signal_backend also can
terminate other backends.

> pg_log_backend_memory_contexts()? Alternatively, it could simply not
> GRANT anything, and leave that up to the administrator to choose who
> can use it.

*I* prefer that. I'm not sure I'm the only one to think so, though..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Oct 25, 2021 at 9:43 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> > Do you think pg_signal_backend is the wrong group to allow usage of
> > pg_log_backend_memory_contexts()? Alternatively, it could simply not
>
> Yes. I think it would be danger that who is allowed to dump memory
> context into log files by granting pg_signal_backend also can
> terminate other backends.
>
> > pg_log_backend_memory_contexts()? Alternatively, it could simply not
> > GRANT anything, and leave that up to the administrator to choose who
> > can use it.
>
> *I* prefer that. I'm not sure I'm the only one to think so, though..

How about we have a separate predefined role for the functions that
deal with server logs? I'm not sure if Mark Dilger's patch on new
predefined roles has one, if not, how about something like
pg_write_server_log/pg_manage_server_log/some other name?

If not with a new predefined role, how about expanding the scope of
existing pg_write_server_files role?

Regards,
Bharath Rupireddy.

On Sun, Oct 24, 2021 at 10:34 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > 5) The following change is being handled in the patch at [3], I know
> > it is appropriate to have it in this patch, but please mention it in
> > the commit message on why we do this change. I will remove this
> > change
> > from my patch at [3].
> > -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
> > +SELECT pg_log_backend_memory_contexts(pg_backend_pid());
>
> What would you like me to mention?

Something like below in the commit message would be good:
"While on this, change the way the tests use pg_log_backend_memory_contexts()
Usually for functions, we don't use "SELECT-FROM-<<function>>",
we just use "SELECT-<<function>>"."

Regards,
Bharath Rupireddy.

On Mon, Oct 25, 2021 at 12:40 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Oct 24, 2021 at 08:31:37PM -0700, Jeff Davis wrote:
> > The current patch doesn't allow members of pg_signal_backend to rotate
> > the log file.
> >
> > Do you think pg_signal_backend is the wrong group to allow usage of
> > pg_log_backend_memory_contexts()? Alternatively, it could simply not
> > GRANT anything, and leave that up to the administrator to choose who
> > can use it.
>
> Hmm.  Why don't you split the patch into two parts that can be
> discussed separately then?  There would be one to remove all the
> superuser() checks you can think of, and a potential second to grant
> those function's execution to some system role.

IMO, in this thread we can focus on remvong the
pg_log_backend_memory_contexts()'s superuser() check and +1 to start a
separate thread to remove superuser() checks for the other functions
and REVOKE the permissions in appropriate places, for system functins
system_functions.sql files, for extension functions, the extension
installation .sql files. See [1] and [2].

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

Regards,
Bharath Rupireddy.

On 10/25/21, 2:21 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Oct 25, 2021 at 12:40 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm.  Why don't you split the patch into two parts that can be
>> discussed separately then?  There would be one to remove all the
>> superuser() checks you can think of, and a potential second to grant
>> those function's execution to some system role.
>
> IMO, in this thread we can focus on remvong the
> pg_log_backend_memory_contexts()'s superuser() check and +1 to start a
> separate thread to remove superuser() checks for the other functions
> and REVOKE the permissions in appropriate places, for system functins
> system_functions.sql files, for extension functions, the extension
> installation .sql files. See [1] and [2].

I like the general idea of removing hard-coded superuser checks first
and granting execution to predefined roles second.  I don't have any
strong opinion about what should be done in this thread and what
should be done elsewhere.

Nathan

Hi,

On 2021-10-23 12:57:02 -0700, Jeff Davis wrote:
> Simple patch to implement $SUBJECT attached.
> 
> pg_signal_backend seems like the appropriate predefined role, because
> pg_log_backend_memory_contexts() is implemented by a sending signal.

I like the idea of making pg_log_backend_memory_contexts() more widely
available. But I think tying it to pg_signal_backend isn't great. It's
unnecessarily baking in an implementation detail.

Greetings,

Andres Freund

On 10/25/21, 1:43 PM, "Jeff Davis" <pgsql@j-davis.com> wrote:
> On Mon, 2021-10-25 at 16:10 +0900, Michael Paquier wrote:
>> Hmm.  Why don't you split the patch into two parts that can be
>> discussed separately then?  There would be one to remove all the
>> superuser() checks you can think of, and a potential second to grant
>> those function's execution to some system role.
>
> Good idea. Attached a patch to remove the superuser check on
> pg_log_backend_memory_contexts(), except in the case when trying to log
> memory contexts of a superuser backend.

LGTM.

Nathan

On Mon, 2021-10-25 at 14:30 -0700, Andres Freund wrote:
> I don't get the reasoning behind the "except ..." logic. What does
> this
> actually protect against? A reasonable use case for this feature is
> is to
> monitor memory usage of all backends, and this restriction practially
> requires
> to still use a security definer function.

Nathan brought it up -- more as a question than a request, so perhaps
it's not necessary. I don't have a strong opinion about it, but I
included it to be conservative (easier to relax a privilege than to
tighten one).

I can cut out the in-function check entirely if there's no objection.

Regards,
    Jeff Davis

[1] https://postgr.es/m/33F34F0C-BB16-48DE-B125-95D340A54AE8@amazon.com

On 10/25/21, 4:29 PM, "Jeff Davis" <pgsql@j-davis.com> wrote:
> On Mon, 2021-10-25 at 14:30 -0700, Andres Freund wrote:
>> I don't get the reasoning behind the "except ..." logic. What does
>> this
>> actually protect against? A reasonable use case for this feature is
>> is to
>> monitor memory usage of all backends, and this restriction practially
>> requires
>> to still use a security definer function.
>
> Nathan brought it up -- more as a question than a request, so perhaps
> it's not necessary. I don't have a strong opinion about it, but I
> included it to be conservative (easier to relax a privilege than to
> tighten one).

I asked about it since we were going to grant execution to
pg_signal_backend, which (per the docs) shouldn't be able to signal a
superuser-owned backend.  I don't mind removing this now that the
pg_signal_backend part is removed.

Nathan

On 2021/10/26 5:42, Jeff Davis wrote:
> On Mon, 2021-10-25 at 16:10 +0900, Michael Paquier wrote:
>> Hmm.  Why don't you split the patch into two parts that can be
>> discussed separately then?  There would be one to remove all the
>> superuser() checks you can think of, and a potential second to grant
>> those function's execution to some system role.
> 
> Good idea. Attached a patch to remove the superuser check on
> pg_log_backend_memory_contexts(), except in the case when trying to log
> memory contexts of a superuser backend.

-        Only superusers can request to log the memory contexts.
+        Only superusers can request to log the memory contexts of superuser
+        backends.

The description "This function is restricted to superusers by default,
but other users can be granted EXECUTE to run the function."
should be added into the docs?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION