Update repository workflow with hardening and security improvements#130
Update repository workflow with hardening and security improvements#130igorpecovnik merged 1 commit intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe workflow Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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 |
igorpecovnik
force-pushed
the
repohandling
branch
from
7cc4cb2 to
2f06441
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/infrastructure-repository-update.yml (7)
163-174: Redundant directory check.Line 163 already confirms
$BETA_REPO_PATHis a directory with-d, so the symlink/directory check on line 167 is partially redundant. However, the symlink check adds value since-dreturns true for symlinks to directories.The comment about the trailing slash "preventing accidental matches" is slightly misleading—the slash ensures the path is treated as a directory but doesn't prevent pattern matches. Consider updating the comment for accuracy.
207-215: Duplicate step names reduce log clarity.Both steps are named "Import GPG key" but import different keys. Consider distinct names for easier debugging.
🔎 Proposed fix
- - name: Import GPG key + - name: Import GPG key (primary) uses: crazy-max/ghaction-import-gpg@v6 with: gpg_private_key: ${{ secrets.GPG_KEY3 }} - - name: Import GPG key + - name: Import GPG key (secondary) uses: crazy-max/ghaction-import-gpg@v6 with: gpg_private_key: ${{ secrets.GPG_KEY4 }}
599-608: Redundant empty string checks.The
-ztest already checks for empty strings, making the|| "..." == ""condition redundant.🔎 Simplified validation
- if [[ -z "${{ secrets.NETBOX_API }}" || "${{ secrets.NETBOX_API }}" == "" ]]; then + if [[ -z "${{ secrets.NETBOX_API }}" ]]; then echo "::error::NETBOX_API secret is not set or is empty" exit 1 fi - if [[ -z "${{ secrets.NETBOX_TOKEN }}" || "${{ secrets.NETBOX_TOKEN }}" == "" ]]; then + if [[ -z "${{ secrets.NETBOX_TOKEN }}" ]]; then echo "::error::NETBOX_TOKEN secret is not set or is empty" exit 1 fi
808-834: Word splitting in array assignment is fragile.The pattern
TARGETS=($(echo ... | jq ...))relies on word splitting, which breaks if tag names contain spaces or special characters. While the current tags (debs,debs-beta) are safe, this pattern is brittle.🔎 Proposed fix using mapfile
- # Extract targets - TARGETS=($(echo "$response" | jq -r '.results[] | .tags[] | .name' 2>/dev/null | grep -v "Push" || echo "")) + # Extract targets safely using mapfile + mapfile -t TARGETS < <(echo "$response" | jq -r '.results[] | .tags[] | .name' 2>/dev/null | grep -v "Push" || true)
866-898: Rsync command duplication risks log/execution mismatch.The
RSYNC_CMDstring logged doesn't execute; the actual rsync runs separately with duplicated arguments. If one is updated without the other, logs become misleading.Consider constructing arguments in an array to avoid duplication:
🔎 Proposed approach using array
RSYNC_ARGS=( $RSYNC_OPTIONS -e "ssh -p ${SERVER_PORT} -o StrictHostKeyChecking=accept-new -o ConnectTimeout=30" --exclude "dists" --exclude "control" "$REPO_PATH/public/" "${SERVER_USERNAME}@${HOSTNAME}:${SERVER_PATH}/apt" ) echo "Command: rsync ${RSYNC_ARGS[*]}" | tee -a "$GITHUB_STEP_SUMMARY" rsync "${RSYNC_ARGS[@]}"
1073-1103: Index job silently skipsdebstarget with a warning.The loop iterates over all
TARGETS(includingdebs), but the case statement only handlesdebs-beta, causingdebsto fall through to the "Unknown target" warning. If this is intentional (only beta needs finalization), consider filtering before the loop or adding a no-op case fordebs:🔎 Proposed fix
for target in "${TARGETS[@]}"; do echo "→ Finalizing $target" | tee -a "$GITHUB_STEP_SUMMARY" case "$target" in + debs) + # Main repository doesn't need finalization step + echo "Skipping $target (no finalization needed)" | tee -a "$GITHUB_STEP_SUMMARY" + ;; debs-beta) REPO_PATH="${PUBLISHING_PATH}-debs-beta" # ... existing logic ... ;; *) echo "::warning::Unknown target: $target" ;; esac done
860-864: Host key removal runs inside target loop.
ssh-keygen -Ris executed for each target, but only needs to run once per hostname. Consider moving it before the loop:🔎 Proposed fix
+ # Remove old host key once before syncing all targets + ssh-keygen -f "${HOME}/.ssh/known_hosts" -R "$HOSTNAME" 2>/dev/null || true + # Sync to each target for target in "${TARGETS[@]}"; do echo "→ Syncing $target" | tee -a "$GITHUB_STEP_SUMMARY" - - # Remove old host key - ssh-keygen -f "${HOME}/.ssh/known_hosts" -R "$HOSTNAME" 2>/dev/null || true
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/infrastructure-repository-update.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T22:48:41.167Z
Learnt from: igorpecovnik
Repo: armbian/armbian.github.io PR: 85
File: .github/workflows/generate-keyring-data.yaml:75-85
Timestamp: 2025-11-04T22:48:41.167Z
Learning: In `.github/workflows/generate-keyring-data.yaml` for the Armbian project: `ubuntu-keyring-ports` is intentionally optional because they're not certain it's needed, while `debian-ports-archive-keyring` must be mandatory because it's always available in Debian sid and is required for the project.
Applied to files:
.github/workflows/infrastructure-repository-update.yml
🪛 actionlint (1.7.9)
.github/workflows/infrastructure-repository-update.yml
56-56: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
134-134: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
195-195: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
292-292: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
356-356: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
502-502: label "repository" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (5)
.github/workflows/infrastructure-repository-update.yml (5)
62-127: Solid permission handling implementation.Good use of
-print0withxargs -0for safe filename handling, defensive checks for setfacl availability, and informative sticky-bit detection. The path validation guards against accidental operations.
305-345: Well-structured matrix generation.Good validation of the build config directory existence and handling of empty releases array. The JSON construction using
jq -sis appropriate.
1084-1097: Double rsync may be redundant.The first rsync (line 1088-1090) transfers files, then the second rsync (line 1095-1097) does the same transfer plus
--delete. The second command alone would accomplish both tasks. Is the first sync intentional (e.g., ensuring files exist before deletion pass)?
610-615: Comprehensive security hardening.Excellent SSRF prevention with URL scheme validation, hostname format validation with regex, path traversal checks, port range validation, and username format validation. The timeout and error handling for curl calls is well implemented.
Also applies to: 728-733, 790-800
509-581: Well-implemented cleanup with safety guards.Good validation of the cleanup flag, path safety checks, use of
-print0/xargs -0for safe filename handling, and verification that deletion succeeded. The additional check for files vs directories adds defense in depth.
igorpecovnik
force-pushed
the
repohandling
branch
from
2f06441 to
241ca97
Compare
- Add comprehensive input validation for all paths and parameters - Implement SSRF prevention for API calls - Add format validation for hostnames, usernames, and ports - Enhance NetBox API error handling and validation - Improve permission management with safety checks - Add dry-run support for sync operations - Strengthen repository path validation throughout - Enhance security for beta repository operations Signed-off-by: Igor Pecovnik <igor@armbian.com>
igorpecovnik
force-pushed
the
repohandling
branch
from
241ca97 to
b865c96
Compare
1 participant
Summary
Changes
This PR consolidates security improvements to the repository update workflow, including:
Test plan