fix(cis-1.12): say why a file permission check failed when the file is absent

[Eljees]

Fixes #1881

Overview

The file permission and ownership checks of the cis-1.12 benchmark guard their audit command against a missing file:

/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 can never match and the check is reported as FAIL — for a file that does not exist. The guard therefore has no effect: without it stat would fail and the check would be reported as FAIL as well.

cfg/cis-1.12/node.yaml 4.1.2 already handles this correctly — it echoes a sentinel in the else branch and accepts it with bin_op: or. This PR applies that same pattern to the remaining file permission and ownership checks of the benchmark.

While doing so it also fixes 4.1.3 and 4.1.4, where bin_op: or is currently a no-op because there is only one test_item to combine.

Changed (21 checks): master.yaml 1.1.1–1.1.8, 1.1.13–1.1.18; node.yaml 4.1.1, 4.1.3–4.1.6, 4.1.9, 4.1.10.

For 1.1.13 and 1.1.14, which loop over admin.conf and super-admin.conf with use_multiple_values: true, the sentinel is only echoed when neither file exists, so that a mix of real output and sentinel lines cannot occur.

Deliberately not changed, because there the missing file is the finding:

check	reason
1.2.28	no encryption provider configuration means encryption at rest is not configured
4.1.7 / 4.1.8	a missing client certificate authority file is a finding of its own

Happy to change those too if you prefer consistency over the current behaviour.

Results before and after

1. New config test (check/controls_test.go, next to TestYamlFiles) — it walks the benchmark and requires every check that guards against a missing file to also tolerate the resulting empty output.

Before:

=== RUN   TestChecksGuardingFileExistenceTolerateMissingFile
    controls_test.go:126: ../cfg/cis-1.12/master.yaml: check 1.1.1 guards against a missing file but reports FAIL when it is missing: ...
    ... 24 checks reported: 1.1.1 1.1.2 1.1.3 1.1.4 1.1.5 1.1.6 1.1.7 1.1.8 1.1.13 1.1.14 1.1.15
    1.1.16 1.1.17 1.1.18 1.2.28 4.1.1 4.1.3 4.1.4 4.1.5 4.1.6 4.1.7 4.1.8 4.1.9 4.1.10
--- FAIL: TestChecksGuardingFileExistenceTolerateMissingFile (0.03s)

After:

--- PASS: TestYamlFiles (1.93s)
--- PASS: TestChecksGuardingFileExistenceTolerateMissingFile (0.03s)
ok  	github.com/aquasecurity/kube-bench/check	1.970s

2. New engine-level cases in TestCheck_Run that pin the behaviour itself:

--- PASS: TestCheck_Run/File_permission_check_should_FAIL_for_a_missing_file_when_the_audit_is_silent_about_it
--- PASS: TestCheck_Run/File_permission_check_should_PASS_for_a_missing_file_when_the_audit_reports_it

3. The audit command itself, on a host without /etc/kubernetes:

before: <no output>
after : File not found

and with the file present the output is unchanged, so a real permission violation is still reported:

$ /bin/sh -c 'if test -e /tmp/k8s/kube-apiserver.yaml; then stat -c permissions=%a /tmp/k8s/kube-apiserver.yaml; else echo "File not found"; fi'
permissions=644

Full suite: go test ./check/... ./cmd/... passes.

Note

The same defect exists in the older benchmarks (about 600 checks across all of cfg/). This PR keeps the change to the current benchmark so the diff stays reviewable; the test has an explicit list of the benchmarks that have been audited, so extending it is a one-line change per benchmark. Glad to follow up with the rest if you want them.

---

Written with AI assistance; reviewed, built and tested by me.

[andypitcher]

Suggested change

	audit: "/bin/sh -c 'if test -e $apiserverconf; then stat -c permissions=%a $apiserverconf; else echo \"File not found\"; fi'"
	audit: "/bin/sh -c 'stat -c permissions=%a $apiserverconf'"

Suggesting for this occurrence but this holds for all the related changes.

@Eljees I agree that providing better feedback to the user would improve debugging when an assessed file is missing. I'm not sure why test was introduced initially, but stat natively handles the "file not found" scenario—and offers more accurate errors overall.

Note: file1.yaml was used for all the below examples, nonexisting.yaml (does not exist)

sh-5.2$ tree
.
└── file1.yaml

1 directory, 1 file

Using test then stat (Custom error):

sh-5.2$ export file=file1.yaml && if test -e $file ; then stat -c permissions=%a $file; else echo "File not found"; fi
permissions=644

sh-5.2$ export file=nonexisting.yaml && if test -e $file ; then stat -c permissions=%a $file; else echo "File not found"; fi
File not found

Using stat alone (Built-in error):

sh-5.2$ export file=nonexisting.yaml && stat -c permissions=%a $file
stat: cannot statx 'nonexisting.yaml': No such file or directory

sh-5.2$ export file=file1.yaml && stat -c permissions=%a $file
permissions=644

Another issue with the proposed fix is how it handles restricted permissions. If a file exists but cannot be accessed due to a permissions error, using test results in a misleading "File not found" message. stat alone reports the exact issue correctly (kube-bench runs as root, but for instance LSM such as SELinux could restrict the access too).

(moving file1.yaml to /root/, trying to access it as a standard user)

Permission Denied with test (Misleading error):

sh-5.2$ export file=/root/file1.yaml && if test -e $file ; then stat -c permissions=%a $file; else echo "File not found"; fi
File not found

Permission Denied with stat alone (Accurate error):

sh-5.2$ export file=/root/file1.yaml && stat -c permissions=%a $file
stat: cannot statx '/root/file1.yaml': Permission denied

@mozillazg Do you recall the historical context for using test in kube-bench? I could dig into the commit history, but my recommendation would be to simplify this and rely solely on stat.

cc @LaibaBareera

[Eljees]

Applied, thanks — you are right that the custom message threw away more than it added.

681a90a drops the test -e guards and uses stat directly in every place the PR touched:
four checks in cfg/cis-1.12/master.yaml (apiserver, controller-manager, scheduler, etcd conf) and
the corresponding ones in cfg/cis-1.12/node.yaml.

One deviation from your snippet that I want to flag rather than sneak in: I kept a trailing || true.

audit: "/bin/sh -c 'stat -c permissions=%a $apiserverconf || true'"

The reason is the exit code, not the message — stat's own cannot statx ...: No such file or directory still reaches the output either way, which is the part you wanted. Without || true the
audit command exits non-zero when the file is absent, and I did not want that to be reported as an
execution problem instead of a plain "file not there" result.

If you would rather have the bare form exactly as suggested, say so and I will drop the || true —
happy to run it both ways and show what the check reports in each case, since that is the bit worth
being sure about.

[Eljees]

Done in 681a90a - applied to all of them, thanks for the pointer and the sample run. Every if test -e ... else echo "File not found" wrapper in cis-1.12 is gone (including the multi-file 1.1.13/1.1.14 loops and the pre-existing sentinel in 4.1.2, so the benchmark now uses one shape throughout), and the tests accept stat's own No such file or directory instead of the custom text - filename and reason included, exactly as in your run.

One deliberate deviation from the literal suggestion: the audits keep || true, i.e. stat -c permissions=%a $apiserverconf || true. A bare stat exits non-zero for a missing file, and runAuditCommands treats a non-zero audit as an error, so run() fails the check on the error path before any test_item is evaluated (check/check.go: the cmd.Run() error propagates up and run() sets FAIL/WARN from err without calling execute()). With || true the exit code stays zero, kube-bench captures stat's stderr into the same buffer (cmd.Stdout/cmd.Stderr share &out), and the bin_op: or test matches the real message. There is a new unit test pair documenting exactly this edge: the bare-stat variant FAILs, the || true variant PASSes.

go test ./...: check ok 2.6s, cmd ok 10.3s.

[Eljees]

Ping on this one and #2125 - both have been quiet since my last comment on 1 August. Happy to rebase either.

[Eljees]

@andypitcher — one item is still open from 1 August, and it's a one-word decision on your side.

I kept a trailing || true on the audits, deviating from your suggested snippet. The reason is the exit code, not the message. runAuditCommands treats a non-zero audit as an execution error, so run() fails the check on the error path before any test_item is evaluated — the missing file gets reported as a broken audit rather than as a plain "file not there" result. With || true the exit stays 0, stat's own No such file or directory still lands in the buffer (cmd.Stdout and cmd.Stderr share it), and the bin_op: or test matches the real message. There's a unit test pair documenting exactly that: the bare-stat variant FAILs, the || true variant PASSes.

If you'd rather have the bare form exactly as you suggested, say so and I'll drop it and re-measure. The branch is behind main now — I'll rebase it together with whichever form you pick, so you only have to look at it once.

[andypitcher]

@andypitcher — one item is still open from 1 August, and it's a one-word decision on your side.

I kept a trailing || true on the audits, deviating from your suggested snippet. The reason is the exit code, not the message. runAuditCommands treats a non-zero audit as an execution error, so run() fails the check on the error path before any test_item is evaluated — the missing file gets reported as a broken audit rather than as a plain "file not there" result. With || true the exit stays 0, stat's own No such file or directory still lands in the buffer (cmd.Stdout and cmd.Stderr share it), and the bin_op: or test matches the real message. There's a unit test pair documenting exactly that: the bare-stat variant FAILs, the || true variant PASSes.

If you'd rather have the bare form exactly as you suggested, say so and I'll drop it and re-measure. The branch is behind main now — I'll rebase it together with whichever form you pick, so you only have to look at it once.

Hey @Eljees thanks for applying the changes. I'd prefer to avoid || true to have the check fail as soon as possible when file is missing. IMO these types of checks are meant to verify the file's permission and content so it's expected to be a valid/present file before you enter the check's logic/test.

Note that it's quite easy then to investigate such an issue (file missing) by increasing the verbosity or using kube-bench --include-test-output.

Maybe we could get another opinion @LaibaBareera @mozillazg or @afdesk ?

The rest LGTM.

[Eljees]

Done in 0f317df - || true is gone and the audits run stat directly, as you suggested. The bin_op: or and the No such file or directory test_item went with it: a non-zero audit fails the check on the error path, before any test_item is evaluated, so they were never reached.

Measured against a present and a missing file:

audit	present	missing
if test -e F; then stat -c permissions=%a F; fi (main)	PASS	FAIL, actual value "", reason ""
stat -c permissions=%a F || true + bin_op: or (previous push)	PASS	PASS
stat -c permissions=%a F (this push)	PASS	FAIL, reason failed to run: ..., output: "stat: cannot statx '/no/such/file': No such file or directory"

So for a missing file the state is what main already produced; what changes is that the reason is no longer empty. runAudit folds the output into the error and run() copies it into Reason, and stderr shares the buffer with stdout, so both -v 2 and --include-test-output show it.

One incidental argument for your form: matching on stat's wording was brittle - coreutils 9.x prints cannot statx, not cannot stat. Only the No such file or directory suffix is stable, and now nothing depends on it.

TestCheck_RunSaysWhyAFilePermissionCheckFailed pins both halves, and the YAML-wide test is reduced to asserting that no cis-1.12 permission check hides a missing file behind a guard, with 1.2.28 and 4.1.7/4.1.8 listed as deliberate exceptions.

I merged main in, so the branch is current. Worth noting that cis-2.0 and rke2-cis-1.9 both landed after this PR opened and carry the same guard in 43 audits - happy to extend this to them in a follow-up once you are satisfied with the shape here.

[andypitcher]

@Eljees thanks for the updates, lgtm. I agree that this can be replicated across other versions, you can tackle this thought this PR or another one.

CC @LaibaBareera could you review and advise regarding the other versions, whether that is done through this PR or another ?

[Eljees]

Thanks @andypitcher. I will send the other benchmarks as a separate PR rather than widening this one, so this stays reviewable and the shape is settled before it is repeated 43 times.

For reference, the same existence guard is in cis-2.0 (14 audits in master.yaml, 10 in node.yaml) and rke2-cis-1.9 (9 and 10). Both landed on main after this PR opened, which is why they are not covered by the YAML-wide test here - benchmarksWithoutFileExistenceGuards lists only cis-1.12, and adding a benchmark to that list is what makes the test start enforcing it.

@LaibaBareera happy to take direction on whether all four benchmarks should move together or one at a time.