Skip to content

Conversation

@evgenyz
Copy link
Member

@evgenyz evgenyz commented Sep 18, 2020

Improvement of #6088.

More info pending.

@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

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

Isn't the oval_argument_file macro very similar to line_in_file macro?

@evgenyz
Copy link
Member Author

evgenyz commented Sep 21, 2020

Isn't the oval_argument_file macro very similar to line_in_file macro?

Yes, it is similar. But also it is different. The line_in_file is tailored for a single value per line. But, if you can see a way to do the same using line_in_file, please give me an example.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

Yes, it is similar. But also it is different. The line_in_file is tailored for a single value per line.

Thanks for clarifying the differences.

But, if you can see a way to do the same using line_in_file, please give me an example.

I think multi_value=true can help.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L17

It is used by oval_grub_config macro for example.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L327

@yuumasato
Copy link
Member

It is used by oval_grub_config macro for example.

And actually, all BLS related templates could leverage oval_grub_config, :)

@mildas
Copy link
Contributor

mildas commented Sep 21, 2020

Changes identified:
Rule sssd_ldap_configure_tls_ca_dir:
 Templatization usage changed.
Rule sssd_ldap_start_tls:
 Templatization usage changed.
Rule grub2_uefi_admin_username:
 The rule doesn't occur in any profile nor product.
 Templatization usage changed in /linux_os/guide/system/bootloader-grub2/grub2_uefi_admin_username/oval/shared.xml.
Rule grub2_admin_username:
 The rule doesn't occur in any profile nor product.
 Templatization usage changed in /linux_os/guide/system/bootloader-grub2/grub2_admin_username/oval/shared.xml.
Rule grub2_password:
 Templatization usage changed in /linux_os/guide/system/bootloader-grub2/grub2_password/oval/shared.xml.
Rule grub2_uefi_password:
 Templatization usage changed in /linux_os/guide/system/bootloader-grub2/grub2_uefi_password/oval/shared.xml.
Rule checks:
 Attribute value changed in OVAL check.
 New node inserted to OVAL check.
 The rule doesn't occur in any profile nor product.
 Deleted attribute from OVAL check.
 Node deleted from OVAL check.
 Text changed in OVAL check.
 Node moved within OVAL check.
Macro bash_sssd_ldap_config:
 In Bash remediation for sssd_ldap_configure_tls_ca_dir.
 In Bash remediation for sssd_ldap_start_tls.
Macro oval_file_absent_criterion:
 In OVAL check for grub2_password.
 Used in template_OVAL_coreos_kernel_option template.
 In OVAL check for grub2_admin_username.
 In OVAL check for grub2_uefi_password.
 In OVAL check for grub2_uefi_admin_username.
Macro oval_file_absent:
 In OVAL check for grub2_password.
 Used in template_OVAL_coreos_kernel_option template.
 In OVAL check for grub2_admin_username.
 In OVAL check for grub2_uefi_password.
 In OVAL check for grub2_uefi_admin_username.
Macro oval_argument_in_file_criterion:
 Used in template_OVAL_coreos_kernel_option template.
 In OVAL check for checks.
 Used in template_OVAL_zipl_bls_entries_option template.
Macro oval_argument_in_file_test:
 Used in template_OVAL_coreos_kernel_option template.
 In OVAL check for checks.
 Used in template_OVAL_zipl_bls_entries_option template.
Macro oval_argument_in_file:
 Used in template_OVAL_argument_in_file template.
 Used in template_OVAL_coreos_kernel_option template.
 In OVAL check for checks.
 Used in template_OVAL_zipl_bls_entries_option template.
Others:
 Python abstract syntax tree change found in ssg/templates.py.
 Python abstract syntax tree change found in ssg/utils.py.

Recommended tests to execute:
 build_product ol7
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-ol7-ds.xml sssd_ldap_configure_tls_ca_dir
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-ol7-ds.xml sssd_ldap_start_tls
 (cd build && cmake ../ && ctest -j4)
 build_product fedora
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-fedora-ds.xml grub2_password
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-fedora-ds.xml grub2_uefi_password

@jan-cerny jan-cerny changed the title Imporve argument rules Improve argument rules Sep 22, 2020
@evgenyz
Copy link
Member Author

evgenyz commented Sep 30, 2020

Yes, it is similar. But also it is different. The line_in_file is tailored for a single value per line.

Thanks for clarifying the differences.

But, if you can see a way to do the same using line_in_file, please give me an example.

I think multi_value=true can help.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L17

It is used by oval_grub_config macro for example.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L327

The only rule that use it does it like this {{{ oval_grub_config(parameter="GRUB_CMDLINE_LINUX", value="nousb") }}}, which is looking for value nousb for parameter GRUB_CMDLINE_LINUX. Can't imagine how it would check that selinux=(1|yes|true) is set.

The resulting OVAL is:

        <ind:textfilecontent54_object id="oval:ssg-obj_grub2_nousb_argument:obj:1" version="1">
          <ind:filepath>/etc/default/grub</ind:filepath>
          <ind:pattern operation="pattern match">^[ \t]*GRUB_CMDLINE_LINUX=([^#]*).*$</ind:pattern>
          <ind:instance operation="greater than or equal" datatype="int">1</ind:instance>
        </ind:textfilecontent54_object>
        <ind:textfilecontent54_state id="oval:ssg-state_grub2_nousb_argument:ste:1" version="1">
          <ind:subexpression datatype="string" operation="pattern match">^.*\bnousb\b.*$</ind:subexpression>
        </ind:textfilecontent54_state>

@evgenyz
Copy link
Member Author

evgenyz commented Sep 30, 2020

It is used by oval_grub_config macro for example.

And actually, all BLS related templates could leverage oval_grub_config, :)

BLS-compatible bootloader is not nesesary the GRUB, hence /etc/default/grub file would be irrelevant for it. Moreover, GRUB, when operating in fully BLS-compatible mode, does not use /etc/default/grub at all. So the assumption that some value present there would make it into the kernel is incorrect. GRUB does support BLS notation to some degree, but GRUB ≠ BLS and BLS ≠ GRUB.

@yuumasato
Copy link
Member

But, if you can see a way to do the same using line_in_file, please give me an example.

I think multi_value=true can help.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L17

It is used by oval_grub_config macro for example.
https://github.com/ComplianceAsCode/content/blob/master/shared/macros-oval.jinja#L327

My point was to illustrate usage of macro oval_check_config_file and its argument multi_value=true.
Which I believe will address the use case in question.

@yuumasato
Copy link
Member

It is used by oval_grub_config macro for example.

And actually, all BLS related templates could leverage oval_grub_config, :)

BLS-compatible bootloader is not nesesary the GRUB, hence /etc/default/grub file would be irrelevant for it. Moreover, GRUB, when operating in fully BLS-compatible mode, does not use /etc/default/grub at all. So the assumption that some value present there would make it into the kernel is incorrect. GRUB does support BLS notation to some degree, but GRUB ≠ BLS and BLS ≠ GRUB.

My bad, you are correct in the BLS assessment.
I think I had oval_check_config_file in mind, but put there oval_grub_config.

@evgenyz
Copy link
Member Author

evgenyz commented Oct 1, 2020

My point was to illustrate usage of macro oval_check_config_file and its argument multi_value=true.
Which I believe will address the use case in question.

I got it. But still it is more tailored to check for

[section]
prefix parameter = value value value

rather than for

prefix parameter=value parameter=value parameter=value

While I actually was able to imagine some regexes for oval_check_config_file to use it for the later case, I strongly believe that it would be an abusive usage of that (already very complex) macro. Let's have a new one oval_argument_in_file and tailor it for the second case.

@evgenyz
Copy link
Member Author

evgenyz commented Oct 1, 2020

@matejak Here you are, oval_metadata. Is it okay to just inherit rule_title in these cases?

@evgenyz evgenyz marked this pull request as ready for review October 1, 2020 07:53
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 1, 2020
@evgenyz
Copy link
Member Author

evgenyz commented Oct 1, 2020

@JAORMX Can you test it, please?

@evgenyz
Copy link
Member Author

evgenyz commented Oct 1, 2020

Ah, dammit, conflicts

and use it as custom Jinja filter and across the code for ID sanitizing
and regular expression escaping unification.

Also document custom filters in the Developer's Guide.
…est'

and high-level 'oval_argument_in_file' macros

And use them to re-factor 'zipl_bls_entries_option', 'bls_entries_option'
and 'coreos_kernel_option' templates.
@evgenyz evgenyz force-pushed the imporve-argument-rules branch from 39e33d7 to e81f130 Compare October 1, 2020 08:11
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Oct 1, 2020
@JAORMX
Copy link
Contributor

JAORMX commented Oct 1, 2020

/test e2e-aws-rhcos4-moderate

@JAORMX
Copy link
Contributor

JAORMX commented Oct 1, 2020

@evgenyz this worked for me!

/lgtm

@matejak
Copy link
Member

matejak commented Oct 1, 2020

@matejak Here you are, oval_metadata. Is it okay to just inherit rule_title in these cases?

I guess that you meant whether it is OK that the OVAL title is determined by the rule's XCCDF title, as that's what rule_title jinja variable contains.
Well, I don't see how this could be a problem.

@jan-cerny
Copy link
Collaborator

@yuumasato How do you like it? Are the ZIPL changes OK from your point of view?

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

While I actually was able to imagine some regexes for oval_check_config_file to use it for the later case, I strongly believe that it would be an abusive usage of that (already very complex) macro. Let's have a new one oval_argument_in_file and tailor it for the second case.

Alright.

@yuumasato yuumasato merged commit abb34ec into ComplianceAsCode:master Oct 6, 2020
@yuumasato yuumasato added this to the 0.1.53 milestone Oct 6, 2020
@yuumasato yuumasato self-assigned this Oct 6, 2020
@evgenyz evgenyz deleted the imporve-argument-rules branch October 6, 2020 12:54
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.

7 participants