User-Id Tracking when Portal was started

The attached patch is delivered from the discussion around row-level
access control feature. A problem Florian pointed out is refcursor
declared in security definer function. Even though all the permission
checks are applied based on privilege of the owner of security-definer
function in case when it tries to define a cursor bound to a particular
query, it shall be executed under the credential of executor.
In the result, "current_user" or "has_table_privilege()" will return
unexpected result, even if it would be used in as a part of security
policy for each row.

This patch enables to switch user-id when user tried to execute
a portal being started under the different user's credential.

postgres=# CREATE FUNCTION f1(refcursor) RETURNS refcursor
postgres-# SECURITY DEFINER LANGUAGE plpgsql
postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user,* FROM t1;
postgres'# RETURN $1; END';
CREATE FUNCTION

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> BEGIN;
BEGIN
postgres=> SELECT f1('abc');
f1
-----
abc
(1 row)

postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
kaigai | 1 | aaa <=== 'abc' was started under the
kaigai's privilege
(1 row)

postgres=> SELECT current_user;
current_user
--------------
alice
(1 row)

postgres=> DECLARE xyz CURSOR FOR SELECT current_user, * FROM t1;
DECLARE CURSOR
postgres=> FETCH xyz;
current_user | a | b
--------------+---+-----
alice | 1 | aaa
(1 row)

postgres=> RESET SESSION AUTHORIZATION;
RESET
postgres=# FETCH xyz; <=== 'xyz' was started under the
kaigai's privilege
current_user | a | b
--------------+---+-----
alice | 2 | bbb
(1 row)

BTW, same problem will be caused in case when security label of
the client would be switched. Probably, a hook should be injected
around the places where I patched with this patch.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Jul 2, 2012 at 10:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is delivered from the discussion around row-level
> access control feature. A problem Florian pointed out is refcursor
> declared in security definer function. Even though all the permission
> checks are applied based on privilege of the owner of security-definer
> function in case when it tries to define a cursor bound to a particular
> query, it shall be executed under the credential of executor.
> In the result, "current_user" or "has_table_privilege()" will return
> unexpected result, even if it would be used in as a part of security
> policy for each row.

Why not just save and restore the user ID and security context
unconditionally, instead of doing this kind of dance?

+ if (portal->userId != GetUserId())
+ SetUserIdAndSecContext(portal->userId, portal->secCo
+ else
+ saveUserId = InvalidOid;

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

2012/7/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jul 2, 2012 at 10:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The attached patch is delivered from the discussion around row-level
>> access control feature. A problem Florian pointed out is refcursor
>> declared in security definer function. Even though all the permission
>> checks are applied based on privilege of the owner of security-definer
>> function in case when it tries to define a cursor bound to a particular
>> query, it shall be executed under the credential of executor.
>> In the result, "current_user" or "has_table_privilege()" will return
>> unexpected result, even if it would be used in as a part of security
>> policy for each row.
>
> Why not just save and restore the user ID and security context
> unconditionally, instead of doing this kind of dance?
>
> + if (portal->userId != GetUserId())
> + SetUserIdAndSecContext(portal->userId, portal->secCo
> + else
> + saveUserId = InvalidOid;
>
In case when CurrentUserId was updated during the code block
(E.g, execution of SET SESSION AUTHORIZATION), it overwrites
this update on restoring user-id and security-context.

Comparison of user-id is a simple marker to check whether this
portal contain an operation to switch user-id, because these
operations are prohibited within security restricted context.
(Is there some other good criteria?)

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2012/7/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Why not just save and restore the user ID and security context
>> unconditionally, instead of doing this kind of dance?
>>
>> + if (portal->userId != GetUserId())
>> + SetUserIdAndSecContext(portal->userId, portal->secCo
>> + else
>> + saveUserId = InvalidOid;
>>
> In case when CurrentUserId was updated during the code block
> (E.g, execution of SET SESSION AUTHORIZATION), it overwrites
> this update on restoring user-id and security-context.

Um... what should happen if there was a SET SESSION AUTHORIZATION
to the portal's userId? This test will think nothing happened.

regards, tom lane

2012/7/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2012/7/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> Why not just save and restore the user ID and security context
>>> unconditionally, instead of doing this kind of dance?
>>>
>>> + if (portal->userId != GetUserId())
>>> + SetUserIdAndSecContext(portal->userId, portal->secCo
>>> + else
>>> + saveUserId = InvalidOid;
>>>
>> In case when CurrentUserId was updated during the code block
>> (E.g, execution of SET SESSION AUTHORIZATION), it overwrites
>> this update on restoring user-id and security-context.
>
> Um... what should happen if there was a SET SESSION AUTHORIZATION
> to the portal's userId? This test will think nothing happened.
>
In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
It makes nothing happen from viewpoint of users.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2012/7/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Um... what should happen if there was a SET SESSION AUTHORIZATION
>> to the portal's userId? This test will think nothing happened.

> In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
> It makes nothing happen from viewpoint of users.

My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.

regards, tom lane

2012/7/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2012/7/3 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> Um... what should happen if there was a SET SESSION AUTHORIZATION
>>> to the portal's userId? This test will think nothing happened.
>
>> In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
>> It makes nothing happen from viewpoint of users.
>
> My point is that it seems like a bug that the secContext gets restored
> in one case and not the other, depending on which user ID was specified
> in SET SESSION AUTHORIZATION.
>
Sorry, the above description mention about a case when it does not use
the marker to distinguish a case to switch user-id from a case not to switch.
(I though I was asked the behavior if this logic always switches /
restores ids.)

The patch itself works correctly, no regression test failed even though
several tests switches user-id using SET SESSION AUTHORIZATION.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> My point is that it seems like a bug that the secContext gets restored
>> in one case and not the other, depending on which user ID was specified
>> in SET SESSION AUTHORIZATION.
>>
> Sorry, the above description mention about a case when it does not use
> the marker to distinguish a case to switch user-id from a case not to switch.
> (I though I was asked the behavior if this logic always switches /
> restores ids.)
>
> The patch itself works correctly, no regression test failed even though
> several tests switches user-id using SET SESSION AUTHORIZATION.

I don't believe that proves anything. There are lots of things that
aren't tested by the regression tests, and there's no guarantee that
any you've added cover all bases, either. We always treat user-ID and
security context as a unit; you haven't given any reason why this case
should be handled differently, and I bet it shouldn't.

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

2012/7/4 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> My point is that it seems like a bug that the secContext gets restored
>>> in one case and not the other, depending on which user ID was specified
>>> in SET SESSION AUTHORIZATION.
>>>
>> Sorry, the above description mention about a case when it does not use
>> the marker to distinguish a case to switch user-id from a case not to switch.
>> (I though I was asked the behavior if this logic always switches /
>> restores ids.)
>>
>> The patch itself works correctly, no regression test failed even though
>> several tests switches user-id using SET SESSION AUTHORIZATION.
>
> I don't believe that proves anything. There are lots of things that
> aren't tested by the regression tests, and there's no guarantee that
> any you've added cover all bases, either. We always treat user-ID and
> security context as a unit; you haven't given any reason why this case
> should be handled differently, and I bet it shouldn't.
>
This patch always handles user-id and security context as a unit.
In case when it was switched, then it shall be always restored.
And, in case when it was not switched, then it shall never be restored.

Was my explanation confusing?
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jul 4, 2012 at 4:24 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/7/4 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> My point is that it seems like a bug that the secContext gets restored
>>>> in one case and not the other, depending on which user ID was specified
>>>> in SET SESSION AUTHORIZATION.
>>>>
>>> Sorry, the above description mention about a case when it does not use
>>> the marker to distinguish a case to switch user-id from a case not to switch.
>>> (I though I was asked the behavior if this logic always switches /
>>> restores ids.)
>>>
>>> The patch itself works correctly, no regression test failed even though
>>> several tests switches user-id using SET SESSION AUTHORIZATION.
>>
>> I don't believe that proves anything. There are lots of things that
>> aren't tested by the regression tests, and there's no guarantee that
>> any you've added cover all bases, either. We always treat user-ID and
>> security context as a unit; you haven't given any reason why this case
>> should be handled differently, and I bet it shouldn't.
>>
> This patch always handles user-id and security context as a unit.
> In case when it was switched, then it shall be always restored.
> And, in case when it was not switched, then it shall never be restored.
>
> Was my explanation confusing?

It's not that your explanation is confusing; it's that it doesn't
match the code.

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