-
Notifications
You must be signed in to change notification settings - Fork 389
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
Fix/additions around hostkey validation #278
Conversation
rdkls
commented
Sep 8, 2018
- Fix for hostkey checking on nonstandard ssh ports
- Move server_key and fingerprint calculation into 'if hostkey_verify' block, since we only need them there
- Add ability to verify remote host against specific hostkey
- Move server_key and fingerprint calculation into 'if hostkey_verify' block, since we only need them there - SSH transport connect() add parameter 'hostkey' for ability to verify remote host against that specific hostkey - Update docs/transport.rst accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question more than a code change request, and one comment on tests.
raise e | ||
# Add the supplied hostkey to my host_keys, to be used in the following .check() | ||
# key_object.get_name() is actually the key type e.g. 'ssh-rsa' | ||
self._host_keys.add(host, key_object.get_name(), key_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the checkin comments says "Add ability to verify remote host against specific hostkey", but the way the code is written means that the provided hostkey will be added to any existing hostkeys? Is that the intent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - will change thanks
@@ -19,6 +19,11 @@ def test_connect_ssh(self, mock_ssh): | |||
manager.connect(host='host') | |||
mock_ssh.assert_called_once_with(host='host') | |||
|
|||
@patch('ncclient.manager.connect_ssh') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this test do more? Like verify a specific, real hostkey?
closing in favour of new PR with different approach |