Repository navigation
Verify SSH host keys by default in SSH and SFTP hooks - #73419
Conversation
51973c0 to
292ce20
Compare
|
Addressed all three points in
Six tests added, two per touched test file. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
|
Two additional pre-existing issues surfaced while reviewing this change. I do not consider them blockers for this PR, but they are worth tracking:
Drafted-by: Codex (GPT-5); reviewed by @shubhamraj-git before posting |
|
Addressed the second round in
The two pre-existing issues you flagged separately are tracked in #73584 (
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
91efd74 to
bbd8eef
Compare
|
Thanks for digging into these. The first one is already fixed on main by #73593 — The second is real and still there: the concurrent workers are built from Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
bbd8eef to
3048224
Compare
|
Follow-up for the second issue: #74209 fixes #74208 — the concurrent SFTP workers are now copies of the parent hook, so they keep Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
dabla
left a comment
There was a problem hiding this comment.
The default flip is consistent across SSHHook, SSHHookAsync, SFTPHook and SFTPHookAsync, and the precedence (constructor argument, then connection extra, then verify) is validated on the effective value in both the sync and the async hooks. CI is green on 30482241ff.
Two new points. The branch conflicts with main in both hook files (#73647 and #74211), and the #73647 side needs more than a mechanical resolution, see comment [1]. And on the async hooks the new default path ends in a FileNotFoundError when the known hosts file does not exist, see comments [2] and [3].
One observation without an inline comment: SSHHookAsync gets no no_host_key_check constructor argument while SFTPHookAsync does. Nothing in the repo needs it (SSHRemoteJobOperator builds both of its hooks from the connection only) and known_hosts="none" still works as an opt-out, so this is an API asymmetry rather than a defect.
I checked the other in-repo consumers of the changed default (Amazon, Google and Azure SFTP transfers, Teradata BTEQ/TPT, SFTPSensor, SFTPClientPool, SSHOperator): all build their hooks from the connection only, so they follow the documented migration path. ComputeEngineSSHHook does not call SSHHook.__init__ and keeps its own host_key_policy, so it is unaffected.
The SFTP filesystem backend has always verified host keys by default, while SSHHook, SSHHookAsync and SFTPHookAsync did not: `no_host_key_check` defaulted to true, so any connection that did not say otherwise got paramiko's AutoAddPolicy, or `known_hosts=None` on the asyncssh paths. This brings the hooks in line with the filesystem backend. This is a breaking change: a connection to a host with no entry in the known hosts file is now refused unless verification is disabled explicitly. Three further changes were needed to make that default workable: * SSHHook gains a `no_host_key_check` constructor argument. The setting could previously only come from a Connection extra, so code building the hook directly had no way to opt out at all -- which also made the documented migration path unusable for those callers. * `ignore_hostkey_verification` is now honoured as a deprecated alias for `no_host_key_check`. Nothing read it: connections setting it were relying on the permissive default rather than on the extra, and would otherwise have broken with no working replacement. * store_directory_concurrently and retrieve_directory_concurrently built their worker hooks with only `ssh_conn_id`, discarding the rest of the parent hook's configuration. That was invisible while the default was permissive; the workers now inherit the host key setting. Generated-by: Claude Opus 5
…key checks `SFTPToS3Operator` and `S3ToSFTPOperator` build their own `SFTPHook` from `sftp_conn_id` and take no hook argument, so these tests could not reuse the `SSHHook` they already configure. They rely on the bundled `ssh_default` connection, a bare `ssh://localhost` with no extras, which host key verification now refuses. Overrides that conn id for the duration of each class with the same host and `no_host_key_check=true` -- the migration step any deployment relying on the previous default has to take. Generated-by: Claude Opus 5
Connections migrating through the deprecated `ignore_hostkey_verification` alias only kept working on the synchronous hook, so a deferrable task could submit successfully and then fail once its trigger reconnected through `SSHHookAsync` or `SFTPHookAsync`. A connection carrying both `host_key` and `no_host_key_check` was rejected before the constructor argument was applied, which left no way to resolve that combination for a connection the caller cannot edit. The SFTP provider now passes a constructor argument that SSH provider 6.0.x does not accept, so its minimum SSH provider version has to move with the next release. Generated-by: Claude Opus 5
A no_host_key_check set on the hook given to a deferrable SFTPOperator was lost at deferral, so a transfer that worked synchronously failed in the trigger once host keys became verified by default. The async hooks also wrote their inline known_hosts entry for the connection's host rather than the host actually connected to, and emitted an entry asyncssh cannot use for a bare base64 host_key -- both undermining the host_key migration path this change recommends. Generated-by: Claude Opus 5
Generated-by: Claude Opus 5
An absent default known_hosts file should still produce an untrusted-host result, rather than a filesystem error. Keep explicit known_hosts paths unchanged and carry the effective host-key policy into concurrent SFTP workers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3048224 to
5355c9a
Compare
|
All comments resolved @dabla |
dabla
left a comment
There was a problem hiding this comment.
All three points from my review of 30482241ff are addressed on 5355c9a9, and CI is green on the rebased head (71 checks passed, 24 skipped).
- The #73647 conflict is resolved the way I suggested:
_build_worker_hook()now passesno_host_key_check=self.no_host_key_checkas a constructor argument instead of assigning it afterwards, so a parent built withno_host_key_check=Falseagainst a connection that sets bothhost_keyandno_host_key_check=trueproduces workers that construct without raising.test_concurrent_transfer_passes_effective_host_key_policy_to_workercovers exactly that connection. - Both async hooks gained
_should_use_known_hosts(): whenknown_hostsis still the expanded default and the file is absent, the key is left out of the asyncssh config, so a triggerer without~/.ssh/known_hostsgetsHostKeyNotVerifiableinstead ofFileNotFoundError. Explicit paths, inlinehost_keyentries (bytes) and"none"are untouched.test_get_conn_omits_missing_default_known_hostsand the two adjustedSFTPHookAsyncconnection tests pin the behaviour on each side.
The five threads from @shubhamraj-git that have no reply in-thread are all answered by the code: the trigger carries no_host_key_check and SFTPOperator forwards its hook's effective value (with the TaskDeferred round-trip test), the inline known-hosts entry uses self.host or conn.host, a bare base64 host_key is normalised to ssh-rsa, the connection docs describe the opt-out as accepting unknown keys, and the async SSH tests use real Connection objects.
I re-checked the in-repo consumers against the rebased base (73a4ffa3a5): the Amazon, Google and Azure SFTP transfers, Teradata BTEQ/TPT, SFTPSensor and SSHOperator still build their hooks from the connection only, so the documented migration path applies to them. SFTPOperator.execute guarantees self.sftp_hook is set before the deferral branch reads no_host_key_check from it. The breaking-change note sits directly under the "Changelog" header in both providers, which is what the file header asks for and what the Amazon, Google and Edge changelogs do.
Two observations, neither needs a change: SSHHookAsync still has no no_host_key_check constructor argument (known_hosts="none" remains the opt-out, and the SSH changelog correctly lists only SSHHook/SFTPHook); and the PR description's "Worker hooks inherit configuration" paragraph and the 404-test count predate the rebase onto #73647, so the squash commit message may want a refresh.
The SFTP filesystem backend (
providers/sftp/.../fs/sftp.py) has always verified host keys by default.SSHHook,SSHHookAsyncandSFTPHookAsyncdid not —no_host_key_checkdefaulted totrue, so any connection that didn't say otherwise got paramiko'sAutoAddPolicy, orknown_hosts=Noneon the asyncssh paths. This brings the hooks in line with the filesystem backend.Breaking change
A connection to a host with no entry in the known hosts file is now refused. Existing behaviour can be kept by any of:
host_keyon the connectionno_host_key_checkconnection extra totrueno_host_key_check=Trueto the hook constructorThe constructor argument takes precedence over the connection extra.
Three supporting changes
no_host_key_checkconstructor argument onSSHHook. The setting could only come from a Connection extra, so callers building the hook directly had no opt-out at all — the migration path above would not have worked for them.ignore_hostkey_verificationis now a deprecated alias. Nothing read this extra. Connections setting it were relying on the permissive default, not on the setting, so flipping the default would have broken them with no working replacement. It now works and emits anAirflowProviderDeprecationWarning.Worker hooks inherit configuration.
store_directory_concurrentlyandretrieve_directory_concurrentlybuilt their per-worker hooks asSFTPHook(ssh_conn_id=self.ssh_conn_id), dropping everything else configured on the parent. Harmless while the default was permissive; now the workers inherit the host key setting.Test plan
breeze testing providers-tests --test-type "Providers[ssh,sftp]"— 404 passed, 0 failedruff check,ruff format --check, and the pre-commit hook set all cleanWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions