feat: Add RHEL GitHub Actions Runner image support for ppc64le#1
Conversation
…runner-image-pz for IBM Power (ppc64le) runners This PR adds RHEL 9/10 as a supported OS for building GitHub Actions Runner images, targeting ppc64le (IBM Power). It introduces a full images/rhel/ directory alongside the existing Ubuntu and CentOS image definitions, and updates the shared build scripts so they recognize RHEL throughout the pipeline. What's included: New RHEL image definition (images/rhel/) - Build scripts for all core components: Docker, Git, Python, .NET SDK, GitHub CLI, Homebrew, Podman, LXD, and more - Toolset JSON configs for RHEL 9 and RHEL 10 - Helper libraries for package installation with retry logic, environment variable management, and OS detection - Post-generation scripts for cleanup, environment setup, and systemd linger
…ckages (parallel, patchelf, shellcheck, etc.) are available before install-dnf-common.sh runs
…PEL/CRB setup, and use sha256sum fallback for checksum verification
On RHEL PowerVS instances there is no ubuntu user — the default is root and vm.sh creates a runner user. Patch the installed service file to User=runner when building RHEL so the systemd service starts correctly. Ubuntu builds are unchanged. Fixes #3 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces complete RHEL image build support: shared helper libraries ( ChangesRHEL Image Build Support
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
This feature is known to be working for ppc64le provisioning as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
images/rhel/scripts/helpers/install.sh-22-22 (1)
22-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound each download attempt with curl timeouts.
Line 22 retries only after
curlreturns; a stalled TCP connection can hang the image build forever before the retry loop advances.Proposed fix
- if http_code=$(curl -4sSLo "$download_path" "$url" -w '%{http_code}'); then + if http_code=$(curl -4sSLo "$download_path" "$url" --connect-timeout 30 --max-time 600 -w '%{http_code}'); then🤖 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 `@images/rhel/scripts/helpers/install.sh` at line 22, The download retry loop in install.sh can hang indefinitely because curl has no timeout bounds. Update the curl invocation used for each download attempt to enforce both connection and overall transfer timeouts, and keep the retry behavior around the existing http_code capture logic so stalled connections fail fast and the loop can continue.images/rhel/scripts/helpers/install.sh-237-244 (1)
237-244: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the requested checksum algorithm in the fallback path.
If
shasumis unavailable andsha_type=512, Line 240 still computes SHA256, so valid SHA512 checksums fail on minimal RHEL images.Proposed fix
- if command -v shasum &>/dev/null; then - local_file_hash=$(shasum --algorithm "$sha_type" "$file_path" | awk '{print $1}') - else - local_file_hash=$(sha256sum "$file_path" | awk '{print $1}') - fi + case "$sha_type" in + 256) + if command -v shasum &>/dev/null; then + local_file_hash=$(shasum --algorithm 256 "$file_path" | awk '{print $1}') + else + local_file_hash=$(sha256sum "$file_path" | awk '{print $1}') + fi + ;; + 512) + if command -v shasum &>/dev/null; then + local_file_hash=$(shasum --algorithm 512 "$file_path" | awk '{print $1}') + else + local_file_hash=$(sha512sum "$file_path" | awk '{print $1}') + fi + ;; + *) + echo "Unsupported SHA type: $sha_type" >&2 + exit 1 + ;; + esac + + checksum=$(printf '%s' "$checksum" | tr '[:upper:]' '[:lower:]')🤖 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 `@images/rhel/scripts/helpers/install.sh` around lines 237 - 244, The checksum fallback in the install script ignores the requested algorithm and always uses SHA256 when shasum is unavailable. Update the fallback logic in the checksum verification block so it computes the hash using the same sha_type requested by the caller, preserving SHA512 support on minimal RHEL images. Keep the behavior consistent with the existing shasum path and the variables file_path, checksum, and sha_type.images/rhel/scripts/helpers/etc-environment.sh-95-102 (1)
95-102: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid evaluating
/etc/environmentas shell code.Line 98 executes file contents via
eval; any metacharacters written by earlier scripts or packages run as code during image build. Line 101 also appends a trailing empty PATH component when no PATH entry exists.Proposed safer parser
reload_etc_environment() { - # add `export ` to every variable of /etc/environemnt except PATH and eval the result shell script - # shellcheck disable=SC2046 - eval $(grep -v '^PATH=' /etc/environment | sed -e 's%^%export %') - # handle PATH specially - etc_path=$(get_etc_environment_variable PATH) - export PATH="$PATH:$etc_path" + local line variable_name variable_value etc_path="" + + while IFS= read -r line || [[ -n "$line" ]]; do + [[ -z "$line" || "$line" =~ ^[[:space:]]*# || "$line" != *=* ]] && continue + + variable_name=${line%%=*} + variable_value=${line#*=} + variable_value=${variable_value%\"} + variable_value=${variable_value#\"} + + if [[ ! "$variable_name" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo "Invalid variable name in /etc/environment: $variable_name" >&2 + return 1 + fi + + if [[ "$variable_name" == "PATH" ]]; then + etc_path=$variable_value + else + export "$variable_name=$variable_value" + fi + done < /etc/environment + + if [[ -n "$etc_path" ]]; then + export PATH="$PATH:$etc_path" + fi }🤖 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 `@images/rhel/scripts/helpers/etc-environment.sh` around lines 95 - 102, The reload_etc_environment function is evaluating /etc/environment as shell code via eval, which should be replaced with a safe parser that reads key/value pairs directly and only exports valid assignments; keep the logic in reload_etc_environment and get_etc_environment_variable, but avoid executing file contents. Also update the PATH handling so it only appends the /etc/environment PATH when present, preventing a trailing empty PATH component when no PATH entry exists.Source: Linters/SAST tools
images/rhel/scripts/helpers/install.sh-153-167 (1)
153-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the not-found checks reachable under
bash -e.grepin both lookup paths exits the shell on a miss before the later-z/ duplicate-match branches can run, so the intended error messages never fire. Switch those searches to a non-fatal form and useprintf '%s\n'/grep -F --instead of treating release/checksum contents as format strings or regexes.🤖 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 `@images/rhel/scripts/helpers/install.sh` around lines 153 - 167, The lookup logic in install.sh’s release-body and checksum search path is failing too early under bash -e because the grep-based searches in the matching_releases/matched_line flow abort before the not-found and duplicate-match checks can run. Update the search steps to use non-fatal matching so the later validation branches remain reachable, and make the content lookups safer by using printf '%s\n' plus grep -F -- when matching file_name or checksum text in the helper functions that build matched_line and hash.images/rhel/assets/post-gen/systemd-linger.sh-4-5 (1)
4-5: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnable linger for the provisioned runner user, not the last passwd record.
These lines target whichever account happens to be appended last to
/etc/passwd. If that is notrunner, linger is enabled for the wrong user and the runner service still will not have its user session available at boot.Suggested fix
-UserId=$(cut -d: -f3 /etc/passwd | tail -1) -loginctl enable-linger "$UserId" +loginctl enable-linger runner🤖 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 `@images/rhel/assets/post-gen/systemd-linger.sh` around lines 4 - 5, The linger setup in systemd-linger.sh is using the last numeric passwd entry instead of the provisioned runner account. Update the script to resolve the intended runner user explicitly before calling loginctl enable-linger, and keep the enable-linger call tied to that resolved username/UID so the boot-time user session is enabled for the correct account.images/rhel/assets/post-gen/environment-variables.sh-5-6 (1)
5-6: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't infer the runner home from the last passwd entry.
Line 5 assumes the final
/etc/passwdrecord is the default runner account. That is not stable once other users are created, so this can rewrite/etc/environmentwith the wrong home directory and break runner PATH/tool resolution. Resolve the provisioned runner user explicitly instead.Suggested fix
-homeDir=$(cut -d: -f6 /etc/passwd | tail -1) -sed -i "s|\$HOME|$homeDir|g" /etc/environment +runnerHome="$(getent passwd runner | cut -d: -f6)" +[ -n "$runnerHome" ] || { echo "runner user not found" >&2; exit 1; } +sed -i "s|\$HOME|$runnerHome|g" /etc/environment🤖 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 `@images/rhel/assets/post-gen/environment-variables.sh` around lines 5 - 6, The home directory logic in environment-variables.sh is relying on the last /etc/passwd entry, which can point to the wrong user once additional accounts exist. Update the script to resolve the provisioned runner user explicitly instead of using tail-based inference, then use that user’s home directory in the sed replacement that updates /etc/environment. Keep the fix localized around the homeDir assignment and the /etc/environment rewrite.scripts/vm.sh-50-55 (1)
50-55: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the
runnerhardening outside the existence guard.If
runneralready exists, reruns now skip bothusermod -Land/etc/sudoers.d/runner, so a partially provisioned machine can leave the service account unlocked or without passwordless sudo. Onlyuseraddshould be conditional.Suggested fix
if ! id -u runner >/dev/null 2>&1; then sudo useradd -c "Action Runner" -m -s /bin/bash runner - sudo usermod -L runner - echo 'runner ALL=(ALL) NOPASSWD: ALL' | sudo tee /etc/sudoers.d/runner > /dev/null - sudo chmod 440 /etc/sudoers.d/runner fi + +sudo usermod -L runner +echo 'runner ALL=(ALL) NOPASSWD: ALL' | sudo tee /etc/sudoers.d/runner > /dev/null +sudo chmod 440 /etc/sudoers.d/runner🤖 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 `@scripts/vm.sh` around lines 50 - 55, Only the user creation in the runner setup should remain under the id -u runner existence check, while the hardening steps in scripts/vm.sh must always run. Move the usermod -L runner and /etc/sudoers.d/runner creation/chmod logic out of the conditional so reruns through the runner provisioning path still lock the account and restore passwordless sudo; use the existing runner setup block as the anchor.scripts/helpers/setup_install.sh-32-44 (1)
32-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize AlmaLinux into this OS family check.
On the VM path,
run.shforwards the raw/etc/os-releaseID; on AlmaLinux hosts that'salmalinux, so this branch is skipped and none of the DNF bootstrap runs. The same family mapping also needs to be applied inscripts/helpers/setup_vars.shand theget_setup_type()check so AlmaLinux 9/10 keeps using the shared CentOS toolsets/minimal flow.Suggested fix
-elif [[ "$IMAGE_OS" == *"centos"* || "$IMAGE_OS" == *"rhel"* ]]; then +elif [[ "$IMAGE_OS" == *"centos"* || "$IMAGE_OS" == *"almalinux"* || "$IMAGE_OS" == *"rhel"* ]]; then🤖 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 `@scripts/helpers/setup_install.sh` around lines 32 - 44, Normalize AlmaLinux handling in the OS-family checks so `setup_install.sh` follows the same CentOS/RHEL DNF bootstrap path when `IMAGE_OS` is `almalinux`. Update the family detection used by `run.sh`/`setup_vars.sh` and the `get_setup_type()` logic so AlmaLinux 9/10 maps into the shared CentOS toolset/minimal flow, ensuring the existing `configure-yum-mock.sh`, `configure-dnf.sh`, and related DNF setup steps still run.images/rhel/scripts/build/configure-dnf.sh-24-25 (1)
24-25: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
dnf-automaticremoval with a presence check.With
set -e, Line 25 aborts the build whendnf-automaticis not installed, even though the comment says "if present".Suggested fix
# Remove unattended-upgrade equivalents if present (e.g., dnf-automatic) -dnf remove -y dnf-automatic +if rpm -q dnf-automatic >/dev/null 2>&1; then + dnf remove -y dnf-automatic +fi🤖 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 `@images/rhel/scripts/build/configure-dnf.sh` around lines 24 - 25, The unconditional dnf-automatic removal in configure-dnf.sh conflicts with the “if present” intent and can fail under set -e when the package is missing. Update the removal step to first check whether dnf-automatic is installed, then only call dnf remove from that guarded path; keep the logic localized around the existing dnf-automatic handling in configure-dnf.sh.images/rhel/scripts/build/configure-dnf.sh-16-17 (1)
16-17: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove
phased_updates=1from DNF configphased_updatesisn’t a supporteddnf.confkey on RHEL 9/10, and DNF5 treats unknown options as a config error.🤖 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 `@images/rhel/scripts/build/configure-dnf.sh` around lines 16 - 17, Remove the unsupported DNF config setting from the configure-dnf.sh script so it no longer appends phased_updates=1 to /etc/dnf/dnf.conf. Update the DNF configuration step in the script to avoid writing any unknown dnf.conf keys, keeping the logic limited to supported settings only.images/rhel/scripts/build/configure-environment.sh-32-36 (1)
32-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid
chmod -R 777on the tool cache.This directory is exported as both
AGENT_TOOLSDIRECTORYandRUNNER_TOOL_CACHE; making it world-writable lets any local account tamper with cached toolchains that later jobs execute. Prefer runner-owned permissions or a dedicated writable group instead.🤖 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 `@images/rhel/scripts/build/configure-environment.sh` around lines 32 - 36, The tool cache setup in configure-environment.sh is making AGENT_TOOLSDIRECTORY and RUNNER_TOOL_CACHE world-writable via chmod -R 777, which is too permissive. Update the AGENT_TOOLSDIRECTORY initialization block to use runner-owned or dedicated group permissions instead, and ensure the directory remains writable by the runner without granting access to all local accounts. Keep the set_etc_environment_variable calls intact, but replace the recursive permission change with a safer ownership/permission approach around the AGENT_TOOLSDIRECTORY setup.Source: Linters/SAST tools
images/rhel/scripts/build/configure-image-data.sh-12-29 (1)
12-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe generated image metadata is still hard-coded to CentOS.
This script labels the image as
centos-*and builds the Included Software URL underimages/centos/toolsets/toolset-${image_version_major}${image_version_minor}.json, but this PR adds RHEL toolsets atimages/rhel/toolsets/toolset-9.jsonandtoolset-10.json. The publishedimagedata.jsonwill therefore advertise the wrong OS and point to a non-existent toolset document.Suggested fix
-# Determine OS name and version for CentOS +# Determine OS name and version for RHEL @@ -image_label="centos-${os_version}" # Set image label +image_label="rhel-${os_version}" # Set image label @@ -software_url="${github_url}/centos/toolsets/toolset-${image_version_major}${image_version_minor}.json" +software_url="${github_url}/rhel/toolsets/toolset-${os_version}.json"🤖 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 `@images/rhel/scripts/build/configure-image-data.sh` around lines 12 - 29, The image metadata generation in configure-image-data.sh is still hard-coded for CentOS, so update the OS labeling and software URL construction to use the RHEL paths introduced by this PR. Replace the centos-* image_label logic and the images/centos/toolsets/toolset-${image_version_major}${image_version_minor}.json URL in the metadata assembly with the corresponding RHEL naming/path, and make sure the variables that build imagedata.json (such as image_label and software_url) reflect the actual RHEL release/toolset being published.images/rhel/scripts/build/configure-system.sh-25-38 (1)
25-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis PATH fix is a no-op when
/etc/environmenthas noPATH=entry.
replace_etc_environment_variableonly rewrites an existing line, so an empty or missingPATHleaves the file unchanged and the added system directories are never persisted. Useset_etc_environment_variablehere so the variable is created when absent.Suggested fix
-ENVPATH=$(grep 'PATH=' /etc/environment | head -n 1 | sed -z 's/^PATH=*//') +ENVPATH=$(grep '^PATH=' /etc/environment | head -n 1 | sed 's/^PATH=//') ENVPATH=${ENVPATH#"\""} ENVPATH=${ENVPATH%"\""} @@ -replace_etc_environment_variable "PATH" "${ENVPATH}" +ENVPATH=${ENVPATH#:} +set_etc_environment_variable "PATH" "${ENVPATH}"🤖 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 `@images/rhel/scripts/build/configure-system.sh` around lines 25 - 38, The PATH update in configure-system.sh is ineffective when /etc/environment does not already contain a PATH entry because replace_etc_environment_variable only updates existing values. In the configure-system.sh flow around ENVPATH construction, switch to set_etc_environment_variable for PATH so the variable is created if missing, while still preserving the appended system directories and the existing PATH parsing logic.images/rhel/scripts/build/install-dotnetcore-sdk.sh-24-26 (1)
24-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInstall all .NET SDKs declared by the toolset on ppc64le/s390x. The RHEL 9 toolset lists
8.0,9.0, and10.0, but this branch installs onlydotnet-sdk-8.0, so those images won’t match the advertised toolset.Proposed fix
if [[ "$ARCH" == "ppc64le" || "$ARCH" == "s390x" ]]; then echo "Installing dotnet for architecture: $ARCH" - install_dnfpkgs dotnet-sdk-8.0 + for version in $dotnet_versions; do + install_dnfpkgs "dotnet-sdk-${version}" + done else🤖 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 `@images/rhel/scripts/build/install-dotnetcore-sdk.sh` around lines 24 - 26, The ppc64le/s390x branch in install-dotnetcore-sdk.sh only installs dotnet-sdk-8.0, but the RHEL 9 toolset expects 8.0, 9.0, and 10.0 to be present. Update the install logic in the install-dotnetcore-sdk.sh path so the existing architecture check installs all SDK packages declared by the toolset, using the same install_dnfpkgs flow and keeping the ARCH-specific condition intact.images/rhel/scripts/build/install-docker.sh-111-121 (1)
111-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not create
/run/docker.sockwith a tmpfilesLentry.
L /run/docker.sock ... root docker 0770defines a symlink, not socket permissions. Because this runs before Docker starts, it can pre-create the socket path incorrectly and break daemon startup.Proposed fix
-# Create systemd-tmpfiles configuration for Docker -cat <<EOF | tee /etc/tmpfiles.d/docker.conf -L /run/docker.sock - - - - root docker 0770 -EOF - -# Reload systemd-tmpfiles to apply the new configuration -systemd-tmpfiles --create /etc/tmpfiles.d/docker.conf +mkdir -p /etc/systemd/system/docker.socket.d +cat <<EOF > /etc/systemd/system/docker.socket.d/socket-group.conf +[Socket] +SocketUser=root +SocketGroup=docker +SocketMode=0770 +EOF +systemctl daemon-reload # Enable docker.service +systemctl is-enabled --quiet docker.socket || systemctl enable docker.socket +systemctl is-active --quiet docker.socket || systemctl start docker.socket systemctl is-active --quiet docker.service || systemctl start docker.service systemctl is-enabled --quiet docker.service || systemctl enable docker.service🤖 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 `@images/rhel/scripts/build/install-docker.sh` around lines 111 - 121, The tmpfiles setup in install-docker.sh is incorrectly creating /run/docker.sock with a symlink entry, which can interfere with Docker startup. Remove the tmpfiles.d/docker.conf generation and the L entry for /run/docker.sock, and rely on Docker/systemd to create the socket path when docker.service starts; keep the existing systemctl start/enable logic in place.images/rhel/scripts/build/install-git-lfs.sh-13-13 (1)
13-13: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid executing the packagecloud script directly from the network.
Line 13 gives packagecloud-controlled mutable content root execution in the image build. Download it to a temporary file, verify a pinned checksum/signature, then execute the verified local copy.
🤖 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 `@images/rhel/scripts/build/install-git-lfs.sh` at line 13, The install-git-lfs.sh flow is executing the packagecloud RPM setup script directly from the network, which should be replaced with a safer verify-then-run approach. Update the script to download the packagecloud installer to a temporary local file, verify it against a pinned checksum or signature, and only then execute the verified copy instead of piping the remote content to bash.Source: Linters/SAST tools
images/rhel/scripts/build/install-zstd.sh-25-35 (1)
25-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBuild zstd inside a unique temporary directory.
Using
/tmp/${release_name}is predictable. A local pre-created path can hijack extraction/build inputs before binaries are copied into/usr/local/bin.Proposed fix
# Extract and build zstd -tar xzf "$archive_path" -C /tmp +tmp_dir="$(mktemp -d)" +trap 'rm -rf "$tmp_dir"' EXIT +tar xzf "$archive_path" -C "$tmp_dir" +source_dir="${tmp_dir}/${release_name}" -make -C "/tmp/${release_name}/contrib/pzstd" all -make -C "/tmp/${release_name}" zstd-release +make -C "${source_dir}/contrib/pzstd" all +make -C "$source_dir" zstd-release # Copy binaries for copyprocess in zstd zstdless zstdgrep; do - cp "/tmp/${release_name}/programs/${copyprocess}" /usr/local/bin/ + cp "${source_dir}/programs/${copyprocess}" /usr/local/bin/ done -cp "/tmp/${release_name}/contrib/pzstd/pzstd" /usr/local/bin/ +cp "${source_dir}/contrib/pzstd/pzstd" /usr/local/bin/🤖 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 `@images/rhel/scripts/build/install-zstd.sh` around lines 25 - 35, The install-zstd.sh flow currently extracts and builds from the predictable /tmp/${release_name} path, which can be pre-created and hijacked before the binaries are copied into /usr/local/bin. Update the install steps around the tar extraction and the make/cp commands to use a unique temporary directory created at runtime, and reference that directory consistently for the release_name-based build paths so the build inputs cannot be spoofed.Source: Linters/SAST tools
images/rhel/scripts/build/install-actions-cache.sh-13-22 (1)
13-22: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not make the action archive cache world-writable.
Line 15 lets any local user/job tamper with cached action archives that the runner later consumes. Keep the cache root-owned and read-only for non-root users after extraction.
Proposed fix
ACTION_ARCHIVE_CACHE_DIR=/opt/actionarchivecache -mkdir -p $ACTION_ARCHIVE_CACHE_DIR -chmod -R 777 $ACTION_ARCHIVE_CACHE_DIR +mkdir -p "$ACTION_ARCHIVE_CACHE_DIR" +chmod 755 "$ACTION_ARCHIVE_CACHE_DIR" echo "Setting up ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE variable to ${ACTION_ARCHIVE_CACHE_DIR}" set_etc_environment_variable "ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE" "${ACTION_ARCHIVE_CACHE_DIR}" # Download latest release from github.com/actions/action-versions and untar to /opt/actionarchivecache download_url=$(resolve_github_release_asset_url "actions/action-versions" "endswith(\"action-versions.tar.gz\")" "latest") archive_path=$(download_with_retry "$download_url") -tar -xzf "$archive_path" -C $ACTION_ARCHIVE_CACHE_DIR +tar -xzf "$archive_path" -C "$ACTION_ARCHIVE_CACHE_DIR" +find "$ACTION_ARCHIVE_CACHE_DIR" -type d -exec chmod 755 {} + +find "$ACTION_ARCHIVE_CACHE_DIR" -type f -exec chmod 644 {} +🤖 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 `@images/rhel/scripts/build/install-actions-cache.sh` around lines 13 - 22, The install-actions-cache.sh setup makes the action archive cache world-writable, which allows local tampering. Update the ACTION_ARCHIVE_CACHE_DIR setup and extraction flow so the cache remains root-owned and writable only during extraction, then lock it down to read-only for non-root users afterward; keep the existing ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE and download_with_retry/tar flow intact while changing the permissions around ACTION_ARCHIVE_CACHE_DIR.Source: Linters/SAST tools
images/rhel/scripts/build/install-git.sh-23-24 (1)
23-24: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the GitHub and Azure DevOps SSH host keys instead of trusting
ssh-keyscanoutput.ssh-keyscandoes not authenticate the server, so these lines can bake attacker-controlled keys into/etc/ssh/ssh_known_hostsduring the image build. Compare against the official fingerprints and append only matching keys.🤖 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 `@images/rhel/scripts/build/install-git.sh` around lines 23 - 24, The ssh-keyscan-based host key setup in install-git.sh is trusting unauthenticated output for GitHub and Azure DevOps; replace it with pinned, official SSH host key fingerprints and only write keys that match those fingerprints. Update the ssh-keyscan handling in the install script so it verifies the fetched keys for github.com and ssh.dev.azure.com before appending them to /etc/ssh/ssh_known_hosts.images/rhel/scripts/build/configure-snap.sh-10-19 (1)
10-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis never updates
/etc/environment, so--norcshells will still miss/snap/bin.The comment describes the right persistence target, but Lines 16-17 only touch the current process and
~/.bashrc.~/.bashrcis also skipped by--norc, and when this runs as root it only affects root’s home. Persist the PATH change through the sourced/etc/environmenthelper instead.🛠️ Proposed fix
if [[ ":$PATH:" == *"/snap/bin"* ]]; then echo "/snap/bin is already in the PATH" else echo "/snap/bin is not in the PATH. Adding it now..." - export PATH=/snap/bin:$PATH - echo "export PATH=/snap/bin:$PATH" >> ~/.bashrc # Persist for future sessions + current_path=$(get_etc_environment_variable PATH) + set_etc_environment_variable PATH "/snap/bin:${current_path}" + reload_etc_environment echo "/snap/bin has been added to the PATH" fi🤖 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 `@images/rhel/scripts/build/configure-snap.sh` around lines 10 - 19, The PATH update in configure-snap.sh only affects the current shell and appends to ~/.bashrc, so it will not persist for --norc sessions or the intended system-wide environment. Change the persistence logic in the script’s PATH handling block to update the /etc/environment-based helper instead of ~/.bashrc, while keeping the existing /snap/bin presence check and ensuring the change is applied system-wide for future shells.images/rhel/scripts/build/install-lxd.sh-9-19 (1)
9-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBroaden the LXD track match and fail fast if no channel is found
images/rhel/scripts/build/install-lxd.sh:9-19— the currentgreponly matchesx.0/stable, so it misses the LXD LTS track format (5.21/stable). If nothing matches, the script still reachessnap installwith--channel=/stable. Match the real channel format and exit before installing whenLATEST_LTS_CHANNELis empty.🤖 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 `@images/rhel/scripts/build/install-lxd.sh` around lines 9 - 19, The LXD channel detection in install-lxd.sh is too narrow and can fall through to an invalid snap install. Update the LATEST_LTS_CHANNEL lookup in the install-lxd.sh logic to match the actual LTS track format used by LXD (for example, 5.21/stable-style channels rather than only x.0/stable), and if no channel is found, exit before reaching the snap install step. Keep the fix centered around the LATEST_LTS_CHANNEL assignment and the subsequent install block.images/rhel/scripts/build/install-homebrew.sh-23-24 (1)
23-24: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't execute the Homebrew installer directly from GitHub. This makes image builds depend on mutable upstream code; pin a specific revision and verify its checksum before running it locally.
🤖 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 `@images/rhel/scripts/build/install-homebrew.sh` around lines 23 - 24, The Homebrew install step currently pulls and executes the upstream installer script directly, which should be replaced with a pinned local download flow. Update the install logic in install-homebrew.sh to fetch a specific installer revision, verify its checksum before execution, and then run the verified local copy instead of piping curl output to bash. Use the existing Homebrew install block as the place to introduce the pinned versioning and checksum validation.Source: Linters/SAST tools
🟡 Minor comments (2)
images/rhel/scripts/build/configure-dnfpkg.sh-16-25 (1)
16-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the DNF settings in
images/rhel/scripts/build/configure-dnfpkg.sh:16-25.override_install_langsonly limits installed locale payloads, so it does not match the config-file prompt behavior described here, andautoclean_metadatais not a validdnf.confkey and will be ignored.🤖 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 `@images/rhel/scripts/build/configure-dnfpkg.sh` around lines 16 - 25, The DNF config in configure-dnfpkg.sh uses the wrong setting for suppressing config-file prompts and includes an invalid dnf.conf option. Update the script so the logic around the dnf.conf writes uses the correct key for noninteractive config file handling instead of override_install_langs, and remove or replace autoclean_metadata with a valid DNF setting. Keep the changes localized to the dnf.conf update block in configure-dnfpkg.sh.images/rhel/scripts/build/configure-environment.sh-20-21 (1)
20-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't persist a literal
$HOMEin/etc/environment.This writes
XDG_CONFIG_HOME=$HOME/.configverbatim, so the value is non-absolute and XDG-aware tools may ignore it or resolve the wrong path. Set this from the runner user's profile or service environment instead.🤖 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 `@images/rhel/scripts/build/configure-environment.sh` around lines 20 - 21, The current use of set_etc_environment_variable is writing a literal $HOME-based value into /etc/environment, which should not be persisted there. Update configure-environment.sh so XDG_CONFIG_HOME is set from the runner user’s profile or service environment instead of using set_etc_environment_variable with '$HOME/.config', and ensure the configured value is an absolute path at runtime.
🤖 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 `@images/rhel/scripts/build/configure-system.sh`:
- Around line 17-21: The permission changes in configure-system.sh are too broad
because chmod -R 777 is making /opt and /usr/share world-writable. Update the
script to scope write access only to the specific runner-owned subdirectories
that need it, and keep the changes localized within the existing
permission-adjustment block so the rest of the system trees remain protected.
In `@images/rhel/scripts/helpers/install.sh`:
- Around line 279-286: The DNF install/update pipeline in install.sh is masking
failures because the exit status comes from tee instead of dnf, so a failed
package operation can still let the build continue. Update the install_dnfpkgs
and update_dnfpkgs paths to preserve the dnf command’s failure status while
still logging output to /tmp/install.errors, and use the existing dnf invocation
points in these helper functions to locate the change.
---
Major comments:
In `@images/rhel/assets/post-gen/environment-variables.sh`:
- Around line 5-6: The home directory logic in environment-variables.sh is
relying on the last /etc/passwd entry, which can point to the wrong user once
additional accounts exist. Update the script to resolve the provisioned runner
user explicitly instead of using tail-based inference, then use that user’s home
directory in the sed replacement that updates /etc/environment. Keep the fix
localized around the homeDir assignment and the /etc/environment rewrite.
In `@images/rhel/assets/post-gen/systemd-linger.sh`:
- Around line 4-5: The linger setup in systemd-linger.sh is using the last
numeric passwd entry instead of the provisioned runner account. Update the
script to resolve the intended runner user explicitly before calling loginctl
enable-linger, and keep the enable-linger call tied to that resolved
username/UID so the boot-time user session is enabled for the correct account.
In `@images/rhel/scripts/build/configure-dnf.sh`:
- Around line 24-25: The unconditional dnf-automatic removal in configure-dnf.sh
conflicts with the “if present” intent and can fail under set -e when the
package is missing. Update the removal step to first check whether dnf-automatic
is installed, then only call dnf remove from that guarded path; keep the logic
localized around the existing dnf-automatic handling in configure-dnf.sh.
- Around line 16-17: Remove the unsupported DNF config setting from the
configure-dnf.sh script so it no longer appends phased_updates=1 to
/etc/dnf/dnf.conf. Update the DNF configuration step in the script to avoid
writing any unknown dnf.conf keys, keeping the logic limited to supported
settings only.
In `@images/rhel/scripts/build/configure-environment.sh`:
- Around line 32-36: The tool cache setup in configure-environment.sh is making
AGENT_TOOLSDIRECTORY and RUNNER_TOOL_CACHE world-writable via chmod -R 777,
which is too permissive. Update the AGENT_TOOLSDIRECTORY initialization block to
use runner-owned or dedicated group permissions instead, and ensure the
directory remains writable by the runner without granting access to all local
accounts. Keep the set_etc_environment_variable calls intact, but replace the
recursive permission change with a safer ownership/permission approach around
the AGENT_TOOLSDIRECTORY setup.
In `@images/rhel/scripts/build/configure-image-data.sh`:
- Around line 12-29: The image metadata generation in configure-image-data.sh is
still hard-coded for CentOS, so update the OS labeling and software URL
construction to use the RHEL paths introduced by this PR. Replace the centos-*
image_label logic and the
images/centos/toolsets/toolset-${image_version_major}${image_version_minor}.json
URL in the metadata assembly with the corresponding RHEL naming/path, and make
sure the variables that build imagedata.json (such as image_label and
software_url) reflect the actual RHEL release/toolset being published.
In `@images/rhel/scripts/build/configure-snap.sh`:
- Around line 10-19: The PATH update in configure-snap.sh only affects the
current shell and appends to ~/.bashrc, so it will not persist for --norc
sessions or the intended system-wide environment. Change the persistence logic
in the script’s PATH handling block to update the /etc/environment-based helper
instead of ~/.bashrc, while keeping the existing /snap/bin presence check and
ensuring the change is applied system-wide for future shells.
In `@images/rhel/scripts/build/configure-system.sh`:
- Around line 25-38: The PATH update in configure-system.sh is ineffective when
/etc/environment does not already contain a PATH entry because
replace_etc_environment_variable only updates existing values. In the
configure-system.sh flow around ENVPATH construction, switch to
set_etc_environment_variable for PATH so the variable is created if missing,
while still preserving the appended system directories and the existing PATH
parsing logic.
In `@images/rhel/scripts/build/install-actions-cache.sh`:
- Around line 13-22: The install-actions-cache.sh setup makes the action archive
cache world-writable, which allows local tampering. Update the
ACTION_ARCHIVE_CACHE_DIR setup and extraction flow so the cache remains
root-owned and writable only during extraction, then lock it down to read-only
for non-root users afterward; keep the existing
ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE and download_with_retry/tar flow intact
while changing the permissions around ACTION_ARCHIVE_CACHE_DIR.
In `@images/rhel/scripts/build/install-docker.sh`:
- Around line 111-121: The tmpfiles setup in install-docker.sh is incorrectly
creating /run/docker.sock with a symlink entry, which can interfere with Docker
startup. Remove the tmpfiles.d/docker.conf generation and the L entry for
/run/docker.sock, and rely on Docker/systemd to create the socket path when
docker.service starts; keep the existing systemctl start/enable logic in place.
In `@images/rhel/scripts/build/install-dotnetcore-sdk.sh`:
- Around line 24-26: The ppc64le/s390x branch in install-dotnetcore-sdk.sh only
installs dotnet-sdk-8.0, but the RHEL 9 toolset expects 8.0, 9.0, and 10.0 to be
present. Update the install logic in the install-dotnetcore-sdk.sh path so the
existing architecture check installs all SDK packages declared by the toolset,
using the same install_dnfpkgs flow and keeping the ARCH-specific condition
intact.
In `@images/rhel/scripts/build/install-git-lfs.sh`:
- Line 13: The install-git-lfs.sh flow is executing the packagecloud RPM setup
script directly from the network, which should be replaced with a safer
verify-then-run approach. Update the script to download the packagecloud
installer to a temporary local file, verify it against a pinned checksum or
signature, and only then execute the verified copy instead of piping the remote
content to bash.
In `@images/rhel/scripts/build/install-git.sh`:
- Around line 23-24: The ssh-keyscan-based host key setup in install-git.sh is
trusting unauthenticated output for GitHub and Azure DevOps; replace it with
pinned, official SSH host key fingerprints and only write keys that match those
fingerprints. Update the ssh-keyscan handling in the install script so it
verifies the fetched keys for github.com and ssh.dev.azure.com before appending
them to /etc/ssh/ssh_known_hosts.
In `@images/rhel/scripts/build/install-homebrew.sh`:
- Around line 23-24: The Homebrew install step currently pulls and executes the
upstream installer script directly, which should be replaced with a pinned local
download flow. Update the install logic in install-homebrew.sh to fetch a
specific installer revision, verify its checksum before execution, and then run
the verified local copy instead of piping curl output to bash. Use the existing
Homebrew install block as the place to introduce the pinned versioning and
checksum validation.
In `@images/rhel/scripts/build/install-lxd.sh`:
- Around line 9-19: The LXD channel detection in install-lxd.sh is too narrow
and can fall through to an invalid snap install. Update the LATEST_LTS_CHANNEL
lookup in the install-lxd.sh logic to match the actual LTS track format used by
LXD (for example, 5.21/stable-style channels rather than only x.0/stable), and
if no channel is found, exit before reaching the snap install step. Keep the fix
centered around the LATEST_LTS_CHANNEL assignment and the subsequent install
block.
In `@images/rhel/scripts/build/install-zstd.sh`:
- Around line 25-35: The install-zstd.sh flow currently extracts and builds from
the predictable /tmp/${release_name} path, which can be pre-created and hijacked
before the binaries are copied into /usr/local/bin. Update the install steps
around the tar extraction and the make/cp commands to use a unique temporary
directory created at runtime, and reference that directory consistently for the
release_name-based build paths so the build inputs cannot be spoofed.
In `@images/rhel/scripts/helpers/etc-environment.sh`:
- Around line 95-102: The reload_etc_environment function is evaluating
/etc/environment as shell code via eval, which should be replaced with a safe
parser that reads key/value pairs directly and only exports valid assignments;
keep the logic in reload_etc_environment and get_etc_environment_variable, but
avoid executing file contents. Also update the PATH handling so it only appends
the /etc/environment PATH when present, preventing a trailing empty PATH
component when no PATH entry exists.
In `@images/rhel/scripts/helpers/install.sh`:
- Line 22: The download retry loop in install.sh can hang indefinitely because
curl has no timeout bounds. Update the curl invocation used for each download
attempt to enforce both connection and overall transfer timeouts, and keep the
retry behavior around the existing http_code capture logic so stalled
connections fail fast and the loop can continue.
- Around line 237-244: The checksum fallback in the install script ignores the
requested algorithm and always uses SHA256 when shasum is unavailable. Update
the fallback logic in the checksum verification block so it computes the hash
using the same sha_type requested by the caller, preserving SHA512 support on
minimal RHEL images. Keep the behavior consistent with the existing shasum path
and the variables file_path, checksum, and sha_type.
- Around line 153-167: The lookup logic in install.sh’s release-body and
checksum search path is failing too early under bash -e because the grep-based
searches in the matching_releases/matched_line flow abort before the not-found
and duplicate-match checks can run. Update the search steps to use non-fatal
matching so the later validation branches remain reachable, and make the content
lookups safer by using printf '%s\n' plus grep -F -- when matching file_name or
checksum text in the helper functions that build matched_line and hash.
In `@scripts/helpers/setup_install.sh`:
- Around line 32-44: Normalize AlmaLinux handling in the OS-family checks so
`setup_install.sh` follows the same CentOS/RHEL DNF bootstrap path when
`IMAGE_OS` is `almalinux`. Update the family detection used by
`run.sh`/`setup_vars.sh` and the `get_setup_type()` logic so AlmaLinux 9/10 maps
into the shared CentOS toolset/minimal flow, ensuring the existing
`configure-yum-mock.sh`, `configure-dnf.sh`, and related DNF setup steps still
run.
In `@scripts/vm.sh`:
- Around line 50-55: Only the user creation in the runner setup should remain
under the id -u runner existence check, while the hardening steps in
scripts/vm.sh must always run. Move the usermod -L runner and
/etc/sudoers.d/runner creation/chmod logic out of the conditional so reruns
through the runner provisioning path still lock the account and restore
passwordless sudo; use the existing runner setup block as the anchor.
---
Minor comments:
In `@images/rhel/scripts/build/configure-dnfpkg.sh`:
- Around line 16-25: The DNF config in configure-dnfpkg.sh uses the wrong
setting for suppressing config-file prompts and includes an invalid dnf.conf
option. Update the script so the logic around the dnf.conf writes uses the
correct key for noninteractive config file handling instead of
override_install_langs, and remove or replace autoclean_metadata with a valid
DNF setting. Keep the changes localized to the dnf.conf update block in
configure-dnfpkg.sh.
In `@images/rhel/scripts/build/configure-environment.sh`:
- Around line 20-21: The current use of set_etc_environment_variable is writing
a literal $HOME-based value into /etc/environment, which should not be persisted
there. Update configure-environment.sh so XDG_CONFIG_HOME is set from the runner
user’s profile or service environment instead of using
set_etc_environment_variable with '$HOME/.config', and ensure the configured
value is an absolute path at runtime.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b69553d-8001-46d5-b80e-00f2eadb74b2
📒 Files selected for processing (39)
images/rhel/assets/post-gen/cleanup-logs.shimages/rhel/assets/post-gen/environment-variables.shimages/rhel/assets/post-gen/systemd-linger.shimages/rhel/scripts/build/cleanup.shimages/rhel/scripts/build/configure-dnf.shimages/rhel/scripts/build/configure-dnfpkg.shimages/rhel/scripts/build/configure-environment.shimages/rhel/scripts/build/configure-image-data.shimages/rhel/scripts/build/configure-limits.shimages/rhel/scripts/build/configure-runner.shimages/rhel/scripts/build/configure-snap.shimages/rhel/scripts/build/configure-system.shimages/rhel/scripts/build/configure-yum-mock.shimages/rhel/scripts/build/install-actions-cache.shimages/rhel/scripts/build/install-dnf-common.shimages/rhel/scripts/build/install-dnf-vital.shimages/rhel/scripts/build/install-docker.shimages/rhel/scripts/build/install-dotnetcore-sdk.shimages/rhel/scripts/build/install-git-lfs.shimages/rhel/scripts/build/install-git.shimages/rhel/scripts/build/install-github-cli.shimages/rhel/scripts/build/install-homebrew.shimages/rhel/scripts/build/install-lxd.shimages/rhel/scripts/build/install-pipx-packages.shimages/rhel/scripts/build/install-podman.shimages/rhel/scripts/build/install-python.shimages/rhel/scripts/build/install-runner-package.shimages/rhel/scripts/build/install-snap.shimages/rhel/scripts/build/install-zstd.shimages/rhel/scripts/helpers/etc-environment.shimages/rhel/scripts/helpers/install.shimages/rhel/scripts/helpers/os.shimages/rhel/toolsets/toolset-10.jsonimages/rhel/toolsets/toolset-9.jsonrun.shscripts/helpers/run_script.shscripts/helpers/setup_install.shscripts/helpers/setup_vars.shscripts/vm.sh
| # Adjust permissions | ||
| echo "chmod -R 777 /opt" | ||
| chmod -R 777 /opt | ||
| echo "chmod -R 777 /usr/share" | ||
| chmod -R 777 /usr/share |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not make /opt and /usr/share world-writable.
chmod -R 777 here turns large system trees into writable locations for any local user, which enables tampering with scripts, package assets, and tool installations that later jobs or privileged processes may consume. Scope the permission changes to the specific runner-owned directories that actually need write access.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 18-18: Granting world-writable permissions (e.g. chmod 777, chmod -R 777, chmod a+w, chmod o+w) lets any local user modify the file or directory, enabling tampering and privilege escalation. Grant the least privilege needed instead, for example chmod 755 for executables/directories or chmod 644 for regular files, and use group ownership (chmod g+w with a dedicated group) when shared write access is required.
Context: chmod -R 777 /opt
Note: [CWE-732] Incorrect Permission Assignment for Critical Resource.
(chmod-world-writable-bash)
[warning] 20-20: Granting world-writable permissions (e.g. chmod 777, chmod -R 777, chmod a+w, chmod o+w) lets any local user modify the file or directory, enabling tampering and privilege escalation. Grant the least privilege needed instead, for example chmod 755 for executables/directories or chmod 644 for regular files, and use group ownership (chmod g+w with a dedicated group) when shared write access is required.
Context: chmod -R 777 /usr/share
Note: [CWE-732] Incorrect Permission Assignment for Critical Resource.
(chmod-world-writable-bash)
🤖 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 `@images/rhel/scripts/build/configure-system.sh` around lines 17 - 21, The
permission changes in configure-system.sh are too broad because chmod -R 777 is
making /opt and /usr/share world-writable. Update the script to scope write
access only to the specific runner-owned subdirectories that need it, and keep
the changes localized within the existing permission-adjustment block so the
rest of the system trees remain protected.
Source: Linters/SAST tools
| # shellcheck disable=SC2086 | ||
| dnf install -y -qq ${FLAGS} ${pkg} 2>&1 >/dev/null | tee -a /tmp/install.errors | ||
| done | ||
| } | ||
|
|
||
| update_dnfpkgs() | ||
| { | ||
| dnf -qq update -y 2>&1 >/dev/null | tee -a /tmp/install.errors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Do not mask DNF install/update failures behind tee.
With #!/bin/bash -e, these pipelines return tee’s status, so a failed dnf install or dnf update can continue the image build with missing packages.
Proposed fix
- # shellcheck disable=SC2086
- dnf install -y -qq ${FLAGS} ${pkg} 2>&1 >/dev/null | tee -a /tmp/install.errors
+ # shellcheck disable=SC2086
+ dnf install -y -qq ${FLAGS} "${pkg}" >/dev/null 2> >(tee -a /tmp/install.errors >&2)- dnf -qq update -y 2>&1 >/dev/null | tee -a /tmp/install.errors
+ dnf -qq update -y >/dev/null 2> >(tee -a /tmp/install.errors >&2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # shellcheck disable=SC2086 | |
| dnf install -y -qq ${FLAGS} ${pkg} 2>&1 >/dev/null | tee -a /tmp/install.errors | |
| done | |
| } | |
| update_dnfpkgs() | |
| { | |
| dnf -qq update -y 2>&1 >/dev/null | tee -a /tmp/install.errors | |
| # shellcheck disable=SC2086 | |
| dnf install -y -qq ${FLAGS} "${pkg}" >/dev/null 2> >(tee -a /tmp/install.errors >&2) | |
| done | |
| } | |
| update_dnfpkgs() | |
| { | |
| dnf -qq update -y >/dev/null 2> >(tee -a /tmp/install.errors >&2) |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 279-279: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/install.errors
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 285-285: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/install.errors
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🤖 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 `@images/rhel/scripts/helpers/install.sh` around lines 279 - 286, The DNF
install/update pipeline in install.sh is masking failures because the exit
status comes from tee instead of dnf, so a failed package operation can still
let the build continue. Update the install_dnfpkgs and update_dnfpkgs paths to
preserve the dnf command’s failure status while still logging output to
/tmp/install.errors, and use the existing dnf invocation points in these helper
functions to locate the change.
1 participant
Summary
Commits
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes