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

Add Variable-based CPE and UFW rules for Ubuntu 20.04 STIG #7635

Closed
wants to merge 32 commits into from

Conversation

dodys
Copy link
Contributor

@dodys dodys commented Sep 23, 2021

Description:

  • This is a long PR as it adds support for variable-based CPEs and as an example I've included the UFW rules which make use of it.
  • The variable-based CPEs commits were mainly developed by @cipherboy. I will let him talk about it, if possible.
  • Let me know if you think I should break this into multiple PRs instead. I also have a few commits that adds the platform bits to iptables rules and others, that I didn't include here but I can certainly add in a separate PR.

@pep8speaks
Copy link

pep8speaks commented Sep 23, 2021

Hello @dodys! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 279:1: E302 expected 2 blank lines, found 1

Comment last updated at 2022-02-10 16:05:57 UTC

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Sep 23, 2021
@openshift-ci
Copy link

openshift-ci bot commented Sep 23, 2021

Hi @dodys. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dodys
Copy link
Contributor Author

dodys commented Sep 24, 2021

rebased to fix conflict in shared/macros-highlevel.jinja

@cipherboy
Copy link
Contributor

Well, happy to see this upstreamed, thanks @dodys! :-) Not really sure where to start any more. :D

Variable based pseudo-CPEs was a solution we came upon early in the development of Canonical's internal CaC port. One feature we wanted was the ability to use a XCCDF variable to switch between one of various choices, but for sure audit against it.

That is, compared to the real CPE system, even if the package is not installed (or could reasonably co-exist), we still want to generate pass/fail messages for the user's selection -- and preferably generate not-applicable (really: pass) messages for everything else. Note that not-applicable isn't possible due to a certain long-standing OVAL bug that still hasn't gotten solved. This further has the consequence that, if a XCCDF variable pseudo-CPE selection does NOT align with the packages on the system, the remediation of the benchmark would result in following user's desires, though it might take two scan+remediate passes in order to do so (given that CPE and pseudo-CPE could co-exist in the rule).

