Conversation
… file is absent
The file permission and ownership checks of the cis-1.12 benchmark wrap
their audit command in a file existence guard, for example
/bin/sh -c 'if test -e $apiserverconf; then stat -c permissions=%a $apiserverconf; fi'
When the file is absent that command produces no output at all, so the
test_item that looks for the "permissions" flag cannot match and the
check is reported as FAIL. The guard therefore has no effect: without it
stat would fail and the check would be reported as FAIL as well.
cis-1.12/node.yaml 4.1.2 already handles this correctly by echoing a
sentinel in the else branch and accepting it with bin_op: or. This
change applies the same pattern to the remaining 21 file permission and
ownership checks of the benchmark.
Checks where the absence of the file is the finding itself are left
untouched: 1.2.28 (encryption provider configuration) and 4.1.7 / 4.1.8
(client certificate authority file).
Fixes aquasecurity#1881
Signed-off-by: Eljees <3.14hell@gmail.com>
Apply andypitcher's review suggestion to every wrapped check: drop the 'if test -e ... else echo' guards and let stat report a missing file itself. stat alone exits non-zero, and runAudit turns that into an error that fails the check before any test_item is evaluated, so the audits keep '|| true' to stay on the flag-matching path; the tests accept stat's own 'No such file or directory' (captured from stderr) via bin_op: or. The 4.1.2 upstream sentinel check is aligned to the same pattern, the multi-file 1.1.13/1.1.14 loops drop their found-sentinel, and the guard meta-test now enforces the new shape. A new unit test documents why the bare stat without '|| true' would regress to FAIL. Signed-off-by: Eljees <3.14hell@gmail.com>
@andypitcher asked for the check to fail as soon as the file is missing. A bare stat does that, and unlike the previous existence guard it says why: runAudit puts stat's own message in the check's Reason, because stderr is collected together with stdout. The bin_op: or and the "No such file or directory" test_item go with it. They were never reached anyway - a non-zero audit fails the check on the error path, before any test_item is evaluated - and matching on stat's wording was brittle: newer coreutils say "cannot statx", not "cannot stat".
…rity#1881) Same fix as aquasecurity#2127, applied to every other benchmark directory: an audit of the shape if test -e $VAR; then stat -c FMT $VAR; fi succeeds with no output when the file is absent, so the check FAILs with nothing to say about why. Running stat directly lets a missing file's own message ("No such file or directory") reach the user in the check's Reason instead of being swallowed. Covers the semicolon and bracket-test ([ -e ], [[ -e ]]) spellings of the same guard, and the paired "else echo \"File not found\"" test_item that existed only to catch the guarded case. Left alone, matching the precedent set in aquasecurity#2127: - node.yaml 4.1.7/4.1.8 (client CA file) across 13 directories: the guard there is compound ($CAFILE is reassigned from a fallback before the existence check), and a missing client CA file is a finding of its own, not something to paper over. - rke2-cis-1.24/master.yaml 1.1.10: the guard covers a directory, not a single file, ahead of a multi-path find/xargs chain rather than a single stat call - needs a case-by-case look. - k3s-cis-1.23/master.yaml 1.1.13: pre-existing bug unrelated to this fix, the audit is missing its closing "fi" entirely; flagging separately rather than guessing at the intended shape here. TestFilePermissionChecksDoNotHideAMissingFile now covers every directory touched here. Note: I don't have a Go toolchain in this environment, so this has been checked by rebuilding every touched YAML through PyYAML and by re-deriving, in a standalone script, exactly what auditGuardsFileExistence + checksWhereTheGuardIsDeliberate will see - zero unexplained guards remain. It has not been run through go test ./check/... itself; please do before merging.
…1.1.10) The audit for 1.1.10 wrapped the same ps/find/xargs pipeline used by the unguarded 1.1.9 check (permissions) in an `if [ -e /etc/cni/net.d ]; then ... else echo "File not found"; fi` guard. The guard tested a hardcoded default path while the pipeline itself resolves the real cni-conf-dir from the running kubelet, so it did not even cover the case it gated, and it skipped the independent /var/lib/cni/networks check too when the default path was absent. Drop the guard and run the pipeline directly, matching 1.1.9 in this same file and the already-fixed 1.1.10 in cfg/rke2-cis-1.23/master.yaml on this branch. `find` reports a missing directory on its own; `xargs --no-run-if-empty` already avoids running stat on empty input. This was the one case in aquasecurity#1881 intentionally left open pending a decision (see PR body) rather than mechanically ported like the other 41 directories. go test ./check/... has not been run in this environment (no Go toolchain available on the Windows host or in the sandbox) - please run it before merging, as noted in the PR description.
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Builds on #2127 (not yet merged) - this branch is stacked on top of
fix/1881-file-permission-checks-missing-file, so the diff GitHub shows here includes that PR's commits until it merges. Once #2127 lands, this should narrow down to just the commit below.Same fix as #2127 (see that PR's description for the rationale), applied to every other benchmark directory under
cfg/: an audit of the shapesucceeds with no output when the file is absent, so the check FAILs with nothing to say about why. Running stat directly lets a missing file's own message ("No such file or directory") reach the user in the check's
Reasoninstead of being swallowed. This covers both thetest -eand bracket-test ([ -e ],[[ -e ]]) spellings of the same guard, and drops the pairedelse echo "File not found"test_item that existed only to catch the guarded case.Left alone, matching the precedent set in #2127:
node.yamlchecks 4.1.7/4.1.8 (client CA file) across 13 directories: the guard there is compound ($CAFILEis reassigned from a fallback before the existence check), and a missing client CA file is a finding of its own, not something to paper over.rke2-cis-1.24/master.yaml1.1.10: the guard covers a directory, not a single file, ahead of a multi-path find/xargs chain rather than a singlestatcall - this needs a case-by-case look rather than the mechanical fix applied elsewhere.k3s-cis-1.23/master.yaml1.1.13: a pre-existing bug unrelated to this fix - the audit is missing its closingfientirely. Flagging it here rather than guessing at the intended shape.TestFilePermissionChecksDoNotHideAMissingFile(added in #2127) now also covers every directory touched in this PR.Caveat: I don't have a Go toolchain available in the environment I used for this pass, so I've checked it by parsing every touched YAML file with PyYAML (all 293 files in
cfg/still parse) and by re-implementingauditGuardsFileExistence+checksWhereTheGuardIsDeliberatein a standalone script against the final files - zero unexplained guards remain. It has not been run throughgo test ./check/...itself. Please run the suite before merging; happy to fix anything that turns up.