-
Notifications
You must be signed in to change notification settings - Fork 9
Atmel hardware reference implementation #1
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
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 follow our commit style guide. https://os.mbed.com/docs/latest/reference/workflow.html#guidelines-for-github-pull-requests
Specifically, use e.g. "Add" instead of "Added" and "Prepare" instead of "Prepared".
@@ -0,0 +1,4 @@ | |||
[submodule "psa-crypto"] |
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.
Can we call this mbedtls-psa
instead to match the repo name? Or are you calling it psa-crypto because you anticipate depending on the psa-crypto
repo instead of mbedtls-psa
later to get the header 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.
exactly, all we need is the headers so it will be less of a pain to update this when we'll be able to fetch the headers from the psa-crypto repo.
@@ -0,0 +1,10 @@ | |||
psa-crypto/configs/* |
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.
Where should the .mbedignore
go? Is there a place we could put it in features? I want users to still be able to use their own custom .mbedignore
file without having to edit our own.
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 with the submodule this was not successful and had to be at the root.
@@ -0,0 +1,4 @@ | |||
[submodule "psa-crypto"] | |||
path = psa-crypto |
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.
Should this be under features/atecc608a
instead of at the root?
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 tried this, it's problematic due to the mbed build system (maybe a bug in mbed?).
I tried a bunch of different ways but the wrong mbedtls/config.h
file was getting included even though the mbed_lib.json
had MBEDTLS_CONFIG_FILE
defined properly to point to the correct file. Just for sport I even tried passing it via the linker but again this was not successful.
It's possible to add the submodule to the root of features
if that's better...
features/atecc608a/ATCAConfig.cpp
Outdated
@@ -0,0 +1,97 @@ | |||
/* mbed Microcontroller Library |
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 a funny name. Is this the mbed Microcontroller Library or an ATECC608A driver?
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.
changed to ATECC608A driver
|
||
bool KeyConfig::IsECCKey() | ||
{ | ||
return ( (_register & KEY_CONFIG_P256_ECC_KEY) == KEY_CONFIG_P256_ECC_KEY); |
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 use Mbed OS C coding style and not Mbed TLS coding style.
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.
There are different styles throughout the code here, but I think it's best to wait until we do some refactoring in the driver code.
} | ||
} | ||
|
||
static ATCAError atca_get_key( ATCAKeyID key_id, ATCAKey *&atca_key ) |
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.
Did you mean to write *&actca_key
here?
Is this geting a key slot or getting a key?
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 getting a key pair object for a requested private key slot.
if( ! PSA_ALG_IS_ECDSA( alg ) ) | ||
return( PSA_ERROR_INVALID_ARGUMENT ); | ||
|
||
atca_err = atca_get_key( (ATCAKeyID) key, atca_key ); |
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.
@gilles-peskine-arm How should the PSA key slot translate to the hardware key slot?
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.
That's not defined yet in the standard and is likely to remain implementation-specific, although I think the standard should present at least one sample architecture. For this demo, which doesn't have access control, allocate a range of hardware slots, e.g. 0x80000000 to 0x8000000f. For these slots, the SPM partition needs to maintain an array of key metadata, without the key data.
Note that this may require a bit of restructuring of the key_slot_t
structure, something like putting all the metadata fields in a meta
substructure. But when the software doesn't have access to the key data, it may need to maintain more metadata, for example the key size, and perhaps the public key for a key pair.
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 PR assumes that the device has already been commissioned and it's configuration is locked. We need to think about how to avoid conflicting hardware/software key slots.
atca_err = atca_key->Sign( hash, hash_length, | ||
signature, signature_size, | ||
signature_length ); | ||
delete( atca_key ); |
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.
Really? We have to C++ delete the key we get back? I don't think we really need all this dynamic memory stuff, allocating and deallocating all the time, do we?
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.
yep, heap allocated, see:
* It is callers responsibility to delete the pointer after use. |
This is how it is out of the box, I suspect there will be some significant refactoring to do here if we want it stack based.
atca_err = atca_get_key( (ATCAKeyID) key, atca_key ); | ||
if( ATCA_SUCCESS == atca_err ) | ||
{ | ||
atca_err = atca_key->Sign( hash, hash_length, |
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.
Should we check the key slot policy before we attempt to use the key or should we assume hardware is enforcing that policy?
How do we set a key policy for an ATECC608A slot? We probably need to implement that function later.
Aside from policy, we should also be thinking about lifetime. Are ATECC608A keys always persistent? Can we do ephemeral keys with ATECC608A?
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 software should enforce its own policies, no matter what the hardware can do. Hardware policies are likely to be more limited than what the software can express.
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.
Since all key slot management is done in psa_crypto.c
we will need to implement a separate key management here if such behavior is desired.
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.
See ARMmbed/mbedtls-psa#143. Because of this issue (and others), we are rearchitecting to split key management out to a higher layer. Crypto implementations like the ATECC608A implementation will instead implement the device driver API.
"Updated psa-crypto submodule" shouldn't be necessary. Use the latest psa-crypto/feature-psa submodule from the very first commit that adds the submodule. |
Add psa-crypto as a git submodule in order to use the psa-crypto headers files. Add an mbedignore file in order to ignore the psa-crypto submodule except for the psa-crypto header files. Add an mbed_lib.json file in order to use the default mbedtls config.h file used by mbed-os.
af5a5ce
to
f74f331
Compare
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've rebased in order to address some of the comments raised thus far (mostly commit messages and the most up to date submodule which was needed for the ECDSA macros).
There's quite a lot of refactoring/optimization/styling to do within the driver code but I think this is a fair base point to start working from.
@@ -0,0 +1,4 @@ | |||
[submodule "psa-crypto"] |
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.
exactly, all we need is the headers so it will be less of a pain to update this when we'll be able to fetch the headers from the psa-crypto repo.
@@ -0,0 +1,4 @@ | |||
[submodule "psa-crypto"] | |||
path = psa-crypto |
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 tried this, it's problematic due to the mbed build system (maybe a bug in mbed?).
I tried a bunch of different ways but the wrong mbedtls/config.h
file was getting included even though the mbed_lib.json
had MBEDTLS_CONFIG_FILE
defined properly to point to the correct file. Just for sport I even tried passing it via the linker but again this was not successful.
It's possible to add the submodule to the root of features
if that's better...
@@ -0,0 +1,10 @@ | |||
psa-crypto/configs/* |
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 with the submodule this was not successful and had to be at the root.
|
||
bool KeyConfig::IsECCKey() | ||
{ | ||
return ( (_register & KEY_CONFIG_P256_ECC_KEY) == KEY_CONFIG_P256_ECC_KEY); |
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.
There are different styles throughout the code here, but I think it's best to wait until we do some refactoring in the driver code.
} | ||
} | ||
|
||
static ATCAError atca_get_key( ATCAKeyID key_id, ATCAKey *&atca_key ) |
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 getting a key pair object for a requested private key slot.
if( ! PSA_ALG_IS_ECDSA( alg ) ) | ||
return( PSA_ERROR_INVALID_ARGUMENT ); | ||
|
||
atca_err = atca_get_key( (ATCAKeyID) key, atca_key ); |
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 PR assumes that the device has already been commissioned and it's configuration is locked. We need to think about how to avoid conflicting hardware/software key slots.
atca_err = atca_get_key( (ATCAKeyID) key, atca_key ); | ||
if( ATCA_SUCCESS == atca_err ) | ||
{ | ||
atca_err = atca_key->Sign( hash, hash_length, |
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.
Since all key slot management is done in psa_crypto.c
we will need to implement a separate key management here if such behavior is desired.
atca_err = atca_key->Sign( hash, hash_length, | ||
signature, signature_size, | ||
signature_length ); | ||
delete( atca_key ); |
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.
yep, heap allocated, see:
* It is callers responsibility to delete the pointer after use. |
This is how it is out of the box, I suspect there will be some significant refactoring to do here if we want it stack based.
Add a test log for htrun
Description
This is the hardware reference implementation for the Atmel HW accelerator.
The psa-crypto APIs implemented here are:
psa_asymmetric_sign
psa_asymmetric_verify
The driver code was copied from https://github.com/ARMmbed/mbed-os/tree/feature-opaque-keys/features/atcryptoauth as is with the following exceptions & modifications:
ATCADevice::Verify
- verify response was never read from RX in case the command was executed successfully.ATCAKey::Verify
- parameters in the wrong order.Note: since psa-crypto was added as a git submodule, when cloning please use:
git clone -b feature-atecc608a-impl --recurse-submodules [email protected]:ARMmbed/mbed-os-atecc608a.git
Pull request type