Skip to content

Merge development-psa commit 409fdae into development #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

AndrzejKurek
Copy link
Contributor

@AndrzejKurek AndrzejKurek commented Feb 1, 2019

This PR is superseded by #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

@AndrzejKurek AndrzejKurek added enhancement New feature or request 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. needs: design review DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. needs: ci Needs a passing full CI run labels Feb 1, 2019
@Patater
Copy link
Contributor

Patater commented Feb 1, 2019

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

@AndrzejKurek
Copy link
Contributor Author

@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
Copy link
Contributor

mpg commented Feb 4, 2019

@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 Merge updated development-psa into development Mbed-TLS/mbedtls#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 Patater deleted the development-merged-dev-psa-409fdae branch March 6, 2019 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. enhancement New feature or request needs: ci Needs a passing full CI run needs: design review 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants