OVAL object model #11041

Honny1 · 2023-08-31T12:58:30Z

Description:

This PR introduces the OVAL object model into the SSG module. The OVAL object model can currently read XML OVAL documents and save yourself to XML OVAL document. The generated file is a valid XML OVAL document according to the oscap oval validate <file> command. The model can be loaded from OVAL shorthand. Integration into the build system and other features will be introduced in future PRs.

Doc with features ideas

Rationale:

Epic: OPENSCAP-2183

Review Hints:

Class Diagram:

Test script

pastebin.com/sSGMFREZ
In the root directory execute the test script via the commands below.

source .pyenv.sh # Optional step to avoid import errors.
python3 test.py

Run pytest

source .pyenv.sh  # Optional step to avoid import errors.
pytest tests/unit/ssg-module/test_oval_object_model/

openshift-ci · 2023-08-31T12:58:41Z

Hi @Honny1. 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.

github-actions · 2023-08-31T13:00:04Z

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

Fedora Environment

Oracle Linux 8 Environment

jan-cerny · 2023-08-31T14:35:09Z

I haven't reviewed the PR thoroughly, but I have some thoughts:

this surely needs unit tests
some of the constants, eg. namespaces, are already defined in ssg.constants module
we will probably need a way to distinguish between different types of OVAL tests, eg. textfilecontent54_object, selinuxboolean_object etc. Ditto for objects and states.

Honny1 · 2023-09-01T15:19:46Z

@jan-cerny
I'll check ssg.constants and try to fix the problems found in Code Climate. But I would like to keep some constants close to the oval_object_model subpackage. A lot of things are TBD. I plan to create unit tests in the next commits.

As for objects, states, and variables, I have created abstract logic. Because these endpoints are very similar. If we are interested in collecting objects, for example, I can create a specific object class that extends the current class. The endpoint type(object/state/variable) can be determined from the class instance of the attribute tag.

jan-cerny

I like this PR very much, amazing!

I only have nit picks:

please make all the CI jobs green
please resolve the Code Climate problems
see my comments below

jan-cerny · 2023-09-08T09:00:57Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+
+
+def test_content_variable():
+    assert False


this will always fail

jan-cerny · 2023-09-08T09:01:59Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+    assert text_file_content_object.list_of_property
+    list_of_property = [
+        OVALEntityProperty(
+            "{http://oval.mitre.org/XMLSchema/oval-definitions-5#independent}filepath",


you should use namespace definitions from ssg/constants.py.

jan-cerny · 2023-09-08T09:03:37Z

ssg/oval_object_model/oval_entities/object.py

+        if notes_el == child_node_el:
+            continue


What is the reason for this special handling of notes element?

Because the notes element is represented as a class. The notes element is loaded using the load_notes() function.

I think it would need at least a comment there.

I have an idea how to make it clear.

jan-cerny · 2023-09-08T09:04:52Z

ssg/oval_object_model/general.py

+BOOL_TO_STR = {True: "true", False: "false"}
+
+OVAL_NAMESPACES = {
+    "oval": "http://oval.mitre.org/XMLSchema/oval-common-5",


If this isn't already defined in ssg/constants.py you should put it there and not here.

Should I move all constants from the OVAL object model to the ssg/constants.py file?

In general, I would say that constants that have a likelihood of being used elsewhere in the project should be moved to ssg/constants.py. Namespace URLs are an example of constants that should definitely go to ssg/constants.py. On the other hand, you can still keep some constants in this module, but those should be constants that are extremely unique for this module and you don't expect reusing them anywhere else in future.

jan-cerny · 2023-09-08T09:12:56Z

ssg/oval_object_model/oval_document.py

+        return "external_variable" in component.tag
+
+    @staticmethod
+    def _add_oval_component(component, component_dict):


This method feels too complex. Also, I'd prefer to build the exception messages using .format() instead of + operators.

jan-cerny · 2023-09-08T09:19:03Z

ssg/oval_object_model/oval_document.py

+        )
+
+        space = "         "
+        oval_definition_el.set(


This looks really suspicious. The library should declare the namespaces and prefixes automatically without you touching it.

In this case, a special attribute with links to schemas is created. This attribute is intended for the validator. For this purpose, the namespace and attributes are set manually.

Could you give an example of the special attribute? I think we need to take a detailed look into this.

The xsi:schemaLocation attribute is with a namespace. I figured out how to add this attribute with namespace correctly. It's nothing special. But in the OVAL context, it's a different type of attribute. (It is not specified by OVAL XSD schemas)

jan-cerny · 2023-09-08T09:40:07Z

ssg/oval_object_model/general.py

+        tag,
+        id_,
+        version,
+        list_of_property,


Would properties be a better name for this variable?

jan-cerny · 2023-09-08T09:42:00Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+    "..",
+)
+OVAL_DOCUMENT_PATH_FROM_BUILD = os.path.join(
+    PROJECT_ROOT, "build", "ssg-rhel9-oval.xml"


What if a different product would be built? I think that a unit test should never depend on built artifacts.

jan-cerny · 2023-09-08T09:43:45Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+@pytest.fixture
+def oval_document_from_build():
+    return _load_oval_document(OVAL_DOCUMENT_PATH_FROM_BUILD)


No, we shouldn't do this, we need to invent a different way. I think that a unit test should never depend on built artifacts. I'm afraid that as the contents of the real products will change the test can start failing.

I can see that you already have other tests testing on test data so I think that you should remove this and move the checks to the test cases that test on the the test data.

jan-cerny · 2023-09-08T09:46:02Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+
+
+def test_content_state():
+    assert False


jan-cerny · 2023-09-12T15:00:44Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+    assert text_file_content_object.properties
+
+    property_file_path = OVALEntityProperty(
+        "{{{}}}filepath".format(OVAL_NAMESPACES.independent)


I've got an idea to split the constructor parameter to 2 parameters: tag name and namespace. This would help us avoid this ugly format composition repeating frequently. WDYT?

I think it's a good idea.

jan-cerny · 2023-09-13T06:35:12Z

ssg/oval_object_model/oval_document.py

+def _get_xml_el(tag_name, xml_el):
+    el = xml_el.find("./{%s}%s" % (OVAL_NAMESPACES.definition, tag_name))
+    return el if el else []


The name of this function can be a little misleading for someone because the purpose of the function is to supply an iterable over the children, hence the empty list returned if the element isn't found.

jan-cerny · 2023-09-13T06:43:50Z

ssg/oval_object_model/oval_document.py

+            "{%s}oval_definitions" % OVAL_NAMESPACES.definition
+        )
+
+        register_namespaces({"xsi": "http://www.w3.org/2001/XMLSchema-instance"})


The URL needs to be moved to ssg/constants.py.

jan-cerny · 2023-09-13T06:44:25Z

ssg/oval_object_model/oval_document.py

+                "{1}{2}#linux linux-definitions-schema.xsd"
+            ).format(
+                OVAL_NAMESPACES.oval,
+                space,


Why is this space here?

I checked the documentation and found no specific requirement for a separator between pairs in xsi:schemaLocation. So, I will replace the nine spaces between pairs with two spaces for better visual separation.

I used the same number of spaces as used in documents generated with ./build_product.

jan-cerny · 2023-09-13T06:50:27Z

ssg/oval_object_model/oval_entities/definition.py

+    title_tag = "{http://oval.mitre.org/XMLSchema/oval-definitions-5}title"
+    description_tag = "{http://oval.mitre.org/XMLSchema/oval-definitions-5}description"


You should use the namespace from ssg/constants.py instead of hardcoding the URI here.

jan-cerny · 2023-09-13T07:03:10Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+    DATA_DIR, "minimal_oval_of_oval_ssg-sshd_rekey_limit_def.xml"
+)
+
+PROJECT_ROOT = os.path.join(


It looks like this variable is not used anywhere.

jan-cerny · 2023-09-13T07:12:06Z

tests/unit/ssg-module/test_oval_object_model/test_load_and_store.py

+    assert not oval_document_from_shorthand.variables
+    assert not oval_document_from_shorthand.states


Wouldn't it be better to have some variables and states in the document as well? That would help you test them as well.

Yes, I will prepare dog-food and extend the test.

jan-cerny · 2023-09-18T06:43:43Z

@Honny1 I'm missing any comment from you. Has every feedback been addressed?

marcusburghardt · 2023-09-18T09:07:54Z

/ok-to-test

Honny1 · 2023-09-18T11:19:32Z

@jan-cerny Not yet.

codeclimate · 2023-09-19T12:52:39Z

Code Climate has analyzed commit 4202324 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 91.3% (50% is the threshold).

This pull request will bring the total coverage in the repository to 56.8% (3.0% change).

View more on Code Climate.

jan-cerny

I have reviewed the code. i have executed the unit tests locally and they passed. I have used your test pastebin but modified it for RHEL 9 and reviewed the generated OVAL and I have compared the input and the output. I have seen that every feedback has been addressed. Thanks for this PR.

openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Aug 31, 2023

openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Aug 31, 2023

jan-cerny added enhancement General enhancements to the project. Infrastructure Our content build system labels Aug 31, 2023

jan-cerny self-assigned this Sep 1, 2023

Honny1 force-pushed the oval-object-model branch 6 times, most recently from 69e2e1d to af94260 Compare September 6, 2023 15:53

jan-cerny requested changes Sep 8, 2023

View reviewed changes

Honny1 force-pushed the oval-object-model branch 2 times, most recently from fcd2c7f to 0cd5600 Compare September 9, 2023 16:20

openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Sep 9, 2023

Honny1 force-pushed the oval-object-model branch from 0cd5600 to 6cc2bdf Compare September 9, 2023 16:20

openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Sep 9, 2023

Honny1 force-pushed the oval-object-model branch 9 times, most recently from a89de7d to 4b576b0 Compare September 9, 2023 20:17

Honny1 force-pushed the oval-object-model branch from dadfae3 to e7fd463 Compare September 12, 2023 14:23

jan-cerny reviewed Sep 12, 2023

View reviewed changes

Honny1 force-pushed the oval-object-model branch from e7fd463 to 056d22d Compare September 12, 2023 15:16

jan-cerny added this to the 0.1.70 milestone Sep 13, 2023

jan-cerny reviewed Sep 13, 2023

View reviewed changes

Honny1 force-pushed the oval-object-model branch from 056d22d to 1afa780 Compare September 13, 2023 17:59

openshift-ci bot added ok-to-test Used by openshift-ci bot. and removed needs-ok-to-test Used by openshift-ci bot. labels Sep 18, 2023

Honny1 added 7 commits September 19, 2023 10:46

Add general utilities for OVAL object model

2288502

Add object reprezentation of OVAL entities

d77a7f8

Add object of OVAL document

6171dda

Rename OVALEndPoint to OVAL Entity

8b30db2

Create tests for load and store of an OVAL document

e855b27

Fix Code climate and python2 support

ecf7e5b

Move constant from ssg/oval_object_model/general.py to ssg/constants.py

0c8e0e5

Honny1 force-pushed the oval-object-model branch from 225505f to f372989 Compare September 19, 2023 08:47

Honny1 added 3 commits September 19, 2023 12:08

Separate tag and namespace

6dfb218

Fix misleading code

6fc1993

Fix attrubite with namespaces

0cf25e4

Honny1 force-pushed the oval-object-model branch from f372989 to 0cf25e4 Compare September 19, 2023 10:09

Add shorthand with variable and state

4202324

Honny1 marked this pull request as ready for review September 19, 2023 14:19

openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 19, 2023

Honny1 requested a review from jan-cerny September 19, 2023 14:19

jan-cerny approved these changes Sep 19, 2023

View reviewed changes

jan-cerny merged commit 9d151bc into ComplianceAsCode:master Sep 19, 2023

		title_tag = "{http://oval.mitre.org/XMLSchema/oval-definitions-5}title"
		description_tag = "{http://oval.mitre.org/XMLSchema/oval-definitions-5}description"

		assert not oval_document_from_shorthand.variables
		assert not oval_document_from_shorthand.states

OVAL object model #11041

OVAL object model #11041

Conversation

Honny1 commented Aug 31, 2023 • edited Loading

Description:

Rationale:

Review Hints:

Class Diagram:

Test script

Run pytest

openshift-ci bot commented Aug 31, 2023

github-actions bot commented Aug 31, 2023 • edited Loading

jan-cerny commented Aug 31, 2023

Honny1 commented Sep 1, 2023 • edited Loading

jan-cerny 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

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

Honny1 Sep 18, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honny1 Sep 18, 2023 • edited Loading

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

jan-cerny commented Sep 18, 2023

marcusburghardt commented Sep 18, 2023

Honny1 commented Sep 18, 2023

codeclimate bot commented Sep 19, 2023

jan-cerny left a comment

Choose a reason for hiding this comment

Honny1 commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited

Loading

Honny1 commented Sep 1, 2023 •

edited

Loading

Honny1 Sep 18, 2023 •

edited

Loading

Honny1 Sep 18, 2023 •

edited

Loading