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

[Tomas Babej]

On 04/04/2013 04:25 PM, Rob Crittenden wrote:
> 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
I added the is_selinux_available function. Both is_selinux_enabled and 
is_selinux_available default to False in services.py.in. Maintainer that 
would want to implement platform file, would have to implement both 
functions for server install. We require SELinux for server anyway. For 
client installs, default versions work fine.

I converted httpinstance.configure_selinux_for_httpd() to use 
is_selinux_enabled(). I also found a similar call in adtrustinstance.py

Tomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0027-6-Add-checks-for-SELinux-in-install-scripts.patch
Type: text/x-patch
Size: 16655 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130405/9460cc56/attachment.bin>