shubhamraj-git commented on code in PR #73419:
URL: https://github.com/apache/airflow/pull/73419#discussion_r4068803815
##########
providers/sftp/docs/changelog.rst:
##########
@@ -27,6 +27,21 @@
Changelog
---------
+.. warning::
+ The ``no_host_key_check`` connection extra now defaults to ``false``. A
connection to a host that has
+ no entry in the known hosts file is refused unless host key verification is
disabled explicitly or a
+ ``host_key`` is supplied in the connection's extra field.
+
+ Deployments that relied on the previous default can keep the earlier
behaviour by adding the host key
+ to the known hosts file, supplying ``host_key`` on the connection, setting
the ``no_host_key_check``
+ connection extra to ``true``, or -- when building ``SSHHook``/``SFTPHook``
directly rather than from a
+ connection -- passing the new ``no_host_key_check=True`` constructor
argument. The constructor argument
Review Comment:
The new constructor opt-out does not survive
`SFTPOperator(deferrable=True)`. For example, when a user supplies
`SFTPHook(ssh_conn_id="my_sftp", no_host_key_check=True)`, the operator
serializes only the connection ID into `SFTPTransferTrigger`; the trigger then
creates a fresh `SFTPHookAsync` using the new strict default. Before this PR,
losing that setting was hidden because the async hook also defaulted to
skipping verification. After this change, the supplied hook can work
synchronously but fail after deferral.
Could we propagate the effective host-key policy through the trigger into
`SFTPHookAsync`? A regression test could create a connection without host-key
extras, supply `SFTPHook(..., no_host_key_check=True)` to a deferrable
operator, capture `TaskDeferred`, and verify that the trigger's async hook
passes `known_hosts=None` to `asyncssh.connect()`.
---
Drafted-by: Codex (GPT-5); reviewed by @shubhamraj-git before posting
##########
providers/ssh/docs/changelog.rst:
##########
@@ -27,6 +27,21 @@
Changelog
---------
+.. warning::
+ The ``no_host_key_check`` connection extra now defaults to ``false``. A
connection to a host that has
+ no entry in the known hosts file is refused unless host key verification is
disabled explicitly or a
+ ``host_key`` is supplied in the connection's extra field.
+
+ Deployments that relied on the previous default can keep the earlier
behaviour by adding the host key
+ to the known hosts file, supplying ``host_key`` on the connection, setting
the ``no_host_key_check``
Review Comment:
This behavior predates the PR, but it affects the `host_key` migration path
now recommended here. Both async hooks construct the inline known-hosts entry
using `conn.host`, while `_get_conn()` connects to the explicit constructor
`host` when provided. With connection host `connection.example` and override
`override.example`, AsyncSSH therefore finds no trusted key for the effective
endpoint. This can make `SSHRemoteJobOperator` submit successfully through the
synchronous hook and then fail in its trigger; deferrable SFTP has the same
issue.
Could both async hooks use the same effective hostname for `known_hosts` and
`asyncssh.connect()`? Please add tests where `conn.host` differs from the
constructor host and assert that both the connection target and the hostname
embedded in `known_hosts` use the override.
---
Drafted-by: Codex (GPT-5); reviewed by @shubhamraj-git before posting
##########
providers/sftp/docs/changelog.rst:
##########
@@ -27,6 +27,21 @@
Changelog
---------
+.. warning::
+ The ``no_host_key_check`` connection extra now defaults to ``false``. A
connection to a host that has
+ no entry in the known hosts file is refused unless host key verification is
disabled explicitly or a
+ ``host_key`` is supplied in the connection's extra field.
+
+ Deployments that relied on the previous default can keep the earlier
behaviour by adding the host key
+ to the known hosts file, supplying ``host_key`` on the connection, setting
the ``no_host_key_check``
Review Comment:
This behavior also predates the PR, but it makes the advertised `host_key`
migration route unreliable for the documented bare-base64 RSA form. The
synchronous hook interprets a bare value as RSA, while the async hooks generate
`server.example <base64>`. AsyncSSH requires `server.example ssh-rsa <base64>`;
a focused probe confirmed that the bare entry loads zero matching host keys.
Could both async hooks normalize bare values to `ssh-rsa`, consistently with
the synchronous hook? The regression tests should use a valid bare RSA key and
assert that the generated entry is `f"server.example ssh-rsa
{key_data}".encode()`. The existing SFTP parametrized test currently asserts
the unusable two-field representation.
---
Drafted-by: Codex (GPT-5); reviewed by @shubhamraj-git before posting
--
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]
