Modify

Opened 7 years ago

Last modified 7 years ago

#545 reopened enhancement

Use ssh ProxyCommand instead of multiple ssh commands

Reported by: Erik Schnetter Owned by: Erik Schnetter
Priority: major Milestone:
Component: SimFactory Version:
Keywords: Cc:

Description

This would simplify quoting, thus also simplify the screen output, and may speed up things on remote connections.

Here is an example:

ssh -Y -o ProxyCommand='ssh eschnetter@… nc -w 1 curry.pi.local 22' eschnett@…

Attachments (1)

simfactory_ssh_proxy (4.6 KB) - added by Frank Löffler 7 years ago.
This seems to work for me, please try and comment.

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by Frank Löffler

Attachment: simfactory_ssh_proxy added

This seems to work for me, please try and comment.

comment:1 Changed 7 years ago by Frank Löffler

Status: newreview

comment:2 Changed 7 years ago by Erik Schnetter

The quote handling in this patch is off. Simfactory has a function QuoteSafe that quotes its argument, so that it can safely be passed to other commands such as bash or ssh. Instead of removing quotes or adding "" around a string, QuoteSafe should be used.

The current trampoline code is written recursively and can handle multiple levels of trampolines. I am not aware of a current machine that requires this, but it was necessary for the SiCortex test machine. If we wanted e.g. to access LONI compute nodes we would still need this. The patch seems to remove the recursion and will thus handle only a single level of trampolines. This may be fine, but there then needs to be a check to ensure that the trampoline does not require a trampoline. Can the ProxyCommand mechanism be nested? Do we want this functionality? It would be elegant, but does complicate things.

The current code is a bit convoluted, because the trampoline handling code is called both for ssh and for rsync commands. You implement this via an explicit switch (test if the argument is RSYNC); this may be a good idea, but an additional boolean argument may be a cleaner approach. Also, the current patch doesn't make it clear under which circumstances cmd would be empty or non-empty. I guess this is for "login" and "execute", respectively?

The nc command should be quoted with QuoteSafe instead of an explicit backslash.

comment:3 Changed 7 years ago by Frank Löffler

QuoteSafe unfortunately does it's job too well. The way simfactory quotes is not working in all cases, and it is not in this. For instance, the sshcmd is taken into a string which is surrounded by single quotes. If now sshsmd is passed to QuoteSafe, it get additional single quotes around it, messing up with the overall quoting. Agreed, the quoting in simfactory should be improved. E.g. quoting an empty string should result in an empty string. But that is another ticket.

You say the current quoting is off. It works for me. Did you try it and it didn't work for you?

Your concern about multiple trampolines is true: the patch probably does not support that. A better quoting should make that possible in a clean way. However, are multiple trampolines a real concern for anyone?

Ok, I agree that we might not want to use the current patch as it is. Do we agree that we need a better way to quote command line arguments?

comment:4 Changed 7 years ago by Erik Schnetter

All quoting in Simfactory (where a shell is involved) should be done via QuoteSafe. (This may currently not always be the case.) If there is a string surrounded by single quotes, then either someone "just added" them (and should have used QuoteSafe instead), or the were added via QuoteSafe, and then they don't need to be quoted again.

No, I didn't try it -- I trust you when you say it works for you. However, we need to define what corner cases we need to be treated correctly (e.g. multiple trampolines, having a remote source base directory with a space in the name, etc.). We currently don't need multiple trampolines, so we may want to drop them. Out of curiosity: How would a recursive trampolining command with ProxyCommand look like? Would the nc command be wrapped by an ssh command with another ProxyCommand?

The basic quoting rule is: whenever a command is constructed that will be passed to a shell, then each argument (except trivial ones) need to be quoted. For example, 'nc %h %p ...' is a command, so %h and %p should be quoted; however, if we assume that hostname and port number are reasonable, we can skip this. The whole nc command is an option to the ssh command, so it then needs to be quoted as a whole:

sshopts += '-o ProxyCommand=%s' % QuoteSafe(tsshcmd)

An empty string, when quoted, should not remain an empty string, it needs to become two consecutive quotes, so that (after passing it to a shell) it becomes an empty string again.

comment:5 Changed 7 years ago by Frank Löffler

ssh proxies can be nested, however it turns out that the %h syntax is a problem here: every %h is replaced by ssh, regardless of quoting. Thus, simfactory would have to do that itself - which isn't a problem since that is known. Similarly for the port, but there simfactory would have to parse non-standard ports from given options.

One example for -> numrel07 -> numrel08 -> queenbee would be:

ssh -o ProxyCommand='ssh -o ProxyCommand='\ssh numrel07.cct.lsu.edu nc numrel08.cct.lsu.edu %p'\ numrel08.cct.lsu.edu nc qb.loni.org %p' qb.loni.org

I agree with what you say about quoting. An empty string however, should not be quoted. It should always remain an empty string (although both options should work). It's not necessary and clutters the output, especially if these quotes are quoted again. An empty string, when passed to a shell, remains an empty string, regardless if quoted or not. Ideally, QuoteSafe would be able to tell if the argument needs quoting at all, and would only quote in that case - but of course that is optional.

comment:6 Changed 7 years ago by Ian Hinder

At the operating system level, new processes are started via execp (and variants). A process receives a list of arguments which are C strings - i.e. null-terminated. When launching a process from the shell, the user has to be able to make a syntactic distinction between the content of the argument and the separators indicating different arguments. So in addition to the need to quote characters which would otherwise be interpreted by the shell, there is also the need to group characters into the appropriate arguments. This is done using double or single quotes so that each shell "word" is passed to the process as a separate argument. As such, it is certainly possible for one of the arguments to a process to be an empty string.

For example (in Python for ease of use, but the same would apply to any language):

#!/usr/bin/env python
import sys
print sys.argv

MacBook-2:~ $ ./test.py 1 2 "" 3
['./test.py', '1', '2', , '3']

So I weigh in on the side of Erik and say that an empty string should be quoted so that it remains an empty string when it is unquoted. This is probably a special-case, but it's important to get QuoteSafe correct, as quoting is a nightmare, and is made doubly so when it doesn't work properly!

Questions:

  • Is the quoting required dependent on which shell is being used?
  • Is it possible to get the shell to quote a string itself?

comment:7 Changed 7 years ago by Ian Hinder

Whoops. TRAC decided to format my example. The output should have been:

MacBook-2:~ $ ./test.py 1 2 "" 3
['./test.py', '1', '2', '', '3']

comment:8 Changed 7 years ago by Erik Schnetter

Quoting is independent of the shell. It is also rather simple, because a shell does not interpret any characters as special within single quotes. The only thing left to do is to quote single quotes themselves; this can be done either by prepending a backslash, or by enclosing it in double quotes. Both become impossible to decipher if there are more than two levels of quoting.

One also needs to know that there needs to be an unquoted space to separate arguments. For example,

Hello' 'World

is a single argument. So, to quote in a single-quote-delimited string, one first has to end the string (single quote), then add a quoted quote (backslash - single quote), then begin the string again (single quote).

The general quoting rule is:

  1. replace any single quote by '\ (single quote - backslash - single quote - single quote)
  2. surround the string by single quotes

We could us a shellquote module for this, but the corresponding Python function is just two lines long, so this may be overkill.

comment:9 Changed 7 years ago by Frank Löffler

Status: reviewreopened

Since there is no current open patch for this, reopening.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Erik Schnetter.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Erik Schnetter to the specified user.
The owner will be changed from Erik Schnetter to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.