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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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
|
/ok-to-test |
|
@vinokurig: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/provision/workspace/ssh-askpass.sh (1)
3-4: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix the malformed
casepattern (script currently won’t parse).At Line 4, the pattern escaping is invalid, which breaks parsing under
/bin/shand prevents the helper from running.Minimal POSIX-safe fix
case "$1" in - \"Enter passphrase for key \'*) ;; + "Enter passphrase for key '"*) ;; *) exit 0 ;; esac🤖 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 - 4, The `case` pattern in the `ssh-askpass.sh` script is malformed and causes `/bin/sh` parsing to fail. Update the `case "$1" in` branch so the quoted passphrase prompt pattern is written in valid POSIX shell syntax, keeping the matching behavior intact. Use the `case` statement and its existing passphrase prompt branch as the locator for the fix.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@pkg/provision/workspace/ssh-askpass.sh`:
- Around line 3-4: The `case` pattern in the `ssh-askpass.sh` script is
malformed and causes `/bin/sh` parsing to fail. Update the `case "$1" in` branch
so the quoted passphrase prompt pattern is written in valid POSIX shell syntax,
keeping the matching behavior intact. Use the `case` statement and its existing
passphrase prompt branch as the locator for the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9ca1eb2-b38f-4d56-86b8-9bc08701eb65
📒 Files selected for processing (1)
pkg/provision/workspace/ssh-askpass.sh
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>
|
/ok-to-test |
|
@vinokurig: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions 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. |
|
/che-ai-assistant ok-pr-readiness Review is complete. Please check the review comments below. |
PR Readiness Assessment: PR #1658 — fix: handle non-passphrase SSH prompts in askpass scriptRepository: devfile/devworkspace-operator
Overall: READY Missing Information
What's Good
|
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