SELinux Module RFC

Archives Ansible Project
1

I've written the first-draft of an SELinux module. I'm not sure it's
ready for a pull request, but I would like your feedback.

Let's call it a code review if you like.

Here's the code:
https://github.com/goozbach/ansible/blob/selinux_module/library/selinux

2

Hi Derek,

This part looks strange:


except ImportError:
    HAVE_SELINUX=False

    fail_json(msg='python-selinux required for this module')

I don’t think you’ll be able to call fail_json like that, since it’s part of module_common now. You should check for HAVE_SELINUX in main() and fail there.

Try this:

try:
import selinux
except ImportError:
HAVE_SELINUX = False
else:
HAVE_SELINUX = True

  • Mark
3

Looks really good.

Random comments.

1) the usage of 'with' requires python 2.6, which breaks our
self-imposed requirement that modules be written to support python 2.4
and later for EL 5 users. So alas, you must do open/close instead.

2) it seems a bit too restrictive to restrict the list of SELinux
policy types to the ones that just Red Hat has provided, so I'd
probably take that out

3) I would probably not set a default policy or mode, and require the
user pick (required=True) so they are sure to make concious decisions
and not be surprised.

4) minor style things, mostly my preferences

s/CHANGED/changed/, as it's not a constant
kill the spaces inside the parens
intra-line whitespace seperating different if-clauses is desirable
don't hide a useful side effect in a logging-related operation where
you have -- msgs.append(set_policy(policy)), assign the variable and
then add to msgs

What Mark says is true, though you could simply do: json.dumps({...}
and sys.exit() at the time of import and it would be fine.

(Pretty sure set_policy is just going to require the reboot, which you
could just handle with a notify -- though you'd have to notify both
the reboot and a delegated wait_for if you wanted to be 100%
foolproof, and that might be somewhat fiddly.)

5) Not required, but a type to manage selinux booleans seems pretty
easy (getsebool/setsebool) under the covers and you usually have to
fiddle with these more often than even the basic SELinux setup (which
is often done once and only once at kickstart time). (This feels like
a seperate module)

Good stuff!

--Michael

4

I’ve been sitting on a seboolean module to manage selinux booleans. It could use a little clean up before submitting. I can throw it somewhere for more eyeballs.

sf

5

Looks really good.

1) the usage of 'with' requires python 2.6, which breaks our
self-imposed requirement that modules be written to support python
2.4 and later for EL 5 users. So alas, you must do open/close
instead.

Fixed

2) it seems a bit too restrictive to restrict the list of SELinux
policy types to the ones that just Red Hat has provided, so I'd
probably take that out

3) I would probably not set a default policy or mode, and require the
user pick (required=True) so they are sure to make concious decisions
and not be surprised.

Good call, removed choices and made it required.

(Pretty sure set_policy is just going to require the reboot, which you could
just handle with a notify -- though you'd have to notify both the
reboot and a delegated wait_for if you wanted to be 100% foolproof,
and that might be somewhat fiddly.)

I'm pretty sure I would want to leave the reboot out of this module and
make it a standard practice when dealing with the playbook changing of
the selinux policy/state. (ie the documentation shows how to do reboot
and delegated wait_for).