Support for mbedTLS 3.6.2 #651
Conversation
WalkthroughThis pull request introduces comprehensive updates to the IoTivity-Lite project's mbedTLS library integration, focusing on upgrading and enhancing support for mbedTLS version 3.6.2. The changes span multiple files across the project, introducing new configuration options, patches, and compatibility modifications to support the newer library version while maintaining backward compatibility with version 3.5.0. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (b05fad0), label this PR with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
patches/mbedtls/3.6/01-ocf-anon-psk.patch (1)
185-187: Suppressing Compiler Warning by CastingIn
block_cipher_df, the assignmentkey[i] = (unsigned char)i;is used to avoid a compiler warning about narrowing conversion frominttounsigned char.This is acceptable, but consider adding a comment to explain the purpose of the cast for future maintainability.
patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (2)
22-25: Include Guard Missing formbedtls_oc_platform.hThe inclusion of
mbedtls_oc_platform.his not protected by an include guard or a check to verify its existence. This could lead to compilation errors if the file is not present.Wrap the inclusion with a check to ensure that the header file exists or is appropriately guarded.
+#ifdef MBEDTLS_OC_PLATFORM_H #include "mbedtls_oc_platform.h" +#endif
1070-1072: Redundant Redefinition of Standard FunctionsDefining
MBEDTLS_PLATFORM_STD_SNPRINTFassnprintfwhile also definingMBEDTLS_PLATFORM_SNPRINTF_ALTmay lead to confusion or conflicts.Review the configuration to ensure that the standard functions are defined appropriately and that alternative implementations are only used when necessary.
deps/mbedtls-patch.cmake (1)
85-88: Consider making the version check more explicit.The current condition
BUILD_MBEDTLS_FORCE_3_6_2 OR BUILD_MBEDTLS_FORCE_3_5_0implies these header files are only needed for these specific versions. Consider documenting why these files are needed for these versions but not for 3.1.0.port/linux/Makefile (1)
121-133: Consider deduplicating the source file lists.The source files for version 3.6.2 are identical to version 3.5.0. Consider extracting these into a variable to avoid duplication and make maintenance easier.
Example:
MBEDTLS_3_5_PLUS_FILES = bignum_core.c \ ssl_client.c ssl_debug_helpers_generated.c ssl_tls12_client.c ssl_tls12_server.c \ x509write.c ifeq ($(MBEDTLS_FORCE_3_6_2),1) DTLS += $(MBEDTLS_3_5_PLUS_FILES) else ifeq ($(MBEDTLS_FORCE_3_5_0),1) DTLS += $(MBEDTLS_3_5_PLUS_FILES) else DTLS += ssl_cli.c ssl_srv.c endif endif🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/cmake-linux.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
CMakeLists.txt(2 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(3 hunks)deps/mbedtls.cmake(1 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(1 hunks)port/linux/Makefile(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- deps/mbedtls
🧰 Additional context used
🪛 GitHub Actions: CI building android
port/linux/Makefile
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
🔇 Additional comments (7)
patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (3)
363-364: DefiningMBEDTLS_NO_PLATFORM_ENTROPYMay Reduce Entropy SourcesDefining
MBEDTLS_NO_PLATFORM_ENTROPYdisables the default platform entropy functions. This could limit the entropy sources available, weakening the security of random number generation.Ensure that alternative entropy sources are properly configured through other mechanisms (e.g.,
MBEDTLS_ENTROPY_HARDWARE_ALT) to maintain cryptographic strength.
1070-1072: Inconsistent Use ofsnprintfAcross PlatformsDefining
MBEDTLS_PLATFORM_STD_SNPRINTFassnprintfassumes thatsnprintfis available and behaves consistently on all target platforms.Confirm that
snprintfis standardized and available on all supported platforms. If not, consider providing or using a portable implementation.
1117-1117: Potential Limitation withMBEDTLS_PSK_MAX_LENSet to 32Setting
MBEDTLS_PSK_MAX_LENto 32 bytes may not be sufficient for systems requiring longer pre-shared keys.Assess whether a maximum PSK length of 32 bytes meets the security requirements of all intended use cases. If longer PSKs are necessary, adjust the value accordingly.
deps/mbedtls-patch.cmake (1)
58-62: LGTM!The branch creation and patch application for version 3.6.2 follows the established pattern and is implemented correctly.
port/linux/Makefile (2)
45-46: LGTM!The variable declaration follows the established pattern and is implemented correctly.
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
Line range hint
732-794: Verify patch application for version 3.6.2.The CI pipeline indicates that some patch hunks are failing to apply cleanly. This needs to be investigated and fixed.
Run the following script to check which patches are failing:
✅ Verification successful
Patches for mbedtls v3.6.2 apply cleanly
The patches are compatible with mbedtls v3.6.2 and apply without conflicts. The Makefile's patch application process is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check which patches are failing to apply. # List all patches for version 3.6.2 echo "Available patches for version 3.6.2:" fd . patches/mbedtls/3.6/ -e patch # Try to apply each patch and report failures for patch in $(fd . patches/mbedtls/3.6/ -e patch); do echo "Testing patch: $patch" git apply --check "$patch" || echo "Patch $patch failed to apply" doneLength of output: 1988
Script:
#!/bin/bash # Description: Initialize submodule and verify patches echo "Initializing mbedtls submodule..." git submodule update --init deps/mbedtls echo "Checking out v3.6.2..." cd deps/mbedtls git checkout v3.6.2 echo "Available patches:" fd . ../../patches/mbedtls/3.6/ -e patch echo "Trying to apply patches..." for patch in $(fd . ../../patches/mbedtls/3.6/ -e patch); do echo "Testing patch: $patch" git apply --check "../../$patch" || echo "Patch $patch failed to apply" doneLength of output: 2006
Script:
#!/bin/bash # Description: Verify patches with correct path resolution echo "Initializing mbedtls submodule..." git submodule update --init deps/mbedtls echo "Checking out v3.6.2..." git -C deps/mbedtls checkout v3.6.2 echo "Available patches:" fd . patches/mbedtls/3.6/ -e patch echo "Trying to apply patches..." for patch in $(fd . patches/mbedtls/3.6/ -e patch); do echo "Testing patch: $patch" git -C deps/mbedtls apply --check "../..$patch" || echo "Patch $patch failed to apply" doneLength of output: 1625
Script:
#!/bin/bash # Description: Verify patches with correct path resolution echo "Initializing mbedtls submodule..." git submodule update --init deps/mbedtls echo "Checking out v3.6.2..." git -C deps/mbedtls checkout v3.6.2 echo "Available patches:" fd . patches/mbedtls/3.6/ -e patch echo "Trying to apply patches..." for patch in $(fd . patches/mbedtls/3.6/ -e patch); do echo "Testing patch: $patch" git -C deps/mbedtls apply --check "../../$patch" || echo "Patch $patch failed to apply" doneLength of output: 1117
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
CMakeLists.txt (1)
55-58: LGTM!The deprecation warning is clear and follows CMake conventions for warning messages.
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
b05fad0 to
0f0f907
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deps/mbedtls-patch.cmake (1)
28-32:⚠️ Potential issueFix incorrect variable usage in shallow repository check.
The
execute_processcommand assigns the exit code toRESULT_VARIABLE, but the condition checksIS_SHALLOW. This may cause incorrect behavior sinceIS_SHALLOWis not being set properly.Apply this diff to correct the variable usage:
-execute_process( - COMMAND ${GIT_EXECUTABLE} -C ${MBEDTLS_SRC_DIR} rev-parse --is-shallow-repository - RESULT_VARIABLE IS_SHALLOW - OUTPUT_QUIET -) +execute_process( + COMMAND ${GIT_EXECUTABLE} -C ${MBEDTLS_SRC_DIR} rev-parse --is-shallow-repository + OUTPUT_VARIABLE IS_SHALLOW + OUTPUT_STRIP_TRAILING_WHITESPACE +) if(IS_SHALLOW STREQUAL "true")This change captures the command's output (
trueorfalse) inIS_SHALLOWand adjusts the condition accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/cmake-linux.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
CMakeLists.txt(2 hunks)deps/mbedtls-patch.cmake(3 hunks)deps/mbedtls.cmake(1 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(1 hunks)port/linux/Makefile(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deps/mbedtls.cmake
🔇 Additional comments (17)
patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (7)
35-35: Consider the impact of disabling assembly optimizations.By commenting out
#define MBEDTLS_HAVE_ASMon line 35, assembly optimizations are disabled. This may lead to reduced performance, especially on platforms where assembly provides significant speed improvements for cryptographic operations.Please verify if disabling assembly optimizations is necessary. If there are no compatibility issues, consider keeping
MBEDTLS_HAVE_ASMenabled to benefit from performance enhancements.
452-453: Re-evaluate the need to disable TLS 1.3 support.By commenting out
#define MBEDTLS_SSL_PROTO_TLS1_3on line 453, you are disabling TLS 1.3 support. TLS 1.3 offers significant security and performance improvements over previous versions.Unless there is a specific reason to disable TLS 1.3 (e.g., compatibility with legacy systems), consider keeping it enabled to leverage its benefits.
154-169: Review the disabling of cipher modes and padding schemes.The block from lines 154 to 169 comments out several cipher modes (
CFB,CTR,OFB,XTS) and padding schemes (PKCS7,ONE_AND_ZEROS,ZEROS_AND_LEN,ZEROS).Ensure that these cipher modes and padding schemes are not required by your application or any protocols you support. Disabling them may limit cryptographic capabilities or interoperability with other systems.
723-725: Verify the impact of disabling MD5 support.At lines 724 to 725,
MBEDTLS_MD5_Cis commented out, disabling MD5 support. While MD5 is considered insecure and is deprecated, some legacy systems or protocols might still require it.Please confirm that disabling MD5 will not affect interoperability with any systems that may still use it.
1029-1030: Adjust entropy source maximum to match application requirements.At lines 1029 to 1030,
MBEDTLS_ENTROPY_MAX_SOURCESis set to2. The default value allows for more entropy sources.Ensure that this value is sufficient for your application's entropy needs. If additional entropy sources are used elsewhere in the code, consider increasing this value accordingly.
117-117:⚠️ Potential issueEnsure no deprecated functions are used before removing them.
Defining
MBEDTLS_DEPRECATED_REMOVEDon line 117 removes all deprecated functions and features from Mbed TLS. This might lead to build errors if any deprecated APIs are still in use within the codebase.Please ensure that your project does not rely on any deprecated functions. If deprecated functions are still in use, consider defining
MBEDTLS_DEPRECATED_WARNINGinstead to receive compiler warnings without breaking the build.
816-817:⚠️ Potential issueConditional inclusion may omit necessary functionality.
On lines 816 to 817,
MBEDTLS_PK_WRITE_Cis defined only whenOC_PKIis enabled.If your application requires public key write functions for operations other than those under
OC_PKI, they will be unavailable whenOC_PKIis not defined.Please verify whether
MBEDTLS_PK_WRITE_Cshould be enabled unconditionally to ensure required functionalities are available.deps/mbedtls-patch.cmake (1)
37-38: Ensure submodules are properly initialized after checkout.After checking out version
v3.6.2on line 37, you might need to update the submodules within the Mbed TLS repository to match this version.Consider adding:
execute_process(COMMAND ${GIT_EXECUTABLE} -C ${MBEDTLS_SRC_DIR} submodule update --init --recursive)This ensures that all submodules are properly synced after the checkout.
port/linux/Makefile (2)
122-125: Verify inclusion of necessary source files for Mbed TLS 3.6.2.In the conditional block starting at line 122, additional source files are added to the
DTLSvariable for Mbed TLS 3.6.2.Ensure that all required source files for Mbed TLS 3.6.2 are included, and no necessary files are missing. Missing files may lead to build errors or incomplete functionality.
737-745:⚠️ Potential issueCorrect shell command syntax in the patch application section.
The use of
&&afterfion line 745 may cause a syntax error in the shell script within the Makefile.Replace the
&&afterfiwith a semicolon;or restructure the commands to ensure proper execution.Apply this diff:
fi && \ git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ + cd ${IOTIVITY_SRC_DIR} && \ + git reset HEAD deps/mbedtls && \ cd ${MBEDTLS_SRC_DIR} && \ for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ echo "Patches applied in $^" > ${@F}This adjustment ensures that the commands execute correctly without syntax errors.
Likely invalid or redundant comment.
patches/mbedtls/3.6/01-ocf-anon-psk.patch (5)
9-12: LGTM! Essential configuration check added.The added configuration check ensures that
MBEDTLS_ECDH_Cis defined whenMBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDis enabled, preventing potential build failures.
25-28: LGTM! Improved readability of IV size calculation.The modification removes unnecessary parentheses while maintaining the same functionality.
206-217: LGTM! Well-structured cipher suite implementation.The implementation correctly:
- Checks for required features (MBEDTLS_AES_C, MBEDTLS_MD_CAN_SHA256)
- Specifies TLS 1.2 version constraints
- Uses appropriate cipher and hash algorithms
423-488: LGTM! Improved PSK identity handling.The changes enhance the PSK identity hint handling by:
- Adding proper PSK configuration checks
- Implementing identity verification
- Adding proper error propagation
- Using constant-time comparison for identity verification
141-142: Verify the cipher suite identifier.The cipher suite identifier 0xFF00 is in the private use range. Ensure this doesn't conflict with other custom cipher suites in your deployment.
✅ Verification successful
The cipher suite identifier 0xFF00 is safe to use.
The identifier is unique within the TLS context and doesn't conflict with any other usage in the codebase. Other occurrences of similar values are in unrelated contexts (CoAP message handling).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other usage of the 0xFF00 identifier rg -l "0xFF00" --type cpp --type cLength of output: 116
Script:
#!/bin/bash # Check the context of 0xFF00 usage in the found files rg "0xFF00" --type cpp --type c -B 2 -A 2Length of output: 1158
CMakeLists.txt (2)
55-57: LGTM! Clear deprecation warning.The warning message clearly informs users about the deprecation of MbedTLS v3.5.0 and future removal plans.
45-46: 🛠️ Refactor suggestionConsider setting the default value to OFF for backward compatibility.
Setting
BUILD_MBEDTLS_FORCE_3_6_2to ON by default might break existing builds. Consider setting it to OFF and letting users explicitly opt-in to version 3.6.2.Apply this diff:
-set(BUILD_MBEDTLS_FORCE_3_6_2 ON CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)") +set(BUILD_MBEDTLS_FORCE_3_6_2 OFF CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/unittest/tcptest.cpp(3 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- deps/mbedtls
🚧 Files skipped from review as they are similar to previous changes (1)
- deps/mbedtls-patch.cmake
🔇 Additional comments (1)
api/unittest/tcptest.cpp (1)
37-38: LGTM! Headers are correctly included.The new mbedTLS headers are properly guarded by the OC_SECURITY macro and logically ordered with other security-related includes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
security/oc_certs_internal.h (1)
Line range hint
60-81: LGTM! Consider enhancing the documentation.The conditional compilation and function declaration look good. The version check is correctly implemented, and the function is properly documented with parameter descriptions and return values.
Consider adding a brief example usage in the documentation to help users understand how to use this function correctly:
// Example usage: unsigned char buffer[BUFFER_SIZE]; int ret = oc_certs_parse_private_key(device_index, cert_pem, cert_pem_len, buffer, sizeof(buffer)); if (ret < 0) { // Handle error }security/unittest/obt_certstest.cpp (1)
157-162: Consider adding more error test cases.While the current tests verify successful private key extraction, consider adding tests for:
- Invalid certificate formats
- Corrupted certificates
- Buffer overflow scenarios
Also applies to: 287-292, 433-438
🧰 Tools
🪛 GitHub Actions: Check formatting
[error] 157-157: Code formatting issues detected: incorrect spacing around < operator in preprocessor conditions
security/unittest/certsgeneratetest.cpp (1)
345-345: Consider removing debug output from test.The
std::coutstatement might clutter the test output and is not necessary for the test's functionality.- std::cout << pem.data() << std::endl;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check formatting
security/unittest/obt_certstest.cpp
[error] 157-157: Code formatting issues detected: incorrect spacing around < operator in preprocessor conditions
[error] 287-287: Code formatting issues detected: incorrect spacing around < operator in preprocessor conditions
[error] 433-433: Code formatting issues detected: incorrect spacing around < operator in preprocessor conditions
⏰ Context from checks skipped due to timeout of 90000ms (70)
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_VERSION_1_1_0_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_DEBUG_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: cmake_linux (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF) / unit-test-with-cfg
🔇 Additional comments (2)
security/unittest/certsgeneratetest.cpp (1)
268-268: LGTM! Good test coverage improvement.The addition of the serial number size initialization and the test case for certificates without serial numbers improves the robustness of the test suite.
Also applies to: 305-306
security/oc_certs.c (1)
Line range hint
185-222: LGTM! Well-structured implementation.The implementation is robust with proper error handling, memory management, and clear error messages. The conditional compilation is correctly implemented to maintain compatibility with older mbedTLS versions.
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
2 times, most recently
from
aab1587 to
f77390d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
patches/mbedtls/3.6/01-ocf-anon-psk.patch.backup (2)
293-310:⚠️ Potential issueAdd NULL Check for
confParameter inmbedtls_ssl_conf_ekusThe function
mbedtls_ssl_conf_ekuslacks a NULL check for theconfparameter. Dereferencing aNULLpointer could lead to undefined behavior or a crash. It's important to validate inputs to prevent such issues.Apply this diff to fix the issue:
+ if (conf == NULL) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + }
293-310:⚠️ Potential issueEnsure OID Data Lifespan Matches SSL Configuration
In
mbedtls_ssl_conf_ekus, the function stores pointers to the OID data directly in thembedtls_ssl_configstructure without copying them. If the OID data is modified or freed after the function call, this could lead to dangling pointers and undefined behavior.Consider copying the OID data into the configuration structure or clearly document that the caller must ensure the OID data remains valid for the lifetime of the
mbedtls_ssl_config.Apply this diff to copy the OID data:
+ conf->client_oid = mbedtls_calloc(1, client_oid_len); + if (conf->client_oid == NULL) { + return MBEDTLS_ERR_SSL_ALLOC_FAILED; + } + memcpy(conf->client_oid, client_oid, client_oid_len); + conf->client_oid_len = client_oid_len; + /* Similarly for server_oid */Ensure to include appropriate cleanup code to free the allocated memory when the configuration is no longer needed.
CMakeLists.txt (1)
45-45: 🛠️ Refactor suggestionConsider setting the default value to OFF for backward compatibility.
Setting BUILD_MBEDTLS_FORCE_3_6_2 to ON by default might break existing builds. Consider setting it to OFF and letting users explicitly opt-in to version 3.6.2.
Apply this diff:
-set(BUILD_MBEDTLS_FORCE_3_6_2 ON CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)") +set(BUILD_MBEDTLS_FORCE_3_6_2 OFF CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)")patches/mbedtls/3.6/01-ocf-anon-psk.patch (1)
293-310:⚠️ Potential issueAdd NULL Pointer Check for
confParameter inmbedtls_ssl_conf_ekusThe function
mbedtls_ssl_conf_ekusdoes not check if theconfpointer isNULLbefore dereferencing it. This could lead to a null pointer dereference ifconfisNULL, resulting in undefined behavior.Apply this diff to add the NULL pointer check:
+ if (conf == NULL) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + } if ((client_oid_len == 0 && client_oid) || (client_oid_len != 0 && !client_oid)|| (server_oid_len == 0 && server_oid) ||
🧹 Nitpick comments (2)
patches/mbedtls/3.6/01-ocf-anon-psk.patch.backup (1)
9-13: Consider Security Implications of Enabling Anonymous ECDH Key ExchangeThe addition of
MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDand the associated cipher suiteMBEDTLS_TLS_ECDH_ANON_WITH_AES_128_CBC_SHA256introduces support for anonymous ECDH key exchange. This key exchange method does not provide authentication and is vulnerable to man-in-the-middle attacks, potentially compromising the security of the communication.It's crucial to carefully assess whether enabling anonymous ECDH key exchange aligns with your application's security requirements and policies. If not strictly necessary, consider avoiding its use to maintain strong security guarantees.
Also applies to: 132-136
api/unittest/tcptest.cpp (1)
Line range hint
279-318: Remove Null Terminator from Personalization StringIn the initialization of the
persvector used for seeding the CTR-DRBG context, a null terminator'\0'is included:std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' };Since the personalization string is treated as a binary buffer, including a null terminator is unnecessary and may affect the perceived length of the string.
Apply this diff to remove the null terminator:
- std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' }; + std::vector<unsigned char> pers = { 't', 'e', 's', 't' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/cmake-linux.ymlis excluded by!**/*.yml
📒 Files selected for processing (13)
CMakeLists.txt(2 hunks)api/unittest/tcptest.cpp(4 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(3 hunks)deps/mbedtls.cmake(1 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch.backup(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(1 hunks)port/linux/Makefile(4 hunks)security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- security/oc_certs_internal.h
- deps/mbedtls
- deps/mbedtls.cmake
- security/unittest/obt_certstest.cpp
- deps/mbedtls-patch.cmake
- patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch
🧰 Additional context used
🪛 GitHub Actions: CI building android
port/linux/Makefile
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
🔇 Additional comments (16)
patches/mbedtls/3.6/01-ocf-anon-psk.patch.backup (1)
285-291:⚠️ Potential issueAdd NULL Check for
confParameter inmbedtls_ssl_conf_iterate_own_certsThe function
mbedtls_ssl_conf_iterate_own_certsdoes not check if theconfparameter isNULLbefore dereferencing it. This could lead to a null pointer dereference ifNULLis passed to the function.Apply this diff to add the NULL check:
+ if (conf == NULL) { + return; + }Likely invalid or redundant comment.
api/unittest/tcptest.cpp (1)
Line range hint
279-318: Entropy Seeding Implemented CorrectlyThe entropy context and CTR-DRBG context are correctly initialized, seeded, and freed. This ensures secure random number generation required for SSL/TLS operations.
security/unittest/certsgeneratetest.cpp (3)
268-268: LGTM: Consistent serial number size initialization.Setting serial_number_size to 20 ensures consistent behavior across test cases.
305-306: LGTM: Good test coverage for edge case.Testing certificate generation without a serial number improves test coverage.
322-323: LGTM: Proper version-specific test handling.Correctly guards private key extraction tests for mbedTLS versions below 3.6.2, as this functionality is not supported in newer versions.
Also applies to: 351-352
security/oc_certs.c (1)
Line range hint
185-222: LGTM: Clean version-specific implementation.Properly guards private key extraction functionality for mbedTLS versions below 3.6.2. The implementation includes appropriate error handling and memory management.
port/linux/Makefile (3)
45-45: LGTM: Clear version control flag.The MBEDTLS_FORCE_3_6_2 flag provides a clean way to control mbedTLS version selection.
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
121-133: LGTM: Proper source file handling.Correctly manages the inclusion of version-specific DTLS source files based on the selected mbedTLS version.
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
732-751: LGTM: Clean patch application process.The patch application process for mbedTLS 3.6.2 follows good practices:
- Cleans the directory before applying patches
- Uses proper git commands for version control
- Applies patches in a sorted order
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 428: Build failed during patching of mbedTLS - multiple patch hunks failed to apply cleanly
CMakeLists.txt (1)
55-57: LGTM: Clear deprecation warning.Good practice to warn users about the upcoming removal of mbedTLS 3.5.0 support.
patches/mbedtls/3.6/01-ocf-anon-psk.patch (6)
9-12: Correct Addition of Prerequisite Check for ECDH_ANON Key ExchangeThe added error check ensures that
MBEDTLS_ECDH_Cis defined whenMBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDis set. This prevents configuration errors related to missing dependencies.
142-142: Verify the Cipher Suite Code Point AssignmentThe cipher suite
MBEDTLS_TLS_ECDH_ANON_WITH_AES_128_CBC_SHA256is assigned the code point0xFF00, which is within the private use range according to RFC 8446. Ensure that this code point does not conflict with existing cipher suites and that all interoperating parties agree on this assignment.
423-486: Ensure Proper Error Handling inssl_parse_server_psk_hintThe updated implementation of
ssl_parse_server_psk_hintimproves error handling by correctly returning error codes when an unknown PSK identity is encountered. This change ensures that the function properly communicates failure states to the caller.
595-616: Consistent Inclusion of ECDH_ANON in Conditional CompilationThe addition of
MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDto the conditional compilation directives ensures that code related to the ECDH anonymous key exchange is included only when this feature is enabled. This maintains consistency and prevents unintended inclusion or exclusion of code segments.
178-187: Cast Integer to Unsigned Char inctr_drbg.cThe explicit cast of
itounsigned charinkey[i] = (unsigned char)i;ensures that there is no unintended data loss or sign extension when assigning values to thekeyarray.
620-648: Validate PSK Identity Length inssl_prepare_server_key_exchangeThe added check for
ssl->conf->psk_identity_len > UINT16_MAXensures that the PSK identity length does not exceed the maximum allowed value, preventing potential overflow issues. Additionally, the code now correctly includes the PSK identity hint in the ServerKeyExchange message for PSK-based key exchanges.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
patches/mbedtls/3.6/01-ocf-anon-psk.patch (1)
293-310:⚠️ Potential issueAdd NULL pointer check for
confparameter.The
mbedtls_ssl_conf_ekusfunction should validate theconfparameter before accessing it.Apply this diff:
int mbedtls_ssl_conf_ekus(mbedtls_ssl_config *conf, const char *client_oid, size_t client_oid_len, const char *server_oid, size_t server_oid_len) { + if (conf == NULL) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + } if ((client_oid_len == 0 && client_oid) || (client_oid_len != 0 && !client_oid)|| (server_oid_len == 0 && server_oid) ||
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/cmake-linux.ymlis excluded by!**/*.yml
📒 Files selected for processing (11)
CMakeLists.txt(2 hunks)api/unittest/tcptest.cpp(4 hunks)deps/mbedtls-patch.cmake(3 hunks)deps/mbedtls.cmake(1 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(1 hunks)port/linux/Makefile(4 hunks)security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- security/oc_certs_internal.h
- deps/mbedtls-patch.cmake
- deps/mbedtls.cmake
- security/unittest/obt_certstest.cpp
- security/unittest/certsgeneratetest.cpp
- patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch
🔇 Additional comments (16)
api/unittest/tcptest.cpp (3)
27-27: LGTM! Added required headers for random number generation and entropy.The new headers are correctly added to support the random number generation and entropy functionality.
Also applies to: 36-36
66-66: LGTM! Added proper initialization and cleanup of random number generator.The random number generator is correctly initialized in
SetUpTestCaseand cleaned up inTearDownTestCase.Also applies to: 71-71
317-318: LGTM! Added proper cleanup of entropy and CTR-DRBG contexts.The entropy and CTR-DRBG contexts are correctly freed in the reverse order of their initialization.
security/oc_certs.c (1)
Line range hint
185-222: LGTM! Added version-specific private key extraction.The private key extraction functions are correctly guarded by version check and will only be compiled for mbedTLS versions < 3.6.2. The implementation includes proper error handling and memory management.
port/linux/Makefile (3)
121-133: LGTM! Added conditional DTLS source files configuration.The DTLS source files are correctly configured based on the mbedTLS version, with proper fallback to older versions.
732-751: LGTM! Added patch application logic for mbedTLS 3.6.2.The patch application logic for version 3.6.2 is correctly implemented with proper git operations and error handling.
45-45: 🛠️ Refactor suggestionConsider setting MBEDTLS_FORCE_3_6_2 to 0 by default.
Setting
MBEDTLS_FORCE_3_6_2to 1 by default might break existing builds. Consider setting it to 0 and letting users explicitly opt-in to version 3.6.2.Apply this diff:
-MBEDTLS_FORCE_3_6_2 ?= 1 +MBEDTLS_FORCE_3_6_2 ?= 0Likely invalid or redundant comment.
CMakeLists.txt (2)
55-58: LGTM! Added deprecation warning for mbedTLS 3.5.0.The warning message about the deprecation of mbedTLS 3.5.0 is clear and informative.
45-45: 🛠️ Refactor suggestionConsider setting BUILD_MBEDTLS_FORCE_3_6_2 to OFF by default.
Setting
BUILD_MBEDTLS_FORCE_3_6_2to ON by default might break existing builds. Consider setting it to OFF and letting users explicitly opt-in to version 3.6.2.Apply this diff:
-set(BUILD_MBEDTLS_FORCE_3_6_2 ON CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)") +set(BUILD_MBEDTLS_FORCE_3_6_2 OFF CACHE BOOL "Force v3.6.2 of the MbedTLS library to be used (by default v3.5.0 is used by master)")Likely invalid or redundant comment.
patches/mbedtls/3.6/01-ocf-anon-psk.patch (7)
9-12: LGTM! Proper prerequisite check added.The configuration check ensures that MBEDTLS_ECDH_C is defined when ECDH_ANON is enabled, which is essential for the anonymous key exchange functionality.
141-142: Verify security implications of anonymous key exchange.While the cipher suite implementation is correct, anonymous key exchange (ECDH_ANON) removes peer authentication, making it vulnerable to man-in-the-middle attacks. Ensure this is used only in scenarios where authentication is handled at a different layer or security level.
Also applies to: 198-199
423-488: LGTM! Improved PSK hint parsing and error handling.The changes enhance the robustness of PSK hint parsing by:
- Properly validating PSK configuration
- Adding proper error handling and propagation
- Improving security by using constant-time comparison for identity verification
565-570: LGTM! Proper handling of anonymous key exchange.The code correctly skips signature verification for ECDH_ANON cipher suites, which is the expected behavior for anonymous key exchange.
638-648: LGTM! Enhanced PSK identity handling.The changes improve PSK identity handling by:
- Adding proper length validation against UINT16_MAX
- Including the actual PSK identity instead of empty bytes
- Using proper memory copying with bounds
171-173: LGTM! Simplified type casting.Removed unnecessary type casting in PSA_KEY_LIFETIME_GET_LOCATION macro, improving code clarity.
185-186: LGTM! Improved type safety in key initialization.Added explicit casting to unsigned char, preventing potential truncation warnings while maintaining the same functionality.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
port/linux/Makefile (2)
121-126: Fix indentation for consistency.The indentation of the added DTLS files is inconsistent with the rest of the Makefile.
Apply this diff to fix the indentation:
ifeq ($(MBEDTLS_FORCE_3_6_2),1) -DTLS += bignum_core.c \ - pk_ecc.c \ - ssl_client.c ssl_debug_helpers_generated.c ssl_tls12_client.c ssl_tls12_server.c \ - x509write.c +DTLS += bignum_core.c pk_ecc.c \ + ssl_client.c ssl_debug_helpers_generated.c ssl_tls12_client.c ssl_tls12_server.c \ + x509write.c
732-751: Improve git fetch reliability.The git fetch command uses
--unshallow --tags || git fetch --allwhich is good for handling both shallow and deep clones. However, the error handling could be improved.Apply this diff to improve the git fetch reliability:
- (git fetch --unshallow --tags || git fetch --all) && \ + (git fetch --unshallow --tags || git fetch --tags --all) && \This ensures that tags are always fetched, even when falling back to
--all.patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch (3)
306-308: Interoperability Issue: Disabling EC Compressed Key ParsingDisabling
MBEDTLS_PK_PARSE_EC_COMPRESSEDremoves support for parsing EC keys in compressed format, which may limit interoperability with other systems or certificates that use compressed points.Consider re-enabling this feature:
-//#define MBEDTLS_PK_PARSE_EC_COMPRESSED +#define MBEDTLS_PK_PARSE_EC_COMPRESSED
377-379: Diagnostic Capability Reduced by Disabling Self-TestsBy commenting out
MBEDTLS_SELF_TEST, the built-in self-test functions are disabled. These tests can be useful for verifying the correct functionality of cryptographic algorithms, especially in constrained environments.Consider re-enabling self-tests to aid in diagnostics:
-//#define MBEDTLS_SELF_TEST +#define MBEDTLS_SELF_TEST
628-628: Unnecessary Inclusion of Deprecated and Weak CiphersThe library includes support for 3DES by defining
MBEDTLS_DES_C. Since 3DES is considered insecure due to its vulnerability to sweet32 attacks, it's recommended to disable it unless explicitly required.Consider disabling 3DES support:
-#define MBEDTLS_DES_C +//#define MBEDTLS_DES_C
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch(1 hunks)port/linux/Makefile(5 hunks)security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- security/oc_certs_internal.h
- security/unittest/obt_certstest.cpp
- security/oc_certs.c
- security/unittest/certsgeneratetest.cpp
🔇 Additional comments (7)
port/linux/Makefile (2)
45-45: LGTM! Default mbedTLS version set to 3.6.2.The variable declaration follows Makefile conventions and allows for command-line override.
202-202: Investigate the-ffreestandingflag compatibility with C++ targets.The TODO comment suggests potential issues with the
-ffreestandingflag in C++ targets. This should be investigated to ensure proper compilation of C++ files.Run the following script to check for any C++ files that might be affected:
✅ Verification successful
TODO comment is valid and should be kept
The
-ffreestandingflag is currently used in multiple Makefiles but not with C++ targets. This is a valid concern as the flag's behavior differs significantly between C and C++ compilation, particularly regarding standard library features. Given the project's extensive C++ codebase (100+ files), proper investigation of-ffreestandingcompatibility with C++ targets is important for maintaining build integrity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find C++ files in the project that might be affected by -ffreestanding # Find all C++ source files echo "C++ files that might be affected by -ffreestanding:" fd -e cpp -e cxx -e ccLength of output: 5117
Script:
#!/bin/bash # Search for Makefiles and compiler flags echo "=== Searching for -ffreestanding in Makefiles ===" rg -l "\-ffreestanding" --type make echo -e "\n=== Checking C++ compiler flags in Makefiles ===" rg "CXX.*FLAGS|CXXFLAGS" --type makeLength of output: 5519
patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch (5)
1075-1079: Ensure Consistent SSL Buffer SizesThe
MBEDTLS_SSL_IN_CONTENT_LENandMBEDTLS_SSL_OUT_CONTENT_LENmacros are set to(OC_PDU_SIZE)whenOC_DYNAMIC_ALLOCATIONis not defined. Confirm thatOC_PDU_SIZEis appropriately defined and that buffer sizes meet the application's requirements to prevent potential issues with message fragmentation or buffer overflows.Ensure
OC_PDU_SIZEis adequately sized for expected TLS record sizes.
703-704: Security Improvement: Disabling MD5 Hash FunctionDisabling
MBEDTLS_MD5_Cis a good practice since MD5 is considered cryptographically broken. Ensure that no dependencies in your project require MD5.
876-878: Security Improvement: Disabling SHA-1 Hash FunctionDisabling
MBEDTLS_SHA1_Cis advisable because SHA-1 is considered weak and vulnerable to collision attacks. Confirm that all components of the project can operate without SHA-1.
342-343:⚠️ Potential issuePotential Security Risk: Disabling Platform Entropy Sources
Defining
MBEDTLS_NO_PLATFORM_ENTROPYdisables the default platform entropy functions. Unless an alternative entropy source is provided, this may lead to insufficient entropy for cryptographic operations, compromising the security of the generated keys.Ensure that a secure entropy source is configured.
If no alternative entropy source is available, consider not defining
MBEDTLS_NO_PLATFORM_ENTROPY:-#define MBEDTLS_NO_PLATFORM_ENTROPY +//#define MBEDTLS_NO_PLATFORM_ENTROPY
97-99: Compilation Issues: Defining MBEDTLS_DEPRECATED_REMOVEDDefining
MBEDTLS_DEPRECATED_REMOVEDwill cause compilation errors if any deprecated functions are used in the codebase. Ensure that no deprecated functions are being used before enabling this macro, or consider usingMBEDTLS_DEPRECATED_WARNINGinstead to receive warnings rather than errors.Do we have assurance that deprecated functions are not in use? If not, consider adjusting the configuration:
-#define MBEDTLS_DEPRECATED_REMOVED +#define MBEDTLS_DEPRECATED_WARNING✅ Verification successful
Enabling MBEDTLS_DEPRECATED_REMOVED is safe ✅
The codebase has been running with deprecation warnings enabled and shows no usage of deprecated MbedTLS functions. The change is part of a structured update to the MbedTLS configuration and is safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all potential source files that might use MbedTLS fd -e c -e cpp -e h -e hpp # Look for includes of mbedtls headers rg -l "include.*mbedtls" # Search for common deprecated MbedTLS functions and patterns rg -i "mbedtls_.*_ret|mbedtls_deprecated|MBEDTLS_DEPRECATED"Length of output: 29729
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
port/android/Makefile (2)
118-120: Default version comment needs update.The comment "for now use v3.1.0 as default" is misleading since the code now supports multiple versions including 3.5.0 and 3.6.2.
-# for now use v3.1.0 as default +# Supports mbedTLS versions: 3.1.0 (default), 3.5.0, and 3.6.2🧰 Tools
🪛 GitHub Actions: CI building android
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
142-155: Consider using a more maintainable approach for version-specific files.The nested if-else structure for version-specific files could become harder to maintain as more versions are added.
Consider using a variable-based approach:
-ifeq ($(MBEDTLS_FORCE_3_6_2),1) -DTLS += bignum_core.c \ - pk_ecc.c \ - ssl_client.c ssl_debug_helpers_generated.c ssl_tls12_client.c ssl_tls12_server.c \ - x509write.c -else -ifeq ($(MBEDTLS_FORCE_3_5_0),1) -DTLS += bignum_core.c \ - ssl_client.c ssl_debug_helpers_generated.c ssl_tls12_client.c ssl_tls12_server.c \ - x509write.c -else -DTLS += ssl_cli.c ssl_srv.c -endif -endif +MBEDTLS_3_6_2_FILES = bignum_core.c pk_ecc.c ssl_client.c ssl_debug_helpers_generated.c \ + ssl_tls12_client.c ssl_tls12_server.c x509write.c +MBEDTLS_3_5_0_FILES = bignum_core.c ssl_client.c ssl_debug_helpers_generated.c \ + ssl_tls12_client.c ssl_tls12_server.c x509write.c +MBEDTLS_3_1_0_FILES = ssl_cli.c ssl_srv.c + +ifeq ($(MBEDTLS_FORCE_3_6_2),1) +DTLS += $(MBEDTLS_3_6_2_FILES) +else ifeq ($(MBEDTLS_FORCE_3_5_0),1) +DTLS += $(MBEDTLS_3_5_0_FILES) +else +DTLS += $(MBEDTLS_3_1_0_FILES) +endif🧰 Tools
🪛 GitHub Actions: CI building android
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (25)
.github/workflows/CLA.ymlis excluded by!**/*.yml.github/workflows/android.ymlis excluded by!**/*.yml.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/check-format.ymlis excluded by!**/*.yml.github/workflows/cmake-linux.ymlis excluded by!**/*.yml.github/workflows/cmake-windows.ymlis excluded by!**/*.yml.github/workflows/coverity.ymlis excluded by!**/*.yml.github/workflows/ctt-bot.ymlis excluded by!**/*.yml.github/workflows/ctt-test.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish-with-cfg.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen.ymlis excluded by!**/*.yml.github/workflows/ghcr-check.ymlis excluded by!**/*.yml.github/workflows/ghcr-cleanup.ymlis excluded by!**/*.yml.github/workflows/plgd-device-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-device-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-tests.ymlis excluded by!**/*.yml.github/workflows/pr-format.ymlis excluded by!**/*.yml.github/workflows/sonar-cloud-analysis.ymlis excluded by!**/*.yml.github/workflows/static-analysis.ymlis excluded by!**/*.yml.github/workflows/unit-test-with-cfg.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
deps/mbedtls(1 hunks)port/android/Makefile(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- deps/mbedtls
🧰 Additional context used
🪛 GitHub Actions: CI building android
port/android/Makefile
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
port/android/Makefile (1)
127-141: LGTM! Well-organized DTLS source files.The DTLS source files are properly organized and include all necessary components for mbedTLS functionality.
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
4 times, most recently
from
2b6ac40 to
0c40061
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
patches/mbedtls/3.5/01-ocf-anon-psk.patch (3)
Line range hint
2-8: Potential Misplacement of Preprocessor DirectiveThe
#endif /* MBEDTLS_ASN1_PARSE_C */has been moved above the#ifdef __cplusplusblock. This change may inadvertently exclude C++ linkage specifications from parts of the code that depend onMBEDTLS_ASN1_PARSE_C.To ensure that all necessary declarations are correctly included when compiling with a C++ compiler, consider keeping the
#endifafter the#ifdef __cplusplusblock as originally positioned.
Line range hint
1479-1482: Initialization of New SSL Configuration FieldsNew fields
client_oid,client_oid_len,server_oid, andserver_oid_lenhave been added tombedtls_ssl_config. It's crucial to ensure these fields are properly initialized to avoid potential null pointer dereferences or undefined behavior.Verify that these fields are initialized in all code paths, especially in
mbedtls_ssl_config_initand default configuration functions. Consider setting them toNULLand0by default.
Line range hint
136-154: Security Risks with Enabling Anonymous Cipher SuitesThe addition of
MBEDTLS_TLS_ECDH_ANON_WITH_AES_128_CBC_SHA256and the key exchange typeMBEDTLS_KEY_EXCHANGE_ECDH_ANONenables support for anonymous ECDH key exchange.Anonymous cipher suites lack authentication and are vulnerable to man-in-the-middle attacks. Enabling them can introduce significant security risks. It's recommended to avoid using anonymous cipher suites in production environments.
Please reevaluate the necessity of this addition and, if required, ensure that it's explicitly documented with appropriate warnings about the security implications.
♻️ Duplicate comments (3)
port/android/Makefile (3)
444-462:⚠️ Potential issueFix the patching process for mbedTLS 3.6.2.
The current implementation has potential issues that could cause build failures.
Apply these changes to improve reliability:
ifeq ($(MBEDTLS_FORCE_3_6_2),1) MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.6/*.patch) $(wildcard ../../patches/mbedtls/3.6/make/*.patch)) $(MBEDTLS_PATCH_FILE): ${MBEDTLS_DIR}/.git ${MBEDTLS_PATCHES} - if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - (git fetch --unshallow --tags || git fetch --all) && \ - git checkout v3.6.2 && \ - cd - && \ - git add -u ${MBEDTLS_DIR} ; \ - fi && \ - git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} && \ + git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout v3.6.2 || exit 1 + @cd - > /dev/null + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}
466-484: 🛠️ Refactor suggestionSynchronize patching improvements with version 3.5.0.
The same patching process improvements should be applied to version 3.5.0 for consistency.
Apply similar improvements to the 3.5.0 version block:
ifeq ($(MBEDTLS_FORCE_3_5_0),1) MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.5/*.patch) $(wildcard ../../patches/mbedtls/3.5/make/*.patch)) $(MBEDTLS_PATCH_FILE): ${MBEDTLS_DIR}/.git ${MBEDTLS_PATCHES} - if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - (git fetch --unshallow --tags || git fetch --all) && \ - git checkout v3.5.0 && \ - cd - && \ - git add -u ${MBEDTLS_DIR} ; \ - fi && \ - git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} && \ + git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout v3.5.0 || exit 1 + @cd - > /dev/null + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}
488-500: 🛠️ Refactor suggestionSynchronize patching improvements with default version.
The same patching process improvements should be applied to the default version (3.1.0) block for consistency.
Apply similar improvements to the default version block as well:
MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.1/*.patch) $(wildcard ../../patches/mbedtls/3.1/make/*.patch)) $(MBEDTLS_PATCH_FILE): ${MBEDTLS_DIR}/.git ${MBEDTLS_PATCHES} - if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - cd -; \ - fi && \ - git submodule update --init && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} && \ + git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout v3.1.0 || exit 1 + @cd - > /dev/null + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}
🧹 Nitpick comments (2)
port/linux/Makefile (1)
763-763: Apply git fetch command modification to version 3.1.0 block for consistency.The git fetch command modification to handle shallow clones should also be applied to the 3.1.0 block for consistency.
Apply this diff to the 3.1.0 block:
- cd -; \ + (git fetch --unshallow --tags || git fetch --all) && \ + cd - && \patches/mbedtls/3.5/01-ocf-anon-psk.patch (1)
680-683: Update Feature List for Version ReportingThe addition of
KEY_EXCHANGE_ECDH_ANON_ENABLEDtoversion_features.censures that this feature is reported correctly. However, given the security concerns with anonymous key exchanges, ensure that this feature is documented appropriately in the version reporting to alert users.Consider adding a warning note in the version reporting or documentation to highlight the potential risks associated with enabling this feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
patches/mbedtls/3.5/01-ocf-anon-psk.patch(11 hunks)port/android/Makefile(2 hunks)port/linux/Makefile(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
- GitHub Check: Test
🔇 Additional comments (10)
port/linux/Makefile (2)
45-45: LGTM! Well-structured version control for mbedTLS 3.6.2.The changes introduce proper version control for mbedTLS 3.6.2 with appropriate DTLS source files.
Also applies to: 121-134
732-751: LGTM! Well-structured patch application logic for mbedTLS 3.6.2.The patch application logic for version 3.6.2 is properly implemented with appropriate git commands and error handling.
port/android/Makefile (3)
127-141: LGTM! Well-organized DTLS source files.The consolidation of DTLS source files improves readability and maintenance.
142-155: LGTM! Clear version-specific file handling.The conditional blocks clearly separate the source files for different mbedTLS versions.
118-120: Consider adding version validation.The version control variables provide flexibility but could benefit from validation to prevent invalid combinations.
Consider adding a check to prevent both flags from being set to 1:
MBEDTLS_FORCE_3_5_0 ?= 1 MBEDTLS_FORCE_3_6_2 ?= 0 +# Ensure only one version is active +ifneq ($(MBEDTLS_FORCE_3_5_0)$(MBEDTLS_FORCE_3_6_2),10) +$(error Only one mbedTLS version can be active at a time) +endifpatches/mbedtls/3.5/01-ocf-anon-psk.patch (5)
19-22: Proper Addition of Configuration Checks for ECDH Anon PrerequisitesThe added preprocessor checks ensure that
MBEDTLS_ECDH_Cis defined whenMBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDis enabled. This is a good practice to prevent compilation errors due to missing dependencies.
Line range hint
1483-1519: Well-Documented Addition of Certificate Management FunctionsThe new functions
mbedtls_ssl_conf_iterate_own_certsandmbedtls_ssl_conf_ekus, along with the callback typembedtls_ssl_conf_iterate_own_certs_cb_t, enhance certificate management capabilities. The comprehensive documentation improves maintainability and ease of use.
Line range hint
373-385: Correct Handling of PSK Identity in ECDHE_PSK Key ExchangeIn the modifications within
ssl_tls12_client.c, the handling of PSK identity hints in thessl_parse_server_psk_hintfunction has been updated. Ensure that the function correctly handles cases where the PSK identity is provided or absent, and that it adheres to the TLS specification.Review the logic to confirm that:
- The function properly processes non-empty PSK identity hints.
- Appropriate errors are raised when the PSK identity is unknown.
Line range hint
619-626: Validation of Certificate Usage with Updated EKU ChecksIn
ssl_tls12_server.c, the call tombedtls_ssl_check_cert_usagenow includes the new OID parameters. Ensure that the EKU (Extended Key Usage) checks are functioning correctly with the custom OIDs and that they do not inadvertently reject valid certificates.Consider adding unit tests to validate different scenarios with custom EKUs to ensure robust certificate validation.
Line range hint
271-294: Ensure Consistent Usage of Modified FunctionsThe function
mbedtls_ssl_check_cert_usagenow includes additional parameters forclient_oid,client_oid_len,server_oid, andserver_oid_len. All calls to this function must be updated accordingly to prevent mismatches and potential bugs.Run the following script to find and review all usages:
Ensure that the new parameters are correctly passed in all instances.
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
port/esp32/sdkconfig.3_5_0.defaults (1)
Line range hint
527-557: Consider enabling long file name support in FATFS.
CONFIG_FATFS_LFN_NONE=ydisables long file name (LFN) support in FATFS. If your application requires files with names longer than 8.3 format, consider enabling LFN support by settingCONFIG_FATFS_LFN_HEAPorCONFIG_FATFS_LFN_STACK.
🧹 Nitpick comments (3)
port/esp32/sdkconfig.3_6_2.defaults (1)
66-67: Consider increasing TCP buffer sizes.The current TCP send buffer and window sizes (5744 bytes) might be insufficient for IoT applications using DTLS, potentially impacting performance. Consider increasing these values to at least 8KB for better throughput.
-CONFIG_LWIP_TCP_SND_BUF_DEFAULT=5744 -CONFIG_LWIP_TCP_WND_DEFAULT=5744 +CONFIG_LWIP_TCP_SND_BUF_DEFAULT=8192 +CONFIG_LWIP_TCP_WND_DEFAULT=8192port/esp32/README.md (2)
Line range hint
66-83: Clarify the usage of the commented-out mbedTLS v3.1.0 instructions.The instructions for setting up mbedTLS v3.1.0 are commented out. If these steps are obsolete, consider removing them to avoid confusion. If they are still relevant, provide guidance on when and why a developer might opt to use mbedTLS v3.1.0 over other versions.
94-113: Integrate the mbedTLS v3.6.2 setup instructions into the main documentation.The setup steps for mbedTLS v3.6.2 are included as a commented-out section. If mbedTLS v3.6.2 is recommended or required for certain features or enhancements, consider moving these instructions into the main workflow. Providing clear guidance on the differences between versions and when to use each will help developers choose the correct setup for their needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
port/esp32/README.md(3 hunks)port/esp32/adapter/src/ipadapter.c(1 hunks)port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch(1 hunks)port/esp32/sdkconfig.3_5_0.defaults(11 hunks)port/esp32/sdkconfig.3_6_2.defaults(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Remove label
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
- GitHub Check: make_android (ANDROID_API=31)
- GitHub Check: CLAssistant
🔇 Additional comments (15)
port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch (3)
9-9: Security Review Required: Anonymous ECDH Key ExchangeEnabling anonymous ECDH key exchange (
MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLED) removes authentication from the key exchange process, making it vulnerable to man-in-the-middle attacks. While this might be intentional for resource-constrained IoT devices, please verify if this aligns with the security requirements of your system.Consider documenting:
- The security implications of this choice
- Any compensating controls in place
- The specific use cases where anonymous key exchange is required
10-10: Verify Need for Private API AccessThe
MBEDTLS_ALLOW_PRIVATE_ACCESSflag exposes internal mbedTLS functions. While this might be necessary for IoTivity integration, it could lead to maintenance issues if the internal APIs change in future mbedTLS versions.Please verify:
- Which private APIs are needed
- Why they can't be replaced with public APIs
- How future mbedTLS upgrades will be handled
11-11: Review Entropy Source LimitationSetting
MBEDTLS_ENTROPY_MAX_SOURCESto 2 might impact the quality of random number generation. This could affect security-critical operations that rely on strong randomness.Please confirm:
- The available entropy sources on ESP32
- Whether 2 sources provide sufficient entropy for your use case
- The impact on security-sensitive operations
port/esp32/sdkconfig.3_6_2.defaults (3)
5-5: Verify the firmware chip ID value.The firmware chip ID
0x0000seems incorrect. ESP32 chips typically use specific non-zero IDs. For example, ESP32-D0WD uses0x0000ESP32.
87-87: Verify if certificate bundle is needed.The certificate bundle is disabled (
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is not set). If the device needs to connect to cloud services or perform HTTPS requests, consider enabling it with a minimal set of required certificates.
24-26: Verify the existence of the custom partition table file.The configuration references a custom partition table file
partitions.csv, but we need to ensure this file exists in the repository.✅ Verification successful
Custom partition table file exists as expected
The referenced
partitions.csvfile is present atport/esp32/partitions.csv, in the same directory as the configuration file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the partition table file exists fd -t f "partitions.csv"Length of output: 50
port/esp32/sdkconfig.3_5_0.defaults (7)
18-18: Verify the configuration ofCONFIG_APP_BUILD_TYPE_RAM.
CONFIG_APP_BUILD_TYPE_RAMis currently not set (# CONFIG_APP_BUILD_TYPE_RAM is not set). SinceCONFIG_APP_BUILD_TYPE_ELF_RAMhas been renamed toCONFIG_APP_BUILD_TYPE_RAM, please ensure that the correct build type is selected for your application to function correctly.
254-254: Review the extra delay for flash readiness.
CONFIG_ESP_SLEEP_WAIT_FLASH_READY_EXTRA_DELAY=2000adds an extra delay of 2000 milliseconds when waiting for the flash to be ready after sleep. Confirm that this delay is necessary for your hardware setup, as setting it too high might impact wake-up times.
261-266: Assess the RTC clock source selection.Setting
CONFIG_RTC_CLK_SRC_INT_RC=yselects the internal RC oscillator as the clock source for the RTC. The internal RC oscillator may have lower accuracy compared to external sources. Ensure this is acceptable for your application's timing requirements.
271-275: Confirm the main crystal frequency configuration.
CONFIG_XTAL_FREQ_40=yandCONFIG_XTAL_FREQ=40configure the main crystal frequency to 40 MHz. Verify that your ESP32 hardware uses a 40 MHz crystal. Incorrect settings can lead to system instability or malfunction.
466-476: Check the brownout detector level settings.Enabling
CONFIG_ESP_BROWNOUT_DET=yactivates the brownout detector withCONFIG_ESP_BROWNOUT_DET_LVL_SEL_0=y, setting the detection level to the lowest threshold. Ensure that this level aligns with your hardware's voltage requirements to prevent unintended resets due to voltage fluctuations.
491-511: Optimize Wi-Fi configuration parameters.The Wi-Fi settings, such as buffer numbers (
CONFIG_ESP_WIFI_STATIC_RX_BUFFER_NUM,CONFIG_ESP_WIFI_DYNAMIC_RX_BUFFER_NUM), AMPDU settings (CONFIG_ESP_WIFI_AMPDU_TX_ENABLED,CONFIG_ESP_WIFI_AMPDU_RX_ENABLED), and task configurations, can significantly impact performance. Review these parameters to ensure they meet the throughput and stability requirements of your application.
861-864: Verify the time syscall source for newlib.
CONFIG_NEWLIB_TIME_SYSCALL_USE_RTC_HRT=yconfigures newlib to use the RTC high-resolution timer for time-related syscalls. Confirm that this setting is compatible with your application's use of time functions and does not introduce any timing inconsistencies.port/esp32/adapter/src/ipadapter.c (1)
61-69: Addition of necessary headers for ESP-IDF integration.Including
"esp_idf_version.h","esp_netif.h","esp_system.h","esp_wifi.h","esp_eth.h", and<lwip/sockets.h>ensures proper integration with the ESP-IDF framework and network functionality. Definingipi_spec_dstasipi_addrmaintains compatibility with network structures.port/esp32/README.md (1)
84-84: Verify the default SDK configuration file being used.Copying
sdkconfig.3_5_0.defaultstosdkconfig.defaultssets up the default configuration for mbedTLS v3.5.0. Ensure that this matches the mbedTLS version you intend to use. If you are using a different version, such as v3.6.2, update the configuration file accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
port/linux/Makefile (1)
184-184: Track the TODO comment about ffreestanding flag.This TODO should be properly tracked to ensure it's addressed in the future.
Would you like me to create an issue to track the investigation of the
-ffreestandingflag compatibility with C++ targets?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/cmake-linux.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(2 hunks)deps/mbedtls.cmake(1 hunks)port/android/Makefile(2 hunks)port/linux/Makefile(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- deps/mbedtls
🚧 Files skipped from review as they are similar to previous changes (3)
- deps/mbedtls.cmake
- deps/mbedtls-patch.cmake
- CMakeLists.txt
🔇 Additional comments (6)
port/android/Makefile (3)
118-119: LGTM! Clear version control setup.Good practice to maintain backward compatibility by defaulting to v3.5.0 while allowing opt-in to v3.6.2.
126-137: LGTM! Well-structured DTLS source files configuration.Good organization of base DTLS files with clean conditional compilation for v3.6.2-specific additions.
426-467:⚠️ Potential issueFix error handling in patch application process.
The current implementation has several reliability issues:
- Silent failures in directory changes
- Insufficient error handling for git commands
- No verification of patch application success
Apply this improvement to both version blocks:
ifeq ($(MBEDTLS_FORCE_3_6_2),1) MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.6/*.patch) $(wildcard ../../patches/mbedtls/3.6/make/*.patch)) $(MBEDTLS_PATCH_FILE): ${MBEDTLS_DIR}/.git ${MBEDTLS_PATCHES} - if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - (git fetch --unshallow --tags || git fetch --all) && \ - git checkout v3.6.2 && \ - git submodule update --init \ - cd - && \ - git add -u ${MBEDTLS_DIR} ; \ - fi && \ - git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Cleaning and updating mbedTLS..." + @git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout v3.6.2 || exit 1 + @cd - > /dev/null || exit 1 + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}Likely invalid or redundant comment.
port/linux/Makefile (3)
43-44: LGTM! Consistent version control setup.Matches the Android Makefile implementation, maintaining good cross-platform consistency.
105-115: LGTM! Consistent DTLS source files configuration.Matches the Android Makefile implementation, ensuring consistent behavior across platforms.
Line range hint
714-754: Fix error handling in patch application process.The implementation has the same reliability issues as the Android Makefile. For consistency, apply the same improvements:
Apply this improvement to both version blocks:
ifeq ($(MBEDTLS_FORCE_3_6_2),1) MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.6/*.patch) $(wildcard ../../patches/mbedtls/3.6/make/*.patch)) $(MBEDTLS_PATCH_FILE): ${MBEDTLS_DIR}/.git ${MBEDTLS_PATCHES} - if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - (git fetch --unshallow --tags || git fetch --all) && \ - git checkout v3.6.2 && \ - git submodule update --init \ - cd - && \ - git add -u ${MBEDTLS_DIR} ; \ - fi && \ - git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Cleaning and updating mbedTLS..." + @git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout v3.6.2 || exit 1 + @cd - > /dev/null || exit 1 + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
port/esp32/README.md (1)
61-61: Add a deprecation notice for mbedTLS v3.5.0.Based on the AI summary, mbedTLS v3.5.0 is being deprecated. Consider adding a deprecation notice at the start of the v3.5.0 section to inform users.
-For ESP32 v5.1 with mbedTLS v3.5.0: +For ESP32 v5.1 with mbedTLS v3.5.0 (Deprecated): + +> **Note:** This version is deprecated. It's recommended to use mbedTLS v3.6.2 for new projects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
iotivity-liteConfig.cmake.in(1 hunks)patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch(0 hunks)port/esp32/README.md(3 hunks)port/linux/Makefile(5 hunks)
💤 Files with no reviewable changes (1)
- patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch
🔇 Additional comments (6)
port/linux/Makefile (5)
43-44: LGTM! Clear version control setup.The comment and variable declaration provide clear indication of the default version while allowing flexibility to use 3.6.2.
105-112: LGTM! Well-organized DTLS source files.The list of DTLS source files is comprehensive and properly formatted.
114-115: LGTM! Clean conditional inclusion.The conditional inclusion of
pk_ecc.cfor mbedTLS 3.6.2 is properly implemented.
Line range hint
714-754: LGTM! Robust version management.The mbedTLS version management and patching logic is well-implemented with proper error handling and consistent patch application for both versions.
184-184: Address the TODO comment.Consider addressing or removing the TODO comment about checking the
-ffreestandingflag compatibility with C++ targets.Would you like me to help investigate the compatibility of the
-ffreestandingflag with C++ targets or create an issue to track this?port/esp32/README.md (1)
Line range hint
61-71: Verify the mbedTLS v3.5.0 commit hash and patches.Let's verify that the specified commit and patches are valid:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(2 hunks)port/android/Makefile(2 hunks)port/esp32/README.md(2 hunks)port/linux/Makefile(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deps/mbedtls
- CMakeLists.txt
🔇 Additional comments (10)
deps/mbedtls-patch.cmake (4)
26-36: LGTM! Well-implemented repository depth handling.The implementation correctly determines if the repository is shallow and executes the appropriate fetch command based on the result.
44-48: LGTM! Proper version management implementation.The checkout process for mbedTLS 3.6.2 is well-structured with appropriate submodule handling and repository state management.
59-62: LGTM! Clean patch management implementation.The branch management and patch selection for version 3.6.2 is properly implemented with appropriate error handling.
73-74: LGTM! Proper header file management.The addition of mbedTLS platform header files is implemented correctly.
port/android/Makefile (3)
118-120: LGTM! Clear version configuration.The default version is properly set to 3.6.2 with a clean configuration option to force 3.5.0 if needed.
126-137: LGTM! Comprehensive DTLS source files update.The DTLS source files are properly updated for mbedTLS 3.6.2 with correct conditional handling of pk_ecc.c.
435-450: 🛠️ Refactor suggestionImprove the patching process reliability.
The current implementation has potential issues:
- The
git fetch --unshallow --tags || git fetch --allcommand might fail if the repository is already unshallowed- The error handling for patch application is insufficient
Apply this diff to improve reliability:
- if [ -d ${MBEDTLS_DIR} ]; then \ - cd ${MBEDTLS_DIR} && \ - git clean -fdx . && \ - git reset --hard && \ - (git fetch --unshallow --tags || git fetch --all) && \ - git checkout "${MBEDTLS_TAG}" && \ - ([ "${MBEDTLS_TAG}" = "v3.5.0" ] || git submodule update --init) && \ - cd - && \ - git add -u ${MBEDTLS_DIR} ; \ - fi && \ - git submodule update --init && \ - git reset HEAD ${MBEDTLS_DIR} && \ - cd ${MBEDTLS_DIR} && \ - for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ - echo "Patches applied in $^" > ${@F} + @if [ ! -d ${MBEDTLS_DIR} ]; then \ + echo "Error: ${MBEDTLS_DIR} does not exist"; \ + exit 1; \ + fi + @cd ${MBEDTLS_DIR} && \ + git clean -fdx . && \ + git reset --hard && \ + git fetch --all --tags && \ + git checkout "${MBEDTLS_TAG}" || exit 1 + @cd - > /dev/null + @git submodule update --init || exit 1 + @cd ${MBEDTLS_DIR} || exit 1 + @echo "Applying patches..." + @for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying $${patch}..."; \ + if ! patch -r - -s -N -p1 < "$${patch}"; then \ + echo "Failed to apply $${patch}"; \ + exit 1; \ + fi \ + done + @echo "All patches applied successfully" + @echo "Patches applied in $^" > ${@F}Likely invalid or redundant comment.
port/linux/Makefile (3)
43-46: LGTM! Clear version configuration.The default version is properly set to 3.6.2 with a clean configuration option to force 3.5.0 if needed.
106-116: LGTM! Comprehensive DTLS source files update.The DTLS source files are properly updated for mbedTLS 3.6.2 with correct conditional handling of pk_ecc.c.
Line range hint
726-737: Improve the patching process reliability.The patching process has the same reliability issues as identified in port/android/Makefile.
|
@coderabbitai Write a descriptive commit message for the PR when the commits are squashed to one. |
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922 Here's a descriptive commit message for the squashed PR: |
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
7e07efa to
a5ad194
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch (1)
Line range hint
258-307: Disabled modern cryptographic features.Several modern cryptographic features are disabled:
- PKCS#1 v2.1 (RSAES-OAEP and RSASSA-PSS)
- EC compressed points parsing
- Error string support
Re-enable PKCS#1 v2.1 for better security:
-//#define MBEDTLS_PKCS1_V21 +#define MBEDTLS_PKCS1_V21patches/mbedtls/3.5/01-ocf-anon-psk.patch (3)
Line range hint
271-307: Certificate usage check needs improvement.The certificate usage check should be more strict when not using anonymous ECDH. Consider adding additional validation for certificate chains.
int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const mbedtls_ssl_ciphersuite_t *ciphersuite, int cert_endpoint, const char *client_oid, size_t client_oid_len, const char *server_oid, size_t server_oid_len, uint32_t *flags) { + /* Additional validation for non-anonymous ciphersuites */ + if (ciphersuite_info->key_exchange != MBEDTLS_KEY_EXCHANGE_ECDH_ANON) { + /* Verify certificate chain length */ + size_t chain_length = 0; + const mbedtls_x509_crt *cur = cert; + while (cur != NULL) { + chain_length++; + cur = cur->next; + } + if (chain_length < 2) { + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + return MBEDTLS_ERR_X509_CERT_VERIFY_FAILED; + } + }
Line range hint
373-460: Anonymous ECDH implementation needs security warning.The implementation of anonymous ECDH should include clear warnings about its security implications and usage restrictions.
Add warning comments:
+/* + * WARNING: Anonymous ECDH provides no authentication and is vulnerable to + * man-in-the-middle attacks. It should only be used in controlled environments + * where the lack of authentication is acceptable and compensated for by other + * security measures. + */ #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLED)
Line range hint
619-680: PSK identity handling needs improvement.The PSK identity handling code should include length validation and timing attack protection.
if (ssl->conf->f_psk != NULL) { + /* Use constant-time comparison for PSK identity */ + unsigned char diff = 0; + const size_t min_len = len < ssl->conf->psk_identity_len ? + len : ssl->conf->psk_identity_len; + + for (size_t i = 0; i < min_len; i++) { + diff |= ssl->conf->psk_identity[i] ^ (*p)[i]; + } + diff |= (unsigned char)(len ^ ssl->conf->psk_identity_len); + + if (diff != 0) { + ret = MBEDTLS_ERR_SSL_UNKNOWN_IDENTITY; + } - if (len != ssl->conf->psk_identity_len || - mbedtls_ct_memcmp(ssl->conf->psk_identity, *p, len) != 0)
🧹 Nitpick comments (9)
security/oc_certs.c (2)
185-186: Add documentation explaining the version constraint.Consider adding a comment explaining why this code is only included for mbedTLS versions below 3.6.2. This helps future maintainers understand the compatibility requirements.
#if MBEDTLS_VERSION_NUMBER < 0x03060200 +/* Private key extraction is only supported in mbedTLS versions below 3.6.2 + * as newer versions have removed this functionality for security reasons. */
Line range hint
187-201: Add input validation for buffer size.Consider adding validation for the minimum required buffer size to prevent potential buffer overflow issues.
static int certs_extract_private_key(size_t device, const mbedtls_x509_crt *cert, unsigned char *buffer, size_t buffer_size) { + if (buffer_size < MBEDTLS_PK_MAX_DER_SIZE) { + OC_ERR("buffer too small for private key extraction"); + return MBEDTLS_ERR_PK_BUFFER_TOO_SMALL; + } int ret = oc_mbedtls_pk_write_key_der(device, &cert->pk, buffer, buffer_size); if (ret <= 0) { OC_ERR("could not extract private key from cert %d", ret); return ret; }patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (1)
Line range hint
1-1183: Architectural Review: Validate security implications of configuration changes.The patch makes extensive changes to mbedTLS configuration to support IoTivity-Lite integration. While the changes are well-structured, please ensure:
- The security implications of enabling anonymous ECDH are understood and documented
- The disabled cipher modes don't impact any required functionality
- The memory management strategy is appropriate for all target platforms
- The platform-specific configurations are complete and tested across all supported platforms
Consider adding a security considerations document that explains:
- The rationale for enabling anonymous ECDH
- The impact of disabled cipher modes
- Memory management strategies across platforms
port/esp32/sdkconfig.3_5_0.defaults (2)
18-25: Consider enabling reproducible builds.Reproducible builds help with debugging and security auditing by ensuring that given the same source code, the same binary is always produced. Consider enabling
CONFIG_APP_REPRODUCIBLE_BUILDunless there's a specific reason not to.
519-521: Consider enabling core dumps for better debugging.Core dumps are currently disabled (
CONFIG_ESP_COREDUMP_ENABLE_TO_NONE). Consider enabling core dumps to flash (CONFIG_ESP_COREDUMP_ENABLE_TO_FLASH) to help with debugging crashes in production.patches/mbedtls/3.6/01-ocf-anon-psk.patch (2)
46-53: Thread-safety consideration for OID pointers.You introduced
client_oidandserver_oidpointers inmbedtls_ssl_config. Ensure the calling code does not modify or free these pointers from another thread while the config is in use, preventing possible concurrency issues.
530-590: ECDH-ANON handshake path.Adding the
goto exitfor the anonymous cipher suite is correct to skip signature checks. Ensure no extra handshake steps are inadvertently skipped.port/linux/Makefile (2)
43-45: Version selection logic needs documentation.The default version selection should be documented with the rationale for choosing 3.6.2 and the implications of forcing 3.5.0.
+# mbedTLS version selection: +# - 3.6.2 is the default version with latest security fixes +# - 3.5.0 can be forced for backward compatibility # use v3.6.2 as default MBEDTLS_FORCE_3_5_0 ?= 0
105-112: DTLS source files list needs organization.The DTLS source files list is hard to maintain. Consider organizing it by functionality or alphabetically.
-DTLS = aes.c asn1parse.c asn1write.c aria.c base64.c bignum_core.c bignum.c \ - ccm.c cipher.c cipher_wrap.c cmac.c constant_time.c ctr_drbg.c debug.c \ - ecdh.c ecdsa.c ecjpake.c ecp.c ecp_curves.c entropy_poll.c entropy.c error.c \ - gcm.c md.c net_sockets.c nist_kw.c oid.c pem.c pk.c pk_wrap.c pkcs5.c \ - pkparse.c pkwrite.c platform.c platform_util.c rsa.c rsa_alt_helpers.c \ - sha1.c sha256.c sha512.c timing.c ssl_ciphersuites.c ssl_client.c ssl_cookie.c \ - ssl_debug_helpers_generated.c ssl_msg.c ssl_tls.c ssl_tls12_client.c ssl_tls12_server.c \ - x509.c x509_create.c x509_crt.c x509_csr.c x509write.c x509write_csr.c x509write_crt.c +# Core crypto +DTLS_CORE = aes.c aria.c ccm.c cipher.c cipher_wrap.c cmac.c gcm.c \ + sha1.c sha256.c sha512.c + +# ASN.1 and PKI +DTLS_PKI = asn1parse.c asn1write.c x509.c x509_create.c x509_crt.c \ + x509_csr.c x509write.c x509write_csr.c x509write_crt.c + +# SSL/TLS +DTLS_SSL = ssl_ciphersuites.c ssl_client.c ssl_cookie.c \ + ssl_debug_helpers_generated.c ssl_msg.c ssl_tls.c \ + ssl_tls12_client.c ssl_tls12_server.c + +DTLS = $(DTLS_CORE) $(DTLS_PKI) $(DTLS_SSL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/build.ymlis excluded by!**/*.yml
📒 Files selected for processing (23)
CMakeLists.txt(1 hunks)api/unittest/tcptest.cpp(4 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(2 hunks)deps/mbedtls.cmake(1 hunks)iotivity-liteConfig.cmake.in(1 hunks)patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch(0 hunks)patches/mbedtls/3.5/01-ocf-anon-psk.patch(11 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(76 hunks)patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch(75 hunks)port/android/Makefile(2 hunks)port/esp32/README.md(2 hunks)port/esp32/adapter/src/ipadapter.c(1 hunks)port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch(1 hunks)port/esp32/sdkconfig.3_5_0.defaults(11 hunks)port/esp32/sdkconfig.3_6_2.defaults(1 hunks)port/linux/Makefile(5 hunks)security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/acltest.cpp(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch
🚧 Files skipped from review as they are similar to previous changes (12)
- iotivity-liteConfig.cmake.in
- security/oc_certs_internal.h
- security/unittest/obt_certstest.cpp
- port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch
- port/esp32/adapter/src/ipadapter.c
- deps/mbedtls
- deps/mbedtls-patch.cmake
- deps/mbedtls.cmake
- security/unittest/certsgeneratetest.cpp
- port/android/Makefile
- CMakeLists.txt
- port/esp32/sdkconfig.3_6_2.defaults
🔇 Additional comments (30)
security/oc_certs.c (2)
Line range hint
202-221: LGTM! Good error handling and memory management.The function properly handles initialization, error cases, and memory cleanup.
222-223: LGTM! Properly formatted conditional compilation end marker.The end marker is correctly placed and includes a helpful comment indicating what condition it's closing.
security/unittest/acltest.cpp (1)
Line range hint
929-941: LGTM! Verify helper functions availability.The filtering logic for PLGD time and well-known core resources is well-implemented.
Let's verify the availability of the helper functions:
Consider adding a brief comment to explain why these resources are being filtered out:
#if defined(OC_HAS_FEATURE_PLGD_TIME) || defined(OC_WKCORE) + // Filter out PLGD time and well-known core resources as they require special handling oc_string_view_t uriv = oc_string_view2(&resource->uri); #ifdef OC_HAS_FEATURE_PLGD_TIME if (plgd_is_time_resource_uri(uriv)) { return false; } #endif /* OC_HAS_FEATURE_PLGD_TIME */✅ Verification successful
Helper functions verified and properly integrated
The filtering logic is well-implemented and all required components are present:
- Helper functions are properly declared in internal headers
- Implementation files exist in the expected locations
- Headers are correctly included in the test file
- Feature flags are properly structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of helper functions used in the filtering logic # Search for plgd_is_time_resource_uri implementation echo "Searching for plgd_is_time_resource_uri implementation..." rg -p "plgd_is_time_resource_uri" --type cpp # Search for oc_is_wkcore_resource_uri implementation echo "Searching for oc_is_wkcore_resource_uri implementation..." rg -p "oc_is_wkcore_resource_uri" --type cppLength of output: 772
Script:
#!/bin/bash # Verify the implementation files and includes # Check for implementation files echo "Searching for implementation files..." fd -e c -e cpp "plgd_time" "oc_discovery" # Check includes in the test file echo -e "\nChecking includes in acltest.cpp..." head -n 100 security/unittest/acltest.cpp | grep -E "plgd_time|oc_discovery"Length of output: 470
Script:
#!/bin/bash # Search for implementation files with corrected patterns echo "Searching for plgd_time implementation files..." fd -e c -e cpp plgd_time echo -e "\nSearching for discovery implementation files..." fd -e c -e cpp discoveryLength of output: 435
patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (5)
9-10: LGTM: Appropriate .gitignore entry added.The addition of the generated header file to .gitignore is correct and well-documented.
18-30: Key configuration changes align with IoTivity requirements.The addition of
mbedtls_oc_platform.hinclude and enabling of anonymous ECDH key exchange are appropriate for IoTivity integration. However, enabling anonymous ECDH requires careful consideration of security implications.Let's verify the security implications of enabling anonymous ECDH:
✅ Verification successful
Anonymous ECDH configuration is properly controlled and secured
The enabled anonymous ECDH is a required feature specifically used during device onboarding (RFOTM state) with proper security controls in place. The implementation restricts anonymous access to specific security endpoints and is part of the standardized Just Works onboarding mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any security advisories or documentation about ECDH_ANON usage rg -i "anonymous|anon.*ecdh" -g "*.{md,txt,h,c}"Length of output: 1090
Line range hint
1033-1088: Platform-specific memory management configuration needs validation.The changes introduce conditional compilation for memory management based on
OC_DYNAMIC_ALLOCATION. This is good for flexibility, but we should verify the memory allocation strategy across different platforms.Let's verify the memory management configuration:
✅ Verification successful
Memory management configuration is correctly implemented
The platform-specific memory management configuration in the mbedtls patch aligns with the codebase's memory allocation strategy and properly handles both dynamic and static allocation cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for memory-related configurations and usage echo "Checking memory allocation patterns..." rg "OC_DYNAMIC_ALLOCATION|MBEDTLS_PLATFORM_MEMORY" -A 5 -g "*.{c,h,cmake}" echo "Checking for custom memory management implementations..." rg "oc_mem|malloc|calloc|free" -g "*.{c,h}" --type-add "cmake:*.cmake"Length of output: 80699
Line range hint
139-187: Security-critical: Verify the implications of disabled cipher modes.The patch disables several cipher modes (CFB, CTR, OFB, XTS) and padding modes. While this reduces the attack surface, ensure this aligns with all IoTivity-Lite use cases.
Run this script to verify cipher mode usage in the codebase:
✅ Verification successful
Disabling cipher modes and padding is safe
The disabled cipher modes (CFB, CTR, OFB, XTS) and padding modes are not used in the IoTivity-Lite codebase. The CTR string matches found were false positives related to socket message flags (MSG_CTRUNC) and random number generation (CTR_DRBG), not cipher modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of disabled cipher modes echo "Searching for CFB mode usage..." rg -l "CFB" -g "*.{c,h}" echo "Searching for CTR mode usage..." rg -l "CTR" -g "*.{c,h}" echo "Searching for OFB mode usage..." rg -l "OFB" -g "*.{c,h}" echo "Searching for XTS mode usage..." rg -l "XTS" -g "*.{c,h}"Length of output: 514
Script:
#!/bin/bash # Check context of CTR usage echo "Checking CTR usage context..." rg "CTR" -C 3 -g "*.{c,h}" security/oc_csr.c security/oc_certs_generate.c port/linux/ip.c port/esp32/adapter/src/ipadapter.c port/android/ipadapter.cLength of output: 2525
Line range hint
1123-1171: Validate platform-specific header completeness.The new platform header files provide good abstraction for IoTivity-specific configurations. However, we should verify:
- All necessary platform-specific includes are present
- The SSL content length definitions are appropriate for all platforms
Let's verify the platform header completeness:
Also applies to: 1171-1183
✅ Verification successful
Platform headers and SSL content length are properly configured
The implementation correctly handles both platform-specific requirements:
- All necessary platform headers are included (config, log, assert, connectivity)
- SSL content length is safely defined using platform PDU size when available, with a reasonable 16KB fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for platform-specific includes that might be missing echo "Checking for platform-specific headers usage..." rg "include.*oc_" -g "*.{c,h}" echo "Checking SSL content length usage..." rg "OC_PDU_SIZE|SSL_CONTENT_LEN" -g "*.{c,h}"Length of output: 85507
port/esp32/sdkconfig.3_5_0.defaults (6)
466-475: Review brownout detection level.The brownout detection is set to the lowest level (
CONFIG_BROWNOUT_DET_LVL_SEL_0). This might not provide adequate protection against voltage drops, potentially leading to system instability or corruption.Consider using a higher brownout detection level (2 or 3) for better system stability unless there's a specific hardware requirement for the lowest level.
491-511: Wi-Fi security configuration looks good.The Wi-Fi configuration properly enables WPA3-SAE support and uses mbedTLS for cryptographic operations, which aligns well with the PR's objective of updating mbedTLS support.
527-529: Review FAT filesystem long filename requirements.Long filename support is disabled (
CONFIG_FATFS_LFN_NONE). This might limit compatibility with applications that use filenames longer than 8.3 format. Consider enabling heap-based LFN support if your application requires long filenames.
561-599: FreeRTOS configuration looks appropriate.The FreeRTOS settings include reasonable defaults for task sizes, priorities, and port-specific configurations. The mutex ownership checking is correctly enabled for debugging.
Line range hint
1001-1070: Note presence of deprecated options.The configuration includes several deprecated options. While these are currently required for backward compatibility, consider tracking them for future removal when the minimum supported ESP-IDF version is upgraded.
242-277: Review sleep configuration implications.The sleep configuration includes several important settings:
- Flash ready extra delay of 2000ms might be excessive and impact wake-up time.
- Using internal RC oscillator (
CONFIG_RTC_CLK_SRC_INT_RC) for RTC clock provides better power efficiency but lower accuracy compared to external crystal.- 40MHz XTAL frequency is set as default.
Please verify if the 2000ms flash ready delay is necessary for your hardware:
patches/mbedtls/3.6/01-ocf-anon-psk.patch (4)
18-29: Formatting change is acceptable.The change from a multi-line return statement to a single-line expression does not alter functionality and appears stylistically consistent.
293-310: Add NULL pointer check forconfparameter.The function
mbedtls_ssl_conf_ekusshould guard againstconf == NULLto prevent potential null pointer dereferences. This recommendation aligns with previous feedback:+ if (conf == NULL) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + }
423-488: LGTM: Improved error-handling logic.Your revised PSK hint parsing handles missing keys, zero-length hints, and unknown identities properly, aligning with best practices for error propagation.
9-12: Consider also verifying MBEDTLS_ECP_C requirement.Conditional compilation for
MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLEDcurrently checks only forMBEDTLS_ECDH_C. In some configurations,MBEDTLS_ECP_Cmight also be required for proper ECDH functionality. You may wish to confirm thatMBEDTLS_ECP_Cis unconditionally enabled or add it to this check.port/esp32/README.md (2)
74-76: Fix patch path references for ESP32 v5.4.You still reference
../../../../../../patches/mbedtls/3.6/. Update the path to match the actual patch location. For example:- ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../../../patches/mbedtls/3.6/01-ocf-anon-psk.patch ) + ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../patches/mbedtls/3.6/01-ocf-anon-psk.patch )
Line range hint
81-91: Fix patch path references for ESP32 v5.1.Similarly, update the path used for the v3.5 patches:
- ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../../../patches/mbedtls/3.5/01-ocf-anon-psk.patch ) + ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../patches/mbedtls/3.5/01-ocf-anon-psk.patch )api/unittest/tcptest.cpp (4)
27-43: Additional includes look good.The newly added headers for
oc_random.h,oc_entropy_internal.h, and mbedTLS are appropriate for secure test coverage.
66-71: Random initialization and teardown.Properly calling
oc_random_init()inSetUpTestCaseandoc_random_destroy()inTearDownTestCaseenhances test integrity.
279-288: Seeding the DRBG with entropy.The code integrates a basic seeding mechanism and checks the return value. This is a solid approach for secure randomness.
317-318: Entropy cleanup is properly handled.Releasing the entropy context ensures no memory leaks remain after the test.
patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch (6)
14-15: Security configuration enables anonymous ECDH.The configuration enables anonymous ECDH key exchange which lacks authentication. While this might be intentional for your use case, it could expose the system to man-in-the-middle attacks.
Please confirm if anonymous ECDH is required for your use case. If yes, ensure proper security measures are in place at the application level.
8-14: 🛠️ Refactor suggestionProject-specific headers should be conditionally included.
Including OCF-specific headers directly in mbedTLS configuration may cause issues if the headers are not available in all build environments. Consider wrapping these includes in appropriate conditional compilation guards.
+#ifdef OCF_ENABLED #include <oc_config.h> #include <oc_log.h> #include <port/oc_assert.h> #include <port/oc_connectivity.h> +#endifLikely invalid or redundant comment.
431-469: 🛠️ Refactor suggestionTLS 1.3 support is disabled.
Disabling TLS 1.3 removes access to important security improvements and performance optimizations. Consider enabling TLS 1.3 support unless there are specific compatibility requirements.
-//#define MBEDTLS_SSL_PROTO_TLS1_3 +#define MBEDTLS_SSL_PROTO_TLS1_3 -//#define MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE +#define MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE -//#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED +#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED -//#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED +#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED -//#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED +#define MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLEDLikely invalid or redundant comment.
Line range hint
168-191: Limited set of elliptic curves may impact interoperability.Most elliptic curves are disabled except for SECP256R1 and SECP384R1. While this reduces code size, it might limit interoperability with systems requiring other curves.
Line range hint
120-148: Disabled cipher modes reduce security options.Multiple cipher modes (CFB, CTR, OFB, XTS) are being disabled. This significantly reduces the flexibility of the crypto system and might impact compatibility with other systems.
1015-1039: Memory allocation configuration needs review.The configuration mixes static and dynamic memory allocation options. This could lead to memory management issues if not properly handled.
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
port/esp32/adapter/src/tcpadapter.c(2 hunks)port/esp32/sdkconfig.3_6_2.defaults(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- port/esp32/sdkconfig.3_6_2.defaults
🧰 Additional context used
🪛 GitHub Actions: Check formatting
port/esp32/adapter/src/tcpadapter.c
[error] 304-304: Code formatting error: Missing space after PRIu32 macro in printf-style format string
[error] 308-308: Code formatting error: Missing space after PRIu32 macro in printf-style format string
🔇 Additional comments (1)
port/esp32/adapter/src/tcpadapter.c (1)
42-42: LGTM! Good practice for portable integer formatting.Including <inttypes.h> enables the use of portable format specifiers for fixed-width integers.
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922 Here's a comprehensive description for the PR: Update default mbedTLS version to 3.6.2This PR updates the default mbedTLS version from 3.1.0 to 3.6.2 while maintaining optional support for v3.5.0 through conditional compilation. The update aims to enhance security and ensure compatibility with newer systems. Key Changes
Technical Details
Platform-specific ChangesLinux & Android
ESP32
Benefits
MigrationUsers can continue using mbedTLS 3.5.0 by setting TestingThe changes have been tested across all supported platforms with both mbedTLS 3.6.2 and 3.5.0 configurations. |
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
9241c01 to
e14c707
Compare
Update the default mbedTLS version from 3.1.0 to 3.6.2 while maintaining optional support for v3.5.0 through conditional compilation. This change enhances security and ensures compatibility with newer systems. Key changes: - Add patches for mbedTLS 3.6.2 support - Maintain compatibility with mbedTLS 3.5.0 via BUILD_MBEDTLS_FORCE_3_5_0 - Update build configurations for Linux, Android, and ESP32 platforms - Update CMake and Make build systems to support version selection - Add new sdkconfig defaults for ESP32 (3.5.0 and 3.6.2) - Update documentation for ESP32 platform setup The changes ensure smooth transition to newer mbedTLS versions while allowing systems to maintain compatibility with 3.5.0 if needed. Deprecated support for 3.1.0 has been removed.
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
e14c707 to
997c7ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
port/linux/Makefile (1)
Line range hint
696-712: Improve patch application error handling.The patch application suppresses errors with
-sflag and ignores return codes. This could mask patch failures.- for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ + for patch in $(MBEDTLS_PATCHES); do \ + if ! patch -r - -N -p1 < $${patch}; then \ + echo "Failed to apply patch: $${patch}" >&2; \ + exit 1; \ + fi \ + done && \
♻️ Duplicate comments (2)
patches/mbedtls/3.6/01-ocf-anon-psk.patch (1)
127-130:⚠️ Potential issue** Add a null-pointer check for
confinmbedtls_ssl_conf_ekus().**This echoes a previously raised concern: the function checks
client_oid_lenandserver_oid_len, but notconf. Adding a check forconf == NULLwould make the code more robust.+ if (conf == NULL) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + } if ((client_oid_len == 0 && client_oid) || ...port/linux/Makefile (1)
43-45: 🛠️ Refactor suggestionVersion selection needs error handling.
The version selection logic should validate the selected version and provide meaningful error messages.
ifeq ($(MBEDTLS_FORCE_3_5_0),1) MBEDTLS_TAG := v3.5.0 MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.5/*.patch) $(wildcard ../../patches/mbedtls/3.5/make/*.patch)) else MBEDTLS_TAG := v3.6.2 MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.6/*.patch) $(wildcard ../../patches/mbedtls/3.6/make/*.patch)) + +# Validate patches exist +ifeq ($(MBEDTLS_PATCHES),) +$(error No patches found for mbedTLS $(MBEDTLS_TAG)) +endif endifAlso applies to: 689-694
🧹 Nitpick comments (12)
patches/mbedtls/3.6/01-ocf-anon-psk.patch (3)
9-12: Consolidate prerequisite checks.Currently, each key exchange feature has its own
#ifguard. Consider consolidating these guards for simpler reading and future maintenance.
206-217: Clarify approach for ECDH-ANON ciphersuite.ECDH-ANON provides no authentication, which can expose connections to man-in-the-middle attacks. Document the security implications of enabling ECDH-ANON to ensure integrators understand the risks.
567-590: Avoid unconditional jump withgoto exitin anonymous cipher suites.The code uses
goto exit;to skip signature checks for ECDH-ANON. Consider structuring this logic withoutgototo simplify flow. For example, return immediately for ECDH-ANON or separate the logic into a smaller function.api/unittest/tcptest.cpp (1)
279-287: Check CTR-DRBG seeding error codes.Good job initializing and seeding CTR-DRBG. For production builds, ensure additional logs or error handling if
mbedtls_ctr_drbg_seedfails (rather than using justASSERT_EQ).port/android/Makefile (2)
426-432: Add error handling for patch selection.While the version selection logic is good, consider validating that patches exist for the selected version to fail early with a clear error message.
ifeq ($(MBEDTLS_FORCE_3_5_0),1) MBEDTLS_TAG := v3.5.0 MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.5/*.patch) $(wildcard ../../patches/mbedtls/3.5/make/*.patch)) else MBEDTLS_TAG := v3.6.2 MBEDTLS_PATCHES := $(sort $(wildcard ../../patches/mbedtls/3.6/*.patch) $(wildcard ../../patches/mbedtls/3.6/make/*.patch)) endif + +# Validate patches exist +ifeq ($(MBEDTLS_PATCHES),) +$(error No patches found for mbedTLS $(MBEDTLS_TAG)) +endif
436-450: Improve patch application process robustness.The patch application process could be more robust with better error handling and logging.
- for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ + for patch in $(MBEDTLS_PATCHES); do \ + echo "Applying patch: $${patch}" && \ + if ! patch -r - -N -p1 < "$${patch}"; then \ + echo "Failed to apply patch: $${patch}" >&2 && \ + exit 1; \ + fi \ + done && \port/esp32/sdkconfig.3_5_0.defaults (1)
583-599: Consider enabling FreeRTOS critical section compliance checking.The critical section compliance check is disabled (
CONFIG_FREERTOS_CHECK_PORT_CRITICAL_COMPLIANCEcommented out). This check helps identify potential issues with critical sections in FreeRTOS tasks.-# CONFIG_FREERTOS_CHECK_PORT_CRITICAL_COMPLIANCE is not set +CONFIG_FREERTOS_CHECK_PORT_CRITICAL_COMPLIANCE=ypatches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (1)
1079-1091: Document platform-specific configuration requirements.The platform-specific configurations need documentation explaining:
- Required platform function implementations
- Memory management expectations
- SSL content length considerations
Consider adding a README.md in the platform directory explaining these requirements.
patches/mbedtls/3.5/01-ocf-anon-psk.patch (2)
Line range hint
1881-1903: Enhance PSK identity verification logging.While the PSK identity verification is improved, consider adding detailed logging to help diagnose authentication failures.
if (ret == MBEDTLS_ERR_SSL_UNKNOWN_IDENTITY) { - MBEDTLS_SSL_DEBUG_BUF(3, "Unknown PSK identity", *p, len); + MBEDTLS_SSL_DEBUG_MSG(2, ("PSK identity verification failed")); + MBEDTLS_SSL_DEBUG_BUF(2, "Received identity", *p, len); + MBEDTLS_SSL_DEBUG_MSG(3, ("Expected length: %d, Received length: %d", + ssl->conf->psk_identity_len, len));
Line range hint
3435-3440: Add testing guidance for custom EKU OIDs.The new certificate handling with custom EKU OIDs needs testing guidance to ensure correct usage.
Consider adding test cases demonstrating:
- Custom OID validation
- Certificate chain verification with custom OIDs
- Error handling for invalid OIDs
CMakeLists.txt (2)
44-44: Consider updating the comment to clarify version support.The comment could be more explicit about version support and deprecation status.
Apply this diff to improve clarity:
-set(BUILD_MBEDTLS_FORCE_3_5_0 OFF CACHE BOOL "Force v3.5.0 of the MbedTLS library to be used (by default v3.6.2 is used)") +set(BUILD_MBEDTLS_FORCE_3_5_0 OFF CACHE BOOL "Force deprecated v3.5.0 of the MbedTLS library to be used (default is v3.6.2, v3.1.0 support removed)")
44-53: Consider documenting migration steps for users.The removal of mbedTLS 3.1.0 support and the change of default version to 3.6.2 is a breaking change that requires clear documentation and migration guidance.
Consider:
- Adding a section in the project's README or UPGRADING document about this change
- Documenting any potential compatibility issues when upgrading from 3.1.0
- Providing migration steps for users who need to transition from 3.1.0 to either 3.5.0 or 3.6.2
- Adding examples of how to configure the build system for each supported version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/pr-format.ymlis excluded by!**/*.yml
📒 Files selected for processing (24)
CMakeLists.txt(1 hunks)api/unittest/tcptest.cpp(4 hunks)deps/mbedtls(1 hunks)deps/mbedtls-patch.cmake(2 hunks)deps/mbedtls.cmake(1 hunks)iotivity-liteConfig.cmake.in(1 hunks)patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch(0 hunks)patches/mbedtls/3.5/01-ocf-anon-psk.patch(11 hunks)patches/mbedtls/3.6/01-ocf-anon-psk.patch(1 hunks)patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch(76 hunks)patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch(75 hunks)port/android/Makefile(2 hunks)port/esp32/README.md(2 hunks)port/esp32/adapter/src/ipadapter.c(1 hunks)port/esp32/adapter/src/tcpadapter.c(2 hunks)port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch(1 hunks)port/esp32/sdkconfig.3_5_0.defaults(11 hunks)port/esp32/sdkconfig.3_6_2.defaults(1 hunks)port/linux/Makefile(5 hunks)security/oc_certs.c(2 hunks)security/oc_certs_internal.h(2 hunks)security/unittest/acltest.cpp(2 hunks)security/unittest/certsgeneratetest.cpp(4 hunks)security/unittest/obt_certstest.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- patches/mbedtls/3.1/01-ocf-x509san-anon-psk.patch
🚧 Files skipped from review as they are similar to previous changes (13)
- iotivity-liteConfig.cmake.in
- port/esp32/adapter/src/tcpadapter.c
- security/oc_certs_internal.h
- security/unittest/obt_certstest.cpp
- deps/mbedtls.cmake
- deps/mbedtls
- port/esp32/adapter/src/ipadapter.c
- security/unittest/acltest.cpp
- security/unittest/certsgeneratetest.cpp
- deps/mbedtls-patch.cmake
- port/esp32/patches/mbedtls/3.6/02-ocf-mbedtls-config.patch
- security/oc_certs.c
- port/esp32/sdkconfig.3_6_2.defaults
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: make_android (ANDROID_API=33)
- GitHub Check: make_linux_java (CLOUD=1 DEBUG=1 MBEDTLS_FORCE_3_6_2=1)
🔇 Additional comments (20)
patches/mbedtls/3.6/01-ocf-anon-psk.patch (2)
50-53: Ensure OID buffers are safely managed.The newly introduced
client_oidandserver_oidpointers require the user to guarantee their lifetimes. Consider clarifying ownership in the documentation to avoid dangling pointers.
420-488: Validate buffer size againstpsk_identity_len.Though there is now a max check (
0xFFFF), confirm that the buffer inssl->out_msgis sufficiently large to handle the entire PSK identity. Setting a policy-based limit (e.g. 2 KB) might further reduce the risk of accidental overflow.port/esp32/README.md (3)
Line range hint
81-91: Double-check patch paths for mbedTLS v3.5.0 as well.Similarly adjust the patch commands for v3.5.0 to maintain consistency:
- ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../../../patches/mbedtls/3.5/01-ocf-anon-psk.patch ) + ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../patches/mbedtls/3.5/01-ocf-anon-psk.patch )
61-79: 🛠️ Refactor suggestionUse uniform patch paths consistent with previous comments.
Instead of
../../../../../../patches/mbedtls/3.6/, consider consolidating to../../../../patches/mbedtls/3.6/. This matches the approach in other sections, reduces complexity, and is easier to maintain.- ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../../../patches/mbedtls/3.6/01-ocf-anon-psk.patch ) + ( cd esp-idf/components/mbedtls/mbedtls && patch -p1 < ../../../../patches/mbedtls/3.6/01-ocf-anon-psk.patch )Likely invalid or redundant comment.
61-79: Confirm the reference commit (v5.4) contains mbedTLS 3.6.2.Verify that commit
c8bb53292d08d6449a09823cf554e62ac839cd8ctruly corresponds to v5.4 with mbedTLS 3.6.2.✅ Verification successful
Commit and mbedTLS version verified correctly
The commit
c8bb53292d08d6449a09823cf554e62ac839cd8cis confirmed to be part of ESP-IDF v5.4 and contains mbedTLS v3.6.2 as stated in the README.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that commit references v5.4 plus mbedTLS 3.6.2 git ls-remote https://github.com/espressif/esp-idf.git c8bb53292d08d6449a09823cf554e62ac839cd8cLength of output: 97
Script:
#!/bin/bash # Clone with minimal history and verify commit and version git clone --depth 1 --branch release/v5.4 https://github.com/espressif/esp-idf.git esp-idf-temp cd esp-idf-temp # Check if commit exists and its relationship to v5.4 git log --oneline -n 1 # Check mbedTLS version cat components/mbedtls/mbedtls/VERSION cat components/mbedtls/mbedtls/include/mbedtls/version.h | grep -A 2 "MBEDTLS_VERSION_"Length of output: 1928
Script:
#!/bin/bash # Clone and check specific commit git clone --depth 1 --branch release/v5.4 https://github.com/espressif/esp-idf.git esp-idf-temp cd esp-idf-temp git checkout c8bb53292d08d6449a09823cf554e62ac839cd8c git submodule update --init components/mbedtls/mbedtls echo "Commit information:" git log -n 1 --oneline echo -e "\nmbedTLS version information:" cat components/mbedtls/mbedtls/include/mbedtls/version.h | grep -A 2 "MBEDTLS_VERSION_STRING"Length of output: 972
Script:
#!/bin/bash # Clean up and perform full clone rm -rf esp-idf-temp git clone https://github.com/espressif/esp-idf.git esp-idf-temp cd esp-idf-temp # Check if commit exists in v5.4 branch git branch -a --contains c8bb53292d08d6449a09823cf554e62ac839cd8c # Try to checkout the commit git checkout c8bb53292d08d6449a09823cf554e62ac839cd8c git submodule update --init components/mbedtls/mbedtls echo "Commit information:" git show --oneline --no-patch echo -e "\nmbedTLS version information:" git ls-tree HEAD components/mbedtls/mbedtlsLength of output: 3260
Script:
#!/bin/bash cd esp-idf-temp/components/mbedtls/mbedtls # Try multiple possible locations for version information echo "Checking version.h:" find . -name "version.h" -type f -exec cat {} \; | grep -A 2 "MBEDTLS_VERSION_STRING" echo -e "\nChecking CMakeLists.txt for version:" find . -name "CMakeLists.txt" -type f -exec grep -l "VERSION" {} \; -exec cat {} \; | grep -i "VERSION.*3.6"Length of output: 705
api/unittest/tcptest.cpp (5)
27-27: Acknowledge new random library inclusion.The inclusion of
"port/oc_random.h"is well-placed, improving clarity by explicitly tying test randomness to the new random utility.
35-37: Include guard usage for security code.The conditional inclusion of
"security/oc_entropy_internal.h"is correct. Confirm that all code using it is likewise guarded by#ifdef OC_SECURITY.
66-66: Well-done on callingoc_random_init().Ensuring the random subsystem is initialized here improves test reproducibility and consistency.
71-71: Properly destroy random subsystem.The call to
oc_random_destroy()ensures resources are deallocated and no random resource leaks occur.
317-318: Entropy cleanup ordering.Freeing the entropy context before freeing
ctr_drbgis correct; reversing it might lead to references to a freed structure.port/android/Makefile (2)
118-120: LGTM! Clear version selection mechanism.Good practice to provide a default version while maintaining backward compatibility through a flag.
126-137: LGTM! Comprehensive DTLS source files update.The DTLS files are correctly configured for mbedTLS 3.6.2, with proper handling of version-specific differences through conditional compilation.
patches/mbedtls/3.6/make/02-ocf-mbedtls-config.patch (3)
Line range hint
355-364: Security Impact: PKCS#1 v2.1 Support Disabled.Disabling PKCS#1 v2.1 removes support for RSAES-OAEP and RSASSA-PSS, which provide improved security over older RSA padding schemes. Consider keeping this enabled.
428-433: Security Impact: TLS 1.3 Support Disabled.Disabling TLS 1.3 removes access to important security improvements and performance optimizations. Consider keeping TLS 1.3 enabled unless there are specific compatibility requirements.
Line range hint
168-200: Security Impact: Modern Elliptic Curves Disabled.Disabling Curve25519 and Curve448 removes support for modern, secure curves that are recommended for ECDH key exchanges. Consider keeping these enabled for better security.
✅ Verification successful
Enabled curves SECP256R1 and SECP384R1 are sufficient for security requirements
While Curve25519 and Curve448 offer modern security properties, the enabled NIST curves (SECP256R1 and SECP384R1) are cryptographically secure and widely supported. The codebase shows no specific dependencies on the disabled curves, and the test suite is designed to work with any available curves.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any code specifically requires older curves echo "Checking for curve-specific dependencies..." rg -l "secp.*k1|secp.*r1" --type c --type cppLength of output: 144
Script:
#!/bin/bash # Search with broader patterns and include headers echo "Searching for curve-related code..." rg -l "curve|CURVE|secp|SECP|ECP_DP" --type c --type cpp --type h # Search specifically for Curve25519 and Curve448 references echo -e "\nSearching for modern curve usage..." rg -l "curve25519|curve448|CURVE25519|CURVE448" --type c --type cpp --type h # Look for test files related to curves echo -e "\nSearching for curve-related tests..." fd -e c -e cpp -e h . | grep -i "test.*curve\|curve.*test"Length of output: 1008
Script:
#!/bin/bash # Check how modern curves are used in the test file echo "Examining modern curve usage in tests..." rg -A 5 -B 5 "curve25519|curve448|CURVE25519|CURVE448" security/unittest/certstest.cpp # Check for configuration dependencies echo -e "\nChecking for configuration dependencies..." rg "MBEDTLS_ECP_DP_(CURVE25519|CURVE448)_ENABLED" security/unittest/certstest.cppLength of output: 1449
port/esp32/sdkconfig.3_5_0.defaults (3)
242-276: LGTM! Hardware settings are well configured.The hardware settings section properly configures:
- Sleep parameters for flash and GPIO handling
- RTC clock source using internal RC oscillator
- XTAL frequency at 40MHz
491-511: LGTM! WiFi settings are well configured.The WiFi configuration includes:
- Appropriate buffer sizes for RX/TX
- AMPDU enabled with reasonable window sizes
- Task properly pinned to core 0
- WPA3 SAE support enabled for enhanced security
Line range hint
739-744: Consider reducing mbedTLS debug level in production.Debug level is set to 3 (debug) which may expose sensitive information in logs. Consider reducing this in production builds.
patches/mbedtls/3.6/cmake/02-ocf-mbedtls-config.patch (2)
Line range hint
632-643: LGTM! Debug configuration properly integrated.The debug configuration is well integrated with the application's logging system and properly handles compiler-specific features.
18-30: Review security implications of anonymous ECDH.Enabling anonymous ECDH (
MBEDTLS_KEY_EXCHANGE_ECDH_ANON_ENABLED) removes peer authentication. This should only be used in conjunction with higher-layer security mechanisms.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
port/esp32/main/CMakeLists.txt (1)
Line range hint
1-276: Add mbedTLS version checks and conditional compilation flags.Given that this PR updates mbedTLS support from 3.1.0 to 3.6.2 with optional 3.5.0 support, consider adding:
- Version checks for mbedTLS
- Conditional compilation flags for different mbedTLS versions
Add this section before the
idf_component_register:+# Check mbedTLS version +include(CheckIncludeFile) +check_include_file(mbedtls/version.h HAVE_MBEDTLS_VERSION_H) +if(NOT HAVE_MBEDTLS_VERSION_H) + message(FATAL_ERROR "mbedtls/version.h not found") +endif() + +# Add version-specific definitions +if(CONFIG_MBEDTLS_VERSION_3_6_2) + list(APPEND OC_DEFINITIONS -DMBEDTLS_VERSION_3_6_2) +elseif(CONFIG_MBEDTLS_VERSION_3_5_0) + list(APPEND OC_DEFINITIONS -DMBEDTLS_VERSION_3_5_0) +else() + message(FATAL_ERROR "Unsupported mbedTLS version. Please select either 3.6.2 or 3.5.0") +endif()
🧹 Nitpick comments (10)
tools/clang-format.cmake (3)
6-8: Consider using a more maintainable approach for exclusions.Instead of having multiple separate
list(FILTER ...)commands, consider maintaining a list of excluded patterns in a variable. This would make it easier to manage exclusions in the future.Here's a suggested improvement:
-list(FILTER iotivity_allsource EXCLUDE REGEX "build/") -list(FILTER iotivity_allsource EXCLUDE REGEX "deps/") -list(FILTER iotivity_allsource EXCLUDE REGEX "port/esp32/esp-idf/") +set(FORMAT_EXCLUDE_PATTERNS + "build/" + "deps/" + "port/esp32/esp-idf/" +) +foreach(pattern IN LISTS FORMAT_EXCLUDE_PATTERNS) + list(FILTER iotivity_allsource EXCLUDE REGEX "${pattern}") +endforeach()
Line range hint
29-31: Consider making the clang-format version requirement configurable.The hard requirement for clang-format 17.0 could be problematic for developers. Consider making this configurable or allowing a range of compatible versions.
Here's a suggested approach:
+# Minimum required clang-format version +set(MIN_CLANG_FORMAT_VERSION "17.0") + # Version check message(STATUS ${CLANG_VERSION}) -if(NOT ${CLANG_VERSION} MATCHES "version 17\.0") - message(FATAL_ERROR "clang-format must be version 17.0") +if(NOT ${CLANG_VERSION} MATCHES "version ${MIN_CLANG_FORMAT_VERSION}") + message(FATAL_ERROR "clang-format must be version ${MIN_CLANG_FORMAT_VERSION}") endif()
Line range hint
34-38: Document the batch processing logic.The code processes files in batches of 8, but this number isn't documented. Consider adding a comment explaining why this batch size was chosen and make it configurable if appropriate.
Add a comment like this:
+# Process files in batches to avoid command line length limits +set(BATCH_SIZE 8) # Adjust this value based on your system's limits foreach(INDEX RANGE 0 ${LIST_LEN} 8) - list(SUBLIST iotivity_allsource ${INDEX} 8 TMP_PATH) + list(SUBLIST iotivity_allsource ${INDEX} ${BATCH_SIZE} TMP_PATH) execute_process(COMMAND ${CLANG_FORMAT_EXE} -i ${TMP_PATH}) endforeach()port/esp32/adapter/src/netif.c (3)
62-78: Consider concurrency-safety measures for interface list operations.
oc_netif_add_ip_interface()usesoc_list_addwithout locking. If multiple threads can add or remove interfaces concurrently, a data race is possible. Consider adding synchronization (e.g., mutex) around list operations or clarifying that this code is single-threaded.
80-92: Function name might be misleading.
oc_netif_check_new_ip_interfaces()is actually iterating and adding new IP interfaces. A more descriptive name such asoc_netif_update_ip_interfaces()oroc_netif_query_and_add_interfaces()may better reflect what it does.-bool oc_netif_check_new_ip_interfaces(void) +bool oc_netif_update_ip_interfaces(void)
108-118: Standardize naming to maintain clarity.
remove_all_ip_interface()removes all IP interfaces but lacks theoc_netif_prefix used by similar functions. This slight inconsistency might confuse new contributors. Consider renaming for uniformity:-void remove_all_ip_interface(void) +void oc_netif_remove_all_ip_interfaces(void)port/esp32/adapter/src/netif.h (4)
3-3: Update the copyright year.The copyright year 2025 appears to be incorrect. Please update it to reflect the current or past year when the code was written.
-* Copyright (c) 2025 plgd.dev s.r.o. +* Copyright (c) 2024 plgd.dev s.r.o.
45-49: Add documentation for the TCP-specific function.Please add documentation for
oc_netif_get_interface_indexto explain its purpose, parameters, and return value.#ifdef OC_TCP +/** @brief Get the network interface index for a socket + * + * @param sock socket file descriptor + * @return interface index on success, negative value on error + */ int oc_netif_get_interface_index(int sock); #endif /* OC_TCP */
51-61: Add documentation for network monitoring functions.Please add documentation for all network monitoring functions to explain their purposes, parameters, and return values.
+/** @brief Deinitialize network interface subsystem */ void oc_netif_deinit(void); #ifdef OC_NETWORK_MONITOR +/** @brief Add a new IP interface to the monitoring list + * + * @param target_index index of the interface to add + * @return true on success, false on failure + */ bool oc_netif_add_ip_interface(int target_index); +/** @brief Remove an IP interface from the monitoring list + * + * @param target_index index of the interface to remove + * @return true on success, false on failure + */ bool oc_netif_remove_ip_interface(int target_index); +/** @brief Check for any new IP interfaces + * + * @return true if new interfaces were found, false otherwise + */ bool oc_netif_check_new_ip_interfaces(void); #endif /* OC_NETWORK_MONITOR */
1-67: Overall implementation looks good with minor documentation improvements needed.The network interface header file is well-structured with clear separation of concerns and proper use of conditional compilation. The implementation aligns well with the PR objectives of updating network interface handling for ESP32. The main improvements needed are in the documentation to ensure maintainability.
Consider adding a brief file-level documentation comment at the top of the header file to explain its overall purpose and relationship with the ESP32 network interface subsystem.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
.github/workflows/CLA.ymlis excluded by!**/*.yml.github/workflows/android.ymlis excluded by!**/*.yml.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/cmake-linux.ymlis excluded by!**/*.yml.github/workflows/cmake-windows.ymlis excluded by!**/*.yml.github/workflows/coverity.ymlis excluded by!**/*.yml.github/workflows/ctt-bot.ymlis excluded by!**/*.yml.github/workflows/ctt-test.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish-with-cfg.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen.ymlis excluded by!**/*.yml.github/workflows/ghcr-check.ymlis excluded by!**/*.yml.github/workflows/ghcr-cleanup.ymlis excluded by!**/*.yml.github/workflows/plgd-device-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-device-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-tests.ymlis excluded by!**/*.yml.github/workflows/pr-format.ymlis excluded by!**/*.yml.github/workflows/sonar-cloud-analysis.ymlis excluded by!**/*.yml.github/workflows/static-analysis.ymlis excluded by!**/*.yml.github/workflows/unit-test-with-cfg.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
port/esp32/adapter/src/ipadapter.c(6 hunks)port/esp32/adapter/src/netif.c(1 hunks)port/esp32/adapter/src/netif.h(1 hunks)port/esp32/adapter/src/tcpadapter.c(3 hunks)port/esp32/main/CMakeLists.txt(1 hunks)tools/clang-format.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- port/esp32/adapter/src/tcpadapter.c
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: build_esp32 (true)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit")
- GitHub Check: build_esp32
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (13)
tools/clang-format.cmake (1)
6-6: LGTM! Good practice to exclude build and ESP-IDF directories.The added filters appropriately exclude the build directory (which contains generated files) and the ESP-IDF SDK directory (which is third-party code) from formatting. This aligns well with the PR's objectives regarding ESP32 platform support.
Also applies to: 8-8
port/esp32/main/CMakeLists.txt (1)
19-26: Source files reorganization looks good with one verification needed.The reordering of adapter source files and addition of
netif.caligns with the network interface management restructuring. However, let's verify the dependencies between these files.✅ Verification successful
Dependencies between network-related source files are properly structured
The network stack components show clean integration with no circular dependencies. The new
netif.cfile is correctly integrated into the existing network infrastructure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies between network-related source files # Check for any circular dependencies or missing includes echo "Checking for dependencies between network-related files..." for file in clock.c ipadapter.c netif.c random.c storage.c vfs_pipe.c; do echo "Analyzing dependencies for $file:" rg -l "$(basename $file .c)\.[h|c]" "port/esp32/adapter/src/" done # Verify netif.c integration echo "Checking netif.c integration:" rg -A 3 "include.*netif\.h" "port/esp32/adapter/src/"Length of output: 3457
port/esp32/adapter/src/netif.c (4)
1-17: License header looks good.
No changes needed here.
94-106: Removal logic appears straightforward.
oc_netif_remove_ip_interface()effectively frees resources and updates the list. This looks correct, but ensure all concurrent usages of this function are guarded or single-threaded to avoid race conditions.
134-150: Consider preventing duplicate callbacks.
oc_add_network_interface_event_callback()allows multiple registrations of the same callback without checks. If that’s undesired, add a pre-check or clarify that duplicates are allowed. Also, note that modifying the list while callbacks are being called may need special handling or documentation.
185-219: Matching logic looks correct for both IPv4 and IPv6.The function
netif_match_inferface()properly handles IPv4 by direct address match and IPv6 by comparing the 16-byte addresses. This appears consistent and correct for typical usage.port/esp32/adapter/src/ipadapter.c (5)
67-72: Remove unusedIN6_IS_ADDR_V4MAPPEDmacro definitionThe macro is defined but never used in the codebase. Additionally, a unit test comment explicitly states that "embedded ipv4 in ipv6 not supported". Since IPv4-mapped IPv6 addresses are not supported, this unused macro definition should be removed to reduce unnecessary code.
127-127: LGTM!The
oc_netif_deinit()call is correctly placed to clean up network interface resources before session events cleanup.
483-486: LGTM!The network interface monitoring code correctly handles both interface up and down events, properly triggering network interface events when the interface state changes.
Also applies to: 508-511
Line range hint
1555-1559: LGTM!The network interface initialization properly checks for new IP interfaces and includes appropriate error handling.
Line range hint
1-1800: Verify security implications of network interface changesWhile the changes maintain the security model, please verify:
- The network interface monitoring doesn't expose sensitive information
- The multicast socket configuration maintains proper isolation
- The secure socket handling remains intact across interface changes
Run the following script to verify security-sensitive code paths:
✅ Verification successful
Security review of network interface changes complete
The changes maintain proper security boundaries and socket isolation. The implementation correctly handles:
- Secure socket lifecycle and configuration
- Multicast group membership and scoping
- Network interface monitoring with proper validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential security issues in network interface handling echo "=== Checking secure socket handling ===" rg -p "secure.*sock" -B 2 -A 2 echo -e "\n=== Checking multicast socket configuration ===" rg -p "configure_mcast_socket|add_mcast_sock" -B 2 -A 2 echo -e "\n=== Checking network interface monitoring ===" rg -p "oc_netif_" -B 2 -A 2Length of output: 38412
port/esp32/adapter/src/netif.h (2)
22-29: LGTM!The includes are appropriate, and the extern "C" guards are correctly implemented for C++ compatibility.
63-67: LGTM!The closing guards for both extern "C" and the header file are properly implemented.
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
fcfe008 to
605340d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
port/esp32/adapter/src/netif.c (2)
45-48: Consider thread safety for shared lists.
ip_interface_listandoc_network_interface_cb_listare accessed and modified without any obvious synchronization, which could lead to data races if multiple tasks operate on them concurrently.Would you like assistance in adding appropriate synchronization (e.g., mutexes or semaphores) to ensure thread-safe operations on these lists?
190-190: Fix typo in function name.
The function namenetif_match_inferfacecontains a misspelling ("inferface" vs. "interface") which can lead to confusion.For consistency, rename it to
netif_match_interfacewherever it appears.-static bool netif_match_inferface(esp_netif_t *netif, void *user_data) +static bool netif_match_interface(esp_netif_t *netif, void *user_data)Also applies to: 256-256, 307-307
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
.github/workflows/CLA.ymlis excluded by!**/*.yml.github/workflows/android.ymlis excluded by!**/*.yml.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/cmake-linux.ymlis excluded by!**/*.yml.github/workflows/cmake-windows.ymlis excluded by!**/*.yml.github/workflows/coverity.ymlis excluded by!**/*.yml.github/workflows/ctt-bot.ymlis excluded by!**/*.yml.github/workflows/ctt-test.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish-with-cfg.ymlis excluded by!**/*.yml.github/workflows/docker-build-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen-publish.ymlis excluded by!**/*.yml.github/workflows/doxygen.ymlis excluded by!**/*.yml.github/workflows/ghcr-check.ymlis excluded by!**/*.yml.github/workflows/ghcr-cleanup.ymlis excluded by!**/*.yml.github/workflows/plgd-device-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-device-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-dps-tests.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-test-with-cfg.ymlis excluded by!**/*.yml.github/workflows/plgd-hub-tests.ymlis excluded by!**/*.yml.github/workflows/pr-format.ymlis excluded by!**/*.yml.github/workflows/sonar-cloud-analysis.ymlis excluded by!**/*.yml.github/workflows/static-analysis.ymlis excluded by!**/*.yml.github/workflows/unit-test-with-cfg.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
port/esp32/adapter/src/netif.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit", true)
- GitHub Check: build_esp32 (true)
- GitHub Check: build_esp32 (-DSDKCONFIG_DEFAULTS="sdkconfig.defaults;hawkbit/sdkconfig.defaults.hawkbit")
- GitHub Check: build_esp32
- GitHub Check: Analyze (c-cpp)
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
605340d to
cd28617
Compare
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
cd28617 to
98f489e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
port/esp32/adapter/src/netif.c (1)
Line range hint
1131-1132: Add error handling for setsockopt calls.The function makes multiple calls to
setsockoptwithout checking their return values. This could lead to silent failures and incorrect behavior.Apply this diff to add error handling:
if (setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_IF, &mif, sizeof(mif)) == -1) { OC_ERR("setting socket option for default IPV6_MULTICAST_IF: %d", errno); continue; } message->endpoint.interface_index = (unsigned)mif; if (ip6_addr_ismulticast_linklocal(ip6)) { message->endpoint.addr.ipv6.scope = mif; unsigned int hops = 1; - setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, - sizeof(hops)); + if (setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, + sizeof(hops)) == -1) { + OC_ERR("setting IPV6_MULTICAST_HOPS failed: %d", errno); + continue; + } } else if (IN6_IS_ADDR_MC_REALM_LOCAL(ip6)) { unsigned int hops = 255; - setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, - sizeof(hops)); + if (setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, + sizeof(hops)) == -1) { + OC_ERR("setting IPV6_MULTICAST_HOPS failed: %d", errno); + continue; + } message->endpoint.addr.ipv6.scope = 0; } else if (ip6_addr_ismulticast_sitelocal(ip6)) { unsigned int hops = 255; - setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, - sizeof(hops)); + if (setsockopt(server_sock, IPPROTO_IPV6, IPV6_MULTICAST_HOPS, &hops, + sizeof(hops)) == -1) { + OC_ERR("setting IPV6_MULTICAST_HOPS failed: %d", errno); + continue; + } message->endpoint.addr.ipv6.scope = 0; }Also applies to: 1140-1141, 1144-1145, 1149-1150
🧹 Nitpick comments (2)
port/esp32/adapter/src/netif.c (1)
Line range hint
1598-1601: Improve error message clarity.The error message for failed IP interface checks could be more descriptive to help with debugging.
if (!oc_netif_check_new_ip_interfaces()) { - OC_ERR("checking new IP interfaces failed."); + OC_ERR("Failed to check and initialize new IP interfaces during network monitor initialization."); return -1; }port/esp32/adapter/src/ipadapter.c (1)
1598-1601: Add debug logging for network interface initialization.Consider adding debug logs to help track the initialization progress and state of IP interfaces.
if (!oc_netif_check_new_ip_interfaces()) { OC_ERR("checking new IP interfaces failed."); + OC_DBG("Network interface initialization failed during device setup"); return -1; } + OC_DBG("Successfully initialized and checked all network interfaces");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/build.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
port/esp32/adapter/src/ipadapter.c(7 hunks)port/esp32/adapter/src/netif.c(1 hunks)port/esp32/adapter/src/netif.h(1 hunks)port/esp32/adapter/src/tcpadapter.c(3 hunks)port/esp32/main/CMakeLists.txt(1 hunks)port/linux/ipadapter.c(1 hunks)tools/clang-format.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/clang-format.cmake
- port/linux/ipadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/main/CMakeLists.txt
- port/esp32/adapter/src/netif.h
🔇 Additional comments (2)
port/esp32/adapter/src/netif.c (1)
67-72: Remove unusedIN6_IS_ADDR_V4MAPPEDmacro definition.The macro is defined but never used in the codebase. Additionally, a unit test comment explicitly states that "embedded ipv4 in ipv6 not supported". Since IPv4-mapped IPv6 addresses are not supported, this unused macro definition should be removed to reduce unnecessary code.
port/esp32/adapter/src/ipadapter.c (1)
1111-1232: LGTM! Well-structured network interface event handling.The implementation properly handles both IPv4 and IPv6 interfaces with appropriate error checking and event propagation.
|
Update default mbedTLS version to 3.6.2
Fixes #650
This PR updates the default mbedTLS version from 3.1.0 to 3.6.2 while maintaining optional support for v3.5.0 through conditional compilation. The update aims to enhance security and ensure compatibility with newer systems.
Key Changes
BUILD_MBEDTLS_FORCE_3_5_0build optionTechnical Details
BUILD_MBEDTLS_FORCE_3_5_0=1)Platform-specific Changes
Linux & Android
ESP32
Benefits
Migration
Users can continue using mbedTLS 3.5.0 by setting
BUILD_MBEDTLS_FORCE_3_5_0=1during build. However, it's recommended to migrate to 3.6.2 as 3.5.0 support will be removed in a future release.Testing
The changes have been tested across all supported platforms with both mbedTLS 3.6.2 and 3.5.0 configurations.