codeant-ai-for-open-source[bot] commented on code in PR #40139:
URL: https://github.com/apache/superset/pull/40139#discussion_r3416612061
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -69,6 +95,154 @@ def test_ssh_tunnel_timeout_setting() -> None:
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.
+ """
+ manager = _make_manager()
+ ssh_tunnel = _make_ssh_tunnel(_make_ed25519_pem())
+
+ with patch("superset.extensions.ssh.sshtunnel.open_tunnel") as mock_open:
+ manager.create_tunnel(
+ ssh_tunnel, "postgresql://user:[email protected]:5432/x"
+ )
+
+ # Key-type-agnostic loader must produce a paramiko PKey usable as ssh_pkey.
+ assert mock_open.called, "open_tunnel was never invoked — key parsing
aborted"
+ forwarded_pkey = mock_open.call_args.kwargs["ssh_pkey"]
+ assert isinstance(forwarded_pkey, Ed25519Key)
+
+
+def test_create_tunnel_accepts_rsa_private_key_unchanged() -> None:
+ """
+ Companion to test_create_tunnel_accepts_ed25519_private_key: pin the
+ backward-compatible RSA path so a fix for ed25519 doesn't regress the
+ historically-supported RSA case. Uses a freshly generated RSA key in
+ OpenSSH format.
+ """
+ rsa_pem = (
+ generate_rsa_key(public_exponent=65537, key_size=2048)
+ .private_bytes(
+ encoding=Encoding.PEM,
+ format=PrivateFormat.OpenSSH,
+ encryption_algorithm=NoEncryption(),
+ )
+ .decode()
+ )
+
+ manager = _make_manager()
+ ssh_tunnel = _make_ssh_tunnel(rsa_pem)
+
+ with patch("superset.extensions.ssh.sshtunnel.open_tunnel") as mock_open:
+ manager.create_tunnel(
+ ssh_tunnel, "postgresql://user:[email protected]:5432/x"
+ )
+
+ assert mock_open.called, "open_tunnel was never invoked — RSA key parsing
aborted"
+ assert isinstance(mock_open.call_args.kwargs["ssh_pkey"], RSAKey)
+
+
+def test_create_tunnel_accepts_ecdsa_private_key() -> None:
+ """
+ Companion to the ed25519 and RSA tests: verify ECDSAKey (the third type
+ in _SSH_KEY_TYPES) is reachable by _load_private_key. Uses NIST P-256,
+ the most common ECDSA curve in practice.
+ """
+ ecdsa_pem = (
+ ec.generate_private_key(ec.SECP256R1())
+ .private_bytes(
+ encoding=Encoding.PEM,
+ format=PrivateFormat.OpenSSH,
+ encryption_algorithm=NoEncryption(),
+ )
+ .decode()
+ )
+
+ manager = _make_manager()
+ ssh_tunnel = _make_ssh_tunnel(ecdsa_pem)
+
+ with patch("superset.extensions.ssh.sshtunnel.open_tunnel") as mock_open:
+ manager.create_tunnel(
+ ssh_tunnel, "postgresql://user:[email protected]:5432/x"
+ )
+
+ assert mock_open.called, "open_tunnel was never invoked — ECDSA key
parsing aborted"
+ assert mock_open.call_args.kwargs["ssh_pkey"] is not None
Review Comment:
**Suggestion:** The ECDSA regression test only checks that `ssh_pkey` is
non-null, so it will still pass if `_load_private_key` incorrectly falls
through and returns the wrong key class (for example, an RSA key object from
permissive parsing). Tighten the assertion to require an actual `ECDSAKey`
instance so this test catches the exact regression it is meant to prevent.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ECDSA regression test may not detect wrong key type.
- ⚠️ Future `_load_private_key` regressions could ship undetected.
- ⚠️ SSH tunnel ECDSA handling less robustly guarded by tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe the ECDSA regression test
`test_create_tunnel_accepts_ecdsa_private_key` in
`tests/unit_tests/extensions/ssh_test.py:167-192`. It generates an ECDSA PEM
using
`ec.generate_private_key(ec.SECP256R1())` and calls
`SSHManager.create_tunnel`, patching
`superset.extensions.ssh.sshtunnel.open_tunnel`.
2. Note that the test only asserts `mock_open.called` and that
`mock_open.call_args.kwargs["ssh_pkey"] is not None` at
`tests/unit_tests/extensions/ssh_test.py:191-192`; it does not assert that
`ssh_pkey` is
an `ECDSAKey` instance.
3. Inspect `_load_private_key` and `_SSH_KEY_TYPES` in
`superset/extensions/ssh.py:50-81`.
`_SSH_KEY_TYPES` is `(Ed25519Key, ECDSAKey, RSAKey)` and `_load_private_key`
returns the
first key class whose `from_private_key` call succeeds, with RSA documented
as the most
permissive parser.
4. If a future regression changes `_SSH_KEY_TYPES` (for example,
accidentally removing
`ECDSAKey` or reordering it after `RSAKey` in
`superset/extensions/ssh.py:54`), an ECDSA
PEM passed through `SSHManager.create_tunnel`
(`superset/extensions/ssh.py:246-251`) could
be parsed as an `RSAKey` while still being non-`None`. The current test at
`tests/unit_tests/extensions/ssh_test.py:167-192` would continue to pass
because it only
checks non-nullity, failing to catch that `_load_private_key` no longer
returns the
correct `ECDSAKey` type for ECDSA private keys.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c60b45b0c6454a4b90688a86a1edd2a9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c60b45b0c6454a4b90688a86a1edd2a9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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:** 192:192
**Comment:**
*Incomplete Implementation: The ECDSA regression test only checks that
`ssh_pkey` is non-null, so it will still pass if `_load_private_key`
incorrectly falls through and returns the wrong key class (for example, an RSA
key object from permissive parsing). Tighten the assertion to require an actual
`ECDSAKey` instance so this test catches the exact regression it is meant to
prevent.
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=e93ff49e2e4af0f59cd50b1961454a5f7bc63e2856ebcc55de22acfd857b66ec&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40139&comment_hash=e93ff49e2e4af0f59cd50b1961454a5f7bc63e2856ebcc55de22acfd857b66ec&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]
