Support for Custom Agent Socket Path in ForwardAgent

[cjcjcj]

Hi,

I encountered a specific use case: in my company, we use a custom socket path for agent forwarding. Currently, in asyncssh, the ForwardAgent parameter only accepts boolean values.

Quote from the man.openbsd.org/ssh_config:

... argument may be yes, no (the default), an explicit path to an agent socket or the name of an environment variable (beginning with ‘$’) in which to find the path.

It would be great to have support for specifying a custom agent socket path in addition to the boolean options.

Here are references:

• Link to OpenSSH config parsing for the ForwardAgent section
• Link to a comment about priorities

Please let me know if you're open to adopting this feature and whether you’d accept a pull request for it.

[ronf]

Thanks for pointing this out! I didn't realize that ForwardAgent accepted those extra values. I must have been looking at older documentation created before OpenSSH added that support when I did my first release of config file parsing.

It looks like in the "yes" case, the agent_forward_path is set to the same as agent_path (which in the config file can come from the IdentityAgent option). I'm guessing setting an explicit pathname or environment variable here just allows different agents to be used locally for authentication vs. for requests forwarded from upstream. Does that match your use case?

As an aside, based on the bit of research I've done so far, it looks like the environment variable syntax will need to support both $FOO and ${FOO}, but that should be easy enough.

Right now, I don't see a way using SSHClientConnectionOptions to set a different path for agent forwarding vs. local agent use. I think that should probably be added as part of this change. If that was set, it would override anything coming from the config file. When not set, the config file would be used to decide what path to use (if any), defaulting to "no" which would disable agent forwarding.

[cjcjcj]

I've found few more things:
They've released it in OpensSSH 8.2(which was released on 2020-02-14) as a part of those feature requests:

• https://bugzilla.mindrot.org/show_bug.cgi?id=2216
• https://bugzilla.mindrot.org/show_bug.cgi?id=1937
• https://bugzilla.mindrot.org/show_bug.cgi?id=3106

allows different agents to be used locally for authentication vs. for requests forwarded from upstream. Does that match your use case?

yes, pretty accurate
here's how typical ssh_config looks like:

Host SPACE_SEPARATED_DEV_HOST_LIST
  # Forward default sock to the remote dev machine
  ForwardAgent <sockets-path>/default.sock

Host *.foobar.net
  # Forward untrusted socket to the untrusted environment
  ForwardAgent <sockets-path>/untrusted.sock
  # But authenticate through default socket
  IdentityAgent <sockets-path>/default.sock

I'll take some time to study the codebase and hopefully come back within a month or so. Do you see any possible pitfalls besides SSHClientConnectionOptions?

[ronf]

The good news here is that SSHClientConnection and SSHClientConnectionOptions already support separate values for agent_path and agent_forward_path, and do the right thing with each of these. So, all the should be needed is to do the extra parsing to handle a pathname and environment variable in ForwardAgent (and probably also the agent_forwarding argument) and make sure the agent_forward_path option is set appropriately. I'll take a closer look at this tonight.

[ronf]

Ok - I've got a first cut at this. Here are the changes:

diff --git a/asyncssh/config.py b/asyncssh/config.py
index 5007ef7..9a10534 100644
--- a/asyncssh/config.py
+++ b/asyncssh/config.py
@@ -520,6 +520,21 @@ class SSHClientConfig(SSHConfig):
                 cast(str, self._options.get(option, self._orig_host))
             self._options[option] = self._expand_val(value)

+    def _set_forward_agent(self, option: str, args: List[str]) -> None:
+        """Set agent forwarding path config option"""
+
+        value_str = args.pop(0)
+
+        if value_str.lower() in ('yes', 'true'):
+            value: Union[bool, str] = True
+        elif value_str.lower() in ('no', 'false'):
+            value = False
+        else:
+            value = value_str
+
+        if option not in self._options:
+            self._options[option] = value
+
     def _set_request_tty(self, option: str, args: List[str]) -> None:
         """Set a pseudo-terminal request config option"""

@@ -587,7 +602,7 @@ class SSHClientConfig(SSHConfig):
         ('Compression',                     SSHConfig._set_bool),
         ('ConnectTimeout',                  SSHConfig._set_int),
         ('EnableSSHKeySign',                SSHConfig._set_bool),
-        ('ForwardAgent',                    SSHConfig._set_bool),
+        ('ForwardAgent',                    _set_forward_agent),
         ('ForwardX11Trusted',               SSHConfig._set_bool),
         ('GlobalKnownHostsFile',            SSHConfig._set_string_list),
         ('GSSAPIAuthentication',            SSHConfig._set_bool),
diff --git a/asyncssh/connection.py b/asyncssh/connection.py
index 201e44b..d79b0b8 100644
--- a/asyncssh/connection.py
+++ b/asyncssh/connection.py
@@ -7640,8 +7640,12 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
            made available for use. This is the default.
        :param agent_forwarding: (optional)
            Whether or not to allow forwarding of ssh-agent requests from
-           processes running on the server. By default, ssh-agent forwarding
-           requests from the server are not allowed.
+           processes running on the server. This argument can also be set
+           to the path of a UNIX domain socket or to an environment variable
+           containing the path to use (indicated by beginning the value with
+           a '$') in cases where forwarded agent requests should be sent to a
+           different path than client agent requests.  By default, forwarding
+           of ssh-agent requests from the server is not allowed.
        :param pkcs11_provider: (optional)
            The path of a shared library which should be used as a PKCS#11
            provider for accessing keys on PIV security tokens. By default,
@@ -7874,7 +7878,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
        :type agent_path: `str`
        :type agent_identities:
            *see* :ref:`SpecifyingPublicKeys` and :ref:`SpecifyingCertificates`
-       :type agent_forwarding: `bool`
+       :type agent_forwarding: `bool` or `str`
        :type pkcs11_provider: `str` or `None`
        :type pkcs11_pin: `str`
        :type client_version: `str`
@@ -8016,7 +8020,7 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
                 disable_trivial_auth: bool = False,
                 agent_path: DefTuple[Optional[str]] = (),
                 agent_identities: DefTuple[Optional[IdentityListArg]] = (),
-                agent_forwarding: DefTuple[bool] = (),
+                agent_forwarding: DefTuple[Union[bool, str]] = (),
                 pkcs11_provider: DefTuple[Optional[str]] = (),
                 pkcs11_pin: Optional[str] = None,
                 command: DefTuple[Optional[str]] = (),
@@ -8242,9 +8246,23 @@ class SSHClientConnectionOptions(SSHConnectionOptions):
             self.pkcs11_pin = None

         if agent_forwarding == ():
-            agent_forwarding = cast(bool, config.get('ForwardAgent', False))
+            agent_forwarding = cast(Union[bool, str],
+                                    config.get('ForwardAgent', False))
+
+        agent_forwarding: Union[bool, str]

-        self.agent_forward_path = agent_path if agent_forwarding else None
+        if not agent_forwarding:
+            self.agent_forward_path = None
+        elif agent_forwarding is True:
+            self.agent_forward_path = agent_path
+        elif agent_forwarding[:1] == '$':
+            var = agent_forwarding[1:]
+            if var[:1] == '{' and var[-1:] == '}':
+                var = var[1:-1]
+
+            self.agent_forward_path = os.environ.get(var)
+        else:
+            self.agent_forward_path = agent_forwarding

         self.command = cast(Optional[str], command if command != () else
             config.get('RemoteCommand'))

I've also got some unit tests written which I'll include in the actual check-in, but the above changes should be enough to let you try it out if you'd like to.

[cjcjcj]

Thanks for the quick changes!

I tested it with various configuration options, and it works in all cases except when the path is specified via a macro.

OK configurations

Use Identity Agent and forward it

Host asyncssh-test
  HostName myhost
  IdentityAgent /Users/cjcjcj/.sockets/sock/default.sock
  ForwardAgent yes

Same path for identity and forward

Host asyncssh-test
  HostName myhost
  IdentityAgent /Users/cjcjcj/.sockets/sock/default.sock
  ForwardAgent /Users/cjcjcj/.sockets/sock/default.sock

Use different agents

Host asyncssh-test
  HostName myhost
  IdentityAgent /Users/cjcjcj/.sockets/sock/default.sock
  ForwardAgent /Users/cjcjcj/.sockets/sock/untrusted.sock

Macros for identity agent and forward it

Host asyncssh-test
  HostName myhost
  IdentityAgent %d/.sockets/sock/default.sock
  ForwardAgent yes

Forward agent absolute path set via env. variable

Host asyncssh-test
  HostName myhost
  IdentityAgent %d/.sockets/sock/default.sock
  ForwardAgent $ASYNCSSH_TEST_FORWARD_AGENT_PATH

Forward agent absolute path set via env. variable

Host asyncssh-test
  HostName myhost
  IdentityAgent %d/.sockets/sock/default.sock
  ForwardAgent ${ASYNCSSH_TEST_FORWARD_AGENT_PATH}

And it doesn't work for the following cases:

1. When the path is set via macros

Host asyncssh-test
  HostName myhost
  IdentityAgent %d/.sockets/sock/default.sock
  ForwardAgent %d/.sockets/sock/untrusted.sock

2. ssh-config with env. variable and env. variable set to the path via macros

Host asyncssh-test
  HostName myhost
  IdentityAgent %d/.sockets/sock/default.sock
  ForwardAgent ${ASYNCSSH_TEST_FORWARD_AGENT_PATH}

ASYNCSSH_TEST_FORWARD_AGENT_PATH="%d/.sockets/sock/default.sock" python3 test.py

Both of them fail with the following error: " error fetching identities: communication with agent failed"

---

Code I've used for testing

import asyncio

import asyncssh


HOST: str = 'asyncssh-test'


async def print_keys(host: str):
    async with asyncssh.connect(host) as conn:
        result = await conn.run('ssh-add -l')
        print(result.stdout, result.stderr, end='')

asyncio.get_event_loop().run_until_complete(print_keys(HOST))

[cjcjcj]

fixed 1st case w/ this change. I'm not sure is 2nd case is even valid, to be honest.

diff --git a/asyncssh/config.py b/asyncssh/config.py
index 5007ef7..14e8003 100644
--- a/asyncssh/config.py
+++ b/asyncssh/config.py
@@ -469,7 +469,7 @@ class SSHClientConfig(SSHConfig):
     _conditionals = {'host', 'match'}
     _no_split = {'proxycommand', 'remotecommand'}
     _percent_expand = {'CertificateFile', 'IdentityAgent',
-                       'IdentityFile', 'ProxyCommand', 'RemoteCommand'}
+                       'IdentityFile', 'ProxyCommand', 'RemoteCommand', 'ForwardAgent'}
 
     def __init__(self, last_config: 'SSHConfig', reload: bool,
                  canonical: bool, final: bool, local_user: str,

[ronf]

I looked into OpenSSH behavior around percent and environment variable expansions. From what I can see, OpenSSH will not expand percent tokens which are present in an environment variable value. So, I think your second case is invalid. The first case where the percent tokens are present directly in the SSH config file appears to be supported, though the ssh_config man page doesn't document this. As you said, though, this is easy to handle.

I also took a closer like at the environment variable support in OpenSSH. I can't get the older "$FOO" syntax to work any more. However, "${FOO}" is supported, and can even be mixed with other text in the value. So, I'll need to be a bit more flexible about matching (probably using a regex subber) if I want to support what OpenSSH allows here. It looks like this environment variable support is also available for other variables, so I may break that out into a separate commit.

[ronf]

Ok - I've added percent substitution support (when using ForwardAgent) and done some other cleanup, but I've removed environment variable support for now and will address that with a separate commit. The updated change is available as commit 1d9e5b8 in the "develop" branch.

[ronf]

Environment variable support is now added for config options which currently support percent expansion in commit 63191ed in the "develop" branch. The syntax is "${var_name]", and just like percent expansion there can be multiple substitutions and a mixture of literal text in the option value being expanded.