Abstraction for synchronous esp-tls

[torkleyy]

I think this should be pretty much good as-is, these TODOs remain:

• Figure out how to map crt_bundle_attach
• Figure out how to map ds_data (not implemented by this PR)
• Is the naming okay / how could async be added in the future?
• Module structure / #[cfg(esp_idf_comp_esp_tls_enabled)]
• non-block config

Let me know if this PR is headed into the right direction.

[ivmarkov]

That's a great idea!

I've left a bunch of (mostly stylistic / code reshuffling) comments, but the approach is solid.

Maybe the one important topic is whether it would be possible to also expose an API, that "adopts" an already existing socket, and does the TLS negotiation on top of it. But we can survive without this initially, I believe.

[ivmarkov]

• Is the naming okay / how could async be added in the future?

For the naming - yes, that's fine.

As for async, adding it - as in - async read / async write should be somehow possible, but I'm not clear on details:

• The easy part: setting cfg.non_block to true is likely to switch the underlying socket to O_NONBLOCK mode post initial TLS negotiation (which itself is either sync or async, but for the async TLS negotiation we can think later).
• The question is more about how to plug the underlying sock_fd into the smol reactor (async-io + polling) or into the upcoming support for tokio's reactor. If your EspTls implementation was capable of "adopting" an already opened socket, that might have been easier, as we would be using e.g. Async<std::net::TcpSocket> (or rather, its T: OwnedFd trait) which would be already registered with smol's reactor and in non-blocking mode, so the TLS traffic would just flow on top then?

[ivmarkov]

One more thing: you need to replace use esp_idf_sys::* with use crate::sys::*, as in the meantime, both esp-idf-hal and esp-idf-svc re-export their esp-idf-sys dependency as sys. (And esp-idf-svc re-exports esp-idf-hal as hal.)

Long term this means most folks would depend only on esp-idf-svc (as they almost always would need a service or two from it anyway), and would use esp-idf-hal and esp-idf-sys via the esp_idf_svc::sys and esp_idf_svc::hal re-exports.

[torkleyy]

I think there's one thing remaining, most of the tls module should be guarded by a cfg, but we can't do it for the full module because X509 is exposed unconditionally.

I could split EspTls etc into a separate module and then reexport it into tls, what do you think?

[torkleyy]

If this is true, should we call conn_new_async instead?

[ivmarkov]

Not necessarily, but VERY IMPORTANT: I was looking at the code and I think I just figured out a way for us to pass an already opened socket!

I think we still should keep the existing new, but we should introduce (maybe post-merge) a new STD-only method along the lines of

#[cfg(feature = "std")]
pub fn wrap(hostname: &str/*We still need this as the hostname is part of the SSL validation*/, socket: std::net::TcpSocket, cfg: &Config) -> Result<EspError, Self> {
    let tls = esp!(unsafe { esp_tls_init(); })?;
    esp!(unsafe { esp_tls_set_conn_state(tls, esp_tls_conn_state_t_ESP_TLS_CONNECTING) })?;
    esp!(unsafe { esp_tls_set_conn_sockfd(tls, socket.fd)? })?;
    esp!(unsafe { esp_tls_conn_new_sync(<hostname_converted_to_char*>, <hostname_len>, 0, cfg, tls) })?;

    Ok(...)
}

The above ^^^ will not try to open a new socket and will re-use ours, as the state of the connection is esp_tls_conn_state_t_ESP_TLS_CONNECTING.

The above approach is important, because it might be a way to implement async as well. Again, to do async in a way which is independent of the concrete async Reactor, we needed a way to pass-in an already opened socket handle, and the above might be just this.

[ivmarkov]

To not make the above STD-only, we can even pass to the method a raw file descriptor, where the assumption is that the file descriptor is a socket descriptor, and also it is already opened.

[torkleyy]

Sounds good, I can give it a try in a follow-up PR.

[ivmarkov]

@torkleyy Build fails because you are still using aliases from the std:: module. Switch these to core::.

[ivmarkov]

I think there's one thing remaining, most of the tls module should be guarded by a cfg, but we can't do it for the full module because X509 is exposed unconditionally.

I could split EspTls etc into a separate module and then reexport it into tls, what do you think?

Yeah, just don't create extra files. Surround your code with

[cfg("esp_idf_comp_esp_tls_enabled")]
mod esptls {
...
}

... and then at the top, do

[cfg("esp_idf_comp_esp_tls_enabled")]
pub use esptls::*;

[torkleyy]

Build is looking good, finally 😅

[ivmarkov]

@torkleyy JFYI I'll be pushing changes to the API in the next hours. Two reasons:

• I found a way to "adopt" an existing socket, including for async, so I have an async implementation ready and working (kind of, there is a bug/something not well thought out in the API of esp_tls_connect - it returns 0 where it should return ESP_TLS_WANT READ or ESP_TLS_WANT_WRITE)
• split is unsound and broken. Turns out, you cannot do overlapping blocking read/write with mbedtls, because the mbedtls SSL context is not thread-safe. The only way to do overlapping read/write with mbedtls is not-blocking (and thus also async). Sounds weird, but this also means the only way to do blocking read/write is to use the async implementation, and then "synchronize" it by using block_on

[torkleyy]

@ivmarkov Alright, thanks for letting me know!