Improve the autotailor script #2039

jan-cerny · 2023-10-03T14:09:18Z

This pull request improves the autotailor utility. The goal of the improvements is that the autotailor can be used for real-world usage and easily extended in future. Specifically, these are the changes made in this PR:

added unit tests
added integration test
refactoring
coding style
add options --rule-role and --rule-severity to refine rules roles and severities

For more details, please read commit messages of every commit.

This PR supersedes PR #1877.

Instead of repeating the smae string all over the place, let's have a single constant defined and then used.

If the `--new-profile-id` isn't provided by the user, we will create the ID of the customized profile ID by appending the `_customized` suffix to the base profile ID. This change makes the behavior according to the help text of the `--new-profile-id` option: > If left out, the new ID will be obtained by appending '_customized' > to the tailored profile ID.

and fix the failed asserts

We will move the condition that determines the ID of the customized profile into the `Tailoring` class. This move helps encapsulate code and also allows easier unit testing of this feature which we immediately use to write a unit test in this commit as well :)

This patch adds two new command line options `--rule-role` and `--rule-severity` that will allow users to refine rule role and rule seveirty in their customized profile. Using these options will generate `refine-rule` elements within the output tailoring file. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2058168

This patch moves the logic for assigning the XCCDF Values for outer space inside the Tailoring class by defining a method.

Explicitly setting namespaces of elements is the recommended way of using namespaces. It helps prevent namespace errors. Also, it simplifies unit testing of code that works with XML elements.

The new name of the variable better describes its actual contents.

This commit adds an integration test for autotailor. The goal of the test is to verify if the tailoring produced by autotailor can be loaded and consumed by oscap and if the generated tailoring leads to the intended behavior of the profile evaluation.

The result will be that the autotailor unit tests will be executed during the CTest which is run in our CI.

evgenyz

In general I approve the changes. Awesome! Thank you.

There are a couple of nitpicks, though. See individual notes.

Also I imagined that a new tool could use little bit more of modern syntax sugar. What do you think, should we embrace f-strings in autotailor?

evgenyz · 2023-10-04T19:13:50Z

tests/utils/autotailor_integration_test.sh

+assert_exists 1 '/Benchmark/TestResult/rule-result[@idref="xccdf_com.example.www_rule_R1"]/result[text()="pass"]'
+assert_exists 1 '/Benchmark/TestResult/rule-result[@idref="xccdf_com.example.www_rule_R2"]/result[text()="pass"]'
+assert_exists 1 '/Benchmark/TestResult/rule-result[@idref="xccdf_com.example.www_rule_R3"]/result[text()="notselected"]'
+assert_exists 1 '/Benchmark/TestResult/rule-result[@idref="xccdf_com.example.www_rule_R4"]/result[text()="notselected"]'


No newline.

evgenyz · 2023-10-04T19:35:15Z

utils/autotailor.8

@@ -5,7 +5,7 @@ autotailor \- CLI tool for tailoring of SCAP data streams.
 autotailor produces tailoring files that SCAP-compliant scanners can use to complement SCAP data streams.
 A tailoring file adds a new profile, which is supposed to extend a profile that is already present in the data stream.

-Tailoring can add or remove rules, and it can redefine contents of XCCDF variables.
+Tailoring can add or remove rules, refine rules, and it can redefine contents of XCCDF variables.


Suggested change

Tailoring can add or remove rules, refine rules, and it can redefine contents of XCCDF variables.

Tailoring can add, remove or refine rules, and it also can redefine contents of XCCDF variables.

evgenyz · 2023-10-04T19:44:50Z

utils/autotailor

+    @staticmethod
+    def _validate_rule_refinement_params(rule_id, attribute, value):
+        if not is_valid_xccdf_id(rule_id):
+            msg = "Rule id '{rule_id}' is invalid!".format(rule_id=rule_id)


As we operate with Python 3 in 1.3.x we probably can do this:

Suggested change

msg = "Rule id '{rule_id}' is invalid!".format(rule_id=rule_id)

msg = f"Rule id '{rule_id}' is invalid!"

evgenyz · 2023-10-04T19:45:21Z

utils/autotailor

        benchmark.set("href", datastream_uri)

-        version = ET.SubElement(root, "xccdf-1.2:version")
+        version = ET.SubElement(root, "{%s}version" % NS)


And this:

Suggested change

version = ET.SubElement(root, "{%s}version" % NS)

version = ET.SubElement(root, f"{NS}version")

You actually need to escape the braces: f"{{{NS}}}version", so here I prefer to use the % form.

evgenyz · 2023-10-04T19:46:09Z

utils/autotailor

-        "If left out, the new ID will be obtained "
+        "The ID of the new profile can be either its full ID, or the suffix, "
+        "in which case the 'xccdf_<id-namespace>_profile_' prefix will be "
+        "prepended internally. If left out, the new ID will be obtained "
        "by appending '{suffix}' to the tailored profile ID."
        .format(suffix=DEFAULT_PROFILE_SUFFIX))


But this might not be worthy of converting into a block of f-strings.

evgenyz · 2023-10-04T19:48:45Z

utils/autotailor

+            "Can't refine {attribute} of rule '{rule_id}' to '{value}'. "
+            "Allowed {attribute} values are: {allowed}.".format(
+                attribute=attribute, rule_id=rule_id, value=value,
+                allowed=allowed))


And this: f"Can't refine {attribute} of rule '{rule_id}' to '{value}'. Allowed {attribute} values are: {allowed}.".

evgenyz · 2023-10-05T12:25:03Z

Test failures are not connected to the autotailor.

matejak and others added 19 commits October 2, 2023 13:54

Refactor autotailor for future exension

93efdd6

Add autotailor unit test stub.

2aacfbd

Use a single constant definition

0f4c498

Instead of repeating the smae string all over the place, let's have a single constant defined and then used.

Reconcile the ID_NAMESPACE variable

48e4069

Add unit test for is_valid_xccdf_id

c3e3210

and fix the failed asserts

Add unit tests for _full_id_* methods

fbc49d3

Fix PEP 8 problems

07b5ae7

Update man page

a8ce89a

Move XCCDF Value assignment to Tailoring class

4256956

This patch moves the logic for assigning the XCCDF Values for outer space inside the Tailoring class by defining a method.

Use a standard way of handling XML namespaces

a46b771

Explicitly setting namespaces of elements is the recommended way of using namespaces. It helps prevent namespace errors. Also, it simplifies unit testing of code that works with XML elements.

Rename and merge a variable

ae3874e

The new name of the variable better describes its actual contents.

Add autotailor unit tests to CTest

48c013b

The result will be that the autotailor unit tests will be executed during the CTest which is run in our CI.

Add pytest for the CI dependencies

4afcbae

Remove unused imports

9596edb

Fix PEP 8 problems

bd68511

jan-cerny added the enhancement label Oct 3, 2023

jan-cerny added this to the 1.3.10 milestone Oct 3, 2023

jan-cerny mentioned this pull request Oct 3, 2023

Add a refine-rule tailoring ability to autotailor #1877

Closed

jan-cerny requested review from matejak, matusmarhefka and evgenyz October 3, 2023 14:13

evgenyz self-assigned this Oct 4, 2023

evgenyz requested changes Oct 4, 2023

View reviewed changes

evgenyz reviewed Oct 4, 2023

View reviewed changes

jan-cerny added 3 commits October 5, 2023 13:25

Add a missing newline

d0c1de5

Improve wording of the man page

015f918

Replace format methods by F-strings

e1d1458

evgenyz merged commit e0ea126 into OpenSCAP:maint-1.3 Oct 5, 2023
12 of 20 checks passed

evgenyz mentioned this pull request Oct 19, 2023

stages:oscap.autotailor: add key/value overrides osbuild/osbuild#1407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the autotailor script #2039

Improve the autotailor script #2039

jan-cerny commented Oct 3, 2023

evgenyz left a comment

evgenyz Oct 4, 2023

evgenyz Oct 4, 2023

evgenyz Oct 4, 2023

evgenyz Oct 4, 2023

jan-cerny Oct 5, 2023

evgenyz Oct 5, 2023

evgenyz Oct 4, 2023

evgenyz Oct 4, 2023

evgenyz commented Oct 5, 2023

	Tailoring can add or remove rules, refine rules, and it can redefine contents of XCCDF variables.
	Tailoring can add, remove or refine rules, and it also can redefine contents of XCCDF variables.

	msg = "Rule id '{rule_id}' is invalid!".format(rule_id=rule_id)
	msg = f"Rule id '{rule_id}' is invalid!"

	version = ET.SubElement(root, "{%s}version" % NS)
	version = ET.SubElement(root, f"{NS}version")

Improve the autotailor script #2039

Improve the autotailor script #2039

Conversation

jan-cerny commented Oct 3, 2023

evgenyz 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

evgenyz commented Oct 5, 2023