SSH Host key checking #280

rdkls · 2018-09-12T03:19:56Z

before initiating connection, set preferred_key (types) to be those that we already possess for the host (if any)
when checking known_hosts, cater for ports other than 22
if hostkey specified, remote host /must/ match that hostkey

Formatting:

reformat function definition for easier reading of arguments
remove use of temporary single-character variables for clarity sake

- before initiating connection, set preferred_key (types) to be those that we already possess for the host (if any) - when checking known_hosts, cater for ports other than 22 - if hostkey specified, remote host /must/ match that hostkey Formatting: - reformat function definition for easier reading of arguments - remove use of temporary single-character variables for clarity sake

…date_onconnect

coveralls · 2018-09-12T03:24:02Z

Coverage increased (+6.1%) to 65.404% when pulling c04c05c on rdkls:feat/hostkey_validate_onconnect into 92d8932 on ncclient:master.

coveralls · 2018-09-12T03:24:02Z

Coverage decreased (-0.7%) to 65.221% when pulling 841063b on rdkls:feat/hostkey_validate_onconnect into a06d73d on ncclient:master.

coveralls · 2018-09-12T03:24:02Z

Coverage increased (+6.1%) to 65.404% when pulling c04c05c on rdkls:feat/hostkey_validate_onconnect into 92d8932 on ncclient:master.

coveralls · 2018-09-12T03:24:02Z

Coverage increased (+6.1%) to 65.404% when pulling c04c05c on rdkls:feat/hostkey_validate_onconnect into 92d8932 on ncclient:master.

coveralls · 2018-09-12T03:24:02Z

Coverage increased (+6.1%) to 65.404% when pulling c04c05c on rdkls:feat/hostkey_validate_onconnect into 92d8932 on ncclient:master.

…client into feat/hostkey_validate_onconnect

rdkls · 2018-09-12T06:45:03Z

Hi @einarnn here is completely redone version of host-key checking.
Much better version which checks the specified host_key MUST match the remote host's.
Also fixes for the existing ~/.ssh/known_hosts checking
and some tests with actual ssh host keys as requested
Kindly review? Thanks - Nick

glennmatthews

I like the general approach, but have a few concerns.

glennmatthews · 2018-09-13T14:31:01Z

ncclient/transport/ssh.py

@@ -312,6 +328,8 @@ def connect(self, host, port=830, timeout=None, unknown_host_cb=default_unknown_

        *hostkey_verify* enables hostkey verification from ~/.ssh/known_hosts

+        *hostkey* only connect when server presents a public hostkey matching this
+


hostkey is a base64 string, correct? Would be good to mention the datatype.

Funny you mention, I had the same in an older version but reverted here. Happy to revert again. Thanks for quick review, I'm going through addressing today ...

glennmatthews · 2018-09-13T14:31:55Z

ncclient/transport/ssh.py


        """Connect via SSH and initialize the NETCONF session. First attempts the publickey authentication method and then password authentication.

        To disable attempting publickey authentication altogether, call with *allow_agent* and *look_for_keys* as `False`.

        *host* is the hostname or IP address to connect to

-        *port* is by default 830, but some devices use the default SSH port of 22 so this may need to be specified
+        *port* is by default PORT_NETCONF_DEFAULT, but some devices use the default SSH port of 22 (PORT_SSH_DEFAULT) so this may need to be specified


Personally I'm not a fan of this change - it takes something clear and makes it less so (now I have to look elsewhere to find out what PORT_NETCONF_DEFAULT is).

It's generally good practice to avoid magic numbers. You're welcome to keep (can revert) but I recommend avoiding

glennmatthews · 2018-09-13T14:34:56Z

ncclient/transport/ssh.py

+            if not hostkey_obj:
+                # We've tried all known host key types and haven't found which one to use - bail
+                raise e
+            self._transport._preferred_keys = [hostkey_obj.get_name()]


If we have both self._host_keys and the hostkey parameter, we set self._transport._preferred_keys once above and then re-set it again here, losing the previously set value. That feels incorrect to me. Should the first if statement be something like if self._host_keys and not hostkey: instead?

The first 'if' is for when host keys have been set by code above (i.e. ssh_config and we read known_hosts)

However regardless of this - if the hostkey has been specified - it takes precedence.

glennmatthews · 2018-09-13T14:37:52Z

ncclient/transport/ssh.py

-
-        fingerprint = _colonify(hexlify(server_key.get_fingerprint()))
+            # Raise original exception; don't swallow details
+            raise


This is a potentially breaking change for existing users of ncclient who are currently doing "except SSHError" and now need to change to "except paramiko.SSHException". Are we sure this is a desired change?

If this is a desired change, there's no point to keeping a try...except block that does nothing but re-raising the exception - just remove the block entirely instead.

That's actually a good point. What I'll do is revert to SSHError, but put in the detail from paramiko. The reason for this change was that while using the library, I had an error where this failed but the swallowed/rewritten error obfuscated the root cause.

Correct there's no point functionally in doing so, only in being more explicit in the type of error we're expecting. Personal thing again; more explicit vs verbosity

glennmatthews · 2018-09-13T14:44:42Z

test/unit/test_manager.py

+    def test_connect_ssh_with_hostkey_ed25519(self, mock_ssh):
+        hostkey = 'AAAAC3NzaC1lZDI1NTE5AAAAIIiHpGSf8fla6tCwLpwshvMGmUK+B/0v5CsRu+5v4uT7'
+        manager.connect(host='host', hostkey=hostkey)
+        mock_ssh.assert_called_once_with(host='host', hostkey=hostkey)


Testing feels a little thin here.

Do we have the ability to check that _preferred_keys is set correctly in each case?

Test for host key lookup when port is non-default?

Negative test for SSHUnknownHostError when key doesn't match?

Yep more tests would be great. I feel like what's there, is in line with precedents though. Might have a look later but are you OK with this PR without @glennmatthews ?

rdkls · 2018-09-15T01:07:59Z

Thanks for quick reviews @glennmatthews @einarnn
Please let me know if you'd like further amendments.

Also may be of interest - if you guys are using Ansible too - I'll be submitting PR to the netconf module to support specifying ssh hostkey in inventory, passing through to ncclient and using this functionality.

The larger point of the work - is orchestrating device management at scale - once device is provisioned, putting its dns/ip along with host key in ansible inventory - and being more certain that orchestration/config is talking to the same device (was quite surprised this wasn't widely done as with ma-ssl...)
Nick

glennmatthews

Looks sane to me; will let @einarnn make the final determination.

…al port number appended) - move into if hostkey_verify, so that setting this False won't check, even when hostkey_b64 supplied - when checking host keys, use the existing is_known_host bool and raise Exception code (neater, fewer code paths)

- hostkey_b64 object creation - raise new (instead of reusing) exception with friendlier message - import NetconfFramingError (used on line 210 in _parse11 - remove unused ncclient.xml_ import - pep8 spacing

rdkls · 2018-09-18T00:22:00Z

Thanks @glennmatthews - also implemented your suggestion and made key types for negotiation more explicit

rdkls added 3 commits September 12, 2018 13:14

Merge branch 'master' into feat/hostkey_validate_onconnect

75c5738

Merge remote-tracking branch 'upstream/master' into feat/hostkey_vali…

7775eaf

…date_onconnect

rdkls added 2 commits September 12, 2018 13:24

Merge branch 'feat/hostkey_validate_onconnect' of github.com:rdkls/nc…

ac25c4f

…client into feat/hostkey_validate_onconnect

more tests

924a4ac

rdkls closed this Sep 12, 2018

rdkls reopened this Sep 12, 2018

remove breaking test for incorrect host key padding

fa8b893

glennmatthews reviewed Sep 13, 2018

View reviewed changes

rdkls added 2 commits September 15, 2018 10:28

change hostkey var name to be explicit on type hostkey_b64

a5b9c5e

revert to raising SSHError

18b5c9f

rdkls mentioned this pull request Sep 17, 2018

SSH Host Key Pinning and Validation ansible/ansible#45707

Closed

glennmatthews approved these changes Sep 17, 2018

View reviewed changes

rdkls added 5 commits September 18, 2018 08:47

fix: comments & whitespace tidy

c2a4ac9

check known_hosts only if hostkey_b64 not specified

fae4537

remove unused import

3e3f892

Housekeeping & Cleaning

b148452

- hostkey_b64 object creation - raise new (instead of reusing) exception with friendlier message - import NetconfFramingError (used on line 210 in _parse11 - remove unused ncclient.xml_ import - pep8 spacing

Merge branch 'master' into feat/hostkey_validate_onconnect

841063b

einarnn merged commit 26b79d1 into ncclient:master Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH Host key checking #280

SSH Host key checking #280

rdkls commented Sep 12, 2018

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018 •

edited

Loading

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018

rdkls commented Sep 12, 2018

glennmatthews left a comment

glennmatthews Sep 13, 2018

rdkls Sep 15, 2018

glennmatthews Sep 13, 2018

rdkls Sep 15, 2018

glennmatthews Sep 13, 2018

rdkls Sep 15, 2018

glennmatthews Sep 13, 2018

rdkls Sep 15, 2018

glennmatthews Sep 13, 2018

rdkls Sep 15, 2018

rdkls commented Sep 15, 2018 •

edited

Loading

glennmatthews left a comment

rdkls commented Sep 18, 2018

		@@ -312,6 +328,8 @@ def connect(self, host, port=830, timeout=None, unknown_host_cb=default_unknown_

		hostkey_verify enables hostkey verification from ~/.ssh/known_hosts

		hostkey only connect when server presents a public hostkey matching this

SSH Host key checking #280

SSH Host key checking #280

Conversation

rdkls commented Sep 12, 2018

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018 • edited Loading

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018

coveralls commented Sep 12, 2018

rdkls commented Sep 12, 2018

glennmatthews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdkls commented Sep 15, 2018 • edited Loading

glennmatthews left a comment

Choose a reason for hiding this comment

rdkls commented Sep 18, 2018

coveralls commented Sep 12, 2018 •

edited

Loading

rdkls commented Sep 15, 2018 •

edited

Loading