Hi,
On 10/5/23 7:10 AM, Michael Paquier wrote:
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.
Thanks!
+# 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.
Oh, thanks, did not know about $node->wait_for_log, good to know!
-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..
Ok, switched back to uint32 in v6-0002 attached (v6-0001 is your v5-0001
unchanged).
@@ -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.
Yeah good point, will work on it once the current one is committed.
-#define BGWORKER_BYPASS_ALLOWCONN 1
+#define BGWORKER_BYPASS_ALLOWCONN 0x0001
Can be a change of its own as well.
Yeah, but I think it's simple enough to just keep this change here
(and I don't think it's "really" needed without introducing 0x0002)
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.
Good idea! I looked at 0001 and it looks ok to me.
0002 is your own patch, with the tests simplified a bit.
Thanks, LGTM.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 5ebe8aa15f22851ae7771137f58366ea9aa21087 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v6 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().
---
doc/src/sgml/bgworker.sgml | 3 +-
src/backend/bootstrap/bootstrap.c | 2 +-
src/backend/postmaster/autovacuum.c | 5 +--
src/backend/postmaster/postmaster.c | 2 +
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 | 6 ++-
.../modules/worker_spi/t/001_worker_spi.pl | 37 +++++++++++++++++++
src/test/modules/worker_spi/worker_spi.c | 2 +
11 files changed, 59 insertions(+), 12 deletions(-)
9.2% doc/src/sgml/
10.7% src/backend/postmaster/
15.1% src/backend/utils/init/
9.5% src/include/postmaster/
47.4% src/test/modules/worker_spi/t/
3.1% src/test/modules/worker_spi/
4.8% 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..1e16643dd7 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -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 */
@@ -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..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/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),
--
2.34.1
From 2f9a209ec9fc5d6514bf1e05ac6cf17af5a0e8ea Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 5 Oct 2023 13:02:14 +0900
Subject: [PATCH v6 1/2] worker_spi: Add tests for BGWORKER_BYPASS_ALLOWCONN
---
.../modules/worker_spi/t/001_worker_spi.pl | 25 +++++++++++++++++++
1 file changed, 25 insertions(+)
100.0% src/test/modules/worker_spi/t/
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.34.1
