Introduce support for TLS #556

ivandkhn · 2022-11-07T09:06:59Z

This PR implements TLS as a new transport level for NETCONF.

The biggest motivation for this change is currently ongoing standardization of Time-Sensitive Networking Profile for Industrial Automation: IEC/IEEE 60802. According to the current draft, the only secure transport for NETCONF should be TLS, and SSH is explicitly discouraged.

Codebase changes are as follows:

The main run loop is moved out of SSH class since the same loop could be re-used for TLS implementation. Please let me know if you'd prefer less invasive approach without changing the original ssh.py.
Added new tls.py to handle TLS connections. All the errors are re-raised as custom TLSError to match the behavior of ssh.py. There's also some room for future improvements, e.g. Call Home over TLS is not implemented.
Added unit tests for new class.

I’ve also tested new TLS connectivity on some hardware running netopeer2-server 0.7.2 and 0.7.0.

See #271

nthe · 2022-11-07T10:21:49Z

ncclient/transport/tls.py

+        if certfile is None:
+            raise TLSError('Missing client certificate file')
+
+        ssl_context = SSLContext(PROTOCOL_TLSv1_2)


Hi, it seems like the PROTOCOL_TLSv* are getting deprecated in 3.6.
The docs suggests to use the PROTOCOL_TLS_CLIENT flag.

This change will bump the TLS support to v3.6+ Python only. To keep the support for v2.7, one has to use PROTOCOL_TLS, but that won't work with the v3.10.

My small proposal is to ~~do a conditional import or~~ add another mandatory protocol argument to the TLSSession.connect method.

Link to ssl.PROTOCOL_TLS_CLIENT doc.

Thanks for the tip! I'll address this issue in the next iteration of the PR.

Any chance you'll able to address those changes any time soon?

I would like to help you, but I'm not sure if I can push to (or somehow edit) this pull-request as I'm not maintainer of this repository.

I was hoping to collect all the feedback before making further code changes. @einarnn, could you have a look at the PR?

@ivandkhn , I've started reviewing!

@nthe, I've updated handling of TLS protocol version as you've suggested with a new protocol argument.

nthe · 2022-11-21T08:11:27Z

