TLS & DTLS fixes #445

npmenard · 2025-03-24T17:42:17Z

covers
RSDK-10196
RSDK-10197
RSDK-10198
RSDK-10200
RSDK-10201

npmenard · 2025-03-24T17:42:34Z

micro-rdk/src/esp32/certificate.rs

@@ -235,6 +237,15 @@ impl GeneratedWebRtcCertificateBuilder {
            ))
        }?;

+        let mut serial_number = [0xAB_u8; 12];


required now to generate a webrtc cert

npmenard · 2025-03-24T17:43:07Z

micro-rdk/src/esp32/dtls.rs

 use futures_lite::{AsyncRead, AsyncWrite, Future};
 use log::{log, Level};
 use thiserror::Error;

 use super::tcp::Esp32TLSContext;

+const MBEDTLS_ERR_NET_SEND_FAILED: i32 = -0x4E;
+const MBEDTLS_ERR_NET_RECV_FAILED: i32 = -0x4C;


had to manually add them, not sure if esp-rs will update their code to include these symbols

I did file an issue upstream with them: https://github.com/viamrobotics/micro-rdk/pull/445/files, but at the time we weren't prioritizing ESP-IDF 5 and following up fell off my todo list. Do you want me to re-engage with them and see if they are willing to make a fix?

Also, do you have any thoughts on the package they indicated: https://github.com/esp-rs/esp-mbedtls?

Either way, can we add a comment here explaining why we are doing this, and maybe referencing the upstream issue?

npmenard · 2025-03-24T17:44:30Z

micro-rdk/src/esp32/dtls.rs

@@ -228,7 +231,7 @@ extern "C" fn mbedtls_timing_dtls_set_delay<S>(
    } else if let Some(ssl_ctx) = ctx.ssl_ctx {
        let (_, cx) = unsafe {
            // TODO(RSDK-10196): The `p_bio` field doesn't exist in ESP-IDF 5. Maybe it should be `private_p_bio`?
-            let state = SSLStreamInner::<S>::from_raw((*ssl_ctx).p_bio);
+            let state = SSLStreamInner::<S>::from_raw((*ssl_ctx).private_p_bio);


we shouldn't do that. We could use a user_data field in the ssl context struct alternatively (it is private as well) however it's accessor functions are inline and therefore not exported by bindgen

Is this hinting that we are somehow misusing mbedtls, that we suddenly need access to "private" state? Or do you think this is more a flaw in the rust bindings, in that we can't make use of the user_data field?

npmenard · 2025-03-24T17:48:47Z

micro-rdk/src/esp32/tcp.rs

@@ -350,13 +336,13 @@ where
            .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?
        };

-        unsafe {
-            mbedtls_ssl_conf_read_timeout(


edit : it's doable but I hate it. I want to remove it unless there is a strong opinion against

why don't you like it? what's the implication of us not having it?

I'd like answers to Matt's questions too. Are you proposing removing setting the SSL read timeout entirely, rather than accessing private_conf (about which I share your distaste)? Are we sure there isn't some other mechanism to do this?

mattjperez

notes from offline discussion:

as we're bypassing the (unavailable) access methods, there are no guarantees
would need to propose a stub or two in esp-idf-sys to actually get those access methods

acmorrow

I think we should move forward with this in the interest of unblocking our efforts, but overall I'm uncomfortable with many of the choices we have been forced to make. I hope we can revisit this work later and work with upstream to understand whether some of these are things we should be approaching differently, or to get them to expose more where that would help.

Procedurally, this closes out several tickets, but it also has open TODOs against those tickets, which is a bit awkward. If you want, I can go ahead and make a new ticket to commit this work under, and a ticket for follow-up work on TLS/DTLS, and then all the TODOs that referenced the older tickets can be adjusted to refer to the follow-up ticket?

acmorrow · 2025-03-28T14:40:56Z

micro-rdk/src/esp32/dtls.rs

 use futures_lite::{AsyncRead, AsyncWrite, Future};
 use log::{log, Level};
 use thiserror::Error;

 use super::tcp::Esp32TLSContext;

+const MBEDTLS_ERR_NET_SEND_FAILED: i32 = -0x4E;
+const MBEDTLS_ERR_NET_RECV_FAILED: i32 = -0x4C;


I did file an issue upstream with them: https://github.com/viamrobotics/micro-rdk/pull/445/files, but at the time we weren't prioritizing ESP-IDF 5 and following up fell off my todo list. Do you want me to re-engage with them and see if they are willing to make a fix?

Also, do you have any thoughts on the package they indicated: https://github.com/esp-rs/esp-mbedtls?

acmorrow · 2025-03-28T14:42:18Z

micro-rdk/src/esp32/dtls.rs

 use futures_lite::{AsyncRead, AsyncWrite, Future};
 use log::{log, Level};
 use thiserror::Error;

 use super::tcp::Esp32TLSContext;

+const MBEDTLS_ERR_NET_SEND_FAILED: i32 = -0x4E;
+const MBEDTLS_ERR_NET_RECV_FAILED: i32 = -0x4C;


Either way, can we add a comment here explaining why we are doing this, and maybe referencing the upstream issue?

acmorrow · 2025-03-28T14:47:18Z

micro-rdk/src/esp32/dtls.rs

@@ -228,7 +231,7 @@ extern "C" fn mbedtls_timing_dtls_set_delay<S>(
    } else if let Some(ssl_ctx) = ctx.ssl_ctx {
        let (_, cx) = unsafe {
            // TODO(RSDK-10196): The `p_bio` field doesn't exist in ESP-IDF 5. Maybe it should be `private_p_bio`?
-            let state = SSLStreamInner::<S>::from_raw((*ssl_ctx).p_bio);
+            let state = SSLStreamInner::<S>::from_raw((*ssl_ctx).private_p_bio);


Is this hinting that we are somehow misusing mbedtls, that we suddenly need access to "private" state? Or do you think this is more a flaw in the rust bindings, in that we can't make use of the user_data field?

acmorrow · 2025-03-28T14:49:15Z

micro-rdk/src/esp32/tcp.rs

-                std::ptr::null_mut(),
-                0,
-                cfg.get_cfg_ptr() as *const c_void,
+            esp!(esp_tls_server_session_create(


Nice to see the write_unaligned's are no longer needed.

acmorrow · 2025-03-28T14:51:45Z

micro-rdk/src/esp32/tcp.rs

@@ -350,13 +336,13 @@ where
            .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?
        };

-        unsafe {
-            mbedtls_ssl_conf_read_timeout(


I'd like answers to Matt's questions too. Are you proposing removing setting the SSL read timeout entirely, rather than accessing private_conf (about which I share your distaste)? Are we sure there isn't some other mechanism to do this?

TLS & DTLS fixes

d37eae8

npmenard requested a review from a team as a code owner March 24, 2025 17:42

npmenard commented Mar 24, 2025

View reviewed changes

npmenard added 2 commits March 24, 2025 13:45

update tcp file

b0e0f61

remove timeout config

f9c8f06

npmenard commented Mar 24, 2025

View reviewed changes

npmenard added 2 commits March 24, 2025 17:27

add timeout

49898b1

serial number as 0

3108d92

mattjperez reviewed Mar 26, 2025

View reviewed changes

update function

a30ae7a

acmorrow approved these changes Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS & DTLS fixes #445

TLS & DTLS fixes #445

npmenard commented Mar 24, 2025

npmenard Mar 24, 2025

npmenard Mar 24, 2025

acmorrow Mar 28, 2025

acmorrow Mar 28, 2025

npmenard Mar 24, 2025

acmorrow Mar 28, 2025

npmenard Mar 24, 2025 •

edited

Loading

mattjperez Mar 26, 2025

acmorrow Mar 28, 2025

mattjperez left a comment

acmorrow left a comment

acmorrow Mar 28, 2025

acmorrow Mar 28, 2025

acmorrow Mar 28, 2025

acmorrow Mar 28, 2025

acmorrow Mar 28, 2025

TLS & DTLS fixes #445

Are you sure you want to change the base?

TLS & DTLS fixes #445

Conversation

npmenard commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npmenard Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez left a comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npmenard Mar 24, 2025 •

edited

Loading