Merge development into API spec branch #92

gilles-peskine-arm · 2019-04-11T22:18:24Z

Make a branch containing both the latest specification improvements and the latest implementation. This is a merge of the development branch at c70a3c7 into psa-api-1.0-beta.
Update the key agreement tests so that the CI passes apart from X509/TLS code.

This makes some updates to the spec:

Updated error codes to be uniform across PSA APIs (Modify PSA related error codes and types #55).
Mention some additional possibilities of returning BAD_STATE.
~~Driver model evolution.~~
Copyediting.

Out of scope:

Backward compatibility with the old one-shot key derivation API from before beta 1.
Good test coverage of key derivation and key agreement.
Making MBEDTLS_USE_PSA_CRYPTO work.

Reviewers: please assume that the current tip of psa-api-1.0-beta and the commit used from development are ok, and review only “Merge remote-tracking branch 'upstream-crypto/development' into psa-api-beta2-merge-development” and the subsequent commits.

In multipart cipher tests, test that each step of psa_cipher_update produces output of the expected length. The length is hard-coded in the test data since it depends on the mode. The length of the output of psa_cipher_finish is effectively tested because it's the total output length minus the length produced by the update steps.

…itial Specification of how keys and the random seed are stored

Prepare support for 64 bit key ids under SPM

It is now required to initialize PSA Crypto operation contexts before calling psa_*_setup(). Otherwise, one gets a PSA_ERROR_BAD_STATE error.

Add missing initializers to PSA Crypto example. Operation contexts must be initialized before calling psa_*_setup().

It is now required to initialize PSA Crypto operation contexts before calling psa_*_setup(). Otherwise, one gets a PSA_ERROR_BAD_STATE error.

…er_update_test Improve multipart cipher tests: 2 blocks, CTR

PSA_MAX_PERSISTENT_KEY_IDENTIFIER was actually one plus the maximum key identifier. Change it to be the maximum value, and change the code that uses it accordingly. There is no semantic change here (the maximum value hasn't changed). This commit only makes the implementation clearer.

Differentiate between _key identifiers_, which are always `uint32_t`, and _key file identifiers_, which are platform-dependent. Normally, the two are the same. In `psa/crypto_platform.h`, define `psa_app_key_id_t` (which is always 32 bits, the standard key identifier type) and `psa_key_file_id_t` (which will be different in some service builds). A subsequent commit will introduce a platform where the two are different. It would make sense for the function declarations in `psa/crypto.h` to use `psa_key_file_id_t`. However this file is currently part of the PSA Crypto API specification, so it must stick to the standard type `psa_key_id_t`. Hence, as long as the specification and Mbed Crypto are not separate, use the implementation-specific file `psa/crypto_platform.h` to define `psa_key_id_t` as `psa_key_file_id_t`. In the library, systematically use `psa_key_file_id_t`. perl -i -pe 's/psa_key_id_t/psa_key_file_id_t/g' library/*.[hc]

Declare the owner as psa_key_owner_id_t, of which an implementation must be provided separately. Make this a configuration option MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER, to make the conditional compilation flow easier to follow. Declare it in config.h to pacify check_names.sh. Support for a specific implementation of psa_key_owner_id_t in storage backends will come in a subsequent commit.

When building for the PSA crypto service (defined(PSA_CRYPTO_SECURE)), define psa_key_owner_id_t as int32_t, which is how a PSA platform encodes partition identity. Note that this only takes effect when the build option MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER is active. Support this configuration in the ITS backend.

…internal_key_id Support key file IDs encoding the key owner

In case the new constants aren't available yet in Mbed TLS, continue to use the deprecated constants if they are available.

Switch to the terminology "key file identifier", as has been done in the code. The owner uid is now in the upper 32 bits of the key file identifier, which facilitates namespacing.

Ensure that when doing MAC operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

Ensure that when doing cipher operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

If a hash context has not been set up, fail with PSA_ERROR_BAD_STATE as documented in crypto.h and the PSA Crypto specification.

Extend hash bad order test in line with the new bad order tests for MAC and cipher, covering more cases and making comments and test layout consistent. Ensure that when doing hash operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

Calling psa_*_setup() twice on a MAC, cipher, or hash context should result in a PSA_ERROR_BAD_STATE error because the operation has already been set up. Fixes ARMmbed#10

psa: Be compatible with deprecated constants

…y_file_id Update the architecture document regarding key file identifiers

In places where we detect a context is in a bad state and there is no sensitive data to clear, simply return PSA_ERROR_BAD_STATE and don't abort on behalf of the application. The application will choose what to do when it gets a bad state error. The motivation for this change is that an application should decide what to do when it misuses the API and encounters a PSA_ERROR_BAD_STATE error. The library should not attempt to abort on behalf of the application, as that may not be the correct thing to do in all circumstances.

Add deprecated error codes to help transition between the previous version of the PSA Crypto specification and the current one.

psa: Add backwards compatible error codes

Disallow use of invalid contexts

Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, then mbedtls_ecdh_calc_secret.

Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, with keys belonging to different groups. This should fail, but currently passes.

Patater · 2019-04-15T14:38:28Z

I've completed my review.

yanesca

Found a minor style issue, but other than that it looks good to me.

yanesca · 2019-04-17T08:05:35Z

library/psa_crypto.c

@@ -4665,6 +4684,38 @@ psa_status_t psa_key_agreement( psa_crypto_generator_t *generator,
    return( status );
 }

+psa_status_t psa_key_agreement_raw_shared_secret(psa_algorithm_t alg,


style: missing space after/before parentheses.

Fixed in rebase

gilles-peskine-arm · 2019-04-17T14:05:44Z

Thanks for the reviews. I rebased some commits to address minor issues.

For the driver model, there hadn't been any changes in the API branch, so the development branch had the latest version.

Patater · 2019-04-17T16:41:45Z

library/psa_crypto.c

+        /* If an error happens and is not handled properly, the output
+         * may be used as a key to protect sensitive data. Arrange for such
+         * a key to be random, which is likely to resist in decryption or
+         * verification errors. This is better than filling the buffer with


In commit "Implement psa_key_agreement_raw_shared_secret":

s/resist/result

- * a key to be random, which is likely to resist in decryption or - * verification errors. This is better than filling the buffer with + * a key to be random, which is likely to result in decryption or + * verification errors. This is better than filling the buffer with

Fixed in rebase

Patater · 2019-04-17T16:43:01Z

I've completed another round of review. Just one typo found.

Patater · 2019-04-17T16:44:26Z

Note that Windows tests are failing until the latest development is again merged into the API branch.

Refactor: split psa_key_agreement_raw_internal out of psa_key_agreement_internal, and call it from psa_key_agreement_raw_shared_secret as well.

Separate test functions for raw key agreement and key agreement with KDF.

Allow either the key derivation step or the key agreement step to fail. These tests should be split into three groups: key derivation setup tests with an algorithm that includes a key agreement step, and multipart key agreement failure tests, and raw key agreement failure tests.

Since the format change for EC public key import from SubjectPublicKeyInfo to the ECPoint content, it is no longer possible to import a key with metadata marking it as ECDH-only. This test was converted systematically but now no longer has any purpose since the public key is now like any other public key.

Change test cases with test data for raw agreement to this new test function.

Basic coverage with one algorithm only and a restricted choice of output lengths.

This gives a little more room to encode key agreement algorithms, while keeping enough space for key derivation algorithms. This doesn't affect any of the already-defined algorithms.

One was obsolete. Reword the other two to avoid the magic word that our CI rejects.

Fix logic error that clang helpfully points out

uintN_t is not a standard type for a bitfield, as armcc points out.

Simplify the logic inside a few case statements. This removes unreachable break statements.

…pi-beta2-merge-development

gilles-peskine-arm · 2019-04-18T07:49:54Z

I added a new merge commit to update to current development, so the CI should pass now.

Patater

LGTM

Patater · 2019-04-18T09:48:06Z

Looks like there are some build failures in CI, but these are expected as we haven't fixed MBEDTLS_USE_PSA_CRYPTO yet.

In file included from /var/lib/build/include/psa/crypto.h:62:0,
                 from /var/lib/build/include/mbedtls/pk.h:49,
                 from /var/lib/build/include/mbedtls/ssl_ciphersuites.h:33,
                 from /var/lib/build/include/mbedtls/ssl.h:36,
                 from /var/lib/build/include/mbedtls/debug.h:33,
                 from /var/lib/build/library/ssl_cli.c:38:
/var/lib/build/library/ssl_cli.c: In function 'ssl_write_client_key_exchange':
/var/lib/build/include/psa/crypto_values.h:1381:49: error: called object is not a function or function pointer
 #define PSA_ALG_ECDH                            ((psa_algorithm_t)0x30200000)
                                                 ^
/var/lib/build/library/ssl_cli.c:3145:35: note: in expansion of macro 'PSA_ALG_ECDH'
                                   PSA_ALG_ECDH( PSA_ALG_SELECT_RAW ) );
                                   ^
/var/lib/build/include/psa/crypto_values.h:1381:49: error: called object is not a function or function pointer
 #define PSA_ALG_ECDH                            ((psa_algorithm_t)0x30200000)
                                                 ^
/var/lib/build/library/ssl_cli.c:3185:37: note: in expansion of macro 'PSA_ALG_ECDH'
                                     PSA_ALG_ECDH( PSA_ALG_SELECT_RAW ) );
                                     ^
/var/lib/build/library/ssl_cli.c:3183:37: error: passing argument 3 of 'psa_key_agreement' makes integer from pointer without a cast [-Werror=int-conversion]
                                     handshake->ecdh_psa_peerkey,
                                     ^
In file included from /var/lib/build/include/mbedtls/pk.h:49:0,
                 from /var/lib/build/include/mbedtls/ssl_ciphersuites.h:33,
                 from /var/lib/build/include/mbedtls/ssl.h:36,
                 from /var/lib/build/include/mbedtls/debug.h:33,
                 from /var/lib/build/library/ssl_cli.c:38:
/var/lib/build/include/psa/crypto.h:3233:14: note: expected 'psa_key_handle_t {aka short unsigned int}' but argument is of type 'unsigned char *'
 psa_status_t psa_key_agreement(psa_crypto_generator_t *generator,
              ^
/var/lib/build/library/ssl_cli.c:3184:37: error: passing argument 4 of 'psa_key_agreement' makes pointer from integer without a cast [-Werror=int-conversion]
                                     handshake->ecdh_psa_peerkey_len,
                                     ^
In file included from /var/lib/build/include/mbedtls/pk.h:49:0,
                 from /var/lib/build/include/mbedtls/ssl_ciphersuites.h:33,
                 from /var/lib/build/include/mbedtls/ssl.h:36,
                 from /var/lib/build/include/mbedtls/debug.h:33,
                 from /var/lib/build/library/ssl_cli.c:38:
/var/lib/build/include/psa/crypto.h:3233:14: note: expected 'const uint8_t * {aka const unsigned char *}' but argument is of type 'size_t {aka long unsigned int}'
 psa_status_t psa_key_agreement(psa_crypto_generator_t *generator,
              ^
cc1: all warnings being treated as errors
library/CMakeFiles/mbedtls.dir/build.make:158: recipe for target 'library/CMakeFiles/mbedtls.dir/ssl_cli.c.o' failed
make[2]: *** [library/CMakeFiles/mbedtls.dir/ssl_cli.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:253: recipe for target 'library/CMakeFiles/mbedtls.dir/all' failed
make[1]: *** [library/CMakeFiles/mbedtls.dir/all] Error 2
make: *** [all] Error 2
Makefile:138: recipe for target 'all' failed
^^^^test_use_psa_crypto_full_cmake_asan: build: cmake, full config + MBEDTLS_USE_PSA_CRYPTO, ASan: command make -> 2^^^^

gilles-peskine-arm · 2019-04-18T09:54:13Z

Oh right, yes, MBEDTLS_USE_PSA_CRYPTO is not expected to build. Everything else should pass.

gilles-peskine-arm and others added 30 commits February 19, 2019 19:45

Merge pull request ARMmbed#53 from gilles-peskine-arm/storage_spec-in…

3971c10

…itial Specification of how keys and the random seed are stored

Fix ChangeLog entry to correct release version

e254f85

Merge pull request ARMmbed#32 from itayzafrir/ipc-support-64-bit-key-ids

97eaea9

Prepare support for 64 bit key ids under SPM

Initialize PSA Crypto operation contexts

fe96fbe

It is now required to initialize PSA Crypto operation contexts before calling psa_*_setup(). Otherwise, one gets a PSA_ERROR_BAD_STATE error.

psa: example: Initialize operation contexts

b281f74

Add missing initializers to PSA Crypto example. Operation contexts must be initialized before calling psa_*_setup().

Initialize PSA Crypto operation contexts

3497323

It is now required to initialize PSA Crypto operation contexts before calling psa_*_setup(). Otherwise, one gets a PSA_ERROR_BAD_STATE error.

Merge pull request ARMmbed#62 from gilles-peskine-arm/psa-stream_ciph…

5483461

…er_update_test Improve multipart cipher tests: 2 blocks, CTR

Move key id validity check into its own function

c8569bc

Merge pull request ARMmbed#59 from gilles-peskine-arm/psa-its-64_bit_…

72f40c6

…internal_key_id Support key file IDs encoding the key owner

psa: Be compatible with deprecated constants

93e2111

In case the new constants aren't available yet in Mbed TLS, continue to use the deprecated constants if they are available.

Update the encoding of owners in key file identifiers

11eca71

Switch to the terminology "key file identifier", as has been done in the code. The owner uid is now in the upper 32 bits of the key file identifier, which facilitates namespacing.

psa: Disallow use of invalid MAC contexts

252ef28

Ensure that when doing MAC operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

psa: Disallow use of invalid cipher contexts

ab43997

Ensure that when doing cipher operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

psa: Disallow use of invalid hash contexts

a0f625a

If a hash context has not been set up, fail with PSA_ERROR_BAD_STATE as documented in crypto.h and the PSA Crypto specification.

psa: Disallow repeated setup

36ee5d0

Calling psa_*_setup() twice on a MAC, cipher, or hash context should result in a PSA_ERROR_BAD_STATE error because the operation has already been set up. Fixes ARMmbed#10

Merge pull request ARMmbed#65 from Patater/deprecated-constants-compat

c8ed00a

psa: Be compatible with deprecated constants

Merge pull request ARMmbed#64 from gilles-peskine-arm/storage_spec-ke…

1ac5e6a

…y_file_id Update the architecture document regarding key file identifiers

psa: Add backwards compatible error codes

5e6d24c

Add deprecated error codes to help transition between the previous version of the PSA Crypto specification and the current one.

Merge pull request ARMmbed#67 from Patater/deprecated-psa-errors

c37fff9

psa: Add backwards compatible error codes

Fix typo in documentation

13cf3ec

Merge pull request ARMmbed#58 from Patater/disallow-invalid-context

bf61ca7

Disallow use of invalid contexts

Add test case for ecdh_calc_secret

552563b

Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, then mbedtls_ecdh_calc_secret.

Add test case for ecdh_get_params with mismatching group

c4dff06

Add a test case for doing an ECDH calculation by calling mbedtls_ecdh_get_params on both keys, with keys belonging to different groups. This should fail, but currently passes.

yanesca previously approved these changes Apr 17, 2019

View reviewed changes

gilles-peskine-arm dismissed yanesca’s stale review via 91824b5 April 17, 2019 13:48

gilles-peskine-arm force-pushed the psa-api-beta2-merge-development branch from 5f2ff84 to 91824b5 Compare April 17, 2019 13:48

Patater reviewed Apr 17, 2019

View reviewed changes

gilles-peskine-arm added 13 commits April 18, 2019 09:42

Implement psa_key_agreement_raw_shared_secret

0216fe1

Refactor: split psa_key_agreement_raw_internal out of psa_key_agreement_internal, and call it from psa_key_agreement_raw_shared_secret as well.

Add exercise_key for raw key agreement

2e46e9c

Update key agreement policy tests for the new derivation API

04ee2d2

Separate test functions for raw key agreement and key agreement with KDF.

New test function for raw agreement

f0cba73

Change test cases with test data for raw agreement to this new test function.

Test multipart key agreement with ECDH+HKDF

f8a9d94

Basic coverage with one algorithm only and a restricted choice of output lengths.

Algorithm encoding: move two bits from derivation to agreement

a52460c

This gives a little more room to encode key agreement algorithms, while keeping enough space for key derivation algorithms. This doesn't affect any of the already-defined algorithms.

psa_constant_names: support key agreement algorithms

882e57e

Remove "TODO" comments

c88644d

One was obsolete. Reword the other two to avoid the magic word that our CI rejects.

fixup! Key derivation by small input steps: proof-of-concept

ab4b201

Fix logic error that clang helpfully points out

Use unsigned int for bitfields

22c5151

uintN_t is not a standard type for a bitfield, as armcc points out.

fixup! Key derivation by small input steps: proof-of-concept

2b522db

Simplify the logic inside a few case statements. This removes unreachable break statements.

gilles-peskine-arm force-pushed the psa-api-beta2-merge-development branch from 91824b5 to 2b522db Compare April 18, 2019 07:42

Merge remote-tracking branch 'upstream-crypto/development' into psa-a…

a780f24

…pi-beta2-merge-development

Patater approved these changes Apr 18, 2019

View reviewed changes

yanesca approved these changes Apr 18, 2019

View reviewed changes

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Apr 18, 2019

Patater merged commit c69af20 into ARMmbed:psa-api-1.0-beta Apr 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merge development into API spec branch #92

Merge development into API spec branch #92

Uh oh!

gilles-peskine-arm commented Apr 11, 2019 •

edited by Patater

Loading

Uh oh!

Patater commented Apr 15, 2019

Uh oh!

yanesca left a comment

Uh oh!

yanesca Apr 17, 2019

Uh oh!

gilles-peskine-arm Apr 17, 2019

Uh oh!

gilles-peskine-arm commented Apr 17, 2019

Uh oh!

Patater Apr 17, 2019 •

edited

Loading

Uh oh!

gilles-peskine-arm Apr 18, 2019

Uh oh!

Patater commented Apr 17, 2019

Uh oh!

Patater commented Apr 17, 2019

Uh oh!

gilles-peskine-arm commented Apr 18, 2019

Uh oh!

Patater left a comment

Uh oh!

Patater commented Apr 18, 2019 •

edited

Loading

Uh oh!

gilles-peskine-arm commented Apr 18, 2019

Uh oh!

Uh oh!

Merge development into API spec branch #92

Merge development into API spec branch #92

Uh oh!

Conversation

gilles-peskine-arm commented Apr 11, 2019 • edited by Patater Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Apr 15, 2019

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

yanesca Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Apr 17, 2019

Uh oh!

Patater Apr 17, 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!

gilles-peskine-arm Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Patater commented Apr 17, 2019

Uh oh!

Patater commented Apr 17, 2019

Uh oh!

gilles-peskine-arm commented Apr 18, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

gilles-peskine-arm commented Apr 18, 2019

Uh oh!

Uh oh!

gilles-peskine-arm commented Apr 11, 2019 •

edited by Patater

Loading

Patater Apr 17, 2019 •

edited

Loading

Patater commented Apr 18, 2019 •

edited

Loading