Horton

Thread

Commits

  1. Allow granting SET and ALTER SYSTEM privileges on GUC parameters.

  1. Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-15T20:37:44Z

    Hackers,

In the ongoing effort [1] to reduce the number of tasks which require operating under superuser privileges, this patch extends the system to allow, per GUC variable, the ability to SET or ALTER SYSTEM for the variable.  A previous patch set was submitted [2] which created hard-coded privileged roles with the authority to manage associated hard-coded sets of GUC variables.  This current patch appears superior in several ways:

- It allows much greater flexibility in how roles and GUCs are associated
- Custom GUC variables defined by extensions can be covered by this approach

and perhaps most importantly,

- It's what Andrew suggested

Granting SET privilege on a USERSET variable makes no practical difference, but for SUSET variables it does, and granting ALTER SYSTEM is meaningful for all variables. The patch does not mandate that non-login roles be created for this, but as a usage suggestion, one could define a non-login role and assign privileges for a set of GUCs, such as:

  CREATE ROLE regress_host_resource_admin NOSUPERUSER;
  GRANT SET VALUE, ALTER SYSTEM ON
    autovacuum_work_mem, hash_mem_multiplier, logical_decoding_work_mem,
    maintenance_work_mem, max_stack_depth, min_dynamic_shared_memory,
    shared_buffers, temp_buffers, temp_file_limit, work_mem
  TO regress_host_resource_admin;

and then delegate authority to manage the set of GUCs to a non-superuser by granting membership in non-login role:

  CREATE ROLE regress_admin_member IN ROLE regress_host_resource_admin;

One disadvantage of this approach is that the GUC variables are represented both in the list of C structures in guc.c and in the new system catalog pg_config_param's .dat file.  Failure to enter a GUC in the .dat file will result in the inability to grant privileges on the GUC, at least unless/until you run CREATE CONFIGURATION PARAMETER on the GUC.  (This is, in fact, how extension scripts deal with the issue.)  It would perhaps be better if the list of GUCs were not duplicated, but I wasn't clever enough to find a clean way to do that without greatly expanding the patch (nor did I complete prototyping any such thing.)

  2. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-16T15:03:34Z

    On Mon, Nov 15, 2021 at 3:37 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> One disadvantage of this approach is that the GUC variables are represented both in the list of C structures in guc.c and in the new system catalog pg_config_param's .dat file.  Failure to enter a GUC in the .dat file will result in the inability to grant privileges on the GUC, at least unless/until you run CREATE CONFIGURATION PARAMETER on the GUC.  (This is, in fact, how extension scripts deal with the issue.)  It would perhaps be better if the list of GUCs were not duplicated, but I wasn't clever enough to find a clean way to do that without greatly expanding the patch (nor did I complete prototyping any such thing.)

I think this imposes an unnecessary burden on developers. It seems
like it would be easy to write some code that lives inside guc.c and
runs during bootstrap, and it could just iterate over each
ConfigureNamesWhatever array and insert catalog entries for everything
it finds.

It's also going to be important to think about what happens with
extension GUCs. If somebody installs an extension, we can't ask them
to perform a manual step in order to be able to grant privileges. And
if somebody then loads up a different .so for that extension, the set
of GUCs that it provides can change without any DDL being executed.
New GUCs could appear, and old GUCs could vanish.

So I wonder if we should instead not do what I proposed above, and
instead just adjust the GRANT command to automatically insert a new
row into the relevant catalog if there isn't one already. That seems
nicer for extensions, and also nicer for core GUCs, since it avoids
bloating the catalog with a bunch of entries that aren't needed.

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

  3. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T15:17:15Z

    Robert Haas <robertmhaas@gmail.com> writes:
> It's also going to be important to think about what happens with
> extension GUCs. If somebody installs an extension, we can't ask them
> to perform a manual step in order to be able to grant privileges. And
> if somebody then loads up a different .so for that extension, the set
> of GUCs that it provides can change without any DDL being executed.
> New GUCs could appear, and old GUCs could vanish.

Right.  I think that any design that involves per-GUC catalog entries
is going to be an abysmal failure, precisely because the set of GUCs
is not stable enough.  So I'm suspicious of this entire proposal.
Maybe there's a way to make it work, but that way isn't how.

			regards, tom lane

  4. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-16T15:23:27Z

    On Tue, Nov 16, 2021 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Right.  I think that any design that involves per-GUC catalog entries
> is going to be an abysmal failure, precisely because the set of GUCs
> is not stable enough.

In practice it's pretty stable. I think it's just a matter of having a
plan that covers the cases where it isn't stable reasonably elegantly.

We already embed GUC names in catalog entries when someone runs ALTER
USER SET or ALTER DATABASE SET, so this proposal doesn't seem to be
moving the goalposts in any meaningful way.

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

  5. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T15:24:51Z

    
> On Nov 16, 2021, at 7:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> It's also going to be important to think about what happens with
> extension GUCs. If somebody installs an extension, we can't ask them
> to perform a manual step in order to be able to grant privileges.

The burden isn't on the installer of an extension.  As implemented, it's the extension's installation .sql file that sets it up, and the upgrade .sql files that make adjustments, if necessary.

> And
> if somebody then loads up a different .so for that extension, the set
> of GUCs that it provides can change without any DDL being executed.
> New GUCs could appear, and old GUCs could vanish.

Well, the same is true for functions, right?  If you add, remove, or redefine functions in the extension, you need an upgrade script that defines the new functions, removes the old ones, changes function signatures, or whatever.  The same is true here for GUCs.

I don't think we support using a .so that is mismatched against the version of the extension that is installed.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T15:28:57Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Nov 16, 2021 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Right.  I think that any design that involves per-GUC catalog entries
>> is going to be an abysmal failure, precisely because the set of GUCs
>> is not stable enough.

> In practice it's pretty stable. I think it's just a matter of having a
> plan that covers the cases where it isn't stable reasonably elegantly.

Um.  Really the point comes down to having sane default behavior
when there's no entry, which ought to eliminate any need to do the
sort of "run over all the entries at startup" processing that you
seemed to be proposing.  So I guess I don't understand why such a
thing would be needed.

> We already embed GUC names in catalog entries when someone runs ALTER
> USER SET or ALTER DATABASE SET, so this proposal doesn't seem to be
> moving the goalposts in any meaningful way.

True; as long as the expectation is that entries will exist for only
a tiny subset of GUCs, it's probably fine.

			regards, tom lane

  7. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T15:32:07Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> I don't think we support using a .so that is mismatched against the
> version of the extension that is installed.

You are entirely mistaken.  That's not only "supported", it's *required*.
Consider binary upgrades, where the SQL objects are transferred as-is
but the receiving installation may have a different (hopefully newer)
version of the .so.  That .so has to cope with the older SQL object
definitions; it doesn't get to assume that the upgrade script has been
run yet.

			regards, tom lane

  8. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T15:45:14Z

    
> On Nov 16, 2021, at 7:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> I don't think we support using a .so that is mismatched against the
>> version of the extension that is installed.
> 
> You are entirely mistaken.  That's not only "supported", it's *required*.
> Consider binary upgrades, where the SQL objects are transferred as-is
> but the receiving installation may have a different (hopefully newer)
> version of the .so.  That .so has to cope with the older SQL object
> definitions; it doesn't get to assume that the upgrade script has been
> run yet.

You are talking about mismatches in the other direction, aren't you?  I was responding to Robert's point that new gucs could appear, and old gucs disappear.  That seems analogous to new functions appearing and old functions disappearing.  If you upgrade (not downgrade) the .so, the new gucs and functions will be in the .so, but won't be callable/grantable from sql until the upgrade script runs.  The old gucs and functions will be missing from the .so, and attempts to call them/grant them from sql before the upgrade will fail.  What am I missing here?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T15:52:06Z

    
> On Nov 16, 2021, at 7:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> True; as long as the expectation is that entries will exist for only
> a tiny subset of GUCs, it's probably fine.

I understand that bloating a frequently used catalog can be pretty harmful to performance.  I wasn't aware that the size of an infrequently used catalog was critical.  This new catalog would be used during GRANT SET ... and GRANT ALTER SYSTEM commands, which should be rare, and potentially consulted when SET or ALTER SYSTEM commands are issued.  Is there a more substantial performance impact to this than I'm aware?  It can be a bit challenging to run performance tests on such things, given the way everything interacts with everything else. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-16T16:07:36Z

    On Tue, Nov 16, 2021 at 10:45 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> You are talking about mismatches in the other direction, aren't you?  I was responding to Robert's point that new gucs could appear, and old gucs disappear.  That seems analogous to new functions appearing and old functions disappearing.  If you upgrade (not downgrade) the .so, the new gucs and functions will be in the .so, but won't be callable/grantable from sql until the upgrade script runs.  The old gucs and functions will be missing from the .so, and attempts to call them/grant them from sql before the upgrade will fail.  What am I missing here?

It's true that we could impose such a restriction, but I don't think
we should. If I install a different .so, I want the new GUCs to be
grantable immediately, without running any separate DDL.

I also don't think we should burden extension authors with putting
stuff in their upgrade scripts for this. We should solve the problem
in our code rather than forcing them to do so in theirs.

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

  11. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T16:29:06Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> You are talking about mismatches in the other direction, aren't you?  I was responding to Robert's point that new gucs could appear, and old gucs disappear.  That seems analogous to new functions appearing and old functions disappearing.  If you upgrade (not downgrade) the .so, the new gucs and functions will be in the .so, but won't be callable/grantable from sql until the upgrade script runs.

