-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ssl: work on anything implementing the socket protocol #8954
Conversation
Is this connected to this PR? Will it resolve this issue? |
00020eb
to
2bb7125
Compare
I'm excited about this PR but realistically we should probably not rush it in before 9.0.0. |
@jepler please let me know if I can do any testing or benchmarks |
@jepler When ready, I can also help testing HTTPS serving with |
Tested TCP socket-level HTTPS request with these artifacts and the PicoW + WIZnet W5100S hat gets addendum: non-SSL without |
Tested HTTPS server on ESP32-S2 TFT. HTTP version still works the same. |
@michalpokusa I tried HTTPS Server on ESP32-S3 with sockets from WIZnet, but client got connection refused. I'll see if I can replicate addendum: ESP32-S3 (Adafruit TFT) HTTPS Server seems to be working OK, though I get: Traceback (most recent call last):
File "adafruit_httpserver/server.py", line 472, in poll
File "adafruit_httpserver/server.py", line 604, in _debug_response_sent
TypeError: 'Socket' object isn't subscriptable library issue, I suspect, that doesn't affect the response being received successfully. I'm using this PR artifact + your PR88 branch of HTTPServer |
@anecdata Thank for testing. HTTPS server works on ESP32-S3 for me too, although I successfully get the response, maybe the problem is due to self-signed cert? When I added The TypeError is also present, which is a result of (I believe) incorrect types being returned by @jepler I belive it got changed in this PR, as previously this did not happen on ESP32-S3 |
@michalpokusa Yes, ESP32-S3 works and I get the response. The config that didn't work was an S3 with Ethernet FeatherWing, using WIZnet sockets for the HTTPS server. I am using the |
cpython socket objects have a `type` property which gives their type as an integer (e.g., SOCK_STREAM). Add this for compatibility with standard Python. It's needed for ssl, which currently just grabs the value directly from an internal structure (naughty!)
This header can be used by ssl even if there's no core socketpool
In principle this allows core SSL code to be used with e.g., wiznet or airlift sockets. It might actually be useful with wiznet ethernet devices (it's probably not with airlift)
the ssl test program also works here with ```py cs = digitalio.DigitalInOut(board.W5500_CS) spi_bus = board.SPI() ```
We're just going to pass it down to the underlying socket, so don't parse it, multiply it, etc.
I believe I found the cause for this. I've updated (rebased) the branch on top of current main and test built espressif, but didn't re-test anything.ese |
The subscripting to get the `peer` object was wrong.
this may simply not be a good combo, with wifi taking up extra RAM. I think SSL needs some large blocks of memory that may simply not be left after parsing in all the wiznet code. anyway, that's a total guess. |
@anecdata you mentioned a separate UF2 for the W5100S-EVB-Pico, which one is it ? |
Is everyone comfortable with this on the client side? It would be nice to get it merged. Any server tweaks could come after. edit: looks like there are other open To Dos, not sure the best strategy to avoid risk of performance or other regressions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK to me. I did not test: thanks everyone else who did test. Let's get it in the next beta!
I saw this after I reviewed. @anecdata Which things are you referring to? |
@dhalbert the open checkboxes in the original post. Serving we're aware of. The question I guess is benchmarking / possible refactoring. |
@jepler how about those unchecked tasks? I will withdraw my approval for the moment. |
awaiting feedback on remaining tasks in OP
I am not too worried about performance, but testing on pico w and testing serving I think are necessary. |
I investigated this. With an older version of wiznet, I also got an "OSError: [Errno 22] Invalid Argument". This appears to be due to incorrect handling of timeout=None in recv_into. (the timeout is always None during an SSL handshake) However, this was corrected as a part of adafruit/Adafruit_CircuitPython_Wiznet5k#156 When using the current latest version of wiznet, My code which is for the wiznet 5500 pico-style board: https://gist.github.com/jepler/f479119ce7526d95106b1db924df92ac Generated a self-signed certificate on linux with |
@@ -9,3 +9,4 @@ CHIP_FAMILY = rp2 | |||
EXTERNAL_FLASH_DEVICES = "W25Q16JVxQ" | |||
|
|||
CIRCUITPY__EVE = 1 | |||
CIRCUITPY_SSL = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable SSL also on wiznet_w5100s_evb_pico? I could do this in a separate PR if desired.
I also re-tested https serving over wifi on matrixportal s3 with the script at #9003 (comment) |
@anecdata yes if you feel like putting in a PR for this I'd be grateful! |
For completeness, tested Pico W with SSL URL, no apparent regressions. Not sure if there was some other aspect of Pico W to test. |
@anecdata pico w and w5100s (using this firmware, this latest adafruit_wiznet5k and this latest adafruit_connection_manager) still runs into MemoryError at 235 in connection_manager- am I missing a part of the discussion here before I try to free any memory (via freezing/ommiting) |
@AndreasTheCat there are comments above about memory issues with Pico W + WIZnet. ESP32-S3 + WIZnet seems OK. Not sure about ESP32-S2. I don't know if Pico + WIZnet was explicitly tested... Pico should have more memory than Pico W. I'll try to set one up to replicate later. |
Looks like jepler did test a WIZnet dev board (Pico + WIZnet): Update 1: Pico + W5500:CircuitPython: HTTPS Requests: OK HTTPS Server: OK¹ Update 2: Pico W:CircuitPython: HTTPS Requests: OK HTTPS Server: Update 3: Pico W + W5100S (using WIZnet interface):CircuitPython: HTTPS Requests: OK HTTPS Server: |
simply amazing to see `{ 41332 //<-gc.memfree()` for the first time on pico w with w5100s. |
@anecdata pointed out that moving ssl to shared-module might pave the way to using ssl with wiznet.
I mentioned that for this to be possible,
ssl
would have to make Python calls for various methods (connect, send, recv, etc) instead of calling the common_hal socket routines.I went ahead and implemented this. I very lightly tested it: it can do the standard socket test and fetch from https pages like https://www.adafruit.com/api/quotes.php on an esp32s3 device. I didn't test serving.
I also didn't benchmark, test with pico w, let alone test with wiznet.
The following socket methods need to exist and behave like a standard socket:
setsockopt(SOL_SOCKET, SO_REUSEADDR, 1)
should be implemented, as a no-op if necessary; these constants should match the ones in the core and be provided on the socketpool-compatible object)Additionally, the following property needs to exist on socket objects, and for stream sockets it has to use the same value as
socketpool.SocketPool.SOCK_STREAM
(1):TODO:
benchmark & short circuit for built in sockets if it is needed to avoid a performance regressionAdditionally, if we choose to go this route we might want to refactor the code a bit so that the Python object representations are sent into the ssl shared_module code, because with this change e.g., a connection address is parsed in shared-bindings/ssl, then turned back into an object in shared-module/ssl to call socket connect, and then parsed a second time in shared-bindings/socket connect. This could be avoided by simply re-organizing so that the shared-module/ssl API took the object.