Re: Granting SET and ALTER SYSTE privileges for GUCs - Mailing list pgsql-hackers

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
parameterwill create a SUSET placeholder, which is not quite right in some cases.  However, the installation scripts
formodules 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
themodule which defines parameter some.such, that revoke won't be retained.  That doesn't seem entirely wrong to me,
sinceno privilege to set the parameter existed when the revoke was performed, but rather was granted along with the
creationof the parameter, but it also doesn't seem entirely right.  Maybe revoke commands (but not grant commands)
shoulderror on unrecognized custom parameters?  I didn't implement that here, but can do so if you think that makes
moresense than this new behavior. 
>
> I changed add_placeholder_variable() to take a GucContext argument.  It previously always used PGC_USERSET, which is
whatall pre-existing call sites now pass into it, but that seems a bit inappropriate where we're creating a placeholder
thatwe intend to treat as a SUSET variable until such time as a module gets installed saying otherwise.  Not changing
add_placeholder_variablein 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
thehook 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.