Skip to content

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

Merged
merged 18 commits into from
Aug 10, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
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"]
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 and free_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).

Copy link
Contributor Author

@sckulkarni246 sckulkarni246 Jul 11, 2022

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

Do you recommend a better option?

Copy link
Contributor Author

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.

path = demos/projects/ESPRESSIF/esp32/components/esp-cryptoauthlib
url = https://github.com/espressif/esp-cryptoauthlib
Submodule esp-cryptoauthlib added at 985ea9
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@AdityaHPatwardhan AdityaHPatwardhan May 17, 2022

Choose a reason for hiding this comment

The 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).
for TrustNGo no additional data is necessary just enabling the boolean and selecting the appropriate type of secure element ( which is set to TrustNGo by default) is enough.

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.
Also currently the esp-tls is hardcoded (For simplicity) to use key block 0 for the TLS connection. Should that be mentioned somethere in a comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 transport_tls_esp32.c file for now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdityaHPatwardhan Added the comment.

31b21c4


esp_transport_ssl_use_secure_element( pNetworkContext->xTransport );

#else

if ( pNetworkCredentials->pucClientCert )
{
Expand All @@ -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)" );
Expand Down
56 changes: 56 additions & 0 deletions demos/sample_azure_iot/sample_azure_iot.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like to maintain common code to be device-agnostic.
We will provide a different include file with an interface that extracts the registration ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 transport_tls_esp32.c.

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.

Copy link
Contributor Author

@sckulkarni246 sckulkarni246 Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still reading about deriving certificates from ATEC608. If I understand correctly, only Trust&Go is guaranteed to always have the same certificate.

Yes, this is always right.

Since for TrustFlex and TrustCUSTOM the certificate can be changed, the ESP-IDF Azure IoT config should take precendence.

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.
But, I see why accommodating everything can be tricky here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design I'm thinking of is to either:

  1. Use the #defines that we already have
  2. Use a common interface to extract information such as registrationID/deviceID, etc (we could, in the future, add hub name, global endpoint, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the #defines that we already have

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 transport_tls_esp32.c file.

Pseudocode:

...

if registration ID macro is blank

    call the function that calls cryptoauthlib API to populate the registration ID

endif

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

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 #define USE_HSM (hardware security module) and a new interface - let's start with a single function to obtain the registration ID.

ifdef USE_HSM
   call getRegistrationID() (transport_tls_esp32.c would need to define one specific to reading it from the ATECC chip _if_ the chip is enabled in kconfig).
else
  use the registration ID maco
endif

For this application specifically, the developer would need to enable the ATECC chip in kconfig. Ideally, that could also automatically define USE_HSM.

This creates a design open to addition of new types of HSMs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 sample_azure_iot_pnp.c looks like this.

#ifdef democonfigUSE_HSM
        /* Redefine the democonfigREGISTRATION_ID macro using registration ID 
        generated dynamically using the HSM */
        
        /* We use a pointer instead of a buffer so that the getRegistrationId
         function can allocate the necessary memory depending on the HSM */
        char *registration_id = NULL;
        ulStatus = getRegistrationId( &registration_id );
        configASSERT( ulStatus == 0);
        #undef democonfigREGISTRATION_ID
        #define democonfigREGISTRATION_ID   registration_id
             
#endif

The democonfigUSE_HSM gets defined inside demo_config.h depending on whether the use of secure element is enabled as below.

/**
 * @brief Defines the macro for HSM usage depending on whether 
 * the support for ATECC608 is enabled in the kconfig menu
 */
#ifdef CONFIG_ESP_TLS_USE_SECURE_ELEMENT
    #if CONFIG_MBEDTLS_ATCA_HW_ECDSA_SIGN & CONFIG_MBEDTLS_ATCA_HW_ECDSA_VERIFY
        #define democonfigUSE_HSM
        uint32_t getRegistrationId( char ** ); // should be defined by provider
    #endif
#endif /* CONFIG_ESP_TLS_USE_SECURE_ELEMENT */

One change that may be necessary is that we can no more use sizeof(democonfigREGISTRATION_ID) - 1 - will have to use strlen(democonfigREGISTRATION_ID).
What are your thoughts on this code structure?

Quick question: Why are we using sizeof(...) - 1 instead of strlen(...) considering these macros are all strings anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Why are we using sizeof(...) - 1 instead of strlen(...) considering these macros are all strings anyway?

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 strlen(const) to the appropriate numerical value.

I'm OK with either solution:

  • use strlen() everywhere
  • create an #else branch leaving the old code as-is

#endif

/*-----------------------------------------------------------*/

/* Compile time error for undefined configs. */
Expand Down Expand Up @@ -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

/**
Expand Down Expand Up @@ -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) {
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

/**
Expand Down Expand Up @@ -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,
Expand Down
58 changes: 58 additions & 0 deletions demos/sample_azure_iot_gsg/sample_azure_iot_gsg.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@

/* Board specific implementation */
#include "sample_gsg_device.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. */
Expand Down Expand Up @@ -128,6 +134,21 @@ struct NetworkContext
};
/*-----------------------------------------------------------*/

#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 */


/* Define buffer for IoT Hub info. */
#ifdef democonfigENABLE_DPS_SAMPLE

Expand Down Expand Up @@ -603,6 +624,33 @@ static uint32_t prvConnectToServerWithBackoffRetries( const char * pcHostName,
}
/*-----------------------------------------------------------*/

#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);
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

/**
Expand Down Expand Up @@ -643,6 +691,16 @@ static uint32_t prvConnectToServerWithBackoffRetries( const char * pcHostName,
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,
Expand Down
56 changes: 56 additions & 0 deletions demos/sample_azure_iot_pnp/sample_azure_iot_pnp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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

/**
Expand Down Expand Up @@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor question -
This call to atcab_read_serial_number will only succeed if the initialization with the secure element which is currently done here is successful.

Are we sure that we have already initialised esp-tls at this point ( and thus the secure element ) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that we have already initialised esp-tls at this point ( and thus the secure element ) ?

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.

...
I (8987) esp-tls-mbedtls: Initialize the ATECC interface...
I (11917) tls_freertos: (Network connection 0x3ffca760) Connection to global.azure-devices-provisioning.net established.
...

Choose a reason for hiding this comment

The 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

/**
Expand Down Expand Up @@ -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,
Expand Down