-
Notifications
You must be signed in to change notification settings - Fork 717
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
ANSSI BP 028 profile for debian12 #11368
Conversation
Hi @a-skr. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@@ -1,6 +1,6 @@ | |||
documentation_complete: true | |||
|
|||
prodtype: ubuntu2004,ubuntu2204 | |||
prodtype: ubuntu2004,ubuntu2204,debian12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prodtypes need to be alphabetically sorted. You have multiple occurrences of unsorting the prodtype key in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that. I will fix them.
@@ -0,0 +1,20 @@ | |||
# platform = multi_platform_debian | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OVAL checks and Ansible and Bash remediations for Debian 12 that you introduce are quite different from the existing ones that are there for other products. Consequently, the description and other texts in the rule.yml
file of this rule aren't properly aligned with the actual check and remediation. Please update the linux_os/guide/system/software/integrity/software-integrity/aide/aide_build_database/rule.yml
in a way so that it better describes the actual situation on Debian 12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debian description explains an interactive aide database update (through the aideinit wrapper) which, I think, is fitting.
The remedation (non interactive) uses options -y -f to automatically create then deploy the new database.
As per aideinit manpage, the user should ideally check the database content before deploying it, so I prefer a description that doesn't automatically force the deployment of the new DB.
@@ -51,7 +51,7 @@ | |||
|
|||
- name: '{{{ rule_title }}} - Extract log files new format' | |||
ansible.builtin.shell: | | |||
set -o pipefail | |||
{{% if not 'debian' in product %}}set -o pipefail{{% endif -%}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change? I think that Ansible lint will require having the pipefail here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default shell in Debian for scripts is dash. dash doesn't have the pipefail option. Some operating systems (such as those based on open-embedded) may not have bash compiled in (they usually use busybox).
Alternatively, I can force bash usage in the playbook fragment.
What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a great explanation.
I have found that at this moment we have in our CI relaxed this Ansible lint check to a warning, due to some other existing ules that don't have the pipefail set in their Ansible code.
content/.config/ansible-lint.yml
Lines 15 to 22 in ae3019d
warn_list: | |
- name[missing] # Rule for checking task and play names. | |
- template-instead-of-copy # Templated files should use template instead of copy | |
- yaml[empty-lines] # Violations reported by yamllint. | |
- yaml[line-length] # Violations reported by yamllint. | |
- name[casing] # Rule for checking task and play names. | |
- no-handler # Style choice, skipping for now | |
- risky-shell-pipe # Skipping for now due to authselect rules |
Also, there is a complex discussion about it on the Ansible lint GitHub ansible/ansible-lint#497 and there is no specific outcome.
So after finding these information I would prefer to keep this if
condition here as you proposed. It would be nice to add an explanatory comment to the code there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add an explanation ASAP.
Change default value from system.administrator@mail.mil to change_me@localhost in case mail.mil suddenly becomes valid.
- enable remediation - fix description - add OVAL test
related macros shared/macros/10-ansible.jinja fixes: - fix name of task adding grub2 arguments - fix kernel arguments matching regexp: kernel command line arguments may be words separated by commas. The \w metacharacter doesn't match the comma, so rules like grub2_l1tf_argument fail if the ansible remediation playbook is executed a second time.
6f1d72c
to
0adab98
Compare
/packit retest-failed |
0adab98
to
92cc544
Compare
- name: '{{{ rule_title }}} - Get include files directives' | ||
ansible.builtin.shell: | | ||
set -o pipefail | ||
{{% if not 'debian' in product %}}set -o pipefail{{% endif -%}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one more problem, and that is a serious one and it causes the fail of the testing farm CI jobs. On other products, this reduces the whitespace and accidentally removes the newline. For example, when you build rhel9
, you will see this in the built cotnent:
set -o pipefailgrep -e '$IncludeConfig' {{ rsyslog_etc_config }} | cut -d ' ' -f 2 || true
Please make sure this won't happen. Dtto for other occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fixed by now.
Thank you for your patience.
The ansible remediation fragment assumes /bin/sh points to bash, which may not be the case depending on linux distribution.
92cc544
to
2d92005
Compare
Code Climate has analyzed commit 2d92005 and detected 0 issues on this pull request. 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 58.5% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the built debian12 content and I have seen that the ANSSI BP28 profiles are present there.
Description: