crypto/mbedtls: Add support for mbedtls 3.x #2122
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirknightj
added
the
test-run
|
Hi @vikramdattu, it seems the CI is failing for some of the jobs, could you take a look? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2122 +/- ##
===========================================
+ Coverage 75.46% 75.51% +0.04%
===========================================
Files 48 48
Lines 14078 14100 +22
===========================================
+ Hits 10624 10647 +23
+ Misses 3454 3453 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @vikramdattu, thanks for looking at the CI. It seems there's still a few issues that need to be reviewed: 0 - clang-format is complaining 1 - the unit tests for TURN seem to be failing. https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/actions/runs/15047512663/job/42300992671?pr=2122 2 - Seems to be a build issue with the Ubuntu with gcc-4.4 path - https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/actions/runs/15047512663/job/42300992482?pr=2122 3 - Seems to be a build issue with M1 mac + gcc - https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/actions/runs/15047512663/job/42300992683?pr=2122 |
|
@sirknightj locally, when tried, the tests seems to be running fine..! Result: I think I should try with Linux. |
Looks like gcc 5.4 is what it is tested oldest! https://github.com/Mbed-TLS/mbedtls?tab=readme-ov-file#tool-versions.
|
|
Hi @vikramdattu, thanks for looking into it. 1 - I'm sure the 4.4 was added for a good reason, maybe we can add a cmake flag for which MbedTLS version (e.g. 2 - Sure we can add that flag to the MbedTLS build, 3 - According to the CI, the failure seems to be related to a missing call to I was able to reproduce on my mac after doing a clean build (
|
|
This change impacts connections with the And using ForceTURN on the JS side, no connection is established. |
|
@sirknightj https://mbed-tls.readthedocs.io/en/latest/security-advisories/mbedtls-security-advisory-2025-03-1/ The hostname verification is on by default for latest mbedtls releases. Though this is not actually required in our case as the turn credentials are securely obtained from kvs endpoint and also RTP data will go over DTLS encryption, I think we should still set the same. I have pushed the commit handling this. |
sirknightj
commented
May 19, 2025
|
Hi @vikramdattu, seems like everything's passing except the gcc-4.4. It looks to still be using the v3 instead of the v2. Since the build is executed as a subprocess, we'll need to forward the flag from the main cmakelists.txt file. Here is a fixed version you can use: diff --git a/CMakeLists.txt b/CMakeLists.txt
index 51f67057b7..9c82dcfd33 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -22,6 +22,7 @@ option(ENABLE_KVS_THREADPOOL "Enable support for KVS thread pool in signaling" O
option(INSTRUMENTED_ALLOCATORS "Enable memory instrumentation" OFF)
option(ENABLE_AWS_SDK_IN_TESTS "Enable support for compiling AWS SDKs for tests" ON)
option(ENABLE_STATS_CALCULATION_CONTROL "Enable support for runtime control of ice agent stat calculations." OFF)
+option(BUILD_OLD_MBEDTLS_VERSION "Use MbedTLS version 2.28.8." OFF)
# Developer Flags
option(BUILD_TEST "Build the testing tree." OFF)
@@ -182,6 +183,7 @@ if(BUILD_DEPENDENCIES)
elseif(USE_MBEDTLS)
set(BUILD_ARGS -DBUILD_STATIC_LIBS=${BUILD_STATIC_LIBS}
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
+ -DBUILD_OLD_MBEDTLS_VERSION=${BUILD_OLD_MBEDTLS_VERSION}
"-DCMAKE_C_FLAGS=${CMAKE_C_FLAGS} -std=c99")
build_dependency(mbedtls ${BUILD_ARGS})
endif()
diff --git a/CMake/Dependencies/libmbedtls-CMakeLists.txt b/CMake/Dependencies/libmbedtls-CMakeLists.txt
index 96ff373093..0c908697ec 100644
--- a/CMake/Dependencies/libmbedtls-CMakeLists.txt
+++ b/CMake/Dependencies/libmbedtls-CMakeLists.txt
@@ -4,7 +4,7 @@ project(libmbedtls-download NONE)
include(ExternalProject)
-if(BUILD_OLD_MBEDTLS_VERSION)
+if (BUILD_OLD_MBEDTLS_VERSION)
SET(MBEDTLS_GIT_TAG "v2.28.8")
else()
SET(MBEDTLS_GIT_TAG "v3.6.3") |
|
@sirknightj thanks for looking this up! Pushed the change. |
- mbedtls 2.8.x is getting out of support: https://github.com/Mbed-TLS/mbedtls/releases/tag/mbedtls-2.28.10 - Clone mbedtls 3.6.x instead of 2.8.x via CMake dependencies - Add related code to mbedtls usage keeping the 2.8.x support intact under mbedtls version macros
- This release handles mbedtls_3.x version support and has some fixes - Cleanup: removed libwebsocket patches as they are not needed anymore
- New API tlsSessionStartWithHostname can receive optional hostname and set the same - It is recommened to set the hostname and is on by default for mbedtls v3.6.3 and above - Since we receive ICE server credentials via secure API and anyway are use DTLS as WebRTC standard, we could skip this, but let's follow the recommendation as precaution
- Newer(3.6.x) mbedtls versions does not test builds on GCC versions as old as 4.4 - We keep this test for older mbedtls version (2.28.x)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
What was changed?
Why was it changed?
How was it changed?
What testing was done for the changes?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.