Sysctl improvements #10534
Closed
Sysctl improvements #10534
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hi @maage. 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. |
maage
force-pushed
the
sysctl-1
branch
5 times, most recently
from
5a0aa92
to
6a40ef1
Compare
Use str.translate method like python does.
Mostly where ever rhel8 / rhel9 is used.
bash_sed_escape_regexp is for s regexp and bash_sed_escape_replacement is for replacement
/etc/sysctl.conf or any related directories might not exist.
Implement `bash_sysctl_test_clean` to ensure all sysctl directories and
files do exist, and there can only be configuration at `/etc/sysctl.conf`.
Implement `bash_sysctl_set_remediate_file_name`. Only one place to set
where file used to set `sysctl` remediation variables.
Implement `bash_sysctl_set_config_directories`. Per product list of
directories we are managing. Not all products manage all directories.
If there is some reason to modify this phase, now there is shared place
to do it. Use it in all `sysctl` template tests.
Also match documentation to implementation.
From: sysctl.conf(5)
...
FILES
/etc/sysctl.d/*.conf
/run/sysctl.d/*.conf
/usr/local/lib/sysctl.d/*.conf
/usr/lib/sysctl.d/*.conf
/lib/sysctl.d/*.conf
/etc/sysctl.conf
...
Use bash pattern to strip key from undesirables instead of invoking sed. Some style fixes.
Baseline function striped_key just does not support complex enough regexps for key.
Sometimes data is not case insensitive and case can not be ignored.
Sometimes data does not end at word boundary as regexp word boundary \> understands it.
|
This seems not to be okay, and there is others. |
|
Code Climate has analyzed commit 436ad22 and detected 2 issues on this pull request. Here's the issue category breakdown:
Note: there is 1 critical issue. The test coverage on the diff in this pull request is 56.5% (50% is the threshold). This pull request will bring the total coverage in the repository to 52.4% (0.0% change). View more on Code Climate. |
Try to implement support for all quirks from manuals. Use slightly changed logic, `bash_sed_escape_regexp`, and `quote` to achieve this. Avoid per product quirks here by using `bash_sysctl_set_config_directories` and `bash_sysctl_set_remediate_file_name`. Add LC_ALL=C to checks. Assume sysctl values can never contain \n. Most probably bad OVAL with too broad regex capture, or no capture at all, so it gets all whitespace around. sed does not allow multiline regexps and you get errors like: sed: -e expression ComplianceAsCode#1, char 67: unterminated `s' command use set -epu to fail if there is any issues. Try to minimize unnecessary newlines in created files.
… directories And thus wrong_value_usr_local_lib.fail.sh is not needed any more. Now wrong_value_d_directory.fail.sh tests OVAL that it finds all relevant wrong values. During remediation test that all relevant wrong values are fixed. I believe testing one directory at a time is not benefical usage of testing resources. As we want to test one item, it is to test if list of directories implemented in OVAL or in remediation are not in sync.
…ng_sysctlval_for_testing Implemented heuristics might not work always, especially with multivalue settings like `net.ipv4.ip_local_port_range`. Also handle 'E226 missing whitespace around arithmetic operator'.
There is patchset to enable this: https://patchwork.kernel.org/project/linux-hardening/patch/1469630746-32279-1-git-send-email-jeffv@google.com/ Some distros might have this enabled. Add variable sysctl_kernel_perf_event_paranoid_value as variable is required when multiple values possible.
Oval set can have only up to 2 items. This converts list to one set as it can get quite tedious if done by hand.
When you combine xccdf variables and other format than simple int / string, there is no generict way to implement comparison. So I decided just to use per name comparison method.
Function sysctl_set_kernel_setting_to is not used anywhere.
Previously SYSCTLVAL == "" was variable and this did not allow to handle empty values. Change SYSCTLVAL to be not defined if having variable and this issue is solved.
Handle emtpy string in ansible because `sysctl` module does not handle empty string.
Change state_aide_check_attributes to ensure no prefix/suffix for pattern. Fix correct_with_selinux.pass.sh Also use packages to ensure aide package is installed in tests.
Default word_boundary in bash_replace_or_append \> does not work with
*.* and like.
Replace regexp in all types to be the same. Avoid copy-paste in OVAL.
rsyslog.conf(5)
...
Rules (selector + action)
Every rule line consists of two fields, a selector field and an action field. These two fields are separated
by one or more spaces or tabs. The selector field specifies a pattern of facilities and priorities belonging
to the specified action.
...
Also drop comment or empty lines with whitespace start of line.
Newer ansible (mine 2.14) has sysctl at ansible.posix.sysctl.
But build system does not accept it:
Found module which is not allowed:
{'tags', 'name', 'when', 'ansible.posix.sysctl'}
and
ERROR! couldn't resolve module/action 'ansible.posix.sysctl'. This often indicates a misspelling, missing collection, or incorrect module path.
the replacement string in double-quoted pattern substitution does
not undergo quote removal, as it does in versions after bash-4.2
|
New issue from Ubuntu 22.04 / Debian I guess I need to extend those BATS tests, I have been running quick wrong with extra filters. |
This was referenced May 16, 2023
Merged
|
PR needs rebase. 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. |
|
Closing due to inactivity. If you still want to work on this PR, please reopen it and resolve the conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
When reading sysctl manuals and looking code there was some differencies. And I also wanted to convert
sysctl_net_ipv4_ip_local_port_rangeto have support for variables. And I also wanted to try to see how jinja macro based implementations would look like.I'm not totally happy with how it looks like.
For example setting value explicitly ensures
variable=valueis accepted by kernel. But it does not ensure that this value will be there at next boot. There is file based approach, but it does not manage all directories at least under RHEL etc. Files have ordering and at least for other types of tasksoscap-scanneris not able to access all files. So remediation should simulate boot in this regard to ensure there is no ordering issues or any other trickery involved. So remediation tasks should be:/etc/sysctl.confbutsystemd-sysctl.servicedoes read onlysysctl.ddirectories and it does rely on symlink/etc/sysctl.d/99-sysctl.conf -> ../sysctl.conf. What if symlink does not exist? (not done atm)sysctl -w/usr/lib/systemd/systemd-sysctl --cat-configorsystemctl restart systemd-sysctland check setting again (not done atm). Ansible can reload all the same but does it ensure file is what really is set during boot? I'm not sure, andbashdoes not do even that. SLE has own remediation file and naming ensures it is easy to override it. And even RHEL style symlink above, it is easy to override that too. So this is why in the end remediation should be rely on boot / service restart simulation to catch any an all possible trickery. This is not aboutsysctlissue, but for most services have this issue too. Set configuration just so, but in the end you need to enable to configuration and then ask system / runtime what their representation is and only that ensures remediation was success.I guess
sysctl.ddirectories should be set via product mechanism and not by each type:OVAL,bashandansible. But it would lack all feature, which is nice for testing. At least now I have reduced it to only one test per type.If there is need to do proper quoting, there should be also definite standards to do regexp matching.
Ansiblehaspythonwith per plugin variants (substing match, full content match, multiline),OVALhas tiny bit differentpcre. Lastly bash has either BRE (grep,sed), ERE (grep -E,sed -E), perlre (grep -P, nosed), awk. Anyjinjamacro structure should support only one type of regexp format. Usual pattern is to quote regexp to make it partially literal and that should be done via specialized and tested function to ensure this macro structure does not break if there is need.There really do need to be #10387 style support to ensure tests do not fail in envs where they are impossible.
I have not expanded
sysctltest suite to match all the possible quirks, but it would probably mean many more tests. Not at #10519 level multiple dimensions but still. But I wonder if this is easiest way. It also is usually quite hard to test only one item per test as there could be quite a lot of need for example testing regexps used is okay. Does regexp testing really need to be separate each case, and is there any way to do some kind of combinations?Another dimension of testing is this seen with comments and symlinks. Is this something that could be tested once per template? Also proper test should check situation after the fact. Now we just run
OVALeval again after remediation, but it just checks what is there, and for example commented out contents does not matter or if file contents was via symlink structure. InOVALthere is no state to check that has this filename changed from symlink to regular file or how actually has file contents changed and is it according to a what remediation should do. Tests should define domain of test and before and after remediation test suite should take copy of this domain and then compare after and before. To see if there was no side effects. For example here:/etc/sysctl.confandsysctl.ddirectory contents andansiblepluginstatstyle dump,sysctl --alldump. This is something that is seen withsedinbash, in many cases--follow-symlinksis missing with-i. And there is comments modification withsysctl.Next item from previous is that
ansibleandbashremediations should produce about the same result and those dumps done previously should also compared with each other.One other case of harmonization is character set. Commands like
grep,sedshould probably always have commonLC_ALL=Cvalue.There is missing
codeclimateforOVAL. And also there is no single style to indent XML attributes. Many attributes are quite long as rule names are quite long. So any such should have their own line. Do we want to indent XML to align 2nd+ line attibute to align with 1st line attibute or have normal 4 space indent with element. And what about ordering?Also I'm not totally happy with patches like fix: sysctl/bash: follow sysctl quirks more where in one swoop I implement multiple changes. It just gets really tedious to keep them separated after a while as after I change one aspect I need to keep multiple temporary states alive. Similar problem with style: sysctl/oval: indent 4.
Rationale:
Add
sysctl_net_ipv4_ip_local_port_range. Enablesysctlrules in Fedora.Have more robust and more base system
sysctl/sysctl.dimplementation followingOVALand remediation following.bash_replace_or_appendneeded to have more features to accomodate all of needs I encountered.I guess bits and peaces could be submitted as smaller PR, but this PR tries to answer question of how and why this feature could be used.
Review Hints:
At this point this probably fails in many interesting ways.
At least following ways:
Tests seem to fail because
JinjaAnalysisdoes not understand recursivejinjamacros. But as far as I can see that is only way to implementoval_list_to_setstyle conversion macros injinja. As macro works as intended, I'm not going to to change it at this time. Fix available at: ComplianceAsCode/content-test-filtering#43I don't think codeclimate complexity requirements are something to fix. Code there is just multiple possibilities. It is possible to split to functions but I don't think it is all that benefical.
I moved all commits where I needed to fix something not working in other or more strict test envs.