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

[Tomas Babej]

On 04/05/2013 07:43 PM, Rob Crittenden wrote:
> Tomas Babej wrote:
>> 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
>
> Ok, this is getting us closer, and opens a philosophical discussion.
>
> As implemented, this forces SELinux to be at least be available on the 
> box, and by default required to be enabled. This is our strong 
> recommendation for all users.
>
> There is a flag to allow it be in permissive, which will help people 
> work around any policy issues or if they don't want SELinux. They'd 
> still have to run without it completely disabled though.
>
> This does leave other platforms in a bad place though, there is no out 
> for them.
>
> How about adding a require_selinux() call to the platform code 
> defaulting to True. If someone wants to override that with False they 
> can. A call to that could be added inside the other is_selinux... 
> calls to not pollute the rest of the code with it, and if it's False 
> we just skip all the SELinux stuff.
Having the call to require_selinux inside the is_selinux_* functions 
would force us to pretend that SELinux is up and in enforcing mode (for 
both is_selinux_available and is_selinux_enabled  to return True). While 
that will allow us to pass the install checks, it would cause failure in 
other parts of the code, e.g. in httpinstance.py while setting SELinux 
booleans.

If we want to provide an easy way for other platforms, it comes with the 
cost of polluting our code with require_selinux(). Personally I'm fine 
with that, it will just clearly mark
which parts of the code are dependant on SELinux.

> This still leaves us with a hardcoded requirement for SELinux in 
> Fedora/RHEL and should provide some flexibility for other platforms as 
> they come.
>
> What do you think?
>
> rob

Tomas