Fix in sebool ansible #11245

rumch-se · 2023-11-03T10:18:53Z

Description:

Changes into template sebool - bash and ansible parts

Rationale:

1. SLE 15 requires some additional packages to be installed 2. The ansible part does not include a check if the targeting OS is SELinux state or not - because of that the FATAL error is generated and the execution of ansible playbook is terminated. 3. Some optimization of the code was done to be more readable

Review Hints:

Using automatus with template command is a good way to test changes in templates. e.g.:

./tests/automatus.py template --libvirt qemu:///session rhel9 --datastream build/ssg-rhel9-ds.xml --remediate-using ansible sebool

openshift-ci · 2023-11-03T10:19:04Z

Hi @rumch-se. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

github-actions · 2023-11-03T10:21:16Z

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment

Oracle Linux 8 Environment

marcusburghardt

Besides my comments regarding Bash and Ansible alignment, I wonder if installing packages in this template are not too much. It would be probably more flexible to create (or use the already existing rules) to install packages and keep this template focusing only in SELinux booleans.

shared/templates/sebool/ansible.template

shared/templates/sebool/bash.template

rumch-se · 2023-11-08T09:37:03Z

Hello @marcusburghardt
1/
According to this link https://docs.ansible.com/ansible/latest/collections/ansible/posix/seboolean_module.html
some packages are required
+++
The below requirements are needed on the host that executes this module.

libselinux-python
libsemanage-python
python3-libsemanage

+++
and SLE does not have these packages / instead it uses packages with different names. Because of that I added some additional block to install packages related to SUSE. The installation only of the package policycoreutils is not enough on SLE.

2/
In the current SUSE's profiles there is no rule/recommendation which covers the installation of packages related to SELinux. Do you know if there is a template which covers this - because the proposed change is related to the template - i.e. to set some bool values we have to cover the requirements about the installation of necessary packages.

3/
I did not change the bash template in the way to check if SELinux is enabled, but this check make sense. Any advice how to be done.

Have a nice day
Rumen

rumch-se · 2023-12-05T10:36:17Z

Hello @marcusburghardt

I have the following question. Will this PR be merged into the planed new release ?

Have a nice day
Rumen

marcusburghardt · 2023-12-05T14:18:57Z

Hello @marcusburghardt

I have the following question. Will this PR be merged into the planed new release ?

Have a nice day Rumen

Hi @rumch-se , thanks for pining me. I will try to review it again until tomorrow. Since the stabilization phase for 0.1.71 is already happening, it should be part of 0.1.72.

marcusburghardt

I reviewed again now and included some more comments. Could you take a look, please?

shared/templates/sebool/ansible.template

marcusburghardt · 2023-12-06T10:42:07Z

shared/templates/sebool/ansible.template

+    state: present
+
+{{% if product == "sle15" %}}
+- name: Ensure list of additional packages are installed on SLE 15


Since we are touching this Ansible file, could you also update the tasks names in alignment to the Project Style Guide, please?
https://complianceascode.readthedocs.io/en/latest/manual/developer/04_style_guide.html#ansible

marcusburghardt · 2023-12-06T10:42:29Z

shared/templates/sebool/ansible.template

+    state: present
+
+{{% if product == "sle15" %}}
+- name: Ensure list of additional packages are installed on SLE 15


Since we are touching this Ansible file, could you also update all tasks names in this Playbook in alignment to the Project Style Guide, please?
https://complianceascode.readthedocs.io/en/latest/manual/developer/04_style_guide.html#ansible

shared/templates/sebool/ansible.template

marcusburghardt · 2023-12-06T10:54:17Z

shared/templates/sebool/ansible.template

    name: {{{ SEBOOLID }}}
    state: "{{ var_{{{ SEBOOLID }}} }}"
    persistent: yes
+  when: ansible_facts.selinux.status == 'enabled'  


This test is used in two tasks. Maybe it would be simpler to use it once in a block and include the tasks inside this block. It would make the Playbook simpler and likely more performative.

shared/templates/sebool/bash.template

rumch-se · 2023-12-06T12:33:00Z

Hello @marcusburghardt
With my last commit I implemented all proposed changes without only the code block at the bottom of the ansible template. The main reasons for that

We have 2 different names of the tasks are different because of SEBOOL_BOOL
We have 2 different states of the tasks and before the second state we have definition of variable after the else
(xccdf-var var_{{{ SEBOOLID }}})
If I use jinja macros I have to set TASK_NAME, but for the set of STATE_NAME I have to define a variable before that .. and in for the 1st state we have "{{{ SEBOOL_BOOL }}}" , while for the second we have "{{ var_{{{ SEBOOLID }}} }}" where before we have to define (xccdf-var var_{{{ SEBOOLID }}}). I don't know how to manage this part

Have a nice day
Rumen

shared/templates/sebool/ansible.template

marcusburghardt · 2023-12-06T12:50:43Z

before we have to define (xccdf-var var_{{{ SEBOOLID }}}). I don't know how to manage this part

It seems more tasks (if not all) could be inserted in a block with when: ansible_facts.selinux.status == 'enabled' . But this is not critical and is not the original intention of this PR. So, I would be fine assess and update this in another PR.

Thanks for the update. I have only some other comments about Style Guide in this Ansible playbook.

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>

rumch-se · 2023-12-06T14:11:57Z

Hello @marcusburghardt
I have committed the proposed change
All the best
Rumen

marcusburghardt · 2023-12-08T09:13:55Z

shared/templates/sebool/ansible.template

