On Tue, Oct 10, 2023 at 06:57:05AM +0200, Drouvot, Bertrand wrote: > Please find attached v9 (v8 rebase due to f483b2090).
I was looking at v8 just before you sent this v9, and still got annoyed by the extra boolean argument added to InitPostgres(). So please let me propose to bite the bullet and refactor that, as of the 0001 attached that means less diff footprints in all the callers of InitPostgres() (I am not wedded to the flag names). It looks like 0002 had the same issues as f483b209: the worker that could not be started because of the login restriction could be detected as stopped by worker_spi_launch(), causing the script to fail hard. 0002 is basically your v9, able to work with the refactoring from 0001. -- Michael
From 6e094435789a0255ddd4961bd43cfae4bf54e135 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 10 Oct 2023 14:41:56 +0900
Subject: [PATCH v10 1/2] Refactor InitPostgres() with flags
---
src/include/miscadmin.h | 6 ++++--
src/backend/bootstrap/bootstrap.c | 2 +-
src/backend/postmaster/autovacuum.c | 5 ++---
src/backend/postmaster/postmaster.c | 16 ++++++++++++----
src/backend/tcop/postgres.c | 5 +++--
src/backend/utils/init/postinit.c | 17 +++++++++--------
6 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc2..1cc3712c0f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -463,12 +463,14 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType;
*****************************************************************************/
/* in utils/init/postinit.c */
+/* flags for InitPostgres() */
+#define INIT_PG_LOAD_SESSION_LIBS 0x0001
+#define INIT_PG_OVERRIDE_ALLOW_CONNS 0x0002
extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
extern void InitializeMaxBackends(void);
extern void InitPostgres(const char *in_dbname, Oid dboid,
const char *username, Oid useroid,
- bool load_session_libraries,
- bool override_allow_connections,
+ bits32 flags,
char *out_dbname);
extern void BaseInit(void);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..e01dca9b7c 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, 0, 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..327ea0d45a 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, 0, 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, 0, dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname);
ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0761b38bf8..3d7fec995a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5562,6 +5562,11 @@ void
BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
{
BackgroundWorker *worker = MyBgworkerEntry;
+ bits32 init_flags = 0; /* never honor session_preload_libraries */
+
+ /* ignore datallowconn? */
+ if (flags & BGWORKER_BYPASS_ALLOWCONN)
+ init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5571,8 +5576,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
InitPostgres(dbname, InvalidOid, /* database to connect to */
username, InvalidOid, /* role to connect as */
- false, /* never honor session_preload_libraries */
- (flags & BGWORKER_BYPASS_ALLOWCONN) != 0, /* ignore datallowconn? */
+ init_flags,
NULL); /* no out_dbname */
/* it had better not gotten out of "init" mode yet */
@@ -5589,6 +5593,11 @@ void
BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
{
BackgroundWorker *worker = MyBgworkerEntry;
+ bits32 init_flags = 0; /* never honor session_preload_libraries */
+
+ /* ignore datallowconn? */
+ if (flags & BGWORKER_BYPASS_ALLOWCONN)
+ init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5598,8 +5607,7 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
InitPostgres(NULL, dboid, /* database to connect to */
NULL, useroid, /* role to connect as */
- false, /* never honor session_preload_libraries */
- (flags & BGWORKER_BYPASS_ALLOWCONN) != 0, /* ignore datallowconn? */
+ init_flags,
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..f3c9f1f9ba 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4206,11 +4206,12 @@ PostgresMain(const char *dbname, const char *username)
* NOTE: if you are tempted to add code in this vicinity, consider putting
* it inside InitPostgres() instead. In particular, anything that
* involves database access should be there, not here.
+ *
+ * Honor session_preload_libraries if not dealing with a WAL sender.
*/
InitPostgres(dbname, InvalidOid, /* database to connect to */
username, InvalidOid, /* role to connect as */
- !am_walsender, /* honor session_preload_libraries? */
- false, /* don't ignore datallowconn */
+ (!am_walsender) ? INIT_PG_LOAD_SESSION_LIBS : 0,
NULL); /* no out_dbname */
/*
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index df4d15a50f..449541e942 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -681,8 +681,9 @@ BaseInit(void)
* Parameters:
* in_dbname, dboid: specify database to connect to, as described below
* username, useroid: specify role to connect as, as described below
- * load_session_libraries: TRUE to honor [session|local]_preload_libraries
- * override_allow_connections: TRUE to connect despite !datallowconn
+ * flags:
+ * - INIT_PG_LOAD_SESSION_LIBS to honor [session|local]_preload_libraries.
+ * - INIT_PG_OVERRIDE_ALLOW_CONNS to connect despite !datallowconn.
* out_dbname: optional output parameter, see below; pass NULL if not used
*
* The database can be specified by name, using the in_dbname parameter, or by
@@ -701,8 +702,8 @@ BaseInit(void)
* database but not a username; conversely, a physical walsender specifies
* username but not database.
*
- * By convention, load_session_libraries should be passed as true in
- * "interactive" sessions (including standalone backends), but false in
+ * By convention, INIT_PG_LOAD_SESSION_LIBS should be passed in "flags" in
+ * "interactive" sessions (including standalone backends), but not in
* background processes such as autovacuum. Note in particular that it
* shouldn't be true in parallel worker processes; those have another
* mechanism for replicating their leader's set of loaded libraries.
@@ -717,8 +718,7 @@ BaseInit(void)
void
InitPostgres(const char *in_dbname, Oid dboid,
const char *username, Oid useroid,
- bool load_session_libraries,
- bool override_allow_connections,
+ bits32 flags,
char *out_dbname)
{
bool bootstrap = IsBootstrapProcessingMode();
@@ -1189,7 +1189,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
* user is a superuser, so the above stuff has to happen first.)
*/
if (!bootstrap)
- CheckMyDatabase(dbname, am_superuser, override_allow_connections);
+ CheckMyDatabase(dbname, am_superuser,
+ (flags & INIT_PG_OVERRIDE_ALLOW_CONNS) != 0);
/*
* Now process any command-line switches and any additional GUC variable
@@ -1227,7 +1228,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
* during the initial transaction in case anything that requires database
* access needs to be done.
*/
- if (load_session_libraries)
+ if ((flags & INIT_PG_LOAD_SESSION_LIBS) != 0)
process_session_preload_libraries();
/* report this backend in the PgBackendStatus array */
--
2.42.0
From ac35173040a30dcadb30bfcd13009ddf49d08b25 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 10 Oct 2023 14:56:36 +0900
Subject: [PATCH v10 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 | 6 ++--
src/backend/postmaster/postmaster.c | 6 ++++
src/backend/utils/init/miscinit.c | 4 +--
src/backend/utils/init/postinit.c | 6 ++--
.../modules/worker_spi/t/001_worker_spi.pl | 31 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 2 ++
doc/src/sgml/bgworker.sgml | 4 ++-
8 files changed, 55 insertions(+), 8 deletions(-)
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1cc3712c0f..e280b8192d 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);
@@ -466,6 +467,7 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType;
/* flags for InitPostgres() */
#define INIT_PG_LOAD_SESSION_LIBS 0x0001
#define INIT_PG_OVERRIDE_ALLOW_CONNS 0x0002
+#define INIT_PG_BYPASS_ROLE_LOGIN 0x0004
extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
extern void InitializeMaxBackends(void);
extern void InitPostgres(const char *in_dbname, Oid dboid,
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 7815507e3d..d7a5c1a946 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -150,9 +150,11 @@ extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, ui
* 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/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 3d7fec995a..b1cabbd0d6 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5567,6 +5567,9 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
/* ignore datallowconn? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+ /* bypass login check? */
+ if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+ init_flags |= INIT_PG_BYPASS_ROLE_LOGIN;
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5598,6 +5601,9 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
/* ignore datallowconn? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+ /* bypass login check? */
+ if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+ init_flags |= INIT_PG_BYPASS_ROLE_LOGIN;
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
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 449541e942..c5b91bd4af 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -684,6 +684,7 @@ BaseInit(void)
* flags:
* - INIT_PG_LOAD_SESSION_LIBS to honor [session|local]_preload_libraries.
* - INIT_PG_OVERRIDE_ALLOW_CONNS to connect despite !datallowconn.
+ * - INIT_PG_BYPASS_ROLE_LOGIN to bypass role login check.
* out_dbname: optional output parameter, see below; pass NULL if not used
*
* The database can be specified by name, using the in_dbname parameter, or by
@@ -901,7 +902,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
}
else
{
- InitializeSessionUserId(username, useroid);
+ InitializeSessionUserId(username, useroid,
+ (flags & INIT_PG_BYPASS_ROLE_LOGIN) != 0);
am_superuser = superuser();
}
}
@@ -910,7 +912,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 178962eddb..b5ac4bcaa2 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -131,4 +131,35 @@ ok( $node->poll_query_until(
qq[noconndb|myrole|WorkerSpiMain]),
'dynamic bgworker with BYPASS_ALLOWCONN started');
+# Check BGWORKER_BYPASS_ROLELOGINCHECK.
+# First create a role without login access.
+$node->safe_psql(
+ 'postgres', qq[
+ CREATE ROLE nologrole WITH NOLOGIN;
+ GRANT CREATE ON DATABASE mydb TO nologrole;
+]);
+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.
+$node->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);
+
+# bgworker bypasses the login restriction, and can be launched.
+$log_offset = -s $node->logfile;
+my $worker5_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 = $worker5_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 1c619d4b18..c26ffe1766 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -452,6 +452,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..8b61cf1c55 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,9 @@ 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 for the
+ role used 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
