Uh oh!
There was an error while loading. Please reload this page.
Remove DSSKey references from SSH provider, bump paramiko to 4.0+ - #69191
Remove DSSKey references from SSH provider, bump paramiko to 4.0+#69191joshuabvarghese wants to merge 9 commits into
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
…from _host_key_mappings
…pes-paramiko>=4.0.0"
b47444a to
22eacaaCompare
aaron-y-chen
left a comment
There was a problem hiding this comment.
Thanks for contributing! Could you help fix the CI error? I also found a regression issue that might need to be fixed :)
| _host_key_mappings = { | ||
| "rsa": paramiko.RSAKey, | ||
| "dss": paramiko.DSSKey, |
There was a problem hiding this comment.
I think we should add a handler or document to avoid regression. For example, if users configured ssh-dss in host_key and then upgrade the ssh provider, this can fail with a KeyError.
e.g.
A connection like this:
Connection(
conn_id="ssh_with_dss_host_key",
host="localhost",
conn_type="ssh",
extra=json.dumps({"host_key": "ssh-dss AAAAB3NzaC1kc3MAAAAA"}),
)would fail here because dss has been removed from _host_key_mappings:
airflow/providers/ssh/src/airflow/providers/ssh/hooks/ssh.py
Lines 229 to 232 in 4d56e6c
There was a problem hiding this comment.
Also, add a small regression test might be better I think :)
There was a problem hiding this comment.
Thanks for catching this, @nailo2c you're right, the host_key lookup would have hit a bare KeyError for anyone with an ssh-dss host key configured. Good catch.
Looking at this more, I noticed #69669 also addresses this issue and goes further — it fixes the equivalent regression in the SFTP hook too (which mine doesn't touch), handles more host key type variants (ecdsa-sha2-nistp*, legacy ssh-ecdsa, whitespace/tab-separated formats), and caps the paramiko version bound more carefully.
I'll close this one in favor of that PR rather than duplicate effort. Thanks again for the review!
closes: #54079
Summary
paramiko.DSSKeyreferences fromSSHHook(_pkey_loadersand_host_key_mappings)paramikominimum to>=4.0.0in thesshandsftpproviders (previously capped at<4.0.0in Limit paramiko to< 4.0.0till we remove DSS support #54078 as a stopgap)types-paramikoindevel-common/pyproject.tomlto drop the matching<4.0.0cap added in Restrict typing-paramiko to < 4 as we do with paramiko #54173Motivation
paramiko4.0 removedDSSKey(DSA) support entirely for security reasons. #54078 temporarily cappedparamiko<4.0.0to keep CI green, and #54173 did the same fortypes-paramiko, both explicitly deferring the real fix to this issue.This PR does the follow-through: drops the DSA loader/host-key-mapping code paths so the provider no longer references a class that doesn't exist in modern paramiko, and lifts both version caps.
Migration
Users with SSH/SFTP connections configured to use a DSA (DSS) private key will need to generate a new RSA, ECDSA, or Ed25519 key (e.g.
ssh-keygen -t ed25519) and update the connection'skey_file/private_keyaccordingly. This is called out in the newsfragment and in the connection docs.Testing
pytest providers/ssh/tests/unit/ssh/hooks/test_ssh.pypytest providers/sftp/tests/unit/sftp/hooks/test_sftp.pyparamiko.DSSKeyno longer exists underparamiko>=4.0.0(confirms the prior code path would have raisedAttributeErrorat import time, matching the CI failure in Limit paramiko to< 4.0.0till we remove DSS support #54078)^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, inairflow-core/newsfragments.