Fix cis compliance stemcell issues - #702
Conversation
|
WalkthroughThe stemcell build enables sudo pseudo-terminal allocation and logging. It creates 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bosh-stemcell/spec/support/os_image_shared_examples.rb`:
- Around line 74-80: Update the `/etc/profile.d/01-tmout.sh` InSpec example to
assert that the file is owned by root and its group is root, alongside the
existing file, mode, and content checks.
- Around line 241-244: Update the “sets MaxStartups to 10:30:60” example to
validate the effective configuration rather than mere presence: assert that
active MaxStartups directives contain exactly one entry with the expected value,
or use sshd -T to verify the process-effective value. Preserve the expected
value “MaxStartups 10:30:60”.
In `@stemcell_builder/stages/base_ssh/apply.sh`:
- Around line 27-28: Quote the chroot-derived paths in the sshd_config update
commands: update the sed target and output redirection associated with the
MaxStartups replacement so "$chroot" cannot undergo word splitting or glob
expansion. Preserve the existing deletion and append behavior.
- Around line 27-28: Update the MaxStartups cleanup in apply.sh to remove
directives with either spaces or tabs before appending the stemcell value.
Ensure all existing MaxStartups lines are normalized away so only the appended
`MaxStartups 10:30:60` directive remains.
In `@stemcell_builder/stages/bosh_users/apply.sh`:
- Around line 41-45: Quote the $chroot path expansion in both the here-document
redirection and the chmod command within the TMOUT profile setup, preserving the
existing file path and permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9395556-6990-419d-af0d-2a4f1a0d305e
📒 Files selected for processing (4)
bosh-stemcell/spec/support/os_image_shared_examples.rbstemcell_builder/stages/base_ssh/apply.shstemcell_builder/stages/bosh_users/apply.shstemcell_builder/stages/bosh_users/assets/sudoers
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
- Set MaxStartups 10:30:100 in sshd_config (CIS 5.1.18) - Enable use_pty and logfile=/var/log/sudo.log in sudoers (CIS 5.2.2, 5.2.3) - Configure TMOUT=900 shell timeout in /etc/profile.d/01-tmout.sh (CIS 5.4.3.2) - Add corresponding rspec tests in os_image_shared_examples
- Change MaxStartups from default 10:30:100 to 10:30:60 in sshd_config - Update corresponding rspec test assertion
- Assert root ownership and group for /etc/profile.d/01-tmout.sh in rspec tests - Ensure MaxStartups scan validates single directive in sshd_config - Normalize MaxStartups sed pattern to match tab/space indentations and quote chroot paths - Quote chroot path expansions in TMOUT profile script creation
1f0b772 to
7936f44
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bosh-stemcell/spec/support/os_image_shared_examples.rb`:
- Around line 29-30: Update the sudo policy assertions in the shared examples
around the content match checks to validate the effective settings for the
intended sudo user, rather than relying only on ordered /etc/sudoers entries.
Ensure included sudoers files cannot override use_pty or logfile, while
preserving verification of the intended values.
In `@stemcell_builder/stages/bosh_users/assets/sudoers`:
- Around line 7-8: Update the sudoers asset by adding the /etc/sudoers.d include
directive after the privilege entries, preserving the required drop-in policy
loading and existing sudo configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8b15601-c6dc-4ad1-aeb0-289e7b975a07
📒 Files selected for processing (4)
bosh-stemcell/spec/support/os_image_shared_examples.rbstemcell_builder/stages/base_ssh/apply.shstemcell_builder/stages/bosh_users/apply.shstemcell_builder/stages/bosh_users/assets/sudoers
| its(:content) { should match(/^Defaults\s+use_pty$/) } | ||
| its(:content) { should match(%r{^Defaults\s+logfile=/var/log/sudo\.log$}) } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching os_image_shared_examples.rb:"
fd -a 'os_image_shared_examples.rb' . || true
file="$(fd 'os_image_shared_examples.rb' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Size:"
wc -l "$file"
echo
echo "Relevant file contents:"
cat -n "$file"
fi
echo
echo "Search for sudoers examples/use_pty/logfile:"
rg -n "use_pty|sudoers|logfile=/var/log/sudo\\.log|Defaults" . || true
echo
echo "Search for sudo executable/config validation in specs/support:"
rg -n "sudo --|visudo|/etc/sudoers|Defaults|PTYS|logfile" . || trueRepository: cloudfoundry/bosh-linux-stemcell-builder
Length of output: 50396
🌐 Web query:
sudoers Defaults use_pty last match wins includedir behavior sudo man pages
💡 Result:
In the sudoers configuration, the following behaviors apply regarding Defaults options, rule matching, and included directories: Defaults use_pty The use_pty option controls whether sudo runs commands in a new pseudo-terminal (pty) [1][2]. As of sudo version 1.9.14, use_pty is enabled by default to enhance security by preventing terminal-based attacks (such as TIOCSTI or TIOCLINUX command injection) [3]. Running in a pty creates an additional process to monitor the command and pass terminal signals [1][2]. If this interferes with specific workflows—such as those requiring direct access to the invoking user's terminal or asynchronous background processing—you can restore the historic behavior by adding Defaults!use_pty to your sudoers file [1][2][3]. Last Match Wins The sudoers file processes entries in the order they appear [4][5]. When multiple entries match a user and command, they are applied sequentially, and the last matching entry determines the final permission, tag, or option [4][6][7]. Because of this last-match-wins policy, the order of rules—including those in included files—is critical [4][5][6]. Includedir Behavior The @includedir directive (and the legacy #includedir) allows sudo to process additional configuration files from a specified directory, such as /etc/sudoers.d/ [8][9]. When sudo encounters an @includedir directive, it suspends processing of the current file, reads each file in the specified directory in lexicographical (alphabetical) order, and then returns to the original file [8][9][10]. Because sudoers rules are cumulative and evaluated in order, the lexicographical sorting of filenames (e.g., 01_file, 10_file) effectively determines which rules are processed last and thus which rules take precedence [4][6][8]. Sudo skips files with names containing a period ('.') or ending in a tilde ('~') to avoid processing editor or package manager temporary/backup files [8][11].
Citations:
- 1: https://github.com/sudo-project/sudo/blob/main/docs/UPGRADE.md
- 2: https://www.sudo.ws/docs/upgrade/
- 3: Please consider enabling option
use_ptyby default (security) sudo-project/sudo#258 - 4: https://askubuntu.com/questions/100051/why-is-sudoers-nopasswd-option-not-working
- 5: https://serverfault.com/questions/545407/sudo-use-on-a-user-in-multiple-groups-asking-for-password-when-one-group-has-no
- 6: https://www.simplified.guide/sudo/sudoers-rule-order-check
- 7: https://superuser.com/questions/1589696/let-a-single-user-execute-a-command-without-sudo-password
- 8: https://www.sudo.ws/docs/man/sudoers.man/
- 9: https://manpages.debian.org/bookworm/sudo/sudoers.5.en.html
- 10: https://manpages.ubuntu.com/manpages/lunar/man5/sudoers.5.html
- 11: https://man.archlinux.org/man/sudoers.5
Validate the effective sudo policy.
/etc/sudoers.d defaults can reorder or override the assertions because Defaults entries are cumulative and later matching entries can disable use_pty or change logfile. Check the effective policy for the intended sudo user, or ensure the included files prohibit conflicting sudoers settings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bosh-stemcell/spec/support/os_image_shared_examples.rb` around lines 29 - 30,
Update the sudo policy assertions in the shared examples around the content
match checks to validate the effective settings for the intended sudo user,
rather than relying only on ordered /etc/sudoers entries. Ensure included
sudoers files cannot override use_pty or logfile, while preserving verification
of the intended values.
| Defaults use_pty | ||
| Defaults logfile=/var/log/sudo.log |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the /etc/sudoers.d include in this asset.
stemcell_builder/stages/bosh_users/apply.sh copies this asset directly to /etc/sudoers at Lines 26-27. This asset has no #includedir /etc/sudoers.d, but bosh-stemcell/spec/support/os_image_shared_examples.rb requires it at Line 28. The image will fail that check, and sudo drop-in policies will not load. Add the include after the privilege entries.
Proposed fix
%bosh_sudoers ALL=(ALL) NOPASSWD: ALL
+
+#includedir /etc/sudoers.d🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stemcell_builder/stages/bosh_users/assets/sudoers` around lines 7 - 8, Update
the sudoers asset by adding the /etc/sudoers.d include directive after the
privilege entries, preserving the required drop-in policy loading and existing
sudo configuration.
1 participant
Changes should be made in the earliest applicable branch, and
merged forward through subsequent branches.
ubuntu-<short_name-N>ubuntu-<short_name-N>intoubuntu-<short_name-N+1>