Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use platform_package_overrides in bash and ansible macros #10626

Closed

Conversation

maage
Copy link
Contributor

@maage maage commented May 23, 2023

Description:

Use platform_package_overrides in bash_package_install and bash_package_remove. And it should be used in every path where packages are installed if at all feasible. Another way is to do it like https://github.com/maage/scap-security-guide/blob/08fa8789b71e0ce2863000bb957c56693942df7e/linux_os/guide/system/software/integrity/software-integrity/aide/aide_periodic_cron_checking/ansible/shared.yml#L9

Add ansible_package_install and it uses platform_package_overrides. Maybe it should handle one package at a time, but if you do it like that, running playbook takes forever. One issue is that testing list is not simplest of tasks.

Rationale:

Using this fully would simplify code I guess and avoid issues like: #10557 (comment)

Why even this was not already the case?

Review Hints:

This is generally not tested at all. At least datastream diff looks sane and it seems actually to fix things already.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Used by openshift-ci bot. needs-ok-to-test Used by openshift-ci bot. labels May 23, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 23, 2023

Hi @maage. 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.

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.

@maage maage mentioned this pull request May 23, 2023
@maage maage force-pushed the platform_package_overrides-2 branch 2 times, most recently from 099974d to 08fa878 Compare May 23, 2023 20:59
@github-actions
Copy link

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

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@github-actions
Copy link

github-actions bot commented May 23, 2023

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
ansible remediation for rule 'xccdf_org.ssgproject.content_rule_aide_build_database' differs.
--- xccdf_org.ssgproject.content_rule_aide_build_database
+++ xccdf_org.ssgproject.content_rule_aide_build_database
@@ -1,9 +1,8 @@
-- name: Ensure AIDE is installed
-  package:
-    name: '{{ item }}'
+- name: Build and Test AIDE Database - Ensure AIDE Package Is Installed
+  ansible.builtin.package:
+    name:
+    - aide
     state: present
-  with_items:
-  - aide
   when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-80675-2

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_aide_check_audit_tools' differs.
--- xccdf_org.ssgproject.content_rule_aide_check_audit_tools
+++ xccdf_org.ssgproject.content_rule_aide_check_audit_tools
@@ -1,9 +1,8 @@
-- name: Ensure aide is installed
-  package:
-    name: '{{ item }}'
+- name: Configure AIDE to Verify the Audit Tools - Ensure AIDE Package Is Installed
+  ansible.builtin.package:
+    name:
+    - aide
     state: present
-  with_items:
-  - aide
   when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-85964-5

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_aide_periodic_cron_checking' differs.
--- xccdf_org.ssgproject.content_rule_aide_periodic_cron_checking
+++ xccdf_org.ssgproject.content_rule_aide_periodic_cron_checking
@@ -1,5 +1,5 @@
-- name: Ensure AIDE is installed
-  package:
+- name: Configure Periodic Execution of AIDE - Ensure AIDE Packages Are Installed
+  ansible.builtin.package:
     name:
     - aide
     - crontabs

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_aide_scan_notification' differs.
--- xccdf_org.ssgproject.content_rule_aide_scan_notification
+++ xccdf_org.ssgproject.content_rule_aide_scan_notification
@@ -4,8 +4,9 @@
   tags:
     - always
 
-- name: Ensure AIDE is installed
-  package:
+- name: Configure Notification of Post-AIDE Scan Details - Ensure AIDE Packages Are
+    Installed
+  ansible.builtin.package:
     name:
     - aide
     - crontabs

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_passwords_pam_faillock_dir' differs.
--- xccdf_org.ssgproject.content_rule_accounts_passwords_pam_faillock_dir
+++ xccdf_org.ssgproject.content_rule_accounts_passwords_pam_faillock_dir
@@ -641,14 +641,13 @@
   - medium_severity
   - no_reboot_needed
 
-- name: Lock Accounts Must Persist - Ensure necessary SELinux packages are installed
+- name: Lock Accounts Must Persist - Ensure necessary SELinux Packages Are Installed
   ansible.builtin.package:
-    name: '{{ item }}'
+    name:
+    - python3-libselinux
+    - python3-policycoreutils
+    - policycoreutils-python-utils
     state: present
-  with_items:
-  - python3-libselinux
-  - python3-policycoreutils
-  - policycoreutils-python-utils
   when: '"pam" in ansible_facts.packages'
   tags:
   - CCE-86067-6

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_wireless_disable_interfaces' differs.
--- xccdf_org.ssgproject.content_rule_wireless_disable_interfaces
+++ xccdf_org.ssgproject.content_rule_wireless_disable_interfaces
@@ -20,12 +20,12 @@
   - unknown_strategy
   - wireless_disable_interfaces
 
-- name: Ensure NetworkManager is installed
+- name: Deactivate Wireless Network Interfaces - Ensure the NetworkManager Package
+    Is Installed
   ansible.builtin.package:
-    name: '{{ item }}'
+    name:
+    - NetworkManager
     state: present
-  with_items:
-  - NetworkManager
   tags:
   - CCE-83501-7
   - DISA-STIG-RHEL-08-040110

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled' differs.
--- xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
+++ xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
@@ -4,14 +4,13 @@
   tags:
     - always
 
-- name: Enable SSH Server firewalld Firewall Exception - Ensure firewalld and NetworkManager
-    packages are installed
+- name: Enable SSH Server firewalld Firewall Exception - Ensure the firewalld and
+    NetworkManager Packages Are Installed
   ansible.builtin.package:
-    name: '{{ item }}'
+    name:
+    - firewalld
+    - NetworkManager
     state: present
-  with_items:
-  - firewalld
-  - NetworkManager
   when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
   tags:
   - CCE-80820-4

bash remediation for rule 'xccdf_org.ssgproject.content_rule_package_sssd_installed' differs.
--- xccdf_org.ssgproject.content_rule_package_sssd_installed
+++ xccdf_org.ssgproject.content_rule_package_sssd_installed
@@ -1,8 +1,8 @@
 # Remediation is applicable only in certain platforms
 if rpm --quiet -q sssd-common; then
 
-if ! rpm -q --quiet "sssd" ; then
-    yum install -y "sssd"
+if ! rpm -q --quiet "sssd-common" ; then
+    yum install -y "sssd-common"
 fi
 
 else

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

A couple of changes to align to the style guide.

shared/macros/10-ansible.jinja Outdated Show resolved Hide resolved
shared/macros/10-ansible.jinja Outdated Show resolved Hide resolved
@maage maage force-pushed the platform_package_overrides-2 branch 2 times, most recently from a72fdc7 to 7783cb1 Compare May 24, 2023 06:35
@maage
Copy link
Contributor Author

maage commented May 24, 2023

Fixed issues raised in review. Added doc to ansible_package_install, used modern python typing format for types.

@jan-cerny
Copy link
Collaborator

@maage This is great stuff! Is it still a work in progress or do you want to click on "Ready for review"?

@jan-cerny jan-cerny self-assigned this Jun 15, 2023
@jan-cerny jan-cerny added this to the 0.1.69 milestone Jun 15, 2023
@jan-cerny
Copy link
Collaborator

@maage bump ^^

@maage maage force-pushed the platform_package_overrides-2 branch from 7783cb1 to cfb1cab Compare June 19, 2023 19:58
@maage
Copy link
Contributor Author

maage commented Jun 19, 2023

Now added other ansible package: sections, but aide_periodic_cron_checking has this cron_pkg_name setup and I am not going to extend this PR to cover that. Some minor issue with py2 needed fixing in my 1st take.

@maage maage force-pushed the platform_package_overrides-2 branch 2 times, most recently from 601b204 to b630196 Compare June 19, 2023 20:09
@maage maage force-pushed the platform_package_overrides-2 branch from b630196 to bb6bcec Compare June 19, 2023 20:17
@codeclimate
Copy link

codeclimate bot commented Jun 19, 2023

Code Climate has analyzed commit bb6bcec 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 52.9% (0.0% change).

View more on Code Climate.

@maage maage marked this pull request as ready for review June 20, 2023 06:11
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jun 20, 2023
{{%- if name is none %}}
{{%- set name = "the " ~ (packages | join_comma_and) -%}}
{{%- endif %}}
- name: "{{{ rule_title }}} - Ensure {{{ name }}} {{% if packages | length > 1 %}}Packages Are{{% else %}}Package Is{{% endif %}} Installed"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if the overridden package name would be also used in the task name.

state: present
with_items:
- NetworkManager
{{{ ansible_package_install(["NetworkManager"]) }}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attention at the task below this. The task uses "when:" statement where the package name will have to be overridden as well

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jun 23, 2023
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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.

@jan-cerny
Copy link
Collaborator

@maage please check the review above and please resolve the conflicts.

@vojtapolasek vojtapolasek modified the milestones: 0.1.69, 0.1.70 Jul 18, 2023
@jan-cerny
Copy link
Collaborator

Closing for no reaction for more than a month. If you still plan to work on this PR, please address the feedback and reopen the PR.

@jan-cerny jan-cerny closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot. needs-rebase Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants