Psa/tfm pkcs11 #3

CharleyChu · 2020-05-31T20:34:57Z

Description

Updates to work with TF-Mv1.0 release and vendor key support

AFR IDT test result on Cypress PSoC64 board

========== Test Summary ==========
Execution Time: 57m24s
Tests Completed: 172
Tests Passed: 171
Tests Failed: 1
Tests Skipped: 0

Test Groups:
FullTLS: PASSED
FullPKCS11_ECC: PASSED
FullPKCS11_RSA: PASSED
FullPKCS11_Core: PASSED
FullMQTT: PASSED
FreeRTOSVersion: PASSED
FreeRTOSIntegrity: FAILED
FullSecureSockets: PASSED
Optional Test Groups:
CmakeBuildSystem: PASSED
FullWiFi: PASSED

aws_demos: mqtt, greengrass

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Charley Chu <haoc@cypress.com>

A typical sequence to query a key’s attributes is as follows: 1. Call psa_get_key_attributes(). 2. Call psa_get_key_xxx functions to retrieve the attribute(s) that you are interested in. 3. Call psa_reset_key_attributes() to free any resources that may be used by the attribute structure. Signed-off-by: Charley Chu <haoc@cypress.com>

Signed-off-by: Charley Chu <haoc@cypress.com>

TF-Mv1.0 doesn't support persistent key Signed-off-by: Charley Chu <haoc@cypress.com>

Sherryzhang2 · 2020-06-02T06:26:49Z

libraries/abstractions/pkcs11/psa/iot_pkcs11_psa.c

