Обсуждение: [PATCH] New default role allowing to change per-role/database settings
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.banck@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
Вложения
Greetings Michael,
* Michael Banck (michael.banck@credativ.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.banck@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".
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
Вложения
Hi,
Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:
> * Michael Banck (michael.banck@credativ.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.banck@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".
>
> 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.banck@credativ.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
Hi, Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck: > Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost: > > Should drop the 'DEFAULT_' to match the others since the rename to > > 'predefined' roles went in. > > Right, will send a rebased patch ASAP. Here's a rebased version that also removes the ALTER DATABASE SET capability from this new predefined role and adds some documentation. Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.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
Вложения
On Wed, Apr 7, 2021 at 5:23 PM Michael Banck <michael.banck@credativ.de> wrote: > > Hi, > > Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck: > > Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost: > > > Should drop the 'DEFAULT_' to match the others since the rename to > > > 'predefined' roles went in. > > > > Right, will send a rebased patch ASAP. > > Here's a rebased version that also removes the ALTER DATABASE SET > capability from this new predefined role and adds some documentation. > The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Regards, Vignesh
Hi, Am Mittwoch, den 14.07.2021, 21:29 +0530 schrieb vignesh C: > On Wed, Apr 7, 2021 at 5:23 PM Michael Banck <michael.banck@credativ.de> wrote: > > Hi, > > > > Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck: > > > Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost: > > > > Should drop the 'DEFAULT_' to match the others since the rename to > > > > 'predefined' roles went in. > > > > > > Right, will send a rebased patch ASAP. > > > > Here's a rebased version that also removes the ALTER DATABASE SET > > capability from this new predefined role and adds some documentation. > > The patch does not apply on Head anymore, could you rebase and post a > patch. I'm changing the status to "Waiting for Author". Thanks for letting me know, I've attached a rebased v4 of this patch, no other changes. Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 E-Mail: michael.banck@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Sascha Heuer, Geoff Richardson, Peter Lilley Unser Umgang mit personenbezogenen Daten unterliegt folgenden Bestimmungen: https://www.credativ.de/datenschutz
Вложения
>Thanks for letting me know, I've attached a rebased v4 of this patch, no other
>changes.
I tried it, but when I used set command, tab completion did not work properly and an error occurred.
---
postgres=> \conninfo
You are connected to database "postgres" as user "aaa" via socket in "/tmp" at port "5432".
postgres=> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+---------------------------
aaa | | {pg_change_role_settings}
penguin | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
postgres=> show log
log_autovacuum_min_duration log_executor_stats log_min_error_statement
log_replication_commands log_timezone
log_checkpoints log_file_mode log_min_messages
log_rotation_age log_transaction_sample_rate
log_connections log_hostname log_parameter_max_length
log_rotation_size log_truncate_on_rotation
log_destination log_line_prefix log_parameter_max_length_on_error log_statement
logging_collector
log_disconnections log_lock_waits log_parser_stats
log_statement_sample_rate logical_decoding_work_mem
log_duration log_min_duration_sample log_planner_stats
log_statement_stats
log_error_verbosity log_min_duration_statement log_recovery_conflict_waits log_temp_files
postgres=> show log_duration ;
log_duration
--------------
off
(1 row)
postgres=> set log
log_parameter_max_length_on_error logical_decoding_work_mem
postgres=> set log_duration to on;
2021-09-08 16:23:39.216 JST [533860] ERROR: permission denied to set parameter "log_duration"
2021-09-08 16:23:39.216 JST [533860] STATEMENT: set log_duration to on;
ERROR: permission denied to set parameter "log_duration"
---
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi, On Wed, Sep 08, 2021 at 07:38:25AM +0000, Shinya11.Kato@nttdata.com wrote: > >Thanks for letting me know, I've attached a rebased v4 of this patch, no other > >changes. > I tried it, but when I used set command, tab completion did not work properly and an error occurred. Also, the patch doesn't apply anymore: http://cfbot.cputube.org/patch_36_2918.log === Applying patches on top of PostgreSQL commit ID 5513dc6a304d8bda114004a3b906cc6fde5d6274 === === applying patch ./v4-0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_ro.patch [...] 1 out of 23 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej I'm switching the patch to Waiting on Author.
As discussed in [1], we're taking this opportunity to return some
patchsets that don't appear to be getting enough reviewer interest.
This is not a rejection, since we don't necessarily think there's
anything unacceptable about the entry, but it differs from a standard
"Returned with Feedback" in that there's probably not much actionable
feedback at all. Rather than code changes, what this patch needs is more
community interest. You might
- ask people for help with your approach,
- see if there are similar patches that your code could supplement,
- get interested parties to agree to review your patch in a CF, or
- possibly present the functionality in a way that's easier to review
overall.
(Doing these things is no guarantee that there will be interest, but
it's hopefully better than endlessly rebasing a patchset that is not
receiving any feedback from the community.)
Once you think you've built up some community support and the patchset
is ready for review, you (or any interested party) can resurrect the
patch entry by visiting
https://commitfest.postgresql.org/38/2918/
and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)
Thanks,
--Jacob
[1]
https://postgr.es/m/flat/0ab66589-2f71-69b3-2002-49e821740b0d%40timescale.com