Modify PSA related error codes and types #55

2019-02-13T07:03:45-06:00

davidsaada commented

2019-02-13 07:03:45 -06:00

(Migrated from github.com)

PSA error codes and storage related types were changed in the PSA spec. So this PR:

Changes the values of PSA error codes according to PSA spec, eliminating ITS specific error codes.
Changes ITS related types to the more generic ones (psa_storage_*), as defined in the PSA spec.

PSA error codes and storage related types were changed in the PSA spec. So this PR: - Changes the values of PSA error codes according to PSA spec, eliminating ITS specific error codes. - Changes ITS related types to the more generic ones (`psa_storage_*`), as defined in the PSA spec.

Patater (Migrated from github.com) requested changes

2019-02-13 10:38:04 -06:00

include/psa/crypto_values.h

					
				@ -70,3 +62,3 @@

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)2)

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)-134)

				/** The requested action is denied by a policy.

The allocated_errors.h header file says this error is defined in error.h. Should we move it to a new error.h file? The header seems to expect certain errors coming from headers with a certain name.

#define PSA_ERROR_GENERIC_ERROR ((psa_status_t)-132) /* Defined in <psa/error.h> */
/* ... */
#define PSA_ERROR_INVALID_PADDING ((psa_status_t)-150) /* Defined in <psa/crypto.h> */

CC @gilles-peskine-arm Are you aware of the expectations on PSA implementations here?

The allocated_errors.h header file says this error is defined in error.h. Should we move it to a new error.h file? The header seems to expect certain errors coming from headers with a certain name. ```C #define PSA_ERROR_GENERIC_ERROR ((psa_status_t)-132) /* Defined in <psa/error.h> */ /* ... */ #define PSA_ERROR_INVALID_PADDING ((psa_status_t)-150) /* Defined in <psa/crypto.h> */ ``` CC @gilles-peskine-arm Are you aware of the expectations on PSA implementations here?

library/psa_crypto_storage_its.c

					
				@ -150,3 +113,3 @@

				    ret = psa_its_get_info( data_identifier, &data_identifier_info );

				    if( ret == PSA_ITS_ERROR_UID_NOT_FOUND )

				    if( ret == PSA_ERROR_DOES_NOT_EXIST )

This commit appears to be mixing ITS type updates with error status updates. Could you please make the identifier update changes in a separate commit?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-13 10:48:32 -06:00

include/psa/crypto_values.h

					
				@ -70,3 +62,3 @@

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)2)

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)-134)

				/** The requested action is denied by a policy.

We're standardizing psa_status_t and a common set of status codes across PSA services, to be defined in a header file psa/error.h that all services provide and which Arm will manage centrally (so if you end up with several copies of psa/error.h, there's always a total order between versions and you can pick the most recent one). The publishing strategy is that error.h will be distributed by every service that uses it. See internal discussions at https://github.com/ARMmbed/PSA-IPC-doc/issues/133 and https://github.com/ARMmbed/PSA-IPC-doc/pull/177

We're standardizing `psa_status_t` and a common set of status codes across PSA services, to be defined in a header file `psa/error.h` that all services provide and which Arm will manage centrally (so if you end up with several copies of `psa/error.h`, there's always a total order between versions and you can pick the most recent one). The publishing strategy is that `error.h` will be distributed by every service that uses it. See internal discussions at https://github.com/ARMmbed/PSA-IPC-doc/issues/133 and https://github.com/ARMmbed/PSA-IPC-doc/pull/177

Patater (Migrated from github.com) reviewed

2019-02-13 11:13:13 -06:00

include/psa/crypto_values.h

					
				@ -70,3 +62,3 @@

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)2)

				#define PSA_ERROR_NOT_SUPPORTED         ((psa_status_t)-134)

				/** The requested action is denied by a policy.

I'm wondering if this commit should then grab and add that error.h file instead, leaving only crypto.h specified errors in crypto.h.

The second commit in this PR can update the ITS types.

I'm wondering if this commit should then grab and add that error.h file instead, leaving only crypto.h specified errors in crypto.h. The second commit in this PR can update the ITS types.

Patater commented

2019-02-13 11:15:01 -06:00

(Migrated from github.com)

This is failing the psa_constant_names test and either this PR attempts to introduce a bug, or we need to adapt the psa_constant_names test to accommodate this PR's changes.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-02-13 11:31:00 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Please break this up into meaningful commits as requested by Jaeden.
This breaks the ITS build (psa_status_t is defined twice).
This reveals a pre-existing defect in psa_constant_names and test_psa_constant_names.py: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in https://github.com/ARMmbed/mbed-crypto/pull/56

* Please break this up into meaningful commits as requested by Jaeden. * This breaks the ITS build (`psa_status_t` is defined twice). * This reveals a pre-existing defect in `psa_constant_names` and `test_psa_constant_names.py`: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in https://github.com/ARMmbed/mbed-crypto/pull/56

include/psa/crypto_values.h

					
				@ -40,25 +40,17 @@

				 * @{

This is a private link. Please do not include private links in published code or documentation.

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

As long as psa/crypto_types.h (included via psa/crypto.h) defines psa_status_t, including psa/error.h as well will cause a compilation error due to the duplicate definition of psa_status_t. We either need to make the definition of psa_status_t conditional in psa/crypto_types.h or to start distributing an official version of psa/error.h.

As long as `psa/crypto_types.h` (included via `psa/crypto.h`) defines `psa_status_t`, including `psa/error.h` as well will cause a compilation error due to the duplicate definition of `psa_status_t`. We either need to make the definition of `psa_status_t` conditional in `psa/crypto_types.h` or to start distributing an official version of `psa/error.h`.

davidsaada commented

2019-02-13 14:14:23 -06:00

(Migrated from github.com)

* Please break this up into meaningful commits as requested by Jaeden.

* This breaks the ITS build (`psa_status_t` is defined twice).

* This reveals a pre-existing defect in `psa_constant_names` and `test_psa_constant_names.py`: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in #56

Will do (will also remove the private link). It's still not clear whether I should move all error codes to a new error.h file or keep them under crypto_values.h

> > > * Please break this up into meaningful commits as requested by Jaeden. > > * This breaks the ITS build (`psa_status_t` is defined twice). > > * This reveals a pre-existing defect in `psa_constant_names` and `test_psa_constant_names.py`: they don't fully support negative constants. This is signalled by CI, so we need to fix it. Fix available in #56 Will do (will also remove the private link). It's still not clear whether I should move all error codes to a new `error.h` file or keep them under `crypto_values.h`

davidsaada (Migrated from github.com) reviewed

2019-02-14 06:00:14 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

Not sure why this would fail the build. Type definitions in both files are the same, meaning that the compiler should not fail this duplication. I build it this way in mbed-os without issues.
Is this an empirical or a theoretical problem? I ask because I only added psa/error.h to my current mbed-os PR, so if this is empirical, than where do you take psa/error.h from?

Not sure why this would fail the build. Type definitions in both files are the same, meaning that the compiler should not fail this duplication. I build it this way in mbed-os without issues. Is this an empirical or a theoretical problem? I ask because I only added `psa/error.h` to my current mbed-os PR, so if this is empirical, than where do you take `psa/error.h` from?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-14 06:12:12 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

Theoretical in this instance, but empirical from a previous version. We had to add guards around the definition of psa_status_t so that it would only be defined in one place. You can have multiple #define for the same macro as long as they expand to the same token sequence, but some compilers warn about it. You can't have multiple typedef statements defining the same type name, even if the definitions are identical.

In your build, what exactly does psa/error.h contain? Who's defining psa_status_t and how do you avoid defining it twice?

Theoretical in this instance, but empirical from a previous version. We had to add guards around the definition of `psa_status_t` so that it would only be defined in one place. You can have multiple `#define` for the same macro as long as they expand to the same token sequence, but some compilers warn about it. You can't have multiple `typedef` statements defining the same type name, even if the definitions are identical. In your build, what exactly does `psa/error.h` contain? Who's defining `psa_status_t` and how do you avoid defining it twice?

davidsaada commented

2019-02-14 06:13:06 -06:00

(Migrated from github.com)

@Patater @gilles-peskine-arm Commits are now split to two: one for error codes, one for the change of ITS types to the generic types.
Error codes are still in crypto_values.h, as it seems you're not sure whether you want to keep them there or move them into a new error.h file. It can be done later on I guess.
All comments addressed except for the psa_status_t duplication.

@Patater @gilles-peskine-arm Commits are now split to two: one for error codes, one for the change of ITS types to the generic types. Error codes are still in `crypto_values.h`, as it seems you're not sure whether you want to keep them there or move them into a new error.h file. It can be done later on I guess. All comments addressed except for the `psa_status_t` duplication.

davidsaada (Migrated from github.com) reviewed

2019-02-14 06:18:10 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

psa/error.h is part of the PSA include directory. I'm adding it now to the parallel PR in mbed-os. Please note that psa_status_t is not a define but a typedef. As opposed to defines, compilers will not warn about duplicate typedefs as long as the referenced type is identical. In addition, as it's a typedef and not a define, one can't ask whether it's defined using #ifdef.

`psa/error.h` is part of the PSA include directory. I'm adding it now to [the parallel PR in mbed-os](https://github.com/ARMmbed/mbed-os/pull/9708). Please note that `psa_status_t` is not a define but a typedef. As opposed to defines, compilers will not warn about duplicate typedefs as long as the referenced type is identical. In addition, as it's a typedef and not a define, one can't ask whether it's defined using `#ifdef`.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-14 06:57:51 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

Duplicate definitions of a typedef are forbidden in C89 and C99. From N1256 (C99 TC3), §6.7 “Declarations”:

3. If an identifier has no linkage, there shall be no more than one declaration of the identifier
(in a declarator or type specifier) with the same scope and in the same name space, except
for tags as specified in 6.7.2.3.

Clang does warn about it if you ask it to use C89 or C99.

$ cat a.c
typedef int foo;
typedef int foo;
$ clang -std=c99 -Wall -Wextra -Werror -O -c a.c
a.c:2:13: error: redefinition of typedef 'foo' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef int foo;
            ^
a.c:1:13: note: previous definition is here
typedef int foo;
            ^
1 error generated.

Neither Clang nor GCC warn about it in non-picky mode, but I remember we've hard this error in at least one of the CC+CFLAGS combination we test in our CI, and we do aim to stick to standard C in Mbed TLS and Mbed Crypto.

Duplicate definitions of a typedef _are_ forbidden in C89 and C99. From N1256 (C99 TC3), §6.7 “Declarations”: > 3\. If an identifier has no linkage, there shall be no more than one declaration of the identifier (in a declarator or type specifier) with the same scope and in the same name space, except for tags as specified in 6.7.2.3. Clang does warn about it if you ask it to use C89 or C99. ``` $ cat a.c typedef int foo; typedef int foo; $ clang -std=c99 -Wall -Wextra -Werror -O -c a.c a.c:2:13: error: redefinition of typedef 'foo' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef int foo; ^ a.c:1:13: note: previous definition is here typedef int foo; ^ 1 error generated. ``` Neither Clang nor GCC warn about it in non-picky mode, but I remember we've hard this error in at least one of the CC+CFLAGS combination we test in our CI, and we do aim to stick to standard C in Mbed TLS and Mbed Crypto.

davidsaada (Migrated from github.com) reviewed

2019-02-14 07:03:19 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

OK, understood (didn't go that far).
How do you suggest to resolve this? It's a bit of a pickle, as there's no #ifdef for typedefs. I could ask whether PSA_SUCCESS is defined, assuming that PSA_SUCCESS and psa_status_t are both defined in psa/error.h, but that just seems pretty awkward to me.

OK, understood (didn't go that far). How do you suggest to resolve this? It's a bit of a pickle, as there's no `#ifdef` for typedefs. I could ask whether `PSA_SUCCESS` is defined, assuming that `PSA_SUCCESS` and `psa_status_t` are both defined in `psa/error.h`, but that just seems pretty awkward to me.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-14 07:06:26 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

That's the hack we had for a long time, and getting rid of that hack was my motivation for requesting a unified psa/error.h (https://github.com/ARMmbed/PSA-IPC-doc/issues/133) as the only place where psa_status_t is defined. Crypto is planning to switch from defining psa_status_t in crypto_types.h to including error.h as soon as version 1 of error.h has stabilized.

That's the hack we had for a long time, and getting rid of that hack was my motivation for requesting a unified `psa/error.h` (https://github.com/ARMmbed/PSA-IPC-doc/issues/133) as the only place where `psa_status_t` is defined. Crypto is planning to switch from defining `psa_status_t` in `crypto_types.h` to including `error.h` as soon as version 1 of `error.h` has stabilized.

davidsaada (Migrated from github.com) reviewed

2019-02-14 07:08:40 -06:00

library/psa_crypto_storage_its.c

					
				@ -27,6 +27,7 @@

				#if defined(MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C)

				#include "psa/error.h"

OK, will add this hack.

davidsaada commented

2019-02-14 07:32:34 -06:00

(Migrated from github.com)

Added the handling of psa_status_t duplication. All comments are addressed now.

Added the handling of `psa_status_t` duplication. All comments are addressed now.

Patater (Migrated from github.com) reviewed

2019-02-14 11:14:57 -06:00

Patater (Migrated from github.com) left a comment

Code LGTM

I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though.

How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to?

psa: Update types to version xxx of PSA ITS

This replaces psa_its_status_t with the more generic psa_status_t,
psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with
the new psa_storage_uid_t. This is necessary in order to integrate with
the newer implementation of PSA ITS landing in Mbed OS soon.

Code LGTM I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though. How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to? ``` psa: Update types to version xxx of PSA ITS This replaces psa_its_status_t with the more generic psa_status_t, psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with the new psa_storage_uid_t. This is necessary in order to integrate with the newer implementation of PSA ITS landing in Mbed OS soon. ```

davidsaada commented

2019-02-14 14:35:26 -06:00

(Migrated from github.com)

Code LGTM

I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though.

How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to?
psa: Update types to version xxx of PSA ITS

This replaces psa_its_status_t with the more generic psa_status_t,
psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with
the new psa_storage_uid_t. This is necessary in order to integrate with
the newer implementation of PSA ITS landing in Mbed OS soon.

Cool. Commit message modified in the spirit of your recommendation. Didn't see any change to the PSA spec version though, so left this one out.

> > > Code LGTM > > I think the commit message for "Change ITS specific types to the more generic ones (defined in PSA spec)" needs some work, though. > > How about the following, but after you fill in xxx with the version of the ITS spec this PR updates to? > > ``` > psa: Update types to version xxx of PSA ITS > > This replaces psa_its_status_t with the more generic psa_status_t, > psa_its_info_t with the new psa_storage_info_t, and psa_its_uid_t with > the new psa_storage_uid_t. This is necessary in order to integrate with > the newer implementation of PSA ITS landing in Mbed OS soon. > ``` Cool. Commit message modified in the spirit of your recommendation. Didn't see any change to the PSA spec version though, so left this one out.

Patater commented

2019-02-15 04:02:56 -06:00

(Migrated from github.com)

Sorry for the nitpicking, but it looks like you forgot to mention

- psa_storage_uid_t replaces psa_its_uid_t

Also, please don't end commit messages with a : or ., as per our git commit style guide

Thanks

Sorry for the nitpicking, but it looks like you forgot to mention ``` - psa_storage_uid_t replaces psa_its_uid_t ``` Also, please don't end commit messages with a `:` or `.`, as per our [git commit style guide](https://os.mbed.com/docs/mbed-os/v5.10/reference/workflow.html#guidelines-for-github-pull-requests) Thanks

davidsaada commented

2019-02-15 04:09:03 -06:00

(Migrated from github.com)

Sorry for the nitpicking, but it looks like you forgot to mention
- psa_storage_uid_t replaces psa_its_uid_t
Also, please don't end commit messages with a : or ., as per our git commit style guide

Thanks

This ain't nitpicking, it was a mistake I made there. Fixed now.

> > > Sorry for the nitpicking, but it looks like you forgot to mention > > ``` > - psa_storage_uid_t replaces psa_its_uid_t > ``` > > Also, please don't end commit messages with a `:` or `.`, as per our [git commit style guide](https://os.mbed.com/docs/mbed-os/v5.10/reference/workflow.html#guidelines-for-github-pull-requests) > > Thanks This ain't nitpicking, it was a mistake I made there. Fixed now.

Patater (Migrated from github.com) reviewed

2019-02-15 04:55:12 -06:00

Patater (Migrated from github.com) left a comment

No description provided.

LGTM

davidsaada commented

2019-02-15 06:32:01 -06:00

(Migrated from github.com)

@Patater Thanks for approving. Can you also approve PR #9708 in the mbed-os repo? It's the corresponding PR in mbed-os, including the same mbed-crypto changes.

@Patater Thanks for approving. Can you also approve [PR #9708 in the mbed-os repo](https://github.com/ARMmbed/mbed-os/pull/9708)? It's the corresponding PR in mbed-os, including the same mbed-crypto changes.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-02-15 11:11:00 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

The numerical values and double-definition guards are ok. Validated with a checkout of the private repository containing the PSA internal allocte error list at ../../PSA-IPC-doc and running

printf '#include <%s.h>\n' allocated_errors error crypto_types crypto_values | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null
printf '#include <%s.h>\n' crypto_types crypto_values allocated_errors error | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null

This pull request keeps some old names and gives them values that don't seem to come from anywhere. Having not-yet-renamed error codes is ok, but some errors are duplicated instead of renamed (EMPTY_SLOT and DOES_NOT_EXIST, OCCUPIED_SLOT and ALREADY_EXISTS), and some errors have values that may clash with values that others are using. Both problems aren't problems for the crypto library itself but they are problems for users of the crypto library.

The numerical values and double-definition guards are ok. Validated with a checkout of the private repository containing the PSA internal allocte error list at `../../PSA-IPC-doc` and running ``` printf '#include <%s.h>\n' allocated_errors error crypto_types crypto_values | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null printf '#include <%s.h>\n' crypto_types crypto_values allocated_errors error | cpp -I include/psa -I ../../PSA-IPC-doc/include/psa -I ../../PSA-IPC-doc >/dev/null ``` This pull request keeps some old names and gives them values that don't seem to come from anywhere. Having not-yet-renamed error codes is ok, but some errors are duplicated instead of renamed (`EMPTY_SLOT` and `DOES_NOT_EXIST`, `OCCUPIED_SLOT` and `ALREADY_EXISTS`), and some errors have values that may clash with values that others are using. Both problems aren't problems for the crypto library itself but they are problems for users of the crypto library.

include/psa/crypto_values.h

					
				@ -92,23 +84,19 @@

				 * buffer would succeed. However implementations may return this

PSA_ERROR_OCCUPIED_SLOT is to be renamed to PSA_ERROR_ALREADY_EXISTS from the base list.

`PSA_ERROR_OCCUPIED_SLOT` is to be renamed to `PSA_ERROR_ALREADY_EXISTS` from the base list.

PSA_ERROR_EMPTY_SLOT is to be renamed to PSA_ERROR_DOES_NOT_EXIST from the base list.

`PSA_ERROR_EMPTY_SLOT` is to be renamed to `PSA_ERROR_DOES_NOT_EXIST` from the base list.

include/psa/crypto_values.h

					
				@ -285,3 +270,4 @@

				#define PSA_ERROR_INSUFFICIENT_DATA     ((psa_status_t)-143)

				/** The key handle is not valid.

				 */