+{{% set PACKAGE_NAME = "libsemanage-python" %}}
+{{% endif %}}
+
+- name: Ensure {{{ PACKAGE_NAME }}} installed


Suggested change

- name: Ensure {{{ PACKAGE_NAME }}} installed

- name: "{{{ rule_title }}} - Ensure {{{ PACKAGE_NAME }}} Installed"

marcusburghardt · 2023-12-08T09:14:15Z

shared/templates/sebool/ansible.template

+    state: present
+
+{{% if product == "sle15" %}}
+- name: "{{{ rule_title }}} - Ensure Additional Packages installed"


Suggested change

- name: "{{{ rule_title }}} - Ensure Additional Packages installed"

- name: "{{{ rule_title }}} - Ensure Additional Packages Installed"

marcusburghardt · 2023-12-08T09:14:40Z

shared/templates/sebool/ansible.template

+{{% endif %}}
+
 {{% if SEBOOL_BOOL %}}
 - name: Set SELinux boolean {{{ SEBOOLID }}} to {{{ SEBOOL_BOOL }}}


Suggested change

- name: Set SELinux boolean {{{ SEBOOLID }}} to {{{ SEBOOL_BOOL }}}

- name: "{{{ rule_title }}} - Set SELinux Boolean {{{ SEBOOLID }}} to {{{ SEBOOL_BOOL }}}"

marcusburghardt · 2023-12-08T09:15:12Z

shared/templates/sebool/ansible.template

-    name: libsemanage-python
-    state: present
-{{% endif %}}
 - name: Set SELinux boolean {{{ SEBOOLID }}} accordingly


Suggested change

- name: Set SELinux boolean {{{ SEBOOLID }}} accordingly

- name: "{{{ rule_title }}} - Set SELinux Boolean {{{ SEBOOLID }}} Accordingly"

marcusburghardt · 2023-12-08T09:29:53Z

Hello @marcusburghardt I have committed the proposed change All the best Rumen

Thanks @rumch-se, but you missed the other tasks in the Playbook. I proposed the all remaining changes to help you.
When updating Ansible Playbooks it is very appreciated to also review it entirely and make sure everything is aligned to the Style Guide.

rumch-se · 2023-12-08T09:51:06Z

Hello @marcusburghardt
I hope that I did all proposed changes.
Have a nice day
Rumen

marcusburghardt

Thanks @rumch-se

qlty-cloud-legacy · 2023-12-08T10:02:42Z

Code Climate has analyzed commit 4cbbaae and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 58.5%.

View more on Code Climate.

rumch-se added 2 commits November 3, 2023 11:09

Changes into template sebool - ansible part

c00946f

Changes into template sebool - bash part

6f4be79

openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Nov 3, 2023

Mab879 added Ansible Ansible remediation update. Bash Bash remediation update. labels Nov 3, 2023

Mab879 added this to the 0.1.71 milestone Nov 3, 2023

marcusburghardt requested changes Nov 8, 2023

View reviewed changes

shared/templates/sebool/ansible.template Show resolved Hide resolved

shared/templates/sebool/bash.template Show resolved Hide resolved

marcusburghardt self-assigned this Nov 8, 2023

Add check if SELinux is enabled in bash sebool remediation

65acdab

vojtapolasek modified the milestones: 0.1.71, 0.1.72 Nov 28, 2023

marcusburghardt requested changes Dec 6, 2023

View reviewed changes

Implementation of proposed changes

5d3cd66

marcusburghardt reviewed Dec 6, 2023

View reviewed changes

shared/templates/sebool/ansible.template Outdated Show resolved Hide resolved

Update shared/templates/sebool/ansible.template

eafa793

Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>

marcusburghardt requested changes Dec 8, 2023

View reviewed changes

Implementation of some additional changes in ansible

4cbbaae

marcusburghardt approved these changes Dec 8, 2023

View reviewed changes

marcusburghardt merged commit c8bdaa4 into ComplianceAsCode:master Dec 8, 2023

	- name: Ensure {{{ PACKAGE_NAME }}} installed
	- name: "{{{ rule_title }}} - Ensure {{{ PACKAGE_NAME }}} Installed"

	- name: "{{{ rule_title }}} - Ensure Additional Packages installed"
	- name: "{{{ rule_title }}} - Ensure Additional Packages Installed"

	- name: Set SELinux boolean {{{ SEBOOLID }}} to {{{ SEBOOL_BOOL }}}
	- name: "{{{ rule_title }}} - Set SELinux Boolean {{{ SEBOOLID }}} to {{{ SEBOOL_BOOL }}}"

Fix in sebool ansible #11245

Fix in sebool ansible #11245

Uh oh!

Conversation

rumch-se commented Nov 3, 2023 • edited by marcusburghardt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Rationale:

Review Hints:

Uh oh!

openshift-ci bot commented Nov 3, 2023

Uh oh!

github-actions bot commented Nov 3, 2023

Uh oh!

marcusburghardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rumch-se commented Nov 8, 2023

Uh oh!

rumch-se commented Dec 5, 2023

Uh oh!

marcusburghardt commented Dec 5, 2023

Uh oh!

marcusburghardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcusburghardt Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcusburghardt Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rumch-se commented Dec 6, 2023

Uh oh!

Uh oh!

marcusburghardt commented Dec 6, 2023

Uh oh!

rumch-se commented Dec 6, 2023

Uh oh!

marcusburghardt Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt commented Dec 8, 2023

Uh oh!

rumch-se commented Dec 8, 2023

Uh oh!

marcusburghardt left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rumch-se commented Nov 3, 2023 •

edited by marcusburghardt

Loading