Fix handling of own certificate in SSL context #3140
Conversation
WalkthroughThis pull request updates the handling of X.509 device certificates. The comments in the device certificate structure were revised to better reflect their purpose. In the MbedTLS files, the certificate management has been refactored by replacing a single certificate pointer with two distinct members ( Changes
Sequence Diagram(s)sequenceDiagram
participant Init as ssl_generic_init_internal
participant Parse as ssl_add_cert_auth_internal
participant Cleanup as ssl_exit_context_internal
Init->>Parse: Allocate and assign ca_cert and own_cert
Parse->>Parse: Parse certificate using ca_cert
Parse->>Init: Return updated SSL context
Cleanup->>Cleanup: Check for non-null pointers (ca_cert, own_cert, etc.)
Cleanup->>Cleanup: Free ca_cert and own_cert safely
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (5)
src/HAL/Include/nanoHAL_Network.h(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/mbedtls.h(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_add_cert_auth_internal.cpp(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_exit_context_internal.cpp(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic_init_internal.cpp(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (14)
src/HAL/Include/nanoHAL_Network.h (2)
303-303: Improved clarity in certificate documentationThe comment change better reflects that this structure is for a device certificate, which aligns with its actual usage. This enhances code readability and reduces potential confusion.
306-306: Consistent terminology for certificate documentationThis comment update maintains consistency with the field name change above, properly identifying this as a device certificate rather than a CA root certificate bundle.
src/PAL/COM/sockets/ssl/MbedTLS/ssl_add_cert_auth_internal.cpp (2)
30-30: Updated certificate reference to use new structure memberThe change from
context->x509_crttocontext->ca_certproperly reflects the structure update in mbedtls.h and improves code clarity by explicitly indicating this is a CA certificate.
33-33: Consistent use of new certificate member nameThis update ensures consistency with the structural changes made to the SSL context, correctly using the CA certificate member for the certificate chain configuration.
src/PAL/COM/sockets/ssl/MbedTLS/mbedtls.h (1)
37-38: Enhanced certificate management with explicit member typesSplitting the single certificate pointer into separate CA certificate and own certificate pointers improves code clarity and aligns with the PR objective to better handle the device's own certificate.
This change:
- Makes the code's intent clearer
- Properly separates concerns between CA certificates and device certificates
- Provides a more maintainable structure for certificate handling
src/PAL/COM/sockets/ssl/MbedTLS/ssl_exit_context_internal.cpp (2)
33-34: Proper cleanup for both certificate typesAdding a separate call to free the device's own certificate ensures proper resource management and prevents memory leaks. This is a necessary change to support the new certificate structure.
41-84: Enhanced memory safety with null checksAdding null checks before freeing memory is an excellent improvement that prevents potential crashes when attempting to free NULL pointers. This defensive programming approach is particularly important in embedded systems.
The code now safely:
- Checks each pointer before freeing
- Handles the new certificate members properly
- Maintains a consistent pattern for all resource cleanup
src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic_init_internal.cpp (7)
133-138: Variable renaming improves clarityRenaming from
x509_crttoca_certmakes the purpose of this certificate clearer, differentiating it from the user's own certificate. This aligns well with the PR objective to enhance clarity in certificate handling.
215-215: Good practice: NULL assignment after freeSetting the pointer to NULL after freeing helps prevent potential use-after-free issues and makes debugging easier by ensuring invalid memory accesses would be null pointer dereferences rather than accessing freed memory.
263-273: Improved certificate management through context structureThis change properly allocates and initializes the user's own certificate directly within the context structure rather than using a local variable. This approach is more maintainable as it keeps all related certificate data together and ensures proper cleanup through the context's lifecycle.
289-289: Good practice: NULL assignment after freeSetting
deviceCertto NULL after freeing it prevents potential use-after-free issues if the pointer is accidentally used later in the function.
300-300: Updated function call with renamed certificate variableFunction call has been correctly updated to use the renamed
ca_certvariable, maintaining consistency with the earlier variable renaming.
349-350: Memory management for both certificate typesThe code now properly frees both types of certificates (
ca_certandown_cert) in the error handling path, which prevents memory leaks.
380-388: Improved error handling with NULL checksThe error handling has been enhanced to check for NULL before freeing both certificate types, which is a good defensive programming practice to prevent null pointer dereferences.
- Rename CA cert element in SSL context (for clarity). - Add new element for own certificate. - Update code to use new element in context instead of local variable. - Add code to free memory of own certificate. - Update comments in HAL_Configuration_X509DeviceCertificate to match intend usage.
|
My only comment is that |
For the naming I've followed the hint of mbedTLS API |
|
@networkfusion any follow-up on the above, or OK to merge? |
OK to merge 👍 |
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit