connectors.ssh: honor ConnectTimeout through ProxyJump (#1679)

Author: wowi42

pyproject.toml

@@ -13,7 +13,7 @@ license-files = ["LICENSE.md"]
 requires-python = ">=3.10,<4.0"
 dependencies = [
     "gevent>=1.5",
-    "paramiko>=2.7,<5", # 2.7 (2019) adds OpenSSH key format + Match SSH config
+    "paramiko>=2.11,<5", # 2.11 (2022) adds Transport.open_channel(timeout=...) for ProxyJump timeout (#971)
     "click>2",
     "jinja2>3,<4",
     "python-dateutil>2,<3",

src/pyinfra/connectors/sshuserclient/client.py

@@ -220,13 +220,14 @@ def connect(  # type: ignore[override]
             session = transport.open_session()
             AgentRequestHandler(session)
 
-    def gateway(self, hostname, host_port, target, target_port):
+    def gateway(self, hostname, host_port, target, target_port, timeout=None):
         transport = self.get_transport()
         assert transport is not None, "No transport"
         return transport.open_channel(
             "direct-tcpip",
             (target, target_port),
             (hostname, host_port),
+            timeout=timeout,
         )
 
     def parse_config(
@@ -284,6 +285,12 @@ def parse_config(
         if "port" in host_config:
             cfg["port"] = int(host_config["port"])
 
+        # Respect ``ConnectTimeout`` from ssh_config (issue #971): without this,
+        # paramiko waits on its own default and a ProxyJump hop can hang for
+        # minutes before failing.
+        if "connecttimeout" in host_config and "timeout" not in cfg:
+            cfg["timeout"] = int(host_config["connecttimeout"])
+
         if "serveraliveinterval" in host_config:
             keep_alive = int(host_config["serveraliveinterval"])
 
@@ -298,14 +305,26 @@ def parse_config(
         elif "proxyjump" in host_config:
             hops = host_config["proxyjump"].split(",")
             sock = None
+            # Propagate the target's timeout down so hop connections and the
+            # direct-tcpip channel don't hang forever when the network misbehaves
+            # (issue #971). Individual hops can still override via their own
+            # ``ConnectTimeout`` in ssh_config.
+            target_timeout = cfg.get("timeout")
 
             for i, hop in enumerate(hops):
                 hop_hostname, hop_config = self.derive_shorthand(ssh_config, hop)
                 logger.debug("SSH ProxyJump through %s:%s", hop_hostname, hop_config["port"])
 
+                hop_connect_kwargs = dict(hop_config)
+                if "timeout" not in hop_connect_kwargs and target_timeout is not None:
+                    hop_connect_kwargs["timeout"] = target_timeout
+
                 c = SSHClient()
                 c.connect(
-                    hop_hostname, _pyinfra_ssh_config_file=ssh_config_file, sock=sock, **hop_config
+                    hop_hostname,
+                    _pyinfra_ssh_config_file=ssh_config_file,
+                    sock=sock,
+                    **hop_connect_kwargs,
                 )
 
                 if i == len(hops) - 1:
@@ -314,7 +333,13 @@ def parse_config(
                 else:
                     target, target_config = self.derive_shorthand(ssh_config, hops[i + 1])
 
-                sock = c.gateway(hostname, cfg["port"], target, target_config["port"])
+                sock = c.gateway(
+                    hostname,
+                    cfg["port"],
+                    target,
+                    target_config["port"],
+                    timeout=target_timeout,
+                )
             cfg["sock"] = sock
 
         return (
@@ -354,6 +379,8 @@ def derive_shorthand(ssh_config, host_string):
             "port": base_config.get("port", 22),
             "username": base_config.get("user"),
         }
+        if "connecttimeout" in base_config:
+            config["timeout"] = int(base_config["connecttimeout"])
         config.update(shorthand_config)
 
         return hostname, config

tests/test_connectors/test_sshuserclient.py

@@ -41,6 +41,26 @@
     ForwardAgent yes
 """
 
+SSH_CONFIG_PROXYJUMP_CONNECTTIMEOUT = """
+Host jump
+    HostName jump.example.com
+    User jumpuser
+    ConnectTimeout 7
+
+Host device
+    HostName 10.0.0.170
+    ProxyJump jump
+    ConnectTimeout 5
+    User deviceuser
+"""
+
+SSH_CONFIG_CONNECTTIMEOUT = """
+Host slowhost
+    HostName slow.example.com
+    User slowuser
+    ConnectTimeout 12
+"""
+
 SSH_CONFIG_MULTIPLE_KNOWN_HOSTS = """
 Host 192.168.1.3
     UserKnownHostsFile ~/.ssh/known_hosts ~/.ssh/known_hosts.infra ~/.ssh/known_hosts.webservers
@@ -337,7 +357,51 @@ def test_load_ssh_config_proxyjump(self, fake_gateway, fake_ssh_connect):
             sock=None,
             username="nottestuser",
         )
-        fake_gateway.assert_called_once_with("192.168.1.2", 1022, "192.168.1.2", 1022)
+        fake_gateway.assert_called_once_with("192.168.1.2", 1022, "192.168.1.2", 1022, timeout=None)
+
+    @patch(
+        "pyinfra.connectors.sshuserclient.client.open",
+        mock_open(read_data=SSH_CONFIG_CONNECTTIMEOUT),
+        create=True,
+    )
+    def test_connecttimeout_sets_timeout_kwarg(self):
+        """Regression test for #971: ``ConnectTimeout`` in ssh_config must be
+        propagated so paramiko doesn't hang on its own default."""
+        client = SSHClient()
+        _, config, *_ = client.parse_config("slowhost")
+        assert config.get("timeout") == 12
+
+    @patch(
+        "pyinfra.connectors.sshuserclient.client.open",
+        mock_open(read_data=SSH_CONFIG_PROXYJUMP_CONNECTTIMEOUT),
+        create=True,
+    )
+    @patch(
+        "pyinfra.connectors.sshuserclient.config.open",
+        mock_open(read_data=SSH_CONFIG_PROXYJUMP_CONNECTTIMEOUT),
+        create=True,
+    )
+    @patch("pyinfra.connectors.sshuserclient.SSHClient.connect")
+    @patch("pyinfra.connectors.sshuserclient.SSHClient.gateway")
+    def test_proxyjump_propagates_connecttimeout(self, fake_gateway, fake_ssh_connect):
+        """Regression test for #971: ``ConnectTimeout`` on both the target and
+        the hop must be honored so neither the hop connect nor the direct-tcpip
+        channel can hang forever."""
+        client = SSHClient()
+
+        _, config, *_ = client.parse_config("device")
+
+        # Target's ConnectTimeout wins for the channel open.
+        assert config.get("timeout") == 5
+        # Hop connect receives the hop's own ConnectTimeout (7s, per its own
+        # ssh_config block) rather than inheriting the target's 5s.
+        fake_ssh_connect.assert_called_once()
+        _, kwargs = fake_ssh_connect.call_args
+        assert kwargs["timeout"] == 7
+        # Channel open (gateway) uses the target's ConnectTimeout.
+        fake_gateway.assert_called_once()
+        _, gw_kwargs = fake_gateway.call_args
+        assert gw_kwargs["timeout"] == 5
 
     @patch("pyinfra.connectors.sshuserclient.client.open", mock_open(), create=True)
     @patch("pyinfra.connectors.sshuserclient.client.ParamikoClient.connect")