Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Fri, Mar 29, 2024 at 10:48 AM Bruce Momjian  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  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

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Fri, Mar 29, 2024 at 08:46:33AM -0400, Robert Haas wrote:
> On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian  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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Thu, Mar 28, 2024 at 3:33 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Thu, Mar 28, 2024 at 01:23:36PM +0100, Jelte Fennema-Nio wrote:
> +   
> +Turning this setting off is intended for environments where the
> +configuration of PostgreSQL is managed by
> +some external tool.
> +In such environments, a well intentioned superuser might
> +mistakenly use ALTER SYSTEM
> +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" ?

> +  
> +   ALTER SYSTEM can be disabled by setting
> +to off, but 
> this
> +   is no security mechanism (as explained in detail in the documentation for

"is no" -> "is not a"

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Thu, Mar 28, 2024 at 1:46 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Wed, Mar 27, 2024 at 6:24 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Thu, 28 Mar 2024 at 12:57, Robert Haas  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.


v12-0001-Add-allow_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Thu, Mar 28, 2024 at 5:42 AM Jelte Fennema-Nio  wrote:
> On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio  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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio  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)


v11-0001-Add-allow_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Thu, 28 Mar 2024 at 01:43, David G. Johnston
 wrote:
>
> On Wed, Mar 27, 2024 at 5:17 PM Bruce Momjian  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.


v10-0001-Add-allow_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wed, Mar 27, 2024 at 5:43 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

>
> This section is also the main entry point for users into the configuration
> subsystem and hasn't been updated to reflect this new feature.  That seems
> like an oversight that needs to be corrected.
>
>
Shouldn't the "alter system" reference page receive an update as well?

David J.

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wed, Mar 27, 2024 at 5:17 PM Bruce Momjian  wrote:

> On Thu, Mar 28, 2024 at 12:43:29AM +0100, Jelte Fennema-Nio wrote:
> > +  xreflabel="allow_alter_system">
> > +  allow_alter_system (boolean)
> > +  
> > +   allow_alter_system configuration
> parameter
> > +  
> > +  
> > +  
> > +   
> > +When allow_alter_system is set to
> > +off, an error is returned if the
> ALTER
> > +SYSTEM command is used. This parameter can only be
> set in
>
> "command is used." -> "command is issued." ?
>

"command is executed" seems even better.  I'd take used over issued.


> > +the postgresql.conf file or on the server
> command
> > +line. The default value is on.
> > +   
> > +
> > +   
> > +Note that this setting cannot be regarded as a security
> feature. It
>
> "setting cannot be regarded" -> "setting should not be regarded"
>

"setting must not be regarded" is the correct option here.  Stronger than
should; we are unable to control whether someone can/does regard it
differently.


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

I strongly prefer temporarily over may/might/could.



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

Feel like "external" is a more context appropriate term here than "outside".

External also has precedent.
https://www.postgresql.org/docs/current/config-setting.html#CONFIG-INCLUDES
"External tools may also modify postgresql.auto.conf. It is not recommended
to do this while the server is running,"

That suggests using "external tools" instead of "outside mechanisms"

This section is also the main entry point for users into the configuration
subsystem and hasn't been updated to reflect this new feature.  That seems
like an oversight that needs to be corrected.

> +   
> > +
> > +   
> > +This parameter only controls the use of ALTER
> SYSTEM.
> > +The settings stored in
> postgresql.auto.conf always
>
> "always" -> "still"
>

Neither qualifier is needed, nor does one seem clearly better than the
other.  Always is true so the weaker "still" seems like the worse choice.

The following is a complete and clear sentence.

The settings stored in postgresql.auto.conf take effect even if
allow_alter_system is set to off.


> Should this paragraph be moved after or as part of the paragraph about
> modifying postgresql.auto.conf?
>
>
I like it by itself.

David J.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Thu, Mar 28, 2024 at 12:43:29AM +0100, Jelte Fennema-Nio wrote:
> +  xreflabel="allow_alter_system">
> +  allow_alter_system (boolean)
> +  
> +   allow_alter_system configuration 
> parameter
> +  
> +  
> +  
> +   
> +When allow_alter_system is set to
> +off, an error is returned if the ALTER
> +SYSTEM command is used. This parameter can only be set in

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

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

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

> +only disables the ALTER SYSTEM 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
> +postgresql.auto.conf regardless of the value of
> +this setting.

I like that you explained how this can be bypassed.

> +
> +   
> +Turning this setting off is intended for environments where the
> +configuration of PostgreSQL is managed by
> +some outside mechanism.
> +In such environments, a well intenioned superuser user might
> +mistakenly use ALTER SYSTEM
> +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 off can
> +help to avoid such mistakes.

"help to avoid" ->  "help avoid"

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

"always" -> "still"

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

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Thu, Mar 28, 2024 at 12:47:46AM +0100, Jelte Fennema-Nio wrote:
> On Wed, 27 Mar 2024 at 23:06, Bruce Momjian  wrote:
> > > > > > +some outside mechanism. In such environments, using 
> > > > > > ALTER
> > > > > > +SYSTEM 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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 27 Mar 2024 at 23:06, Bruce Momjian  wrote:
> > > > > +some outside mechanism. In such environments, using 
> > > > > ALTER
> > > > > +SYSTEM 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.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

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

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


v9-0001-Add-allow_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 27 Mar 2024 at 23:23, Bruce Momjian  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.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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 
> wrote:
> 
> On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian  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 <
> postg...@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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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 
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+3wesnhxjvpmutecjxvvst91sqbdo...@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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wed, Mar 27, 2024 at 3:18 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

> On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian  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 <
>> postg...@jeltef.nl> wrote:
>> > > > Alright, changed the GUC name to "allow_alter_system" since that
>> seems
>> > > > to have the most "votes". One other option would be to call it
>> simply
>> > > > "alter_system", just like "jit" is not called "allow_jit" or
>> > > > "enable_jit".
>> > > >
>> > > > But personally I feel that the "allow_alter_system" is clearer than
>> > > > plain "alter_system" for the GUC name.
>> > >
>> > > I agree, and have committed your 0001.
>> >
>> > So, I email "Is this really a patch we think we can push into PG 17. I
>> > am having my doubts," and the patch is applied a few hours after my
>> > email.  Wow.
>>
>> Also odd is that I don't see the commit in git master, so now I am
>> confused.
>>
>
> The main feature being discussed is in the 0002 patch while Robert pushed
> a doc section rename in the 0001 patch.
>
>
Well, the internal category name was changed though the docs did remain
unchanged.

David J.

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wed, Mar 27, 2024 at 3:13 PM Bruce Momjian  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 
> wrote:
> > > > Alright, changed the GUC name to "allow_alter_system" since that
> seems
> > > > to have the most "votes". One other option would be to call it simply
> > > > "alter_system", just like "jit" is not called "allow_jit" or
> > > > "enable_jit".
> > > >
> > > > But personally I feel that the "allow_alter_system" is clearer than
> > > > plain "alter_system" for the GUC name.
> > >
> > > I agree, and have committed your 0001.
> >
> > So, I email "Is this really a patch we think we can push into PG 17. I
> > am having my doubts," and the patch is applied a few hours after my
> > email.  Wow.
>
> Also odd is that I don't see the commit in git master, so now I am
> confused.
>

The main feature being discussed is in the 0002 patch while Robert pushed a
doc section rename in the 0001 patch.

David J.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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  
> > 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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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  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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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 
> > > > ALTER
> > > > +SYSTEM 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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Maciek Sakrejda]

On Wed, Mar 27, 2024, 11:46 Robert Haas  wrote:

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


+1. And while "non-malicious" may technically be more correct, I don't
think it's any clearer.

>

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

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

I think the emphasis is entirely warranted in this case.

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

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wed, Mar 27, 2024 at 10:12 AM Isaac Morland 
wrote:

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

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

David J.

Re: Possibility to disable `ALTER SYSTEM`

[Isaac Morland]

On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane 
wrote:

