extmod/modssl_mbedtls: Add cert verification callback. #12259

felixdoerre · 2023-08-18T13:37:30Z

the update of mbedtls has activated MBEDTLS_SSL_KEEP_PEER_CERTIFICATE. This results in the old getpeercert function not working anymore. On top, storing the certificate for the whole duration of the connection is wasting RAM.

This PR enables access to the certificate through a callback so it does not have to be stored if it is not needed, an can be inspected and analyzed in python while the connection is established, optionally aborting the connection.

fixes #5835

github-actions · 2023-08-18T13:47:12Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1424 +0.171% standard[incl +672(data)]
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

codecov · 2023-08-18T13:56:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5a68e82) 98.36% compared to head (aaba1d8) 98.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12259      +/-   ##
==========================================
- Coverage   98.36%   98.36%   -0.01%     
==========================================
  Files         159      159              
  Lines       21093    21069      -24     
==========================================
- Hits        20748    20724      -24     
  Misses        345      345

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

felixdoerre · 2023-12-18T22:50:02Z

@dpgeorge I've rebased and updated this PR to match the conflicting changes from #13181.

dpgeorge · 2023-12-18T23:11:25Z

This seems like a pretty nice feature to have. But... it's not CPython compatible. We are really trying to keep these modules implementing a subset of CPython behaviour, and this is quite a difference to CPython.

Does CPython provide any way to react to an incoming connection with a callback?

To break with CPython, the way forward would be to follow the ideas of the deflate module, which is a MicroPython specific module and then CPython compatible gzip and zlib modules are provided on top of / implemented using deflate.

So we could define a new module, eg tls, in which we can have whatever API we want. And then implement the ssl module as a simple Python wrapper around tls which is CPython compatible.

Actually, in the case of TLS support, it's a pretty advanced feature that most users don't want to worry about. Instead a user would see a higher-level interface like asyncio.start_server(), or aiohttp. So it's likely that a user never uses the ssl module directly. In that case we can replace it with a custom tls module, and then add features like the one proposed here.

Note that all the hard work we have done on the ssl module so far won't be lost. We'd simply rename ssl to tls, and provide a very simple Python ssl.py wrapper around tls. This is anyway what CPython does, it implements ssl.py in terms of the built-in _ssl library.

felixdoerre · 2023-12-19T15:19:48Z

Yes, cpython does not have such a feature: python/cpython#75425
Maybe also, because it's lets prudent, because getpeercert is always available.

(Maybe as a quick sidenote why I want this feature: I want to have a TLS-Client (around mqtt) that verifies the server certificate, but instead of supplying a CA certificate, I want to pin the certificate and verify its hash in the callback. This usecase does not feel so niche to me.)

Thinking about the code as it exists a bit more critically in terms of compatibility, the current ssl-package is pretty far from being a subset of cpython (I've tested with cpython 3.10.2):

SSLContext.set_ciphers() micropython: accepts a list of IANA-Ciphersuite names; cpython: expects a single string indicating an openssl cipher preference list
SSLContext.get_ciphers() micropython: outputs a list of IANA-Ciphersuite names; cpython: outputs a dictionary with multiple fields per selected cipher
ssl.wrap_socket many keywords don't match with the cpython variant, but this function is luckily deprecated
ssl.MBEDTLS_VERSION does not exist in cpython
SSLContext.load_cert_chain allows byte objects in micropython. This is also explicitly documented.
verify_mode for clients defaults to VERIFY_NONE for backwards compatibility reasons, differing from cpython

I understand why all those differences are there, and don't find them unreasonable, but still they are differences from the cpython behavior.

The change from this PR is arguably very compatible (in the sense, that nearly all cpython programs that worked previously will still work with this change), even more than many of the other noted differences, as adding an additional optional KV-parameter and property can be easily ignored.

Abstracting the cpython compatible subset-interface would mean:

no get/set ciphers
we should probably leave out ssl.wrap_socket as that is deprecated and later removed in cpython
load_cert_chain only accepts file names
verify_mode for client defaults to CERT_REQUIRED.

Replacing the current micropython ssl-module with this would break compatibility, is that intended?

Carglglz · 2023-12-23T16:39:21Z

Note that all the hard work we have done on the ssl module so far won't be lost. We'd simply rename ssl to tls, and provide a very simple Python ssl.py wrapper around tls. This is anyway what CPython does, it implements ssl.py in terms of the built-in _ssl library.

That was my idea when I started porting SSLContext (see the outdated micropython/micropython-lib#697), so if there is no firmware size concerns, it may be the way to go.

felixdoerre · 2023-12-25T14:08:31Z

So, I am looking at the tests, cpython, the differences between mbedtls and axtls and starting see more and more cpython/micropython differences. I also noted the linked package in micropython-lib before.

So I am starting to wonder how to balance these goals against each other:

keep (full) compatibility with the current micropython interface
increase/reach compatibility with a cpython-subset interface, by removing/hiding features that are not fully compatible.

Looking a bit at the state of compatibility, here is how I would "sort" features:

cpython compatible

SSLContext.wrap_socket, with server_side and server_hostname
SSLContext.load_verify_locations with cafile and cadata
SSLContext.load_cert_chain with certfile and keyfile only accepting paths
SSLContext.verify_mode taking CERT_NONE, CERT_OPTIONAL, CERT_REQUIRED defaulting to CERT_REQUIRED for client contexts, and CERT_NONE for server contexts.
(Wrapped Socket) setblocking

micropython specific

SSLContext.wrap_socket with do_handshake_on_connect (this behaves slightly different from cpython, so I would move it here.
SSLContext.load_cert_chain with certfile and keyfile also accepting bytes
SSLContext.get_ciphers/SSLContext.set_ciphers
ssl.MBEDTLS_VERSION (and maybe add AXTLS_VERSION?)
(and the verification callback)

micropython legacy compat

"micropython specific" + ssl.wrap_socket
override the SSLContext.verify_mode default to CERT_NONE.

As next steps I would suggest the following:

Move the implementation as-is to tls (or _ssl to mimic the current python behavior), removing ssl.wrap_socket
Implement a thin wrapper to provide "micropython legacy compat", that does not hide internals, but adds ssl.wrap_socket, to keep compatibility for now.
Document any non-cpython compatible behavior in ssl as deprecated (and/or remove it from the documentation)

After enough time has passed for the deprecation, i.e. code that uses SSLContext.wrap_socket(do_handshake_on_connect=True, or ssl.wrap_socket (like asyncio.start_server()) has had chance to migrate to tls/_ssl, remove the compat-ssl.
At this point we can decide if we want to embed the micropython-lib ssl module

What do you think of that plan? If it sounds good to you, I'd start implementing these changes.

felixdoerre · 2023-12-26T18:06:50Z

I went ahead and implemented what I suggested. Compatibility with the current micropython-api is maintained through a python-implemented module. I've moved get/ciphers to the internal API, as that is pretty new and hopefully not already relied upon too much.

dpgeorge · 2024-01-29T03:30:12Z

Thanks @felixdoerre for updating the PR, it looks reasonable although I need to review it in depth.

Move the implementation as-is to tls (or _ssl to mimic the current python behavior),

I think we should stay away from _ssl as a name because it clashes with CPython's module of that name and obviously does not have the same methods/classes/API as the CPython version (eg it may confuse tests that try to import _ssl, which would pass on both CPython and MicroPython).

I think tls is a good name, and we can then promote this tls module as the way to do SSL/TLS in MicroPython.

As for the ssl.py wrapper, that should probably go in micropython-lib, like, eg, gzip.py wrapper around the deflate module.

Carglglz · 2024-01-30T17:00:44Z

extmod/modtls_mbedtls.c

@@ -91,24 +92,6 @@ STATIC mp_obj_t ssl_socket_make_new(mp_obj_ssl_context_t *ssl_context, mp_obj_t
 /******************************************************************************/
 // Helper functions.

-STATIC mp_obj_t read_file(mp_obj_t self_in) {


@felixdoerre why would you delete this? I thought this was better (i.e. less code size/memory) than reading the files in ssl.py module? Have you compared both implementations?

I didn't until now, because for me the comparison was "have it implement in c" vs "don't have it in c", as the goal is to only have the tls module and eventually make ssl.py optional.

However I've just now tested it and it seems that the implementation in python is indeed smaller (and handle an error in f.read correctly, by closing the file):

text data bss dec hex filename 718248 75912 7056 801216 c39c0 build-standard/micropython (as implemented in this PR) 718568 75944 7056 801568 c3b20 build-standard/micropython (with the file read logic moved back to C)

Then I guess it makes sense to move it to ssl.py, thanks for testing it 👍🏼

Good to know that it's smaller in Python!

Carglglz · 2024-01-30T19:13:12Z

@felixdoerre Could you add a test to multi_net to test the cert callback? (these tests are run in CI unix builds and it is where coverage result comes from)

dpgeorge · 2024-01-31T23:51:16Z

Link to corresponding micropython-lib changes: micropython/micropython-lib#793

dpgeorge · 2024-01-31T23:56:44Z

docs/library/ssl.rst

@@ -13,7 +13,7 @@ facilities for network sockets, both client-side and server-side.
 Functions
 ---------

-.. function:: ssl.wrap_socket(sock, server_side=False, keyfile=None, certfile=None, cert_reqs=CERT_NONE, cadata=None, server_hostname=None, do_handshake=True)
+.. function:: ssl.wrap_socket(sock, server_side=False, key=None, cert=None, cert_reqs=CERT_NONE, cadata=None, server_hostname=None, do_handshake=True)


I will fix this in a separate commit. The docs obviously don't match the code.

Done in ac8e7f7

extmod/modtls_axtls.c

extmod/modtls_mbedtls.c

ports/rp2/boards/manifest.py

ports/unix/variants/nanbox/mpconfigvariant.mk

tests/extmod/ssl_sslcontext_ciphers.py

tests/extmod/ssl_sslcontext_micropython.py

tests/multi_net/sslcontext_certcallback.py

extmod/modssl_mbedtls.c

dpgeorge · 2024-02-01T00:17:12Z

If possible, can you please swap the order of commits around and do the renaming of ssl to tls first, then add the verification callback? That seems more logical to me, and is easier to review.

felixdoerre · 2024-02-01T17:59:38Z

Thanks for the review. I've incorporated all changes, and changed the order of commits.

ports/unix/variants/nanbox/manifest.py

ports/windows/variants/standard/mpconfigvariant.props

tests/multi_net/sslcontext_server_client_ciphers.py

dpgeorge · 2024-02-05T02:30:52Z

extmod/modtls_mbedtls.c

+    if (o->ssl_context->handler == mp_const_none) {
+        return 0;
+    }
+    return mp_obj_get_int(mp_call_function_2(o->ssl_context->handler, mp_obj_new_bytes(crt->raw.p, crt->raw.len), MP_OBJ_NEW_SMALL_INT(depth)));


I suggest using mp_obj_new_bytearray_by_ref(ctr->raw.len, crt->raw.p) (instead of mp_obj_new_bytes). Using a bytearray by reference will save RAM by not creating a copy of the data.

The downside of using bytearray by reference is that the object (memory it points to) becomes invalid after the callback. But, this is an advanced feature and we can document the behaviour. We need to save memory wherever possible.

Also, is it worth passing through the *flags argument?? I'm not sure what this does though.

The flags argument is an in-out parameter that allows the inspection and modification of the "default" verification errors: https://github.com/Mbed-TLS/mbedtls/blob/32c28cebb41a25bb4e18d3a530b59c0e5929a324/include/mbedtls/x509.h#L91

So I thought its more hassle and confusion to pass that through, than it would actually be worth.

extmod/modtls_mbedtls.c

tests/net_hosted/ssl_verify_callback.py

dpgeorge · 2024-02-05T02:45:59Z

I'm not sure why the AppVeyor CI is failing. Something to do with micropython-lib not being found. Maybe I need to merge the micropython-lib changes first...

dpgeorge · 2024-02-06T00:26:34Z

extmod/modtls_mbedtls.c

+    if (o->handler == mp_const_none) {
+        return 0;
+    }
+    return mp_obj_get_int(mp_call_function_2(o->handler, mp_obj_new_bytes(crt->raw.p, crt->raw.len), MP_OBJ_NEW_SMALL_INT(depth)));


What about changing the mp_obj_new_bytes() to mp_obj_new_bytearray_by_ref()?

tests/net_hosted/ssl_verify_callback.py

dpgeorge · 2024-02-06T08:48:31Z

extmod/modtls_mbedtls.c

+    if (o->handler == mp_const_none) {
+        return 0;
+    }
+    return mp_obj_get_int(mp_call_function_2(o->handler, mp_obj_new_bytearray_by_ref(crt->raw.len, crt->raw.p), MP_OBJ_NEW_SMALL_INT(depth)));


Actually, thinking about this some more, it's probable even better to use a memoryview, because then it can be made read-only.

This is how it's done for bluetooth.BLE, in the IRQ callback handler.

Eg:

mp_obj_array_t cert; mp_obj_memoryview_init(&cert, 'B', 0, crt->raw.len, crt->raw.len); return mp_obj_get_int(mp_call_function_2(o->handler, MP_OBJ_NEW_FROM_PTR(&cert), MP_OBJ_NEW_SMALL_INT(depth)));

Thanks for updating with this suggestion. Looks good now!

Signed-off-by: Damien George <damien@micropython.org>

To match the mbedtls implementation. Signed-off-by: Felix Dörre <felix@dogcraft.de>

The current `ssl` module has quite a few differences to the CPython implementation. This change moves the MicroPython variant to a new `tls` module and provides a wrapper module for `ssl` (in micropython-lib). Users who only rely on implemented comparible behavior can continue to use `ssl`, while users that rely on non-compatible behavior should switch to `tls`. Then we can make the facade in `ssl` more strictly adhere to CPython. Signed-off-by: Felix Dörre <felix@dogcraft.de>

This is a useful alternative to .getpeercert() when the certificate is not stored to reduce RAM usage. Signed-off-by: Felix Dörre <felix@dogcraft.de>

dpgeorge · 2024-02-07T02:02:57Z

The Windows CI is now passing (it needed an explicit git submodule update --init lib/micropython-lib).

dpgeorge · 2024-02-07T02:20:10Z

Now merged!

Thanks a lot for your efforts on this @felixdoerre .

felixdoerre force-pushed the mbedtls_cert_callback branch from 6f58e67 to 73af61e Compare August 18, 2023 14:07

dpgeorge added the extmod Relates to extmod/ directory in source label Oct 11, 2023

felixdoerre force-pushed the mbedtls_cert_callback branch 4 times, most recently from 28eeb68 to 8bcb476 Compare December 18, 2023 22:36

felixdoerre force-pushed the mbedtls_cert_callback branch 9 times, most recently from 7552a80 to 37fc0b9 Compare December 26, 2023 17:44

felixdoerre force-pushed the mbedtls_cert_callback branch from 37fc0b9 to 27baf04 Compare January 30, 2024 08:46

felixdoerre mentioned this pull request Jan 30, 2024

ssl: restructure micropython interface in a tls module micropython/micropython-lib#793

Merged

felixdoerre force-pushed the mbedtls_cert_callback branch 2 times, most recently from 6b5750b to ec9ffff Compare January 30, 2024 08:56

Carglglz reviewed Jan 30, 2024

View reviewed changes

felixdoerre force-pushed the mbedtls_cert_callback branch 2 times, most recently from 2d0794e to b1b548f Compare January 31, 2024 12:48

dpgeorge reviewed Feb 1, 2024

View reviewed changes

extmod/modssl_mbedtls.c Outdated Show resolved Hide resolved

felixdoerre force-pushed the mbedtls_cert_callback branch 2 times, most recently from 128168c to f9db748 Compare February 1, 2024 12:44

dpgeorge added this to the release-1.23.0 milestone Feb 2, 2024

dpgeorge reviewed Feb 5, 2024

View reviewed changes

ports/unix/variants/nanbox/manifest.py Outdated Show resolved Hide resolved

ports/windows/variants/standard/mpconfigvariant.props Outdated Show resolved Hide resolved

tests/multi_net/sslcontext_server_client_ciphers.py Show resolved Hide resolved

dpgeorge reviewed Feb 5, 2024

View reviewed changes

felixdoerre force-pushed the mbedtls_cert_callback branch 2 times, most recently from 7dd847a to abfdf73 Compare February 5, 2024 18:21

dpgeorge reviewed Feb 6, 2024

View reviewed changes

tests/net_hosted/ssl_verify_callback.py Outdated Show resolved Hide resolved

felixdoerre force-pushed the mbedtls_cert_callback branch from abfdf73 to 72125e2 Compare February 6, 2024 08:07

dpgeorge reviewed Feb 6, 2024

View reviewed changes

felixdoerre force-pushed the mbedtls_cert_callback branch from 72125e2 to 951f713 Compare February 6, 2024 17:24

dpgeorge force-pushed the mbedtls_cert_callback branch from 951f713 to f23e4b9 Compare February 7, 2024 01:33

dpgeorge and others added 5 commits February 7, 2024 12:58

github/workflows: Initialise micropython-lib submodule for windows CI.

ff73683

Signed-off-by: Damien George <damien@micropython.org>

lib/micropython-lib: Update submodule to latest.

c68462d

Signed-off-by: Damien George <damien@micropython.org>

extmod/modssl_axtls: Add SSLContext.load_cert_chain().

f8f1f29

To match the mbedtls implementation. Signed-off-by: Felix Dörre <felix@dogcraft.de>

extmod/modtls_mbedtls: Implement cert verification callback for mbedtls.

aaba1d8

This is a useful alternative to .getpeercert() when the certificate is not stored to reduce RAM usage. Signed-off-by: Felix Dörre <felix@dogcraft.de>

dpgeorge force-pushed the mbedtls_cert_callback branch from bac56d5 to aaba1d8 Compare February 7, 2024 02:02

dpgeorge merged commit aaba1d8 into micropython:master Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extmod/modssl_mbedtls: Add cert verification callback. #12259

extmod/modssl_mbedtls: Add cert verification callback. #12259

felixdoerre commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

felixdoerre commented Dec 18, 2023

dpgeorge commented Dec 18, 2023

felixdoerre commented Dec 19, 2023 •

edited

Loading

Carglglz commented Dec 23, 2023

felixdoerre commented Dec 25, 2023 •

edited

Loading

felixdoerre commented Dec 26, 2023

dpgeorge commented Jan 29, 2024

Carglglz Jan 30, 2024

felixdoerre Jan 30, 2024 •

edited

Loading

Carglglz Jan 30, 2024

dpgeorge Jan 31, 2024

Carglglz commented Jan 30, 2024

dpgeorge commented Jan 31, 2024

dpgeorge Jan 31, 2024

dpgeorge Feb 5, 2024

dpgeorge commented Feb 1, 2024

felixdoerre commented Feb 1, 2024 •

edited

Loading

dpgeorge Feb 5, 2024

felixdoerre Feb 5, 2024

felixdoerre Feb 5, 2024

dpgeorge commented Feb 5, 2024

dpgeorge Feb 6, 2024

dpgeorge Feb 6, 2024

dpgeorge Feb 7, 2024

dpgeorge commented Feb 7, 2024

dpgeorge commented Feb 7, 2024

extmod/modssl_mbedtls: Add cert verification callback. #12259

extmod/modssl_mbedtls: Add cert verification callback. #12259

Conversation

felixdoerre commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 • edited Loading

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

felixdoerre commented Dec 18, 2023

dpgeorge commented Dec 18, 2023

felixdoerre commented Dec 19, 2023 • edited Loading

Carglglz commented Dec 23, 2023

felixdoerre commented Dec 25, 2023 • edited Loading

cpython compatible

micropython specific

micropython legacy compat

felixdoerre commented Dec 26, 2023

dpgeorge commented Jan 29, 2024

Choose a reason for hiding this comment

felixdoerre Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carglglz commented Jan 30, 2024

dpgeorge commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Feb 1, 2024

felixdoerre commented Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Feb 7, 2024

dpgeorge commented Feb 7, 2024

github-actions bot commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

felixdoerre commented Dec 19, 2023 •

edited

Loading

felixdoerre commented Dec 25, 2023 •

edited

Loading

felixdoerre Jan 30, 2024 •

edited

Loading

felixdoerre commented Feb 1, 2024 •

edited

Loading