Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add allow_alter_system GUC.

  2. Rename COMPAT_OPTIONS_CLIENT to COMPAT_OPTIONS_OTHER.

  3. Remove support for version-0 calling conventions.

  1. Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-07T19:51:14Z

    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
--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  2. Re: Possibility to disable `ALTER SYSTEM`

    Joe Conway <mail@joeconway.com> — 2023-09-07T19:57:22Z

    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

  3. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-07T20:03:16Z

    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
Vice President, Cloud Native at EDB
enterprisedb.com

  4. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-07T20:27:14Z

    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

  5. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-08T11:31:16Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  6. Re: Possibility to disable `ALTER SYSTEM`

    Isaac Morland <isaac.morland@gmail.com> — 2023-09-08T14:11:30Z

    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.

  7. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-08T14:17:04Z

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

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.

Thanks,
Gabriele
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  8. Re: Possibility to disable `ALTER SYSTEM`

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-09-08T14:55:47Z

    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.

  9. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-08T15:31:22Z

    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

  10. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-08T21:31:50Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  11. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2023-09-08T21:43:08Z

    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/

  12. Re: Possibility to disable `ALTER SYSTEM`

    Alvaro Hernandez <aht@ongres.com> — 2023-09-08T23:24:02Z

    
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 
> <http://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

  13. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-09T06:37:47Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  14. Re: Possibility to disable `ALTER SYSTEM`

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-09-09T15:14:50Z

    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)

  15. Re: Possibility to disable `ALTER SYSTEM`

    Martín Marqués <martin.marques@gmail.com> — 2023-09-11T11:55:53Z

    Hi,

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

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

  16. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2023-09-11T13:50:19Z

    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/

  17. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2023-09-11T14:04:40Z

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

  18. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-11T14:59:02Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  19. Re: Possibility to disable `ALTER SYSTEM`

    Stephen Frost <sfrost@snowman.net> — 2023-09-11T15:12:01Z

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

  20. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-11T15:56:01Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  21. Re: Possibility to disable `ALTER SYSTEM`

    Isaac Morland <isaac.morland@gmail.com> — 2023-09-11T16:15:29Z

    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.

  22. Re: Possibility to disable `ALTER SYSTEM`

    Martín Marqués <martin.marques@gmail.com> — 2023-09-12T12:33:56Z

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

  23. Re: Possibility to disable `ALTER SYSTEM`

    Greg Sabino Mullane <htamfids@gmail.com> — 2023-09-13T17:10:16Z

    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

  24. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2023-09-15T09:16:09Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  25. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2023-09-15T09:18:51Z

    > 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

  26. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2024-01-30T17:05:37Z

    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
> --
> Gabriele Bartolini
> Vice President, Cloud Native at EDB
> enterprisedb.com
>


-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  27. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-01-30T17:48:50Z

    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

  28. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-30T19:20:04Z

    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

  29. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-01-30T21:25:12Z

    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

  30. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-30T21:48:03Z

    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

  31. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-01-30T21:58:56Z

    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/

  32. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-31T04:25:28Z

    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

  33. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-01-31T05:10:01Z

    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.

  34. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-31T05:28:37Z

    "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

  35. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-01-31T05:43:57Z

    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.

  36. Re: Possibility to disable `ALTER SYSTEM`

    Peter Eisentraut <peter@eisentraut.org> — 2024-01-31T07:43:14Z

    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.

  37. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2024-01-31T10:16:34Z

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

-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  38. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-01-31T14:35:12Z

    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

  39. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-01-31T15:56:13Z

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

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

  40. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-02-01T12:33:04Z

    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.

  41. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-02-01T15:27:56Z

    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

  42. Re: Possibility to disable `ALTER SYSTEM`

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-06T14:10:27Z

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

  43. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-02-06T14:38:06Z

    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.

  44. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-06T15:22:28Z

    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.

  45. Re: Possibility to disable `ALTER SYSTEM`

    Joel Jacobson <joel@compiler.org> — 2024-02-07T08:56:58Z

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

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 leads to a better PostgreSQL over the long term. It creates a crucial feedback loop between users facing query planner issues and 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 these valuable opportunities for improvement of the Query Planner.

Similarly, I think it's crucial to identify functionalities that currently require superuser privileges and cannot yet be explicitly granted to non-superusers.

/Joel

  46. Re: Possibility to disable `ALTER SYSTEM`

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-07T10:16:06Z

    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.

  47. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2024-02-07T10:35:14Z

    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?

Cheers,
Gabriele
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  48. Re: Possibility to disable `ALTER SYSTEM`

    Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> — 2024-02-07T10:37:20Z

    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
-- 
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

  49. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-07T13:31:28Z

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

  50. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-02-07T13:34:06Z

    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.

  51. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-07T13:49:01Z

    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.

  52. Re: Possibility to disable `ALTER SYSTEM`

    Euler Taveira <euler@eulerto.com> — 2024-02-07T17:07:08Z

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

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.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  53. Re: Possibility to disable `ALTER SYSTEM`

    Andrew Dunstan <andrew@dunslane.net> — 2024-02-10T16:16:46Z

    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

  54. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-02-11T13:58:20Z

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

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

  55. Re: Possibility to disable `ALTER SYSTEM`

    Joel Jacobson <joel@compiler.org> — 2024-02-13T07:05:03Z

    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

  56. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-14T16:37:14Z

    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

  57. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-14T18:27:57Z

    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.

  58. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-14T19:13:36Z

    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

  59. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-14T19:23:48Z

    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

  60. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-14T20:08:23Z

    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

  61. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-14T20:38:35Z

    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

  62. Re: Possibility to disable `ALTER SYSTEM`

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-03-14T21:14:29Z

    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

  63. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-14T23:43:15Z

    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

  64. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-15T02:58:37Z

    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.

  65. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-15T10:03:18Z

    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.

  66. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-15T10:08:14Z

    > 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

  67. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-15T11:09:10Z

    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.

  68. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-18T12:57:34Z

    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

  69. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-18T13:09:04Z

    > 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

  70. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-18T14:12:25Z

    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)

  71. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-18T15:34:26Z

    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/

  72. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-18T15:44:20Z

    > 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

  73. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-18T15:46:33Z

    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/

  74. Re: Possibility to disable `ALTER SYSTEM`

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-03-18T16:18:46Z

    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?

  75. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-18T17:24:44Z

    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

  76. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-18T17:27:43Z

    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

  77. Re: Possibility to disable `ALTER SYSTEM`

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-03-18T17:37:45Z

    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

  78. Re: Possibility to disable `ALTER SYSTEM`

    Robert Treat <rob@xzilla.net> — 2024-03-18T20:07:35Z

    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

  79. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-18T20:59:37Z

    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

  80. Re: Possibility to disable `ALTER SYSTEM`

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-03-18T23:38:07Z

    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

  81. Re: Possibility to disable `ALTER SYSTEM`

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-03-19T09:26:20Z

    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)

  82. Re: Possibility to disable `ALTER SYSTEM`

    Andrew Dunstan <andrew@dunslane.net> — 2024-03-19T11:49:10Z

    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

  83. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-19T13:13:21Z

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

  84. Re: Possibility to disable `ALTER SYSTEM`

    Joe Conway <mail@joeconway.com> — 2024-03-19T14:39:25Z

    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

  85. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-19T14:51:50Z

    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

  86. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-19T15:36:43Z

    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

  87. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-19T16:05:20Z

    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

  88. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-19T16:53:59Z

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

  89. Re: Possibility to disable `ALTER SYSTEM`

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-03-19T16:56:01Z

    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

  90. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-19T17:56:08Z

    > 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

  91. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-19T17:57:10Z

    > 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

  92. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-19T18:28:12Z

    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

  93. Re: Possibility to disable `ALTER SYSTEM`

    Wolfgang Walther <walther@technowledgy.de> — 2024-03-19T20:53:46Z

    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

  94. Re: Possibility to disable `ALTER SYSTEM`

    Andrew Dunstan <andrew@dunslane.net> — 2024-03-19T22:24:47Z

    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

  95. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-19T22:35:27Z

    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

  96. Re: Possibility to disable `ALTER SYSTEM`

    Michael Banck <mbanck@gmx.net> — 2024-03-20T09:30:54Z

    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

  97. Re: Possibility to disable `ALTER SYSTEM`

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-03-20T13:04:21Z

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

  98. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-20T15:06:56Z

    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 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/ :)


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

  99. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-20T19:03:38Z

    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?

--
Robert Haas
EDB: http://www.enterprisedb.com

  100. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-20T19:11:32Z

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

Yeah, I don't see why it's our responsibility to decide what permissions
people should have on their config files.

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

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  101. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-20T19:14:47Z

    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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  102. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-20T19:16:52Z

    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.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  103. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-20T19:52:04Z

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

  104. Re: Possibility to disable `ALTER SYSTEM`

    Michael Banck <mbanck@gmx.net> — 2024-03-20T19:55:16Z

    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

  105. Re: Possibility to disable `ALTER SYSTEM`

    David Steele <david@pgmasters.net> — 2024-03-20T21:21:08Z

    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

  106. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-21T02:30:43Z

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

--
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  107. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-21T12:42:04Z

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

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

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

  108. Re: Possibility to disable `ALTER SYSTEM`

    Robert Treat <rob@xzilla.net> — 2024-03-21T15:37:39Z

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

And yet somehow query hints are more scary than ALL of these things. Go figure!

Robert Treat
https://xzilla.net

  109. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-25T17:29:46Z

    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

  110. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-25T17:47:13Z

    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

  111. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-25T18:13:21Z

    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

  112. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-25T18:26:55Z

    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

  113. Re: Possibility to disable `ALTER SYSTEM`

    Magnus Hagander <magnus@hagander.net> — 2024-03-25T18:30:03Z

    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.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  114. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-25T18:45:40Z

    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

  115. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-25T19:16:09Z

    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.

  116. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-25T20:40:55Z

    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"

  117. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-25T21:04:31Z

    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.

  118. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-26T12:11:33Z

    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

  119. Re: Possibility to disable `ALTER SYSTEM`

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-26T12:15:25Z

    > 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

  120. Re: Possibility to disable `ALTER SYSTEM`

    Abhijit Menon-Sen <ams@toroid.org> — 2024-03-26T12:55:04Z

    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)

  121. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-26T13:43:58Z

    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

  122. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-26T14:23:51Z

    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

  123. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-26T16:35:39Z

    On Tue, Mar 26, 2024 at 10:23:51AM -0400, Tom Lane wrote:
> 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.

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.

Will people think this allows non-root users to use ALTER SYSTEM if
enabled?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

  124. Re: Possibility to disable `ALTER SYSTEM`

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-26T17:23:56Z

    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.

> Will people think this allows non-root users to use ALTER SYSTEM if
> enabled?

They'll soon find out differently, so I'm not concerned about that.

			regards, tom lane

  125. Re: Possibility to disable `ALTER SYSTEM`

    Andrew Dunstan <andrew@dunslane.net> — 2024-03-27T01:23:55Z

    
> 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

  126. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-27T14:43:28Z

    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.

  127. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T15:01:28Z

    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.

  128. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-27T15:05:55Z

    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

  129. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-27T15:10:31Z

    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

  130. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-27T15:50:27Z

    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.

  131. Re: Possibility to disable `ALTER SYSTEM`

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-03-27T17:05:08Z

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

  132. Re: Possibility to disable `ALTER SYSTEM`

    Isaac Morland <isaac.morland@gmail.com> — 2024-03-27T17:12:17Z

    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.

  133. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-03-27T17:34:57Z

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

Prevent non-malicious modifications via ALTER SYSTEM in environments where
...

David J.

  134. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-27T18:46:01Z

    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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  135. Re: Possibility to disable `ALTER SYSTEM`

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-03-27T19:10:02Z

    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.

>

  136. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T22:06:32Z

    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.

  137. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T22:09:02Z

    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.

  138. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T22:13:22Z

    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.

  139. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-03-27T22:18:26Z

    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.

  140. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-03-27T22:20:38Z

    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.

  141. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T22:23:54Z

    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.

  142. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T22:24:15Z

    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.

  143. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-27T23:00:43Z

    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.

  144. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-27T23:43:29Z

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

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.

  145. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-27T23:47:46Z

    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.

  146. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-27T23:58:56Z

    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.

  147. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-28T00:17:09Z

    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.

  148. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-03-28T00:43:06Z

    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.
https://www.postgresql.org/docs/current/config-setting.html#CONFIG-INCLUDES
"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.

  149. Re: Possibility to disable `ALTER SYSTEM`

    David G. Johnston <david.g.johnston@gmail.com> — 2024-03-28T00:45:50Z

    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.

  150. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-28T09:24:34Z

    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.

  151. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-28T09:41:50Z

    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)

  152. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-28T11:57:24Z

    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

  153. Re: Possibility to disable `ALTER SYSTEM`

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-28T12:23:36Z

    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.

  154. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-28T12:38:24Z

    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

  155. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-28T17:45:54Z

    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.

  156. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-28T18:43:38Z

    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

  157. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-28T19:33:00Z

    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.

  158. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-28T20:24:05Z

    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.

  159. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-29T12:46:33Z

    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

  160. Re: Possibility to disable `ALTER SYSTEM`

    Bruce Momjian <bruce@momjian.us> — 2024-03-29T14:47:43Z

    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.

  161. Re: Possibility to disable `ALTER SYSTEM`

    Robert Haas <robertmhaas@gmail.com> — 2024-03-29T16:06:08Z

    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