> The purpose of the setting is to prevent accidental
>> modifications via ALTER SYSTEM in environments where
>
>
> The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just
> "to prevent modifications via ALTER SYSTEM in environments where..." is
> enough?
>

Not necessarily disagreeing, but it's very important nobody ever mistake
this for a security feature. I don't know if the extra word "accidental" is
necessary, but I think that's the motivation.

Re: Possibility to disable `ALTER SYSTEM`

[Greg Sabino Mullane]

>
> The purpose of the setting is to prevent accidental
> modifications via ALTER SYSTEM in environments where


The emphasis on 'accidental' seems a bit heavy here, and odd. Surely, just
"to prevent modifications via ALTER SYSTEM in environments where..." is
enough?

Cheers,
Greg

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 27 Mar 2024 at 16:10, Robert Haas  wrote:
>
> On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian  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 
> > > ALTER
> > > +SYSTEM 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
> > > +on can help to avoid such mistakes.
> > > +   
> >
> > "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.


v8-0001-Rename-COMPAT_OPTIONS_CLIENT-to-COMPAT_OPTIONS_OT.patch
Description: Binary data


v8-0002-Add-allow_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Wed, Mar 27, 2024 at 11:01 AM Bruce Momjian  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.

> > +
> > +   
> > +Note that this setting cannot be regarded as a security feature. It
> > +only disables the ALTER SYSTEM 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 ALTER
> > +SYSTEM 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
> > +on can help to avoid such mistakes.
> > +   
>
> "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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Wed, Mar 27, 2024 at 10:43 AM Jelte Fennema-Nio  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

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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

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

> +
> +   
> +Note that this setting cannot be regarded as a security feature. It
> +only disables the ALTER SYSTEM 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
> +postgresql.auto.conf regardless of the value of
> +this setting. The purpose of the setting is to prevent
> +accidental modifications via ALTER
> +SYSTEM in environments where
> +PostgreSQL its configuration is managed by

"its"?

> +some outside mechanism. In such environments, using ALTER
> +SYSTEM 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
> +on can help to avoid such mistakes.
> +   

"off"

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

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 27 Mar 2024 at 02:24, Andrew Dunstan  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.


v7-0002-Add-allow_alter_system-GUC.patch
Description: Binary data


v7-0001-Rename-COMPAT_OPTIONS_CLIENT-to-COMPAT_OPTIONS_OT.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Andrew Dunstan]

> On Mar 27, 2024, at 3:53 AM, Tom Lane  wrote:
> 
> Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

Bruce Momjian  writes:
> I am thinking "enable_alter_system_command" is probably good because we
> already use "enable" so why not reuse that idea, and I think "command"
> is needed because we need to clarify we are talking about the command,
> and not generic altering of the system.  We could use
> "enable_sql_alter_system" if people want something shorter.

Robert already mentioned why not use "enable_": up to now that prefix
has only been applied to planner plan-type-enabling GUCs.  I'd be okay
with "allow_alter_system_command", although I find it unnecessarily
verbose.

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

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

regards, tom lane

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Tue, Mar 26, 2024 at 10:23:51AM -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian  wrote:
> >> To me, externally_managed_configuration is promising a lot more than it
> >> delivers because there is still a lot of ocnfiguration it doesn't
> >> control.  I am also confused why the purpose of the feature, external
> >> management of configuation, is part of the variable name.  We usually
> >> name parameters for what they control.
> 
> > I actually agree with this. I wasn't going to quibble with it because
> > other people seemed to like it. But I think something like
> > allow_alter_system would be better, as it would describe the exact
> > thing that the parameter does, rather than how we think the parameter
> > ought to be used.
> 
> +1.  The overpromise-and-underdeliver aspect of the currently proposed
> name is a lot of the reason I've been unhappy and kept pushing for it
> to lock things down more.  "allow_alter_system" is a lot more
> straightforward about exactly what it does, and if that is all we want
> it to do, then a name like that is good.

I am thinking "enable_alter_system_command" is probably good because we
already use "enable" so why not reuse that idea, and I think "command"
is needed because we need to clarify we are talking about the command,
and not generic altering of the system.  We could use
"enable_sql_alter_system" if people want something shorter.

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

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Tue, Mar 26, 2024 at 8:55 AM Abhijit Menon-Sen  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

Re: Possibility to disable `ALTER SYSTEM`

[Abhijit Menon-Sen]

At 2024-03-26 08:11:33 -0400, robertmh...@gmail.com wrote:
>
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian  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)

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 26 Mar 2024, at 13:11, Robert Haas  wrote:
> On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 25, 2024 at 5:04 PM Bruce Momjian  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

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Mon, Mar 25, 2024 at 09:40:55PM +0100, Jelte Fennema-Nio wrote:
> On Mon, 25 Mar 2024 at 20:16, Bruce Momjian  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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Mon, 25 Mar 2024 at 20:16, Bruce Momjian  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"

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 25, 2024 at 2:26 PM Tom Lane  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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Mon, Mar 25, 2024 at 7:27 PM Tom Lane  wrote:

> Robert Haas  writes:
> > OK, great. The latest patch doesn't specifically talk about backing it
> > up with filesystem-level controls, but it does clearly say that this
> > feature is not going to stop a determined superuser from bypassing the
> > feature, which I think is the appropriate level of detail. We don't
> > actually know whether a user has filesystem-level controls available
> > on their system that are equal to the task; certainly chmod isn't good
> > enough, unless you can prevent the superuser from just running chmod
> > again, which you probably can't. An FS-level immutable flag or some
> > other kind of OS-level wizardry might well get the job done, but I
> > don't think our documentation needs to speculate about that.
>
> True.  For postgresql.conf, you can put it outside the data directory
> and make it be owned by some other user, and the job is done.  It's
> harder for postgresql.auto.conf because that always lives in the data
> directory which is necessarily postgres-writable, so even if you
> did those two things to it the superuser could just rename or
> remove it and then write postgresql.auto.conf of his choosing.
>

Just to add to that -- if you use chattr +i on it, the superuser in
postgres won't be able to rename it -- only the actual root user.

Just chowning it won't help of course, then the rename part works.

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

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 25, 2024 at 1:47 PM Tom Lane  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

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Tue, Mar 19, 2024 at 9:13 AM Jelte Fennema-Nio  wrote:
> On Mon, 18 Mar 2024 at 18:27, Robert Haas  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Treat]

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

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

Robert Treat
https://xzilla.net

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

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

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

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

Good point.

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

Ha!

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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Wed, Mar 20, 2024 at 8:52 PM Robert Haas  wrote:

> On Wed, Mar 20, 2024 at 3:17 PM Magnus Hagander 
> wrote:
> > Right, what I meant is that making it a packaging decision is the better
> place. Wherever it goes, allowing the administrator to choose what fits
> them should be made possible.
>
> +1. Which is also the justification for this patch, when it comes
> right down to it. The administrator gets to decide how the contents of
> postgresql.conf are to be managed on their particular installation.
>

Not really. The administrator can *already* do that. It's trivial.

This patch is about doing it in a way that doesn't produce as ugly a
message.But if we're "delegating" it to packagers and "os administrators",
then the problem is already solved. This patch is about trying to solve it
*without* involving the packagers or OS administrators.

Not saying we shouldn't do it, but I'd argue the exact opposite of yours
aboe, which is that it's very much not the justification of the patch :)



> They can decide that postgresql.conf should be writable by the same
> user that runs PostgreSQL, or not. And they should also be able to
> decide that ALTER SYSTEM is an OK way to change configuration, or that
> it isn't. How we enable them to make that decision is a point for
> discussion, and how exactly we phrase the documentation is a point for
> discussion, but we have no business trying to impose conditions, as if
> they're only allowed to make that decision if they conform to some
> (IMHO ridiculous) requirements that we dictate from on high. It's
> their system, not ours.
>

Agreed on all those except they can already do this. It's just that the
error message is ugly. The path of least resistance would be to just
specifically detect a permissions error on the postgresql.auto.conf file
when you try to do ALTER SYSTEM, and throw at least an error hint about
"you must allow writing to this file for the feature to work".

