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]> 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 03:55 Michael Paquier <[email protected]> parent: 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 08:54 Masahiro Ikeda <[email protected]> parent: 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 09:29 Michael Paquier <[email protected]> parent: Masahiro Ikeda <[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 09:39 Bharath Rupireddy <[email protected]> parent: Michael Paquier <[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