-
Notifications
You must be signed in to change notification settings - Fork 706
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
Refactor firewalld_sshd_port_enabled rule #9712
Refactor firewalld_sshd_port_enabled rule #9712
Conversation
94ccc4b
to
8f0648c
Compare
This datastream diff is auto generated by the check Click here to see the full diffNew content has different text for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
--- xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
+++ xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
@@ -3,9 +3,9 @@
Enable SSH Server firewalld Firewall Exception
[description]:
-By default, inbound connections to SSH's port are allowed. If
-the SSH server is being used but denied by the firewall, this exception should
-be added to the firewall configuration.
+If the SSH server is in use, inbound connections to SSH's port should be allowed to permit
+remote access through SSH. In more restrictive firewalld settings, the SSH port should be
+added to the proper firewalld zone in order to allow SSH remote access.
@@ -15,6 +15,23 @@
Then run the following command to load the newly created rule(s):
firewall-cmd --reload
+
+[warning]:
+The remediation for this rule uses firewall-cmd and nmcli tools.
+Therefore, it will only be executed if firewalld and NetworkManager
+services are running. Otherwise, the remediation will be aborted and a informative message
+will be shown in the remediation report.
+These respective services will not be started in order to preserve any intentional change
+in network components related to firewall and network interfaces.
+
+[warning]:
+This rule also checks if the SSH port was modified by the administrator in the firewalld
+services definitions and is reflecting the expected port number. Although this is checked,
+fixing the custom ssh.xml file placed by the administrator at /etc/firewalld/services it
+is not in the scope of the remediation since there is no reliable way to manually change
+the respective file. If the default SSH port is modified, it is on the administrator
+responsibility to ensure the firewalld customizations in the service port level are
+properly configured.
[reference]:
3.1.12
@@ -38,7 +55,7 @@
SRG-OS-000096-GPOS-00050
[rationale]:
-If inbound SSH connections are expected, adding a firewall rule exception
+If inbound SSH connections are expected, adding the SSH port to the proper firewalld zone
will allow remote access through the SSH port.
[ident]:
OVAL for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled' differs.
--- oval:ssg-firewalld_sshd_port_enabled:def:1
+++ oval:ssg-firewalld_sshd_port_enabled:def:1
@@ -1,7 +1,10 @@
+criteria AND
+criterion oval:ssg-test_firewalld_sshd_port_enabled_all_nics_in_zones:tst:1
criteria OR
-criterion oval:ssg-test_firewalld_service_sshd_enabled:tst:1
-criterion oval:ssg-test_firewalld_service_sshd_port_enabled:tst:1
criteria AND
-criterion oval:ssg-test_firewalld_zone_sshd_enabled:tst:1
-criterion oval:ssg-test_nic_assigned_to_sshd_enabled_zone:tst:1
-criterion oval:ssg-test_firewalld_zone_sshd_port_enabled:tst:1
+criterion oval:ssg-test_firewalld_sshd_port_enabled_zone_ssh_enabled_usr:tst:1
+criterion oval:ssg-test_firewalld_sshd_port_enabled_usr_zones_not_overridden:tst:1
+criterion oval:ssg-test_firewalld_sshd_port_enabled_zone_ssh_enabled_etc:tst:1
+criteria AND
+criterion oval:ssg-test_firewalld_sshd_port_enabled_ssh_service_usr:tst:1
+criterion oval:ssg-test_firewalld_sshd_port_enabled_ssh_service_etc:tst:1
OCIL for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled' differs.
--- ocil:ssg-firewalld_sshd_port_enabled_ocil:questionnaire:1
+++ ocil:ssg-firewalld_sshd_port_enabled_ocil:questionnaire:1
@@ -17,5 +17,5 @@
If it is a port:
22/tcp
- Is it the case that sshd service is disabled by firewall?
+ Is it the case that sshd service is not enabled in the proper firewalld zone?
bash 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,73 +4,43 @@
if ! rpm -q --quiet "firewalld" ; then
yum install -y "firewalld"
fi
-
+if ! rpm -q --quiet "NetworkManager" ; then
+ yum install -y "NetworkManager"
+fi
firewalld_sshd_zone=''
+if systemctl is-active NetworkManager && systemctl is-active firewalld; then
+ # First make sure the SSH service is enabled in run-time for the proper zone.
+ # This is to avoid connection issues when new interfaces are addeded to this zone.
+ firewall-cmd --zone="$firewalld_sshd_zone" --add-service=ssh
-
+ # This will collect all NetworkManager connections names
+ readarray -t nm_connections < <(nmcli -f UUID,TYPE con | grep ethernet | awk '{ print $1 }')
+ # If the connection is not yet assigned to a firewalld zone, assign it to the proper zone.
+ # This will not change connections which are already assigned to any firewalld zone.
+ for connection in "${nm_connections[@]}"; do
+ current_zone=$(nmcli -f connection.zone connection show "$connection" | awk '{ print $2}')
+ if [ $current_zone = "--" ]; then
+ nmcli connection modify "$connection" connection.zone $firewalld_sshd_zone
+ fi
+ done
+ systemctl restart NetworkManager
-
-# This assumes that firewalld_sshd_zone is one of the pre-defined zones
-if [ ! -f "/etc/firewalld/zones/${firewalld_sshd_zone}.xml" ]; then
- cp "/usr/lib/firewalld/zones/${firewalld_sshd_zone}.xml" "/etc/firewalld/zones/${firewalld_sshd_zone}.xml"
-fi
-if ! grep -q 'service name="ssh"' "/etc/firewalld/zones/${firewalld_sshd_zone}.xml"; then
- sed -i '/<\/description>/a \
- <service name="ssh"/>' "/etc/firewalld/zones/${firewalld_sshd_zone}.xml"
-fi
-
-# Check if any eth interface is bounded to the zone with SSH service enabled
-nic_bound=false
-readarray -t eth_interface_list < <(ip link show up | cut -d ' ' -f2 | cut -d ':' -s -f1 | grep -E '^(en|eth)')
-for interface in "${eth_interface_list[@]}"; do
- if grep -qi "ZONE=$firewalld_sshd_zone" "/etc/sysconfig/network-scripts/ifcfg-${interface}"; then
- nic_bound=true
- break;
- fi
-done
-
-if [ $nic_bound = false ];then
- # Add first NIC to SSH enabled zone
- interface="${eth_interface_list[0]}"
-
- if ! firewall-cmd --state -q; then
-
- # Test if the config_file is a symbolic link. If so, use --follow-symlinks with sed.
- # Otherwise, regular sed command will do.
- sed_command=('sed' '-i')
- if test -L "/etc/sysconfig/network-scripts/ifcfg-${interface}"; then
- sed_command+=('--follow-symlinks')
- fi
-
- # Strip any search characters in the key arg so that the key can be replaced without
- # adding any search characters to the config file.
- stripped_key=$(sed 's/[\^=\$,;+]*//g' <<< "^ZONE=")
-
- # shellcheck disable=SC2059
- printf -v formatted_output "%s=%s" "$stripped_key" "$firewalld_sshd_zone"
-
- # If the key exists, change it. Otherwise, add it to the config_file.
- # We search for the key string followed by a word boundary (matched by \>),
- # so if we search for 'setting', 'setting2' won't match.
- if LC_ALL=C grep -q -m 1 -i -e "^ZONE=\\>" "/etc/sysconfig/network-scripts/ifcfg-${interface}"; then
- escaped_formatted_output=$(sed -e 's|/|\\/|g' <<< "$formatted_output")
- "${sed_command[@]}" "s/^ZONE=\\>.*/$escaped_formatted_output/gi" "/etc/sysconfig/network-scripts/ifcfg-${interface}"
- else
- # \n is precaution for case where file ends without trailing newline
- cce="CCE-80820-4"
- printf '\n# Per %s: Set %s in %s\n' "$cce" "$formatted_output" "/etc/sysconfig/network-scripts/ifcfg-${interface}" >> "/etc/sysconfig/network-scripts/ifcfg-${interface}"
- printf '%s\n' "$formatted_output" >> "/etc/sysconfig/network-scripts/ifcfg-${interface}"
- fi
-
- else
- # If firewalld service is running, we need to do this step with firewall-cmd
- # Otherwise firewalld will communicate with NetworkManage and will revert assigned zone
- # of NetworkManager managed interfaces upon reload
- firewall-cmd --permanent --zone="$firewalld_sshd_zone" --add-interface="${eth_interface_list[0]}"
- firewall-cmd --reload
- fi
+ # Active zones are zones with at least one interface assigned to it.
+ # It is possible that traffic is comming by any active interface and consequently any
+ # active zone. So, this make sure all active zones are permanently allowing SSH service.
+ readarray -t firewalld_active_zones < <(firewall-cmd --get-active-zones | grep -v interfaces)
+ for zone in "${firewalld_active_zones[@]}"; do
+ firewall-cmd --permanent --zone="$zone" --add-service=ssh
+ done
+ firewall-cmd --reload
+else
+ echo "
+ firewalld and NetworkManager services are not active. Remediation aborted!
+ This remediation could not be applied because it depends on firewalld and NetworkManager services running.
+ The service is not started by this remediation in order to prevent connection issues."
+ exit 1
fi
else
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
@@ -1,9 +1,17 @@
-- name: Ensure firewalld is installed
- package:
+- name: XCCDF Value firewalld_sshd_zone # promote to variable
+ set_fact:
+ firewalld_sshd_zone: !!str
+ tags:
+ - always
+
+- name: Enable SSH Server firewalld Firewall Exception - Ensure firewalld and NetworkManager
+ packages are installed
+ ansible.builtin.package:
name: '{{ item }}'
state: present
with_items:
- firewalld
+ - NetworkManager
when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
tags:
- CCE-80820-4
@@ -18,20 +26,12 @@
- low_disruption
- medium_severity
- no_reboot_needed
-- name: XCCDF Value sshd_listening_port # promote to variable
- set_fact:
- sshd_listening_port: !!str
- tags:
- - always
-- name: Enable SSHD in firewalld (custom port)
- firewalld:
- port: '{{ sshd_listening_port }}/tcp'
- permanent: true
- state: enabled
- when:
- - ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
- - sshd_listening_port != 22
+- name: Enable SSH Server firewalld Firewall Exception - Collect facts about system
+ services
+ ansible.builtin.service_facts: null
+ register: result_services_states
+ when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
tags:
- CCE-80820-4
- NIST-800-171-3.1.12
@@ -46,14 +46,75 @@
- medium_severity
- no_reboot_needed
-- name: Enable SSHD in firewalld (default port)
- firewalld:
- service: ssh
- permanent: true
- state: enabled
+- name: Enable SSH Server firewalld Firewall Exception - Remediation is applicable
+ if firewalld and NetworkManager services are running
+ block:
+
+ - name: Enable SSH Server firewalld Firewall Exception - Collect NetworkManager
+ connections names
+ ansible.builtin.shell:
+ cmd: nmcli -f UUID,TYPE con | grep ethernet | awk '{ print $1 }'
+ register: result_nmcli_cmd_connections_names
+ changed_when: false
+
+ - name: Enable SSH Server firewalld Firewall Exception - Collect NetworkManager
+ connections zones
+ ansible.builtin.shell:
+ cmd: nmcli -f connection.zone connection show {{ item | trim }} | awk '{ print
+ $2}'
+ register: result_nmcli_cmd_connections_zones
+ changed_when: false
+ with_items:
+ - '{{ result_nmcli_cmd_connections_names.stdout_lines }}'
+
+ - name: Enable SSH Server firewalld Firewall Exception - Ensure NetworkManager connections
+ are assigned to a firewalld zone
+ ansible.builtin.command:
+ cmd: nmcli connection modify {{ item.0 }} connection.zone {{ firewalld_sshd_zone
+ }}
+ register: result_nmcli_cmd_connections_assignment
+ with_together:
+ - '{{ result_nmcli_cmd_connections_names.stdout_lines }}'
+ - '{{ result_nmcli_cmd_connections_zones.results }}'
+ when:
+ - item.1.stdout == '--'
+
+ - name: Enable SSH Server firewalld Firewall Exception - Ensure NetworkManager connections
+ changes are applied
+ ansible.builtin.service:
+ name: NetworkManager
+ state: restarted
+ when:
+ - result_nmcli_cmd_connections_assignment is changed
+
+ - name: Enable SSH Server firewalld Firewall Exception - Collect firewalld active
+ zones
+ ansible.builtin.shell:
+ cmd: firewall-cmd --get-active-zones | grep -v interfaces
+ register: result_firewall_cmd_zones_names
+ changed_when: false
+
+ - name: Enable SSH Server firewalld Firewall Exception - Ensure firewalld zones
+ allow SSH
+ ansible.builtin.command:
+ cmd: firewall-cmd --permanent --zone={{ item }} --add-service=ssh
+ register: result_nmcli_cmd_connections_assignment
+ changed_when:
+ - '''ALREADY_ENABLED'' not in result_nmcli_cmd_connections_assignment.stderr'
+ with_items:
+ - '{{ result_firewall_cmd_zones_names.stdout_lines }}'
+
+ - name: Enable SSH Server firewalld Firewall Exception - Ensure firewalld changes
+ are applied
+ ansible.builtin.service:
+ name: firewalld
+ state: reloaded
+ when:
+ - result_nmcli_cmd_connections_assignment is changed
when:
- ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
- - sshd_listening_port == 22
+ - ansible_facts.services['firewalld.service'].state == 'running'
+ - ansible_facts.services['NetworkManager.service'].state == 'running'
tags:
- CCE-80820-4
- NIST-800-171-3.1.12
@@ -67,3 +128,32 @@
- low_disruption
- medium_severity
- no_reboot_needed
+
+- name: Enable SSH Server firewalld Firewall Exception - Informative message based
+ on services states
+ ansible.builtin.assert:
+ that:
+ - ansible_facts.services['firewalld.service'].state == 'running'
+ - ansible_facts.services['NetworkManager.service'].state == 'running'
+ fail_msg:
+ - firewalld and NetworkManager services are not active. Remediation aborted!
+ - This remediation could not be applied because it depends on firewalld and NetworkManager
+ services running.
+ - The service is not started by this remediation in order to prevent connection
+ issues.
+ success_msg:
+ - Enable SSH Server firewalld Firewall Exception remediation successfully executed
+ when: ansible_virtualization_type not in ["docker", "lxc", "openvz", "podman", "container"]
+ tags:
+ - CCE-80820-4
+ - NIST-800-171-3.1.12
+ - NIST-800-53-AC-17(a)
+ - NIST-800-53-CM-6(b)
+ - NIST-800-53-CM-7(a)
+ - NIST-800-53-CM-7(b)
+ - configure_strategy
+ - firewalld_sshd_port_enabled
+ - low_complexity
+ - low_disruption
+ - medium_severity
+ - no_reboot_needed
Platform has been changed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'
--- xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
+++ xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
@@ -1 +1 @@
-
+cpe:/a:machine |
b68020f
to
7c717b6
Compare
The The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really excellent! I also like the verbosity using comments.
linux_os/guide/services/ssh/ssh_server/firewalld_sshd_port_enabled/bash/shared.sh
Outdated
Show resolved
Hide resolved
linux_os/guide/services/ssh/ssh_server/firewalld_sshd_port_enabled/rule.yml
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,34 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a RHEL 8 VM back end I got
INFO - Script only_zones_configured.fail.sh using profile (all) OK
ERROR - Rule evaluation resulted in error, instead of expected fixed during remediation stage
ERROR - The remediation failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
Did you encounter the same issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have this issue, but I believe it is related to my last commit. The remediation uses the nmcli
command to collect the connection names. However, the output of this command was bringing connection names with extra spaces. Could you try again with the last commit I have just sent, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now it's all green:
[jcerny@thinkpad scap-security-guide{pr/9712}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel8 firewalld_sshd_port_enabled
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-10-20-1143/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
INFO - Script customized_zone_configured.pass.sh using profile (all) OK
INFO - Script customized_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script new_zone_configured.pass.sh using profile (all) OK
INFO - Script new_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script only_nics_configured.fail.sh using profile (all) OK
INFO - Script only_zones_configured.fail.sh using profile (all) OK
INFO - Script zones_and_nics_configured.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_no_custom_files.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_port_changed.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/9712}]$ python3 tests/automatus.py rule --remediate-using ansible --libvirt qemu:///system ssgts_rhel8 firewalld_sshd_port_enabled
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-10-20-1149/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
INFO - Script customized_zone_configured.pass.sh using profile (all) OK
INFO - Script customized_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script new_zone_configured.pass.sh using profile (all) OK
INFO - Script new_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script only_nics_configured.fail.sh using profile (all) OK
INFO - Script only_zones_configured.fail.sh using profile (all) OK
INFO - Script zones_and_nics_configured.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_no_custom_files.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_port_changed.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/9712}]$ python3 tests/automatus.py rule --libvirt qemu:///system ssgts_rhel9 firewalld_sshd_port_enabled
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-10-20-1157/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
INFO - Script customized_zone_configured.pass.sh using profile (all) OK
INFO - Script customized_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script new_zone_configured.pass.sh using profile (all) OK
INFO - Script new_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script only_nics_configured.fail.sh using profile (all) OK
INFO - Script only_zones_configured.fail.sh using profile (all) OK
INFO - Script zones_and_nics_configured.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_no_custom_files.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_port_changed.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/9712}]$ python3 tests/automatus.py rule --remediate-using ansible --libvirt qemu:///system ssgts_rhel9 firewalld_sshd_port_enabled
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2022-10-20-1328/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
INFO - Script customized_zone_configured.pass.sh using profile (all) OK
INFO - Script customized_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script new_zone_configured.pass.sh using profile (all) OK
INFO - Script new_zone_without_ssh.fail.sh using profile (all) OK
INFO - Script only_nics_configured.fail.sh using profile (all) OK
INFO - Script only_zones_configured.fail.sh using profile (all) OK
INFO - Script zones_and_nics_configured.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_no_custom_files.pass.sh using profile (all) OK
INFO - Script zones_and_nics_ok_port_changed.pass.sh using profile (all) OK
[jcerny@thinkpad scap-security-guide{pr/9712}]$
|
It looks like it didn't help, maybe the infrastructure in testing-farm has some special network setup? We can also consider disabling this rule in the testing-farm. |
I couldn't reproduce the issue locally but yesterday I analysed the upstream reports and it seems the CentOS VM somehow doesn't have NetworkManager key files present. This is weird but to confirm I would need to reproduce this specific test environment. My plan is to confirm it and have a conclusion on Monday. |
It was also included the platform section since the scope of this rule is only applicable to machines and not to containers.
The remediation was not capable to properly treat some special cases, like a system with multiple interfaces. It wasn't also capable to safely configure the correct interface since it was assuming the NetworkManager connection file was prefixed with the network interface name. In addition, it is not stable to manually change firewalld XML files while a proper command is present. This commit makes the remediation reliable and assertive by using firewall-cmd and nmcli commands.
The test scenarios were aligned to the old remediation approach, making them also incomplete and incapable to catch real cases. Once the remediation was robust, test scenarios also need the same level of robustness in order to ensure the rules is as much realistic as possible. They are now covering cases with multiple interfaces and multiple active zones. It is also covered custom SSH port.
There are some corner cases involving possible realistic scenarios with firewalld and NetworkManager. Based on the remediation refactoring, the OVAL assessment was also reformulated to be more simple and much more reliable. It is now checking firewalld packaged files and also custom files respecting the proper order in case of custom files.
The previous remediation, besides being disaligned to the previous bash remediation, was also problematic. It was completly rewritten in this commit in order to be aligned to the Bash remediation. It was also enabled this Ansible remediation for all platforms, including RHEL9.
The output from nmcli command was including leading spaces in the connection names. This was causing the the subsequent nmcli command to fail resulting in connections without a firewalld zone defined.
210342a
to
ae72cf2
Compare
/retest |
Wow! The error was legitimate and was happening because the VM had more NetworkManager |
|
directory. So, its necessary to ensure the default file is interger and the, if is the | ||
case, the customized service is properly configured. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean the default file is interger
? I thought there is a typo and the word should be integer
but it still doesn't make sense to me.
Also there is a unnecessary the
in the sentence (the one before comma).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed there is a typo there. Thanks for noticing.
By default file
it means the file in /usr/lib/firewalld
dir, which is part of the package. This file should not be modified. If there is a intention to modify the default ssh port, the proper way is to create a modified file with the same name within /etc/firewalld
.
By the way, I believe this comment was created very early, when I started to work on this rule and I didn't review it before sending the PR. I think its wording can be improved too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the wording could be improved, it would be great :)
Not saying the wording is bad, but it would make it easier for person who doesn't have knowledge of the topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RHEL 8 and 9 tests pass for both remediations. But RHEL7 is not remediated correctly:
$ python3 tests/automatus.py rule --libvirt qemu:///session test-suite-rhel7 --datastream build/ssg-rhel7-ds.xml firewalld_sshd_port_enabled
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/mlysonek/SCAP/content/logs/rule-custom-2022-10-25-1322/test_suite.log
INFO - xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled
ERROR - Script customized_zone_configured.pass.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in fail, instead of expected pass during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
INFO - Script customized_zone_without_ssh.fail.sh using profile (all) OK
ERROR - Rule evaluation resulted in error, instead of expected fixed during remediation stage
ERROR - The remediation failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
ERROR - Script new_zone_configured.pass.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in fail, instead of expected pass during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
INFO - Script new_zone_without_ssh.fail.sh using profile (all) OK
ERROR - Rule evaluation resulted in error, instead of expected fixed during remediation stage
ERROR - The remediation failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
INFO - Script only_nics_configured.fail.sh using profile (all) OK
ERROR - Rule evaluation resulted in error, instead of expected fixed during remediation stage
ERROR - The remediation failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
INFO - Script only_zones_configured.fail.sh using profile (all) OK
ERROR - Rule evaluation resulted in error, instead of expected fixed during remediation stage
ERROR - The remediation failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
ERROR - Script zones_and_nics_configured.pass.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in fail, instead of expected pass during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
ERROR - Script zones_and_nics_ok_no_custom_files.pass.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in fail, instead of expected pass during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
ERROR - Script zones_and_nics_ok_port_changed.pass.sh using profile (all) found issue:
ERROR - Rule evaluation resulted in fail, instead of expected pass during initial stage
ERROR - The initial scan failed for rule 'xccdf_org.ssgproject.content_rule_firewalld_sshd_port_enabled'.
Great. I already found the issue and will fix it soon. |
It is done @mildas |
Some conditions were removed from test scenarios in order to make them more resilient to test environment peculiarities.
The remediation was initially consirering to set a firewalld zone only to active NetworkManager connections. However, it is possible that a system has more valid connection which are simply not in use at the moment. These inactive connections can be used at some point and if this happen, they will also be compliant with an explicit firewalld zone assigned to them. This way it is indeeded ensured all connections have a firewalld zone assigned.
On RHEL7 and probably other distros which rely on ifcfg files by default, there is a ifcfg file for the loopback interface, which is out of the scope in this rule and should be ignored. This commit also improved the wording in a OVAL comment to make it more clear.
64f2099
to
657c1cc
Compare
Code Climate has analyzed commit 657c1cc 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 46.5% (0.0% change). View more on Code Climate. |
/test e2e-aws-rhcos4-e8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing Automatus is expected because tests and remediations use services which are not available in containers.
I have run tests on my local VMs and Automatus (rule and profile mode) passes everywhere. Also tried kickstart installation and the rule fails, but that's expected becauseservices are not running during installation.
Thank you @marcusburghardt! Amazing job
Description:
Thanks to the
testing-farm:centos-stream-9-x86_64
test, a fail related to thefirewalld_sshd_port_enabled
rule was reported.An issue was created (#9495) and an investigation was conducted. Even in the initial analysis, many issues involving evaluation and remediation were revealed, suggesting a considerable amount of work to make it robust enough.
When I started working on this rule, I could see that it wouldn't be robust enough without a redesign. So I decided to rewrite the rule entirely, with new logic in line with the
firewalld
andNetworkManager
behaviors. This made the rule simpler, more realistic, and much more robust.In short, OVAL, Bash, Ansible and Test Scenarios have been redesigned.
Rationale:
firewalld_sshd_port_enabled
fails to remediate on Centos Stream 9 #9495testing-farm:centos-stream-9-x86_64
test green again.Review Hints:
This rule was completely redesigned. Therefore, I recommend to not be distracted by the old approach since it was not robust enough and wasn't aligned to the best practices. Instead, the review should focus on the new logic.
Think on it as a completely new rule.
The remediation and assessment were inspired by the
nmcli
andfirewall-cmd
commands and their respective behaviors.These were the most important commands used during the development.
nmcli --fields CONNECTION device status
to collect the connections namesnmcli -f connection.zone connection show <connection name>
to verify which zone is assigned to a connection namenmcli connection modify <connection name> connection.zone <zone>
to assign afirewalld
zone to a connectionfirewall-cmd --get-active-zones
to collect active zones (zones with at least one connection assigned)firewall-cmd --permanent --zone=<zone> --add-service=ssh
to ensure a service is allowed in a zonefirewall-cmd --permanent --zone=<zone> --remove-service=ssh
to simulate a default zone customizationAs the changes are significant, I recommend reviewing commit by commit, following their order in this PR.
For this rule, I've guaranteed a lot of comments in OVAL, Bash, and Test Scenarios. These comments facilitate the understanding of the adopted logic, providing the necessary context to understand the technical decisions made around this logic.