Обсуждение: Extension ownership and misuse of SET ROLE/SET SESSION AUTHORIZATION

pg_dump/restore fail to restore the ownership of an extension correctly:
in practice it'll always end up owned by whoever runs the restore
script.  We've sort of averted our eyes from that up to now, because
it's not a big deal in a world where most extensions have to be
superuser-owned anyway.  But I think it's no longer acceptable in a
world with trusted extensions.  So I started looking into fixing that.

Meanwhile ...

pg_dump and pg_restore have a --role switch, which causes them
to attempt to SET ROLE to the specified user name at startup.

They also have a --use-set-session-authorization switch, which causes
them to use SET SESSION AUTHORIZATION before CREATE, rather than
ALTER OWNER after CREATE, to set the ownership of restored objects.
Obviously, those commands will be issued per-object.

Now, for pg_dump there's no real conflict because --role determines
what we send to the source database server, not what is put into the
dump output.  But AFAICS, these two switches do not work together
in pg_restore.  We'll send SET ROLE at the start of the restore but
it'll be immediately and permanently overridden by the first
SET SESSION AUTHORIZATION.  Moreover, because SetSessionAuthorization
inspects the original (authenticated) user ID to decide if the command
is allowed, the SET ROLE doesn't help pass that permission check
even the first time.

Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
I don't actually see any way that we could get these features to
play together.  SET SESSION AUTHORIZATION insists on the originally
authenticated user being a superuser, so that the documented point of
--role (to allow you to start the restore from a not-superuser role)
isn't going to work.  I thought about starting to use SET ROLE for
both purposes, but it checks whether you have role privilege based
on the session userid, so that a previous SET ROLE doesn't get you
past that check even if it was a successful SET ROLE to a superuser.

The quick-and-dirty answer is to disallow these switches from being
used together in pg_restore, and I'm inclined to think maybe we should
do that in the back branches.

But ... the reason I noticed this is that I don't see any way to
restore extension ownership correctly unless we use the SET SESSION
AUTHORIZATION technique.  We don't have ALTER EXTENSION OWNER, and I'm
afraid that we never can have it now that we've institutionalized the
expectation that not all objects within an extension need have the
same owner --- that means ALTER EXTENSION OWNER could not know which
contained objects to change the owner of.  So while it might be an
acceptable restriction that --role prevents use of
--use-set-session-authorization, it's surely not acceptable that
--role is unable to restore extensions correctly.

The outline of a fix that I'm considering is

(1) In the backend, allow SET ROLE to succeed if either the session
userid or the current userid is a member of the desired role.  This
would mean that, given the use-case for --role that you are logging
into an account that can "SET ROLE postgres", it'd work to do

    SET ROLE postgres;
    SET ROLE anybody;
    ... create an object to be owned by anybody
    SET ROLE postgres;
    SET ROLE somebodyelse;
    ... create an object to be owned by somebodyelse
    SET ROLE postgres;
    ... lather rinse repeat

(2) Adjust pg_dump/pg_restore so that instead of SET SESSION
AUTHORIZATION, they use SET ROLE pairs as shown above to control
object ownership, when not using ALTER OWNER.  I'm not sure whether
to rename the --use-set-session-authorization switch ... it'd be
misleadingly named now, but there's backwards compatibility issues
if we change it.  Or maybe keep it and invent a separate
--use-set-role switch, though that opens the door for lots of
confusion.

(3) Adjust pg_dump/pg_restore so that extension ownership is
always restored using SET ROLE, whether you gave that switch or not.

Having said that ... I can't find the discussion right now, but
I recall Peter or Stephen complaining recently about how SET ROLE
and SET SESSION AUTHORIZATION allow more than the SQL spec says
they should.  Do we want to make successful restores dependent
on an even-looser definition of SET ROLE?  If not, how might we
handle this problem without assuming non-SQL semantics?

Thoughts?

            regards, tom lane

> On 13 Feb 2020, at 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is this being worked on for the 13 cycle such that it should be an open item?

> Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
> I don't actually see any way that we could get these features to
> play together.  SET SESSION AUTHORIZATION insists on the originally
> authenticated user being a superuser, so that the documented point of
> --role (to allow you to start the restore from a not-superuser role)
> isn't going to work.  I thought about starting to use SET ROLE for
> both purposes, but it checks whether you have role privilege based
> on the session userid, so that a previous SET ROLE doesn't get you
> past that check even if it was a successful SET ROLE to a superuser.
> 
> The quick-and-dirty answer is to disallow these switches from being
> used together in pg_restore, and I'm inclined to think maybe we should
> do that in the back branches.

..or should we do this for v13 and back-branches and leave fixing it for 14?
Considering the potential invasiveness of the fix I think the latter sounds
rather appealing at this point in the cycle.  Something like the attached
should be enough IIUC.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 13 Feb 2020, at 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
>> I don't actually see any way that we could get these features to
>> play together.

> Is this being worked on for the 13 cycle such that it should be an open item?

I didn't have it on my list, but yeah maybe we should add it to the
"pre-existing issues" list.

>> The quick-and-dirty answer is to disallow these switches from being
>> used together in pg_restore, and I'm inclined to think maybe we should
>> do that in the back branches.

