PSA Update return code for non-existing key in various key operations #4198

maulik-arm · 2021-03-04T12:59:51Z

Description

Update return code for non-existing key in various key operations
Fixes issue #4162

Status

READY

Requires Backporting

No, PSA only.

Migrations

If there is any API change, what's the incentive and logic for it.

YES | NO

Additional comments

Any additional information that could be of interest

library/psa_crypto_slot_management.c

bensze01

Looks good to me.

bensze01

Copied over review comments from #4189

library/psa_crypto.c

bensze01 · 2021-03-05T16:09:58Z

library/psa_crypto_slot_management.h

 *
 * \retval #PSA_SUCCESS
+ * \retval #PSA_ERROR_INVALID_HANDLE


Originally posted by @Summer-ARM in #4189 (comment)

sorry, do I miss something? didn't see this return value in the function implementation.

@Summer-ARM It looks like psa_validate_key_id() is returning PSA_ERROR_INVALID_HANDLE instead of PSA_ERROR_INVALID_ARGUMENT, as it's documentation would suggest.

There is indeed an issue here but not related to the issue this PR is solving. Thus not addressed in this PR. It is now addressed in #4279.

ronald-cron-arm · 2021-03-05T17:08:03Z

library/psa_crypto_slot_management.h

 *
 * \retval #PSA_SUCCESS
+ * \retval #PSA_ERROR_INVALID_HANDLE
+ *         \p key_id is not a valid key identifier.


There is psa_validate_key_id() to validate key identifiers thus why do you introduce such validation in this function that validates the persistence of a key ?

ronald-cron-arm

In its current version, I think that there are still APIs that are returning PSA_ERROR_DOES_NOT_EXIST. The approach of this PR is to fix the return value just before they return. I think it would be better to fix the source of the PSA_ERROR_DOES_NOT_EXIST error code for APIs which seems to be only psa_get_and_lock_key_slot(). This implies some changes in PSA test suites and in the coding of psa_open/close_key() I think. I don't know how much time you can spend on this. Please let me know if you are ok to follow the suggested path of if you prefer me to do it.

maulik-arm · 2021-03-09T09:46:02Z

Hello Ronald, I have limited time this week as I need to prepare tf-m for your 2.26 release(development branch) and on training next week. If you do not need it soon, I will start looking into this, but I may not be as fast as you 🙂 . So I leave it upto you. Thank you Best Regards, Maulik

…

