ports/esp32: Enable mbedtls cert time validation. #13100
Conversation
ports/esp32/main.c
Outdated
| #include "mbedtls/version.h" | ||
| #endif | ||
| #ifdef MICROPY_MBEDTLS_PLATFORM_TIME_ALT | ||
| #include "mbedtls/mbedtls_config.h" |
There was a problem hiding this comment.
I think you want to include #include "mbedtls/platform.h" instead.
There was a problem hiding this comment.
mbedtls/platform.h includes mbedtls/platform_time.h, since this only requires mbedtls_platform_set_time I think it's simpler to include platform_time directly.
ports/esp32/main.c
Outdated
| #include "mbedtls/platform_time.h" | ||
| #else | ||
| #include "mbedtls/version.h" | ||
| #endif |
There was a problem hiding this comment.
I don't think this set of includes is needed??
ports/esp32/esp32_common.cmake
Outdated
| @@ -63,6 +63,8 @@ list(APPEND MICROPY_SOURCE_PORT | |||
| mphalport.c | |||
| fatfs_port.c | |||
| help.c | |||
| modtime.c | |||
There was a problem hiding this comment.
This line should not be needed.
ports/esp32/mbedtls/mbedtls_port.c
Outdated
| #include "shared/timeutils/timeutils.h" | ||
|
|
||
|
|
||
| #ifdef MICROPY_MBEDTLS_PLATFORM_TIME_ALT |
There was a problem hiding this comment.
Can't you use #ifdef MBEDTLS_PLATFORM_TIME_ALT here instead?
8407d27 to
93aff62
Compare
ports/esp32/mbedtls/mbedtls_config.h
Outdated
|
|
||
| #define MBEDTLS_HAVE_ASM | ||
|
|
||
| time_t platform_mbedtls_time(time_t *timer); |
There was a problem hiding this comment.
I see that this file is indeed included in the build. But that seems to be by accident: esp-idf/components/mbedtls/port/include/mbedtls/esp_config.h includes mbedtls/mbedtls_config.h which is intended to pick up the default mbedtls_config.h from the mbedtls component of the IDF.
So having this file here will completely override the default set of settings.
Why is this file needed? I don't think the MBEDTLS_HAVE_ASM option does anything, that seems to only be needed for x84 architectures.
ports/esp32/mbedtls/mbedtls_port.c
Outdated
| #include "shared/timeutils/timeutils.h" | ||
| #include "mbedtls/platform_time.h" | ||
|
|
||
| time_t platform_mbedtls_time(time_t *timer) { |
There was a problem hiding this comment.
To keep things simple, you could just put this function in main.c, before the mp_task() function. Then this separate file is not needed, nor are any additional headers like mbedtls_config.h.
There was a problem hiding this comment.
I agree, I've just tested this and it is in fact the simplest option, thanks for the help again 🙏🏼 !!
93aff62 to
6de72bc
Compare
|
This looks much better now. Is it ready to be merged? |
|
Yes I've been testing this and it works as expected 👍🏼 . With this commit now all ports that use mbedtls have cert time validation enabled, so I think a note should be added to otherwise |
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
6de72bc to
30b0ee3
Compare
|
Thanks for testing. Now merged. |
WIP