Develop support for Atmel crypto engine ATCAECC508A #6104
Conversation
|
@gilles-peskine-arm I havn't finished the documentation. But you can review the code while I finish documentation. |
| return err; | ||
| } | ||
|
|
||
| ATCAError ATCADevice::GenPubKey(ATCAKeyID keyId, uint8_t *pk, |
There was a problem hiding this comment.
The function name (“generate public key”) makes no sense. Apparently this function retrieves the public key from a slot that contains a key pair. So it should be called something like GetPubKey or ExportPublicKey.
There was a problem hiding this comment.
This function indeed generates the Public key from an existing Private in the HW. Please see section 9.7 Point 2 in the datasheet
There was a problem hiding this comment.
Ok. “Generate” has a very specific meaning in cryptography: it means generate at random, not construct from existing data. So please keep the function name if it comes from the datasheet, but use a different word in the explanation. The public key is calculated or reconstructed from the private key, or you could say that it's exported from the device. It's the device's own business whether it just copies a bunch of bits or makes a mathematical calculation to provide the public key, although you may want to document this if it makes an important performance difference.
There was a problem hiding this comment.
Changing it to
124 /** Calculate Public key from corresponding Private Key Id in the device
125 * and output it.
features/atcryptoauth/ATCADevice.cpp
Outdated
| * Key Index (0-7) | ||
| * pk_len == 64 | ||
| */ | ||
| ATCAError ATCADevice::GenPrivateKey(ATCAKeyID keyId, uint8_t *pk, |
There was a problem hiding this comment.
The output buffer pk contains the public key, right?
| return err; | ||
| } | ||
|
|
||
| ATCAError ATCADevice::Nonce(const uint8_t * message, size_t len) |
There was a problem hiding this comment.
Please give this function a better name. It sets a nonce, for what?
There was a problem hiding this comment.
These function names are picked to match commands they run on the HW. See section 9.12 for Nonce command in the datasheet
There was a problem hiding this comment.
Ok, thanks. I've looked at the datasheet now. So keep the name, but please do explain what the commands do in the header file.
features/atcryptoauth/ATCADevice.cpp
Outdated
| cmd[1] = 0x03; // Passthrough mode to load input message in TempKey | ||
| cmd[2] = 0x00; // Param2 is 0 for Passthrough mode. | ||
| cmd[3] = 0x00; | ||
| for (size_t i = 0; i < len; i++) |
There was a problem hiding this comment.
memcpy(cmd + 4, message, len)?
features/atcryptoauth/ATCADevice.cpp
Outdated
| return ATCA_ERR_NO_ERROR; | ||
| } | ||
|
|
||
| ATCAError ATCADevice::ResetWatchdog() |
There was a problem hiding this comment.
I don't understand the logic of when the watchdog needs to be reset. Please include that in the upcoming documentation.
features/atcryptoauth/atca_app.cpp
Outdated
| { | ||
| printf("\r\nRunning ssl client...\r\n"); | ||
| run_ssl_client(); | ||
| printf("\r\nNote: For SSL client sample to work sample_client.c\r\n" |
There was a problem hiding this comment.
Why print this note after running the client (which may or may not have worked)?
There was a problem hiding this comment.
Actually this is part of the documentation hence I will remove it.
|
|
||
| static TCPSocket * _tcpsocket = NULL; | ||
|
|
||
| int mbedtls_net_init() |
There was a problem hiding this comment.
How come these functions aren't part of the Mbed TLS integration in Mbed OS?
There was a problem hiding this comment.
Can be. But mbed-os offers alternatives to EthernetInterface. Leaving it on developer's choice.
| return MBEDTLS_ERR_SSL_WANT_WRITE; | ||
| }else if(size < 0){ | ||
| mbedtls_printf("Socket send error %d\n", size); | ||
| return -1; |
There was a problem hiding this comment.
This should be an MBEDTLS_ERR_SSL_XXX error code, translated from the error returned by TCPSocket::send.
Same remark in mbedtls_net_recv.
|
|
||
| mbedtls_printf( " ok\r\n" ); | ||
|
|
||
| /* OPTIONAL is not optimal for security, |
| */ | ||
| mbedtls_printf( " . Verifying peer X.509 certificate ..." ); | ||
|
|
||
| if( ( flags = mbedtls_ssl_get_verify_result( &ssl ) ) != 0 ) |
There was a problem hiding this comment.
Since you used VERIFY_REQUIRED above, this call is irrelevant: you already know the verification succeeded. Remove the whole “Verify the server certificate” section.
|
ATCAECC508A datasheet |
|
ARM Internal Ref: IOTSSL-2102 |
|
@0xc0170 can you please add @AndrzejKurek as reviewer. |
features/atcryptoauth/atca_app.cpp
Outdated
|
|
||
| #if MBED_CONF_ATCAECC_APP_ENABLE | ||
|
|
||
| /** ATCAECC508A sample config zone data. Required for calculating CRC while |
There was a problem hiding this comment.
Please explain where this data is from. Is that a binary blob from a datasheet or from sample code? Or did you write it yourself?
features/atcryptoauth/atca_app.cpp
Outdated
| /** ATCAECC508A sample config zone data. Required for calculating CRC while | ||
| * locking the config zone. | ||
| */ | ||
| uint8_t atcaecc508a_sample_config[] = { |
There was a problem hiding this comment.
If this is only required to calculate the CRC, it would be better to embed only the CRC here (save code size).
This requires changing the LockCommand interface and can wait until after the demo.
features/atcryptoauth/ATCA.cpp
Outdated
| :plt(I2C_SDA, I2C_SCL, | ||
| ATCA_ECC_I2C_FREQUENCY, | ||
| ATCA_ECC_508A_I2C_ADDR), | ||
| device(plt) | ||
| { | ||
| } | ||
|
|
||
| ATCA * ATCA::GetInstance() | ||
| ATCADevice * ATCAFactory::GetDevice( ATCAError & err ) |
There was a problem hiding this comment.
This is one of several methods that return both an error code and some data. They return the data as a return value and the error code through a reference argument. Is this a common pattern in Mbed OS? The opposite pattern (return an error code, use reference arguments for the data) is more common in general.
| @@ -17,9 +17,8 @@ | |||
| #ifndef ATCAECCCONSTANTS_H | |||
| #define ATCAECCCONSTANTS_H | |||
|
|
|||
| /* Datasheet defined constants */ | |||
| /** Datasheet defined constants */ | |||
There was a problem hiding this comment.
This doesn't document the next definition (it documents the next bunch of definitions) so it either it should not be a Doxygen comment, or it should be a Doxygen section start and each definition inside the section should be documented.
features/atcryptoauth/ATCAKey.h
Outdated
| * | ||
| * @param dev Device driver reference. | ||
| * @param keyId Key Id/Slot number in device data zone. | ||
| * @param pk Public key buffer with X & Y concatenated. |
There was a problem hiding this comment.
Please clarify the description. I don't even know whether pk is an input or an output or what it's used for. The public key is not generated, it's exported.
| * | ||
| * Currently interface is only defined for sign and verify functions. | ||
| */ | ||
| class CryptoEngineInterface |
There was a problem hiding this comment.
In the Mbed TLS opaque key interface, there are more mandatory methods, I guess they should all be reflected here: get_signature_size (when sign is present), get_bitlen, can_do. (Plus alloc or a setup and free, which I guess are implemented here as a constructor and a destructor which don't need to be declared explicitly.)
There was a problem hiding this comment.
I suppose these suggested methods should go in class ATCAKey or in an abstraction of it.
This interface is to the driver that can provide different keys. I will keep it for rework later as I have to think it through.
features/atcryptoauth/README.md
Outdated
| - Public keys are not stored in the device but generated whenever required. | ||
|
|
||
| ## ATCAECC508A configuration | ||
| The device configuration should be **locked** before use. To achieve demo requirements it is commissioned with a minimilistic configuration. The commissioning can be done using the sample commissioning app contained in the driver code. This app commissions following configuration in the device: |
There was a problem hiding this comment.
Should or must be locked?
Explain what “locked” means.
minimilistic → minimalistic
“it is commissioned”: when? What you're describing is actually what the demo expects, so: “it should be commissioned”
features/atcryptoauth/README.md
Outdated
| ``` | ||
|
|
||
| Modified ```cert_write.exe``` takes parameters ```subject_key``` and ```issuer_key``` formatted in a special way to identify HW keys. | ||
| The format is ```remote0COM18```. Here: |
There was a problem hiding this comment.
Please put a separator between the parts, e.g. remote:0:COM18 or ///opaque_pk/ATCA/0:COM18
| printf ("atca_sign_func called \r\n"); | ||
|
|
||
| if ( md_alg != MBEDTLS_MD_SHA256 ) | ||
| return -1; |
There was a problem hiding this comment.
@gilles-peskine-arm would MBEDTLS_ERR_MD_FEATURE_UNAVAILABLE be a good error here.
There was a problem hiding this comment.
Yes, that's what FEATURE_UNAVAILABLE is for.
|
@gilles-peskine-arm incorporated your comments. |
features/atcryptoauth/ATCADevice.h
Outdated
| /** Rx buffer */ | ||
| uint8_t rx_buf[ATCA_ECC_MAX_RESP_LEN]; | ||
| public: | ||
| /** Instantiate ATCAECC508A device driver class with paltform reference. |
features/atcryptoauth/ATCADevice.h
Outdated
| */ | ||
| ATCAError Init(); | ||
|
|
||
| /** Configure keyID (Slot) as a ECC Private key. |
features/atcryptoauth/ATCADevice.h
Outdated
| */ | ||
| ATCAError ConfigPrivKey(ATCAKeyID keyId); | ||
|
|
||
| /** Configure keyID (Slot) as a ECC Public key. |
features/atcryptoauth/ATCADevice.h
Outdated
| ATCAError ConfigCertStore(); | ||
|
|
||
| /** Lock configuration zone. Configuration zone locking is required | ||
| * for using cprypto functions of the device. |
features/atcryptoauth/ATCADevice.h
Outdated
| * | ||
| * @param keyId Key Id of Private slot. | ||
| * @param pk Public key output buffer. | ||
| * @param pk_buf_len Public key output buffer length. |
| ATCAError MbedPlatform::Read(uint8_t * buf, size_t len) | ||
| { | ||
| int status = i2c->read(addr, (char *)buf, len); | ||
| //Dump(buf, len, false); |
|
|
||
| ATCAError MbedPlatform::Write(uint8_t * buf, size_t len) | ||
| { | ||
| //Dump(buf, len, true); |
| return (status != 0)?ATCA_ERR_I2C_WRITE_ERROR:ATCA_ERR_NO_ERROR; | ||
| } | ||
|
|
||
| void MbedPlatform::Dump(uint8_t * buf, size_t len, bool tx) |
features/atcryptoauth/MbedPlatform.h
Outdated
| #include "mbed.h" | ||
| #include "ATCAPlatformInterface.h" | ||
|
|
||
| #define DEV_DELAY_TPU_MS (100 + 10) |
There was a problem hiding this comment.
Please comment on where do these values come from.
features/atcryptoauth/MbedPlatform.h
Outdated
|
|
||
| /** Deinit I2C. Particularly useful when ATCAECC508A watchdog timer is to be | ||
| * reset. That requires pulling SDA low. Hence requiring a re-init of I2C | ||
| * afterwords. |
There was a problem hiding this comment.
afterwords -> afterwards
features/atcryptoauth/ATCADevice.h
Outdated
| * boundary is not allowed. | ||
| * | ||
| * @param address Byte address in data zone. Should be 4 byte aligned. | ||
| * @param len input buffer length. |
There was a problem hiding this comment.
Mention ≤4 in the parameter description, and state the name of the buffer (data) explicitly.
… atecc508a_se_dev
|
/morph build |
Build : FAILUREBuild number : 1251 |
|
/morph build |
Build : SUCCESSBuild number : 1253 Triggering tests/morph test |
|
@0xc0170 |
I'll find out about that ci-generic, did not publish the correct status, will be fixed |
| #include "mbedtls/net_sockets.h" | ||
|
|
||
| /* Mbed OS includes */ | ||
| #include "mbed.h" |
There was a problem hiding this comment.
include what is required instead, we should not include mbed.h here
| const char *ip_addr = eth_iface.get_ip_address(); | ||
| if (ip_addr) | ||
| mbedtls_printf("Client IP Address is %s\r\n", ip_addr); | ||
| else |
There was a problem hiding this comment.
I assume this is mbed OS file, thus should follow the style . Same of the rest of files in features/atcryptoauth
| * | ||
| * @param us Delay in micro seconds | ||
| */ | ||
| virtual void SDALow(uint32_t us); |
There was a problem hiding this comment.
methods are lower case - sda_low
|
|
||
| /** Mbed implementation of abstract Platform interface. | ||
| */ | ||
| class MbedPlatform : public ATCAPlatformInterface { |
There was a problem hiding this comment.
why this class has generic name MbedPlatform ? This does not look a generic platform class that can be used outside of this feature?
| #ifndef MBEDPLATFORM_H | ||
| #define MBEDPLATFORM_H | ||
|
|
||
| #include "mbed.h" |
There was a problem hiding this comment.
replace with headers that are needed here
| class MbedPlatform : public ATCAPlatformInterface { | ||
| private: | ||
| /** I2C pin names */ | ||
| PinName sda; |
There was a problem hiding this comment.
private members starts with _
| * | ||
| * @return Error code from enum ATCAError. | ||
| */ | ||
| virtual ATCAError Init(); |
There was a problem hiding this comment.
why do we need init/deinit, not the job done in constructor/destructor?
|
|
||
| ATCAError MbedPlatform::Write(uint8_t * buf, size_t len) | ||
| { | ||
| //Dump(buf, len, true); |
| Reset(); | ||
| } | ||
|
|
||
| void Reset(){ _register = 0; } |
There was a problem hiding this comment.
missing any documentation for methods
Test : SUCCESSBuild number : 1050 |
Exporter Build : SUCCESSBuild number : 917 |
|
Approved, not blocking progress on this feature on its branch (styling changes mostly requested, will be addressed as a new pull request to the branch). |
Description
Driver for ATCAECC508A Atmel crypto engine. Datasheet
Status
IN DEVELOPMENT
Sneak Peak state. Do not review. This PR is suppose to target a new feature branch 'feature-opaque-keys'. Hence the CI builds here will fail.
Waiting for 'feature-opaque-keys' branch be created @0xc0170