[Fixes: mosip/mosip-infra#1874] Update SSH key secret and token refer…#251
[Fixes: mosip/mosip-infra#1874] Update SSH key secret and token refer…#251Ivanmeneges wants to merge 7 commits into
Conversation
…ences Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
|
Warning Review limit reached
More reviews will be available in 5 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR updates the WireGuard onboarding workflow and script to improve credential handling, execution isolation, and peer allocation robustness. The workflow renames the SSH key dispatch input from ChangesWireGuard Onboarding Credential Updates and Allocation Robustness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/wg-onboard.yml:
- Around line 58-59: The workflow is using dynamic secret resolution where the
secret name is controlled by user input (inputs.MOSIP_AWS_PEM), which allows
dispatch inputs to select arbitrary repository secrets and violates
least-privilege principles. Replace the dynamic secret reference in SSH_KEY with
a fixed, hardcoded secret name instead of ${{ secrets[inputs.MOSIP_AWS_PEM] }},
and update SSH_KEY_NAME to use the same fixed secret name instead of the user
input, ensuring only predetermined secrets can be accessed regardless of
dispatch parameters.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 038f1b16-66e6-48fb-a9bb-0f6aeb370d30
📒 Files selected for processing (1)
.github/workflows/wg-onboard.yml
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent generated fixes only for
Lines 55–62
- name: Write SSH private key
env:
- SSH_KEY: ${{ secrets[inputs.MOSIP_AWS_PEM] }}
- SSH_KEY_NAME: ${{ inputs.MOSIP_AWS_PEM }}
+ SSH_KEY: ${{ secrets.MOSIP_AWS_PEM }}
+ SSH_KEY_NAME: MOSIP_AWS_PEM
run: |
mkdir -p ~/.ssh
# Use printf + strip CR so multi-line PEMs / CRLF secrets are written intact |
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/wg-onboard.yml:
- Around line 60-61: The workflow now references the secrets.MOSIP_AWS_PEM
secret for the SSH key, but this secret must be configured in the repository for
the workflow to function. Navigate to the repository settings under Secrets and
variables, create a new repository secret named MOSIP_AWS_PEM, and paste a valid
SSH private key into it. This secret will be used by the SSH_KEY environment
variable during the workflow execution, and without it, the validation step
(lines 67-70) where ssh-keygen verifies the key format will fail.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59a6248e-ed70-4279-aea5-58daa3873db6
📒 Files selected for processing (1)
.github/workflows/wg-onboard.yml
Updated the WireGuard onboarding workflow to reflect changes in the required script and removed the MOSIP_AWS_PEM input option. Adjusted the handling of the SSH private key and improved error handling for git operations. Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
Refactor argument handling and improve temporary file management in wg-onboard.sh. Enhance error handling and ensure proper cleanup of temporary files. Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/wg-onboard.sh (1)
312-363:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle a missing assignment file consistently.
The preflight allows
assigned.txtto be absent when its directory is writable, butrecord_assignment()still runsawk ... "$file"; underset -e, the first write exits before creating any assignment. Use/dev/nullas the input when the file does not exist.🐛 Suggested fix
- local dir tmp + local dir tmp input dir="$(dirname "$file")" + input="$file" + [[ -e "$input" ]] || input=/dev/null tmp="$(mktemp "$dir/.assigned.XXXXXX")" || { echo "ERROR: cannot create temp file in $dir" >&2 return 1 } @@ - ' "$file" > "$tmp" + ' "$input" > "$tmp" @@ - ' "$file" > "$tmp" + ' "$input" > "$tmp"🤖 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/scripts/wg-onboard.sh around lines 312 - 363, In the record_assignment() function, both awk command invocations (one in the if block for "colon" format and one in the else block) attempt to read from "$file" which may not exist. When the file doesn't exist and set -e is enabled, the script exits before creating the assignment. Modify both awk invocations to use /dev/null as the input file when "$file" does not exist, by checking if the file is present first and passing either "$file" or "/dev/null" to awk accordingly, so the function can create new assignments even when the file doesn't initially exist.
🧹 Nitpick comments (1)
.github/workflows/wg-onboard.yml (1)
53-57: ⚡ Quick winPass secrets through
envbefore checking them.Lines 56-57 expand secrets directly into the generated shell script; for the multiline private key, that makes secret contents part of Bash syntax before execution. Use step environment variables and test those instead.
🔐 Suggested refactor
- name: Validate required secrets + env: + ACTION_PAT: ${{ secrets.ACTION_PAT }} + MOSIP_AWS_PEM: ${{ secrets.MOSIP_AWS_PEM }} run: | missing=() - [[ -z "${{ secrets.ACTION_PAT }}" ]] && missing+=(ACTION_PAT) - [[ -z "${{ secrets.MOSIP_AWS_PEM }}" ]] && missing+=(MOSIP_AWS_PEM) + [[ -z "${ACTION_PAT:-}" ]] && missing+=(ACTION_PAT) + [[ -z "${MOSIP_AWS_PEM:-}" ]] && missing+=(MOSIP_AWS_PEM)🤖 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/wg-onboard.yml around lines 53 - 57, The "Validate required secrets" step is directly expanding secrets into the shell script, which exposes their contents in the generated Bash syntax. Instead, use the step's env property to pass the secrets as environment variables, then test those environment variables in the shell script. Specifically, create an env section that maps ACTION_PAT and MOSIP_AWS_PEM secrets to environment variables, then modify the conditional checks to test the environment variables (like checking [[ -z "$ACTION_PAT" ]]) rather than directly expanding the secrets with the ${{ secrets.* }} syntax.
🤖 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 @.github/scripts/wg-onboard.sh:
- Around line 619-627: The `remove_peer_line` function calls
`atomic_replace_file` at line 627, but this function is only defined in the
`REMOTE_ATOMIC_ALLOCATE` here-doc and is not available in the rollback here-doc
which starts a fresh remote shell. Include the definition of the
`atomic_replace_file` function inside the rollback here-doc section so that when
`remove_peer_line` is executed as part of the rollback process, the
`atomic_replace_file` function is already defined and available in that shell
context.
- Around line 224-235: Create a new helper function (e.g., ssh_bash) that wraps
ssh_cmd bash -s and applies remote_quote to each argument before passing them to
the remote shell. This function should take multiple arguments and quote each
one using the existing remote_quote function already used elsewhere in the
codebase. Then replace all three occurrences of the ssh_cmd bash -s -- pattern
(at the locations where variables like ASSIGNED_FILE, CONFIG_DIR, ENV_NAME,
LABEL, and other parameters are passed) with calls to this new helper function,
ensuring all variables are properly quoted to prevent command injection on the
jumpserver.
In @.github/workflows/wg-onboard.yml:
- Around line 64-68: The checkout action disables persist-credentials which
causes authentication issues for private repositories when the workflow later
attempts to pull from the unauthenticated origin remote at line 126 while using
GH_TOKEN only for the push at line 132. Either enable persist-credentials in the
checkout action with uses actions/checkout, or configure an authenticated remote
URL using git remote set-url before the rebase pull operation to ensure both the
pull and push operations at lines 126 and 132 consistently use the authenticated
remote with GH_TOKEN credentials.
- Line 50: The `runs-on: self-hosted` configuration on this job is too
permissive and poses a security risk since the job uses sensitive secrets
(ACTION_PAT and MOSIP_AWS_PEM). Replace the generic `runs-on: self-hosted` with
a dedicated label that restricts execution to a specific trusted Linux runner
owned only by the WireGuard onboarding process. Verify that the intended
self-hosted runner is properly configured with this dedicated label and has the
necessary tools (bash, ssh, git) before merging.
---
Outside diff comments:
In @.github/scripts/wg-onboard.sh:
- Around line 312-363: In the record_assignment() function, both awk command
invocations (one in the if block for "colon" format and one in the else block)
attempt to read from "$file" which may not exist. When the file doesn't exist
and set -e is enabled, the script exits before creating the assignment. Modify
both awk invocations to use /dev/null as the input file when "$file" does not
exist, by checking if the file is present first and passing either "$file" or
"/dev/null" to awk accordingly, so the function can create new assignments even
when the file doesn't initially exist.
---
Nitpick comments:
In @.github/workflows/wg-onboard.yml:
- Around line 53-57: The "Validate required secrets" step is directly expanding
secrets into the shell script, which exposes their contents in the generated
Bash syntax. Instead, use the step's env property to pass the secrets as
environment variables, then test those environment variables in the shell
script. Specifically, create an env section that maps ACTION_PAT and
MOSIP_AWS_PEM secrets to environment variables, then modify the conditional
checks to test the environment variables (like checking [[ -z "$ACTION_PAT" ]])
rather than directly expanding the secrets with the ${{ secrets.* }} syntax.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89d91e8d-67bf-4442-8107-a5e2e63b8e69
📒 Files selected for processing (2)
.github/scripts/wg-onboard.sh.github/workflows/wg-onboard.yml
| jobs: | ||
| onboard: | ||
| runs-on: ubuntu-latest | ||
| runs-on: self-hosted |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List self-hosted runners and their labels for this repository.
# Expect: exactly the intended trusted runner has the chosen WireGuard onboarding label.
gh api repos/:owner/:repo/actions/runners \
--jq '.runners[] | "\(.name)\t\([.labels[].name] | join(","))"'Repository: mosip/infra
Length of output: 315
🏁 Script executed:
# First, let's examine the complete workflow file to understand the job context
cat -n .github/workflows/wg-onboard.ymlRepository: mosip/infra
Length of output: 5973
🏁 Script executed:
# Search for any documentation about runners or the wg-onboard runner
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.yml" -o -name "*.yaml" \) | xargs grep -l "wg-onboard\|self-hosted.*runner" 2>/dev/null | head -20Repository: mosip/infra
Length of output: 90
🏁 Script executed:
# Look for other workflow files to see if any use similar runner pinning patterns
fd -e yml -e yaml ".github/workflows" --exec grep -l "runs-on:" {} \; | xargs grep -A 1 "runs-on:" | head -50Repository: mosip/infra
Length of output: 429
🏁 Script executed:
fd . '.github/workflows' --type f -e yml -e yamlRepository: mosip/infra
Length of output: 612
🏁 Script executed:
# Check other workflows to see how they use runs-on
fd . '.github/workflows' --type f | xargs grep "runs-on:" 2>/dev/null | head -20Repository: mosip/infra
Length of output: 1494
🏁 Script executed:
# Search for any documentation about runner setup, labels, or infrastructure
rg "runner|self-hosted|label" --type md README.md CONTRIBUTING.md 2>/dev/null || echo "No matches in doc files"Repository: mosip/infra
Length of output: 79
🏁 Script executed:
# Check if there's any infrastructure-as-code or runner config files
find . -maxdepth 3 -type f \( -name "*runner*" -o -name "*infrastructure*" -o -name "*host*" \) 2>/dev/nullRepository: mosip/infra
Length of output: 96
🏁 Script executed:
# Check the wg-onboard.sh script to confirm Linux requirements
head -50 .github/scripts/wg-onboard.shRepository: mosip/infra
Length of output: 2034
🏁 Script executed:
# Search for any infrastructure setup or runner documentation in repo
find . -maxdepth 4 -type f \( -name "*.md" -o -name "*.rst" -o -name "*.txt" \) | xargs grep -l "self-hosted\|runner.*setup\|infrastructure" 2>/dev/null | head -10Repository: mosip/infra
Length of output: 360
🏁 Script executed:
# Check the workflow-specific README and scripts README
cat -n ./.github/workflows/README.mdRepository: mosip/infra
Length of output: 26661
🏁 Script executed:
# Also check the scripts README
cat -n ./.github/scripts/README.mdRepository: mosip/infra
Length of output: 9420
Pin this job to a dedicated trusted Linux runner.
runs-on: self-hosted can schedule this secret-bearing job (using ACTION_PAT and MOSIP_AWS_PEM secrets) on any repo/org self-hosted runner, including one without bash/ssh/git tooling this workflow requires. Use a dedicated label owned only by the WireGuard onboarding runner.
🔐 Suggested hardening
- runs-on: self-hosted
+ runs-on: [self-hosted, linux, wg-onboard]Verify the actual label name before merging—ensure the intended trusted runner has the chosen WireGuard onboarding label by checking the repo's configured self-hosted runners.
🤖 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/wg-onboard.yml at line 50, The `runs-on: self-hosted`
configuration on this job is too permissive and poses a security risk since the
job uses sensitive secrets (ACTION_PAT and MOSIP_AWS_PEM). Replace the generic
`runs-on: self-hosted` with a dedicated label that restricts execution to a
specific trusted Linux runner owned only by the WireGuard onboarding process.
Verify that the intended self-hosted runner is properly configured with this
dedicated label and has the necessary tools (bash, ssh, git) before merging.
Define atomic_replace_file in rollback heredoc, use tokenized remote for git pull/push, and pin job to [self-hosted, Linux, X64]. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
remote_quote applies to inline remote shell strings only; bash -s args are already passed as separate SSH argv tokens and must not be shell-quoted. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
…ences
Summary by CodeRabbit