Re: [v9.2] Add GUC sepgsql.client_label

2012/1/30 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/1/28 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>>> 2012/1/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>> 2012/1/26 Robert Haas <robertmhaas(at)gmail(dot)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.
>
I tried to implement it based on the function approach.

The new sepgsql_setcon(TEXT) enables to set and reset alternative
security label of the database client, unless it violates permission
checks.

The supplied alternative security label is once appended on the
client_label_pending list, then an active item shall be saved on
the transaction commit time. If sub-transaction is rollbacked to
a particular savepoint, items on the pending-list shall be removed
according to its sub-transaction-id.

It extends idea of client_label into three types; peer-label, setcon-label
and func-label. The first one is obtained via getpeercon(3) on
database logon time, and always not null. The second one is set
using the new sepgsql_setcon(), and the other one is set during
execution of trusted procedure.

Thus, the logic of sepgsql_get_client_label() were modified to handle
these three kind of client labels correctly.

The attached part-1 patch moves related routines from hooks.c to
label.c because of references to static variables.
The part-2 patch implements above mechanism.

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