________________________________ From: Ronald Cron <[email protected]> Sent: Monday, March 8, 2021 2:32 PM To: ARMmbed/mbedtls <[email protected]> Cc: Maulik Patel <[email protected]>; Author <[email protected]> Subject: Re: [ARMmbed/mbedtls] PSA Update return code for non-existing key in various key operations (#4198) @ronald-cron-arm requested changes on this pull request. In its current version, I think that there are still APIs that are returning PSA_ERROR_DOES_NOT_EXIST. The approach of this PR is to fix the return value just before they return. I think it would be better to fix the source of the PSA_ERROR_DOES_NOT_EXIST error code for APIs which seems to be only psa_get_and_lock_key_slot(). This implies some changes in PSA test suites and in the coding of psa_open/close_key() I think. I don't know how much time you can spend on this. Please let me know if you are ok to follow the suggested path of if you prefer me to do it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#4198 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ASCUHYX7JDSMYH44AHRQ3GDTCTNYBANCNFSM4YTGTBVA>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ronald-cron-arm · 2021-03-11T09:10:47Z

I started to investigate what is need to change in the PSA crypto implementation, in order for PSA APIs not to return PSA_ERROR_DOES_NOT_EXIST error code but for the deprecated psa_open_key() and thus address completely #4162. Please have a look to this. I've started to update the unit tests accordingly and I think it seems to go in the right direction, not completed though.

Could you check that the changes in PSA crypto as done here fix the issues related to the PSA_ERROR_DOES_NOT_EXIST when running the compliance test suite ?

ronald-cron-arm

Thanks for the changes. The final result seems correct to me but I think the commit history should be reworked:

Remove the three first commits.
When fixing the return codes in the tests please do it in three commits: one with the fixes for psa_open_key, one with the fixes for psa_close_key and the last one for the other APIs.
Do that locally and then force push. In case you are not familiar with the force push: git push --force maulik-arm/fix-4162.

gilles-peskine-arm · 2021-03-17T09:07:57Z

Please use git push --force-with-lease when you do a force-push, never git push --force. It seems that push --force sometimes causes a race condition with GitHub that causes it to close the PR and makes it difficult to reopen.

ronald-cron-arm · 2021-03-17T09:26:45Z

I never had any issue with --force but if --force-with-lease is safer I will use that.

Update expected return values of psa_open_key() to PSA_ERROR_DOES_NOT_EXIST for invalid key handle operations. Signed-off-by: Maulik Patel <[email protected]>

Update expected return values of psa_close_key() to PSA_ERROR_INVALID_HANDLE for invalid key handle operations. Signed-off-by: Maulik Patel <[email protected]>

Update expected return values of psa_get_key_attributes(), psa_export_key() and other key api(s) to PSA_ERROR_INVALID_HANDLE for invalid key. Signed-off-by: Maulik Patel <[email protected]>

maulik-arm · 2021-03-17T17:04:53Z

Hello Ronald, Giles.
Thank you for comments. I have reworked the commit history and added the change log entry file. Regarding the documentation, I see that psa_open_key already mentions return value PSA_ERROR_DOES_NOT_EXIST for invalid ID. Is there any other place that needs update?
Thank you

ChangeLog.d/fix_return_type_for_invalid_crypto_key.txt

gilles-peskine-arm · 2021-03-17T17:48:32Z

psa_open_key was the only function that documented DOES_NOT_EXIST, so it doesn't need to be removed anywhere. Changing the invalid-handle case of psa_open_key is the only necessary documentation change I can see.

ronald-cron-arm · 2021-03-18T07:58:43Z

Brilliant. Thanks for the rework. The flow of changes is easy to follow and understand that way. The documentation of psa_open_key already lists PSA_ERROR_DOES_NOT_EXIST and not PSA_ERROR_INVALID_HANDLE (as defined in v1 beta 3 of the spec). I think it is rather the case that the documentation and the code were not aligned before but are aligned now. Thus nothing to change here I think. It seems the only things left to take care of are:
. Update the change log
. Make sure tests/scripts/check_files.py is passing (not passing yet on the CI but seems related to the change log thus if you change it it may be ok with just modifying the change log).

ronald-cron-arm

LGTM

maulik-arm · 2021-03-19T10:12:30Z

Thank you for review Ronald. There is Jenkins Test failure in checks: PR-4198-merging-TLS-Testing (https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-merge/detail/PR-4198-merge/18/pipeline) showing here.. I tried enabling config MBEDTLS_MEMORY_BUFFER_ALLOC_C, MBEDTLS_MEMORY_BACKTRACE. and MBEDTLS_PLATFORM_MEMORY in config.h and it is passing tests locally on my ubuntu machine (taking really long though). Is there any cause of concern here?

ronald-cron-arm · 2021-03-19T11:23:23Z

I think it is just a CI timeout thus nothing to worry on your side.

ChangeLog.d/fix_return_type_for_invalid_crypto_key.txt

Signed-off-by: Maulik Patel <[email protected]>

There are two comments associated to this "change requested". The first one is not longer relevant as related to code that is not changed anymore. The second one is still valid but not relevant to this PR and addressed in #4279.

PSA Update return code for non-existing key in various key operations

PSA Crypto API spec requests key operation to return error code PSA_ERROR_INVALID_HANDLE when the key doesn't exist. However, according to [1], PSA key operation implementation in Mbed TLS returns PSA_ERROR_DOES_NOT_EXIST instead. TF-M currently works normally since TF-M specific key handle check will return PSA_ERROR_INVALID_HANDLE for a non-existing key, without calling Mbed TLS PSA key operation. Apply the merged Mbed TLS fix to TF-M to prepare for enhancement of TF-M key handle check. [1]: Mbed-TLS/mbedtls#4198 Change-Id: I79dda1c54dc8377afbfaefdf180bb81c7ff99f02 Signed-off-by: David Hu <[email protected]>

maulik-arm force-pushed the maulik-arm/fix-4162 branch from 3ee50a2 to 70b4301 Compare March 4, 2021 13:53

bensze01 requested changes Mar 5, 2021

View reviewed changes

library/psa_crypto_slot_management.c Outdated Show resolved Hide resolved

ronald-cron-arm self-requested a review March 5, 2021 09:30

bensze01 added bug needs-ci Needs to pass CI tests PSA compliance labels Mar 5, 2021

bensze01 self-assigned this Mar 5, 2021

maulik-arm force-pushed the maulik-arm/fix-4162 branch from bb12893 to aac7145 Compare March 5, 2021 10:40

bensze01 added the needs-review Every commit must be reviewed by at least two team members, label Mar 5, 2021

bensze01 self-requested a review March 5, 2021 10:43

bensze01 removed the needs-ci Needs to pass CI tests label Mar 5, 2021

bensze01 approved these changes Mar 5, 2021

View reviewed changes

bensze01 linked an issue Mar 5, 2021 that may be closed by this pull request

Do not return PSA_ERROR_DOES_NOT_EXIST for a non-existing key #4162

Closed

bensze01 mentioned this pull request Mar 5, 2021

Update return code for various key operations #4189

Closed

bensze01 previously requested changes Mar 5, 2021

View reviewed changes

bensze01 changed the title ~~Fixes issue #4162, correct return type~~ PSA Update return code for non-existing key in various key operations Mar 5, 2021

bensze01 added the size-s Estimated task size: small (~2d) label Mar 5, 2021

ronald-cron-arm requested changes Mar 5, 2021

View reviewed changes

bensze01 added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 8, 2021

ronald-cron-arm requested changes Mar 8, 2021

View reviewed changes

maulik-arm force-pushed the maulik-arm/fix-4162 branch 2 times, most recently from ace4792 to 817f07b Compare March 15, 2021 15:19

ronald-cron-arm requested changes Mar 17, 2021

View reviewed changes

maulik-arm added 3 commits March 17, 2021 14:55

Update psa_open_key tests for invalid key.

f65ad8c

Update expected return values of psa_open_key() to PSA_ERROR_DOES_NOT_EXIST for invalid key handle operations. Signed-off-by: Maulik Patel <[email protected]>

Update tests for psa_close_key for invalid key.

02a1750

Update expected return values of psa_close_key() to PSA_ERROR_INVALID_HANDLE for invalid key handle operations. Signed-off-by: Maulik Patel <[email protected]>

Update tests for other invalid key operations.

3240c9d

Update expected return values of psa_get_key_attributes(), psa_export_key() and other key api(s) to PSA_ERROR_INVALID_HANDLE for invalid key. Signed-off-by: Maulik Patel <[email protected]>

maulik-arm force-pushed the maulik-arm/fix-4162 branch 2 times, most recently from 53cfe6c to abfffd2 Compare March 17, 2021 17:01

gilles-peskine-arm requested changes Mar 17, 2021

View reviewed changes

ChangeLog.d/fix_return_type_for_invalid_crypto_key.txt Outdated Show resolved Hide resolved

ChangeLog.d/fix_return_type_for_invalid_crypto_key.txt Outdated Show resolved Hide resolved

maulik-arm force-pushed the maulik-arm/fix-4162 branch from abfffd2 to 29ca5d5 Compare March 18, 2021 16:52

ronald-cron-arm previously approved these changes Mar 19, 2021

View reviewed changes

gilles-peskine-arm requested review from bensze01 and gilles-peskine-arm March 22, 2021 16:27

gilles-peskine-arm requested changes Mar 22, 2021

View reviewed changes

ChangeLog.d/fix_return_type_for_invalid_crypto_key.txt Outdated Show resolved Hide resolved

maulik-arm dismissed ronald-cron-arm’s stale review via aa59823 March 23, 2021 17:06

maulik-arm force-pushed the maulik-arm/fix-4162 branch from 29ca5d5 to aa59823 Compare March 23, 2021 17:06

Add Change log entry for bug fix.

f41be14

Signed-off-by: Maulik Patel <[email protected]>

maulik-arm force-pushed the maulik-arm/fix-4162 branch from aa59823 to f41be14 Compare April 1, 2021 09:03

gilles-peskine-arm approved these changes Apr 1, 2021

View reviewed changes

ronald-cron-arm approved these changes Apr 1, 2021

View reviewed changes

ronald-cron-arm merged commit 2af9641 into Mbed-TLS:development Apr 1, 2021

daverodgman pushed a commit that referenced this pull request Apr 23, 2021

Merge pull request #4198 from maulik-arm/maulik-arm/fix-4162

6911e0d

PSA Update return code for non-existing key in various key operations

gilles-peskine-arm mentioned this pull request Jun 2, 2021

Return code of psa_get_key_attributes() #4607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA Update return code for non-existing key in various key operations #4198

PSA Update return code for non-existing key in various key operations #4198

maulik-arm commented Mar 4, 2021 •

edited by bensze01

Loading

bensze01 left a comment

bensze01 left a comment

bensze01 Mar 5, 2021

bensze01 Mar 5, 2021 •

edited

Loading

ronald-cron-arm Apr 1, 2021

ronald-cron-arm Mar 5, 2021

ronald-cron-arm left a comment

maulik-arm commented Mar 9, 2021 via email

ronald-cron-arm commented Mar 11, 2021

ronald-cron-arm left a comment

gilles-peskine-arm commented Mar 17, 2021

ronald-cron-arm commented Mar 17, 2021

maulik-arm commented Mar 17, 2021

gilles-peskine-arm commented Mar 17, 2021

ronald-cron-arm commented Mar 18, 2021

ronald-cron-arm left a comment

maulik-arm commented Mar 19, 2021

ronald-cron-arm commented Mar 19, 2021

PSA Update return code for non-existing key in various key operations #4198

PSA Update return code for non-existing key in various key operations #4198

Conversation

maulik-arm commented Mar 4, 2021 • edited by bensze01 Loading

Description

Status

Requires Backporting

Migrations

Additional comments

bensze01 left a comment

Choose a reason for hiding this comment

bensze01 left a comment

Choose a reason for hiding this comment

bensze01 Mar 5, 2021

Choose a reason for hiding this comment

bensze01 Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Apr 1, 2021

Choose a reason for hiding this comment

ronald-cron-arm Mar 5, 2021

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

maulik-arm commented Mar 9, 2021 via email

ronald-cron-arm commented Mar 11, 2021

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Mar 17, 2021

ronald-cron-arm commented Mar 17, 2021

maulik-arm commented Mar 17, 2021

gilles-peskine-arm commented Mar 17, 2021

ronald-cron-arm commented Mar 18, 2021

ronald-cron-arm left a comment

Choose a reason for hiding this comment

maulik-arm commented Mar 19, 2021

ronald-cron-arm commented Mar 19, 2021

maulik-arm commented Mar 4, 2021 •

edited by bensze01

Loading

bensze01 Mar 5, 2021 •

edited

Loading