(superset) branch feat/ssh-tunnel-host-key-checking updated: fix(ssh_tunnel): pin verified host key on the tunnel connection (close TOCTOU)

[ru...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rusackas pushed a commit to branch feat/ssh-tunnel-host-key-checking
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/feat/ssh-tunnel-host-key-checking by this push:
     new 4c391a4508f fix(ssh_tunnel): pin verified host key on the tunnel connection (close TOCTOU)
4c391a4508f is described below

commit 4c391a4508fcac33c292a9110f51c1978833d453
Author: Evan <ev...@preset.io>
AuthorDate: Mon Jun 15 10:36:52 2026 -0700

    fix(ssh_tunnel): pin verified host key on the tunnel connection (close TOCTOU)
    
    The pre-flight probe and sshtunnel.open_tunnel establish separate
    connections, so verifying only the probe left a TOCTOU gap (DNS
    re-resolution, selective interception). Pass the parsed expected key as
    ssh_host_key so paramiko verifies the host key on the same transport that
    carries traffic. The probe is kept for the precise early error and the
    strict-no-key policy (sshtunnel swallows paramiko's host-key SSHException
    into a generic 'could not establish session' error).
    
    Co-Authored-By: Claude Opus 4.8 <no...@anthropic.com>
---
 superset/extensions/ssh.py              | 24 +++++++++++++---
 tests/unit_tests/extensions/ssh_test.py | 49 +++++++++++++++++++++++++++++++--
 2 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/superset/extensions/ssh.py b/superset/extensions/ssh.py
index df7a71e9e07..8777a5368c4 100644
--- a/superset/extensions/ssh.py
+++ b/superset/extensions/ssh.py
@@ -79,7 +79,7 @@ class SSHManager:
             port=server.local_bind_port,
         )
 
-    def _verify_host_key(self, ssh_tunnel: "SSHTunnel") -> None:
+    def _verify_host_key(self, ssh_tunnel: "SSHTunnel") -> "paramiko.PKey | None":
         """
         Opt-in defense-in-depth: verify the SSH server's host key before opening the
         tunnel, to resist man-in-the-middle attacks (paramiko's ``Transport`` does no
@@ -95,6 +95,10 @@ class SSHManager:
           enabled, fail closed and raise.
         - If no expected key is set and strict checking is disabled, do nothing,
           preserving existing (unverified) behavior.
+
+        :returns: the parsed expected host key when one is configured (so the caller
+            can pin it on the tunnel's own connection), or ``None`` when no key is
+            configured.
         """
         expected_raw = ssh_tunnel.server_host_key
 
@@ -106,7 +110,7 @@ class SSHManager:
                         "expected server host key is configured for this tunnel."
                     )
                 )
-            return
+            return None
 
         try:
             expected_key = _parse_authorized_key(expected_raw)
@@ -154,6 +158,8 @@ class SSHManager:
                 )
             )
 
+        return expected_key
+
     def create_tunnel(
         self,
         ssh_tunnel: "SSHTunnel",
@@ -167,8 +173,10 @@ class SSHManager:
         if not port:
             raise SSHTunnelDatabasePortError()
 
-        # Opt-in host-key verification runs before the tunnel is opened.
-        self._verify_host_key(ssh_tunnel)
+        # Opt-in host-key verification runs before the tunnel is opened. It returns
+        # the parsed expected key (or None) so we can also pin it on the tunnel's own
+        # connection below.
+        expected_host_key = self._verify_host_key(ssh_tunnel)
 
         params = {
             "ssh_address_or_host": (ssh_tunnel.server_address, ssh_tunnel.server_port),
@@ -178,6 +186,14 @@ class SSHManager:
             "debug_level": logging.getLogger("flask_appbuilder").level,
         }
 
+        if expected_host_key is not None:
+            # Pin the expected key on the tunnel's own connection, so paramiko verifies
+            # the host that actually carries traffic on the same transport. The probe
+            # above and the tunnel open separate connections, so verifying only the
+            # probe would leave a TOCTOU gap (DNS re-resolution, selective
+            # interception); pinning here closes it.
+            params["ssh_host_key"] = expected_host_key
+
         if ssh_tunnel.password:
             params["ssh_password"] = ssh_tunnel.password
         elif ssh_tunnel.private_key:
diff --git a/tests/unit_tests/extensions/ssh_test.py b/tests/unit_tests/extensions/ssh_test.py
index 62a91bd1c42..d3e22d0e71e 100644
--- a/tests/unit_tests/extensions/ssh_test.py
+++ b/tests/unit_tests/extensions/ssh_test.py
@@ -79,7 +79,7 @@ def test_verify_host_key_match(
     transport = mock_transport_cls.return_value
     transport.get_remote_server_key.return_value = server_key
 
-    manager._verify_host_key(tunnel)  # should not raise
+    result = manager._verify_host_key(tunnel)  # should not raise
 
     # The TCP connect is bounded by an explicit timeout, and the resulting
     # socket is handed to Transport.
@@ -89,6 +89,8 @@ def test_verify_host_key_match(
     mock_transport_cls.assert_called_once_with(mock_create_connection.return_value)
     transport.start_client.assert_called_once()
     transport.close.assert_called_once()
+    # The parsed expected key is returned so the caller can pin it on the tunnel.
+    assert result == server_key
 
 
 @patch("superset.extensions.ssh.socket.create_connection")
@@ -133,7 +135,7 @@ def test_verify_host_key_unset_non_strict_skips(mock_transport_cls: Mock) -> Non
     manager = _make_manager(strict=False)
     tunnel = _ssh_tunnel(None)
 
-    manager._verify_host_key(tunnel)  # should not raise
+    assert manager._verify_host_key(tunnel) is None  # should not raise
 
     mock_transport_cls.assert_not_called()
 
@@ -177,6 +179,49 @@ def test_verify_host_key_invalid_expected_raises() -> None:
         manager._verify_host_key(tunnel)
 
 
+@patch("superset.extensions.ssh.sshtunnel.open_tunnel")
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_create_tunnel_pins_verified_host_key(
+    mock_transport_cls: Mock,
+    mock_create_connection: Mock,
+    mock_open_tunnel: Mock,
+) -> None:
+    # When an expected host key is configured and verified, it is also pinned on the
+    # tunnel's own connection (``ssh_host_key``) so paramiko verifies the host that
+    # actually carries traffic on the same transport — closing the probe-vs-tunnel
+    # TOCTOU gap rather than trusting only the pre-flight probe.
+    server_key = paramiko.RSAKey.generate(2048)
+    manager = _make_manager(strict=False)
+    tunnel = _ssh_tunnel(_authorized_key(server_key))
+    tunnel.username = "user"
+    tunnel.password = None
+    tunnel.private_key = None
+
+    mock_transport_cls.return_value.get_remote_server_key.return_value = server_key
+
+    manager.create_tunnel(tunnel, "postgresql://u:p@db:5432/ex")
+
+    _, kwargs = mock_open_tunnel.call_args
+    assert kwargs["ssh_host_key"] == server_key
+
+
+@patch("superset.extensions.ssh.sshtunnel.open_tunnel")
+def test_create_tunnel_without_host_key_does_not_pin(mock_open_tunnel: Mock) -> None:
+    # No expected key configured (non-strict): nothing is pinned, preserving the
+    # prior behavior.
+    manager = _make_manager(strict=False)
+    tunnel = _ssh_tunnel(None)
+    tunnel.username = "user"
+    tunnel.password = None
+    tunnel.private_key = None
+
+    manager.create_tunnel(tunnel, "postgresql://u:p@db:5432/ex")
+
+    _, kwargs = mock_open_tunnel.call_args
+    assert "ssh_host_key" not in kwargs
+
+
 def test_ssh_tunnel_schema_round_trips_server_host_key() -> None:
     # The schema accepts and preserves the public host key field.
     from superset.databases.schemas import DatabaseSSHTunnel