Possibility to disable `ALTER SYSTEM`

On 9/7/23 15:51, Gabriele Bartolini wrote:

I would like to propose a patch that allows administrators to disable
`ALTER SYSTEM` via either a runt-time option to pass to the Postgres
server process at startup (e.g. `--disable-alter-system=true`, false by
default) or a new GUC (or even both), without changing the current
default method of the server.

Without trying to debate the particulars, a huge +1 for the concept of
allowing ALTER SYSTEM to be disabled. FWIW I would vote the same for
control over COPY PROGRAM.

Not coincidentally both concepts were built into set_user:
https://github.com/pgaudit/set_user

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi Joe,

On Thu, 7 Sept 2023 at 21:57, Joe Conway <mail@joeconway.com> wrote:

Without trying to debate the particulars, a huge +1 for the concept of
allowing ALTER SYSTEM to be disabled. FWIW I would vote the same for
control over COPY PROGRAM.

Oh ... another one of my favourite security friendly features! :)

That sounds like a good idea to me.

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

Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> writes:

I would like to propose a patch that allows administrators to disable
`ALTER SYSTEM` via either a runt-time option to pass to the Postgres server
process at startup (e.g. `--disable-alter-system=true`, false by default)
or a new GUC (or even both), without changing the current default method of
the server.

ALTER SYSTEM is already heavily restricted. I don't think we need random
kluges added to the permissions system. I especially don't believe in
kluges to the effect of "superuser doesn't have all permissions anymore".

If you nonetheless feel that that's a good idea for your use case,
you can implement the restriction with an event trigger or the like.

regards, tom lane

Hi Tom,

On Thu, 7 Sept 2023 at 22:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> writes:

I would like to propose a patch that allows administrators to disable
`ALTER SYSTEM` via either a runt-time option to pass to the Postgres

server

process at startup (e.g. `--disable-alter-system=true`, false by default)
or a new GUC (or even both), without changing the current default method

of

the server.

ALTER SYSTEM is already heavily restricted.

Could you please help me better understand what you mean here?

I don't think we need random kluges added to the permissions system.

If you allow me, why do you think disabling ALTER SYSTEM altogether is a
random kluge? Again, I'd like to better understand this position. I've
personally been in many conversations on the security side of things for
Postgres in Kubernetes environments, and this is a frequent concern by
users who request that changes to the Postgres system (not a database)
should only be done declaratively and prevented from within the system.

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

On Fri, 8 Sept 2023 at 10:03, Gabriele Bartolini <
gabriele.bartolini@enterprisedb.com> wrote:

ALTER SYSTEM is already heavily restricted.

Could you please help me better understand what you mean here?

I don't think we need random kluges added to the permissions system.

If you allow me, why do you think disabling ALTER SYSTEM altogether is a
random kluge? Again, I'd like to better understand this position. I've
personally been in many conversations on the security side of things for
Postgres in Kubernetes environments, and this is a frequent concern by
users who request that changes to the Postgres system (not a database)
should only be done declaratively and prevented from within the system.

Alternate idea, not sure how good this is: Use existing OS security
features (regular permissions, or more modern features such as the
immutable attribute) to mark the postgresql.auto.conf file as not being
writeable. Then any attempt to ALTER SYSTEM should result in an error.

Hi Isaac,

On Fri, 8 Sept 2023 at 16:11, Isaac Morland <isaac.morland@gmail.com> wrote:

Alternate idea, not sure how good this is: Use existing OS security
features (regular permissions, or more modern features such as the
immutable attribute) to mark the postgresql.auto.conf file as not being
writeable. Then any attempt to ALTER SYSTEM should result in an error.

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

On 2023-Sep-08, Gabriele Bartolini wrote:

That is the point I highlighted in the initial post in the thread. We could
make it readonly, but the returned error is misleading and definitely poor
UX:

```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR: could not open file "postgresql.auto.conf": Permission denied
```

IMO we should clearly state that `ALTER SYSTEM` is deliberately disabled in
a system, rather than indirectly hinting it through an inaccessible file.
Not sure if I am clearly highlighting the fine difference here.

Come on. This is not a "fine difference" -- it's the difference between
a crummy hack and a real implementation of an important system
restriction.

I don't understand Tom's resistance to this request. I understand the
use case and I agree with Gabriele that this is a very simple code
change(*) that Postgres could make to help it get run better in a
different kind of environment than what we're accustomed to.

I've read that containers people consider services in a different light
than how we've historically seen them; they say "cattle, not pets".
This affects the way you think about these services. postgresql.conf
(all the PG configuration, really) is just a derived file from an
overall system description that lives outside the database server. You
no longer feed your PG servers one by one, but rather they behave as a
herd, and the herder is some container supervisor (whatever it's called).

Ensuring that the configuration state cannot change from within is
important to maintain the integrity of the service. If the user wants
to change things, the tools to do that are operated from outside; this
lets things like ancillary services to be kept in sync (say, start a
replica here, or a backup system there, or WAL archival/collection is
handled in this or that way). If users are allowed to change the config
from within they break things, and the supervisor program can't put
things together again.

I did not like the mention of COPY PROGRAM, though, and in principle I
do not support the idea of treating it the same way as ALTER SYSTEM. If
people are using that to write into postgresql.conf, then they deserve
all the hell they get when their containers break. I think trying to
restrict this outside of the privilege system is going to be more of a
wart than ALTER SYSTEM.

(*) To be proven. Let's see the patch.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

I don't understand Tom's resistance to this request.

It's false security. If you think you are going to prevent a superuser
from messing with the system's configuration, you are going to need a
lot more restrictions than this, and we'll be forever getting security
reports that "hey, I found another way for a superuser to get filesystem
access". I think the correct answer to this class of problems is "don't
give superuser privileges to clients running inside the container".

I did not like the mention of COPY PROGRAM, though, and in principle I
do not support the idea of treating it the same way as ALTER SYSTEM.

It's one of the easiest ways to modify postgresql.conf from SQL. If you
don't block that off, the feature is certainly not secure. (But of
course, there are more ways.)

regards, tom lane

Hi Tom and Alvaro,

On Fri, 8 Sept 2023 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

I don't understand Tom's resistance to this request.

It's false security. If you think you are going to prevent a superuser
from messing with the system's configuration, you are going to need a
lot more restrictions than this, and we'll be forever getting security
reports that "hey, I found another way for a superuser to get filesystem
access". I think the correct answer to this class of problems is "don't
give superuser privileges to clients running inside the container".

Ok, this is clearer. That makes sense now, and this probably helps me
explain better the goal here. I also omitted in the initial email all the
security precautions that a Kubernetes should take. This could be another
step towards that direction but, you are right, it won't fix it entirely
(in case of malicious superusers).

In my opinion, the biggest benefit of this possibility is on the usability
side, providing a clear and configurable way to disable ALTER SYSTEM in
those environments where declarative configuration is a requirement. For
example, this should at least "warn" human beings that have the permissions
to connect to a Postgres database (think of SREs managing a DBaaS solution
or a DBA) and try to change a setting in an instance. Moreover, for those
who are managing through declarative configuration not only one instance,
but a Postgres cluster that controls standby instances too, the benefit of
impeding these modifications could be even higher (think of the hot standby
sensitive parameters like max_connections that require coordination
depending whether you increase or decrease them).

I hope this is clearer. For what it's worth, I have done a basic PoC patch
(roughly 20 lines of code), which I have attached here just to provide some
basis for further analysis and comments. The general idea is to disable
ALTER SYSTEM at startup, like this:

pg_ctl start -o "-c enable_alter_system=off"

The setting can be verified with:

psql -c 'SHOW enable_alter_system'
enable_alter_system
---------------------
off
(1 row)

And then:

psql -c 'ALTER SYSTEM SET max_connections TO 10'
ERROR: permission denied to run ALTER SYSTEM

Thanks for your attention and looking forward to getting feedback and
advice.

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

On Fri, Sep 8, 2023 at 5:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

I don't understand Tom's resistance to this request.

It's false security. If you think you are going to prevent a superuser
from messing with the system's configuration, you are going to need a
lot more restrictions than this, and we'll be forever getting security
reports that "hey, I found another way for a superuser to get filesystem
access". I think the correct answer to this class of problems is "don't
give superuser privileges to clients running inside the container".

+1. And to make that happen, the appropriate thing is to identify
*why* they are using superuser today, and focus efforts on finding
ways for them to do that without being superuser.

I did not like the mention of COPY PROGRAM, though, and in principle I
do not support the idea of treating it the same way as ALTER SYSTEM.

It's one of the easiest ways to modify postgresql.conf from SQL. If you
don't block that off, the feature is certainly not secure. (But of
course, there are more ways.)

It's easier to just create a function in an untrusted language. Same
principle. Once you're superuser, you can break through anything.

We need a "allowlist" of things a user can do, rather than a blocklist
of "they can do everything they can possibly think of and a computer
is capable of doing, except for this one specific thing". Blocklisting
individual permissions of a superuser will never be secure.

Now, it might be that you don't care at all about the *security* side
of the feature, and only care about the convenience side. But in that
case, the original suggestion from Tom of using an even trigger seems
like a fine enough solution?

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

On 7/9/23 21:51, Gabriele Bartolini wrote:

Hi everyone,

I would like to propose a patch that allows administrators to disable
`ALTER SYSTEM` via either a runt-time option to pass to the Postgres
server process at startup (e.g. `--disable-alter-system=true`, false
by default) or a new GUC (or even both), without changing the current
default method of the server.

The main reason is that this would help improve the “security by
default” posture of Postgres in a Kubernetes/Cloud Native environment
- and, in general, in any environment on VMs/bare metal behind a
configuration management system in which changes should only be made
in a declarative way and versioned like Ansible Tower, to cite one.

Below you find some background information and the longer story behind
this proposal.

Sticking to the Kubernetes use case, I am primarily speaking on behalf
of the CloudNativePG open source operator (cloudnative-pg.io
<http://cloudnative-pg.io&gt;, of which I am one of the maintainers).
However, I am sure that this option could benefit any operator for
Postgres - an operator is the most common and recommended way to run a
complex application like a PostgreSQL database management system
inside Kubernetes.

In this case, the state of a PostgreSQL cluster (for example its
number of replicas, configuration, storage, etc.) is defined in a
Custom Resource Definition in the form of configuration, typically
YAML, and the operator works with Kubernetes to ensure that, at any
moment, the requested Postgres cluster matches the observed one. This
is a very basic example in CloudNativePG:
https://cloudnative-pg.io/documentation/current/samples/cluster-example.yaml

As I was mentioning above, in a Cloud Native environment it is
expected that workloads are secure by default. Without going into much
detail, many decisions have been made in that direction by operators
for Postgres, including CloudNativePG. The goal of this proposal is to
provide a way to ensure that changes to the PostgreSQL configuration
in a Kubernetes controlled Postgres cluster are allowed only through
the Kubernetes API.

Basically, if you want to change an option for PostgreSQL, you need to
change the desired state in the Kubernetes resource, then Kubernetes
will converge (through the operator). In simple words, it’s like
empowering the operator to impersonate the PostgreSQL superuser.

    Coming from a similar background to Gabriele's, I support this
proposal.

    In StackGres (https://stackgres.io) we also allow users to manage
postgresql.conf's configuration declaratively. We have a CRD (Custom
Resource Definition) that precisely defines and controls how a
postgresql.conf configuration looks like (see
https://stackgres.io/doc/latest/reference/crd/sgpgconfig/). This
configuration, once created by the user, is strongly validated by
StackGres (parameters are valid for the given major version, values are
within the ranges and appropriate types) and then periodically applied
to the database if there's any drift between that user-declared
(desired) state and current system status.

    Similarly, we also have some parameters which the user is not
allowed to change
(https://gitlab.com/ongresinc/stackgres/-/blob/main/stackgres-k8s/src/operator/src/main/resources/postgresql-blocklist.properties).
If the user is allowed to use ALTER SYSTEM and modify some of these
parameters, significant breakage can happen in the cluster, as the
operator may become "confused" and manual operation may be required,
breaking many of the user's expectations of stability and how the system
works and heals automatically.

    Sure, as mentioned elsewhere in the thread, a "malicious" user can
still use other mechanisms such as COPY or untrusted PLs to still
overwrite the configuration. But both attempts are obviously conscious
attempts to break the system (and if so, it's all yours to break it).
But ALTER SYSTEM may be an *unintended* way to break it, causing a bad
user's experience. This may be defined more of a way to avoid users
shooting themselves in the feet, inadvertedly.

    There's apparently some opposition to implementing this. But given
that there's also interest in having it, I'd like to know what the
negative effects of implementing such a startup configuration property
would be, so that advantages can be compared with the disadvantages.

    All that being said, the behavior to prevent ALTER SYSTEM can also
be easily implemented as an extension. Actually some colleagues wrote
one with a similar scope
(https://gitlab.com/ongresinc/extensions/noset), and I believe it could
be the base for a similar extension focused on preventing ALTER SYSTEM.

    Regards,

    Álvaro

--

Alvaro Hernandez

-----------
OnGres

Hi Magnus,

On Fri, 8 Sept 2023 at 23:43, Magnus Hagander <magnus@hagander.net> wrote:

+1. And to make that happen, the appropriate thing is to identify
*why* they are using superuser today, and focus efforts on finding
ways for them to do that without being superuser.

As I am explaining in the other post (containing a very basic proof of
concept patch), it is not about restricting superuser. It is primarily a
usability and configuration matter of a running instance, which helps
control an entire cluster like in the case of Kubernetes (where, in order
to provide self-healing and high availability we are forced to go beyond
the single instance and think in terms of primary with one or more standbys
or at least continuous backup in place).

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

On 2023-Sep-08, Magnus Hagander wrote:

Now, it might be that you don't care at all about the *security* side
of the feature, and only care about the convenience side. But in that
case, the original suggestion from Tom of using an even trigger seems
like a fine enough solution?

ALTER SYSTEM, like all system-wide commands, does not trigger event
triggers. These are per-database only.

https://www.postgresql.org/docs/16/event-trigger-matrix.html

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)

Hi,

I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option to 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

On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Sep-08, Magnus Hagander wrote:

Now, it might be that you don't care at all about the *security* side
of the feature, and only care about the convenience side. But in that
case, the original suggestion from Tom of using an even trigger seems
like a fine enough solution?

ALTER SYSTEM, like all system-wide commands, does not trigger event
triggers. These are per-database only.

https://www.postgresql.org/docs/16/event-trigger-matrix.html

Hah, didn't think of that. And yes, that's a very good point. But one
way to fix that would be to actually make event triggers for system
wide commands, which would then be useful for other things as well...

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

On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin.marques@gmail.com> wrote:

Hi,

I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option 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/

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

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

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