PSA Storage: Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs #180

simonqhughes · 2019-07-18T12:30:59Z

This PR should be squashed and merged. The following is offered as the squash commit message:

Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs

The following provides more information on this PR:
- PSA stands for Platform Security Architecture.
- Add support for use of psa_trusted_storage_api internal-trusted_storage.h v1.0.0
  as the interface to the psa_trusted_storage_linux backend (i.e. for persistent
  storage when MBEDTLS_PSA_ITS_FILE_C is not defined). This requires changes
  to psa_crypto_its.h and psa_crypto_storage.c to migrate to the new API.
- Add MBEDTLS_PSA_ITS_FILE_C_INCLUDE_TEST to config.h to build
  useful storage related tests (e.g. test_suite_psa_its and 
  test_suite_psa_crypto_persistent_key) even when MBEDTLS_PSA_ITS_FILE_C 
  is not defined. This is used by the psa_trusted_storage_linux
  mbed_crypto_psa_storage_config.h for example.
- Add MBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX in config.h so the
  file storage directory can be specified. It can also be specified in the
  following way:
      make CFLAGS="-DMBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX='\"/home/username/\"'"
  (Note the inclusion of the trailing '/' in the prefix path.)
- Add CMake option for explicitly link library to trusted_storage
    option name: LINK_WITH_TRUSTED_STORAGE
    default value: ON

simonqhughes · 2019-07-18T13:15:07Z

@jenia81 @moshe-shahar @shelib01 Please request the project administrators to add you as reviewers as I'm unable to do so.

moshe-shahar · 2019-07-18T14:22:19Z

library/CMakeLists.txt

@@ -1,7 +1,6 @@
 option(USE_STATIC_MBEDTLS_LIBRARY "Build mbed TLS static library." ON)
 option(USE_SHARED_MBEDTLS_LIBRARY "Build mbed TLS shared library." OFF)
 option(LINK_WITH_PTHREAD "Explicitly link mbed TLS library to pthread." OFF)
-option(LINK_WITH_TRUSTED_STORAGE "Explicitly link mbed TLS library to trusted_storage." ON)


5c61791
@Patater ,
@simonqhughes revert this commit because of the tests fails.
Can you please help me integrate that change so it will not fails the tests?

simonqhughes · 2019-07-19T12:46:16Z

This PR has been submitted to mbedtls, which @Patater request for completing automated testing of the current PR:
Mbed-TLS/mbedtls#2750

Patater · 2019-07-22T14:53:52Z

include/mbedtls/config.h

+#if ! defined(MBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX)
+#define MBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX ""
+#endif
+


Please revert the config.h changes. Each new option is something we are stuck with for a long time, and it seems like these are options made just for now.

MBEDTLS_PSA_ITS_FILE_C_INCLUDE_TEST is so we can test the Linux-native PSA storage using Mbed Crypto's test suite. Long term, the Linux-native PSA storage library would have its own test suite and Mbed Crypto could trust that it is implemented correctly and just use it. Mbed Crypto's own ITS tests would test that its own impelementation of ITS was OK, and, even if it doesn't do so now, it can make assumptions about the implementation that a more generic test couldn't.

MBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX isn't needed. We could add PSA_ITS_STORAGE_PREFIX to your Mbed TLS user config file for the same effect.

Patater · 2019-07-22T14:54:10Z

library/psa_crypto_storage.c

@@ -40,6 +40,7 @@
 #else /* Native ITS implementation */
 #include "psa/error.h"
 #include "psa/internal_trusted_storage.h"
+


Avoid adding this extra newline, please.

OK, extra line removed. Thanks.

Patater · 2019-07-22T14:56:15Z

library/psa_its_file.c

@@ -44,7 +44,7 @@
 #include <stdio.h>
 #include <string.h>

-#define PSA_ITS_STORAGE_PREFIX ""
+#define PSA_ITS_STORAGE_PREFIX MBEDTLS_PSA_ITS_FILE_C_STORAGE_PREFIX


Suggested change:

- #define PSA_ITS_STORAGE_PREFIX "" + #if !defined(PSA_ITS_STORAGE_PREFIX) + #define PSA_ITS_STORAGE_PREFIX "" + #endif

OK thanks. Done.

Patater · 2019-07-22T14:57:18Z

tests/suites/test_suite_psa_its.function

 #include "../library/psa_crypto_its.h"
+#else
+#include "psa/internal_trusted_storage.h"
+#endif


Probably don't need this change if we remove MBEDTLS_PSA_ITS_FILE_C_INCLUDE_TEST, since this test isn't really meant to test any "psa/internal_trusted_storage.h" implementation, just Mbed Crypto's own.

Patater · 2019-07-22T15:02:50Z

library/psa_its_file.c

@@ -137,7 +137,8 @@ psa_status_t psa_its_get_info( psa_storage_uid_t uid,
 psa_status_t psa_its_get( psa_storage_uid_t uid,
                          uint32_t data_offset,
                          uint32_t data_length,
-                          void *p_data )
+                          void *p_data,
+                          size_t* p_data_length )


Please put the * next to the variable name, not the type.

Patater · 2019-07-22T15:06:17Z

tests/suites/test_suite_psa_its.function


    ASSERT_ALLOC( buffer, MAX( data1->len, data2->len ) );

    PSA_ASSERT( psa_its_set_wrap( uid, data1->len, data1->x, flags1 ) );
    PSA_ASSERT( psa_its_get_info( uid, &info ) );
    TEST_ASSERT( info.size == data1->len );
    TEST_ASSERT( info.flags == flags1 );
-    PSA_ASSERT( psa_its_get( uid, 0, data1->len, buffer ) );
+    PSA_ASSERT( psa_its_get( uid, 0, data1->len, buffer, &ret_len ) );


We should probably assert that ret_len is equal to data1->len.

Patater · 2019-07-22T15:06:28Z

tests/suites/test_suite_psa_its.function

    ASSERT_COMPARE( data1->x, data1->len, buffer, data1->len );

    PSA_ASSERT( psa_its_set_wrap( uid, data2->len, data2->x, flags2 ) );
    PSA_ASSERT( psa_its_get_info( uid, &info ) );
    TEST_ASSERT( info.size == data2->len );
    TEST_ASSERT( info.flags == flags2 );
-    PSA_ASSERT( psa_its_get( uid, 0, data2->len, buffer ) );
+    ret_len = 0;
+    PSA_ASSERT( psa_its_get( uid, 0, data2->len, buffer, &ret_len ) );


We should probably assert that ret_len is equal to data2->len.

Patater · 2019-07-22T15:06:58Z

tests/suites/test_suite_psa_its.function

@@ -143,11 +152,11 @@ void set_multiple( int first_id, int count )
    {
        mbedtls_snprintf( stored, sizeof( stored ),
                          "Content of file 0x%08lx", (unsigned long) uid );
-        PSA_ASSERT( psa_its_get( uid, 0, sizeof( stored ), retrieved ) );
+        PSA_ASSERT( psa_its_get( uid, 0, sizeof( stored ), retrieved, &ret_len ) );


We should probably assert that ret_len is equal to sizeof( stored ).

Patater · 2019-07-22T15:07:17Z

tests/suites/test_suite_psa_its.function

        ASSERT_COMPARE( retrieved, sizeof( stored ),
                        stored, sizeof( stored ) );
        PSA_ASSERT( psa_its_remove( uid ) );
-        TEST_ASSERT( psa_its_get( uid, 0, 0, NULL ) ==
+        TEST_ASSERT( psa_its_get( uid, 0, 0, NULL, NULL ) ==


Good to see that we are also testing the NULL data_length parameter case.

Patater · 2019-07-22T15:08:01Z

tests/suites/test_suite_psa_its.function


    ASSERT_ALLOC( buffer, length + 16 );
    trailer = buffer + length;
    memset( trailer, '-', 16 );

    PSA_ASSERT( psa_its_set_wrap( uid, data->len, data->x, 0 ) );

-    status = psa_its_get( uid, offset, length_arg, buffer );
+    status = psa_its_get( uid, offset, length_arg, buffer, &ret_len );


We should probably assert that ret_len is equal to length when status == PSA_SUCCESS

simonqhughes · 2019-07-23T13:14:24Z

This PR should be squashed and merged. The following is offered as the squash commit message having done the rework:

Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs

The following provides more information on this PR:
- PSA stands for Platform Security Architecture.
- Add support for use of psa_trusted_storage_api internal-trusted_storage.h v1.0.0
  as the interface to the psa_trusted_storage_linux backend (i.e. for persistent
  storage when MBEDTLS_PSA_ITS_FILE_C is not defined). This requires changes
  to psa_crypto_its.h and psa_crypto_storage.c to migrate to the new API.

Patater

Thanks for the changes. This looks pretty close to me. We should make a few changes.

Could you please arrange your commits as you desire locally, giving them the commit messages you want? We don't use the "squash and merge" feature of GitHub.

Thanks!

Patater · 2019-07-23T14:45:47Z

library/psa_crypto_its.h

@@ -106,7 +107,8 @@ psa_status_t psa_its_set(psa_storage_uid_t uid,
 psa_status_t psa_its_get(psa_storage_uid_t uid,
                         uint32_t data_offset,
                         uint32_t data_length,
-                         void *p_data);
+                         void *p_data,
+                         size_t* p_data_length );


Please put the * next to the variable name.

Patater · 2019-07-23T14:46:37Z

library/psa_crypto_storage.c


    status = psa_its_get_info( data_identifier, &data_identifier_info );
    if( status  != PSA_SUCCESS )
        return( status );

-    status = psa_its_get( data_identifier, 0, (uint32_t) data_size, data );
+    status = psa_its_get( data_identifier, 0, (uint32_t) data_size, data, &data_length );


Should we check data_length is equal to data_size and return an error (probably PSA_ERROR_STORAGE_FAILURE) if not?

yes, its better.

Patater · 2019-07-23T14:54:43Z

tests/suites/test_suite_psa_its.function

    TEST_ASSERT( status == (psa_status_t) expected_status );
    if( status == PSA_SUCCESS )
+    {
+        TEST_ASSERT( (size_t) length_arg == ret_len );
        ASSERT_COMPARE( data->x + offset, length,


We can do this in one assert if we replace the second length with length_arg. Same applies to other similar asserts added in this PR.

I think you mean the code changes to the following (which uses ret_len rather than length_arg):

if( status == PSA_SUCCESS ) ASSERT_COMPARE( data->x + offset, length, buffer, ret_len );

Otherwise you're not testing the returned value. The above is the change that will go into the rework as I think this is what you're requesting.

However, it would be better to have this:

if( status == PSA_SUCCESS ) ASSERT_COMPARE( data->x + offset, length_arg, buffer, ret_len );

as it's length_arg that's supplied to the psa_its_get(), not length which may be different. This version tests psa_its_get() rather than get_at().

But I think this is marginal.

Yes, I like your suggestion better, to match what's passed to psa_its_get().

Ok, will change it to the new version.

The following provides more information on this PR: - PSA stands for Platform Security Architecture. - Add support for use of psa_trusted_storage_api internal_trusted_storage.h v1.0.0 as the interface to the psa_trusted_storage_linux backend (i.e. for persistent storage when MBEDTLS_PSA_ITS_FILE_C is not defined). This requires changes to psa_crypto_its.h and psa_crypto_storage.c to migrate to the new API.

Patater

LGTM

Patater · 2019-07-24T09:13:23Z

CI failure on merge run is flaky m->m dtls1_2,yes TLS-ECDHE-RSA-WITH-AES-128-GCM-SHA256 test, which passed fine on the head run.

simonqhughes changed the title ~~Feature psa storage~~ PSA Storage: Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs Jul 18, 2019

moshe-shahar approved these changes Jul 18, 2019

View reviewed changes

Patater requested review from jenia81, moshe-shahar and shelib01 July 18, 2019 14:12

Patater added the needs: ci Needs a passing full CI run label Jul 18, 2019

moshe-shahar previously approved these changes Jul 18, 2019

View reviewed changes

moshe-shahar reviewed Jul 18, 2019

View reviewed changes

simonqhughes dismissed moshe-shahar’s stale review via b30e23c July 18, 2019 14:56

simonqhughes mentioned this pull request Jul 19, 2019

PSA Storage: Add new define symbols to config.h Mbed-TLS/mbedtls#2750

Closed

moshe-shahar mentioned this pull request Jul 21, 2019

Add CMake option for explicitly link library to trusted_storage #186

Merged

Patater suggested changes Jul 22, 2019

View reviewed changes

Patater suggested changes Jul 23, 2019

View reviewed changes

Patater added needs: review The pull request is ready for review. This generally means that it has no known issues. compliance Discrepancy between the library and the PSA Crypto API specification labels Jul 23, 2019

Patater approved these changes Jul 24, 2019

View reviewed changes

moshe-shahar approved these changes Jul 24, 2019

View reviewed changes

Patater merged commit b992313 into ARMmbed:development Jul 24, 2019

PSA Storage: Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs #180

PSA Storage: Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs #180

Uh oh!

Conversation

simonqhughes commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonqhughes commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonqhughes commented Jul 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonqhughes Jul 23, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonqhughes Jul 23, 2019 • 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!

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!

simonqhughes commented Jul 23, 2019

Uh oh!

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

simonqhughes Jul 23, 2019 • 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!

Choose a reason for hiding this comment

simonqhughes commented Jul 18, 2019 •

edited

Loading

simonqhughes commented Jul 18, 2019 •

edited

Loading

simonqhughes Jul 23, 2019 •

edited

Loading

simonqhughes Jul 23, 2019 •

edited

Loading

simonqhughes Jul 23, 2019 •

edited

Loading