feat: provide timer / clock support to mbedtls

[ericschaal]

Closes #92

Adds opt-in timer and wall clock support to mbedtls.

Two new features in mbedtls-rs-sys:

• hook-timer: enables registration of a custom timer hook via the MbedtlsTimer trait
• hook-wall-clock: enables registration of a custom wall clock hook via the MbedtlsWallClock trait

Ready-to-use implementations:

• timer-embassy: MbedtlsTimer impl backed by embassy-time
• timer-std: MbedtlsTimer impl backed by std
• wall-clock-esp32xx: MbedtlsWallClock impl backed by the ESP32 RTC for all ESP32 variants (ESP32, C2/C3/C6, H2, S2/S3)
• wall-clock-std: MbedtlsWallClock impl backed by std

Design decision: mbedtls_platform_gmtime_r ignores the time_t parameter and instead queries the wall clock directly for the current calendar time. This decouples the timer and wall clock implementations, since mbedtls always calls it with a freshly retrieved mbedtls_time(NULL) anyway.

[ivmarkov]

(Sit tight this is going to be tricky.)

Some thoughts after looking at the MbedTLS timer & wall clock support:

First off, the wall clock support in MbedTLS is pretty horrible, in that it binds the notion of a

• "wall clock" (something that can tell you the current calendar time, as in year, month, day, hh, mm, ss in UTC)
• ... with the notion of a "timer" (something that can tell you how many millis had elapsed (monotonically! wall clocks might not have this property!) since some unknown "epoch", where the "epoch" - by their own documentation - might be "whatever" and not necessarily 1970-01-01T00:00:00UTC

Where they should have allowed the user to just override (note that by "time" below they really mean "wall clock time"):

• mbedtls_x509_time_is_past
• and mbedtls_x509_time_is_future

... with her own definitions and thus be done with the platform "wall clock" support, they've instead horribly mandated that the user should support a transition from a timer duration to a wall clock instant!!

I.e. we need to support x509_get_current_time = mbedtls_x509_time_gmtime which really means we need to define a global function
mbedtls_platform_gmtime_r(<timer_duration>, <wall-clock-instant>) which - in theory - should be able to convert the timer duration into a wall-clock instant.

Now, the small light at the end of the tunnel is that we might not have to necessarily do this conversion.

By looking at x509_get_current_time and x509_crt_verify_chain - which are the only two functions that ever call mbedtls_platform_gmtime_r, it seems they always call it with a freshly retrieved timer duration via mbedtls_time(NULL).

So in a way, we can change the implementation semantics of our mbedtls_platform_gmtime_r from:

• From "It needs to convert a timer duration to a wall clock time"
• To "Disregard the <timer-duration> parameter and just give me the current wall clock time"

This (to some extent) decouples the timer impl from the wall clock impl. Only to some extent though because we cannot define a wall clock provider without defining a timer provider.

However a timer provider is trivially easy. Basically embassy_time::now() (not that the sys crate should depend on embassy-time but the timer driver impl is one line downstream in esp-mbedtls).

Further to that, providing a wall-clock time also becomes trivially easy:
We should just provide a mbedtls_platform_gmtime_r which disregards the first parameter (<timer-duration>) and just returns the current wall clock time.

And it would all work.

=====

I'll do a thorough review of your code shortly, but let me say upfront that using the RTC component of esp-hal is not a great idea.

Contrary to its name, this thing is anything but a Real Time Clock:

• It tends to drift quite seriously;
• It loses its time on power-cycling of the device. And there is no way to battery-power it (esp32 circuitry restrictions).

[ivmarkov]

Hmmm, on a second thought, we might keep a "default" implementation of the wall clock time based on the RTC clock in esp-hal/esp32. At least it survives deep sleep.

But we absolutely need to introduce the "platform-neutral" MbedtlsTimer and MbedtlsWallClock traits. An esp32 RTC-based impl for MbedtlsWallClock should be provided on top.

====

BTW: sorry for the tons of comments. I know the PR is not ready for review yet, but wanted to provide an early feedback. Hope it is useful.

[ericschaal]

Hi @ivmarkov feedback is useful thank you, I'll keep working on this PR 👍

[ericschaal]

@ivmarkov

Currently we have two opt-in features for mbedtls time support:

• hook-timer - enables registration of a timer hook for mbedtls
• hook-wall-clock - enables registration of a wall clock hook for mbedtls

Design Rationale:
I've made these features opt-in (disabled by default). This is why I chose the hook-* naming convention rather than nohook-* - the latter implies opting out of a default behavior, whereas these features must be actively enabled.

Question on Naming Consistency:
I notice the codebase has existing nohook-* features (nohook-sha1, nohook-sha256, etc.) that follow an opt-OUT pattern, while these new time features follow an opt-IN pattern with hook-* naming. Should we keep this approach, or would you prefer consistency with the existing nohook-* pattern?

I based my opt-in design decision on your comment. However, if you prefer a different approach, we could change it and potentially include at least timer support in the precompiled bindings.

Implementations of MbedtlsTimer and MbedtlsWallClock traits:
The crate also provides two ready-to-use implementations:

• Timer using embassy-time feature timer-embassy, better name pending
• Wall clock for all ESP32 variants (ESP32, ESP32-C2/C3/C6, ESP32-H2, ESP32-S2/S3) using the ESP32 RTC, feature wall-clock-esp32xx (maybe rename to wall-clock-rtc-esp32 ?)

Let me know your preference on the feature design, and I'm happy to adjust as needed.

[ivmarkov]

Question on Naming Consistency: I notice the codebase has existing nohook-* features (nohook-sha1, nohook-sha256, etc.) that follow an opt-OUT pattern, while these new time features follow an opt-IN pattern with hook-* naming. Should we keep this approach, or would you prefer consistency with the existing nohook-* pattern?

The problem with following an opt-OUT pattern for the date-time functionality is that if we switch to it (i.e. from "hook" to "nohook") then we are forcing the user to provide implementations for the timer and/or for date-time API.

Moreover, we cannot easily "enforce" that the user did indeed provide hooks for these before calling the MbedTLS C API. So that would mean we need to panic at runtime if MbedTLS ends up calling our timer/date-time traits and there is no implementation for these.

With the timer, this is not such a big deal, as embassy-time is a super-easy provider for a timer.

I wonder though with date-time? Asking the user to provide a date-time trait before being able to establish the simplest TLS connection is maybe asking too much.

Therefore, let's leave the features for timer and date-time as "hook" for now.

Implementations of MbedtlsTimer and MbedtlsWallClock traits: The crate also provides two ready-to-use implementations:

• Timer using embassy-time feature timer-embassy, better name pending

That's fine though the dependencies on the mbedtls-rs crate keep growing but oh well.

• Wall clock for all ESP32 variants (ESP32, ESP32-C2/C3/C6, ESP32-H2, ESP32-S2/S3) using the ESP32 RTC, feature wall-clock-esp32xx (maybe rename to wall-clock-rtc-esp32 ?)

Up to you.

I think you just need to finish it, address all my comments (which are not outdated) and then make sure CI passes.

[ericschaal]

Hi @ivmarkov, CI is passing. Could you offer your guidance on the 2 items below:

1. std support
   Unclear to me what is the plan/vision for std platforms, particularly esp-idf integration:

• "ESP-IDF is also supported and in that case mbedtls-rs-sys becomes just an alias for esp-idf-sys and uses the MbedTLS library which is built-in inside ESP-IDF."
• Meaning that we should not even expose timer/clock hooks and related traits to espidf platforms right? But we should expose them to std (other than espidf) platforms?
• Do we need default MbedtlsTimer/MbedtlsWallClock trait implementations for std (but non-espidf)? embassy-time could be used for MbedtlsTimer but so can std::time right?
• basically, what specifically needs to be done for std (and by extension esp-idf targets) in this MR?

Sorry in advance if the answer to these questions is trivial. I'm quite new to this esp on rust ecosystem.

2. Bindings metadata approach

Examples with timer/wall-clock need rebuilt bindings (opt-in features not in pre-compiled bindings/libs).

Solution implemented:

• CI builds two artifact sets: one only with crypto hooks as before and one with extra timer/wall-clock for examples
• Generated .toml metadata describes included hooks for each artifact.
• Build script compares needed hooks against metadata. Re-compiles if needed hooks != metadata hooks
• Examples download artifacts from generation job (no on-the-fly compilation)

This deviates from the main purpose of this PR. Should this be a separate PR and is the approach acceptable? Or should use on the fly compilation when building examples (need fetch submodule & setup espressif clang)?

I added a simple example: TLS connection to httpbin.org with correct RTC vs. 2x current time to trigger certificate validation failure. Could also be combined with the client example if needed.

[ivmarkov]

Hi @ivmarkov, CI is passing. Could you offer your guidance on the 2 items below:

1. std support
   Unclear to me what is the plan/vision for std platforms, particularly esp-idf integration:

• "ESP-IDF is also supported and in that case mbedtls-rs-sys becomes just an alias for esp-idf-sys and uses the MbedTLS library which is built-in inside ESP-IDF."
• Meaning that we should not even expose timer/clock hooks and related traits to espidf platforms right? But we should expose them to std (other than espidf) platforms?

Yes, exactly. The same is done for the accel hooks.

• Do we need default MbedtlsTimer/MbedtlsWallClock trait implementations for std (but non-espidf)? embassy-time could be used for MbedtlsTimer but so can std::time right?
• basically, what specifically needs to be done for std (and by extension esp-idf targets) in this MR?

For std, where std != ESP-IDF we just need to do exactly the same thing we do for no-std -> expose the two traits if the relevant features are enabled.

For ESP-IDF mbedtls: we can't re-compile it on the fly so easily as we do for the other platforms here, because its shape is controlled in a completely different manner (using esp-idf kconfig values) that I don't think we want to introduce to this project.

The reason why esp-idf - while being "std" - is treated in a special way is that we would like there to re-use the MbedTLS C lib which is coming with the ESP-IDF SDK itself. This version of the C lib already has hardware-accel built-in, and wired timer and date-time.

In future we might also - and only for STD where STD != esp-idf - default the timer and date-time functions to their STD equivalents. Not sure we necessarily want to do this right now.

Sorry in advance if the answer to these questions is trivial. I'm quite new to this esp on rust ecosystem.

2. Bindings metadata approach

Examples with timer/wall-clock need rebuilt bindings (opt-in features not in pre-compiled bindings/libs).

Solution implemented:

• CI builds two artifact sets: one only with crypto hooks as before and one with extra timer/wall-clock for examples
• Generated .toml metadata describes included hooks for each artifact.
• Build script compares needed hooks against metadata. Re-compiles if needed hooks != metadata hooks
• Examples download artifacts from generation job (no on-the-fly compilation)

Hmmm, am I missing something? Can't see how that would work?

Currently, mbedtls-rs-sys is designed in such a way, that it can support just one set of vendored artefacts (.a libs + bindgen .rs files).

If you provide two sets, the second set of artefacts will override the first one. For this not to happen, you need to implement something like this. However, given that:
(a) Supporting "artefact-per-set-of-enabled-features" <=> combinatorial explosion of artefacts we need to support + extra complexity + potential crate-size issues once we publish to crates.io
(b) mbedtls-rs can now be compiled on the fly using ONLY clang without the need to have your MCU-specific GCC toolchain pre-installed

... I don't think keeping more that one artefact per Rust target is a worthy goal anymore.
Whoever wants to use/enables a non-default features' set on the sys crate will end up with an on-the-fly compilation of the .a libs with clang. And that should be good enough.

This deviates from the main purpose of this PR. Should this be a separate PR and is the approach acceptable? Or should use on the fly compilation when building examples (need fetch submodule & setup espressif clang)?

The latter. Need to fetch submodules and setup espressif clang. But isn't this done anyway? Remember, we also have STD examples. And these - by necessity - use on-the-fly compilation, which means git submodules are initialized. The espressif clang might not be there in the examples CI job, but you can copy it over from the xtask libraries pre-compilation job.

I added a simple example: TLS connection to httpbin.org with correct RTC vs. 2x current time to trigger certificate validation failure. Could also be combined with the client example if needed.

Keeping it separate for now is just fine.

[ericschaal]

Hi @ivmarkov, I think there is a misunderstanding about the bindings metadata approach.

this MR uses the build-mbedtls job (xtask gen) (which already has all build dependencies: espup, clang, initialized submodules) to generate two sets of bindings:

1. Library bindings (hardware accel hooks only) → uploaded as mbedtls-rs-sys artifact
2. Example bindings (hardware accel + timer + wall-clock) → uploaded as mbedtls-rs-sys-examples artifact

The build-mcu job (which builds & runs examples) downloads the example bindings artifact to build examples.

Only the library bindings get committed to the repository. The example bindings are ephemeral CI artifacts with 1-day retention, they're never published.

Alternative approach (what you are proposing I believe): Force on-the-fly compilation in the build-mcu job by adding steps to:

• Initialize git submodules (git submodule update --init --recursive)
• Install espup and espressif clang (copy from build-mbedtls job setup)

Both approaches should work, but the current approach is likely faster since it piggybacks on the already-installed toolchain and fetched submodules from build-mbedtls, avoiding redundant setup in build-mcu.

[ivmarkov]

Hi @ivmarkov, I think there is a misunderstanding about the bindings metadata approach.

this MR uses the build-mbedtls job (xtask gen) (which already has all build dependencies: espup, clang, initialized submodules) to generate two sets of bindings:

1. Library bindings (hardware accel hooks only) → uploaded as mbedtls-rs-sys artifact
2. Example bindings (hardware accel + timer + wall-clock) → uploaded as mbedtls-rs-sys-examples artifact

The build-mcu job (which builds & runs examples) downloads the example bindings artifact to build examples.

Only the library bindings get committed to the repository. The example bindings are ephemeral CI artifacts with 1-day retention, they're never published.

Alternative approach (what you are proposing I believe): Force on-the-fly compilation in the build-mcu job by adding steps to:

• Initialize git submodules (git submodule update --init --recursive)
• Install espup and espressif clang (copy from build-mbedtls job setup)

Both approaches should work, but the current approach is likely faster since it piggybacks on the already-installed toolchain and fetched submodules from build-mbedtls, avoiding redundant setup in build-mcu.

I agree both should work.
What is giving me a bit of an an uncomfortable feeling is that this PR is currently maybe making a bit more changes than strictly necessary to just "provide time/wall-clock support to mbedtls as opt-in features".

Here are a few:

• A bunch of new .toml files with hooks = 15. Why?
• Some .a files committed as part of this PR, but no corresponding bindgen-generated .rs files. And then not all .a but just some? UPDATE: Given that the approach is "opt-in" why are we even having changes to the .a files (and yet - no changes to the bindgen-d files?)
• The "build-libs" CI which is anyway the most fragile one (with state) is touched. I don't understand all the details of this stuff in there (was not written by me) hence I feel uneasy we are touching it. Do we strictly need to do that? CI might be faster, but would it, really, by how much, and is it worth it? For example: at one place in the "build-libs" you have replaced - in the artefacts - the /src directory with the /include directory, and I don't understand why EDIT: Got it now, and this change is good - we are otherwise putting "too much" in the artefact.

[ericschaal]

A bunch of new .toml files with hooks = 15. Why?

It's the serialized hook Set that was used to generate the bindings. It is read by build.rs to determine if we need on the fly compilation (i.e requested hooks != pregen hooks). It is necessary to avoid on the fly compilation when running the example job (that way build.rs knows the time/wall-clock hooks are part of bindings!)

Some .a files committed as part of this PR, but no corresponding bindgen-generated .rs files. And then not all .a but just some? UPDATE: Given that the approach is "opt-in" why are we even having changes to the .a files (and yet - no changes to the bindgen-d files?)

Yeah that's unexpected, not sure if I compiled everything properly. I'll make sure to clean things up before undrafting.

totally fair, I'll make the changes. And maybe submit a separate PR with the CI/builder changes if deemed necessary.

[ivmarkov]

@ericschaal Thanks!

One thing before merge which still worries me slightly: the seconds'-resolution timer of mbedtls requires us to define a global symbol named "time". Literally, and unprefixed by the usual mbedtls_ prefix. Wouldn't that conflict with the same time symbol that libc on STD platforms exposes?

[ericschaal]

Yeah, that's probably not great.

We could compile out the time definition using #[cfg(target_os = "none")] (https://doc.rust-lang.org/reference/conditional-compilation.html#target_os):

#[cfg(target_os = "none")]
#[no_mangle]
unsafe extern "C" fn time(t: *mut mbedtls_time_t) -> mbedtls_time_t {

This should exclude ourtime from std targets, since #[cfg(not(std))] isn't a thing unfortunately. The concern would be a bare-metal target (target_os = "none") that for some reason provides its own time()... our definition
would then conflict with it.

Option 2: Add a std feature to mbedtls-rs-sys, enabled on std targets, then use #[cfg(not(feature = "std"))]. A bit heavy just for this, but could be useful for other things down the line.

I don't think using MBEDTLS_PLATFORM_TIME_ALT helps as platform.h sets MBEDTLS_PLATFORM_STD_TIME = time, so platform.c still takes a linker dependency on time. Overriding It (using -DMBEDTLS_PLATFORM_STD_TIME=something_else) requires detecting no-std, which brings us back to option 1/2.

fyi MBEDTLS_PLATFORM_TIME_ALT enables mbedtls_platform_set_time(), allowing to pass a function pointer to a time fn.

could be a use case of #[linkage = "weak"] which sounds cool but only available on the nightly compiler: rust-lang/rust#29603. Maybe using some kind of C shim could work??

I'm leaning towards option 1, what do you think?

EDIT: I'm able to compile std example on x86_64-unknown-linux-gnu just fine (no conflicts). I'm looking deeper into this. probably because glibc is dynamically linked. Your concern is still valid.

[ivmarkov]

@ericschaal

Sorry for the late reply - been quite busy these days.

A super-simple solution is to just -DMBEDTLS_PLATFORM_STD_TIME=mbedtls_sec_time (or a similar name) in builder.rs.

This would effectively force MbedTLS to do almost the same it does for the milliseconds time function - i.e. look for a pre-defined symbol called mbedtls_sec_time.

We can make the solution more complex by also -DMBEDTLS_PLATFORM_TIME_ALT, but not sure we would need that.

[ericschaal]

Good idea!

Unfortunately we have to define MBEDTLS_PLATFORM_TIME_ALT
because of check_config.h requiring it:

#if defined(MBEDTLS_PLATFORM_STD_TIME) &&\
    ( !defined(MBEDTLS_PLATFORM_TIME_ALT) ||\
        !defined(MBEDTLS_HAVE_TIME) )
#error "MBEDTLS_PLATFORM_STD_TIME defined, but not all prerequisites"
#endif

but yes should work!

[ivmarkov]

@ericschaal I'm sorry - merging #111 got this PR unmergable (merge conflicts). Can you rebase your work on top of newest main?