Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

itayzafrir
Copy link

Description

This is the hardware reference implementation for the Atmel HW accelerator.
The psa-crypto APIs implemented here are:

  1. psa_asymmetric_sign
  2. 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:

  • Removed anything related to mbedtls.
  • Removed the commissioning application (this PR assumes the device is already commissioned) .
  • Driver bug fix in ATCADevice::Verify - verify response was never read from RX in case the command was executed successfully.
  • Driver bug fix in 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

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

@itayzafrir itayzafrir requested review from Patater and alekshex July 4, 2018 13:54
@itayzafrir itayzafrir added the needs review The pull request is ready for review. This generally means that it has no known issues. label Jul 4, 2018
Copy link
Contributor

@Patater Patater left a 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"]
Copy link
Contributor

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?

Copy link
Author

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/*
Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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,97 @@
/* mbed Microcontroller Library
Copy link
Contributor

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?

Copy link
Author

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);
Copy link
Contributor

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.

https://os.mbed.com/docs/latest/reference/style.html

Copy link
Author

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 )
Copy link
Contributor

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?

Copy link
Author

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 );
Copy link
Contributor

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?

Copy link

@gilles-peskine-arm gilles-peskine-arm Jul 4, 2018

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.

Copy link
Author

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 );
Copy link
Contributor

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?

Copy link
Author

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,
Copy link
Contributor

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?

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.

Copy link
Author

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.

Copy link
Contributor

@Patater Patater Aug 24, 2018

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.

@Patater
Copy link
Contributor

Patater commented Jul 4, 2018

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

itayzafrir added 3 commits July 5, 2018 14:18
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.
@itayzafrir itayzafrir force-pushed the feature-atecc608a-impl branch from af5a5ce to f74f331 Compare July 5, 2018 11:24
Copy link
Author

@itayzafrir itayzafrir left a 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"]
Copy link
Author

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
Copy link
Author

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/*
Copy link
Author

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);
Copy link
Author

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 )
Copy link
Author

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 );
Copy link
Author

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,
Copy link
Author

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 );
Copy link
Author

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.

@itayzafrir itayzafrir closed this Nov 29, 2018
rajkan01 pushed a commit to rajkan01/mbed-os-atecc608a that referenced this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request is ready for review. This generally means that it has no known issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants