Re: [PATCH] New default role allowing to change per-role/database settings

Greetings Michael,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> Am Montag, den 08.03.2021, 20:54 +0500 schrieb Ibrar Ahmed:
> > On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael(dot)banck(at)credativ(dot)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".

If we're going to make the context be called 'administrator' then it
would seem like we should make the predefined role be
"pg_change_admin_settings"...? Thoughts on that?

> > > 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

Just to be clear- the predefined role being added here actually allows a
user with this role to change both 'admin' and 'user' GUCs across all
databases and across all non-superuser roles, right? (A bit
disappointed at the lack of any documentation included in this patch..
presumably that would have made it clear).

> > > 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

Being able to set temp_file_limit certainly seems appropriate.

> > > * session_replication_role

I'm less sure about 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 tend to agree that it should be included, I don't see any particular
reason why that would be worse than, say, log_statement.

> > > 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.

Yeah, context is how these have been managed before and it seems to make
sense to continue with that general idea.

> Please find attached the rebase; I've removed the catversion hunk as I
> believe it is customary to leave that to committers.

Yeah, generally no need to include that in the patch.

> 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.

I don't really see an issue with this approach but I do have to admit to
wondering about ALTER SYSTEM. Any thoughts regarding that..?

> ---
> 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);

I have to admit to wondering if we should still require database owner
rights when it comes to this (that is- possibly require both?). With
this approach, the user with the predefined role could change the
settings for any database, even one they don't have rights to connect
to..

> 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_' },

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Thanks!

Stephen