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
Attachments:
0001-Allow-pg_signal_backend-members-to-call-pg_log_backe.patchtext/x-patch; charset=UTF-8; name=0001-Allow-pg_signal_backend-members-to-call-pg_log_backe.patchDownload
From 473e0bb2e1ae0d56dbc6b0262c16b10c6c0454cc Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 22 Oct 2021 13:40:35 -0700
Subject: [PATCH] Allow pg_signal_backend members to call
pg_log_backend_memory_contexts().
---
src/backend/catalog/system_functions.sql | 5 +++++
src/backend/utils/adt/mcxtfuncs.c | 6 -----
src/include/catalog/catversion.h | 2 +-
src/test/regress/expected/misc_functions.out | 23 ++++++++++++++++++--
src/test/regress/sql/misc_functions.sql | 15 +++++++++++--
5 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index a416e94d371..2cd5e3fe17b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -699,6 +699,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC;
+
--
-- We also set up some things as accessible to standard roles.
--
@@ -713,6 +715,9 @@ GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO pg_signal_backend;
+
GRANT pg_read_all_settings TO pg_monitor;
GRANT pg_read_all_stats TO pg_monitor;
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc32..e06280ad712 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -177,12 +177,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
int pid = PG_GETARG_INT32(0);
PGPROC *proc;
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
proc = BackendPidGetProc(pid);
/*
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..039f6338604 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110230
#endif
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38d..38e36fec43e 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -138,14 +138,33 @@ HINT: No function matches the given name and argument types. You might need to
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
pg_log_backend_memory_contexts
--------------------------------
t
(1 row)
+CREATE ROLE testrole1 IN ROLE pg_signal_backend;
+CREATE ROLE testrole2;
+SELECT has_function_privilege('testrole1',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('testrole2',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+DROP ROLE testrole1;
+DROP ROLE testrole2;
--
-- Test some built-in SRFs
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc6..abf8b33b11c 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -35,9 +35,20 @@ SELECT num_nulls();
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+
+CREATE ROLE testrole1 IN ROLE pg_signal_backend;
+CREATE ROLE testrole2;
+SELECT has_function_privilege('testrole1',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('testrole2',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+DROP ROLE testrole1;
+DROP ROLE testrole2;
--
-- Test some built-in SRFs
--
2.17.1
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]/messages/by-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw@mail.gmail.com. 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]* Permission checking for this function is managed through the normal * GRANT system. */) 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]: /messages/by-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw@mail.gmail.com
[2]: * Permission checking for this function is managed through the normal * GRANT system. */
* Permission checking for this function is managed through the normal
* GRANT system.
*/
[3]: /messages/by-id/CALj2ACVXk1roswqFpiCOMHrsB+xxW7HG536krGAzF=mWXh3eWQ@mail.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
Attachments:
0001-Allow-pg_signal_backend-members-to-call-pg_log_backe.patchtext/x-patch; charset=UTF-8; name=0001-Allow-pg_signal_backend-members-to-call-pg_log_backe.patchDownload
From 3be296819d6195cbafce72ab085c8430f1e8a502 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 22 Oct 2021 13:40:35 -0700
Subject: [PATCH] Allow pg_signal_backend members to call
pg_log_backend_memory_contexts().
Discussion: https://postgr.es/m/e5cf6684d17c8d1ef4904ae248605ccd6da03e72.camel@j-davis.com
---
doc/src/sgml/func.sgml | 5 +++--
src/backend/catalog/system_functions.sql | 5 +++++
src/backend/utils/adt/mcxtfuncs.c | 20 ++++++++---------
src/include/catalog/catversion.h | 2 +-
src/test/regress/expected/misc_functions.out | 23 ++++++++++++++++++--
src/test/regress/sql/misc_functions.sql | 15 +++++++++++--
6 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5677032cb28..ee8903fe484 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25332,8 +25332,9 @@ SELECT collation for ('foo' COLLATE "de_DE");
(See <xref linkend="runtime-config-logging"/> for more information),
but will not be sent to the client regardless of
<xref linkend="guc-client-min-messages"/>.
- Only superusers can request to log the memory contexts.
- </para></entry>
+ This is also allowed if the calling role has been granted
+ <literal>pg_signal_backend</literal>, however only superusers can
+ cancel superuser backends. </para></entry>
</row>
<row>
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index a416e94d371..2cd5e3fe17b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -699,6 +699,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC;
+
--
-- We also set up some things as accessible to standard roles.
--
@@ -713,6 +715,9 @@ GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO pg_signal_backend;
+
GRANT pg_read_all_settings TO pg_monitor;
GRANT pg_read_all_stats TO pg_monitor;
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc32..a2f45fb8578 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -162,10 +162,10 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
* pg_log_backend_memory_contexts
* Signal a backend process to log its memory contexts.
*
- * Only superusers are allowed to signal to log the memory contexts
- * because allowing any users to issue this request at an unbounded
- * rate would cause lots of log messages and which can lead to
- * denial of service.
+ * Only members of pg_signal_backend are allowed to signal to log the memory
+ * contexts because allowing any users to issue this request at an unbounded
+ * rate would cause lots of log messages and which can lead to denial of
+ * service.
*
* On receipt of this signal, a backend sets the flag in the signal
* handler, which causes the next CHECK_FOR_INTERRUPTS() to log the
@@ -177,12 +177,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
int pid = PG_GETARG_INT32(0);
PGPROC *proc;
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
proc = BackendPidGetProc(pid);
/*
@@ -205,6 +199,12 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
+ /* Only allow superusers to signal superuser-owned backends. */
+ if (superuser_arg(proc->roleId) && !superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be a superuser to log memory contexts of superuser backend")));
+
if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
{
/* Again, just a warning to allow loops */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..039f6338604 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110230
#endif
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38d..a279c164020 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -138,14 +138,33 @@ HINT: No function matches the given name and argument types. You might need to
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
pg_log_backend_memory_contexts
--------------------------------
t
(1 row)
+CREATE ROLE regress_signal IN ROLE pg_signal_backend;
+CREATE ROLE regress_nosignal;
+SELECT has_function_privilege('regress_signal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_nosignal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+DROP ROLE regress_signal;
+DROP ROLE regress_nosignal;
--
-- Test some built-in SRFs
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc6..0dd8998b62c 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -35,9 +35,20 @@ SELECT num_nulls();
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+
+CREATE ROLE regress_signal IN ROLE pg_signal_backend;
+CREATE ROLE regress_nosignal;
+SELECT has_function_privilege('regress_signal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_nosignal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+DROP ROLE regress_signal;
+DROP ROLE regress_nosignal;
--
-- Test some built-in SRFs
--
2.17.1
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 notYes. 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 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
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]/messages/by-id/CALj2ACUhCFSUQmZhiQ+w1kZdJGmhNP2cd1LZS4GVGowyjiqftQ@mail.gmail.com and [2]/messages/by-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw@mail.gmail.com.
[1]: /messages/by-id/CALj2ACUhCFSUQmZhiQ+w1kZdJGmhNP2cd1LZS4GVGowyjiqftQ@mail.gmail.com
[2]: /messages/by-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw@mail.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 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
Attachments:
pg-log-memory-contexts.difftext/x-patch; charset=UTF-8; name=pg-log-memory-contexts.diffDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5677032cb28..b7003fd37a3 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25332,7 +25332,8 @@ SELECT collation for ('foo' COLLATE "de_DE");
(See <xref linkend="runtime-config-logging"/> for more information),
but will not be sent to the client regardless of
<xref linkend="guc-client-min-messages"/>.
- Only superusers can request to log the memory contexts.
+ Only superusers can request to log the memory contexts of superuser
+ backends.
</para></entry>
</row>
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index a416e94d371..54c93b16c4c 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -699,6 +699,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC;
+
--
-- We also set up some things as accessible to standard roles.
--
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc32..38ca0ddee73 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -162,10 +162,11 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
* pg_log_backend_memory_contexts
* Signal a backend process to log its memory contexts.
*
- * Only superusers are allowed to signal to log the memory contexts
- * because allowing any users to issue this request at an unbounded
- * rate would cause lots of log messages and which can lead to
- * denial of service.
+ * By default, only superusers are allowed to signal to log the memory
+ * contexts because allowing any users to issue this request at an unbounded
+ * rate would cause lots of log messages and which can lead to denial of
+ * service. Additional roles can be permitted with GRANT, but they will still
+ * be prevented from logging the memory contexts of a superuser backend.
*
* On receipt of this signal, a backend sets the flag in the signal
* handler, which causes the next CHECK_FOR_INTERRUPTS() to log the
@@ -177,12 +178,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
int pid = PG_GETARG_INT32(0);
PGPROC *proc;
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
proc = BackendPidGetProc(pid);
/*
@@ -205,6 +200,12 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
+ /* Only allow superusers to signal superuser-owned backends. */
+ if (superuser_arg(proc->roleId) && !superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be a superuser to log memory contexts of superuser backend")));
+
if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
{
/* Again, just a warning to allow loops */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..039f6338604 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110230
#endif
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38d..07c3d3311b1 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -138,14 +138,40 @@ HINT: No function matches the given name and argument types. You might need to
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
pg_log_backend_memory_contexts
--------------------------------
t
(1 row)
+CREATE ROLE regress_log_memory;
+SELECT has_function_privilege('regress_log_memory',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO regress_log_memory;
+SELECT has_function_privilege('regress_log_memory',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- still fails because it's a superuser backend
+SET ROLE regress_log_memory;
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+ERROR: must be a superuser to log memory contexts of superuser backend
+RESET ROLE;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ FROM regress_log_memory;
+DROP ROLE regress_log_memory;
--
-- Test some built-in SRFs
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc6..577ac3d7171 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -35,9 +35,32 @@ SELECT num_nulls();
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+
+CREATE ROLE regress_log_memory;
+
+SELECT has_function_privilege('regress_log_memory',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO regress_log_memory;
+
+SELECT has_function_privilege('regress_log_memory',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+
+-- still fails because it's a superuser backend
+SET ROLE regress_log_memory;
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+RESET ROLE;
+
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ FROM regress_log_memory;
+
+DROP ROLE regress_log_memory;
--
-- Test some built-in SRFs
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
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
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]: /messages/by-id/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
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