fix: handle non-passphrase SSH prompts in askpass script#1658
Conversation
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>
vinokurig
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
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. |
📝 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
| if [[ "$1" != "Enter passphrase for key '"* ]]; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the base image / interpreter used for the project-clone container that runs this script
rg -nP -C3 '(project-clone|projectClone|PROJECT_CLONE|/bin/sh|dash|busybox)' --type=go --type=docker -g '!**/vendor/**'
fd -i -e dockerfile
fd -t f 'Dockerfile*' --exec cat -n {}Repository: devfile/devworkspace-operator
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ssh-askpass.sh =="
git ls-files pkg/provision/workspace/ssh-askpass.sh
cat -n pkg/provision/workspace/ssh-askpass.sh
echo
echo "== search for where script is embedded/invoked =="
rg -n --hidden --no-ignore-vcs -C 3 'ssh-askpass\.sh|SSH_ASKPASS|askpass' pkg .github .devcontainer .docker . | sed -n '1,220p'
echo
echo "== locate container/Dockerfile references =="
fd -a -i -e dockerfile -e Dockerfile -t f .
fd -a -t f 'Dockerfile*' .
echo
echo "== search for base image / shell in workspace provisioning =="
rg -n --hidden --no-ignore-vcs -C 3 '(project-clone|projectClone|PROJECT_CLONE|/bin/sh|dash|busybox|alpine|debian|ubuntu)' pkg .github .devcontainer .docker . | sed -n '1,240p'Repository: devfile/devworkspace-operator
Length of output: 6846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ssh-askpass.sh =="
git ls-files pkg/provision/workspace/ssh-askpass.sh
cat -n pkg/provision/workspace/ssh-askpass.sh
echo
echo "== search for where script is embedded/invoked =="
rg -n --hidden --no-ignore-vcs -C 3 'ssh-askpass\.sh|SSH_ASKPASS|askpass' pkg .github .devcontainer .docker . | sed -n '1,220p'
echo
echo "== locate container/Dockerfile references =="
fd -a -i -e dockerfile -e Dockerfile -t f .
fd -a -t f 'Dockerfile*' .
echo
echo "== search for base image / shell in workspace provisioning =="
rg -n --hidden --no-ignore-vcs -C 3 '(project-clone|projectClone|PROJECT_CLONE|/bin/sh|dash|busybox|alpine|debian|ubuntu)' pkg .github .devcontainer .docker . | sed -n '1,240p'Repository: devfile/devworkspace-operator
Length of output: 6846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file contents =="
cat -n pkg/provision/workspace/ssh-askpass.sh
echo
echo "== find workspace/container definition files =="
find . -path '*/.git' -prune -o \( -name 'Dockerfile*' -o -name '*.dockerfile' -o -name 'devfile.yaml' -o -name 'devfile.yml' -o -name '.devfile.yaml' -o -name '.devfile.yml' -o -name '*.yaml' -o -name '*.yml' \) -print | sed -n '1,200p'
echo
echo "== grep likely relevant files =="
rg -n --hidden --no-ignore-vcs -C 2 'ssh-askpass\.sh|SSH_ASKPASS|SSH_ASKPASS_REQUIRE|/bin/sh|busybox|alpine|dash' . --glob '!**/vendor/**' --glob '!**/node_modules/**' | sed -n '1,260p'Repository: devfile/devworkspace-operator
Length of output: 33500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== project-clone/Dockerfile =="
cat -n project-clone/Dockerfile | sed -n '1,120p'
echo
echo "== workspace-related image references =="
rg -n --hidden --no-ignore-vcs -C 2 '(project-clone|workspace image|workspaceImage|image:|container image|base image|ssh-askpass)' .devfile.yaml deploy pkg controllers build project-clone --glob '!**/vendor/**' | sed -n '1,260p'Repository: devfile/devworkspace-operator
Length of output: 24263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== project-clone/Dockerfile =="
cat -n project-clone/Dockerfile | sed -n '1,120p'
echo
echo "== workspace-related image references =="
rg -n --hidden --no-ignore-vcs -C 2 '(project-clone|workspace image|workspaceImage|image:|container image|base image|ssh-askpass)' .devfile.yaml deploy pkg controllers build project-clone --glob '!**/vendor/**' | sed -n '1,260p'Repository: devfile/devworkspace-operator
Length of output: 24163
Use POSIX syntax in the askpass guard. #!/bin/sh cannot rely on [[ ... ]]; switch this to case so non-passphrase prompts still exit cleanly.
Suggested replacement
- if [[ "$1" != "Enter passphrase for key '"* ]]; then
- exit 0
- fi
+ case "$1" in
+ "Enter passphrase for key '"*) ;;
+ *) exit 0 ;;
+ esac📝 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.
| if [[ "$1" != "Enter passphrase for key '"* ]]; then | |
| exit 0 | |
| fi | |
| case "$1" in | |
| "Enter passphrase for key '"*) ;; | |
| *) exit 0 ;; | |
| esac |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
🤖 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 - 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.
Source: Linters/SAST tools
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
| @@ -1,4 +1,8 @@ | |||
| #!/bin/sh | |||
| # Only handle passphrase prompts - exit silently for other prompts (e.g., HTTPS git repo username/password request) | |||
| if [[ "$1" != "Enter passphrase for key '"* ]]; then | |||
There was a problem hiding this comment.
Critical: Replace bash-specific syntax with POSIX-compatible case statement
The script uses #!/bin/sh but contains [[ ]] 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.
Replace with POSIX case statement:
| if [[ "$1" != "Enter passphrase for key '"* ]]; then | |
| case "$1" in | |
| "Enter passphrase for key ''*) ;; | |
| *) exit 0 ;; | |
| esac |
This was also flagged by CodeRabbit/ShellCheck SC3010.
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