You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/01 13:08:55 UTC
[GitHub] [airflow] ashb opened a new pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
ashb opened a new pull request #16756:
URL: https://github.com/apache/airflow/pull/16756
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.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#issuecomment-872384050
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#discussion_r662274215
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -63,13 +63,13 @@ class SSHHook(BaseHook):
:type keepalive_interval: int
"""
- # key type name to paramiko PKey class
- _default_pkey_mappings = {
- 'dsa': paramiko.DSSKey,
- 'ecdsa': paramiko.ECDSAKey,
- 'ed25519': paramiko.Ed25519Key,
- 'rsa': paramiko.RSAKey,
- }
+ # List of classes to try loading private keys as, ordered (roughly) by most common to least common
+ _pkey_loaders = (
+ paramiko.RSAKey,
+ paramiko.ECDSAKey,
+ paramiko.Ed25519Key,
+ paramiko.DSSKey,
Review comment:
Fix 1.
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -357,15 +357,17 @@ def _pkey_from_private_key(self, private_key: str, passphrase: Optional[str] = N
Creates appropriate paramiko key for given private key
:param private_key: string containing private key
- :return: `paramiko.PKey` appropriate for given key
+ :return: ``paramiko.PKey`` appropriate for given key
:raises AirflowException: if key cannot be read
"""
- allowed_pkey_types = self._default_pkey_mappings.values()
- for pkey_type in allowed_pkey_types:
+ for pkey_class in self._pkey_loaders:
try:
- key = pkey_type.from_private_key(StringIO(private_key), password=passphrase)
+ 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'')
Review comment:
Fix 2.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#discussion_r662274826
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -473,6 +475,47 @@ def test_ssh_connection_with_no_host_key_where_no_host_key_check_is_false(self,
assert ssh_client.return_value.connect.called is True
assert ssh_client.return_value.get_host_keys.return_value.add.called is False
+ def test_openssh_private_key(self):
+ # 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(
Review comment:
The string is split here here is to avoid the "no private keys" precommit check.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] anto155 commented on pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
anto155 commented on pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#issuecomment-1081968344
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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#discussion_r662274088
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -63,13 +63,13 @@ class SSHHook(BaseHook):
:type keepalive_interval: int
"""
- # key type name to paramiko PKey class
- _default_pkey_mappings = {
- 'dsa': paramiko.DSSKey,
- 'ecdsa': paramiko.ECDSAKey,
- 'ed25519': paramiko.Ed25519Key,
- 'rsa': paramiko.RSAKey,
- }
+ # List of classes to try loading private keys as, ordered (roughly) by most common to least common
+ _pkey_loaders = (
Review comment:
We were never using the keys of this dict, so I changed it to just a list anyway.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#issuecomment-872248457
/cc @ecerulm
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16756:
URL: https://github.com/apache/airflow/pull/16756#discussion_r662431552
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -357,15 +357,17 @@ def _pkey_from_private_key(self, private_key: str, passphrase: Optional[str] = N
Creates appropriate paramiko key for given private key
:param private_key: string containing private key
- :return: `paramiko.PKey` appropriate for given key
+ :return: ``paramiko.PKey`` appropriate for given key
:raises AirflowException: if key cannot be read
"""
- allowed_pkey_types = self._default_pkey_mappings.values()
- for pkey_type in allowed_pkey_types:
+ for pkey_class in self._pkey_loaders:
try:
- key = pkey_type.from_private_key(StringIO(private_key), password=passphrase)
+ 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'')
Review comment:
Nice!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #16756: Correctly load openssh-gerenated private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #16756:
URL: https://github.com/apache/airflow/pull/16756
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org