Skip to content

fix: suppress CIS noise - #9061

Open
zachary-bailey wants to merge 3 commits into
mainfrom
zb/CISSuppression
Open

fix: suppress CIS noise#9061
zachary-bailey wants to merge 3 commits into
mainfrom
zb/CISSuppression

Conversation

@zachary-bailey

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

This PR adds a loop that constantly changes file permissions while the CIS assessor runs in order to prevent guest agent extension logs from causing the assessor to report failures outside of Node SIG control.

Which issue(s) this PR fixes:

Not a public issues

Fixes #

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the VHD CIS reporting script to reduce intermittent CIS assessor failures caused by background processes recreating or rewriting /var/log files with more permissive permissions during the scan window.

Changes:

  • Adds a normalize_log_perms helper and runs it repeatedly in a background loop during CIS assessment.
  • Adds cleanup logic (stop_perms_guard + trap ... EXIT) to ensure the background guard is stopped before report uploads.

Comment thread vhdbuilder/packer/cis-report.sh Outdated
Comment on lines +63 to +65
normalize_log_perms() {
find /var/log -type f -exec chmod 640 {} \;
}
Comment thread vhdbuilder/packer/cis-report.sh Outdated
# at 0640 for the entire assessment phase by re-normalizing in the background,
# then stop the guard before reading/uploading the reports.
normalize_log_perms
( while :; do normalize_log_perms; sleep 2; done ) &
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Windows Unit Test Results

  3 files   11 suites   47s ⏱️
381 tests 381 ✅ 0 💤 0 ❌
384 runs  384 ✅ 0 💤 0 ❌

Results for commit 345f697.

♻️ This comment has been updated with latest results.

Copilot AI review requested due to automatic review settings July 28, 2026 21:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

vhdbuilder/packer/cis-report.sh:65

  • chmod 640 sets permissions to exactly 0640, which can increase access for files that are already more restrictive (e.g., 0600). Since the CIS rule is "0640 or more restrictive", prefer removing the disallowed bits rather than forcing 0640. This also lets you batch chmod calls via -exec ... + for better performance.
reset_log_perms() {
    find /var/log -type f -exec chmod 640 {} \;
}

vhdbuilder/packer/cis-report.sh:76

  • The permissions guard runs in a background subshell, and because the script has set -x enabled, each loop iteration will be xtraced (every ~2s) and can significantly spam build logs during the assessor run. Consider disabling xtrace in the guard subshell.
reset_log_perms
( while :; do reset_log_perms; sleep 2; done ) &
PERMS_GUARD_PID=$!

vhdbuilder/packer/cis-report.sh:80

  • stop_perms_guard kills only the subshell PID. If reset_log_perms is in the middle of a find when the signal arrives, the find process can be orphaned and keep chmod'ing during uploads. Killing the guard's process group helps ensure any in-flight find/chmod children are terminated too.
    [ -n "${PERMS_GUARD_PID:-}" ] || return 0
    kill "$PERMS_GUARD_PID" 2>/dev/null || true
    wait "$PERMS_GUARD_PID" 2>/dev/null || true

Copilot AI review requested due to automatic review settings July 29, 2026 15:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

vhdbuilder/packer/cis-report.sh:65

  • reset_log_perms currently forces every /var/log file to exactly 0640, which can relax more-restrictive modes (e.g. 0600 -> 0640) and runs one chmod per file. You can avoid broadening permissions and reduce work by only touching files with disallowed bits and clearing them (while keeping already-restrictive files as-is).
# Reset /var/log permissions to satisfy the CIS logfile-access control
# (6.1.3.1 / 6.1.4.1), which requires generic /var/log files to be 0640 or
# more restrictive.
reset_log_perms() {
    find /var/log -type f -exec chmod 640 {} \;
}

vhdbuilder/packer/cis-report.sh:76

  • The permissions guard loop runs under set -e; if reset_log_perms returns non-zero (e.g. transient find errors during log rotation), the background subshell can exit and stop enforcing perms, bringing back the flake. Make the loop resilient by ignoring errors inside the guard.
reset_log_perms
( while :; do reset_log_perms; sleep 2; done ) &
PERMS_GUARD_PID=$!

vhdbuilder/packer/cis-report.sh:84

  • The guard cleanup is only trapped on EXIT. If this script is interrupted/terminated (e.g. CI cancellation sending SIGTERM), being explicit about trapping INT/TERM helps ensure the background process is always reaped.
# Ensure the guard is reaped even if the assessor exits non-zero under set -e.
trap stop_perms_guard EXIT

# (6.1.3.1 / 6.1.4.1), which requires generic /var/log files to be 0640 or
# more restrictive.
reset_log_perms() {
find /var/log -type f -exec chmod 640 {} \;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we explicitly use chmod 0640?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants