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

all_apparmor_profiles_in_enforce_complain_mode: Fix OVAL logic #11672

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

dodys
Copy link
Contributor

@dodys dodys commented Mar 11, 2024

Description:

  • Current OVAL fails with unknown result because the variables are looking for a subexpression field of the object when there's none. Instead use text part of the object. Also change how you seek for unconfined.
  • Also fixes test not working on sle15.

Rationale:

  • To have a subexpression field from the object you would need to select some specific text with parenthesis, but that's not being done. Since we don't care about the actual pattern being matched but instead the number of matches, using the text field instead is enough.

Review Hints:

  • OVAL passes.

@dodys dodys added OVAL OVAL update. Related to the systems assessments. SLES SUSE Linux Enterprise Server product related. Ubuntu Ubuntu product related. CIS CIS Benchmark related. labels Mar 11, 2024
Copy link

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

sle12 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

Copy link

github-actions bot commented Mar 11, 2024

🤖 A k8s content image for this PR is available at:
ghcr.io/complianceascode/k8scontent:11672
This image was built from commit: bbff647

Click here to see how to deploy it

If you alread have Compliance Operator deployed:
utils/build_ds_container.py -i ghcr.io/complianceascode/k8scontent:11672

Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and:
CONTENT_IMAGE=ghcr.io/complianceascode/k8scontent:11672 make deploy-local

@dodys
Copy link
Contributor Author

dodys commented Mar 11, 2024

@teacup-on-rockingchair is the tests you added to this rule actually working? they seem completely broken right now. I tried to fix it by adding the apparmor-utils to platform-packages-override and now it fails with something else.
I will leave it to you to add what is missing there and to test my changes.

@teacup-on-rockingchair
Copy link
Contributor

@teacup-on-rockingchair is the tests you added to this rule actually working? they seem completely broken right now. I tried to fix it by adding the apparmor-utils to platform-packages-override and now it fails with something else. I will leave it to you to add what is missing there and to test my changes.

I am afraid for the aa-teardown, and apparmor_parser used in the tests you need to add also apparmor-parser package, but anyways the tools in the tests require appropriate kernel patches etc, so should not consider them ready for container environment.

@dodys dodys force-pushed the apparmor branch 2 times, most recently from df2abe3 to 65e351b Compare March 13, 2024 15:01
@dodys
Copy link
Contributor Author

dodys commented Mar 13, 2024

@teacup-on-rockingchair is the tests you added to this rule actually working? they seem completely broken right now. I tried to fix it by adding the apparmor-utils to platform-packages-override and now it fails with something else. I will leave it to you to add what is missing there and to test my changes.

I am afraid for the aa-teardown, and apparmor_parser used in the tests you need to add also apparmor-parser package, but anyways the tools in the tests require appropriate kernel patches etc, so should not consider them ready for container environment.

in that case, we should waiver the test failures.

Could you please check now if it works fine on sles?

Current OVAL fails with unknown result because the variables are looking
for a subexpression of the subject when there's none. Also remove check
for unconfined as it is not needed
Copy link

codeclimate bot commented Mar 13, 2024

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

View more on Code Climate.

@teacup-on-rockingchair
Copy link
Contributor

@teacup-on-rockingchair is the tests you added to this rule actually working? they seem completely broken right now. I tried to fix it by adding the apparmor-utils to platform-packages-override and now it fails with something else. I will leave it to you to add what is missing there and to test my changes.

I am afraid for the aa-teardown, and apparmor_parser used in the tests you need to add also apparmor-parser package, but anyways the tools in the tests require appropriate kernel patches etc, so should not consider them ready for container environment.

in that case, we should waiver the test failures.

Could you please check now if it works fine on sles?

Yes they work with the patches and are failing on master branch with my half baked commit so I guess we are ok there :)

Copy link
Contributor

@teacup-on-rockingchair teacup-on-rockingchair left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and corrections @dodys 🙇 . If you decide to go with the suggested more simplistic approach just ping me to approve it again. Overall I think most important is that we have a working precedent of oval check working with apparmor ;)

@dodys
Copy link
Contributor Author

dodys commented Mar 14, 2024

Thanks for the contribution and corrections @dodys 🙇 . If you decide to go with the suggested more simplistic approach just ping me to approve it again. Overall I think most important is that we have a working precedent of oval check working with apparmor ;)

I feel like this OVAL still doesn't implement the whole check from CIS. It only checks profiles, not processes.
I was mostly fixing it so Ubuntu can still rely on the SCE check instead.
I will leave it as-is for now. If you could merge it, it would be great :)
Then any later work on this OVAL can be done in a next PR.

@Mab879 Mab879 added this to the 0.1.73 milestone Mar 14, 2024
@teacup-on-rockingchair teacup-on-rockingchair merged commit 353e6e0 into ComplianceAsCode:master Mar 15, 2024
40 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CIS CIS Benchmark related. OVAL OVAL update. Related to the systems assessments. SLES SUSE Linux Enterprise Server product related. Ubuntu Ubuntu product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants