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

configure_openssl_cryptopolicy: align remediations with rule description #10828

Conversation

vojtapolasek
Copy link
Collaborator

Description:

  • change the ".include" line which is used in remediations to be coherent with rule descriptions for different products

Rationale:

  • remediations were always using the include statement without the "=" sign, whereas rule description used different statements for different products. This PR aligns remediations with the description.

Review Hints:

  1. ./build_product rhel8 rhel9
  2. compare Bash and Ansible remediations for configure_openssl_crypto_policy between rhel8 and rhel9 products
  3. compare rule descriptions of configure_openssl_crypto_policy between rhel8 and rhel9 product
  4. descriptions should match remediations in respective products

@vojtapolasek vojtapolasek added bugfix Fixes to reported bugs. Ansible Ansible remediation update. Bash Bash remediation update. labels Jul 12, 2023
@vojtapolasek vojtapolasek added this to the 0.1.69 milestone Jul 12, 2023
@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 Jul 12, 2023

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

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy' differs.
--- xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy
+++ xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy
@@ -1,8 +1,11 @@
 
 OPENSSL_CRYPTO_POLICY_SECTION='[ crypto_policy ]'
 OPENSSL_CRYPTO_POLICY_SECTION_REGEX='\[\s*crypto_policy\s*\]'
-OPENSSL_CRYPTO_POLICY_INCLUSION='.include = /etc/crypto-policies/back-ends/opensslcnf.config'
+
+OPENSSL_CRYPTO_POLICY_INCLUSION='.include /etc/crypto-policies/back-ends/opensslcnf.config'
+
 OPENSSL_CRYPTO_POLICY_INCLUSION_REGEX='^\s*\.include\s*(?:=\s*)?/etc/crypto-policies/back-ends/opensslcnf.config$'
+
 
 
   

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy' differs.
--- xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy
+++ xccdf_org.ssgproject.content_rule_configure_openssl_crypto_policy
@@ -1,9 +1,10 @@
-- name: Test for crypto_policy group
-  command: grep '^\s*\[\s*crypto_policy\s*]' /etc/pki/tls/openssl.cnf
+- name: Configure OpenSSL library to use System Crypto Policy - Search for crypto_policy
+    Section
+  ansible.builtin.find:
+    paths: /etc/pki/tls
+    patterns: openssl.cnf
+    contains: ^\s*\[\s*crypto_policy\s*]
   register: test_crypto_policy_group
-  failed_when: test_crypto_policy_group.rc not in [0, 1]
-  changed_when: false
-  check_mode: false
   tags:
   - CCE-80938-4
   - DISA-STIG-RHEL-08-010293
@@ -23,15 +24,13 @@
   - no_reboot_needed
   - unknown_strategy
 
-- name: Add .include for opensslcnf.config to crypto_policy section
-  lineinfile:
-    create: true
-    insertafter: ^\s*\[\s*crypto_policy\s*]\s*
-    line: .include = /etc/crypto-policies/back-ends/opensslcnf.config
-    path: /etc/pki/tls/openssl.cnf
-  when:
-  - test_crypto_policy_group.stdout is defined
-  - test_crypto_policy_group.stdout | length > 0
+- name: Configure OpenSSL library to use System Crypto Policy - Search for crypto_policy
+    Section Together With .include Directive
+  ansible.builtin.find:
+    paths: /etc/pki/tls
+    patterns: openssl.cnf
+    contains: ^\s*\.include\s*(?:=\s*)?/etc/crypto-policies/back-ends/opensslcnf.config$
+  register: test_crypto_policy_include_directive
   tags:
   - CCE-80938-4
   - DISA-STIG-RHEL-08-010293
@@ -51,16 +50,16 @@
   - no_reboot_needed
   - unknown_strategy
 
-- name: Add crypto_policy group and set include opensslcnf.config
-  lineinfile:
+- name: '{{ rule_title }} - Add .include Line for opensslcnf.config File in crypto_policy
+    Section'
+  ansible.builtin.lineinfile:
     create: true
