Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd validate_key hooks and tests #3780
Conversation
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
4db092b
to
7988a81
|
@gilles-peskine-arm / @ronald-cron-arm / @danh-arm what's the CI complaining about? |
|
One of the errors (looks the main one): In file included from /var/lib/build/library/psa_crypto_driver_wrappers.c:22:
make[2]: *** [library/CMakeFiles/mbedcrypto.dir/psa_crypto_driver_wrappers.c.o] Error 1 In file included from /var/lib/build/library/psa_crypto.c:28:
In file included from /var/lib/build/library/psa_crypto_slot_management.c:28:
library/CMakeFiles/mbedcrypto.dir/build.make:1310: recipe for target 'library/CMakeFiles/mbedcrypto.dir/psa_crypto_slot_management.c.o' failed make[2]: *** [library/CMakeFiles/mbedcrypto.dir/psa_crypto_slot_management.c.o] Error 1 library/CMakeFiles/mbedcrypto.dir/build.make:1238: recipe for target 'library/CMakeFiles/mbedcrypto.dir/psa_crypto.c.o' failed CMakeFiles/Makefile2:400: recipe for target 'library/CMakeFiles/mbedcrypto.dir/all' failed Makefile:138: recipe for target 'all' failed
^^^^test_valgrind: build: Release (clang): command make -> 2^^^^ (skipped because the build failed)
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! |
|
Another one: |
|
@ronald-cron-arm What are the errors now? |
|
Hi, ABI-API checking script is complaining that |
|
Still some CI glitches but otherwise the CI is happy with this PR. Nothing to change on your side regarding CI. |
|
Thanks. Who's going to review? |
|
I will review it. |
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
fccab34
to
6916c40
|
I have a question. Am I correct in thinking that with #3748 we can get rid of the |
Partially. #3748 leaves room for a backwards-compatible setup, where keys that are already stored will have their |
I am on the side of not supporting "backwards-compatible setup" and thus removing |
|
We're about to change the format version from 0 (draft without backward compatibility) to 1. Once we do this, version-0 storage will not be accepted anymore. There is no upgrade path from version 0 to version 1. |
|
ok, thanks Gilles. Thus I would suggest to rebase this PR on top of #3748 and remove the |
| size_t *bits ) | ||
| { | ||
| #if defined(PSA_CRYPTO_ACCELERATOR_DRIVER_PRESENT) | ||
| psa_status_t status = PSA_ERROR_BAD_STATE; |
ronald-cron-arm
Oct 20, 2020
Contributor
I think we prefer PSA_ERROR_CORRUPTION_DETECTED.
I think we prefer PSA_ERROR_CORRUPTION_DETECTED.
stevew817
Oct 26, 2020
Author
Contributor
Done
Done
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Test driver for generating keys. | |||
| * Test driver for generating and verifying keys. | |||
ronald-cron-arm
Oct 20, 2020
Contributor
Maybe better to rename this file to key_management.h?
Maybe better to rename this file to key_management.h?
stevew817
Oct 26, 2020
Author
Contributor
done
done
| @@ -1,6 +1,6 @@ | |||
| /* | |||
| * Test driver for generating keys. | |||
| * Currently only supports generating ECC keys. | |||
| * Test driver for generating and verifying keys. | |||
ronald-cron-arm
Oct 20, 2020
Contributor
Ditto rename this file to key_management.c ?
Ditto rename this file to key_management.c ?
stevew817
Oct 26, 2020
Author
Contributor
done
done
|
|
||
| psa_status_t psa_detect_bit_size_in_slot( psa_key_slot_t *slot ) | ||
| { | ||
| if( slot->attr.bits == 0 ) |
ronald-cron-arm
Oct 20, 2020
Contributor
if( slot->attr.bits != 0 )
return( PSA_SUCCESS );
This will reduce the indentation below.
if( slot->attr.bits != 0 )
return( PSA_SUCCESS );This will reduce the indentation below.
stevew817
Oct 26, 2020
Author
Contributor
done
done
| * \param[in] data Buffer containing the key material to parse and import. | ||
| * \param data_length Size of \p data in bytes. | ||
| * | ||
| * \retval PSA_SUCCESS |
ronald-cron-arm
Oct 20, 2020
Contributor
#PSA_SUCCESS and the same just below.
#PSA_SUCCESS and the same just below.
stevew817
Oct 26, 2020
Author
Contributor
done
done
| psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, | ||
| const uint8_t *data, | ||
| size_t data_length ) | ||
| psa_status_t psa_copy_key_material_into_slot( psa_key_slot_t *slot, |
ronald-cron-arm
Oct 20, 2020
Contributor
Please introduce this function in a separate commit.
Please introduce this function in a separate commit.
stevew817
Oct 26, 2020
Author
Contributor
done
done
I prefer to merge #3748 first, that way no need to review |
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
6916c40
to
6e810fc
|
@ronald-cron-arm applied your feedback and reworked the history. Not yet rebased on #3748. |
|
As we discussed yesterday, we'll move to an |
|
I still think it makes sense to get this in, since there's a major performance gain in not doing the import-export dance on key copy and load-from-storage, and we'll need to branch off for our own release soon. If you can get validate_key redefined into import_key early next week, then I can rework this PR. If not, then I'd kindly ask to review it as-is, and then we'll update it later on to import_key. As a side-note, import sounds a bit like a misnomer for what the function actually does. validate_and_convert (or something in that direction) would be better, no? |
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
6e810fc
to
5b26ffd
|
Rebased on top of #3748 |
There's no need for calling export-and-import when the key is guaranteed to have been stored in export representation. Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
Now that there's a validate_key entry point for drivers, it becomes much more important to separate the import action (where a key needs to be validated) from the load action (where a key has been previously validated, and thus re-validating it would be a waste of time). This also exposes why not storing the 'bits' attribute persistently was a bad idea. The only reason there's a rather large function to detect bit size is because loading from persistent storage requires it. Signed-off-by: Steven Cooreman <[email protected]>
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
5b26ffd
to
3ca2657
|
Rebased on development now that #3748 is merged |
| *key_length = bytes; | ||
| } | ||
|
|
||
| mbedtls_ecp_keypair_free( &ecp ); |
paul-elliott-arm
Oct 27, 2020
Contributor
The original in psa_cyrpto.c had a call to psa_remove_key_data_from_memory() here on failure to copy / write. Is this no longer required here?
The original in psa_cyrpto.c had a call to psa_remove_key_data_from_memory() here on failure to copy / write. Is this no longer required here?
stevew817
Oct 29, 2020
Author
Contributor
Not sure this is required for a test driver, but OK, I can add a buffer zeroisation on failure.
Not sure this is required for a test driver, but OK, I can add a buffer zeroisation on failure.
|
CI failures were ABI checker (expected due to change in ABI) and known CI failures. Kicking it again to see if we can get a better result. |
|
Please omit the changelog entry until validate_key is replaced by import_key. There's one place where I think the code should be simplified. Other than that, LGTM. |
| * Implementation of the validate_key interface for PSA Crypto accelerator | ||
| drivers, as defined in #3695. Contributed in #3780. |
gilles-peskine-arm
Oct 27, 2020
Contributor
Since validate_key won't ever make it into the driver interface specification, I'd prefer not to mention it in a changelog entry.
Since validate_key won't ever make it into the driver interface specification, I'd prefer not to mention it in a changelog entry.
stevew817
Oct 29, 2020
Author
Contributor
So I just drop the changelog entry? Or do you want to rephrase it?
So I just drop the changelog entry? Or do you want to rephrase it?
gilles-peskine-arm
Oct 29, 2020
Contributor
Please drop it for this PR.
Please drop it for this PR.
Signed-off-by: Steven Cooreman <[email protected]>
Storage format has been changed to always store the key's bit size Signed-off-by: Steven Cooreman <[email protected]>
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
702c649
to
11120d1
|
@gilles-peskine-arm and @paul-elliott-arm All your review feedback is now incorporated. |
|
Thankyou for the changes - this looks good to me now |
|
|
||
| /* Key format is not supported by any accelerator, try software fallback | ||
| * if present. */ | ||
| if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) |
gilles-peskine-arm
Oct 29, 2020
Contributor
Please move the if( … ) condition inside the corresponding #if defined(MBEDTLS_…). The code is correct either way, but this way is fragile (it depends on this if-chain running with status already set to PSA_ERROR_NOT_SUPPORTED) and wastes a few bytes of code (if you remove e.g. RSA support, there's no need to keep a call to PSA_KEY_TYPE_IS_RSA).
Please move the if( … ) condition inside the corresponding #if defined(MBEDTLS_…). The code is correct either way, but this way is fragile (it depends on this if-chain running with status already set to PSA_ERROR_NOT_SUPPORTED) and wastes a few bytes of code (if you remove e.g. RSA support, there's no need to keep a call to PSA_KEY_TYPE_IS_RSA).
stevew817
Oct 30, 2020
Author
Contributor
I don't agree with it wasting code bytes (no statement in the conditional is volatile or in a different CU, so if the clause body is empty the compiler will optimise out the check as well), but if this really throws you off I can change it.
I don't agree with it wasting code bytes (no statement in the conditional is volatile or in a different CU, so if the clause body is empty the compiler will optimise out the check as well), but if this really throws you off I can change it.
* zero key buffer on failure * readability improvements * psa_finish_key_creation adjustment after removing import_key_into_slot Signed-off-by: Steven Cooreman <[email protected]>
stevew817
force-pushed the
stevew817:feature/validate_key_in_driver
branch
from
11120d1
to
40120f6
|
@paul-elliott-arm I force-pushed the last change requested by @gilles-peskine-arm . Can you re-approve? |
|
Looks fine to me now. |
|
pr-head didn't run due to an infrastructure problem but pr-merge passed except for API changes in library-internal functions so CI can be considered to have passed. |
gilles-peskine-arm
dismissed
ronald-cron-arm’s
stale review
Paul has taken over reviewing for Ronald and has validated that Ronald's change requests have been addressed in rework.
gilles-peskine-arm
merged commit a455e71
into
ARMmbed:development
Description
This PR sets up the
validate_keyentry point as described in #3695 , and with the clearer distinction between 'import' and 'load' actions, cleans these up in the PSA Crypto Core to provide better performance on load-from-storage, as well as semantically cleaner code.@torstenes for the bit size persistency issue
@gilles-peskine-arm who has defined
validate_key.Status
READY
Requires Backporting
NO
Migrations
NO
Todos
Steps to test or reproduce
Relevant tests added to regression suite.