Re: Possible regression setting GUCs on \connect - Mailing list pgsql-hackers

On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Nathan Bossart <nathandbossart@gmail.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?

Additionally, I think if we start recording role OID, then we need a
full set of management clauses for each individual option ownership.
Otherwise, we would leave this new role OID without necessarily
management facilities.  But with them, the whole stuff will look like
awful overengineering.

> Also, in the case of an extension-defined GUC, we don't necessarily
> know its privilege level at either ALTER ROLE time or session start,
> since the extension might not yet be loaded at either point.

Yes, that's it.

> I've forgotten exactly what restrictive hack we use to get around
> that,

As I understand the restrictive hack is to assume that the role is at
least SUSET.

> but the *only* way to do that fully correctly is to record
> the role that did the ALTER and then check its privileges at extension
> load time.

This depends on the understanding of correctness.   Recording role OID
would mean altering that role privileges or deleting the role would
affect the settings.  Even if that is correct, this is very very far
from the behavior we had for decades.

> I think that 096dd80f3 has made it harder not easier
> to get to the point of doing that correctly, because it's added
> a feature that we'll have to figure out how to make interoperate
> with a correct implementation.

With 096dd80f3, we still may revoke the setting of USERSET options
from the public.  Even if the option is not currently loaded at ALTER
time, we still may find an explicit revoke recorder in the system
catalog.  That behavior will be current if we understand the default
options as separate material things (as it is today), but not part of
the setter role.

------
Regards,
Alexander Korotkov