Merge development-psa commit `409fdae` into development #40

Closed

AndrzejKurek wants to merge 1 commit from development-merged-dev-psa-409fdae into development

AndrzejKurek commented

2019-02-01 04:12:58 -06:00

(Migrated from github.com)

This PR is superseded by https://github.com/ARMmbed/mbed-crypto/pull/42.
This PR merges development_psa from mbedtls repository (commit 409fdae) into development. I have performed the following tests:

make test (passed);
ssl-opt.sh (passed).

Following conflicts have been resolved:

CMakeFiles.txt: used PSA - added "include_directories(include/)" and "include_directories(library/)";
include/mbedtls/check_config.h: used PSA - added crypto_spm, crypto_storage_c, crypto_storage_file_c and crypto_storage_its_c prerequisites;
cipher.h:
- used mbedtls: formatting style of mbedtls_cipher_set_iv;
- used mbedtls: documentation of mbedtls_cipher_update_ad;
- used mbedtls: documentation of mbedtls_cipher_auth_encrypt;
- used mbedtls: documentation of mbedtls_cipher_auth_decrypt;
pk.h: used mbedtls - documentation of mbedtls_pk_free;
library/version_features.c: used PSA - definition of additional features like psa_crypto_storage_c, storage_file_c, etc.;
scripts/config.pl: used PSA - added "psa_crypto_spm", "psa_has_its_io", "psa_crypto_storage_its_c", "use_psa_crypto" to @excluded;
Readme.md - used PSA - used the bare version of readme that PSA repository has (cut off at "the test suites need Perl [...]"). Not sure about it though.
include/mbedtls/config.h - used PSA - used the less elaborate descriptions of PSA defines from PSA repository, with PSA_CRYPTO_C defined on default and with added new storage-related defines.
library/ssl_srv.c: used PSA - used mbedtls - MBEDTLS_SSL_DEBUG_ECDH instead of DEBUG_ECP in line ~3925;
library/pk.c: used mbedtls - parameter validation in mbedtls_pk_check_pair (makes checks for pub == NULL and pr == NULL conditional);
library/rsa.c:
- used both - rsa_rsaes_pkcs1_v15_encrypt: additional null checks from PSA and mode & padding checks from mbedtls;
- used mbedtls - mbedtls_rsa_rsaes_pkcs1_v15_decrypt with mbedtls logic of handling padding - not sure about that;
library/pk_wrap.c: used psa - handling public key format in the new way (even though the PR will have to be reworked), since it has already been merged in the crypto repository;
tests/suites/helpers.function - used both - parameter validation handling via new test macros and the new PSA_ASSERT and other definitions.
tests/scripts/all.sh - used both - removed a check for crypto/Makefile (crypto submodule initialization), removed tests of crypto submodule, added parameter validation tests according to the even newer development head - unfortunately the commit that is merged into development-psa is a commit just before adding component checks to all.sh, but it's already present in mbed-crypto repository, so it is added here as a component check, but in development-psa - it's the old way.
library/CMakeLists.txt - used both - PSA version of the file with mbedTLS SOVERSION's.
library/cipher.c:
- return parentheses added at line ~416 - used mbedtls
- mbedtls_cipher_set_iv - used mbedtls - added parameter validation and removed the "ctx->iv_size = 0;" setting if ( iv == NULL && iv_len == 0 );
- mbedtls_cipher_crypt - used mbedtls - added parameter validation;
- mbedtls_cipher_auth_encrypt - used mbedtls - added parameter validation;
- mbedtls_cipher_auth_decrypt - used mbedtls - added parameter validation;
- mbedtls_cipher_setkey - used mbedtls - added parameter validation;

Manual changes:

removed .gitmodules file and crypto submodule
removed submodule option from Makefile, programs/Makefile, library/Makefile, tests/Makefile, and CMakeLists.txt

**This PR is superseded by https://github.com/ARMmbed/mbed-crypto/pull/42.** This PR merges development_psa from mbedtls repository (commit 409fdae) into development. I have performed the following tests: - `make test` (passed); - `ssl-opt.sh` (passed). ### Following conflicts have been resolved: - CMakeFiles.txt: used PSA - added "include_directories(include/)" and "include_directories(library/)"; - include/mbedtls/check_config.h: used PSA - added crypto_spm, crypto_storage_c, crypto_storage_file_c and crypto_storage_its_c prerequisites; - cipher.h: - used mbedtls: formatting style of mbedtls_cipher_set_iv; - used mbedtls: documentation of mbedtls_cipher_update_ad; - used mbedtls: documentation of mbedtls_cipher_auth_encrypt; - used mbedtls: documentation of mbedtls_cipher_auth_decrypt; - pk.h: used mbedtls - documentation of mbedtls_pk_free; - library/version_features.c: used PSA - definition of additional features like psa_crypto_storage_c, storage_file_c, etc.; - scripts/config.pl: used PSA - added "psa_crypto_spm", "psa_has_its_io", "psa_crypto_storage_its_c", "use_psa_crypto" to @excluded; - Readme.md - used PSA - used the bare version of readme that PSA repository has (cut off at "the test suites need Perl [...]"). Not sure about it though. - include/mbedtls/config.h - used PSA - used the less elaborate descriptions of PSA defines from PSA repository, with PSA_CRYPTO_C defined on default and with added new storage-related defines. - library/ssl_srv.c: used PSA - used mbedtls - MBEDTLS_SSL_DEBUG_ECDH instead of DEBUG_ECP in line ~3925; - library/pk.c: used mbedtls - parameter validation in mbedtls_pk_check_pair (makes checks for pub == NULL and pr == NULL conditional); - library/rsa.c: - used both - rsa_rsaes_pkcs1_v15_encrypt: additional null checks from PSA and mode & padding checks from mbedtls; - used mbedtls - mbedtls_rsa_rsaes_pkcs1_v15_decrypt with mbedtls logic of handling padding - not sure about that; - library/pk_wrap.c: used psa - handling public key format in the new way (even though the PR will have to be reworked), since it has already been merged in the crypto repository; - tests/suites/helpers.function - used both - parameter validation handling via new test macros and the new PSA_ASSERT and other definitions. - tests/scripts/all.sh - used both - removed a check for crypto/Makefile (crypto submodule initialization), removed tests of crypto submodule, added parameter validation tests according to the even newer development head - unfortunately the commit that is merged into development-psa is a commit just before adding component checks to all.sh, but it's already present in mbed-crypto repository, so it is added here as a component check, but in development-psa - it's the old way. - library/CMakeLists.txt - used both - PSA version of the file with mbedTLS SOVERSION's. - library/cipher.c: - return parentheses added at line ~416 - used mbedtls - mbedtls_cipher_set_iv - used mbedtls - added parameter validation and removed the "ctx->iv_size = 0;" setting if ( iv == NULL && iv_len == 0 ); - mbedtls_cipher_crypt - used mbedtls - added parameter validation; - mbedtls_cipher_auth_encrypt - used mbedtls - added parameter validation; - mbedtls_cipher_auth_decrypt - used mbedtls - added parameter validation; - mbedtls_cipher_setkey - used mbedtls - added parameter validation; ### Manual changes: - removed .gitmodules file and crypto submodule - removed submodule option from Makefile, programs/Makefile, library/Makefile, tests/Makefile, and CMakeLists.txt

mpg (Migrated from github.com) reviewed

2019-02-01 04:12:58 -06:00

hanno-becker (Migrated from github.com) reviewed

2019-02-01 04:12:58 -06:00

Patater commented

2019-02-01 04:51:07 -06:00

(Migrated from github.com)

Could we instead merge from the mbedtls/development branch after mbedtls/development-psa and mbedtls/development have been merged?

AndrzejKurek commented

2019-02-01 04:56:59 -06:00

(Migrated from github.com)

@Patater - We can do it this way, sure. I just wanted to create space for discussion about reviews and have a branch in mbed-crypto as a new point to be referenced in development_psa from mbedtls repository.

mpg commented

2019-02-04 03:17:02 -06:00

(Migrated from github.com)

@Patater I think the issue is that if we just merge mbedtls/development-psa and mbedtls-development with making any change to the submodule, our tests with USE_CRYPTO_SUBMODULE will fail. I can see several ways out of this:

Temporarily ignore those failures on the TLS side, wait for Crypto to merge back from mbedtls/development and then update the submodule and re-run the CI to make sure the failures disappear. I don't like it because it means that mbedtls/development would be temporarily broken, I'm only mentioning it for the sake of completion.
Have a "sibling PR" to our "Big Merge" PR that only cherry-picks the necessary commits from mbedtls/development and/or mbedtls/development-psa to make the tests pass on the TLS side after the Big Merge (and updating the submodule)
Have a "sibling PR" to our "Big Merge" PR that merges back the result of the Big Merge so that, after updating our submodule to that, the tests pass on the TLS side. Unless I misunderstood, that's the approach taken by @AndrzejKurek in the present PR. (Perhaps the description could be clearer, but you'll notice that commit 409fdae is present in https://github.com/ARMmbed/mbedtls/pull/2395 but not in mbedtls/development-psa.)

I think option 3 is very close to your suggestion "merge from the mbedtls/development branch after mbedtls/development-psa and mbedtls/development have been merged" with only a slight different in timing: instead of after, merge the result of merging mbedtls/development and mbedtls/development-psa to mbed-crypto/development at the same time that it is merged (or pushed) to mbedtls/development.

@Patater I think the issue is that if we just merge `mbedtls/development-psa` and `mbedtls-development` with making any change to the submodule, our tests with `USE_CRYPTO_SUBMODULE` will fail. I can see several ways out of this: 1. Temporarily ignore those failures on the TLS side, wait for Crypto to merge back from `mbedtls/development` and then update the submodule and re-run the CI to make sure the failures disappear. I don't like it because it means that `mbedtls/development` would be temporarily broken, I'm only mentioning it for the sake of completion. 2. Have a "sibling PR" to our "Big Merge" PR that only cherry-picks the necessary commits from `mbedtls/development` and/or `mbedtls/development-psa` to make the tests pass on the TLS side after the Big Merge (and updating the submodule) 3. Have a "sibling PR" to our "Big Merge" PR that merges back the result of the Big Merge so that, after updating our submodule to that, the tests pass on the TLS side. Unless I misunderstood, that's the approach taken by @AndrzejKurek in the present PR. (Perhaps the description could be clearer, but you'll notice that commit 409fdae is present in https://github.com/ARMmbed/mbedtls/pull/2395 but not in `mbedtls/development-psa`.) I think option 3 is very close to your suggestion "merge from the mbedtls/development branch after mbedtls/development-psa and mbedtls/development have been merged" with only a slight different in timing: instead of after, merge the result of merging mbedtls/development and mbedtls/development-psa to mbed-crypto/development at the same time that it is merged (or pushed) to mbedtls/development.