Re: Possibility to disable `ALTER SYSTEM`

On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin(dot)marques(at)gmail(dot)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/