[BUG] autovacuum may skip tables when session_authorization/role is set on database
Hi,
A recent case in the field in which a database session_authorization is
altered to a non-superuser, non-owner of tables via alter database .. set session_authorization ..
caused autovacuum to skip tables.
The issue was discovered on 13.10, and the logs show such messages:
warning: skipping "table1" --- only table or database owner can vacuum it
In HEAD, I can repro, but the message is now a bit different due to [1]/messages/by-id/20220726.104712.912995710251150228.horikyota.ntt@gmail.com.
WARNING: permission denied to vacuum "table1”, skipping it
It seems to me we should force an autovacuum worker to set the session userid to
a superuser.
Attached is a repro and a patch which sets the session user to the BOOTSTRAP superuser
at the start of the autovac worker.
Thoughts?
Regards,
Sami
Amazon Web Services (AWS)
[1]: /messages/by-id/20220726.104712.912995710251150228.horikyota.ntt@gmail.com
Attachments:
0001-v1-Force-autovacuum-to-use-bootstrap-superuser.patchapplication/octet-stream; name=0001-v1-Force-autovacuum-to-use-bootstrap-superuser.patchDownload
From d9702ce17f2f78d4c5139a241a69c432823f8233 Mon Sep 17 00:00:00 2001
From: EC2 Default User <ec2-user@ip-172-31-22-197.ec2.internal>
Date: Wed, 13 Dec 2023 19:20:12 +0000
Subject: [PATCH 1/1] Force autovacuum to use bootstrap superuser
---
src/backend/postmaster/autovacuum.c | 7 +++++++
src/backend/utils/init/miscinit.c | 2 +-
src/include/miscadmin.h | 1 +
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index b04fcfc8c8..f468661916 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -75,6 +75,7 @@
#include "access/xact.h"
#include "catalog/dependency.h"
#include "catalog/namespace.h"
+#include "catalog/pg_authid.h"
#include "catalog/pg_database.h"
#include "commands/dbcommands.h"
#include "commands/vacuum.h"
@@ -2025,6 +2026,12 @@ do_autovacuum(void)
ALLOCSET_DEFAULT_SIZES);
MemoryContextSwitchTo(AutovacMemCxt);
+ /*
+ * Autovacuum must be run as superuser, so set the session userid to the
+ * bootstrap superuser id.
+ */
+ SetSessionUserId(BOOTSTRAP_SUPERUSERID, true);
+
/* Start a transaction so our commands have one to play into. */
StartTransactionCommand();
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 819936ec02..10bc2add2a 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -546,7 +546,7 @@ GetSessionUserId(void)
}
-static void
+void
SetSessionUserId(Oid userid, bool is_superuser)
{
Assert(SecurityRestrictionContext == 0);
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1043a4d782..4f56c85333 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -354,6 +354,7 @@ extern char *GetUserNameFromId(Oid roleid, bool noerr);
extern Oid GetUserId(void);
extern Oid GetOuterUserId(void);
extern Oid GetSessionUserId(void);
+extern void SetSessionUserId(Oid userid, bool is_superuser);
extern Oid GetAuthenticatedUserId(void);
extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
--
2.40.1
"Imseih (AWS), Sami" <simseih@amazon.com> writes:
A recent case in the field in which a database session_authorization is
altered to a non-superuser, non-owner of tables via alter database .. set session_authorization ..
caused autovacuum to skip tables.
That seems like an extremely not-bright idea. What is the actual
use case for such a setting? Doesn't it risk security problems?
Attached is a repro and a patch which sets the session user to the BOOTSTRAP superuser
at the start of the autovac worker.
I'm rather unimpressed by this proposal, first because there are
probably ten other ways to break autovac with ill-considered settings,
and second because if we do want to consider this a supported case,
what about other background processes? They'd likely have issues
as well.
regards, tom lane
What is the actual
use case for such a setting?
I don't have exact details on the use-case, bit this is not a common
use-case.
Doesn't it risk security problems?
I cannot see how setting it on the database being more problematic than
setting it on a session level.
I'm rather unimpressed by this proposal, first because there are
probably ten other ways to break autovac with ill-considered settings,
There exists code in autovac that safeguard for such settings. For example,
statement_timeout, lock_timeout are disabled. There are a dozen or
more other settings that are overridden for autovac.
I see this being just another one to ensure that autovacuum always runs
as superuser.
and second because if we do want to consider this a supported case,
what about other background processes? They'd likely have issues
as well.
I have not considered other background processes, but autovac is the only
one that I can think of which checks for relation permissions.
Regards,
Sami
On Thu, 14 Dec 2023 at 02:13, Imseih (AWS), Sami <simseih@amazon.com> wrote:
Hi,
A recent case in the field in which a database session_authorization is
altered to a non-superuser, non-owner of tables via alter database .. set session_authorization ..
caused autovacuum to skip tables.
The issue was discovered on 13.10, and the logs show such messages:
warning: skipping "table1" --- only table or database owner can vacuum it
In HEAD, I can repro, but the message is now a bit different due to [1].
WARNING: permission denied to vacuum "table1”, skipping it
It seems to me we should force an autovacuum worker to set the session userid to
a superuser.
Attached is a repro and a patch which sets the session user to the BOOTSTRAP superuser
at the start of the autovac worker.
Since there is not much interest on this patch, I have changed the
status with "Returned with Feedback". Feel free to propose a stronger
use case for the patch and add an entry for the same.
Regards,
Vignesh