rusackas commented on code in PR #40139:
URL: https://github.com/apache/superset/pull/40139#discussion_r3245497604
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +40,100 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+def _make_ed25519_pem() -> str:
+ """Generate a fresh OpenSSH-format ed25519 private key PEM."""
+ key = Ed25519PrivateKey.generate()
+ return key.private_bytes(
+ encoding=Encoding.PEM,
+ format=PrivateFormat.OpenSSH,
+ encryption_algorithm=NoEncryption(),
+ ).decode()
+
+
+def test_create_tunnel_accepts_ed25519_private_key() -> None:
+ """
+ Regression for #24180: ed25519 SSH keys must be loadable for tunnel
+ setup. The bug surfaces as ``unpack requires 4 bytes`` because the
+ code unconditionally calls ``RSAKey.from_private_key`` regardless of
+ key type, which mis-parses the ed25519 byte stream.
+
+ The Superset UI accepts any key the user pastes, so the fix is to
+ detect the key type (or use ``paramiko.PKey.from_private_key`` once
+ paramiko exposes a polymorphic loader) rather than hard-coding RSA.
+
+ This test exercises ``create_tunnel`` end-to-end with a freshly
+ generated ed25519 key. It mocks ``sshtunnel.open_tunnel`` so the
+ test does not actually open a network connection — only the key
+ parsing path is exercised.
+ """
+ app = Mock()
+ app.config = {
+ "SSH_TUNNEL_LOCAL_BIND_ADDRESS": "127.0.0.1",
+ "SSH_TUNNEL_TIMEOUT_SEC": 10.0,
+ "SSH_TUNNEL_PACKET_TIMEOUT_SEC": 10.0,
+ }
+ manager = SSHManager(app)
+
+ ssh_tunnel = Mock()
+ ssh_tunnel.server_address = "ssh.example.com"
+ ssh_tunnel.server_port = 22
+ ssh_tunnel.username = "tunneluser"
+ ssh_tunnel.password = None
+ ssh_tunnel.private_key = _make_ed25519_pem()
+ ssh_tunnel.private_key_password = None
+
+ with patch("superset.extensions.ssh.sshtunnel.open_tunnel") as mock_open:
+ manager.create_tunnel(
+ ssh_tunnel, "postgresql://user:[email protected]:5432/x"
+ )
Review Comment:
This is intentional — the PR is a TDD-style validation, not a fix. The PR
description spells this out: 'CI green → bug is fixed; merging closes the
issue. CI red → bug is still live.' A failing test is the expected signal, not
a CI break to avoid. Per-PR check failure does not block other PRs (CI runs
per-PR), so the broader test suite is not blocked. The hardcoded RSAKey path
will be addressed in a separate fix PR; this one's job is to make the
regression visible and lock it in once fixed.
--
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]
