rusackas commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3401918770
##########
superset/commands/database/ssh_tunnel/exceptions.py:
##########
@@ -75,3 +75,9 @@ class SSHTunnelMissingCredentials(CommandInvalidError,
SSHTunnelError): # noqa:
class SSHTunnelInvalidCredentials(CommandInvalidError, SSHTunnelError): #
noqa: N818
message = _("Cannot have multiple credentials for the SSH Tunnel")
+
+
+class SSHTunnelHostKeyVerificationError(CommandInvalidError, SSHTunnelError):
+ message = _(
+ "The SSH server host key could not be verified against the expected
key."
+ )
Review Comment:
Added a docstring.
##########
superset/migrations/versions/2026-06-01_10-00_78a40c08b4be_add_server_host_key_to_ssh_tunnels.py:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add server_host_key to ssh_tunnels
+
+Adds a nullable ``server_host_key`` column to the ``ssh_tunnels`` table. It
stores the
+expected SSH server host key in authorized-key form (e.g. "ssh-ed25519
AAAA...") so
+operators can opt in to verifying the SSH server's host key before a tunnel is
opened.
+This is a public key and is stored in plaintext (not encrypted). The column is
+nullable, so existing tunnels are unaffected.
+
+Revision ID: 78a40c08b4be
+Revises: 31dae2559c05
+Create Date: 2026-06-01 10:00:00.000000
+
+"""
+
+import sqlalchemy as sa
+
+from superset.migrations.shared.utils import add_columns, drop_columns
+
+# revision identifiers, used by Alembic.
+revision = "78a40c08b4be"
+down_revision = "31dae2559c05"
+
+
+def upgrade():
+ add_columns(
+ "ssh_tunnels",
+ sa.Column("server_host_key", sa.Text(), nullable=True),
+ )
+
+
+def downgrade():
+ drop_columns("ssh_tunnels", "server_host_key")
Review Comment:
Added `-> None` to both.
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
Review Comment:
Done!
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
+ app = Mock()
+ app.config = {
+ "SSH_TUNNEL_MAX_RETRIES": 2,
+ "SSH_TUNNEL_LOCAL_BIND_ADDRESS": "127.0.0.1",
+ "SSH_TUNNEL_TIMEOUT_SEC": 123.0,
+ "SSH_TUNNEL_PACKET_TIMEOUT_SEC": 321.0,
+ "SSH_TUNNEL_MANAGER_CLASS": "superset.extensions.ssh.SSHManager",
+ "SSH_TUNNEL_STRICT_HOST_KEY_CHECKING": strict,
+ }
+ return SSHManager(app)
+
+
+def _authorized_key(key: paramiko.PKey) -> str:
Review Comment:
Added.
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
+ app = Mock()
+ app.config = {
+ "SSH_TUNNEL_MAX_RETRIES": 2,
+ "SSH_TUNNEL_LOCAL_BIND_ADDRESS": "127.0.0.1",
+ "SSH_TUNNEL_TIMEOUT_SEC": 123.0,
+ "SSH_TUNNEL_PACKET_TIMEOUT_SEC": 321.0,
+ "SSH_TUNNEL_MANAGER_CLASS": "superset.extensions.ssh.SSHManager",
+ "SSH_TUNNEL_STRICT_HOST_KEY_CHECKING": strict,
+ }
+ return SSHManager(app)
+
+
+def _authorized_key(key: paramiko.PKey) -> str:
+ return f"{key.get_name()} {key.get_base64()}"
+
+
+def _ssh_tunnel(server_host_key: str | None) -> Mock:
Review Comment:
Docstring added.
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,141 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
Review Comment:
Turned the comment into a docstring.
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,141 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
+ # The server presents the same key we expect: verification passes.
+ server_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(_authorized_key(server_key))
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = server_key
+
+ 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.
+ mock_create_connection.assert_called_once_with(
+ ("ssh.example.com", 22), timeout=321.0
+ )
+
mock_transport_cls.assert_called_once_with(mock_create_connection.return_value)
+ transport.start_client.assert_called_once()
+ transport.close.assert_called_once()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_mismatch_raises(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
Review Comment:
Same here, done.
##########
superset/databases/api.py:
##########
@@ -569,7 +574,10 @@ def put(self, pk: int) -> Response:
exc_info=True,
)
return self.response_422(message=str(ex))
- except (SSHTunnelingNotEnabledError, SSHTunnelDatabasePortError) as ex:
+ except (
+ SSHTunnelingNotEnabledError,
+ SSHTunnelDatabasePortError,
+ ) as ex:
return self.response_400(message=str(ex))
Review Comment:
Good catch! Added `SSHTunnelHostKeyVerificationError` to the `put` handler
so update matches the create and test-connection paths.
##########
superset/extensions/ssh.py:
##########
@@ -48,6 +79,81 @@ 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
Review Comment:
Yep, that was a gap... `_parse_authorized_key` wraps
`SSHException`/`UnknownKeyType` as `ValueError` so they surface as the
verification error. Added a test for the unknown-type case.
--
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]