PSA_ERROR_INSUFFICIENT_CAPACITY is to be renamed to PSA_ERROR_INSUFFICIENT_DATA from the base list.

`PSA_ERROR_INSUFFICIENT_CAPACITY` is to be renamed to `PSA_ERROR_INSUFFICIENT_DATA` from the base list.

davidsaada commented

2019-02-15 14:04:47 -06:00

(Migrated from github.com)

Thanks @gilles-peskine-arm. Wasn't sure about these error codes myself. Replaced them according to your suggestion.

Patater (Migrated from github.com) reviewed

2019-02-18 04:42:31 -06:00

Patater (Migrated from github.com) left a comment

No description provided.

LGTM

gilles-peskine-arm (Migrated from github.com) requested changes

2019-02-18 04:59:13 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Blocker: please update psa_its_status_t and PSA_ITS_xxx in tests/suites/test_suite_psa_crypto_entropy.function as well. Make sure it's tested before merging this PR.

While you're at it, please update the comment in test_suite_psa_crypto.function.

Blocker: please update `psa_its_status_t` and `PSA_ITS_xxx` in `tests/suites/test_suite_psa_crypto_entropy.function` as well. Make sure it's tested before merging this PR. While you're at it, please update the comment in `test_suite_psa_crypto.function`.

include/psa/crypto_values.h

					
				@ -155,3 +142,4 @@

				#define PSA_ERROR_INSUFFICIENT_STORAGE  ((psa_status_t)-142)

				/** There was a communication failure inside the implementation.

				 *

The descriptions of PSA_ERROR_ALREADY_EXISTS, PSA_ERROR_DOES_NOT_EXIST and PSA_ERROR_INSUFFICIENT_DATA need work (grammaticality, specific meaning for crypto) but this is not a blocker, I can fix it later.

The descriptions of `PSA_ERROR_ALREADY_EXISTS`, `PSA_ERROR_DOES_NOT_EXIST` and `PSA_ERROR_INSUFFICIENT_DATA` need work (grammaticality, specific meaning for crypto) but this is not a blocker, I can fix it later.

tests/suites/test_suite_psa_crypto.function

					
				@ -420,3 +420,3 @@

				    uint8_t *public_key = NULL;

				    size_t public_key_length;

				    /* Return UNKNOWN_ERROR if something other than the final call to

				    /* Return GENERIC_ERROR if something other than the final call to

Comment needs updating

davidsaada commented

2019-02-18 05:59:47 -06:00

(Migrated from github.com)

@gilles-peskine-arm Pushed a fix according to your comments. Left the descriptions for the new error codes out (thought it would be better if an mbed crypto specialist would do that).

gilles-peskine-arm (Migrated from github.com) approved these changes

2019-02-18 06:21:54 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Looks fine.

Approved provided that this has been tested (including running test_suite_psa_crypto_entropy) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment?

Looks fine. Approved provided that this has been tested (including running `test_suite_psa_crypto_entropy`) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment?

davidsaada commented

2019-02-18 06:25:25 -06:00

(Migrated from github.com)

Looks fine.

Approved provided that this has been tested (including running test_suite_psa_crypto_entropy) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment?

Thanks for the approval.
As for the tests - I currently test it locally, but regarding CI, changes aren't currently tested yet. There's a pending PR in mbed-os, which includes these changes as well. This PR awaits your approval in order to start CI. ~~I'll copy the latest changes from here to there and then will appreciate your quick approval for it.~~ Changes from here copied there - again temporarily, just in order to start moving the CI chains.

> > > Looks fine. > > Approved provided that this has been tested (including running `test_suite_psa_crypto_entropy`) on Mbed OS in configurations with ITS with and without entropy injection. @Patater Is this tested on CI at the moment? Thanks for the approval. As for the tests - I currently test it locally, but regarding CI, changes aren't currently tested yet. There's a [pending PR](https://github.com/ARMmbed/mbed-os/pull/9708) in mbed-os, which includes these changes as well. This PR awaits your approval in order to start CI. ~I'll copy the latest changes from here to there and then will appreciate your quick approval for it.~ Changes from here copied there - again temporarily, just in order to start moving the CI chains.

Patater commented

2019-02-18 09:22:18 -06:00

(Migrated from github.com)

@Patater Is this tested on CI at the moment?

No, otherwise we'd have seen a failure in CI.

> @Patater Is this tested on CI at the moment? No, otherwise we'd have seen a failure in CI.

davidsaada commented

2019-02-18 09:33:34 -06:00

(Migrated from github.com)

@gilles-peskine-arm @Patater Sorry for nagging, but can you approve PR #9708 in mbed-os? The mbed-crypto part changes there are identical to the ones in this PR (they are all concentrated in the second commit). They will be discarded before that PR is merged.

@gilles-peskine-arm @Patater Sorry for nagging, but can you approve [PR #9708](https://github.com/ARMmbed/mbed-os/pull/9708) in mbed-os? The mbed-crypto part changes there are identical to the ones in this PR (they are all concentrated in the second commit). They will be discarded before that PR is merged.

Patater commented

2019-02-18 09:36:46 -06:00

(Migrated from github.com)

Travis failure is DTLS 3d Proxy. Jenkins failure is FreeBSD timing test. Both are known issues.

Pre-merge test failed because this PR is not based on a version of development recent enough to have psa_constant_names that handles signed error codes (https://github.com/ARMmbed/mbed-crypto/pull/56).

Travis failure is DTLS 3d Proxy. Jenkins failure is FreeBSD timing test. Both are known issues. Pre-merge test failed because this PR is not based on a version of development recent enough to have psa_constant_names that handles signed error codes (https://github.com/ARMmbed/mbed-crypto/pull/56).

Patater (Migrated from github.com) approved these changes

2019-02-18 09:39:08 -06:00

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

Patater