Add hardware signing and key exporting #2
Conversation
|
|
||
| psa_status_t atecc608a_check_config_locked() | ||
| { | ||
| bool config_locked; |
There was a problem hiding this comment.
Should this header include stdbool.h?
| return PSA_ERROR_HARDWARE_FAILURE; | ||
| } | ||
|
|
||
| if (atcab_release() != ATCA_SUCCESS) |
There was a problem hiding this comment.
Do we need to init and release after every operation? I was under the impression we could do init once when initializing the hardware, and then release when we deinit'd the hardware. We may not have driver callbacks for this functionality yet, so we may want to lazy-init the hardware on first access.
There was a problem hiding this comment.
These are the two possible approaches that I can think of. I opted for this approach for two reasons:
- It was written this way when I approached the code (hence I thought that's a design decision);
- It doesn't depend on the fact that someone else initialized the library - every call takes care of its prerequisites;
Let me know if I should change it, I'm fine either way.
| atcab_release(); | ||
| return atecc608a_to_psa_error(ret); | ||
| } | ||
| p_data[0] = 4; |
There was a problem hiding this comment.
https://github.com/ARMmbed/mbed-crypto/blob/0cad4bac30e021199735041d85c9a11c3291366e/include/psa/crypto.h#L605
I can add a comment explaining it.
There was a problem hiding this comment.
Yes, please do add a comment that explains that what you get from atcab_get_pubkey is the concatenated x and y values, and the desired format is 0x04 + x + y. That explains both the 4 here and the p_data+1 above.
| const uint16_t key_id = key_slot; | ||
|
|
||
| /* We can only do ECDSA on SHA-256 */ | ||
| /* PSA_ALG_ECDSA(PSA_ALG_SHA_256) */ |
There was a problem hiding this comment.
This comment (only line 174) is just a note for which algorithm we support. We don't really need it, since the line below shows this.
|
|
||
| /* Signature will be returned here. Format is R and S integers in | ||
| * big-endian format. 64 bytes for P256 curve. */ | ||
|
|
There was a problem hiding this comment.
Suggest to remove newline here
|
|
||
| if ((ret = atcab_get_pubkey(slot, &p_data[1])) != ATCA_SUCCESS) | ||
| { | ||
| atcab_release(); |
There was a problem hiding this comment.
If actab_release fails, that means there's something wrong with the hardware or there's a driver bug. Should we return HARDWARE_FAILURE in that case? Mbed Crypto currently tends to ignore cleanup errors when an error has already happened, but I wonder if that's the right decision. If something is wrong during cleanup, like failing hardware or memory corruption, shouldn't we let the caller know?
| atcab_release(); | ||
| return atecc608a_to_psa_error(ret); | ||
| } | ||
| p_data[0] = 4; |
There was a problem hiding this comment.
Yes, please do add a comment that explains that what you get from atcab_get_pubkey is the concatenated x and y values, and the desired format is 0x04 + x + y. That explains both the 4 here and the p_data+1 above.
|
@Patater, @gilles-peskine-arm - all of your suggestions have been addressed, but unfortunately due to the change of repository visibility (from private to public), my fork has been disconnected and this PR had to be recreated here: #3 |
|
Work continued here: #3. |
Adjust the driver to the new PSA API
ECDSA signing & public key exporting
This PR implements hardware ECDSA signing and public key exporting using ATECC608A or ATECC508A.
Prerequisite - a cryptoauth device (508A or 608A) comissioned, with a locked configuration and a private key generated in slot 0 (warning - configuration locking is irreversible!). An example application doing this with a quite safe config can be seen here.
This PR is a prerequisite for ARMmbed/mbed-os-example-atecc608a#2.