bito-code-review[bot] commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3383918132
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,131 @@ 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:
+ # The server presents a different key than expected: verification fails.
+ expected_key = paramiko.RSAKey.generate(2048)
+ presented_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(_authorized_key(expected_key))
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = presented_key
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+ mock_create_connection.assert_called_once()
+ transport.close.assert_called_once()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+def test_verify_host_key_connect_failure_raises(
+ mock_create_connection: Mock,
+) -> None:
+ # A bounded TCP connect failure surfaces as a host-key verification error.
+ manager = _make_manager(strict=False)
+ server_key = paramiko.RSAKey.generate(2048)
+ tunnel = _ssh_tunnel(_authorized_key(server_key))
+
+ mock_create_connection.side_effect = OSError("connection refused")
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_unset_non_strict_skips(mock_transport_cls: Mock) ->
None:
+ # Back-compat: no expected key + strict checking off => no verification at
all.
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(None)
+
+ manager._verify_host_key(tunnel) # should not raise
+
+ mock_transport_cls.assert_not_called()
+
+
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_unset_strict_raises(mock_transport_cls: Mock) -> None:
+ # Fail-closed: no expected key + strict checking on => reject.
+ manager = _make_manager(strict=True)
+ tunnel = _ssh_tunnel(None)
+
+ with pytest.raises(SSHTunnelHostKeyVerificationError):
+ manager._verify_host_key(tunnel)
+
+ mock_transport_cls.assert_not_called()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match_ignores_comment_and_whitespace(
+ mock_transport_cls: Mock,
+ mock_create_connection: Mock,
+) -> None:
+ # The stored key may carry a trailing comment and extra whitespace.
+ server_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ stored = f" {_authorized_key(server_key)} user@host "
+ tunnel = _ssh_tunnel(stored)
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = server_key
+
+ manager._verify_host_key(tunnel) # should not raise
+
Review Comment:
<!-- Bito Reply -->
The added assertions correctly address the concern by ensuring that the TCP
connection and Transport initialization are explicitly verified with the
expected parameters. This change prevents potential regressions where
verification might be silently bypassed, and it aligns the test with the
established testing pattern used elsewhere in the file.
**tests/unit_tests/extensions/ssh_test.py**
```
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()
```
##########
superset/databases/api.py:
##########
@@ -568,7 +573,11 @@ def put(self, pk: int) -> Response:
exc_info=True,
)
return self.response_422(message=str(ex))
- except (SSHTunnelingNotEnabledError, SSHTunnelDatabasePortError) as ex:
+ except (
+ SSHTunnelingNotEnabledError,
+ SSHTunnelDatabasePortError,
+ SSHTunnelHostKeyVerificationError,
+ ) as ex:
Review Comment:
<!-- Bito Reply -->
The user's update correctly addresses the identified issue by removing the
unreachable exception handler. Since the `UpdateDatabaseCommand` does not
trigger the SSH host-key verification path, removing
`SSHTunnelHostKeyVerificationError` from the `put()` handler is appropriate and
improves code cleanliness.
**superset/databases/api.py**
```
except (
SSHTunnelingNotEnabledError,
SSHTunnelDatabasePortError,
- SSHTunnelHostKeyVerificationError,
) as ex:
return self.response_400(message=str(ex))
```
--
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]
