Re: Preventing non-superusers from altering session authorization
От | Nathan Bossart |
---|---|
Тема | Re: Preventing non-superusers from altering session authorization |
Дата | |
Msg-id | 20230709044708.GA4102497@nathanxps13 обсуждение исходный текст |
Ответ на | Re: Preventing non-superusers from altering session authorization (Joseph Koshakow <koshy44@gmail.com>) |
Ответы |
Re: Preventing non-superusers from altering session authorization
|
Список | pgsql-hackers |
On Sat, Jul 08, 2023 at 07:08:35PM -0400, Joseph Koshakow wrote: > On Sat, Jul 8, 2023 at 6:09 PM Nathan Bossart <nathandbossart@gmail.com> > wrote: > >>> I think the issue here is that if a session loses the ability to set >>> their session authorization in the middle of a transaction, then >>> rolling back the transaction may fail and cause the server to panic. >>> That's probably what the deleted comment mean when it said: >>> >>>> * It's OK because the check does not require catalog access and can't >>>> * fail during an end-of-transaction GUC reversion >> >> Yeah. IIUC the ERROR longjmps to a block that calls AbortTransaction(), >> which ERRORs again when resetting the session authorization, which causes >> us to call AbortTransaction() again, etc., etc. src/backend/utils/misc/README has the following relevant text: Note that there is no provision for a failure result code. assign_hooks should never fail except under the most dire circumstances, since a failure may for example result in GUC settings not being rolled back properly during transaction abort. In general, try to do anything that could conceivably fail in a check_hook instead, and pass along the results in an "extra" struct, so that the assign hook has little to do beyond copying the data to someplace. This applies particularly to catalog lookups: any required lookups must be done in the check_hook, since the assign_hook may be executed during transaction rollback when lookups will be unsafe. > Everything seems to work fine if the privilege check is moved to > check_session_authorization. Which is maybe what the comment meant > instead of assign_session_authorization. Ah, that does make more sense. I think we should split this into two patches: one to move the permission check to check_session_authorization() and another for the behavior change. I've attached an attempt at the first one (that borrows heavily from your latest patch). AFAICT the only reason that the permission check lives in SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is static to miscinit.c and doesn't have an accessor function. I added one, but it would probably just be removed by the following patch. WDYT? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Вложения
В списке pgsql-hackers по дате отправления: