Re: [v9.2] Add GUC sepgsql.client_label - Mailing list pgsql-hackers

On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2012/1/28 Kohei KaiGai <kaigai@kaigai.gr.jp>:
>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>>>> I'm wondering if a function would be a better fit than a GUC.  I don't
>>>>> think you can really restrict the ability to revert a GUC change -
>>>>> i.e. if someone does a SET and then a RESET, you pretty much have to
>>>>> allow that.  I think.  But if you expose a function then it can work
>>>>> however you like.
>>>>>
>>>> One benefit to use GUC is that we can utilize existing mechanism to
>>>> revert a value set within a transaction block on error.
>>>> If we implement same functionality with functions, XactCallback allows
>>>> sepgsql to get control on appropriate timing?
>>>
>>> Not sure, but I thought the use case was to set this at connection
>>> startup time and then hand the connection off to a client.  What keeps
>>> the client from just issuing RESET?
>>>
>> In the use case of Joshua, the original security label does not privileges
>> to reference any tables, and it cannot translate any domains without
>> credential within IC-cards. Thus, RESET is harmless.
>>
>> However, I also assume one other possible use-case; the originator
>> has privileges to translate 10 of different domains, but disallows to
>> revert it. In this case, RESET without permission checks are harmful.
>> (This scenario will be suitable with multi-category-model.)
>>
>> Apart from this issue, I found a problem on implementation using GUC
>> options. It makes backend crash, if it tries to reset sepgsql.client_label
>> without permissions within error handler because of double-errors.
>>
> I found an idea to avoid this scenario.
>
> The problematic case is reset GUC variable because of transaction
> rollback and permission violation, however, we don't intend to apply
> permission checks, since it is not committed yet.
> Thus, I considered to check state of the transaction using
> IsTransactionState() on the assign_hook of GUC variable.
>
> Unlike function based implementation, it is available to utilize existing
> infrastructure to set the client_label to be transaction-aware.
>
> It shall be implemented as:
>
> void
> sepgsql_assign_client_label(const char *newval, void *extra)
> {
>    if (!IsTransactionState)
>        return;
>
>    /* check whether valid security context */
>
>    /* check permission check to switch current context */
> }
>
> How about such an implementation?

Well, I think the idea that GUC changes can be reverted at any time is
fairly deeply ingrained in the GUC machinery.  So I think that's a
hack: it might happen to work today (or at least on the cases we can
think of to test), but I wouldn't be a bit surprised if there are
other cases where it fails, either now or in the future.  I don't see
any good reason to assume that unrevertable changes are OK even inside
a transaction context.  For example, if someone applies a context to a
function with ALTER FUNCTION, they're going to expect that the altered
context remains in effect while the function is running and gets
reverted on exit.  If you throw an error when the system tries to
revert the change at function-exit time, it might not crash the
server, but it's certainly going to violate the principle of least
astonishment.

Of course, we already have a trusted procedure mechanism which is
intended to support temporary changes to the effective security label,
so you might say, hey, people shouldn't do that.  But they might.  And
I wouldn't like to bet that's the only case that could be problematic
either.  What about a setting in postgresql.conf?  You could end up
being asked to set the security label to some other value before
you've initialized it.  What about SET LOCAL?  It's not OK to fail to
revert that at transaction exit.

Hence my suggestion of a function: if you use functions, you can
implement whatever semantics you want.

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