Introduce machine and package platform conditionals to Bash remediations #6061

yuumasato · 2020-09-08T13:46:35Z

Description:

Trickle down the CPE Platform to the Bash remediations
- When a rule has a platform be it machine or a Package CPE platform, the whole Bash remediation is wrapped around an if condition
- Also adds conditionals for machine platform

Rationale:

When a rule has a Package CPE platform, like platform: dconf for example.
It is meant to be evaluated and remediated when the systems has dconf package installed.
Evaluation and remediation done with OpenSCAP respects these CPEs, as the scanner understands the CPE and the XCCDF:Platform field.
But when the Bash scripts are generated, the Package applicability of the remediation is lost.

Rationale:

Analogous to Introduce package conditionals to Ansible remediations #6025

pep8speaks · 2020-09-08T13:46:39Z

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

In the file ssg/build_remediations.py:

Line 315:100: E501 line too long (101 > 99 characters)

Comment last updated at 2020-09-10 15:27:00 UTC

matejak

Well done, there are some minor remarks, but the functionality works as expected.

The {} in format strings can be probably replaced by {0} to keep the code working on Python<2.7.
I see potential problems with tests, as I can imagine that sometest scenarios may misbehave due to the newly introduced "do nothing when it's not my business" functionality, but I see this as unlikely - OVALs are already platform-aware.

matejak · 2020-09-09T15:42:56Z

ssg/build_remediations.py

+                platform_conditionals.append(pkg_check_command.format(platform))
+
+        if platform_conditionals:
+            platform_fix_text = "# Remediation is applicable only in certain platforms\n"


It is advisable to construct a multi-line string by creating a list of lines and then joining it. The primary aspect are performance reasons, but I believe that it would be nicer as well.
Next, I would suggest placing a blank line between the header and the remediation body for greater readability.

matejak · 2020-09-09T15:44:30Z

ssg/build_remediations.py

+            platform_fix_text += "if {}".format(cond)
+            for cond in platform_conditionals:
+                platform_fix_text += " && {}".format(cond)
+            platform_fix_text += '; then\n{}\nelse\necho "Remediation is not applicable, nothing was done"\nfi'.format(result.contents)


I would use textwrap. indent to indent the innner original body of the remediation if it doesn't break our dark remediation generation magic.

What do you think of redirecting the echo to stderr?

Indenting the body of the the remediation didn't break the build nor the behavior.
Unfortunately, not everything is indented.
When the scanner performs the XCCDF substitutions, the Bash remediation functions are not properly indented.
Main issue being that they have multiple lines, and can be used "platform wrapped" or not.

@matejak indent is not available in python2.
Do you know of a trivial substitute?

Oh, that Python2 not being able to do this - that's a surprise. We can have an indent as an optional feature in the try-except block.
However, I wouldn't do it now, as it conflicts with those XCCDF substitutions as you have pointed out, and there is a small likelihood that some remediations may not be indent-safe - consider here-docs.

Interesting find. I'll drop adcb827 for now.

matejak · 2020-09-09T15:47:28Z

ssg/build_remediations.py

+            platform_fix_text = "# Remediation is applicable only in certain platforms\n"
+
+            cond = platform_conditionals.pop(0)
+            platform_fix_text += "if {}".format(cond)


I think that it could be slightly more elegant to join the platform_conditionals list by &&.

matejak · 2020-09-09T15:48:59Z

ssg/build_remediations.py

@@ -691,24 +738,27 @@ def expand_xccdf_subs(fix, remediation_type, remediation_functions):
        patcomp = re.compile(pattern, re.DOTALL)
        fixparts = re.split(patcomp, fix.text)
        if fixparts[0] is not None:


I can't wait for the moment when we take some of the existing magic out and simplify this function.

I suspect that when all bash remediation functions are converted to Jinja macros expand_xccdf_subs won't be needed anymore.

jan-cerny · 2020-09-10T11:31:50Z

Can you show us an example of the conditional?

matejak · 2020-09-10T14:59:58Z

Can you show us an example of the conditional?

That's easy - compile e.g. Fedora and then grep build/fixes/bash with &&, and you will see all sorts of platforms.

Regarding the PR, I gave a second thought on the indentation, and I suggest to put there a comment that there is such possibility, but there is a Python2/3 catch, plus a small risk for remediations themselves.

matejak · 2020-09-10T15:00:40Z

It may be that if you rebase against master, OCP tests will start to pass :-)

Wraps the remediation of rules with Packager CPE Platform with an if condition that checks for the respective platforms's package.

Adjust expansion of subs and variables not to remove the whole beginning of the fix test. This was removing the package conditional wrapping.

mildas · 2020-09-10T15:27:21Z

Changes identified:
Others:
Python abstract syntax tree change found in ssg/build_remediations.py.

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

matejak · 2020-09-11T10:05:43Z

/retest

matejak · 2020-09-14T12:49:56Z

ssg/build_remediations.py

+            wrapped_fix_text.append("{0}".format(result.contents))
+            wrapped_fix_text.append("")
+            wrapped_fix_text.append("else")
+            wrapped_fix_text.append("    >&2 echo 'Remediation is not applicable, nothing was done'")


Is this leading >&2 intentional? I would place it after the echo.

Yes, it follows syntax similar to what OpenSCAP does when generating Bash fixes.
https://github.com/OpenSCAP/openscap/blob/maint-1.3/src/XCCDF_POLICY/xccdf_policy_remediate.c#L631

Apparently you can place redirects before of after "simple commands" in Bash. Interesting!

matejak · 2020-09-14T13:48:08Z

This is a great step and historic moment for the project - our most common remediations are now platform-aware, and therefore consistent with OVALs and Ansible, who became fully platform-aware recently.
Great work!

yuumasato · 2020-09-14T14:27:57Z

@matejak Thank you very much for the review!

yuumasato requested a review from matejak September 8, 2020 13:46

yuumasato force-pushed the bash-platforms branch from 9ab5f7a to 6dc5687 Compare September 8, 2020 19:13

yuumasato changed the title ~~Introduce package conditionals to Bash remediations~~ Introduce machine and package platform conditionals to Bash remediations Sep 9, 2020

matejak self-assigned this Sep 9, 2020

matejak requested changes Sep 9, 2020

View reviewed changes

yuumasato force-pushed the bash-platforms branch from adcb827 to dc9b126 Compare September 10, 2020 15:15

yuumasato added 4 commits September 10, 2020 17:15

Align Bash applicability with CPE platform

c05cce1

Wraps the remediation of rules with Packager CPE Platform with an if condition that checks for the respective platforms's package.

Handle Bash platform wrapping in xccdf expansion

19e0c3b

Adjust expansion of subs and variables not to remove the whole beginning of the fix test. This was removing the package conditional wrapping.

Check if remediation has associated rule before use

1292b93

Fix python2 compat and improve code readability

7953a02

yuumasato force-pushed the bash-platforms branch from dc9b126 to 7953a02 Compare September 10, 2020 15:15

Document the perils of indenting wrapped Bash fixes

0bd3912

matejak reviewed Sep 14, 2020

View reviewed changes

matejak approved these changes Sep 14, 2020

View reviewed changes

matejak merged commit 1e73f5e into ComplianceAsCode:master Sep 14, 2020

matejak added this to the 0.1.53 milestone Sep 14, 2020

matejak added Bash Bash remediation update. Highlight This PR/Issue should make it to the featured changelog. labels Sep 14, 2020

yuumasato deleted the bash-platforms branch September 14, 2020 14:28

matejak mentioned this pull request Nov 20, 2020

Introduce test suite script wrappers #6405

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce machine and package platform conditionals to Bash remediations #6061

Introduce machine and package platform conditionals to Bash remediations #6061

yuumasato commented Sep 8, 2020 •

edited

Loading

pep8speaks commented Sep 8, 2020 •

edited

Loading

matejak left a comment

matejak Sep 9, 2020

matejak Sep 9, 2020

yuumasato Sep 10, 2020 •

edited

Loading

yuumasato Sep 10, 2020

matejak Sep 10, 2020 •

edited

Loading

yuumasato Sep 10, 2020

matejak Sep 9, 2020

matejak Sep 9, 2020

yuumasato Sep 9, 2020

jan-cerny commented Sep 10, 2020

matejak commented Sep 10, 2020

matejak commented Sep 10, 2020

mildas commented Sep 10, 2020

matejak commented Sep 11, 2020

matejak Sep 14, 2020

yuumasato Sep 14, 2020 •

edited

Loading

matejak Sep 14, 2020

matejak commented Sep 14, 2020

yuumasato commented Sep 14, 2020

Introduce machine and package platform conditionals to Bash remediations #6061

Introduce machine and package platform conditionals to Bash remediations #6061

Conversation

yuumasato commented Sep 8, 2020 • edited Loading

Description:

Rationale:

Rationale:

pep8speaks commented Sep 8, 2020 • edited Loading

Comment last updated at 2020-09-10 15:27:00 UTC

matejak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuumasato Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejak Sep 10, 2020 • 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

jan-cerny commented Sep 10, 2020

matejak commented Sep 10, 2020

matejak commented Sep 10, 2020

mildas commented Sep 10, 2020

matejak commented Sep 11, 2020

Choose a reason for hiding this comment

yuumasato Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejak commented Sep 14, 2020

yuumasato commented Sep 14, 2020

yuumasato commented Sep 8, 2020 •

edited

Loading

pep8speaks commented Sep 8, 2020 •

edited

Loading

yuumasato Sep 10, 2020 •

edited

Loading

matejak Sep 10, 2020 •

edited

Loading

yuumasato Sep 14, 2020 •

edited

Loading