From 00df1fda1072223580022a9e42d25fba393c494a Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Tue, 3 Oct 2017 23:38:58 -0400 Subject: [PATCH] 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 | 4 +- lib/ansible/modules/system/selinux.py | 96 +++++++--- test/integration/targets/selinux/aliases | 2 + .../targets/selinux/tasks/main.yml | 30 ++++ .../targets/selinux/tasks/selinux.yml | 170 ++++++++++++++++++ 5 files changed, 274 insertions(+), 28 deletions(-) create mode 100644 test/integration/targets/selinux/aliases create mode 100644 test/integration/targets/selinux/tasks/main.yml create mode 100644 test/integration/targets/selinux/tasks/selinux.yml diff --git a/lib/ansible/module_utils/facts/utils.py b/lib/ansible/module_utils/facts/utils.py index 2446ae678c..728934c1a5 100644 --- a/lib/ansible/module_utils/facts/utils.py +++ b/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: diff --git a/lib/ansible/modules/system/selinux.py b/lib/ansible/modules/system/selinux.py index 7cd4011284..7d1cb8d9fa 100644 --- a/lib/ansible/modules/system/selinux.py +++ b/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', - 'status': ['stableinterface'], - 'supported_by': 'core'} +ANSIBLE_METADATA = { + 'metadata_version': '1.1', + 'status': ['stableinterface'], + '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)) + 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: - msgs.append('state change will take effect next reboot') - changed = True + 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() diff --git a/test/integration/targets/selinux/aliases b/test/integration/targets/selinux/aliases new file mode 100644 index 0000000000..53b32510a0 --- /dev/null +++ b/test/integration/targets/selinux/aliases @@ -0,0 +1,2 @@ +needs/root +posix/ci/group2 diff --git a/test/integration/targets/selinux/tasks/main.yml b/test/integration/targets/selinux/tasks/main.yml new file mode 100644 index 0000000000..dc3e6781e4 --- /dev/null +++ b/test/integration/targets/selinux/tasks/main.yml @@ -0,0 +1,30 @@ +# (c) 2017, Sam Doran + +# 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 . + +- 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' diff --git a/test/integration/targets/selinux/tasks/selinux.yml b/test/integration/targets/selinux/tasks/selinux.yml new file mode 100644 index 0000000000..ff8b2fa159 --- /dev/null +++ b/test/integration/targets/selinux/tasks/selinux.yml @@ -0,0 +1,170 @@ +# (c) 2017, Sam Doran + +# 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 . + + +# 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