On Wed, Oct 04, 2023 at 12:54:24PM +0200, Drouvot, Bertrand wrote: > Except the Nit that I mentioned in 0001, that looks all good to me (with the > new wait in 001_worker_spi.pl).
Thanks, I've applied the refactoring, including in it the stuff to be able to control the flags used when launching a dynamic worker. 0001 also needed some indenting. You'll notice that the diffs in worker_spi are minimal now. worker_spi_main is no more, as an effect of.. Cough.. c8e318b1b. +# An error message should be issued. +my $nb_errors = 0; +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) +{ + if ($node->log_contains( + "role \"nologrole\" is not permitted to log in", $log_start)) + { + $nb_errors = 1; + last; + } + usleep(100_000); +} This can be switched to use $node->wait_for_log, making the test simpler. No need for Time::HiRes, either. -extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags); +extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username, bits32 flags); [...] -BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags) +BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, bits32 flags) That's changing signatures just for the sake of breaking them. I would leave that alone, I guess.. @@ -719,6 +719,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, Oid useroid, bool load_session_libraries, bool override_allow_connections, + bool bypass_login_check, char *out_dbname) I was not paying much attention here, but load_session_libraries gives a good argument in favor of switching all these booleans to a single bits32 argument as that would make three of them now, with a different set of flags than the bgworker ones. This can be refactored on its own. -#define BGWORKER_BYPASS_ALLOWCONN 1 +#define BGWORKER_BYPASS_ALLOWCONN 0x0001 Can be a change of its own as well. While looking at the refactoring at worker_spi. I've noticed that it would be simple and cheap to have some coverage for BYPASS_ALLOWCONN, something we've never done since this has been introduced. Let me suggest 0001 to add some coverage. 0002 is your own patch, with the tests simplified a bit. -- Michael
From fe8373992d2e2083d53e75ecd954ba2b71e454a1 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 5 Oct 2023 13:02:14 +0900 Subject: [PATCH v5 1/2] worker_spi: Add tests for BGWORKER_BYPASS_ALLOWCONN --- .../modules/worker_spi/t/001_worker_spi.pl | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl index 4b46b1336b..7e5a4b1402 100644 --- a/src/test/modules/worker_spi/t/001_worker_spi.pl +++ b/src/test/modules/worker_spi/t/001_worker_spi.pl @@ -104,4 +104,29 @@ postgres|myrole|WorkerSpiMain]), 'dynamic bgworkers all launched' ) or die "Timed out while waiting for dynamic bgworkers to be launched"; +# Check BGWORKER_BYPASS_ALLOWCONN. +$node->safe_psql('postgres', q(ALTER DATABASE mydb ALLOW_CONNECTIONS false;)); +my $log_offset = -s $node->logfile; + +# bgworker cannot be launched with connection restriction. +my $worker3_pid = $node->safe_psql('postgres', + qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id);]); +$node->wait_for_log( + qr/database "mydb" is not currently accepting connections/, $log_offset); + +$result = $node->safe_psql('postgres', + "SELECT count(*) FROM pg_stat_activity WHERE pid = $worker3_pid;"); +is($result, '0', 'dynamic bgworker without BYPASS_ALLOWCONN not started'); + +# bgworker bypasses the connection check, and can be launched. +my $worker4_pid = $node->safe_psql('postgres', + qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id, '{"ALLOWCONN"}');]); +ok( $node->poll_query_until( + 'postgres', + qq[SELECT datname, usename, wait_event FROM pg_stat_activity + WHERE backend_type = 'worker_spi dynamic' AND + pid IN ($worker4_pid) ORDER BY datname;], + qq[mydb|myrole|WorkerSpiMain]), + 'dynamic bgworker with BYPASS_ALLOWCONN started'); + done_testing(); -- 2.42.0
From 59b1e8827c20a3e801e64b3b089efb3d06d71f42 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 5 Oct 2023 14:06:37 +0900 Subject: [PATCH v5 2/2] Allow background workers to bypass login check This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags being used in BackgroundWorkerInitializeConnection() and BackgroundWorkerInitializeConnectionByOid(). This flag allows the background workers to bypass the login check done in InitializeSessionUserId(). --- src/include/miscadmin.h | 4 +- src/include/postmaster/bgworker.h | 10 +++-- src/backend/bootstrap/bootstrap.c | 2 +- src/backend/postmaster/autovacuum.c | 5 +-- src/backend/postmaster/postmaster.c | 6 ++- src/backend/tcop/postgres.c | 1 + src/backend/utils/init/miscinit.c | 4 +- src/backend/utils/init/postinit.c | 5 ++- .../modules/worker_spi/t/001_worker_spi.pl | 37 +++++++++++++++++++ src/test/modules/worker_spi/worker_spi.c | 2 + doc/src/sgml/bgworker.sgml | 3 +- 11 files changed, 63 insertions(+), 16 deletions(-) diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 14bd574fc2..f1d03f2df8 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -364,7 +364,8 @@ extern bool InSecurityRestrictedOperation(void); extern bool InNoForceRLSOperation(void); extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context); extern void SetUserIdAndContext(Oid userid, bool sec_def_context); -extern void InitializeSessionUserId(const char *rolename, Oid roleid); +extern void InitializeSessionUserId(const char *rolename, Oid roleid, + bool bypass_login_check); extern void InitializeSessionUserIdStandalone(void); extern void SetSessionAuthorization(Oid userid, bool is_superuser); extern Oid GetCurrentRoleId(void); @@ -469,6 +470,7 @@ extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username, Oid useroid, bool load_session_libraries, bool override_allow_connections, + bool bypass_login_check, char *out_dbname); extern void BaseInit(void); diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h index 7815507e3d..0fdbfaf431 100644 --- a/src/include/postmaster/bgworker.h +++ b/src/include/postmaster/bgworker.h @@ -141,18 +141,20 @@ extern PGDLLIMPORT BackgroundWorker *MyBgworkerEntry; * If dbname is NULL, connection is made to no specific database; * only shared catalogs can be accessed. */ -extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags); +extern void BackgroundWorkerInitializeConnection(const char *dbname, const char *username, bits32 flags); /* Just like the above, but specifying database and user by OID. */ -extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags); +extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, bits32 flags); /* * Flags to BackgroundWorkerInitializeConnection et al * * - * Allow bypassing datallowconn restrictions when connecting to database + * Allow bypassing datallowconn restrictions and login check when connecting + * to database */ -#define BGWORKER_BYPASS_ALLOWCONN 1 +#define BGWORKER_BYPASS_ALLOWCONN 0x0001 +#define BGWORKER_BYPASS_ROLELOGINCHECK 0x0002 /* Block/unblock signals in a background worker process */ diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 5810f8825e..bdd005b66a 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only) if (pg_link_canary_is_frontend()) elog(ERROR, "backend is incorrectly linked to frontend functions"); - InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL); + InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, NULL); /* Initialize stuff for bootstrap-file processing */ for (i = 0; i < MAXATTR; i++) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index ae9be9b911..4686b81f68 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[]) /* Early initialization */ BaseInit(); - InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL); + InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, NULL); SetProcessingMode(NormalProcessing); @@ -1706,8 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[]) * Note: if we have selected a just-deleted database (due to using * stale stats info), we'll fail and exit here. */ - InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, - dbname); + InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, false, dbname); SetProcessingMode(NormalProcessing); set_ps_display(dbname); ereport(DEBUG1, diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 54e9bfb8c4..37f5b94469 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -5575,7 +5575,7 @@ MaxLivePostmasterChildren(void) * Connect background worker to a database. */ void -BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags) +BackgroundWorkerInitializeConnection(const char *dbname, const char *username, bits32 flags) { BackgroundWorker *worker = MyBgworkerEntry; @@ -5589,6 +5589,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u username, InvalidOid, /* role to connect as */ false, /* never honor session_preload_libraries */ (flags & BGWORKER_BYPASS_ALLOWCONN) != 0, /* ignore datallowconn? */ + (flags & BGWORKER_BYPASS_ROLELOGINCHECK) != 0, /* bypass login check? */ NULL); /* no out_dbname */ /* it had better not gotten out of "init" mode yet */ @@ -5602,7 +5603,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u * Connect background worker to a database using OIDs. */ void -BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags) +BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, bits32 flags) { BackgroundWorker *worker = MyBgworkerEntry; @@ -5616,6 +5617,7 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags) NULL, useroid, /* role to connect as */ false, /* never honor session_preload_libraries */ (flags & BGWORKER_BYPASS_ALLOWCONN) != 0, /* ignore datallowconn? */ + (flags & BGWORKER_BYPASS_ROLELOGINCHECK) != 0, /* bypass login check? */ NULL); /* no out_dbname */ /* it had better not gotten out of "init" mode yet */ diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 21b9763183..5c1e21b99d 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4211,6 +4211,7 @@ PostgresMain(const char *dbname, const char *username) username, InvalidOid, /* role to connect as */ !am_walsender, /* honor session_preload_libraries? */ false, /* don't ignore datallowconn */ + false, /* don't bypass login check */ NULL); /* no out_dbname */ /* diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 1e671c560c..182d666852 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -725,7 +725,7 @@ has_rolreplication(Oid roleid) * Initialize user identity during normal backend startup */ void -InitializeSessionUserId(const char *rolename, Oid roleid) +InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_check) { HeapTuple roleTup; Form_pg_authid rform; @@ -789,7 +789,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid) /* * Is role allowed to login at all? */ - if (!rform->rolcanlogin) + if (!bypass_login_check && !rform->rolcanlogin) ereport(FATAL, (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), errmsg("role \"%s\" is not permitted to log in", diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index df4d15a50f..dcfeb30832 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -719,6 +719,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, Oid useroid, bool load_session_libraries, bool override_allow_connections, + bool bypass_login_check, char *out_dbname) { bool bootstrap = IsBootstrapProcessingMode(); @@ -901,7 +902,7 @@ InitPostgres(const char *in_dbname, Oid dboid, } else { - InitializeSessionUserId(username, useroid); + InitializeSessionUserId(username, useroid, bypass_login_check); am_superuser = superuser(); } } @@ -910,7 +911,7 @@ InitPostgres(const char *in_dbname, Oid dboid, /* normal multiuser case */ Assert(MyProcPort != NULL); PerformAuthentication(MyProcPort); - InitializeSessionUserId(username, useroid); + InitializeSessionUserId(username, useroid, false); /* ensure that auth_method is actually valid, aka authn_id is not NULL */ if (MyClientConnectionInfo.authn_id) InitializeSystemUser(MyClientConnectionInfo.authn_id, diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl index 7e5a4b1402..a8169c175b 100644 --- a/src/test/modules/worker_spi/t/001_worker_spi.pl +++ b/src/test/modules/worker_spi/t/001_worker_spi.pl @@ -128,5 +128,42 @@ ok( $node->poll_query_until( pid IN ($worker4_pid) ORDER BY datname;], qq[mydb|myrole|WorkerSpiMain]), 'dynamic bgworker with BYPASS_ALLOWCONN started'); +$node->safe_psql('postgres', q(ALTER DATABASE mydb ALLOW_CONNECTIONS true;)); + +# Check BGWORKER_BYPASS_ROLELOGINCHECK. +# First create a role without login access. +$node->safe_psql( + 'postgres', qq[ + CREATE ROLE nologrole with nologin; + ALTER ROLE nologrole with superuser; +]); +my $nologrole_id = $node->safe_psql('mydb', + "SELECT oid FROM pg_roles where rolname = 'nologrole';"); +$log_offset = -s $node->logfile; + +# bgworker cannot be launched with login restriction. +my $worker5_pid = $node->safe_psql('postgres', + qq[SELECT worker_spi_launch(13, $mydb_id, $nologrole_id);]); +$node->wait_for_log(qr/role "nologrole" is not permitted to log in/, + $log_offset); + +$result = $node->safe_psql('postgres', + "SELECT count(*) FROM pg_stat_activity WHERE pid = $worker5_pid;"); +is($result, '0', + 'dynamic bgworker without BYPASS_ROLELOGINCHECK not started'); + +# bgworker bypasses the login restriction, and can be launched. +$log_offset = -s $node->logfile; +my $worker6_pid = $node->safe_psql('mydb', + qq(SELECT worker_spi_launch(13, $mydb_id, $nologrole_id, '{"ROLELOGINCHECK"}');) +); +ok( $node->poll_query_until( + 'mydb', + qq[SELECT datname, usename, wait_event FROM pg_stat_activity + WHERE backend_type = 'worker_spi dynamic' AND + pid = $worker6_pid;], + 'mydb|nologrole|WorkerSpiMain'), + 'dynamic bgworker with BYPASS_ROLELOGINCHECK launched' +) or die "Timed out while waiting for dynamic bgworkers to be launched"; done_testing(); diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index 5d81cf4563..af7256bd89 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -443,6 +443,8 @@ worker_spi_launch(PG_FUNCTION_ARGS) if (strcmp(optname, "ALLOWCONN") == 0) flags |= BGWORKER_BYPASS_ALLOWCONN; + else if (strcmp(optname, "ROLELOGINCHECK") == 0) + flags |= BGWORKER_BYPASS_ROLELOGINCHECK; else ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml index 9ad1146ba0..7335c02ed8 100644 --- a/doc/src/sgml/bgworker.sgml +++ b/doc/src/sgml/bgworker.sgml @@ -200,7 +200,8 @@ typedef struct BackgroundWorker <literal>InvalidOid</literal>, the process will run as the superuser created during <command>initdb</command>. If <literal>BGWORKER_BYPASS_ALLOWCONN</literal> is specified as <varname>flags</varname> it is possible to bypass the restriction - to connect to databases not allowing user connections. + to connect to databases not allowing user connections. If <literal>BGWORKER_BYPASS_ROLELOGINCHECK</literal> + is specified as <varname>flags</varname> it is possible to bypass the login check to connect to databases. A background worker can only call one of these two functions, and only once. It is not possible to switch databases. </para> -- 2.42.0
signature.asc
Description: PGP signature