That is, if UFW is selected in the XCCDF variable but iptables was present, and given that all rules included both CPEs and pseudo-CPEs, the iptables would pass (see note below -- less than ideal but given our constraints) and all other would return not-applicable (except the package_ufw_installed and package_iptables_removed). On the first remediation, this would remove iptables and install UFW. On the next scan, iptables would return not-applicable (since iptables-persistent is no longer installed) and then UFW rules would return fail (since presumably they don't comply with the benchmark) -- and on this remediation, now the remediate-able rules would then finally be remediated.

Since the CPE evaluation is independent of XCCDF variables (which is to say, CPE evaluation has no access to XCCDF variables), we end up creating a "pseudo-CPE": shared OVAL definitions placed in the main OVAL content. Each rule with a pseudo-CPE selector then has its OVAL modified to essentially include a block like so:

<criteria operator="OR">
   <criterion id="not_variable">
   <criteria operator="AND">
       <criterion id="variable">
       ....existing criteria/criterion....
   </criteria>
</criteria> 

Ideally the first criterion would result in a hard not-applicable, but due to OVAL-Community/OVAL#24, this isn't possible. Thus, the first criterion results in a "pass". This gets put into both bespoke and templated OVALs.

I think we ended up using pseudo-CPEs in a couple of places internally:

  • Firewall selection (ufw/iptables/nftables)
  • Timesync selection (chrony/ntpd/systemd-timesyncd)
  • One or two other places I'm forgetting.

Notably, in both of these categories, two or more of the selections could co-exist peacefully on the system (firewalld and ufw are both front-ends to either iptables/nftables and thus definitely could co-exist with them --- or systemd-tymesyncd could be installed and inactive and co-exist with either of chrony/ntpd). Without XCCDF variable selector, alignment with the benchmarks (which, as of time of creation, audited firewall frontends rather than backends) is harder, if we want to preserve that users still need to make an explicit choice.

Finally, note that this support only applies to the OVAL. Since SCE could in theory be any number of languages, and it is easy enough to provision+read a variable and return not-applicable manually, SCE does not have automatic pseudo-variable CPE support. IMO, if we restrict SCE to purely bash, this could be useful, but if we wish to support any other language (e.g., Python), it grows to become too complex to do automatically.

Hope that helps to provide some context on the design and requirements for this system.

@jan-cerny
Copy link
Collaborator

Thanks for a detailed explanation, I think the feature can be a great improvement.

Currently, @vojtapolasek and @yuumasato are working on some changes in CPEs, so it might be a good idea to consult this with them.

@vojtapolasek or @yuumasato What are your thoughts on this proposal?

@yuumasato
Copy link
Member

One feature we wanted was the ability to use a XCCDF variable to switch between one of various choices, but for sure audit against it.

With but for sure audit against it you mean to still evaluate the rules for the choices not made?

That is, compared to the real CPE system, even if the package is not installed (or could reasonably co-exist), we still want to generate pass/fail messages for the user's selection -- and preferably generate not-applicable (really: pass) messages for everything else.

I guess this issue is mostly caused because rules that configure a certain package have the CPE for applicability of said certain packages.
I guess that is why in RHEL8 CIS the rules for package install are not automated.

Each rule with a pseudo-CPE selector then has its OVAL modified to essentially include a block like so:

<criteria operator="OR">
   <criterion id="not_variable">
   <criteria operator="AND">
       <criterion id="variable">
       ....existing criteria/criterion....
   </criteria>
</criteria> 

I did a local build and none of the ufw checks had the above construct, but the remediations had the check for variable=package.

Notably, in both of these categories, two or more of the selections could co-exist peacefully on the system (firewalld and ufw are both front-ends to either iptables/nftables and thus definitely could co-exist with them --- or systemd-tymesyncd could be installed and inactive and co-exist with either of chrony/ntpd). Without XCCDF variable selector, alignment with the benchmarks (which, as of time of creation, audited firewall frontends rather than backends) is harder, if we want to preserve that users still need to make an explicit choice.

Not sure I understand, could you clarify why is it hard to align the benchmark?

I guess the XCCDF way of making a choice is via profile tailorings, and actually making a choice via XCCDF variable will in fact require a tailoring.
Maybe the difficulty is knowing witch rules need to be selected for each choice?

It looks like the variable-based CPE is trying to generalize the approach that some SSHD rules have with regards to "system install status" and "profile intention" (or user intention).
For example: sshd_use_approved_macs_ordered_stig
Together with the sshd_*_or_unset.xml in shared/checks/oval

These "sshd requires" mechanism were implemented by me a long time ago based on some quirky requirements (that may actually no have been true, meaning they can probably be removed.), they are a bit convoluted and confusing I admit. But it seems to me that the proposed variable-based CPEs can get quite complex and confusing as well.

I kinda got confused by "CPE" in the naming. I understand the proposed mechanism as a way to bundle up a few rules in a choice mechanism, but really determining applicability (validity or relevancy) of the rules.

I see the difficulty in making available "choices" aligned with the policy, and see value in trying to make it easier for users to pick those choices. But I have a slight feeling this is adding a bit more complexity than necessary.
Will think on it a bit more.

@yuumasato
Copy link
Member

Currently, @vojtapolasek and @yuumasato are working on some changes in CPEs, so it might be a good idea to consult this with them.

I don't think this proposal affects the CPE Applicability being worked on by Vojta.
The CPE applicability is just a more powerful way to describe the relevancy (not to say applicability again) of the rules in a "static" way. The applicability language is not intended to allow for choices in a profile tailoring

@cipherboy
Copy link
Contributor

Many thanks for the thoughtful comments Watson!

@yuumasato said:

@cipherboy said:

One feature we wanted was the ability to use a XCCDF variable to switch between one of various choices, but for sure audit against it.

With but for sure audit against it you mean to still evaluate the rules for the choices not made?

Close. Consider these rules (relevant for a benchmark):

  • package_iptables_installed
  • package_iptables_removed
  • iptables_default_deny (cpe: iptables)
  • package_ufw_installed
  • package_ufw_removed
  • ufw_default_deny (cpe: ufw)

In a CPE-only scenario, we'd have to make a choice here as to which rules to select in the profile. Let's assume the profile favors iptables and that's the package installed. Then there's two scenarios:

  1. sysadmin wants iptables installed. Easy, gets audited, expected.
  2. sysamdin wants ufw installed. Since iptables is selected and ufw isn't, the admin has to tailor the entire profile to unselect the iptables rules (ok, easier) and to select the ufw rules (which is harder! --- notably, the sysadmin has to understand the benchmark behind the profile and find all relevant ufw rules among the set of all ufw rules). Now the sysadmin must do a two-pass "audit+remediate" -- one to install ufw and the other to actually use the rules (due to CPE -- this is fine and expected).

Now consider the case where we use variable-based pseudo-CPEs. Here, we add all rules relevant to the benchmark to the profile. That means all of the rules listed above. Let's again assume iptables is the method selected in the XCCDF variable.

  1. sysadmin wants iptables installed. Easy, gets audited, expected. For the ufw-based rules, they all report pass (due to the bug mentioned above -- unless they use SCE, in which case they can report not applicable --- or if they use real CPE, then it might also be not applicable).
  2. sysadmin wants ufw installed. Since iptables is selected in the XCCDF variable, the admin merely needs to change the variable to ufw instead. Now the sysadmin must do the same two-pass "audit+remediate". For iptables, we see pass/not-applicable, much like ufw above.

Notably, what this means is that a the tailoring merely contains organization-specific policy -- and NOT benchmark+organization-specific policy (in the case of CPE-only -- all ufw rules must be enabled aligning with the benchmark and then rules not desired by the organization must be removed). This means if the benchmark changes, the organization must manually pull in additional rules.

This doesn't lose any of the ability for CaC to be opinionated about auditing/remediation of the benchmark (it can still favor whatever approach/tools it desires), and CPE is still present and IMO, useful.

That is, compared to the real CPE system, even if the package is not installed (or could reasonably co-exist), we still want to generate pass/fail messages for the user's selection -- and preferably generate not-applicable (really: pass) messages for everything else.

I guess this issue is mostly caused because rules that configure a certain package have the CPE for applicability of said certain packages. I guess that is why in RHEL8 CIS the rules for package install are not automated.

Right, and with pseudo-variable based CPEs, we can now automate them and they only get executed when this variable says so. :-)

