-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Comments
Yes it should stay for a while.
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
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. |
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? |
Excellent! That's will be a amazing feature! :) |
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).
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? |
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. |
@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). |
I meant specifically this change - Lines 56 to 62 in 15fea3a
But I fear the answer is no if the |
Fantastic!! That will make a big difference for esp32 and SSL.
Right, so lwIP has statically allocated memory pools, defined by Apart from that, I think we can actually increase the 166*1024 value. Let me check. |
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. |
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 |
I agree with this.
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 👍🏼 |
SSLContext has been implemented. Commit f3f215e added certificate-based methods. |
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? |
…x_main Pimoroni Pico DV Base W: Fix I2S Audio Assignments
Currently MicroPython supports SSL/TLS connections using
ussl.wrap_socket()
. CPython long ago replaced this function withssl.SSLContext
: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:SSLContext
provide more control/options).uasyncio
can support TLS clients and servers (CPythonasyncio
requires passing in anSSLContext
object toasyncio.start_server()
andasyncio.open_connection()
).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:Things to discuss/decide:
ussl.wrap_socket()
for backwards compatibility for a couple of releases. It could print a deprecation warning.load_cert_chain()
takes filenames as arguments and loads the key/cert from a file. This is different to the existingkey
/cert
args inussl.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 likeload_cert_chain(self, certfile, keyfile, *, keydata, certdata)
, or a new method likeload_cert_chain_data(self, keydata, certdata)
.The text was updated successfully, but these errors were encountered: