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

Change custom zones check in firewalld_sshd_port_enabled #10162

Conversation

marcusburghardt
Copy link
Member

Description:

One test in the OVAL check of this rule verifies if new zones or customized zones allow ssh.
Basically, if any default zone is modified by the administrator, the respective zone file is placed in /etc/firewalld/zones directory in order to override the default zone settings.
The same directory is applicable for new zones created by the administrator.
Therefore, all files in this directory should also allow SSH. This test was updated in a reaction to OpenSCAP/openscap#1923, which changed the behavior of xmlfilecontent probe in OpenSCAP 1.3.7.

Rationale:

The previous behavior in OpenSCAP xmlfilecontent probe was caused by a bug which was fixed on 1.3.7 release and consequently the OVAL check had to be adapted to the expected behavior.

Review Hints:

This rule will fail in systems which use OpenSCAP version older than 1.3.7.
It is not feasible to keep compatibility with the old (also buggy) behavior.

If any default zone is modified by the administrator, the respective
zone file is placed in /etc/firewalld/zones dir in order to override
the default zone settings. The same directory is applicable for new
zones created by the administrator. Therefore, all files in this
directory should also allow SSH. This test was updated in a reaction
to OpenSCAP/openscap#1923, which changed the
behavior of xmlfilecontent probe in OpenSCAP 1.3.7.
@marcusburghardt marcusburghardt added bugfix Fixes to reported bugs. Update Rule Issues or pull requests related to Rules updates. labels Feb 3, 2023
@marcusburghardt marcusburghardt added this to the 0.1.67 milestone Feb 3, 2023
@packit-as-a-service
Copy link

We were not able to find or create Copr project packit/ComplianceAsCode-content-10162 specified in the config with the following error:

Cannot create a new Copr project (owner=packit project=ComplianceAsCode-content-10162 chroots=['centos-stream-9-x86_64', 'epel-7-x86_64', 'fedora-36-x86_64', 'fedora-37-x86_64', 'centos-stream-8-x86_64', 'fedora-rawhide-x86_64']): Login invalid/expired. Please visit https://copr.fedorainfracloud.org/api to get or renew your API token.

Please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

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

@mildas
Copy link
Contributor

mildas commented Feb 3, 2023

/packit build

@matejak
Copy link
Member

matejak commented Feb 3, 2023

The OVAL test is pretty complex. Could you describe what was the change?

It seems to me that you converted the test_firewalld_sshd_port_enabled_zone_ssh_enabled_etc test to compare assert equality between the count of all files that define zones and the count of files that allow the SSH connection, while the former is a superset of the latter.

Then, is the problem with previous scanner versions the fact that there is always the equality although SSH is not allowed in all zones?

@marcusburghardt
Copy link
Member Author

The OVAL test is pretty complex. Could you describe what was the change?

Unfortunately it became a little more complex now to adapt for the scanner change.

It seems to me that you converted the test_firewalld_sshd_port_enabled_zone_ssh_enabled_etc test to compare assert equality between the count of all files that define zones and the count of files that allow the SSH connection, while the former is a superset of the latter.

Then, is the problem with previous scanner versions the fact that there is always the equality although SSH is not allowed in all zones?

You are correct.

test_firewalld_sshd_port_enabled_zone_ssh_enabled_etc was using xmlfilecontent_test where the object was always returning a list of xml files regardless of matching the xpath or not. Then, the state in this test ensured goal of checking that all files were allowing SSH. This was the main reason of this xpath in both object and state element in the previous approach.

Once the behavior for the xmlfilecontent probe was changed, only objects which a matched xpath are returned.
This demanded the change in this check. So now two variables were created:

  • first contains the number of zone files in /etc/firewalld/zones, regardless of their content.
  • second contains the number of zone files in /etc/firewalld/zones, which are allowing ssh.

These two variables are compared and should match in a compliant system with newer scanner version.

@yuumasato yuumasato self-assigned this Feb 3, 2023
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

The patch works, but I have suggested a simplification that worked locally.
If there is no special reason to use variable var_firewalld_sshd_port_enabled_custom_zone_files I suggest removing it and reverting object_firewalld_sshd_port_enabled_zone_files_etc to its previous form.

Simplify definition of xmlfilecontent object for ssh services.
There is no need to use a variable to define the object, the collected
set will be the same.
@yuumasato
Copy link
Member

Talked offline with @marcusburghardt and pushed 50bbd13.

All test scenarios still pass with openscap-1.3.6 and 1.3.7.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

@Mab879 Could take a quick look too?
Thanks

@marcusburghardt
Copy link
Member Author

@Mab879 Could take a quick look too? Thanks

Thanks for committing the simplification @yuumasato . I could only take a look on this next week.

@codeclimate
Copy link

codeclimate bot commented Feb 3, 2023

Code Climate has analyzed commit 50bbd13 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 49.5% (0.0% change).

View more on Code Climate.

@Mab879
Copy link
Member

Mab879 commented Feb 3, 2023

I cannot corroborate that "All test scenarios still pass with openscap-1.3.6 and 1.3.7.". At least in my testing.

@yuumasato
Copy link
Member

yuumasato commented Feb 6, 2023

I cannot corroborate that "All test scenarios still pass with openscap-1.3.6 and 1.3.7.". At least in my testing.

I may have misunderstood Marcus and overstated the test results.
I ran all the tests again and neither eb34d3a nor 50bbd13 pass all the test scenarios with openscap-1.3.6.
But both commits pass all test scenarios when openscap-1.3.7 is used.

Do you also get the same results, @marcusburghardt ?

@marcusburghardt
Copy link
Member Author

marcusburghardt commented Feb 6, 2023

I cannot corroborate that "All test scenarios still pass with openscap-1.3.6 and 1.3.7.". At least in my testing.

The simplification created by @yuumasato is pretty valid. However, also in my tests, they are failing for 1.3.6 and passing for 1.3.7. In my tests last week I didn't find a way pass on both versions without creating two different tests, one for each version.

@marcusburghardt
Copy link
Member Author

I cannot corroborate that "All test scenarios still pass with openscap-1.3.6 and 1.3.7.". At least in my testing.

I may have misunderstood Marcus and overstated the test results. I ran all the tests again and neither eb34d3a nor 50bbd13 pass all the test scenarios with openscap-1.3.6. But both commits pass all test scenarios when openscap-1.3.7 is used.

Do you also get the same results, @marcusburghardt ?

Yes @yuumasato . This is what I mentioned in the "Review Hints".

@yuumasato
Copy link
Member

Yes @yuumasato . This is what I mentioned in the "Review Hints".

Thanks, sorry for the confusion.
I'm merging this then.

@yuumasato
Copy link
Member

The Automatus CS8, CS9 and Fedora tests are failing because containers are not applicable for this rule. The commands systemctl, firewall-cmd and nmcli are not available there.

@yuumasato yuumasato merged commit 2a66e62 into ComplianceAsCode:master Feb 6, 2023
@marcusburghardt marcusburghardt deleted the firewalld_sshd_port_enabled_tests branch February 6, 2023 10:06
@marcusburghardt
Copy link
Member Author

Relates to #9712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes to reported bugs. Update Rule Issues or pull requests related to Rules updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants