fix: handle non-passphrase SSH prompts in askpass script#1658
Conversation
vinokurig
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
Hi @vinokurig. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changesssh-askpass prompt filtering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 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)
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 |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
![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 `@pkg/provision/workspace/ssh-askpass.sh`:
- Around line 3-5: The askpass guard in ssh-askpass.sh is using a bash-only
conditional even though the script is declared with a POSIX shell shebang.
Update the prompt check in the top-level guard to use POSIX-compatible syntax
such as a case statement, and keep the behavior in the script’s early-exit path
the same so only prompts matching the passphrase prefix continue while all other
prompts exit cleanly.
🪄 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
Run ID: 340019fc-3154-459d-b6e7-ea31c75e11ea
📒 Files selected for processing (1)
pkg/provision/workspace/ssh-askpass.sh
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Comprehensive Review Summary
I've completed a multi-stage review (summary, standard, deep, and impact analysis) of this PR. Here's the consolidated feedback:
Critical Issue - Must Fix Before Merge
POSIX Shell Compatibility (line 3): The script declares #!/bin/sh but uses [[ ]] syntax, which is bash-specific. On workspace containers using Alpine, BusyBox, or other images where /bin/sh is dash/ash, this will produce a syntax error and break all SSH operations.
Fix: Replace with POSIX case statement:
case "$1" in
"Enter passphrase for key '"*) ;;
*) exit 0 ;;
esacThis issue was also flagged by CodeRabbit/ShellCheck SC3010.
Suggestions (Non-Blocking)
- Add test coverage: Consider adding a Go test to validate the embedded script is valid POSIX shell (
sh -nsyntax check) - Observability: The new guard exits silently for non-passphrase prompts. Consider adding optional debug logging (e.g.,
echo "DEBUG: ignoring non-passphrase prompt: $1" >&2behind an env var) to aid production debugging - Shell best practices: While not introduced by this PR, consider quoting
$PASSPHRASE_FILE_PATHin the existing file check to follow POSIX best practices
Does This PR Solve the Issue?
✅ Yes - This correctly fixes eclipse-che/che#23874 by preventing the askpass script from responding to HTTPS credential prompts with misleading SSH passphrase errors.
Positive Feedback
- ✅ Minimal, well-scoped fix
- ✅ Clear comment explaining the intent
- ✅ PR description includes reproduction steps and links to upstream issue
- ✅ Pattern match is specific enough to avoid false positives
Review Verdict
🔄 Request Changes - The POSIX shell compatibility issue must be fixed (straightforward case statement replacement). Everything else looks good.
Review generated by Claude Code with ok-pr-review skills: /ok-pr-review:learn-repo, /ok-pr-review:summary, /ok-pr-review:review, /ok-pr-review:deep-review, /ok-pr-review:impact
|
I tested the PR with abovementioned steps and can confirm it's working as expected: project-clone logs with fix (askpass no longer interferes): project-clone logs without fix (HTTPS clone triggers askpass errors): |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, vinokurig The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
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 `@pkg/provision/workspace/ssh-askpass.sh`:
- Around line 3-8: The askpass guard in ssh-askpass.sh has broken shell syntax,
so fix the malformed case pattern in the prompt check and remove the stray
unmatched fi after esac. Update the case block around the existing
prompt-matching logic so it parses cleanly under /bin/sh, and keep the fallback
branch in the case statement exiting 0 for non-passphrase prompts.
🪄 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
Run ID: 5b18987d-345e-4d4c-9f2a-fd0faed12bb1
📒 Files selected for processing (1)
pkg/provision/workspace/ssh-askpass.sh
| case "$1" in | ||
| "Enter passphrase for key ''*) ;; | ||
| *) exit 0 ;; | ||
| esac | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Syntax is broken in the prompt guard and prevents the script from running.
At Line 4, the case item pattern is malformed, and at Line 8 there is an unmatched fi after esac. This makes the script fail to parse under /bin/sh, so the askpass helper won’t work at all. Please correct the case pattern and remove the stale fi/extra branch residue so non-passphrase prompts still exit cleanly.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 3-3: The mentioned syntax error was in this case expression.
(SC1009)
[error] 4-4: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
🤖 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 `@pkg/provision/workspace/ssh-askpass.sh` around lines 3 - 8, The askpass guard
in ssh-askpass.sh has broken shell syntax, so fix the malformed case pattern in
the prompt check and remove the stray unmatched fi after esac. Update the case
block around the existing prompt-matching logic so it parses cleanly under
/bin/sh, and keep the fallback branch in the case statement exiting 0 for
non-passphrase prompts.
Source: Linters/SAST tools
Add early exit for SSH prompts that aren't requesting passphrase input. This prevents the askpass script from attempting to provide a passphrase for other SSH prompts like host key verification. Assisted-by: Claude Sonnet 4.5 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Ihor Vinokur <ivinokur@redhat.com>
3 participants
What does this PR do?
Add early exit for SSH prompts that aren't requesting passphrase input. This prevents the askpass script from attempting to provide a passphrase for other SSH prompts like host key verification.
What issues does this PR fix or reference?
fixes eclipse-che/che#23874
Is it tested? How?
project-clonecontainer logs and see no error messages.PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit