rusackas commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3342879132
##########
superset/extensions/ssh.py:
##########
@@ -48,6 +78,68 @@ def build_sqla_url(
port=server.local_bind_port,
)
+ def _verify_host_key(self, ssh_tunnel: "SSHTunnel") -> 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
+ known-hosts checking by default).
+
+ Behavior:
+
+ - If the tunnel declares an expected ``server_host_key``, connect to
the SSH
+ server, read the host key it presents, and compare. On mismatch (or
if the
+ expected key cannot be parsed) raise
+ :class:`SSHTunnelHostKeyVerificationError`.
+ - If no expected key is set and
``SSH_TUNNEL_STRICT_HOST_KEY_CHECKING`` is
+ enabled, fail closed and raise.
+ - If no expected key is set and strict checking is disabled, do
nothing,
+ preserving existing (unverified) behavior.
+ """
+ expected_raw = ssh_tunnel.server_host_key
+
+ if not expected_raw or not expected_raw.strip():
+ if self.strict_host_key_checking:
+ raise SSHTunnelHostKeyVerificationError(
+ message=(
+ "SSH_TUNNEL_STRICT_HOST_KEY_CHECKING is enabled but no
"
+ "expected server host key is configured for this
tunnel."
+ )
+ )
+ return
+
+ try:
+ expected_key = _parse_authorized_key(expected_raw)
+ except ValueError as ex:
+ raise SSHTunnelHostKeyVerificationError(
+ message=f"The configured expected server host key is invalid:
{ex}"
+ ) from ex
+
+ transport = paramiko.Transport(
+ (ssh_tunnel.server_address, ssh_tunnel.server_port)
+ )
Review Comment:
Good catch — the bare paramiko.Transport((host, port)) constructor
establishes the socket synchronously with no timeout, so
start_client(timeout=...) only governs the SSH handshake after the TCP connect
already succeeded. On an unreachable host this can block for the full OS-level
TCP timeout. I will switch this to build the socket with
socket.create_connection((host, port), timeout=SSH_TIMEOUT) and hand that
socket to Transport so the connect phase is bounded too.
##########
superset/extensions/ssh.py:
##########
@@ -60,6 +152,10 @@ def create_tunnel(
port = url.port or get_default_port(backend)
if not port:
raise SSHTunnelDatabasePortError()
+
+ # Opt-in host-key verification runs before the tunnel is opened.
+ self._verify_host_key(ssh_tunnel)
Review Comment:
Partly valid. SSHTunnelHostKeyVerificationError is a CommandInvalidError,
and when create_tunnel raises it during a connection attempt it is caught by
the broad except in test_connection and re-raised through the normal DBAPIError
path, so the HTTP status is still 400 rather than a 500 — it does not surface
as an unhandled error. What is lost is the specific host-key message, which
gets flattened into the generic connection-failure text. I will add explicit
handling so the verification error keeps its own message in the
create/update/test-connection paths rather than wiring it as a hard failure;
thanks for flagging the message-fidelity gap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
