public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 1/1] move session auth permission check
6+ messages / 4 participants
[nested] [flat]
* [PATCH v4 1/1] move session auth permission check
@ 2023-07-09 04:35 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Bossart @ 2023-07-09 04:35 UTC (permalink / raw)
---
src/backend/commands/variable.c | 23 +++++++++++++++++++++++
src/backend/utils/init/miscinit.c | 29 ++++++++++-------------------
src/include/miscadmin.h | 1 +
3 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..f8e308f1d0 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,29 @@ check_session_authorization(char **newval, void **extra, GucSource source)
ReleaseSysCache(roleTup);
+ /*
+ * Only a superuser may set auth ID to something other than himself. Note
+ * that in case of multiple SETs in a single session, the original
+ * userid's superuserness is what matters. But we set the GUC variable
+ * is_superuser to indicate whether the *current* session userid is a
+ * superuser.
+ */
+ if (roleid != GetAuthenticatedUserId() &&
+ !GetAuthenticatedUserIsSuperuser())
+ {
+ if (source == PGC_S_TEST)
+ {
+ ereport(NOTICE,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission will be denied to set session authorization \"%s\"",
+ *newval)));
+ return true;
+ }
+ GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+ GUC_check_errmsg("permission denied to set session authorization");
+ return false;
+ }
+
/* Set up "extra" struct for assign_session_authorization to use */
myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
if (!myextra)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..f5548a0f47 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -582,6 +582,16 @@ GetAuthenticatedUserId(void)
return AuthenticatedUserId;
}
+/*
+ * Return whether the authenticated user was superuser at connection start.
+ */
+bool
+GetAuthenticatedUserIsSuperuser(void)
+{
+ Assert(OidIsValid(AuthenticatedUserId));
+ return AuthenticatedUserIsSuperuser;
+}
+
/*
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -888,29 +898,10 @@ system_user(PG_FUNCTION_ARGS)
/*
* Change session auth ID while running
- *
- * Only a superuser may set auth ID to something other than himself. Note
- * that in case of multiple SETs in a single session, the original userid's
- * superuserness is what matters. But we set the GUC variable is_superuser
- * to indicate whether the *current* session userid is a superuser.
- *
- * Note: this is not an especially clean place to do the permission check.
- * It's OK because the check does not require catalog access and can't
- * fail during an end-of-transaction GUC reversion, but we may someday
- * have to push it up into assign_session_authorization.
*/
void
SetSessionAuthorization(Oid userid, bool is_superuser)
{
- /* Must have authenticated already, else can't make permission check */
- Assert(OidIsValid(AuthenticatedUserId));
-
- if (userid != AuthenticatedUserId &&
- !AuthenticatedUserIsSuperuser)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to set session authorization")));
-
SetSessionUserId(userid, is_superuser);
SetConfigOption("is_superuser",
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc2..11d6e6869d 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -357,6 +357,7 @@ extern Oid GetUserId(void);
extern Oid GetOuterUserId(void);
extern Oid GetSessionUserId(void);
extern Oid GetAuthenticatedUserId(void);
+extern bool GetAuthenticatedUserIsSuperuser(void);
extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
--
2.25.1
--45Z9DzgjV8m4Oswq--
^ permalink raw reply [nested|flat] 6+ messages in thread
* Support worker_spi to execute the function dynamically.
@ 2023-07-20 02:15 Masahiro Ikeda <[email protected]>
2023-07-20 03:55 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Masahiro Ikeda @ 2023-07-20 02:15 UTC (permalink / raw)
To: [email protected]
Hi,
While I'm working on the thread[1], I found that the function of
worker_spi module fails if 'shared_preload_libraries' doesn't have
worker_spi.
The reason is that the database name is NULL because the database name
is initialized only when process_shared_preload_libraries_in_progress
is true.
```
psql=# SELECT worker_spi_launch(1) ;
2023-07-20 11:00:56.491 JST [1179891] LOG: worker_spi worker 1
initialized with schema1.counted
2023-07-20 11:00:56.491 JST [1179891] FATAL: cannot read pg_class
without having selected a database at character 22
2023-07-20 11:00:56.491 JST [1179891] QUERY: select count(*) from
pg_namespace where nspname = 'schema1'
2023-07-20 11:00:56.491 JST [1179891] STATEMENT: select count(*) from
pg_namespace where nspname = 'schema1'
2023-07-20 11:00:56.492 JST [1179095] LOG: background worker
"worker_spi" (PID 1179891) exited with exit code 1
```
In my understanding, the restriction is not required. So, I think it's
better to change the behavior.
(v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)
What do you think?
[1] Support to define custom wait events for extensions
https://www.postgresql.org/message-id/flat/b9f5411acda0cf15c8fbb767702ff43e%40oss.nttdata.com
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachments:
[text/x-diff] v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch (1.7K, ../../[email protected]/2-v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)
download | inline diff:
From c6e60c66c4066b4a01981ffae5a168901e7283eb Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <[email protected]>
Date: Thu, 20 Jul 2023 10:34:50 +0900
Subject: [PATCH] Support worker_spi to execute the function dynamically.
Currently, the database name to connect is initialized only
when process_shared_preload_libraries_in_progress is true.
It means that worker_spi_launch() fails if shared_preload_libraries
is empty because the database to connect is NULL.
The patch changes that the database name is always initilized
when called _PG_init(). We can call worker_spi_launch() and
launch SPI workers dynamically.
---
src/test/modules/worker_spi/worker_spi.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 7227cfaa45..ccc38a36d3 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -296,6 +296,15 @@ _PG_init(void)
NULL,
NULL);
+ DefineCustomStringVariable("worker_spi.database",
+ "Database to connect to.",
+ NULL,
+ &worker_spi_database,
+ "postgres",
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
+
if (!process_shared_preload_libraries_in_progress)
return;
@@ -312,15 +321,6 @@ _PG_init(void)
NULL,
NULL);
- DefineCustomStringVariable("worker_spi.database",
- "Database to connect to.",
- NULL,
- &worker_spi_database,
- "postgres",
- PGC_POSTMASTER,
- 0,
- NULL, NULL, NULL);
-
MarkGUCPrefixReserved("worker_spi");
/* set up common data for all our workers */
--
2.25.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 02:15 Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
@ 2023-07-20 03:55 ` Michael Paquier <[email protected]>
2023-07-20 08:54 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2023-07-20 03:55 UTC (permalink / raw)
To: Masahiro Ikeda <[email protected]>; +Cc: [email protected]
On Thu, Jul 20, 2023 at 11:15:51AM +0900, Masahiro Ikeda wrote:
> While I'm working on the thread[1], I found that the function of
> worker_spi module fails if 'shared_preload_libraries' doesn't have
> worker_spi.
I guess that you were patching worker_spi to register dynamically a
wait event and embed that in a TAP test or similar without loading it
in shared_preload_libraries? FWIW, you could use a trick like what I
am attaching here to load a wait event dynamically with the custom
wait event API. You would need to make worker_spi_init_shmem() a bit
more aggressive with an extra hook to reserve a shmem area size, but
that's enough to show the custom wait event in the same backend as the
one that launches a worker_spi dynamically, while demonstrating how
the API can be used in this case.
> In my understanding, the restriction is not required. So, I think it's
> better to change the behavior.
> (v1-0001-Support-worker_spi-to-execute-the-function-dynamical.patch)
>
> What do you think?
+1. I'm OK to lift this restriction with a SIGHUP GUC for the
database name and that's not a pattern to encourage in a template
module. Will do so, if there are no objections.
--
Michael
Attachments:
[text/x-diff] 0001-Add-tweak-to-allow-worker_spi-to-register-wait_event.patch (2.5K, ../../ZLiwP5vrT%[email protected]/2-0001-Add-tweak-to-allow-worker_spi-to-register-wait_event.patch)
download | inline diff:
From 2fcd773cd4009cffd626640e1553d4efdceb0777 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 20 Jul 2023 12:48:07 +0900
Subject: [PATCH] Add tweak to allow worker_spi to register wait_event
dynamically
---
src/test/modules/worker_spi/worker_spi.c | 35 +++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index b87d6c75d8..4e3da93129 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -48,11 +48,20 @@ PG_FUNCTION_INFO_V1(worker_spi_launch);
PGDLLEXPORT void worker_spi_main(Datum main_arg) pg_attribute_noreturn();
+static void worker_spi_init_shmem(void);
+
/* GUC variables */
static int worker_spi_naptime = 10;
static int worker_spi_total_workers = 2;
static char *worker_spi_database;
+typedef struct WorkerSpiState
+{
+ uint32 wait_event_info;
+} WorkerSpiState;
+
+/* the pointer to the shared memory */
+static WorkerSpiState *worker_spi_state = NULL;
typedef struct worktable
{
@@ -149,6 +158,8 @@ worker_spi_main(Datum main_arg)
/* We're now ready to receive signals */
BackgroundWorkerUnblockSignals();
+ worker_spi_init_shmem();
+
/* Connect to our database */
BackgroundWorkerInitializeConnection(worker_spi_database, NULL, 0);
@@ -199,7 +210,7 @@ worker_spi_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
worker_spi_naptime * 1000L,
- WAIT_EVENT_EXTENSION);
+ worker_spi_state->wait_event_info);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
@@ -346,6 +357,26 @@ _PG_init(void)
}
}
+static void
+worker_spi_init_shmem(void)
+{
+ bool found = false;
+
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ worker_spi_state = ShmemInitStruct("worker_spi",
+ sizeof(WorkerSpiState),
+ &found);
+ if (!found)
+ {
+ /* First time through ... */
+ worker_spi_state->wait_event_info = WaitEventExtensionNew();
+ }
+ LWLockRelease(AddinShmemInitLock);
+
+ WaitEventExtensionRegisterName(worker_spi_state->wait_event_info,
+ "worker_spi_custom");
+}
+
/*
* Dynamically launch an SPI worker.
*/
@@ -358,6 +389,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
BgwHandleStatus status;
pid_t pid;
+ worker_spi_init_shmem();
+
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
BGWORKER_BACKEND_DATABASE_CONNECTION;
--
2.40.1
[application/pgp-signature] signature.asc (833B, ../../ZLiwP5vrT%[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 02:15 Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 03:55 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-20 08:54 ` Masahiro Ikeda <[email protected]>
2023-07-20 09:29 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Masahiro Ikeda @ 2023-07-20 08:54 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]
On 2023-07-20 12:55, Michael Paquier wrote:
> On Thu, Jul 20, 2023 at 11:15:51AM +0900, Masahiro Ikeda wrote:
>> While I'm working on the thread[1], I found that the function of
>> worker_spi module fails if 'shared_preload_libraries' doesn't have
>> worker_spi.
>
> I guess that you were patching worker_spi to register dynamically a
> wait event and embed that in a TAP test or similar without loading it
> in shared_preload_libraries? FWIW, you could use a trick like what I
> am attaching here to load a wait event dynamically with the custom
> wait event API. You would need to make worker_spi_init_shmem() a bit
> more aggressive with an extra hook to reserve a shmem area size, but
> that's enough to show the custom wait event in the same backend as the
> one that launches a worker_spi dynamically, while demonstrating how
> the API can be used in this case.
Yes, you're right. When I tried using worker_spi to test wait event,
I found the behavior. And thanks a lot for your patch. I wasn't aware
of the way. I'll merge your patch to the tests for wait events.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 02:15 Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 03:55 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 08:54 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
@ 2023-07-20 09:29 ` Michael Paquier <[email protected]>
2023-07-20 09:39 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2023-07-20 09:29 UTC (permalink / raw)
To: Masahiro Ikeda <[email protected]>; +Cc: [email protected]
On Thu, Jul 20, 2023 at 05:54:55PM +0900, Masahiro Ikeda wrote:
> Yes, you're right. When I tried using worker_spi to test wait event,
> I found the behavior. And thanks a lot for your patch. I wasn't aware
> of the way. I'll merge your patch to the tests for wait events.
Be careful when using that. I have not spent more than a few minutes
to show my point, but what I sent lacks a shmem_request_hook in
_PG_init(), for example, to request an amount of shared memory equal
to the size of the state structure.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Support worker_spi to execute the function dynamically.
2023-07-20 02:15 Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 03:55 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 08:54 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 09:29 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
@ 2023-07-20 09:39 ` Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Bharath Rupireddy @ 2023-07-20 09:39 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Masahiro Ikeda <[email protected]>; [email protected]
On Thu, Jul 20, 2023 at 2:59 PM Michael Paquier <[email protected]> wrote:
>
> On Thu, Jul 20, 2023 at 05:54:55PM +0900, Masahiro Ikeda wrote:
> > Yes, you're right. When I tried using worker_spi to test wait event,
> > I found the behavior. And thanks a lot for your patch. I wasn't aware
> > of the way. I'll merge your patch to the tests for wait events.
>
> Be careful when using that. I have not spent more than a few minutes
> to show my point, but what I sent lacks a shmem_request_hook in
> _PG_init(), for example, to request an amount of shared memory equal
> to the size of the state structure.
I think the preferred way to grab a chunk of shared memory for an
external module is by using shmem_request_hook and shmem_startup_hook.
Wait events shared memory too can use them.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-07-20 09:39 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-07-09 04:35 [PATCH v4 1/1] move session auth permission check Nathan Bossart <[email protected]>
2023-07-20 02:15 Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 03:55 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 08:54 ` Re: Support worker_spi to execute the function dynamically. Masahiro Ikeda <[email protected]>
2023-07-20 09:29 ` Re: Support worker_spi to execute the function dynamically. Michael Paquier <[email protected]>
2023-07-20 09:39 ` Re: Support worker_spi to execute the function dynamically. Bharath Rupireddy <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox