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

Remove jinja condition to make rule applicability to all products in Kerberos rules #9412

Merged

Conversation

ggbecker
Copy link
Member

@ggbecker ggbecker commented Aug 26, 2022

Description:

  • Remove jinja condition to make rule applicability to all products in Kerberos rules.

Rationale:

@ggbecker ggbecker added the OVAL OVAL update. Related to the systems assessments. label Aug 26, 2022
@ggbecker ggbecker added this to the 0.1.64 milestone Aug 26, 2022
@github-actions
Copy link

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@ggbecker
Copy link
Member Author

@Xeicker FYI, I'm removing the jinja conditions here since it should be applicable to all products and the version of the package will determine the applicability...

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.

The changes LGTM.

But as distros adopt newer versions of krb5 this rule will not be needed anymore.
So how about also restricting the products this rule is available for?

We should avoid removing the rule from a product that already released with this rule.
But explicitly listing the current prodtypes, should avoid automatically adding these rules to new products, or product versions.
And then, in 10 years, when all current products are EOL, these rules can be deleted, :)

@ggbecker
Copy link
Member Author

The changes LGTM.

But as distros adopt newer versions of krb5 this rule will not be needed anymore. So how about also restricting the products this rule is available for?

We should avoid removing the rule from a product that already released with this rule. But explicitly listing the current prodtypes, should avoid automatically adding these rules to new products, or product versions. And then, in 10 years, when all current products are EOL, these rules can be deleted, :)

a comment on the prodtype can help preventing people from adding new products in the future, but it's not bulletproof. Some kind of negate mechanism would be the best, like: !rhel10

@vojtapolasek vojtapolasek modified the milestones: 0.1.64, 0.1.65 Sep 19, 2022
@marcusburghardt
Copy link
Member

The changes LGTM.
But as distros adopt newer versions of krb5 this rule will not be needed anymore. So how about also restricting the products this rule is available for?
We should avoid removing the rule from a product that already released with this rule. But explicitly listing the current prodtypes, should avoid automatically adding these rules to new products, or product versions. And then, in 10 years, when all current products are EOL, these rules can be deleted, :)

a comment on the prodtype can help preventing people from adding new products in the future, but it's not bulletproof. Some kind of negate mechanism would be the best, like: !rhel10

I would prefer to work with "allow list" instead "deny list". We already know the current relevant products, but we don't know about future products. Also, working with "deny list", technically we should increase the list whenever a new product appears.I believe this is avoidable maintenance. In any case, the comment on prodtype is handy.

@yuumasato yuumasato assigned yuumasato and unassigned yuumasato Sep 30, 2022
@ggbecker ggbecker force-pushed the remove-jinja-condition-krb branch 2 times, most recently from bb5d408 to 4af607b Compare October 4, 2022 10:20
@ggbecker
Copy link
Member Author

ggbecker commented Oct 4, 2022

I have refreshed the PR with some more changes and I believe it should be good to go.

@ggbecker ggbecker force-pushed the remove-jinja-condition-krb branch 2 times, most recently from 6b14943 to 7efdba4 Compare October 4, 2022 15:43
Kerberos rules in newer OSes should not be applicable since they have
kerberos that supports FIPS algorithms and do not pose a threat anymore.
@ggbecker
Copy link
Member Author

ggbecker commented Oct 7, 2022

There was a problem in the CPE conditionals which hopefully is fixed by: bbd391e

         Start 186: rhel9-bash-shellcheck
185/262 Test #186: rhel9-bash-shellcheck .................................................***Failed   19.60 sec

In /__w/content/content/build/rhel9/fixes/bash/kerberos_disable_no_keytab.sh line 3:
if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && { (  ); }; then
                                                            ^-- SC1073: Couldn't parse this explicit subshell. Fix to allow more checks.
                                                                ^-- SC1072: Unexpected keyword/token. Fix any mentioned problems and try again.


In /__w/content/content/build/rhel9/fixes/bash/package_krb5-workstation_removed.sh line 7:
if (  ); then
   ^-- SC1073: Couldn't parse this explicit subshell. Fix to allow more checks.
       ^-- SC1072: Unexpected keyword/token. Fix any mentioned problems and try again.

For more information:
  https://www.shellcheck.net/wiki/SC1072 -- Unexpected keyword/token. Fix any...
  https://www.shellcheck.net/wiki/SC1073 -- Couldn't parse this explicit subs...

When there would be no bash/ansible conditional stated for a CPE
composed using the CPE language, the conditional was returning "( )"
resulting in wrong syntax in the remediation.
@jan-cerny jan-cerny self-assigned this Oct 14, 2022
Comment on lines 217 to +224
def to_ansible_conditional(self):
child_ansible_conds = [
a.to_ansible_conditional() for a in self.args
if a.to_ansible_conditional() != '']

if not child_ansible_conds:
return ""

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add unit tests that cover your changes in the build_cpe.py.

@codeclimate
Copy link

codeclimate bot commented Oct 14, 2022

Code Climate has analyzed commit 5aa839d and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2
Complexity 1

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

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

View more on Code Climate.

@jan-cerny
Copy link
Collaborator

The Code Climate issues can be waived - the 2 sections in code marked as duplicate actually works with different type and unifying them would not improve the readability; and changing CPEALLogicalTest is out of scope of this PR.

@openshift-ci
Copy link

openshift-ci bot commented Oct 14, 2022

@ggbecker: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ocp4-high-node 5aa839d link true /test e2e-aws-ocp4-high-node

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

Thanks, the unit test is spot in!

@jan-cerny jan-cerny merged commit 8ac4cea into ComplianceAsCode:master Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OVAL OVAL update. Related to the systems assessments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants