Hi,
On 9/29/23 8:19 AM, Michael Paquier wrote:
On Thu, Sep 28, 2023 at 02:37:02PM +0200, Drouvot, Bertrand wrote:
This patch allows the role provided in BackgroundWorkerInitializeConnection()
and BackgroundWorkerInitializeConnectionByOid() to lack login authorization.
Interesting. Yes, there would be use cases for that, I suppose.
Thanks for looking at it!
+ uint32 flags,
char *out_dbname)
{
This may be more adapted with a bits32 for the flags.
Done that way in v2 attached.
+# Ask the background workers to connect with this role with the flag in place.
+$node->append_conf(
+ 'postgresql.conf', q{
+worker_spi.role = 'nologrole'
+worker_spi.bypass_login_check = true
+});
+$node->restart;
+
+# An error message should not be issued.
+ok( !$node->log_contains(
+ "role \"nologrole\" is not permitted to log in", $log_start),
+ "nologrole allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is set");
+
done_testing();
It would be cheaper to use a dynamic background worker for such tests.
Something that I've been tempted to do in this module is to extend the
amount of data that's given to bgw_main_arg when launching a worker
with worker_spi_launch(). How about extending the SQL function so as
it is possible to give in input a role name (or a regrole), a database
name (or a database OID) and a text[] for the flags? This would
require a bit more refactoring, but this would be benefitial to show
or one can pass down a full structure from the registration to the
main() routine. On top of that, it would make the addition of the new
GUCs worker_spi.bypass_login_check and worker_spi.role unnecessary.
I think that would make sense to have more flexibility in the worker_spi
module. I think that could be done in a dedicated patch though. I think it makes
more sense to have the current patch "focusing" on this new flag (while adding
a test
about it without too much refactoring). What about doing the worker_spi module
re-factoring as a follow up of this one?
+# return the size of logfile of $node in bytes
+sub get_log_size
+{
+ my ($node) = @_;
+
+ return (stat $node->logfile)[7];
+}
Just use -s here.
Done in v2 attached.
See other tests that want to check the contents of
the logs from an offset.
Oh right, worth to modify 019_replslot_limit.pl, 002_corrupted.pl and
001_pg_controldata.pl in a separate patch for consistency? (they are using
"(stat $node->logfile)[7]" or "(stat($pg_control))[7]").
- * 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
The structure of the patch is inconsistent. These flags are in
bgworker.h, but they are used also by InitPostgres(). Perhaps a
second boolean flag would be OK rather than a second set of flags for
InitPostgres() mapping with the bgworker set.
I did not want initially to add an extra parameter to InitPostgres() but
I agree it would make more sense. So done that way in v2.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 59de41f3c305b009babfa6d05c054d8ae9e4d730 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Wed, 20 Sep 2023 08:28:59 +0000
Subject: [PATCH v2] 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().
---
doc/src/sgml/bgworker.sgml | 3 +-
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 ++-
src/include/miscadmin.h | 4 +-
src/include/postmaster/bgworker.h | 10 +++--
.../modules/worker_spi/t/001_worker_spi.pl | 40 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 25 +++++++++++-
11 files changed, 88 insertions(+), 17 deletions(-)
7.6% doc/src/sgml/
11.4% src/backend/postmaster/
12.6% src/backend/utils/init/
11.8% src/include/postmaster/
29.2% src/test/modules/worker_spi/t/
23.2% src/test/modules/worker_spi/
4.0% src/
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>
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/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/test/modules/worker_spi/t/001_worker_spi.pl
b/src/test/modules/worker_spi/t/001_worker_spi.pl
index 2965acd789..dcf37606a4 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -93,4 +93,44 @@ ok( $node->poll_query_until(
'dynamic bgworkers all launched'
) or die "Timed out while waiting for dynamic bgworkers to be launched";
+# Check BGWORKER_BYPASS_ROLELOGINCHECK behaves as expected.
+
+# First create a role without login.
+$node->safe_psql(
+ 'postgres', qq[
+ CREATE ROLE nologrole with nologin;
+ ALTER ROLE nologrole with superuser;
+]);
+
+my $log_start = -s $node->logfile;
+
+# Ask the background workers to connect with this role.
+$node->append_conf(
+ 'postgresql.conf', q{
+worker_spi.role = 'nologrole'
+});
+$node->restart;
+
+# An error message should be issued.
+ok( $node->log_contains(
+ "role \"nologrole\" is not permitted to log in", $log_start),
+ "nologrole not allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is
not set"
+);
+
+
+$log_start = -s $node->logfile;
+
+# Ask the background workers to connect with this role with the flag in place.
+$node->append_conf(
+ 'postgresql.conf', q{
+worker_spi.role = 'nologrole'
+worker_spi.bypass_login_check = true
+});
+$node->restart;
+
+# An error message should not be issued.
+ok( !$node->log_contains(
+ "role \"nologrole\" is not permitted to log in", $log_start),
+ "nologrole allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is
set");
+
done_testing();
diff --git a/src/test/modules/worker_spi/worker_spi.c
b/src/test/modules/worker_spi/worker_spi.c
index 98f8d4194b..d2ca99f2c5 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -52,6 +52,8 @@ PGDLLEXPORT void worker_spi_main(Datum main_arg)
pg_attribute_noreturn();
static int worker_spi_naptime = 10;
static int worker_spi_total_workers = 2;
static char *worker_spi_database = NULL;
+static char *worker_spi_role = NULL;
+static bool worker_spi_bypass_login_check = false;
/* value cached, fetched from shared memory */
static uint32 worker_spi_wait_event_main = 0;
@@ -152,7 +154,10 @@ worker_spi_main(Datum main_arg)
BackgroundWorkerUnblockSignals();
/* Connect to our database */
- BackgroundWorkerInitializeConnection(worker_spi_database, NULL, 0);
+ if (worker_spi_bypass_login_check)
+ BackgroundWorkerInitializeConnection(worker_spi_database,
worker_spi_role, BGWORKER_BYPASS_ROLELOGINCHECK);
+ else
+ BackgroundWorkerInitializeConnection(worker_spi_database,
worker_spi_role, 0);
elog(LOG, "%s initialized with %s.%s",
MyBgworkerEntry->bgw_name, table->schema, table->name);
@@ -316,6 +321,24 @@ _PG_init(void)
0,
NULL, NULL, NULL);
+ DefineCustomStringVariable("worker_spi.role",
+ "Role to connect
with.",
+ NULL,
+ &worker_spi_role,
+ NULL,
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
+
+ DefineCustomBoolVariable("worker_spi.bypass_login_check",
+ "Should BGW allows to
connect with non login role.",
+ NULL,
+
&worker_spi_bypass_login_check,
+ false,
+ PGC_SIGHUP,
+ 0,
+ NULL, NULL, NULL);
+
if (!process_shared_preload_libraries_in_progress)
return;
--
2.34.1
