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

Generate the CPE Dictionary dynamically #6304

Merged
merged 17 commits into from
Nov 11, 2020

Conversation

yuumasato
Copy link
Member

@yuumasato yuumasato commented Oct 27, 2020

Description:

  • This PR attempts to simplify definition syntax of CPE
    • Removes the products' static cpe-dictionary and generate them dynamically based on the platform elements of the content in the shorthand xml.
    • The build system identifies the required CPEs and generates the appropriate product cpe-dictionary.
  • The the main product doesn't need to contain cpe-items related to derivate product anymore.
    • CPEs for derivative products are injected in the derivative product's cpe-dictionary.
  • Product related CPEs are now defined in their respective product.yml file.
  • Rule related CPEs are sourced from cpes_root defined in products.yml file.
    • For the CPEs defined in upstream, cpes_root is shared/applicability/cpes.yml.
  • Remove constants XCCDF_PLATFORM_TO_CPE and PRODUCT_TO_CPE_MAPPING.
    These mapping are now generated dynamically, based on the cpes defined to the product.

Rationale:

  • Simplify CPE declaration
  • Avoids the need to manually add a new cpe-item for each product.

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 27, 2020
@openscap-ci
Copy link
Collaborator

openscap-ci commented Oct 27, 2020

Changes identified:
Others:
 Changes in Python files.

Show details

Others:
 Python abstract syntax tree change found in build-scripts/cpe_generate.py.
 Python abstract syntax tree change found in build-scripts/enable_derivatives.py.
 Python abstract syntax tree change found in build-scripts/yaml_to_shorthand.py.
 Python abstract syntax tree change found in ssg/build_cpe.py.
 Python abstract syntax tree change found in ssg/build_derivatives.py.
 Python abstract syntax tree change found in ssg/build_yaml.py.
 Python abstract syntax tree change found in ssg/constants.py.
 Python abstract syntax tree change found in ssg/products.py.
 Python abstract syntax tree change found in tests/ssg_test_suite/common.py.

Recommended tests to execute:
 (cd build && cmake ../ && ctest -j4)

@yuumasato yuumasato force-pushed the unify_cpe_dictionaries branch from 80f0d77 to 22438ae Compare October 27, 2020 20:19
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
shared/applicability/cpes.yml Outdated Show resolved Hide resolved
ssg/constants.py Outdated
],
"centos8": [
"cpe:/o:centos:centos:8",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

all cpes need to be dropped from constants.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually a lot of CPE mappings can be dropped from ssg/constants.py.
I'm not sure if it would better be done in this PR or a subsequent one.
I lean towards doing it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

PRODUCT_TO_CPE_MAPPING was removed

"cpe:/a:redhat:enterprise_virtualization_manager:4":
id: rhvm4
title: "Red Hat Virtualization 4 Manager"
check_id: installed_app_is_rhv4
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall structure needs to be cleaner and clearer like so:

    - id: rhvm4
      title: "Red Hat Virtualization 4 Manager"
      cpe:  "cpe:/a:redhat:enterprise_virtualization_manager:4"
      check_id: installed_app_is_rhv4

Copy link
Contributor

Choose a reason for hiding this comment

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

there also needs to be an additional key called additional_cpes or include_cpes that maps to shared/applicability/cpes.yml

Copy link
Member Author

Choose a reason for hiding this comment

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

there also needs to be an additional key called additional_cpes or include_cpes that maps to shared/applicability/cpes.yml

Why, if the CPEs are picked up automatically based on the content included in the benchmark?

Copy link
Contributor

Choose a reason for hiding this comment

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

To not only make the content readable and understandable, but also remove the abstraction of the build system. Makes it easy for content authors and everyone to read and understand additional cpe is being added in product.yml

Copy link
Member Author

@yuumasato yuumasato Oct 29, 2020

Choose a reason for hiding this comment

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

I find it rather cumbersome to, after selecting a rule in the profile, or adding to a rule's prodtype, to have to add any cpe-item it uses to the product.yml.
I'd expect the build-system to be handle that automatically for me (include the CPE in the dictionary of the product).

I think the readability and understandability should be solved in another way.
Maybe "compiled" products yamls? Build logs?

Copy link
Member

Choose a reason for hiding this comment

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

... having the cpe in the product.yml makes it easy for content authors to understand...

Imagine that there is a CPE in product.yml, and I think that it got there by mistake, or that the reason why it was introduced is not valid any more. What would I have to do in order to resolve those doubts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I don't understand where you are heading...

If a CPE entry in a product.yml was not trivial, I would expect comment lines, if there weren't any I'd look into the commit that added it.

Copy link
Member

Choose a reason for hiding this comment

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

Everything has a cost - if the automation performs some magic behind the content author's back, that can have a cost, especially when the magic doesn't work right, or if the author would like something slightly different.

Conversely, if there is no automation, the obvious cost is that you have to add config items s.a. CPEs manually. But that's not where it ends - you get risk of copy-paste errors, and you also have to deal with removal of items that are no longer needed or that have been removed by refactoring.

I think I understand @redhatrises concern of automating too much, and I think that we should look both ways - what are the risks of automating too little?
So far it seems that we should require non-trivial manually added CPEs to be described by comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that automation can make it hard to do something slightly different. But that was not the original point of the question, and I think it can be handled in a different way.

So far it seems that we should require non-trivial manually added CPEs to be described by comments.

Now I'm not sure if your previous question was about:

  • the manually added CPE for the product in product.yml or
  • the automatically added CPEs in the product's cpe-dictionary.

To clarify, the automation doesn't add anything to product.yml file, it will include cpe-item in the cpe-dictionary, as the files removed by this PR (rhel8/cpe/rhel8-cpe-dictionary.xml).
And to check the CPEs added by the build system one can check the built dictionary. It should have the same complexity as reviewing the old static files.

Copy link
Member

Choose a reason for hiding this comment

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

My question was about items that would have to be added to product.yml if there is no automation. The automation is supposed to make sure that only and only CPEs that are needed get added. Conversely, when a CPE is added manually, it has to be kept in sync with the actual content.

You have answered the question, thanks.

@pep8speaks
Copy link

pep8speaks commented Oct 29, 2020

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

Line 113:100: E501 line too long (109 > 99 characters)

Line 100:66: E225 missing whitespace around operator
Line 100:100: E501 line too long (103 > 99 characters)
Line 103:5: E303 too many blank lines (2)
Line 108:17: E201 whitespace after '['
Line 108:64: E202 whitespace before ']'

Comment last updated at 2020-11-06 14:22:44 UTC

@yuumasato yuumasato force-pushed the unify_cpe_dictionaries branch from 6de663d to a1f18b5 Compare October 29, 2020 20:15
@yuumasato yuumasato force-pushed the unify_cpe_dictionaries branch from a1f18b5 to 892ee23 Compare October 29, 2020 21:50
@yuumasato yuumasato marked this pull request as ready for review October 29, 2020 22:01
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 29, 2020
@yuumasato
Copy link
Member Author

I'm currently updating documentation around CPEs.
Don't merge yet, please.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Used by openshift-ci bot. label Nov 1, 2020
chromium/product.yml Outdated Show resolved Hide resolved
chromium/product.yml Outdated Show resolved Hide resolved
jre/product.yml Outdated
jre-oracle:
name: "cpe:/a:oracle:jre:"
title: "Oracle's Java Runtime Environment"
check_id: installed_app_is_java
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need jre-oracle? Why not skip it and start with - name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The product CPES don't need to have these jre-oracle and jre-ibm... They are like IDs for the CPEs, but are not actually used by the build system.

But, the IDs of the CPEs in shared/applicability/cpes.yml are needed, these are IDs are referenced by rules and profiles.
I just tried to keep same declaration format.

Copy link
Contributor

Choose a reason for hiding this comment

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

The product CPES don't need to have these jre-oracle and jre-ibm... They are like IDs for the CPEs, but are not actually used by the build system.

But, the IDs of the CPEs in shared/applicability/cpes.yml are needed, these are IDs are referenced by rules and profiles.
I just tried to keep same declaration format.

So, if I am understanding you correctly, jre-oracle, jre-ibm, etc. aren't needed by the build system in product.yml but are needed by the build system in cpes.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the IDs are there for any content to refer to them.
But as they are already declared in the product.yml they are for, nothing else references them.
Rules and profiles refer to the IDs of cpes declared in cpes.yml.

I'd rather keep the syntax the same for cpes in product.yml and cpes.yml.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the CPE IDs are needed.
The ocp4 profiles reference CPE's defined in the product.yml.

chromium:
name: "cpe:/a:google:chromium-browser"
title: "Google Chromium Browser"
check_id: installed_app_is_chromium
Copy link
Contributor

Choose a reason for hiding this comment

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

Just like the profiles_root, I want to have additional_cpes in this file, because as a content author, I don't want to choose to add cpes.yml or not. I might come with my own cpes.yml file that doesn't contain what we have now.

Copy link
Member Author

@yuumasato yuumasato Nov 4, 2020

Choose a reason for hiding this comment

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

So you'd like to be able to choose the file to source the CPE's from?

So if product.yml specified a directory, and any yml (or even xml) file there was sourced. Would it satisfy your needs?

cpes_root: "../shared/applicability"

Copy link
Contributor

Choose a reason for hiding this comment

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

So you'd like to be able to choose the file to source the CPE's from?

So if product.yml specified a directory, and any yml (or even xml) file there was sourced. Would it satisfy your needs?

cpes_root: "../shared/applicability"

It definitely would address the needs!

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibility to load multiple source CPE yml files added in 05506be

@yuumasato yuumasato force-pushed the unify_cpe_dictionaries branch from 9bce03a to 05506be Compare November 5, 2020 17:29
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Nov 5, 2020
@yuumasato yuumasato marked this pull request as draft November 5, 2020 17:41
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 5, 2020
@yuumasato yuumasato marked this pull request as ready for review November 5, 2020 21:12
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 5, 2020
To avoid the need to manually add a new cpe-item for each product,
let's generate the list of cpe-items based on the platforms included in the
Benchmark (i.e. shorthand).

The cpe-item for the derivate product is injected by the
enable_derivatives.py script. The the main product doesn't need to
contain cpe-items related to derivate product anymore.
Do no derivate the reference OVAL file from the cpe reference.
They may be the same, but let's not carry this assumption.
Refactored the CPE loader to understand the new format and simpler.
One definition was overwriting the other.
The mapping is not static anymore, it is sourced directly from the
cpes.yml file.
This should make management of product related CPEs easier.
From feedback, adding "-" improves readability of the CPEs.
This changes the value of 'cpes' from a dictonary to a list of dictionaries.
Add parameter to product.yml to specify source directory of CPEs.
As the source of CPEs can vary from product to product, now the
"CPE loader" is product specific (via ProductCPEs class).
Let's not use the statically geenerated mapping, and use ProductCPEs to
get the CPE names.
This mapping is now provided by class ProductCPEs via
get_product_cpe_names() call.
@openshift-ci-robot openshift-ci-robot added the needs-rebase Used by openshift-ci bot. label Nov 6, 2020
This should make it easy to manage the CPEs.
@yuumasato yuumasato force-pushed the unify_cpe_dictionaries branch from f4a00cc to 077eb49 Compare November 6, 2020 14:22
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Nov 6, 2020
@redhatrises
Copy link
Contributor

At this point, looks good to me. I also like how you broke out the shared CPEs into categories. I think that that is a nice touch.

@redhatrises redhatrises merged commit 27db2b8 into ComplianceAsCode:master Nov 11, 2020
@yuumasato yuumasato deleted the unify_cpe_dictionaries branch November 12, 2020 08:55
@yuumasato yuumasato added this to the 0.1.54 milestone Nov 12, 2020
ggbecker added a commit to ggbecker/content that referenced this pull request Nov 16, 2020
ggbecker added a commit to ggbecker/content that referenced this pull request Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants