Re: Granting SET and ALTER SYSTE privileges for GUCs

On Wed, Mar 30, 2022 at 12:00 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
> > On Mar 28, 2022, at 3:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.