So this patch isn't at all about enabling this functionality. It's about
making it more user friendly.


I mean, for crying out loud, users can set enable_seqscan=off in
> postgresql.conf and GLOBALLY DISABLE SEQUENTIAL SCANS. They can set
>

This is actually a good example, because it's kind of like this patch. It
doesn't *actually* disable the ability to run sequential scans, it just
disables the "usual way". Just like this patch doesn't prevent the
superuser from editing the config, but it does prevent them droin doing it
"the usual way".



> zero_damaged_pages=on in postgresql.conf and silently remove vast
> quantities of data without knowing that they're doing anything. We
> don't even question that stuff ... although we probably should be
>

I like how you got this far and didn't even mention fsync=off :)

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

Re: Possibility to disable `ALTER SYSTEM`

[David Steele]

On 3/20/24 22:30, Michael Banck wrote:


On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote:

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

Re: Possibility to disable `ALTER SYSTEM`

[Michael Banck]

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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

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

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

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

It's not.

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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Wed, Mar 20, 2024 at 8:14 PM Robert Haas  wrote:

> On Wed, Mar 20, 2024 at 3:11 PM Magnus Hagander 
> wrote:
> > I would argue that having the default permissions not allow postgres to
> edit it's own config files *except* for postgresql.auto.conf would be a
> better default than what we have now, but that's completely independent of
> the patch being discussed on this thread. (And FWIW also already solved on
> debian-based platforms for example, which but the main config files in /etc
> with postgres only having read permissions on them - and having the
> *packagers* adapt such things for their platforms in general seems like a
> better place).
>
> I don't think that I agree that it's categorically better, but it
> might be better for some people or in some circumstances. I very much
> do agree that it's a packaging question rather than our job to sort
> out.
>

Right, what I meant is that making it a packaging decision is the better
place. Wherever it goes, allowing the administrator to choose what fits
them should be made possible.

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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

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

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

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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Wed, Mar 20, 2024 at 8:04 PM Robert Haas  wrote:

> On Wed, Mar 20, 2024 at 11:07 AM Jelte Fennema-Nio 
> wrote:
> > > Ugh, please let's not do this. This was bouncing around in my head
> last night, and this is really a quite radical change - especially just to
> handle the given ask, which is to prevent a specific command from running.
> Not implement a brand new security system. There are so many ways this
> could go wrong if we start having separate permissions for some of our
> files. In addition to backups and other tools that need to write to the
> conf files as the postgres user, what about systems that create a new
> cluster automatically e.g. Patroni? It will now need elevated privs just to
> create the conf files and assign the new ownership to them. Lots of moving
> pieces there and ways things could go wrong. So a big -1 from me, as they
> say/ :)
> >
> > Well put. I don't think the effort of making all tooling handle this
> > correctly is worth the benefit that it brings. afaict everyone on this
> > thread that actually wants to use this feature would be happy with the
> > functionality that the current patch provides (i.e. having
> > postgresql.auto.conf writable, but having ALTER SYSTEM error out).
>
> Yeah, I agree with this completely. I don't understand why people who
> hate the feature and hope it dies in a fire get to decide how it has
> to work.
>
> And also, if we verify that the configuration files are all read-only
> at the OS level, that also prevents the external tool from managing
> them. Well, it can: it can make them non-read-only after server start,
> then modify them, then make them read-only again, and it can make sure
> that if the system crashes, it again marks them read-only before
> trying to start PG. But it seems quite obvious that this will be
> inconvenient and difficult to get right. I find it quite easy to
> understand the idea that someone wants the PostgreSQL configuration to
> be managed by Kubernetes rather than via by ALTER SYSTEM, but I can't
> think of any scenario when you just don't want to be able to manage
> the configuration at all. Who in the world would want that?
>

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

I would argue that having the default permissions not allow postgres to
edit it's own config files *except* for postgresql.auto.conf would be a
better default than what we have now, but that's completely independent of
the patch being discussed on this thread. (And FWIW also already solved on
debian-based platforms for example, which but the main config files in /etc
with postgres only having read permissions on them - and having the
*packagers* adapt such things for their platforms in general seems like a
better place).

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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

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

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

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

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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

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


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

Re: Possibility to disable `ALTER SYSTEM`

[Greg Sabino Mullane]

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

Ugh, please let's not do this. This was bouncing around in my head last
night, and this is really a quite radical change - especially just to
handle the given ask, which is to prevent a specific command from running.
Not implement a brand new security system. There are so many ways this
could go wrong if we start having separate permissions for some of our
files. In addition to backups and other tools that need to write to the
conf files as the postgres user, what about systems that create a new
cluster automatically e.g. Patroni? It will now need elevated privs just to
create the conf files and assign the new ownership to them. Lots of moving
pieces there and ways things could go wrong. So a big -1 from me, as they
say/ :)

Cheers,
Greg

Re: Possibility to disable `ALTER SYSTEM`

[Michael Banck]

Hi,

On Tue, Mar 19, 2024 at 10:51:50AM -0400, Tom Lane wrote:
> Heikki Linnakangas  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

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

Andrew Dunstan  writes:
> On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander  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

Re: Possibility to disable `ALTER SYSTEM`

[Andrew Dunstan]

On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander  wrote:

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


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

cheers

andrew

Re: Possibility to disable `ALTER SYSTEM`

[walther]

Greg Sabino Mullane:
On Tue, Mar 19, 2024 at 12:05 PM Tom Lane > 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 = "", 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

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 19 Mar 2024, at 15:51, Tom Lane  wrote:
> 
> Heikki Linnakangas  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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Tue, Mar 19, 2024 at 3:52 PM Tom Lane  wrote:
>
> Heikki Linnakangas  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

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 19 Mar 2024, at 17:53, Jelte Fennema-Nio  wrote:
> 
> On Tue, 19 Mar 2024 at 17:05, Tom Lane  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

Re: Possibility to disable `ALTER SYSTEM`

[Greg Sabino Mullane]

On Tue, Mar 19, 2024 at 12:05 PM Tom Lane  wrote:

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

There is a huge gap between using a well-documented standard tool like
ALTER SYSTEM and going out of your way to modify the configuration files
through trickery. I think we need to only solve the former as in "hey,
please don't do that because your changes will be overwritten"

Cheers,
Greg

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Tue, 19 Mar 2024 at 17:05, Tom Lane  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).

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

Jelte Fennema-Nio  writes:
> On Tue, 19 Mar 2024 at 15:52, Tom Lane  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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Tue, 19 Mar 2024 at 15:52, Tom Lane  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  wrote:
>
> Heikki Linnakangas  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

Re: Possibility to disable `ALTER SYSTEM`

[Tom Lane]

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

Re: Possibility to disable `ALTER SYSTEM`

[Joe Conway]

On 3/19/24 07:49, Andrew Dunstan wrote:



On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas > 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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Mon, 18 Mar 2024 at 18:27, Robert Haas  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  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  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)
>


v6-0002-Add-externally_managed_configuration-GUC.patch
Description: Binary data


v6-0001-Rename-COMPAT_OPTIONS_CLIENT-to-COMPAT_OPTIONS_OT.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Andrew Dunstan]

On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas  wrote:

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

I agree with pretty much all of this.

cheers

andrew

Re: Possibility to disable `ALTER SYSTEM`

[Heikki Linnakangas]

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)

Re: Possibility to disable `ALTER SYSTEM`

[Greg Sabino Mullane]

Going to agree with Robert Treat here about an extension being a great
solution. I resisted posting earlier as I wanted to see how this all pans
out, but I wrote a quick little POC extension some months ago that does
the disabling and works well (and cannot be easily worked around).

On Mon, Mar 18, 2024 at 4:59 PM Robert Haas  wrote:

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

