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

Hi,

Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:
> * 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?

Well, I think the "administrator" guc context would be much less visible
than a "pg_change_admin_settings" predefined role. As a user, I would
assume such a predefined role would provide something more universal
than what is proposed here. That could still be the case later on, of
course.

Maybe somebody else has another idea on naming things?

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

About all databases, see below for your suggestion to limit it to the
intersection of this predefined role and the database owner.

Not exactly sure what you mean with "both 'admin' and 'user' GUCs", but
yeah, one could classify the above categories as such I guess. Other
SUSET-GUCs like the developer options are not touched, though.

I did not
include documentation in the initial patch because I wasn't sure whether
there was any interest in it (and admittedly, the commitfest deadline
came up I think), I can work on that now.

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

I'll keep those two out for now.

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

Ok.

[...]

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

Not sure what you mean here?

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

That's a good point. I have to admit my itch was mostly about role-based
settings and database-based settings are kinda in there as well.

I will
ponder this some more, but it seems to me it would be better to err on
the more restrictive side of things for now and we could open it up
later?

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

Right, will send a rebased patch ASAP.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email:
michael(dot)banck(at)credativ(dot)de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen:
https://www.credativ.de/datenschutz