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

Wed Apr 10 13:26:57 UTC 2013

On 04/08/2013 12:28 PM, Tomas Babej wrote:
> 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
I implemented the approach.

Updated patch attached.

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