8000 RFC: change ussl to implement SSLContext · Issue #8915 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

RFC: change ussl to implement SSLContext #8915

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

Closed
dpgeorge opened this issue Jul 16, 2022 · 15 comments
Closed

RFC: change ussl to implement SSLContext #8915

dpgeorge opened this issue Jul 16, 2022 · 15 comments
Labels
enhancement Feature requests, new feature implementations rfc Request for Comment

Comments

@dpgeorge
Copy link
Member

Currently MicroPython supports SSL/TLS connections using ussl.wrap_socket(). CPython long ago replaced this function with ssl.SSLContext:

Since Python 3.2 and 2.7.9, it is recommended to use the SSLContext.wrap_socket() of an SSLContext instance to wrap sockets as SSLSocket objects.

And in CPython ssl.wrap_socket() is deprecated since CPython version 3.7.

I propose that MicroPython also switch to using SSLContext. The reasons to change are:

  1. To more closely match CPython (it's hard to get TLS correct and having things match CPython makes it easier to write tests to compare uPy and CPy).
  2. To support more sophisticated use of TLS connections (SSLContext provide more control/options).
  3. So that uasyncio can support TLS clients and servers (CPython asyncio requires passing in an SSLContext object to asyncio.start_server() and asyncio.open_connection()).
  4. To support server_hostname in a CPython-compatible way.

One other benefit of using SSLContext is that it can be used to preallocate the large TLS buffers (16K) needed for a wrapped socket, and the buffer can be reused over and over as long as only one wrapped socket is needed at a time. This will help to reduce memory fragmentation and guarantee that sockets can be wrapped without running out of memory.

The proposed ussl module would now look like this:

# constants
PROTOCOL_TLS_CLIENT
PROTOCOL_TLS_SERVER

class SSLSocket:
    # same as existing ssl socket object

class SSLContext:
    def __init__(self, protocol):
        # protocol is PROTOCOL_TLS_CLIENT or PROTOCOL_TLS_SERVER
    def load_cert_chain(self, certfile, keyfile):
        # load certificate/key from a file
    def wrap_socket(self, sock, *, server_side=False, do_handshake_on_connect=True, server_hostname=None):
        # wrap a socket and return an SSLSocket

def wrap_socket(...):
    # existing function for backwards compatibility

Things to discuss/decide:

  1. Should probably keep the existing ussl.wrap_socket() for backwards compatibility for a couple of releases. It could print a deprecation warning.
  2. load_cert_chain() takes filenames as arguments and loads the key/cert from a file. This is different to the existing key/cert args in ussl.wrap_socket() (which are not CPython compatible) that take a str/bytes object containing the key/cert data. The question is if we should add, as a MicroPython-extension, a way to pass in data directly like this, and if yes how that would be done. For example add keyword-only args like load_cert_chain(self, certfile, keyfile, *, keydata, certdata), or a new method like load_cert_chain_data(self, keydata, certdata).
  3. Whether to implement it in C or Python.
@dpgeorge dpgeorge added enhancement Feature requests, new feature implementations rfc Request for Comment labels Jul 16, 2022
@dpgeorge
8000 Copy link
Member Author

Related to: #5436, #5611, #5840, #7315, #8252, #8854.

@iabdalkader
Copy link
Contributor
1. Should probably keep the existing `ussl.wrap_socket()` for backwards compatibility for a couple of releases.

Yes it should stay for a while.

The question is if we should add, as a MicroPython-extension, a way to pass in data directly

I don't think this was ever needed because key and cert are read from files anyway before being passed as buffers, and I just don't see the reasoning behind reading/extracting key/cert from some external source, because that's just as (in)secure as storing them in files (so may as well just store them in files). That said, these kwargs should still be supported for backwards compatibility with code that uses wrap_socket, but if possible load_cert_chain should fail if it's used with these kwargs. I'm not sure how to implement that, but this way when wrap_socket is gone these kwargs can go as well

3. Whether to implement it in C or Python.

I'm not sure if it matters, but at some point I want to support URIs for key/cert in crypto devices, so may make sense to keep things in C for now if it makes it easier to support that.

@Gadgetoid
Copy link
Contributor

I have encountered issues where failing to explicitly ".close()" a wrapped socket immediately after use will eventually lead to system level OOM errors, and that's with - imho - fairly trivial programs. With many of our C drivers using small amounts of RAM here and there it has become quite dicey!

Would this potentially allow returning some of the large (25K?) chunk of RAM taken from MicroPython to the gc_heap?

@beyonlo
Copy link
beyonlo commented Jul 18, 2022

Excellent! That's will be a amazing feature! :)

@dpgeorge
Copy link
Member Author

I have encountered issues where failing to explicitly ".close()" a wrapped socket immediately after use will eventually lead to system level OOM errors

You must always close sockets to free up internal resources. lwIP has a memory pool independent to the uPy heap, and the only way to free lwIP memory is by a socket close. Similar with SSL wrapped sockets, you must close them to free up memory.

They shouldn't need to be close immediately, but definitely once they are no longer needed (and before you create a new one).

Would this potentially allow returning some of the large (25K?) chunk of RAM taken from MicroPython to the gc_heap?

Do you mean RAM taken when you create a wrapped socket? If you close the socket that RAM should be returned to the uPy heap automatically. (Note that on esp32 wrapped sockets take RAM from the FreeRTOS heap, not the uPy heap.)

@chrisovergaauw
Copy link
Contributor

Do you mean RAM taken when you create a wrapped socket? If you close the socket that RAM should be returned to the uPy heap automatically. (Note that on esp32 wrapped sockets take RAM from the FreeRTOS heap, not the uPy heap.)

Will this RFC also apply to esp32 or won't it be able to, due to mentioned differences?
Currently we're forced to make sure that wrapping a socket is one of the first things we do, (before enabling BLE i.e.).
Otherwise there's the risk of ENOMEM errors. it would definitely be nice to have more control over memory usage on the ESP32 as well.

@dpgeorge
Copy link
Member Author

Will this RFC also apply to esp32 or won't it be able to, due to mentioned differences?
Currently we're forced to make sure that wrapping a socket is one of the first things we do

The idea is to be able to do this:

import ssl

# pre-allocate context and SSL buffers
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)

# do lots of other stuff
...

# use ctx, uses pre-allocated SSL buffers
s = ctx.wrap_socket(...)
s.close()

# reuse ctx, reuse buffers
s = ctx.wrap_socket(...)
s.close()

At least, that's the idea. And the API is still compatible with CPython. Not sure how this will actually be implemented on esp32 though.

@jimmo
Copy link
Member
jimmo commented Jul 20, 2022

Note that on esp32 wrapped sockets take RAM from the FreeRTOS heap, not the uPy heap.
Not sure how this will actually be implemented on esp32 though.

@dpgeorge I now have ESP32 working with mbedtls using the uPy heap via m_tracked_alloc (and Rob's split heap PR for more heap). So I don't think ESP32 will be any different to the other ports in this regard. (Also, FWIW, https requests that previously failed now work on IDF 4.4 with this).

@Gadgetoid
Copy link
Contributor

Do you mean RAM taken when you create a wrapped socket? If you close the socket that RAM should be returned to the uPy heap automatically. (Note that on esp32 wrapped sockets take RAM from the FreeRTOS heap, not the uPy heap.)

I meant specifically this change -

#ifndef MICROPY_GC_HEAP_SIZE
#if MICROPY_PY_LWIP
#define MICROPY_GC_HEAP_SIZE 166 * 1024
#else
#define MICROPY_GC_HEAP_SIZE 192 * 1024
#endif
#endif
which dramatically reduces the amount of RAM available to MicroPython... quite painful when you're trying to juggle network sockets and screens 😬

But I fear the answer is no if the lwIP memory pool remains separate.

@dpgeorge
Copy link
Member Author

I now have ESP32 working with mbedtls using the uPy heap via m_tracked_alloc

Fantastic!! That will make a big difference for esp32 and SSL.


which dramatically reduces the amount of RAM available to MicroPython [on rp2]

Right, so lwIP has statically allocated memory pools, defined by ports/rp2/lwip_inc/lwipopts.h. It's currently tuned for reasonable TCP performance without taking up too much RAM. But you can easily reconfigure it to use less RAM, at the expense of throughput. See ports/stm32/lwip_inc/lwipopts.h which has 4 levels of RAM usage.

Apart from that, I think we can actually increase the 166*1024 value. Let me check.

@Gadgetoid
Copy link
Contributor

Thank you @dpgeorge I may have to look into this.

Someone off-topic, but the sad truth is that we're also making quite a lot of unchecked, temporary (and not so temporary) allocations outside of gc_heap in our C++ drivers, so the extra memory pressure in RP2 + W has caused some things that (albeit through pure blind luck) worked before to hardlock. Any savings to RAM usage (or even any additional knowledge about how it works) would buy me some time while I try to fix these up using the new tools (tracked alloc, more flexible root pointers) you've provided. As such I'm keeping an especially close eye on the project lately.

@dpgeorge
Copy link
Member Author

allocations outside of gc_heap in our C++ drivers

Where is this memory coming from, is it from the uPy gc_heap, or elsewhere in RAM?

I tried increasing the MICROPY_GC_HEAP_SIZE to 180*1024 and that compiles, and leaves 11k of C stack. Compare that to 32k stack for the Pico and 25k stack for standard Pico-W. 11k should be enough, and there are runtime checks to make sure it doesn't overflow. I suggest trying this value (180k heap).

Also looks like there is 2k of unused C heap which can be reclaimed by removing the .heap section in memmap_mp.ld.

@Carglglz
Copy link
Contributor

Should probably keep the existing ussl.wrap_socket() for backwards compatibility for a couple of releases. It could print a deprecation warning.

I agree with this.

Whether to implement it in C or Python.

I think it is easier to implement it in Python and extend some features in C, I made demo in #8968 about this, @dpgeorge if you want to check it out 👍🏼

@dpgeorge
Copy link
Member Author

SSLContext has been implemented. Commit f3f215e added certificate-based methods.

@rocus
Copy link
rocus commented Feb 2, 2024

Hi George. I am using this new ssl module (in the latest Micropython version for a raspberry pico W). The reason for me is only better memorymanagement: I often encountered ENOMEM errors during urequests.post calls due to fragmentation.

See your first post (july 2022):

One other benefit of using SSLContext is that it can be used to preallocate the large TLS buffers (16K) needed for a wrapped socket, and the buffer can be reused over and over as long as only one wrapped socket is needed at a time. This will help to reduce memory fragmentation and guarantee that sockets can be wrapped without running out of memory.

So I declare a global varable

a= ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) in the hope that that covers the data for my post requests.

I use this variable "a" later in my request call: s = a.wrap_socket(s, server_hostname=host).

This all works fine but I have doubts about the memory usage: with the help of micropython.meminfo(1) I see that the declaration of "a" takes only 2k memory. Am I doing something wrong here?

tannewt pushed a commit to tannewt/circuitpython that referenced this issue Feb 14, 2024
…x_main

Pimoroni Pico DV Base W: Fix I2S Audio Assignments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests, new feature implementations rfc Request for Comment
Projects
None yet
Development

No branches or pull requests

8 participants
0