- changed status to open
- removed comment
Use ssh ProxyCommand instead of multiple ssh commands
Issue #545
open
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@mars.perimeterinstitute.ca nc -w 1 curry.pi.local 22' eschnett@curry.pi.local
Keyword:
Comments (9)
-
-
reporter
- removed comment
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.
-
- removed comment
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?
-
reporter
- removed comment
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.
-
- removed comment
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.
-
- removed comment
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?
- Is there a standard Python way to perform quoting? Answering my own question, Stack Overflow has this to say on the matter: http://stackoverflow.com/questions/967443/python-module-to-shellquote-unshellquote. Maybe we could just use the snippet provided?
-
- removed comment
Whoops. TRAC decided to format my example. The output should have been:
MacBook-2:~ $ ./test.py 1 2 "" 3 ['./test.py', '1', '2', '', '3']
-
reporter
- removed comment
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.
-
- changed status to open
- removed comment
Since there is no current open patch for this, reopening.
- Log in to comment