Each rule with a pseudo-CPE selector then has its OVAL modified to essentially include a block like so:

<criteria operator="OR">
   <criterion id="not_variable">
   <criteria operator="AND">
       <criterion id="variable">
       ....existing criteria/criterion....
   </criteria>
</criteria> 

I did a local build and none of the ufw checks had the above construct, but the remediations had the check for variable=package.

Right, in the remediation it should all be firewall_choice == "ufw". Was that not the case?

Notably, in both of these categories, two or more of the selections could co-exist peacefully on the system (firewalld and ufw are both front-ends to either iptables/nftables and thus definitely could co-exist with them --- or systemd-tymesyncd could be installed and inactive and co-exist with either of chrony/ntpd). Without XCCDF variable selector, alignment with the benchmarks (which, as of time of creation, audited firewall frontends rather than backends) is harder, if we want to preserve that users still need to make an explicit choice.

Not sure I understand, could you clarify why is it hard to align the benchmark?

I guess the XCCDF way of making a choice is via profile tailorings, and actually making a choice via XCCDF variable will in fact require a tailoring. Maybe the difficulty is knowing witch rules need to be selected for each choice?

Exactly. When you build CaC, you'd get dozens of rules for a single firewall. CIS, STIG, ... everything all creates slightly different firewall rules and gets dumped into the datastream. When the sysadmin wants to change from the default firewalling agent to another (iptables->ufw in my above examples) -- they must not only be an expert in their organization's preferences/requirements -- AND in the benchmark itself -- BUT also in CaC to know exactly which rules to select that are actually relevant to the benchmark and not the other benchmarks so the tailoring can be created correctly.

Also, this is still a profile tailoring: it just changes it from a choice of N benchmark required rules + K organization policy rules, to a single variable change + K organization policy rules -- meaning the intersection/interaction of the N+K rules isn't muddied.

It looks like the variable-based CPE is trying to generalize the approach that some SSHD rules have with regards to "system install status" and "profile intention" (or user intention). For example: sshd_use_approved_macs_ordered_stig Together with the sshd_*_or_unset.xml in shared/checks/oval

These "sshd requires" mechanism were implemented by me a long time ago based on some quirky requirements (that may actually no have been true, meaning they can probably be removed.), they are a bit convoluted and confusing I admit. But it seems to me that the proposed variable-based CPEs can get quite complex and confusing as well.

I kinda got confused by "CPE" in the naming. I understand the proposed mechanism as a way to bundle up a few rules in a choice mechanism, but really determining applicability (validity or relevancy) of the rules.

Yeah, the name "pseudo-CPE" came about because originally, we were going to have the rules not selected (by the variable_ return "not applicable" -- just like CPE would've if the package wasn't installed. It was more of mimicing the functionality of CPE (from a user perspective) than the implementation from a benchmark creator's perspective.

However, OVAL doesn't allow that right now. :/

Other name suggestions welcome :-)

I see the difficulty in making available "choices" aligned with the policy, and see value in trying to make it easier for users to pick those choices. But I have a slight feeling this is adding a bit more complexity than necessary. Will think on it a bit more.

Yeah, if you know of another way of doing this, I'd be interested to hear it. We discussed it in broad terms on a face to face call, but at the time nobody came up with any alternative approaches and this is kinda the thing that worked.

@openshift-ci openshift-ci bot added the needs-rebase Used by openshift-ci bot. label Oct 11, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Used by openshift-ci bot. label Oct 11, 2021
@yuumasato
Copy link
Member

@cipherboy Just a short technical comment

Each rule with a pseudo-CPE selector then has its OVAL modified to essentially include a block like so:

<criteria operator="OR">
   <criterion id="not_variable">
   <criteria operator="AND">
       <criterion id="variable">
       ....existing criteria/criterion....
   </criteria>
</criteria> 

I did a local build and none of the ufw checks had the above construct, but the remediations had the check for variable=package.

Right, in the remediation it should all be firewall_choice == "ufw". Was that not the case?

Yes, the remediation has the check for `var_firewall_package, but the OVAL doesn't get the extra criteria from update_with_var_platforms()

@evgenyz
Copy link
Member

evgenyz commented Oct 13, 2021

@cipherboy @dodys Can you please explain to me why XCCDF's rule groups can't be used for this purpose?

@cipherboy
Copy link
Contributor

@evgenyz said:

@cipherboy @dodys Can you please explain to me why XCCDF's rule groups can't be used for this purpose?

Just to clarify, you mean the existing, hierarchical groups we already provision, <XCCDF:Group /> elements?

My understanding is getting foggy as time goes on (not doing as much XCCDF work any more...) -- but here's what I recall. Perhaps @dodys or @richardmaciel-canonical can fill in the gaps.

Groups, as currently created in CaC, necessarily aren't aligned with any benchmark. They're a reasonable grouping that provides some semblance of structure, based upon external, logical criteria -- services in one, programs in another, firewalls in a third, &c -- plus nesting. Additionally, they're globally static to a XCCDF/DS -- not dynamically computed based upon which Profile is selected.

Suppose we introduced groups called CISFirewallUFW and STIGFirewallUFW. Because rules must exist under a single group entity, they cannot be shared between these two groups; they're mutually elusive. (This holds further, that if we have CISFirewallIPTables, CISFirewallUFW, and CISFirewallFirewalld, we can't share the package-iptables-removed rule between CISFirewallUFW and CISFirewallFirewalld, even though they're reasonably both selectable under the same profile). So we have no way of creating any moderately complicated benchmark under existing grouping semantics. Additionally, we'd need a different group for each benchmark level. I think the ultimate end point of this path would be build-time created dynamic groups.

There's a way around this exclusivity shortcoming, though: we'd need to introduce rule extensions. Here, we could take a base definition of a rule and extend it (according to XCCDF extends="..." field semantics), blessing it with a new unique identifier -- but this doesn't allow for mutual exclusion. Namely, under this codified implementation above, we can assert that each concrete rule only gets run once. Per project guidelines, exactly one implementation of package-iptables-removed --- however, with extensions, there might be a dozen or more. This means that, if any manual changes to the XCCDF are made, an administrator must spend even more time and care than they do now, to ensure they select/deselect the right instance of that rule (if they're going that far out of the benchmark and aren't merely changing firewall implementations).

These two properties, IMO, make this implementation better. And the trade-off between not considering the rule at all (under grouping semantics proposed in this comment) vs pass/eventually not-applicable (as proposed in the codified implementation above) are hopefully alright.

@openshift-ci openshift-ci bot added the needs-rebase Used by openshift-ci bot. label Oct 17, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Used by openshift-ci bot. label Oct 18, 2021
@dodys
Copy link
Contributor Author

dodys commented Oct 18, 2021

@cipherboy Just a short technical comment

Each rule with a pseudo-CPE selector then has its OVAL modified to essentially include a block like so:

<criteria operator="OR">
   <criterion id="not_variable">
   <criteria operator="AND">
       <criterion id="variable">
       ....existing criteria/criterion....
   </criteria>
</criteria> 

I did a local build and none of the ufw checks had the above construct, but the remediations had the check for variable=package.

Right, in the remediation it should all be firewall_choice == "ufw". Was that not the case?

Yes, the remediation has the check for `var_firewall_package, but the OVAL doesn't get the extra criteria from update_with_var_platforms()

I've added a missing commit that hopefully fixes it.

Also rebased and since there were changes done to build_yaml.py Profile class and Group class (removed from_yaml(), @cipherboy you might want to review if that's still fine.

@openshift-ci openshift-ci bot added the needs-rebase Used by openshift-ci bot. label Nov 5, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Used by openshift-ci bot. label Nov 8, 2021
@dodys
Copy link
Contributor Author

dodys commented Nov 8, 2021

/retest

@openshift-ci
Copy link

openshift-ci bot commented Nov 8, 2021

@dodys: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dodys
Copy link
Contributor Author

dodys commented Nov 8, 2021

@yuumasato is this build failure after my rebase related to PR #7808, that you mentioned about SCE checks?
or is it something else?

@yuumasato
Copy link
Member

@dodys Not sure which build failure you are talking about.
The two failures I see are about the content test filter, I have restarted them.

@openshift-ci openshift-ci bot removed the needs-rebase Used by openshift-ci bot. label Feb 10, 2022
@Mab879
Copy link
Member

Mab879 commented Feb 15, 2022

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Used by openshift-ci bot. and removed needs-ok-to-test Used by openshift-ci bot. labels Feb 15, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 21, 2022

@dodys: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ocp4-moderate c5b263f link true /test e2e-aws-ocp4-moderate
ci/prow/e2e-aws-ocp4-cis-node c5b263f link true /test e2e-aws-ocp4-cis-node
ci/prow/e2e-aws-ocp4-pci-dss-node c5b263f link true /test e2e-aws-ocp4-pci-dss-node
ci/prow/e2e-aws-rhcos4-high c5b263f link true /test e2e-aws-rhcos4-high
ci/prow/e2e-aws-ocp4-high-node c5b263f link true /test e2e-aws-ocp4-high-node

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci
Copy link

openshift-ci bot commented Mar 31, 2022

@dodys: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-rebase Used by openshift-ci bot. label Mar 31, 2022
@matejak matejak self-assigned this Apr 26, 2022
@matejak
Copy link
Member

matejak commented Apr 26, 2022

Is this PR alive? Conflicts can be resolved easily, but the Python code changes to the build system will need refactoring to increase their maintainability, and it is also possible that the new CPE system that somehow unifies the system of XCCDF applicability with applicability of individual remediations may change the game a little bit.

@dodys
Copy link
Contributor Author

dodys commented Apr 26, 2022

Is this PR alive? Conflicts can be resolved easily, but the Python code changes to the build system will need refactoring to increase their maintainability, and it is also possible that the new CPE system that somehow unifies the system of XCCDF applicability with applicability of individual remediations may change the game a little bit.

I'm planning on dropping the CPE changes and just push the UFW changes instead, I will create a new PR for that when the time comes.
Closing this.

@dodys dodys closed this Apr 26, 2022
@dodys dodys deleted the ufw-rules branch March 31, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Used by openshift-ci bot. ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants