Re: Possible regression setting GUCs on \connect

On Fri, Apr 28, 2023 at 5:01 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote:
> > >> The right way to do this was not to add some
> > >> poorly-explained option to ALTER ROLE, but to record the role OID that
> > >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE
> > >> setting whether that role (still) has the right to change the
> > >> specified setting. As implemented, this can't possibly track changes
> > >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
> > >> introducing outright security holes like the one fixed by 13d838815.
> >
> > > I generally agree. At least, I think it would be nice to avoid adding a
> > > new option if possible. It's not clear to me why we'd need to also check
> > > privileges at login time as opposed to only checking them at ALTER ROLE SET
> > > time.
> >
> > Perhaps there's room to argue about that. But ISTM that if someone
> > does ALTER ROLE SET on the strength of some privilege you granted
> > them, and then you regret that and revoke the privilege, then the
> > ALTER ROLE setting should not continue to work. So I would regard
> > the session-start-time check as the primary one. Checking when
> > ALTER ROLE is done is just a user-friendliness detail.
>
> From my point of view that is much different from what we're doing
> with other database objects. If some role gets revoked from
> privilege, that doesn't affect the actions done with that privilege
> before. The law is not retroactive. If one has created some tables,
> those tables still work if the creator gets revoked privilege or even
> gets deleted. Why should the setting behave differently?
>

I see there are mainly three concerns (a) Avoid adding the new option
USER SET, (b) The behavior of this feature varies from the precedents
set by a0ffa885e and 13d838815, (c) As per discussion, not following
13d838815 could lead to a similar security hole in this feature.

Now, I don't know whether Tom and or Nathan share your viewpoint and
feel that nothing should be done. It would have been better if such a
discussion happens during development but I can understand that mostly
the other senior people are sometimes busy enough to pay attention to
all the work going on. I see that when Alexander proposed this new
option and behavior in the original thread [1], there were no
objections, so the commit followed the normal community rules but we
have seen various times that post-commit reviews also lead to changing
or reverting the feature.

I see that you seem to think it would be over-engineering to follow
the suggestions shared here but without the patch or some further
discussion, it won't be easy to conclude that.

Tom/Nathan, do you have any further suggestions here?

[1] - https://www.postgresql.org/message-id/CAPpHfdsLd6E--epnGqXENqLP6dLwuNZrPMcNYb3wJ87WR7UBOQ@mail.gmail.com

--
With Regards,
Amit Kapila.