Hi,
Am Montag, den 08.03.2021, 20:54 +0500 schrieb Ibrar Ahmed:
> On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael.ba...@credativ.de>
> wrote:
> > 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
>
> The patch (0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_role_.patch)
> does
> not apply successfully and has some hunks failed.
Thanks for letting me know.
> http://cfbot.cputube.org/patch_32_2918.log
> 1 out of 23 hunks FAILED -- saving rejects to file
> src/backend/utils/misc/guc.c.rej
> patching file src/include/catalog/catversion.h
> Hunk #1 FAILED at 53.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/include/catalog/catversion.h.rej
> patching file src/include/catalog/pg_authid.dat
> Can we get a rebase?
Please find attached the rebase; I've removed the catversion hunk as I
believe it is customary to leave that to committers.
> I am marking the patch "Waiting on Author".
I've put that back to "Needs Review".
Cheers,
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 d0eb8091d294ed1092eebcce30a3d67a13bdc84f 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 v2] 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.
---
src/backend/commands/dbcommands.c | 7 +++-
src/backend/commands/user.c | 7 ++--
src/backend/utils/misc/guc.c | 56 +++++++++++++++++++------------
src/include/catalog/pg_authid.dat | 5 +++
src/include/utils/guc.h | 1 +
5 files changed, 52 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 2b159b60eb..a59b1dbeb8 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 ed243e3d14..f2da06cf24 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 3fd1a5fbe2..33457bf26e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -637,6 +637,7 @@ const char *const GucContext_Names[] =
/* PGC_SU_BACKEND */ "superuser-backend",
/* PGC_BACKEND */ "backend",
/* PGC_SUSET */ "superuser",
+ /* PGC_ADMINSET */ "administrator",
/* PGC_USERSET */ "user"
};
@@ -1320,7 +1321,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
},
@@ -1363,7 +1364,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
},
@@ -1408,7 +1409,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
},
@@ -1417,7 +1418,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
},
@@ -1426,7 +1427,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
},
@@ -1435,7 +1436,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
},
@@ -1457,7 +1458,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 "
@@ -1468,7 +1469,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
},
@@ -1477,7 +1478,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
},
@@ -1565,7 +1566,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
},
@@ -2857,7 +2858,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."),
@@ -2870,7 +2871,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."),
@@ -2894,7 +2895,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
@@ -3369,7 +3370,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
@@ -3705,7 +3706,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).")
},
@@ -3715,7 +3716,7 @@ static struct config_real ConfigureNamesReal[] =
},
{
- {"log_transaction_sample_rate", PGC_SUSET, LOGGING_WHEN,
+ {"log_transaction_sample_rate", PGC_ADMINSET, LOGGING_WHEN,
gettext_noop("Sets 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 "
@@ -4590,7 +4591,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
},
@@ -4600,7 +4601,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.")
@@ -4611,7 +4612,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.")
@@ -4622,7 +4623,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
},
@@ -4703,7 +4704,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
},
@@ -7256,6 +7257,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/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 87d917ffc3..b86cd1e4f3 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -64,5 +64,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 5004ee4177..6aa7319e8b 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
