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/06/07 21:01:56 UTC
[GitHub] [airflow] malthe opened a new pull request #16314: Add support for non rsa type client host key
malthe opened a new pull request #16314:
URL: https://github.com/apache/airflow/pull/16314
<!--
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/
-->
This adds support for specifying an SSH client host key along with its type, e.g.
```
ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIGTA+qK7VvFL3oZTgbVwFYcp5ZrPiRkPQd8YhVZDH946
```
Previously, an RSA key was required.
A key name (which is optional in the _known hosts_ format) is allowed – but ignored.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe closed pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe closed pull request #16314:
URL: https://github.com/apache/airflow/pull/16314
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-865349770
@potiuk something must have eaten a commit or two. I have pushed those fixes back now – and installed the pre-commit tools :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe edited a comment on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe edited a comment on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-864016126
@potiuk in this case I think there is no particular need for a refactoring; the code is alright as it is – if I was going to suggest changing anything, it would be the ability to provide username, password etc. as arguments to the hook function – rather than relying on the connection alone (that is, I would remove it).
I have seen some hooks use a connection object instead of a connection id and this makes sense I think. It allows you to write out the connection details or look up using `get_connection`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-864367344
@malthe not really :) - static + pylint + some tests are failing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-863979786
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-856704684
The CI has been pretty bad recently. 😞
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-856699721
@uranusjr if you have time to review this one it would be great. I'm not sure why the image building times out though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #16314:
URL: https://github.com/apache/airflow/pull/16314
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-867474971
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-863988505
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on a change in pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on a change in pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#discussion_r647388197
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -159,9 +166,15 @@ def __init__( # pylint: disable=too-many-statements
and str(extra_options["look_for_keys"]).lower() == 'false'
):
self.look_for_keys = False
- if "host_key" in extra_options and self.no_host_key_check is False:
- decoded_host_key = decodebytes(extra_options["host_key"].encode('utf-8'))
- self.host_key = paramiko.RSAKey(data=decoded_host_key)
+ host_key = extra_options.get("host_key")
+ if host_key is not None and self.no_host_key_check is False:
+ if host_key.startswith("ssh-"):
+ key_type, host_key = host_key.split(" ")[:2]
+ key_constructor = _host_key_mappings[key_type[4:]]
Review comment:
@uranusjr I have updated to use `None` instead – nice. Also, I have added a slightly more intrusive change which that if you specify a `host_key` then the system will insist on using that key.
This simplifies the documentation as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-865355214
This time some tests are failing - pylint etc. is good but the unit tests are not happy :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-865349770
@potiuk something must have eaten a commit or two. I have pushed those fixes back now – and installed the pre-commit tools :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-865355214
This time some tests are failing - pylint etc. is good but the unit tests are not happy :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-867401371
@uranusjr green :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#discussion_r647371839
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -159,9 +166,15 @@ def __init__( # pylint: disable=too-many-statements
and str(extra_options["look_for_keys"]).lower() == 'false'
):
self.look_for_keys = False
- if "host_key" in extra_options and self.no_host_key_check is False:
- decoded_host_key = decodebytes(extra_options["host_key"].encode('utf-8'))
- self.host_key = paramiko.RSAKey(data=decoded_host_key)
+ host_key = extra_options.get("host_key")
+ if host_key is not None and self.no_host_key_check is False:
+ if host_key.startswith("ssh-"):
+ key_type, host_key = host_key.split(" ")[:2]
+ key_constructor = _host_key_mappings[key_type[4:]]
Review comment:
Not quite sure about the parsing logic here (is the separator always one single space or should we [split with `None`](https://docs.python.org/3/library/stdtypes.html#str.split) instead? should the split be `host_key.split(" ", 2)` instead? etc.), but otherwise LGTM. We can probably just release this first and fix anything if someone complains 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe closed pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe closed pull request #16314:
URL: https://github.com/apache/airflow/pull/16314
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #16314: Add support for non-RSA type client host key
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #16314:
URL: https://github.com/apache/airflow/pull/16314#issuecomment-864364899
@potiuk looks ready now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org