Override the default ssh askpass flow on project clone step #1291
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Signed-off-by: ivinokur <[email protected]>
|
/test v8-devworkspace-operator-e2e, v8-che-happy-path |
|
@vinokurig: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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. |
|
/ok-to-test |
|
Thanks for the PR @vinokurig :) I didn't get a chance to properly look at this today, but I should be able to tomorrow (Wednesday). |
| @@ -211,7 +212,8 @@ EOF | |||
| kubectl create secret -n "$NAMESPACE" generic git-ssh-key \ | |||
| --from-file=dwo_ssh_key="$SSH_KEY" \ | |||
| --from-file=dwo_ssh_key.pub="$SSH_PUB_KEY" \ | |||
| --from-file=ssh_config=/tmp/ssh_config | |||
| --from-file=ssh_config=/tmp/ssh_config \ | |||
| --from-literal=passphrase="$PASSPHRASE" | |||
There was a problem hiding this comment.
We need to verify that adding the --from-literal=passphrase="$PASSPHRASE" will not break things if the user does not provide a passphrase. Perhaps we should add an alternate instruction for users that are providing a passphrase?
There was a problem hiding this comment.
I have tried the command without the PASSPHRASE env variable and it worked ok, but the passphrase key of the secret data has no value:
data:
dwo_ssh_key: ***
dwo_ssh_key.pub: ***
passphrase: ''
ssh_config: ***Such empty keys will not be mounted to the container file system as files and if the ssh-askpass.sh is executed it will throw an error about missing passphrase file.
There was a problem hiding this comment.
Thanks for checking. Just to confirm: if the user's SSH key does not have a passphrase, will ssh-askpass.sh be invoked? I imagine it won't, based on it's documentation:
If ssh-add **needs** a passphrase, it will read the passphrase from the current terminal if it was run from a terminal...
The concern is just making sure that users are able to copy & paste the instructions and have them work, even if their SSH key does not have a passphrase (though their secret will have an empty passphrase data field).
There was a problem hiding this comment.
if the user's SSH key does not have a passphrase, will
ssh-askpass.shbe invoked?
I have tried the flow with SSH key with an empty passphrase, and neither the default askpass scriptn nore the provided ssh-askpass.sh script was not invoked.
Signed-off-by: ivinokur <[email protected]>
There was a problem hiding this comment.
LGTM, great work @vinokurig :)
I've opened #1294 for tracking in the DWO milestones.
Please squash your fixup commit before we merge this PR :)
|
@AObuchow Thank you for the review. Looks like we need @dmytro-ndp approval after he verifies the functionality. |
| PASSPHRASE_FILE_PATH="/etc/ssh/passphrase" | ||
| if [ ! -f $PASSPHRASE_FILE_PATH ]; then | ||
| echo "Error: passphrase file is missing in the '/etc/ssh/' directory" 1>&2 | ||
| exit 1 |
There was a problem hiding this comment.
@vinokurig could you elaborate a bit? why do we exit 1 if the optional passphrase is missing?
There was a problem hiding this comment.
Passphrase is optional because we do not need to specify passphrase if the ssh key is generated with an empty passphrase, but if the ssh key is generated WITH a passphrase we do not consider the passphrase as optional.
There was a problem hiding this comment.
@vinokurig have we checked that old ssh keys would be working as expected
|
@musienko-maxim please review, we need QA approval for that one |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AObuchow, dkwon17, musienko-maxim, vinokurig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?
We support project clone from ssh urls, but if the ssh key has a passphrase, the project clone step will fail because the default ssh askpass flow will be invoked, but it does not support any other input then a manual input from keyboard. To override the default flow, a script file is used. The passphrase file must be mounted to
/etc/ssh/folder. TheSSH_ASKPASSenv variables defines the script file for the ssh flow, see: https://man.openbsd.org/ssh-add#ENVIRONMENT. The currentopen-sshversion does not support theSSH_ASKPASS_REQUIREkey, so we need to set theDISPLAYenv variable as the command process does not have an associated terminal.What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-6614
Fix #1294
Is it tested? How?
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 Che