codeant-ai-for-open-source[bot] commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3400958144
##########
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:
**Suggestion:** The host-key parsing error handling assumes
`_parse_authorized_key` only raises `ValueError`, but
`paramiko.PKey.from_type_string` can raise Paramiko-specific exceptions (for
example unknown key types). Those exceptions bypass this block, so invalid
configured keys can leak out as generic unexpected failures instead of the
intended `SSHTunnelHostKeyVerificationError` with a clear 4xx-style message.
Catch Paramiko key-parsing exceptions here and wrap them consistently. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Test connection endpoint returns generic error on invalid host key.
- ❌ Database create UI shows generic failure not validation error.
- ⚠️ Host-key misconfigurations harder to troubleshoot for operators.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a database with an SSH tunnel that has `server_host_key` set to
an invalid
authorized-key string (for example, an unsupported key type like `ssh-foo
...`) on the
SSHTunnel model used by that database (referenced via `Database.ssh_tunnel`
in
`superset/models/core.py:24-32`).
2. Call the database test-connection API endpoint, implemented by
`DatabaseRestApi.test_connection` in `superset/databases/api.py:1295-1317`,
providing the
database ID for the misconfigured SSH tunnel so that
`TestConnectionDatabaseCommand(item).run()` is executed.
3. Inside `TestConnectionDatabaseCommand.run` (see
`superset/commands/database/test_connection.py` around lines 150-220), the
command
attempts to connect to the database, which in turn uses
`Database.get_sqla_engine`
(`superset/models/core.py:7-35`); since `self.ssh_tunnel` is set, this calls
`ssh_manager_factory.instance.create_tunnel(ssh_tunnel=self.ssh_tunnel,
sqlalchemy_database_uri=sqlalchemy_uri)` at `superset/models/core.py:26-31`.
4. `SSHManager.create_tunnel` (`superset/extensions/ssh.py:157-190`) invokes
`self._verify_host_key(ssh_tunnel)` at line 170, which calls
`_parse_authorized_key(expected_raw)` at
`superset/extensions/ssh.py:111-116`; if
Paramiko's `PKey.from_type_string` inside `_parse_authorized_key` raises a
Paramiko-specific exception (e.g. for an unknown key type), it is not a
`ValueError` and
therefore bypasses the `except ValueError as ex:` block at lines 113-116,
bubbling up as
an unhandled Paramiko exception instead of being wrapped in
`SSHTunnelHostKeyVerificationError`. This means the error is not caught by
the specific
`except SSHTunnelHostKeyVerificationError` handlers in
`superset/databases/api.py:480-519`
and `superset/databases/api.py:1290-1318`, but instead falls into the
generic exception
handling paths (e.g. `except Exception as ex` in
`superset/commands/database/test_connection.py:22-39`), resulting in a
generic
test-connection failure rather than a clear 4xx-style host-key-validation
error.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3fe16c2339df4cbfb16d656c84b9e2ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3fe16c2339df4cbfb16d656c84b9e2ac&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:** superset/extensions/ssh.py
**Line:** 111:116
**Comment:**
*Api Mismatch: The host-key parsing error handling assumes
`_parse_authorized_key` only raises `ValueError`, but
`paramiko.PKey.from_type_string` can raise Paramiko-specific exceptions (for
example unknown key types). Those exceptions bypass this block, so invalid
configured keys can leak out as generic unexpected failures instead of the
intended `SSHTunnelHostKeyVerificationError` with a clear 4xx-style message.
Catch Paramiko key-parsing exceptions here and wrap them consistently.
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%2F40673&comment_hash=4d3982be38c3db7d20926afa692aa7f9073cdbdd2c9a9b065230b99686fbac54&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=4d3982be38c3db7d20926afa692aa7f9073cdbdd2c9a9b065230b99686fbac54&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]
