Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly load openssh-gerenated private keys in SSHHook #16756

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

ashb
Copy link
Member

@ashb ashb commented Jul 1, 2021

When Paramiko loads an openssh-generated RSA private key it would
happily "parse" it as valid a DSS key, only to fail at first use.

This commit fixes the problem in two ways:

  1. It re-orders the list to move DSA to the last format to be tried
    (which is now not widely used)
  2. Attempts to "use" the key by signing some data, causing it to be
    checked early.

Closes #16738


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

When Paramiko loads an openssh-generated RSA private key it would
happily "parse" it as valid a DSS key, only to fail at first use.

This commit fixes the problem in two ways:

1. It re-orders the list to move DSA to the last format to be tried
   (which is now not widely used)
2. Attempts to "use" the key by signing some data, causing it to be
   checked early.
'rsa': paramiko.RSAKey,
}
# List of classes to try loading private keys as, ordered (roughly) by most common to least common
_pkey_loaders = (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were never using the keys of this dict, so I changed it to just a list anyway.

paramiko.RSAKey,
paramiko.ECDSAKey,
paramiko.Ed25519Key,
paramiko.DSSKey,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix 1.

key = pkey_class.from_private_key(StringIO(private_key), password=passphrase)
# Test it acutally works. If Paramiko loads an openssh generated key, sometimes it will
# happily load it as the wrong type, only to fail when actually used.
key.sign_ssh_data(b'')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix 2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

# Paramiko behaves differently with OpenSSH generated keys to paramiko
# generated keys, so we need a test one.
# This has been gernerated specifically to put here, it is not otherwise in use
TEST_OPENSSH_PRIVATE_KEY = "-----BEGIN OPENSSH " + textwrap.dedent(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string is split here here is to avoid the "no private keys" precommit check.

@ashb ashb requested review from kaxil and potiuk July 1, 2021 13:10
@ashb
Copy link
Member Author

ashb commented Jul 1, 2021

/cc @ecerulm

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jul 1, 2021
@github-actions
Copy link

github-actions bot commented Jul 1, 2021

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@ashb ashb merged commit 7777d4f into apache:main Jul 1, 2021
@ashb ashb deleted the sshhook-dsa-key-to-permissive branch July 1, 2021 18:32
@anto155
Copy link

anto155 commented Mar 29, 2022

While using rsa key in pem format, ssh hook seem to expect ed25519 type of key as highlighted. rsa keys were working fine earlier and started having issue from last few months. Is there something recently changed?

Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/airflow/models/taskinstance.py", line 984, in _run_raw_task
result = task_copy.execute(context=context)
File "/usr/local/lib/python3.7/site-packages/airflow/contrib/operators/s3_to_sftp_operator.py", line 81, in execute
sftp_client = ssh_hook.get_conn().open_sftp()
File "/usr/local/lib/python3.7/site-packages/airflow/contrib/hooks/ssh_hook.py", line 194, in get_conn
client.connect(**connect_kwargs)
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/client.py", line 446, in connect
passphrase,
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/client.py", line 766, in _auth
raise saved_exception
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/client.py", line 679, in _auth
key_filename, pkey_class, passphrase
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/client.py", line 588, in _key_from_filepath
key = klass.from_private_key_file(key_path, password)
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/pkey.py", line 249, in from_private_key_file
key = cls(filename=filename, password=password)
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/ed25519key.py", line 58, in init
pkformat, data = self._read_private_key("OPENSSH", f)
File "/usr/local/airflow/.local/lib/python3.7/site-packages/paramiko/pkey.py", line 355, in _read_private_key
"encountered {} key, expected {} key".format(keytype, tag)
paramiko.ssh_exception.SSHException: encountered RSA key, expected OPENSSH key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSHHook will not work if extra.private_key is a RSA key
4 participants