Support volatile keys in external SE #3382

stevew817 · 2020-06-01T16:29:00Z

Description

Implemented changes requested in #3288 to support volatile keys located in Secure Element drivers, and to not have the PSA core store information about those volatile keys persistently.

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

Todos

Tests
Documentation

Steps to test or reproduce

Tested through the updated SE HAL test suite.

danh-arm · 2020-06-02T08:22:09Z

Thanks @stevew817 for the PR. This doesn't build in some configs, e.g.


******************************************************************

* build_crypto_full: build: make, crypto only, full config 

* Mon Jun  1 16:51:26 UTC 2020

******************************************************************

  CC    test_suite_gcm.aes128_en.c

suites/test_suite_psa_crypto_se_driver_hal.function: In function 'test_key_creation_import_export':

suites/test_suite_psa_crypto_se_driver_hal.function:898:54: error: expected statement before ')' token

         if( ! check_no_persistent_data( location ) ) )

                                                      ^

suites/main_test.function: In function 'get_expression':

suites/main_test.function:136:30: error: 'TEST_SE_VOLATILE_LIFETIME' undeclared (first use in this function)

  *                  function parameters.

                              ^

suites/main_test.function:136:30: note: each undeclared identifier is reported only once for each function it appears in

stevew817 · 2020-06-02T08:28:59Z

Interesting. I'm running with tests/scripts/basic-in-docker.sh, which does not throw this error.

Is this script somehow only testing a previous version, and not updating with the latest in the working directory? Is there some script that needs to be run before it to get it to regenerate the output of the .function and .data files?

gilles-peskine-arm · 2020-06-02T09:00:16Z

basic-in-docker.sh only runs basic-build-test.sh which builds the library in one configuration and runs tests. You need all-in-docker.sh which runs tests/scripts/all.sh. Compared with all-in-docker.sh, the CI additionally runs some Windows and Arm Compiler jobs.

You can pass component names (tests/scripts/all.sh --list-components) to all.sh or all-in-docker.sh to run one or more component without spending the hours of the whole script.

gilles-peskine-arm

Overall this mostly looks good, but a few areas need work.

The CI results aren't very useful right now because builds without SE support break on p_drv being unused in psa_start_key_creation.

tests/suites/test_suite_psa_crypto_se_driver_hal.function

library/psa_crypto.c

gilles-peskine-arm · 2020-06-02T18:59:57Z

library/psa_crypto.c


-    if( attributes->core.lifetime != PSA_KEY_LIFETIME_VOLATILE )
+    /* Check there is a proper handler for this lifetime */
+    if ( PSA_KEY_LIFETIME_GET_LOCATION( lifetime )


The code for persistent/volatile and local/external should be mostly orthogonal. As it stands, some validation is missing on persistent SE keys.

If MBEDTLS_PSA_CRYPTO_SE_C is enabled and MBEDTLS_PSA_CRYPTO_STORAGE_C is disabled, a persistent external key should be rejected. This configuration is currently rejected in check_config.h, but only because there is currently no support for volatile external keys, which this PR changes.

To keep this PR small, it's ok to keep the restriction in check_config.h and do the following later:

Remove the restriction in check_config.h.

In all.sh, no longer systematically disable MBEDTLS_PSA_CRYPTO_SE_C when MBEDTLS_PSA_CRYPTO_STORAGE_C is disabled.

Fix any failing test.

But please fix the existing bug that the key id isn't validated for keys in a secure element and add a non-regression test.

Is there any intent to support persistent SE-stored keys without having to require PSA Crypto Storage?

No, because the metadata is always in internal storage. Due to the interface design (the application only remembers the key id, not its lifetime), it's difficult to have metadata spread over multiple storage areas.

library/psa_crypto.c

gilles-peskine-arm · 2020-06-03T11:52:14Z

Sorry I forgot to mention this earlier, but for future reference, please don't mark GitHub conversations as resolved when you're the author of the code: that's for the commenter to do when they've decided that the comment was resolved satisfactorily.

In Mbed TLS, we have a convention for the coder use the 🚀 emoji to indicate “I've handled this comment in my local copy”, if you want to keep track of which comments you've handled.

stevew817 · 2020-06-03T13:32:23Z

@gilles-peskine-arm Do you have insight into why those two CI jobs are still failing?

gilles-peskine-arm · 2020-06-03T13:42:25Z

The remaining CI failure is from the ABI compatibility checker. It says that the symbol psa_validate_persistent_key_parameters has disappeared. Since this is an internal library function, this is not a change of the ABI and no update to the soversion is needed.

So that's a pass from CI. I'll review shortly.

gilles-peskine-arm

I'm happy with the library code and with the tests. The documentation needs a little improvement.

The commit history is hard to follow, so I'm requesting a rewrite of the history. The commits “Update PSA core to not persist SE keys declared volatile” and “Fixups after review” do too many things. Please break them up. At this point, you might as well remove the back-and-forth between these two commits. In general, please don't do mix refactoring and adding new things in the same commit.

And please use more descriptive commit messages. For example, “Enable figuring out number of cores when running on OS X” is a good commit message: it explains exactly what's going on. “Fix error from #3302” is not so good; it's acceptable because the diff is very simple, but “Fix typo in a macro that isn't used yet” would be more informative. “Update PSA core to not persist SE keys declared volatile” is not bad in itself but the commit does too much which isn't described in the commit message. “Simplify key checking” lacks explanations and “Fixups after review” is not informative at all.

Please add a changelog entry in ChangeLog.d.

To facilitate review, please save a copy of the current state in your fork (I use the naming convention original-branch-name-1, original-branch-name-2, etc. for successive versions) before force-pushing. Please avoid combining new content with a force-push that rewrites the history. Also, please don't rebase on top of the current target branch if there's no strong reason for it (like a force push). The reason for these requests is to make the force-push diff easy to review (in particular, if you're only restructuring commits or rewriting commit messages, the force-push diff should be empty).

library/psa_crypto_slot_management.c

library/psa_crypto_slot_management.h

tests/suites/test_suite_psa_crypto_se_driver_hal.function

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

gilles-peskine-arm

I'm happy with the code and commit structure, thanks.

But CI is still unhappy.

stevew817 · 2020-06-15T11:10:18Z

@gilles-peskine-arm I've made the CI happy (at least the parts that are visible to me).

ronald-cron-arm

There are a few minor things and a few things I am not sure about.

ronald-cron-arm · 2020-06-16T07:19:58Z

tests/scripts/docker_env.sh

    DOCKER="sudo docker"
 fi

+# Figure out the number of processors available


This commit is not related to the subject of this PR thus it would be better to put it in another PR. This is not a blocker though. If you are ok/have time to do it great, otherwise it's not a big deal.

A practical reason not to “sneak in” unrelated commits is when these unrelated commits need to be backported. But this script doesn't exist in the LTS branches, so it's ok.

ronald-cron-arm · 2020-06-16T07:48:44Z

tests/suites/test_suite_psa_crypto_se_driver_hal.data


 Key registration: smoke test
-register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:PSA_SUCCESS
+register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:1:1:PSA_SUCCESS


This commit message is not rendered correctly by GitHub (too long), could you please rewrite it?

ronald-cron-arm · 2020-06-16T07:51:58Z

library/psa_crypto.c

    psa_se_drv_table_entry_t **p_drv )
 {
-    psa_status_t status;
+    psa_status_t status = PSA_ERROR_INVALID_ARGUMENT;


Ditto, the commit title is too long.

ronald-cron-arm · 2020-06-16T09:05:33Z

library/psa_crypto_slot_management.c

-    psa_key_file_id_t id,
-    psa_se_drv_table_entry_t **p_drv,
-    int creating )
+psa_status_t psa_validate_key_location( const psa_key_attributes_t *attributes,


What about to pass only the lifetime and not all attributes? The idea would be to pass only what is needed. I can see that psa_validate_key_policy() is designed that way.

ronald-cron-arm · 2020-06-16T09:13:34Z

library/psa_crypto_slot_management.h

 }

-/** Test whether the given parameters are acceptable for a persistent key.
+/** Validate that a key's attributes point to a known location.


What about just "Validate key's location" ?

Because users will wonder what exactly the validation entails?

ronald-cron-arm · 2020-06-16T09:34:22Z

library/psa_crypto_slot_management.c

-    if( ! psa_is_key_id_valid( id, ! creating ) )
-        return( PSA_ERROR_INVALID_ARGUMENT );
-    return( PSA_SUCCESS );
+psa_status_t psa_validate_key_persistence( const psa_key_attributes_t *attributes )


With such a name, I would expect the function to validate the persistence part of the lifetime (LSB byte) but in fact it rather validates the key identifier thus this is a bit confusing to me.
What about for this function to just test the validity of the key's persistence? It should probably be moved to psa_crypto.c as not related to SE.
Then, I would check the key identifier by calling psa_is_key_id_valid() directly from psa_validate_key_attributes(). I don't see why psa_is_key_id_valid() is in this C file and not in psa_crypto.c as it does not seem to be related to SE. It could probably be renamed to psa_validate_key_id() for naming alignement.

That's an unfortunate clash of terminology, with “persistence” referring both to the general idea of persisting a key in storage (which involves both the id attribute and the lifetime attribute), and to the persistence level in particular which is encoded in the lifetime.

I prefer to have a single entry point for the persistence attributes (id and lifetime). We may add further constraints in the future (for example, that a certain id range is reserved to a certain lifetime). This belongs in slot management, which is not specific to SE support.

So I don't think this part of the code should change.

ok, yes in my mind, persistence here was for persistence level, thus I misunderstood the intent. Fine by me to keep it as it is then but probably that the documentation of the function should be improved along the line of @gilles-peskine-arm comments.

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 · 2020-06-17T11:55:19Z

Force-push updated commit message length as requested by @ronald-cron-arm

Inline with review comments. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 · 2020-06-18T14:23:34Z

@gilles-peskine-arm again, all failing CI is inside the ARM network and I cannot see what would've cause failure.

danh-arm · 2020-06-18T15:26:00Z

The CI failures seem mainly to be in our Mbed OS testing:

Compile [  0.1%]: mbed_tz_context.c

Compile [  0.2%]: rf_configuration.c

Compile [  0.3%]: fslittle_test.c

Compile [  0.4%]: MCR20Drv.c

Compile [  0.4%]: main.cpp

[Fatal Error] pk.h@49,10: psa/crypto.h: No such file or directory

[ERROR] In file included from ./mbed-os/features/mbedtls/inc/mbedtls/ssl_ciphersuites.h:33,

                 from ./mbed-os/features/mbedtls/inc/mbedtls/ssl.h:36,

                 from ./mbed-os/features/netsocket/TLSSocketWrapper.h:29,

                 from ./mbed-os/features/netsocket/nsapi.h:42,

                 from ./mbed-os/mbed.h:26,

                 from ./main.cpp:20:

./mbed-os/features/mbedtls/inc/mbedtls/pk.h:49:10: fatal error: psa/crypto.h: No such file or directory

   49 | #include "psa/crypto.h"

      |          ^~~~~~~~~~~~~~

compilation terminated.

However, I don't know enough about this change or that testing to suggest what the problem is.

There's also the ABI/API compatibility check failure, which Gilles said earlier can be ignored.

ronald-cron-arm · 2020-06-19T13:02:14Z

Mbed OS Testing is currently broken thus just ignore it. Sorry for the inconvenience. Thus this PR does not have any CI issues currently.

ronald-cron-arm

Thank you for addressing my comments. LGTM.

gilles-peskine-arm · 2020-06-26T18:26:54Z

CI passed except for:

Mbed OS which is currently failing for unrelated reasons.
ABI changes only in internal functions (false positives).

So this is as good as a pass.

gilles-peskine-arm · 2020-10-12T11:51:36Z

library/psa_crypto_slot_management.c

-    return( PSA_SUCCESS );
-
+        if( psa_is_key_id_valid( key_id,
+                                 psa_key_lifetime_is_external( lifetime ) ) )


Sorry for missing this at the time, but psa_key_lifetime_is_external is wrong here. This allows any application to create a key with a key id that's reserved for the implementation, as long as the key is in a secure element. This wasn't a big issue at the time of this pull request because Mbed TLS didn't use the implementation key id range. But with #3547 (comment) it's beginning to use the range and we're going to have to backtrack this change.

danh-arm added the enhancement label Jun 2, 2020

danh-arm added the needs-ci Needs to pass CI tests label Jun 2, 2020

danh-arm assigned gilles-peskine-arm Jun 2, 2020

stevew817 force-pushed the feature/volatile-keys-in-SE branch from b062248 to 68e9556 Compare June 2, 2020 15:28

gilles-peskine-arm requested changes Jun 2, 2020

View reviewed changes

gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces needs-work labels Jun 2, 2020

stevew817 force-pushed the feature/volatile-keys-in-SE branch from 169de02 to 3561551 Compare June 3, 2020 11:16

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests needs-work labels Jun 3, 2020

danh-arm requested a review from ronald-cron-arm June 8, 2020 10:13

gilles-peskine-arm requested changes Jun 8, 2020

View reviewed changes

library/psa_crypto_slot_management.c Outdated Show resolved Hide resolved

library/psa_crypto_slot_management.h Outdated Show resolved Hide resolved

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 8, 2020

gilles-peskine-arm reviewed Jun 8, 2020

View reviewed changes

tests/suites/test_suite_psa_crypto_se_driver_hal.function Outdated Show resolved Hide resolved

stevew817 added 5 commits June 8, 2020 18:19

Fix typo in currently unused macro constant

db06445

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Enable figuring out number of cores when running on OS X

8335f41

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Refactor lifetime checking to reflect split in location and persistence

c59de6a

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Do not persist transactions on volatile external keys

bbeaf18

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Add test to check that volatile external keys do not get persisted

223f287

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 force-pushed the feature/volatile-keys-in-SE branch from 3561551 to 62ec1d4 Compare June 8, 2020 16:38

stevew817 added a commit to stevew817/mbedtls that referenced this pull request Jun 8, 2020

Add changelog entry for Mbed-TLS#3382

44e84b4

stevew817 added a commit to stevew817/mbedtls that referenced this pull request Jun 8, 2020

Add changelog entry for Mbed-TLS#3382

4288356

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

gilles-peskine-arm previously approved these changes Jun 12, 2020

View reviewed changes

stevew817 dismissed gilles-peskine-arm’s stale review via bd95312 June 15, 2020 09:33

ronald-cron-arm requested changes Jun 16, 2020

View reviewed changes

stevew817 added 7 commits June 17, 2020 13:52

Add test to check key ID's from the vendor range are accepted

a50f283

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Split 'validate persistent key parameters' into independent validation

81fe7c3

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Add changelog entry for Mbed-TLS#3382

de18338

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Refactor equality-testing asserts in SE driver tests to TEST_EQUAL

f5a5e45

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Minor edit to comply with pointer naming standard

00106a1

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Clarify behaviour of psa_validate_key_location

74161ce

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Added missing newline in changelog entry

14b8184

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

stevew817 force-pushed the feature/volatile-keys-in-SE branch from bd95312 to 14b8184 Compare June 17, 2020 11:54

stevew817 added 2 commits June 17, 2020 14:52

Documentation and new function signature update

8c1e759

Inline with review comments. Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

Declare unused parameter

fa68609

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

ronald-cron-arm approved these changes Jun 19, 2020

View reviewed changes

ronald-cron-arm linked an issue Jun 19, 2020 that may be closed by this pull request

PSA: Support volatile secure element keys #3288

Closed

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 19, 2020

mpg requested a review from gilles-peskine-arm June 22, 2020 09:54

gilles-peskine-arm approved these changes Jun 26, 2020

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 26, 2020

gilles-peskine-arm merged commit 961914d into Mbed-TLS:development Jun 26, 2020

gilles-peskine-arm mentioned this pull request Oct 12, 2020

Openless PSA crypto APIs implementation #3547

Merged

gilles-peskine-arm reviewed Oct 12, 2020

View reviewed changes

Support volatile keys in external SE #3382

Support volatile keys in external SE #3382

Uh oh!

Conversation

stevew817 commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

Uh oh!

danh-arm commented Jun 2, 2020

Uh oh!

stevew817 commented Jun 2, 2020

Uh oh!

gilles-peskine-arm commented Jun 2, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Jun 3, 2020

Uh oh!

stevew817 commented Jun 3, 2020

Uh oh!

gilles-peskine-arm commented Jun 3, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

stevew817 commented Jun 15, 2020

Uh oh!

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevew817 commented Jun 17, 2020

Uh oh!

stevew817 commented Jun 18, 2020

stevew817 commented Jun 1, 2020 •

edited

Loading

gilles-peskine-arm Jun 16, 2020 •

edited

Loading