ncclient/transport/__init__.py

 from ncclient.transport.errors import *

 __all__ = [
    'Session',
    'SessionListener',
    'SSHSession',
+    'TLSSession'


The comma is missing at the end of the line.

nthe · 2022-11-21T08:24:15Z

ncclient/transport/tls.py

+                raise TLSError('CA certificate file not found')
+
+        sock = socket.socket(AF_INET, SOCK_STREAM)
+        ssl_sock = ssl_context.wrap_socket(sock, do_handshake_on_connect=False)


When user chooses to use PROTOCOL_TLS_CLIENT protocol, it automatically enables the hostname checking (sets SSLContext.check_hostname to True). With checking enabled, the wrap_socket method expects server_hostname argument, which is usually same as the host, but might be useful to have it as an separate argument which defaults to host value when not provided (jumphost, or port-forwarding).

Something like

def connect(self, ..., server_hostname=None):

and then

ssl_sock = ssl_context.wrap_socket(sock, do_handshake_on_connect=False, server_hostname=(server_hostname or host)

Here's link to the doc which mentions this behavior.

Ok, added it! This also gave me an idea to introduce check_hostname parameter to disable (or enable) hostname checking in the first place, because before it was explicitly disabled.

einarnn · 2022-11-21T14:04:13Z

@ivandkhn, there are a whole bunch of errors like this:

ImportError: cannot import name 'AF_INET' from 'ssl'

Please tidy those up so we can get a clean run of existing tests. I'll continue the review once all tests run clean.

ivandkhn · 2022-11-22T14:35:35Z

Please tidy those up so we can get a clean run of existing tests.

My bad, wrong module name. The tests run now in my local environment, hope they will run in CI pipeline as well.

nthe · 2022-11-29T06:57:58Z

Hi @einarnn, could we ask you for another round of review?

einarnn · 2022-12-09T14:24:39Z

@nthe, @ivandkhn -- I've created a PR on [ivandkhn:introduce-tls-support](https://github.com/ivandkhn/ncclient/tree/introduce-tls-support) to update the CI checking to run properly. Could you please merge that to your fork? I will continue review this weekend.

einarnn · 2022-12-09T21:54:55Z

@ivandkhn -- could you take a look at the CI results?

https://github.com/ncclient/ncclient/actions/runs/3659536172

There is a failure with Python 3.5, and I would prefer if we were able to continue supporting 3.5 if possible.

I will still try to review the code this weekend.

ivandkhn · 2022-12-10T08:28:32Z

@einarnn I see, I’ll look into that and will fix the problem.

Make run() loop independent of underlying transport so that it can be shared between SSH/TLS implementations. Signed-off-by: Ivan Dakhnenko <ivan.dakhnenko@siemens.com>

RFC 5539 introduced NETCONF-over-TLS back in 2009. Recently, we've seen more interest in using TLS instead of SSH for NETCONF (IEEE 60802). This patch introduces initial implementation of such communication. See ncclient#271, ncclient#421, ncclient#417 Signed-off-by: Ivan Dakhnenko <ivan.dakhnenko@siemens.com>

We use here the same key/certificate test files as in netopeer2: https://github.com/CESNET/netopeer2/tree/v0.7-r2/server/configuration/tls This makes it easier to re-use these files for hardware tests, since they are included in netopeer2-server example configuration. Signed-off-by: Ivan Dakhnenko <ivan.dakhnenko@siemens.com>

ivandkhn · 2022-12-12T10:50:20Z

@einarnn I've adjusted my patches – the issue with Python 3.5 should be resolved.

coveralls · 2023-01-01T12:07:18Z

Coverage: 83.799% (-0.4%) from 84.19% when pulling bb5c012 on ivandkhn:introduce-tls-support into 89df388 on ncclient:master.

einarnn

@ivandkhn, @nthe – Sorry this review has taken so long. Thanks for this contribution!!

nsgbegin · 2023-05-16T10:27:18Z

Hi I'm getting error while running netconf connection w.r.t TLS, it seems TLSSession class is not there in updated version

**ImportError: cannot import name 'TLSSession' from 'ncclient.transport' (/usr/local/lib/python3.8/dist-packages/ncclient/transport/__init__.py)**

please provide updated version or provide ways to resolve this error, thanks

nthe reviewed Nov 7, 2022

View reviewed changes

nthe reviewed Nov 21, 2022

View reviewed changes

ivandkhn force-pushed the introduce-tls-support branch from 41cce4f to 6336e2a Compare November 22, 2022 14:18

ivandkhn requested a review from einarnn November 22, 2022 14:36

Ivan Dakhnenko added 3 commits December 12, 2022 11:44

Move SSHSession run loop into superclass

34894b0

Make run() loop independent of underlying transport so that it can be shared between SSH/TLS implementations. Signed-off-by: Ivan Dakhnenko <ivan.dakhnenko@siemens.com>

ivandkhn force-pushed the introduce-tls-support branch from 1d1a598 to bb5c012 Compare December 12, 2022 10:46

einarnn approved these changes Jan 1, 2023

View reviewed changes

einarnn merged commit 75ec56d into ncclient:master Jan 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce support for TLS #556

Introduce support for TLS #556

ivandkhn commented Nov 7, 2022

nthe Nov 7, 2022 •

edited

ivandkhn Nov 8, 2022

nthe Nov 18, 2022

ivandkhn Nov 18, 2022

einarnn Nov 20, 2022

ivandkhn Nov 22, 2022

nthe Nov 21, 2022

nthe Nov 21, 2022 •

edited

ivandkhn Nov 22, 2022

einarnn commented Nov 21, 2022 •

edited

ivandkhn commented Nov 22, 2022

nthe commented Nov 29, 2022

einarnn commented Dec 9, 2022

einarnn commented Dec 9, 2022

ivandkhn commented Dec 10, 2022

ivandkhn commented Dec 12, 2022

coveralls commented Jan 1, 2023

einarnn left a comment

nsgbegin commented May 16, 2023

Introduce support for TLS #556

Introduce support for TLS #556

Conversation

ivandkhn commented Nov 7, 2022

nthe Nov 7, 2022 • edited

Choose a reason for hiding this comment

ivandkhn Nov 8, 2022

Choose a reason for hiding this comment

nthe Nov 18, 2022

Choose a reason for hiding this comment

ivandkhn Nov 18, 2022

Choose a reason for hiding this comment

einarnn Nov 20, 2022

Choose a reason for hiding this comment

ivandkhn Nov 22, 2022

Choose a reason for hiding this comment

nthe Nov 21, 2022

Choose a reason for hiding this comment

nthe Nov 21, 2022 • edited

Choose a reason for hiding this comment

ivandkhn Nov 22, 2022

Choose a reason for hiding this comment

einarnn commented Nov 21, 2022 • edited

ivandkhn commented Nov 22, 2022

nthe commented Nov 29, 2022

einarnn commented Dec 9, 2022

einarnn commented Dec 9, 2022

ivandkhn commented Dec 10, 2022

ivandkhn commented Dec 12, 2022

coveralls commented Jan 1, 2023

einarnn left a comment

Choose a reason for hiding this comment

nsgbegin commented May 16, 2023

nthe Nov 7, 2022 •

edited

nthe Nov 21, 2022 •

edited

einarnn commented Nov 21, 2022 •

edited