codeant-ai-for-open-source[bot] commented on code in PR #40139:
URL: https://github.com/apache/superset/pull/40139#discussion_r3245276671
##########
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:
**Suggestion:** This new regression test is added without the corresponding
production change, so it will fail deterministically on the current
`SSHManager.create_tunnel` implementation (which still hard-codes RSA parsing).
Include the SSH key-loader fix in the same PR (or explicitly mark this as
expected-failing in a temporary TDD branch) to avoid breaking CI for the main
test suite. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SSH ed25519 regression test fails on every test run.
- ❌ Full test suite/CI blocked until SSH loader is fixed.
- ⚠️ Prevents merging unrelated changes while test remains failing.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the new regression test
`test_create_tunnel_accepts_ed25519_private_key` in
`tests/unit_tests/extensions/ssh_test.py:55-95` (e.g. `pytest
tests/unit_tests/extensions/ssh_test.py::test_create_tunnel_accepts_ed25519_private_key
-v`).
2. Inside this test, `_make_ed25519_pem()` at
`tests/unit_tests/extensions/ssh_test.py:45-52` generates an OpenSSH-format
ed25519
private key string, which is assigned to `ssh_tunnel.private_key` at
`tests/unit_tests/extensions/ssh_test.py:79-85`.
3. The test patches `sshtunnel.open_tunnel` and calls
`manager.create_tunnel(ssh_tunnel,
...)` in the snippet at `tests/unit_tests/extensions/ssh_test.py:87-90`,
which dispatches
to `SSHManager.create_tunnel` in `superset/extensions/ssh.py:51-80`.
4. In `SSHManager.create_tunnel`, the branch `elif ssh_tunnel.private_key:`
at
`superset/extensions/ssh.py:71-79` executes
`RSAKey.from_private_key(private_key_file,
ssh_tunnel.private_key_password)` at `superset/extensions/ssh.py:75-77`;
this hard-coded
RSA loader attempts to parse the ed25519 key and raises (e.g. "unpack
requires 4 bytes" /
invalid RSA key), causing the test to fail deterministically and preventing
`sshtunnel.open_tunnel` from ever being invoked, which in turn makes the new
test (and
thus CI) red until the production key-loading logic is fixed or the test is
marked xfail.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fextensions%2Fssh_test.py%0A%2A%2ALine%3A%2A%2A%2087%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20This%20new%20regression%20test%20is%20added%20without%20the%20corresponding%20production%20change%2C%20so%20it%20will%20fail%20deterministically%20on%20the%20current%20%60SSHManager.create_tunnel%60%20implementation%20%28which%20still%20hard-codes%20RSA%20parsing%29.%20Include%20the%20SSH%20key-loader%20fix%20in%20the%20same%20PR%20%28or%20explicitly%20mark%20this%20as%20expected-failing%20in%20a%20temporary%20TDD%20branch%29%20to%20avoid%20breaking%20CI%20for%20the%20main%20test%20suite.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20co
ncise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fextensions%2Fssh_test.py%0A%2A%2ALine%3A%2A%2A%2087%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20This%20new%20regression%20test%20is%20added%20without%20the%20corresponding%20production%20change%2C%20so%20it%20will%20fail%20deterministically%20on%20the%20current%20%60SSHManager.create_tunnel%60%20implementation%20%28which%20still%20hard-codes%20RSA%20parsing%29.%20Include%20the%20SSH%20key-loader%20fix%20in%20the%20same%20PR%20%28or%20
explicitly%20mark%20this%20as%20expected-failing%20in%20a%20temporary%20TDD%20branch%29%20to%20avoid%20breaking%20CI%20for%20the%20main%20test%20suite.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 87:90
**Comment:**
*Incomplete Implementation: This new regression test is added without
the corresponding production change, so it will fail deterministically on the
current `SSHManager.create_tunnel` implementation (which still hard-codes RSA
parsing). Include the SSH key-loader fix in the same PR (or explicitly mark
this as expected-failing in a temporary TDD branch) to avoid breaking CI for
the main test suite.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40139&comment_hash=c6a2755ca6658f23a468c5cf3d0a95fa161e64e243aad94a5142605855141593&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40139&comment_hash=c6a2755ca6658f23a468c5cf3d0a95fa161e64e243aad94a5142605855141593&reaction=dislike'>👎</a>
--
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]
