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

implement parsing of logical expressions in platform definitions #7915

Conversation

vojtapolasek
Copy link
Collaborator

Description:

  • CPEALPlatformDefinition now subclasses Algebra class
    • this is due to the fact that if you want to compare two boolean expressions, they have to be apparently created by the same Algebra class
  • CPEALLogicalTest now subclasses Function
  • CPEALFactRef now subclasses Symbol
  • build_yaml.py is modified to make use of these new features while parsing platforms

Rationale:

  • this PR actually allows to use logical expressions within platform definitions. It converts them into CPE applicability language logical expressions which are then present in the resulting datastream.

@vojtapolasek vojtapolasek added Infrastructure Our content build system usability Enhancements related to usability. Highlight This PR/Issue should make it to the featured changelog. labels Nov 23, 2021
@vojtapolasek vojtapolasek added this to the 0.1.60 milestone Nov 23, 2021
@openshift-ci
Copy link

openshift-ci bot commented Nov 23, 2021

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 openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 23, 2021
@pep8speaks
Copy link

pep8speaks commented Nov 23, 2021

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-23 15:56:21 UTC

@github-actions
Copy link

github-actions bot commented Nov 23, 2021

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
OCIL for rule 'xccdf_org.ssgproject.content_rule_audit_rules_execution_restorecon' differs:
--- old datastream
+++ new datastream
@@ -1,6 +1,6 @@
 To verify that execution of the command is being audited, run the following command:
 $ sudo grep "path=/usr/sbin/restorecon" /etc/audit/audit.rules /etc/audit/rules.d/*
 The output should return something similar to:
--a always,exit -F path=/usr/sbin/restorecon -F perm=x -F auid>=1000 -F auid!=unset -F key=privileged
+-a always,exit -F path=/usr/sbin/restorecon -F auid>=1000 -F auid!=unset -F key=privileged
 Is it the case that ?
 
OCIL for rule 'xccdf_org.ssgproject.content_rule_audit_rules_execution_seunshare' differs:
--- old datastream
+++ new datastream
@@ -1,6 +1,6 @@
 To verify that execution of the command is being audited, run the following command:
 $ sudo grep "path=/usr/sbin/seunshare" /etc/audit/audit.rules /etc/audit/rules.d/*
 The output should return something similar to:
--a always,exit -F path=/usr/sbin/seunshare -F perm=x -F auid>=1000 -F auid!=unset -F key=privileged
+-a always,exit -F path=/usr/sbin/seunshare -F auid>=1000 -F auid!=unset -F key=privileged
 Is it the case that ?
 
bash remediation for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_set_maxpoll' differs:
--- old datastream
+++ new datastream
@@ -1,5 +1,5 @@
 # Remediation is applicable only in certain platforms
-if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && { rpm --quiet -q chrony || rpm --quiet -q ntp; }; then
+if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && { rpm --quiet -q ntp or chrony; }; then
 
 var_time_service_set_maxpoll=''
 

bash remediation for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_specify_remote_server' differs:
--- old datastream
+++ new datastream
@@ -1,5 +1,5 @@
 # Remediation is applicable only in certain platforms
-if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]; then
+if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && { rpm --quiet -q machine and (chrony or ntp); }; then
 
 var_multiple_time_servers=''
 

Platform has been changed for rule 'xccdf_org.ssgproject.content_rule_chronyd_or_ntpd_specify_remote_server'
--- old datastream
+++ new datastream
-['cpe:/a:machine']
+['cpe:/a:chrony', 'cpe:/a:ntp', 'cpe:/a:machine']

@evgenyz
Copy link
Member

evgenyz commented Nov 23, 2021

Was inclusion of Add rule for SC-5: resource_requests_quota commit intentional? Can't we test on something simpler?

@vojtapolasek vojtapolasek force-pushed the cpe_logical_expressions branch from cf639b6 to fbe2649 Compare November 23, 2021 15:54
@vojtapolasek vojtapolasek force-pushed the cpe_logical_expressions branch from fbe2649 to 5928958 Compare November 23, 2021 15:56
@vojtapolasek
Copy link
Collaborator Author

No, it was a mistake. I rebased and it should be fine now.

@vojtapolasek
Copy link
Collaborator Author

One thing I notice is that current implementation breaks our way in which we modify remediations based on platforms. We are supposing that platform names are usually package names, but they are not anymore. SEe the datastream diff. We need to fix it somehow.

@vojtapolasek
Copy link
Collaborator Author

@evgenyz We have a problem with Python2. Run following:

import ssg.boolean_expression as be
a = be.Algebra(function_cls=be.Function, symbol_cls=be.Symbol)
e = a.parse("not_s390x_arch", simplify=True)
str(a)

With Python 3, it returns not_s390x_arch.
With Python 2, it returns not-s390x-arch.
Could it be a problem with the pkg_resources module between Python versions?

@evgenyz
Copy link
Member

evgenyz commented Nov 24, 2021

With Python 3, it returns not_s390x_arch. With Python 2, it returns not-s390x-arch. Could it be a problem with the pkg_resources module between Python versions?

It could be. But the expression is weird all by itself. The not could easily be extracted. And _arch seems to be redundant.

Nonetheless, I'll check _ and - handling.

@evgenyz
Copy link
Member

evgenyz commented Nov 24, 2021

Could it be a problem with the pkg_resources module between Python versions?

The problem is indeed in pkg_resources: pypa/setuptools#2522. We can either monkey-patch it (bugfood/fpm@fa91e61) or disallow _.

@matejak
Copy link
Member

matejak commented Nov 30, 2021

I don't like the idea of subclassing. The CPEAL... classes shouldn't even know how the applicability was defined, the only thing that they should use is the interface. Subclassing boolean_expression types means that those classes know too much, and that they drag dependencies along - doesn't it look strange that you can't e.g. run a test of CPEALLogicalTest unless you have the functionality of boolean_expression available?

self._replace_cpe_names(self.test)

def _replace_cpe_names(self, exp):
if isinstance(exp, CPEALFactRef):
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to call something like exp.replace_cpe_name() that would either perform the operation, or it would recurse. The current approach has the CPEALPlatform class that knows too much - instead of telling exp to do the right thing, we either treat it in a special way, or we recurse on its children.

@@ -175,15 +174,24 @@ class CPEALPlatformSpecification(object):
prefix = "cpe-lang"
ns = PREFIX_TO_NS[prefix]

def __init__(self):
def __init__(self, cpe_products):
Copy link
Member

Choose a reason for hiding this comment

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

This class basically parses platform definitions into expressions, and maintains a list of distinct platforms. This functionality could be relocated to a lower level - a Rule data structure could contain the parsed expression (i.e. the logical tree) already. Moreover, having a list of platforms close to the list of rules would make sense as well.

@vojtapolasek
Copy link
Collaborator Author

Thank you all for the feedback. It i s really valuable. I am closing this PR in favor of #7950

@yuumasato yuumasato removed the Highlight This PR/Issue should make it to the featured changelog. label Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Used by openshift-ci bot. Infrastructure Our content build system usability Enhancements related to usability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants