ProxyCommand none breaks connection

[dtrifiro]

According to man ssh_config, setting ProxyCommand none should disable the option entirely.

asyncssh interprets none as the command to execute as proxy command, thus returning the following error:

File "asyncssh/asyncssh/connection.py", line 434, in _connect
  conn = await _open_proxy(loop, proxy_command, conn_factory)
File "asyncssh/asyncssh/connection.py", line 352, in _open_proxy
  _, tunnel = await loop.subprocess_exec(_ProxyCommandTunnel, *command)
File "/usr/lib/python3.10/asyncio/base_events.py", line 1670, in subprocess_exec
  transport = await self._make_subprocess_transport(
File "/usr/lib/python3.10/asyncio/unix_events.py", line 207, in _make_subprocess_transport
  transp = _UnixSubprocessTransport(self, protocol, args, shell,
File "/usr/lib/python3.10/asyncio/base_subprocess.py", line 36, in __init__
  self._start(args=args, shell=shell, stdin=stdin, stdout=stdout,
File "/usr/lib/python3.10/asyncio/unix_events.py", line 800, in _start
  self._proc = subprocess.Popen(
File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
  self._execute_child(args, executable, preexec_fn, close_fds,
File "/usr/lib/python3.10/subprocess.py", line 1847, in _execute_child
  raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'none'

[ronf]

Thanks for the report and the proposed fix. I don't think it makes sense to make this change in connection.py, though. There's already code in config.py which attempts to handle this for regular string parameters in the config file and other cases like rekey limits. ProxyCommand is currently set as a string list, and the special-casing for 'none' is not present there yet, so that's probably why you ran into this.

That said, I'm trying to figure out the use case for this, since ProxyCommand defaults to none, and normally when you have multiple instances of a property in the config file the earliest one is chosen. So, how would 'none' ever do anything? Once the value was set, I'd expect later instances of ProxyCommand to be ignored, and if it wasn't previously set, setting it to 'none' shouldn't actually do anything. Can you give an example of how you're using it? Does it involve something like reading from both a host config file and a user config file?

As for the proposed documentation changes, I don't actually think it makes sense to allow 'none' when using the AsyncSSH Python APIs directly (outside of the config file). In those cases, I'd prefer it to only accept the native Python None value. However, if someone uses 'none' in a config file, AsyncSSH should treat that the same as OpenSSH (disabling the ProxyCommand in this case).

[dtrifiro]

Hi @ronf,

thanks for the suggestions. I agree that would be cleaner, I updated the PR accordingly.

As for the use case: I'm assuming having ProxyCommand none is useful for cases in which you need a proxy to access all resources but a few internal ones, something like this:

Host <internal host>
    ProxyCommand none

Host *
    ProxyCommand <proxy command>

edit: swapped ordering, leaving the more generic option last

[ronf]

I need to do some experiments with OpenSSH to be sure, but I don't think the above example you have with "Host *" first will do what you want here. According to the OpenSSH ssh_config man page:

 For each parameter, the first obtained value will be used.  The
 configuration files contain sections separated by Host specifications,
 and that section is only applied for hosts that match one of the patterns
 given in the specification.  The matched host name is usually the one
 given on the command line (see the CanonicalizeHostname option for
 exceptions).

 Since the first obtained value for each parameter is used, more host-
 specific declarations should be given near the beginning of the file, and
 general defaults at the end.

That said, you could reverse the order of those two "Host" blocks to make it work, and I see now why "none" would be needed. You need to set "none" explicitly in the "Host \<internal host>" block to make sure that this is the value which gets set first for requests to that internal host. That way, when the second "Host *" block is hit, the "ProxyCommand \<proxy command>" entry will be ignored, since ProxyCommand would already have been set.

Regarding the patch, I think support for none is also needed for some of the other commands (GlobalKnownHostsFile and UserKnownHostsFile, as well as AuthorizedKeysFile in an SSH server configuration). The latter explicitly mentions none in the sshd_config man page. The others don't mention none, but in a quick test here it seemed to accept it and treat it differently than a filename of "none".

That said, I think I'll need to make changes to connection.py to properly handle GlobalKnownHostsFile or UserKnownHostsFile being set to None. Once I've done that, it should be possible to simplify the config.py change down to just modifying set_string_list() directly and not having anything specific to ProxyCommand.

I also need to look more closely at the append_string_list() calls (used right now for SendEnv and SetEnv) to see if similar treatment is needed there, mirroring what was done in append_string(). I think it will be, to set an empty list of environment variables in one Host block to make sure that gets locked in and isn't changed by a later block (similar to the case above).

[ronf]

Actually, thinking a bit more about it, none in the config for the set_string_list case should set the empty list and not translate into a Python None value the way it does for plain strings. So, I won't need to change how GlobalKnownHostsFile and UserKnownHostsFile are processed in connection.py after all.

The only remaining piece is to confirm what happens with SendEnv/SetEnv when none is used. Unlike other variables, these allow you to have multiple SendEnv/SetEnv directives and all of them will supposedly be added, but based on a quick test it seems like maybe "none" isn't supported in these two cases. It's just treated as an environment variable named "none".. So, there's probably no way to clear the whole list set by previous matching host blocks and start over with a different list.

Based on that, I guess append_string_list() can be left alone for now, and all that's needed is a change to set_string_list(). I should be able to get that in fairly quickly.

[ronf]

Here's a patch to try:

diff --git a/asyncssh/config.py b/asyncssh/config.py
index c460817..5921f01 100644
--- a/asyncssh/config.py
+++ b/asyncssh/config.py
@@ -243,7 +243,10 @@ class SSHConfig:
         """Set whitespace-separated string config options as a list"""

         if option not in self._options:
-            self._options[option] = args[:]
+            if len(args) == 1 and args[0].lower() == 'none':
+                self._options[option] = []
+            else:
+                self._options[option] = args[:]

         args.clear()

diff --git a/tests/test_config.py b/tests/test_config.py
index 51a383f..d06eec6 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -279,6 +279,10 @@ class _TestClientConfig(_TestConfig):
                                     'UserKnownHostsFile file2')
         self.assertEqual(config.get('UserKnownHostsFile'), ['file1'])

+        config = self._parse_config('UserKnownHostsFile none\n'
+                                    'UserKnownHostsFile file2')
+        self.assertEqual(config.get('UserKnownHostsFile'), [])
+
     def test_append_string_list(self):
         """Test appending multiple string config options to a list"""

[dtrifiro]

That said, you could reverse the order of those two "Host" blocks to make it work, and I see now why "none" would be needed.

Indeed. Reversed the order in my original comment too.

Regarding the patch: that looks good. Updated the PR with that patch if that can help, feel free to close it otherwise.

Thanks!

[ronf]

Thanks very much - the fix for this is now available in the "develop" branch as commit https://github.com/ronf/asyncssh/commit/aaf7d2910993a87d059bddcbf04c42df3d017ca9 and will be included in the next release.