-            /* Close device private key which is a persistent key. Note that here we only close it other than
-             * destroying it so that the key can be activated by calling psa_open_key then next time it is used.
-             */
-            xResult = PKCS11PSARemoveObject( (uint8_t *)pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS,


A persistent key is stored in non-volatile memory. When it is opened, the PSA crypto service read the data from the non-volatile memory and load it into volatile memory. Each time a persistent key is opened, a new volatile memory is taken. So the persistent key should be closed to free the volatile memory at the end of the shim layer. Otherwise, there will be multiple volatile memory occupied by the key.

the decision can be made by the upper layer. If the object no longer needed, it should call C_DestroyObject() before close the session.

In AFR IDT testing, it has such scenario

Create key/certificate objects

run test_case_1

run test_case_2

Destroy the objects.

if the object is removed while close the session 1, the test_case_2 will fail as objects can't be found.

If the object is removed and closed in C_Finalize, then in the c_findobjects, it will open the key and import it into the P11KeyConfig. So in test_case_2, it will find the object by psa_open_key. This is my implementation.
If does not remove the objects here, and does not call PKCS11PSAGetKeyHandle in c_findobjects, the key will be opened multi-times during the tests while it is already opened. This may cause open key failure as there are only 16 key entries in TF-M. This will cause the test_case_2 fail.
In your code, PKCS11PSAGetKeyHandle is called in c_findobjects and the key is not removed here, so the key is only opened one time. This should also work.

In your implementation, the key is not closed in the PKCS#11 and uses the same key_handle that is returned by psa_open_key. But the key may be closed by other APPs, if so, the original key handle is not valid anymore or it has been allocated to another key. So I prefer to closing the key after using it.

I see. the key is closed, not destroyed. However, we have TF-Mv1.0, which doesn't support persistent key. So, close means destroy.

I will upload a change to make this conditional - only skip those two calls for TF-Mv1.0.

Thanks,
Charley

Sherryzhang2 · 2020-06-02T06:35:33Z

libraries/abstractions/pkcs11/psa/iot_pkcs11_psa.c

+                             pkcs11configLABEL_CODE_VERIFICATION_KEY ) == 0 )
+            {
+                xResult = FindKeyObjects( pxSession,
+                                          PSA_CODE_VERIFICATION_KEY_ID,


Just confirming, in c_createobjects, the code verification key is not set as a persistent key. Here still try to open the key. Is it because the key may be provisioned by others with persistent key type?

On our board, it is capable to provision keys/certificates during manufacture.

If so, it is reasonable to treat the code verification key as a persistent key here.

- Let upper layer to handle object removing. Signed-off-by: Charley Chu <haoc@cypress.com>

- Too many warning messages Signed-off-by: Charley Chu <haoc@cypress.com>

Sherryzhang2

From my understanding, the main changes in the PR are: 1. wrapped some similar code into functions. 2. add the TF-Mv1.0 macro which controls whether persistent keys are supported.
The PR looks good to me.
Thanks,
Sherry

david-wang-2015 · 2020-06-11T10:44:41Z

Hi @CharleyChu ,
The PR looks good.
We're moving to a new repository dedicated for this shim layer (today or tomorrow). Could you wait one day for the new repository ready for PR?
When moved to the new repository, we will leave this branch as it is here. So I think it's worth to do the PR merge in the new repository instead of this branch.
What do you think?

CharleyChu · 2020-06-11T16:26:50Z

No problem. Thanks!

david-wang-2015 · 2020-06-11T16:45:36Z

Hi @CharleyChu ,
The new repository is ready. Could you push the refined PR to https://github.com/Linaro/freertos-pkcs11-psa ?
Thanks.

CharleyChu · 2020-06-11T17:08:15Z

Will do. Thanks!

Charley Chu added 5 commits May 30, 2020 23:02

Resolve compiling errors and warnings

73903d5

Signed-off-by: Charley Chu <haoc@cypress.com>

Update code verification key

3c719ce

Signed-off-by: Charley Chu <haoc@cypress.com>

Handle certificate in both PEM and DER format

5e4d779

Signed-off-by: Charley Chu <haoc@cypress.com>

Update to make it work with TF-Mv1.0 release

6d1dbe1

TF-Mv1.0 doesn't support persistent key Signed-off-by: Charley Chu <haoc@cypress.com>

Sherryzhang2 reviewed Jun 10, 2020

View reviewed changes

Charley Chu added 2 commits June 10, 2020 22:51

Fix IDT test error

6a87a31

- Let upper layer to handle object removing. Signed-off-by: Charley Chu <haoc@cypress.com>

Fix IDT test failure

30a6360

- Too many warning messages Signed-off-by: Charley Chu <haoc@cypress.com>

CharleyChu force-pushed the psa/tfm-pkcs11 branch from 32e4c66 to 30a6360 Compare June 10, 2020 22:56

Sherryzhang2 approved these changes Jun 11, 2020

View reviewed changes

Psa/tfm pkcs11 #3

Are you sure you want to change the base?

Psa/tfm pkcs11 #3

Uh oh!

Conversation

CharleyChu commented May 31, 2020

Description

========== Test Summary ========== Execution Time: 57m24s Tests Completed: 172 Tests Passed: 171 Tests Failed: 1 Tests Skipped: 0

Test Groups: FullTLS: PASSED FullPKCS11_ECC: PASSED FullPKCS11_RSA: PASSED FullPKCS11_Core: PASSED FullMQTT: PASSED FreeRTOSVersion: PASSED FreeRTOSIntegrity: FAILED FullSecureSockets: PASSED Optional Test Groups: CmakeBuildSystem: PASSED FullWiFi: PASSED

Checklist:

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!

Sherryzhang2 left a comment

Choose a reason for hiding this comment

Uh oh!

david-wang-2015 commented Jun 11, 2020

Uh oh!

CharleyChu commented Jun 11, 2020

Uh oh!

david-wang-2015 commented Jun 11, 2020

Uh oh!

CharleyChu commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

========== Test Summary ==========
Execution Time: 57m24s
Tests Completed: 172
Tests Passed: 171
Tests Failed: 1
Tests Skipped: 0

Test Groups:
FullTLS: PASSED
FullPKCS11_ECC: PASSED
FullPKCS11_RSA: PASSED
FullPKCS11_Core: PASSED
FullMQTT: PASSED
FreeRTOSVersion: PASSED
FreeRTOSIntegrity: FAILED
FullSecureSockets: PASSED
Optional Test Groups:
CmakeBuildSystem: PASSED
FullWiFi: PASSED