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 issues
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.
| +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 ((client_oid_len == 0 && client_oid) || | ||
| + (client_oid_len != 0 && !client_oid)|| | ||
| + (server_oid_len == 0 && server_oid) || | ||
| + (server_oid_len != 0 && !server_oid)) { | ||
| + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; | ||
| + } | ||
| + | ||
| + conf->client_oid = client_oid; | ||
| + conf->client_oid_len = client_oid_len; | ||
| + conf->server_oid = server_oid; | ||
| + conf->server_oid_len = server_oid_len; | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
💡 Codebase verification
Add NULL pointer check for conf parameter.
The NULL check for conf parameter should be added as suggested, following mbedtls's defensive programming practices. However, the direct pointer storage without copying is the standard pattern for mbedtls configuration functions, where the caller is responsible for ensuring the data remains valid throughout the SSL configuration's lifetime.
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) ||🔗 Analysis chain
Add NULL pointer check and consider data ownership.
Two potential issues in mbedtls_ssl_conf_ekus:
- Missing NULL check for
confparameter - Direct pointer assignment without data copying could lead to dangling pointers
Apply this diff to add the NULL check:
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) ||Consider copying the OID data if the pointers' lifetime cannot be guaranteed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for callers of mbedtls_ssl_conf_ekus to analyze pointer lifetime
ast-grep --pattern 'mbedtls_ssl_conf_ekus($$$)'
Length of output: 49
Script:
#!/bin/bash
# Search for similar configuration functions to understand the patterns
ast-grep --pattern 'mbedtls_ssl_conf_$_($$$)'
# Also search with ripgrep to ensure we don't miss anything
rg "mbedtls_ssl_conf_" -A 2
Length of output: 17520
Script:
#!/bin/bash
# Search for implementation of similar configuration functions to check NULL handling
ast-grep --pattern 'int mbedtls_ssl_conf_$_($$$) {
$$$
}'
# Also check with ripgrep for better coverage
rg "^int mbedtls_ssl_conf_.*\(" -A 5
Length of output: 100
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.
api/unittest/tcptest.cpp
Outdated
| mbedtls_ctr_drbg_context ctr_drbg; | ||
| mbedtls_ctr_drbg_init(&ctr_drbg); | ||
| mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg); |
There was a problem hiding this comment.
Add entropy seeding for the CTR-DRBG context.
The CTR-DRBG context is initialized but not seeded with entropy, which is required for secure random number generation.
Add entropy seeding before configuring the RNG:
mbedtls_ctr_drbg_context ctr_drbg;
mbedtls_ctr_drbg_init(&ctr_drbg);
+ mbedtls_entropy_context entropy;
+ mbedtls_entropy_init(&entropy);
+ int ret = mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, &entropy,
+ nullptr, 0);
+ ASSERT_EQ(0, ret) << " failed with error: " << ret;
mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg);And cleanup the entropy context:
mbedtls_ssl_free(&ssl);
mbedtls_ctr_drbg_free(&ctr_drbg);
+ mbedtls_entropy_free(&entropy);
mbedtls_ssl_config_free(&conf);Also applies to: 304-304
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
from
791ecbe to
aab1587
Compare
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
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.
| mbedtls_ctr_drbg_context ctr_drbg; | ||
| mbedtls_ctr_drbg_init(&ctr_drbg); | ||
| mbedtls_entropy_context entropy_ctx; | ||
| mbedtls_entropy_init(&entropy_ctx); | ||
| oc_entropy_add_source(&entropy_ctx); | ||
| std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' }; | ||
| ASSERT_EQ(0, mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, | ||
| &entropy_ctx, pers.data(), pers.size())); | ||
| mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg); |
There was a problem hiding this comment.
Add error handling for entropy seeding.
The entropy seeding operation could fail, but the error code is not checked.
Apply this diff to add error handling:
mbedtls_ctr_drbg_init(&ctr_drbg);
mbedtls_entropy_context entropy_ctx;
mbedtls_entropy_init(&entropy_ctx);
oc_entropy_add_source(&entropy_ctx);
std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' };
- ASSERT_EQ(0, mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func,
- &entropy_ctx, pers.data(), pers.size()));
+ int ret = mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func,
+ &entropy_ctx, pers.data(), pers.size());
+ ASSERT_EQ(0, ret) << "CTR-DRBG seeding failed with error: " << ret;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mbedtls_ctr_drbg_context ctr_drbg; | |
| mbedtls_ctr_drbg_init(&ctr_drbg); | |
| mbedtls_entropy_context entropy_ctx; | |
| mbedtls_entropy_init(&entropy_ctx); | |
| oc_entropy_add_source(&entropy_ctx); | |
| std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' }; | |
| ASSERT_EQ(0, mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, | |
| &entropy_ctx, pers.data(), pers.size())); | |
| mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg); | |
| mbedtls_ctr_drbg_context ctr_drbg; | |
| mbedtls_ctr_drbg_init(&ctr_drbg); | |
| mbedtls_entropy_context entropy_ctx; | |
| mbedtls_entropy_init(&entropy_ctx); | |
| oc_entropy_add_source(&entropy_ctx); | |
| std::vector<unsigned char> pers = { 't', 'e', 's', 't', '\0' }; | |
| int ret = mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, | |
| &entropy_ctx, pers.data(), pers.size()); | |
| ASSERT_EQ(0, ret) << "CTR-DRBG seeding failed with error: " << ret; | |
| mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg); |
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
| +#include <oc_config.h> | ||
| +#include <oc_log.h> | ||
| +#include <port/oc_assert.h> | ||
| +#include <port/oc_connectivity.h> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Including Project-Specific Headers in Mbed TLS Configuration
Including OCF-specific headers like <oc_config.h> and <oc_log.h> directly in mbedtls_config.h may lead to namespace pollution and tight coupling between Mbed TLS and your project code. This practice can cause maintenance issues and conflicts with other projects using Mbed TLS.
Consider removing these #include directives and defining necessary macros externally or using configuration options provided by Mbed TLS.
| -#define MBEDTLS_PKCS1_V21 | ||
| +//#define MBEDTLS_PKCS1_V21 |
There was a problem hiding this comment.
Disabling PKCS#1 v2.1 May Reduce Security of RSA Operations
By commenting out MBEDTLS_PKCS1_V21, support for RSAES-OAEP and RSASSA-PSS is disabled. These schemes provide improved security over older RSA padding schemes and are recommended for new applications.
Consider re-enabling PKCS#1 v2.1:
-//#define MBEDTLS_PKCS1_V21
+#define MBEDTLS_PKCS1_V21📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -#define MBEDTLS_PKCS1_V21 | |
| +//#define MBEDTLS_PKCS1_V21 | |
| #define MBEDTLS_PKCS1_V21 |
| -#define MBEDTLS_ECP_DP_CURVE25519_ENABLED | ||
| -#define MBEDTLS_ECP_DP_CURVE448_ENABLED | ||
| +//#define MBEDTLS_ECP_DP_CURVE25519_ENABLED | ||
| +//#define MBEDTLS_ECP_DP_CURVE448_ENABLED | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disabling Modern Elliptic Curves May Impact Security
Disabling support for MBEDTLS_ECP_DP_CURVE25519_ENABLED and MBEDTLS_ECP_DP_CURVE448_ENABLED removes Curve25519 and Curve448, which are recommended for ECDH key exchanges due to their strong security properties and performance.
Consider re-enabling these curves:
-//#define MBEDTLS_ECP_DP_CURVE25519_ENABLED
-//#define MBEDTLS_ECP_DP_CURVE448_ENABLED
+#define MBEDTLS_ECP_DP_CURVE25519_ENABLED
+#define MBEDTLS_ECP_DP_CURVE448_ENABLED📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -#define MBEDTLS_ECP_DP_CURVE25519_ENABLED | |
| -#define MBEDTLS_ECP_DP_CURVE448_ENABLED | |
| +//#define MBEDTLS_ECP_DP_CURVE25519_ENABLED | |
| +//#define MBEDTLS_ECP_DP_CURVE448_ENABLED | |
| #define MBEDTLS_ECP_DP_CURVE25519_ENABLED | |
| #define MBEDTLS_ECP_DP_CURVE448_ENABLED | |
| -#define MBEDTLS_SSL_ALPN | ||
| +//#define MBEDTLS_SSL_ALPN | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Impact on Application Layer Protocol Negotiation (ALPN)
Disabling MBEDTLS_SSL_ALPN removes support for ALPN. ALPN is essential for applications that negotiate the protocol to be used over a secure channel, such as HTTP/2 or CoAP.
Consider re-enabling ALPN support:
-//#define MBEDTLS_SSL_ALPN
+#define MBEDTLS_SSL_ALPN📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -#define MBEDTLS_SSL_ALPN | |
| +//#define MBEDTLS_SSL_ALPN | |
| #define MBEDTLS_SSL_ALPN | |
| +//#define MBEDTLS_ECDSA_DETERMINISTIC | ||
|
|
There was a problem hiding this comment.
Potential Security Risk: Disabling Deterministic ECDSA
Disabling MBEDTLS_ECDSA_DETERMINISTIC may introduce vulnerabilities. Deterministic ECDSA ensures that the same message and key pair always produce the same signature, reducing the risk associated with poor randomness in signature generation.
Apply this diff to re-enable deterministic ECDSA:
-//#define MBEDTLS_ECDSA_DETERMINISTIC
+#define MBEDTLS_ECDSA_DETERMINISTIC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +//#define MBEDTLS_ECDSA_DETERMINISTIC | |
| #define MBEDTLS_ECDSA_DETERMINISTIC | |
| */ | ||
| -#define MBEDTLS_SSL_PROTO_TLS1_3 | ||
| +//#define MBEDTLS_SSL_PROTO_TLS1_3 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disabling TLS 1.3 Support Limits Future Compatibility
By commenting out MBEDTLS_SSL_PROTO_TLS1_3, TLS 1.3 support is being disabled. TLS 1.3 offers significant security and performance improvements over previous versions. Consider keeping TLS 1.3 enabled to ensure compatibility with modern clients and enhance security.
Apply this diff to re-enable TLS 1.3 support:
-//#define MBEDTLS_SSL_PROTO_TLS1_3
+#define MBEDTLS_SSL_PROTO_TLS1_3Committable suggestion skipped: line range outside the PR's diff.
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
port/android/Makefile
Outdated
| 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} | ||
| endif | ||
|
|
||
| endif | ||
|
|
||
| endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Synchronize 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.
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
port/android/Makefile
Outdated
| 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 || 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} && \ | ||
| git clean -fdx . && \ | ||
| git reset --hard && \ | ||
| cd -; \ | ||
| for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ | ||
| echo "Patches applied in $^" > ${@F} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Synchronize patching improvements across versions.
The patching process improvements suggested for version 3.6.2 should be applied to version 3.5.0 as well 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 || 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}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 || 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} && \ | |
| git clean -fdx . && \ | |
| git reset --hard && \ | |
| cd -; \ | |
| for patch in $(MBEDTLS_PATCHES); do patch -r - -s -N -p1 < $${patch} ; done && \ | |
| echo "Patches applied in $^" > ${@F} | |
| 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 \ | |
| 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} | |
🧰 Tools
🪛 GitHub Actions: CI building android
[error] 489: Build failed while trying to patch mbedTLS files - '../../deps/mbedtls/patched.txt' target failed
port/android/Makefile
Outdated
| 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} | ||
|
|
There was a problem hiding this comment.
Fix the patching process for mbedTLS 3.6.2.
The pipeline failure indicates issues with the patching process. 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 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}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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} | |
| 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 \ | |
| 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} |
🧰 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
3 times, most recently
from
af29573 to
2b6ac40
Compare
Danielius1922
force-pushed
the
adam/feature/mbedTLS_3_6_2
branch
from
2b6ac40 to
0c40061
Compare
|
| @@ -42,6 +42,7 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON) | |||
| set(BUILD_EXAMPLE_APPLICATIONS ON CACHE BOOL "Build example applications.") | |||
| set(BUILD_MBEDTLS ON CACHE BOOL "Build Mbed TLS library. When set to OFF, the Mbed TLS library with the OCF patches has to be provided.") | |||
| set(BUILD_MBEDTLS_FORCE_3_5_0 OFF CACHE BOOL "Force v3.5.0 of the MbedTLS library to be used (by default v3.1.0 is used by master)") | |||
| 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)") | |||
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.
No description provided.