Thread
Commits
-
Allow GRANT on pg_log_backend_memory_contexts().
- f0b051e322d5 15.0 landed
-
Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-23T19:57:02Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Nathan Bossart <bossartn@amazon.com> — 2021-10-23T20:42:29Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Michael Paquier <michael@paquier.xyz> — 2021-10-23T23:41:52Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-10-24T14:28:16Z
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.
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-24T16:50:58Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-24T17:04:48Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Nathan Bossart <bossartn@amazon.com> — 2021-10-24T20:59:48Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-10-25T02:53:33Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-25T03:31:37Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-10-25T04:13:43Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-10-25T04:27:52Z
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.
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Michael Paquier <michael@paquier.xyz> — 2021-10-25T07:10:15Z
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. FWIW, if the barrier between a role and a function is thin, perhaps we'd better just limit ourselves to the removal of the superuser() checks for now rather than trying to plug more groups into the functions. When I have dealt with such issues in the past, I tend to just do the superuser()/REVOKE part without more GRANTs or even more system roles, as this is enough to give room to users to do what they want with their clusters. And this is a no-brainer. -- Michael
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-10-25T09:14:51Z
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.
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-10-25T09:19:57Z
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.
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Nathan Bossart <bossartn@amazon.com> — 2021-10-25T16:15:20Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Andres Freund <andres@anarazel.de> — 2021-10-25T17:29:39Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-25T20:42:07Z
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. Using pg_signal_backend does not seem to be universally acceptable, so I'll just drop the idea of granting to that predefined role. Regards, Jeff Davis
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Nathan Bossart <bossartn@amazon.com> — 2021-10-25T21:26:47Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Andres Freund <andres@anarazel.de> — 2021-10-25T21:30:38Z
Hi, On 2021-10-25 13:42:07 -0700, Jeff Davis wrote: > 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. 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. Greetings, Andres Freund
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-25T23:28:13Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Nathan Bossart <bossartn@amazon.com> — 2021-10-25T23:58:31Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-10-26T14:32:00Z
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
-
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
Jeff Davis <pgsql@j-davis.com> — 2021-10-26T21:04:14Z
On Tue, 2021-10-26 at 23:32 +0900, Fujii Masao wrote: > 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? A similar statement already exists right above the table of functions: " Use of these functions is restricted to superusers by default but access may be granted to others using GRANT, with noted exceptions." Committed the version that merely removes the superuser check, and revokes from public. Privilege can be granted to non-superusers if desired. Thanks everyone for looking. Regards, Jeff Davis