Updated alignment with esp-idf/mbedtls needed to build with latest esp-idf

[FredrikFornstad]

In the esp-idf commit "feature/nvs_encryption" two days ago, new definitions for mbedtls_aes_xts_context etc have been added in the mbedtls component. Other parts of esp-idf framwork expects these functions, why lws-esp32-factory (and also the test-demos-app) will not compile with the latest esp-idf.
-> suggestion is to update the mbedtls version that comes with lws-esp32-factory and lws-eps32-test-demos-app (together with the dynamic allocation patch).

EDIT: I also saw that in the new mbedtls version 2.12.0 that is now in esp-idf they have introduced a feature (not sure if it affects your dynamic patch):

• Make the receive and transmit buffers independent sizes, for situations

  where the outgoing buffer can be fixed at a smaller size than the incoming

  buffer, which can save some RAM. If buffer lengths are kept equal, there

  is no functional difference. Contributed by Angus Gratton, and also

  independently contributed again by Paul Sokolovsky.

[negativekelvin]

https://github.com/negativekelvin/mbedtls/tree/mbedtls-2.12.0-idf-dynbuf

[lws-team]

@negativekelvin dude.... thanks for stepping up again.

Personally I wrote the original stuff to get it into mbedtls... it wasn't my concept to be in the wilderness doing it over and over OOT because the project refuses to accept it. I genuinely thought if someone provided it, with the advantages, since ESP32 is not special in terms of being low-end enough to be needing it, the mbedtls guys would see why it is needed on stuff like cortex-M (+ RISC-V...) and then it would simply be a contribution to them and we can all be happy.

Unfortunately that isn't the culture in the mbedtls project. While clearly at 10,000ft there is management, productivity, and forward movement on that project overall, in terms of being able to get their head around non-trivial external contribution for the very low end they are a complete failure: to their mind external contribution is just some burden they have to scrape off their shoes. Not just this series, the whole mbedtls PR queue with their calculated "go-nowhere" comments. In private emails they understand the situation. They are in an era where they just won't work with anyone for real work without an @arm.com email address 'cos NIH.

The fact a third party is willing to do serious work uplevelling the series repeatedly over time is further evidence mbedtls are failing to understand what they should be doing to support low-end devices, including arm devices for tls... since that's what their project is supposed to be about...

[FredrikFornstad]

Andy
I fully understand you and I agree that having this included upstream would be really nice. But if the mbedtls team does not want it, I guess the second best option is to ask the espressif-team to have it as part of the esp-idf distribution? Has this been done already?

Recently, there seem to be some esp-idf patches introduced into the mbedtls-lib that ships as default with esp-idf, for instance:
mbedtls: configurable options for controlling dynamic memory allocations

Related to the above patch I also found a thread on the forum about lack of memory due to tls and some discussion around it:
Any way to override MBEDTLS_PLATFORM_MEMORY define?

I guess that if your patch could be introduced in esp-idf, then you will not need to include a mbedtls component in lws-esp32-factory (and test-server-demos) anymore?

[negativekelvin]

There were only minor tweaks from the previous patch and I was just playing around with it. I think it would be more palatable upstream if it had fewer line changes and was a compile time option which would not be too hard to massage it into. But who knows.

[negativekelvin]

am getting stack overflows on test-server-demos though

[lws-team]

@FredrikFornstad those patches seem related to managing existing allocations from different pools. The dynamic stuff is a whole other thing where the connections track their own sizes at runtime, not by a compile-time constant.

The patches you mentioned in the first post are hacks no better than the original build-time hack mbedtls had for setting both directions size limit at once. Different individual tls connections require different rx and tx buffer sizes (cf -factory firmware upload mandating 16K RX) but all you can do is pick one number globally at build-time. Too small and the connections fail due to payload corruption / truncation, since these hacks are illegal for tls protocol.

It's not actually they don't want the feature, the have written privately they may implement it themselves eventually. If espressif find it useful to refer to my patches they're welcome to do so. But I think their current work on top of mbedtls in idf is in the way of platform adaptation, and that 'pays its way' for them to maintain and uplevel it OOT. Even though the hack-on-a-hack patches taken in mbedtls recently came from espressif, they did not have them in idf mbedtls in the two years mbedtls ignored the PR from espressif AFAIK. Therefore I guess espressif don't want the burden of dragging the much larger dynamic alloc patches around OOT either. The solution is mbedtls take them or recreate them, which I expect we'll see in a couple of years, since the utility of it is clear.

think it would be more palatable upstream if it had fewer line changes and was a compile time option which would not be too hard to massage it into. But who knows.

The patch is a major, invasive change to how the library works. It's no more invasive than it has to be to do what it does. And no larger nor more invasive than patches mbedtls applies on itself all the time. Why is it easier to try to explain this as my fault when you can just look at their PRs and see hundreds all ground to a halt the same way?

am getting stack overflows on test-server-demos though

This is solved by giving it more task stack or so?

[negativekelvin]

No I just mean employ some psychological tricks by making the patch look small. Small patches are cute and friendly.

Trying 12k stack.

[lws-team]

... the size of the patch is not relevent to getting it into that project. The projectgus hack-on-a-hack patch was a few lines, got comment and sat there for two years until from reading the comments, evidently something happened via other channels making being seen doing something urgent for mbedtls.

They're not dumb, they don't need 'psychological tricks'. They simply have a policy (or had, I studied their commit log about 9 months ago which was unbroken arm.com authors going back 18 months then) which is to only pretend to operate as a FOSS project wrt contribution from outside arm.

[negativekelvin]

You're right but all we can do is play games outside the ropes.

high water mark at 508 with 12k stack so far

This band-aid also required negativekelvin/libwebsockets@5c69b86

[negativekelvin]

Just to humor myself:
Showing 5 changed files with 916 additions and 727 deletions.
to
Showing 5 changed files with 445 additions and 50 deletions
https://github.com/negativekelvin/mbedtls/tree/mbedtls-2.12.0-idf-dynbuf-cute

[lws-team]

The original series I offered to mbedtls you can actually compare like this:

$ git clone https://github.com/lws-team/mbedtls
$ git checkout origin/dynamic-ssl-buffers
$ git diff d629411212ab85772a8e682695edb2b01fc63d52.. --stat
 include/mbedtls/ssl.h                |   68 ++++----
 include/mbedtls/ssl_internal.h       |   18 ++-
 library/ssl_cli.c                    |  156 +++++++++---------
 library/ssl_srv.c                    |  170 ++++++++++++--------
 library/ssl_tls.c                    | 1129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------
 tests/suites/test_suite_ssl.function |   12 +-
 6 files changed, 887 insertions(+), 666 deletions(-)

... in other words, I add a net of +221 lines, including the needed test adaptations. You're adding +395 lines. Cute is in the eye of the beholder, but that's more bloated, isn't it?

I think it's great you want to meddle with my patches and keep them alive, either way. But when you maintain a large project, it is actually a good thing someone turns up wanting to refactor internal stuff that needs refactoring as part of other work, not a ding.

Still, I'll have no (additional) complaints if mbedtls turn out to want your "don't do the necessary refactor" and add more lines plumper version... it'll be a win either way. But don't hold your breath unless you're planning on working for arm and getting the magic ticket email address.

[negativekelvin]

Well net is one metric but I am concerned with how long is the diff and how easy is it to grok and will that make it easier to apply it to future versions.

[lws-team]

I am concerned

Well, I was "concerned" I couldn't actually use esp32 + mbedtls usefully. I took action about it and wrote from scratch a series aligned with mbedtls that solved it, and tried to get mbedtls to accept the patches.

Actually if mbedtls, the upstream, don't take the series (or as I found, are so up themselves with NIH they will wait their own sweet time to reinvent it with their own name on it) then there's no good solution. It's very expensive to keep stuff going OOT, which is why I am happy you seem motivated to do it. Right now I have no reason to spend any more hours of my life on esp32 or mbedtls.

If you're "concerned", the only useful way to show that is to press mbedtls (honestly... not espressif) to provide a path to accepting this functionality... I don't mean "take the patches" I mean actually have the discussion about how to get from having the working patches that pass the selftests that I provided, which is certainly a very strong POC, to actually accepting some other version of them.

As I have repeatedly said, I have private email with the maintainer and related arm guys they offer no path to co-work on getting this functionality in. They simply won't work with external guys. If you're "concerned" you can be more effective by stopping making up imaginary blockers like not enough "cuteness" and pressure mbedtls about their actual outrageous behaviour.

[squonk11]

I was trying to get lws-esp32-factory with the PR #48 work. Unfortunately without success. Is there any plan to restart your activities in order to make it work again?

[roysG]

Any update with this for the latest version?