Copilot commented on code in PR #64898:
URL: https://github.com/apache/airflow/pull/64898#discussion_r3066476077
##########
providers/ssh/src/airflow/providers/ssh/hooks/ssh.py:
##########
@@ -224,7 +223,18 @@ def __init__(
if host_key is not None:
if host_key.startswith("ssh-"):
key_type, host_key = host_key.split(None)[:2]
- key_constructor = self._host_key_mappings[key_type[4:]]
+ key_alg = key_type[4:]
+ if key_alg == "dss":
+ raise ValueError(
+ "DSA/DSS host keys are not supported. Paramiko
4.0 removed DSS support; "
+ "use an RSA, ECDSA, or Ed25519 host key and
update the connection `host_key`."
+ )
+ key_constructor = self._host_key_mappings.get(key_alg)
+ if key_constructor is None:
+ raise ValueError(
+ f"Unsupported SSH host key algorithm
{key_alg!r}. "
+ "Supported types are: rsa, ecdsa, ed25519."
+ )
else:
key_constructor = paramiko.RSAKey
Review Comment:
This parsing only recognizes algorithms provided as `ssh-<alg>` and
otherwise defaults to `RSAKey`. In OpenSSH `known_hosts`, ECDSA host keys are
typically `ecdsa-sha2-nistp256` / `ecdsa-sha2-nistp384` / `ecdsa-sha2-nistp521`
(no `ssh-` prefix), so ECDSA host key pinning will be mis-detected and likely
fail. Consider parsing the first token (`key_type`) directly and mapping
supported key types (including `ecdsa-sha2-*`) to `paramiko.ECDSAKey`, while
still rejecting `ssh-dss` explicitly; also update the unsupported-types error
text to list the actual accepted tokens.
##########
providers/ssh/docs/connections/ssh.rst:
##########
@@ -54,7 +54,7 @@ Extra (optional)
* ``no_host_key_check`` - Set to ``false`` to restrict connecting to hosts
with no entries in ``~/.ssh/known_hosts`` (Hosts file). This provides maximum
protection against trojan horse attacks, but can be troublesome when the
``/etc/ssh/ssh_known_hosts`` file is poorly maintained or connections to new
hosts are frequently made. This option forces the user to manually add all new
hosts. Default is ``true``, ssh will automatically add new host keys to the
user known hosts files.
* ``allow_host_key_change`` - Set to ``true`` if you want to allow
connecting to hosts that has host key changed or when you get 'REMOTE HOST
IDENTIFICATION HAS CHANGED' error. This won't protect against
Man-In-The-Middle attacks. Other possible solution is to remove the host entry
from ``~/.ssh/known_hosts`` file. Default is ``false``.
* ``look_for_keys`` - Set to ``false`` if you want to disable searching
for discoverable private key files in ``~/.ssh/``
- * ``host_key`` - The base64 encoded ssh-rsa public key of the host or
"ssh-<key type> <key data>" (as you would find in the ``known_hosts`` file).
Specifying this allows making the connection if and only if the public key of
the endpoint matches this value.
+ * ``host_key`` - The base64 encoded ssh-rsa public key of the host or
"ssh-<key type> <key data>" (as you would find in the ``known_hosts`` file).
Specifying this allows making the connection if and only if the public key of
the endpoint matches this value. Supported key types are ``rsa``, ``ecdsa``,
and ``ed25519``. DSA/DSS (``ssh-dss``) host keys are not supported because
`paramiko 4.0 removed DSS support <https://www.paramiko.org/changelog.html>`__;
generate a new host key and update this field.
Review Comment:
The documented `host_key` format and supported `key type` labels don’t match
common `known_hosts` values for ECDSA (usually
`ecdsa-sha2-nistp256`/`384`/`521`, not `ssh-ecdsa`). Once the code supports the
right tokens, update this line to reflect the real `known_hosts` key type
strings and (optionally) add an example (e.g., `ssh-rsa ...`, `ssh-ed25519
...`, `ecdsa-sha2-nistp256 ...`) so users can copy/paste from `ssh-keyscan`
output without guesswork.
```suggestion
* ``host_key`` - The base64 encoded public key of the host or ``"<key
type> <key data>"`` (as you would find in the ``known_hosts`` file). Specifying
this allows making the connection if and only if the public key of the endpoint
matches this value. Supported ``known_hosts`` key type strings include
``ssh-rsa``, ``ssh-ed25519``, ``ecdsa-sha2-nistp256``, ``ecdsa-sha2-nistp384``,
and ``ecdsa-sha2-nistp521``. For example: ``ssh-rsa AAAA...``, ``ssh-ed25519
AAAA...``, or ``ecdsa-sha2-nistp256 AAAA...``. DSA/DSS (``ssh-dss``) host keys
are not supported because `paramiko 4.0 removed DSS support
<https://www.paramiko.org/changelog.html>`__; generate a new host key and
update this field.
```
##########
providers/ssh/tests/unit/ssh/hooks/test_ssh.py:
##########
@@ -574,6 +574,32 @@ def
test_ssh_connection_with_host_key_extra_with_type(self, ssh_client):
hook.remote_host, "ssh-rsa", hook.host_key
)
+ @mock.patch.object(SSHHook, "get_connection")
+ def test_dss_host_key_in_connection_extra_raises(self,
mock_get_connection):
+ mock_get_connection.return_value = Connection(
+ conn_id="ssh_dss_host_key",
+ conn_type="ssh",
+ host="remote_host",
+ login="user",
+ extra=json.dumps({"host_key": "ssh-dss AAAAB3NzaC1kc3MAAA==",
"no_host_key_check": False}),
+ )
+ with pytest.raises(ValueError, match="DSA/DSS host keys"):
+ SSHHook(ssh_conn_id="ssh_dss_host_key")
+
+ @mock.patch.object(SSHHook, "get_connection")
+ def test_unsupported_host_key_algorithm_raises(self, mock_get_connection):
+ mock_get_connection.return_value = Connection(
+ conn_id="ssh_fake_alg",
+ conn_type="ssh",
+ host="remote_host",
+ login="user",
+ extra=json.dumps(
+ {"host_key": "ssh-fake AAAAB3NzaC1yc2EAAAADAQABAA==",
"no_host_key_check": False}
+ ),
+ )
+ with pytest.raises(ValueError, match=r"Unsupported SSH host key
algorithm 'fake'"):
+ SSHHook(ssh_conn_id="ssh_fake_alg")
Review Comment:
These tests cover the new failure paths, but there’s no unit test asserting
acceptance/selection of supported host key types. Adding a focused test that
validates `host_key` parsing/constructor selection for at least one supported
type (notably an ECDSA `ecdsa-sha2-nistp256` token) would prevent regressions
and would catch the current mis-detection behavior; you can avoid needing real
key material by patching the relevant Paramiko key class/constructor and
asserting it was chosen.
--
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]
