SSL rampage #2938
SSL rampage #2938
Conversation
|
If it's good to go, let me make sure that we're sync'd up with mbedtls 2.17 before landing this one. Or maybe we should jump further off the beaten track to the 2.18 release, since 2.17 seems still-born? Or we could stick with 2.16.3, the latest official release, it seems. |
|
Does this mean that we can now configure the incoming/outgoing ssl buffers independently? This will significantly improve interoperability and memory consumption!! |
I wasn't able to work out their release strategy. However, as per https://tls.mbed.org/tech-updates it seems 2.16 is their newest LTS branch. That maybe explains why 2.17, 2.18, etc. aren't announced and the only exist as tags (but not releases) on GitHub. The latest in this line currently is 2.19.1. |
|
@marcelstoer, we're going to have to do an intelligent rebaseline / merge anyway, and we're not getting the testing effort that we really need so let's unblock some of these critical issues for now. |
|
@marcelstoer In that case, I guess the "go big or go home" is 2.19.1. I'll see if jumping up breaks anything for us when I have a minute. @pjsg Oh, that would be kind of nice, but I think we should keep the 4K buffer sizes as the default. @TerryE I apologize for having not had time to do any real work on NodeMCU in months. Day job significantly ramped up its time consumption. |
|
I would leave them as 4k each for now, but we could change the incoming one to 6k and the outgoing one to 2k and significantly improve interoperability without consuming extra memory. |
|
@pjsg , did you mean MBEDTLS_SSL_MAX_IN_CONTENT_LEN and MBEDTLS_SSL_MAX_OUT_CONTENT_LEN. I am not sure if this is supported on the latest mbedtls ? |
|
I checked some time ago (maybe a year or more ago), and it was a feature in the current version of mbedtls, but not in the version that was in our codebase. This PR is adding support for the IN/OUT_CONTENT_LEN and we can use that fact to improve our interoperability without increasing the total RAM demands. |
|
Super. That opens open up new possibilities for sure to control/tune the in and out buffers! |
This hasn't worked in a while, presumably since one of our upstream merges. Don't bother making it work, since MD2 is generally considered insecure.
There was some... frankly kind of scary buffer and data shuffling if ESP8266_PLATFORM was defined. Since we don't, in fact, define that preprocessor symbol, just drop the code lest anyone (possibly future-me) be scared.
No functional changes
There's no need to malloc a structure that's used only locally.
What an absolute shitshow this is. mbedtls should absolutely not be mentioned inside sys/socket.h and app/mbedtls/app/lwIPSocket.c is not so much glue as it as a complete copy of a random subset of lwIP; it should go, but we aren't there yet. Get rid of the mysterious "mbedlts_record" struct, which housed merely a length of bytes sent solely for gating the "record sent" callback. Remove spurious __attribute__((weak)) from symbols not otherwise defined and rename them to emphasize that they are not actually part of mbedtls proper.
This at least makes the shitshow smaller
I presume these also need the new arguments
Depending on who you ask it's either EOL already or EOL soon, so we may as well get rid of it now.
|
This is a very slightly less aggressive rampage now, in that it lands the latest commit on the 2.16.3 series, which is the latest one they seem to believe in supporting. However, importantly, this series now includes a test program ( |
|
I have moved the test infrastructure out to its own PR, #2983, as I do not think it should be blocked by TLS changes. |
|
@TerryE do you mind if we land this on |
|
Given TerryE's thumbs up, I'm going to go ahead and click merge. Sorry if this breaks anything for anyone. |
* Remove stale putative MD2 support This hasn't worked in a while, presumably since one of our upstream merges. Don't bother making it work, since MD2 is generally considered insecure. * Land mbedtls 2.16.3-77-gf02988e57 * TLS: remove some dead code from espconn_mbedtls There was some... frankly kind of scary buffer and data shuffling if ESP8266_PLATFORM was defined. Since we don't, in fact, define that preprocessor symbol, just drop the code lest anyone (possibly future-me) be scared. * TLS: espconn_mbedtls: run through astyle No functional changes * TLS: espconn_mbedtls: put the file_params on the stack There's no need to malloc a structure that's used only locally. * TLS: Further minor tidying of mbedtls glue What an absolute shitshow this is. mbedtls should absolutely not be mentioned inside sys/socket.h and app/mbedtls/app/lwIPSocket.c is not so much glue as it as a complete copy of a random subset of lwIP; it should go, but we aren't there yet. Get rid of the mysterious "mbedlts_record" struct, which housed merely a length of bytes sent solely for gating the "record sent" callback. Remove spurious __attribute__((weak)) from symbols not otherwise defined and rename them to emphasize that they are not actually part of mbedtls proper. * TLS: Rampage esp mbedtls glue and delete unused code This at least makes the shitshow smaller * TLS: lwip: fix some memp definitions I presume these also need the new arguments * TLS: Remove more non-NodeMCU code from our mbedtls * TLS: drop support for 1.1 Depending on who you ask it's either EOL already or EOL soon, so we may as well get rid of it now.
* Remove stale putative MD2 support This hasn't worked in a while, presumably since one of our upstream merges. Don't bother making it work, since MD2 is generally considered insecure. * Land mbedtls 2.16.3-77-gf02988e57 * TLS: remove some dead code from espconn_mbedtls There was some... frankly kind of scary buffer and data shuffling if ESP8266_PLATFORM was defined. Since we don't, in fact, define that preprocessor symbol, just drop the code lest anyone (possibly future-me) be scared. * TLS: espconn_mbedtls: run through astyle No functional changes * TLS: espconn_mbedtls: put the file_params on the stack There's no need to malloc a structure that's used only locally. * TLS: Further minor tidying of mbedtls glue What an absolute shitshow this is. mbedtls should absolutely not be mentioned inside sys/socket.h and app/mbedtls/app/lwIPSocket.c is not so much glue as it as a complete copy of a random subset of lwIP; it should go, but we aren't there yet. Get rid of the mysterious "mbedlts_record" struct, which housed merely a length of bytes sent solely for gating the "record sent" callback. Remove spurious __attribute__((weak)) from symbols not otherwise defined and rename them to emphasize that they are not actually part of mbedtls proper. * TLS: Rampage esp mbedtls glue and delete unused code This at least makes the shitshow smaller * TLS: lwip: fix some memp definitions I presume these also need the new arguments * TLS: Remove more non-NodeMCU code from our mbedtls * TLS: drop support for 1.1 Depending on who you ask it's either EOL already or EOL soon, so we may as well get rid of it now.
devbranch rather than formaster.docs/*.Update us to mbedtls 2.17 and then angrily rampage around Espressif's goo. It turns out that they basically have a second lwip just for SSL. This is completely bogus and should go away. Some of it can go away right now because it's unused; the rest is used and will require, among other things, making our primary lwIP build with NO_SYS==0, which requires some understanding of mutexes and such.
Anyway, this is not really a PR per se so much as a request for review and guidance.