[Freeipa-devel] [PATCH 0027] Add checks for SELinux in install scripts

[Rob Crittenden]

Tomas Babej wrote:
> On Tue 02 Apr 2013 10:05:06 AM CEST, Tomas Babej wrote:
>> On Mon 01 Apr 2013 10:01:14 PM CEST, Rob Crittenden wrote:
>>> Tomas Babej wrote:
>>>> On Tue 19 Feb 2013 08:37:26 PM CET, Rob Crittenden wrote:
>>>>> Tomas Babej wrote:
>>>>>> On 02/04/2013 04:21 PM, Rob Crittenden wrote:
>>>>>>> Tomas Babej wrote:
>>>>>>>> On 01/30/2013 05:12 PM, Tomas Babej wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> The checks make sure that SELinux is:
>>>>>>>>>   - installed and enabled (on server install)
>>>>>>>>>   - installed and enabled OR not installed (on client install)
>>>>>>>>>
>>>>>>>>> Please note that client installs with SELinux not installed are
>>>>>>>>> allowed since freeipa-client package has no dependency on SELinux.
>>>>>>>>> (any objections to this approach?)
>>>>>>>>>
>>>>>>>>> The (unsupported) option --allow-no-selinux has been added. It can
>>>>>>>>> used to bypass the checks.
>>>>>>>>>
>>>>>>>>> Parts of platform-dependant code were refactored to use newly
>>>>>>>>> added
>>>>>>>>> is_selinux_enabled() function.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3359
>>>>>>>>>
>>>>>>>>> Tomas
>>>>>>>>
>>>>>>>> I forgot to edit the man pages. Thanks Rob!
>>>>>>>>
>>>>>>>> Updated patch attached.
>>>>>>>>
>>>>>>>> Tomas
>>>>>>>
>>>>>>> After a bit of off-line discussion I don't think we're quite ready
>>>>>>> yet
>>>>>>> to require SELinux by default on client installations (even with a
>>>>>>> flag to work around it). The feeling is this would be disruptive to
>>>>>>> existing automation.
>>>>>>>
>>>>>>> Can you still do the check but not enforce it, simply display a big
>>>>>>> warning if SELinux is disabled?
>>>>>>>
>>>>>>> rob
>>>>>>>
>>>>>>
>>>>>> Sure, here is the updated patch.
>>>>>>
>>>>>> I edited the commit message, RFE description and man pages
>>>>>> according to
>>>>>> the new behaviour.
>>>>>>
>>>>>> Tomas
>>>>>
>>>>> The patch looks good, I'm just wondering about one thing. The default
>>>>> value for is_selinux_enabled() is True in ipapython/services.py.in.
>>>>>
>>>>> So this means that any non-Red Hat/non-Fedora system, by default, is
>>>>> going to assume that SELinux is enabled.
>>>>>
>>>>> My hesitation has to when we call check_selinux_status(). It may
>>>>> incorrectly error out. I suspect that the user would have to work
>>>>> around this using --allow-selinux-disabled but this wouldn't make a
>>>>> lot of sense since they actually do have SELinux disabled.
>>>>
>>>> Yes, you're right. And the error message would not even be helpful
>>>> since
>>>> it would tell the user to install policycoreutils package. This
>>>> would be
>>>> the
>>>> case both with server and client installs when selinux would not be
>>>> installed
>>>> at all.
>>>>
>>>>> What do you think?
>>>>>
>>>>> rob
>>>>
>>>> Well we have 2 options as I see it:
>>>>
>>>> 1.) We can either return None as default, and add checks to
>>>> check_selinux_status, restore_context and install scripts that would
>>>> ensure that we behave properly when is_selinux_enabled() is not
>>>> implemented.
>>>>
>>>> 2.) We can remove the default value, since it would cause forementioned
>>>> crash and add comment that this function needs to be implemented
>>>> properly in every platform file.
>>>>
>>>> I'm probably for option 2, there's no need to clutter the code with
>>>> checks
>>>> that compensate for improper platform file implementations.
>>>>
>>>> Tomas
>>>
>>> I agree with you on option 2.
>>>
>>> rob
>>
>> I updated the patch accordingly.
>>
>> Tomas
>
> Sorry, wrong patch. Correct version attached.
>
> Tomas

I'm sorry to throw this back again after so long (and having agreed with 
the approach).

So I was thinking about how another distro maintainer would have to deal 
with this. By default with this patch check_selinux_status() returns 
None which is evaluated as False, so the warning will get thrown. If 
they set it to be True to avoid the warning then other things may blow 
up because SELinux really isn't enabled, so we really haven't gotten 
anywhere.

I think the problem is we're trying to cram too much into one function. 
I wonder if a is_selinux_available() command would help which would 
short-circuit all of this.

While trying to figure out how this worked I found 
httpinstance.configure_selinux_for_httpd() which makes a similar call to 
see if SELinux is available, so maybe we should convert that as well.

rob