Обсуждение: 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

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

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

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/

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.

-- 

Alvaro Hernandez


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

On 2023-Sep-08, Magnus Hagander wrote:

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

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

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

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

Hi,

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

I'm actually going to put a strong +1 to Gabriele's proposal. It's an
undeniable problem (I'm only seeing arguments regarding other ways the
system would be insecure), and there might be real use cases for users
outside kubernetes for having this feature and using it (meaning
disabling the use of ALTER SYSTEM).

In Patroni for example, the PostgreSQL service is controlled on all
nodes by Patroni, and these kinds of changes could end up breaking the
cluster if there was a failover. For this reason Patroni starts
postgres with some GUC options as CMD arguments so that values in
postgresql.conf or postgresql.auto.conf are ignored. The values in the
DCS are the ones that matter.

```
postgres 1171221  0.0  1.9 903560 55168 ?        S    10:16   0:00
/usr/pgsql-15/bin/postgres -D /opt/postgres/data
--config-file=/opt/postgres/data/postgresql.conf
--listen_addresses=0.0.0.0 --port=5432 --cluster_name=patroni-tpa
--wal_level=logical --hot_standby=on --max_connections=250
--max_wal_senders=6 --max_prepared_transactions=0
--max_locks_per_transaction=64 --track_commit_timestamp=off
--max_replication_slots=6 --max_worker_processes=16 --wal_log_hints=on
```

(see more about how Patroni manages this here:
https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni

But let's face it, that's a hack, not something to be proud of, even
if it does what is intended. And this is a product and we shouldn't be
advertising hacks to overcome limitations.

I find the opposition to this lacking good reasons, while I find the
implementation to be useful in some cases.

Kind regards, Martín


--
Martín Marqués
It’s not that I have something to hide,
it’s that I have nothing I want you to see

On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Sep-08, Magnus Hagander wrote:
>
> > Now, it might be that you don't care at all about the *security* side
> > of the feature, and only care about the convenience side. But in that
> > case, the original suggestion from Tom of using an even trigger seems
> > like a fine enough solution?
>
> ALTER SYSTEM, like all system-wide commands, does not trigger event
> triggers.  These are per-database only.
>
> https://www.postgresql.org/docs/16/event-trigger-matrix.html

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

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

On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin.marques@gmail.com> wrote:
>
> Hi,
>
> > I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time option
topass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new GUC
(oreven both), without changing the current default method of the server. 
>
> I'm actually going to put a strong +1 to Gabriele's proposal. It's an
> undeniable problem (I'm only seeing arguments regarding other ways the
> system would be insecure), and there might be real use cases for users
> outside kubernetes for having this feature and using it (meaning
> disabling the use of ALTER SYSTEM).

If enough people are in favor of it *given the known issues with it*,
I can drop my objection to a "meh, but I still don't think it's a good
idea".

But to do that, there would need to be a very in-your-face warning in
the documentation about it like "note that this only disables the
ALTER SYSTEM command. It does not prevent a superuser from changing
the configuration remotely using other means".

For example, in the very simplest, wth the POC patch out there now, I
can still run:
postgres=# CREATE TEMP TABLE x(t text);
CREATE TABLE
postgres=# INSERT INTO x VALUES ('work_mem=1TB');
INSERT 0 1
postgres=# COPY x TO '/home/mha/postgresql/inst/head/data/postgresql.auto.conf';
COPY 1
postgres=# SELECT pg_reload_conf();
 pg_reload_conf
----------------
 t
(1 row)
postgres=# show work_mem;
 work_mem
----------
 1TB
(1 row)


Or anything similar to that.

Yes, this is marginally harder than saying ALTER SYSTEM SET
work_mem='1TB', but only very very marginally so. And from a security
perspective, there is zero difference.

But we do also allow "trust" authentication which is another major
footgun from a security perspective, against which we only defend with
warnings, so that in itself is not a reason not to do the same here.


> In Patroni for example, the PostgreSQL service is controlled on all
> nodes by Patroni, and these kinds of changes could end up breaking the
> cluster if there was a failover. For this reason Patroni starts
> postgres with some GUC options as CMD arguments so that values in
> postgresql.conf or postgresql.auto.conf are ignored. The values in the
> DCS are the ones that matter.

Right. And patroni would need to continue to do that even with this
patch, because it also needs to override if somebody puts something in
postgresql.conf, no? Removing the defence against that seems like a
bad idea...


> (see more about how Patroni manages this here:
> https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni
>
> But let's face it, that's a hack, not something to be proud of, even
> if it does what is intended. And this is a product and we shouldn't be
> advertising hacks to overcome limitations.

It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd
also have to manage postgresql.conf. One slightly less hacky part
might be to have patroni generate a config file of it's own and
include it with the highest priority -- at that point it *would* be
come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher
priority than any manual user config file. But it is not today.

Another idea to solve the problem could be to instead introduce a
specific configuration file (either hardcoded or an
include_final_parameter=<path> parameter, in which case patroni or the
k8s operator could set that parameter on the command line and that
parameter only) that is parsed *after* postgresql.auto.conf and
thereby would override the manual settings. This file would explicilty
be documented as intended for this type of tooling, and when you have
a tool - whether patroni or another declarative operator - it owns
this file and can overwrite it with whatever it wants. And this would
also retain the ability to use ALTER SYSTEM SET for *other*
parameters, if they want to.

That's just a very quick idea and there may definitely be holes in it,
but I'm not sure those holes are any worse than what's suggested here,
and I do thin kit's cleaner.

> I find the opposition to this lacking good reasons, while I find the
> implementation to be useful in some cases.

Stopping ALTER SYSTEM SET without actually preventing the superuser
from doing the same thing as they were doing before would to me be at
least as much of a hack as what patroni does today is.

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

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués <martin.marques@gmail.com> wrote:
> > > I would like to propose a patch that allows administrators to disable `ALTER SYSTEM` via either a runt-time
optionto pass to the Postgres server process at startup (e.g. `--disable-alter-system=true`, false by default) or a new
GUC(or even both), without changing the current default method of the server. 
> >
> > I'm actually going to put a strong +1 to Gabriele's proposal. It's an
> > undeniable problem (I'm only seeing arguments regarding other ways the
> > system would be insecure), and there might be real use cases for users
> > outside kubernetes for having this feature and using it (meaning
> > disabling the use of ALTER SYSTEM).
>
> If enough people are in favor of it *given the known issues with it*,
> I can drop my objection to a "meh, but I still don't think it's a good
> idea".

A lot of the objections seem to be on the grounds of returning a
'permission denied' kind of error and I generally agree with that being
the wrong approach.

As an alternative idea- what if we had something in postgresql.conf
along the lines of:

include_alter_system = true/false

and use that to determine if the postgresql.auto.conf is included or
not..?

> But to do that, there would need to be a very in-your-face warning in
> the documentation about it like "note that this only disables the
> ALTER SYSTEM command. It does not prevent a superuser from changing
> the configuration remotely using other means".

With the above, we could throw a WARNING or maybe just NOTICE when ALTER
SYSTEM is run that 'include_alter_system is false and therefore these
changes won't be included in the running configuration' or similar.

What this does cause problems with is that pg_basebackup and other tools
(eg: pgbackrest) write into postgresql.auto.conf currently and we'd want
those to still work.  That's an opportunity, imv, though, since I don't
really think where ALTER SYSTEM writes to and where backup/restore
tools are writing to should really be the same place anyway.  Therefore,
perhaps we add a 'postgresql.system.conf' or similar and maybe a
corresponding option in postgresql.conf to include it or not.

> For example, in the very simplest, wth the POC patch out there now, I
> can still run:
> postgres=# CREATE TEMP TABLE x(t text);
> CREATE TABLE
> postgres=# INSERT INTO x VALUES ('work_mem=1TB');
> INSERT 0 1
> postgres=# COPY x TO '/home/mha/postgresql/inst/head/data/postgresql.auto.conf';
> COPY 1
> postgres=# SELECT pg_reload_conf();
>  pg_reload_conf
> ----------------
>  t
> (1 row)
> postgres=# show work_mem;
>  work_mem
> ----------
>  1TB
> (1 row)
>
> Or anything similar to that.

This is an issue if you're looking at it as a security thing.  This
isn't an issue if don't view it that way.  Still, I do see some merit in
making it so that you can't actually change the config that's loaded at
system start from inside the data directory or as the PG superuser,
which my proposal above would support- just configure in postgresql.conf
to not include any of the alter-system or generated config.  The actual
postgresql.conf could be owned by root then too.

> > In Patroni for example, the PostgreSQL service is controlled on all
> > nodes by Patroni, and these kinds of changes could end up breaking the
> > cluster if there was a failover. For this reason Patroni starts
> > postgres with some GUC options as CMD arguments so that values in
> > postgresql.conf or postgresql.auto.conf are ignored. The values in the
> > DCS are the ones that matter.
>
> Right. And patroni would need to continue to do that even with this
> patch, because it also needs to override if somebody puts something in
> postgresql.conf, no? Removing the defence against that seems like a
> bad idea...
>
>
> > (see more about how Patroni manages this here:
> > https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni
> >
> > But let's face it, that's a hack, not something to be proud of, even
> > if it does what is intended. And this is a product and we shouldn't be
> > advertising hacks to overcome limitations.
>
> It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd
> also have to manage postgresql.conf. One slightly less hacky part
> might be to have patroni generate a config file of it's own and
> include it with the highest priority -- at that point it *would* be
> come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher
> priority than any manual user config file. But it is not today.

I suppose we could invent a priority control thing as part of the above
proposal too.. but I would think just having include_alter_system and
include_auto_config (or whatever we name them) would be enough, with the
auto-config bit being loaded last and therefore having the 'highest'
priority.

> Another idea to solve the problem could be to instead introduce a
> specific configuration file (either hardcoded or an
> include_final_parameter=<path> parameter, in which case patroni or the
> k8s operator could set that parameter on the command line and that
> parameter only) that is parsed *after* postgresql.auto.conf and
> thereby would override the manual settings. This file would explicilty
> be documented as intended for this type of tooling, and when you have
> a tool - whether patroni or another declarative operator - it owns
> this file and can overwrite it with whatever it wants. And this would
> also retain the ability to use ALTER SYSTEM SET for *other*
> parameters, if they want to.

Yeah, this is along the lines of what I propose above, but with the
addition of having a way to control if these are loaded or not in the
first place, instead of having to deal with every possible option that
might be an issue.

Generally, I do think having a separate file for tools to write into
that's independent of ALTER SYSTEM would just be a good idea.  I don't
care for the way those are mixed in the same file these days.

> That's just a very quick idea and there may definitely be holes in it,
> but I'm not sure those holes are any worse than what's suggested here,
> and I do thin kit's cleaner.

Perhaps not surprising, I tend to agree that something along these lines
is cleaner.

Thanks,

Stephen

Hi,

> Maybe in addition to making "ALTER SYSTEM" throw an error, the feature that disables it should also disable reading
postgresql.auto.conf?Maybe even delete it and make it an error if it is present on startup (maybe even warn if it shows
upwhile the DB is running?). 

The outcome looked for is that the system GUCs that require a restart
or reload are not modified unless it's through some orchestration or
someone with physical access to the configuration files (yeah, we
still have the COPY PROGRAM).

We shouldn't mix this with not reading postgresql.auto.conf, or even
worse, deleting it. I don't think it's a good idea to delete the file.
Ignoring it might be of interest, but completely outside the scope of
the intention I'm seeing from the k8s teams.

> Counterpoint: maybe the idea is to disable ALTER SYSTEM but still use postgresql.auto.conf, maintained by an external
program,to control the instance's behaviour. 

I believe that's the idea, although we have `include` and
`include_dir` which can be used the same way as `postgresql.auto.conf`
is automatically included.

Kind regards, Martín

--
Martín Marqués
It’s not that I have something to hide,
it’s that I have nothing I want you to see

> 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

On Tue, Sep 12, 2023 at 10:39 AM Martín Marqués
<martin.marques@gmail.com> wrote:
> The outcome looked for is that the system GUCs that require a restart
> or reload are not modified unless it's through some orchestration or
> someone with physical access to the configuration files (yeah, we
> still have the COPY PROGRAM).

If I understand this correctly, you're saying it's not a security
vulnerability if someone finds a way to use COPY PROGRAM or some other
mechanism to bypass the ALTER SYSTEM restriction, because the point of
the constraint isn't to make it impossible for the superuser to modify
the configuration in a way that they shouldn't, but rather to make it
inconvenient for them to do so.

I have to admit that I'm a little afraid that people will mistake this
for an actual security feature and file bug reports or CVEs about the
superuser being able to circumvent these restrictions. If we add this,
we had better make sure that the documentation is extremely clear
about what we are guaranteeing, or more to the point about what we are
not guaranteeing.

I understand that there's some frustration on the part of Gabriele and
others that this proposal hasn't been enthusiastically adopted, but I
would ask for a little bit of forbearance because those are also, by
and large, not the people who will not have to cope with it when we
start getting security researchers threatening to publish our evilness
in the Register. Such conversations are no fun at all. Explaining that
we're not actually evil doesn't tend to work, because the security
researchers are just as convinced that they are right as anyone
arguing for this feature is. Statements like "we don't actually intend
to guarantee X" tend to fall on deaf ears.

In fact, I would go so far as to argue that many of our security
problems (and non-problems) are widely misunderstood even within our
own community, and that far from being something anyone should dismiss
as pedantry, it's actually a critical issue for the project to solve
and something we really need to address in order to be able to move
forward. From that point of view, this feature seems bound to make an
already-annoying problem worse. I don't necessarily expect the people
who are in favor of this feature to accept that as a reason not to do
this, but I do hope to be taken seriously when I say there's a real
issue there. Something can be a serious problem even if it's not YOUR
problem, and in this case, that apparently goes both ways.

I also think that using the GUC system to manage itself is a little
bit suspect. I wonder if it would be better to do this some other way,
e.g. a sentinel file in the data directory. For example, suppose we
refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or
something like that. It seems like it would be very easy for an
external management solution (k8s or whatever) to drop that file in
place if desired, and then it would be crystal clear that there's no
way of bypassing the restriction from within the GUC system itself
(though you could still bypass it via filesystem access).

I agree with those who have said that this shouldn't disable
postgresql.auto.conf, but only the ability of ALTER SYSTEM to modify
it. Right now, third-party tools external to the server can count on
being able to add things to postgresql.auto.conf with the reasonable
expectations that they'll take effect. I'd rather not break that
property.

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

Robert Haas <robertmhaas@gmail.com> writes:
> I have to admit that I'm a little afraid that people will mistake this
> for an actual security feature and file bug reports or CVEs about the
> superuser being able to circumvent these restrictions. If we add this,
> we had better make sure that the documentation is extremely clear
> about what we are guaranteeing, or more to the point about what we are
> not guaranteeing.

> I understand that there's some frustration on the part of Gabriele and
> others that this proposal hasn't been enthusiastically adopted, but I
> would ask for a little bit of forbearance because those are also, by
> and large, not the people who will not have to cope with it when we
> start getting security researchers threatening to publish our evilness
> in the Register. Such conversations are no fun at all.

Indeed.  I'd go so far as to say that we should reject not only this
proposal, but any future ones that intend to prevent superusers from
doing things that superusers normally could do (and, indeed, are
normally expected to do).  That sort of thing is not part of our
security model, never has been, and it's simply naive to believe that
it won't have a boatload of easily-reachable holes in it.  Which we
*will* get complaints about, if we claim that thus-and-such feature
prevents it.  So why bother?  Don't give out superuser to people you
don't trust to not do the things you wish they wouldn't.

> I also think that using the GUC system to manage itself is a little
> bit suspect.

Something like contrib/sepgsql would be a better mechanism, perhaps.

            regards, tom lane

On Tue, Jan 30, 2024 at 2:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Indeed.  I'd go so far as to say that we should reject not only this
> proposal, but any future ones that intend to prevent superusers from
> doing things that superusers normally could do (and, indeed, are
> normally expected to do).  That sort of thing is not part of our
> security model, never has been, and it's simply naive to believe that
> it won't have a boatload of easily-reachable holes in it.  Which we
> *will* get complaints about, if we claim that thus-and-such feature
> prevents it.  So why bother?  Don't give out superuser to people you
> don't trust to not do the things you wish they wouldn't.

In my opinion, we need to have the conversation, whereas you seem to
want to try to shut it down before it starts. If we take that
approach, people are going to get (more) frustrated.

Also in my opinion, there is a fair amount of nuance here. On the one
hand, I and others put a lot of work into making it possible to not
give people superuser and still be able to do a controlled subset of
the things that a superuser can do. For example, thanks to Mark
Dilger's work, you can make somebody not a superuser and still allow
them to set GUCs that can normally be set only by superusers, and you
can choose which GUCs you do and do not want them to be able to set.
And, thanks to my work, you can make someone a CREATEROLE user without
letting them escalate to superuser, and you can then allow them to
manage the users that they create almost exactly as if they were a
superuser, with only the limitations that seem necessary to maintain
system security. It is worth asking - and I would like to hear a real,
non-flip answer - why someone who wants to do what is proposed here
isn't using those mechanisms instead of handing out SUPERUSER and then
complaining that it grants too much power.

On the other hand, I don't see why it isn't legitimate to imagine a
scenario where there is no security boundary between the Kubernetes
administrator and the PostgreSQL DBA, and yet the PostgreSQL DBA
should still be pushed in the direction of doing things in a way that
doesn't break Kubernetes. It surprises me a little bit that Gabriele
and others want to build the system that way, though, because you
might expect that in a typical install the Kubernetes administrator
would want to FORCIBLY PREVENT the PostgreSQL administrator from
messing things up instead of doing what is proposed here, which
amounts to suggesting perhaps the PostgreSQL administrator would be
kind enough not to mess things up. Nonetheless, there's no law against
suggestions. When my wife puts the ground beef that I'm supposed to
use to cook dinner at the top of the freezer and the stuff I'm
supposed to not use at the bottom, nothing prevents me from digging
out the other ground beef and using it, but I don't, because I can
take a hint. And indeed, I benefit from that hint. This seems like it
could be construed as a very similar type of hint.

I don't think we should pretend like one of the two paragraphs above
is valid and the other is hot garbage. That's not solving anything. We
can't resolve the tension between those two things in either direction
by somebody hammering on the side of the argument that they believe to
be correct and ignoring the other one.

> Something like contrib/sepgsql would be a better mechanism, perhaps.

There's nothing wrong with that exactly, but what does it gain us over
my proposal of a sentinel file? I don't see much value in adding a
hook and then a module that uses that hook to return false or
unconditionally ereport.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 30, 2024 at 2:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Indeed.  I'd go so far as to say that we should reject not only this
>> proposal, but any future ones that intend to prevent superusers from
>> doing things that superusers normally could do (and, indeed, are
>> normally expected to do).

> Also in my opinion, there is a fair amount of nuance here. On the one
> hand, I and others put a lot of work into making it possible to not
> give people superuser and still be able to do a controlled subset of
> the things that a superuser can do.

Sure, and that is a line of thought that we should continue to pursue.
But we already have enough mechanism to let a non-superuser set only
the ALTER SYSTEM stuff she's authorized to.  There is no reason to
think that a non-superuser could break through that restriction at
all, let alone easily.  So that's an actual security feature, not
security theater.  I don't see how the feature proposed here isn't
security theater, or at least close enough to that.

>> Something like contrib/sepgsql would be a better mechanism, perhaps.

> There's nothing wrong with that exactly, but what does it gain us over
> my proposal of a sentinel file?

I was imagining using selinux and/or sepgsql to directly prevent
writing postgresql.auto.conf from the Postgres account.  Combine that
with a non-Postgres-owned postgresql.conf (already supported) and you
have something that seems actually bulletproof, rather than a hint.
Admittedly, using that approach requires knowing something about a
non-Postgres security mechanism.

            regards, tom lane

On Tue, Jan 30, 2024 at 10:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
> > There's nothing wrong with that exactly, but what does it gain us over
> > my proposal of a sentinel file?
>
> I was imagining using selinux and/or sepgsql to directly prevent
> writing postgresql.auto.conf from the Postgres account.  Combine that
> with a non-Postgres-owned postgresql.conf (already supported) and you
> have something that seems actually bulletproof, rather than a hint.
> Admittedly, using that approach requires knowing something about a
> non-Postgres security mechanism.

Wouldn't a simple "chattr +i postgresql.auto.conf" work?

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

Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Jan 30, 2024 at 10:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I was imagining using selinux and/or sepgsql to directly prevent
>> writing postgresql.auto.conf from the Postgres account.

> Wouldn't a simple "chattr +i postgresql.auto.conf" work?

Hmm, I'm not too familiar with that file attribute, but it looks
like it'd work (on platforms that support it).

My larger point here is that trying to enforce restrictions on
superusers *within* Postgres is simply not a good plan, for
largely the same reasons that Robert questioned making the
GUC mechanism police itself.  It needs to be done outside,
either at the filesystem level or via some other kernel-level
security system.

            regards, tom lane

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.

"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

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

On 31.01.24 06:28, Tom Lane wrote:
>> The idea of adding a file to the data directory appeals to me.
>>
>> optional_runtime_features.conf
>> alter_system=enabled
>> copy_from_program=enabled
>> copy_to_program=disabled
> ... so, exactly what keeps an uncooperative superuser from
> overwriting that file?

The point of this feature would be to keep the honest people honest.

The first thing I did when ALTER SYSTEM came out however many years ago 
was to install Nagios checks to warn when postgresql.auto.conf exists. 
Because the thing is an attractive nuisance, especially when you want to 
do centralized configuration control.  Of course you can bypass it using 
COPY PROGRAM etc., but then you *know* that you are *bypassing* 
something.  If you just see ALTER SYSTEM, you'll think, "that is 
obviously the appropriate tool", and there is no generally accepted way 
to communicate that, in particular environment, it might not be.

On Wed, Jan 31, 2024 at 12:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> You cannot enforce such restrictions within Postgres.
> It has to be done by an outside mechanism.  If you think
> different, you are mistaken.

It seems like the biggest reason why we can't enforce such
restrictions with Postgres is that you won't hear of anyone committing
any code which would let us enforce such restrictions in Postgres. I'm
not saying that there's no other problem here, but you're just digging
in your heels. I wrote upthread "We can't resolve the tension between
those two things in either direction by somebody hammering on the side
of the argument that they believe to be correct and ignoring the other
one" and you replied to that by quoting what I said about the side of
the argument that you believe and hammering on it some more. I really
wish you wouldn't do stuff like that.

One thing that I think might be helpful here is to address the
question of exactly how the superuser can get general-purpose
filesystem access. They can definitely do it if there are any
untrusted PLs installed, but the person who configures the machine can
control that. They can also do it if extensions like adminpack are
available, but the server administrator can control that, too. They
can do it through COPY TO/FROM PROGRAM, but we could provide a way to
restrict that, and I think an awful lot of people want that. I don't
know of any other "normal" way of getting filesystem access, but the
superuser can also hack the system catalogs. That means they can
create a function definition that tries to run an arbitrary function
either in PostgreSQL itself or any .so they can get their hands on --
but this is a much less powerful technique since
5ded4bd21403e143dd3eb66b92d52732fdac1945 removed the version 0 calling
convention. You can no longer manufacture calls to random C functions
that aren't expecting to be called from SQL. The superuser can also
arrange to call a function that *is* intended to be SQL-callable with
the wrong argument types. It's not hard to manufacture a crash that
way, because if you call a function that's expecting a varlena with an
integer, you can induce the function to read more memory than intended
and run right off the stack. I'm not quite sure whether this can be
parlayed into arbitrary code execution; I think it's possible.

And, then, of course, you can use ALTER SYSTEM to set archive_command
or restore_command or similar to a shell command of your choosing.

What else is there? We should actually document the whole list of ways
that a superuser can escape the sandbox. Because right now there are
tons of people, even experienced PG users, who think that superusers
can't escape from PG at all, or that it's just about COPY TO/FROM
PROGRAM. The lack of clarity about what the issues are makes
intelligent discussion difficult. Our documentation hints at the fact
that there's no privilege boundary between the superuser and the OS
user, but it's not very clear or very detailed or in any very central
place, and it's not surprising that not everyone understands the
situation correctly.

At any rate, unless there are way more ways to get filesystem access
than what I've listed here, it's not unreasonable for people to want
to shut off the most obvious ones, like COPY TO/FROM PROGRAM and ALTER
SYSTEM. And there's no real reason we can't provide a way to do that.
It's just sticking your head in the stand to say "well, because we
can't prevent people from crafting a stack overrun attack to access
the filesystem, we shouldn't have a feature that tells them ALTER
SYSTEM is disabled on this instance."

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

On Wed, Jan 31, 2024 at 5:16 AM Gabriele Bartolini
<gabriele.bartolini@enterprisedb.com> wrote:
> I very much like the idea of a file in the data directory that also controls the copy operations.
>
> Just wanted to highlight though that in our operator we have already applied the read-only postgresql.auto.conf trick
todisable the system (see https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system).
However,having that file read-only triggered an issue when using pg_rewind to resync a former primary, as pg_rewind
immediatelybails out when a read-only file is encountered in the PGDATA (see
https://github.com/cloudnative-pg/cloudnative-pg/issues/3698).
>
> We might keep this in mind if we go down the path of the separate file.

Yeah. It would be possible to teach pg_rewind and other utilities to
handle unreadable or unwritable files in the data directory, but I'm
not sure that's the best path forward here, and it would require some
consensus that it's the way we want to go.

Another option I thought of would be to control these sorts of things
with a command-line switch. I doubt whether that does anything really
fundamental from a security point of view, but it removes the control
of the toggles from anything in the data directory while still leaving
it within the server administrator's remit.

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

On Tue, Jan 30, 2024 at 04:25:12PM -0500, Robert Haas wrote:
> I don't think we should pretend like one of the two paragraphs above
> is valid and the other is hot garbage. That's not solving anything. We
> can't resolve the tension between those two things in either direction
> by somebody hammering on the side of the argument that they believe to
> be correct and ignoring the other one.

What if we generate log messages when certain commands are used, like
ALTER TABLE?  We could have GUC which controls which commands are
logged.

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

  Only you can decide what is important to you.

On Thu, Feb 1, 2024 at 7:33 AM Bruce Momjian <bruce@momjian.us> wrote:
> On Tue, Jan 30, 2024 at 04:25:12PM -0500, Robert Haas wrote:
> > I don't think we should pretend like one of the two paragraphs above
> > is valid and the other is hot garbage. That's not solving anything. We
> > can't resolve the tension between those two things in either direction
> > by somebody hammering on the side of the argument that they believe to
> > be correct and ignoring the other one.
>
> What if we generate log messages when certain commands are used, like
> ALTER TABLE?  We could have GUC which controls which commands are
> logged.

Well, as I understand it, that doesn't solve the problem here. The
problem some people want to solve here seems to be:

On my system, the PostgreSQL configuration parameters are being
managed by $EXTERNAL_TOOL. Therefore, they should not be managed by
PostgreSQL itself. Therefore, if someone uses ALTER SYSTEM, they've
made a mistake, so we should give them an ERROR telling them that,
like:

ERROR: you're supposed to update the configuration via k8s, not ALTER
SYSTEM, you dummy!
DETAIL: Stop being an idiot.

The exact message text might need some wordsmithing. :-)

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

On 31.01.24 11:16, Gabriele Bartolini wrote:
> I very much like the idea of a file in the data directory that also 
> controls the copy operations.
> 
> Just wanted to highlight though that in our operator we have already 
> applied the read-only postgresql.auto.conf trick to disable the system 
> (see 
> https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system
<https://cloudnative-pg.io/documentation/current/postgresql_conf/#enabling-alter-system>).However, having that file
read-onlytriggered an issue when using pg_rewind to resync a former primary, as pg_rewind immediately bails out when a
read-onlyfile is encountered in the PGDATA (see https://github.com/cloudnative-pg/cloudnative-pg/issues/3698
<https://github.com/cloudnative-pg/cloudnative-pg/issues/3698>).
> 
> We might keep this in mind if we go down the path of the separate file.

How about ALTER SYSTEM is disabled if the file 
postgresql.auto.conf.disabled exists? This is somewhat similar to making 
the file read-only, but doesn't risk other tools breaking when they 
encounter such a file.  And it's more obvious and self-explaining.

On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote:
> I also think that using the GUC system to manage itself is a little
> bit suspect. I wonder if it would be better to do this some other way,
> e.g. a sentinel file in the data directory. For example, suppose we
> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or
> something like that.

On Tue, 6 Feb 2024 at 15:10, Peter Eisentraut <peter@eisentraut.org> wrote:
> How about ALTER SYSTEM is disabled if the file
> postgresql.auto.conf.disabled exists? This is somewhat similar to making
> the file read-only, but doesn't risk other tools breaking when they
> encounter such a file.  And it's more obvious and self-explaining.

I'm not convinced we need a new file to disable ALTER SYSTEM. I feel
like an "enable_alter_system" GUC that defaults to ON would work fine
for this. If we make that GUC be PGC_POSTMASTER then an operator can
disable ALTER SYSTEM either with a command line argument or by
changing the main config file. Since this feature is mostly useful
when the config file is managed by an external system, it seems rather
simple for that system to configure one extra GUC in the config file.

On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote:
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  could not open file "postgresql.auto.conf": Permission denied
> ```

+1 to simply mark postgresql.auto.conf file as not being writeable.

To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a
user-friendlyhint?
 

```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  could not open file "postgresql.auto.conf": Permission denied
HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only.
```

On Fri, Sep 8, 2023, at 23:43, Magnus Hagander wrote:
> We need a "allowlist" of things a user can do, rather than a blocklist
> of "they can do everything they can possibly think of and a computer
> is capable of doing, except for this one specific thing". Blocklisting
> individual permissions of a superuser will never be secure.

+1 for preferring an "allowlist" approach over a blocklist.

In a way, I think this is similar to the project's philosophy on Query Hints, which I strongly support as I think it
leadsto a better PostgreSQL over the long term. It creates a crucial feedback loop between users facing query planner
issuesand our developer community, providing essential insights for enhancing the Query Planner.
 

If users were to simply apply Query Hints as a quick fix instead of reporting underlying problems, we would often lose
thesevaluable opportunities for improvement of the Query Planner.
 

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

/Joel

On 06.02.24 16:22, Jelte Fennema-Nio wrote:
> On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote:
>> I also think that using the GUC system to manage itself is a little
>> bit suspect. I wonder if it would be better to do this some other way,
>> e.g. a sentinel file in the data directory. For example, suppose we
>> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or
>> something like that.
> 
> On Tue, 6 Feb 2024 at 15:10, Peter Eisentraut <peter@eisentraut.org> wrote:
>> How about ALTER SYSTEM is disabled if the file
>> postgresql.auto.conf.disabled exists? This is somewhat similar to making
>> the file read-only, but doesn't risk other tools breaking when they
>> encounter such a file.  And it's more obvious and self-explaining.
> 
> I'm not convinced we need a new file to disable ALTER SYSTEM. I feel
> like an "enable_alter_system" GUC that defaults to ON would work fine
> for this. If we make that GUC be PGC_POSTMASTER then an operator can
> disable ALTER SYSTEM either with a command line argument or by
> changing the main config file. Since this feature is mostly useful
> when the config file is managed by an external system, it seems rather
> simple for that system to configure one extra GUC in the config file.

Yeah, I'm all for that, but some others didn't like handling this in the 
GUC system, so I'm throwing around other ideas.

On Wed, 7 Feb 2024 at 11:16, Peter Eisentraut <peter@eisentraut.org> wrote:
> On 06.02.24 16:22, Jelte Fennema-Nio wrote:
> > On Tue, 30 Jan 2024 at 18:49, Robert Haas <robertmhaas@gmail.com> wrote:
> >> I also think that using the GUC system to manage itself is a little
> >> bit suspect. I wonder if it would be better to do this some other way,
> >> e.g. a sentinel file in the data directory. For example, suppose we
> >> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or
> >> something like that.
> > I'm not convinced we need a new file to disable ALTER SYSTEM. I feel
> > like an "enable_alter_system" GUC that defaults to ON would work fine
> > for this. If we make that GUC be PGC_POSTMASTER then an operator can
> > disable ALTER SYSTEM either with a command line argument or by
> > changing the main config file. Since this feature is mostly useful
> > when the config file is managed by an external system, it seems rather
> > simple for that system to configure one extra GUC in the config file.
>
> Yeah, I'm all for that, but some others didn't like handling this in the
> GUC system, so I'm throwing around other ideas.

Okay, then we're agreeing here. Reading back the email thread the only
argument against GUCs that I could find was Robert thinking it is a "a
little bit suspect" to let the GUC system manage itself. This would
not be the first time we're doing that though, the same is true for
"config_file" and "data_directory" (which even needed the introduction
of GUC_DISALLOW_IN_AUTO_FILE).

So, I personally would like to hear some other options before we start
entertaining some new ways of configuring Postgres its behaviour (even
the read-only postgresql.auto.conf seems quite strange to me).

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

On Wed, 7 Feb 2024 at 11:35, Gabriele Bartolini
<gabriele.bartolini@enterprisedb.com> wrote:
> This is mostly the approach I have taken in the patch, except allowing to change the value in the configuration
file.

(I had missed the patch in the long thread). I think it would be nice
to have this be PGC_SIGHUP, and set GUC_DISALLOW_IN_AUTO_FILE. That
way this behaviour can be changed without shutting down postgres (but
not with ALTER SYSTEM, because that seems confusing).

> but wasn't sure in which `config_group` to place the 'enable_alter_system` GUC, based on the
src/include/utils/guc_tables.h.Any thoughts/hints?
 

I agree that none of the existing groups fit particularly well. I see
a few options:

1. Create a new group (maybe something like "Administration" or
"Enabled Features")
2. Use FILE_LOCATIONS, which seems sort of related at least.
3. Instead of adding an "enable_alter_system" GUC we would add an
"auto_config_file" guc (and use the FILE_LOCATIONS group). Then if a
user sets "auto_config_file" to an empty string, we would disable the
auto config file and thus ALTER SYSTEM.

I'd prefer 1 or 3 I think. I kinda like option 3 for its consistency
of being able to configure other config file locations, but I think
that would be quite a bit more work, and I'm not sure how useful it is
to change the location of the auto file.

On Wed, 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).

Hi Joel,

On Wed, 7 Feb 2024 at 10:00, Joel Jacobson <joel@compiler.org> wrote:

On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote:
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  could not open file "postgresql.auto.conf": Permission denied
> ```

+1 to simply mark postgresql.auto.conf file as not being writeable.

To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendly hint?

```
postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  could not open file "postgresql.auto.conf": Permission denied
HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only.
```

This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA.

This seems like the simplest solution. And maybe we should be fixing pg_rewind regardless of this issue?

cheers

andrew

--

Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, Feb 10, 2024 at 9:47 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>> To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a
user-friendlyhint? 
>>
>> ```
>> postgres=# ALTER SYSTEM SET wal_level TO minimal;
>> ERROR:  could not open file "postgresql.auto.conf": Permission denied
>> HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only.
>> ```
>
> This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA.
>
> This seems like the simplest solution. And maybe we should be fixing pg_rewind regardless of this issue?

Is it just pg_rewind? What about pg_basebackup, for example? Will it
preserve permissions on that file in both directory and tar-format
mode? Will any of the other tools that access the data directory via
the filesystem care about this? What about third-party backup tools,
or other third-party tools that access the data directory? I think in
general we've taken the approach so far of basically making everything
in the data directory have the same permissions as each other, and
overall either everything is only user-accessible, or it's also
group-readable, and there's a fair amount of code in various places
that assumes these things are true.

What I like about using a sentinel file for this -- I like Peter's
suggestion of postgresql.auto.conf.disabled -- is that it keeps that
property that our tools and third-party tools mostly don't need to
care about file permissions, because they're all uniform. I think it
may be simpler in the long run if we stick with that idea. I suspect
that if we deviate from it we'll slowly find bugs here and there and
have to add special-case logic in various unanticipated places to
compensate.

We can also make a GUC work, if people prefer that approach. If we go
that route, the suggestion of making it PGC_SIGHUP and
GUC_DISALLOW_IN_AUTO_FILE is a good one. When I earlier referred to
managing the GUC system with GUCs as "suspect," what I really meant
was that (1) there shouldn't be an easy way to make an end run around
the thing that's disabling ALTER SYSTEM and (2) you shouldn't be able
to use ALTER SYSTEM to disable ALTER SYSTEM. It sounds like those
flags might be strong enough to prevent that. If it turns out they're
not we can always add more flags.

It's not entirely clear to me what our wider vision is here. Some
people seem to want a whole series of flags that can disable various
things that the superuser might otherwise be able to do, which is fine
with me, except that we have no plan to disable all of the things a
superuser can do to get filesystem/OS access, and I don't think it's
possible to construct such a plan. To do so, we'd have to lock down
the superuser account to the point where it can't create functions
written in any untrusted procedural language -- in particular, C
functions -- which would preclude installing most extensions; and we'd
also have to forbid direct access to the catalogs. I think those kinds
of restrictions are basically untenable. A service provider might not
want a customer to have the ability to do those kinds of things, but
some user must retain those capabilities, at the very least to handle
emergencies. So, the solution there seems to be for the service
provider to be the superuser and the customer to not be the
super-user, rather than for the service provider and the customer to
both be super-user but with some attempt at sandboxing. I'm not trying
to kill this particular proposal, which I think is broadly reasonable,
but I'm still uncomfortable with the fact that it looks a lot like
pseudo-security.

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

On Sun, Feb 11, 2024, at 14:58, Robert Haas wrote:
> It's not entirely clear to me what our wider vision is here. Some
> people seem to want a whole series of flags that can disable various
> things that the superuser might otherwise be able to do,

Yes, that's what bothers me a little with the idea of a special fix for this special case.

On Thu, Sep 7, 2023, at 22:27, Tom Lane wrote:
> If you nonetheless feel that that's a good idea for your use case,
> you can implement the restriction with an event trigger or the like.

On Fri, Sep 15, 2023, at 11:18, Daniel Gustafsson wrote:
>> On 11 Sep 2023, at 15:50, Magnus Hagander <magnus@hagander.net> wrote:
>>
>> On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>>
>>> On 2023-Sep-08, Magnus Hagander wrote:
>>>
>>>> Now, it might be that you don't care at all about the *security* side
>>>> of the feature, and only care about the convenience side. But in that
>>>> case, the original suggestion from Tom of using an even trigger seems
>>>> like a fine enough solution?
>>>
>>> ALTER SYSTEM, like all system-wide commands, does not trigger event
>>> triggers.  These are per-database only.
>>>
>>> https://www.postgresql.org/docs/16/event-trigger-matrix.html
>>
>> Hah, didn't think of that. And yes, that's a very good point. But one
>> way to fix that would be to actually make event triggers for system
>> wide commands, which would then be useful for other things as well...
>
> Wouldn't having system wide EVTs be a generic solution which could be the
> infrastructure for this requested change as well as others in the same area?

+1

I like the wider vision of providing the necessary infrastructure to provide a solution for the general case.

/Joel

On Tue, Feb 13, 2024 at 2:05 AM Joel Jacobson <joel@compiler.org> wrote:
> > Wouldn't having system wide EVTs be a generic solution which could be the
> > infrastructure for this requested change as well as others in the same area?
>
> +1
>
> I like the wider vision of providing the necessary infrastructure to provide a solution for the general case.

We don't seem to be making much progress here.

As far as I can see from reading the thread, most people agree that
it's reasonable to have some way to disable ALTER SYSTEM, but there
are at least six competing ideas about how to do that:

1. command-line option
2. GUC
3. event trigger
4. extension
5. sentinel file
6. remove permissions on postgresql.auto.conf

As I see it, (5) or (6) are most convenient for the system
administrator, since they let that person make changes without needing
to log into the database or, really, worry very much about the
database's usual configuration mechanisms at all, and (5) seems like
less work to implement than (6), because (6) probably breaks a bunch
of client tools in weird ways that might not be easy for us to
discover during patch review. (1) doesn't allow changing things at
runtime, and might require the system administrator to fiddle with the
startup scripts, which seems like it could be inconvenient. (2) and
(3) seem like they put the superuser in a position to easily reverse a
policy about what the superuser ought to do, but in the case of (2),
that can be mitigated if the GUC can only be set in postgresql.conf
and not elsewhere. (4) has no real advantages except for allowing core
to maintain the fiction that we don't support this while actually
supporting it; I think we should reject that approach outright.

So what I'd like to see is a patch that implements (5), or in the
alternative (2) but with the GUC being PGC_SIGHUP and
GUC_DISALLOW_IN_AUTO_FILE. I believe there would be adequate consensus
to proceed with either of those approaches. Anybody feel like coding
it up?

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

On Thu, 14 Mar 2024 at 17:37, Robert Haas <robertmhaas@gmail.com> wrote:
> or in the
> alternative (2) but with the GUC being PGC_SIGHUP and
> GUC_DISALLOW_IN_AUTO_FILE. I believe there would be adequate consensus
> to proceed with either of those approaches. Anybody feel like coding
> it up?

Here is a slightly modified version of Gabrielle his original patch,
which already implemented GUC approach. The changes I made are adding
PGC_SIGHUP and GUC_DISALLOW_IN_AUTO_FILE as well as adding some more
docs.

Robert Haas <robertmhaas@gmail.com> writes:
> As far as I can see from reading the thread, most people agree that
> it's reasonable to have some way to disable ALTER SYSTEM, but there
> are at least six competing ideas about how to do that:

> 1. command-line option
> 2. GUC
> 3. event trigger
> 4. extension
> 5. sentinel file
> 6. remove permissions on postgresql.auto.conf

With the possible exception of #1, every one of these is easily
defeatable by an uncooperative superuser.  I'm not excited about
adding a "security" feature with such obvious holes in it.
We reverted MAINTAIN last year for much less obvious holes;
how is it that we're going to look the other way on this one?

#2 with the GUC_DISALLOW_IN_AUTO_FILE flag can be made secure
(I think) by putting the main postgresql.conf file outside the
data directory and then making it not owned by or writable by the
postgres user.  But I doubt that's a common configuration, and
I'm sure we will get complaints from people who failed to set it
up that way.  The proposed patch certainly doesn't bother to
document the hazard.

Really we'd need to do something about removing superusers'
access to the filesystem in order to build something with
fewer holes.  I'm not against inventing such a feature,
but it'd take a fair amount of work and likely would end
in a noticeably less usable system (no plpython for example).

            regards, tom lane

On Thu, Mar 14, 2024 at 3:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> With the possible exception of #1, every one of these is easily
> defeatable by an uncooperative superuser.  I'm not excited about
> adding a "security" feature with such obvious holes in it.
> We reverted MAINTAIN last year for much less obvious holes;
> how is it that we're going to look the other way on this one?

We're going to document that it's not a security feature along the
lines of what Magnus suggested in
http://postgr.es/m/CABUevEx9m=CV8=WpXVW+rtVVs858kDJ6YpRkExV7n+F6MK05CQ@mail.gmail.com

And then maybe someday we'll do this:

> Really we'd need to do something about removing superusers'
> access to the filesystem in order to build something with
> fewer holes.  I'm not against inventing such a feature,
> but it'd take a fair amount of work and likely would end
> in a noticeably less usable system (no plpython for example).

Yep. It would be useful if you replied to the portion of
http://postgr.es/m/CA+TgmoasUgkZ27x0XZH4EdmQ_b6JbRT6cSUxf+pHdgj-ESk_zA@mail.gmail.com
where I enumerate the methods that I know about for the superuser to
get filesystem access. I don't think it's going to be practical to
block all of those methods in a single commit, and I'm not entirely
convinced that we can ever close all the holes without compromising
the superuser's ability to do necessary system administration tasks,
but maybe it's possible, and documenting the list of such methods
would make it a lot easier for users to understand the risks and
hackers to pick problems to try to tackle.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 14, 2024 at 3:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> With the possible exception of #1, every one of these is easily
>> defeatable by an uncooperative superuser.  I'm not excited about
>> adding a "security" feature with such obvious holes in it.

> We're going to document that it's not a security feature along the
> lines of what Magnus suggested in
> http://postgr.es/m/CABUevEx9m=CV8=WpXVW+rtVVs858kDJ6YpRkExV7n+F6MK05CQ@mail.gmail.com

The patch-of-record contains no such wording.  And if this isn't a
security feature, then what is it?  If you have to say to your
(super) users "please don't mess with the system configuration",
you might as well just trust them not to do it the easy way as not
to do it the hard way.  If they're untrustworthy, why have they
got superuser?

What I think this is is a loaded foot-gun painted in kid-friendly
colors.  People will use it and then file CVEs about how it did
not turn out to be as secure as they imagined (probably without
reading the documentation).

            regards, tom lane

On Thu, Mar 14, 2024 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The patch-of-record contains no such wording.

I plan to fix that, if nobody else beats me to it.

> And if this isn't a
> security feature, then what is it?  If you have to say to your
> (super) users "please don't mess with the system configuration",
> you might as well just trust them not to do it the easy way as not
> to do it the hard way.  If they're untrustworthy, why have they
> got superuser?

I mean, I feel like this question has been asked and answered before,
multiple times, on this thread. If you sincerely don't understand the
use case, I can try again to explain it. But somehow I feel like it's
more that you just don't like the idea, which is fair, but it seems
like a considerable number of people feel otherwise.

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

On Thu, Mar 14, 2024 at 1:38 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 14, 2024 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The patch-of-record contains no such wording.
>
> I plan to fix that, if nobody else beats me to it.
>
> > And if this isn't a
> > security feature, then what is it?  If you have to say to your
> > (super) users "please don't mess with the system configuration",
> > you might as well just trust them not to do it the easy way as not
> > to do it the hard way.  If they're untrustworthy, why have they
> > got superuser?
>
> I mean, I feel like this question has been asked and answered before,
> multiple times, on this thread. If you sincerely don't understand the
> use case, I can try again to explain it. But somehow I feel like it's
> more that you just don't like the idea, which is fair, but it seems
> like a considerable number of people feel otherwise.

I know I'm jumping into a long thread here, but I've been following it
out of interest. I'm sympathetic to the use case, since I used to work
at a Postgres cloud provider, and while our system intentionally did
not give our end users superuser privileges, I can imagine other
managed environments where that's not an issue. I'd like to give
answering this question again a shot, because I think this has been a
persistent misunderstanding in this thread, and I don't think it's
been made all that clear.

It's not a security feature: it's a usability feature.

It's a usability feature because, when Postgres configuration is
managed by an outside mechanism (e.g., as in a Kubernetes
environment), ALTER SYSTEM currently allows a superuser to make
changes that appear to work, but may be discarded at some point in the
future when that outside mechanism updates the config. They may also
be represented incorrectly in a management dashboard if that dashboard
is based on the values in the outside configuration mechanism, rather
than values directly from Postgres.

In this case, the end user with access to Postgres superuser
privileges presumably also has access to the outside configuration
mechanism. The goal is not to prevent them from changing settings, but
to offer guard rails that prevent them from changing settings in a way
that will be unstable (revertible by a future update) or confusing
(not showing up in a management UI).

There are challenges here in making sure this is _not_ seen as a
security feature. But I do think the feature itself is sensible and
worthwhile.

Thanks,
Maciek

On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> It's not a security feature: it's a usability feature.
>
> It's a usability feature because, when Postgres configuration is
> managed by an outside mechanism (e.g., as in a Kubernetes
> environment), ALTER SYSTEM currently allows a superuser to make
> changes that appear to work, but may be discarded at some point in the
> future when that outside mechanism updates the config. They may also
> be represented incorrectly in a management dashboard if that dashboard
> is based on the values in the outside configuration mechanism, rather
> than values directly from Postgres.
>
> In this case, the end user with access to Postgres superuser
> privileges presumably also has access to the outside configuration
> mechanism. The goal is not to prevent them from changing settings, but
> to offer guard rails that prevent them from changing settings in a way
> that will be unstable (revertible by a future update) or confusing
> (not showing up in a management UI).
>
> There are challenges here in making sure this is _not_ seen as a
> security feature. But I do think the feature itself is sensible and
> worthwhile.

This is what I would have said if I'd tried to offer an explanation,
except you said it better than I would have done.

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

On Thu, Mar 14, 2024 at 07:43:15PM -0400, Robert Haas wrote:
> On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> > It's not a security feature: it's a usability feature.
> >
> > It's a usability feature because, when Postgres configuration is
> > managed by an outside mechanism (e.g., as in a Kubernetes
> > environment), ALTER SYSTEM currently allows a superuser to make
> > changes that appear to work, but may be discarded at some point in the
> > future when that outside mechanism updates the config. They may also
> > be represented incorrectly in a management dashboard if that dashboard
> > is based on the values in the outside configuration mechanism, rather
> > than values directly from Postgres.
> >
> > In this case, the end user with access to Postgres superuser
> > privileges presumably also has access to the outside configuration
> > mechanism. The goal is not to prevent them from changing settings, but
> > to offer guard rails that prevent them from changing settings in a way
> > that will be unstable (revertible by a future update) or confusing
> > (not showing up in a management UI).
> >
> > There are challenges here in making sure this is _not_ seen as a
> > security feature. But I do think the feature itself is sensible and
> > worthwhile.
> 
> This is what I would have said if I'd tried to offer an explanation,
> except you said it better than I would have done.

I do think the docs need to clearly say this is not a security feature.
In fact, I wonder if the ALTER SYSTEM error message should explain the
GUC that is causing the failure.

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

  Only you can decide what is important to you.

On Thu, 14 Mar 2024 at 22:15, Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> In this case, the end user with access to Postgres superuser
> privileges presumably also has access to the outside configuration
> mechanism. The goal is not to prevent them from changing settings, but
> to offer guard rails that prevent them from changing settings in a way
> that will be unstable (revertible by a future update) or confusing
> (not showing up in a management UI).

Great explanation! Attached is a much changed patch that updates to
docs and code to reflect this. I particularly liked your use of the
word "guard rail" as that reflects the intent of the feature very well
IMO. So I included that wording in both the GUC group and the error
code.

> On 15 Mar 2024, at 03:58, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Mar 14, 2024 at 07:43:15PM -0400, Robert Haas wrote:
>> On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
>>> It's not a security feature: it's a usability feature.
>>>
>>> It's a usability feature because, when Postgres configuration is
>>> managed by an outside mechanism (e.g., as in a Kubernetes
>>> environment), ALTER SYSTEM currently allows a superuser to make
>>> changes that appear to work, but may be discarded at some point in the
>>> future when that outside mechanism updates the config. They may also
>>> be represented incorrectly in a management dashboard if that dashboard
>>> is based on the values in the outside configuration mechanism, rather
>>> than values directly from Postgres.
>>>
>>> In this case, the end user with access to Postgres superuser
>>> privileges presumably also has access to the outside configuration
>>> mechanism. The goal is not to prevent them from changing settings, but
>>> to offer guard rails that prevent them from changing settings in a way
>>> that will be unstable (revertible by a future update) or confusing
>>> (not showing up in a management UI).
>>>
>>> There are challenges here in making sure this is _not_ seen as a
>>> security feature. But I do think the feature itself is sensible and
>>> worthwhile.
>>
>> This is what I would have said if I'd tried to offer an explanation,
>> except you said it better than I would have done.
>
> I do think the docs need to clearly say this is not a security feature.

A usability feature whose purpose is to guard against a superuser willingly
acting against how the system is managed, or not even knowing how it is
managed, does have a certain security feature smell.  We've already had a few
CVE's filed against usability features so I don't think Tom's fears are at all
ungrounded.

Another quirk for the documentation of this: if I disable ALTER SYSTEM I would
assume that postgresql.auto.conf is no longer consumed, but it still is (and
still need to be), so maybe "enable/disable" is the wrong choice of words?

--
Daniel Gustafsson

On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson <daniel@yesql.se> wrote:
> Another quirk for the documentation of this: if I disable ALTER SYSTEM I would
> assume that postgresql.auto.conf is no longer consumed, but it still is (and
> still need to be), so maybe "enable/disable" is the wrong choice of words?

Updated the docs to reflect this quirk. But I kept the same name for
the GUC for now, because I couldn't come up with a better name myself.
If someone suggests a better name, I'm happy to change it though.

On Fri, Mar 15, 2024 at 7:09 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson <daniel@yesql.se> wrote:
> > Another quirk for the documentation of this: if I disable ALTER SYSTEM I would
> > assume that postgresql.auto.conf is no longer consumed, but it still is (and
> > still need to be), so maybe "enable/disable" is the wrong choice of words?
>
> Updated the docs to reflect this quirk. But I kept the same name for
> the GUC for now, because I couldn't come up with a better name myself.
> If someone suggests a better name, I'm happy to change it though.

Hmm. So in this patch, we have a whole new kind of GUC - guard rails -
of which enable_alter_system is the first member. Is that what we
want? I would have been somewhat inclined to find an existing section
of postgresql.auto.conf for this parameter, perhaps "platform and
version compatibility". But if we're going to add a bunch of similar
GUCs, maybe grouping them all together is the way to go.

Even if that is what we're going to do, do we want to call them "guard
rails"? I'm not sure I'd find that name terribly clear, as a user. We
know what we mean right now because we're having a very active
discussion about this topic, but it might not seem as clear to someone
coming at it fresh.

On balance, I'm disinclined to add a new category for this. If we get
to a point where we have several of these and we want to break them
out into a new category, we can do it then. Maybe by that time the
naming will seem more clear, too.

I also don't think it's good enough to just say that this isn't a
security feature. Talk is cheap. I think we need to say why it's not a
security feature. So my proposal is something like this, taking a
bunch of text from Jelte's patch and some inspiration from Magnus's
earlier remarks:

==
When <literal>enable_alter_system</literal> is set to
<literal>off</literal>, an error is returned if the <command>ALTER
SYSTEM</command> command is used. This parameter can only be set in
the <filename>postgresql.conf</filename> file or on the server command
line. The default value is <literal>on</literal>.

Note that this setting cannot be regarded as a security feature. It
only disables the <literal>ALTER SYSTEM</literal> command. It does not
prevent a superuser from changing the configuration remotely using
other means. A superuser has many ways of executing shell commands at
the operating system level, and can therefore modify
<literal>postgresql.auto.conf</literal> regardless of the value of
this setting. The purpose of the setting is to prevent
<emphasis>accidental</emphasis> modifications via <literal>ALTER
SYSTEM</literal> in environments where <literal>PostgreSQL</literal>'s
configuration is managed by some outside mechanism. In such
environments, using <command>ALTER SYSTEM</command> to make
configuration changes might appear to work, but then may be discarded
at some point in the future when that outside mechanism updates the
configuration. Setting this parameter to <literal>false</literal> can
help to avoid such mistakes.
==

I agree with Daniel's comment that Tom's concerns about people filing
CVEs are not without merit; indeed, I said the same thing in my first
post to this thread. However, I also believe that's not a sufficient
reason for rejecting a feature that many people seem to want. I think
the root of this problem is that our documentation is totally unclear
about the fact that we don't intend for there to be privilege
separation between the operating system user and the PostgreSQL
superuser; people want there to be a distinction, and think there is.
Hence CVE-2019-9193, for example. Several people, including me, wrote
blog posts about how that's not a security vulnerability, but while I
was researching mine, I went looking for where in the documentation we
actually SAY that there's no privilege separation between the OS user
and the superuser. The only mention I found at the time was the
PL/perlu documentation, which said this:

"The writer of a PL/PerlU function must take care that the function
cannot be used to do anything unwanted, since it will be able to do
anything that could be done by a user logged in as the database
administrator."

That statement, from the official documentation, in my mind at least,
DOES confirm that we don't intend privilege separation, but it's
really oblique. You have to think through the fact that the superuser
has to be the one to install plperlu, and that plperlu functions can
usurp the OS user; since both of those things are documented to be the
case, it follows that we know and expect that the superuser can usurp
the OS user. But someone who is wondering how PostgreSQL's security
model works is not going to read the plperlu documentation and make
the inferences I just described. It's crazy to me that a principle
frequently cited as gospel on this mailing list and others is nearly
undocumented. Obviously, even if we did document it clearly, people
could still get confused (or just disagree with our position) and file
CVEs anyway, but we're not helping our case by having nothing to cite.

A difficulty is where to PUT such a mention in the documentation.
There's not a single section title in the top-level documentation
index that includes the word "security". Perhaps figuring out how to
document this is best left to a separate thread, and there's also the
question of whether a new section that talks about this also ought to
talk about anything else. But I feel like we're way overdue to do
something about this.

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

> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:

> my proposal is something like this, taking a
> bunch of text from Jelte's patch and some inspiration from Magnus's
> earlier remarks:

I still think any wording should clearly mention that settings in the file are
still applied.  The proposed wording says to implicitly but to avoid confusion
I think it should be explicit.

> Perhaps figuring out how to
> document this is best left to a separate thread, and there's also the
> question of whether a new section that talks about this also ought to
> talk about anything else. But I feel like we're way overdue to do
> something about this.

Seconded, both that it needs to be addressed and that it should be done on a
separate thread from this one.

--
Daniel Gustafsson

On Mon, 18 Mar 2024 at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
> I would have been somewhat inclined to find an existing section
> of postgresql.auto.conf for this parameter, perhaps "platform and
> version compatibility".

I tried to find an existing section, but I couldn't find any that this
new GUC would fit into naturally. "Version and Platform Compatibility
/ Previous PostgreSQL Versions" (the one you suggested) seems wrong
too. The GUCs there are to get back to Postgres behaviour from
previous versions. So that section would only make sense if we'd turn
enable_alter_system off by default (which obviously no-one in this
thread suggests/wants).

If you have another suggestion for an existing category that we should
use, feel free to share. But imho, none of the existing ones are a
good fit.

> Even if that is what we're going to do, do we want to call them "guard
> rails"? I'm not sure I'd find that name terribly clear, as a user.

If anyone has a better suggestion, I'm happy to change it.


On Mon, 18 Mar 2024 at 14:09, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
>
> > my proposal is something like this, taking a
> > bunch of text from Jelte's patch and some inspiration from Magnus's
> > earlier remarks:
>
> I still think any wording should clearly mention that settings in the file are
> still applied.  The proposed wording says to implicitly but to avoid confusion
> I think it should be explicit.

I updated the first two paragraphs with Robert his wording (and did
not remove the third one as that addresses the point made by Daniel)

On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
>
> > my proposal is something like this, taking a
> > bunch of text from Jelte's patch and some inspiration from Magnus's
> > earlier remarks:
>
> I still think any wording should clearly mention that settings in the file are
> still applied.  The proposed wording says to implicitly but to avoid confusion
> I think it should be explicit.

I haven't kept up with the thread, but in general I'd prefer it to
actually turn off parsing the file as well. I think just turning off
the ability to change it -- including the ability to *revert* changes
that were made to it before -- is going to be confusing.

But, if we have decided it shouldn't do that, then IMHO we should
consider naming it maybe enable_alter_system_command instead -- since
we're only disabling the alter system command, not the actual feature
in total.


> > Perhaps figuring out how to
> > document this is best left to a separate thread, and there's also the
> > question of whether a new section that talks about this also ought to
> > talk about anything else. But I feel like we're way overdue to do
> > something about this.
>
> Seconded, both that it needs to be addressed and that it should be done on a
> separate thread from this one.

+1.

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

> On 18 Mar 2024, at 16:34, Magnus Hagander <magnus@hagander.net> wrote:
>
> On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>>> my proposal is something like this, taking a
>>> bunch of text from Jelte's patch and some inspiration from Magnus's
>>> earlier remarks:
>>
>> I still think any wording should clearly mention that settings in the file are
>> still applied.  The proposed wording says to implicitly but to avoid confusion
>> I think it should be explicit.
>
> I haven't kept up with the thread, but in general I'd prefer it to
> actually turn off parsing the file as well. I think just turning off
> the ability to change it -- including the ability to *revert* changes
> that were made to it before -- is going to be confusing.

Wouldn't that break pgBackrest which IIRC write to .auto.conf directly
without using ALTER SYSTEM?

> But, if we have decided it shouldn't do that, then IMHO we should
> consider naming it maybe enable_alter_system_command instead -- since
> we're only disabling the alter system command, not the actual feature
> in total.

Good point.

--
Daniel Gustafsson

On Mon, Mar 18, 2024 at 4:44 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 18 Mar 2024, at 16:34, Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >>
> >>> On 18 Mar 2024, at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
> >>
> >>> my proposal is something like this, taking a
> >>> bunch of text from Jelte's patch and some inspiration from Magnus's
> >>> earlier remarks:
> >>
> >> I still think any wording should clearly mention that settings in the file are
> >> still applied.  The proposed wording says to implicitly but to avoid confusion
> >> I think it should be explicit.
> >
> > I haven't kept up with the thread, but in general I'd prefer it to
> > actually turn off parsing the file as well. I think just turning off
> > the ability to change it -- including the ability to *revert* changes
> > that were made to it before -- is going to be confusing.
>
> Wouldn't that break pgBackrest which IIRC write to .auto.conf directly
> without using ALTER SYSTEM?

Ugh of course. And not only that, it would also break pg_basebackup
which does the same.

So I guess that's not a good idea. I guess nobody anticipated this
when that was done:)


> > But, if we have decided it shouldn't do that, then IMHO we should
> > consider naming it maybe enable_alter_system_command instead -- since
> > we're only disabling the alter system command, not the actual feature
> > in total.
>
> Good point.


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

On Mon, Mar 18, 2024 at 7:12 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Mon, 18 Mar 2024 at 13:57, Robert Haas <robertmhaas@gmail.com> wrote:
> > I would have been somewhat inclined to find an existing section
> > of postgresql.auto.conf for this parameter, perhaps "platform and
> > version compatibility".
>
> I tried to find an existing section, but I couldn't find any that this
> new GUC would fit into naturally. "Version and Platform Compatibility
> / Previous PostgreSQL Versions" (the one you suggested) seems wrong
> too. The GUCs there are to get back to Postgres behaviour from
> previous versions. So that section would only make sense if we'd turn
> enable_alter_system off by default (which obviously no-one in this
> thread suggests/wants).
>
> If you have another suggestion for an existing category that we should
> use, feel free to share. But imho, none of the existing ones are a
> good fit.

+1 on Version and Platform Compatibility. Maybe it just needs a new
subsection there? This is for compatibility with a "deployment
platform". The "Platform and Client Compatibility" subsection has just
one entry, so a new subsection with also just one entry seems
defensible, maybe just "Deployment Compatibility"? I think it's also
plausible that there will be other similar settings for managed
deployments in the future.

> > Even if that is what we're going to do, do we want to call them "guard
> > rails"? I'm not sure I'd find that name terribly clear, as a user.
>
> If anyone has a better suggestion, I'm happy to change it.

No better suggestion at the moment, but while I used the term to
explain the feature, I also don't think that's a great official name.
For one thing, the section could easily be misinterpreted as guard
rails for end-users who are new to Postgres. Also, I think it's more
colloquial in tone than Postgres docs conventions.

Further, I think we may want to change the GUC name itself. All the
other GUCs that start with enable_ control planner behavior:

maciek=# select name from pg_settings where name like 'enable_%';
              name
--------------------------------
 enable_async_append
 enable_bitmapscan
 enable_gathermerge
 enable_hashagg
 enable_hashjoin
 enable_incremental_sort
 enable_indexonlyscan
 enable_indexscan
 enable_material
 enable_memoize
 enable_mergejoin
 enable_nestloop
 enable_parallel_append
 enable_parallel_hash
 enable_partition_pruning
 enable_partitionwise_aggregate
 enable_partitionwise_join
 enable_presorted_aggregate
 enable_seqscan
 enable_sort
 enable_tidscan
(21 rows)

Do we really want to break that pattern?

On Mon, Mar 18, 2024 at 11:46 AM Magnus Hagander <magnus@hagander.net> wrote:
> > Wouldn't that break pgBackrest which IIRC write to .auto.conf directly
> > without using ALTER SYSTEM?
>
> Ugh of course. And not only that, it would also break pg_basebackup
> which does the same.
>
> So I guess that's not a good idea. I guess nobody anticipated this
> when that was done:)

I'm also +1 for the idea that the feature should only disable ALTER
SYSTEM, not postgresql.auto.conf. I can't really see any reason why it
needs to do both, and it might be more convenient if it didn't. If
you're managing PostgreSQL's configuration externally, you might find
it convenient to write the configuration you're managing into
postgresql.auto.conf. Or you might want to write it to
postgresql.conf. Or you might want to do something more complicated
with include directives or whatever. But there's no reason why you
*couldn't* want to use postgresql.auto.conf, and on the other hand I
don't see how anyone benefits from that file not being read. That just
seems confusing.

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

On Mon, Mar 18, 2024 at 12:19 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> +1 on Version and Platform Compatibility. Maybe it just needs a new
> subsection there? This is for compatibility with a "deployment
> platform". The "Platform and Client Compatibility" subsection has just
> one entry, so a new subsection with also just one entry seems
> defensible, maybe just "Deployment Compatibility"? I think it's also
> plausible that there will be other similar settings for managed
> deployments in the future.

Right, we're adding this because of environments like Kubernetes,
which isn't a version, but it is a platform, or at least a deployment
mode, which is why I thought of that section. I think for now we
should just file this under "Other platforms and clients," which only
has one existing setting. If the number of settings of this type
grows, we can split it out.

> Do we really want to break that pattern?

Using enable_* as code for "this is a planner GUC" is a pretty stupid
pattern, honestly, but I agree with you that it's long-established and
we probably shouldn't deviate from it lightly. Perhaps just rename to
allow_alter_system?

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

On Mon, Mar 18, 2024 at 10:27 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Right, we're adding this because of environments like Kubernetes,
> which isn't a version, but it is a platform, or at least a deployment
> mode, which is why I thought of that section. I think for now we
> should just file this under "Other platforms and clients," which only
> has one existing setting. If the number of settings of this type
> grows, we can split it out.

Fair enough, +1.

> Using enable_* as code for "this is a planner GUC" is a pretty stupid
> pattern, honestly, but I agree with you that it's long-established and
> we probably shouldn't deviate from it lightly. Perhaps just rename to
> allow_alter_system?

+1

On Thu, Mar 14, 2024 at 12:37 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Feb 13, 2024 at 2:05 AM Joel Jacobson <joel@compiler.org> wrote:
> > > Wouldn't having system wide EVTs be a generic solution which could be the
> > > infrastructure for this requested change as well as others in the same area?
> >
> > +1
> >
> > I like the wider vision of providing the necessary infrastructure to provide a solution for the general case.
>
> We don't seem to be making much progress here.
>
> As far as I can see from reading the thread, most people agree that
> it's reasonable to have some way to disable ALTER SYSTEM, but there
> are at least six competing ideas about how to do that:
>
> 1. command-line option
> 2. GUC
> 3. event trigger
> 4. extension
> 5. sentinel file
> 6. remove permissions on postgresql.auto.conf
>
> As I see it, (5) or (6) are most convenient for the system
> administrator, since they let that person make changes without needing
> to log into the database or, really, worry very much about the
> database's usual configuration mechanisms at all, and (5) seems like
> less work to implement than (6), because (6) probably breaks a bunch
> of client tools in weird ways that might not be easy for us to
> discover during patch review. (1) doesn't allow changing things at
> runtime, and might require the system administrator to fiddle with the
> startup scripts, which seems like it could be inconvenient. (2) and
> (3) seem like they put the superuser in a position to easily reverse a
> policy about what the superuser ought to do, but in the case of (2),
> that can be mitigated if the GUC can only be set in postgresql.conf
> and not elsewhere. (4) has no real advantages except for allowing core
> to maintain the fiction that we don't support this while actually
> supporting it; I think we should reject that approach outright.
>

You know it's funny, you say #4 has no advantage and should be
rejected outright, but AFAICT

a) no one has actually laid out why it wouldn't work for them,
b) and it's the one solution that can be implemented now
c) and that implementation would be backwards compatible with some set
of existing releases
d) and certainly anyone running k8s or config management system would
have the ability to install
e) and it could be custom tailored to individual deployments as needed
(including other potential commands that some systems might care
about)
f) and it seems like the least likely option to be mistaken for a
security feature
g) and also seems pretty safe wrt not breaking existing tooling (like
5/6 might do)

Looking at it, you could make the argument that #4 is actually the
best of the solutions proposed, except it has the one drawback that it
requires folks to double down on the fiction that we think extensions
are a good way to build solutions when really everyone just wants to
have everything in core.

Robert Treat
https://xzilla.net

On Mon, Mar 18, 2024 at 4:07 PM Robert Treat <rob@xzilla.net> wrote:
> You know it's funny, you say #4 has no advantage and should be
> rejected outright, but AFAICT
>
> a) no one has actually laid out why it wouldn't work for them,
> b) and it's the one solution that can be implemented now
> c) and that implementation would be backwards compatible with some set
> of existing releases
> d) and certainly anyone running k8s or config management system would
> have the ability to install
> e) and it could be custom tailored to individual deployments as needed
> (including other potential commands that some systems might care
> about)
> f) and it seems like the least likely option to be mistaken for a
> security feature
> g) and also seems pretty safe wrt not breaking existing tooling (like
> 5/6 might do)
>
> Looking at it, you could make the argument that #4 is actually the
> best of the solutions proposed, except it has the one drawback that it
> requires folks to double down on the fiction that we think extensions
> are a good way to build solutions when really everyone just wants to
> have everything in core.

I think that all of this is true except for (c). I think we'd need a
new hook to make it work.

That said, I think that extensions are a good way of implementing some
functionality, but not this functionality. Extensions are a good
approach when there's a bunch of stuff core can't know but an
extension author can. For instance, the FDW interface caters to
situations where the extension author knows how to access some data
that PostgreSQL doesn't know how to access; and the operator class
stuff is useful when the extension author knows how some user-defined
data type should behave and we don't. But there's not really a
substantial policy question here. All we do by pushing a feature like
this out of core is wash our hands of it. Your (f) argues that might
be a good thing, but I don't think so. When we know that a feature is
widely-needed, it's better to have one good implementation of it in
core than several perhaps not-so-good implementations out of core.
That allows us to focus all of our efforts on that one implementation
instead of splitting them across several -- which is the whole selling
point of open source, really -- and it makes it easier for users who
want the feature to get access to it.

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

I want to remind everyone of this from Gabriele's first message that 
started this thread:

> At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked
> by making the postgresql.auto.conf read only, but the returned message is
> misleading and that’s certainly bad user experience (which is very
> important in a cloud native environment):
> 
> 
> ```
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  could not open file "postgresql.auto.conf": Permission denied
> ```

I think making the config file read-only is a fine solution. If you 
don't want postgres to mess with the config files, forbid it with the 
permission system.

Problems with pg_rewind, pg_basebackup were mentioned with that 
approach. I think if you want the config files to be managed outside 
PostgreSQL, by kubernetes, patroni or whatever, it would be good for 
them to be read-only to the postgres user anyway, even if we had a 
mechanism to disable ALTER SYSTEM. So it would be good to fix the 
problems with those tools anyway.

The error message is not great, I agree with that. Can we improve it? 
Maybe just add a HINT like this:

postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  could not open file "postgresql.auto.conf" for writing: 
Permission denied
HINT:  Configuration might be managed outside PostgreSQL


Perhaps we could make that even better with a GUC though. I propose a 
GUC called 'configuration_managed_externally = true / false". If you set 
it to true, we prevent ALTER SYSTEM and make the error message more 
definitive:

postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  configuration is managed externally

As a bonus, if that GUC is set, we could even check at server startup 
that all the configuration files are not writable by the postgres user, 
and print a warning or refuse to start up if they are.

(Another way to read this proposal is to rename the GUC that's been 
discussed in this thread to 'configuration_managed_externally'. That 
makes it look less like a security feature, and describes the intended 
use case.)

-- 
Heikki Linnakangas
Neon (https://neon.tech)

On Mon, 18 Mar 2024 at 18:27, Robert Haas <robertmhaas@gmail.com> wrote:
> I think for now we
> should just file this under "Other platforms and clients," which only
> has one existing setting. If the number of settings of this type
> grows, we can split it out.

Done. I also included a patch to rename COMPAT_OPTIONS_CLIENTS to
COMPAT_OPTIONS_OTHER, since that enum variant naming doesn't match the
new intent of the section.

On Tue, 19 Mar 2024 at 10:26, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> (Another way to read this proposal is to rename the GUC that's been
> discussed in this thread to 'configuration_managed_externally'. That
> makes it look less like a security feature, and describes the intended
> use case.)

I like this idea of naming the GUC in such a way. I swapped the words
a bit and went for externally_managed_configuration, since order
matches other GUCs e.g. standard_conforming_strings. But if you feel
strongly about the ordering of the words, I'm happy to change it back.

For the errorcode I now went for ERRCODE_FEATURE_NOT_SUPPORTED, which
seemed most fitting.

On Tue, 19 Mar 2024 at 10:26, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> I want to remind everyone of this from Gabriele's first message that
> started this thread:
>
> > At the moment, a possible workaround is that `ALTER SYSTEM` can be blocked
> > by making the postgresql.auto.conf read only, but the returned message is
> > misleading and that’s certainly bad user experience (which is very
> > important in a cloud native environment):
> >
> >
> > ```
> > postgres=# ALTER SYSTEM SET wal_level TO minimal;
> > ERROR:  could not open file "postgresql.auto.conf": Permission denied
> > ```
>
> I think making the config file read-only is a fine solution. If you
> don't want postgres to mess with the config files, forbid it with the
> permission system.
>
> Problems with pg_rewind, pg_basebackup were mentioned with that
> approach. I think if you want the config files to be managed outside
> PostgreSQL, by kubernetes, patroni or whatever, it would be good for
> them to be read-only to the postgres user anyway, even if we had a
> mechanism to disable ALTER SYSTEM. So it would be good to fix the
> problems with those tools anyway.
>
> The error message is not great, I agree with that. Can we improve it?
> Maybe just add a HINT like this:
>
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  could not open file "postgresql.auto.conf" for writing:
> Permission denied
> HINT:  Configuration might be managed outside PostgreSQL
>
>
> Perhaps we could make that even better with a GUC though. I propose a
> GUC called 'configuration_managed_externally = true / false". If you set
> it to true, we prevent ALTER SYSTEM and make the error message more
> definitive:
>
> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  configuration is managed externally
>
> As a bonus, if that GUC is set, we could even check at server startup
> that all the configuration files are not writable by the postgres user,
> and print a warning or refuse to start up if they are.
>
> (Another way to read this proposal is to rename the GUC that's been
> discussed in this thread to 'configuration_managed_externally'. That
> makes it look less like a security feature, and describes the intended
> use case.)
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>

On 3/19/24 07:49, Andrew Dunstan wrote:
> 
> 
> On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas <hlinnaka@iki.fi 
> <mailto:hlinnaka@iki.fi>> wrote:
> 
>     I want to remind everyone of this from Gabriele's first message that
>     started this thread:
> 
>      > At the moment, a possible workaround is that `ALTER SYSTEM` can
>     be blocked
>      > by making the postgresql.auto.conf read only, but the returned
>     message is
>      > misleading and that’s certainly bad user experience (which is very
>      > important in a cloud native environment):
>      >
>      >
>      > ```
>      > postgres=# ALTER SYSTEM SET wal_level TO minimal;
>      > ERROR:  could not open file "postgresql.auto.conf": Permission denied
>      > ```
> 
>     I think making the config file read-only is a fine solution. If you
>     don't want postgres to mess with the config files, forbid it with the
>     permission system.
> 
>     Problems with pg_rewind, pg_basebackup were mentioned with that
>     approach. I think if you want the config files to be managed outside
>     PostgreSQL, by kubernetes, patroni or whatever, it would be good for
>     them to be read-only to the postgres user anyway, even if we had a
>     mechanism to disable ALTER SYSTEM. So it would be good to fix the
>     problems with those tools anyway.
> 
>     The error message is not great, I agree with that. Can we improve it?
>     Maybe just add a HINT like this:
> 
>     postgres=# ALTER SYSTEM SET wal_level TO minimal;
>     ERROR:  could not open file "postgresql.auto.conf" for writing:
>     Permission denied
>     HINT:  Configuration might be managed outside PostgreSQL
> 
> 
>     Perhaps we could make that even better with a GUC though. I propose a
>     GUC called 'configuration_managed_externally = true / false". If you
>     set
>     it to true, we prevent ALTER SYSTEM and make the error message more
>     definitive:
> 
>     postgres=# ALTER SYSTEM SET wal_level TO minimal;
>     ERROR:  configuration is managed externally
> 
>     As a bonus, if that GUC is set, we could even check at server startup
>     that all the configuration files are not writable by the postgres user,
>     and print a warning or refuse to start up if they are.
> 
>     (Another way to read this proposal is to rename the GUC that's been
>     discussed in this thread to 'configuration_managed_externally'. That
>     makes it look less like a security feature, and describes the intended
>     use case.)
> 
> 
> 
> 
> I agree with pretty much all of this.


+1 me too.

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

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Perhaps we could make that even better with a GUC though. I propose a 
> GUC called 'configuration_managed_externally = true / false". If you set 
> it to true, we prevent ALTER SYSTEM and make the error message more 
> definitive:

> postgres=# ALTER SYSTEM SET wal_level TO minimal;
> ERROR:  configuration is managed externally

> As a bonus, if that GUC is set, we could even check at server startup 
> that all the configuration files are not writable by the postgres user, 
> and print a warning or refuse to start up if they are.

I like this idea.  The "bonus" is not optional though, because
setting the files' ownership/permissions is the only way to be
sure that the prohibition is even a little bit bulletproof.

One small issue: how do we make that work on Windows?  Have recent
versions grown anything that looks like real file permissions?

Another question is whether this should be one-size-fits-all for
all the configuration files.  I can imagine situations where
you'd like to lock down postgresql[.auto].conf but not pg_hba.conf.
But maybe that can wait for somebody to show up with a use-case.

            regards, tom lane

On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I like this idea.  The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.

I don't agree with this. The only "normal" way of modifying
postgresql.auto.conf from within postgres is using ALTER SYSTEM, so
simply disabling ALTER SYSTEM seems enough to me.

> Another question is whether this should be one-size-fits-all for
> all the configuration files.  I can imagine situations where
> you'd like to lock down postgresql[.auto].conf but not pg_hba.conf.
> But maybe that can wait for somebody to show up with a use-case.

Afaik there's no way to modify pg_hba.conf from within postgres, only
read it. (except for COPY TO FILE/PROGRAM etc) So, I don't think we
need to worry about this now.

On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > Perhaps we could make that even better with a GUC though. I propose a
> > GUC called 'configuration_managed_externally = true / false". If you set
> > it to true, we prevent ALTER SYSTEM and make the error message more
> > definitive:
>
> > postgres=# ALTER SYSTEM SET wal_level TO minimal;
> > ERROR:  configuration is managed externally
>
> > As a bonus, if that GUC is set, we could even check at server startup
> > that all the configuration files are not writable by the postgres user,
> > and print a warning or refuse to start up if they are.
>
> I like this idea.  The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.
>
> One small issue: how do we make that work on Windows?  Have recent
> versions grown anything that looks like real file permissions?
>
> Another question is whether this should be one-size-fits-all for
> all the configuration files.  I can imagine situations where
> you'd like to lock down postgresql[.auto].conf but not pg_hba.conf.
> But maybe that can wait for somebody to show up with a use-case.
>
>                         regards, tom lane

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> On Tue, 19 Mar 2024 at 15:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I like this idea.  The "bonus" is not optional though, because
>> setting the files' ownership/permissions is the only way to be
>> sure that the prohibition is even a little bit bulletproof.

> I don't agree with this. The only "normal" way of modifying
> postgresql.auto.conf from within postgres is using ALTER SYSTEM, so
> simply disabling ALTER SYSTEM seems enough to me.

I've said this repeatedly: it's not enough.  The only reason we need
any feature whatsoever is that somebody doesn't trust their database
superusers to not try to modify the configuration.  Given that
requirement, merely disabling ALTER SYSTEM isn't a solution, it's a
fig leaf that might fool incompetent auditors but no more.

If you aren't willing to build a solution that blocks off mods
using COPY TO FILE/PROGRAM and other readily-available-to-superusers
tools (plpythonu for instance), I think you shouldn't bother asking
for a feature at all.  Just trust your superusers.

            regards, tom lane

On Tue, 19 Mar 2024 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've said this repeatedly: it's not enough.  The only reason we need
> any feature whatsoever is that somebody doesn't trust their database
> superusers to not try to modify the configuration.

And as everyone else on this thread has said: It is enough. Because
the point is not security, the point is hinting to a superuser that a
workflow they know from other systems (or an ALTER SYSTEM command they
copied from the internet) is not the intended way to modify their
server configuration on the system they are currently working on.

I feel like the docs and error message in the current active patch are
very clear on that. If you think they are not clear, feel free to
suggest what could clarify the intent of this feature. But at this
point, it's really starting to seem to me like you're willingly trying
to interpret this feature as a thing that it is not (i.e. a security
feature).

> On 19 Mar 2024, at 17:53, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> 
> On Tue, 19 Mar 2024 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've said this repeatedly: it's not enough.  The only reason we need
>> any feature whatsoever is that somebody doesn't trust their database
>> superusers to not try to modify the configuration.
> 
> And as everyone else on this thread has said: It is enough. Because
> the point is not security, the point is hinting to a superuser that a
> workflow they know from other systems (or an ALTER SYSTEM command they
> copied from the internet) is not the intended way to modify their
> server configuration on the system they are currently working on.

Well.  Protection against superusers randomly copying ALTER SYSTEM commands
from the internet actually does turn this into a security feature =)

--
Daniel Gustafsson

> On 19 Mar 2024, at 15:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Perhaps we could make that even better with a GUC though. I propose a 
>> GUC called 'configuration_managed_externally = true / false". If you set 
>> it to true, we prevent ALTER SYSTEM and make the error message more 
>> definitive:
> 
>> postgres=# ALTER SYSTEM SET wal_level TO minimal;
>> ERROR:  configuration is managed externally
> 
>> As a bonus, if that GUC is set, we could even check at server startup 
>> that all the configuration files are not writable by the postgres user, 
>> and print a warning or refuse to start up if they are.
> 
> I like this idea.  The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.

Agreed, assuming we can solve the below..

> One small issue: how do we make that work on Windows?  Have recent
> versions grown anything that looks like real file permissions?

--
Daniel Gustafsson

On Tue, Mar 19, 2024 at 3:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > Perhaps we could make that even better with a GUC though. I propose a
> > GUC called 'configuration_managed_externally = true / false". If you set
> > it to true, we prevent ALTER SYSTEM and make the error message more
> > definitive:
>
> > postgres=# ALTER SYSTEM SET wal_level TO minimal;
> > ERROR:  configuration is managed externally
>
> > As a bonus, if that GUC is set, we could even check at server startup
> > that all the configuration files are not writable by the postgres user,
> > and print a warning or refuse to start up if they are.
>
> I like this idea.  The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.
>
> One small issue: how do we make that work on Windows?  Have recent
> versions grown anything that looks like real file permissions?

Windows has had full ACL support since 1993. The  easiest way to do
what you're doing here is to just set a DENY permission on the
postgres operating system user.


//Magnus

Greg Sabino Mullane:
> On Tue, Mar 19, 2024 at 12:05 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     If you aren't willing to build a solution that blocks off mods
>     using COPY TO FILE/PROGRAM and other readily-available-to-superusers
>     tools (plpythonu for instance), I think you shouldn't bother asking
>     for a feature at all.  Just trust your superusers.
> 
> 
> There is a huge gap between using a well-documented standard tool like 
> ALTER SYSTEM and going out of your way to modify the configuration files 
> through trickery. I think we need to only solve the former as in "hey, 
> please don't do that because your changes will be overwritten"

Recap: The requested feature is not supposed to be a security feature. 
It is supposed to prevent the admin from accidentally doing the wrong 
thing - but not from willfully doing the same through different means.

This very much sounds like a "warning" - how about turning the feature 
into one?

Have a GUC warn_on_alter_system = "<message>", which allows the 
kubernetes operator to set it to something like "hey, please don't do 
that because your changes will be overwritten. Use xyz operator instead.".

This will hardly be taken as a security feature by anyone, but should 
essentially achieve what is asked for.

A more sophisticated way would be to make that GUC throw an error, but 
have a syntax for ALTER SYSTEM to override this - i.e. similar to a 
--force flag.

Best,

Wolfgang

Andrew Dunstan <andrew@dunslane.net> writes:
> On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander <magnus@hagander.net> wrote:
>> Windows has had full ACL support since 1993. The  easiest way to do
>> what you're doing here is to just set a DENY permission on the
>> postgres operating system user.

> Yeah. See <
> https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls>
> for example.

Cool.  Maybe somebody should take a fresh look at the places where
we're assuming Windows has nothing comparable to Unix permissions
(for example, checking for world readability of ssl_key_file).
It's off-topic for this thread though.

            regards, tom lane

Hi,

On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > Perhaps we could make that even better with a GUC though. I propose a 
> > GUC called 'configuration_managed_externally = true / false". If you set 
> > it to true, we prevent ALTER SYSTEM and make the error message more 
> > definitive:
> 
> > postgres=# ALTER SYSTEM SET wal_level TO minimal;
> > ERROR:  configuration is managed externally
> 
> > As a bonus, if that GUC is set, we could even check at server startup 
> > that all the configuration files are not writable by the postgres user, 
> > and print a warning or refuse to start up if they are.
> 
> I like this idea.  The "bonus" is not optional though, because
> setting the files' ownership/permissions is the only way to be
> sure that the prohibition is even a little bit bulletproof.

Isn't this going to break pgbackrest restore then, which (AIUI, and was
mentioned upthread) writes recovery configs into postgresql.auto.conf? 
Or do I misunderstand the proposal? I think it would be awkward if only
root users are able to run pgbackrest restore. I have added David to the
CC list to make him aware of this, in case he was not following this
thread.

The other candidate for breakage that was mentioned was pg_basebackup
-R, but I guess that could be worked around.


Michael

On Wed, 20 Mar 2024 at 14:04, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>>
>> As a bonus, if that GUC is set, we could even check at server startup that all the configuration files are not
writableby the postgres user, 
>> and print a warning or refuse to start up if they are.
>
>
> Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical
change- especially just to handle the given ask, which is to prevent a specific command from running. Not implement a
brandnew security system. There are so many ways this could go wrong if we start having separate permissions for some
ofour files. In addition to backups and other tools that need to write to the conf files as the postgres user, what
aboutsystems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the
conffiles and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1
fromme, as they say/ :) 


Well put. I don't think the effort of making all tooling handle this
correctly is worth the benefit that it brings. afaict everyone on this
thread that actually wants to use this feature would be happy with the
functionality that the current patch provides (i.e. having
postgresql.auto.conf writable, but having ALTER SYSTEM error out).

On Wed, Mar 20, 2024 at 11:07 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > Ugh, please let's not do this. This was bouncing around in my head last night, and this is really a quite radical
change- especially just to handle the given ask, which is to prevent a specific command from running. Not implement a
brandnew security system. There are so many ways this could go wrong if we start having separate permissions for some
ofour files. In addition to backups and other tools that need to write to the conf files as the postgres user, what
aboutsystems that create a new cluster automatically e.g. Patroni? It will now need elevated privs just to create the
conffiles and assign the new ownership to them. Lots of moving pieces there and ways things could go wrong. So a big -1
fromme, as they say/ :) 
>
> Well put. I don't think the effort of making all tooling handle this
> correctly is worth the benefit that it brings. afaict everyone on this
> thread that actually wants to use this feature would be happy with the
> functionality that the current patch provides (i.e. having
> postgresql.auto.conf writable, but having ALTER SYSTEM error out).

Yeah, I agree with this completely. I don't understand why people who
hate the feature and hope it dies in a fire get to decide how it has
to work.

And also, if we verify that the configuration files are all read-only
at the OS level, that also prevents the external tool from managing
them. Well, it can: it can make them non-read-only after server start,
then modify them, then make them read-only again, and it can make sure
that if the system crashes, it again marks them read-only before
trying to start PG. But it seems quite obvious that this will be
inconvenient and difficult to get right. I find it quite easy to
understand the idea that someone wants the PostgreSQL configuration to
be managed by Kubernetes rather than via by ALTER SYSTEM, but I can't
think of any scenario when you just don't want to be able to manage
the configuration at all. Who in the world would want that?

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

On Wed, Mar 20, 2024 at 3:11 PM Magnus Hagander <magnus@hagander.net> wrote:
> I would argue that having the default permissions not allow postgres to edit it's own config files *except* for
postgresql.auto.confwould be a better default than what we have now, but that's completely independent of the patch
beingdiscussed on this thread. (And FWIW also already solved on debian-based platforms for example, which but the main
configfiles in /etc with postgres only having read permissions on them - and having the *packagers* adapt such things
fortheir platforms in general seems like a better place). 

I don't think that I agree that it's categorically better, but it
might be better for some people or in some circumstances. I very much
do agree that it's a packaging question rather than our job to sort
out.

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

On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote:
> Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the
administratorto choose what fits them should be made possible. 

+1. Which is also the justification for this patch, when it comes
right down to it. The administrator gets to decide how the contents of
postgresql.conf are to be managed on their particular installation.
They can decide that postgresql.conf should be writable by the same
user that runs PostgreSQL, or not. And they should also be able to
decide that ALTER SYSTEM is an OK way to change configuration, or that
it isn't. How we enable them to make that decision is a point for
discussion, and how exactly we phrase the documentation is a point for
discussion, but we have no business trying to impose conditions, as if
they're only allowed to make that decision if they conform to some
(IMHO ridiculous) requirements that we dictate from on high. It's
their system, not ours.

I mean, for crying out loud, users can set enable_seqscan=off in
postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set
zero_damaged_pages=on in postgresql.conf and silently remove vast
quantities of data without knowing that they're doing anything. We
don't even question that stuff ... although we probably should be
questioning the second one, because, in my experience, it's just a
foot-gun and never solves anything. Nonetheless, as of today, we have
it. So somehow we're talking ourselves into believing that letting the
user just shut off ALTER SYSTEM, without taking any other action as a
prerequisite, is more scary than those things.

It's not.

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

Hi,

On Wed, Mar 20, 2024 at 08:11:32PM +0100, Magnus Hagander wrote:
> (And FWIW also already solved on debian-based platforms for example,
> which but the main config files in /etc with postgres only having read
> permissions on them 

JFTR - Debian/Ubuntu keep postgresql.conf under /etc/postgresql, but
that directory is owned by the postgres user by default and it can
change the configuration files (if that wasn't the case, external tools
like Patroni that run under the postgres user and manage postgresql.conf
would work much less easily on them).


Michael

On 3/20/24 22:30, Michael Banck wrote:
> 
> On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> Perhaps we could make that even better with a GUC though. I propose a
>>> GUC called 'configuration_managed_externally = true / false". If you set
>>> it to true, we prevent ALTER SYSTEM and make the error message more
>>> definitive:
>>
>>> postgres=# ALTER SYSTEM SET wal_level TO minimal;
>>> ERROR:  configuration is managed externally
>>
>>> As a bonus, if that GUC is set, we could even check at server startup
>>> that all the configuration files are not writable by the postgres user,
>>> and print a warning or refuse to start up if they are.
>>
>> I like this idea.  The "bonus" is not optional though, because
>> setting the files' ownership/permissions is the only way to be
>> sure that the prohibition is even a little bit bulletproof.
> 
> Isn't this going to break pgbackrest restore then, which (AIUI, and was
> mentioned upthread) writes recovery configs into postgresql.auto.conf?
> Or do I misunderstand the proposal? I think it would be awkward if only
> root users are able to run pgbackrest restore. I have added David to the
> CC list to make him aware of this, in case he was not following this
> thread.

It doesn't sound like people are in favor of requiring read-only 
permissions for postgresql.auto.conf, but in any case it would not be a 
big issue for pgBackRest or other backup solutions as far as I can see.

pgBackRest stores all permissions and ownership so a restore by the user 
will bring everything back just as it was. Restoring as root sounds bad 
on the face of it, but for managed environments like k8s it would not be 
all that unusual.

There is also the option of restoring and then modifying permissions 
later, or in pgBackRest use the --type=preserve option to leave 
postgresql.auto.conf as it is. Permissions could also be updated before 
the backup tool is run and then set back.

Since this feature is intended for managed environments scripting these 
kinds of changes should be pretty easy and not a barrier to using any 
backup tool.

Regards,
-David

On Wed, Mar 20, 2024 at 10:30 PM Magnus Hagander <magnus@hagander.net> wrote:
> Not really. The administrator can *already* do that. It's trivial.
>
> This patch is about doing it in a way that doesn't produce as ugly a message.But if we're "delegating" it to
packagersand "os administrators", then the problem is already solved. This patch is about trying to solve it *without*
involvingthe packagers or OS administrators. 
>
> Not saying we shouldn't do it, but I'd argue the exact opposite of yours aboe, which is that it's very much not the
justificationof the patch :) 

OK, that's a fair way of looking at it, too (and also you break client tools).

>> I mean, for crying out loud, users can set enable_seqscan=off in
>> postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set
>
> This is actually a good example, because it's kind of like this patch. It doesn't *actually* disable the ability to
runsequential scans, it just disables the "usual way". Just like this patch doesn't prevent the superuser from editing
theconfig, but it does prevent them droin doing it "the usual way". 

Good point.

>> zero_damaged_pages=on in postgresql.conf and silently remove vast
>> quantities of data without knowing that they're doing anything. We
>> don't even question that stuff ... although we probably should be
>
> I like how you got this far and didn't even mention fsync=off :)

Ha!

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

On Wed, Mar 20, 2024 at 10:31 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Mar 20, 2024 at 8:52 PM Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote:
>> > Right, what I meant is that making it a packaging decision is the better place. Wherever it goes, allowing the
administratorto choose what fits them should be made possible. 
>>
>> +1. Which is also the justification for this patch, when it comes
>> right down to it. The administrator gets to decide how the contents of
>> postgresql.conf are to be managed on their particular installation.
>
>
> Not really. The administrator can *already* do that. It's trivial.
>
> This patch is about doing it in a way that doesn't produce as ugly a message.But if we're "delegating" it to
packagersand "os administrators", then the problem is already solved. This patch is about trying to solve it *without*
involvingthe packagers or OS administrators. 
>
> Not saying we shouldn't do it, but I'd argue the exact opposite of yours aboe, which is that it's very much not the
justificationof the patch :) 
>
>
>>
>> They can decide that postgresql.conf should be writable by the same
>> user that runs PostgreSQL, or not. And they should also be able to
>> decide that ALTER SYSTEM is an OK way to change configuration, or that
>> it isn't. How we enable them to make that decision is a point for
>> discussion, and how exactly we phrase the documentation is a point for
>> discussion, but we have no business trying to impose conditions, as if
>> they're only allowed to make that decision if they conform to some
>> (IMHO ridiculous) requirements that we dictate from on high. It's
>> their system, not ours.
>
>
> Agreed on all those except they can already do this. It's just that the error message is ugly. The path of least
resistancewould be to just specifically detect a permissions error on the postgresql.auto.conf file when you try to do
ALTERSYSTEM, and throw at least an error hint about "you must allow writing to this file for the feature to work". 
>
> So this patch isn't at all about enabling this functionality. It's about making it more user friendly.
>
>
>> I mean, for crying out loud, users can set enable_seqscan=off in
>> postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set
>
>
> This is actually a good example, because it's kind of like this patch. It doesn't *actually* disable the ability to
runsequential scans, it just disables the "usual way". Just like this patch doesn't prevent the superuser from editing
theconfig, but it does prevent them droin doing it "the usual way". 
>
>
>>
>> zero_damaged_pages=on in postgresql.conf and silently remove vast
>> quantities of data without knowing that they're doing anything. We
>> don't even question that stuff ... although we probably should be
>
>
> I like how you got this far and didn't even mention fsync=off :)
>

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

Robert Treat
https://xzilla.net

On Tue, Mar 19, 2024 at 9:13 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Mon, 18 Mar 2024 at 18:27, Robert Haas <robertmhaas@gmail.com> wrote:
> > I think for now we
> > should just file this under "Other platforms and clients," which only
> > has one existing setting. If the number of settings of this type
> > grows, we can split it out.
>
> Done. I also included a patch to rename COMPAT_OPTIONS_CLIENTS to
> COMPAT_OPTIONS_OTHER, since that enum variant naming doesn't match the
> new intent of the section.

I reviewed these patches. I think 0001 probably isn't strictly
necessary, but I don't think it's problematic either. And I'm quite
happy with 0002 also. In particular, I think the documentation - which
must be by far the most important of the patch - does an excellent job
explaining the limitations of this feature. My only quibbles are:

- 0002 deletes a blank line from postgresql.conf.sample, and I think
it shouldn't; and
- I think the last sentence of the documentation is odd and could be
dropped; who would expect changing a GUC to reset the contents of a
config file, anyway?

Since those are just minor points, that brings us to the question of
whether there is consensus to proceed with this. I believe that there
is a clear consensus that there should be some way to disable ALTER
SYSTEM. Sure, some people, particularly Tom, disagree, but I don't
think there is any way of counting up the votes that leads to the
conclusion that we shouldn't have this feature at all. If someone
feels otherwise, show us how you counted the votes. What is less clear
is whether there is a consensus in favor of this particular method of
disabling ALTER SYSTEM, namely, via a GUC. The two alternate
approaches that seem to enjoy some level of support are (a) an
extension or (b) changing the permissions on the files.

I haven't tried to count up how many people are specifically in favor
of each approach. I personally think that it doesn't matter very much,
because I interpret the comments in favor of one or another
implementation as saying "I want us to have this feature and of the
possible approaches I prefer $WHATEVER" rather than "the only
architecturally acceptable approach to this feature is $WHATEVER and
if we can't have that then i'd rather have nothing at all." Of course,
like everything else, that conclusion is open to debate, and certainly
to correction by the people who have voted in favor of one of the
alternate approaches, if I've misinterpreted their views.

But, as a practical matter, this is the patch we have, because this is
the patch that Gabriele and Jelte took time to write and polish.
Nobody else has taken the opportunity to produce a competing one. And,
if we nevertheless insist that it has to be done some other way, I
think the inevitable result will be that nothing gets into this
release at all, because we're less than 2 weeks from feature freeze,
and there's not time for a complete do-over of something that was
originally proposed all the way back in September. And my reading of
the thread, at least, is that more people will be happy if something
gets committed here, even if it's not exactly what they would have
preferred, than if we get nothing at all.

I'm going to wait a few days for any final comments. If it becomes
clear that there is in fact no consensus to commit this version of the
patch set (or something very similar) then I'll mark this as Returned
with Feedback. Otherwise, I plan to commit these patches (perhaps
after adjusting in accordance with my comments above).

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

Robert Haas <robertmhaas@gmail.com> writes:
> Since those are just minor points, that brings us to the question of
> whether there is consensus to proceed with this. I believe that there
> is a clear consensus that there should be some way to disable ALTER
> SYSTEM. Sure, some people, particularly Tom, disagree, but I don't
> think there is any way of counting up the votes that leads to the
> conclusion that we shouldn't have this feature at all.

FWIW, I never objected to the idea of being able to disable ALTER
SYSTEM.  I felt that it ought to be part of a larger feature that
would provide a more bulletproof guarantee that a superuser can't
alter the system configuration; but I'm clearly in the minority
on that.  I'm content with just having it disable ALTER SYSTEM
and no more, as long as the documentation is sufficiently clear
that an uncooperative superuser can easily bypass this if you don't
back it up with filesystem-level controls.

            regards, tom lane

On Mon, Mar 25, 2024 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> FWIW, I never objected to the idea of being able to disable ALTER
> SYSTEM.  I felt that it ought to be part of a larger feature that
> would provide a more bulletproof guarantee that a superuser can't
> alter the system configuration; but I'm clearly in the minority
> on that.  I'm content with just having it disable ALTER SYSTEM
> and no more, as long as the documentation is sufficiently clear
> that an uncooperative superuser can easily bypass this if you don't
> back it up with filesystem-level controls.

OK, great. The latest patch doesn't specifically talk about backing it
up with filesystem-level controls, but it does clearly say that this
feature is not going to stop a determined superuser from bypassing the
feature, which I think is the appropriate level of detail. We don't
actually know whether a user has filesystem-level controls available
on their system that are equal to the task; certainly chmod isn't good
enough, unless you can prevent the superuser from just running chmod
again, which you probably can't. An FS-level immutable flag or some
other kind of OS-level wizardry might well get the job done, but I
don't think our documentation needs to speculate about that.

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

Robert Haas <robertmhaas@gmail.com> writes:
> OK, great. The latest patch doesn't specifically talk about backing it
> up with filesystem-level controls, but it does clearly say that this
> feature is not going to stop a determined superuser from bypassing the
> feature, which I think is the appropriate level of detail. We don't
> actually know whether a user has filesystem-level controls available
> on their system that are equal to the task; certainly chmod isn't good
> enough, unless you can prevent the superuser from just running chmod
> again, which you probably can't. An FS-level immutable flag or some
> other kind of OS-level wizardry might well get the job done, but I
> don't think our documentation needs to speculate about that.

True.  For postgresql.conf, you can put it outside the data directory
and make it be owned by some other user, and the job is done.  It's
harder for postgresql.auto.conf because that always lives in the data
directory which is necessarily postgres-writable, so even if you
did those two things to it the superuser could just rename or
remove it and then write postgresql.auto.conf of his choosing.

I wonder whether this feature should include teaching the server
to ignore postgresql.auto.conf altogether, which would make it
relatively easy to get to a bulletproof configuration.

            regards, tom lane

On Mon, Mar 25, 2024 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wonder whether this feature should include teaching the server
> to ignore postgresql.auto.conf altogether, which would make it
> relatively easy to get to a bulletproof configuration.

This has been debated a few times on the thread already, but a number
of problems with that idea have been raised, and as far as I can see,
everyone who suggested went on to recant and agree that we shouldn't
do that. If you feel a strong need to relitigate that, please check
the prior discussion first.

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

On Mon, Mar 25, 2024 at 01:29:46PM -0400, Robert Haas wrote:
> What is less clear is whether there is a consensus in favor of this
> particular method of disabling ALTER SYSTEM, namely, via a GUC. The
> two alternate approaches that seem to enjoy some level of support are
> (a) an extension or (b) changing the permissions on the files.

I am wondering if the fact that you would be able to do:

        ALTER SYSTEM SET externally_managed_configuration = false

and then be unable to use ALTER SYSTEM to revert the change is
significant.  I can't think of many such cases.

Isn't "configuration" too generic a term for disabling ALTER SYSTEM?

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

  Only you can decide what is important to you.

On Mon, 25 Mar 2024 at 20:16, Bruce Momjian <bruce@momjian.us> wrote:
> I am wondering if the fact that you would be able to do:
>
>         ALTER SYSTEM SET externally_managed_configuration = false
>
> and then be unable to use ALTER SYSTEM to revert the change is
> significant.

This is not possible, due to the externally_managed_configuration GUC
having the GUC_DISALLOW_IN_AUTO_FILE flag.

> Isn't "configuration" too generic a term for disabling ALTER SYSTEM?

maybe "externally_managed_auto_config"

On Mon, Mar 25, 2024 at 09:40:55PM +0100, Jelte Fennema-Nio wrote:
> On Mon, 25 Mar 2024 at 20:16, Bruce Momjian <bruce@momjian.us> wrote:
> > I am wondering if the fact that you would be able to do:
> >
> >         ALTER SYSTEM SET externally_managed_configuration = false
> >
> > and then be unable to use ALTER SYSTEM to revert the change is
> > significant.
> 
> This is not possible, due to the externally_managed_configuration GUC
> having the GUC_DISALLOW_IN_AUTO_FILE flag.

Ah, good, thanks.

> > Isn't "configuration" too generic a term for disabling ALTER SYSTEM?
> 
> maybe "externally_managed_auto_config"

How many people associate "auto" with ALTER SYSTEM?  I assume not many. 

To me, externally_managed_configuration is promising a lot more than it
delivers because there is still a lot of ocnfiguration it doesn't
control.  I am also confused why the purpose of the feature, external
management of configuation, is part of the variable name.  We usually
name parameters for what they control.

It seems this is really controlling the ability to alter system
variables at the SQL level, maybe sql_alter_system_vars.

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

  Only you can decide what is important to you.

On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote:
> > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM?
> >
> > maybe "externally_managed_auto_config"
>
> How many people associate "auto" with ALTER SYSTEM?  I assume not many.
>
> To me, externally_managed_configuration is promising a lot more than it
> delivers because there is still a lot of ocnfiguration it doesn't
> control.  I am also confused why the purpose of the feature, external
> management of configuation, is part of the variable name.  We usually
> name parameters for what they control.

I actually agree with this. I wasn't going to quibble with it because
other people seemed to like it. But I think something like
allow_alter_system would be better, as it would describe the exact
thing that the parameter does, rather than how we think the parameter
ought to be used.

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

> On 26 Mar 2024, at 13:11, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote:

>> To me, externally_managed_configuration is promising a lot more than it
>> delivers because there is still a lot of ocnfiguration it doesn't
>> control.  I am also confused why the purpose of the feature, external
>> management of configuation, is part of the variable name.  We usually
>> name parameters for what they control.
>
> I actually agree with this. I wasn't going to quibble with it because
> other people seemed to like it. But I think something like
> allow_alter_system would be better, as it would describe the exact
> thing that the parameter does, rather than how we think the parameter
> ought to be used.

+Many.  Either allow_alter_system or enable_alter_system_command is IMO
preferrable, not least because someone might use this without using any
external configuration tool, making the name even more misleading.

--
Daniel Gustafsson

At 2024-03-26 08:11:33 -0400, robertmhaas@gmail.com wrote:
>
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote:
> > > > Isn't "configuration" too generic a term for disabling ALTER SYSTEM?
> > >
> > > maybe "externally_managed_auto_config"
> >
> > How many people associate "auto" with ALTER SYSTEM?  I assume not many.
> >
> > To me, externally_managed_configuration is promising a lot more than it
> > delivers because there is still a lot of ocnfiguration it doesn't
> > control.  I am also confused why the purpose of the feature, external
> > management of configuation, is part of the variable name.  We usually
> > name parameters for what they control.
> 
> I actually agree with this. I wasn't going to quibble with it because
> other people seemed to like it. But I think something like
> allow_alter_system would be better, as it would describe the exact
> thing that the parameter does, rather than how we think the parameter
> ought to be used.

Yes, "externally_managed_configuration" raises far more questions than
it answers. "enable_alter_system" is clearer in terms of what to expect
when you set it. "enable_alter_system_command" is rather long, but even
better in that it is specific enough to not promise anything about not
allowing superusers to change the configuration some other way.

-- Abhijit (as someone who could find a use for this feature)

On Tue, Mar 26, 2024 at 8:55 AM Abhijit Menon-Sen <ams@toroid.org> wrote:
> Yes, "externally_managed_configuration" raises far more questions than
> it answers. "enable_alter_system" is clearer in terms of what to expect
> when you set it. "enable_alter_system_command" is rather long, but even
> better in that it is specific enough to not promise anything about not
> allowing superusers to change the configuration some other way.

It was previously suggested that we shouldn't start the GUC name with
"enable," since those are all planner GUCs currently. It's sort of a
silly precedent, but we have it, so that's why I proposed "allow"
instead.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian <bruce@momjian.us> wrote:
>> To me, externally_managed_configuration is promising a lot more than it
>> delivers because there is still a lot of ocnfiguration it doesn't
>> control.  I am also confused why the purpose of the feature, external
>> management of configuation, is part of the variable name.  We usually
>> name parameters for what they control.

> I actually agree with this. I wasn't going to quibble with it because
> other people seemed to like it. But I think something like
> allow_alter_system would be better, as it would describe the exact
> thing that the parameter does, rather than how we think the parameter
> ought to be used.

+1.  The overpromise-and-underdeliver aspect of the currently proposed
name is a lot of the reason I've been unhappy and kept pushing for it
to lock things down more.  "allow_alter_system" is a lot more
straightforward about exactly what it does, and if that is all we want
it to do, then a name like that is good.

            regards, tom lane

> On Mar 27, 2024, at 3:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Bruce Momjian <bruce@momjian.us> writes:
>> I am thinking "enable_alter_system_command" is probably good because we
>> already use "enable" so why not reuse that idea, and I think "command"
>> is needed because we need to clarify we are talking about the command,
>> and not generic altering of the system.  We could use
>> "enable_sql_alter_system" if people want something shorter.
>
> Robert already mentioned why not use "enable_": up to now that prefix
> has only been applied to planner plan-type-enabling GUCs.  I'd be okay
> with "allow_alter_system_command", although I find it unnecessarily
> verbose.

Agree. I don’t think “_command” adds much clarity.

Cheers

Andrew

On Wed, 27 Mar 2024 at 02:24, Andrew Dunstan <andrew@dunslane.net> wrote:
> Agree. I don’t think “_command” adds much clarity.

Alright, changed the GUC name to "allow_alter_system" since that seems
to have the most "votes". One other option would be to call it simply
"alter_system", just like "jit" is not called "allow_jit" or
"enable_jit".

But personally I feel that the "allow_alter_system" is clearer than
plain "alter_system" for the GUC name.

On Wed, Mar 27, 2024 at 03:43:28PM +0100, Jelte Fennema-Nio wrote:
> +      </term>
> +      <listitem>
> +       <para>
> +        When <literal>allow_alter_system</literal> is set to
> +        <literal>on</literal>, an error is returned if the <command>ALTER
> +        SYSTEM</command> command is used. This parameter can only be set in
> +        the <filename>postgresql.conf</filename> file or on the server command
> +        line. The default value is <literal>on</literal>.
> +       </para>

Uh, the above is clearly wrong.  I think you mean "off" on the second line.

> +
> +       <para>
> +        Note that this setting cannot be regarded as a security feature. It
> +        only disables the <literal>ALTER SYSTEM</literal> command. It does not
> +        prevent a superuser from changing the configuration remotely using

Why "remotely"?

> +        other means. A superuser has many ways of executing shell commands at
> +        the operating system level, and can therefore modify
> +        <literal>postgresql.auto.conf</literal> regardless of the value of
> +        this setting. The purpose of the setting is to prevent
> +        <emphasis>accidental</emphasis> modifications via <literal>ALTER
> +        SYSTEM</literal> in environments where
> +        <productname>PostgreSQL</productname> its configuration is managed by

"its"?

> +        some outside mechanism. In such environments, using <command>ALTER
> +        SYSTEM</command> to make configuration changes might appear to work,
> +        but then may be discarded at some point in the future when that outside

"might"

> +        mechanism updates the configuration. Setting this parameter to
> +        <literal>on</literal> can help to avoid such mistakes.
> +       </para>

"off"

Is this really a patch we think we can push into PG 17. I am having my
doubts.

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

  Only you can decide what is important to you.

On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> Alright, changed the GUC name to "allow_alter_system" since that seems
> to have the most "votes". One other option would be to call it simply
> "alter_system", just like "jit" is not called "allow_jit" or
> "enable_jit".
>
> But personally I feel that the "allow_alter_system" is clearer than
> plain "alter_system" for the GUC name.

I agree, and have committed your 0001.

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

On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian <bruce@momjian.us> wrote:
> Uh, the above is clearly wrong.  I think you mean "off" on the second line.

Woops. When the name changed from externally_managed_configuration to
allow_alter_system, the sense of it was reversed, and I guess Jelte
missed flipping the documentation references around. I likely would
have made the same mistake, but it's easily fixed.

> > +
> > +       <para>
> > +        Note that this setting cannot be regarded as a security feature. It
> > +        only disables the <literal>ALTER SYSTEM</literal> command. It does not
> > +        prevent a superuser from changing the configuration remotely using
>
> Why "remotely"?

This wording was suggested upthread. I think the point here is that if
the superuser is logging in from the local machine, it's obvious that
they can do whatever they want. The point is to emphasize that a
superuser without a local login can, too.

> "its"?

Yeah, that seems like an extra word.

> > +        some outside mechanism. In such environments, using <command>ALTER
> > +        SYSTEM</command> to make configuration changes might appear to work,
> > +        but then may be discarded at some point in the future when that outside
>
> "might"

This does not seem like a mistake to me. I'm not sure why you think it is.

> > +        mechanism updates the configuration. Setting this parameter to
> > +        <literal>on</literal> can help to avoid such mistakes.
> > +       </para>
>
> "off"

This is another case that needs to be fixed now that the sense of the
GUC is reversed. (We'd better make sure the code has the test the
right way around, too.)

> Is this really a patch we think we can push into PG 17. I am having my
> doubts.

If the worst thing that happens in PG 17 is that we push a patch that
needs a few documentation corrections, we're going to be doing
fabulously well.

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

On Wed, 27 Mar 2024 at 16:10, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian <bruce@momjian.us> wrote:
> > Uh, the above is clearly wrong.  I think you mean "off" on the second line.
>
> Woops. When the name changed from externally_managed_configuration to
> allow_alter_system, the sense of it was reversed, and I guess Jelte
> missed flipping the documentation references around.

Yeah, that's definitely what happened. I did change a few, but I
indeed missed a few others (or maybe flipped some twice by accident,
or hadn't been flipped before when it reversed previously).

> > Why "remotely"?
>
> This wording was suggested upthread. I think the point here is that if
> the superuser is logging in from the local machine, it's obvious that
> they can do whatever they want. The point is to emphasize that a
> superuser without a local login can, too.

Changed this from "remotely using other means" to "using other SQL commands".

> > "its"?
>
> Yeah, that seems like an extra word.

Changed this to "the configuration of PostgreSQL"

> > > +        some outside mechanism. In such environments, using <command>ALTER
> > > +        SYSTEM</command> to make configuration changes might appear to work,
> > > +        but then may be discarded at some point in the future when that outside
> >
> > "might"
>
> This does not seem like a mistake to me. I'm not sure why you think it is.

I also think the original sentence was correct, but I don't think it
read very naturally. Changed it now in hopes to improve that.

> > > +        mechanism updates the configuration. Setting this parameter to
> > > +        <literal>on</literal> can help to avoid such mistakes.
> > > +       </para>
> >
> > "off"
>
> This is another case that needs to be fixed now that the sense of the
> GUC is reversed. (We'd better make sure the code has the test the
> right way around, too.)

Fixed this one too, and the code is the right way around.

On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>>> The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER
SYSTEM</literal>in environments where 
>> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER
SYSTEMin environments where..." is enough? 
> Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know if
theextra word "accidental" is necessary, but I think that's the motivation. 

I think the emphasis is entirely warranted in this case.

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

On Wed, Mar 27, 2024 at 04:50:27PM +0100, Jelte Fennema-Nio wrote:
> > This wording was suggested upthread. I think the point here is that if
> > the superuser is logging in from the local machine, it's obvious that
> > they can do whatever they want. The point is to emphasize that a
> > superuser without a local login can, too.
> 
> Changed this from "remotely using other means" to "using other SQL commands".

Yes, I like the SQL emphasis since "remote" just doesn't seem like the
right thing to highlight here.

> > > > +        some outside mechanism. In such environments, using <command>ALTER
> > > > +        SYSTEM</command> to make configuration changes might appear to work,
> > > > +        but then may be discarded at some point in the future when that outside
> > >
> > > "might"
> >
> > This does not seem like a mistake to me. I'm not sure why you think it is.
> 
> I also think the original sentence was correct, but I don't think it
> read very naturally. Changed it now in hopes to improve that.

So, might means "possibility" while "may" means permission, so "might"
is clearer here.

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

  Only you can decide what is important to you.

On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote:
> On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > Alright, changed the GUC name to "allow_alter_system" since that seems
> > to have the most "votes". One other option would be to call it simply
> > "alter_system", just like "jit" is not called "allow_jit" or
> > "enable_jit".
> >
> > But personally I feel that the "allow_alter_system" is clearer than
> > plain "alter_system" for the GUC name.
> 
> I agree, and have committed your 0001.

So, I email "Is this really a patch we think we can push into PG 17. I
am having my doubts," and the patch is applied a few hours after my
email.  Wow.

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

  Only you can decide what is important to you.

On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote:
> On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote:
> > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > > Alright, changed the GUC name to "allow_alter_system" since that seems
> > > to have the most "votes". One other option would be to call it simply
> > > "alter_system", just like "jit" is not called "allow_jit" or
> > > "enable_jit".
> > >
> > > But personally I feel that the "allow_alter_system" is clearer than
> > > plain "alter_system" for the GUC name.
> > 
> > I agree, and have committed your 0001.
> 
> So, I email "Is this really a patch we think we can push into PG 17. I
> am having my doubts," and the patch is applied a few hours after my
> email.  Wow.

Also odd is that I don't see the commit in git master, so now I am
confused.

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

  Only you can decide what is important to you.

On Wed, Mar 27, 2024 at 11:10:31AM -0400, Robert Haas wrote:
> > Is this really a patch we think we can push into PG 17. I am having my
> > doubts.
> 
> If the worst thing that happens in PG 17 is that we push a patch that
> needs a few documentation corrections, we're going to be doing
> fabulously well.

My point is that we are designing the user API in the last weeks of the
commitfest, which usually ends badly for us, and the fact the docs were
not even right in the patch just reenforces that concern.

But, as I stated in another email, you said you committed the patch,
yet I don't see it committed in git master, so I am confused.

Ah, I figured it out.  You were talking about the GUC renaming:

    commit de7e96bd0fc
    Author: Robert Haas <rhaas@postgresql.org>
    Date:   Wed Mar 27 10:45:28 2024 -0400
    
        Rename COMPAT_OPTIONS_CLIENT to COMPAT_OPTIONS_OTHER.
    
        The user-facing name is "Other Platforms and Clients", but the
        internal name seems too focused on clients specifically, especially
        given the plan to add a new setting to this session that is about
        platform or deployment model compatibility rather than client
        compatibility.
    
        Jelte Fennema-Nio
    
        Discussion: http://postgr.es/m/CAGECzQTfMbDiM6W3av+3weSnHxJvPmuTEcjxVvSt91sQBdOxuQ@mail.gmail.com

Please ignore my complaints, and my apologies.

As far as the GUC change, let's just be careful since we have a bad
history of pushing things near the end that we regret.  I am not saying
that would be this feature, but let's be careful.

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

  Only you can decide what is important to you.

On Wed, Mar 27, 2024 at 03:20:38PM -0700, David G. Johnston wrote:
> On Wed, Mar 27, 2024 at 3:18 PM David G. Johnston <david.g.johnston@gmail.com>
> wrote:
> 
>     On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian <bruce@momjian.us> wrote:
> 
>         On Wed, Mar 27, 2024 at 06:09:02PM -0400, Bruce Momjian wrote:
>         > On Wed, Mar 27, 2024 at 11:05:55AM -0400, Robert Haas wrote:
>         > > On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio <
>         postgres@jeltef.nl> wrote:
>         > > > Alright, changed the GUC name to "allow_alter_system" since that
>         seems
>         > > > to have the most "votes". One other option would be to call it
>         simply
>         > > > "alter_system", just like "jit" is not called "allow_jit" or
>         > > > "enable_jit".
>         > > >
>         > > > But personally I feel that the "allow_alter_system" is clearer
>         than
>         > > > plain "alter_system" for the GUC name.
>         > >
>         > > I agree, and have committed your 0001.
>         >
>         > So, I email "Is this really a patch we think we can push into PG 17.
>         I
>         > am having my doubts," and the patch is applied a few hours after my
>         > email.  Wow.
> 
>         Also odd is that I don't see the commit in git master, so now I am
>         confused.
> 
> 
>     The main feature being discussed is in the 0002 patch while Robert pushed a
>     doc section rename in the 0001 patch.
> 
> 
> 
> Well, the internal category name was changed though the docs did remain
> unchanged.

Yes, I figured that out, thank you.

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

  Only you can decide what is important to you.

On Wed, 27 Mar 2024 at 23:23, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Wed, Mar 27, 2024 at 11:10:31AM -0400, Robert Haas wrote:
> > > Is this really a patch we think we can push into PG 17. I am having my
> > > doubts.
> >
> > If the worst thing that happens in PG 17 is that we push a patch that
> > needs a few documentation corrections, we're going to be doing
> > fabulously well.
>
> My point is that we are designing the user API in the last weeks of the
> commitfest, which usually ends badly for us, and the fact the docs were
> not even right in the patch just reenforces that concern.

This user API is exactly the same as the original patch from Gabriele
in September (apart from enable->allow). And we spent half a year
discussing other designs for the user API. So I disagree that we're
designing the user API in the last weeks of the commitfest.

On Wed, 27 Mar 2024 at 20:10, Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
>
> On Wed, Mar 27, 2024, 11:46 Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland <isaac.morland@gmail.com> wrote:
>> > On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane <htamfids@gmail.com> wrote:
>> >>> The purpose of the setting is to prevent <emphasis>accidental</emphasis> modifications via <literal>ALTER
SYSTEM</literal>in environments where 
>> >> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just "to prevent modifications via ALTER
SYSTEMin environments where..." is enough? 
>> > Not necessarily disagreeing, but it's very important nobody ever mistake this for a security feature. I don't know
ifthe extra word "accidental" is necessary, but I think that's the motivation. 
>>
>> I think the emphasis is entirely warranted in this case.
>
> +1. And while "non-malicious" may technically be more correct, I don't think it's any clearer.

Attached is a new version of the patch with some sentences reworded. I
changed accidentally to mistakenly (which still has emphasis). And I
hope with the rewording it's now clearer to the reader why that
emphasis is there.

On Wed, 27 Mar 2024 at 23:06, Bruce Momjian <bruce@momjian.us> wrote:
> > > > > +        some outside mechanism. In such environments, using <command>ALTER
> > > > > +        SYSTEM</command> to make configuration changes might appear to work,
> > > > > +        but then may be discarded at some point in the future when that outside
> > > >
> > > > "might"
> > >
> > > This does not seem like a mistake to me. I'm not sure why you think it is.
> >
> > I also think the original sentence was correct, but I don't think it
> > read very naturally. Changed it now in hopes to improve that.
>
> So, might means "possibility" while "may" means permission, so "might"
> is clearer here.

Aaah, I misunderstood your original feedback then. I thought you
didn't like the use of "might" in "might appear to work". But I now
realize you meant "may be discarded" should be changed to "might be
discarded". I addressed that in my latest version of the patch.

On Thu, Mar 28, 2024 at 12:47:46AM +0100, Jelte Fennema-Nio wrote:
> On Wed, 27 Mar 2024 at 23:06, Bruce Momjian <bruce@momjian.us> wrote:
> > > > > > +        some outside mechanism. In such environments, using <command>ALTER
> > > > > > +        SYSTEM</command> to make configuration changes might appear to work,
> > > > > > +        but then may be discarded at some point in the future when that outside
> > > > >
> > > > > "might"
> > > >
> > > > This does not seem like a mistake to me. I'm not sure why you think it is.
> > >
> > > I also think the original sentence was correct, but I don't think it
> > > read very naturally. Changed it now in hopes to improve that.
> >
> > So, might means "possibility" while "may" means permission, so "might"
> > is clearer here.
> 
> Aaah, I misunderstood your original feedback then. I thought you
> didn't like the use of "might" in "might appear to work". But I now
> realize you meant "may be discarded" should be changed to "might be
> discarded". I addressed that in my latest version of the patch.

Thanks.  I did the may/might/can changes in the docs years ago so I
remember the distinction.

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

  Only you can decide what is important to you.

On Thu, Mar 28, 2024 at 12:43:29AM +0100, Jelte Fennema-Nio wrote:
> +     <varlistentry id="guc-allow-alter-system" xreflabel="allow_alter_system">
> +      <term><varname>allow_alter_system</varname> (<type>boolean</type>)
> +      <indexterm>
> +       <primary><varname>allow_alter_system</varname> configuration parameter</primary>
> +      </indexterm>
> +      </term>
> +      <listitem>
> +       <para>
> +        When <literal>allow_alter_system</literal> is set to
> +        <literal>off</literal>, an error is returned if the <command>ALTER
> +        SYSTEM</command> command is used. This parameter can only be set in

"command is used." -> "command is issued." ?

> +        the <filename>postgresql.conf</filename> file or on the server command
> +        line. The default value is <literal>on</literal>.
> +       </para>
> +
> +       <para>
> +        Note that this setting cannot be regarded as a security feature. It

"setting cannot be regarded" -> "setting should not be regarded"

> +        only disables the <literal>ALTER SYSTEM</literal> command. It does not
> +        prevent a superuser from changing the configuration using other SQL
> +        commands. A superuser has many ways of executing shell commands at
> +        the operating system level, and can therefore modify
> +        <literal>postgresql.auto.conf</literal> regardless of the value of
> +        this setting.

I like that you explained how this can be bypassed.

> +
> +       <para>
> +        Turning this setting off is intended for environments where the
> +        configuration of <productname>PostgreSQL</productname> is managed by
> +        some outside mechanism.
> +        In such environments, a well intenioned superuser user might
> +        <emphasis>mistakenly</emphasis> use <command>ALTER SYSTEM</command>
> +        to change the configuration instead of using the outside mechanism.
> +        This might even appear to update the configuration as intended, but

"This might even appear to update" -> "This might temporarily update"

> +        then might be discarded at some point in the future when that outside

"that outside" -> "the outside"

> +        mechanism updates the configuration.
> +        Setting this parameter to <literal>off</literal> can
> +        help to avoid such mistakes.

"help to avoid" ->  "help avoid"

> +       </para>
> +
> +       <para>
> +        This parameter only controls the use of <command>ALTER SYSTEM</command>.
> +        The settings stored in <filename>postgresql.auto.conf</filename> always

"always" -> "still"

Should this paragraph be moved after or as part of the paragraph about
modifying postgresql.auto.conf?

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

  Only you can decide what is important to you.

On Thu, 28 Mar 2024 at 01:43, David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 5:17 PM Bruce Momjian <bruce@momjian.us> wrote:
>>
>> <snip many documentation suggestions>

I addressed them all I think. Mostly the small changes that were
suggested, but I rewrote the sentence with "might be discarded". And I
added references to the new GUC in both places suggested by David.

On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I addressed them all I think. Mostly the small changes that were
> suggested, but I rewrote the sentence with "might be discarded". And I
> added references to the new GUC in both places suggested by David.

Changed the error hint to use "external tool" too. And removed a
duplicate header definition of AllowAlterSystem (I moved it to guc.h
so it was together with other definitions a few patches ago, but
apparently forgot to remove it from guc_tables.h)

On Thu, Mar 28, 2024 at 5:42 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > I addressed them all I think. Mostly the small changes that were
> > suggested, but I rewrote the sentence with "might be discarded". And I
> > added references to the new GUC in both places suggested by David.
>
> Changed the error hint to use "external tool" too. And removed a
> duplicate header definition of AllowAlterSystem (I moved it to guc.h
> so it was together with other definitions a few patches ago, but
> apparently forgot to remove it from guc_tables.h)

I disagree with a lot of these changes. I think the old version was
mostly better. But I can live with a lot of it if it makes other
people happy. However:

+        Which might result in unintended behavior, such as the external tool
+        discarding the change at some later point in time when it updates the
+        configuration.

This is not OK from a grammatical point of view. You can't just start
a sentence with "which" like this. You could replace "Which" with
"This", though.

+ if (!AllowAlterSystem)
+ {
+
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER SYSTEM is not allowed in this environment"),
+ errhint("Global configuration changes should be made using an
external tool, not by using ALTER SYSTEM.")));
+ }

The extra blank line should go. The brackets should go. And I think
the errhint should go, too, because the errhint implies that we know
why the user chose to set allow_alter_system=false. There's no reason
for this message to be opinionated about that.

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

On Thu, 28 Mar 2024 at 12:57, Robert Haas <robertmhaas@gmail.com> wrote:
> I disagree with a lot of these changes. I think the old version was
> mostly better. But I can live with a lot of it if it makes other
> people happy.

I'd have been fine with many of the previous versions of the docs too.
(I'm not a native english speaker though, so that might be part of it)

> However:

Attached is a patch with your last bit of feedback addressed.

On Wed, Mar 27, 2024 at 6:24 PM Bruce Momjian <bruce@momjian.us> wrote:
> Please ignore my complaints, and my apologies.
>
> As far as the GUC change, let's just be careful since we have a bad
> history of pushing things near the end that we regret.  I am not saying
> that would be this feature, but let's be careful.

Even if what I had pushed was the patch itself, so what? This patch
has been sitting around, largely unchanged, for six months. There has
been plenty of time for wordsmithing the documentation, yet nobody got
interested in doing it until I expressed interest in committing the
patch. Meanwhile, there are over 100 other patches that no committer
is paying attention to right now, some of which could probably really
benefit from some wordsmithing of the documentation. It drives me
insane that this is the patch everyone is getting worked up about.
This is a 27-line code change that does something many people want,
and we're acting like the future of the project depends on it. Both I
and others have committed thousands of lines of new code over the last
few months that could easily be full of bugs that will eat your data
without nearly the scrutiny that this patch is getting.

To be honest, I had every intention of pushing the main patch right
after I pushed that preliminary patch, but I stopped because I saw you
had emailed the thread. I'm pretty sure that I would have missed the
fact that the documentation hadn't been properly updated for the fact
that the sense of the GUC had been inverted. That would have been
embarrassing, and I would have had to push a follow-up commit to fix
that. But no real harm would have been done, except that somebody
surely would have seized on my mistake as proof that this patch wasn't
ready to be committed and that I was being irresponsible and
inconsiderate by pushing forward with it, which is a garbage argument.
Committers make mistakes like that all the time, every week, even
every day. It doesn't mean that they're bad committers, and it doesn't
mean that the patches suck. Some of the patches that get committed do
suck, but it's not because there are a few words wrong in the
documentation.

Let's please, please stop pretending like this patch is somehow
deserving of special scrutiny. There's barely even anything to
scrutinize. It's literally if (!variable) ereport(...) plus some
boilerplate and docs. I entirely agree that, because of the risk of
someone filing a bogus CVE, the docs do need to be carefully worded.
But, I'm going to be honest: I feel completely confident in my ability
to review a patch well enough to know whether the documentation for a
single test-and-ereport has been done up to project standard. It
saddens and frustrates me that you don't seem to agree.

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

On Thu, Mar 28, 2024 at 08:38:24AM -0400, Robert Haas wrote:
> Let's please, please stop pretending like this patch is somehow
> deserving of special scrutiny. There's barely even anything to
> scrutinize. It's literally if (!variable) ereport(...) plus some
> boilerplate and docs. I entirely agree that, because of the risk of
> someone filing a bogus CVE, the docs do need to be carefully worded.
> But, I'm going to be honest: I feel completely confident in my ability
> to review a patch well enough to know whether the documentation for a
> single test-and-ereport has been done up to project standard. It
> saddens and frustrates me that you don't seem to agree.

The concern about this patch is not its contents but because it is our
first attempt at putting limits on the superuser for an external tool. 
If done improperly, this could open a flood of problems, including CVE
and user confusion, which would reflect badly on the project.

I think the email discussion has expressed those concerns clearly, and
it is only recently that we have gotten to a stage where we are ready to
add this, and doing this near the closing of the last commitfest can be
a valid concern.  I do agree with your analysis of other patches in the
commitfest, but I just don't see them stretching our boundaries like
this patch.

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

  Only you can decide what is important to you.

On Thu, Mar 28, 2024 at 1:46 PM Bruce Momjian <bruce@momjian.us> wrote:
> The concern about this patch is not its contents but because it is our
> first attempt at putting limits on the superuser for an external tool.
> If done improperly, this could open a flood of problems, including CVE
> and user confusion, which would reflect badly on the project.
>
> I think the email discussion has expressed those concerns clearly, and
> it is only recently that we have gotten to a stage where we are ready to
> add this, and doing this near the closing of the last commitfest can be
> a valid concern.  I do agree with your analysis of other patches in the
> commitfest, but I just don't see them stretching our boundaries like
> this patch.

I do understand the concern, and I'm not saying that you're wrong to
have it at some level, but I do sincerely think it's excessive. I
don't think this is even close to being the scariest patch in this
release, or even in this CommitFest. I also agree that doing things
near the end of the last CommitFest isn't great, because even if your
patch is fantastic, people start to think maybe you're only committing
it to beat the deadline, and then the conversation can get unpleasant.
However, I don't think that's really what is happening here. If this
patch gets bounced out of this release, it won't be in any better
shape a year from now than it is right now. It can't be, because the
code is completely trivial; and the documentation has already been
extensively wordsmithed. Surely we don't need another whole release
cycle to polish three paragraphs of documentation. I think it has to
be right to get this done while we're all thinking about it and the
issue is fresh in everybody's mind.

How would you like to proceed from here? I think that in addressing
all of the comments given in the last few days, the documentation has
gotten modestly worse. I think it was crisp and clear before, and now
it feels a little ... over-edited. But if you're happy with the latest
version, we can go with that. Or, do you need more time to review?

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

On Thu, Mar 28, 2024 at 02:43:38PM -0400, Robert Haas wrote:
> How would you like to proceed from here? I think that in addressing
> all of the comments given in the last few days, the documentation has
> gotten modestly worse. I think it was crisp and clear before, and now
> it feels a little ... over-edited. But if you're happy with the latest
> version, we can go with that. Or, do you need more time to review?

I am fine with moving ahead.  I thought my later emails explaining we
have to be careful communicated that.

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

  Only you can decide what is important to you.

On Thu, Mar 28, 2024 at 01:23:36PM +0100, Jelte Fennema-Nio wrote:
> +       <para>
> +        Turning this setting off is intended for environments where the
> +        configuration of <productname>PostgreSQL</productname> is managed by
> +        some external tool.
> +        In such environments, a well intentioned superuser might
> +        <emphasis>mistakenly</emphasis> use <command>ALTER SYSTEM</command>
> +        to change the configuration instead of using the external tool.
> +        This might result in unintended behavior, such as the external tool
> +        discarding the change at some later point in time when it updates the

"discarding" -> "overwriting" ?

> +  <para>
> +   <literal>ALTER SYSTEM</literal> can be disabled by setting
> +   <xref linkend="guc-allow-alter-system"/> to <literal>off</literal>, but this
> +   is no security mechanism (as explained in detail in the documentation for

"is no" -> "is not a"

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

  Only you can decide what is important to you.

On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote:
> I am fine with moving ahead.  I thought my later emails explaining we
> have to be careful communicated that.

OK. Thanks for clarifying. I've committed the patch with the two
wording changes that you suggested in your subsequent email.

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

On Fri, Mar 29, 2024 at 08:46:33AM -0400, Robert Haas wrote:
> On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote:
> > I am fine with moving ahead.  I thought my later emails explaining we
> > have to be careful communicated that.
> 
> OK. Thanks for clarifying. I've committed the patch with the two
> wording changes that you suggested in your subsequent email.

Great, I know this has been frustrating, and you are right that this
wouldn't have been any simpler next year.

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

  Only you can decide what is important to you.

On Fri, Mar 29, 2024 at 10:48 AM Bruce Momjian <bruce@momjian.us> wrote:
> On Fri, Mar 29, 2024 at 08:46:33AM -0400, Robert Haas wrote:
> > On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian <bruce@momjian.us> wrote:
> > > I am fine with moving ahead.  I thought my later emails explaining we
> > > have to be careful communicated that.
> >
> > OK. Thanks for clarifying. I've committed the patch with the two
> > wording changes that you suggested in your subsequent email.
>
> Great, I know this has been frustrating, and you are right that this
> wouldn't have been any simpler next year.

Thanks, Bruce.

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