Re: Possibility to disable `ALTER SYSTEM`

Greetings,

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin(dot)marques(at)gmail(dot)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