Hi,
in today's world, some DBAs have no superuser rights, but we can
delegate them additional powers like CREATEDB or the pg_monitor default
role etc. Usually, the DBA can also view the database logs, either via
shell access or some web interface.
One thing that I personally find lacking is that it is not possible to
change role-specific log settings (like log_statement = 'mod' for a
security sensitive role) without being SUPERUSER as their GUC context is
"superuser". This makes setup auditing much harder if there is no
SUPERUSER access, also pgaudit then only allows to configure object-
based auditing. Amazon RDS e.g. has patched Postgres to allow the
cluster owner/pseudo-superuser `rds_superuser' to change those log
settings that define what/when we log something, while keeping the
"where to log" entries locked down.
The attached patch introduces a new guc context "administrator" (better
names/bikeshedding for this welcome) that is meant as a middle ground
between "superuser" and "user". It also adds a new default role
"pg_change_role_settings" (better names also welcome) that can be
granted to DBAs so that they can change the "administrator"-context GUCs
on a per-role (or per-database) basis. Whether the latter should be
included is maybe debatable, but I added both on the basis that they are
the same "source".
The initial set of "administrator" GUCs are all current GUCs with
"superuser" context from these categories:
* Reporting and Logging / What to Log
* Reporting and Logging / When to
Log
* Statistics / Query and Index Statistics Collector
* Statistics /
Monitoring
Of course, further categories (or particular GUCs) could be added now or
in the future, e.g. RDS also patches the following GUCs in their v12
offering:
* temp_file_limit
* session_replication_role
RDS does *not* patch log_transaction_sample_rate from "Reporting and
Logging / When to Log", but that might be more of an oversight than a
security consideration, or does anybody see a big problem with that
(compared to the others in that set)?
I initially pondered not introducing a new context but just filtering on
category, but as categories seem to be only descriptive in guc.c and not
used for any policy decisions so far, I have abandoned this pretty
quickly.
Thoughts?
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
From 71c1134ca8ea9267be6b605fa140d7d97f54ac6d Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.ba...@credativ.de>
Date: Tue, 15 Dec 2020 20:53:59 +0100
Subject: [PATCH] Add new PGC_ADMINSET guc context and pg_change_role_settings
default role.
This commit introduces `administrator' as a middle ground between `superuser'
and `user' for guc contexts.
Those settings initially include all previously `superuser'-context settings
from the 'Reporting and Logging' ('What/When to Log' but not 'Where to Log')
and both 'Statistics' categories. Further settings could be added in follow-up
commits.
Also, a new default role pg_change_role_settings is introduced. This allows
administrators (that are not superusers) that have been granted this role to
change the above PGC_ADMINSET settings on a per-role/database basis like "ALTER
ROLE [...] SET log_statement".
The rationale is to allow certain settings that affect logging to be changed
in setups where the DBA has access to the database log, but is not a full
superuser.
Catversion is bumped.
---
src/backend/commands/dbcommands.c | 7 +++-
src/backend/commands/user.c | 7 ++--
src/backend/utils/misc/guc.c | 56 +++++++++++++++++++------------
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_authid.dat | 5 +++
src/include/utils/guc.h | 1 +
6 files changed, 53 insertions(+), 25 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index f27c3fe8c1..03787eb5cf 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1657,7 +1657,12 @@ AlterDatabaseSet(AlterDatabaseSetStmt *stmt)
*/
shdepLockAndCheckObject(DatabaseRelationId, datid);
- if (!pg_database_ownercheck(datid, GetUserId()))
+ /*
+ * Only allow the database owner or a member of the
+ * pg_change_role_settings default role to change database settings.
+ */
+ if (!pg_database_ownercheck(datid, GetUserId()) &&
+ !is_member_of_role(GetUserId(), DEFAULT_ROLE_CHANGE_ROLE_SETTINGS))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
stmt->dbname);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 0e6800bf3e..884f15d1c5 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -936,7 +936,8 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
/*
* To mess with a superuser you gotta be superuser; else you need
- * createrole, or just want to change your own settings
+ * createrole, the pg_change_role_settings default role, or just want
+ * to change your own settings.
*/
if (roleform->rolsuper)
{
@@ -947,7 +948,9 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
}
else
{
- if (!have_createrole_privilege() && roleid != GetUserId())
+ if (!have_createrole_privilege() &&
+ roleid != GetUserId() &&
+ !is_member_of_role(GetUserId(), DEFAULT_ROLE_CHANGE_ROLE_SETTINGS))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied")));
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 878fcc2236..875c928c40 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -634,6 +634,7 @@ const char *const GucContext_Names[] =
/* PGC_SU_BACKEND */ "superuser-backend",
/* PGC_BACKEND */ "backend",
/* PGC_SUSET */ "superuser",
+ /* PGC_ADMINSET */ "administrator",
/* PGC_USERSET */ "user"
};
@@ -1317,7 +1318,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_replication_commands", PGC_SUSET, LOGGING_WHAT,
+ {"log_replication_commands", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Logs each replication command."),
NULL
},
@@ -1360,7 +1361,7 @@ static struct config_bool ConfigureNamesBool[] =
},
{
- {"log_duration", PGC_SUSET, LOGGING_WHAT,
+ {"log_duration", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Logs the duration of each completed SQL statement."),
NULL
},
@@ -1405,7 +1406,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_parser_stats", PGC_SUSET, STATS_MONITORING,
+ {"log_parser_stats", PGC_ADMINSET, STATS_MONITORING,
gettext_noop("Writes parser performance statistics to the server log."),
NULL
},
@@ -1414,7 +1415,7 @@ static struct config_bool ConfigureNamesBool[] =
check_stage_log_stats, NULL, NULL
},
{
- {"log_planner_stats", PGC_SUSET, STATS_MONITORING,
+ {"log_planner_stats", PGC_ADMINSET, STATS_MONITORING,
gettext_noop("Writes planner performance statistics to the server log."),
NULL
},
@@ -1423,7 +1424,7 @@ static struct config_bool ConfigureNamesBool[] =
check_stage_log_stats, NULL, NULL
},
{
- {"log_executor_stats", PGC_SUSET, STATS_MONITORING,
+ {"log_executor_stats", PGC_ADMINSET, STATS_MONITORING,
gettext_noop("Writes executor performance statistics to the server log."),
NULL
},
@@ -1432,7 +1433,7 @@ static struct config_bool ConfigureNamesBool[] =
check_stage_log_stats, NULL, NULL
},
{
- {"log_statement_stats", PGC_SUSET, STATS_MONITORING,
+ {"log_statement_stats", PGC_ADMINSET, STATS_MONITORING,
gettext_noop("Writes cumulative performance statistics to the server log."),
NULL
},
@@ -1454,7 +1455,7 @@ static struct config_bool ConfigureNamesBool[] =
#endif
{
- {"track_activities", PGC_SUSET, STATS_COLLECTOR,
+ {"track_activities", PGC_ADMINSET, STATS_COLLECTOR,
gettext_noop("Collects information about executing commands."),
gettext_noop("Enables the collection of information on the currently "
"executing command of each session, along with "
@@ -1465,7 +1466,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"track_counts", PGC_SUSET, STATS_COLLECTOR,
+ {"track_counts", PGC_ADMINSET, STATS_COLLECTOR,
gettext_noop("Collects statistics on database activity."),
NULL
},
@@ -1474,7 +1475,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"track_io_timing", PGC_SUSET, STATS_COLLECTOR,
+ {"track_io_timing", PGC_ADMINSET, STATS_COLLECTOR,
gettext_noop("Collects timing statistics for database I/O activity."),
NULL
},
@@ -1562,7 +1563,7 @@ static struct config_bool ConfigureNamesBool[] =
#endif
{
- {"log_lock_waits", PGC_SUSET, LOGGING_WHAT,
+ {"log_lock_waits", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Logs long lock waits."),
NULL
},
@@ -2823,7 +2824,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"log_min_duration_sample", PGC_SUSET, LOGGING_WHEN,
+ {"log_min_duration_sample", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Sets the minimum execution time above which "
"a sample of statements will be logged."
" Sampling is determined by log_statement_sample_rate."),
@@ -2836,7 +2837,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"log_min_duration_statement", PGC_SUSET, LOGGING_WHEN,
+ {"log_min_duration_statement", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Sets the minimum execution time above which "
"all statements will be logged."),
gettext_noop("Zero prints all queries. -1 turns this feature off."),
@@ -2860,7 +2861,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
+ {"log_parameter_max_length", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
gettext_noop("-1 to print values in full."),
GUC_UNIT_BYTE
@@ -3335,7 +3336,7 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"log_temp_files", PGC_SUSET, LOGGING_WHAT,
+ {"log_temp_files", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Log the use of temporary files larger than this number of kilobytes."),
gettext_noop("Zero logs all files. The default is -1 (turning this feature off)."),
GUC_UNIT_KB
@@ -3648,7 +3649,7 @@ static struct config_real ConfigureNamesReal[] =
},
{
- {"log_statement_sample_rate", PGC_SUSET, LOGGING_WHEN,
+ {"log_statement_sample_rate", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Fraction of statements exceeding log_min_duration_sample to be logged."),
gettext_noop("Use a value between 0.0 (never log) and 1.0 (always log).")
},
@@ -3658,7 +3659,7 @@ static struct config_real ConfigureNamesReal[] =
},
{
- {"log_transaction_sample_rate", PGC_SUSET, LOGGING_WHEN,
+ {"log_transaction_sample_rate", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Set the fraction of transactions to log for new transactions."),
gettext_noop("Logs all statements from a fraction of transactions. "
"Use a value between 0.0 (never log) and 1.0 (log all "
@@ -4523,7 +4524,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"log_error_verbosity", PGC_SUSET, LOGGING_WHAT,
+ {"log_error_verbosity", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Sets the verbosity of logged messages."),
NULL
},
@@ -4533,7 +4534,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"log_min_messages", PGC_SUSET, LOGGING_WHEN,
+ {"log_min_messages", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Sets the message levels that are logged."),
gettext_noop("Each level includes all the levels that follow it. The later"
" the level, the fewer messages are sent.")
@@ -4544,7 +4545,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"log_min_error_statement", PGC_SUSET, LOGGING_WHEN,
+ {"log_min_error_statement", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Causes all statements generating error at or above this level to be logged."),
gettext_noop("Each level includes all the levels that follow it. The later"
" the level, the fewer messages are sent.")
@@ -4555,7 +4556,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"log_statement", PGC_SUSET, LOGGING_WHAT,
+ {"log_statement", PGC_ADMINSET, LOGGING_WHAT,
gettext_noop("Sets the type of statements logged."),
NULL
},
@@ -4636,7 +4637,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"track_functions", PGC_SUSET, STATS_COLLECTOR,
+ {"track_functions", PGC_ADMINSET, STATS_COLLECTOR,
gettext_noop("Collects function-level statistics on database activity."),
NULL
},
@@ -7166,6 +7167,19 @@ set_config_option(const char *name, const char *value,
return 0;
}
break;
+ case PGC_ADMINSET:
+ if ((context == PGC_USERSET &&
+ !((source == PGC_S_DATABASE_USER || source == PGC_S_TEST) &&
+ is_member_of_role(GetUserId(), DEFAULT_ROLE_CHANGE_ROLE_SETTINGS))) ||
+ context == PGC_BACKEND)
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to set parameter \"%s\"",
+ name)));
+ return 0;
+ }
+ break;
case PGC_USERSET:
/* always okay */
break;
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index db4814e57a..3cd7d7df1c 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202012293
+#define CATALOG_VERSION_NO 202012311
#endif
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 7c08851550..f5985fe0c0 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -60,5 +60,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '8801', oid_symbol => 'DEFAULT_ROLE_CHANGE_ROLE_SETTINGS',
+ rolname => 'pg_change_role_settings', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
]
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 6a20a3bcec..f3619b0c25 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -73,6 +73,7 @@ typedef enum
PGC_SU_BACKEND,
PGC_BACKEND,
PGC_SUSET,
+ PGC_ADMINSET,
PGC_USERSET
} GucContext;
--
2.20.1
