-
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
Conversation
…ks correctly for device certificate and registration ID generated at runtime using added functions
} | ||
|
||
ATCA_STATUS s; | ||
s = atcab_read_serial_number(sernum); |
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.
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 ) ?
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.
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.
...
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.
Okay, thanks for the explanation. I think this is fine.
@@ -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 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 ?
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.
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.
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.
Okay, sounds good!
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.
@AdityaHPatwardhan Added the comment.
…ATECC608-Trust&GO device
Thank you @sckulkarni246 and @AdityaHPatwardhan for the contribution to add support for ATECC608! Here are some overall comments and questions:
|
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.
Left a few comments.
.gitmodules
Outdated
@@ -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"] |
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
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).
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
- [Simplest] Make esp-cryptoauthlib a component of the ESP-IDF itself rather than for one of the IDF examples only... this removes the need to download it into the Azure IoT middleware repo and may even reduce compatibility issues. I see this configuration happens here.
https://github.com/espressif/esp-idf/blob/master/.gitmodules#:~:text=%5Bsubmodule%20%22examples/peripherals,../../espressif/esp%2Dcryptoauthlib.git - [Slightly complicated] Trigger a download of the esp-cryptoauthlib if someone selects the secure element option inside the esp-tls menuconfig. A good location for this can be this - https://github.com/espressif/esp-idf/blob/v4.4/components/esp-tls/CMakeLists.txt#:~:text=if(CONFIG_ESP_TLS_USE_SE),endif()
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.
esp_transport_ssl_use_secure_element( pNetworkContext->xTransport ); | ||
|
||
#ifndef CONFIG_ATECC608A_TNG | ||
/* This is either a TrustFLEX or a TrustCUSTOM chip - generate (using atcacert_* API) or plug in |
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.
This is unclear. I'm not familiar with TrustFLEX vs TrustCUSTOM (I assume it's a discussion between using the factory provisioned ATECC certificates vs custom ones?)
Please clarify or add code on how one would add their certificate.
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 assume it's a discussion between using the factory provisioned ATECC certificates vs custom ones?)
You are correct - it is indeed about that.
Please clarify or add code on how one would add their certificate.
Let me work on this and commit. Also, I noticed an error in my comment. The parts that need no action are TrustFLEX and TrustCUSTOM. Thank you for bringing this up.
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.
@@ -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 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.
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 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 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.
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 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.
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.
The design I'm thinking of is to either:
- Use the #defines that we already have
- Use a common interface to extract information such as registrationID/deviceID, etc (we could, in the future, add hub name, global endpoint, etc)
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.
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
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.
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.
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.
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( ®istration_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?
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.
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
… TrustCUSTOM v/s other variants - added code that will plug in the provided custom device cert to the network context. No code needed for other variants - esp-tls handles device cert attachment internally
@CIPop Below, I attempt to respond to some of your comments/questions posted above.
@AdityaHPatwardhan I remember Amey mentioning ESP32 DevkitC supports the WROOM32-SE module. Do you know which kits can be added to the below list?
I found this resource on esp-cryptoauthlib repo. Is this close to what you would need?
For pre-provisioned variants, that CA would be the Microchip signer. The Microchip signer cert is one of the outputs of the esp_cryptoauth_utility shared above.
Yes. More specifically, the device cert CN would be snXXXXXXXXXXXXXXXXXX where XX...XX is the 9-byte serial number of the chip.
That is theoretically correct. But, I do not know if the utility linked above generates a CSR with the device serial number as the CN or uses a fixed value. @AdityaHPatwardhan I seek your help for getting this info...thanks!
Yes, this compilation would happen once the configuration has been correctly done to enable the ATECC support.
There is an option to toggle the usage of secure element on or off - |
@CIPop Thanks for your review comments. @sckulkarni246 has already anserwed most of them. Please find additional explanation below.
I think we can add ESP32-WROOM32-SE to that list. The module has microchips
I think this document would surely provide the information on how to generate the device certificate for
Hi, Thanks for raising this point. I think we might have to change the code for obtaining the registration id slightly for that. The reason being that in case of |
@AdityaHPatwardhan Just for clarity, is the below assumption correct? I will make the changes accordingly.
Where XXXXXXXXX is the device serial number hex string. |
Yes, this is right. |
… the beginning of the CN - this change is now done in each of the implementations of registration ID preparation functions
Thank you for the documentation pointers. We'll need to add a readme document to point to the official Espressif docs. Once I have everything tested, I can help refactor the code to maintain all sample code separate from any ESP32 or ATECC changes (we'll need a separate compilation unit for that).
We have confirmed with Espressif that the above module is not recommended for new designs (NRND) and should not be referenced. Instead I have investigated other possibilities, ideally without the need for a breadboard/hardware changes and found a likely candidate: I just got the ATECC608 board and will test with an ESP32-WROOM-DevKit and connector wires. I'll need some time to set everything up and give this a quick try. |
Hi @CIPop - Just want to check in... were you able to use the software to connect to the Azure DPS? |
@sckulkarni246 , so far this isn't working for me. I'm still investigating. I'm seeing issues with the driver and my ATECC:
Looking at the I2C driver, I see that the pins I wanted to use are considered to be on a secondary bus and I couldn't find any documentation how that is configured yet (I wanted to use SDA pin 22 and SCL pin 20). The ATECC608 that I have is stating the address is 0x60 but, looking at the driver, I see different addresses being used for different purposes during initialization: I'm using an ESP-WROVER-KIT with JTAG debugging and I will be attaching a logic analyzer to see if this is a hardware issue. |
@CIPop I just checked, I think there seems to be some discrepancy in updating the latest config option in esp-cryptoauthlib and esp-idf about selecting I2C slave address. |
Hi @CIPop, I think there might be some problem with the I2C PIN numbers. From where did you refer pin 16 and pin 17 as the I2C SDA and SCL pins ?. I think those pin numbers are for ESP32-WROOM32-SE but AFAIK for esp32 the numbers were SDA=21 ans SCL=22. Can you please try those, Thanks |
@AdityaHPatwardhan those pins were the defaults. I originally tried with 21, 22. I can't find any documentation where I2C is tied to specific pins so I'm assuming this is a software I2C implementation and any pins can be used? I've tried 21,22 and 25,26 and, in both cases the I2C signal as recorded by my logic analyzer doesn't look correct. There is still something wrong with the I2C communication:
Updated above: Regardless of the configured address, the code is always trying to send data on address 0. |
I think we need to investigate more issues for the cryptoauthlibrary:
I have confirmed this using 2 different logic analyzers at very high sampling rates (>25MHz). I used both DSView and Sigrock (with 2 different USB analyzers). Note how the reading is adding an extra 0 at the beginning and reinterpreting the last bit:
I tried with I2C was configured at either 100kHz (default) or 400kHz (below images): I did find this issue mentioning pull-up resistors espressif/arduino-esp32#741. @AdityaHPatwardhan I noticed that the HAL ( |
@CIPop Thanks for your analysis. I shall also investigate from my side. I would just like to note that I am using ATECC608B of TnG type for atecc608_ecdsa example in esp-idf and it is succeeding at my end.
|
Thank you @AdityaHPatwardhan. I have narrowed this down I2C timing (SCL). I have tried turning on pull-up resistors, tried 2 separate ESP boards (different types), 100kHz/300kHz, ESP-IDF versions 4.4.1 (latest) and 4.3. |
Hi @CIPop ,
You have mentioned above about the address 0x00 being sent out. I want to clarify that it may be happening because the cryptoauthlib sends out a wake token to the ATECC608 chip. The wake token is implemented using I2C at 100 kHz and sending out 0x00 as the device address. This is followed by the actual command for the device. So, the 0x00 should not be a problem. About I2C addressesIf the 7-bit address of the device is 0x60 (I2C write should send 0xC0), it means it is a TrustCUSTOM device (blank). The device address for Trust&GO is 0x35 (I2C write should send 0x6A) and TrustFLEX device is 0x36 (I2C write should send 0x6C). A quick question - what is the I2C speed you are working at? If not 100 kHz, can you try switching the I2C frequency to 100 kHz and try the operations that don't work? |
@AdityaHPatwardhan, here's the SDKConfig (redacted to remove credentials): sdkconfig.txt Also, this is the UX config related to ATECC. The DT100104 board has all switches off except either 5 or 6 (first two ATECC608B TrustCUSTOM): For the case that worked (TnG), the only changes were the chip type and the I2C address (0x6A). The DT100104 was configured with switch 1 on (rest off). |
Hi @CIPop Thanks for sharing the configurations. For TrustnGo/TFlex case the certificate have already been stored in the ATECC608 by Microchip. In that case they can be read from the device by using microchip provided API. |
@CIPop @AdityaHPatwardhan Gentlemen, unfortunately I am down with a rather serious health thing. |
I am very sorry to hear that! Take care @sckulkarni246 ! I will also be on vacation - we can resume working on this in about 2 weeks.
@AdityaHPatwardhan I have used the defaults within esp-cryptoauthlib to generate the certificate and generation was successful, but I did have to change the crypto library version in the Python tool to get it working:
I have sent you the certificates and the log output via email. Please compare with your configuration and let me know if there is anything wrong on your end. OpenSSL is able to parse both: Trust and Go
TrustCUSTOM
|
@sckulkarni246 No issues, Please take care ! |
@sckulkarni246 , @AdityaHPatwardhan any progress on this?
|
Hi @CIPop - I am working on it. I shall push the usage doc and change the ESP-Cryptoauthlib to a CMake Fetch module soon and push both of these. |
1. Removes esp-cryptoauthlib as a git submodule 2. Adds esp-cryptoauthlib as a CMake FetchComponent for the ESPRESSIF esp32/ target (aziotkit support will be added later) 3. Adds a README for ATECC608 support enablement 4. Adds images required by the README referred to in 3 above
@CIPop This is now done. I have tested this by cloning separately into a new folder and following the steps as mentioned in the newly added README. |
1. Defines a new macro for enabling usage of HSM 2. Uses the new macro to dynamically generate registration ID for Azure DPS 3. For ESPRESSIF: Separates implementation for registration ID generation depending on the ATECC608 Trust variant selected 4. Removes any device specific code from Azure common code
Hi @CIPop , @AdityaHPatwardhan - I am summarising here the various feedbacks addressed by the last two commits:
Do let me know your comments about the PR in its current state and how we can take it to the next stage. |
@CIPop Bump! Any comments or feedback on the last 2 commits? These address some of the points mentioned in the review comments. Regards, |
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.
Thank you @sckulkarni246 !
I'm going to try this out on my board before merging.
**If build fails**, ensure that above steps are followed correctly. Before re-attempting the process, | ||
- delete the build directory using `rm -rf build/` | ||
- delete the sdkconfig file by using `rm sdkconfig`. | ||
|
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.
Nit: we need more information and steps how to provision the ATECC as well as how to configure the application. I think we can move forward with this PR and add more documentation later.
ulStatus = getRegistrationId( ®istration_id ); | ||
configASSERT( ulStatus == 0); | ||
#undef democonfigREGISTRATION_ID | ||
#define democonfigREGISTRATION_ID registration_id |
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.
Thank you for maintaining the old behavior: very elegant solution!
Hi @CIPop - thank you for approving the changes and your review comments. It was fun working with you and @AdityaHPatwardhan ! |
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.
Sorry for the late review, I have reviewed the MR again.
The code now looks very good to me!
Thanks @sckulkarni246 for adding the support of using ATECC608 in Azure.
Thanks @CIPop for your contribution and additions.
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.
sorry for the late review but otherwise looks good to me! tried the compilation locally
demos/projects/ESPRESSIF/esp32/components/sample-azure-iot/transport_tls_esp32.c
Outdated
Show resolved
Hide resolved
demos/projects/ESPRESSIF/esp32/components/sample-azure-iot/transport_tls_esp32.c
Outdated
Show resolved
Hide resolved
demos/projects/ESPRESSIF/esp32/components/sample-azure-iot/transport_tls_esp32.c
Outdated
Show resolved
Hide resolved
Co-authored-by: Dane Walton <[email protected]>
Thank you @sckulkarni246 and @AdityaHPatwardhan for the contribution! |
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
What to Check
Verify that once the Wi-Fi connection is successful, the logs now mention that ATECC is used for the TLS handshake and that the registration happens successfully
Other Information