> ..or should we do this for v13 and back-branches and leave fixing it for 14?
> Considering the potential invasiveness of the fix I think the latter sounds
> rather appealing at this point in the cycle.  Something like the attached
> should be enough IIUC.

pg_dump and pg_dumpall also have that switch no?

            regards, tom lane

> On 19 May 2020, at 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 13 Feb 2020, at 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
>>> I don't actually see any way that we could get these features to
>>> play together.
>
>> Is this being worked on for the 13 cycle such that it should be an open item?
>
> I didn't have it on my list, but yeah maybe we should add it to the
> "pre-existing issues" list.
>
>>> The quick-and-dirty answer is to disallow these switches from being
>>> used together in pg_restore, and I'm inclined to think maybe we should
>>> do that in the back branches.
>
>> ..or should we do this for v13 and back-branches and leave fixing it for 14?
>> Considering the potential invasiveness of the fix I think the latter sounds
>> rather appealing at this point in the cycle.  Something like the attached
>> should be enough IIUC.
>
> pg_dump and pg_dumpall also have that switch no?

They do, but there the switches actually work as intended and the combination
should be allowed AFAICT.  Since SET ROLE is sent to the source server and not
the output we can use for starting the dump without being a superuser.

cheers ./daniel

On Thu, Feb 13, 2020 at 5:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> (1) In the backend, allow SET ROLE to succeed if either the session
> userid or the current userid is a member of the desired role.  This
> would mean that, given the use-case for --role that you are logging
> into an account that can "SET ROLE postgres", it'd work to do
>
>         SET ROLE postgres;
>         SET ROLE anybody;
>         ... create an object to be owned by anybody
>         SET ROLE postgres;
>         SET ROLE somebodyelse;
>         ... create an object to be owned by somebodyelse
>         SET ROLE postgres;
>         ... lather rinse repeat

Honestly, the fact that this would work where a direct SET ROLE would
fail seems horribly counterintuitive.

> Having said that ... I can't find the discussion right now, but
> I recall Peter or Stephen complaining recently about how SET ROLE
> and SET SESSION AUTHORIZATION allow more than the SQL spec says
> they should.  Do we want to make successful restores dependent
> on an even-looser definition of SET ROLE?  If not, how might we
> handle this problem without assuming non-SQL semantics?

I don't know how to solve this problem, but I think loosening the
requirements for 'SET ROLE' is something where a lot of caution is
warranted.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On 19 May 2020, at 23:07, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 19 May 2020, at 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>> On 13 Feb 2020, at 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
>>>> I don't actually see any way that we could get these features to
>>>> play together.
>>
>>> Is this being worked on for the 13 cycle such that it should be an open item?
>>
>> I didn't have it on my list, but yeah maybe we should add it to the
>> "pre-existing issues" list.

The recent work on pg_dump reminded me about this thread, AFAICT this was never
addressed?  Are you including it in the current line of work (if so, sorry for
missing it in the threads) or should I take a stab at it?

>>>> The quick-and-dirty answer is to disallow these switches from being
>>>> used together in pg_restore, and I'm inclined to think maybe we should
>>>> do that in the back branches.
>>
>>> ..or should we do this for v13 and back-branches and leave fixing it for 14?
>>> Considering the potential invasiveness of the fix I think the latter sounds
>>> rather appealing at this point in the cycle.  Something like the attached
>>> should be enough IIUC.
>>
>> pg_dump and pg_dumpall also have that switch no?
>
> They do, but there the switches actually work as intended and the combination
> should be allowed AFAICT.  Since SET ROLE is sent to the source server and not
> the output we can use for starting the dump without being a superuser.

This patch still seems relevant for back-branches, but starting at 14 this time.

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
> On 13 Feb 2020, at 23:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Given the current behavior of SET ROLE and SET SESSION AUTHORIZATION,
>>> I don't actually see any way that we could get these features to
>>> play together.

> The recent work on pg_dump reminded me about this thread, AFAICT this was never
> addressed?  Are you including it in the current line of work (if so, sorry for
> missing it in the threads) or should I take a stab at it?

No, I'm not working on this --- I'd kind of forgotten about it.
People didn't seem to like the idea of loosening the requirements
for SET ROLE, but I'm not sure how to solve the extension-ownership
problem without it.

> This patch still seems relevant for back-branches, but starting at 14 this time.

I think the appropriate thing to do is stick your patch into all branches
for the moment.  We can remove it again whenever we invent a fix for the
problem.

            regards, tom lane

> On 29 Oct 2021, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:

>> This patch still seems relevant for back-branches, but starting at 14 this time.
>
> I think the appropriate thing to do is stick your patch into all branches
> for the moment.  We can remove it again whenever we invent a fix for the
> problem.

Fair enough, I'll make that happen.

--
Daniel Gustafsson        https://vmware.com/

> On 29 Oct 2021, at 20:00, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 29 Oct 2021, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>
>>> This patch still seems relevant for back-branches, but starting at 14 this time.
>>
>> I think the appropriate thing to do is stick your patch into all branches
>> for the moment.  We can remove it again whenever we invent a fix for the
>> problem.
>
> Fair enough, I'll make that happen.

I added a small note to the doc page and a simple test, unless objections I'll
apply the attached v2 all the way down.

--
Daniel Gustafsson        https://vmware.com/