Update mbedtls-rs support

[ARogovskyy]

The most recent refactor as well as the renaming of esp-mbedtls (now mbedtls-rs) broke quite a bit.

This fixes it, making it usable again.

However, there are a few points I would like to discuss before possibly merging this:

• mbedtls_rs wants C-Strings for the servername, and we only get it as a &str, so we need some sort of allocation. Here, I did it similarly to how it was before and copied, but that needs the alloc crate.
  • I think that's fine since any use of mbedtls needs some form of allocation anyway, so it can as well be the rust-one.
• Referring to a git repo can break things at any time. Should we at least pin the latest mbedtls-rs commit?
• mbedtls's Session has a close() function, which the library seems to expect to be called (and produces a warning when not). Currently, this is not implemented.
  • There's a bit of a difficulty and how to do this nicely, since said close() is async and hence cannot be called from, say, drop()
  • My current solution (not in this PR) is to add a new API function to HttpClient, close, but don't know if it's the best solution.

[AnthonyGrondin]

This is an incompatible upgrade; Where previously esp_mbedtls::Certificates contained a client certificate, a private key and a CA chain, now Certificate represents only a parsed X509 chain.

By setting creds: None, there's now no way to use client certificates for authentication.

[ARogovskyy]

Fixed now :)

[AnthonyGrondin]

@ivmarkov pinging you for a review, since you have a broader knowledge of the refactoring than I do.

[ivmarkov]

Really, just one nit from my side but overall - LGTM.

[ivmarkov]

Nit: strictly speaking, there is no need for unsafe. Isn't it better to do CStr::from_bytes_with_nul(&servername).unwrap()?

[ARogovskyy]

unsafe is the way it was done before, so I thought I'd leave it. But yeah I agree, the performance benefit is absolutely negligible. Fixed now.

[ivmarkov]

• Referring to a git repo can break things at any time. Should we at least pin the latest mbedtls-rs commit?

You can also make the case that we should publish a "0.1" version of mbedtls-rs on crates.io.

I think once esp-rs/mbedtls-rs#108 is in, we should really do it.

[ARogovskyy]

• Referring to a git repo can break things at any time. Should we at least pin the latest mbedtls-rs commit?

You can also make the case that we should publish a "0.1" version of mbedtls-rs on crates.io.

I think once esp-rs/mbedtls-rs#108 is in, we should really do it.

Yes, absolutely. Is that something that's imminent? If yes, sure, let's wait for that. Because if not, or if other API changes are coming up, I'd just reference the current commit for the time.

What about the close thing? There is a slight API mismatch between mbedtls and embedded-tls here as embedded-tls consumes the connection on close whereas mbedtls does not.

What I think we can do here is call mbedtls's close in ResponseBody.read_to_end, which consumes the body. So there, both mbedtls and embedded-tls's respective close can be called.

[ivmarkov]

• Referring to a git repo can break things at any time. Should we at least pin the latest mbedtls-rs commit?

You can also make the case that we should publish a "0.1" version of mbedtls-rs on crates.io.
I think once esp-rs/mbedtls-rs#108 is in, we should really do it.

Yes, absolutely. Is that something that's imminent? If yes, sure, let's wait for that. Because if not, or if other API changes are coming up, I'd just reference the current commit for the time.

I hope it is a matter of a week or two - at most.

What about the close thing? There is a slight API mismatch between mbedtls and embedded-tls here as embedded-tls consumes the connection on close whereas mbedtls does not.

What I think we can do here is call mbedtls's close in ResponseBody.read_to_end, which consumes the body. So there, both mbedtls and embedded-tls's respective close can be called.

Up to you but I can't really change the signature of Session::close, because it needs to match the TcpShutdown::close signature.

[ARogovskyy]

I hope it is a matter of a week or two - at most.

Alright, then let's wait for that! We can either put the crates.io version into Cargo.toml in this PR as soon as it's published or do it separately, I don't mind either variant.

Up to you but I can't really change the signature of Session::close, because it needs to match the TcpShutdown::close signature.

I implemented a draft for close (with consuming the connection; works for both TLS libraries) on this branch.

[ARogovskyy]

Now that the crates.io version is done, this can also be updated, right? I have updated my branch.

But I saw that @lulf you have another one that basically does the same thing..? ( #138 )

[lulf]

Thanks @ARogovskyy . I just created that PR in case this did not get updated.