Обсуждение: Possibility to disable `ALTER SYSTEM`
Hi everyone,
I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (or even both), without changing the current default method of the server.
The main reason is that this would help improve the “security by default” posture of Postgres in a Kubernetes/Cloud Native environment - and, in general, in any environment on VMs/bare metal behind a configuration management system in which changes should only be made in a declarative way and versioned like Ansible Tower, to cite one.
Below you find some background information and the longer story behind this proposal.
Sticking to the Kubernetes use case, I am primarily speaking on behalf of the CloudNativePG open source operator (cloudnative-pg.io, of which I am one of the maintainers). However, I am sure that this option could benefit any operator for Postgres - an operator is the most common and recommended way to run a complex application like a PostgreSQL database management system inside Kubernetes.
In this case, the state of a PostgreSQL cluster (for example its number of replicas, configuration, storage, etc.) is defined in a Custom Resource Definition in the form of configuration, typically YAML, and the operator works with Kubernetes to ensure that, at any moment, the requested Postgres cluster matches the observed one. This is a very basic example in CloudNativePG: https://cloudnative-pg.io/documentation/current/samples/cluster-example.yaml
As I was mentioning above, in a Cloud Native environment it is expected that workloads are secure by default. Without going into much detail, many decisions have been made in that direction by operators for Postgres, including CloudNativePG. The goal of this proposal is to provide a way to ensure that changes to the PostgreSQL configuration in a Kubernetes controlled Postgres cluster are allowed only through the Kubernetes API.
Basically, if you want to change an option for PostgreSQL, you need to change the desired state in the Kubernetes resource, then Kubernetes will converge (through the operator). In simple words, it’s like empowering the operator to impersonate the PostgreSQL superuser.
However, given that we cannot force this use case, there could be roles with the login+superuser privileges connecting to the PostgreSQL instance and potentially “interfering” with the requested state defined in the configuration by imperatively running “ALTER SYSTEM” commands.
For example: CloudNativePG has a fixed value for some GUCs in order to manage a full HA cluster, including SSL, log, some WAL and replication settings. While the operator eventually reconciles those settings, even the temporary change of those settings in a cluster might be harmful. Think for example of a user that, through `ALTER SYSTEM`, tries to change WAL level to minimal, or change the setting of the log (we require CSV), potentially creating issues to the underlying instance and cluster (potentially leaving it in an unrecoverable state in the case of other more invasive GUCS).
At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked by making the postgresql.auto.conf read only, but the returned message is misleading and that’s certainly bad user experience (which is very important in a cloud native environment):
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```
For this reason, I would like to propose the option to be given to the postgres process at startup, in order to be as less invasive as possible (the operator could then start Postgres requesting `ALTER SYSTEM` to be disabled). That’d be my preference at the moment, if possible.
Alternatively, or in addition, the introduction of a GUC to disable `ALTER SYSTEM` altogether. This enables tuning this setting through configuration at the Kubernetes level, only if the operators require it - without damaging the rest of the users.
Before I start writing any lines of code and propose a patch, I would like first to understand if there’s room for it.
Thanks for your attention and … looking forward to your feedback!
Ciao,
Gabriele
I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (or even both), without changing the current default method of the server.
The main reason is that this would help improve the “security by default” posture of Postgres in a Kubernetes/Cloud Native environment - and, in general, in any environment on VMs/bare metal behind a configuration management system in which changes should only be made in a declarative way and versioned like Ansible Tower, to cite one.
Below you find some background information and the longer story behind this proposal.
Sticking to the Kubernetes use case, I am primarily speaking on behalf of the CloudNativePG open source operator (cloudnative-pg.io, of which I am one of the maintainers). However, I am sure that this option could benefit any operator for Postgres - an operator is the most common and recommended way to run a complex application like a PostgreSQL database management system inside Kubernetes.
In this case, the state of a PostgreSQL cluster (for example its number of replicas, configuration, storage, etc.) is defined in a Custom Resource Definition in the form of configuration, typically YAML, and the operator works with Kubernetes to ensure that, at any moment, the requested Postgres cluster matches the observed one. This is a very basic example in CloudNativePG: https://cloudnative-pg.io/documentation/current/samples/cluster-example.yaml
As I was mentioning above, in a Cloud Native environment it is expected that workloads are secure by default. Without going into much detail, many decisions have been made in that direction by operators for Postgres, including CloudNativePG. The goal of this proposal is to provide a way to ensure that changes to the PostgreSQL configuration in a Kubernetes controlled Postgres cluster are allowed only through the Kubernetes API.
Basically, if you want to change an option for PostgreSQL, you need to change the desired state in the Kubernetes resource, then Kubernetes will converge (through the operator). In simple words, it’s like empowering the operator to impersonate the PostgreSQL superuser.
However, given that we cannot force this use case, there could be roles with the login+superuser privileges connecting to the PostgreSQL instance and potentially “interfering” with the requested state defined in the configuration by imperatively running “ALTER SYSTEM” commands.
For example: CloudNativePG has a fixed value for some GUCs in order to manage a full HA cluster, including SSL, log, some WAL and replication settings. While the operator eventually reconciles those settings, even the temporary change of those settings in a cluster might be harmful. Think for example of a user that, through `ALTER SYSTEM`, tries to change WAL level to minimal, or change the setting of the log (we require CSV), potentially creating issues to the underlying instance and cluster (potentially leaving it in an unrecoverable state in the case of other more invasive GUCS).
At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked by making the postgresql.auto.conf read only, but the returned message is misleading and that’s certainly bad user experience (which is very important in a cloud native environment):
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```
For this reason, I would like to propose the option to be given to the postgres process at startup, in order to be as less invasive as possible (the operator could then start Postgres requesting `ALTER SYSTEM` to be disabled). That’d be my preference at the moment, if possible.
Alternatively, or in addition, the introduction of a GUC to disable `ALTER SYSTEM` altogether. This enables tuning this setting through configuration at the Kubernetes level, only if the operators require it - without damaging the rest of the users.
Before I start writing any lines of code and propose a patch, I would like first to understand if there’s room for it.
Thanks for your attention and … looking forward to your feedback!
Ciao,
Gabriele
--
On 9/7/23 15:51, Gabriele Bartolini wrote: > I would like to propose a patch that allows administrators to disable > `ALTER SYSTEM` via either a runt-time option to pass to the Postgres > server process at startup (e.g. `--disable-alter-system=true`, false by > default) or a new GUC (or even both), without changing the current > default method of the server. Without trying to debate the particulars, a huge +1 for the concept of allowing ALTER SYSTEM to be disabled. FWIW I would vote the same for control over COPY PROGRAM. Not coincidentally both concepts were built into set_user: https://github.com/pgaudit/set_user -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi Joe,
On Thu, 7 Sept 2023 at 21:57, Joe Conway <mail@joeconway.com> wrote:
Without trying to debate the particulars, a huge +1 for the concept of
allowing ALTER SYSTEM to be disabled. FWIW I would vote the same for
control over COPY PROGRAM.
Oh ... another one of my favourite security friendly features! :)
That sounds like a good idea to me.
Thanks,
Gabriele
-- Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> writes: > I would like to propose a patch that allows administrators to disable > `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server > process at startup (e.g. `--disable-alter-system=true`, false by default) > or a new GUC (or even both), without changing the current default method of > the server. ALTER SYSTEM is already heavily restricted. I don't think we need random kluges added to the permissions system. I especially don't believe in kluges to the effect of "superuser doesn't have all permissions anymore". If you nonetheless feel that that's a good idea for your use case, you can implement the restriction with an event trigger or the like. regards, tom lane
Hi Tom,
On Thu, 7 Sept 2023 at 22:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> writes:
> I would like to propose a patch that allows administrators to disable
> `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server
> process at startup (e.g. `--disable-alter-system=true`, false by default)
> or a new GUC (or even both), without changing the current default method of
> the server.
ALTER SYSTEM is already heavily restricted.
Could you please help me better understand what you mean here?
I don't think we need random kluges added to the permissions system.
If you allow me, why do you think disabling ALTER SYSTEM altogether is a random kluge? Again, I'd like to better understand this position. I've personally been in many conversations on the security side of things for Postgres in Kubernetes environments, and this is a frequent concern by users who request that changes to the Postgres system (not a database) should only be done declaratively and prevented from within the system.
Thanks,
Gabriele
Thanks,
Gabriele
On Fri, 8 Sept 2023 at 10:03, Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> wrote:
ALTER SYSTEM is already heavily restricted.Could you please help me better understand what you mean here?I don't think we need random kluges added to the permissions system.If you allow me, why do you think disabling ALTER SYSTEM altogether is a random kluge? Again, I'd like to better understand this position. I've personally been in many conversations on the security side of things for Postgres in Kubernetes environments, and this is a frequent concern by users who request that changes to the Postgres system (not a database) should only be done declaratively and prevented from within the system.
Alternate idea, not sure how good this is: Use existing OS security features (regular permissions, or more modern features such as the immutable attribute) to mark the postgresql.auto.conf file as not being writeable. Then any attempt to ALTER SYSTEM should result in an error.
Hi Isaac,
On Fri, 8 Sept 2023 at 16:11, Isaac Morland <isaac.morland@gmail.com> wrote:
Alternate idea, not sure how good this is: Use existing OS security features (regular permissions, or more modern features such as the immutable attribute) to mark the postgresql.auto.conf file as not being writeable. Then any attempt to ALTER SYSTEM should result in an error.
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```
IMO we should clearly state that `ALTER SYSTEM` is deliberately disabled in a system, rather than indirectly hinting it through an inaccessible file. Not sure if I am clearly highlighting the fine difference here.
Thanks,
Gabriele
-- On 2023-Sep-08, Gabriele Bartolini wrote: > That is the point I highlighted in the initial post in the thread. We could > make it readonly, but the returned error is misleading and definitely poor > UX: > > ``` > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf": Permission denied > ``` > > IMO we should clearly state that `ALTER SYSTEM` is deliberately disabled in > a system, rather than indirectly hinting it through an inaccessible file. > Not sure if I am clearly highlighting the fine difference here. Come on. This is not a "fine difference" -- it's the difference between a crummy hack and a real implementation of an important system restriction. I don't understand Tom's resistance to this request. I understand the use case and I agree with Gabriele that this is a very simple code change(*) that Postgres could make to help it get run better in a different kind of environment than what we're accustomed to. I've read that containers people consider services in a different light than how we've historically seen them; they say "cattle, not pets". This affects the way you think about these services. postgresql.conf (all the PG configuration, really) is just a derived file from an overall system description that lives outside the database server. You no longer feed your PG servers one by one, but rather they behave as a herd, and the herder is some container supervisor (whatever it's called). Ensuring that the configuration state cannot change from within is important to maintain the integrity of the service. If the user wants to change things, the tools to do that are operated from outside; this lets things like ancillary services to be kept in sync (say, start a replica here, or a backup system there, or WAL archival/collection is handled in this or that way). If users are allowed to change the config from within they break things, and the supervisor program can't put things together again. I did not like the mention of COPY PROGRAM, though, and in principle I do not support the idea of treating it the same way as ALTER SYSTEM. If people are using that to write into postgresql.conf, then they deserve all the hell they get when their containers break. I think trying to restrict this outside of the privilege system is going to be more of a wart than ALTER SYSTEM. (*) To be proven. Let's see the patch. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Syntax error: function hell() needs an argument. Please choose what hell you want to involve.
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > I don't understand Tom's resistance to this request. It's false security. If you think you are going to prevent a superuser from messing with the system's configuration, you are going to need a lot more restrictions than this, and we'll be forever getting security reports that "hey, I found another way for a superuser to get filesystem access". I think the correct answer to this class of problems is "don't give superuser privileges to clients running inside the container". > I did not like the mention of COPY PROGRAM, though, and in principle I > do not support the idea of treating it the same way as ALTER SYSTEM. It's one of the easiest ways to modify postgresql.conf from SQL. If you don't block that off, the feature is certainly not secure. (But of course, there are more ways.) regards, tom lane
Hi Tom and Alvaro,
On Fri, 8 Sept 2023 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I don't understand Tom's resistance to this request.
It's false security. If you think you are going to prevent a superuser
from messing with the system's configuration, you are going to need a
lot more restrictions than this, and we'll be forever getting security
reports that "hey, I found another way for a superuser to get filesystem
access". I think the correct answer to this class of problems is "don't
give superuser privileges to clients running inside the container".
Ok, this is clearer. That makes sense now, and this probably helps me explain better the goal here. I also omitted in the initial email all the security precautions that a Kubernetes should take. This could be another step towards that direction but, you are right, it won't fix it entirely (in case of malicious superusers).
In my opinion, the biggest benefit of this possibility is on the usability side, providing a clear and configurable way to disable ALTER SYSTEM in those environments where declarative configuration is a requirement. For example, this should at least "warn" human beings that have the permissions to connect to a Postgres database (think of SREs managing a DBaaS solution or a DBA) and try to change a setting in an instance. Moreover, for those who are managing through declarative configuration not only one instance, but a Postgres cluster that controls standby instances too, the benefit of impeding these modifications could be even higher (think of the hot standby sensitive parameters like max_connections that require coordination depending whether you increase or decrease them).
I hope this is clearer. For what it's worth, I have done a basic PoC patch (roughly 20 lines of code), which I have attached here just to provide some basis for further analysis and comments. The general idea is to disable ALTER SYSTEM at startup, like this:
pg_ctl start -o "-c enable_alter_system=off"
The setting can be verified with:
psql -c 'SHOW enable_alter_system'enable_alter_system---------------------off(1 row)
And then:
psql -c 'ALTER SYSTEM SET max_connections TO 10'ERROR: permission denied to run ALTER SYSTEM
Thanks for your attention and looking forward to getting feedback and advice.
Cheers,
Gabriele
-- Вложения
On Fri, Sep 8, 2023 at 5:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > I don't understand Tom's resistance to this request. > > It's false security. If you think you are going to prevent a superuser > from messing with the system's configuration, you are going to need a > lot more restrictions than this, and we'll be forever getting security > reports that "hey, I found another way for a superuser to get filesystem > access". I think the correct answer to this class of problems is "don't > give superuser privileges to clients running inside the container". +1. And to make that happen, the appropriate thing is to identify *why* they are using superuser today, and focus efforts on finding ways for them to do that without being superuser. > > I did not like the mention of COPY PROGRAM, though, and in principle I > > do not support the idea of treating it the same way as ALTER SYSTEM. > > It's one of the easiest ways to modify postgresql.conf from SQL. If you > don't block that off, the feature is certainly not secure. (But of > course, there are more ways.) It's easier to just create a function in an untrusted language. Same principle. Once you're superuser, you can break through anything. We need a "allowlist" of things a user can do, rather than a blocklist of "they can do everything they can possibly think of and a computer is capable of doing, except for this one specific thing". Blocklisting individual permissions of a superuser will never be secure. Now, it might be that you don't care at all about the *security* side of the feature, and only care about the convenience side. But in that case, the original suggestion from Tom of using an even trigger seems like a fine enough solution? -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On 7/9/23 21:51, Gabriele Bartolini wrote:
Hi everyone,
I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (or even both), without changing the current default method of the server.
The main reason is that this would help improve the “security by default” posture of Postgres in a Kubernetes/Cloud Native environment - and, in general, in any environment on VMs/bare metal behind a configuration management system in which changes should only be made in a declarative way and versioned like Ansible Tower, to cite one.
Below you find some background information and the longer story behind this proposal.
Sticking to the Kubernetes use case, I am primarily speaking on behalf of the CloudNativePG open source operator (cloudnative-pg.io, of which I am one of the maintainers). However, I am sure that this option could benefit any operator for Postgres - an operator is the most common and recommended way to run a complex application like a PostgreSQL database management system inside Kubernetes.
In this case, the state of a PostgreSQL cluster (for example its number of replicas, configuration, storage, etc.) is defined in a Custom Resource Definition in the form of configuration, typically YAML, and the operator works with Kubernetes to ensure that, at any moment, the requested Postgres cluster matches the observed one. This is a very basic example in CloudNativePG: https://cloudnative-pg.io/documentation/current/samples/cluster-example.yaml
As I was mentioning above, in a Cloud Native environment it is expected that workloads are secure by default. Without going into much detail, many decisions have been made in that direction by operators for Postgres, including CloudNativePG. The goal of this proposal is to provide a way to ensure that changes to the PostgreSQL configuration in a Kubernetes controlled Postgres cluster are allowed only through the Kubernetes API.
Basically, if you want to change an option for PostgreSQL, you need to change the desired state in the Kubernetes resource, then Kubernetes will converge (through the operator). In simple words, it’s like empowering the operator to impersonate the PostgreSQL superuser.
Coming from a similar background to Gabriele's, I support this proposal.
In StackGres (https://stackgres.io) we also allow users to manage postgresql.conf's configuration declaratively. We have a CRD (Custom Resource Definition) that precisely defines and controls how a postgresql.conf configuration looks like (see https://stackgres.io/doc/latest/reference/crd/sgpgconfig/). This configuration, once created by the user, is strongly validated by StackGres (parameters are valid for the given major version, values are within the ranges and appropriate types) and then periodically applied to the database if there's any drift between that user-declared (desired) state and current system status.
Similarly, we also have some parameters which the user is not allowed to change (https://gitlab.com/ongresinc/stackgres/-/blob/main/stackgres-k8s/src/operator/src/main/resources/postgresql-blocklist.properties). If the user is allowed to use ALTER SYSTEM and modify some of these parameters, significant breakage can happen in the cluster, as the operator may become "confused" and manual operation may be required, breaking many of the user's expectations of stability and how the system works and heals automatically.
Sure, as mentioned elsewhere in the thread, a "malicious" user can still use other mechanisms such as COPY or untrusted PLs to still overwrite the configuration. But both attempts are obviously conscious attempts to break the system (and if so, it's all yours to break it). But ALTER SYSTEM may be an *unintended* way to break it, causing a bad user's experience. This may be defined more of a way to avoid users shooting themselves in the feet, inadvertedly.
There's apparently some opposition to implementing this. But given that there's also interest in having it, I'd like to know what the negative effects of implementing such a startup configuration property would be, so that advantages can be compared with the disadvantages.
All that being said, the behavior to prevent ALTER SYSTEM can also be easily implemented as an extension. Actually some colleagues wrote one with a similar scope (https://gitlab.com/ongresinc/extensions/noset), and I believe it could be the base for a similar extension focused on preventing ALTER SYSTEM.
Regards,
Álvaro
-- Alvaro Hernandez ----------- OnGres
Hi Magnus,
On Fri, 8 Sept 2023 at 23:43, Magnus Hagander <magnus@hagander.net> wrote:
+1. And to make that happen, the appropriate thing is to identify
*why* they are using superuser today, and focus efforts on finding
ways for them to do that without being superuser.
As I am explaining in the other post (containing a very basic proof of concept patch), it is not about restricting superuser. It is primarily a usability and configuration matter of a running instance, which helps control an entire cluster like in the case of Kubernetes (where, in order to provide self-healing and high availability we are forced to go beyond the single instance and think in terms of primary with one or more standbys or at least continuous backup in place).
Thanks,
Gabriele
On 2023-Sep-08, Magnus Hagander wrote: > Now, it might be that you don't care at all about the *security* side > of the feature, and only care about the convenience side. But in that > case, the original suggestion from Tom of using an even trigger seems > like a fine enough solution? ALTER SYSTEM, like all system-wide commands, does not trigger event triggers. These are per-database only. https://www.postgresql.org/docs/16/event-trigger-matrix.html -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "If it is not right, do not do it. If it is not true, do not say it." (Marcus Aurelius, Meditations)
Hi, > I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to passto the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (or evenboth), without changing the current default method of the server. I'm actually going to put a strong +1 to Gabriele's proposal. It's an undeniable problem (I'm only seeing arguments regarding other ways the system would be insecure), and there might be real use cases for users outside kubernetes for having this feature and using it (meaning disabling the use of ALTER SYSTEM). In Patroni for example, the PostgreSQL service is controlled on all nodes by Patroni, and these kinds of changes could end up breaking the cluster if there was a failover. For this reason Patroni starts postgres with some GUC options as CMD arguments so that values in postgresql.conf or postgresql.auto.conf are ignored. The values in the DCS are the ones that matter. ``` postgres 1171221 0.0 1.9 903560 55168 ? S 10:16 0:00 /usr/pgsql-15/bin/postgres -D /opt/postgres/data --config-file=/opt/postgres/data/postgresql.conf --listen_addresses=0.0.0.0 --port=5432 --cluster_name=patroni-tpa --wal_level=logical --hot_standby=on --max_connections=250 --max_wal_senders=6 --max_prepared_transactions=0 --max_locks_per_transaction=64 --track_commit_timestamp=off --max_replication_slots=6 --max_worker_processes=16 --wal_log_hints=on ``` (see more about how Patroni manages this here: https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni But let's face it, that's a hack, not something to be proud of, even if it does what is intended. And this is a product and we shouldn't be advertising hacks to overcome limitations. I find the opposition to this lacking good reasons, while I find the implementation to be useful in some cases. Kind regards, Martín -- Martín Marqués It’s not that I have something to hide, it’s that I have nothing I want you to see
On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Sep-08, Magnus Hagander wrote: > > > Now, it might be that you don't care at all about the *security* side > > of the feature, and only care about the convenience side. But in that > > case, the original suggestion from Tom of using an even trigger seems > > like a fine enough solution? > > ALTER SYSTEM, like all system-wide commands, does not trigger event > triggers. These are per-database only. > > https://www.postgresql.org/docs/16/event-trigger-matrix.html Hah, didn't think of that. And yes, that's a very good point. But one way to fix that would be to actually make event triggers for system wide commands, which would then be useful for other things as well... -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin.marques@gmail.com> wrote: > > Hi, > > > I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option topass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (oreven both), without changing the current default method of the server. > > I'm actually going to put a strong +1 to Gabriele's proposal. It's an > undeniable problem (I'm only seeing arguments regarding other ways the > system would be insecure), and there might be real use cases for users > outside kubernetes for having this feature and using it (meaning > disabling the use of ALTER SYSTEM). If enough people are in favor of it *given the known issues with it*, I can drop my objection to a "meh, but I still don't think it's a good idea". But to do that, there would need to be a very in-your-face warning in the documentation about it like "note that this only disables the ALTER SYSTEM command. It does not prevent a superuser from changing the configuration remotely using other means". For example, in the very simplest, wth the POC patch out there now, I can still run: postgres=# CREATE TEMP TABLE x(t text); CREATE TABLE postgres=# INSERT INTO x VALUES ('work_mem=1TB'); INSERT 0 1 postgres=# COPY x TO '/home/mha/postgresql/inst/head/data/postgresql.auto.conf'; COPY 1 postgres=# SELECT pg_reload_conf(); pg_reload_conf ---------------- t (1 row) postgres=# show work_mem; work_mem ---------- 1TB (1 row) Or anything similar to that. Yes, this is marginally harder than saying ALTER SYSTEM SET work_mem='1TB', but only very very marginally so. And from a security perspective, there is zero difference. But we do also allow "trust" authentication which is another major footgun from a security perspective, against which we only defend with warnings, so that in itself is not a reason not to do the same here. > In Patroni for example, the PostgreSQL service is controlled on all > nodes by Patroni, and these kinds of changes could end up breaking the > cluster if there was a failover. For this reason Patroni starts > postgres with some GUC options as CMD arguments so that values in > postgresql.conf or postgresql.auto.conf are ignored. The values in the > DCS are the ones that matter. Right. And patroni would need to continue to do that even with this patch, because it also needs to override if somebody puts something in postgresql.conf, no? Removing the defence against that seems like a bad idea... > (see more about how Patroni manages this here: > https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni > > But let's face it, that's a hack, not something to be proud of, even > if it does what is intended. And this is a product and we shouldn't be > advertising hacks to overcome limitations. It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd also have to manage postgresql.conf. One slightly less hacky part might be to have patroni generate a config file of it's own and include it with the highest priority -- at that point it *would* be come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher priority than any manual user config file. But it is not today. Another idea to solve the problem could be to instead introduce a specific configuration file (either hardcoded or an include_final_parameter=<path> parameter, in which case patroni or the k8s operator could set that parameter on the command line and that parameter only) that is parsed *after* postgresql.auto.conf and thereby would override the manual settings. This file would explicilty be documented as intended for this type of tooling, and when you have a tool - whether patroni or another declarative operator - it owns this file and can overwrite it with whatever it wants. And this would also retain the ability to use ALTER SYSTEM SET for *other* parameters, if they want to. That's just a very quick idea and there may definitely be holes in it, but I'm not sure those holes are any worse than what's suggested here, and I do thin kit's cleaner. > I find the opposition to this lacking good reasons, while I find the > implementation to be useful in some cases. Stopping ALTER SYSTEM SET without actually preventing the superuser from doing the same thing as they were doing before would to me be at least as much of a hack as what patroni does today is. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Hi Magnus,
On Mon, 11 Sept 2023 at 16:04, Magnus Hagander <magnus@hagander.net> wrote:
But to do that, there would need to be a very in-your-face warning in
the documentation about it like "note that this only disables the
ALTER SYSTEM command. It does not prevent a superuser from changing
the configuration remotely using other means".
Although I did not include any docs in the PoC patch, that's exactly the plan. So +1 from me.
Yes, this is marginally harder than saying ALTER SYSTEM SET
work_mem='1TB', but only very very marginally so. And from a security
perspective, there is zero difference.
Agree, but the primary goal is not security. Indeed, security requires a more holistic approach (and in my initial thread I deliberately did not mention all the knobs that the operator provides, with stricter and stricter default values, as I thought it was not relevant from a Postgres' PoV). However, as I explained in the patch PoC thread, the change is intended primarily to warn legitimate administrators in a configuration managed controlled environment that ALTER SYSTEM has been disabled for that system. These are environments where network access for a superuser is prohibited, but still possible for local SREs to log in via the container in the pod for incident resolution - very often this happens in high stress conditions and I believe that this gate will help them remind that if they want to change the settings they need to do it through the Kubernetes resources. So primarily: usability.
Another idea to solve the problem could be to instead introduce a
specific configuration file (either hardcoded or an
include_final_parameter=<path> parameter, in which case patroni or the
k8s operator could set that parameter on the command line and that
parameter only) that is parsed *after* postgresql.auto.conf and
thereby would override the manual settings. This file would explicilty
be documented as intended for this type of tooling, and when you have
a tool - whether patroni or another declarative operator - it owns
this file and can overwrite it with whatever it wants. And this would
also retain the ability to use ALTER SYSTEM SET for *other*
parameters, if they want to.
But that is exactly the whole point of this request: disable the last operation altogether. This option will easily give any operator (or deployment in a configuration management scenario) the possibility to ship a system that, out-of-the box, facilitates this one direction update of the configuration, allowing the observed state to easily reconcile with the desired one. Without breaking any existing deployment.
Stopping ALTER SYSTEM SET without actually preventing the superuser
from doing the same thing as they were doing before would to me be at
least as much of a hack as what patroni does today is.
Agree, but as I said above, that'd be at that point the role of an operator. An operator, at that point, will have the possibility to configure this knob in conjunction with others. A possibility that Postgres is not currently giving.
Postgres itself should be able to give this possibility, as these environments demand Postgres to address their emerging needs.
Thank you,
Gabriele
Greetings, * Magnus Hagander (magnus@hagander.net) wrote: > On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin.marques@gmail.com> wrote: > > > I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time optionto pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC(or even both), without changing the current default method of the server. > > > > I'm actually going to put a strong +1 to Gabriele's proposal. It's an > > undeniable problem (I'm only seeing arguments regarding other ways the > > system would be insecure), and there might be real use cases for users > > outside kubernetes for having this feature and using it (meaning > > disabling the use of ALTER SYSTEM). > > If enough people are in favor of it *given the known issues with it*, > I can drop my objection to a "meh, but I still don't think it's a good > idea". A lot of the objections seem to be on the grounds of returning a 'permission denied' kind of error and I generally agree with that being the wrong approach. As an alternative idea- what if we had something in postgresql.conf along the lines of: include_alter_system = true/false and use that to determine if the postgresql.auto.conf is included or not..? > But to do that, there would need to be a very in-your-face warning in > the documentation about it like "note that this only disables the > ALTER SYSTEM command. It does not prevent a superuser from changing > the configuration remotely using other means". With the above, we could throw a WARNING or maybe just NOTICE when ALTER SYSTEM is run that 'include_alter_system is false and therefore these changes won't be included in the running configuration' or similar. What this does cause problems with is that pg_basebackup and other tools (eg: pgbackrest) write into postgresql.auto.conf currently and we'd want those to still work. That's an opportunity, imv, though, since I don't really think where ALTER SYSTEM writes to and where backup/restore tools are writing to should really be the same place anyway. Therefore, perhaps we add a 'postgresql.system.conf' or similar and maybe a corresponding option in postgresql.conf to include it or not. > For example, in the very simplest, wth the POC patch out there now, I > can still run: > postgres=# CREATE TEMP TABLE x(t text); > CREATE TABLE > postgres=# INSERT INTO x VALUES ('work_mem=1TB'); > INSERT 0 1 > postgres=# COPY x TO '/home/mha/postgresql/inst/head/data/postgresql.auto.conf'; > COPY 1 > postgres=# SELECT pg_reload_conf(); > pg_reload_conf > ---------------- > t > (1 row) > postgres=# show work_mem; > work_mem > ---------- > 1TB > (1 row) > > Or anything similar to that. This is an issue if you're looking at it as a security thing. This isn't an issue if don't view it that way. Still, I do see some merit in making it so that you can't actually change the config that's loaded at system start from inside the data directory or as the PG superuser, which my proposal above would support- just configure in postgresql.conf to not include any of the alter-system or generated config. The actual postgresql.conf could be owned by root then too. > > In Patroni for example, the PostgreSQL service is controlled on all > > nodes by Patroni, and these kinds of changes could end up breaking the > > cluster if there was a failover. For this reason Patroni starts > > postgres with some GUC options as CMD arguments so that values in > > postgresql.conf or postgresql.auto.conf are ignored. The values in the > > DCS are the ones that matter. > > Right. And patroni would need to continue to do that even with this > patch, because it also needs to override if somebody puts something in > postgresql.conf, no? Removing the defence against that seems like a > bad idea... > > > > (see more about how Patroni manages this here: > > https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni > > > > But let's face it, that's a hack, not something to be proud of, even > > if it does what is intended. And this is a product and we shouldn't be > > advertising hacks to overcome limitations. > > It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd > also have to manage postgresql.conf. One slightly less hacky part > might be to have patroni generate a config file of it's own and > include it with the highest priority -- at that point it *would* be > come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher > priority than any manual user config file. But it is not today. I suppose we could invent a priority control thing as part of the above proposal too.. but I would think just having include_alter_system and include_auto_config (or whatever we name them) would be enough, with the auto-config bit being loaded last and therefore having the 'highest' priority. > Another idea to solve the problem could be to instead introduce a > specific configuration file (either hardcoded or an > include_final_parameter=<path> parameter, in which case patroni or the > k8s operator could set that parameter on the command line and that > parameter only) that is parsed *after* postgresql.auto.conf and > thereby would override the manual settings. This file would explicilty > be documented as intended for this type of tooling, and when you have > a tool - whether patroni or another declarative operator - it owns > this file and can overwrite it with whatever it wants. And this would > also retain the ability to use ALTER SYSTEM SET for *other* > parameters, if they want to. Yeah, this is along the lines of what I propose above, but with the addition of having a way to control if these are loaded or not in the first place, instead of having to deal with every possible option that might be an issue. Generally, I do think having a separate file for tools to write into that's independent of ALTER SYSTEM would just be a good idea. I don't care for the way those are mixed in the same file these days. > That's just a very quick idea and there may definitely be holes in it, > but I'm not sure those holes are any worse than what's suggested here, > and I do thin kit's cleaner. Perhaps not surprising, I tend to agree that something along these lines is cleaner. Thanks, Stephen
Вложения
Hi Stephen,
On Mon, 11 Sept 2023 at 17:12, Stephen Frost <sfrost@snowman.net> wrote:
A lot of the objections seem to be on the grounds of returning a
'permission denied' kind of error and I generally agree with that being
the wrong approach.
As an alternative idea- what if we had something in postgresql.conf
along the lines of:
include_alter_system = true/false
and use that to determine if the postgresql.auto.conf is included or
not..?
That sounds like a very good idea. I had thought about that when writing the PoC, as a SIGHUP controlled GUC. I had trouble finding an adequate GUC category for that (ideas?), and thought it was a more intrusive patch to trigger the conversation. But I am willing to explore that too (which won't change by any inch the goal of the patch).
With the above, we could throw a WARNING or maybe just NOTICE when ALTER
SYSTEM is run that 'include_alter_system is false and therefore these
changes won't be included in the running configuration' or similar.
That's also an option I'd be willing to explore with folks here.
What this does cause problems with is that pg_basebackup and other tools
(eg: pgbackrest) write into postgresql.auto.conf currently and we'd want
those to still work. That's an opportunity, imv, though, since I don't
really think where ALTER SYSTEM writes to and where backup/restore
tools are writing to should really be the same place anyway. Therefore,
perhaps we add a 'postgresql.system.conf' or similar and maybe a
corresponding option in postgresql.conf to include it or not.
Totally. We are for example in the same position with the CloudNativePG operator, and it is something we are intending to fix (https://github.com/cloudnative-pg/cloudnative-pg/issues/2727). I agree with you that it is the wrong place to do it.
This is an issue if you're looking at it as a security thing. This
isn't an issue if don't view it that way. Still, I do see some merit in
making it so that you can't actually change the config that's loaded at
system start from inside the data directory or as the PG superuser,
which my proposal above would support- just configure in postgresql.conf
to not include any of the alter-system or generated config. The actual
postgresql.conf could be owned by root then too.
+1.
Thank you,
Gabriele
On Mon, 11 Sept 2023 at 11:11, Magnus Hagander <magnus@hagander.net> wrote:
> I'm actually going to put a strong +1 to Gabriele's proposal. It's an
> undeniable problem (I'm only seeing arguments regarding other ways the
> system would be insecure), and there might be real use cases for users
> outside kubernetes for having this feature and using it (meaning
> disabling the use of ALTER SYSTEM).
If enough people are in favor of it *given the known issues with it*,
I can drop my objection to a "meh, but I still don't think it's a good
idea".
But to do that, there would need to be a very in-your-face warning in
the documentation about it like "note that this only disables the
ALTER SYSTEM command. It does not prevent a superuser from changing
the configuration remotely using other means".
For example, in the very simplest, wth the POC patch out there now, I
can still run:
[…]
Maybe in addition to making "ALTER SYSTEM" throw an error, the feature that disables it should also disable reading postgresql.auto.conf? Maybe even delete it and make it an error if it is present on startup (maybe even warn if it shows up while the DB is running?).
Interesting corner case: What happens if I do "ALTER SYSTEM SET alter_system_disabled = true"?
Counterpoint: maybe the idea is to disable ALTER SYSTEM but still use postgresql.auto.conf, maintained by an external program, to control the instance's behaviour.
Hi, > Maybe in addition to making "ALTER SYSTEM" throw an error, the feature that disables it should also disable reading postgresql.auto.conf?Maybe even delete it and make it an error if it is present on startup (maybe even warn if it shows upwhile the DB is running?). The outcome looked for is that the system GUCs that require a restart or reload are not modified unless it's through some orchestration or someone with physical access to the configuration files (yeah, we still have the COPY PROGRAM). We shouldn't mix this with not reading postgresql.auto.conf, or even worse, deleting it. I don't think it's a good idea to delete the file. Ignoring it might be of interest, but completely outside the scope of the intention I'm seeing from the k8s teams. > Counterpoint: maybe the idea is to disable ALTER SYSTEM but still use postgresql.auto.conf, maintained by an external program,to control the instance's behaviour. I believe that's the idea, although we have `include` and `include_dir` which can be used the same way as `postgresql.auto.conf` is automatically included. Kind regards, Martín -- Martín Marqués It’s not that I have something to hide, it’s that I have nothing I want you to see
Seems to be some resistance to getting this in core, so why not just use an extension? I was able to create a quick POC to do just that. Hook into PG and look for AlterSystemStmt, throw a "Sorry, ALTER SYSTEM is not currently allowed" error. Put into shared_preload_libraries and you're done. As a bonus, works on all supported versions, so no need to wait for Postgres 17 - or Postgres 18/19 given the feature drift this thread is experiencing :)
Cheers,
Greg
Hi Greg,
On Wed, 13 Sept 2023 at 19:10, Greg Sabino Mullane <htamfids@gmail.com> wrote:
Seems to be some resistance to getting this in core, so why not just use an extension? I was able to create a quick POC to do just that. Hook into PG and look for AlterSystemStmt, throw a "Sorry, ALTER SYSTEM is not currently allowed" error. Put into shared_preload_libraries and you're done. As a bonus, works on all supported versions, so no need to wait for Postgres 17 - or Postgres 18/19 given the feature drift this thread is experiencing :)
As much as I would like to see your extension, I would still like to understand why Postgres itself shouldn't solve this basic requirement coming from the configuration management driven/Kubernetes space. It shouldn't be a big deal to have such an option, either as a startup one or a GUC, should it?
Thanks,
Gabriele
-- > On 11 Sep 2023, at 15:50, Magnus Hagander <magnus@hagander.net> wrote: > > On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> >> On 2023-Sep-08, Magnus Hagander wrote: >> >>> Now, it might be that you don't care at all about the *security* side >>> of the feature, and only care about the convenience side. But in that >>> case, the original suggestion from Tom of using an even trigger seems >>> like a fine enough solution? >> >> ALTER SYSTEM, like all system-wide commands, does not trigger event >> triggers. These are per-database only. >> >> https://www.postgresql.org/docs/16/event-trigger-matrix.html > > Hah, didn't think of that. And yes, that's a very good point. But one > way to fix that would be to actually make event triggers for system > wide commands, which would then be useful for other things as well... Wouldn't having system wide EVTs be a generic solution which could be the infrastructure for this requested change as well as others in the same area? -- Daniel Gustafsson
Hi,
I am sending an updated patch, and submitting this to the next commit fest, as I still believe this could be very useful.
Thanks,
Gabriele
On Thu, 7 Sept 2023 at 21:51, Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> wrote:
Hi everyone,
I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC (or even both), without changing the current default method of the server.
The main reason is that this would help improve the “security by default” posture of Postgres in a Kubernetes/Cloud Native environment - and, in general, in any environment on VMs/bare metal behind a configuration management system in which changes should only be made in a declarative way and versioned like Ansible Tower, to cite one.
Below you find some background information and the longer story behind this proposal.
Sticking to the Kubernetes use case, I am primarily speaking on behalf of the CloudNativePG open source operator (cloudnative-pg.io, of which I am one of the maintainers). However, I am sure that this option could benefit any operator for Postgres - an operator is the most common and recommended way to run a complex application like a PostgreSQL database management system inside Kubernetes.
In this case, the state of a PostgreSQL cluster (for example its number of replicas, configuration, storage, etc.) is defined in a Custom Resource Definition in the form of configuration, typically YAML, and the operator works with Kubernetes to ensure that, at any moment, the requested Postgres cluster matches the observed one. This is a very basic example in CloudNativePG: https://cloudnative-pg.io/documentation/current/samples/cluster-example.yaml
As I was mentioning above, in a Cloud Native environment it is expected that workloads are secure by default. Without going into much detail, many decisions have been made in that direction by operators for Postgres, including CloudNativePG. The goal of this proposal is to provide a way to ensure that changes to the PostgreSQL configuration in a Kubernetes controlled Postgres cluster are allowed only through the Kubernetes API.
Basically, if you want to change an option for PostgreSQL, you need to change the desired state in the Kubernetes resource, then Kubernetes will converge (through the operator). In simple words, it’s like empowering the operator to impersonate the PostgreSQL superuser.
However, given that we cannot force this use case, there could be roles with the login+superuser privileges connecting to the PostgreSQL instance and potentially “interfering” with the requested state defined in the configuration by imperatively running “ALTER SYSTEM” commands.
For example: CloudNativePG has a fixed value for some GUCs in order to manage a full HA cluster, including SSL, log, some WAL and replication settings. While the operator eventually reconciles those settings, even the temporary change of those settings in a cluster might be harmful. Think for example of a user that, through `ALTER SYSTEM`, tries to change WAL level to minimal, or change the setting of the log (we require CSV), potentially creating issues to the underlying instance and cluster (potentially leaving it in an unrecoverable state in the case of other more invasive GUCS).
At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked by making the postgresql.auto.conf read only, but the returned message is misleading and that’s certainly bad user experience (which is very important in a cloud native environment):
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```
For this reason, I would like to propose the option to be given to the postgres process at startup, in order to be as less invasive as possible (the operator could then start Postgres requesting `ALTER SYSTEM` to be disabled). That’d be my preference at the moment, if possible.
Alternatively, or in addition, the introduction of a GUC to disable `ALTER SYSTEM` altogether. This enables tuning this setting through configuration at the Kubernetes level, only if the operators require it - without damaging the rest of the users.
Before I start writing any lines of code and propose a patch, I would like first to understand if there’s room for it.
Thanks for your attention and … looking forward to your feedback!
Ciao,
Gabriele--
Вложения
On Tue, Sep 12, 2023 at 10:39 AM Martín Marqués <martin.marques@gmail.com> wrote: > The outcome looked for is that the system GUCs that require a restart > or reload are not modified unless it's through some orchestration or > someone with physical access to the configuration files (yeah, we > still have the COPY PROGRAM). If I understand this correctly, you're saying it's not a security vulnerability if someone finds a way to use COPY PROGRAM or some other mechanism to bypass the ALTER SYSTEM restriction, because the point of the constraint isn't to make it impossible for the superuser to modify the configuration in a way that they shouldn't, but rather to make it inconvenient for them to do so. I have to admit that I'm a little afraid that people will mistake this for an actual security feature and file bug reports or CVEs about the superuser being able to circumvent these restrictions. If we add this, we had better make sure that the documentation is extremely clear about what we are guaranteeing, or more to the point about what we are not guaranteeing. I understand that there's some frustration on the part of Gabriele and others that this proposal hasn't been enthusiastically adopted, but I would ask for a little bit of forbearance because those are also, by and large, not the people who will not have to cope with it when we start getting security researchers threatening to publish our evilness in the Register. Such conversations are no fun at all. Explaining that we're not actually evil doesn't tend to work, because the security researchers are just as convinced that they are right as anyone arguing for this feature is. Statements like "we don't actually intend to guarantee X" tend to fall on deaf ears. In fact, I would go so far as to argue that many of our security problems (and non-problems) are widely misunderstood even within our own community, and that far from being something anyone should dismiss as pedantry, it's actually a critical issue for the project to solve and something we really need to address in order to be able to move forward. From that point of view, this feature seems bound to make an already-annoying problem worse. I don't necessarily expect the people who are in favor of this feature to accept that as a reason not to do this, but I do hope to be taken seriously when I say there's a real issue there. Something can be a serious problem even if it's not YOUR problem, and in this case, that apparently goes both ways. I also think that using the GUC system to manage itself is a little bit suspect. I wonder if it would be better to do this some other way, e.g. a sentinel file in the data directory. For example, suppose we refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or something like that. It seems like it would be very easy for an external management solution (k8s or whatever) to drop that file in place if desired, and then it would be crystal clear that there's no way of bypassing the restriction from within the GUC system itself (though you could still bypass it via filesystem access). I agree with those who have said that this shouldn't disable postgresql.auto.conf, but only the ability of ALTER SYSTEM to modify it. Right now, third-party tools external to the server can count on being able to add things to postgresql.auto.conf with the reasonable expectations that they'll take effect. I'd rather not break that property. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > I have to admit that I'm a little afraid that people will mistake this > for an actual security feature and file bug reports or CVEs about the > superuser being able to circumvent these restrictions. If we add this, > we had better make sure that the documentation is extremely clear > about what we are guaranteeing, or more to the point about what we are > not guaranteeing. > I understand that there's some frustration on the part of Gabriele and > others that this proposal hasn't been enthusiastically adopted, but I > would ask for a little bit of forbearance because those are also, by > and large, not the people who will not have to cope with it when we > start getting security researchers threatening to publish our evilness > in the Register. Such conversations are no fun at all. Indeed. I'd go so far as to say that we should reject not only this proposal, but any future ones that intend to prevent superusers from doing things that superusers normally could do (and, indeed, are normally expected to do). That sort of thing is not part of our security model, never has been, and it's simply naive to believe that it won't have a boatload of easily-reachable holes in it. Which we *will* get complaints about, if we claim that thus-and-such feature prevents it. So why bother? Don't give out superuser to people you don't trust to not do the things you wish they wouldn't. > I also think that using the GUC system to manage itself is a little > bit suspect. Something like contrib/sepgsql would be a better mechanism, perhaps. regards, tom lane
On Tue, Jan 30, 2024 at 2:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Indeed. I'd go so far as to say that we should reject not only this > proposal, but any future ones that intend to prevent superusers from > doing things that superusers normally could do (and, indeed, are > normally expected to do). That sort of thing is not part of our > security model, never has been, and it's simply naive to believe that > it won't have a boatload of easily-reachable holes in it. Which we > *will* get complaints about, if we claim that thus-and-such feature > prevents it. So why bother? Don't give out superuser to people you > don't trust to not do the things you wish they wouldn't. In my opinion, we need to have the conversation, whereas you seem to want to try to shut it down before it starts. If we take that approach, people are going to get (more) frustrated. Also in my opinion, there is a fair amount of nuance here. On the one hand, I and others put a lot of work into making it possible to not give people superuser and still be able to do a controlled subset of the things that a superuser can do. For example, thanks to Mark Dilger's work, you can make somebody not a superuser and still allow them to set GUCs that can normally be set only by superusers, and you can choose which GUCs you do and do not want them to be able to set. And, thanks to my work, you can make someone a CREATEROLE user without letting them escalate to superuser, and you can then allow them to manage the users that they create almost exactly as if they were a superuser, with only the limitations that seem necessary to maintain system security. It is worth asking - and I would like to hear a real, non-flip answer - why someone who wants to do what is proposed here isn't using those mechanisms instead of handing out SUPERUSER and then complaining that it grants too much power. On the other hand, I don't see why it isn't legitimate to imagine a scenario where there is no security boundary between the Kubernetes administrator and the PostgreSQL DBA, and yet the PostgreSQL DBA should still be pushed in the direction of doing things in a way that doesn't break Kubernetes. It surprises me a little bit that Gabriele and others want to build the system that way, though, because you might expect that in a typical install the Kubernetes administrator would want to FORCIBLY PREVENT the PostgreSQL administrator from messing things up instead of doing what is proposed here, which amounts to suggesting perhaps the PostgreSQL administrator would be kind enough not to mess things up. Nonetheless, there's no law against suggestions. When my wife puts the ground beef that I'm supposed to use to cook dinner at the top of the freezer and the stuff I'm supposed to not use at the bottom, nothing prevents me from digging out the other ground beef and using it, but I don't, because I can take a hint. And indeed, I benefit from that hint. This seems like it could be construed as a very similar type of hint. I don't think we should pretend like one of the two paragraphs above is valid and the other is hot garbage. That's not solving anything. We can't resolve the tension between those two things in either direction by somebody hammering on the side of the argument that they believe to be correct and ignoring the other one. > Something like contrib/sepgsql would be a better mechanism, perhaps. There's nothing wrong with that exactly, but what does it gain us over my proposal of a sentinel file? I don't see much value in adding a hook and then a module that uses that hook to return false or unconditionally ereport. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Jan 30, 2024 at 2:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Indeed. I'd go so far as to say that we should reject not only this >> proposal, but any future ones that intend to prevent superusers from >> doing things that superusers normally could do (and, indeed, are >> normally expected to do). > Also in my opinion, there is a fair amount of nuance here. On the one > hand, I and others put a lot of work into making it possible to not > give people superuser and still be able to do a controlled subset of > the things that a superuser can do. Sure, and that is a line of thought that we should continue to pursue. But we already have enough mechanism to let a non-superuser set only the ALTER SYSTEM stuff she's authorized to. There is no reason to think that a non-superuser could break through that restriction at all, let alone easily. So that's an actual security feature, not security theater. I don't see how the feature proposed here isn't security theater, or at least close enough to that. >> Something like contrib/sepgsql would be a better mechanism, perhaps. > There's nothing wrong with that exactly, but what does it gain us over > my proposal of a sentinel file? I was imagining using selinux and/or sepgsql to directly prevent writing postgresql.auto.conf from the Postgres account. Combine that with a non-Postgres-owned postgresql.conf (already supported) and you have something that seems actually bulletproof, rather than a hint. Admittedly, using that approach requires knowing something about a non-Postgres security mechanism. regards, tom lane
On Tue, Jan 30, 2024 at 10:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > > There's nothing wrong with that exactly, but what does it gain us over > > my proposal of a sentinel file? > > I was imagining using selinux and/or sepgsql to directly prevent > writing postgresql.auto.conf from the Postgres account. Combine that > with a non-Postgres-owned postgresql.conf (already supported) and you > have something that seems actually bulletproof, rather than a hint. > Admittedly, using that approach requires knowing something about a > non-Postgres security mechanism. Wouldn't a simple "chattr +i postgresql.auto.conf" work? -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes: > On Tue, Jan 30, 2024 at 10:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I was imagining using selinux and/or sepgsql to directly prevent >> writing postgresql.auto.conf from the Postgres account. > Wouldn't a simple "chattr +i postgresql.auto.conf" work? Hmm, I'm not too familiar with that file attribute, but it looks like it'd work (on platforms that support it). My larger point here is that trying to enforce restrictions on superusers *within* Postgres is simply not a good plan, for largely the same reasons that Robert questioned making the GUC mechanism police itself. It needs to be done outside, either at the filesystem level or via some other kernel-level security system. regards, tom lane
On Tuesday, January 30, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
My larger point here is that trying to enforce restrictions on
superusers *within* Postgres is simply not a good plan, for
largely the same reasons that Robert questioned making the
GUC mechanism police itself. It needs to be done outside,
either at the filesystem level or via some other kernel-level
security system.
The idea of adding a file to the data directory appeals to me.
optional_runtime_features.conf
alter_system=enabled
copy_from_program=enabled
copy_to_program=disabled
If anyone tries to use disabled features the system emits an error:
ERROR: Cannot send copy output to program, action disabled by host.
My main usability question is whether restart required is an acceptable restriction.
Making said file owned by root (or equivalent) and only readable by the postgres process user suffices to lock it down. Refusing to start if the file is writable, and at least one feature is disabled can be considered, with a startup option to bypass that check if desired.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes: > On Tuesday, January 30, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> My larger point here is that trying to enforce restrictions on >> superusers *within* Postgres is simply not a good plan, for >> largely the same reasons that Robert questioned making the >> GUC mechanism police itself. It needs to be done outside, >> either at the filesystem level or via some other kernel-level >> security system. > The idea of adding a file to the data directory appeals to me. > > optional_runtime_features.conf > alter_system=enabled > copy_from_program=enabled > copy_to_program=disabled ... so, exactly what keeps an uncooperative superuser from overwriting that file? You cannot enforce such restrictions within Postgres. It has to be done by an outside mechanism. If you think different, you are mistaken. regards, tom lane
On Tuesday, January 30, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Tuesday, January 30, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My larger point here is that trying to enforce restrictions on
>> superusers *within* Postgres is simply not a good plan, for
>> largely the same reasons that Robert questioned making the
>> GUC mechanism police itself. It needs to be done outside,
>> either at the filesystem level or via some other kernel-level
>> security system.
> The idea of adding a file to the data directory appeals to me.
>
> optional_runtime_features.conf
> alter_system=enabled
> copy_from_program=enabled
> copy_to_program=disabled
... so, exactly what keeps an uncooperative superuser from
overwriting that file?
You cannot enforce such restrictions within Postgres.
It has to be done by an outside mechanism. If you think
different, you are mistaken.
If the only user on the OS that can modify that file is root, how does the superuser, who is hard coded to not be root, modify it? The root/admin user on the OS and it’s filesystem permissions is the outside mechanism being suggested here.
If the complaint is that the in-memory boolean is changeable by the superuser, or even the logic pertaining to the error branch of the code, then yes this is a lost cause.
But root prevents superuser from controlling that file and then that file can prevent the superuser from escaping to the operating system and leveraging its OS postgres user.
David J.
On 31.01.24 06:28, Tom Lane wrote: >> The idea of adding a file to the data directory appeals to me. >> >> optional_runtime_features.conf >> alter_system=enabled >> copy_from_program=enabled >> copy_to_program=disabled > ... so, exactly what keeps an uncooperative superuser from > overwriting that file? The point of this feature would be to keep the honest people honest. The first thing I did when ALTER SYSTEM came out however many years ago was to install Nagios checks to warn when postgresql.auto.conf exists. Because the thing is an attractive nuisance, especially when you want to do centralized configuration control. Of course you can bypass it using COPY PROGRAM etc., but then you *know* that you are *bypassing* something. If you just see ALTER SYSTEM, you'll think, "that is obviously the appropriate tool", and there is no generally accepted way to communicate that, in particular environment, it might not be.
Hi there,
I very much like the idea of a file in the data directory that also controls the copy operations.
Just wanted to highlight though that in our operator we have already applied the read-only postgresql.auto.conf trick to disable the system (see https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system). However, having that file read-only triggered an issue when using pg_rewind to resync a former primary, as pg_rewind immediately bails out when a read-only file is encountered in the PGDATA (see https://github.com/cloudnative-pg/cloudnative-pg/issues/3698).
We might keep this in mind if we go down the path of the separate file.
Thanks,
Gabriele
On Wed, 31 Jan 2024 at 08:43, Peter Eisentraut <peter@eisentraut.org> wrote:
On 31.01.24 06:28, Tom Lane wrote:
>> The idea of adding a file to the data directory appeals to me.
>>
>> optional_runtime_features.conf
>> alter_system=enabled
>> copy_from_program=enabled
>> copy_to_program=disabled
> ... so, exactly what keeps an uncooperative superuser from
> overwriting that file?
The point of this feature would be to keep the honest people honest.
The first thing I did when ALTER SYSTEM came out however many years ago
was to install Nagios checks to warn when postgresql.auto.conf exists.
Because the thing is an attractive nuisance, especially when you want to
do centralized configuration control. Of course you can bypass it using
COPY PROGRAM etc., but then you *know* that you are *bypassing*
something. If you just see ALTER SYSTEM, you'll think, "that is
obviously the appropriate tool", and there is no generally accepted way
to communicate that, in particular environment, it might not be.
On Wed, Jan 31, 2024 at 12:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > You cannot enforce such restrictions within Postgres. > It has to be done by an outside mechanism. If you think > different, you are mistaken. It seems like the biggest reason why we can't enforce such restrictions with Postgres is that you won't hear of anyone committing any code which would let us enforce such restrictions in Postgres. I'm not saying that there's no other problem here, but you're just digging in your heels. I wrote upthread "We can't resolve the tension between those two things in either direction by somebody hammering on the side of the argument that they believe to be correct and ignoring the other one" and you replied to that by quoting what I said about the side of the argument that you believe and hammering on it some more. I really wish you wouldn't do stuff like that. One thing that I think might be helpful here is to address the question of exactly how the superuser can get general-purpose filesystem access. They can definitely do it if there are any untrusted PLs installed, but the person who configures the machine can control that. They can also do it if extensions like adminpack are available, but the server administrator can control that, too. They can do it through COPY TO/FROM PROGRAM, but we could provide a way to restrict that, and I think an awful lot of people want that. I don't know of any other "normal" way of getting filesystem access, but the superuser can also hack the system catalogs. That means they can create a function definition that tries to run an arbitrary function either in PostgreSQL itself or any .so they can get their hands on -- but this is a much less powerful technique since 5ded4bd21403e143dd3eb66b92d52732fdac1945 removed the version 0 calling convention. You can no longer manufacture calls to random C functions that aren't expecting to be called from SQL. The superuser can also arrange to call a function that *is* intended to be SQL-callable with the wrong argument types. It's not hard to manufacture a crash that way, because if you call a function that's expecting a varlena with an integer, you can induce the function to read more memory than intended and run right off the stack. I'm not quite sure whether this can be parlayed into arbitrary code execution; I think it's possible. And, then, of course, you can use ALTER SYSTEM to set archive_command or restore_command or similar to a shell command of your choosing. What else is there? We should actually document the whole list of ways that a superuser can escape the sandbox. Because right now there are tons of people, even experienced PG users, who think that superusers can't escape from PG at all, or that it's just about COPY TO/FROM PROGRAM. The lack of clarity about what the issues are makes intelligent discussion difficult. Our documentation hints at the fact that there's no privilege boundary between the superuser and the OS user, but it's not very clear or very detailed or in any very central place, and it's not surprising that not everyone understands the situation correctly. At any rate, unless there are way more ways to get filesystem access than what I've listed here, it's not unreasonable for people to want to shut off the most obvious ones, like COPY TO/FROM PROGRAM and ALTER SYSTEM. And there's no real reason we can't provide a way to do that. It's just sticking your head in the stand to say "well, because we can't prevent people from crafting a stack overrun attack to access the filesystem, we shouldn't have a feature that tells them ALTER SYSTEM is disabled on this instance." -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Jan 31, 2024 at 5:16 AM Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> wrote: > I very much like the idea of a file in the data directory that also controls the copy operations. > > Just wanted to highlight though that in our operator we have already applied the read-only postgresql.auto.conf trick todisable the system (see https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system). However,having that file read-only triggered an issue when using pg_rewind to resync a former primary, as pg_rewind immediatelybails out when a read-only file is encountered in the PGDATA (see https://github.com/cloudnative-pg/cloudnative-pg/issues/3698). > > We might keep this in mind if we go down the path of the separate file. Yeah. It would be possible to teach pg_rewind and other utilities to handle unreadable or unwritable files in the data directory, but I'm not sure that's the best path forward here, and it would require some consensus that it's the way we want to go. Another option I thought of would be to control these sorts of things with a command-line switch. I doubt whether that does anything really fundamental from a security point of view, but it removes the control of the toggles from anything in the data directory while still leaving it within the server administrator's remit. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Jan 30, 2024 at 04:25:12PM -0500, Robert Haas wrote: > I don't think we should pretend like one of the two paragraphs above > is valid and the other is hot garbage. That's not solving anything. We > can't resolve the tension between those two things in either direction > by somebody hammering on the side of the argument that they believe to > be correct and ignoring the other one. What if we generate log messages when certain commands are used, like ALTER TABLE? We could have GUC which controls which commands are logged. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, Feb 1, 2024 at 7:33 AM Bruce Momjian <bruce@momjian.us> wrote: > On Tue, Jan 30, 2024 at 04:25:12PM -0500, Robert Haas wrote: > > I don't think we should pretend like one of the two paragraphs above > > is valid and the other is hot garbage. That's not solving anything. We > > can't resolve the tension between those two things in either direction > > by somebody hammering on the side of the argument that they believe to > > be correct and ignoring the other one. > > What if we generate log messages when certain commands are used, like > ALTER TABLE? We could have GUC which controls which commands are > logged. Well, as I understand it, that doesn't solve the problem here. The problem some people want to solve here seems to be: On my system, the PostgreSQL configuration parameters are being managed by $EXTERNAL_TOOL. Therefore, they should not be managed by PostgreSQL itself. Therefore, if someone uses ALTER SYSTEM, they've made a mistake, so we should give them an ERROR telling them that, like: ERROR: you're supposed to update the configuration via k8s, not ALTER SYSTEM, you dummy! DETAIL: Stop being an idiot. The exact message text might need some wordsmithing. :-) -- Robert Haas EDB: http://www.enterprisedb.com
On 31.01.24 11:16, Gabriele Bartolini wrote: > I very much like the idea of a file in the data directory that also > controls the copy operations. > > Just wanted to highlight though that in our operator we have already > applied the read-only postgresql.auto.conf trick to disable the system > (see > https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system <https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system>).However, having that file read-onlytriggered an issue when using pg_rewind to resync a former primary, as pg_rewind immediately bails out when a read-onlyfile is encountered in the PGDATA (see https://github.com/cloudnative-pg/cloudnative-pg/issues/3698 <https://github.com/cloudnative-pg/cloudnative-pg/issues/3698>). > > We might keep this in mind if we go down the path of the separate file. How about ALTER SYSTEM is disabled if the file postgresql.auto.conf.disabled exists? This is somewhat similar to making the file read-only, but doesn't risk other tools breaking when they encounter such a file. And it's more obvious and self-explaining.
On Tue, Feb 6, 2024 at 7:10 AM Peter Eisentraut <peter@eisentraut.org> wrote:
How about ALTER SYSTEM is disabled if the file
postgresql.auto.conf.disabled exists? This is somewhat similar to making
the file read-only, but doesn't risk other tools breaking when they
encounter such a file. And it's more obvious and self-explaining.
A separate configuration file would be self-documenting and able to always exist; the same properties as postgres.conf
ISTM the main requirement regardless of how the file system API is designed - assuming there is a filesystem API - is that the running postgres process be unable to write to the file. It seems immaterial how the OS admin accomplishes that goal.
The command line argument method seems appealing but it seems harder in that case to ensure that the postgres process be disallowed from modifyIng whatever file defines what should be run.
One concern with a file configuration is that if we require it to be present in the data directory that goes somewhat against the design of allowing configuration files to be placed anywhere by changing the config_file guc.
Any design should factor in the almost immediate need to be extended to prevent copy variants that touch the local filesystem or shell directly.
I was pondering a directory in pgdata where you could add *.disabled files indicating which features to disable. This is a bit more pluggable than a single configuration file but the later still seems better to me.
David J.
On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote: > I also think that using the GUC system to manage itself is a little > bit suspect. I wonder if it would be better to do this some other way, > e.g. a sentinel file in the data directory. For example, suppose we > refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or > something like that. On Tue, 6 Feb 2024 at 15:10, Peter Eisentraut <peter@eisentraut.org> wrote: > How about ALTER SYSTEM is disabled if the file > postgresql.auto.conf.disabled exists? This is somewhat similar to making > the file read-only, but doesn't risk other tools breaking when they > encounter such a file. And it's more obvious and self-explaining. I'm not convinced we need a new file to disable ALTER SYSTEM. I feel like an "enable_alter_system" GUC that defaults to ON would work fine for this. If we make that GUC be PGC_POSTMASTER then an operator can disable ALTER SYSTEM either with a command line argument or by changing the main config file. Since this feature is mostly useful when the config file is managed by an external system, it seems rather simple for that system to configure one extra GUC in the config file.
On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote: > ``` > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf": Permission denied > ``` +1 to simply mark postgresql.auto.conf file as not being writeable. To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendlyhint? ``` postgres=# ALTER SYSTEM SET wal_level TO minimal; ERROR: could not open file "postgresql.auto.conf": Permission denied HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only. ``` On Fri, Sep 8, 2023, at 23:43, Magnus Hagander wrote: > We need a "allowlist" of things a user can do, rather than a blocklist > of "they can do everything they can possibly think of and a computer > is capable of doing, except for this one specific thing". Blocklisting > individual permissions of a superuser will never be secure. +1 for preferring an "allowlist" approach over a blocklist. In a way, I think this is similar to the project's philosophy on Query Hints, which I strongly support as I think it leadsto a better PostgreSQL over the long term. It creates a crucial feedback loop between users facing query planner issuesand our developer community, providing essential insights for enhancing the Query Planner. If users were to simply apply Query Hints as a quick fix instead of reporting underlying problems, we would often lose thesevaluable opportunities for improvement of the Query Planner. Similarly, I think it's crucial to identify functionalities that currently require superuser privileges and cannot yet beexplicitly granted to non-superusers. /Joel
On 06.02.24 16:22, Jelte Fennema-Nio wrote: > On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote: >> I also think that using the GUC system to manage itself is a little >> bit suspect. I wonder if it would be better to do this some other way, >> e.g. a sentinel file in the data directory. For example, suppose we >> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or >> something like that. > > On Tue, 6 Feb 2024 at 15:10, Peter Eisentraut <peter@eisentraut.org> wrote: >> How about ALTER SYSTEM is disabled if the file >> postgresql.auto.conf.disabled exists? This is somewhat similar to making >> the file read-only, but doesn't risk other tools breaking when they >> encounter such a file. And it's more obvious and self-explaining. > > I'm not convinced we need a new file to disable ALTER SYSTEM. I feel > like an "enable_alter_system" GUC that defaults to ON would work fine > for this. If we make that GUC be PGC_POSTMASTER then an operator can > disable ALTER SYSTEM either with a command line argument or by > changing the main config file. Since this feature is mostly useful > when the config file is managed by an external system, it seems rather > simple for that system to configure one extra GUC in the config file. Yeah, I'm all for that, but some others didn't like handling this in the GUC system, so I'm throwing around other ideas.
Hi Jelte,
On Tue, 6 Feb 2024 at 16:22, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
I'm not convinced we need a new file to disable ALTER SYSTEM. I feel
like an "enable_alter_system" GUC that defaults to ON would work fine
for this. If we make that GUC be PGC_POSTMASTER then an operator can
disable ALTER SYSTEM either with a command line argument or by
changing the main config file. Since this feature is mostly useful
when the config file is managed by an external system, it seems rather
simple for that system to configure one extra GUC in the config file.
This is mostly the approach I have taken in the patch, except allowing to change the value in the configuration file. The patch at the moment was enforcing just the setting at startup (which is more than enough for a Kubernetes operator given that Postgres runs in the container). I had done some experiments enabling the change in the configuration file, but wasn't sure in which `config_group` to place the 'enable_alter_system` GUC, based on the src/include/utils/guc_tables.h. Any thoughts/hints?
Hi Joel,
On Wed, 7 Feb 2024 at 10:00, Joel Jacobson <joel@compiler.org> wrote:
On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote:
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR: could not open file "postgresql.auto.conf": Permission denied
> ```
+1 to simply mark postgresql.auto.conf file as not being writeable.
To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendly hint?
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only.
```
This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA.
Cheers,
Gabriele
On Wed, 7 Feb 2024 at 11:16, Peter Eisentraut <peter@eisentraut.org> wrote: > On 06.02.24 16:22, Jelte Fennema-Nio wrote: > > On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote: > >> I also think that using the GUC system to manage itself is a little > >> bit suspect. I wonder if it would be better to do this some other way, > >> e.g. a sentinel file in the data directory. For example, suppose we > >> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or > >> something like that. > > I'm not convinced we need a new file to disable ALTER SYSTEM. I feel > > like an "enable_alter_system" GUC that defaults to ON would work fine > > for this. If we make that GUC be PGC_POSTMASTER then an operator can > > disable ALTER SYSTEM either with a command line argument or by > > changing the main config file. Since this feature is mostly useful > > when the config file is managed by an external system, it seems rather > > simple for that system to configure one extra GUC in the config file. > > Yeah, I'm all for that, but some others didn't like handling this in the > GUC system, so I'm throwing around other ideas. Okay, then we're agreeing here. Reading back the email thread the only argument against GUCs that I could find was Robert thinking it is a "a little bit suspect" to let the GUC system manage itself. This would not be the first time we're doing that though, the same is true for "config_file" and "data_directory" (which even needed the introduction of GUC_DISALLOW_IN_AUTO_FILE). So, I personally would like to hear some other options before we start entertaining some new ways of configuring Postgres its behaviour (even the read-only postgresql.auto.conf seems quite strange to me).
On Wednesday, February 7, 2024, Joel Jacobson <joel@compiler.org> wrote:
On Fri, Sep 8, 2023, at 23:43, Magnus Hagander wrote:
> We need a "allowlist" of things a user can do, rather than a blocklist
> of "they can do everything they can possibly think of and a computer
> is capable of doing, except for this one specific thing". Blocklisting
> individual permissions of a superuser will never be secure.
+1 for preferring an "allowlist" approach over a blocklist.
The status quo is allow everything so while the theory is nice it seems that requiring it to be allowlist is just going to scare anyone off of actually improving matters.
Also, this isn’t necessarily about blocking the superuser, it is about effectively disabling features deemed undesirable at runtime. All features enabled by default seems like a valid policy.
While the only features likely to be disabled are those involving someone’s definition of security the security policy is still that superuser can do everything the system is capable of doing.
David J.
On Wed, 7 Feb 2024 at 11:35, Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> wrote: > This is mostly the approach I have taken in the patch, except allowing to change the value in the configuration file. (I had missed the patch in the long thread). I think it would be nice to have this be PGC_SIGHUP, and set GUC_DISALLOW_IN_AUTO_FILE. That way this behaviour can be changed without shutting down postgres (but not with ALTER SYSTEM, because that seems confusing). > but wasn't sure in which `config_group` to place the 'enable_alter_system` GUC, based on the src/include/utils/guc_tables.h.Any thoughts/hints? I agree that none of the existing groups fit particularly well. I see a few options: 1. Create a new group (maybe something like "Administration" or "Enabled Features") 2. Use FILE_LOCATIONS, which seems sort of related at least. 3. Instead of adding an "enable_alter_system" GUC we would add an "auto_config_file" guc (and use the FILE_LOCATIONS group). Then if a user sets "auto_config_file" to an empty string, we would disable the auto config file and thus ALTER SYSTEM. I'd prefer 1 or 3 I think. I kinda like option 3 for its consistency of being able to configure other config file locations, but I think that would be quite a bit more work, and I'm not sure how useful it is to change the location of the auto file.
On Wed, Feb 7, 2024, at 10:49 AM, Jelte Fennema-Nio wrote:
On Wed, 7 Feb 2024 at 11:35, Gabriele Bartolini<gabriele.bartolini@enterprisedb.com> wrote:> This is mostly the approach I have taken in the patch, except allowing to change the value in the configuration file.(I had missed the patch in the long thread). I think it would be niceto have this be PGC_SIGHUP, and set GUC_DISALLOW_IN_AUTO_FILE. Thatway this behaviour can be changed without shutting down postgres (butnot with ALTER SYSTEM, because that seems confusing).
Based on Gabriele's use case (Kubernetes) and possible others like a cloud
vendor, I think it should be more restrictive not permissive. I mean,
PGC_POSTMASTER and *only* allow this GUC to be from command-line. (I don't
inspect the code but maybe setting GUC_DISALLOW_IN_FILE is not sufficient to
accomplish this goal.) The main advantages of the GUC system are (a) the
setting is dynamically assigned during startup and (b) you can get the current
setting via SQL.
Another idea is to set it per cluster during initdb like data checksums. You
don't rely on the GUC system but store this information into pg_control. I
think for the referred use cases, you will never have to change it but you can
have a mechanism to change it.
On 2024-02-07 We 05:37, Gabriele Bartolini wrote:
Hi Joel,On Wed, 7 Feb 2024 at 10:00, Joel Jacobson <joel@compiler.org> wrote:On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote:
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR: could not open file "postgresql.auto.conf": Permission denied
> ```
+1 to simply mark postgresql.auto.conf file as not being writeable.
To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendly hint?
```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only.
```This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA.
This seems like the simplest solution. And maybe we should be fixing pg_rewind regardless of this issue?
cheers
andrew
--
Andrew Dunstan EDB: https://www.enterprisedb.com
On Sat, Feb 10, 2024 at 9:47 PM Andrew Dunstan <andrew@dunslane.net> wrote: >> To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendlyhint? >> >> ``` >> postgres=# ALTER SYSTEM SET wal_level TO minimal; >> ERROR: could not open file "postgresql.auto.conf": Permission denied >> HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only. >> ``` > > This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA. > > This seems like the simplest solution. And maybe we should be fixing pg_rewind regardless of this issue? Is it just pg_rewind? What about pg_basebackup, for example? Will it preserve permissions on that file in both directory and tar-format mode? Will any of the other tools that access the data directory via the filesystem care about this? What about third-party backup tools, or other third-party tools that access the data directory? I think in general we've taken the approach so far of basically making everything in the data directory have the same permissions as each other, and overall either everything is only user-accessible, or it's also group-readable, and there's a fair amount of code in various places that assumes these things are true. What I like about using a sentinel file for this -- I like Peter's suggestion of postgresql.auto.conf.disabled -- is that it keeps that property that our tools and third-party tools mostly don't need to care about file permissions, because they're all uniform. I think it may be simpler in the long run if we stick with that idea. I suspect that if we deviate from it we'll slowly find bugs here and there and have to add special-case logic in various unanticipated places to compensate. We can also make a GUC work, if people prefer that approach. If we go that route, the suggestion of making it PGC_SIGHUP and GUC_DISALLOW_IN_AUTO_FILE is a good one. When I earlier referred to managing the GUC system with GUCs as "suspect," what I really meant was that (1) there shouldn't be an easy way to make an end run around the thing that's disabling ALTER SYSTEM and (2) you shouldn't be able to use ALTER SYSTEM to disable ALTER SYSTEM. It sounds like those flags might be strong enough to prevent that. If it turns out they're not we can always add more flags. It's not entirely clear to me what our wider vision is here. Some people seem to want a whole series of flags that can disable various things that the superuser might otherwise be able to do, which is fine with me, except that we have no plan to disable all of the things a superuser can do to get filesystem/OS access, and I don't think it's possible to construct such a plan. To do so, we'd have to lock down the superuser account to the point where it can't create functions written in any untrusted procedural language -- in particular, C functions -- which would preclude installing most extensions; and we'd also have to forbid direct access to the catalogs. I think those kinds of restrictions are basically untenable. A service provider might not want a customer to have the ability to do those kinds of things, but some user must retain those capabilities, at the very least to handle emergencies. So, the solution there seems to be for the service provider to be the superuser and the customer to not be the super-user, rather than for the service provider and the customer to both be super-user but with some attempt at sandboxing. I'm not trying to kill this particular proposal, which I think is broadly reasonable, but I'm still uncomfortable with the fact that it looks a lot like pseudo-security. -- Robert Haas EDB: http://www.enterprisedb.com
On Sun, Feb 11, 2024, at 14:58, Robert Haas wrote: > It's not entirely clear to me what our wider vision is here. Some > people seem to want a whole series of flags that can disable various > things that the superuser might otherwise be able to do, Yes, that's what bothers me a little with the idea of a special fix for this special case. On Thu, Sep 7, 2023, at 22:27, Tom Lane wrote: > If you nonetheless feel that that's a good idea for your use case, > you can implement the restriction with an event trigger or the like. On Fri, Sep 15, 2023, at 11:18, Daniel Gustafsson wrote: >> On 11 Sep 2023, at 15:50, Magnus Hagander <magnus@hagander.net> wrote: >> >> On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >>> >>> On 2023-Sep-08, Magnus Hagander wrote: >>> >>>> Now, it might be that you don't care at all about the *security* side >>>> of the feature, and only care about the convenience side. But in that >>>> case, the original suggestion from Tom of using an even trigger seems >>>> like a fine enough solution? >>> >>> ALTER SYSTEM, like all system-wide commands, does not trigger event >>> triggers. These are per-database only. >>> >>> https://www.postgresql.org/docs/16/event-trigger-matrix.html >> >> Hah, didn't think of that. And yes, that's a very good point. But one >> way to fix that would be to actually make event triggers for system >> wide commands, which would then be useful for other things as well... > > Wouldn't having system wide EVTs be a generic solution which could be the > infrastructure for this requested change as well as others in the same area? +1 I like the wider vision of providing the necessary infrastructure to provide a solution for the general case. /Joel
On Tue, Feb 13, 2024 at 2:05 AM Joel Jacobson <joel@compiler.org> wrote: > > Wouldn't having system wide EVTs be a generic solution which could be the > > infrastructure for this requested change as well as others in the same area? > > +1 > > I like the wider vision of providing the necessary infrastructure to provide a solution for the general case. We don't seem to be making much progress here. As far as I can see from reading the thread, most people agree that it's reasonable to have some way to disable ALTER SYSTEM, but there are at least six competing ideas about how to do that: 1. command-line option 2. GUC 3. event trigger 4. extension 5. sentinel file 6. remove permissions on postgresql.auto.conf As I see it, (5) or (6) are most convenient for the system administrator, since they let that person make changes without needing to log into the database or, really, worry very much about the database's usual configuration mechanisms at all, and (5) seems like less work to implement than (6), because (6) probably breaks a bunch of client tools in weird ways that might not be easy for us to discover during patch review. (1) doesn't allow changing things at runtime, and might require the system administrator to fiddle with the startup scripts, which seems like it could be inconvenient. (2) and (3) seem like they put the superuser in a position to easily reverse a policy about what the superuser ought to do, but in the case of (2), that can be mitigated if the GUC can only be set in postgresql.conf and not elsewhere. (4) has no real advantages except for allowing core to maintain the fiction that we don't support this while actually supporting it; I think we should reject that approach outright. So what I'd like to see is a patch that implements (5), or in the alternative (2) but with the GUC being PGC_SIGHUP and GUC_DISALLOW_IN_AUTO_FILE. I believe there would be adequate consensus to proceed with either of those approaches. Anybody feel like coding it up? -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, 14 Mar 2024 at 17:37, Robert Haas <robertmhaas@gmail.com> wrote: > or in the > alternative (2) but with the GUC being PGC_SIGHUP and > GUC_DISALLOW_IN_AUTO_FILE. I believe there would be adequate consensus > to proceed with either of those approaches. Anybody feel like coding > it up? Here is a slightly modified version of Gabrielle his original patch, which already implemented GUC approach. The changes I made are adding PGC_SIGHUP and GUC_DISALLOW_IN_AUTO_FILE as well as adding some more docs.
Вложения
Robert Haas <robertmhaas@gmail.com> writes: > As far as I can see from reading the thread, most people agree that > it's reasonable to have some way to disable ALTER SYSTEM, but there > are at least six competing ideas about how to do that: > 1. command-line option > 2. GUC > 3. event trigger > 4. extension > 5. sentinel file > 6. remove permissions on postgresql.auto.conf With the possible exception of #1, every one of these is easily defeatable by an uncooperative superuser. I'm not excited about adding a "security" feature with such obvious holes in it. We reverted MAINTAIN last year for much less obvious holes; how is it that we're going to look the other way on this one? #2 with the GUC_DISALLOW_IN_AUTO_FILE flag can be made secure (I think) by putting the main postgresql.conf file outside the data directory and then making it not owned by or writable by the postgres user. But I doubt that's a common configuration, and I'm sure we will get complaints from people who failed to set it up that way. The proposed patch certainly doesn't bother to document the hazard. Really we'd need to do something about removing superusers' access to the filesystem in order to build something with fewer holes. I'm not against inventing such a feature, but it'd take a fair amount of work and likely would end in a noticeably less usable system (no plpython for example). regards, tom lane
On Thu, Mar 14, 2024 at 3:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > With the possible exception of #1, every one of these is easily > defeatable by an uncooperative superuser. I'm not excited about > adding a "security" feature with such obvious holes in it. > We reverted MAINTAIN last year for much less obvious holes; > how is it that we're going to look the other way on this one? We're going to document that it's not a security feature along the lines of what Magnus suggested in http://postgr.es/m/CABUevEx9m=CV8=WpXVW+rtVVs858kDJ6YpRkExV7n+F6MK05CQ@mail.gmail.com And then maybe someday we'll do this: > Really we'd need to do something about removing superusers' > access to the filesystem in order to build something with > fewer holes. I'm not against inventing such a feature, > but it'd take a fair amount of work and likely would end > in a noticeably less usable system (no plpython for example). Yep. It would be useful if you replied to the portion of http://postgr.es/m/CA+TgmoasUgkZ27x0XZH4EdmQ_b6JbRT6cSUxf+pHdgj-ESk_zA@mail.gmail.com where I enumerate the methods that I know about for the superuser to get filesystem access. I don't think it's going to be practical to block all of those methods in a single commit, and I'm not entirely convinced that we can ever close all the holes without compromising the superuser's ability to do necessary system administration tasks, but maybe it's possible, and documenting the list of such methods would make it a lot easier for users to understand the risks and hackers to pick problems to try to tackle. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Mar 14, 2024 at 3:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> With the possible exception of #1, every one of these is easily >> defeatable by an uncooperative superuser. I'm not excited about >> adding a "security" feature with such obvious holes in it. > We're going to document that it's not a security feature along the > lines of what Magnus suggested in > http://postgr.es/m/CABUevEx9m=CV8=WpXVW+rtVVs858kDJ6YpRkExV7n+F6MK05CQ@mail.gmail.com The patch-of-record contains no such wording. And if this isn't a security feature, then what is it? If you have to say to your (super) users "please don't mess with the system configuration", you might as well just trust them not to do it the easy way as not to do it the hard way. If they're untrustworthy, why have they got superuser? What I think this is is a loaded foot-gun painted in kid-friendly colors. People will use it and then file CVEs about how it did not turn out to be as secure as they imagined (probably without reading the documentation). regards, tom lane
On Thu, Mar 14, 2024 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > The patch-of-record contains no such wording. I plan to fix that, if nobody else beats me to it. > And if this isn't a > security feature, then what is it? If you have to say to your > (super) users "please don't mess with the system configuration", > you might as well just trust them not to do it the easy way as not > to do it the hard way. If they're untrustworthy, why have they > got superuser? I mean, I feel like this question has been asked and answered before, multiple times, on this thread. If you sincerely don't understand the use case, I can try again to explain it. But somehow I feel like it's more that you just don't like the idea, which is fair, but it seems like a considerable number of people feel otherwise. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Mar 14, 2024 at 1:38 PM Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Mar 14, 2024 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > The patch-of-record contains no such wording. > > I plan to fix that, if nobody else beats me to it. > > > And if this isn't a > > security feature, then what is it? If you have to say to your > > (super) users "please don't mess with the system configuration", > > you might as well just trust them not to do it the easy way as not > > to do it the hard way. If they're untrustworthy, why have they > > got superuser? > > I mean, I feel like this question has been asked and answered before, > multiple times, on this thread. If you sincerely don't understand the > use case, I can try again to explain it. But somehow I feel like it's > more that you just don't like the idea, which is fair, but it seems > like a considerable number of people feel otherwise. I know I'm jumping into a long thread here, but I've been following it out of interest. I'm sympathetic to the use case, since I used to work at a Postgres cloud provider, and while our system intentionally did not give our end users superuser privileges, I can imagine other managed environments where that's not an issue. I'd like to give answering this question again a shot, because I think this has been a persistent misunderstanding in this thread, and I don't think it's been made all that clear. It's not a security feature: it's a usability feature. It's a usability feature because, when Postgres configuration is managed by an outside mechanism (e.g., as in a Kubernetes environment), ALTER SYSTEM currently allows a superuser to make changes that appear to work, but may be discarded at some point in the future when that outside mechanism updates the config. They may also be represented incorrectly in a management dashboard if that dashboard is based on the values in the outside configuration mechanism, rather than values directly from Postgres. In this case, the end user with access to Postgres superuser privileges presumably also has access to the outside configuration mechanism. The goal is not to prevent them from changing settings, but to offer guard rails that prevent them from changing settings in a way that will be unstable (revertible by a future update) or confusing (not showing up in a management UI). There are challenges here in making sure this is _not_ seen as a security feature. But I do think the feature itself is sensible and worthwhile. Thanks, Maciek
On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote: > It's not a security feature: it's a usability feature. > > It's a usability feature because, when Postgres configuration is > managed by an outside mechanism (e.g., as in a Kubernetes > environment), ALTER SYSTEM currently allows a superuser to make > changes that appear to work, but may be discarded at some point in the > future when that outside mechanism updates the config. They may also > be represented incorrectly in a management dashboard if that dashboard > is based on the values in the outside configuration mechanism, rather > than values directly from Postgres. > > In this case, the end user with access to Postgres superuser > privileges presumably also has access to the outside configuration > mechanism. The goal is not to prevent them from changing settings, but > to offer guard rails that prevent them from changing settings in a way > that will be unstable (revertible by a future update) or confusing > (not showing up in a management UI). > > There are challenges here in making sure this is _not_ seen as a > security feature. But I do think the feature itself is sensible and > worthwhile. This is what I would have said if I'd tried to offer an explanation, except you said it better than I would have done. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Mar 14, 2024 at 07:43:15PM -0400, Robert Haas wrote: > On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote: > > It's not a security feature: it's a usability feature. > > > > It's a usability feature because, when Postgres configuration is > > managed by an outside mechanism (e.g., as in a Kubernetes > > environment), ALTER SYSTEM currently allows a superuser to make > > changes that appear to work, but may be discarded at some point in the > > future when that outside mechanism updates the config. They may also > > be represented incorrectly in a management dashboard if that dashboard > > is based on the values in the outside configuration mechanism, rather > > than values directly from Postgres. > > > > In this case, the end user with access to Postgres superuser > > privileges presumably also has access to the outside configuration > > mechanism. The goal is not to prevent them from changing settings, but > > to offer guard rails that prevent them from changing settings in a way > > that will be unstable (revertible by a future update) or confusing > > (not showing up in a management UI). > > > > There are challenges here in making sure this is _not_ seen as a > > security feature. But I do think the feature itself is sensible and > > worthwhile. > > This is what I would have said if I'd tried to offer an explanation, > except you said it better than I would have done. I do think the docs need to clearly say this is not a security feature. In fact, I wonder if the ALTER SYSTEM error message should explain the GUC that is causing the failure. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, 14 Mar 2024 at 22:15, Maciek Sakrejda <m.sakrejda@gmail.com> wrote: > In this case, the end user with access to Postgres superuser > privileges presumably also has access to the outside configuration > mechanism. The goal is not to prevent them from changing settings, but > to offer guard rails that prevent them from changing settings in a way > that will be unstable (revertible by a future update) or confusing > (not showing up in a management UI). Great explanation! Attached is a much changed patch that updates to docs and code to reflect this. I particularly liked your use of the word "guard rail" as that reflects the intent of the feature very well IMO. So I included that wording in both the GUC group and the error code.
Вложения
> On 15 Mar 2024, at 03:58, Bruce Momjian <bruce@momjian.us> wrote: > > On Thu, Mar 14, 2024 at 07:43:15PM -0400, Robert Haas wrote: >> On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote: >>> It's not a security feature: it's a usability feature. >>> >>> It's a usability feature because, when Postgres configuration is >>> managed by an outside mechanism (e.g., as in a Kubernetes >>> environment), ALTER SYSTEM currently allows a superuser to make >>> changes that appear to work, but may be discarded at some point in the >>> future when that outside mechanism updates the config. They may also >>> be represented incorrectly in a management dashboard if that dashboard >>> is based on the values in the outside configuration mechanism, rather >>> than values directly from Postgres. >>> >>> In this case, the end user with access to Postgres superuser >>> privileges presumably also has access to the outside configuration >>> mechanism. The goal is not to prevent them from changing settings, but >>> to offer guard rails that prevent them from changing settings in a way >>> that will be unstable (revertible by a future update) or confusing >>> (not showing up in a management UI). >>> >>> There are challenges here in making sure this is _not_ seen as a >>> security feature. But I do think the feature itself is sensible and >>> worthwhile. >> >> This is what I would have said if I'd tried to offer an explanation, >> except you said it better than I would have done. > > I do think the docs need to clearly say this is not a security feature. A usability feature whose purpose is to guard against a superuser willingly acting against how the system is managed, or not even knowing how it is managed, does have a certain security feature smell. We've already had a few CVE's filed against usability features so I don't think Tom's fears are at all ungrounded. Another quirk for the documentation of this: if I disable ALTER SYSTEM I would assume that postgresql.auto.conf is no longer consumed, but it still is (and still need to be), so maybe "enable/disable" is the wrong choice of words? -- Daniel Gustafsson
On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson <daniel@yesql.se> wrote: > Another quirk for the documentation of this: if I disable ALTER SYSTEM I would > assume that postgresql.auto.conf is no longer consumed, but it still is (and > still need to be), so maybe "enable/disable" is the wrong choice of words? Updated the docs to reflect this quirk. But I kept the same name for the GUC for now, because I couldn't come up with a better name myself. If someone suggests a better name, I'm happy to change it though.
Вложения
On Fri, Mar 15, 2024 at 7:09 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson <daniel@yesql.se> wrote: > > Another quirk for the documentation of this: if I disable ALTER SYSTEM I would > > assume that postgresql.auto.conf is no longer consumed, but it still is (and > > still need to be), so maybe "enable/disable" is the wrong choice of words? > > Updated the docs to reflect this quirk. But I kept the same name for > the GUC for now, because I couldn't come up with a better name myself. > If someone suggests a better name, I'm happy to change it though. Hmm. So in this patch, we have a whole new kind of GUC - guard rails - of which enable_alter_system is the first member. Is that what we want? I would have been somewhat inclined to find an existing section of postgresql.auto.conf for this parameter, perhaps "platform and version compatibility". But if we're going to add a bunch of similar GUCs, maybe grouping them all together is the way to go. Even if that is what we're going to do, do we want to call them "guard rails"? I'm not sure I'd find that name terribly clear, as a user. We know what we mean right now because we're having a very active discussion about this topic, but it might not seem as clear to someone coming at it fresh. On balance, I'm disinclined to add a new category for this. If we get to a point where we have several of these and we want to break them out into a new category, we can do it then. Maybe by that time the naming will seem more clear, too. I also don't think it's good enough to just say that this isn't a security feature. Talk is cheap. I think we need to say why it's not a security feature. So my proposal is something like this, taking a bunch of text from Jelte's patch and some inspiration from Magnus's earlier remarks: == When <literal>enable_alter_system</literal> is set to <literal>off</literal>, an error is returned if the <command>ALTER SYSTEM</command> command is used. This parameter can only be set in the <filename>postgresql.conf</filename> file or on the server command line. The default value is <literal>on</literal>. Note that this setting cannot be regarded as a security feature. It only disables the <literal>ALTER SYSTEM</literal> command. It does not prevent a superuser from changing the configuration remotely using other means. A superuser has many ways of executing shell commands at the operating system level, and can therefore modify <literal>postgresql.auto.conf</literal> regardless of the value of this setting. The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal> in environments where <literal>PostgreSQL</literal>'s configuration is managed by some outside mechanism. In such environments, using <command>ALTER SYSTEM</command> to make configuration changes might appear to work, but then may be discarded at some point in the future when that outside mechanism updates the configuration. Setting this parameter to <literal>false</literal> can help to avoid such mistakes. == I agree with Daniel's comment that Tom's concerns about people filing CVEs are not without merit; indeed, I said the same thing in my first post to this thread. However, I also believe that's not a sufficient reason for rejecting a feature that many people seem to want. I think the root of this problem is that our documentation is totally unclear about the fact that we don't intend for there to be privilege separation between the operating system user and the PostgreSQL superuser; people want there to be a distinction, and think there is. Hence CVE-2019-9193, for example. Several people, including me, wrote blog posts about how that's not a security vulnerability, but while I was researching mine, I went looking for where in the documentation we actually SAY that there's no privilege separation between the OS user and the superuser. The only mention I found at the time was the PL/perlu documentation, which said this: "The writer of a PL/PerlU function must take care that the function cannot be used to do anything unwanted, since it will be able to do anything that could be done by a user logged in as the database administrator." That statement, from the official documentation, in my mind at least, DOES confirm that we don't intend privilege separation, but it's really oblique. You have to think through the fact that the superuser has to be the one to install plperlu, and that plperlu functions can usurp the OS user; since both of those things are documented to be the case, it follows that we know and expect that the superuser can usurp the OS user. But someone who is wondering how PostgreSQL's security model works is not going to read the plperlu documentation and make the inferences I just described. It's crazy to me that a principle frequently cited as gospel on this mailing list and others is nearly undocumented. Obviously, even if we did document it clearly, people could still get confused (or just disagree with our position) and file CVEs anyway, but we're not helping our case by having nothing to cite. A difficulty is where to PUT such a mention in the documentation. There's not a single section title in the top-level documentation index that includes the word "security". Perhaps figuring out how to document this is best left to a separate thread, and there's also the question of whether a new section that talks about this also ought to talk about anything else. But I feel like we're way overdue to do something about this. -- Robert Haas EDB: http://www.enterprisedb.com
> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > my proposal is something like this, taking a > bunch of text from Jelte's patch and some inspiration from Magnus's > earlier remarks: I still think any wording should clearly mention that settings in the file are still applied. The proposed wording says to implicitly but to avoid confusion I think it should be explicit. > Perhaps figuring out how to > document this is best left to a separate thread, and there's also the > question of whether a new section that talks about this also ought to > talk about anything else. But I feel like we're way overdue to do > something about this. Seconded, both that it needs to be addressed and that it should be done on a separate thread from this one. -- Daniel Gustafsson
On Mon, 18 Mar 2024 at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > I would have been somewhat inclined to find an existing section > of postgresql.auto.conf for this parameter, perhaps "platform and > version compatibility". I tried to find an existing section, but I couldn't find any that this new GUC would fit into naturally. "Version and Platform Compatibility / Previous PostgreSQL Versions" (the one you suggested) seems wrong too. The GUCs there are to get back to Postgres behaviour from previous versions. So that section would only make sense if we'd turn enable_alter_system off by default (which obviously no-one in this thread suggests/wants). If you have another suggestion for an existing category that we should use, feel free to share. But imho, none of the existing ones are a good fit. > Even if that is what we're going to do, do we want to call them "guard > rails"? I'm not sure I'd find that name terribly clear, as a user. If anyone has a better suggestion, I'm happy to change it. On Mon, 18 Mar 2024 at 14:09, Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > > > my proposal is something like this, taking a > > bunch of text from Jelte's patch and some inspiration from Magnus's > > earlier remarks: > > I still think any wording should clearly mention that settings in the file are > still applied. The proposed wording says to implicitly but to avoid confusion > I think it should be explicit. I updated the first two paragraphs with Robert his wording (and did not remove the third one as that addresses the point made by Daniel)
Вложения
On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > > > my proposal is something like this, taking a > > bunch of text from Jelte's patch and some inspiration from Magnus's > > earlier remarks: > > I still think any wording should clearly mention that settings in the file are > still applied. The proposed wording says to implicitly but to avoid confusion > I think it should be explicit. I haven't kept up with the thread, but in general I'd prefer it to actually turn off parsing the file as well. I think just turning off the ability to change it -- including the ability to *revert* changes that were made to it before -- is going to be confusing. But, if we have decided it shouldn't do that, then IMHO we should consider naming it maybe enable_alter_system_command instead -- since we're only disabling the alter system command, not the actual feature in total. > > Perhaps figuring out how to > > document this is best left to a separate thread, and there's also the > > question of whether a new section that talks about this also ought to > > talk about anything else. But I feel like we're way overdue to do > > something about this. > > Seconded, both that it needs to be addressed and that it should be done on a > separate thread from this one. +1. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
> On 18 Mar 2024, at 16:34, Magnus Hagander <magnus@hagander.net> wrote: > > On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote: >> >>> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: >> >>> my proposal is something like this, taking a >>> bunch of text from Jelte's patch and some inspiration from Magnus's >>> earlier remarks: >> >> I still think any wording should clearly mention that settings in the file are >> still applied. The proposed wording says to implicitly but to avoid confusion >> I think it should be explicit. > > I haven't kept up with the thread, but in general I'd prefer it to > actually turn off parsing the file as well. I think just turning off > the ability to change it -- including the ability to *revert* changes > that were made to it before -- is going to be confusing. Wouldn't that break pgBackrest which IIRC write to .auto.conf directly without using ALTER SYSTEM? > But, if we have decided it shouldn't do that, then IMHO we should > consider naming it maybe enable_alter_system_command instead -- since > we're only disabling the alter system command, not the actual feature > in total. Good point. -- Daniel Gustafsson
On Mon, Mar 18, 2024 at 4:44 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 18 Mar 2024, at 16:34, Magnus Hagander <magnus@hagander.net> wrote: > > > > On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote: > >> > >>> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > >> > >>> my proposal is something like this, taking a > >>> bunch of text from Jelte's patch and some inspiration from Magnus's > >>> earlier remarks: > >> > >> I still think any wording should clearly mention that settings in the file are > >> still applied. The proposed wording says to implicitly but to avoid confusion > >> I think it should be explicit. > > > > I haven't kept up with the thread, but in general I'd prefer it to > > actually turn off parsing the file as well. I think just turning off > > the ability to change it -- including the ability to *revert* changes > > that were made to it before -- is going to be confusing. > > Wouldn't that break pgBackrest which IIRC write to .auto.conf directly > without using ALTER SYSTEM? Ugh of course. And not only that, it would also break pg_basebackup which does the same. So I guess that's not a good idea. I guess nobody anticipated this when that was done:) > > But, if we have decided it shouldn't do that, then IMHO we should > > consider naming it maybe enable_alter_system_command instead -- since > > we're only disabling the alter system command, not the actual feature > > in total. > > Good point. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On Mon, Mar 18, 2024 at 7:12 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > On Mon, 18 Mar 2024 at 13:57, Robert Haas <robertmhaas@gmail.com> wrote: > > I would have been somewhat inclined to find an existing section > > of postgresql.auto.conf for this parameter, perhaps "platform and > > version compatibility". > > I tried to find an existing section, but I couldn't find any that this > new GUC would fit into naturally. "Version and Platform Compatibility > / Previous PostgreSQL Versions" (the one you suggested) seems wrong > too. The GUCs there are to get back to Postgres behaviour from > previous versions. So that section would only make sense if we'd turn > enable_alter_system off by default (which obviously no-one in this > thread suggests/wants). > > If you have another suggestion for an existing category that we should > use, feel free to share. But imho, none of the existing ones are a > good fit. +1 on Version and Platform Compatibility. Maybe it just needs a new subsection there? This is for compatibility with a "deployment platform". The "Platform and Client Compatibility" subsection has just one entry, so a new subsection with also just one entry seems defensible, maybe just "Deployment Compatibility"? I think it's also plausible that there will be other similar settings for managed deployments in the future. > > Even if that is what we're going to do, do we want to call them "guard > > rails"? I'm not sure I'd find that name terribly clear, as a user. > > If anyone has a better suggestion, I'm happy to change it. No better suggestion at the moment, but while I used the term to explain the feature, I also don't think that's a great official name. For one thing, the section could easily be misinterpreted as guard rails for end-users who are new to Postgres. Also, I think it's more colloquial in tone than Postgres docs conventions. Further, I think we may want to change the GUC name itself. All the other GUCs that start with enable_ control planner behavior: maciek=# select name from pg_settings where name like 'enable_%'; name -------------------------------- enable_async_append enable_bitmapscan enable_gathermerge enable_hashagg enable_hashjoin enable_incremental_sort enable_indexonlyscan enable_indexscan enable_material enable_memoize enable_mergejoin enable_nestloop enable_parallel_append enable_parallel_hash enable_partition_pruning enable_partitionwise_aggregate enable_partitionwise_join enable_presorted_aggregate enable_seqscan enable_sort enable_tidscan (21 rows) Do we really want to break that pattern?
On Mon, Mar 18, 2024 at 11:46 AM Magnus Hagander <magnus@hagander.net> wrote: > > Wouldn't that break pgBackrest which IIRC write to .auto.conf directly > > without using ALTER SYSTEM? > > Ugh of course. And not only that, it would also break pg_basebackup > which does the same. > > So I guess that's not a good idea. I guess nobody anticipated this > when that was done:) I'm also +1 for the idea that the feature should only disable ALTER SYSTEM, not postgresql.auto.conf. I can't really see any reason why it needs to do both, and it might be more convenient if it didn't. If you're managing PostgreSQL's configuration externally, you might find it convenient to write the configuration you're managing into postgresql.auto.conf. Or you might want to write it to postgresql.conf. Or you might want to do something more complicated with include directives or whatever. But there's no reason why you *couldn't* want to use postgresql.auto.conf, and on the other hand I don't see how anyone benefits from that file not being read. That just seems confusing. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Mar 18, 2024 at 12:19 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote: > +1 on Version and Platform Compatibility. Maybe it just needs a new > subsection there? This is for compatibility with a "deployment > platform". The "Platform and Client Compatibility" subsection has just > one entry, so a new subsection with also just one entry seems > defensible, maybe just "Deployment Compatibility"? I think it's also > plausible that there will be other similar settings for managed > deployments in the future. Right, we're adding this because of environments like Kubernetes, which isn't a version, but it is a platform, or at least a deployment mode, which is why I thought of that section. I think for now we should just file this under "Other platforms and clients," which only has one existing setting. If the number of settings of this type grows, we can split it out. > Do we really want to break that pattern? Using enable_* as code for "this is a planner GUC" is a pretty stupid pattern, honestly, but I agree with you that it's long-established and we probably shouldn't deviate from it lightly. Perhaps just rename to allow_alter_system? -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Mar 18, 2024 at 10:27 AM Robert Haas <robertmhaas@gmail.com> wrote: > Right, we're adding this because of environments like Kubernetes, > which isn't a version, but it is a platform, or at least a deployment > mode, which is why I thought of that section. I think for now we > should just file this under "Other platforms and clients," which only > has one existing setting. If the number of settings of this type > grows, we can split it out. Fair enough, +1. > Using enable_* as code for "this is a planner GUC" is a pretty stupid > pattern, honestly, but I agree with you that it's long-established and > we probably shouldn't deviate from it lightly. Perhaps just rename to > allow_alter_system? +1
On Thu, Mar 14, 2024 at 12:37 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Feb 13, 2024 at 2:05 AM Joel Jacobson <joel@compiler.org> wrote: > > > Wouldn't having system wide EVTs be a generic solution which could be the > > > infrastructure for this requested change as well as others in the same area? > > > > +1 > > > > I like the wider vision of providing the necessary infrastructure to provide a solution for the general case. > > We don't seem to be making much progress here. > > As far as I can see from reading the thread, most people agree that > it's reasonable to have some way to disable ALTER SYSTEM, but there > are at least six competing ideas about how to do that: > > 1. command-line option > 2. GUC > 3. event trigger > 4. extension > 5. sentinel file > 6. remove permissions on postgresql.auto.conf > > As I see it, (5) or (6) are most convenient for the system > administrator, since they let that person make changes without needing > to log into the database or, really, worry very much about the > database's usual configuration mechanisms at all, and (5) seems like > less work to implement than (6), because (6) probably breaks a bunch > of client tools in weird ways that might not be easy for us to > discover during patch review. (1) doesn't allow changing things at > runtime, and might require the system administrator to fiddle with the > startup scripts, which seems like it could be inconvenient. (2) and > (3) seem like they put the superuser in a position to easily reverse a > policy about what the superuser ought to do, but in the case of (2), > that can be mitigated if the GUC can only be set in postgresql.conf > and not elsewhere. (4) has no real advantages except for allowing core > to maintain the fiction that we don't support this while actually > supporting it; I think we should reject that approach outright. > You know it's funny, you say #4 has no advantage and should be rejected outright, but AFAICT a) no one has actually laid out why it wouldn't work for them, b) and it's the one solution that can be implemented now c) and that implementation would be backwards compatible with some set of existing releases d) and certainly anyone running k8s or config management system would have the ability to install e) and it could be custom tailored to individual deployments as needed (including other potential commands that some systems might care about) f) and it seems like the least likely option to be mistaken for a security feature g) and also seems pretty safe wrt not breaking existing tooling (like 5/6 might do) Looking at it, you could make the argument that #4 is actually the best of the solutions proposed, except it has the one drawback that it requires folks to double down on the fiction that we think extensions are a good way to build solutions when really everyone just wants to have everything in core. Robert Treat https://xzilla.net
On Mon, Mar 18, 2024 at 4:07 PM Robert Treat <rob@xzilla.net> wrote: > You know it's funny, you say #4 has no advantage and should be > rejected outright, but AFAICT > > a) no one has actually laid out why it wouldn't work for them, > b) and it's the one solution that can be implemented now > c) and that implementation would be backwards compatible with some set > of existing releases > d) and certainly anyone running k8s or config management system would > have the ability to install > e) and it could be custom tailored to individual deployments as needed > (including other potential commands that some systems might care > about) > f) and it seems like the least likely option to be mistaken for a > security feature > g) and also seems pretty safe wrt not breaking existing tooling (like > 5/6 might do) > > Looking at it, you could make the argument that #4 is actually the > best of the solutions proposed, except it has the one drawback that it > requires folks to double down on the fiction that we think extensions > are a good way to build solutions when really everyone just wants to > have everything in core. I think that all of this is true except for (c). I think we'd need a new hook to make it work. That said, I think that extensions are a good way of implementing some functionality, but not this functionality. Extensions are a good approach when there's a bunch of stuff core can't know but an extension author can. For instance, the FDW interface caters to situations where the extension author knows how to access some data that PostgreSQL doesn't know how to access; and the operator class stuff is useful when the extension author knows how some user-defined data type should behave and we don't. But there's not really a substantial policy question here. All we do by pushing a feature like this out of core is wash our hands of it. Your (f) argues that might be a good thing, but I don't think so. When we know that a feature is widely-needed, it's better to have one good implementation of it in core than several perhaps not-so-good implementations out of core. That allows us to focus all of our efforts on that one implementation instead of splitting them across several -- which is the whole selling point of open source, really -- and it makes it easier for users who want the feature to get access to it. -- Robert Haas EDB: http://www.enterprisedb.com
Going to agree with Robert Treat here about an extension being a great solution. I resisted posting earlier as I wanted to see how this all pans out, but I wrote a quick little POC extension some months ago that does the disabling and works well (and cannot be easily worked around).
On Mon, Mar 18, 2024 at 4:59 PM Robert Haas <robertmhaas@gmail.com> wrote:
I think that all of this is true except for (c). I think we'd need a
new hook to make it work.
Seems we can just use ProcessUtility and:
if (IsA(parsetree, AlterSystemStmt) { ereport(ERROR, ...
When we know that a feature is
widely-needed, it's better to have one good implementation of it in
core than several perhaps not-so-good implementations out of core.
Meh, maybe. This one seems pretty dirt simple. Granted, I have expanded my original POC to allow *some* things to be changed by ALTER SYSTEM, but the original use case warrants a very small extension.
That allows us to focus all of our efforts on that one implementation
instead of splitting them across several -- which is the whole selling
point of open source, really -- and it makes it easier for users who
want the feature to get access to it.
Well, yeah, but they have to wait until version 18 at best, while an extension can run on any current version and probably be pretty future-proof as well.
Cheers,
Greg
I want to remind everyone of this from Gabriele's first message that started this thread: > At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked > by making the postgresql.auto.conf read only, but the returned message is > misleading and that’s certainly bad user experience (which is very > important in a cloud native environment): > > > ``` > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf": Permission denied > ``` I think making the config file read-only is a fine solution. If you don't want postgres to mess with the config files, forbid it with the permission system. Problems with pg_rewind, pg_basebackup were mentioned with that approach. I think if you want the config files to be managed outside PostgreSQL, by kubernetes, patroni or whatever, it would be good for them to be read-only to the postgres user anyway, even if we had a mechanism to disable ALTER SYSTEM. So it would be good to fix the problems with those tools anyway. The error message is not great, I agree with that. Can we improve it? Maybe just add a HINT like this: postgres=# ALTER SYSTEM SET wal_level TO minimal; ERROR: could not open file "postgresql.auto.conf" for writing: Permission denied HINT: Configuration might be managed outside PostgreSQL Perhaps we could make that even better with a GUC though. I propose a GUC called 'configuration_managed_externally = true / false". If you set it to true, we prevent ALTER SYSTEM and make the error message more definitive: postgres=# ALTER SYSTEM SET wal_level TO minimal; ERROR: configuration is managed externally As a bonus, if that GUC is set, we could even check at server startup that all the configuration files are not writable by the postgres user, and print a warning or refuse to start up if they are. (Another way to read this proposal is to rename the GUC that's been discussed in this thread to 'configuration_managed_externally'. That makes it look less like a security feature, and describes the intended use case.) -- Heikki Linnakangas Neon (https://neon.tech)
On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I want to remind everyone of this from Gabriele's first message that
started this thread:
> At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked
> by making the postgresql.auto.conf read only, but the returned message is
> misleading and that’s certainly bad user experience (which is very
> important in a cloud native environment):
>
>
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR: could not open file "postgresql.auto.conf": Permission denied
> ```
I think making the config file read-only is a fine solution. If you
don't want postgres to mess with the config files, forbid it with the
permission system.
Problems with pg_rewind, pg_basebackup were mentioned with that
approach. I think if you want the config files to be managed outside
PostgreSQL, by kubernetes, patroni or whatever, it would be good for
them to be read-only to the postgres user anyway, even if we had a
mechanism to disable ALTER SYSTEM. So it would be good to fix the
problems with those tools anyway.
The error message is not great, I agree with that. Can we improve it?
Maybe just add a HINT like this:
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf" for writing:
Permission denied
HINT: Configuration might be managed outside PostgreSQL
Perhaps we could make that even better with a GUC though. I propose a
GUC called 'configuration_managed_externally = true / false". If you set
it to true, we prevent ALTER SYSTEM and make the error message more
definitive:
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: configuration is managed externally
As a bonus, if that GUC is set, we could even check at server startup
that all the configuration files are not writable by the postgres user,
and print a warning or refuse to start up if they are.
(Another way to read this proposal is to rename the GUC that's been
discussed in this thread to 'configuration_managed_externally'. That
makes it look less like a security feature, and describes the intended
use case.)
I agree with pretty much all of this.
cheers
andrew
On Mon, 18 Mar 2024 at 18:27, Robert Haas <robertmhaas@gmail.com> wrote: > I think for now we > should just file this under "Other platforms and clients," which only > has one existing setting. If the number of settings of this type > grows, we can split it out. Done. I also included a patch to rename COMPAT_OPTIONS_CLIENTS to COMPAT_OPTIONS_OTHER, since that enum variant naming doesn't match the new intent of the section. On Tue, 19 Mar 2024 at 10:26, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > (Another way to read this proposal is to rename the GUC that's been > discussed in this thread to 'configuration_managed_externally'. That > makes it look less like a security feature, and describes the intended > use case.) I like this idea of naming the GUC in such a way. I swapped the words a bit and went for externally_managed_configuration, since order matches other GUCs e.g. standard_conforming_strings. But if you feel strongly about the ordering of the words, I'm happy to change it back. For the errorcode I now went for ERRCODE_FEATURE_NOT_SUPPORTED, which seemed most fitting. On Tue, 19 Mar 2024 at 10:26, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > I want to remind everyone of this from Gabriele's first message that > started this thread: > > > At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked > > by making the postgresql.auto.conf read only, but the returned message is > > misleading and that’s certainly bad user experience (which is very > > important in a cloud native environment): > > > > > > ``` > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: could not open file "postgresql.auto.conf": Permission denied > > ``` > > I think making the config file read-only is a fine solution. If you > don't want postgres to mess with the config files, forbid it with the > permission system. > > Problems with pg_rewind, pg_basebackup were mentioned with that > approach. I think if you want the config files to be managed outside > PostgreSQL, by kubernetes, patroni or whatever, it would be good for > them to be read-only to the postgres user anyway, even if we had a > mechanism to disable ALTER SYSTEM. So it would be good to fix the > problems with those tools anyway. > > The error message is not great, I agree with that. Can we improve it? > Maybe just add a HINT like this: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf" for writing: > Permission denied > HINT: Configuration might be managed outside PostgreSQL > > > Perhaps we could make that even better with a GUC though. I propose a > GUC called 'configuration_managed_externally = true / false". If you set > it to true, we prevent ALTER SYSTEM and make the error message more > definitive: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: configuration is managed externally > > As a bonus, if that GUC is set, we could even check at server startup > that all the configuration files are not writable by the postgres user, > and print a warning or refuse to start up if they are. > > (Another way to read this proposal is to rename the GUC that's been > discussed in this thread to 'configuration_managed_externally'. That > makes it look less like a security feature, and describes the intended > use case.) > > -- > Heikki Linnakangas > Neon (https://neon.tech) >
Вложения
On 3/19/24 07:49, Andrew Dunstan wrote: > > > On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas <hlinnaka@iki.fi > <mailto:hlinnaka@iki.fi>> wrote: > > I want to remind everyone of this from Gabriele's first message that > started this thread: > > > At the moment, a possible workaround is that `ALTER SYSTEM` can > be blocked > > by making the postgresql.auto.conf read only, but the returned > message is > > misleading and that’s certainly bad user experience (which is very > > important in a cloud native environment): > > > > > > ``` > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: could not open file "postgresql.auto.conf": Permission denied > > ``` > > I think making the config file read-only is a fine solution. If you > don't want postgres to mess with the config files, forbid it with the > permission system. > > Problems with pg_rewind, pg_basebackup were mentioned with that > approach. I think if you want the config files to be managed outside > PostgreSQL, by kubernetes, patroni or whatever, it would be good for > them to be read-only to the postgres user anyway, even if we had a > mechanism to disable ALTER SYSTEM. So it would be good to fix the > problems with those tools anyway. > > The error message is not great, I agree with that. Can we improve it? > Maybe just add a HINT like this: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf" for writing: > Permission denied > HINT: Configuration might be managed outside PostgreSQL > > > Perhaps we could make that even better with a GUC though. I propose a > GUC called 'configuration_managed_externally = true / false". If you > set > it to true, we prevent ALTER SYSTEM and make the error message more > definitive: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: configuration is managed externally > > As a bonus, if that GUC is set, we could even check at server startup > that all the configuration files are not writable by the postgres user, > and print a warning or refuse to start up if they are. > > (Another way to read this proposal is to rename the GUC that's been > discussed in this thread to 'configuration_managed_externally'. That > makes it look less like a security feature, and describes the intended > use case.) > > > > > I agree with pretty much all of this. +1 me too. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Heikki Linnakangas <hlinnaka@iki.fi> writes: > Perhaps we could make that even better with a GUC though. I propose a > GUC called 'configuration_managed_externally = true / false". If you set > it to true, we prevent ALTER SYSTEM and make the error message more > definitive: > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: configuration is managed externally > As a bonus, if that GUC is set, we could even check at server startup > that all the configuration files are not writable by the postgres user, > and print a warning or refuse to start up if they are. I like this idea. The "bonus" is not optional though, because setting the files' ownership/permissions is the only way to be sure that the prohibition is even a little bit bulletproof. One small issue: how do we make that work on Windows? Have recent versions grown anything that looks like real file permissions? Another question is whether this should be one-size-fits-all for all the configuration files. I can imagine situations where you'd like to lock down postgresql[.auto].conf but not pg_hba.conf. But maybe that can wait for somebody to show up with a use-case. regards, tom lane
On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I like this idea. The "bonus" is not optional though, because > setting the files' ownership/permissions is the only way to be > sure that the prohibition is even a little bit bulletproof. I don't agree with this. The only "normal" way of modifying postgresql.auto.conf from within postgres is using ALTER SYSTEM, so simply disabling ALTER SYSTEM seems enough to me. > Another question is whether this should be one-size-fits-all for > all the configuration files. I can imagine situations where > you'd like to lock down postgresql[.auto].conf but not pg_hba.conf. > But maybe that can wait for somebody to show up with a use-case. Afaik there's no way to modify pg_hba.conf from within postgres, only read it. (except for COPY TO FILE/PROGRAM etc) So, I don't think we need to worry about this now. On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > Perhaps we could make that even better with a GUC though. I propose a > > GUC called 'configuration_managed_externally = true / false". If you set > > it to true, we prevent ALTER SYSTEM and make the error message more > > definitive: > > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: configuration is managed externally > > > As a bonus, if that GUC is set, we could even check at server startup > > that all the configuration files are not writable by the postgres user, > > and print a warning or refuse to start up if they are. > > I like this idea. The "bonus" is not optional though, because > setting the files' ownership/permissions is the only way to be > sure that the prohibition is even a little bit bulletproof. > > One small issue: how do we make that work on Windows? Have recent > versions grown anything that looks like real file permissions? > > Another question is whether this should be one-size-fits-all for > all the configuration files. I can imagine situations where > you'd like to lock down postgresql[.auto].conf but not pg_hba.conf. > But maybe that can wait for somebody to show up with a use-case. > > regards, tom lane
Jelte Fennema-Nio <postgres@jeltef.nl> writes: > On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I like this idea. The "bonus" is not optional though, because >> setting the files' ownership/permissions is the only way to be >> sure that the prohibition is even a little bit bulletproof. > I don't agree with this. The only "normal" way of modifying > postgresql.auto.conf from within postgres is using ALTER SYSTEM, so > simply disabling ALTER SYSTEM seems enough to me. I've said this repeatedly: it's not enough. The only reason we need any feature whatsoever is that somebody doesn't trust their database superusers to not try to modify the configuration. Given that requirement, merely disabling ALTER SYSTEM isn't a solution, it's a fig leaf that might fool incompetent auditors but no more. If you aren't willing to build a solution that blocks off mods using COPY TO FILE/PROGRAM and other readily-available-to-superusers tools (plpythonu for instance), I think you shouldn't bother asking for a feature at all. Just trust your superusers. regards, tom lane
On Tue, 19 Mar 2024 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I've said this repeatedly: it's not enough. The only reason we need > any feature whatsoever is that somebody doesn't trust their database > superusers to not try to modify the configuration. And as everyone else on this thread has said: It is enough. Because the point is not security, the point is hinting to a superuser that a workflow they know from other systems (or an ALTER SYSTEM command they copied from the internet) is not the intended way to modify their server configuration on the system they are currently working on. I feel like the docs and error message in the current active patch are very clear on that. If you think they are not clear, feel free to suggest what could clarify the intent of this feature. But at this point, it's really starting to seem to me like you're willingly trying to interpret this feature as a thing that it is not (i.e. a security feature).
On Tue, Mar 19, 2024 at 12:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
If you aren't willing to build a solution that blocks off mods
using COPY TO FILE/PROGRAM and other readily-available-to-superusers
tools (plpythonu for instance), I think you shouldn't bother asking
for a feature at all. Just trust your superusers.
There is a huge gap between using a well-documented standard tool like ALTER SYSTEM and going out of your way to modify the configuration files through trickery. I think we need to only solve the former as in "hey, please don't do that because your changes will be overwritten"
Cheers,
Greg
> On 19 Mar 2024, at 17:53, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > On Tue, 19 Mar 2024 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I've said this repeatedly: it's not enough. The only reason we need >> any feature whatsoever is that somebody doesn't trust their database >> superusers to not try to modify the configuration. > > And as everyone else on this thread has said: It is enough. Because > the point is not security, the point is hinting to a superuser that a > workflow they know from other systems (or an ALTER SYSTEM command they > copied from the internet) is not the intended way to modify their > server configuration on the system they are currently working on. Well. Protection against superusers randomly copying ALTER SYSTEM commands from the internet actually does turn this into a security feature =) -- Daniel Gustafsson
> On 19 Mar 2024, at 15:51, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Perhaps we could make that even better with a GUC though. I propose a >> GUC called 'configuration_managed_externally = true / false". If you set >> it to true, we prevent ALTER SYSTEM and make the error message more >> definitive: > >> postgres=# ALTER SYSTEM SET wal_level TO minimal; >> ERROR: configuration is managed externally > >> As a bonus, if that GUC is set, we could even check at server startup >> that all the configuration files are not writable by the postgres user, >> and print a warning or refuse to start up if they are. > > I like this idea. The "bonus" is not optional though, because > setting the files' ownership/permissions is the only way to be > sure that the prohibition is even a little bit bulletproof. Agreed, assuming we can solve the below.. > One small issue: how do we make that work on Windows? Have recent > versions grown anything that looks like real file permissions? -- Daniel Gustafsson
On Tue, Mar 19, 2024 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > Perhaps we could make that even better with a GUC though. I propose a > > GUC called 'configuration_managed_externally = true / false". If you set > > it to true, we prevent ALTER SYSTEM and make the error message more > > definitive: > > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: configuration is managed externally > > > As a bonus, if that GUC is set, we could even check at server startup > > that all the configuration files are not writable by the postgres user, > > and print a warning or refuse to start up if they are. > > I like this idea. The "bonus" is not optional though, because > setting the files' ownership/permissions is the only way to be > sure that the prohibition is even a little bit bulletproof. > > One small issue: how do we make that work on Windows? Have recent > versions grown anything that looks like real file permissions? Windows has had full ACL support since 1993. The easiest way to do what you're doing here is to just set a DENY permission on the postgres operating system user. //Magnus
Greg Sabino Mullane: > On Tue, Mar 19, 2024 at 12:05 PM Tom Lane <tgl@sss.pgh.pa.us > <mailto:tgl@sss.pgh.pa.us>> wrote: > > If you aren't willing to build a solution that blocks off mods > using COPY TO FILE/PROGRAM and other readily-available-to-superusers > tools (plpythonu for instance), I think you shouldn't bother asking > for a feature at all. Just trust your superusers. > > > There is a huge gap between using a well-documented standard tool like > ALTER SYSTEM and going out of your way to modify the configuration files > through trickery. I think we need to only solve the former as in "hey, > please don't do that because your changes will be overwritten" Recap: The requested feature is not supposed to be a security feature. It is supposed to prevent the admin from accidentally doing the wrong thing - but not from willfully doing the same through different means. This very much sounds like a "warning" - how about turning the feature into one? Have a GUC warn_on_alter_system = "<message>", which allows the kubernetes operator to set it to something like "hey, please don't do that because your changes will be overwritten. Use xyz operator instead.". This will hardly be taken as a security feature by anyone, but should essentially achieve what is asked for. A more sophisticated way would be to make that GUC throw an error, but have a syntax for ALTER SYSTEM to override this - i.e. similar to a --force flag. Best, Wolfgang
On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Mar 19, 2024 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > Perhaps we could make that even better with a GUC though. I propose a
> > GUC called 'configuration_managed_externally = true / false". If you set
> > it to true, we prevent ALTER SYSTEM and make the error message more
> > definitive:
>
> > postgres=# ALTER SYSTEM SET wal_level TO minimal;
> > ERROR: configuration is managed externally
>
> > As a bonus, if that GUC is set, we could even check at server startup
> > that all the configuration files are not writable by the postgres user,
> > and print a warning or refuse to start up if they are.
>
> I like this idea. The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.
>
> One small issue: how do we make that work on Windows? Have recent
> versions grown anything that looks like real file permissions?
Windows has had full ACL support since 1993. The easiest way to do
what you're doing here is to just set a DENY permission on the
postgres operating system user.
Yeah. See <https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls> for example.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes: > On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander <magnus@hagander.net> wrote: >> Windows has had full ACL support since 1993. The easiest way to do >> what you're doing here is to just set a DENY permission on the >> postgres operating system user. > Yeah. See < > https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls> > for example. Cool. Maybe somebody should take a fresh look at the places where we're assuming Windows has nothing comparable to Unix permissions (for example, checking for world readability of ssl_key_file). It's off-topic for this thread though. regards, tom lane
Hi, On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > Perhaps we could make that even better with a GUC though. I propose a > > GUC called 'configuration_managed_externally = true / false". If you set > > it to true, we prevent ALTER SYSTEM and make the error message more > > definitive: > > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: configuration is managed externally > > > As a bonus, if that GUC is set, we could even check at server startup > > that all the configuration files are not writable by the postgres user, > > and print a warning or refuse to start up if they are. > > I like this idea. The "bonus" is not optional though, because > setting the files' ownership/permissions is the only way to be > sure that the prohibition is even a little bit bulletproof. Isn't this going to break pgbackrest restore then, which (AIUI, and was mentioned upthread) writes recovery configs into postgresql.auto.conf? Or do I misunderstand the proposal? I think it would be awkward if only root users are able to run pgbackrest restore. I have added David to the CC list to make him aware of this, in case he was not following this thread. The other candidate for breakage that was mentioned was pg_basebackup -R, but I guess that could be worked around. Michael
As a bonus, if that GUC is set, we could even check at server startup that all the configuration files are not writable by the postgres user,
and print a warning or refuse to start up if they are.
Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical change - especially just to handle the given ask, which is to prevent a specific command from running. Not implement a brand new security system. There are so many ways this could go wrong if we start having separate permissions for some of our files. In addition to backups and other tools that need to write to the conf files as the postgres user, what about systems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the conf files and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1 from me, as they say/ :)
Cheers,
Greg
On Wed, 20 Mar 2024 at 14:04, Greg Sabino Mullane <htamfids@gmail.com> wrote: >> >> As a bonus, if that GUC is set, we could even check at server startup that all the configuration files are not writableby the postgres user, >> and print a warning or refuse to start up if they are. > > > Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical change- especially just to handle the given ask, which is to prevent a specific command from running. Not implement a brandnew security system. There are so many ways this could go wrong if we start having separate permissions for some ofour files. In addition to backups and other tools that need to write to the conf files as the postgres user, what aboutsystems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the conffiles and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1 fromme, as they say/ :) Well put. I don't think the effort of making all tooling handle this correctly is worth the benefit that it brings. afaict everyone on this thread that actually wants to use this feature would be happy with the functionality that the current patch provides (i.e. having postgresql.auto.conf writable, but having ALTER SYSTEM error out).
On Wed, Mar 20, 2024 at 11:07 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical change- especially just to handle the given ask, which is to prevent a specific command from running. Not implement a brandnew security system. There are so many ways this could go wrong if we start having separate permissions for some ofour files. In addition to backups and other tools that need to write to the conf files as the postgres user, what aboutsystems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the conffiles and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1 fromme, as they say/ :) > > Well put. I don't think the effort of making all tooling handle this > correctly is worth the benefit that it brings. afaict everyone on this > thread that actually wants to use this feature would be happy with the > functionality that the current patch provides (i.e. having > postgresql.auto.conf writable, but having ALTER SYSTEM error out). Yeah, I agree with this completely. I don't understand why people who hate the feature and hope it dies in a fire get to decide how it has to work. And also, if we verify that the configuration files are all read-only at the OS level, that also prevents the external tool from managing them. Well, it can: it can make them non-read-only after server start, then modify them, then make them read-only again, and it can make sure that if the system crashes, it again marks them read-only before trying to start PG. But it seems quite obvious that this will be inconvenient and difficult to get right. I find it quite easy to understand the idea that someone wants the PostgreSQL configuration to be managed by Kubernetes rather than via by ALTER SYSTEM, but I can't think of any scenario when you just don't want to be able to manage the configuration at all. Who in the world would want that? -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Mar 20, 2024 at 8:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 20, 2024 at 11:07 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical change - especially just to handle the given ask, which is to prevent a specific command from running. Not implement a brand new security system. There are so many ways this could go wrong if we start having separate permissions for some of our files. In addition to backups and other tools that need to write to the conf files as the postgres user, what about systems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the conf files and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1 from me, as they say/ :)
>
> Well put. I don't think the effort of making all tooling handle this
> correctly is worth the benefit that it brings. afaict everyone on this
> thread that actually wants to use this feature would be happy with the
> functionality that the current patch provides (i.e. having
> postgresql.auto.conf writable, but having ALTER SYSTEM error out).
Yeah, I agree with this completely. I don't understand why people who
hate the feature and hope it dies in a fire get to decide how it has
to work.
And also, if we verify that the configuration files are all read-only
at the OS level, that also prevents the external tool from managing
them. Well, it can: it can make them non-read-only after server start,
then modify them, then make them read-only again, and it can make sure
that if the system crashes, it again marks them read-only before
trying to start PG. But it seems quite obvious that this will be
inconvenient and difficult to get right. I find it quite easy to
understand the idea that someone wants the PostgreSQL configuration to
be managed by Kubernetes rather than via by ALTER SYSTEM, but I can't
think of any scenario when you just don't want to be able to manage
the configuration at all. Who in the world would want that?
I would argue that having the default permissions not allow postgres to edit it's own config files *except* for postgresql.auto.conf would be a better default than what we have now, but that's completely independent of the patch being discussed on this thread. (And FWIW also already solved on debian-based platforms for example, which but the main config files in /etc with postgres only having read permissions on them - and having the *packagers* adapt such things for their platforms in general seems like a better place).
--
On Wed, Mar 20, 2024 at 3:11 PM Magnus Hagander <magnus@hagander.net> wrote: > I would argue that having the default permissions not allow postgres to edit it's own config files *except* for postgresql.auto.confwould be a better default than what we have now, but that's completely independent of the patch beingdiscussed on this thread. (And FWIW also already solved on debian-based platforms for example, which but the main configfiles in /etc with postgres only having read permissions on them - and having the *packagers* adapt such things fortheir platforms in general seems like a better place). I don't think that I agree that it's categorically better, but it might be better for some people or in some circumstances. I very much do agree that it's a packaging question rather than our job to sort out. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Mar 20, 2024 at 8:14 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 20, 2024 at 3:11 PM Magnus Hagander <magnus@hagander.net> wrote:
> I would argue that having the default permissions not allow postgres to edit it's own config files *except* for postgresql.auto.conf would be a better default than what we have now, but that's completely independent of the patch being discussed on this thread. (And FWIW also already solved on debian-based platforms for example, which but the main config files in /etc with postgres only having read permissions on them - and having the *packagers* adapt such things for their platforms in general seems like a better place).
I don't think that I agree that it's categorically better, but it
might be better for some people or in some circumstances. I very much
do agree that it's a packaging question rather than our job to sort
out.
Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the administrator to choose what fits them should be made possible.
On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote: > Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the administratorto choose what fits them should be made possible. +1. Which is also the justification for this patch, when it comes right down to it. The administrator gets to decide how the contents of postgresql.conf are to be managed on their particular installation. They can decide that postgresql.conf should be writable by the same user that runs PostgreSQL, or not. And they should also be able to decide that ALTER SYSTEM is an OK way to change configuration, or that it isn't. How we enable them to make that decision is a point for discussion, and how exactly we phrase the documentation is a point for discussion, but we have no business trying to impose conditions, as if they're only allowed to make that decision if they conform to some (IMHO ridiculous) requirements that we dictate from on high. It's their system, not ours. I mean, for crying out loud, users can set enable_seqscan=off in postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set zero_damaged_pages=on in postgresql.conf and silently remove vast quantities of data without knowing that they're doing anything. We don't even question that stuff ... although we probably should be questioning the second one, because, in my experience, it's just a foot-gun and never solves anything. Nonetheless, as of today, we have it. So somehow we're talking ourselves into believing that letting the user just shut off ALTER SYSTEM, without taking any other action as a prerequisite, is more scary than those things. It's not. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On Wed, Mar 20, 2024 at 08:11:32PM +0100, Magnus Hagander wrote: > (And FWIW also already solved on debian-based platforms for example, > which but the main config files in /etc with postgres only having read > permissions on them JFTR - Debian/Ubuntu keep postgresql.conf under /etc/postgresql, but that directory is owned by the postgres user by default and it can change the configuration files (if that wasn't the case, external tools like Patroni that run under the postgres user and manage postgresql.conf would work much less easily on them). Michael
On 3/20/24 22:30, Michael Banck wrote: > > On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote: >> Heikki Linnakangas <hlinnaka@iki.fi> writes: >>> Perhaps we could make that even better with a GUC though. I propose a >>> GUC called 'configuration_managed_externally = true / false". If you set >>> it to true, we prevent ALTER SYSTEM and make the error message more >>> definitive: >> >>> postgres=# ALTER SYSTEM SET wal_level TO minimal; >>> ERROR: configuration is managed externally >> >>> As a bonus, if that GUC is set, we could even check at server startup >>> that all the configuration files are not writable by the postgres user, >>> and print a warning or refuse to start up if they are. >> >> I like this idea. The "bonus" is not optional though, because >> setting the files' ownership/permissions is the only way to be >> sure that the prohibition is even a little bit bulletproof. > > Isn't this going to break pgbackrest restore then, which (AIUI, and was > mentioned upthread) writes recovery configs into postgresql.auto.conf? > Or do I misunderstand the proposal? I think it would be awkward if only > root users are able to run pgbackrest restore. I have added David to the > CC list to make him aware of this, in case he was not following this > thread. It doesn't sound like people are in favor of requiring read-only permissions for postgresql.auto.conf, but in any case it would not be a big issue for pgBackRest or other backup solutions as far as I can see. pgBackRest stores all permissions and ownership so a restore by the user will bring everything back just as it was. Restoring as root sounds bad on the face of it, but for managed environments like k8s it would not be all that unusual. There is also the option of restoring and then modifying permissions later, or in pgBackRest use the --type=preserve option to leave postgresql.auto.conf as it is. Permissions could also be updated before the backup tool is run and then set back. Since this feature is intended for managed environments scripting these kinds of changes should be pretty easy and not a barrier to using any backup tool. Regards, -David
On Wed, Mar 20, 2024 at 8:52 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote:
> Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the administrator to choose what fits them should be made possible.
+1. Which is also the justification for this patch, when it comes
right down to it. The administrator gets to decide how the contents of
postgresql.conf are to be managed on their particular installation.
Not really. The administrator can *already* do that. It's trivial.
This patch is about doing it in a way that doesn't produce as ugly a message.But if we're "delegating" it to packagers and "os administrators", then the problem is already solved. This patch is about trying to solve it *without* involving the packagers or OS administrators.
Not saying we shouldn't do it, but I'd argue the exact opposite of yours aboe, which is that it's very much not the justification of the patch :)
They can decide that postgresql.conf should be writable by the same
user that runs PostgreSQL, or not. And they should also be able to
decide that ALTER SYSTEM is an OK way to change configuration, or that
it isn't. How we enable them to make that decision is a point for
discussion, and how exactly we phrase the documentation is a point for
discussion, but we have no business trying to impose conditions, as if
they're only allowed to make that decision if they conform to some
(IMHO ridiculous) requirements that we dictate from on high. It's
their system, not ours.
Agreed on all those except they can already do this. It's just that the error message is ugly. The path of least resistance would be to just specifically detect a permissions error on the postgresql.auto.conf file when you try to do ALTER SYSTEM, and throw at least an error hint about "you must allow writing to this file for the feature to work".
So this patch isn't at all about enabling this functionality. It's about making it more user friendly.
I mean, for crying out loud, users can set enable_seqscan=off in
postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set
This is actually a good example, because it's kind of like this patch. It doesn't *actually* disable the ability to run sequential scans, it just disables the "usual way". Just like this patch doesn't prevent the superuser from editing the config, but it does prevent them droin doing it "the usual way".
zero_damaged_pages=on in postgresql.conf and silently remove vast
quantities of data without knowing that they're doing anything. We
don't even question that stuff ... although we probably should be
I like how you got this far and didn't even mention fsync=off :)
--
On Wed, Mar 20, 2024 at 10:30 PM Magnus Hagander <magnus@hagander.net> wrote: > Not really. The administrator can *already* do that. It's trivial. > > This patch is about doing it in a way that doesn't produce as ugly a message.But if we're "delegating" it to packagersand "os administrators", then the problem is already solved. This patch is about trying to solve it *without* involvingthe packagers or OS administrators. > > Not saying we shouldn't do it, but I'd argue the exact opposite of yours aboe, which is that it's very much not the justificationof the patch :) OK, that's a fair way of looking at it, too (and also you break client tools). >> I mean, for crying out loud, users can set enable_seqscan=off in >> postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set > > This is actually a good example, because it's kind of like this patch. It doesn't *actually* disable the ability to runsequential scans, it just disables the "usual way". Just like this patch doesn't prevent the superuser from editing theconfig, but it does prevent them droin doing it "the usual way". Good point. >> zero_damaged_pages=on in postgresql.conf and silently remove vast >> quantities of data without knowing that they're doing anything. We >> don't even question that stuff ... although we probably should be > > I like how you got this far and didn't even mention fsync=off :) Ha! -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Mar 20, 2024 at 10:31 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Wed, Mar 20, 2024 at 8:52 PM Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote: >> > Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the administratorto choose what fits them should be made possible. >> >> +1. Which is also the justification for this patch, when it comes >> right down to it. The administrator gets to decide how the contents of >> postgresql.conf are to be managed on their particular installation. > > > Not really. The administrator can *already* do that. It's trivial. > > This patch is about doing it in a way that doesn't produce as ugly a message.But if we're "delegating" it to packagersand "os administrators", then the problem is already solved. This patch is about trying to solve it *without* involvingthe packagers or OS administrators. > > Not saying we shouldn't do it, but I'd argue the exact opposite of yours aboe, which is that it's very much not the justificationof the patch :) > > >> >> They can decide that postgresql.conf should be writable by the same >> user that runs PostgreSQL, or not. And they should also be able to >> decide that ALTER SYSTEM is an OK way to change configuration, or that >> it isn't. How we enable them to make that decision is a point for >> discussion, and how exactly we phrase the documentation is a point for >> discussion, but we have no business trying to impose conditions, as if >> they're only allowed to make that decision if they conform to some >> (IMHO ridiculous) requirements that we dictate from on high. It's >> their system, not ours. > > > Agreed on all those except they can already do this. It's just that the error message is ugly. The path of least resistancewould be to just specifically detect a permissions error on the postgresql.auto.conf file when you try to do ALTERSYSTEM, and throw at least an error hint about "you must allow writing to this file for the feature to work". > > So this patch isn't at all about enabling this functionality. It's about making it more user friendly. > > >> I mean, for crying out loud, users can set enable_seqscan=off in >> postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set > > > This is actually a good example, because it's kind of like this patch. It doesn't *actually* disable the ability to runsequential scans, it just disables the "usual way". Just like this patch doesn't prevent the superuser from editing theconfig, but it does prevent them droin doing it "the usual way". > > >> >> zero_damaged_pages=on in postgresql.conf and silently remove vast >> quantities of data without knowing that they're doing anything. We >> don't even question that stuff ... although we probably should be > > > I like how you got this far and didn't even mention fsync=off :) > And yet somehow query hints are more scary than ALL of these things. Go figure! Robert Treat https://xzilla.net
On Tue, Mar 19, 2024 at 9:13 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > On Mon, 18 Mar 2024 at 18:27, Robert Haas <robertmhaas@gmail.com> wrote: > > I think for now we > > should just file this under "Other platforms and clients," which only > > has one existing setting. If the number of settings of this type > > grows, we can split it out. > > Done. I also included a patch to rename COMPAT_OPTIONS_CLIENTS to > COMPAT_OPTIONS_OTHER, since that enum variant naming doesn't match the > new intent of the section. I reviewed these patches. I think 0001 probably isn't strictly necessary, but I don't think it's problematic either. And I'm quite happy with 0002 also. In particular, I think the documentation - which must be by far the most important of the patch - does an excellent job explaining the limitations of this feature. My only quibbles are: - 0002 deletes a blank line from postgresql.conf.sample, and I think it shouldn't; and - I think the last sentence of the documentation is odd and could be dropped; who would expect changing a GUC to reset the contents of a config file, anyway? Since those are just minor points, that brings us to the question of whether there is consensus to proceed with this. I believe that there is a clear consensus that there should be some way to disable ALTER SYSTEM. Sure, some people, particularly Tom, disagree, but I don't think there is any way of counting up the votes that leads to the conclusion that we shouldn't have this feature at all. If someone feels otherwise, show us how you counted the votes. What is less clear is whether there is a consensus in favor of this particular method of disabling ALTER SYSTEM, namely, via a GUC. The two alternate approaches that seem to enjoy some level of support are (a) an extension or (b) changing the permissions on the files. I haven't tried to count up how many people are specifically in favor of each approach. I personally think that it doesn't matter very much, because I interpret the comments in favor of one or another implementation as saying "I want us to have this feature and of the possible approaches I prefer $WHATEVER" rather than "the only architecturally acceptable approach to this feature is $WHATEVER and if we can't have that then i'd rather have nothing at all." Of course, like everything else, that conclusion is open to debate, and certainly to correction by the people who have voted in favor of one of the alternate approaches, if I've misinterpreted their views. But, as a practical matter, this is the patch we have, because this is the patch that Gabriele and Jelte took time to write and polish. Nobody else has taken the opportunity to produce a competing one. And, if we nevertheless insist that it has to be done some other way, I think the inevitable result will be that nothing gets into this release at all, because we're less than 2 weeks from feature freeze, and there's not time for a complete do-over of something that was originally proposed all the way back in September. And my reading of the thread, at least, is that more people will be happy if something gets committed here, even if it's not exactly what they would have preferred, than if we get nothing at all. I'm going to wait a few days for any final comments. If it becomes clear that there is in fact no consensus to commit this version of the patch set (or something very similar) then I'll mark this as Returned with Feedback. Otherwise, I plan to commit these patches (perhaps after adjusting in accordance with my comments above). -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > Since those are just minor points, that brings us to the question of > whether there is consensus to proceed with this. I believe that there > is a clear consensus that there should be some way to disable ALTER > SYSTEM. Sure, some people, particularly Tom, disagree, but I don't > think there is any way of counting up the votes that leads to the > conclusion that we shouldn't have this feature at all. FWIW, I never objected to the idea of being able to disable ALTER SYSTEM. I felt that it ought to be part of a larger feature that would provide a more bulletproof guarantee that a superuser can't alter the system configuration; but I'm clearly in the minority on that. I'm content with just having it disable ALTER SYSTEM and no more, as long as the documentation is sufficiently clear that an uncooperative superuser can easily bypass this if you don't back it up with filesystem-level controls. regards, tom lane
On Mon, Mar 25, 2024 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > FWIW, I never objected to the idea of being able to disable ALTER > SYSTEM. I felt that it ought to be part of a larger feature that > would provide a more bulletproof guarantee that a superuser can't > alter the system configuration; but I'm clearly in the minority > on that. I'm content with just having it disable ALTER SYSTEM > and no more, as long as the documentation is sufficiently clear > that an uncooperative superuser can easily bypass this if you don't > back it up with filesystem-level controls. OK, great. The latest patch doesn't specifically talk about backing it up with filesystem-level controls, but it does clearly say that this feature is not going to stop a determined superuser from bypassing the feature, which I think is the appropriate level of detail. We don't actually know whether a user has filesystem-level controls available on their system that are equal to the task; certainly chmod isn't good enough, unless you can prevent the superuser from just running chmod again, which you probably can't. An FS-level immutable flag or some other kind of OS-level wizardry might well get the job done, but I don't think our documentation needs to speculate about that. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > OK, great. The latest patch doesn't specifically talk about backing it > up with filesystem-level controls, but it does clearly say that this > feature is not going to stop a determined superuser from bypassing the > feature, which I think is the appropriate level of detail. We don't > actually know whether a user has filesystem-level controls available > on their system that are equal to the task; certainly chmod isn't good > enough, unless you can prevent the superuser from just running chmod > again, which you probably can't. An FS-level immutable flag or some > other kind of OS-level wizardry might well get the job done, but I > don't think our documentation needs to speculate about that. True. For postgresql.conf, you can put it outside the data directory and make it be owned by some other user, and the job is done. It's harder for postgresql.auto.conf because that always lives in the data directory which is necessarily postgres-writable, so even if you did those two things to it the superuser could just rename or remove it and then write postgresql.auto.conf of his choosing. I wonder whether this feature should include teaching the server to ignore postgresql.auto.conf altogether, which would make it relatively easy to get to a bulletproof configuration. regards, tom lane
On Mon, Mar 25, 2024 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
> OK, great. The latest patch doesn't specifically talk about backing it
> up with filesystem-level controls, but it does clearly say that this
> feature is not going to stop a determined superuser from bypassing the
> feature, which I think is the appropriate level of detail. We don't
> actually know whether a user has filesystem-level controls available
> on their system that are equal to the task; certainly chmod isn't good
> enough, unless you can prevent the superuser from just running chmod
> again, which you probably can't. An FS-level immutable flag or some
> other kind of OS-level wizardry might well get the job done, but I
> don't think our documentation needs to speculate about that.
True. For postgresql.conf, you can put it outside the data directory
and make it be owned by some other user, and the job is done. It's
harder for postgresql.auto.conf because that always lives in the data
directory which is necessarily postgres-writable, so even if you
did those two things to it the superuser could just rename or
remove it and then write postgresql.auto.conf of his choosing.
Just to add to that -- if you use chattr +i on it, the superuser in postgres won't be able to rename it -- only the actual root user.
Just chowning it won't help of course, then the rename part works.
On Mon, Mar 25, 2024 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wonder whether this feature should include teaching the server > to ignore postgresql.auto.conf altogether, which would make it > relatively easy to get to a bulletproof configuration. This has been debated a few times on the thread already, but a number of problems with that idea have been raised, and as far as I can see, everyone who suggested went on to recant and agree that we shouldn't do that. If you feel a strong need to relitigate that, please check the prior discussion first. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Mar 25, 2024 at 01:29:46PM -0400, Robert Haas wrote: > What is less clear is whether there is a consensus in favor of this > particular method of disabling ALTER SYSTEM, namely, via a GUC. The > two alternate approaches that seem to enjoy some level of support are > (a) an extension or (b) changing the permissions on the files. I am wondering if the fact that you would be able to do: ALTER SYSTEM SET externally_managed_configuration = false and then be unable to use ALTER SYSTEM to revert the change is significant. I can't think of many such cases. Isn't "configuration" too generic a term for disabling ALTER SYSTEM? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Mon, 25 Mar 2024 at 20:16, Bruce Momjian <bruce@momjian.us> wrote: > I am wondering if the fact that you would be able to do: > > ALTER SYSTEM SET externally_managed_configuration = false > > and then be unable to use ALTER SYSTEM to revert the change is > significant. This is not possible, due to the externally_managed_configuration GUC having the GUC_DISALLOW_IN_AUTO_FILE flag. > Isn't "configuration" too generic a term for disabling ALTER SYSTEM? maybe "externally_managed_auto_config"
On Mon, Mar 25, 2024 at 09:40:55PM +0100, Jelte Fennema-Nio wrote: > On Mon, 25 Mar 2024 at 20:16, Bruce Momjian <bruce@momjian.us> wrote: > > I am wondering if the fact that you would be able to do: > > > > ALTER SYSTEM SET externally_managed_configuration = false > > > > and then be unable to use ALTER SYSTEM to revert the change is > > significant. > > This is not possible, due to the externally_managed_configuration GUC > having the GUC_DISALLOW_IN_AUTO_FILE flag. Ah, good, thanks. > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM? > > maybe "externally_managed_auto_config" How many people associate "auto" with ALTER SYSTEM? I assume not many. To me, externally_managed_configuration is promising a lot more than it delivers because there is still a lot of ocnfiguration it doesn't control. I am also confused why the purpose of the feature, external management of configuation, is part of the variable name. We usually name parameters for what they control. It seems this is really controlling the ability to alter system variables at the SQL level, maybe sql_alter_system_vars. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote: > > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM? > > > > maybe "externally_managed_auto_config" > > How many people associate "auto" with ALTER SYSTEM? I assume not many. > > To me, externally_managed_configuration is promising a lot more than it > delivers because there is still a lot of ocnfiguration it doesn't > control. I am also confused why the purpose of the feature, external > management of configuation, is part of the variable name. We usually > name parameters for what they control. I actually agree with this. I wasn't going to quibble with it because other people seemed to like it. But I think something like allow_alter_system would be better, as it would describe the exact thing that the parameter does, rather than how we think the parameter ought to be used. -- Robert Haas EDB: http://www.enterprisedb.com
> On 26 Mar 2024, at 13:11, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote: >> To me, externally_managed_configuration is promising a lot more than it >> delivers because there is still a lot of ocnfiguration it doesn't >> control. I am also confused why the purpose of the feature, external >> management of configuation, is part of the variable name. We usually >> name parameters for what they control. > > I actually agree with this. I wasn't going to quibble with it because > other people seemed to like it. But I think something like > allow_alter_system would be better, as it would describe the exact > thing that the parameter does, rather than how we think the parameter > ought to be used. +Many. Either allow_alter_system or enable_alter_system_command is IMO preferrable, not least because someone might use this without using any external configuration tool, making the name even more misleading. -- Daniel Gustafsson
At 2024-03-26 08:11:33 -0400, robertmhaas@gmail.com wrote: > > On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote: > > > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM? > > > > > > maybe "externally_managed_auto_config" > > > > How many people associate "auto" with ALTER SYSTEM? I assume not many. > > > > To me, externally_managed_configuration is promising a lot more than it > > delivers because there is still a lot of ocnfiguration it doesn't > > control. I am also confused why the purpose of the feature, external > > management of configuation, is part of the variable name. We usually > > name parameters for what they control. > > I actually agree with this. I wasn't going to quibble with it because > other people seemed to like it. But I think something like > allow_alter_system would be better, as it would describe the exact > thing that the parameter does, rather than how we think the parameter > ought to be used. Yes, "externally_managed_configuration" raises far more questions than it answers. "enable_alter_system" is clearer in terms of what to expect when you set it. "enable_alter_system_command" is rather long, but even better in that it is specific enough to not promise anything about not allowing superusers to change the configuration some other way. -- Abhijit (as someone who could find a use for this feature)
On Tue, Mar 26, 2024 at 8:55 AM Abhijit Menon-Sen <ams@toroid.org> wrote: > Yes, "externally_managed_configuration" raises far more questions than > it answers. "enable_alter_system" is clearer in terms of what to expect > when you set it. "enable_alter_system_command" is rather long, but even > better in that it is specific enough to not promise anything about not > allowing superusers to change the configuration some other way. It was previously suggested that we shouldn't start the GUC name with "enable," since those are all planner GUCs currently. It's sort of a silly precedent, but we have it, so that's why I proposed "allow" instead. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote: >> To me, externally_managed_configuration is promising a lot more than it >> delivers because there is still a lot of ocnfiguration it doesn't >> control. I am also confused why the purpose of the feature, external >> management of configuation, is part of the variable name. We usually >> name parameters for what they control. > I actually agree with this. I wasn't going to quibble with it because > other people seemed to like it. But I think something like > allow_alter_system would be better, as it would describe the exact > thing that the parameter does, rather than how we think the parameter > ought to be used. +1. The overpromise-and-underdeliver aspect of the currently proposed name is a lot of the reason I've been unhappy and kept pushing for it to lock things down more. "allow_alter_system" is a lot more straightforward about exactly what it does, and if that is all we want it to do, then a name like that is good. regards, tom lane
> On Mar 27, 2024, at 3:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bruce Momjian <bruce@momjian.us> writes: >> I am thinking "enable_alter_system_command" is probably good because we >> already use "enable" so why not reuse that idea, and I think "command" >> is needed because we need to clarify we are talking about the command, >> and not generic altering of the system. We could use >> "enable_sql_alter_system" if people want something shorter. > > Robert already mentioned why not use "enable_": up to now that prefix > has only been applied to planner plan-type-enabling GUCs. I'd be okay > with "allow_alter_system_command", although I find it unnecessarily > verbose. Agree. I don’t think “_command” adds much clarity. Cheers Andrew
On Wed, 27 Mar 2024 at 02:24, Andrew Dunstan <andrew@dunslane.net> wrote: > Agree. I don’t think “_command” adds much clarity. Alright, changed the GUC name to "allow_alter_system" since that seems to have the most "votes". One other option would be to call it simply "alter_system", just like "jit" is not called "allow_jit" or "enable_jit". But personally I feel that the "allow_alter_system" is clearer than plain "alter_system" for the GUC name.
Вложения
On Wed, Mar 27, 2024 at 03:43:28PM +0100, Jelte Fennema-Nio wrote: > + </term> > + <listitem> > + <para> > + When <literal>allow_alter_system</literal> is set to > + <literal>on</literal>, an error is returned if the <command>ALTER > + SYSTEM</command> command is used. This parameter can only be set in > + the <filename>postgresql.conf</filename> file or on the server command > + line. The default value is <literal>on</literal>. > + </para> Uh, the above is clearly wrong. I think you mean "off" on the second line. > + > + <para> > + Note that this setting cannot be regarded as a security feature. It > + only disables the <literal>ALTER SYSTEM</literal> command. It does not > + prevent a superuser from changing the configuration remotely using Why "remotely"? > + other means. A superuser has many ways of executing shell commands at > + the operating system level, and can therefore modify > + <literal>postgresql.auto.conf</literal> regardless of the value of > + this setting. The purpose of the setting is to prevent > + <emphasis>accidental</emphasis> modifications via <literal>ALTER > + SYSTEM</literal> in environments where > + <productname>PostgreSQL</productname> its configuration is managed by "its"? > + some outside mechanism. In such environments, using <command>ALTER > + SYSTEM</command> to make configuration changes might appear to work, > + but then may be discarded at some point in the future when that outside "might" > + mechanism updates the configuration. Setting this parameter to > + <literal>on</literal> can help to avoid such mistakes. > + </para> "off" Is this really a patch we think we can push into PG 17. I am having my doubts. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Alright, changed the GUC name to "allow_alter_system" since that seems > to have the most "votes". One other option would be to call it simply > "alter_system", just like "jit" is not called "allow_jit" or > "enable_jit". > > But personally I feel that the "allow_alter_system" is clearer than > plain "alter_system" for the GUC name. I agree, and have committed your 0001. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian <bruce@momjian.us> wrote: > Uh, the above is clearly wrong. I think you mean "off" on the second line. Woops. When the name changed from externally_managed_configuration to allow_alter_system, the sense of it was reversed, and I guess Jelte missed flipping the documentation references around. I likely would have made the same mistake, but it's easily fixed. > > + > > + <para> > > + Note that this setting cannot be regarded as a security feature. It > > + only disables the <literal>ALTER SYSTEM</literal> command. It does not > > + prevent a superuser from changing the configuration remotely using > > Why "remotely"? This wording was suggested upthread. I think the point here is that if the superuser is logging in from the local machine, it's obvious that they can do whatever they want. The point is to emphasize that a superuser without a local login can, too. > "its"? Yeah, that seems like an extra word. > > + some outside mechanism. In such environments, using <command>ALTER > > + SYSTEM</command> to make configuration changes might appear to work, > > + but then may be discarded at some point in the future when that outside > > "might" This does not seem like a mistake to me. I'm not sure why you think it is. > > + mechanism updates the configuration. Setting this parameter to > > + <literal>on</literal> can help to avoid such mistakes. > > + </para> > > "off" This is another case that needs to be fixed now that the sense of the GUC is reversed. (We'd better make sure the code has the test the right way around, too.) > Is this really a patch we think we can push into PG 17. I am having my > doubts. If the worst thing that happens in PG 17 is that we push a patch that needs a few documentation corrections, we're going to be doing fabulously well. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, 27 Mar 2024 at 16:10, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian <bruce@momjian.us> wrote: > > Uh, the above is clearly wrong. I think you mean "off" on the second line. > > Woops. When the name changed from externally_managed_configuration to > allow_alter_system, the sense of it was reversed, and I guess Jelte > missed flipping the documentation references around. Yeah, that's definitely what happened. I did change a few, but I indeed missed a few others (or maybe flipped some twice by accident, or hadn't been flipped before when it reversed previously). > > Why "remotely"? > > This wording was suggested upthread. I think the point here is that if > the superuser is logging in from the local machine, it's obvious that > they can do whatever they want. The point is to emphasize that a > superuser without a local login can, too. Changed this from "remotely using other means" to "using other SQL commands". > > "its"? > > Yeah, that seems like an extra word. Changed this to "the configuration of PostgreSQL" > > > + some outside mechanism. In such environments, using <command>ALTER > > > + SYSTEM</command> to make configuration changes might appear to work, > > > + but then may be discarded at some point in the future when that outside > > > > "might" > > This does not seem like a mistake to me. I'm not sure why you think it is. I also think the original sentence was correct, but I don't think it read very naturally. Changed it now in hopes to improve that. > > > + mechanism updates the configuration. Setting this parameter to > > > + <literal>on</literal> can help to avoid such mistakes. > > > + </para> > > > > "off" > > This is another case that needs to be fixed now that the sense of the > GUC is reversed. (We'd better make sure the code has the test the > right way around, too.) Fixed this one too, and the code is the right way around.
Вложения
The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal> in environments where
The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEM in environments where..." is enough?
Cheers,
Greg
On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote:
The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal> in environments whereThe emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEM in environments where..." is enough?
Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know if the extra word "accidental" is necessary, but I think that's the motivation.
On Wed, Mar 27, 2024 at 10:12 AM Isaac Morland <isaac.morland@gmail.com> wrote:
On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote:The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal> in environments whereThe emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEM in environments where..." is enough?Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know if the extra word "accidental" is necessary, but I think that's the motivation.
Prevent non-malicious modifications via ALTER SYSTEM in environments where ...
David J.
On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland <isaac.morland@gmail.com> wrote: > On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote: >>> The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal>in environments where >> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEMin environments where..." is enough? > Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know if theextra word "accidental" is necessary, but I think that's the motivation. I think the emphasis is entirely warranted in this case. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Mar 27, 2024, 11:46 Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>>> The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal> in environments where
>> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEM in environments where..." is enough?
> Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know if the extra word "accidental" is necessary, but I think that's the motivation.
I think the emphasis is entirely warranted in this case.
+1. And while "non-malicious" may technically be more correct, I don't think it's any clearer.
On Wed, Mar 27, 2024 at 04:50:27PM +0100, Jelte Fennema-Nio wrote: > > This wording was suggested upthread. I think the point here is that if > > the superuser is logging in from the local machine, it's obvious that > > they can do whatever they want. The point is to emphasize that a > > superuser without a local login can, too. > > Changed this from "remotely using other means" to "using other SQL commands". Yes, I like the SQL emphasis since "remote" just doesn't seem like the right thing to highlight here. > > > > + some outside mechanism. In such environments, using <command>ALTER > > > > + SYSTEM</command> to make configuration changes might appear to work, > > > > + but then may be discarded at some point in the future when that outside > > > > > > "might" > > > > This does not seem like a mistake to me. I'm not sure why you think it is. > > I also think the original sentence was correct, but I don't think it > read very naturally. Changed it now in hopes to improve that. So, might means "possibility" while "may" means permission, so "might" is clearer here. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote: > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > Alright, changed the GUC name to "allow_alter_system" since that seems > > to have the most "votes". One other option would be to call it simply > > "alter_system", just like "jit" is not called "allow_jit" or > > "enable_jit". > > > > But personally I feel that the "allow_alter_system" is clearer than > > plain "alter_system" for the GUC name. > > I agree, and have committed your 0001. So, I email "Is this really a patch we think we can push into PG 17. I am having my doubts," and the patch is applied a few hours after my email. Wow. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote: > On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote: > > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > > Alright, changed the GUC name to "allow_alter_system" since that seems > > > to have the most "votes". One other option would be to call it simply > > > "alter_system", just like "jit" is not called "allow_jit" or > > > "enable_jit". > > > > > > But personally I feel that the "allow_alter_system" is clearer than > > > plain "alter_system" for the GUC name. > > > > I agree, and have committed your 0001. > > So, I email "Is this really a patch we think we can push into PG 17. I > am having my doubts," and the patch is applied a few hours after my > email. Wow. Also odd is that I don't see the commit in git master, so now I am confused. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote:
> On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote:
> > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > > Alright, changed the GUC name to "allow_alter_system" since that seems
> > > to have the most "votes". One other option would be to call it simply
> > > "alter_system", just like "jit" is not called "allow_jit" or
> > > "enable_jit".
> > >
> > > But personally I feel that the "allow_alter_system" is clearer than
> > > plain "alter_system" for the GUC name.
> >
> > I agree, and have committed your 0001.
>
> So, I email "Is this really a patch we think we can push into PG 17. I
> am having my doubts," and the patch is applied a few hours after my
> email. Wow.
Also odd is that I don't see the commit in git master, so now I am
confused.
The main feature being discussed is in the 0002 patch while Robert pushed a doc section rename in the 0001 patch.
David J.
On Wed, Mar 27, 2024 at 3:18 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian <bruce@momjian.us> wrote:On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote:
> On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote:
> > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > > Alright, changed the GUC name to "allow_alter_system" since that seems
> > > to have the most "votes". One other option would be to call it simply
> > > "alter_system", just like "jit" is not called "allow_jit" or
> > > "enable_jit".
> > >
> > > But personally I feel that the "allow_alter_system" is clearer than
> > > plain "alter_system" for the GUC name.
> >
> > I agree, and have committed your 0001.
>
> So, I email "Is this really a patch we think we can push into PG 17. I
> am having my doubts," and the patch is applied a few hours after my
> email. Wow.
Also odd is that I don't see the commit in git master, so now I am
confused.The main feature being discussed is in the 0002 patch while Robert pushed a doc section rename in the 0001 patch.
Well, the internal category name was changed though the docs did remain unchanged.
David J.
On Wed, Mar 27, 2024 at 11:10:31AM -0400, Robert Haas wrote: > > Is this really a patch we think we can push into PG 17. I am having my > > doubts. > > If the worst thing that happens in PG 17 is that we push a patch that > needs a few documentation corrections, we're going to be doing > fabulously well. My point is that we are designing the user API in the last weeks of the commitfest, which usually ends badly for us, and the fact the docs were not even right in the patch just reenforces that concern. But, as I stated in another email, you said you committed the patch, yet I don't see it committed in git master, so I am confused. Ah, I figured it out. You were talking about the GUC renaming: commit de7e96bd0fc Author: Robert Haas <rhaas@postgresql.org> Date: Wed Mar 27 10:45:28 2024 -0400 Rename COMPAT_OPTIONS_CLIENT to COMPAT_OPTIONS_OTHER. The user-facing name is "Other Platforms and Clients", but the internal name seems too focused on clients specifically, especially given the plan to add a new setting to this session that is about platform or deployment model compatibility rather than client compatibility. Jelte Fennema-Nio Discussion: http://postgr.es/m/CAGECzQTfMbDiM6W3av+3weSnHxJvPmuTEcjxVvSt91sQBdOxuQ@mail.gmail.com Please ignore my complaints, and my apologies. As far as the GUC change, let's just be careful since we have a bad history of pushing things near the end that we regret. I am not saying that would be this feature, but let's be careful. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 03:20:38PM -0700, David G. Johnston wrote: > On Wed, Mar 27, 2024 at 3:18 PM David G. Johnston <david.g.johnston@gmail.com> > wrote: > > On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian <bruce@momjian.us> wrote: > > On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote: > > On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote: > > > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio < > postgres@jeltef.nl> wrote: > > > > Alright, changed the GUC name to "allow_alter_system" since that > seems > > > > to have the most "votes". One other option would be to call it > simply > > > > "alter_system", just like "jit" is not called "allow_jit" or > > > > "enable_jit". > > > > > > > > But personally I feel that the "allow_alter_system" is clearer > than > > > > plain "alter_system" for the GUC name. > > > > > > I agree, and have committed your 0001. > > > > So, I email "Is this really a patch we think we can push into PG 17. > I > > am having my doubts," and the patch is applied a few hours after my > > email. Wow. > > Also odd is that I don't see the commit in git master, so now I am > confused. > > > The main feature being discussed is in the 0002 patch while Robert pushed a > doc section rename in the 0001 patch. > > > > Well, the internal category name was changed though the docs did remain > unchanged. Yes, I figured that out, thank you. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, 27 Mar 2024 at 23:23, Bruce Momjian <bruce@momjian.us> wrote: > > On Wed, Mar 27, 2024 at 11:10:31AM -0400, Robert Haas wrote: > > > Is this really a patch we think we can push into PG 17. I am having my > > > doubts. > > > > If the worst thing that happens in PG 17 is that we push a patch that > > needs a few documentation corrections, we're going to be doing > > fabulously well. > > My point is that we are designing the user API in the last weeks of the > commitfest, which usually ends badly for us, and the fact the docs were > not even right in the patch just reenforces that concern. This user API is exactly the same as the original patch from Gabriele in September (apart from enable->allow). And we spent half a year discussing other designs for the user API. So I disagree that we're designing the user API in the last weeks of the commitfest.
On Wed, 27 Mar 2024 at 20:10, Maciek Sakrejda <m.sakrejda@gmail.com> wrote: > > On Wed, Mar 27, 2024, 11:46 Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland <isaac.morland@gmail.com> wrote: >> > On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote: >> >>> The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER SYSTEM</literal>in environments where >> >> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER SYSTEMin environments where..." is enough? >> > Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know ifthe extra word "accidental" is necessary, but I think that's the motivation. >> >> I think the emphasis is entirely warranted in this case. > > +1. And while "non-malicious" may technically be more correct, I don't think it's any clearer. Attached is a new version of the patch with some sentences reworded. I changed accidentally to mistakenly (which still has emphasis). And I hope with the rewording it's now clearer to the reader why that emphasis is there.
Вложения
On Wed, 27 Mar 2024 at 23:06, Bruce Momjian <bruce@momjian.us> wrote: > > > > > + some outside mechanism. In such environments, using <command>ALTER > > > > > + SYSTEM</command> to make configuration changes might appear to work, > > > > > + but then may be discarded at some point in the future when that outside > > > > > > > > "might" > > > > > > This does not seem like a mistake to me. I'm not sure why you think it is. > > > > I also think the original sentence was correct, but I don't think it > > read very naturally. Changed it now in hopes to improve that. > > So, might means "possibility" while "may" means permission, so "might" > is clearer here. Aaah, I misunderstood your original feedback then. I thought you didn't like the use of "might" in "might appear to work". But I now realize you meant "may be discarded" should be changed to "might be discarded". I addressed that in my latest version of the patch.
On Thu, Mar 28, 2024 at 12:47:46AM +0100, Jelte Fennema-Nio wrote: > On Wed, 27 Mar 2024 at 23:06, Bruce Momjian <bruce@momjian.us> wrote: > > > > > > + some outside mechanism. In such environments, using <command>ALTER > > > > > > + SYSTEM</command> to make configuration changes might appear to work, > > > > > > + but then may be discarded at some point in the future when that outside > > > > > > > > > > "might" > > > > > > > > This does not seem like a mistake to me. I'm not sure why you think it is. > > > > > > I also think the original sentence was correct, but I don't think it > > > read very naturally. Changed it now in hopes to improve that. > > > > So, might means "possibility" while "may" means permission, so "might" > > is clearer here. > > Aaah, I misunderstood your original feedback then. I thought you > didn't like the use of "might" in "might appear to work". But I now > realize you meant "may be discarded" should be changed to "might be > discarded". I addressed that in my latest version of the patch. Thanks. I did the may/might/can changes in the docs years ago so I remember the distinction. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, Mar 28, 2024 at 12:43:29AM +0100, Jelte Fennema-Nio wrote: > + <varlistentry id="guc-allow-alter-system" xreflabel="allow_alter_system"> > + <term><varname>allow_alter_system</varname> (<type>boolean</type>) > + <indexterm> > + <primary><varname>allow_alter_system</varname> configuration parameter</primary> > + </indexterm> > + </term> > + <listitem> > + <para> > + When <literal>allow_alter_system</literal> is set to > + <literal>off</literal>, an error is returned if the <command>ALTER > + SYSTEM</command> command is used. This parameter can only be set in "command is used." -> "command is issued." ? > + the <filename>postgresql.conf</filename> file or on the server command > + line. The default value is <literal>on</literal>. > + </para> > + > + <para> > + Note that this setting cannot be regarded as a security feature. It "setting cannot be regarded" -> "setting should not be regarded" > + only disables the <literal>ALTER SYSTEM</literal> command. It does not > + prevent a superuser from changing the configuration using other SQL > + commands. A superuser has many ways of executing shell commands at > + the operating system level, and can therefore modify > + <literal>postgresql.auto.conf</literal> regardless of the value of > + this setting. I like that you explained how this can be bypassed. > + > + <para> > + Turning this setting off is intended for environments where the > + configuration of <productname>PostgreSQL</productname> is managed by > + some outside mechanism. > + In such environments, a well intenioned superuser user might > + <emphasis>mistakenly</emphasis> use <command>ALTER SYSTEM</command> > + to change the configuration instead of using the outside mechanism. > + This might even appear to update the configuration as intended, but "This might even appear to update" -> "This might temporarily update" > + then might be discarded at some point in the future when that outside "that outside" -> "the outside" > + mechanism updates the configuration. > + Setting this parameter to <literal>off</literal> can > + help to avoid such mistakes. "help to avoid" -> "help avoid" > + </para> > + > + <para> > + This parameter only controls the use of <command>ALTER SYSTEM</command>. > + The settings stored in <filename>postgresql.auto.conf</filename> always "always" -> "still" Should this paragraph be moved after or as part of the paragraph about modifying postgresql.auto.conf? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Mar 27, 2024 at 5:17 PM Bruce Momjian <bruce@momjian.us> wrote:
On Thu, Mar 28, 2024 at 12:43:29AM +0100, Jelte Fennema-Nio wrote:
> + <varlistentry id="guc-allow-alter-system" xreflabel="allow_alter_system">
> + <term><varname>allow_alter_system</varname> (<type>boolean</type>)
> + <indexterm>
> + <primary><varname>allow_alter_system</varname> configuration parameter</primary>
> + </indexterm>
> + </term>
> + <listitem>
> + <para>
> + When <literal>allow_alter_system</literal> is set to
> + <literal>off</literal>, an error is returned if the <command>ALTER
> + SYSTEM</command> command is used. This parameter can only be set in
"command is used." -> "command is issued." ?
"command is executed" seems even better. I'd take used over issued.
> + the <filename>postgresql.conf</filename> file or on the server command
> + line. The default value is <literal>on</literal>.
> + </para>
> +
> + <para>
> + Note that this setting cannot be regarded as a security feature. It
"setting cannot be regarded" -> "setting should not be regarded"
"setting must not be regarded" is the correct option here. Stronger than should; we are unable to control whether someone can/does regard it differently.
> +
> + <para>
> + Turning this setting off is intended for environments where the
> + configuration of <productname>PostgreSQL</productname> is managed by
> + some outside mechanism.
> + In such environments, a well intenioned superuser user might
> + <emphasis>mistakenly</emphasis> use <command>ALTER SYSTEM</command>
> + to change the configuration instead of using the outside mechanism.
> + This might even appear to update the configuration as intended, but
"This might even appear to update" -> "This might temporarily update"
I strongly prefer temporarily over may/might/could.
> + then might be discarded at some point in the future when that outside
"that outside" -> "the outside"
Feel like "external" is a more context appropriate term here than "outside".
External also has precedent.
"External tools may also modify postgresql.auto.conf. It is not recommended to do this while the server is running,"
That suggests using "external tools" instead of "outside mechanisms"
This section is also the main entry point for users into the configuration subsystem and hasn't been updated to reflect this new feature. That seems like an oversight that needs to be corrected.
> + </para>
> +
> + <para>
> + This parameter only controls the use of <command>ALTER SYSTEM</command>.
> + The settings stored in <filename>postgresql.auto.conf</filename> always
"always" -> "still"
Neither qualifier is needed, nor does one seem clearly better than the other. Always is true so the weaker "still" seems like the worse choice.
The following is a complete and clear sentence.
The settings stored in postgresql.auto.conf take effect even if allow_alter_system is set to off.
Should this paragraph be moved after or as part of the paragraph about
modifying postgresql.auto.conf?
I like it by itself.
David J.
On Wed, Mar 27, 2024 at 5:43 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
This section is also the main entry point for users into the configuration subsystem and hasn't been updated to reflect this new feature. That seems like an oversight that needs to be corrected.
Shouldn't the "alter system" reference page receive an update as well?
David J.
On Thu, 28 Mar 2024 at 01:43, David G. Johnston <david.g.johnston@gmail.com> wrote: > > On Wed, Mar 27, 2024 at 5:17 PM Bruce Momjian <bruce@momjian.us> wrote: >> >> <snip many documentation suggestions> I addressed them all I think. Mostly the small changes that were suggested, but I rewrote the sentence with "might be discarded". And I added references to the new GUC in both places suggested by David.
Вложения
On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > I addressed them all I think. Mostly the small changes that were > suggested, but I rewrote the sentence with "might be discarded". And I > added references to the new GUC in both places suggested by David. Changed the error hint to use "external tool" too. And removed a duplicate header definition of AllowAlterSystem (I moved it to guc.h so it was together with other definitions a few patches ago, but apparently forgot to remove it from guc_tables.h)
Вложения
On Thu, Mar 28, 2024 at 5:42 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > I addressed them all I think. Mostly the small changes that were > > suggested, but I rewrote the sentence with "might be discarded". And I > > added references to the new GUC in both places suggested by David. > > Changed the error hint to use "external tool" too. And removed a > duplicate header definition of AllowAlterSystem (I moved it to guc.h > so it was together with other definitions a few patches ago, but > apparently forgot to remove it from guc_tables.h) I disagree with a lot of these changes. I think the old version was mostly better. But I can live with a lot of it if it makes other people happy. However: + Which might result in unintended behavior, such as the external tool + discarding the change at some later point in time when it updates the + configuration. This is not OK from a grammatical point of view. You can't just start a sentence with "which" like this. You could replace "Which" with "This", though. + if (!AllowAlterSystem) + { + + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("ALTER SYSTEM is not allowed in this environment"), + errhint("Global configuration changes should be made using an external tool, not by using ALTER SYSTEM."))); + } The extra blank line should go. The brackets should go. And I think the errhint should go, too, because the errhint implies that we know why the user chose to set allow_alter_system=false. There's no reason for this message to be opinionated about that. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, 28 Mar 2024 at 12:57, Robert Haas <robertmhaas@gmail.com> wrote: > I disagree with a lot of these changes. I think the old version was > mostly better. But I can live with a lot of it if it makes other > people happy. I'd have been fine with many of the previous versions of the docs too. (I'm not a native english speaker though, so that might be part of it) > However: Attached is a patch with your last bit of feedback addressed.
Вложения
On Wed, Mar 27, 2024 at 6:24 PM Bruce Momjian <bruce@momjian.us> wrote: > Please ignore my complaints, and my apologies. > > As far as the GUC change, let's just be careful since we have a bad > history of pushing things near the end that we regret. I am not saying > that would be this feature, but let's be careful. Even if what I had pushed was the patch itself, so what? This patch has been sitting around, largely unchanged, for six months. There has been plenty of time for wordsmithing the documentation, yet nobody got interested in doing it until I expressed interest in committing the patch. Meanwhile, there are over 100 other patches that no committer is paying attention to right now, some of which could probably really benefit from some wordsmithing of the documentation. It drives me insane that this is the patch everyone is getting worked up about. This is a 27-line code change that does something many people want, and we're acting like the future of the project depends on it. Both I and others have committed thousands of lines of new code over the last few months that could easily be full of bugs that will eat your data without nearly the scrutiny that this patch is getting. To be honest, I had every intention of pushing the main patch right after I pushed that preliminary patch, but I stopped because I saw you had emailed the thread. I'm pretty sure that I would have missed the fact that the documentation hadn't been properly updated for the fact that the sense of the GUC had been inverted. That would have been embarrassing, and I would have had to push a follow-up commit to fix that. But no real harm would have been done, except that somebody surely would have seized on my mistake as proof that this patch wasn't ready to be committed and that I was being irresponsible and inconsiderate by pushing forward with it, which is a garbage argument. Committers make mistakes like that all the time, every week, even every day. It doesn't mean that they're bad committers, and it doesn't mean that the patches suck. Some of the patches that get committed do suck, but it's not because there are a few words wrong in the documentation. Let's please, please stop pretending like this patch is somehow deserving of special scrutiny. There's barely even anything to scrutinize. It's literally if (!variable) ereport(...) plus some boilerplate and docs. I entirely agree that, because of the risk of someone filing a bogus CVE, the docs do need to be carefully worded. But, I'm going to be honest: I feel completely confident in my ability to review a patch well enough to know whether the documentation for a single test-and-ereport has been done up to project standard. It saddens and frustrates me that you don't seem to agree. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Mar 28, 2024 at 08:38:24AM -0400, Robert Haas wrote: > Let's please, please stop pretending like this patch is somehow > deserving of special scrutiny. There's barely even anything to > scrutinize. It's literally if (!variable) ereport(...) plus some > boilerplate and docs. I entirely agree that, because of the risk of > someone filing a bogus CVE, the docs do need to be carefully worded. > But, I'm going to be honest: I feel completely confident in my ability > to review a patch well enough to know whether the documentation for a > single test-and-ereport has been done up to project standard. It > saddens and frustrates me that you don't seem to agree. The concern about this patch is not its contents but because it is our first attempt at putting limits on the superuser for an external tool. If done improperly, this could open a flood of problems, including CVE and user confusion, which would reflect badly on the project. I think the email discussion has expressed those concerns clearly, and it is only recently that we have gotten to a stage where we are ready to add this, and doing this near the closing of the last commitfest can be a valid concern. I do agree with your analysis of other patches in the commitfest, but I just don't see them stretching our boundaries like this patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, Mar 28, 2024 at 1:46 PM Bruce Momjian <bruce@momjian.us> wrote: > The concern about this patch is not its contents but because it is our > first attempt at putting limits on the superuser for an external tool. > If done improperly, this could open a flood of problems, including CVE > and user confusion, which would reflect badly on the project. > > I think the email discussion has expressed those concerns clearly, and > it is only recently that we have gotten to a stage where we are ready to > add this, and doing this near the closing of the last commitfest can be > a valid concern. I do agree with your analysis of other patches in the > commitfest, but I just don't see them stretching our boundaries like > this patch. I do understand the concern, and I'm not saying that you're wrong to have it at some level, but I do sincerely think it's excessive. I don't think this is even close to being the scariest patch in this release, or even in this CommitFest. I also agree that doing things near the end of the last CommitFest isn't great, because even if your patch is fantastic, people start to think maybe you're only committing it to beat the deadline, and then the conversation can get unpleasant. However, I don't think that's really what is happening here. If this patch gets bounced out of this release, it won't be in any better shape a year from now than it is right now. It can't be, because the code is completely trivial; and the documentation has already been extensively wordsmithed. Surely we don't need another whole release cycle to polish three paragraphs of documentation. I think it has to be right to get this done while we're all thinking about it and the issue is fresh in everybody's mind. How would you like to proceed from here? I think that in addressing all of the comments given in the last few days, the documentation has gotten modestly worse. I think it was crisp and clear before, and now it feels a little ... over-edited. But if you're happy with the latest version, we can go with that. Or, do you need more time to review? -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Mar 28, 2024 at 02:43:38PM -0400, Robert Haas wrote: > How would you like to proceed from here? I think that in addressing > all of the comments given in the last few days, the documentation has > gotten modestly worse. I think it was crisp and clear before, and now > it feels a little ... over-edited. But if you're happy with the latest > version, we can go with that. Or, do you need more time to review? I am fine with moving ahead. I thought my later emails explaining we have to be careful communicated that. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, Mar 28, 2024 at 01:23:36PM +0100, Jelte Fennema-Nio wrote: > + <para> > + Turning this setting off is intended for environments where the > + configuration of <productname>PostgreSQL</productname> is managed by > + some external tool. > + In such environments, a well intentioned superuser might > + <emphasis>mistakenly</emphasis> use <command>ALTER SYSTEM</command> > + to change the configuration instead of using the external tool. > + This might result in unintended behavior, such as the external tool > + discarding the change at some later point in time when it updates the "discarding" -> "overwriting" ? > + <para> > + <literal>ALTER SYSTEM</literal> can be disabled by setting > + <xref linkend="guc-allow-alter-system"/> to <literal>off</literal>, but this > + is no security mechanism (as explained in detail in the documentation for "is no" -> "is not a" -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote: > I am fine with moving ahead. I thought my later emails explaining we > have to be careful communicated that. OK. Thanks for clarifying. I've committed the patch with the two wording changes that you suggested in your subsequent email. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, Mar 29, 2024 at 08:46:33AM -0400, Robert Haas wrote: > On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote: > > I am fine with moving ahead. I thought my later emails explaining we > > have to be careful communicated that. > > OK. Thanks for clarifying. I've committed the patch with the two > wording changes that you suggested in your subsequent email. Great, I know this has been frustrating, and you are right that this wouldn't have been any simpler next year. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Fri, Mar 29, 2024 at 10:48 AM Bruce Momjian <bruce@momjian.us> wrote: > On Fri, Mar 29, 2024 at 08:46:33AM -0400, Robert Haas wrote: > > On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote: > > > I am fine with moving ahead. I thought my later emails explaining we > > > have to be careful communicated that. > > > > OK. Thanks for clarifying. I've committed the patch with the two > > wording changes that you suggested in your subsequent email. > > Great, I know this has been frustrating, and you are right that this > wouldn't have been any simpler next year. Thanks, Bruce. -- Robert Haas EDB: http://www.enterprisedb.com