True, but in general users might not care about getting access to new
features (or at least, they might not care until much later, at which
point they'd bother to run ALTER EXTENSION UPDATE).

> The old gucs and functions will be missing from the .so, and attempts to call them/grant them from sql before the upgrade will fail.  What am I missing here?

Here, you are describing behavior that's against project policy and would
be rejected immediately if anyone submitted a patch that made an extension
do that.  Newer .so versions are expected to run successfully, not fail,
with an older version of their SQL objects.  Were this not so, it's almost
inevitable that a binary upgrade would fail, because the extension is
likely to be called into action somewhere before there is any opportunity
to issue ALTER EXTENSION UPDATE.  Even in-place updates of extensions
would become problematic: you can't assume that a rollout of new
executables will be instantly followed by ALTER EXTENSION UPDATE.

Basically, I think the proposed new system catalog is both unworkable
and entirely unnecessary.  Maybe it would have been okay if Peter had
designed GUCs like that a couple of decades ago, but at this point
we have a ton of infrastructure --- much of it not under the core
project's control --- that assumes that GUCs can be created with just
a bit of code.  I do not think that this feature is worth the cost of
changing that.  Or IOW: ALTER SYSTEM SET has gotten along fine without
such a catalog; why can't this feature?

I also notice that the patch seems to intend to introduce tracking
of which user "owns" a GUC, which I think is even more unworkable.
They are all going to wind up owned by the bootstrap superuser
(extension GUCs too), so why bother?

			regards, tom lane

  12. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T16:44:07Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Nov 16, 2021, at 7:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> True; as long as the expectation is that entries will exist for only
>> a tiny subset of GUCs, it's probably fine.

> I understand that bloating a frequently used catalog can be pretty
> harmful to performance.  I wasn't aware that the size of an infrequently
> used catalog was critical.

My concern is not about performance, it's about the difficulty of
maintaining a catalog that expects to be a more-or-less exhaustive
list of GUCs.  I think you need to get rid of that expectation.
In the analogy to ALTER DATABASE/USER SET, we don't expect that
pg_db_role_setting catalog entries will exist for all, or even
very many, GUCs.  Also, the fact that pg_db_role_setting entries
aren't tied very hard to the actual existence of a GUC is a good
thing from the maintenance and upgrade standpoint.

BTW, if we did create such a catalog, there would need to be
pg_dump and pg_upgrade support for its entries, and you'd have to
think about (e.g.) whether pg_upgrade would attempt to preserve
the same OIDs.  I don't see any indication that the patch has
addressed that infrastructure ... which is probably just as well,
since it's work that I'd be wanting to reject.  (Hm, but actually,
doesn't pg_dump need work anyway to dump this new type of GRANT?)

			regards, tom lane

  13. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T19:48:16Z

    
> On Nov 16, 2021, at 8:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> My concern is not about performance, it's about the difficulty of
> maintaining a catalog that expects to be a more-or-less exhaustive
> list of GUCs.  I think you need to get rid of that expectation.

I'm preparing a new version of the patch that has the catalog empty to begin with, and only adds values in response to GRANT commands.  That also handles the issues of extension upgrades, which I think the old patch handled better than the discussion on this thread suggested, but no matter.  The new behavior will allow granting privileges on non-existent gucs, just as ALTER ROLE..SET allows registering settings on non-existent gucs.

The reason I had resisted allowing grants of privileges on non-existent gucs is that you can get the following sort of behavior (note the last two lines):

  DROP USER regress_priv_user7;
  ERROR:  role "regress_priv_user7" cannot be dropped because some objects depend on it
  DETAIL:  privileges for table persons2
  privileges for configuration parameter sort_mem
  privileges for configuration parameter no_such_param

Rejecting "no_such_param" in the original GRANT statement seemed cleaner to me, but this discussion suggests pretty strongly that I can't do it that way.  Changing the historical "sort_mem" to the canonical "work_mem" name also seems better to me, as otherwise you could have different grants on the same GUC under different names.  I'm inclined to keep the canonicalization of names where known, but maybe that runs afoul the rule that these grants should not be tied very hard to the GUC?

> In the analogy to ALTER DATABASE/USER SET, we don't expect that
> pg_db_role_setting catalog entries will exist for all, or even
> very many, GUCs.  Also, the fact that pg_db_role_setting entries
> aren't tied very hard to the actual existence of a GUC is a good
> thing from the maintenance and upgrade standpoint.

Doing it that way....

> BTW, if we did create such a catalog, there would need to be
> pg_dump and pg_upgrade support for its entries, and you'd have to
> think about (e.g.) whether pg_upgrade would attempt to preserve
> the same OIDs.  I don't see any indication that the patch has
> addressed that infrastructure ... which is probably just as well,
> since it's work that I'd be wanting to reject.

Yeah, that's why I didn't write it.  I wanted feedback on the basic implementation before doing that work.

>  (Hm, but actually,
> doesn't pg_dump need work anyway to dump this new type of GRANT?)

Yes, if the idea of this kind of grant isn't being outright rejected, then I'll need to write that.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  14. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-16T20:38:07Z

    On 11/16/21 14:48, Mark Dilger wrote:
>
>> On Nov 16, 2021, at 8:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> My concern is not about performance, it's about the difficulty of
>> maintaining a catalog that expects to be a more-or-less exhaustive
>> list of GUCs.  I think you need to get rid of that expectation.
> I'm preparing a new version of the patch that has the catalog empty to begin with, and only adds values in response to GRANT commands.  That also handles the issues of extension upgrades, which I think the old patch handled better than the discussion on this thread suggested, but no matter.  The new behavior will allow granting privileges on non-existent gucs, just as ALTER ROLE..SET allows registering settings on non-existent gucs.



Your original and fairly simple set of patches used hardcoded role names
and sets of GUCs they could update via ALTER SYSTEM. I suggested to you
privately that a more flexible approach would be to drive this from a
catalog table. I had in mind a table of more or less <roleid, guc_name>.
You could prepopulate it with the roles / GUCs from your original patch
set. I don't think it needs to be initially empty. But DBAs would be
able to modify and extend the settings. I agree with Tom that we
shouldn't try to cover all GUCs in the table - any GUC without an entry
can only be updated by a superuser.


To support pg_dump and pg_upgrade, it might be better to have an
enabled/disabled flag rather than to delete rows.


cheers


andrew


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

  15. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T21:02:44Z

    
> On Nov 16, 2021, at 12:38 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> Your original and fairly simple set of patches used hardcoded role names
> and sets of GUCs they could update via ALTER SYSTEM. I suggested to you
> privately that a more flexible approach would be to drive this from a
> catalog table. I had in mind a table of more or less <roleid, guc_name>.

Right.  I prefer a table of <guc_name, acl> matching the structure of most of the rest of the grantable objects, so that entries from pg_depend or pg_shdepend can track the dependencies in the usual way and prevent dangling references when DROP ROLE is executed.  Is there a reason to prefer an ad hoc tables structure?

> You could prepopulate it with the roles / GUCs from your original patch
> set. I don't think it needs to be initially empty. But DBAs would be
> able to modify and extend the settings. I agree with Tom that we
> shouldn't try to cover all GUCs in the table - any GUC without an entry
> can only be updated by a superuser.

The patch already posted on this thread already works that way.  Robert and Tom seemed to infer that all gucs need to be present in the catalog, but in fact, not entering them in the catalog simply means that they aren't grantable.  I think the confusion arose from the fact that I created a mechanism for extension authors to enter additional gucs into the catalog, which gave the false impression that such entries were required.  They are not.  I didn't bother explaining that before, because Robert and Tom both seem to expect all gucs to be grantable, an expectation you do not appear to share.

> To support pg_dump and pg_upgrade, it might be better to have an
> enabled/disabled flag rather than to delete rows.

I'm not really sure what this means.  Are you suggesting that the <guc_name, acl> (or in your formulation <roleid, guc_name>) should have a "bool enabled" field, and when the guc gets dropped, the "enabled" field gets set to false?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-16T22:12:49Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> The patch already posted on this thread already works that way.  Robert and Tom seemed to infer that all gucs need to be present in the catalog, but in fact, not entering them in the catalog simply means that they aren't grantable.  I think the confusion arose from the fact that I created a mechanism for extension authors to enter additional gucs into the catalog, which gave the false impression that such entries were required.  They are not.  I didn't bother explaining that before, because Robert and Tom both seem to expect all gucs to be grantable, an expectation you do not appear to share.

The question is why you need pg_config_param at all, then.
AFAICS it just adds maintenance complexity we could do without.
I think we'd be better off with a catalog modeled on the design of
pg_db_role_setting, which would have entries for roles and lists
of GUC names that those roles could set.

BTW, another objection to pg_config_param as designed here is that
a "name" is not an appropriate way to store possibly-qualified
custom GUC names.  It's not long enough (cf. valid_custom_variable_name).

>> To support pg_dump and pg_upgrade, it might be better to have an
>> enabled/disabled flag rather than to delete rows.

> I'm not really sure what this means.

I didn't see the point of this either.  We really need to KISS here.
Every bit of added complexity in the catalog representation is another
opportunity for bugs-of-omission, not to mention a detail that you
have to provide mechanisms to dump and restore.

			regards, tom lane

  17. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T22:25:12Z

    
> On Nov 16, 2021, at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> The question is why you need pg_config_param at all, then.
> AFAICS it just adds maintenance complexity we could do without.
> I think we'd be better off with a catalog modeled on the design of
> pg_db_role_setting, which would have entries for roles and lists
> of GUC names that those roles could set.

Originally, I was trying to have dependency linkage between two proper types of objects, so that DROP ROLE and DROP CONFIGURATION PARAMETER would behave as expected, complaining about privileges remaining rather than dropping an object and leaving a dangling reference.

I've deleted the whole CREATE CONFIGURATION PARAMETER and DROP CONFIGURATION PARAMETER syntax and implementation, but it still seems odd to me that:

  CREATE ROLE somebody;
  GRANT SELECT ON TABLE sometable TO ROLE somebody;
  GRANT ALTER SYSTEM ON someguc TO ROLE somebody;
  DROP ROLE somebody;
  ERROR:  role "somebody" cannot be dropped because some objects depend on it
  DETAIL:  privileges for table sometable

would not mention privileges for "someguc" as well.  That's why I want configuration parameters to be proper objects with OIDs and with entries in pg_depend and/or pg_shdepend.  Maybe there is some better way to do it, but that's why I've been doing this.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-16T22:45:12Z

    
> On Nov 16, 2021, at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> BTW, another objection to pg_config_param as designed here is that
> a "name" is not an appropriate way to store possibly-qualified
> custom GUC names.  It's not long enough (cf. valid_custom_variable_name).

I was aware of that, but figured not all GUCs have to be grantable.  If it doesn't fit in a NameData, you can't grant on it.

If we want to be more accommodating than that, we can store it as text, just like pg_db_role_names does, but then we need more code complexity to look it up and to verify that it is unique.  (We wouldn't want multiple records for the same <role,guc> pair.) 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  19. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-17T13:26:33Z

    On Tue, Nov 16, 2021 at 2:48 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I'm preparing a new version of the patch that has the catalog empty to begin with, and only adds values in response to GRANT commands.  That also handles the issues of extension upgrades, which I think the old patch handled better than the discussion on this thread suggested, but no matter.  The new behavior will allow granting privileges on non-existent gucs, just as ALTER ROLE..SET allows registering settings on non-existent gucs.
>
> The reason I had resisted allowing grants of privileges on non-existent gucs is that you can get the following sort of behavior (note the last two lines):
>
>   DROP USER regress_priv_user7;
>   ERROR:  role "regress_priv_user7" cannot be dropped because some objects depend on it
>   DETAIL:  privileges for table persons2
>   privileges for configuration parameter sort_mem
>   privileges for configuration parameter no_such_param
>
> Rejecting "no_such_param" in the original GRANT statement seemed cleaner to me, but this discussion suggests pretty strongly that I can't do it that way.

I think it's perfectly fine to refuse a GRANT statement on a GUC that
doesn't exist, and I don't believe I ever said otherwise. What I don't
think is OK is to require a preparatory statement like CREATE
CONFIGURATION PARAMETER to be executed before you can grant
permissions on it. There's no reason for that. If somebody tries to
grant privileges on a GUC that does not exist, fail. If the GUC does
exist but there's no catalog entry, make one. If the GUC exists and
the catalog entry also exists, update it.

At REVOKE time, don't check whether the GUC exists - only check the
catalog. That way people can remove privileges on GUCs that don't
exist any more. If somebody issues a REVOKE and there's no catalog
entry, do nothing. If somebody issues a REVOKE and there is a catalog
entry, remove stuff from it; but if that would leave it completely
empty, instead delete it.

Whenever you create a catalog entry, also add dependency entries
pointing to it. When you delete one, remove those entries.

> Changing the historical "sort_mem" to the canonical "work_mem" name also seems better to me, as otherwise you could have different grants on the same GUC under different names.  I'm inclined to keep the canonicalization of names where known, but maybe that runs afoul the rule that these grants should not be tied very hard to the GUC?

No. If somebody grants privileges on an old name, record the grant
under the canonical name.

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

  20. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-17T13:27:59Z

    On Tue, Nov 16, 2021 at 3:38 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> Your original and fairly simple set of patches used hardcoded role names
> and sets of GUCs they could update via ALTER SYSTEM. I suggested to you
> privately that a more flexible approach would be to drive this from a
> catalog table. I had in mind a table of more or less <roleid, guc_name>.
> You could prepopulate it with the roles / GUCs from your original patch
> set. I don't think it needs to be initially empty. But DBAs would be
> able to modify and extend the settings. I agree with Tom that we
> shouldn't try to cover all GUCs in the table - any GUC without an entry
> can only be updated by a superuser.

I simply can't understand the point of this. You're basically
proposing that somebody has to execute one SQL statement to make a GUC
grantable, and then a second SQL statement to actually grant access to
it. What is the value in that? It is the same person doing both
things, and the system can work out automatically what needs to be
done.

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

  21. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-17T13:32:27Z

    On Tue, Nov 16, 2021 at 5:45 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I was aware of that, but figured not all GUCs have to be grantable.  If it doesn't fit in a NameData, you can't grant on it.

Such restrictions are rather counterintuitive for users, and here it
doesn't even buy anything. Using 'text' rather than 'name' as the data
type isn't going to cost any meaningful amount of performance.

> If we want to be more accommodating than that, we can store it as text, just like pg_db_role_names does, but then we need more code complexity to look it up and to verify that it is unique.  (We wouldn't want multiple records for the same <role,guc> pair.)

If you're verifying that it's unique in any way other than using a
unique index, I think you're doing it wrong.

Also, maybe I'm confused here, but why isn't the schema:

gucoid
gucname
gucacl

IOW, I don't understand why this table has <role,guc> as the primary
key rather than just guc. Everywhere else, we have a single ACL array
for the all privileges on an object. Why wouldn't we do the same thing
here?

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

  22. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-17T14:24:39Z

    On 11/17/21 08:32, Robert Haas wrote:
> On Tue, Nov 16, 2021 at 5:45 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>> I was aware of that, but figured not all GUCs have to be grantable.  If it doesn't fit in a NameData, you can't grant on it.
> Such restrictions are rather counterintuitive for users, and here it
> doesn't even buy anything. Using 'text' rather than 'name' as the data
> type isn't going to cost any meaningful amount of performance.


indeed


>> If we want to be more accommodating than that, we can store it as text, just like pg_db_role_names does, but then we need more code complexity to look it up and to verify that it is unique.  (We wouldn't want multiple records for the same <role,guc> pair.)
> If you're verifying that it's unique in any way other than using a
> unique index, I think you're doing it wrong.


yeah


>
> Also, maybe I'm confused here, but why isn't the schema:
>
> gucoid
> gucname
> gucacl
>
> IOW, I don't understand why this table has <role,guc> as the primary
> key rather than just guc. Everywhere else, we have a single ACL array
> for the all privileges on an object. Why wouldn't we do the same thing
> here?
>


Yes, that should work, It seems like a better scheme.


cheers


andrew

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

  23. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-17T14:31:40Z

    On 11/16/21 17:12, Tom Lane wrote:
>
>>> To support pg_dump and pg_upgrade, it might be better to have an
>>> enabled/disabled flag rather than to delete rows.
>> I'm not really sure what this means.
> I didn't see the point of this either.  We really need to KISS here.
> Every bit of added complexity in the catalog representation is another
> opportunity for bugs-of-omission, not to mention a detail that you
> have to provide mechanisms to dump and restore.
>
> 			


Well, I was trying (perhaps not very well) to imagine how to deal with
someone modifying the permissions of one of the predefined roles. Say
pg_foo has initial permission to set bar and baz, and the DBA removes
permission to set baz. How is pg_dump going to emit the right commands
to allow a safe pg_upgrade? Maybe we should say that the permissions for
the predefined roles are immutable, so only permissions sets for user
defined roles are mutable.


cheers


andrew

-- 

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

  24. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-17T15:57:41Z

    
> On Nov 17, 2021, at 5:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
>> I was aware of that, but figured not all GUCs have to be grantable.  If it doesn't fit in a NameData, you can't grant on it.
> 
> Such restrictions are rather counterintuitive for users, and here it
> doesn't even buy anything. Using 'text' rather than 'name' as the data
> type isn't going to cost any meaningful amount of performance.

That sounds fine.

>> If we want to be more accommodating than that, we can store it as text, just like pg_db_role_names does, but then we need more code complexity to look it up and to verify that it is unique.  (We wouldn't want multiple records for the same <role,guc> pair.)
> 
> If you're verifying that it's unique in any way other than using a
> unique index, I think you're doing it wrong.

No, I'm using a unique index.  I was overthinking it, concerned about changing from name_ops to text_ops and needing the toast table, but that's silly, because I need one for the acl anyway.

> Also, maybe I'm confused here, but why isn't the schema:
> 
> gucoid
> gucname
> gucacl

It is, both in v2 already posted, and in the v3, written but not yet posted, as I haven't finished the pg_dump work, and also I'm waiting to see how this discussion gets resolved before asking for a review of v3.

> IOW, I don't understand why this table has <role,guc> as the primary
> key rather than just guc.

I was responding to Tom's recommendation that I follow the pattern in pg_db_role_setting, and speculating how that would work.  I was not proposing to do it that way.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  25. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-17T16:07:04Z

    
> On Nov 17, 2021, at 6:31 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> Well, I was trying (perhaps not very well) to imagine how to deal with
> someone modifying the permissions of one of the predefined roles. Say
> pg_foo has initial permission to set bar and baz, and the DBA removes
> permission to set baz. How is pg_dump going to emit the right commands
> to allow a safe pg_upgrade? Maybe we should say that the permissions for
> the predefined roles are immutable, so only permissions sets for user
> defined roles are mutable.

I find this somewhat amusing.  When you suggested off-list that I make gucs individually grantable rather than creating predefined roles with privileges, that was a great idea precisely because sites could define their own security policies using their own site-defined roles:

  CREATE ROLE admin_type_a NOLOGIN NOSUPERUSER;
  CREATE ROLE admin_type_b NOLOGIN NOSUPERUSER;
  ...
  GRANT ALTER SYSTEM ON guc_a1, guc_a2, guc_a3, ... TO admin_type_a;
  GRANT ALTER SYSTEM ON guc_b1, guc_b2, guc_b3, ... TO admin_type_b;
  ...

That has all the power of a system based on predefined roles, but with site-specific flexibility, which is better.  So it amuses me that we'd now be talking about granting some of these to predefined roles, as that is a regression in flexibility.  (How would a site revoke it from one of those predefined roles if they wanted a different policy?)

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  26. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-17T17:05:53Z

    On Wed, Nov 17, 2021 at 9:31 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> Well, I was trying (perhaps not very well) to imagine how to deal with
> someone modifying the permissions of one of the predefined roles. Say
> pg_foo has initial permission to set bar and baz, and the DBA removes
> permission to set baz. How is pg_dump going to emit the right commands
> to allow a safe pg_upgrade? Maybe we should say that the permissions for
> the predefined roles are immutable, so only permissions sets for user
> defined roles are mutable.

That's a great question, but it isn't a new problem. If I create a
brand new database and do thIs:

rhaas=# revoke execute on function pg_ls_waldir() from pg_monitor;
REVOKE

And then I do this:

[rhaas pgsql]$ pg_dump

Then the output includes this:

REVOKE ALL ON FUNCTION pg_catalog.pg_ls_waldir(OUT name text, OUT size
bigint, OUT modification timestamp with time zone) FROM pg_monitor;

I recommend looking at how that works and making this work the same way.

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

  27. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-17T17:06:50Z

    On 11/17/21 11:07, Mark Dilger wrote:
>
>> On Nov 17, 2021, at 6:31 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> Well, I was trying (perhaps not very well) to imagine how to deal with
>> someone modifying the permissions of one of the predefined roles. Say
>> pg_foo has initial permission to set bar and baz, and the DBA removes
>> permission to set baz. How is pg_dump going to emit the right commands
>> to allow a safe pg_upgrade? Maybe we should say that the permissions for
>> the predefined roles are immutable, so only permissions sets for user
>> defined roles are mutable.
> I find this somewhat amusing.  When you suggested off-list that I make gucs individually grantable rather than creating predefined roles with privileges, that was a great idea precisely because sites could define their own security policies using their own site-defined roles:
>
>   CREATE ROLE admin_type_a NOLOGIN NOSUPERUSER;
>   CREATE ROLE admin_type_b NOLOGIN NOSUPERUSER;
>   ...
>   GRANT ALTER SYSTEM ON guc_a1, guc_a2, guc_a3, ... TO admin_type_a;
>   GRANT ALTER SYSTEM ON guc_b1, guc_b2, guc_b3, ... TO admin_type_b;
>   ...
>
> That has all the power of a system based on predefined roles, but with site-specific flexibility, which is better.  So it amuses me that we'd now be talking about granting some of these to predefined roles, as that is a regression in flexibility.  (How would a site revoke it from one of those predefined roles if they wanted a different policy?)
>

I agree it's not ideal. At the time I suggested a more flexible approach
I hadn't really thought about the problems of upgrading. If you can come
up with something that works there then I'll be all ears.


cheers


andrew


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

  28. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-17T17:12:06Z

    
> On Nov 17, 2021, at 9:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> I agree it's not ideal. At the time I suggested a more flexible approach
> I hadn't really thought about the problems of upgrading. If you can come
> up with something that works there then I'll be all ears.

Are you talking about upgrades preserving revocations of privileges on gucs from predefined roles, or merely preserving grants and revocation of privileges on gucs to regular roles?  I think the former problem is easily handled by not shipping any predefined roles with such privileges.  The latter problem would seem to be a mere matter of programming, something I'm working on but don't have finished.  (But maybe you see dragons ahead for me that I'm not seeing yet?)

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  29. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-17T20:24:09Z

    On 11/17/21 12:12, Mark Dilger wrote:
>
>> On Nov 17, 2021, at 9:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> I agree it's not ideal. At the time I suggested a more flexible approach
>> I hadn't really thought about the problems of upgrading. If you can come
>> up with something that works there then I'll be all ears.
> Are you talking about upgrades preserving revocations of privileges on gucs from predefined roles, or merely preserving grants and revocation of privileges on gucs to regular roles?  I think the former problem is easily handled by not shipping any predefined roles with such privileges.  The latter problem would seem to be a mere matter of programming, something I'm working on but don't have finished.  (But maybe you see dragons ahead for me that I'm not seeing yet?)
>

Yes, if we don't ship with any preset privileges then the problem I was
thinking about disappears.


cheers


andrew


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

  30. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-23T00:21:42Z

    
> On Nov 16, 2021, at 7:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> It's also going to be important to think about what happens with
> extension GUCs. If somebody installs an extension, we can't ask them
> to perform a manual step in order to be able to grant privileges. And
> if somebody then loads up a different .so for that extension, the set
> of GUCs that it provides can change without any DDL being executed.
> New GUCs could appear, and old GUCs could vanish.

The v3 patch allows grants on unrecognized guc names.  This should allow a grant statement to precede the loading of a new .so which provides the named guc.

> instead just adjust the GRANT command to automatically insert a new
> row into the relevant catalog if there isn't one already. That seems
> nicer for extensions, and also nicer for core GUCs, since it avoids
> bloating the catalog with a bunch of entries that aren't needed.

Grants on GUCs create a new catalog entry if necessary, or update the existing catalog entry if found.

There is a new information_schema.guc_privileges view, not present in v2.

  31. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2021-11-23T16:07:12Z

    On Mon, Nov 22, 2021 at 7:21 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> There is a new information_schema.guc_privileges view, not present in v2.

It's my impression that information_schema is a child of the SQL
standard, and that inventions specific to PG go in pg_catalog.

Also, I think the user-facing name for GUCs is "settings".

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

  32. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-11-24T02:14:54Z

    
> On Nov 23, 2021, at 8:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> It's my impression that information_schema is a child of the SQL
> standard, and that inventions specific to PG go in pg_catalog.
> 
> Also, I think the user-facing name for GUCs is "settings".

Thanks.  These issues should be fixed in v4.

Along the way, I also added has_setting_privilege() functions overlooked in v3 and before.

  33. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2021-12-13T20:56:34Z

    On 11/23/21 21:14, Mark Dilger wrote:
>
>> On Nov 23, 2021, at 8:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> It's my impression that information_schema is a child of the SQL
>> standard, and that inventions specific to PG go in pg_catalog.
>>
>> Also, I think the user-facing name for GUCs is "settings".
> Thanks.  These issues should be fixed in v4.
>
> Along the way, I also added has_setting_privilege() functions overlooked in v3 and before.



This patch had bit-rotted slightly, and I was attempting to remedy it.
However, I got a failure running the TAP tests because of this change:


diff --git a/src/test/modules/test_pg_dump/t/001_base.pl
b/src/test/modules/test_pg_dump/t/001_base.pl
index 16f7610883..7fbf2d871b 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -9,7 +9,12 @@ use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
 
-my $tempdir       = PostgreSQL::Test::Utils::tempdir;
+# my $tempdir       = PostgreSQL::Test::Utils::tempdir;
+my $tempbase = '/tmp/test_pg_dump';
+my $subdir = 0;
+$subdir++ while (-e "$tempbase/$subdir");
+my $tempdir = "$tempbase/$subdir";
+system("mkdir $tempdir");
 


What's going on here?


cheers


andrew

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

  34. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-13T20:58:25Z

    
> On Dec 13, 2021, at 12:56 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> This patch had bit-rotted slightly, and I was attempting to remedy it.

I have that already, and getting ready to post.  Give me a few minutes and I'll repost.

> However, I got a failure running the TAP tests because of this change:
> 
> 
> diff --git a/src/test/modules/test_pg_dump/t/001_base.pl
> b/src/test/modules/test_pg_dump/t/001_base.pl
> index 16f7610883..7fbf2d871b 100644
> --- a/src/test/modules/test_pg_dump/t/001_base.pl
> +++ b/src/test/modules/test_pg_dump/t/001_base.pl
> @@ -9,7 +9,12 @@ use PostgreSQL::Test::Cluster;
>  use PostgreSQL::Test::Utils;
>  use Test::More;
>  
> -my $tempdir       = PostgreSQL::Test::Utils::tempdir;
> +# my $tempdir       = PostgreSQL::Test::Utils::tempdir;
> +my $tempbase = '/tmp/test_pg_dump';
> +my $subdir = 0;
> +$subdir++ while (-e "$tempbase/$subdir");
> +my $tempdir = "$tempbase/$subdir";
> +system("mkdir $tempdir");
>  
> 
> 
> What's going on here?

Yeah, I hit that, too.  That was an accidentally committed bit of local testing.  Please ignore it for now.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  35. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-13T21:33:58Z

    
> On Dec 13, 2021, at 12:58 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> Give me a few minutes and I'll repost.

Ok, this new version is rebased, cleaned up, and has the ability to revoke SET VALUE on most settings which were previously userset, but are now suset with initdb time granted SET VALUE privilege to public.

I have not yet altered gucs defined by contrib modules, but will repost in a few hours, or tomorrow, with those changes.

  36. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-13T22:34:35Z

    
> On Dec 13, 2021, at 1:33 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> but will repost in a few hours

... and here it is:

  37. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2021-12-14T22:26:24Z

    On Mon, Dec 13, 2021 at 5:34 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Dec 13, 2021, at 1:33 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> >
> > but will repost in a few hours
>
> ... and here it is:

currently there is a failure in check-world (not sure if it's known):

diff -U3 /src/postgres/src/test/regress/expected/guc_privs.out
/src/postgres/src/test/regress/results/guc_privs.out
--- /src/postgres/src/test/regress/expected/guc_privs.out 2021-12-14
14:11:45.000000000 +0000
+++ /src/postgres/src/test/regress/results/guc_privs.out 2021-12-14
15:50:52.219583421 +0000
@@ -39,8 +39,10 @@
 ALTER SYSTEM RESET autovacuum;  -- ok
 -- PGC_SUSET
 SET lc_messages = 'en_US.UTF-8';  -- ok
+ERROR:  invalid value for parameter "lc_messages": "en_US.UTF-8"
 RESET lc_messages;  -- ok
 ALTER SYSTEM SET lc_messages = 'en_US.UTF-8';  -- ok
+ERROR:  invalid value for parameter "lc_messages": "en_US.UTF-8"
 ALTER SYSTEM RESET lc_messages;  -- ok
 -- PGC_SU_BACKEND
 SET jit_debugging_support = OFF;  -- fail, cannot be set after connection start

Aside from that I've tested this and it seems to function as
advertised and in my view is worth adding to PG.

One thing that seems like an omission to me is the absence of a
InvokeObjectPostAlterHook in pg_setting_acl_aclcheck or
pg_setting_acl_aclmask so that MAC extensions can also block this,
InvokeObjectPostCreateHook is already in the create path so a
PostAlter hook seems appropriate.

Thank you.

  38. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-15T15:56:12Z

    
> On Dec 14, 2021, at 2:26 PM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> 
> currently there is a failure in check-world (not sure if it's known):

That one is definitely my fault.  'en_US.UTF-8' exists on my platform, so I hadn't noticed.  I've changed it to use 'C', which should be portable.

> One thing that seems like an omission to me is the absence of a
> InvokeObjectPostAlterHook in pg_setting_acl_aclcheck or
> pg_setting_acl_aclmask so that MAC extensions can also block this,
> InvokeObjectPostCreateHook is already in the create path so a
> PostAlter hook seems appropriate.

Good catch, but that seems like a strange place to put a PostAlterHook, so I added it to ExecGrant_Setting for v6, instead.  This seems more consistent with the hook in SetDefaultACL.

(If you are really trying to do Managed Access Control (MAC), wouldn't that be a separate patch which adds security hooks into all *_aclcheck functions?)

  39. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2021-12-15T18:02:35Z

    On Wed, Dec 15, 2021 at 10:56 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Dec 14, 2021, at 2:26 PM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> >
> > currently there is a failure in check-world (not sure if it's known):
>
> That one is definitely my fault.  'en_US.UTF-8' exists on my platform, so I hadn't noticed.  I've changed it to use 'C', which should be portable.
>
> > One thing that seems like an omission to me is the absence of a
> > InvokeObjectPostAlterHook in pg_setting_acl_aclcheck or
> > pg_setting_acl_aclmask so that MAC extensions can also block this,
> > InvokeObjectPostCreateHook is already in the create path so a
> > PostAlter hook seems appropriate.
>
> Good catch, but that seems like a strange place to put a PostAlterHook, so I added it to ExecGrant_Setting for v6, instead.  This seems more consistent with the hook in SetDefaultACL.

Ah, I was actually requesting a hook where the acl check was done for
setting a GUC, such that we could deny setting them in a hook,
something that would be useful for the set_user extension
(github.com/pgaudit/set_user) but having a hook for grant/revoke is
also helpful.

> (If you are really trying to do Managed Access Control (MAC), wouldn't that be a separate patch which adds security hooks into all *_aclcheck functions?)

MAC is mandatory access controls, so something that can be layered on
top of DAC and can only be enforced even on object owners and
superuser. sepgsql is a MAC extension, for example. It uses the object
access hooks to enforce SELinux policy on PG objects.

  40. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-15T18:18:05Z

    
> On Dec 15, 2021, at 10:02 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> 
> Ah, I was actually requesting a hook where the acl check was done for
> setting a GUC, such that we could deny setting them in a hook,
> something that would be useful for the set_user extension
> (github.com/pgaudit/set_user)

Hmm, this seems orthogonal to the patch under discussion.  This patch only adds a pg_setting_acl_aclcheck in ExecSetVariableStmt() for settings which have been explicitly granted, otherwise it works the traditional way (checking whether the setting is suset/userset).  I don't think you'd get MAC support without finding a way to fire the hook for all settings, regardless of their presence in the new pg_setting_acl table.  That is hard, because InvokeObjectPostAlterHook expects the classId (SettingAclRelationId) and the objectId (pg_setting_acl.oid), but you don't have those for many (most?) settings.  As discussed upthread, we *do not* want to force an entry into the table for all settings, only for ones that have been explicitly granted.

Do you agree?  I'm happy to support MAC in this patch if can explain a simple way of doing so.

> but having a hook for grant/revoke is
> also helpful.

Yes, I see no reason to rip this out.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  41. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2021-12-16T15:43:15Z

    On Wed, Dec 15, 2021 at 1:18 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
> > On Dec 15, 2021, at 10:02 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> >
> > Ah, I was actually requesting a hook where the acl check was done for
> > setting a GUC, such that we could deny setting them in a hook,
> > something that would be useful for the set_user extension
> > (github.com/pgaudit/set_user)
>
> Hmm, this seems orthogonal to the patch under discussion.  This patch only adds a pg_setting_acl_aclcheck in ExecSetVariableStmt() for settings which have been explicitly granted, otherwise it works the traditional way (checking whether the setting is suset/userset).  I don't think you'd get MAC support without finding a way to fire the hook for all settings, regardless of their presence in the new pg_setting_acl table.  That is hard, because InvokeObjectPostAlterHook expects the classId (SettingAclRelationId) and the objectId (pg_setting_acl.oid), but you don't have those for many (most?) settings.  As discussed upthread, we *do not* want to force an entry into the table for all settings, only for ones that have been explicitly granted.
>
> Do you agree?  I'm happy to support MAC in this patch if can explain a simple way of doing so.

Ah, I understand now. Would it be possible to pass the
SettingAclRelationId if it exists or InvalidOid if not? That way if a
MAC implementation cares about a particular GUC it'll ensure it's in
pg_setting_acl.

I don't know if others will object to that but it seems like an okay
compromise.

> > but having a hook for grant/revoke is
> > also helpful.
>
> Yes, I see no reason to rip this out.
>

  42. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-12-16T17:53:45Z

    
> On Dec 16, 2021, at 7:43 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> 
> Ah, I understand now. Would it be possible to pass the
> SettingAclRelationId if it exists or InvalidOid if not?

SettingAclRelationId is always defined, so we can always pass that value.  But the settingId itself may sometimes be InvalidOid.

> That way if a
> MAC implementation cares about a particular GUC it'll ensure it's in
> pg_setting_acl.

A much cleaner solution would be to create new ObjectAccessTypes with a corresponding new Invoke macro and Run function.  Those could take setting names, not Oids, and include additional information about whether the operation is SET, RESET or ALTER SYSTEM, what the new value is (if any), what kind of setting it is (bool, int, ...), etc.  I don't think such a patch would even be all that hard to write.

What do you think?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  43. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2021-12-16T18:31:58Z

    On Thu, Dec 16, 2021 at 12:53 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Dec 16, 2021, at 7:43 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> >
> > Ah, I understand now. Would it be possible to pass the
> > SettingAclRelationId if it exists or InvalidOid if not?
>
> SettingAclRelationId is always defined, so we can always pass that value.  But the settingId itself may sometimes be InvalidOid.

Yes, that is what I meant.

> > That way if a
> > MAC implementation cares about a particular GUC it'll ensure it's in
> > pg_setting_acl.
>
> A much cleaner solution would be to create new ObjectAccessTypes with a corresponding new Invoke macro and Run function.  Those could take setting names, not Oids, and include additional information about whether the operation is SET, RESET or ALTER SYSTEM, what the new value is (if any), what kind of setting it is (bool, int, ...), etc.  I don't think such a patch would even be all that hard to write.
>
> What do you think?

Personally, I would be happy with that, but since it's a whole new
hooking method I suspect it'll be an uphill battle. That definitely
seems like another patchset though, if you do submit this I will test
and review.

Thank you.

  44. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-01-26T15:24:31Z

    
> On Dec 15, 2021, at 7:56 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> <v6-0001-Allow-GRANT-of-SET-and-ALTER-SYSTEM-SET-for-gucs.patch>

Rebased:

  45. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-01-31T19:17:12Z

    
> On Jan 26, 2022, at 7:24 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> Rebased:
> 
> <v7-0001-Allow-GRANT-of-SET-and-ALTER-SYSTEM-SET-for-gucs.patch>

Hackers,

Joshua and I had some off-list back-and-forth on this patch and I think the attached v8 patch meets his needs.  Specifically, v8 adds support for object access hooks in the case of SET and ALTER SYSTEM SET of a guc variable.

The way InvokeObjectPostAlterHookArg() is written, it expects some things like a Oid that we don't always have.  I abused the parameters of that function to pass as much information as possible.  Anyone wanting to review this might consider how that is coded.

Joshua, this is byte-for-byte the same patch that you already reviewed off-list (or I attached the wrong patch!)  If you are satisfied, could you set the status to ready-for-committer?

  46. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-06T22:13:44Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> [ v8-0001-Allow-GRANT-of-SET-and-ALTER-SYSTEM-SET-for-gucs.patch ]

I noticed that this is failing in the cfbot as a side-effect of my
cc50080a8, so here's a rebase (basically it just removes the no-
longer-necessary sanity_check.out hunk).

I haven't reviewed this particularly, or read the thread, but I'm
pretty disturbed by the contrib/ changes that it proposes.

1. If we need to change these two contrib modules, doesn't that imply
a lot of changes forced on external modules as well?  What are the
security implications if somebody doesn't make such a change?

2. It looks to me like if someone installs the updated postgres_fdw.so,
but doesn't run ALTER EXTENSION UPDATE, they are going to be left with a
rather broken extension.  This seems not good either, especially if it's
multiplied by a boatload of third-party extensions requiring updates.

So I think some more thought is needed to see if we can't avoid
the need to touch extension modules in this patch.  Or at least
avoid the need for synchronized C-level and SQL-level updates,
because that is going to create a lot of pain for end users.

I'm also fairly distressed by the number of changes in guc.c,
mainly because I fear that that means that pending patches that
add GUC variables will be subtly incorrect/insecure if they're not
updated to account for this.  Frankly, I also reject the apparent
position that we need to support forbidding users from touching
many of these GUCs.  Or, if that's our position, why are there
per-GUC changes at all, rather than just redefining what the
context values mean?  (That is, why not redefine USERSET and
SUSET as simply indicating the default ACL to be applied if there's
no entry in the catalog.)

			regards, tom lane

  47. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-06T22:40:50Z

    
> On Mar 6, 2022, at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> 1. If we need to change these two contrib modules, doesn't that imply
> a lot of changes forced on external modules as well?  What are the
> security implications if somebody doesn't make such a change?
> 
> 2. It looks to me like if someone installs the updated postgres_fdw.so,
> but doesn't run ALTER EXTENSION UPDATE, they are going to be left with a
> rather broken extension.  This seems not good either, especially if it's
> multiplied by a boatload of third-party extensions requiring updates.
> 
> So I think some more thought is needed to see if we can't avoid
> the need to touch extension modules in this patch.  Or at least
> avoid the need for synchronized C-level and SQL-level updates,
> because that is going to create a lot of pain for end users.
> 
> I'm also fairly distressed by the number of changes in guc.c,
> mainly because I fear that that means that pending patches that
> add GUC variables will be subtly incorrect/insecure if they're not
> updated to account for this.  Frankly, I also reject the apparent
> position that we need to support forbidding users from touching
> many of these GUCs.  Or, if that's our position, why are there
> per-GUC changes at all, rather than just redefining what the
> context values mean?  (That is, why not redefine USERSET and
> SUSET as simply indicating the default ACL to be applied if there's
> no entry in the catalog.)

To my knowledge, there is no mechanism to revoke an implicit privilege.  You can revoke a privilege explicitly listed in an aclitem[], but only if the privilege is being tracked that way.

Userset variables are implicitly settable by any user.  There was a request, off-list as I recall, to make it possible to revoke the privilege to set variables such as "work_mem".  To make that possible, but not change the default behavior vis-a-vis prior releases, I upgraded most userset variables to suset with a corresponding grant to public on the variable.  Sites which wish to have a more restrictive policy on such variables can revoke that privilege from public and instead issue more restrictive grants.  There were a few variables where such treatment didn't seem sensible, such as ones to do with client connections, and I left them alone.  I didn't insist on a defense for why any particular setting needed to be revocable in order to apply this treatment.  My assumption was that sites should be allowed to determine their own security policies per setting unless there is a technical difficulty for a given setting that would make it overly burdensome to implement.

It seemed more complete to do the same thing for contrib modules, so I did.

If a third-party module doesn't do the equivalent operation, they would simply continue with their userset variables working as before.  I don't see a specific security concern with that, though I'd be happy to consider arguments to the contrary.

I believe there are also some issues with this patch relating to pg_dump/pg_restore and to pg_upgrade, which EDB is working to address.  We intend to repost something soon.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  48. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-06T22:57:10Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Mar 6, 2022, at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... Or, if that's our position, why are there
>> per-GUC changes at all, rather than just redefining what the
>> context values mean?  (That is, why not redefine USERSET and
>> SUSET as simply indicating the default ACL to be applied if there's
>> no entry in the catalog.)

> To my knowledge, there is no mechanism to revoke an implicit privilege.  You can revoke a privilege explicitly listed in an aclitem[], but only if the privilege is being tracked that way.

So?  What I'm suggesting is along the lines of

(1) pg_setting_acl starts out empty, or at least mostly empty (maybe
there are a few GUCs that need custom values).

(2) If there's a pg_setting_acl entry for a GUC that's to be set,
we apply it: either it grants the desired permission or it doesn't.

(3) If there's no entry, then for a USERSET GUC we assume that the
entry would be like "=s/postgres", while for any other context value
we assume the ACL grants nothing.

I don't think this is materially different from what we do with
permissions on (say) functions.  If you want to revoke the public
SET privilege on some USERSET variable, you instantiate the default
and then revoke.  You end up with an empty ACL stored in pg_setting_acl,
and voila.

It'd likely be necessary to refuse to record a grant/revoke on
an unknown GUC, since if we don't know the GUC then we can't know
what the relevant default ACL ought to be.  But I bet your existing
patch has some dubious behavior in that case too.

			regards, tom lane

  49. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-06T23:06:43Z

    
> On Mar 6, 2022, at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I don't think this is materially different from what we do with
> permissions on (say) functions.  If you want to revoke the public
> SET privilege on some USERSET variable, you instantiate the default
> and then revoke.  You end up with an empty ACL stored in pg_setting_acl,
> and voila.

I assume you mean the implementation of REVOKE does this, not that the user needs to do both a grant and a revoke.

> It'd likely be necessary to refuse to record a grant/revoke on
> an unknown GUC, since if we don't know the GUC then we can't know
> what the relevant default ACL ought to be.  But I bet your existing
> patch has some dubious behavior in that case too.

The existing patch allows grants on unknown gucs, because it can't know what guc an upgrade script will introduce, and the grant statement may need to execute before the guc exists.  That opens a window for granting privileges on non-existent gucs.  That sounds bad, but I don't know of any actual harm that it does, beyond just being ugly.

With your proposal, it sounds like we could avoid that ugliness, so I'm inclined to at least try doing it as you propose.

Thanks for the suggestion!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  50. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-06T23:27:43Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Mar 6, 2022, at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think this is materially different from what we do with
>> permissions on (say) functions.  If you want to revoke the public
>> SET privilege on some USERSET variable, you instantiate the default
>> and then revoke.  You end up with an empty ACL stored in pg_setting_acl,
>> and voila.

> I assume you mean the implementation of REVOKE does this, not that the user needs to do both a grant and a revoke.

Right.  Again, look at what happens when you create a function and
then revoke its default PUBLIC EXECUTE permission.

>> It'd likely be necessary to refuse to record a grant/revoke on
>> an unknown GUC, since if we don't know the GUC then we can't know
>> what the relevant default ACL ought to be.  But I bet your existing
>> patch has some dubious behavior in that case too.

> The existing patch allows grants on unknown gucs, because it can't know what guc an upgrade script will introduce, and the grant statement may need to execute before the guc exists.

Yeah, that's the problematic case.  It might mostly work to assume that
an unknown GUC has an empty default ACL.  This could fail to retain the
default PUBLIC SET permission if it later turns out the GUC is USERSET
... but I suspect in most cases anybody who's messing with the permissions
would've started out by revoking that anyway.  We could make this
definitely work in pg_dump if we teach pg_dump to explicitly grant or
revoke the PUBLIC SET permission anytime it's emitting anything for
a GUC, even if it thinks that would be the default state anyway.
Extension scripts that want to modify permissions for their GUCs
could follow that same principle to be sure.

			regards, tom lane

  51. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-15T20:59:08Z

    
> On Mar 6, 2022, at 3:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> The existing patch allows grants on unknown gucs, because it can't know what guc an upgrade script will introduce, and the grant statement may need to execute before the guc exists.
> 
> Yeah, that's the problematic case.  It might mostly work to assume that
> an unknown GUC has an empty default ACL.  This could fail to retain the
> default PUBLIC SET permission if it later turns out the GUC is USERSET

On further reflection, I concluded this isn't needed.  No current extension, whether in-core or third party, expects to be able to create a new GUC and then grant or revoke permissions on it.  They can already specify the guc context (PGC_USERS, etc).  Introducing a feature that depends on the dubious assumption that unrecognized GUCs will turn out to be USERSET doesn't seem warranted.

The patch attributes all grants of setting privileges to the bootstrap superuser.  Only superusers can grant or revoke privileges on settings, and all settings are implicitly owned by the bootstrap superuser because there is no explicit owner associated with settings.  Consequently, select_best_grantor(some_superuser, ..., BOOTSTRAP_SUPERUSERID, ...) always chooses the bootstrap superuser.  I don't see a problem with this, but wouldn't mind a second opinion.  Some people might find it surprising when viewing the pg_setting_acl.setacl field.

  52. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-16T11:43:28Z

    On 3/15/22 16:59, Mark Dilger wrote:
>> On Mar 6, 2022, at 3:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>> The existing patch allows grants on unknown gucs, because it can't know what guc an upgrade script will introduce, and the grant statement may need to execute before the guc exists.
>> Yeah, that's the problematic case.  It might mostly work to assume that
>> an unknown GUC has an empty default ACL.  This could fail to retain the
>> default PUBLIC SET permission if it later turns out the GUC is USERSET
> On further reflection, I concluded this isn't needed.  No current extension, whether in-core or third party, expects to be able to create a new GUC and then grant or revoke permissions on it.  They can already specify the guc context (PGC_USERS, etc).  Introducing a feature that depends on the dubious assumption that unrecognized GUCs will turn out to be USERSET doesn't seem warranted.

Agreed.


>
> The patch attributes all grants of setting privileges to the bootstrap superuser.  Only superusers can grant or revoke privileges on settings, and all settings are implicitly owned by the bootstrap superuser because there is no explicit owner associated with settings.  Consequently, select_best_grantor(some_superuser, ..., BOOTSTRAP_SUPERUSERID, ...) always chooses the bootstrap superuser.  I don't see a problem with this, but wouldn't mind a second opinion.  Some people might find it surprising when viewing the pg_setting_acl.setacl field.


I think it's OK as long as we document it. An alternative might be to
invent a pseudo-superuser called, say, 'postgres_system', but that seems
like overkill to solve what is in effect a cosmetic problem.

Generally I think this is now in fairly good shape, I've played with it
and it seems to do what I expect in every case, and the things I found
surprising are gone.


cheers


andrew


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

  53. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-16T18:47:24Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> Generally I think this is now in fairly good shape, I've played with it
> and it seems to do what I expect in every case, and the things I found
> surprising are gone.

Stepping back a bit ... do we really want to institutionalize the
term "setting" for GUC variables?  I realize that the view pg_settings
exists, but the documentation generally prefers the term "configuration
parameters".  Where config.sgml uses "setting" as a noun, it's usually
talking about a specific concrete value for a parameter, and you can
argue that the view's name comports with that meaning.  But you can't
GRANT a parameter's current value.

I don't have a better name to offer offhand --- I surely am not proposing
that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x",
because that's way too wordy.  But now is the time to bikeshed if we're
gonna bikeshed, or else admit that we're not interested in precise
vocabulary.

I'm also fairly allergic to the way that this patch has decided to assign
multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
is no existing precedent for that, and I think it's going to break
client-side code that we don't need to break.  It's not coincidental that
this forces weird changes in rules about whitespace in the has_privilege
functions, for example; and if you think that isn't going to cause
problems I think you are wrong.  Perhaps we could just use "SET" and
"ALTER", or "SET" and "SYSTEM"?

I also agree with the upthread criticism that this is abusing the
ObjectPostAlterHook API unreasonably much.  In particular, it's
trying to use pg_setting_acl OIDs as identities for GUCs, which
they are not, first because they're unstable and second because
most GUCs won't even have an entry there.  I therefore judge the
hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
to be 100% useless, in fact probably counterproductive because they
introduce a boatload of worries about whether the right things happen
if the hook errors out or does something guc.c isn't expecting.

I suggest that what might be saner is to consider that the "objects"
that the hook calls are concerned with are the pg_setting_acl entries,
not the underlying GUCs, and thus that the hooks need be invoked only
when creating, destroying or altering those entries.  If we do have
a need for a hook editorializing on GUC value settings, that would
have to be an independent API --- but I have heard no calls for
the ability to have such a hook, and I don't think that this patch
is the place to introduce one.

			regards, tom lane

  54. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-16T18:59:06Z

    
> On Mar 16, 2022, at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables?  I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters".  Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning.  But you can't
> GRANT a parameter's current value.
> 
> I don't have a better name to offer offhand --- I surely am not proposing
> that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x",
> because that's way too wordy.  But now is the time to bikeshed if we're
> gonna bikeshed, or else admit that we're not interested in precise
> vocabulary.

Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the two obvious choices.  I preferred "configuration parameter" originally and was argued out of it.  My take on "setting" was also that it more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter "work_mem" has the setting "8192".  I'm happy to change the patch along these lines, but I vaguely recall it being Robert who liked the "setting" language.  Robert?  (Sorry if I misremember that...)

> I'm also fairly allergic to the way that this patch has decided to assign
> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
> is no existing precedent for that, and I think it's going to break
> client-side code that we don't need to break.  It's not coincidental that
> this forces weird changes in rules about whitespace in the has_privilege
> functions, for example; and if you think that isn't going to cause
> problems I think you are wrong.  Perhaps we could just use "SET" and
> "ALTER", or "SET" and "SYSTEM"?

I chose those particular multi-word names to avoid needing to promote any keywords.  That's been a while ago, and I don't recall exactly where the shift-reduce or reduce-reduce errors were coming from.  I'll play with it to see what I can find.

> I also agree with the upthread criticism that this is abusing the
> ObjectPostAlterHook API unreasonably much.  In particular, it's
> trying to use pg_setting_acl OIDs as identities for GUCs, which
> they are not, first because they're unstable and second because
> most GUCs won't even have an entry there.  I therefore judge the
> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
> to be 100% useless, in fact probably counterproductive because they
> introduce a boatload of worries about whether the right things happen
> if the hook errors out or does something guc.c isn't expecting.

I think Joshua was planning to use these hooks for security purposes.  The hooks are supposed to check whether the Oid is valid, and if not, still be able to make choices based on the other information.  Joshua, any comment on this?

> I suggest that what might be saner is to consider that the "objects"
> that the hook calls are concerned with are the pg_setting_acl entries,
> not the underlying GUCs, and thus that the hooks need be invoked only
> when creating, destroying or altering those entries.

That's certainly a thing we could do, but I got the impression that Joshua wanted to hook into SET, RESET, and ALTER SYSTEM SET, not merely into GRANT and REVOKE.

>  If we do have
> a need for a hook editorializing on GUC value settings, that would
> have to be an independent API --- but I have heard no calls for
> the ability to have such a hook, and I don't think that this patch
> is the place to introduce one.

Well, the call for it was in this thread, but I'm ok with yanking out that part of the patch if it seems half-baked.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  55. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-16T19:00:09Z

    <snip>

> I suggest that what might be saner is to consider that the "objects"
> that the hook calls are concerned with are the pg_setting_acl entries,
> not the underlying GUCs, and thus that the hooks need be invoked only
> when creating, destroying or altering those entries.  If we do have
> a need for a hook editorializing on GUC value settings, that would
> have to be an independent API --- but I have heard no calls for
> the ability to have such a hook, and I don't think that this patch
> is the place to introduce one.

I requested it here:
https://www.postgresql.org/message-id/CAGB%2BVh5pVFAqw8YzeXy4xxmEt_4Hq_8pEUHdCQvv3mCjvC-S-w%40mail.gmail.com

with compromises here:
https://www.postgresql.org/message-id/CAGB%2BVh6wLJ3FKsno62fi54pfg0FDrZRWcpuuCJBkHcCj-G1ndw%40mail.gmail.com
https://www.postgresql.org/message-id/0A3D3CBA-6548-4C9E-9F46-59D5C51A1F31%40enterprisedb.com
https://www.postgresql.org/message-id/CAGB%2BVh65R5vKC4rEt7r2_pK3kMZd-VY0n99RJwcP8Bic7xvOxQ%40mail.gmail.com

and the new version here:
https://www.postgresql.org/message-id/C8DF19D8-C15D-4C2D-91CA-391390F1E421%40enterprisedb.com

which I wrote. a toy module to test and was satisfied with it, despite
the limitations.

If we are adding a DAC Grant for a type of object it seems untenable
that it would not come with analogous MAC capable hooks.

Thank you.

  56. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-16T19:06:04Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Mar 16, 2022, at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... I therefore judge the
>> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
>> to be 100% useless, in fact probably counterproductive because they
>> introduce a boatload of worries about whether the right things happen
>> if the hook errors out or does something guc.c isn't expecting.

> I think Joshua was planning to use these hooks for security purposes.  The hooks are supposed to check whether the Oid is valid, and if not, still be able to make choices based on the other information.  Joshua, any comment on this?

It's going to be hard to do anything useful in a hook that (a) does
not know which GUC is being assigned to and (b) cannot do catalog
accesses for fear that we're not inside a transaction.  (b), in
particular, seems like a rather thorough API break; up to now
ObjectPostAlter hooks could assume that catalog accesses are OK.

			regards, tom lane

  57. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-16T19:36:21Z

    On Wed, Mar 16, 2022 at 3:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
> > On Mar 16, 2022, at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> ... I therefore judge the
> >> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
> >> to be 100% useless, in fact probably counterproductive because they
> >> introduce a boatload of worries about whether the right things happen
> >> if the hook errors out or does something guc.c isn't expecting.
>
> > I think Joshua was planning to use these hooks for security purposes.  The hooks are supposed to check whether the Oid is valid, and if not, still be able to make choices based on the other information.  Joshua, any comment on this?
>
> It's going to be hard to do anything useful in a hook that (a) does
> not know which GUC is being assigned to and (b) cannot do catalog
> accesses for fear that we're not inside a transaction.  (b), in
> particular, seems like a rather thorough API break; up to now
> ObjectPostAlter hooks could assume that catalog accesses are OK.
>

Can you elaborate on this point? This is perhaps an area where I don't
know the rules of the road, to test this hook I modified the set_user
extension, which normally uses the parse tree to figure out if someone
is trying to SET log_statement or ALTER SYSTEM log_statement and
replaced it with:

                case OAT_POST_ALTER:
                if (classId == SettingAclRelationId)
                {
                        ObjectAddress   object;
                        object.classId = SettingAclRelationId;
                        object.objectId = objectId;
                        object.objectSubId = 0;
                        if (strcmp(getObjectIdentity(&object),
"log_statement") == 0)
                        {
                                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                 errmsg("Setting %s blocked",
getObjectIdentity(&object))));
                        }
                  }

Is that inherently unsafe?

  58. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-16T19:58:57Z

    Joshua Brindle <joshua.brindle@crunchydata.com> writes:
> On Wed, Mar 16, 2022 at 3:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's going to be hard to do anything useful in a hook that (a) does
>> not know which GUC is being assigned to and (b) cannot do catalog
>> accesses for fear that we're not inside a transaction.  (b), in
>> particular, seems like a rather thorough API break; up to now
>> ObjectPostAlter hooks could assume that catalog accesses are OK.

> Can you elaborate on this point?

Hmm, I glossed over too many details there perhaps.  I was thinking
about the restrictions on GUC check_hooks, which can be run outside
a transaction altogether.  But that's not quite relevant here.
ExecSetVariableStmt can assume it's inside a transaction, but what
it *can't* assume is that we've set a transaction snapshot as yet
(cf. PlannedStmtRequiresSnapshot).  If we call an ObjectPostAlter hook
there, and it does a catalog access, that's going to break things for
modifications of GUCs that are supposed to be modifiable without
freezing the transaction snapshot.  So the net result is the same:
catalog access not okay, at least not in general.

Between that and the fact that an OID-based API is largely useless here,
I don't think it's sane to try to use the existing ObjectPostAlter API.
Maybe there is a case for inventing a separate hook API that could pass
the GUC name as a string, instead.  I remain of the opinion that this
patch should not concern itself with that, though.

			regards, tom lane

  59. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-16T20:07:43Z

    On 3/16/22 14:47, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Generally I think this is now in fairly good shape, I've played with it
>> and it seems to do what I expect in every case, and the things I found
>> surprising are gone.
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables?  I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters".  Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning.  But you can't
> GRANT a parameter's current value.
>
> I don't have a better name to offer offhand --- I surely am not proposing
> that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x",
> because that's way too wordy.  But now is the time to bikeshed if we're
> gonna bikeshed, or else admit that we're not interested in precise
> vocabulary.
>
> I'm also fairly allergic to the way that this patch has decided to assign
> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
> is no existing precedent for that, and I think it's going to break
> client-side code that we don't need to break.  It's not coincidental that
> this forces weird changes in rules about whitespace in the has_privilege
> functions, for example; and if you think that isn't going to cause
> problems I think you are wrong.  Perhaps we could just use "SET" and
> "ALTER", or "SET" and "SYSTEM"?


That's going to look weird, ISTM. This is less clear about what it's
granting.

     GRANT ALTER ON SOMETHING shared_buffers TO myuser;

If you don't like that maybe ALTER_SYSTEM and SET_VALUE would work,
although mostly we have avoided things like that.

How about MODIFY instead of SET VALUE and CONFIGURE instead of ALTER SYSTEM?

Personally I don't have problem with the use of SETTING. I think the
meaning is pretty plain in context and unlikely to produce any confusion.


cheers


andrew

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

  60. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-16T20:53:02Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/16/22 14:47, Tom Lane wrote:
>> I'm also fairly allergic to the way that this patch has decided to assign
>> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
>> is no existing precedent for that, and I think it's going to break
>> client-side code that we don't need to break.  It's not coincidental that
>> this forces weird changes in rules about whitespace in the has_privilege
>> functions, for example; and if you think that isn't going to cause
>> problems I think you are wrong.  Perhaps we could just use "SET" and
>> "ALTER", or "SET" and "SYSTEM"?

> That's going to look weird, ISTM. This is less clear about what it's
> granting.
>      GRANT ALTER ON SOMETHING shared_buffers TO myuser;

True.  I think "GRANT SET" is clear enough, and it fits with the custom of
using the name of the SQL statement that the privilege allows you to
invoke.  (I gather from Mark's comments that Bison gave him problems with
that, but maybe that can be dealt with.)  But I concede that "ALTER" by
itself is pretty vague.

> If you don't like that maybe ALTER_SYSTEM and SET_VALUE would work,
> although mostly we have avoided things like that.
> How about MODIFY instead of SET VALUE and CONFIGURE instead of ALTER SYSTEM?

I thought about ALTER_SYSTEM too.  It's not great but maybe the best we
can do.  Not sure that CONFIGURE is better.

> Personally I don't have problem with the use of SETTING. I think the
> meaning is pretty plain in context and unlikely to produce any confusion.

I'm just unhappy about the disconnect with the documentation.  I wonder
if we could get away with s/configuration parameter/setting/g in the docs.

			regards, tom lane

  61. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-16T21:09:35Z

    On 3/16/22 16:53, Tom Lane wrote:

>> Personally I don't have problem with the use of SETTING. I think the
>> meaning is pretty plain in context and unlikely to produce any confusion.
> I'm just unhappy about the disconnect with the documentation.  I wonder
> if we could get away with s/configuration parameter/setting/g in the docs.
>
> 			


I don't think we need to fix that here though. If you can live with
SETTING for now I will undertake to fix the docs post-feature-freeze if
necessary.


cheers


andrew


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

  62. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-17T13:24:54Z

    <snip>

> I remain of the opinion that this
> patch should not concern itself with that, though.

So you are saying that people can add new object types to PG with DAC
permissions and not concern themselves with MAC capable hooks? Is that
an official PG community stance?

  63. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-17T13:52:34Z

    On 16.03.22 19:59, Mark Dilger wrote:
> Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the two obvious choices.  I preferred "configuration parameter" originally and was argued out of it.  My take on "setting" was also that it more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter "work_mem" has the setting "8192".

"The current setting of the work_mem parameter is 8192."

I think something based on "parameter" is good.  We also use that 
language in the protocol (e.g., ParameterStatus).

  64. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-17T13:54:57Z

    On 16.03.22 19:47, Tom Lane wrote:
> I'm also fairly allergic to the way that this patch has decided to assign
> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
> is no existing precedent for that, and I think it's going to break
> client-side code that we don't need to break.  It's not coincidental that
> this forces weird changes in rules about whitespace in the has_privilege
> functions, for example; and if you think that isn't going to cause
> problems I think you are wrong.  Perhaps we could just use "SET" and
> "ALTER", or "SET" and "SYSTEM"?

I think Oracle and MS SQL Server have many multi-word privilege names. 
So users are quite used to that.  And if we want to add more complex 
privileges, we might run out of sensible single words eventually.  So I 
would not exclude this approach.

  65. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-17T14:47:03Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 16.03.22 19:47, Tom Lane wrote:
>> ...  Perhaps we could just use "SET" and
>> "ALTER", or "SET" and "SYSTEM"?

> I think Oracle and MS SQL Server have many multi-word privilege names. 
> So users are quite used to that.  And if we want to add more complex 
> privileges, we might run out of sensible single words eventually.  So I 
> would not exclude this approach.

Well, I still say that "SET" is sufficient for the one privilege name
(unless we really can't make Bison handle that, which I doubt).  But
I'm willing to yield on using "ALTER SYSTEM" for the other.

If we go with s/SETTING/PARAMETER/ as per your other message, then
that would be adequately consistent with the docs I think.  So it'd
be

GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...

and the new catalog would be pg_parameter_acl, and so on.

			regards, tom lane

  66. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-17T15:41:38Z

    On 3/17/22 10:47, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 16.03.22 19:47, Tom Lane wrote:
>>> ...  Perhaps we could just use "SET" and
>>> "ALTER", or "SET" and "SYSTEM"?
>> I think Oracle and MS SQL Server have many multi-word privilege names. 
>> So users are quite used to that.  And if we want to add more complex 
>> privileges, we might run out of sensible single words eventually.  So I 
>> would not exclude this approach.
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt).  But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
>
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think.  So it'd
> be
>
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
>
> and the new catalog would be pg_parameter_acl, and so on.
>
> 			



The upside of this is that it avoids the inelegant


    GRANT SET ON SETTING ...


But I was just looking again at the grammar, and the only reason we need
this keyword at all AFAICS is to disambiguate ALL [PRIVILEGES] cases. 
If we abandoned that for this form of GRANT/REVOKE I think we could
probably get away with


    GRANT { SET | ALTER SYSTEM } ON setting_name ...


I haven't tried it, so I could be all wrong.


cheers


andrew

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

  67. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2022-03-17T16:04:42Z

    On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
> <snip>
>
> > I remain of the opinion that this
> > patch should not concern itself with that, though.
>
> So you are saying that people can add new object types to PG with DAC
> permissions and not concern themselves with MAC capable hooks? Is that
> an official PG community stance?

I don't know that the community has an official position on that
topic, but I do not think it's reasonable to expect everyone who
tinkers with MAC permissions to try to make a corresponding equivalent
for DAC. The number of people using PostgreSQL with DAC is relatively
small, and the topic is extremely complicated, and a lot of hackers
don't really understand it well enough to be sure that whatever they
might do is right. I think it's reasonable to expect people who
understand DAC and care about it to put some energy into the topic,
and not just in terms of telling other people how they have to write
their patches.

I *don't* think it's appropriate for a patch that touches MAC to
deliberately sabotage the existing support we have for DAC or to just
ignore it where the right thing to do is obvious. But maintaining a
million lines of code is a lot of work, and I can't think of any
reason why the burden of maintaining relatively little-used features
should fall entirely on people who don't care about them.

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

  68. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2022-03-17T16:18:51Z

    On Wed, Mar 16, 2022 at 2:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables?  I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters".  Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning.  But you can't
> GRANT a parameter's current value.

I agree that the lack of a good user-friendly term for GUCs is a real
problem. Here at EDB I've observed even relatively non-technical
people using that term, which appears nowhere in the documentation and
is utterly unintelligible to a typical end-user. Somebody gets on the
phone and tells the customer that they need to set a GUC and the
customer is like "what's a guck?" except that they probably don't
actually ask that question but are just confused and fail to
understand that a postgresql.conf change is being proposed. I hate it.
It sucks.

I have sort of been trying to promote the use of the word "setting"
and use it in my own writing, especially to end-users. That is
definitely more intelligible to random users, but it's admittedly also
awkward. "Set a setting" just sounds redundant. But "set a
configuration variable" sounds wordy, so I don't know.

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

  69. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-17T16:19:24Z

    
> On Mar 17, 2022, at 9:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> not just in terms of telling other people how they have to write
> their patches.

...

>  the burden of maintaining relatively little-used features
> should fall entirely on people who don't care about them.

Joshua helped test the DAC portion of this patch, and answered a number of my questions on the topic, including in-person back in December.  I take your point, Robert, on the general principle, but the archives should reflect that Joshua did contribute to this specific patch.

Joshua, should we drop the DAC portion for postgres 15 and revisit the issue for postgres 16?  I think it's getting late in the development cycle to attempt what Tom described upthread, and I'd hate to see the rest of this patch punted.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  70. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2022-03-17T16:29:11Z

    On Thu, Mar 17, 2022 at 12:19 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Joshua helped test the DAC portion of this patch, and answered a number of my questions on the topic, including in-person back in December.  I take your point, Robert, on the general principle, but the archives should reflect that Joshua did contribute to this specific patch.

I wasn't intending to say otherwise, and if the changes needed for DAC
here are straightforward and don't make the patch significantly harder
to finish, then I would say Tom is wrong and we should just go ahead
and make them. But if they do, then I think it's perfectly fine to say
that we're going to leave that alone and let someone with
subject-matter expertise sort it out when they have time. We do that
all the time with other things, most notably MSVC builds, where we
just can't expect every hacker or even every committer to understand
exactly what's required to make it all work. I try my best to commit
things that don't break it, but sometimes I do, and then Andrew helps
sort it out, because he understands it and I don't. I think DAC should
fall into that kind of category as well.

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

  71. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-17T16:29:53Z

    On Thu, Mar 17, 2022 at 12:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle
> <joshua.brindle@crunchydata.com> wrote:
> > <snip>
> >
> > > I remain of the opinion that this
> > > patch should not concern itself with that, though.
> >
> > So you are saying that people can add new object types to PG with DAC
> > permissions and not concern themselves with MAC capable hooks? Is that
> > an official PG community stance?
>
> I don't know that the community has an official position on that
> topic, but I do not think it's reasonable to expect everyone who
> tinkers with MAC permissions to try to make a corresponding equivalent
> for DAC. The number of people using PostgreSQL with DAC is relatively
> small, and the topic is extremely complicated, and a lot of hackers
> don't really understand it well enough to be sure that whatever they
> might do is right. I think it's reasonable to expect people who
> understand DAC and care about it to put some energy into the topic,
> and not just in terms of telling other people how they have to write
> their patches.
>
> I *don't* think it's appropriate for a patch that touches MAC to
> deliberately sabotage the existing support we have for DAC or to just
> ignore it where the right thing to do is obvious. But maintaining a
> million lines of code is a lot of work, and I can't think of any
> reason why the burden of maintaining relatively little-used features
> should fall entirely on people who don't care about them.

I agree with this view, outside of the mixup between MAC and DAC (DAC
is in-core, MAC is via hooks)

So there should be some committers or contributors that keep an eye
out and make sure new objects have appropriate hooks, and since an Oid
based hook for GUCs is inappropriate, I hope this patch can be rolled
into the contribution from Mark.

The attached patch adds a set of hooks for strings, and changes the
GUC patch from Mark to use it, I tested with this code:

void set_user_object_access_str(ObjectAccessType access, Oid classId,
const char *objName, int subId, void *arg)
{
if (next_object_access_hook_str)
{
(*next_object_access_hook_str)(access, classId, objName, subId, arg);
}
switch (access)
{
case OAT_POST_ALTER:
if (classId == SettingAclRelationId)
{
Oid userid = GetUserId();
bool is_superuser = superuser_arg(userid);
char *username = GETUSERNAMEFROMID(GetUserId());
const char *setting = "setting value";
const char *altering = "altering system";
const char *unknown = "unknown";
const char *action;
if (subId && ACL_SET_VALUE)
action = setting;
else if (subId && ACL_ALTER_SYSTEM)
action = altering;
else
action = unknown;

elog(WARNING, "%s is %s %s (%d)", username, action, objName, is_superuser);
if (strcmp(objName, "log_statement") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Setting %s blocked", objName)));
}
}
default:
break;
}
}

static void
set_user_object_access (ObjectAccessType access, Oid classId, Oid
objectId, int subId, void *arg)
{
if (next_object_access_hook)
{
(*next_object_access_hook)(access, classId, objectId, subId, arg);
}
switch (access)
{
case OAT_FUNCTION_EXECUTE:
{
/* Update the `set_config` Oid cache if necessary. */
set_user_cache_proc(InvalidOid);

/* Now see if this function is blocked */
set_user_block_set_config(objectId);
break;
}

/* fallthrough */

case OAT_POST_CREATE:
{
if (classId == ProcedureRelationId)
{
set_user_cache_proc(objectId);
}
break;
}
default:
break;
}
}

  72. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Robert Haas <robertmhaas@gmail.com> — 2022-03-17T16:34:25Z

    On Thu, Mar 17, 2022 at 12:30 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
> I agree with this view, outside of the mixup between MAC and DAC (DAC
> is in-core, MAC is via hooks)

An excellent point! Exactly why we need expert-level help with this stuff! :-)

> So there should be some committers or contributors that keep an eye
> out and make sure new objects have appropriate hooks, and since an Oid
> based hook for GUCs is inappropriate, I hope this patch can be rolled
> into the contribution from Mark.

I'm not going to take a position on this patch right this minute, but
I appreciate you providing it.

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

  73. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-17T16:36:49Z

    
> On Mar 17, 2022, at 9:29 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> 
> I hope this patch can be rolled
> into the contribution from Mark.

Working on it.... Thanks for the patch!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  74. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-17T18:19:48Z

    On Thu, Mar 17, 2022 at 12:36 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On Mar 17, 2022, at 9:29 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> >
> > I hope this patch can be rolled
> > into the contribution from Mark.
>
> Working on it.... Thanks for the patch!

Great, thanks.

I missed one objectId reference (InvokeObjectDropHookStr), fixed
version attached.

  75. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-24T16:49:24Z

    
> On Mar 17, 2022, at 8:41 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> If we abandoned that for this form of GRANT/REVOKE I think we could
> probably get away with
> 
> 
>     GRANT { SET | ALTER SYSTEM } ON setting_name ...
> 
> 
> I haven't tried it, so I could be all wrong.

Version 12 of the patch uses SET and ALTER SYSTEM as the names of the privileges, and PARAMETER as the name of the thing on which the privilege is granted.  The catalog table which tracks these grants is now named pg_parameter_acl, and various other parts of the patch have been adjusted to use a "parameter" based, rather than a "setting" based, naming scheme.  One exception to this rule is the "setacl" column in pg_parameter_acl, which is much more compact than the "parameteracl" name would be, so that remains under the old name.

The tab completion for grants and revokes of "set" and "alter system" on parameters has been extended compared to the previously submitted patch.  The tab completion for grants and revokes of other privilege types remains fairly thin, as I resisted the urge to fix that generally, given that it is unrelated to this patch.  However, in making tab completion for grant/revoke on parameters work, this patch incidentally improves the coverage for other privilege types, in so far as the prefixes for those commands match the prefixes of these new commands. 

Version 12 also introduces \dcp (pneumonic, "Describe Configuration Parameter") for listing parameters, with \dcp+ also showing the acl, like:

mark.dilger=# \dcp+ %client%
                               List of configuration parameters
            Parameter             | Context | Setting | Unit |       Access privileges        
----------------------------------+---------+---------+------+--------------------------------
 client_connection_check_interval | user    | 0       | ms   | 
 client_encoding                  | user    | UTF8    |      | "mark.dilger"=sA/"mark.dilger"
 client_min_messages              | user    | notice  |      | "mark.dilger"=sA/"mark.dilger"+
                                  |         |         |      | =s/"mark.dilger"              +
                                  |         |         |      | alice=A*/"mark.dilger"
(3 rows)

The "Access privileges" column is blank for parameters which have neither been granted nor revoked.  In the example above, that's true for client_connection_check_interval.

\dcp[+] only shows "user" and "superuser" parameters:

mark.dilger=# \dcp %wal%
   List of configuration parameters
        Parameter         |  Context  
--------------------------+-----------
 track_wal_io_timing      | superuser
 wal_compression          | superuser
 wal_consistency_checking | superuser
 wal_init_zero            | superuser
 wal_recycle              | superuser
 wal_sender_timeout       | user
 wal_skip_threshold       | user
(7 rows)

whereas \dcpS[+] also shows all parameters (so, also "sighup", "backend", "superuser-backend", "postmaster", and "internal".)

mark.dilger=# \dcpS %wal%
      List of configuration parameters
           Parameter           |  Context   
-------------------------------+------------
 max_slot_wal_keep_size        | sighup
 max_wal_senders               | postmaster
 max_wal_size                  | sighup
 min_wal_size                  | sighup
 track_wal_io_timing           | superuser
 wal_block_size                | internal
 wal_buffers                   | postmaster
 wal_compression               | superuser
 wal_consistency_checking      | superuser
 wal_init_zero                 | superuser
 wal_keep_size                 | sighup
 wal_level                     | postmaster
 wal_log_hints                 | postmaster
 wal_receiver_create_temp_slot | sighup
 wal_receiver_status_interval  | sighup
 wal_receiver_timeout          | sighup
 wal_recycle                   | superuser
 wal_retrieve_retry_interval   | sighup
 wal_segment_size              | internal
 wal_sender_timeout            | user
 wal_skip_threshold            | user
 wal_sync_method               | sighup
 wal_writer_delay              | sighup
 wal_writer_flush_after        | sighup
(24 rows)

  76. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-24T19:06:21Z

    On 3/24/22 12:49, Mark Dilger wrote:
>
>> On Mar 17, 2022, at 8:41 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> If we abandoned that for this form of GRANT/REVOKE I think we could
>> probably get away with
>>
>>
>>     GRANT { SET | ALTER SYSTEM } ON setting_name ...
>>
>>
>> I haven't tried it, so I could be all wrong.
> Version 12 of the patch uses SET and ALTER SYSTEM as the names of the privileges, and PARAMETER as the name of the thing on which the privilege is granted.  The catalog table which tracks these grants is now named pg_parameter_acl, and various other parts of the patch have been adjusted to use a "parameter" based, rather than a "setting" based, naming scheme.  One exception to this rule is the "setacl" column in pg_parameter_acl, which is much more compact than the "parameteracl" name would be, so that remains under the old name.


I can live with it I guess, but it seems perverse to me to have
pg_settings but pg_paramater_acl effectively referring to the same set
of things. If we're going to do this perhaps we should create a
pg_parameters view which is identical to pg_settings and deprecate
pg_settings. I don;t want to hold up this patch, I think this can
probably be managed as a follow up item.


cheers


andrew


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

  77. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-24T19:50:42Z

    
> On Mar 24, 2022, at 12:06 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> 
> On 3/24/22 12:49, Mark Dilger wrote:
>> 
>>> On Mar 17, 2022, at 8:41 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> 
>>> If we abandoned that for this form of GRANT/REVOKE I think we could
>>> probably get away with
>>> 
>>> 
>>>    GRANT { SET | ALTER SYSTEM } ON setting_name ...
>>> 
>>> 
>>> I haven't tried it, so I could be all wrong.
>> Version 12 of the patch uses SET and ALTER SYSTEM as the names of the privileges, and PARAMETER as the name of the thing on which the privilege is granted.  The catalog table which tracks these grants is now named pg_parameter_acl, and various other parts of the patch have been adjusted to use a "parameter" based, rather than a "setting" based, naming scheme.  One exception to this rule is the "setacl" column in pg_parameter_acl, which is much more compact than the "parameteracl" name would be, so that remains under the old name.
> 
> 
> I can live with it I guess, but it seems perverse to me to have
> pg_settings but pg_paramater_acl effectively referring to the same set
> of things. If we're going to do this perhaps we should create a
> pg_parameters view which is identical to pg_settings and deprecate
> pg_settings. I don;t want to hold up this patch, I think this can
> probably be managed as a follow up item.

Right, the version 12 patch was following Peter's and Tom's comments upthread:

> On Mar 17, 2022, at 7:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt).  But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
> 
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think.  So it'd
> be
> 
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
> 
> and the new catalog would be pg_parameter_acl, and so on.

We could debate that again, but it seems awfully late in the development cycle.  I'd rather just get this committed, barring any objections?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  78. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T18:31:47Z

    I've started reviewing this patch in earnest, and almost immediately
hit a serious problem: these regression tests aren't even a little bit
committable.  For one thing, they fail if you do "make installcheck"
twice in a row.  This seems to be because the first run leaves some
cruft behind in pg_parameter_acl that affects the results of
subsequent runs.  But the bigger problem is that it is ABSOLUTELY NOT
OKAY to test ALTER SYSTEM during an "installcheck" run.  That would be
true even if you cleaned up all the settings by the end of the run, as
the patch fails to do (and for extra demerit, it leaves a superuser
role laying around).  Even transient effects on the behavior of
sessions in other DBs aren't acceptable in installcheck mode, IMO.

I think we probably have to trash the core-regression-tests part of
the patch altogether and instead use a TAP test for whatever testing
we want to do.  It might be all right to test SET privileges without
testing ALTER SYSTEM, but I'm not sure there's much point in that.

			regards, tom lane

  79. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-28T18:42:42Z

    
> On Mar 28, 2022, at 11:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I think we probably have to trash the core-regression-tests part of
> the patch altogether and instead use a TAP test for whatever testing
> we want to do.  It might be all right to test SET privileges without
> testing ALTER SYSTEM, but I'm not sure there's much point in that.

How about putting them under src/test/modules/unsafe_tests ?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  80. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T19:11:15Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> Version 12 also introduces \dcp (pneumonic, "Describe Configuration Parameter") for listing parameters, with \dcp+ also showing the acl, like:

The fact that that code is not dry behind the ears is painfully obvious.
It's not documented in psql-ref, not tested anywhere AFAICS, and its
handling of the pattern parameter is inconsistent with every other
\d command.  The wildcard character should be * not %.  It only
accidentally fails to dump core if no pattern is given, too.

> \dcp[+] only shows "user" and "superuser" parameters:

Why make that restriction?  Also, I find it astonishing that this doesn't
show the GUC's value by default.  The non-plus form of the command seems
useless as it stands, or at least its intended use-case is so narrow
I can't see it.  If we're to have it at all, it seems like it ought to
be a reasonably useful shortcut for interrogating pg_settings.  I'd
expect the base set of columns to be name, value, and possibly unit;
then add ACL with +.  I'm not sure that GucContext belongs in this at all,
but if it does, it's a + column.

On the whole perhaps this should be taken out again; it's a bit
late in the cycle to be introducing new features, especially ones
as subject to bikeshedding as a \d command is.  My ideas about what
columns to show probably wouldn't match anyone else's ... and we
haven't even gotten to whether \dcp is an okay choice of name.

			regards, tom lane

  81. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T19:14:04Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Mar 28, 2022, at 11:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think we probably have to trash the core-regression-tests part of
>> the patch altogether and instead use a TAP test for whatever testing
>> we want to do.  It might be all right to test SET privileges without
>> testing ALTER SYSTEM, but I'm not sure there's much point in that.

> How about putting them under src/test/modules/unsafe_tests ?

Ah, that could work; I'd forgotten about that subdirectory.

It'd still be a good idea if they didn't fail when run twice in a row.

			regards, tom lane

  82. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-28T20:57:50Z

    
> On Mar 28, 2022, at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> Version 12 also introduces \dcp (pneumonic, "Describe Configuration Parameter") for listing parameters, with \dcp+ also showing the acl, like:
> 
> The fact that that code is not dry behind the ears is painfully obvious.

<snip>

> On the whole perhaps this should be taken out again

I added the \dcp command after noticing that doc/src/sgml/ddl.sgml's "Summary of Access Privileges" table had an empty column for the "psql Command" column for the "PARAMETER" row.  Maybe that's ok.  I thought it would be better if I added a command for that.

I don't feel too strongly about it, so I'm removing \dcp from this patch with the intention of introducing it as a separate patch for the postgres 16 development cycle.  For now, I'm just putting "none" in that table cell.


This patch also moves the grant/revoke tests in guc_privs.sql into the unsafe_tests directory, fixes them up to be repeatable, and drops the superuser role that had been negligently left around in prior patches.  The tests that had been added in privileges.sql are simply removed.

  83. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T21:16:14Z

    I just came across something odd in v12 that is still there in v13:
ExecGrant_Parameter uses InvokeObjectPostAlterHook not
InvokeObjectPostAlterHookArgStr.  This seems pretty inconsistent.
Is there a good argument for it?

... or, for that matter, why is there any such call at all?
No other GRANT/REVOKE operation calls such a hook.

			regards, tom lane

  84. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-28T21:42:46Z

    
> On Mar 28, 2022, at 2:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I just came across something odd in v12 that is still there in v13:
> ExecGrant_Parameter uses InvokeObjectPostAlterHook not
> InvokeObjectPostAlterHookArgStr.  This seems pretty inconsistent.
> Is there a good argument for it?
> 

For SET and ALTER SYSTEM, the target of the action may not have an entry in pg_parameter_acl, nor an assigned Oid anywhere, so the only consistent way to pass the argument to the hook is by name.  For GRANT/REVOKE, the parameter must have an Oid, at least by the time the hook gets called.  Upthread there was some discussion of a hook not being able to assume a snapshot and working transaction, and hence not being able to query the catalogs.  I would think that in a GRANT or REVOKE that hasn't already errored, the hook would have a transaction and could look up whatever it likes?  There is a CommandCounterIncrement() call issued in objectNamesToOids() for new parameters, so by the time the hook is running it should be able to see the parameter.

Am I reasoning about this the wrong way?

> ... or, for that matter, why is there any such call at all?
> No other GRANT/REVOKE operation calls such a hook.

I think ALTER DEFAULT PRIVILEGES does, though that's not quite the same thing.  I don't have a strong opinion on this.  Joshua, what's your take?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  85. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T21:54:28Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Mar 28, 2022, at 2:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I just came across something odd in v12 that is still there in v13:
>> ExecGrant_Parameter uses InvokeObjectPostAlterHook not
>> InvokeObjectPostAlterHookArgStr.  This seems pretty inconsistent.
>> Is there a good argument for it?

> For SET and ALTER SYSTEM, the target of the action may not have an entry
> in pg_parameter_acl, nor an assigned Oid anywhere, so the only
> consistent way to pass the argument to the hook is by name.  For
> GRANT/REVOKE, the parameter must have an Oid, at least by the time the
> hook gets called.

Yeah, I know it's *possible* to make this work.  The question is why is
it good to do it like this rather than to use the string API, now that
we have the latter.  AFAICS this way just guarantees that the hook must
do a catalog lookup in order to figure out what you're talking about.

The core point here is that the actual identity of a GUC is its name.
Any OID that may exist in pg_parameter_acl is just a nonce alias that
means nothing to anybody.  Anyone who's trying to, say, enforce that
Joe Blow can't change shared_buffers is going to need to see the GUC
name.  (I am, btw, busy doing a lot of renaming in the patch to try
to clarify that these OIDs are not identifiers for GUCs; imagining
that they are just risks confusion.)

			regards, tom lane

  86. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-28T22:04:12Z

    
> On Mar 28, 2022, at 2:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Yeah, I know it's *possible* to make this work.  The question is why is
> it good to do it like this rather than to use the string API, now that
> we have the latter.  AFAICS this way just guarantees that the hook must
> do a catalog lookup in order to figure out what you're talking about.

Ok, thanks for clarifying.  I took the *HookStr versions of the hooks to be an alternative to be used when no Oid was present, something of a last resort.  I never thought much about using them under other circumstances.

> The core point here is that the actual identity of a GUC is its name.
> Any OID that may exist in pg_parameter_acl is just a nonce alias that
> means nothing to anybody.  Anyone who's trying to, say, enforce that
> Joe Blow can't change shared_buffers is going to need to see the GUC
> name.  (I am, btw, busy doing a lot of renaming in the patch to try
> to clarify that these OIDs are not identifiers for GUCs; imagining
> that they are just risks confusion.)

I was about to write another patch using the HookStr form, but if you are already editing, then I'll let you make the change.  I don't see a problem with what you are proposing.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  87. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T22:31:02Z

    I'm going to be mostly unavailable till Wednesday, but I'll leave
you with another thing to chew on:

regression=# create user joe;
CREATE ROLE
regression=# grant set on parameter plpgsql.extra_warnings to joe;
ERROR:  unrecognized configuration parameter "plpgsql.extra_warnings"

This is problematic, because once plpgsql is loaded it works:

regression=# load 'plpgsql';                        
LOAD
regression=# grant set on parameter plpgsql.extra_warnings to joe;
GRANT

If we then do

$ pg_dumpall -g

it falls over:
pg_dumpall: error: query failed: ERROR:  unrecognized configuration parameter "plpgsql.extra_warnings"

apparently because aclparameterdefault() is allergic to being asked about
unknown GUCs, and plpgsql is not loaded in pg_dumpall's session.  But
if pg_dumpall hadn't failed, it'd produce a dump script containing that
same command, which would fail at load time (because, again, plpgsql
isn't going to be loaded in the backend reading the restore script).

This is what I meant by saying that you can't just refuse to GRANT on
unknown GUCs.  It makes custom GUCs into a time bomb for dump/restore.
And that means you need a strategy for dealing with the possibility
that you don't know whether the GUC is USERSET or not.  I think though
that it might work to just assume that it isn't, in which case dumps
on unrecognized GUCs that really are USERSET will end up issuing an
explicit GRANT SET TO PUBLIC that we didn't actually need to do, but it
won't hurt anything.  (Testing that assertion would be a good thing
to do.)

			regards, tom lane

  88. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T22:37:07Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> I was about to write another patch using the HookStr form, but if you are already editing, then I'll let you make the change.  I don't see a problem with what you are proposing.

Don't sweat about that, I can easily rebase what I've done so far
over your updates.

			regards, tom lane

  89. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-30T04:00:36Z

    
> On Mar 28, 2022, at 3:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> This is what I meant by saying that you can't just refuse to GRANT on
> unknown GUCs.  It makes custom GUCs into a time bomb for dump/restore.
> And that means you need a strategy for dealing with the possibility
> that you don't know whether the GUC is USERSET or not.  I think though
> that it might work to just assume that it isn't, in which case dumps
> on unrecognized GUCs that really are USERSET will end up issuing an
> explicit GRANT SET TO PUBLIC that we didn't actually need to do, but it
> won't hurt anything.  (Testing that assertion would be a good thing
> to do.)

Ok, I returned to the idea upthread for a solution to this problem.  A grant or revoke on an unrecognized custom parameter will create a SUSET placeholder, which is not quite right in some cases.  However, the installation scripts for modules have been updated to manually grant SET privilege on their custom USERSET parameters, which cleans up the problem, with one exception:  if the user executes a "revoke set on parameter some.such from public" prior to loading the module which defines parameter some.such, that revoke won't be retained.  That doesn't seem entirely wrong to me, since no privilege to set the parameter existed when the revoke was performed, but rather was granted along with the creation of the parameter, but it also doesn't seem entirely right.  Maybe revoke commands (but not grant commands) should error on unrecognized custom parameters?  I didn't implement that here, but can do so if you think that makes more sense than this new behavior.
 
I changed add_placeholder_variable() to take a GucContext argument.  It previously always used PGC_USERSET, which is what all pre-existing call sites now pass into it, but that seems a bit inappropriate where we're creating a placeholder that we intend to treat as a SUSET variable until such time as a module gets installed saying otherwise.  Not changing add_placeholder_variable in this fashion seems to work just fine.  I just didn't feel comfortable with doing it that way.  But if you feel it generates needless code churn, I could be talked out of doing this.

I also changed the patch to use the ...HookStr functions for parameters.  I would really like a comment on this from Joshua, to be sure what I'm doing comports with what he wanted.  In particular, I'm uncertain that simply passing the AclMode (in other words, the istmt->privileges field) for the grant/revoke to the hook is sufficient.  For one, how does the hook want to distinguish grants from revokes?  Do we want a bit for that?  And what about distinguishing WITH GRANT OPTION?  I think the hooks are usable right now, but they might be made better.

  90. Re: Granting SET and ALTER SYSTE privileges for GUCs

    David G. Johnston <david.g.johnston@gmail.com> — 2022-03-30T05:27:44Z

    On Tue, Mar 29, 2022 at 9:00 PM Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

>  A grant or revoke on an unrecognized custom parameter will create a SUSET
> placeholder,

[...]

> which cleans up the problem, with one exception:  if the user executes a
> "revoke set on parameter some.such from public" prior to loading the module
> which defines parameter some.such, that revoke won't be retained.  That
> doesn't seem entirely wrong to me, since no privilege to set the parameter
> existed when the revoke was performed, but rather was granted along with
> the creation of the parameter, but it also doesn't seem entirely right.
> Maybe revoke commands (but not grant commands) should error on unrecognized
> custom parameters?


The only revoke role target that makes sense here is the default grant
given to public.  Aside from that technicality the grant system is purely
additive and so only the GRANTS end up retained so far as perpetual state
is concerned.

For a revoke that doesn't target public we should remove the corresponding
unrecognized setting grant if one is present.  We don't generally/always
raise a notice if a revoke doesn't actually cause something to be ungranted
-  though I'm partial to being so informed.  I suppose we could distinguish
the cases where the not-yet-loaded setting name is unrecognized by the
system from the one where it is recognized but the grant is actually on a
different role (or a revoke entry from public for the setting name is
present).

For a revoke of an unknown setting from public we should keep an entry
somewhere that tells the system that the default grant to public for that
setting has been revoked.  Maybe there isn't the same timing concern here
as there is for GRANT, but if only for symmetry it seems like a good thing
to implement.

I have the impression I'm missing something in what I wrote above but
cannot quite figure out what.  In any case as a first pass at this the
behavior described is kinda what I'm expecting.

David J.

P.S.

Skimming the patch we are, to my agreement, not touching the ALTER DEFAULT
PRIVILEGES command to work with this feature.  Should the omission be noted
explicitly?  At least in the commit message I would think.  Though the
sentence in [1] "Also, these default privilege settings can be overridden
using the ALTER DEFAULT PRIVILEGES command." is rendered only partially
correct.

[1] https://www.postgresql.org/docs/current/ddl-priv.html

P.P.S.

+   The default privileges for a <literal>user</literal> parameter allow
+   <literal>PUBLIC</literal> to <command>SET</command> and
+   <command>RESET</command> the assigned value.  By default,
+   <literal>PUBLIC</literal> has no privileges on
+   <literal>postmaster</literal>, <literal>superuser-backend</literal>,
+   <literal>internal</literal>, <literal>backend</literal>,
+   <literal>sighup</literal>, and <literal>superuser</literal> parameters.

Can we rephrase this to something like:

By default, PUBLIC has no privileges on parameters in the postmaster, ...,
and superuser contexts.

pg_settings.context exists and those are the values found there.  My
initial interpretation of the wording was the postmaster, etc..., were
themselves parameters, not containers for many parameters.

  91. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-30T12:30:42Z

    On Wed, Mar 30, 2022 at 12:00 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
> > On Mar 28, 2022, at 3:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > This is what I meant by saying that you can't just refuse to GRANT on
> > unknown GUCs.  It makes custom GUCs into a time bomb for dump/restore.
> > And that means you need a strategy for dealing with the possibility
> > that you don't know whether the GUC is USERSET or not.  I think though
> > that it might work to just assume that it isn't, in which case dumps
> > on unrecognized GUCs that really are USERSET will end up issuing an
> > explicit GRANT SET TO PUBLIC that we didn't actually need to do, but it
> > won't hurt anything.  (Testing that assertion would be a good thing
> > to do.)
>
> Ok, I returned to the idea upthread for a solution to this problem.  A grant or revoke on an unrecognized custom parameter will create a SUSET placeholder, which is not quite right in some cases.  However, the installation scripts for modules have been updated to manually grant SET privilege on their custom USERSET parameters, which cleans up the problem, with one exception:  if the user executes a "revoke set on parameter some.such from public" prior to loading the module which defines parameter some.such, that revoke won't be retained.  That doesn't seem entirely wrong to me, since no privilege to set the parameter existed when the revoke was performed, but rather was granted along with the creation of the parameter, but it also doesn't seem entirely right.  Maybe revoke commands (but not grant commands) should error on unrecognized custom parameters?  I didn't implement that here, but can do so if you think that makes more sense than this new behavior.
>
> I changed add_placeholder_variable() to take a GucContext argument.  It previously always used PGC_USERSET, which is what all pre-existing call sites now pass into it, but that seems a bit inappropriate where we're creating a placeholder that we intend to treat as a SUSET variable until such time as a module gets installed saying otherwise.  Not changing add_placeholder_variable in this fashion seems to work just fine.  I just didn't feel comfortable with doing it that way.  But if you feel it generates needless code churn, I could be talked out of doing this.
>
> I also changed the patch to use the ...HookStr functions for parameters.  I would really like a comment on this from Joshua, to be sure what I'm doing comports with what he wanted.  In particular, I'm uncertain that simply passing the AclMode (in other words, the istmt->privileges field) for the grant/revoke to the hook is sufficient.  For one, how does the hook want to distinguish grants from revokes?  Do we want a bit for that?  And what about distinguishing WITH GRANT OPTION?  I think the hooks are usable right now, but they might be made better.

I had not even been thinking about hooking grant/revoke TBH. In
SELinux-type MAC systems we don't always try to control DAC permission
changes, and when we do it's not granular, all permission changes just
boil down to setattr permission or something of that nature.

Thank you.

  92. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-30T13:26:21Z

    After sleeping on it, I have a modest proposal for simplifying
these issues.  Consider this design:

1. In the SET code path, we assume (without any catalog lookup)
that USERSET GUCs can be set.  Only for SUSET GUCs do we perform
a permissions lookup.  (ALTER SYSTEM does a lookup in both cases.)

2. Given this, the default ACL for any GUC can be empty, greatly
simplifying all these management issues.  Superusers could do what
they want anyway, so modeling an "owner's default grant" becomes
unnecessary.

What this loses is the ability to revoke public SET permissions
on USERSET GUCs.  I claim that that is not so valuable as to
justify all the complication needed to deal with it.  (If a GUC
seems to require some defenses, why is it USERSET?)  Avoiding
a permissions lookup in the default SET code path seems like
a pretty important benefit, too.  If we force that to happen
it's going to be a noticeable drag on functions with SET clauses.

			regards, tom lane

  93. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-30T13:59:48Z

    
> On Mar 30, 2022, at 6:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Consider this design:

Isn't that just the design I had implemented in v8 several months ago?

Subject: [PATCH v8] Allow GRANT of SET and ALTER SYSTEM SET for gucs

Allow granting of privilege to set or alter system set variables
which otherwise can only be managed by superusers.  Each
(role,variable,privilege) triple is independently grantable, so a
user may be granted privilege to SET but not to ALTER SYSTEM SET on
a variable, or vice versa.  The privilege to SET a userset variable
may be granted, though doing so has no practical effect, since any
role can set userset variables anyway.  Worse, there is no way to
revoke the privilege to SET a userset variable.  To remedy that,
most core userset variables have been changed to suset, with
explicit grants to set the variable to public.


I don't think v9 ever got posted to the list, but v10 has:

Subject: [PATCH v10] Allow grant and revoke of privileges on settings

Allow grant and revoke of privileges to set or alter system set
configuration variables.  Each (role,variable,privilege) triple can
be independently granted or revoked, so a user may be granted
privilege to SET but not to ALTER SYSTEM SET on a variable, or vice
versa.  Privilege to SET a userset variable is implicitly granted to
public, but may be revoked.


If we want to backtrack to v8, that's fine.  I can rebase that, port some of the other changes from v14 to it, and repost it as v15.  We should review the conversation from December and January which included some arguments for allowing revokes of SET on USERSET from PUBLIC.  I don't want to keep going around in circles on this.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  94. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-30T14:45:36Z

    
> On Mar 30, 2022, at 6:59 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> We should review the conversation from December and January which included some arguments for allowing revokes of SET on USERSET from PUBLIC.  I don't want to keep going around in circles on this.

Hmm, I guess that conversation was mostly off-list at the PGConn in December.  I made a reference to it upthread:

> On Mar 6, 2022, at 2:40 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> Userset variables are implicitly settable by any user.  There was a request, off-list as I recall, to make it possible to revoke the privilege to set variables such as "work_mem".  To make that possible, but not change the default behavior vis-a-vis prior releases, I upgraded most userset variables to suset with a corresponding grant to public on the variable.  Sites which wish to have a more restrictive policy on such variables can revoke that privilege from public and instead issue more restrictive grants.  There were a few variables where such treatment didn't seem sensible, such as ones to do with client connections, and I left them alone.  I didn't insist on a defense for why any particular setting needed to be revocable in order to apply this treatment.  My assumption was that sites should be allowed to determine their own security policies per setting unless there is a technical difficulty for a given setting that would make it overly burdensome to implement.

Your proposal to just punt on supporting revocation of set on userset from public seems fine.  We could revisit that in the next development cycle if anyone really wants to defend it.  In particular, I don't see that committing this feature without that part would create any additional backward compatibility problems when implementing that later.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  95. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-30T15:12:46Z

    On 3/30/22 09:26, Tom Lane wrote:
> After sleeping on it, I have a modest proposal for simplifying
> these issues.  Consider this design:
>
> 1. In the SET code path, we assume (without any catalog lookup)
> that USERSET GUCs can be set.  Only for SUSET GUCs do we perform
> a permissions lookup.  (ALTER SYSTEM does a lookup in both cases.)
>
> 2. Given this, the default ACL for any GUC can be empty, greatly
> simplifying all these management issues.  Superusers could do what
> they want anyway, so modeling an "owner's default grant" becomes
> unnecessary.
>
> What this loses is the ability to revoke public SET permissions
> on USERSET GUCs.  I claim that that is not so valuable as to
> justify all the complication needed to deal with it.  (If a GUC
> seems to require some defenses, why is it USERSET?)  Avoiding
> a permissions lookup in the default SET code path seems like
> a pretty important benefit, too.  If we force that to happen
> it's going to be a noticeable drag on functions with SET clauses.
>
> 			



The last point is telling, so +1


cheers


andrew


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

  96. Re: Granting SET and ALTER SYSTE privileges for GUCs

    David G. Johnston <david.g.johnston@gmail.com> — 2022-03-30T15:44:53Z

    On Wed, Mar 30, 2022 at 8:12 AM Andrew Dunstan <andrew@dunslane.net> wrote:

>
> On 3/30/22 09:26, Tom Lane wrote:
>
> >
> > What this loses is the ability to revoke public SET permissions
> > on USERSET GUCs.  I claim that that is not so valuable as to
> > justify all the complication needed to deal with it.


Agreed, and in line with my thinking from last night.  These default public
set grants are indeed the complication and I'm good with the status quo
where they are non-revocable.

I'm finding it curious that we are choosing to document every (all 6)
context that doesn't have this default privilege instead of saying that
only the user context variables are granted this default, and now
irrevocable, default set privilege.  This is in addition to making sure we
distinguish between parameter and context in my earlier email.


> > Avoiding
> > a permissions lookup in the default SET code path seems like
> > a pretty important benefit, too.  If we force that to happen
> > it's going to be a noticeable drag on functions with SET clauses.
> >
> >
> The last point is telling, so +1
>
>
Indeed. +1

David J.

  97. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-30T15:45:53Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> Your proposal to just punt on supporting revocation of set on userset from public seems fine.  We could revisit that in the next development cycle if anyone really wants to defend it.  In particular, I don't see that committing this feature without that part would create any additional backward compatibility problems when implementing that later.

Yeah.  Also, as you noted, we could mark some individual built-in
variables as SUSET and add a default GRANT.  I don't want to do that with
a blunderbuss, but perhaps there's an argument to do it for specific
cases (search_path comes to mind, though the performance cost could be
significant, since I think setting that in function SET clauses is
common).

For now, though, saying that you can't restrict SET for USERSET variables
seems fine --- there's certainly no loss of capability compared to where
we stand today.  I'd prefer to get the feature committed in that form
and then look at whether we want to tighten things around the margins.

			regards, tom lane

  98. Re: Granting SET and ALTER SYSTE privileges for GUCs

    David G. Johnston <david.g.johnston@gmail.com> — 2022-03-30T15:51:05Z

    On Wed, Mar 30, 2022 at 8:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I don't want to do that with
> a blunderbuss, but perhaps there's an argument to do it for specific
> cases (search_path comes to mind, though the performance cost could be
> significant, since I think setting that in function SET clauses is
> common).
>


I suspect it became considerably moreso when we fixed the search_path CVE
since we basically told people that doing so, despite the possible
performance hit, was the easiest solution to their immediate dump/restore
failures.  But ISTM that because that SET has a function invocation context
it could bypass any such check.  Though maybe the DO command exposes a flaw
in that idea.
David J.

  99. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T15:36:18Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> If we want to backtrack to v8, that's fine.  I can rebase that, port
> some of the other changes from v14 to it, and repost it as v15.

Are you working on that?  I've set aside time this week to hopefully
get this over the finish line, but I don't want to find out that
I've been duplicating your effort.

			regards, tom lane

  100. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T15:47:46Z

    
> On Apr 4, 2022, at 8:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> If we want to backtrack to v8, that's fine.  I can rebase that, port
>> some of the other changes from v14 to it, and repost it as v15.
> 
> Are you working on that?  I've set aside time this week to hopefully
> get this over the finish line, but I don't want to find out that
> I've been duplicating your effort.

Yes, I expect to be posting the latest in maybe an hour?  I believe the latest patch (just reviewing, adjusting code comments, etc.) that I'm preparing to post has all the changes we've discussed, aside from your parameterId renaming.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  101. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T16:31:41Z

    
> On Apr 4, 2022, at 8:47 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> posting the latest in maybe an hour

Here it is:

  102. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T21:26:35Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> [ v15 patch ]

Thanks.  As I'm working through this, I'm kind of inclined to drop
the has_parameter_privilege() variants that take an OID as object
identifier.  This gets back to the fact that I don't think
pg_parameter_acl OIDs have any outside use; we wouldn't even have
them except that we need a way to track their role dependencies
in pg_shdepend.  AFAICS users will only ever be interested in
looking up a GUC by name.  Any objections?

Another thought here is that I see you're expending some code
to store the canonical name of a GUC in pg_parameter_acl, but
I think that's probably going too far.  In particular, for the
legacy mixed-case names like "DateStyle", what ends up in the
table is the mixed-case name, which seems problematic.  It would
definitely be problematic if an extension used such a name,
because we might or might not be aware of the idiosyncratic
casing at the time a GRANT is issued.  I'm thinking that we
really want to avoid looking up custom GUCs at all during GRANT,
because that can't do anything except create hazards.

So I think that instead of what you've got here, we should
(1) apply the map_old_guc_names[] mapping, which is constant
    (for any one PG release anyway)
(2) smash to lower case
(3) verify validity per valid_variable_name.

This also simplifies life on the lookup side, where it's sufficient
to do steps (1) and (2) before performing a catalog search.

Thoughts?

			regards, tom lane

  103. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-04T22:15:19Z

    On 4/4/22 17:26, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> [ v15 patch ]
> Thanks.  As I'm working through this, I'm kind of inclined to drop
> the has_parameter_privilege() variants that take an OID as object
> identifier.  This gets back to the fact that I don't think
> pg_parameter_acl OIDs have any outside use; we wouldn't even have
> them except that we need a way to track their role dependencies
> in pg_shdepend.  AFAICS users will only ever be interested in
> looking up a GUC by name.  Any objections?


Not from me


cheers


andrew

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

  104. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-05T00:01:00Z

    
> On Apr 4, 2022, at 2:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Thanks.  As I'm working through this, I'm kind of inclined to drop
> the has_parameter_privilege() variants that take an OID as object
> identifier.  This gets back to the fact that I don't think
> pg_parameter_acl OIDs have any outside use; we wouldn't even have
> them except that we need a way to track their role dependencies
> in pg_shdepend.  AFAICS users will only ever be interested in
> looking up a GUC by name.  Any objections?

None.

> Another thought here is that I see you're expending some code
> to store the canonical name of a GUC in pg_parameter_acl, but
> I think that's probably going too far.  In particular, for the
> legacy mixed-case names like "DateStyle", what ends up in the
> table is the mixed-case name, which seems problematic.  It would
> definitely be problematic if an extension used such a name,
> because we might or might not be aware of the idiosyncratic
> casing at the time a GRANT is issued.  I'm thinking that we
> really want to avoid looking up custom GUCs at all during GRANT,
> because that can't do anything except create hazards.

Yikes.  It took a few tries to see what you mean.  Yes, if the GRANT happens before the LOAD, that can have bad consequences.  So I agree something should be changed.

> So I think that instead of what you've got here, we should
> (1) apply the map_old_guc_names[] mapping, which is constant
>    (for any one PG release anyway)
> (2) smash to lower case
> (3) verify validity per valid_variable_name.
> 
> This also simplifies life on the lookup side, where it's sufficient
> to do steps (1) and (2) before performing a catalog search.
> 
> Thoughts?

That sounds right.  Do you already have something like that coded, or would you like me to post a patch?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  105. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-05T00:12:35Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Apr 4, 2022, at 2:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I think that instead of what you've got here, we should
>> (1) apply the map_old_guc_names[] mapping, which is constant
>> (for any one PG release anyway)
>> (2) smash to lower case
>> (3) verify validity per valid_variable_name.
>> 
>> This also simplifies life on the lookup side, where it's sufficient
>> to do steps (1) and (2) before performing a catalog search.
>> 
>> Thoughts?

> That sounds right.  Do you already have something like that coded, or would you like me to post a patch?

Wrote it already, no need for you to do it.

			regards, tom lane

  106. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-05T00:16:23Z

    
> On Apr 4, 2022, at 5:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Wrote it already, no need for you to do it.

Thanks!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  107. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-05T21:50:02Z

    Here's a v16, which I have editorialized on rather heavily.
I have not yet really looked at the docs, but I'm satisfied
with the code and tests now.

The main non-cosmetic changes I made:

* I got rid of the mechanisms that tried to make has_parameter_privilege()
act like USERSET GUCs have a forced public SET grant.  I think that's
fundamentally unworkable for custom GUCs, and it's just going to create
confusion and inconsistency.  As the code stands, it'll report SET
privilege if you're superuser or have been explicitly granted that
privilege, but having that privilege only matters if the GUC is SUSET.
Need to craft some documentation wording to explain that, but I'd rather
explain that than say "it might or might not tell you that SET privileges
exist".  In any case I think this is perfectly symmetrical with the
ALTER SYSTEM privilege: you can grant that all you want, but it still
won't let you set variables whose context value forbids it.

* That meant we have only one default ACL setting, so I was able to
get rid of aclparameterdefault() altogether.

* I took out the post-alter hook call in ExecGrant_Parameter.  I'm
prepared to listen to arguments why it should be put back in, but
said arguments would have to explain why GRANT ON PARAMETER should
do that when no other form of GRANT does.  (Or, possibly, it'd be
sane for all the forms to call that hook; but that would be material
for a different patch IMV.)

* I took out the pg_parameter_privileges view, which was undocumented
anyway, and which I think people would not find useful.  The fact that
it's missing entries for most GUCs and might contain bogus entries
for nonexistent GUCs seems like a strong reason why it's not very
useful in this form.  (But see below.)

* I thought the tab-completion stuff was completely excessive.
I got rid of the support for tab completion with both privileges
spelled out, because I think people are just going to write ALL
instead.  We don't have tab completion support for commands listing
multiple privileges of other object types, and I don't see why
PARAMETER is the place to start.  I also merged a bunch of duplicative
rules by relying on MatchAny where possible.


Aside from documentation, there is one loose end that perhaps would
be best done in a follow-up patch: I'm somewhat of the opinion that
we ought to reinvent a \dcp command for psql.  I'd define it as being
a compact version of the pg_settings view, showing the name, value,
and unit columns and probably not much else by default.  With "+",
we could add on the privileges if any, and perhaps the context since
you need that to interpret the privileges.  I think this'd be more
useful than the pg_parameter_privileges view, and it'd be handier
than pg_settings itself since you could do things like
	\dcp autovac*
with a lot less typing than it takes to get the same info
out of pg_settings directly.

Anyway, I'm out of patience with this for today, and I'm throwing
it up for the cfbot to have a look at.  I'll hit the docs tomorrow.

			regards, tom lane

  108. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-05T22:52:18Z

    I wrote:
> Anyway, I'm out of patience with this for today, and I'm throwing
> it up for the cfbot to have a look at.  I'll hit the docs tomorrow.

... and of course, the cfbot gave me the raspberry, because of a
merge conflict in gram.y.  Here's v17 rebased up to HEAD.

			regards, tom lane

  109. Re: Granting SET and ALTER SYSTE privileges for GUCs

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-06T17:29:38Z

    I wrote:
> Here's v17 rebased up to HEAD.

Pushed after fooling around with the docs.

I have a couple of followup ideas in mind (\dcp and another one),
which I'll start separate threads about.

			regards, tom lane