Add GetDevicePublicKey #2400
Add GetDevicePublicKey #2400
Conversation
nfbot
added
the
Type: dependencies
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp
Outdated
Show resolved
Hide resolved
aromaa
force-pushed
the
feature/device-public-key
branch
from
918c50c to
e66624a
Compare
nfbot
added
invalid
and removed
Type: dependencies
|
Okay, I somehow did something wrong while rebasing this to fix the force push conflict and messed up the whole commit history and lost my own while at it. sigh Anyways, fixed it. |
josesimoes
force-pushed
the
main
branch
2 times, most recently
from
a02925b to
ee07b5a
Compare
|
@aromaa Is this PR still valid given all the changes? |
|
This needs a bit updating to stub the new platforms and a bit of updating. I'm planning to do that in the future as I will be needing to update my devices soon for the new Azure certificate so the branch will be up to date after that. |
aromaa
force-pushed
the
feature/device-public-key
branch
from
405b6c8 to
2e99008
Compare
|
Actually there wasn't much else to do as the new platforms are already falling back to the existing stubs. Fixed few copy & paste mistakes. Still seems to work just fine on my ESP32. |
nfbot
added
the
Type: dependencies
josesimoes
force-pushed
the
main
branch
2 times, most recently
from
02e0d18 to
591537f
Compare
josesimoes
force-pushed
the
main
branch
3 times, most recently
from
288fd6f to
c6dee33
Compare
josesimoes
force-pushed
the
main
branch
3 times, most recently
from
f1c86c5 to
449142a
Compare
aromaa
force-pushed
the
feature/device-public-key
branch
from
c7f5b09 to
1f3b8b8
Compare
WalkthroughThe pull request introduces a new method for retrieving a device's public key in raw byte array format across multiple system components. This change involves adding a new method Changes
Sequence DiagramsequenceDiagram
participant CertificateManager
participant SSL
participant DeviceCertificate
CertificateManager->>DeviceCertificate: GetDeviceCertificate()
DeviceCertificate-->>CertificateManager: Return Certificate
CertificateManager->>SSL: SSL_GetPublicKeyRaw(certificate)
SSL->>SSL: Initialize SSL if needed
SSL-->>CertificateManager: Extract Raw Public Key
CertificateManager->>CertificateManager: Create Byte Array
CertificateManager-->>CertificateManager: Return Public Key
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp (1)
63-64: Add error logging for certificate parsing failures.The function silently fails when certificate parsing fails. Consider adding error logging to help with debugging.
Apply this diff to add error logging:
ret = mbedtls_x509_crt_parse(&cacert, (const unsigned char *)certificate, size); if (ret < 0) { + char error_buf[100]; + mbedtls_strerror(ret, error_buf, sizeof(error_buf)); + TRACE_ERROR("Certificate parsing failed: %s\n", error_buf); mbedtls_x509_crt_free(&cacert); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/DeviceInterfaces/System.Net/sys_net_native.cpp(2 hunks)src/DeviceInterfaces/System.Net/sys_net_native.h(1 hunks)src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp(1 hunks)src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager_stubs.cpp(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp(1 hunks)src/PAL/COM/sockets/ssl/ssl.cpp(1 hunks)src/PAL/COM/sockets/ssl/ssl_functions.h(1 hunks)src/PAL/COM/sockets/ssl/ssl_stubs.cpp(1 hunks)src/PAL/Include/nanoPAL_Sockets.h(2 hunks)targets/AzureRTOS/ST/_common/drivers/wifi/inventek/ssl_ISM43362.cpp(3 hunks)targets/TI_SimpleLink/_common/ssl_simplelink.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/DeviceInterfaces/System.Net/sys_net_native.h
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (11)
src/PAL/COM/sockets/ssl/ssl.cpp (1)
87-97: LGTM!The implementation follows the established pattern in the file and properly handles SSL initialization.
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager_stubs.cpp (1)
18-26: LGTM!The implementation follows the established pattern in the file and properly uses
NotImplementedStub().targets/AzureRTOS/ST/_common/drivers/wifi/inventek/ssl_ISM43362.cpp (2)
26-34: Implementation follows the established pattern for stub functions.The stub implementation correctly follows the pattern used throughout the file for unsupported operations, properly handling unused parameters and providing a clear comment explaining the return value.
234-234: LGTM! Improved comment formatting.The added space after // improves readability and maintains consistency with the codebase's comment style.
src/DeviceInterfaces/System.Net/sys_net_native.cpp (2)
198-199: LGTM! Method handler properly added to the lookup table.The new method handler for
GetDevicePublicKeyRawis correctly placed in the method_lookup array.
350-352: Version numbers properly updated.Version identifier and version info are correctly incremented to reflect the addition of new functionality.
targets/TI_SimpleLink/_common/ssl_simplelink.cpp (1)
34-42: LGTM! Implementation maintains cross-platform consistency.The stub implementation matches the one in ssl_ISM43362.cpp, maintaining consistency across different platform implementations.
src/PAL/Include/nanoPAL_Sockets.h (2)
41-45: LGTM! Well-designed structure for raw certificate data.The
_X509RawDatastructure is properly designed with appropriate fields for handling raw certificate data:
len: Size of the raw datap: Pointer to the raw data buffer
723-723: LGTM! Function declaration follows the API pattern.The
SSL_GetPublicKeyRawfunction declaration properly follows the established API pattern with clear parameter names and appropriate types.src/PAL/COM/sockets/ssl/ssl_functions.h (1)
39-39: LGTM! Verify function declaration consistency.The new function declaration follows the established pattern and matches the implementation.
Let's verify the consistency of this function declaration across different implementations:
✅ Verification successful
Function declaration is consistent across all implementations
The
ssl_get_public_key_raw_internalfunction signature matches perfectly in all platform-specific implementations (MbedTLS, Inventek WiFi, and TI SimpleLink), with consistent parameter types, order, and return type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all implementations of ssl_get_public_key_raw_internal rg -l "ssl_get_public_key_raw_internal" | grep -v "ssl_functions.h"Length of output: 281
Script:
#!/bin/bash # Check implementations of ssl_get_public_key_raw_internal echo "=== Main SSL Implementation ===" rg "ssl_get_public_key_raw_internal" "src/PAL/COM/sockets/ssl/ssl.cpp" -A 2 echo -e "\n=== MbedTLS Implementation ===" rg "ssl_get_public_key_raw_internal" "src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp" -A 2 echo -e "\n=== Inventek WiFi Implementation ===" rg "ssl_get_public_key_raw_internal" "targets/AzureRTOS/ST/_common/drivers/wifi/inventek/ssl_ISM43362.cpp" -A 2 echo -e "\n=== TI SimpleLink Implementation ===" rg "ssl_get_public_key_raw_internal" "targets/TI_SimpleLink/_common/ssl_simplelink.cpp" -A 2Length of output: 1148
src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp (1)
55-78: Add unit tests for the new functionality.Ensure comprehensive test coverage for the new public key extraction functionality.
Let's check for existing tests:
Would you like me to help create unit tests for this new functionality?
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp
Outdated
Show resolved
Hide resolved
src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp
Outdated
Show resolved
Hide resolved
src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp (2)
94-114: Optimize success path and error handling.The nested if conditions can be flattened for better readability and error handling.
Apply this diff to optimize the code structure:
- if (deviceCert) + X509RawData rawData; + bool success = SSL_GetPublicKeyRaw((const char *)deviceCert->Certificate, deviceCert->CertificateSize, &rawData); + + // Free device cert as soon as possible + platform_free(deviceCert); + + if (success) { - X509RawData rawData; - bool success = SSL_GetPublicKeyRaw((const char *)deviceCert->Certificate, deviceCert->CertificateSize, &rawData); + CLR_RT_HeapBlock_Array *array; - platform_free(deviceCert); + NANOCLR_CHECK_HRESULT( + CLR_RT_HeapBlock_Array::CreateInstance(ret, rawData.len, g_CLR_RT_WellKnownTypes.m_UInt8)); - if (success) - { - CLR_RT_HeapBlock_Array *array; + array = ret.DereferenceArray(); + memcpy(array->GetFirstElement(), rawData.p, rawData.len); + platform_free(rawData.p); + } - NANOCLR_CHECK_HRESULT( - CLR_RT_HeapBlock_Array::CreateInstance(ret, rawData.len, g_CLR_RT_WellKnownTypes.m_UInt8)); - - array = ret.DereferenceArray(); - - memcpy(array->GetFirstElement(), rawData.p, rawData.len); - - platform_free(rawData.p); - } - }
116-116: Update cleanup macro to match error handling.Since we're not using any cleanup labels, use
NANOCLR_NOCLEANUP_NOLABEL()instead.Apply this diff:
- NANOCLR_NOCLEANUP(); + NANOCLR_NOCLEANUP_NOLABEL();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp(1 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (3)
src/PAL/COM/sockets/ssl/MbedTLS/ssl_parse_certificate_internal.cpp (2)
70-79:⚠️ Potential issueAdd bounds checking and document memory ownership.
The implementation needs:
- Size validation before allocation
- Documentation about memory ownership
Apply this diff to add size validation and documentation:
+ // Validate certificate size before allocation + if (cacert.raw.len == 0 || cacert.raw.len > MAX_CERTIFICATE_SIZE) + { + mbedtls_x509_crt_free(&cacert); + return false; + } + x509->len = cacert.raw.len; + // Note: Caller takes ownership of the allocated memory and must free it using platform_free x509->p = (unsigned char *)platform_malloc(x509->len); if (x509->p == NULL) { mbedtls_x509_crt_free(&cacert); return false; } memcpy(x509->p, cacert.raw.p, x509->len);Likely invalid or redundant comment.
55-58:⚠️ Potential issueAdd input parameter validation.
The function should validate input parameters before proceeding with operations.
Apply this diff to add parameter validation:
bool ssl_get_public_key_raw_internal(void *certificate, size_t size, void *x509RawData) { int ret; + if (certificate == NULL || x509RawData == NULL || size == 0) + { + return false; + } + X509RawData *x509 = (X509RawData *)x509RawData;Likely invalid or redundant comment.
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_CertificateManager.cpp (1)
91-93:⚠️ Potential issueAdd error handling for device certificate retrieval.
The code should validate the device certificate retrieval result using
FAULT_ON_NULL.Apply this diff to add proper error handling:
CLR_RT_HeapBlock &ret = stack.PushValueAndClear(); HAL_Configuration_X509DeviceCertificate *deviceCert = ConfigurationManager_GetDeviceCertificate(); + FAULT_ON_NULL(deviceCert);Likely invalid or redundant comment.
|
Rebased and updated the System.Net native bindings. Everything seems to still work just fine on my ESP32. |
josesimoes
force-pushed
the
main
branch
2 times, most recently
from
eeab92b to
1da6e94
Compare
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Version Update
Technical Improvements