Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper #14815

LDong-Arm · 2021-06-22T15:20:52Z

Summary of changes

This PR includes two main changes:

Enable PSA tests on Mbed OS PSA targets (K64F and K66F):

When using the mbed test command to build and run tests, some targets have additional configurations/overrides defined in tools/test_configs/:
- target_configs.json lists which targets support which configs.
- config_paths.json maps the name of each config to the JSON file to
  use.
By default, only default_test_configuration from target_configs.json gets used (i.e. automatically picked up by Mbed CLI 1) when building and running tests. Others listed in test_configuration need to be switched via --test-config <NAME>.

Enable Experimental API in the default configurations on K64F and K66F in order to test Mbed OS PSA. Any existing configs are kept, which is why HeapBlockDeviceAndEthernetAndExperimental.json is created for K64F.
Fix missing PSA Crypto initialization in TLSSocketWrapper:

With Experimental API/PSA enabled as above, TLSSocket does not work anymore and its test connectivity/netsocket/tests/TESTS/netsocket/tls fails. This is because Mbed TLS uses PSA Crypto for cryptographic operations when available, but PSA Crypto needs to be initialized first (and it currently isn't).

To fix this, we call psa_crypto_init() in TLSSocketWrapper when MBEDTLS_USE_PSA_CRYPTO.

Impact of changes

Migration actions required

Documentation

None

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual testing of CMake support (not yet automated by CTest or run in CI):
Build:

cd connectivity/netsocket/tests/TESTS/netsocket/tls
mbedtools configure -t GCC_ARM -m K64F --mbed-os-path <path-to>/mbed-os --app-config <path-to>/mbed-os/tools/test_configs/experimental.json
cmake --build .

Run (K64F with Ethernet):

mbedhtrun -f mbed-connectivity-netsocket-tls.bin -p <port> -d <mount>

Reviewers

@ARMmbed/mbed-os-core @Patater @saheerb

LDong-Arm · 2021-06-22T15:24:09Z

@ARMmbed/mbed-os-core I didn't know about the "test_configs" mechanism of mbed test of Mbed CLI 1. So some configs are automatically used for Greentea, even without specifying them. See PR description/commit message for details.

When we add CTest support to build & run all Greentea tests with Mbed CLI 2, we need to consider how to handle this...

ciarmcom · 2021-06-22T15:30:14Z

@LDong-Arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

Patater · 2021-06-24T16:41:38Z

CI started

mbed-ci · 2021-06-24T17:30:43Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	❌

LDong-Arm · 2021-06-25T10:08:47Z

Enabling PSA on K64F causes TLSSocket->connect() to fail...

In `tools/test_configs/`, target-specific test configurations are defined in `target_configs.json` and parsed by `__init__.py`. The latter only makes use of `default_test_configuration` (default test configuration to use) and `test_configurations` (more configurations selectable via `mbed test --test-config <config>`. Anything else is ignored, including nsapi, so this commit cleans up dead entries.

When using the `mbed test` command to build and run tests, some targets have additional configurations/overrides defined in `tools/test_configs/`: * `target_configs.json` lists which targets support which configs. * `config_paths.json` maps the name of each config to the JSON file to use. By default, only `default_test_configuration` from `target_configs.json` gets used when building and running tests. Others listed in `test_configuration` need to be switched via `--test-config <NAME>`. This commit enables Experimental API in the default configurations of K64F and K66F in order to test Mbed OS PSA. Any existing configs are kept, which is why `HeapBlockDeviceAndEthernetAndExperimental.json` is created for K64F.

Some of the lines in `platform_mbed.h` only have `FEATURE_PSA` checked, which is always set for Mbed OS PSA targets but the PSA APIs are not actually available unless `FEATURE_EXPERIMENTAL_API` is also enabled. To fix this and improve readability, group all PSA-related lines and check both macros.

Pull request has been modified.

LDong-Arm · 2021-06-28T11:16:32Z

Enabling PSA on K64F causes TLSSocket->connect() to fail...

@Patater @ARMmbed/mbed-os-core This is now addressed - added psa_crypto_init() call to mbedtls_platform_init() which is called before using Mbed TLS (in our case, by Mbed OS's TLSSocket). Ready for review.

connectivity/mbedtls/platform/inc/platform_mbed.h

Patater · 2021-06-29T09:32:24Z

platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/src/mbedtls_crypto_setup.c

+
+// Required by mbedtls_platform_setup()
+
+int crypto_platform_setup(crypto_platform_ctx *unused)


Should we have an ifdef to catch if cryptocell is enabled to provide a more friendly error message than duplicate symbol definition? We can't have both PSA and cryptocell enabled, unless we chain together their initializers.

Good point, I need to check whether Mbed TLS can use CryptoCell and PSA Crypto at the same time. If it can, we need to make sure they are both initialised.

It's tricky to initialise both CryptoCell and PSA Crypto here, because any crypto hardware (e.g. CryptoCell) relies on MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT (from Mbed TLS) to add initialisation hooks, but the same macro is for overriding mbedtls_platform_setup() for PSA Crypto too. So, there's no way to check if only PSA is available or PSA and some crypto hardware both need to be initialised - we just don't have any macro or anything for this.

Introducing a new config/macro would be a breaking change - it's easy to change CryptoCell as it's in Mbed OS, but if a third-party project has its own crypto hardware integrated with the same hook, it will break. Breaking changes (without a backward compatibility mechanism) are only allowed in Mbed OS between major releases (e.g. 5 -> 6).

So I just've completely changed the approach, and simply call psa_crypto_init() in TLSSocketWrapper if MBEDTLS_USE_PSA_CRYPTO. This is what an example program of Mbed TLS does too.

Pull request has been modified.

When `MBEDTLS_USE_PSA_CRYPTO` is set, Mbed TLS uses the PSA Crypto API where possible. It is necessary to initialize PSA Crypto beforehand.

connectivity/netsocket/source/TLSSocketWrapper.cpp

Patater · 2021-06-30T09:38:01Z

CI started

mbed-ci · 2021-06-30T10:32:32Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

LDong-Arm requested review from Patater, a team and saheerb June 22, 2021 15:21

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jun 22, 2021

ciarmcom requested a review from a team June 22, 2021 15:30

ciarmcom added needs: review component: tools labels Jun 22, 2021

Patater previously approved these changes Jun 24, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jun 24, 2021

mergify bot added needs: work and removed needs: CI labels Jun 24, 2021

LDong-Arm added 3 commits June 28, 2021 10:31

LDong-Arm force-pushed the test_psa_k64f_k66f branch from 21fb7c7 to de2dd02 Compare June 28, 2021 10:52

LDong-Arm changed the title ~~Test Experimental API (PSA) on K64F/K66F~~ Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in Mbed TLS Jun 28, 2021

LDong-Arm requested a review from Patater June 28, 2021 11:13

LDong-Arm commented Jun 28, 2021

View reviewed changes

connectivity/mbedtls/platform/inc/platform_mbed.h Show resolved Hide resolved

LDong-Arm added needs: review and removed needs: work labels Jun 28, 2021

Patater previously requested changes Jun 29, 2021

View reviewed changes

mergify bot added the needs: work label Jun 29, 2021

mergify bot removed the needs: review label Jun 29, 2021

LDong-Arm force-pushed the test_psa_k64f_k66f branch from de2dd02 to 1e247c2 Compare June 29, 2021 16:45

LDong-Arm changed the title ~~Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in Mbed TLS~~ Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper Jun 29, 2021

LDong-Arm requested a review from Patater June 29, 2021 17:12

TLSSocketWrapper: Initialize PSA Crypto if used by Mbed TLS

929956d

When `MBEDTLS_USE_PSA_CRYPTO` is set, Mbed TLS uses the PSA Crypto API where possible. It is necessary to initialize PSA Crypto beforehand.

LDong-Arm force-pushed the test_psa_k64f_k66f branch from 1e247c2 to 929956d Compare June 29, 2021 17:17

Patater self-assigned this Jun 30, 2021

Patater reviewed Jun 30, 2021

View reviewed changes

connectivity/netsocket/source/TLSSocketWrapper.cpp Show resolved Hide resolved

Patater approved these changes Jun 30, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jun 30, 2021

mergify bot added ready for merge and removed needs: CI labels Jun 30, 2021

Patater merged commit 270ea5d into ARMmbed:master Jun 30, 2021

mergify bot removed the ready for merge label Jun 30, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper #14815

Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper #14815

Uh oh!

LDong-Arm commented Jun 22, 2021 •

edited

Loading

Uh oh!

LDong-Arm commented Jun 22, 2021

Uh oh!

ciarmcom commented Jun 22, 2021

Uh oh!

Patater commented Jun 24, 2021

Uh oh!

mbed-ci commented Jun 24, 2021

Uh oh!

LDong-Arm commented Jun 25, 2021

Uh oh!

LDong-Arm commented Jun 28, 2021

Uh oh!

Uh oh!

Uh oh!

Patater Jun 29, 2021

Uh oh!

LDong-Arm Jun 29, 2021 •

edited

Loading

Uh oh!

LDong-Arm Jun 29, 2021

Uh oh!

Uh oh!

Patater commented Jun 30, 2021

Uh oh!

mbed-ci commented Jun 30, 2021

Uh oh!

Uh oh!


		// Required by mbedtls_platform_setup()

		int crypto_platform_setup(crypto_platform_ctx *unused)

Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper #14815

Enable PSA tests on K64F/K66F and fix missing PSA Crypto init in TLSSocketWrapper #14815

Uh oh!

Conversation

LDong-Arm commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

LDong-Arm commented Jun 22, 2021

Uh oh!

ciarmcom commented Jun 22, 2021

Uh oh!

Patater commented Jun 24, 2021

Uh oh!

mbed-ci commented Jun 24, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

LDong-Arm commented Jun 25, 2021

Uh oh!

LDong-Arm commented Jun 28, 2021

Uh oh!

Uh oh!

Uh oh!

Patater Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Jun 29, 2021 • 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!

LDong-Arm Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Patater commented Jun 30, 2021

Uh oh!

mbed-ci commented Jun 30, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

Uh oh!

LDong-Arm commented Jun 22, 2021 •

edited

Loading

LDong-Arm Jun 29, 2021 •

edited

Loading