Re: Possible regression setting GUCs on \connect
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: David Steele <david@pgmasters.net>,
Alexander Korotkov <akorotkov@postgresql.org>,
Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2023-04-27T22:38:03Z
Lists: pgsql-hackers
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. 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. I've forgotten exactly what restrictive hack we use to get around that, 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. 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. regards, tom lane
Commits
-
Revert "Add USER SET parameter values for pg_db_role_setting"
- b9a7a822723a 16.0 landed
-
Fix wrong construct_array_builtin() call in GUCArrayDelete()
- db93e739ac61 16.0 landed
-
In hstore_plpython, avoid crashing when return value isn't a mapping.
- df38157d9466 16.0 cited
-
Allow granting SET and ALTER SYSTEM privileges on GUC parameters.
- a0ffa885e478 15.0 cited