-
Notifications
You must be signed in to change notification settings - Fork 48
Adds ATECC608 secure element support for Espressif esp32 #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
43a6073
31b21c4
c405422
1f32e26
1a5d701
7d8738e
ab2b01c
0a0ccf4
1e75252
e959cec
73e1865
7f09074
3d3e73f
01a1ec6
a9bef7e
bdc8181
ff82cc2
7f87ce2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
[submodule "libs/azure-iot-middleware-freertos"] | ||
path = libs/azure-iot-middleware-freertos | ||
url = https://github.com/Azure/azure-iot-middleware-freertos.git | ||
[submodule "demos/projects/ESPRESSIF/esp32/components/esp-cryptoauthlib"] | ||
path = demos/projects/ESPRESSIF/esp32/components/esp-cryptoauthlib | ||
url = https://github.com/espressif/esp-cryptoauthlib |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,11 @@ TlsTransportStatus_t TLS_Socket_Connect( NetworkContext_t * pNetworkContext, | |
{ | ||
esp_transport_ssl_set_cert_data_der( pNetworkContext->xTransport, ( const char * ) pNetworkCredentials->pucRootCa, pNetworkCredentials->xRootCaSize ); | ||
} | ||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you have just enabled the secure element boolean in the esp_tls_cfg_t with this structure I think it would only be possible to use secure element of type TrustNGo ( and TrustFlex if it is not modified). But if you choose to use TrustCustom or TrustFlex with a modified certificate then I think you will have to provide the option to also provide the client certificate with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hi @AdityaHPatwardhan - this is a very good point.. thanks for bringing this up! I actually tested the development with a trustflex chip as well (with Microchip factory certs in them). It worked correctly as the cert defs were found correctly. I will add a comment to this effect in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, sounds good! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AdityaHPatwardhan Added the comment. |
||
|
||
esp_transport_ssl_use_secure_element( pNetworkContext->xTransport ); | ||
|
||
#else | ||
|
||
if ( pNetworkCredentials->pucClientCert ) | ||
{ | ||
|
@@ -101,6 +106,8 @@ TlsTransportStatus_t TLS_Socket_Connect( NetworkContext_t * pNetworkContext, | |
esp_transport_ssl_set_client_key_data_der( pNetworkContext->xTransport, (const char *) pNetworkCredentials->pucPrivateKey, pNetworkCredentials->xPrivateKeySize ); | ||
} | ||
|
||
#endif | ||
|
||
if ( esp_transport_connect( pNetworkContext->xTransport, pHostName, usPort, ulReceiveTimeoutMs ) < 0 ) | ||
{ | ||
ESP_LOGE( TAG, "Failed establishing TLS connection (esp_transport_connect failed)" ); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,11 @@ | |
/* Crypto helper header. */ | ||
#include "crypto.h" | ||
|
||
/* For using the ATECC608 secure element if support is configured */ | ||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT && CONFIG_ATCA_MBEDTLS_ECDSA | ||
#include "cryptoauthlib.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd like to maintain common code to be device-agnostic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! Do let me know if there is a change you want me to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move any code related to ESP32 to files within demos/projects/espressif/esp32/*. I think this should be moved into I am still reading about deriving certificates from ATEC608. If I understand correctly, only Trust&Go is guaranteed to always have the same certificate. Our Provisioning Service validates the certificate's key and the Common Name must match the Registration ID. Since for TrustFlex and TrustCUSTOM the certificate can be changed, the ESP-IDF Azure IoT config should take precendence. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, this is always right.
For TrustFLEX, if someone chooses to use the Microchip template certs, then the CN and hence the registration ID can be derived from ATECC608 chip. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The design I'm thinking of is to either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do we have a #define to allow using any alternative mechanism for private key, cert, etc. if not using the kconfig menu? If not, how do you feel about checking if the registration ID macro is blank wherever the registration ID is being used? If blank, we call the function that populates the registration ID correctly. This function can then be moved to the Pseudocode:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We do not, and the sample code is common among many other platforms so we cannot rely on kconfig only. I propose adding a new
For this application specifically, the developer would need to enable the ATECC chip in kconfig. Ideally, that could also automatically define This creates a design open to addition of new types of HSMs in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @CIPop - I need your feedback. Based on the comments above, I have altered the implementation such that there are no cryptoauthlib or ESP32 macros/functions in the common code. For instance, the dynamic registration ID part in
The democonfigUSE_HSM gets defined inside
One change that may be necessary is that we can no more use sizeof(democonfigREGISTRATION_ID) - 1 - will have to use strlen(democonfigREGISTRATION_ID). Quick question: Why are we using sizeof(...) - 1 instead of strlen(...) considering these macros are all strings anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me!
Asked the original devs: It was out of caution to make sure the value isn't determined at runtime, in case the compiler isn't smart enough to collapse I'm OK with either solution:
|
||
#endif | ||
|
||
/*-----------------------------------------------------------*/ | ||
|
||
/* Compile time error for undefined configs. */ | ||
|
@@ -145,6 +150,20 @@ struct NetworkContext | |
static AzureIoTHubClient_t xAzureIoTHubClient; | ||
/*-----------------------------------------------------------*/ | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/** | ||
* @brief Generates the registration ID using the ATECC608 chip dynamically using | ||
* cryptoauthlib API | ||
* | ||
* | ||
* @param[out] pucSecureElementSerNum An unsigned char buffer to hold the 9-byte serial number of the ATECC608 chip | ||
* @param[out] pcRegistrationID The string that will hold the registration ID - should be 21 bytes or more | ||
*/ | ||
static uint32_t prvPrepareRegistrationIdFromATECC608( uint8_t *sernum, | ||
char *registration_id_string ); | ||
|
||
#endif /* CONFIG_ESP_TLS_USE_SECURE_ELEMENT */ | ||
|
||
#ifdef democonfigENABLE_DPS_SAMPLE | ||
|
||
/** | ||
|
@@ -471,6 +490,33 @@ static void prvAzureDemoTask( void * pvParameters ) | |
} | ||
/*-----------------------------------------------------------*/ | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/** | ||
* @brief Get the serial number of the ATECC608 chip and the registration ID that will be used | ||
* by the DPS client | ||
* | ||
*/ | ||
static uint32_t prvPrepareRegistrationIdFromATECC608(uint8_t *sernum, char *registration_id_string) { | ||
CIPop marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if(sernum == NULL || registration_id_string == NULL || (strlen(registration_id_string) < 21)) { | ||
return -1; // improper parameters | ||
} | ||
|
||
ATCA_STATUS s; | ||
s = atcab_read_serial_number(sernum); | ||
if(s != ATCA_SUCCESS) { | ||
return -3; | ||
} | ||
sprintf(registration_id_string,"sn%02X%02X%02X%02X%02X%02X%02X%02X%02X",sernum[0],sernum[1],\ | ||
sernum[2],sernum[3],sernum[4],sernum[5],sernum[6],sernum[7],sernum[8]); | ||
registration_id_string[20] = '\0'; | ||
|
||
return 0; | ||
|
||
} | ||
|
||
#endif /* CONFIG_ESP_TLS_USE_SECURE_ELEMENT */ | ||
|
||
|
||
#ifdef democonfigENABLE_DPS_SAMPLE | ||
|
||
/** | ||
|
@@ -503,6 +549,16 @@ static void prvAzureDemoTask( void * pvParameters ) | |
xTransport.xSend = TLS_Socket_Send; | ||
xTransport.xRecv = TLS_Socket_Recv; | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/* Redefine the democonfigREGISTRATION_ID macro dynamically */ | ||
#undef democonfigREGISTRATION_ID | ||
char registration_id_string[21]; | ||
uint8_t sernum[9]; | ||
ulStatus = prvPrepareRegistrationIdFromATECC608(sernum,registration_id_string); | ||
configASSERT( ulStatus == 0); | ||
#define democonfigREGISTRATION_ID registration_id_string | ||
#endif | ||
|
||
xResult = AzureIoTProvisioningClient_Init( &xAzureIoTProvisioningClient, | ||
( const uint8_t * ) democonfigENDPOINT, | ||
sizeof( democonfigENDPOINT ) - 1, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,11 @@ | |
|
||
/* Data Interface Definition */ | ||
#include "sample_azure_iot_pnp_data_if.h" | ||
|
||
/* For using the ATECC608 secure element if support is configured */ | ||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT && CONFIG_ATCA_MBEDTLS_ECDSA | ||
#include "cryptoauthlib.h" | ||
#endif | ||
/*-----------------------------------------------------------*/ | ||
|
||
/* Compile time error for undefined configs. */ | ||
|
@@ -153,6 +158,20 @@ static uint8_t ucReportedPropertiesUpdate[ 320 ]; | |
static uint32_t ulReportedPropertiesUpdateLength; | ||
/*-----------------------------------------------------------*/ | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/** | ||
* @brief Generates the registration ID using the ATECC608 chip dynamically using | ||
* cryptoauthlib API | ||
* | ||
* | ||
* @param[out] pucSecureElementSerNum An unsigned char buffer to hold the 9-byte serial number of the ATECC608 chip | ||
* @param[out] pcRegistrationID The string that will hold the registration ID - should be 21 bytes or more | ||
*/ | ||
static uint32_t prvPrepareRegistrationIdFromATECC608( uint8_t *sernum, | ||
char *registration_id_string ); | ||
|
||
#endif /* CONFIG_ESP_TLS_USE_SECURE_ELEMENT */ | ||
|
||
#ifdef democonfigENABLE_DPS_SAMPLE | ||
|
||
/** | ||
|
@@ -486,6 +505,33 @@ static void prvAzureDemoTask( void * pvParameters ) | |
} | ||
/*-----------------------------------------------------------*/ | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/** | ||
* @brief Get the serial number of the ATECC608 chip and the registration ID that will be used | ||
* by the DPS client | ||
* | ||
*/ | ||
static uint32_t prvPrepareRegistrationIdFromATECC608(uint8_t *sernum, char *registration_id_string) { | ||
if(sernum == NULL || registration_id_string == NULL || (strlen(registration_id_string) < 21)) { | ||
return -1; // improper parameters | ||
} | ||
|
||
ATCA_STATUS s; | ||
s = atcab_read_serial_number(sernum); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a minor question - Are we sure that we have already initialised esp-tls at this point ( and thus the secure element ) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, we can be sure as the call to the Azure function would be done only after the TLS connection has been established with the Azure DPS service. The hint for me was that these lines appear in the log before the Azure code kicks in.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks for the explanation. I think this is fine. |
||
if(s != ATCA_SUCCESS) { | ||
return -3; | ||
} | ||
sprintf(registration_id_string,"sn%02X%02X%02X%02X%02X%02X%02X%02X%02X",sernum[0],sernum[1],\ | ||
sernum[2],sernum[3],sernum[4],sernum[5],sernum[6],sernum[7],sernum[8]); | ||
registration_id_string[20] = '\0'; | ||
|
||
return 0; | ||
|
||
} | ||
|
||
#endif /* CONFIG_ESP_TLS_USE_SECURE_ELEMENT */ | ||
|
||
|
||
#ifdef democonfigENABLE_DPS_SAMPLE | ||
|
||
/** | ||
|
@@ -518,6 +564,16 @@ static void prvAzureDemoTask( void * pvParameters ) | |
xTransport.xSend = TLS_Socket_Send; | ||
xTransport.xRecv = TLS_Socket_Recv; | ||
|
||
#if CONFIG_ESP_TLS_USE_SECURE_ELEMENT | ||
/* Redefine the democonfigREGISTRATION_ID macro dynamically */ | ||
#undef democonfigREGISTRATION_ID | ||
char registration_id_string[21]; | ||
uint8_t sernum[9]; | ||
ulStatus = prvPrepareRegistrationIdFromATECC608(sernum,registration_id_string); | ||
configASSERT( ulStatus == 0); | ||
#define democonfigREGISTRATION_ID registration_id_string | ||
#endif | ||
|
||
xResult = AzureIoTProvisioningClient_Init( &xAzureIoTProvisioningClient, | ||
( const uint8_t * ) democonfigENDPOINT, | ||
sizeof( democonfigENDPOINT ) - 1, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to move this to CMake FetchContent to avoid downloading the new lib for samples not using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Do I need to amend anything in my code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please move this to be a CMake component instead.
The implementation should be similar to the
FREERTOS_PATH
andfree_rtos_fetch()
function in the root CMakeLists.txt.We would also need documentation on how to use this in conjunction with the ESP32 sample (i.e. within the demo\projects\espressif\esp32\readme.md file).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to see where to plug in the esp-cryptoauthlib as a CMake component. One finding is that the CMakeLists.txt for the ESPRESSIF projects (esp32, aziot) simply calls the project.cmake of the IDF.
Considering that the IDF CMake does all the steps within itself (configure -> generate -> build...), there seems to be no way to tell the CMake file in the Azure IoT project that we need the esp-cryptoauthlib component.
If this is true, a viable solution can be one of the below two (unless I have misunderstood the CMake component concept itself - please correct me if I am wrong here).... @CIPop and @AdityaHPatwardhan
https://github.com/espressif/esp-idf/blob/master/.gitmodules#:~:text=%5Bsubmodule%20%22examples/peripherals,../../espressif/esp%2Dcryptoauthlib.git
Do you recommend a better option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CIPop Please ignore my comment above - I have figured out what I need to do to read the sdkconfig variable and conditionally fetch the esp-cryptoauthlib.