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

Introduce network access control rule #10596

Conversation

freddieRv
Copy link
Contributor

Description:

  • Introduce a new rule network_implement_access_control with OVAL checks and some tests
  • This rule intends to cover the OL07-00-040810 DISA STIG requirement

Rationale:

  • OL STIG efforts

@freddieRv freddieRv requested a review from a team as a code owner May 19, 2023 20:00
@github-actions
Copy link

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

ol7 (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

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

Click here to see the full diff
New content has different text for rule 'xccdf_org.ssgproject.content_rule_set_firewalld_default_zone'.%0A--- xccdf_org.ssgproject.content_rule_set_firewalld_default_zone%0A+++ xccdf_org.ssgproject.content_rule_set_firewalld_default_zone%0A@@ -249,12 +249,6 @@%0A [reference]:%0A SRG-OS-000480-GPOS-00227%0A %0A-[reference]:%0A-OL07-00-040810%0A-%0A-[reference]:%0A-SV-221892r603260_rule%0A-%0A [rationale]:%0A In firewalld the default zone is applied only after all%0A the applicable rules in the table are examined for a match. Setting the

@marcusburghardt marcusburghardt added Oracle Linux Oracle Linux product related. New Rule Issues or pull requests related to new Rules. STIG STIG Benchmark related. labels May 22, 2023
@vojtapolasek vojtapolasek self-assigned this May 23, 2023
Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Hello,
thank you for the rule. I am actually wondering if this rule should be automated... because you are not checking for exact hosts / services, you are just checking if Firewalld allows access to some hosts / services... but that is just a thought.
Anyway, please see my two comments.
Could you also please rewrite test scenarios so that they do not use sed and echo but rather firewalld commands?
Because in current way, you are actually creating invalid XML files.
Thank you.

specific hosts" test_ref="test_firewalld_public_zone_hosts_configured" />
<criterion comment="firewalld's public zone is configured to grant access to
specific services" test_ref="test_firewalld_public_zone_services_configured" />
<criterion comment="/etc/firewalld/zones/public does not exist
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please what is the goal of this criterria? You either check if the public.xml contains dfinitions allowing certain hosts or services or if the file does not exist at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From firewalld.zones(5):

[...] you can create or copy a zone file in one of the configuration directories. /usr/lib/firewalld/zones is used for default and fallback configurations and /etc/firewalld/zones is used for user created and customized configuration files.

So the logic in the OVAL is to look for configurations for specific hosts or services in a user-defined public zone. Or to make sure that there is no user defined public zone and therefore the one included with the firewalld packages is being used. This works under the assumption (perhaps a big assumption) that the default public zone is properly configured and was not modified.

Now that I write this I think that rather than forcing users to configure the public zone as the default one. A smarter approach wold be to get whatever zone is configured as the default and then check that it is properly configured; either in /usr/lib or in /etc.

@freddieRv
Copy link
Contributor Author

@vojtapolasek

I am actually wondering if this rule should be automated... because you are not checking for exact hosts / services, you are just checking if Firewalld allows access to some hosts / services

I think it is not feasible to check for specific hosts and/or services.

We had initially introduced this rule as a manual one in #9909 and got some push-back precisely because it was not automated 🙃

Having this rule as a purely manual one would also be alright. We want to have it in the OL stig profile for completeness.

Could you also please rewrite test scenarios so that they do not use sed and echo but rather firewalld commands?
Because in current way, you are actually creating invalid XML files.

Sure. It makes more sense to use firewalld-cmd to make these changes. I'll update them (if the automation is accepted/still desired)

@vojtapolasek
Copy link
Collaborator

Hello @freddieRv, thank you for explaining it. I guess you refer to this comment: #9909 (comment)
This rule is difficult. The current implementation you suggest checks if Firewalld is installed and active - that is good. It also checks if the Firewalld / TCP wrappers grant or deny access to some services or hosts - and that is problematic. My point is that without checking for a particular services or hosts, this rule can bring a false sense of security... because by showing this rule as passed, it might seem that the system is configured correctly, but the rule definitely does not guarantee it.
This approach is valid, although I think it should be spelled out somewhere.
I see following ways what to do:

  1. Drop the rule because this configuration can't be easily and reliably automated.
  2. Keep the rule as it is, but add a paragraph to the description section or a warning section which states that the rule does NOT check for specific hosts and services so that it is clear it has limitations.

I noticed that you tried to submit the rule without automation and it was questioned. In general, I think that submitting rules without automation currently does not make much sense within scope of this project. The main scope of the project is to provide automated checks and remediations based on security policies. Therefore, having a rule in the HTML guide / HTML report which is NOT automated does not bring value.

What are your thoughts? Is one of my suggestions above acceptable for you?

@freddieRv
Copy link
Contributor Author

@vojtapolasek

My point is that without checking for a particular services or hosts, this rule can bring a false sense of security

The issue here is that which hosts and services should be allowed on a system will most definitely change from system to system so it is not feasible to have some fixed list of hosts/services to check for.

The logic on the OVAL is to make sure that the system is configured to allow just some hosts and services as opposed to allow any and all network traffic and connections.

Therefore, having a rule in the HTML guide / HTML report which is NOT automated does not bring value.

I disagree. While I get that the 'A' in 'SCAP' stands for 'automation', as I mentioned earlier, we want to have this rule in the stig profile for the sake of completeness. Even if it is not automated, the reports will show a notchecked result and the users can go to the HTML guides to look for guidance on how to check and/or remediate this requirement.

We aim for users to be able to use SSG to harden their systems to be fully compliant with the profiles we provide. If we don't cover a requirement at all just because it is not (easily) automatable, then users will get a false sense of security since they wouldn't even be aware that there are some requirements not being checked unless they use some other tool/content to audit their systems.

Is one of my suggestions above acceptable for you?

Yes, adding a warning makes sense. While I'll still update the OVAL content to allow any zone to be used instead of forcing the public one.

While we are on that, how does this sound for a warning?
"This rule checks that either firewalld or tcpwrappers are being used to restrict system access to some hosts and/or services. It does not check for any specific hosts/services. Make sure that the allowed hosts/services meet your operational needs. "

Implement a new rule to cover DISA STIG requirement OL07-00-040810
Removed this STIG id from set_firewalld_default_zone and add
the new rule to OL7 stig profile

Signed-off-by: Federico Ramirez <federico.r.ramirez@oracle.com>
Also add a warning about the OVAL limitations to
the rule yaml file

Signed-off-by: Federico Ramirez <federico.r.ramirez@oracle.com>
@freddieRv freddieRv force-pushed the introduce-network-access-control-rule branch from 244d5b8 to a755823 Compare May 26, 2023 22:48
Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Hello,
this looks reasonable.
Just a note, please add the following line at the begining of all test scenarios, right below the shebang line. It ensures that the firewalld package gets installed.

# packages = firewalld

Signed-off-by: Federico Ramirez <federico.r.ramirez@oracle.com>
@freddieRv freddieRv force-pushed the introduce-network-access-control-rule branch from a755823 to a3d5cb6 Compare May 29, 2023 18:46
@codeclimate
Copy link

codeclimate bot commented May 29, 2023

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

View more on Code Climate.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

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

Looks good now, thank you. Automatus failures are caused by the fact that the rule currently does not have any rhel prodtype. Out of curiosity, I tested the rule on RHEL 7 and RHEL 8 and tests pass.

@vojtapolasek vojtapolasek merged commit 71dfc0d into ComplianceAsCode:master May 30, 2023
25 of 28 checks passed
@Mab879 Mab879 added this to the 0.1.69 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Rule Issues or pull requests related to new Rules. Oracle Linux Oracle Linux product related. STIG STIG Benchmark related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants