Add conditions to the SSH agent post-start command #1338

vinokurig · 2024-11-01T07:15:53Z

What does this PR do?

Add extra conditions to the SSH agent post-start command:

Check the $HOME directory for the write permissions in order to create the ssh-environment file in the user home directory.
Add timeout to the ssh-add <key path> < <passphrase path> command. If the passphrase is incorrect, then the ssh-add asks to input a new password:

Enter passphrase for <key path>: 
Bad passphrase, try again for <key path>:

The command will stuck to wait the user input, but the timeout will interrupt the command.

What issues does this PR fix or reference?

fixes #1337 eclipse-che/che#23213

Is it tested? How?

see #1337

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

openshift-ci · 2024-11-01T07:16:03Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

AObuchow · 2024-11-01T18:35:32Z

@vinokurig Just to clarify: the intention of adding the timeout to the ssh-add is prevent the workspace from failing in the case where the user has provided an incorrect SSH passphrase?

AObuchow

@vinokurig Thank you for your quick PR in fixing this issue. I still have to do further testing, but here are my current thoughts & suggestions:

Maybe we should improve the formatting of the commands for clarity? Here's an idea of how this could look:

SSH_ENV_PATH=$HOME/ssh-environment && \
if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null; then
    ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
    && chmod 600 $SSH_ENV_PATH \
    && source $SSH_ENV_PATH \
    && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase && [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then
        echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc
    fi
fi

We could also add some else statements to clarify which steps might have failed:

SSH_ENV_PATH=$HOME/ssh-environment && \
if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null; then
    ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
    && chmod 600 $SSH_ENV_PATH \
    && source $SSH_ENV_PATH \
    && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase; then
        if [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then
            echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc
        else
            echo "$HOME/.bashrc does not exist or is not writable."
        fi
    else
        echo "Provided SSH passphrase is invalid"
    fi
else
    if [ ! -f /etc/ssh/passphrase ]; then
        echo "/etc/ssh/passphrase file does not exist."
    fi
    if [ ! -w $HOME ]; then
        echo "$HOME directory is not writable."
    fi
    if ! command -v ssh-add >/dev/null; then
        echo "ssh-add command not found."
    fi
fi

We could wrap the entire command with a (... || true) as an added safety mechanism for commands that are unlikely to fail, but still could theoretically fail. For example, if the ssh-agent command is missing for some reason:

(
SSH_ENV_PATH=$HOME/ssh-environment && \
if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null; then
    ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
    && chmod 600 $SSH_ENV_PATH \
    && source $SSH_ENV_PATH \
    && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase; then
        if [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then
            echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc
        else
            echo "$HOME/.bashrc does not exist or is not writable."
        fi
    else
        echo "Provided SSH passphrase is invalid"
    fi
else
    if [ ! -f /etc/ssh/passphrase ]; then
        echo "/etc/ssh/passphrase file does not exist."
    fi
    if [ ! -w $HOME ]; then
        echo "$HOME directory is not writable."
    fi
    if ! command -v ssh-add >/dev/null; then
        echo "ssh-add command not found."
    fi
fi
) || true

I realize with all these suggestions applied, the final command is extremely verbose (maybe overkill). A nice middle ground might be to apply some formatting and wrap the entire command in (... || true) to ensure this postStart will never fail a workspace that uses a container image with sh installed. For example:

(
SSH_ENV_PATH=$HOME/ssh-environment && \
if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null; then
    ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
    && chmod 600 $SSH_ENV_PATH \
    && source $SSH_ENV_PATH \
    && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase && [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then
        echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc
    fi
fi
) || true

When wrapping the entire command with (... || true), any uncaught errors that occur will be outputted to /tmp/poststart-stdout.txt.

AObuchow · 2024-11-01T19:42:19Z

@vinokurig I tested out your PR and it works as expected :)
I set up an SSH key with a passphrase, and got the following (previously failing) devworkspace to start up:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-go-toolset
spec:
  started: true
  routingClass: 'basic'
  template:
    components:
      - name: web-terminal
        container:
          image: registry.access.redhat.com/ubi9/go-toolset:1.19.13-4.1697647145
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

AObuchow · 2024-11-01T22:02:11Z

/ok-to-test

AObuchow · 2024-11-04T03:41:26Z

/retest-required

ibuziuk · 2024-11-04T08:52:26Z

pkg/library/ssh/event.go

 then ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
 && chmod 600 $SSH_ENV_PATH && source $SSH_ENV_PATH \
-&& ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase \
-&& if [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc; fi; fi`
+&& if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase \


@vinokurig could you clarify the timeout part, looks error prone

@ibuziuk This is need to skip the passphrase input in case if the stored passphrase is wrong. There are some elaborations in the pull request description.

vinokurig · 2024-11-04T12:50:15Z

@AObuchow

Just to clarify: the intention of adding the timeout to the ssh-add is prevent the workspace from failing in the case where the user has provided an incorrect SSH passphrase?

yes

vinokurig · 2024-11-04T13:06:26Z

@AObuchow Thank you for the review and for the proposals.

Maybe we should improve the formatting of the commands for clarity? Here's an idea of how this could look|

done

We could also add some else statements to clarify which steps might have failed

The post start event output is not exposed to the pod events or logs so echoing is useless at this point. We can redirect the output to a file but that my add some instability e.g. the path where the log file is going to be located may not have the write permissions, WDYT?
I just have noticed that the output is redirected to /tmp/poststart-stdout.txt but as you mentioned the echoing might be redundant.

We could wrap the entire command with a (... || true) as an added safety mechanism for commands that are unlikely to fail, but still could theoretically fail.

done

AObuchow · 2024-11-04T16:00:20Z

/retest-required

AObuchow · 2024-11-04T16:45:00Z

pkg/library/ssh/event.go

-&& chmod 600 $SSH_ENV_PATH && source $SSH_ENV_PATH \
-&& ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase \
-&& if [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc; fi; fi`
+const commandLine = `(SSH_ENV_PATH=$HOME/ssh-environment \


This looks good to me, but in my testing I was able to copy & paste my final suggestion, which did not include a \ for every line as well as ; and had slightly different formatting that seemed a bit clearer to me:

const commandLine = `( SSH_ENV_PATH=$HOME/ssh-environment && \ if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null; then ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \ && chmod 600 $SSH_ENV_PATH \ && source $SSH_ENV_PATH \ && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase && [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc fi fi ) || true`

WDYT? Do we really need the \ line breaks and the ;?

Thanks for the suggestion, applied.

BTW I added a new check for the ssh-agent availability.

Awesome, thank you so much @vinokurig & good call on the check for the ssh-agent command 👍

AObuchow · 2024-11-04T17:30:45Z

/test v14-devworkspace-operator-e2e

Signed-off-by: ivinokur <[email protected]>

AObuchow

LGTM, great work & thank you for fixing this so quickly @vinokurig :)

In my testing, I tried the following devworkspace which also includes a succeeding and failing command, to ensure they would not be affected:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-go-toolset-with-event
spec:
  started: true
  routingClass: 'basic'
  template:
    components:
      - name: web-terminal
        container:
          image: registry.access.redhat.com/ubi9/go-toolset:1.19.13-4.1697647145
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"
    commands:
      - id: say-hello
        exec:
          component: web-terminal
          commandLine: echo "Hello from $(pwd)"
          workingDir: /
      - id: invalid
        exec:
          component: web-terminal
          commandLine: invalid-command
          workingDir: /
    events:
      postStart:
        - say-hello
        - invalid

The resulting postStart lifecycle hook in the pod:

    lifecycle:
      postStart:
        exec:
          command:
          - /bin/sh
          - -c
          - |
            {
            cd /
            echo "Hello from $(pwd)"
            cd /
            invalid-command
            (
            SSH_ENV_PATH=$HOME/ssh-environment && \
            if [ -f /etc/ssh/passphrase ] && [ -w $HOME ] && command -v ssh-add >/dev/null && command -v ssh-agent >/dev/null; then
                ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \
                && chmod 600 $SSH_ENV_PATH \
                && source $SSH_ENV_PATH \
                && if timeout 3 ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase && [ -f $HOME/.bashrc ] && [ -w $HOME/.bashrc ]; then
                    echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc
                fi
            fi
            ) || true
            } 1>/tmp/poststart-stdout.txt 2>/tmp/poststart-stderr.txt

I saw that the valid command had it's stdout logged, and the invalid command had stderr logged correctly:

bash-5.1$ cat /tmp/poststart-stderr.txt 
/bin/sh: line 5: invalid-command: command not found
bash-5.1$ cat /tmp/poststart-stdout.txt 
Hello from /

AObuchow · 2024-11-06T06:47:39Z

@dkwon17 when you get a chance, could you please take a look at this PR?

openshift-ci · 2024-11-06T20:42:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow, dkwon17, 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:

~~OWNERS~~ [AObuchow,dkwon17]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

AObuchow · 2024-11-06T21:26:31Z

Great work @vinokurig & thank you for reviewing this @dkwon17 -- merging :)

vinokurig requested review from AObuchow, dkwon17 and ibuziuk as code owners November 1, 2024 07:15

openshift-ci bot added the needs-ok-to-test label Nov 1, 2024

l0rd mentioned this pull request Nov 1, 2024

SSH agent initialization postStart event fails workspace if $HOME is not writable #1337

Closed

AObuchow reviewed Nov 1, 2024

View reviewed changes

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Nov 1, 2024

ibuziuk reviewed Nov 4, 2024

View reviewed changes

vinokurig force-pushed the dwo-1337 branch from 27f8531 to b337868 Compare November 4, 2024 12:44

vinokurig force-pushed the dwo-1337 branch from b337868 to 5a923f2 Compare November 4, 2024 13:23

AObuchow reviewed Nov 4, 2024

View reviewed changes

Add conditions to the SSH agent post-start command

c60ad5a

Signed-off-by: ivinokur <[email protected]>

vinokurig force-pushed the dwo-1337 branch from 5a923f2 to c60ad5a Compare November 5, 2024 08:43

AObuchow approved these changes Nov 6, 2024

View reviewed changes

openshift-ci bot assigned AObuchow Nov 6, 2024

openshift-ci bot added lgtm approved labels Nov 6, 2024

dkwon17 approved these changes Nov 6, 2024

View reviewed changes

openshift-ci bot assigned dkwon17 Nov 6, 2024

AObuchow merged commit fb59513 into devfile:main Nov 6, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditions to the SSH agent post-start command #1338

Add conditions to the SSH agent post-start command #1338

vinokurig commented Nov 1, 2024 •

edited

Loading

openshift-ci bot commented Nov 1, 2024

AObuchow commented Nov 1, 2024

AObuchow left a comment •

edited

Loading

AObuchow commented Nov 1, 2024

AObuchow commented Nov 1, 2024

AObuchow commented Nov 4, 2024

ibuziuk Nov 4, 2024

vinokurig Nov 4, 2024

vinokurig commented Nov 4, 2024

vinokurig commented Nov 4, 2024 •

edited

Loading

AObuchow commented Nov 4, 2024

AObuchow Nov 4, 2024

vinokurig Nov 5, 2024

vinokurig Nov 5, 2024

AObuchow Nov 6, 2024

AObuchow commented Nov 4, 2024

AObuchow left a comment

AObuchow commented Nov 6, 2024

openshift-ci bot commented Nov 6, 2024

AObuchow commented Nov 6, 2024

Add conditions to the SSH agent post-start command #1338

Add conditions to the SSH agent post-start command #1338

Conversation

vinokurig commented Nov 1, 2024 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Nov 1, 2024

AObuchow commented Nov 1, 2024

AObuchow left a comment • edited Loading

Choose a reason for hiding this comment

AObuchow commented Nov 1, 2024

AObuchow commented Nov 1, 2024

AObuchow commented Nov 4, 2024

ibuziuk Nov 4, 2024

Choose a reason for hiding this comment

vinokurig Nov 4, 2024

Choose a reason for hiding this comment

vinokurig commented Nov 4, 2024

vinokurig commented Nov 4, 2024 • edited Loading

AObuchow commented Nov 4, 2024

AObuchow Nov 4, 2024

Choose a reason for hiding this comment

vinokurig Nov 5, 2024

Choose a reason for hiding this comment

vinokurig Nov 5, 2024

Choose a reason for hiding this comment

AObuchow Nov 6, 2024

Choose a reason for hiding this comment

AObuchow commented Nov 4, 2024

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow commented Nov 6, 2024

openshift-ci bot commented Nov 6, 2024

AObuchow commented Nov 6, 2024

vinokurig commented Nov 1, 2024 •

edited

Loading

AObuchow left a comment •

edited

Loading

vinokurig commented Nov 4, 2024 •

edited

Loading