Subject: Re: Granting SET and ALTER SYSTE privileges for GUCs

From: Tom Lane <[email-masked]>
Date: Sun, 06 Mar 2022 17:57:10 -0500

Mark Dilger <[email-masked]> writes:
>> On Mar 6, 2022, at 2:13 PM, Tom Lane <[email-masked]> 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

---

From: Mark Dilger <[email-masked]>
Date: Sun, 6 Mar 2022 15:06:43 -0800

> On Mar 6, 2022, at 2:57 PM, Tom Lane <[email-masked]> wrote:
> 

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

> 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

---

From: Tom Lane <[email-masked]>
Date: Sun, 06 Mar 2022 18:27:43 -0500

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

> 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

---

From: Mark Dilger <[email-masked]>
Date: Tue, 15 Mar 2022 13:59:08 -0700

> On Mar 6, 2022, at 3:27 PM, Tom Lane <[email-masked]> wrote:
> 

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.

—
Mark Dilger

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 16 Mar 2022 07:43:28 -0400

> 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…

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…

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Mar 2022 14:47:24 -0400

> and it seems to do what I expect in every case, and the things I found

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

---

From: Mark Dilger <[email-masked]>
Date: Wed, 16 Mar 2022 11:59:06 -0700

> On Mar 16, 2022, at 11:47 AM, Tom Lane <[email-masked]> 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".  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...)

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

> 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?

> 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

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Mar 2022 15:06:04 -0400

Mark Dilger <[email-masked]> writes:
>> ... I therefore judge the

> 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 informa…

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

---

From: Joshua Brindle <[email-masked]>
Date: Wed, 16 Mar 2022 15:36:21 -0400

> 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?

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Mar 2022 15:58:57 -0400

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 16 Mar 2022 16:07:43 -0400

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Mar 2022 16:53:02 -0400

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

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

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 16 Mar 2022 17:09:35 -0400

> 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

---

From: Joshua Brindle <[email-masked]>
Date: Thu, 17 Mar 2022 09:24:54 -0400

<snip>

> I remain of the opinion that this

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?

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 17 Mar 2022 14:52:34 +0100

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

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 17 Mar 2022 14:54:57 +0100

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

---

From: Robert Haas <[email-masked]>
Date: Thu, 17 Mar 2022 12:04:42 -0400

> 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

---

From: Robert Haas <[email-masked]>
Date: Thu, 17 Mar 2022 12:18:51 -0400

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

---

From: Mark Dilger <[email-masked]>
Date: Thu, 17 Mar 2022 09:19:24 -0700

> On Mar 17, 2022, at 9:04 AM, Robert Haas <[email-masked]> wrote:
> 
> not just in terms of telling other people how they have to write
> their patches.

...

>  the burden of maintaining relatively little-used features

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

---

From: Joshua Brindle <[email-masked]>
Date: Thu, 17 Mar 2022 12:29:53 -0400

> 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;
}
}

---

From: Robert Haas <[email-masked]>
Date: Thu, 17 Mar 2022 12:34:25 -0400

> is in-core, MAC is via hooks)

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

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

---

From: Mark Dilger <[email-masked]>
Date: Thu, 17 Mar 2022 09:36:49 -0700

> On Mar 17, 2022, at 9:29 AM, Joshua Brindle <[email-masked]> wrote:
> 
> I hope this patch can be rolled

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

—
Mark Dilger

---

From: Joshua Brindle <[email-masked]>
Date: Thu, 17 Mar 2022 14:19:48 -0400

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

Great, thanks.

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