ci(security): fix Security Hardening Tests sudo failure on self-hosted runners#77
ci(security): fix Security Hardening Tests sudo failure on self-hosted runners#77somethingwithproof wants to merge 1 commit into
Conversation
…ssword The self-hosted runners already have jq, bc, and curl, but the dependency step ran sudo apt-get unconditionally and failed with 'a password is required', killing the job before any security test executed.
📝 WalkthroughWalkthroughThe security-hardening-tests workflow is updated to make system dependency installation more robust. The "Install system dependencies" step now conditionally installs ChangesConditional Dependency Installation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/security-tests.yml (1)
249-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same passwordless sudo fix to this job.
The
security-regression-testjob still uses unconditionalsudo apt-get install(lines 251-252), which will fail on self-hosted runners without passwordless sudo—the exact issue this PR fixes for thesecurity-hardening-testsjob. This job runs on the same[self-hosted, linux, x64]runner type and would encounter the same "sudo: a password is required" error.🔧 Proposed fix using the same conditional logic
- name: Install dependencies run: | - sudo apt-get update - sudo apt-get install -y jq bc + missing="" + for tool in jq bc; do + command -v "$tool" >/dev/null 2>&1 || missing="$missing $tool" + done + if [ -z "$missing" ]; then + echo "jq, bc already present; skipping apt install" + exit 0 + fi + if command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null; then + sudo apt-get update + sudo apt-get install -y$missing + else + echo "::error::Missing tools ($missing) and passwordless sudo unavailable on this runner" + exit 1 + fi python -m pip install --upgrade pip pip install ansible>=2.14 ansible-lint🤖 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 @.github/workflows/security-tests.yml around lines 249 - 254, The security-regression-test job currently runs unconditional sudo apt-get install (the commands installing jq and bc) which will fail on self-hosted runners without passwordless sudo; update the security-regression-test job to use the same conditional sudo logic used in security-hardening-tests: detect whether passwordless sudo is available and only prefix apt-get with sudo when it is, otherwise run the install command without sudo or skip with a safe fallback so the workflow won’t error on "sudo: a password is required" during the pip/apt steps.
🧹 Nitpick comments (1)
.github/workflows/security-tests.yml (1)
190-217: ⚡ Quick winAdd defensive dependency installation for consistency.
The
analyze-results.shscript requires jq and will fail if not installed, and this job uses bothjq(line 199) andbc(line 206) without installing them. While the PR description indicates these tools are pre-installed on the current self-hosted runners, applying the same conditional installation logic from thesecurity-hardening-testsjob would make this workflow more robust and consistent.♻️ Suggested defensive installation step
Add this step before "Analyze security compliance":
- name: Install system dependencies run: | missing="" for tool in jq bc; do command -v "$tool" >/dev/null 2>&1 || missing="$missing $tool" done if [ -z "$missing" ]; then echo "jq, bc already present; skipping apt install" exit 0 fi if command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null; then sudo apt-get update sudo apt-get install -y$missing else echo "::error::Missing tools ($missing) and passwordless sudo unavailable on this runner" exit 1 fi - name: Analyze security compliance run: |🤖 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 @.github/workflows/security-tests.yml around lines 190 - 217, The "Analyze security compliance" step uses jq and bc but doesn't ensure they exist; add a preceding step (e.g., "Install system dependencies") that checks for jq and bc and installs any missing tools using the same conditional logic as the security-hardening-tests job (detect missing commands, use passwordless sudo to apt-get update/install if available, and fail with a clear error if missing and sudo not available) so the Analyze security compliance step can run reliably on all runners.
🤖 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.
Outside diff comments:
In @.github/workflows/security-tests.yml:
- Around line 249-254: The security-regression-test job currently runs
unconditional sudo apt-get install (the commands installing jq and bc) which
will fail on self-hosted runners without passwordless sudo; update the
security-regression-test job to use the same conditional sudo logic used in
security-hardening-tests: detect whether passwordless sudo is available and only
prefix apt-get with sudo when it is, otherwise run the install command without
sudo or skip with a safe fallback so the workflow won’t error on "sudo: a
password is required" during the pip/apt steps.
---
Nitpick comments:
In @.github/workflows/security-tests.yml:
- Around line 190-217: The "Analyze security compliance" step uses jq and bc but
doesn't ensure they exist; add a preceding step (e.g., "Install system
dependencies") that checks for jq and bc and installs any missing tools using
the same conditional logic as the security-hardening-tests job (detect missing
commands, use passwordless sudo to apt-get update/install if available, and fail
with a clear error if missing and sudo not available) so the Analyze security
compliance step can run reliably on all runners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0da09ad7-dc60-4b04-b9d2-b9cc1e7b686c
📒 Files selected for processing (1)
.github/workflows/security-tests.yml
1 participant
The Security Hardening Tests job dies at the 'Install system dependencies' step:
The self-hosted runners lack passwordless sudo, but they already have jq, bc, and curl (the coverage step uses them). The step now skips apt when the tools are present and only attempts sudo when passwordless sudo is available, failing with a clear message otherwise. No security test is altered or skipped.
This does not make Lint pass; main has pre-existing ansible-lint debt tracked separately.
Summary by CodeRabbit