Seems we can just use ProcessUtility and:
if (IsA(parsetree, AlterSystemStmt) { ereport(ERROR, ...

When we know that a feature is
> widely-needed, it's better to have one good implementation of it in
> core than several perhaps not-so-good implementations out of core.
>

Meh, maybe. This one seems pretty dirt simple. Granted, I have expanded my
original POC to allow *some* things to be changed by ALTER SYSTEM, but the
original use case warrants a very small extension.

That allows us to focus all of our efforts on that one implementation
> instead of splitting them across several -- which is the whole selling
> point of open source, really -- and it makes it easier for users who
> want the feature to get access to it.
>

Well, yeah, but they have to wait until version 18 at best, while an
extension can run on any current version and probably be pretty
future-proof as well.

Cheers,
Greg

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 18, 2024 at 4:07 PM Robert Treat  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Treat]

On Thu, Mar 14, 2024 at 12:37 PM Robert Haas  wrote:
>
> On Tue, Feb 13, 2024 at 2:05 AM Joel Jacobson  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

Re: Possibility to disable `ALTER SYSTEM`

[Maciek Sakrejda]

On Mon, Mar 18, 2024 at 10:27 AM Robert Haas  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 18, 2024 at 12:19 PM Maciek Sakrejda  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Mon, Mar 18, 2024 at 11:46 AM Magnus Hagander  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

Re: Possibility to disable `ALTER SYSTEM`

[Maciek Sakrejda]

On Mon, Mar 18, 2024 at 7:12 AM Jelte Fennema-Nio  wrote:
>
> On Mon, 18 Mar 2024 at 13:57, Robert Haas  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?

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Mon, Mar 18, 2024 at 4:44 PM Daniel Gustafsson  wrote:
>
> > On 18 Mar 2024, at 16:34, Magnus Hagander  wrote:
> >
> > On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson  wrote:
> >>
> >>> On 18 Mar 2024, at 13:57, Robert Haas  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/

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 18 Mar 2024, at 16:34, Magnus Hagander  wrote:
> 
> On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson  wrote:
>> 
>>> On 18 Mar 2024, at 13:57, Robert Haas  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

Re: Possibility to disable `ALTER SYSTEM`

[Magnus Hagander]

On Mon, Mar 18, 2024 at 2:09 PM Daniel Gustafsson  wrote:
>
> > On 18 Mar 2024, at 13:57, Robert Haas  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/

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Mon, 18 Mar 2024 at 13:57, Robert Haas  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  wrote:
>
> > On 18 Mar 2024, at 13:57, Robert Haas  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)


v5-0001-Add-enable_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 18 Mar 2024, at 13:57, Robert Haas  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

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Fri, Mar 15, 2024 at 7:09 AM Jelte Fennema-Nio  wrote:
> On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson  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 enable_alter_system is set to
off, an error is returned if the ALTER
SYSTEM command is used. This parameter can only be set in
the postgresql.conf file or on the server command
line. The default value is on.

Note that this setting cannot be regarded as a security feature. It
only disables the ALTER SYSTEM 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
postgresql.auto.conf regardless of the value of
this setting. The purpose of the setting is to prevent
accidental modifications via ALTER
SYSTEM in environments where PostgreSQL's
configuration is managed by some outside mechanism. In such
environments, using ALTER SYSTEM 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 false 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 

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Fri, 15 Mar 2024 at 11:08, Daniel Gustafsson  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.


v4-0001-Add-enable_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Daniel Gustafsson]

> On 15 Mar 2024, at 03:58, Bruce Momjian  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  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

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Thu, 14 Mar 2024 at 22:15, Maciek Sakrejda  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.


v3-0001-Add-enable_alter_system-GUC.patch
Description: Binary data

Re: Possibility to disable `ALTER SYSTEM`

[Bruce Momjian]

On Thu, Mar 14, 2024 at 07:43:15PM -0400, Robert Haas wrote:
> On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda  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  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Possibility to disable `ALTER SYSTEM`

[Robert Haas]

On Thu, Mar 14, 2024 at 5:15 PM Maciek Sakrejda  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

Re: Possibility to disable `ALTER SYSTEM`

[Maciek Sakrejda]

On Thu, Mar 14, 2024 at 1:38 PM Robert Haas  wrote:
> On Thu, Mar 14, 2024 at 4:08 PM Tom Lane  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