Correctly write SELinux config file (#31251)

* Add new lines to end of config file lines

* Properly write out selinux config file

Change module behavior to not always report a change but warn if a reboot is needed and return reboot_required.

Improve the output messages.

Add strip parameter to get_file_lines utility to help with parsing the selinux config file.

* Add return documentation

* Add integration tests for selinux module

* Use consistent capitalization for SELinux

* Use atomic_move in selinux module

* Don't copy the config file initially

There's no need to make a copy just for reading.

* Put message after set_config_policy in case the change fails

* Add aliases to selinux tests

lib/ansible/module_utils/facts/utils.py

@@ -36,9 +36,9 @@ def get_file_content(path, default=None, strip=True):
     return data
 
 
-def get_file_lines(path):
+def get_file_lines(path, strip=True):
     '''get list of lines from file'''
-    data = get_file_content(path)
+    data = get_file_content(path, strip=strip)
     if data:
         ret = data.splitlines()
     else:

lib/ansible/modules/system/selinux.py

@@ -8,9 +8,11 @@ from __future__ import absolute_import, division, print_function
 __metaclass__ = type
 
 
-ANSIBLE_METADATA = {'metadata_version': '1.1',
+ANSIBLE_METADATA = {
+    'metadata_version': '1.1',
     'status': ['stableinterface'],
-                    'supported_by': 'core'}
+    'supported_by': 'core'
+}
 
 
 DOCUMENTATION = '''
@@ -59,21 +61,51 @@ EXAMPLES = '''
     state: disabled
 '''
 
+RETURN = '''
+msg:
+    description: Messages that describe changes that were made
+    returned: always
+    type: string
+    sample: Config SELinux state changed from 'disabled' to 'permissive'
+configfile:
+    description: Path to SELinux configuration file
+    returned: always
+    type: string
+    sample: /etc/selinux/config
+policy:
+    description: Name of the SELinux policy
+    returned: always
+    type: string
+    sample: targeted
+state:
+    description: SELinux mode
+    returned: always
+    type: string
+    sample: enforcing
+reboot_required:
+    description: Whether or not an reboot is required for the changes to take effect
+    returned: always
+    type: bool
+    sample: true
+'''
+
 import os
 import re
+import tempfile
 
 try:
     import selinux
     HAS_SELINUX = True
 except ImportError:
     HAS_SELINUX = False
+
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.facts.utils import get_file_lines
 
 
 # getter subroutines
 def get_config_state(configfile):
-    lines = get_file_lines(configfile)
+    lines = get_file_lines(configfile, strip=False)
 
     for line in lines:
         stateline = re.match(r'^SELINUX=.*$', line)
@@ -82,7 +114,7 @@ def get_config_state(configfile):
 
 
 def get_config_policy(configfile):
-    lines = get_file_lines(configfile)
+    lines = get_file_lines(configfile, strip=False)
 
     for line in lines:
         stateline = re.match(r'^SELINUXTYPE=.*$', line)
@@ -91,16 +123,19 @@ def get_config_policy(configfile):
 
 
 # setter subroutines
-def set_config_state(state, configfile):
+def set_config_state(module, state, configfile):
     # SELINUX=permissive
     # edit config file with state value
     stateline = 'SELINUX=%s' % state
+    lines = get_file_lines(configfile, strip=False)
 
-    lines = get_file_lines(configfile)
+    tmpfd, tmpfile = tempfile.mkstemp()
 
-    with open(configfile, "w") as write_file:
+    with open(tmpfile, "w") as write_file:
         for line in lines:
-            write_file.write(re.sub(r'^SELINUX=.*', stateline, line))
+            write_file.write(re.sub(r'^SELINUX=.*', stateline, line) + '\n')
+
+    module.atomic_move(tmpfile, configfile)
 
 
 def set_state(module, state):
@@ -115,15 +150,19 @@ def set_state(module, state):
         module.fail_json(msg=msg)
 
 
-def set_config_policy(policy, configfile):
+def set_config_policy(module, policy, configfile):
     # edit config file with state value
     # SELINUXTYPE=targeted
     policyline = 'SELINUXTYPE=%s' % policy
-    lines = get_file_lines(configfile)
+    lines = get_file_lines(configfile, strip=False)
 
-    with open(configfile, "w") as write_file:
+    tmpfd, tmpfile = tempfile.mkstemp()
+
+    with open(tmpfile, "w") as write_file:
         for line in lines:
-            write_file.write(re.sub(r'^SELINUXTYPE=.*', policyline, line))
+            write_file.write(re.sub(r'^SELINUXTYPE=.*', policyline, line) + '\n')
+
+    module.atomic_move(tmpfile, configfile)
 
 
 def main():
@@ -148,6 +187,7 @@ def main():
     runtime_enabled = selinux.is_selinux_enabled()
     runtime_policy = selinux.selinux_getpolicytype()[1]
     runtime_state = 'disabled'
+    reboot_required = False
 
     if runtime_enabled:
         # enabled means 'enforcing' or 'permissive'
@@ -167,7 +207,7 @@ def main():
     # check to see if policy is set if state is not 'disabled'
     if state != 'disabled':
         if not policy:
-            module.fail_json(msg='policy is required if state is not \'disabled\'')
+            module.fail_json(msg='Policy is required if state is not \'disabled\'')
     else:
         if not policy:
             policy = config_policy
@@ -177,14 +217,14 @@ def main():
         if module.check_mode:
             module.exit_json(changed=True)
         # cannot change runtime policy
-        msgs.append('reboot to change the loaded policy')
+        msgs.append('Running SELinux policy changed from \'%s\' to \'%s\'' % (runtime_policy, policy))
         changed = True
 
     if policy != config_policy:
         if module.check_mode:
             module.exit_json(changed=True)
-        msgs.append('config policy changed from \'%s\' to \'%s\'' % (config_policy, policy))
-        set_config_policy(policy, configfile)
+        set_config_policy(module, policy, configfile)
+        msgs.append('SELinux policy configuration in \'%s\' changed from \'%s\' to \'%s\'' % (configfile, config_policy, policy))
         changed = True
 
     if state != runtime_state:
@@ -195,26 +235,30 @@ def main():
                 if runtime_state != 'permissive':
                     # Temporarily set state to permissive
                     set_state(module, 'permissive')
-                    msgs.append('runtime state temporarily changed from \'%s\' to \'permissive\', state change will take effect next reboot' % (runtime_state))
+                    module.warn('SELinux state temporarily changed from \'%s\' to \'permissive\'. State change will take effect next reboot.' % (runtime_state))
                 else:
-                    msgs.append('state change will take effect next reboot')
+                    module.warn('SELinux state change will take effect next reboot')
+                reboot_required = True
             else:
                 set_state(module, state)
-                msgs.append('runtime state changed from \'%s\' to \'%s\'' % (runtime_state, state))
-        else:
-            msgs.append('state change will take effect next reboot')
+                msgs.append('SELinux state changed from \'%s\' to \'%s\'' % (runtime_state, state))
+
+                # Only report changes if the file is changed.
+                # This prevents the task from reporting changes every time the task is run.
                 changed = True
+        else:
+            module.warn("Reboot is required to set SELinux state to %s" % state)
+            reboot_required = True
 
     if state != config_state:
         if module.check_mode:
             module.exit_json(changed=True)
-        msgs.append('config state changed from \'%s\' to \'%s\'' % (config_state, state))
-        set_config_state(state, configfile)
+        msgs.append('Config SELinux state changed from \'%s\' to \'%s\'' % (config_state, state))
+        set_config_state(module, state, configfile)
         changed = True
 
-    module.exit_json(changed=changed, msg=', '.join(msgs), configfile=configfile, policy=policy, state=state)
+    module.exit_json(changed=changed, msg=', '.join(msgs), configfile=configfile, policy=policy, state=state, reboot_required=reboot_required)
 
-#################################################
 
 if __name__ == '__main__':
     main()

test/integration/targets/selinux/aliases

@@ -0,0 +1,2 @@
+needs/root
+posix/ci/group2

test/integration/targets/selinux/tasks/main.yml

@@ -0,0 +1,30 @@
+# (c) 2017, Sam Doran <sdoran@redhat.com>
+
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+- debug:
+    msg: SELinux is disabled
+  when: ansible_selinux is defined and ansible_selinux == False
+
+- debug:
+    msg: SELinux is {{ ansible_selinux.status }}
+  when: ansible_selinux is defined and ansible_selinux != False
+
+- include: selinux.yml
+  when:
+    - ansible_selinux is defined
+    - ansible_selinux != False
+    - ansible_selinux.status == 'enabled'

test/integration/targets/selinux/tasks/selinux.yml

@@ -0,0 +1,170 @@
+# (c) 2017, Sam Doran <sdoran@redhat.com>
+
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# First Test
+# ##############################################################################
+# Test changing the state, which requires a reboot
+
+- name: TEST 1 | Get current SELinux config file contents
+  set_fact:
+    selinux_config_original: "{{ lookup('file', '/etc/sysconfig/selinux').split('\n') }}"
+    before_test_sestatus: "{{ ansible_selinux }}"
+
+- debug:
+    var: "{{ item }}"
+    verbosity: 1
+  with_items:
+    - selinux_config_original
+    - before_test_sestatus
+    - ansible_selinux
+
+- name: TEST 1 | Setup SELinux configuration for tests
+  selinux:
+    state: enforcing
+    policy: targeted
+
+- name: TEST 1 | Disable SELinux
+  selinux:
+    state: disabled
+    policy: targeted
+  register: _disable_test1
+
+- debug:
+    var: _disable_test1
+    verbosity: 1
+
+- name: TEST 1 | Re-gather facts
+  setup:
+
+- name: TEST 1 | Assert that status was changed, reboot_required is True, a warning was displayed, and SELinux is configured properly
+  assert:
+    that:
+      - _disable_test1 | changed
+      - _disable_test1.reboot_required
+      - (_disable_test1.warnings | length ) >= 1
+      - ansible_selinux.config_mode == 'disabled'
+      - ansible_selinux.type == 'targeted'
+
+- debug:
+    var: ansible_selinux
+    verbosity: 1
+
+- name: TEST 1 | Disable SELinux again
+  selinux:
+    state: disabled
+    policy: targeted
+  register: _disable_test2
+
+- debug:
+    var: _disable_test2
+    verbosity: 1
+
+- name: TEST 1 | Assert that no change is reported, a warnking was dispalyed, and reboot_required is True
+  assert:
+    that:
+      - not _disable_test2 | changed
+      - (_disable_test1.warnings | length ) >= 1
+      - _disable_test2.reboot_required
+
+- name: TEST 1 | Get modified config file
+  set_fact:
+    selinux_config_after: "{{ lookup('file', '/etc/sysconfig/selinux').split('\n') }}"
+
+- debug:
+    var: selinux_config_after
+    verbosity: 1
+
+- name: TEST 1 | Ensure SELinux config file is properly formatted
+  assert:
+    that:
+      - selinux_config_original | length == selinux_config_after | length
+      - selinux_config_after[selinux_config_after.index('SELINUX=disabled')] | search("^SELINUX=\w+$")
+      - selinux_config_after[selinux_config_after.index('SELINUXTYPE=targeted')] | search("^SELINUXTYPE=\w+$")
+
+- name: TEST 1 | Reset SELinux configuration for next test
+  selinux:
+    state: enforcing
+    policy: targeted
+
+
+# Second Test
+# ##############################################################################
+# Test changing only the policy, which does not require a reboot
+
+- name: TEST 2 | Set SELinux policy
+  selinux:
+    state: enforcing
+    policy: mls
+  register: _state_test1
+
+- debug:
+    var: _state_test1
+    verbosity: 1
+
+- name: TEST 2 | Re-gather facts
+  setup:
+
+- debug:
+    var: ansible_selinux
+  tags: debug
+
+- name: TEST 2 | Assert that status was changed, reboot_required is False, no warnings were displayed, and SELinux is configured properly
+  assert:
+    that:
+      - _state_test1 | changed
+      - not _state_test1.reboot_required
+      - _state_test1.warnings is not defined
+      - ansible_selinux.config_mode == 'enforcing'
+      - ansible_selinux.type == 'mls'
+
+- name: TEST 2 | Set SELinux policy again
+  selinux:
+    state: enforcing
+    policy: mls
+  register: _state_test2
+
+- debug:
+    var: _state_test2
+    verbosity: 1
+
+- name: TEST 2 | Assert that no change was reported, no warnings were dispalyed, and reboot_required is False
+  assert:
+    that:
+      - not _state_test2 | changed
+      - _state_test2.warnings is not defined
+      - not _state_test2.reboot_required
+
+- name: TEST 2 | Get modified config file
+  set_fact:
+    selinux_config_after: "{{ lookup('file', '/etc/sysconfig/selinux').split('\n') }}"
+
+- debug:
+    var: selinux_config_after
+    verbosity: 1
+
+- name: TEST 2 | Ensure SELinux config file is properly formatted
+  assert:
+    that:
+      - selinux_config_original | length == selinux_config_after | length
+      - selinux_config_after[selinux_config_after.index('SELINUX=enforcing')] | search("^SELINUX=\w+$")
+      - selinux_config_after[selinux_config_after.index('SELINUXTYPE=mls')] | search("^SELINUXTYPE=\w+$")
+
+- name: TEST 2 | Reset SELinux configuration for next test
+  selinux:
+    state: enforcing
+    policy: targeted