Make psa_close_key(0) and psa_destroy_key(0) succeed #292

gilles-peskine-arm · 2019-10-08T13:58:53Z

Although this was not written explicitly, the specification strongly suggested that this would return PSA_ERROR_INVALID_HANDLE. While returning INVALID_HANDLE makes sense, it was awkward for a very common programming style where applications can store 0 in a handle variable to indicate that the handle has been closed or has never been open: applications had to either check if (handle != 0) before calling psa_close_key(handle) or psa_destroy_key(handle), or ignore errors from the close/destroy function. Now applications following this style can just call psa_close_key(handle) or psa_destroy_key(handle).

Document that passing 0 to a close/destroy function does nothing and returns PSA_SUCCESS. Although this was not written explicitly, the specification strongly suggested that this would return PSA_ERROR_INVALID_HANDLE. While returning INVALID_HANDLE makes sense, it was awkward for a very common programming style where applications can store 0 in a handle variable to indicate that the handle has been closed or has never been open: applications had to either check if (handle != 0) before calling psa_close_key(handle) or psa_destroy_key(handle), or ignore errors from the close/destroy function. Now applications following this style can just call psa_close_key(handle) or psa_destroy_key(handle).

athoelke

This looks good 👍

dgreen-arm

LGTM

Patater · 2019-10-09T16:32:32Z

tests/suites/test_suite_psa_crypto_slot_management.function

+        TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );
+        TEST_EQUAL( psa_destroy_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );
+    }
+    if( handle1 + 1 != 0 )


When would handle1 + 1 be equal to zero? I don't think psa_import_key() would ever return -1 for a handle, would it? Certainly not in this test. What are we trying to test with this and how can we guarantee we test it?

It won't happen with our implementation (handle1 is deterministically PSA_KEY_SLOT_COUNT), but (psa_key_handle_t)(-1) is a potentially valid handle.

The goal of this test is to close a nonzero, invalid handle and check that this seems to keep the key store in a good state. We check this by checking that handle1 is still there and that the shutdown is clean.

Patater · 2019-10-09T16:35:27Z

tests/suites/test_suite_psa_crypto_slot_management.function

+    /* 0 is special: it isn't a valid handle, but close/destroy
+     * succeeds on it. */
+    TEST_EQUAL( psa_close_key( 0 ), PSA_SUCCESS );
+    TEST_EQUAL( psa_destroy_key( 0 ), PSA_SUCCESS );


Do we need these checks here? They don't use handle1 at all. We get equivalent coverage from the close_invalid() and destroy_invalid() tests (because they are called with handle 0), don't we?

psa_destroy_key(0) destroy_invalid:0:PSA_SUCCESS psa_close_key(0) close_invalid:0:PSA_SUCCESS

Yes, there's some redundancy between this test case and the xxx_invalid ones in psa_crypto.function. I don't think this redundancy is harmful so I didn't bother to remove it.

Redundancy was added by this PR. Redundancy is harmful to maintainability, as there are more places to update if behavior changes.

No, the redundancy was there before, and I kept it to avoid scope creep. But ok, I'll fix it.

I've consolidated the two test functions.

Patater · 2019-10-09T16:38:45Z

tests/suites/test_suite_psa_crypto_slot_management.function

-    TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE );
+    if( handle1 - 1 != 0 )
+    {
+        TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );


Do we ever get inside this block? As currently written, doesn't psa_import_key() always return 1 as the handle? The determinism of our current handle allocation scheme may be at odds with what we are aiming to test here. We may need to call psa_import_key or psa_generate_key multiple times to get the coverage we are looking for, or depend entirely on the destroy_invalid() and close_invalid() tests.

We could add the "Allocate a handle and store a key in it" and "check that the original handle is intact" bits to the destroy_invalid() and close_invalid() tests in order to more directly test what we want, or add a handle parameter to this test and rename it from invalid_handle() to something like test_invalid_handles_dont_invalidate_valid_handle()...

We currently allocate handles from the top, so handle1 - 1 is nonzero. But that's an implementation detail. There are many other possible ways to write this test, but I think the current way is good enough.

OK, I had read this as allocating from the bottom. If we switch to allocating from the bottom, we could easily end up not testing what we want to test anymore with these tests. Could you please change the if blocks to asserts and comment about this dependency? Then at least we won't silently reduce our test coverage if the assumptions the test was built on change.

So I made this change to not try both valid+1 and valid-1, but now that I look back I don't understand why you requested this. Before, the tests covered both allocating from the top and allocating from the bottom, without making any assumption about the range. Now, the tests hard-code that the range starts at 0. Why do you object to making the test more robust against implementation details?

Exactly the opposite. I would like the test to be robust against implementation details. The test as written had if statements which if not executed, we wouldn't notice. Replacing them with asserts will ensure we don't accidentally reduce test coverage.

What would you assert on? I don't understand why you want to make the tests fail on certain changes in implementation details. The tests as written before were robust to implementation changes: there would have been no loss of coverage if the handle allocation mechanism had changed.

The tests as written before were robust to implementation changes

I don't believe that was the case.

Patater · 2019-10-10T11:50:30Z

tests/suites/test_suite_psa_crypto_slot_management.function

+    /* 0 is special: it isn't a valid handle, but close/destroy
+     * succeeds on it. */
+    TEST_EQUAL( psa_close_key( 0 ), PSA_SUCCESS );
+    TEST_EQUAL( psa_destroy_key( 0 ), PSA_SUCCESS );


Redundancy was added by this PR. Redundancy is harmful to maintainability, as there are more places to update if behavior changes.

Patater · 2019-10-10T11:52:30Z

tests/suites/test_suite_psa_crypto_slot_management.function

-    TEST_EQUAL( psa_destroy_key( handle1 + 1 ), PSA_ERROR_INVALID_HANDLE );
+    if( handle1 - 1 != 0 )
+    {
+        TEST_EQUAL( psa_close_key( handle1 - 1 ), PSA_ERROR_INVALID_HANDLE );


OK, I had read this as allocating from the bottom. If we switch to allocating from the bottom, we could easily end up not testing what we want to test anymore with these tests. Could you please change the if blocks to asserts and comment about this dependency? Then at least we won't silently reduce our test coverage if the assumptions the test was built on change.

Consolidate the invalid-handle tests from test_suite_psa_crypto and test_suite_psa_crypto_slot_management. Start with the code in test_suite_psa_crypto_slot_management and adapt it to test one invalid handle value per run of the test function.

Patater · 2019-10-11T11:32:14Z

tests/suites/test_suite_psa_crypto_slot_management.function

+            if( valid_handle == 1 )
+                invalid_handle = 2;
+            else
+                invalid_handle = valid_handle - 1;


Do we sometimes want to test invalid_handle = valid_handle + 1? We may need a INVALID_HANDLE_UNOPENED_ABOVE for this, renaming the - 1 case to INVALID_HANDLE_UNOPENED_BELOW.

I think it would be better to test both, but you objected to that.

I objected to the tests silently only testing one way without letting us know we reduced test coverage.

So it's a misunderstanding then? The tests were testing both ways, skipping one of the ways if it was not applicable.

I didn't like skipping if one wasn't applicable, as both might be silently skipped. I'd like to always see both ways tested (above and below). If this means we have to allocate multiple keys, we should do that.

No, it was not possible for both to be silently skipped. handle1 - 1 != 0 and handle1 + 1 != 0 can't both be false.

gilles-peskine-arm · 2019-10-14T09:21:48Z

CI only failed on ATECC jobs, which is a known unrelated issue.

* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed * ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed * ARMmbed#259: Check `len` against buffers size upper bound in PSA tests * ARMmbed#288: Add ECDSA tests with hash and key of different lengths * ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed * ARMmbed#316: Stop transactions from being reentrant * ARMmbed#317: getting_started: Make it clear that keys are passed in * ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value * ARMmbed#298: Test a build without any asymmetric cryptography * ARMmbed#284: Fix some possibly-undefined variable warnings * ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE * ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split

gilles-peskine-arm added 3 commits October 8, 2019 15:43

Test that psa_close_key(0) and psa_destroy_key(0) succeed

f102e4e

Make psa_close_key(0) and psa_destroy_key(0) succeed

1841cf4

gilles-peskine-arm added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Oct 8, 2019

athoelke previously approved these changes Oct 8, 2019

View reviewed changes

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Oct 9, 2019

Update slot management tests now that {close,destroy}_key(0) succeed

04129a0

gilles-peskine-arm dismissed athoelke’s stale review via 04129a0 October 9, 2019 14:24

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Oct 9, 2019

dgreen-arm self-requested a review October 9, 2019 15:04

dgreen-arm previously approved these changes Oct 9, 2019

View reviewed changes

Patater reviewed Oct 9, 2019

View reviewed changes

Patater suggested changes Oct 10, 2019

View reviewed changes

Patater added the needs: work The pull request needs rework before it can be merged. label Oct 11, 2019

Consolidate invalid-handle tests

b8cde4e

Consolidate the invalid-handle tests from test_suite_psa_crypto and test_suite_psa_crypto_slot_management. Start with the code in test_suite_psa_crypto_slot_management and adapt it to test one invalid handle value per run of the test function.

gilles-peskine-arm dismissed dgreen-arm’s stale review via b8cde4e October 11, 2019 09:46

gilles-peskine-arm requested review from Patater and dgreen-arm October 11, 2019 09:46

Patater reviewed Oct 11, 2019

View reviewed changes

dgreen-arm approved these changes Oct 14, 2019

View reviewed changes

Patater approved these changes Oct 14, 2019

View reviewed changes

gilles-peskine-arm removed needs: review The pull request is ready for review. This generally means that it has no known issues. needs: work The pull request needs rework before it can be merged. labels Oct 14, 2019

gilles-peskine-arm merged commit 150d577 into ARMmbed:development Oct 14, 2019

Make psa_close_key(0) and psa_destroy_key(0) succeed #292

Make psa_close_key(0) and psa_destroy_key(0) succeed #292

Uh oh!

Conversation

gilles-peskine-arm commented Oct 8, 2019

Uh oh!

athoelke left a comment

Choose a reason for hiding this comment

Uh oh!

dgreen-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!

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!

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 commented Oct 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants