Express more accurate per package platform limitation for firewall rules #10812

teacup-on-rockingchair · 2023-07-07T02:55:14Z

Description:

Define more accurate per package limitation

Rationale:

It can be the case where we have installed firewalld package and it should be used to express user configuration, but also we have installed nftables as provider of backend mechanism of firewalld, in those cases only firewalld rules should be applicable

github-actions · 2023-07-07T02:56:51Z

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

Fedora Environment

Oracle Linux 8 Environment

marcusburghardt

I don't think we can safely assume that firewalld is mandatory if installed. There are policies which allow the admin to choose which solution to use. It may have valid cases where both firewalld and nftables are installed but only nftables is used. I think the policy and consequently the profile may be opinionated on this by removing the rules, but not the rule itself.

marcusburghardt · 2023-07-07T09:27:38Z

linux_os/guide/system/network/network-nftables/nftables_ensure_default_deny_policy/rule.yml

@@ -15,7 +15,7 @@ rationale: |-

 severity: medium

-platform: package[nftables]
+platform: package[nftables] and not package[firewalld]


In most cases, both packages are installed but only one is enabled/started. I don't think it is a good idea to make them mutually exclusive. But including package[nftables] sounds reasonable to me.

The idea here is that the check from my perspective should be performed when we have only nftables and firewall is managed through the nftables service itself, because if we have firewalld the configuration for default policy will be done through firewalld configuration

I see. But this would limit the rule for a very specific case, making it less useful in general.
There are valid scenarios where this rule would report notapplicable when it would be indeed applicable and desired by the users.
For example, it is possible to have both firewalld and nftables installed but the Admin decided to use nftables without removing the firewalld package, but only disabling it. So, we can't assume that firewalld is the chosen solution for managing firewall rules simply by the presence of the package.

Since the rule is about nftables, having nftables package present is enough to make it applicable.
The kind of conflict you are trying to solve could be better resolved in the profile level.
For example, in the profile you also select the rules to remove firewalld and iptables packages. This would ensure nftables is used.

This is exactly what I was trying to avoid - to have different profiles depending on what package user has installed. The situation is that currently we might end up with most of the distros have any combination of firewalld, nftables, iptables, and using the package platform seemed like the only solution to go with so the user has the freedom to install any package and still be compliant with the profile that covers all.

marcusburghardt · 2023-07-07T09:29:45Z

linux_os/guide/system/network/network-nftables/nftables_rules_permanent/rule.yml

@@ -17,7 +17,7 @@ rationale: |-

 severity: medium

-platform: package[nftables]
+platform: package[nftables] and not package[firewalld]


Same case of nftables_ensure_default_deny_policy rule.

Same motivation as for default_deny_policy rule

marcusburghardt · 2023-07-07T09:34:12Z

linux_os/guide/system/network/network-nftables/package_nftables_removed/rule.yml

@@ -14,6 +14,8 @@ rationale: |-

 severity: medium

+platform: package[nftables] and not package[firewalld]


This rule is only for nftables, regardless of having firewalld installed. Maybe for this rule, the entire platform condition could be removed. It is about removing the package. If the package is not installed, it will report notapplicable which sounds not the proper message to me. I think that pass would fit better in this case.

I agree not the smoothest way to approach this but still the rationale here is that we do not have the nftables package , when we have firewalld enabled. But as you mentioned earlier in most cases we have nftables package as dependency to firewalld , since firewalld implements its rules through nft rules.

What do you think about removing this line completely? I believe it is not necessary in this rule.

marcusburghardt · 2023-07-07T09:34:44Z

linux_os/guide/system/network/network-nftables/service_nftables_disabled/rule.yml

@@ -39,7 +39,7 @@ ocil: |-

 fixtext: '{{{ fixtext_service_disabled("nftables") }}}'

-platform: machine
+platform: machine and package[nftables] and package[firewalld]


I think it is good to include and package[nftables] but it not necessary to include and package[firewalld]

Same motivation as for default_deny_policy rule

Ideally we should also have a service_enabled[firewalld] here.

linux_os/guide/system/network/network-nftables/service_nftables_enabled/rule.yml

linux_os/guide/system/network/network-nftables/set_nftables_loopback_traffic/rule.yml

teacup-on-rockingchair · 2023-08-07T07:07:05Z

@marcusburghardt I re-checked the issue you linked #10896 , but even if we assume that the the platform limitation for iptables that was added is extra restrictive, both the sce check and the bash remeditation rely explicitly on ip6tables, so I would say that reported behaviour is actually enhancement, as far as I understand it, except if I am missing there.

On more general note I would really appreciate some additional feedback on the approach in the PR. If your hard opinion is that, it is against the policy of the framework to limit the firewall rules based on installed packages, than I will rework the current one to have only platform package requirements for available packets, when the rule requires a file or tool part of the packet and will drop the limitations not to allow nftables configuration rules, when firewalld is installed or similar.

marcusburghardt

@teacup-on-rockingchair , I expanded my concern in comments from this last review and proposed you an option to move forward with this PR. Could you take a look, please?

marcusburghardt · 2023-08-08T06:40:49Z

linux_os/guide/system/network/network-nftables/package_nftables_removed/rule.yml

@@ -14,6 +14,8 @@ rationale: |-

 severity: medium

+platform: package[nftables] and not package[firewalld]


What do you think about removing this line completely? I believe it is not necessary in this rule.

linux_os/guide/system/network/network-nftables/service_nftables_disabled/rule.yml

linux_os/guide/system/network/network-nftables/set_nftables_loopback_traffic/rule.yml

linux_os/guide/system/network/network-nftables/service_nftables_enabled/rule.yml

linux_os/guide/system/network/network-nftables/nftables_ensure_default_deny_policy/rule.yml

marcusburghardt · 2023-08-16T10:29:15Z

@teacup-on-rockingchair , could you resolve the conflict and check the CI errors, please?

It can be the case where we have installed firewalld package and it should be used to express user configuration, but also we have installed nftables as provider of backend mechanism of firewalld, in those cases only firewalld rules should be applicable

Move the criteria and test code to macro to simplify and reuse

@marcusburghardt

…ibility Kudos to @marcusburghardt for discussion and hints

teacup-on-rockingchair · 2023-08-16T12:19:08Z

@teacup-on-rockingchair , could you resolve the conflict and check the CI errors, please?

Should be ok now. Unfortunately could not find fix for code duplication between the cpe-oval and oval code.

shared/macros/10-oval.jinja

marcusburghardt

I am testing the changes locally. In this meantime, it would be great if the lines in the new macros fit within 99 chars. Could you take a look on it, please?

@marcusburghardt

Also limit char count per line Thanks @marcusburghardt for noting 🙇

marcusburghardt

I tested these rules locally and they are working fine. I have only one more minor comment in package_nftables_removed and we should be ready to merge it.

linux_os/guide/system/network/network-nftables/package_nftables_removed/rule.yml

…s_removed/rule.yml Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>

marcusburghardt

Thanks

codeclimate · 2023-08-22T13:43:21Z

Code Climate has analyzed commit f83a08e and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

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.3% (0.0% change).

View more on Code Climate.

teacup-on-rockingchair added the Update Rule Issues or pull requests related to Rules updates. label Jul 7, 2023

teacup-on-rockingchair marked this pull request as draft July 7, 2023 04:17

openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Jul 7, 2023

teacup-on-rockingchair force-pushed the firewalld_vs_nftables_more_accurate_per_package_platform branch from 646e798 to a0e71f9 Compare July 7, 2023 04:49

teacup-on-rockingchair marked this pull request as ready for review July 7, 2023 04:50

openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jul 7, 2023

marcusburghardt requested changes Jul 7, 2023

View reviewed changes

marcusburghardt self-assigned this Jul 7, 2023

marcusburghardt added the CPE-AL CPE Applicability Language label Jul 7, 2023

marcusburghardt mentioned this pull request Jul 25, 2023

CIS Workstation L2 profile produces unexpected notapplicable results #10896

Closed

marcusburghardt requested changes Aug 8, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Aug 14, 2023

teacup-on-rockingchair force-pushed the firewalld_vs_nftables_more_accurate_per_package_platform branch from 1ad177b to 347d04e Compare August 14, 2023 04:48

teacup-on-rockingchair added 5 commits August 16, 2023 14:24

Rework platform requirements to check for service firewalld disabled

1ccc4d3

Add rule and template for service disabled cpe check

a8ad14e

Move the criteria and test code to macro to simplify and reuse

Reuse macro for service disabled checks

92bfbd2

Refine platform requirement from package to service for improved flex…

8362598

…ibility Kudos to @marcusburghardt for discussion and hints

teacup-on-rockingchair force-pushed the firewalld_vs_nftables_more_accurate_per_package_platform branch from 347d04e to 8362598 Compare August 16, 2023 11:26

openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Aug 16, 2023

marcusburghardt reviewed Aug 17, 2023

View reviewed changes

shared/macros/10-oval.jinja Outdated Show resolved Hide resolved

marcusburghardt requested changes Aug 17, 2023

View reviewed changes

Improved indentation in the new macros

3692f93

Also limit char count per line Thanks @marcusburghardt for noting 🙇

marcusburghardt requested changes Aug 22, 2023

View reviewed changes

linux_os/guide/system/network/network-nftables/package_nftables_removed/rule.yml Outdated Show resolved Hide resolved

Update linux_os/guide/system/network/network-nftables/package_nftable…

f83a08e

…s_removed/rule.yml Co-authored-by: Marcus Burghardt <2074099+marcusburghardt@users.noreply.github.com>

marcusburghardt approved these changes Aug 22, 2023

View reviewed changes

teacup-on-rockingchair merged commit 2ea9497 into ComplianceAsCode:master Aug 22, 2023
35 of 38 checks passed

Mab879 added this to the 0.1.70 milestone Sep 14, 2023

marcusburghardt mentioned this pull request Dec 20, 2023

Make package installation for iptables and nftables mutually exclusive #11191

Merged

marcofortina mentioned this pull request Apr 26, 2024

Issue on check on firewall rules (Ubuntu 22-04 + UFW) #11907

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Express more accurate per package platform limitation for firewall rules #10812

Express more accurate per package platform limitation for firewall rules #10812

teacup-on-rockingchair commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

marcusburghardt left a comment

marcusburghardt Jul 7, 2023

teacup-on-rockingchair Jul 11, 2023

marcusburghardt Jul 11, 2023

teacup-on-rockingchair Jul 12, 2023

marcusburghardt Jul 7, 2023

teacup-on-rockingchair Jul 11, 2023

marcusburghardt Jul 7, 2023

teacup-on-rockingchair Jul 11, 2023

marcusburghardt Aug 8, 2023

marcusburghardt Jul 7, 2023

teacup-on-rockingchair Jul 11, 2023

marcusburghardt Aug 17, 2023

teacup-on-rockingchair commented Aug 7, 2023

marcusburghardt left a comment

marcusburghardt Aug 8, 2023

marcusburghardt commented Aug 16, 2023

teacup-on-rockingchair commented Aug 16, 2023

marcusburghardt left a comment

marcusburghardt left a comment

marcusburghardt left a comment

codeclimate bot commented Aug 22, 2023

		@@ -14,6 +14,8 @@ rationale: \|-

		severity: medium

		platform: package[nftables] and not package[firewalld]

Express more accurate per package platform limitation for firewall rules #10812

Express more accurate per package platform limitation for firewall rules #10812

Conversation

teacup-on-rockingchair commented Jul 7, 2023

Description:

Rationale:

github-actions bot commented Jul 7, 2023

marcusburghardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teacup-on-rockingchair commented Aug 7, 2023

marcusburghardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusburghardt commented Aug 16, 2023

teacup-on-rockingchair commented Aug 16, 2023

marcusburghardt left a comment

Choose a reason for hiding this comment

marcusburghardt left a comment

Choose a reason for hiding this comment

marcusburghardt left a comment

Choose a reason for hiding this comment

codeclimate bot commented Aug 22, 2023