Change MbedTLS configuration to use the Mbed OS configuration system #9669

dreemkiller · 2019-02-11T22:03:25Z

Description

Change the way that MbedTLS is configured in Mbed OS.
Previously, to configure MbedTLS developers had to edit a header file.
After this change, developers can use the MBed OS configuration system for most configuration options.
Developers still have the option of creating a header file, which they will need to reference in their mbed_app.json file.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

This PR changes the way that MbedTLS is configured on Mbed OS. Users can now choose the TLS ciphersuites that they want supported by adding entries in their mbed_app.json file. Users who have existing header files that they currently use to configure Mbed TLS can continue to do so by adding an "mbedtls.app-config-file" entry in their mbed_app.json file.

When configuring MbedTLS on Mbed OS, developers should add the Mbed TLS ciphersuites that the device will need to support in the target_overrides section of their mbed_app.json file. Thus, to add the ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 ciphersuite, they should include:

"mbedtls.ecdhe-ecdsa-with-aes-128-gcm-sha256": 1

in their target_overrides section. Developers should include as many ciphersuites as they deem necessary, keeping in mind that the more they add, the larger the binary footprint of MbedTLS will be on their device.

If developers have an existing configuration header file for MbedTLS that they currently use by defining MBEDTLS_USER_CONFIG_FILE, they can include that by including the path to that file in the "mbedtls.app-config-file" entry in their mbed_app.json file.

ciarmcom · 2019-02-12T00:00:38Z

@dreemkiller, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-tls please review.

cmonr · 2019-02-12T00:21:38Z

Removed some review groups I don't quite thing should've been included (@ciarmcom is having a day).

cmonr · 2019-02-12T04:13:53Z

Reviweers, appologies for the the overly... insistent, reviewbot. There's a newly discovered issue with the script but because I misplaced my credentials to the server running the bot, actions to correct the behavior will have to wait until @adbridge is up.

hasnainvirk · 2019-02-12T07:12:15Z

features/mbedtls/mbed_lib.json

+    "config": {
+        "MBED_CLOUD_CLIENT": {
+            "help": "Include support for ciphersuites required by the Mbed Cloud Client",
+            "value": 1


Turning on the cipher-suites by default for cloud client applications would mean that all other applications who do not require these cipher-suites must explicitly disable these configs in their mbed_app.json.

Probably, by default nothing should be turned on. On-demand application can choose to turn on features as required.

I see your point to a degree. However, if an developer does not change the default configuration, and does not use any TLS features, the linker should optimize all of MbedTLS out anyway, correct?
I've tried to leave the default settings as close as possible to the current default settings to cause as little disruption to developers as possible.

I've tried to leave the default settings as close as possible to the current default settings to cause as little disruption to developers as possible.

What are the differences ? Could you detail the breaking change made to the default configuration ?

@dreemkiller For example LoRaWAN will need AES and CMAC from MbedTLS. However it doesn't require a collection of ciphersuites which are necessary for CLOUD client applications. If we take your current proposal as it is at the moment, a LoRaWAN developer has to set MBED_CLOUD_CLIENT = 0 for example. This step wouldn't be needed if we let the appropriate application to choose for itself, i.e., the CLOUD_CLIENT application could set the MBED_CLOUD_CLINET = 1 on demand. Hope you understand what I mean. We can take this offline if you wish.

Thank you, I will take a look at that and try to resolve your problems.

@hasnainvirk Thanks so much for this use-case.

From what I can tell, the mbedtls_lora_config.h file is not a complete specification of what you need, but just a subset. You were relying on the Mbed TLS configuration file to provide the rest.
This is different from what I expected, so I am very glad you brought this up.

I've submitted a change that takes this into account, and I've compiled your example app with no issues (it required removing the MBEDTLS_USER_CONFIG define from the mbed_app.json file and replacing it with: "mbedtls.app-config-file": ""mbedtls_lora_config.h"" in the "*" target_overrides).

Since I don't have a test set up for that application, I was unable to run it, but it did compile. Could you take a look to ensure that what I've done is consistent with your expectations?

@dreemkiller Thanks. I will take your PR and test, and then update you asap.

@dreemkiller I have tested your PR with our setup and everything works fine.
However, I am seeing an increase in flash footprint by 1K (1048 bytes precisely).
Compiler used is GCC. Maybe its just coding overhead. If anything comes into your head other than coding overhead, please let me know.

Numbers with your PR: Elf2Bin: mbed-os-example-lorawan | Module | .text | .data | .bss | |----------------------------|---------------|----------|-----------| | [fill] | 220(-2) | 4(+0) | 2106(+0) | | [lib]/c.a | 28951(+0) | 2472(+0) | 89(+0) | | [lib]/gcc.a | 3232(+0) | 0(+0) | 0(+0) | | [lib]/m.a | 652(+0) | 0(+0) | 0(+0) | | [lib]/misc | 248(+0) | 8(+0) | 28(+0) | | [lib]/nosys.a | 32(+0) | 0(+0) | 0(+0) | | main.o | 1828(-4) | 0(+0) | 3892(+0) | | mbed-lora-radio-drv/SX1272 | 383(+0) | 0(+0) | 0(+0) | | mbed-lora-radio-drv/SX1276 | 8381(+0) | 0(+0) | 0(+0) | | mbed-os/cmsis | 1033(+0) | 0(+0) | 0(+0) | | mbed-os/components | 290(+0) | 0(+0) | 0(+0) | | mbed-os/drivers | 1532(+0) | 0(+0) | 52(+0) | | mbed-os/events | 1490(+0) | 0(+0) | 0(+0) | | mbed-os/features | 32252(+1046) | 4(+0) | 184(+0) | | mbed-os/hal | 1878(+0) | 8(+0) | 130(+0) | | mbed-os/platform | 4310(+0) | 260(+0) | 293(+0) | | mbed-os/rtos | 11166(+0) | 168(+0) | 5973(+0) | | mbed-os/targets | 13886(+0) | 12(+0) | 1029(+0) | | trace_helper.o | 2(+0) | 0(+0) | 0(+0) | | Subtotals | 111766(+1040) | 2936(+0) | 13776(+0) | Total Static RAM memory (data + bss): 16712(+0) bytes Total Flash memory (text + data): 114702(+1040) bytes Numbers with Mbed OS master Elf2Bin: mbed-os-example-lorawan | Module | .text | .data | .bss | |----------------------------|---------------|----------|-----------| | [fill] | 218(-2) | 4(+0) | 2106(+0) | | [lib]/c.a | 28951(+0) | 2472(+0) | 89(+0) | | [lib]/gcc.a | 3232(+0) | 0(+0) | 0(+0) | | [lib]/m.a | 652(+0) | 0(+0) | 0(+0) | | [lib]/misc | 248(+0) | 8(+0) | 28(+0) | | [lib]/nosys.a | 32(+0) | 0(+0) | 0(+0) | | main.o | 1828(+0) | 0(+0) | 3892(+0) | | mbed-lora-radio-drv/SX1272 | 383(+0) | 0(+0) | 0(+0) | | mbed-lora-radio-drv/SX1276 | 8381(+0) | 0(+0) | 0(+0) | | mbed-os/cmsis | 1033(+0) | 0(+0) | 0(+0) | | mbed-os/components | 290(+0) | 0(+0) | 0(+0) | | mbed-os/drivers | 1532(+0) | 0(+0) | 52(+0) | | mbed-os/events | 1490(+0) | 0(+0) | 0(+0) | | mbed-os/features | 31206(-1046) | 4(+0) | 184(+0) | | mbed-os/hal | 1878(+0) | 8(+0) | 130(+0) | | mbed-os/platform | 4310(+0) | 260(+0) | 293(+0) | | mbed-os/rtos | 11166(+0) | 168(+0) | 5973(+0) | | mbed-os/targets | 13886(+0) | 12(+0) | 1029(+0) | | trace_helper.o | 2(+0) | 0(+0) | 0(+0) | | Subtotals | 110718(-1048) | 2936(+0) | 13776(+0) | Total Static RAM memory (data + bss): 16712(+0) bytes Total Flash memory (text + data): 113654(-1048) bytes ``

Very interesting. Thanks for the feedback.

Here's the diagnosis: I coded the app-config-file option to be included first, before the rest of the defines. This meant that any undefs in the file might get overridden by the rest of the configuration options, including the default ones.

I have now moved the app-config-file option to be last, so it has "supremacy" and can override any of the other options. The size is now smaller than the previous settings (there are a few options that were previously default that have been optimized out when users don't select things).

AnttiKauppila · 2019-02-12T07:17:59Z

@yogpan01 Can you check this from client perspective?

hasnainvirk

Just a minor comment. It's better to rebase on top of master rather than merging master into your work. It will give a cleaner baseline if you rebase.

AnttiKauppila

LGTM +1

0xc0170

Add Release notes section pls, following https://os.mbed.com/docs/mbed-os/v5.11/contributing/workflow.html#functionality-change

mbed-http.lib

0xc0170 · 2019-02-12T08:16:47Z

TESTS/mbedtls/config/rsa_test.h

@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2013-2018, ARM Limited, All Rights Reserved


New files should have 2019 only here (or 2018 if work started the previous year)

The work was started last year, but finished up this year. Should I change it or leave it as is?

TESTS/mbedtls/config/connection_test.cpp

SeppoTakalo

I like the idea of this.

I'm just proposing that we do not duplicate the effort of connectivity tests.

TESTS/mbedtls/config/connection_test.cpp

features/mbedtls/mbed_lib.json

mbed-http.lib

dreemkiller · 2019-02-21T17:46:58Z

@RonEld You are correct, I have not addressed #9669 (comment) yet. Stay tuned.
Regarding conflicts between a chosen ciphersuite and a custom header file, that's certainly a risk with this system, but it's not new. The existing system could result in the same problem.
In the future when we add more functionality, the need for custom header files will be reduced.
In general, as before, check_config.h is still our bulwark against conflicting configurations.

Release notes added. Please re-review.

…othing's enabled

… platforms

…fig.h with the output

…orms

… with default configuration

andresag01

Hi @dreemkiller,

Thanks for looking at improving the Mbed TLS integration with Mbed OS. Changes like this are much needed to make the Mbed TLS configuration options more approachable and easier to use for developers.

In general, I like the idea of the work proposed in this PR, but I have a few comments/questions:

There are many new files in this PR that add test code. As far as I can tell, most of this is to check that the new configuration is working. For example, if I enable ciphersuite X using the new JSON configuration options, there is a test for X to ensure that the relevant functions are actually included in the compiled binary. Is this correct? If not, could you please explain the purpose of this code? If my explanation was correct, could you please explain why do we need such verbose tests? I mean, Mbed TLS has a few ways using well defined macros to test that these features are available in the compiled binary. Why not use those? Do we really need to duplicate most of the AES self test code just for checking this?
There are multiple mechanisms in Mbed TLS to change the library configuration. (1) The first is to use the MBEDTLS_USER_CONFIG_FILE that you are relying on here. It allows you to modify the default config the library has. (2) The other way is to use the MBEDTLS_CONFIG_FILE macro that allows users to completely replace the default config with their own. After the changes in this PR, one could use an option in the json config (e.g. mbedtls.ecdhe-ecdsa-with-aes-128-gcm-sha256) and also set (2). But I think that (1) will have no effect and (2) will be used exclusively. Perhaps we could add a compile-time check to ensure that (1) and (2) are not set at the same time?
I think in a previous conversation in this PR there was a conversation about conflicting configurations from different components of the application. I agree that this is a pre-existing issue and I do not expect this problem to be solved here. However, could you please comment regarding how you think the changes in this PR help us get closer to addressing that problem? My concern is that we approve and merge these changes. Then, when we want to fix the other issue, we realise that the new changes we have to apply conflict with the mechanism introduced in this PR and then we have to deprecate or add a breaking change.
I left a few other comments as I read through some of the code. Please note that I did not conduct a very thorough check, just tried to look at the design of the new feature.

andresag01 · 2019-02-24T21:27:44Z