Date: Thu, 14 Jul 2022 14:52:25 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: pg_parameter_aclcheck() and trusted extensions
Message-ID: <20220714215225.GA3173833@nathanxps13>
References: <20220706224727.GA2158260@nathanxps13>
 <YsYxEtX8zOYKf3Ru@paquier.xyz>
 <2995921.1657212060@sss.pgh.pa.us>
 <3126292.1657828950@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3126292.1657828950@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20220714215225.GA3173833%40nathanxps13>
Precedence: bulk

On Thu, Jul 14, 2022 at 04:02:30PM -0400, Tom Lane wrote:
> Here's a draft patch for that.  I initially ran around and changed all
> the set_config_option callers as I threatened before, but as I did it
> I could not help observing that they were all changing in exactly the
> same way: basically, they were passing GetUserId() if the GucContext
> is PGC_S_SESSION and BOOTSTRAP_SUPERUSERID otherwise.  Not counting
> guc.c internal call sites, there is a grand total of one caller that
> fails to fit the pattern.  So that brought me around to liking the idea
> of keeping set_config_option's API stable by making it a thin wrapper
> around another function with an explicit role argument.  The result,
> attached, poses far less of an API/ABI hazard than I was anticipating.
> If you're not poking into the GUC tables you have little to fear.
> 
> Most of the bulk of this is mechanical changes to pass the source
> role around properly in guc.c's data structures.  That's all basically
> copy-and-paste from the code to track the source context (scontext).

At first glance, this looks pretty reasonable to me.  

> I noted something that ought to be looked at separately:
> validate_option_array_item() seems like it needs to be taught about
> grantable permissions on GUCs.  I think that right now it may report
> permissions failures in some cases where it should succeed.

Which cases do you think might be inappropriately reporting permissions
failures?  It looked to me like this stuff was mostly used for
pg_db_role_setting, which wouldn't be impacted by the current set of
grantable GUC permissions.  Is the idea that you should be able to do ALTER
ROLE SET for GUCs that you have SET permissions for?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com