Skip to content
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

Host (and ESP-IDF) build; less memory usage; edge_nal::TcpConnect impl #59

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

ivmarkov
Copy link

@ivmarkov ivmarkov commented Nov 19, 2024

Summary of Changes

===================================================

esp-mbedtls-sys

Host build (changes to esp-mbedtls-sys)

In order to be able to build and run the library on the host machine, the C build has to happen on the machine, with a build.rs script:

  • I've therefore implemented a utility - builder::MbedtlsBuilder - which does the C library build and codegen and which is used by both the existing xtasks as well as from the build.rs of esp-mbedtls-sys
  • The mbedtls GIT submodule is moved to esp-mbedtls-sys. Otherwise, we cannot publish esp-mbedtls-sys to crates.io
  • Ditto for the libs folder (the .a stuff): it needs to be in esp-mbedtls-sys or else we can't publish it
  • NOTE: The C build is now incremental (necessary or else when working on the host machine you'll wait forever). NO mbedtls C code is copied around

ESP-IDF build (changes to esp-mbedtls-sys)

This is also added. It was very easy because for the -espidf targets esp-mbedtls-sys does nothing and delegates the binding generation, the library build, linking etc. etc. to esp-idf-sys.

Support for future baremetal targets with pre-generated bindings and .a libs

Currently not on the table, but adding a new SOC to the xtasks should be even easier now

esp-mbedtls

Removal of async memory buffers (a change to esp-mbedtls)

This is probably the biggest change.

Please look at the new Session async instance, and specifically - at PollCtx. Especially the latter is very extensively documented in terms of what is the idea and how it does it.

While at it, I also implemented a public connect/read/write API on the Session types themselves, so that users don't have to import the Read/Write traits for simple use cases.

Tls and TlsReference (a change to esp-mbedtls).

Tls is a singleton that basically represents the Mbedtls lib itself.

  • For baremetal, it takes the SHA periph on new and optionally - with with_hardware_RSA the RSA perih
  • For the host, ESP IDF and potential future software-only targets, Tls::new does not have parameters.

TlsReference is just like &Tls but allows us to erase the invariant peripheral 'd lifetime of the SHA and RSA periphs that would otherwise contaminate the Session types too.

Note that now, Tls is the only type which does have platform-specific aspects!

Everything else is cross-platform just like the mbedtls C lib itself, as it does not assume esp-hal or anything.

TlsConnector and simpler TlsAcceptor for edge-nal compat (a change to esp-mbedtls)

Now that we don't have or need TLS buffers, the signature of TlsAcceptor is much simpler and has no generics. Note also that I've removed the "bind" logic from TlsAcceptor. Not sure it belongs there.

I also implemented TlsConnector to have a symmetry with TlsAcceptor and because I actually do need it for e.g. the edge-http HTTPS client.

Finally, the above ^^^ types are no longer hard-coded to edge-nal-embassy and can be used with any edge-nal impl (I use them with edge-nal-std for example).

Session::close (a change to esp-mbedtls)

(Only for async sessions, maybe we need it for blocking too?)

The idea of this method is to send asynchronously a notif to the other peer that the TLS session is being closed. I just use the mbedtls facilities for that.
Not sure it works 100%, but if not, we need to fix it, as I think such a method is good to have. A bit like the raw TCP "close" we introduced recently in edge-nal and which is wrapped by Session::close. Session::close also implements the TcpClose trait now.

Next steps

(ASAP) Fix the alignment issues with xtensa

Just locate aligned_calloc and uncomment the commented out code at the top of the function.
The fact that these alignment issues are not hitting us is a pure coincidence and only a matter of time until we start seeing these (I do see these with the mbedtls-3.6-idf branch, but they are also there in the currently used mbedtls-3.4-idf branch).

How to fix:

  • Enable in MbedTLS the malloc/free "custom hooks" as described here. This way MbedTLS would call us for every malloc/free call it does

  • Implement these hooks ourselves, by using GlobalAlloc::alloc for malloc and GlobalAlloc::dealloc for free. This way we decouple ourselves from esp-wifi and just use the Rust global allocator (which is anyway defined on all platforms, including ESP-baremetal)

  • In the hooks, for riscv align the requested memory to 4 bytes and for xtensa - to 8 bytes. The latter would solve the xtensa alignment issues even if we are unsure what object MbedTLS allocates, as I doubt MbedTLS uses u128 anywhere - only up to u64 (the u64s, i.e. their C equivalents are causing us the headaches, as we currently return for structs containing them memory aligned to 4 bytes instead of 8, which is required on xtensa, but not for riscv)

  • (Optional) in Certificates, cluster most or all raw MbedTLS struct pointers into a custom mega-struct which is allocated with a single, safe Box::new() call. This way this code would become shorter and much safer.

(Later) Implement session splitting (full-duplex read and write)

(For web-sockets etc.)

I have an idea, but need to experiment with it a bit. If it works out, the change would be trivial - just one (or two) async mutexes, thanks to PollCtx::poll_mut being actually non-blocking and non-awaiting (even if it is marked as async).

(Later) Get rid of Session::connect

Forgot to mention that already with these changes, Session::connect (a) no longer takes self but &mut self and (b) calling it is completely optional, as read and write call it too.

However: it seems MbedTLS anyway calls mbedtls_ssl_connect from mbedtls_ssl_read and mbedtls_ssl_write so we might not even need all the Session::connect machinery in the first place...

@ivmarkov ivmarkov marked this pull request as ready for review November 19, 2024 20:03
@bjoernQ
Copy link
Collaborator

bjoernQ commented Nov 20, 2024

Awesome and thanks for working on this! Looks all good and sane to me but I leave the real review to @AnthonyGrondin (I haven't done much here besides the first few commits .... time to remove me from the authors in Cargo.toml)

@bjoernQ
Copy link
Collaborator

bjoernQ commented Nov 20, 2024

Maybe the README.md would need some love (not necessarily in this PR)

@ivmarkov
Copy link
Author

Maybe the README.md would need some love (not necessarily in this PR)

Agreed.
Perhaps indeed let's touch the README after this PR (which is very large already), and also after the "ASAP fix the alignment problems" because fixing the alignment problems also means we won't need esp-wifi anymore, or even esp-alloc. Just "some" global allocator, whatever it is. esp-alloc or other.

@ivmarkov
Copy link
Author

Removing esp-wifi from the equation means we also need a replacement for the extern "C" random() fn, but that's easy - the Tls singleton would simply expect a RngCore instance at construction (new) time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants