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

Hi, Yeb.

Thanks for your reviewing and patch updates.
(and sorry my delayed response...)

I'd like to point out a case when plabel->label is NULL.

In case of sepgsql_setcon() being invoked with null argument
to reset security label of the client, but not committed yet,
the last item of the client_label_pending has null label.
(It performs as a mark of a security label being reset.)

It is a case when sepgsql_get_client_label() should return
the client_label_peer, not plabel->label.
So, I reverted some of your replacement; that assumes the
pending label is valid with Assert() check to null value.

Most of comments update are quite helpful for me.
So, I merged your revised one in this patch.

Thanks so much!

2012/3/3 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
> On 2012-02-24 17:25, Yeb Havinga wrote:
>>
>> On 2012-02-23 12:17, Kohei KaiGai wrote:
>>>
>>> 2012/2/20 Yeb Havinga<yebhavinga(at)gmail(dot)com>:
>>>>
>>>> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> I took a short look at this patch but am stuck getting the regression
>>>> test
>>>> to run properly.
>>>>
>>>> First, patch 2 misses the file sepgsql.sql.in and therefore the creation
>>>> function command for sepgsql_setcon is missing.
>>>>
>>> Thanks for your comments.
>>>
>>> I added the definition of sepgsql_setcon function to sepgsql.sql.in file,
>>> in addition to patch rebasing.
>>
>>
>> Very brief comments due to must leave keyboard soon:
>>
>> I read the source code and played a bit with setcon and the debugger, no
>> strange things found.
>>
>> Code comments / questions:
>
>
> I took the liberty to change a few things, mostly comments, in the attached
> patch:
>
>>
>> maybe client_label_committed is a better name for client_label_setcon?
>
>
> this change was made.
>
>>
>> Is the double negation in the sentence below intended?
>
>
> several comments were changed / moved. There is now one place where te
> behaviour of the different client_label variables are explained.
>
>
>>
>> sepgsql_set_client_label(), maybe add a comment to !new_label that it is
>> reset to the peer label.
>
>
> done.
>
>>
>> Is the assert client_label_peer != NULL in sepgsql_get_client_label
>> necessary?
>> new_label == NULL / pending_label->label == NULL means use the peer label.
>> Why not use the peer label instead?
>
>
> It turned out that pending_label->label is invariantly non null. Changed
> code to assume that and added some Asserts.
>
>
>>
>> set_label: if new_label == current label according to getcon, is it
>> necessary to add to the pending list?
>
>
> this question still remains. Maybe there would be reasons to hit selinux
> with the question: can I change from A->A.
>
>>
>> sepgsql_subxact_callback(), could this be made easier to read by just
>> taking llast(client_label_pending), assert that plabel->subid == mySubId and
>> then list_delete on pointer of that listcell?
>
>
> no this was a naieve suggestion, which fails in any case of a subtransaction
> with not exactly one call to sepgsql_setcon()
>
>
>> Some comments contain typos, I can spend some time on this, though I'm not
>> a native english speaker so it won't be perfect.
>
>
> sgml documentation must still be added. If time permits I can spend some
> time on that tomorrow.
>
>
> regards,
> Yeb Havinga
>
>
> --
> Yeb Havinga
> http://www.mgrid.net/
> Mastering Medical Data
>

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