-    line: |-
-      [crypto_policy]
-      .include = /etc/crypto-policies/back-ends/opensslcnf.config
+    insertafter: ^\s*\[\s*crypto_policy\s*]\s*
+    line: .include /etc/crypto-policies/back-ends/opensslcnf.config
     path: /etc/pki/tls/openssl.cnf
   when:
-  - test_crypto_policy_group.stdout is defined
-  - test_crypto_policy_group.stdout | length < 1
+  - test_crypto_policy_group.matched > 0
+  - test_crypto_policy_include_directive.matched == 0
   tags:
   - CCE-80938-4
   - DISA-STIG-RHEL-08-010293
@@ -79,3 +78,31 @@
   - medium_severity
   - no_reboot_needed
   - unknown_strategy
+
+- name: Configure OpenSSL library to use System Crypto Policy - Add crypto_policy
+    Section With .include for opensslcnf.config File
+  ansible.builtin.lineinfile:
+    create: true
+    line: |-
+      [crypto_policy]
+      .include /etc/crypto-policies/back-ends/opensslcnf.config
+    path: /etc/pki/tls/openssl.cnf
+  when: test_crypto_policy_group.matched == 0
+  tags:
+  - CCE-80938-4
+  - DISA-STIG-RHEL-08-010293
+  - NIST-800-53-AC-17(2)
+  - NIST-800-53-AC-17(a)
+  - NIST-800-53-CM-6(a)
+  - NIST-800-53-MA-4(6)
+  - NIST-800-53-SC-12(2)
+  - NIST-800-53-SC-12(3)
+  - NIST-800-53-SC-13
+  - PCI-DSS-Req-2.2
+  - PCI-DSSv4-2.2
+  - configure_openssl_crypto_policy
+  - low_complexity
+  - medium_disruption
+  - medium_severity
+  - no_reboot_needed
+  - unknown_strategy

@marcusburghardt marcusburghardt self-assigned this Jul 13, 2023
Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

There is a concern about duplicating the configuration during the remediation.

@marcusburghardt
Copy link
Member

In a private discussion we saw benefits and drawbacks on aligning the syntax for all products, so the remediation would always ensure a line without the = signal,
like:
.include /etc/crypto-policies/back-ends/opensslcnf.config

instead of:
.include = /etc/crypto-policies/back-ends/opensslcnf.config

Both syntax are valid while the longer option (with =) is acceptable by compatibility purposes only. So, there is no technical impact on choosing any of them. The OVAL already accept both, but the remediation may differ based on product. I suggested to simplify and keep the same syntax everywhere. The drawbacks would be a misalignment with some benchmarks which explicitly ask a syntax. In short, if this is the case, the respective benchmarks should update their requirement descriptions to be indifferent to this matter, even if using one specific syntax as example.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

I have only some minor suggestions for the Ansible tasks names. They should follow the Title Case in alignment to our Style Guides.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

Thanks @vojtapolasek

@marcusburghardt
Copy link
Member

The CI tests were broken after the #10842 was merged and this PR is blocked by this.

In this particular PR, I have successfully tested it in local VMs.
Before the #10842 PR , CI tests also passed and after the #10842 PR, commits were only about Ansible tasks names, without technical impact in the remediation. Therefore, I am sure there won't be negative consequences to waive the required tests. It can be safely merged as it is, if necessary.

vojtapolasek and others added 5 commits July 14, 2023 16:37
do not remediate if the value is correct, accept the .include directive with or without = in this case
Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>
@vojtapolasek vojtapolasek force-pushed the align_remediations_configure_openssl_crypto_policy branch from 7d888a8 to d83e742 Compare July 14, 2023 14:37
@codeclimate
Copy link

codeclimate bot commented Jul 14, 2023

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

View more on Code Climate.

@marcusburghardt marcusburghardt merged commit 93f2967 into ComplianceAsCode:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update. Bash Bash remediation update. bugfix Fixes to reported bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants