Vendor load-singlesshagent.sh script #409

danielhollas · 2023-09-27T01:04:58Z

Couple changes here:

I think it is better to vendor the load_singlesshagent.sh script in this repo, rather than downloading it from a super-weird readthedocs URL.
Fixed an annoying spurious and confusing message from this script when you first enter the container and the ssh-agent is not yet running. (see third commit)
Enabled autocompletion for aiidalab CLI app.

unkcpz

@danielhollas Thanks! Fully agree to move the sshagent script here. Just to sure that you didn't change any code in this script correct? Other changes are all good to me.

danielhollas · 2023-09-27T14:19:32Z

Just to sure that you didn't change any code in this script correct? Other changes are all good to me.

I did actually, sorry for not making that clear. I removed the spurious error message (point 2. in the OP), see the third commit. Also in the last commit a made a small bugfix, substituting $USER to $NB_USER.

danielhollas · 2023-09-27T14:20:45Z

stack/base/load-singlesshagent.sh

@@ -32,7 +32,7 @@ load_singlesshagent() {
        [ "$VERBOSE" == "true" ] && echo "- no existing environment to source" >&2
    fi

-    SSH_ADD_OUTPUT=`ssh-add -l`
+    SSH_ADD_OUTPUT=`ssh-add -l 2> /dev/null`


@unkcpz here's the small change to get rid of spurious errors.. Note that it is okay to redirect error to dev/null here since we check the return value of this ssh-add command below.

unkcpz · 2023-09-28T18:37:44Z

Also in the last commit a made a small bugfix, substituting $USER to $NB_USER.

Is this means the original downloaded script should not work properly. I'll have a test to see how sshagent is actually working, I guess it is related to when ssh key phrase is not empty.

unkcpz · 2023-09-28T21:56:51Z

@danielhollas thanks for the change. I read some detailed introductions on ssh-agent, along with the script. I think maybe the correct solution is to

remove the eval ssh-agen since the script takes care of starting ssh-agent.
Then move the load-singlesshagent.sh source section after the ssh key pairs are generated.

Can you give this a try? Or if you don't mind I can add commits to test it.

danielhollas · 2023-10-02T01:07:52Z

@unkcpz feel free to add commits with your proposed changes. Thanks!

danielhollas · 2023-10-02T11:02:47Z

@unkcpz I am wondering: I think we still need the eval ssh-agent, since we want the ssh-agent to be started upon container startup, whereas I think .bashrc gets executed only when you enter the container? (maybe I am wrong on this). In which case, I am wondering if the singlesshagent script should be called directly, and not from .bashrc? I am not sure...

unkcpz · 2023-10-02T12:06:30Z

Hi @danielhollas, thanks for bringing this up. I was testing exactly what you mentioned as well before lunch.

whereas I think .bashrc gets executed only when you enter the container?

This is true. However, the load_sshagent.sh script also includes this and it already starts the ssh-agent. The script needs to be sourced so it makes sense to put it in .bashrc.
What is missing previously is the ssh-add I think. You can try the new image now and if you go into the container you'll find ssh-add -l will show that the key is added to the list.

docker pull ghcr.io/aiidalab/full-stack:sha-7a773b3
docker run --rm -it -p 8888:8888 ghcr.io/aiidalab/sha-7a773b3

Open the terminal (either from docker exec or from aiidalab terminal) and ssh-add -l to check the key is added.

I personally don't know why this ssh-agent is needed in our AiiDAlab case in the first place. We set the ssh private key with the empty passphrase, thus no need to input the passphrase even without ssh-agent to ssh to the remote once the passfree ssh is set up.
From what I understand, the only benefit is the ssh-agent is helpful when there is a jump server, and then no need to add the private key to the jump server.
I guess @yakutovicha is more exporter on the SSH stuff, maybe he can see the proper way this was used and how it should be put in the container.

danielhollas · 2023-10-02T12:19:58Z

stack/base/load-singlesshagent.sh

+    then
+        [ "$VERBOSE" == "true" ] && echo "  - ssh-agent found (${SSH_AGENT_PID}), no keys (I might want to add keys here)" >&2
+        # Maybe the user wants to do it by himself?
+        #ssh-add


@unkcpz I would prefer to uncomment this instead of adding it separately.

For us, I think yes. Is this you commented out or it was there already?

For this use case, I don't know if automatically calling ssh-add works since the user needs to provide the passphrase.

I see, then I think maybe calling it outside explicitly as I did in the last commit is a proper solution.

danielhollas · 2023-10-02T12:44:46Z

I found this comment from @yakutovicha in #254 (review)

Another thing we should not forget is the ssh-agent to be able to connect to machines that require ssh keys with password

For this use case, I don't know if automatically calling ssh-add works since the user needs to provide the passphrase.

We need more info from @yakutovicha to see how this is actually being used before we can continue here.

danielhollas · 2023-10-11T17:15:10Z

@yakutovicha can you please clarify what is the current workload for private keys? Do we support private keys that require a passphrase? And if so how does the user provide the passphrase to get the key to ssh-agent?

unkcpz · 2023-10-12T09:29:13Z

My two cents, see the reply for questions below.

can you please clarify what is the current workload for private keys?

The private key file can be uploaded by the SshComputerWidget and add a section in .ssh/config with the identityFile pointed to the path of the key. You found there are some issue on this when the key file has the same name of the private key existed, and it is fixed in aiidalab/aiidalab-widgets-base#511

Otherwise, only the id_rsa which was generated the first time the container started is used.

Do we support private keys that require a passphrase?

No, I don't think so. But I think it would be a problem, user who has the private key with a passphrase means they generate it without using widget and has security concern about it so it is their duty to add the key to the ssh-agent and they can do it manually in terminal.

Then it comes to the question, if we don't support passphrase, do we still need ssh-agent?
My answer is yes and in the AiiDAlab context we support only non-passphrase ssh key pairs.

The ssh-agent provides the following advantages, 1) if using ssh-add to put the private key to the agent, during the session the passphrase won't be asked anymore and with the passphrase the key is secure even it is stolen. sadly we didn't support passphrase in AiiDAlab. 2) ssh-agent can manage multiple keys, if you have multiple SSH keys for different purposes, ssh-agent can help you manage them without explicitly setting it in the .ssh/config. (we didn't use this feature either but it because necessary when we start to support MFA of CSCS, see the explain below).

The aiidalab-mfa-cscs app will generate cscs-key and cscs-key.pub and the key will expire in 1d. From the ComputationalResourceWidget it has no way to know the name of the private key that will be generated by other tools so it is only responsible for adding minimal information to the .ssh/config which has no IdentityFile option. With ssh-agent, aiidalab-mfc-cscs can do ssh-add to add the key it generated and then the connection is constructed. I have tested it locally and it works in charm.

In summary, we need ssh-agent but we can not support private key with passphrase.

stack/base/load-singlesshagent.sh

stack/base/before-notebook.d/10_prepare-home-config.sh

unkcpz · 2023-10-13T09:09:59Z

@danielhollas I add two comments for what I think it is the proper way to deal with the agent and ssh-add. If you agree, we can apply the changes and then can test the image locally.
Also worth to take a look with aiidalab/aiidalab-mfa-cscs#6

danielhollas

@unkcpz thank you for looking into this and a detailed write up! See my comments, I agree with your plan about ssh-add

danielhollas · 2023-10-13T09:56:12Z

stack/base/before-notebook.d/10_prepare-home-config.sh

 if [[ ! -f  /home/${NB_USER}/.ssh/id_rsa ]]; then
  # Generate ssh key that works with `paramiko`
  # See: https://aiida.readthedocs.io/projects/aiida-core/en/latest/get_started/computers.html#remote-computer-requirements
  ssh-keygen -f /home/${NB_USER}/.ssh/id_rsa -t rsa -b 4096 -m PEM -N ''
 fi

-# Start the ssh-agent.
-eval `ssh-agent`


Based on the discussion, I think we should source the load-singlesshagent.sh script here. Otherwise I don't think the ssh-agent will be running when the container starts, since .bashrc is not evaluated AFAIK.

Ha! I think the one of the ssh-agent mentioned in aiidalab/aiidalab-mfa-cscs#6 is from here!
I agree to source the load-singlesshagent.sh here. It avoids the duplicate ssh-agent I think.

Yeah, interesting. That would mean that it is started somewhere else before we get to this script. Maybe the Jupyter image that we use already starts it somewhere. In any case, source the script here is the right thing to do I think.

stack/base/before-notebook.d/10_prepare-home-config.sh

stack/base/load-singlesshagent.sh

unkcpz · 2023-10-13T15:32:23Z

stack/base/load-singlesshagent.sh

+        [ "$VERBOSE" == "true" ] && echo "- no existing environment to source" >&2
+    fi
+
+    SSH_ADD_OUTPUT=`ssh-add -l`


The error here should not be redirect, to /dev/null, it will cause the exit_code not properly get and the expected condition not triggered (2 is expected to trigger the launch of ssh-agent).

Huh, that is really strange, are you sure? I am only redirecting stderr, that should AFAIK not change the exit code.

I am not sure 😛 if I don’t remove it the ssh-agent create clause is not hit. I’ll play more to figure out why. But overall everything works fine no matter if the output is redirected or not.

I changed it back to ssh-add -l 2> /dev/null and it still works fine. The problem I encountered is not because of this line but the other redirected probably.

Then this PR is ready, please have another look. @danielhollas

unkcpz · 2023-10-13T15:33:18Z

@danielhollas should all work as expected now. I also added a test to check the ssh-agent is started properly.
Please have a test on the image after it is successfully built.

danielhollas · 2023-10-18T14:01:29Z

This is looking great, thanks @unkcpz for adding the tests! I didn't have a chance to test this but I trust that you did. Feel free to merge.

danielhollas requested a review from unkcpz September 27, 2023 01:05

unkcpz reviewed Sep 27, 2023

View reviewed changes

danielhollas commented Sep 27, 2023

View reviewed changes

danielhollas requested a review from unkcpz September 27, 2023 14:21

danielhollas enabled auto-merge (squash) September 27, 2023 20:36

unkcpz force-pushed the vendor-ssh-script branch from 0a15973 to ded39f9 Compare October 2, 2023 10:43

danielhollas commented Oct 2, 2023

View reviewed changes

unkcpz force-pushed the vendor-ssh-script branch from 06a2b0b to ded39f9 Compare October 2, 2023 12:49

unkcpz mentioned this pull request Oct 13, 2023

Run and add ssh key to agent aiidalab/aiidalab-mfa-cscs#6

Merged

unkcpz reviewed Oct 13, 2023

View reviewed changes

stack/base/load-singlesshagent.sh Outdated Show resolved Hide resolved

stack/base/before-notebook.d/10_prepare-home-config.sh Outdated Show resolved Hide resolved

danielhollas commented Oct 13, 2023

View reviewed changes

danielhollas and others added 6 commits October 13, 2023 16:32

Vendor load-singlesshagent.sh

a8e5d79

Enable aiidalab CLI autocompletion

d9702d7

Remove spurious message when ssh-agent is not running

42d9a34

Fix path

378cfa8

bl22441 ->

ebb4046

f-jason: change launch order for sshagent

9f9a9af

unkcpz force-pushed the vendor-ssh-script branch 3 times, most recently from 1ca249f to a2d9261 Compare October 13, 2023 15:21

unkcpz force-pushed the vendor-ssh-script branch 2 times, most recently from e5a05cd to e27b168 Compare October 13, 2023 15:30

unkcpz reviewed Oct 13, 2023

View reviewed changes

f-dd

dad0f82

unkcpz force-pushed the vendor-ssh-script branch from e27b168 to dad0f82 Compare October 13, 2023 15:46

redirect 1 & 2 properly

eefe86a

unkcpz disabled auto-merge October 18, 2023 15:31

unkcpz merged commit d292ea8 into main Oct 18, 2023
24 checks passed

unkcpz deleted the vendor-ssh-script branch October 18, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor load-singlesshagent.sh script #409

Vendor load-singlesshagent.sh script #409

danielhollas commented Sep 27, 2023

unkcpz left a comment

danielhollas commented Sep 27, 2023

danielhollas Sep 27, 2023 •

edited

Loading

unkcpz commented Sep 28, 2023 •

edited

Loading

unkcpz commented Sep 28, 2023

danielhollas commented Oct 2, 2023

danielhollas commented Oct 2, 2023

unkcpz commented Oct 2, 2023

danielhollas Oct 2, 2023

unkcpz Oct 2, 2023

unkcpz Oct 2, 2023

danielhollas commented Oct 2, 2023

danielhollas commented Oct 11, 2023

unkcpz commented Oct 12, 2023

unkcpz commented Oct 13, 2023 •

edited

Loading

danielhollas left a comment

danielhollas Oct 13, 2023

unkcpz Oct 13, 2023

danielhollas Oct 13, 2023

unkcpz Oct 13, 2023

danielhollas Oct 13, 2023

unkcpz Oct 13, 2023

unkcpz Oct 16, 2023

unkcpz commented Oct 13, 2023

danielhollas commented Oct 18, 2023

Vendor load-singlesshagent.sh script #409

Vendor load-singlesshagent.sh script #409

Conversation

danielhollas commented Sep 27, 2023

unkcpz left a comment

Choose a reason for hiding this comment

danielhollas commented Sep 27, 2023

danielhollas Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz commented Sep 28, 2023 • edited Loading

unkcpz commented Sep 28, 2023

danielhollas commented Oct 2, 2023

danielhollas commented Oct 2, 2023

unkcpz commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Oct 2, 2023

danielhollas commented Oct 11, 2023

unkcpz commented Oct 12, 2023

unkcpz commented Oct 13, 2023 • edited Loading

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Oct 13, 2023

danielhollas commented Oct 18, 2023

danielhollas Sep 27, 2023 •

edited

Loading

unkcpz commented Sep 28, 2023 •

edited

Loading

unkcpz commented Oct 13, 2023 •

edited

Loading