gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

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

Merged
simonqhughes merged 1 commit from feature-psa-storage into development 2019-07-24 05:19:33 -05:00
Conversation 31 Commits 1 Files changed 4 +31 -16
simonqhughes commented 2019-07-18 07:30:59 -05:00 (Migrated from github.com)

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
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 ```
jenia81 (Migrated from github.com) reviewed 2019-07-18 07:30:59 -05:00
shelib01 (Migrated from github.com) reviewed 2019-07-18 07:30:59 -05:00
simonqhughes commented 2019-07-18 08:15:07 -05:00 (Migrated from github.com)

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

@jenia81 @moshe-shahar @shelib01 Please request the project administrators to add you as reviewers as I'm unable to do so.
moshe-shahar (Migrated from github.com) approved these changes 2019-07-18 09:12:43 -05:00
moshe-shahar (Migrated from github.com) reviewed 2019-07-18 09:14:11 -05:00
moshe-shahar (Migrated from github.com) reviewed 2019-07-18 09:22:19 -05:00
library/CMakeLists.txt
moshe-shahar (Migrated from github.com) commented 2019-07-18 09:22:19 -05:00

https://github.com/ARMmbed/mbed-crypto/pull/180/commits/5c6179145a09fbb8097b432a50c46030854063e9
@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?

https://github.com/ARMmbed/mbed-crypto/pull/180/commits/5c6179145a09fbb8097b432a50c46030854063e9 @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 commented 2019-07-19 07:46:16 -05:00 (Migrated from github.com)

This PR has been submitted to mbedtls, which @Patater request for completing automated testing of the current PR:
https://github.com/ARMmbed/mbedtls/pull/2750

This PR has been submitted to mbedtls, which @Patater request for completing automated testing of the current PR: https://github.com/ARMmbed/mbedtls/pull/2750
Patater (Migrated from github.com) requested changes 2019-07-22 10:08:04 -05:00
include/mbedtls/config.h
Patater (Migrated from github.com) commented 2019-07-22 09:53:52 -05:00

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.

  1. 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.
  2. 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.
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. 1. 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. 2. `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.
library/psa_crypto_storage.c
Patater (Migrated from github.com) commented 2019-07-22 09:54:10 -05:00

Avoid adding this extra newline, please.

Avoid adding this extra newline, please.
library/psa_its_file.c
@ -44,7 +44,9 @@
#include <stdio.h>
Patater (Migrated from github.com) commented 2019-07-22 09:56:15 -05:00

Suggested change:

- #define PSA_ITS_STORAGE_PREFIX ""
+ #if !defined(PSA_ITS_STORAGE_PREFIX)
+ #define PSA_ITS_STORAGE_PREFIX ""
+ #endif
Suggested change: ```diff - #define PSA_ITS_STORAGE_PREFIX "" + #if !defined(PSA_ITS_STORAGE_PREFIX) + #define PSA_ITS_STORAGE_PREFIX "" + #endif ```
library/psa_its_file.c
Patater (Migrated from github.com) commented 2019-07-22 10:02:50 -05:00

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

Please put the `*` next to the variable name, not the type.
tests/scripts/all.sh
Patater (Migrated from github.com) commented 2019-07-22 10:04:31 -05:00

We shouldn't need to add these lines when the MBEDTLS_PSA_ITS_FILE_C_INCLUDE_TEST option is removed.

We shouldn't need to add these lines when the `MBEDTLS_PSA_ITS_FILE_C_INCLUDE_TEST` option is removed.
tests/suites/test_suite_psa_crypto_persistent_key.data
Patater (Migrated from github.com) commented 2019-07-22 10:04:50 -05:00

Not needed change anymore

Not needed change anymore
tests/suites/test_suite_psa_its.function
Patater (Migrated from github.com) commented 2019-07-22 09:57:18 -05:00

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.

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.
tests/suites/test_suite_psa_its.function
Patater (Migrated from github.com) commented 2019-07-22 10:06:05 -05:00

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

We should probably assert that `ret_len` is equal to `data->len`.
tests/suites/test_suite_psa_its.function
@ -130,6 +133,7 @@ void set_multiple( int first_id, int count )
psa_storage_uid_t uid;
char stored[40];
char retrieved[40];
size_t ret_len = 0;
Patater (Migrated from github.com) commented 2019-07-22 10:06:17 -05:00

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

We should probably assert that `ret_len` is equal to `data1->len`.
tests/suites/test_suite_psa_its.function
Patater (Migrated from github.com) commented 2019-07-22 10:06:28 -05:00

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

We should probably assert that `ret_len` is equal to `data2->len`.
tests/suites/test_suite_psa_its.function
@ -205,3 +209,4 @@
buffer, ret_len );
for( i = 0; i < 16; i++ )
TEST_ASSERT( trailer[i] == '-' );
PSA_ASSERT( psa_its_remove( uid ) );
Patater (Migrated from github.com) commented 2019-07-22 10:06:58 -05:00

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

We should probably assert that `ret_len` is equal to `sizeof( stored )`.
tests/suites/test_suite_psa_its.function
Patater (Migrated from github.com) commented 2019-07-22 10:07:17 -05:00

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

Good to see that we are also testing the `NULL` `data_length` parameter case.
tests/suites/test_suite_psa_its.function
Patater (Migrated from github.com) commented 2019-07-22 10:08:01 -05:00

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

We should probably assert that `ret_len` is equal to `length` when `status == PSA_SUCCESS`
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:19:17 -05:00
include/mbedtls/config.h
simonqhughes (Migrated from github.com) commented 2019-07-23 07:19:17 -05:00

OK. Done.

OK. Done.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:20:33 -05:00
library/psa_crypto_storage.c
simonqhughes (Migrated from github.com) commented 2019-07-23 07:20:32 -05:00

OK, extra line removed. Thanks.

OK, extra line removed. Thanks.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:25:03 -05:00
library/psa_its_file.c
@ -44,7 +44,9 @@
#include <stdio.h>
simonqhughes (Migrated from github.com) commented 2019-07-23 07:25:03 -05:00

OK thanks. Done.

OK thanks. Done.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:28:48 -05:00
tests/suites/test_suite_psa_its.function
simonqhughes (Migrated from github.com) commented 2019-07-23 07:28:48 -05:00

OK. Done.

OK. Done.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:30:28 -05:00
library/psa_its_file.c
simonqhughes (Migrated from github.com) commented 2019-07-23 07:30:28 -05:00

Done.

Done.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:36:42 -05:00
tests/suites/test_suite_psa_its.function
simonqhughes (Migrated from github.com) commented 2019-07-23 07:36:42 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:40:44 -05:00
tests/suites/test_suite_psa_its.function
@ -130,6 +133,7 @@ void set_multiple( int first_id, int count )
psa_storage_uid_t uid;
char stored[40];
char retrieved[40];
size_t ret_len = 0;
simonqhughes (Migrated from github.com) commented 2019-07-23 07:40:43 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:40:53 -05:00
tests/suites/test_suite_psa_its.function
simonqhughes (Migrated from github.com) commented 2019-07-23 07:40:53 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 07:59:57 -05:00
tests/suites/test_suite_psa_its.function
@ -205,3 +209,4 @@
buffer, ret_len );
for( i = 0; i < 16; i++ )
TEST_ASSERT( trailer[i] == '-' );
PSA_ASSERT( psa_its_remove( uid ) );
simonqhughes (Migrated from github.com) commented 2019-07-23 07:59:57 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 08:00:10 -05:00
tests/suites/test_suite_psa_its.function
simonqhughes (Migrated from github.com) commented 2019-07-23 08:00:10 -05:00

Ok

Ok
simonqhughes (Migrated from github.com) reviewed 2019-07-23 08:00:23 -05:00
tests/suites/test_suite_psa_its.function
simonqhughes (Migrated from github.com) commented 2019-07-23 08:00:23 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 08:01:44 -05:00
tests/scripts/all.sh
simonqhughes (Migrated from github.com) commented 2019-07-23 08:01:43 -05:00

Done

Done
simonqhughes (Migrated from github.com) reviewed 2019-07-23 08:02:02 -05:00
tests/suites/test_suite_psa_crypto_persistent_key.data
simonqhughes (Migrated from github.com) commented 2019-07-23 08:02:01 -05:00

OK, removed.

OK, removed.
simonqhughes commented 2019-07-23 08:14:24 -05:00 (Migrated from github.com)

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.
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 (Migrated from github.com) requested changes 2019-07-23 09:55:50 -05:00
Patater (Migrated from github.com) left a comment

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!

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!
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,
Patater (Migrated from github.com) commented 2019-07-23 09:45:47 -05:00

Please put the * next to the variable name.

Please put the `*` next to the variable name.
library/psa_crypto_storage.c
@ -103,2 +104,3 @@
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 );
if( data_size != data_length )
Patater (Migrated from github.com) commented 2019-07-23 09:46:37 -05:00

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

Should we check `data_length` is equal to `data_size` and return an error (probably `PSA_ERROR_STORAGE_FAILURE`) if not?
tests/suites/test_suite_psa_its.function
@ -191,3 +195,4 @@
unsigned char *trailer;
size_t i;
size_t ret_len = 0;
Patater (Migrated from github.com) commented 2019-07-23 09:54:43 -05:00

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.

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.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 11:02:25 -05:00
tests/suites/test_suite_psa_its.function
@ -191,3 +195,4 @@
unsigned char *trailer;
size_t i;
size_t ret_len = 0;
simonqhughes (Migrated from github.com) commented 2019-07-23 11:02:24 -05:00

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.

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.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 11:10:15 -05:00
library/psa_crypto_storage.c
@ -103,2 +104,3 @@
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 );
if( data_size != data_length )
simonqhughes (Migrated from github.com) commented 2019-07-23 11:10:15 -05:00

yes, its better.

yes, its better.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 11:19:01 -05:00
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,
simonqhughes (Migrated from github.com) commented 2019-07-23 11:19:01 -05:00

Done.

Done.
Patater (Migrated from github.com) reviewed 2019-07-23 11:21:26 -05:00
tests/suites/test_suite_psa_its.function
@ -191,3 +195,4 @@
unsigned char *trailer;
size_t i;
size_t ret_len = 0;
Patater (Migrated from github.com) commented 2019-07-23 11:21:26 -05:00

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

Yes, I like your suggestion better, to match what's passed to `psa_its_get()`.
simonqhughes (Migrated from github.com) reviewed 2019-07-23 11:23:44 -05:00
tests/suites/test_suite_psa_its.function
@ -191,3 +195,4 @@
unsigned char *trailer;
size_t i;
size_t ret_len = 0;
simonqhughes (Migrated from github.com) commented 2019-07-23 11:23:44 -05:00

Ok, will change it to the new version.

Ok, will change it to the new version.
Patater (Migrated from github.com) approved these changes 2019-07-24 04:12:44 -05:00
Patater (Migrated from github.com) left a comment

LGTM

LGTM
Patater commented 2019-07-24 04:13:23 -05:00 (Migrated from github.com)

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.

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.
moshe-shahar (Migrated from github.com) approved these changes 2019-07-24 05:16:46 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
jenia81
shelib01
Patater
moshe-shahar
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
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.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!180
No description provided.