pkg/mbedtls: initial pkg import to use entropy module#15671
pkg/mbedtls: initial pkg import to use entropy module#15671benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
pkg/mbedtls/include/riot_config.h
Outdated
| @@ -0,0 +1,3344 @@ | |||
| /** | |||
| * \file config.h | |||
There was a problem hiding this comment.
I might be mistaking, but this files seems similar to what I did with lwipopts.h in lwIP. Does this file has to be exactly this format or can we adapt it to be more RIOT-y? The fact, that this file is 96% comments, makes me think the latter.
I also don't think that you have to be that verbose on documentation, as the documentation (hopefully) lies with mbedtls (which you could just link here).
There was a problem hiding this comment.
The file is a copy-paste from mbed TLS. For sure I could write a similar RIOT(-y) file, but for further integration of the package, I think it simplifies the development process to have a (almost) copy available. With that, a developer (i) sees immediately all available configuration options (&doc) and (ii) can easily look at diffs to the original file during version bump or the like...
There was a problem hiding this comment.
Mh from the other point of view, I might be interested as a user, which configuration options of mbedtls are actually used. Scrolling though this 3300 line file to filter out the 142 lines that are actually important seems cumbersome, especially given, that you want to exclude this file from out doxygen build. Furthermore, we might want to make options configurable from the app (like we do e.g. with lwIP...), which seems a bit more complicated when you need to comment out / comment in defines.
I also don't see the maintenance advantage you propose. If you want to see which options are available in mbedtls: go to the mbedtls documentation. If there is a version bump and config options change this will usually lead to failing compilation, giving you exactly the names of the macros that are new (and thus missing).
There was a problem hiding this comment.
From #15671 (comment)
Lets give it some more time and if no one opposes, we'll go with your proposal.
Yes, please @RIOT-OS/maintainers, can we have more thoughts and comments on this?
There was a problem hiding this comment.
(I haven't checked the code, just followed the discussion)
If we can avoid the 3300 lines of configuration code, that would be better. If it's added, there's no reason to skip it from doxygen, or any other checks.
IIUC, the goal of this file is to provide the configuration of the package. Why not link it to the module mechanism of RIOT to define flags (like MBEDTLS_SHA256_ALT) or via a Kconfig file or both (depending on the cases) ?
- for the module version:
#if IS_USED(MODULE_MBEDTLS_CIPHER_MODE_CBC)
#define MBEDTLS_CIPHER_MODE_CBC
#endif- for the Kconfig version (assuming there's a corresponding Kconfig boolean option):
#if IS_ACTIVE(CONFIG_MBEDTLS_ECP_DP_SECP192R1_ENABLED)
#define MBEDTLS_ECP_DP_SECP192R1_ENABLED
#endif|
To answer @miri64's question in this comment:
It is an mbed TLS config file which I slightly adjusted for RIOT, by en-/disabling some options. By enabling additional library features, further changes might be required. To be still mostly "copy-paste-able" I've tried to keep changes at minimum. Technically I could import & patch it. Practically, as a user, I would like the have the file directly available instead of a patch. |
| @@ -0,0 +1,73 @@ | |||
| /* | |||
There was a problem hiding this comment.
Shouldn't this be its on module so in the future people can use mbedtls without the entropy source (if they so desire)?
So pkg/mbedtls/contrib/mbedtls_entropy_source/.... This way you also might not need to define pseudo-modules.
Again: see lwIP how to implement actual modules within the contrib directory of a package.
There was a problem hiding this comment.
Good point. I have tried this in my last commit, mind to have another look?
Why do you want to have it "copy-paste-able"? Copy-paste from where? Is configuration so complicated within
|
Well, I guess it strongly depends on the point of view. A look at the amount of configuration options in that file gives you a first impression. Personally (proposing the library import here) I don't have a problem providing a specific configuration file. Still, I could think of other developers seeing this differently. Lets give it some more time and if no one opposes, we'll go with your proposal. |
01539b8 to
36bccad
Compare
aabadie
left a comment
There was a problem hiding this comment.
Went through this PR and have some comment related to the build system and other minor things.
I also think that the riot_config.h should be rewritten and not excluded from the doxygen build (since it might contain important configuration information) and the CI checks.
pkg/mbedtls/Makefile
Outdated
| @@ -0,0 +1,10 @@ | |||
| PKG_NAME=mbedtls | |||
| PKG_URL=https://github.com/ARMmbed/mbedtls.git | |||
| PKG_VERSION=2a1d9332d55d1270084232e42df08fdb08129f1b | |||
There was a problem hiding this comment.
If this commit hash correspond to a tagged version, please add it in a comment at the end of the line
pkg/mbedtls/Makefile
Outdated
|
|
||
| all: | ||
| "$(MAKE)" -C $(PKGDIRBASE)/mbedtls/library/ \ | ||
| -f $(RIOTPKG)/mbedtls/Makefile.$(PKG_NAME) |
There was a problem hiding this comment.
Makefile.mbedtls only contains a minimal setup, you could just use Makefile.base directly and pass the MODULE variable in the command line:
| -f $(RIOTPKG)/mbedtls/Makefile.$(PKG_NAME) | |
| -f $(RIOTBASE)/Makefile.base MODULE=$(PKG_NAME) |
And Makefile.mbedtls can be removed.
pkg/mbedtls/Makefile.dep
Outdated
| # include sources if hardware is available | ||
| FEATURES_OPTIONAL += periph_hwrng | ||
| ifneq (,$(filter periph_hwrng,$(FEATURES_USED))) | ||
| DEFAULT_MODULE += mbedtls_entropy_source_hwrng |
There was a problem hiding this comment.
indentation is off, it should be 2 spaces inside the ifneq block. Maybe it's a tab ?
Same comment applies in the rest of this file.
| # include sources if hardware is available | ||
| FEATURES_OPTIONAL += periph_hwrng | ||
| ifneq (,$(filter periph_hwrng,$(FEATURES_USED))) | ||
| DEFAULT_MODULE += mbedtls_entropy_source_hwrng | ||
| endif | ||
|
|
||
| FEATURES_OPTIONAL += periph_adc | ||
| ifneq (,$(filter periph_adc,$(FEATURES_USED))) | ||
| DEFAULT_MODULE += mbedtls_entropy_source_adc | ||
| endif |
There was a problem hiding this comment.
What happens if the hardware provides both ? What is the entropy source selected in this case ?
There was a problem hiding this comment.
I think those are not mutually exclusive, @PeterKietzmann ?
There was a problem hiding this comment.
Then both are used (if not manually disabled). That is actually the intent of the entropy module, to accumulate multiple entropy sources.
pkg/mbedtls/Makefile.dep
Outdated
| CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_COND=0 | ||
| CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_HEALTH_TEST=1 |
There was a problem hiding this comment.
Those 2 constants seem to be unused.
There was a problem hiding this comment.
Nope, they configure the ADC noise source which was added in #14324.
There was a problem hiding this comment.
Shouldn't this be better placed in the Makefile.include?
pkg/mbedtls/doc.txt
Outdated
| * | ||
| * ``` | ||
| * FEATURES_PROVIDED += periph_hwrng | ||
| * FEATURES_PROVIDED += entropy_source_adc_noise |
There was a problem hiding this comment.
| * FEATURES_PROVIDED += entropy_source_adc_noise | |
| * FEATURES_PROVIDED += periph_adc |
?
| */ | ||
|
|
||
| /** | ||
| * @ingroup pkg_mbedtls |
There was a problem hiding this comment.
| * @ingroup pkg_mbedtls | |
| * @ingroup pkg_mbedtls_entropy |
?
There was a problem hiding this comment.
Yes, probably a good idea to spend an own doxygen group now that we moved the entropy support to its own module.
| */ | ||
|
|
||
| /** | ||
| * @ingroup pkg_mbedtls |
There was a problem hiding this comment.
| * @ingroup pkg_mbedtls | |
| * @ingroup pkg_mbedtls_entropy |
?
pkg/mbedtls/include/sha256_alt.h
Outdated
| extern "C" { | ||
| #endif | ||
|
|
||
| #if defined (MBEDTLS_SHA256_ALT) |
There was a problem hiding this comment.
| #if defined (MBEDTLS_SHA256_ALT) | |
| #if IS_ACTIVE(MBEDTLS_SHA256_ALT) |
| ifneq (,$(filter mbedtls_entropy_source_adc,$(USEMODULE))) | ||
| ifndef CONFIG_KCONFIG_USEMODULE_ENTROPY_SOURCE_ADC_NOISE | ||
| CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_HMIN=65536 | ||
| endif | ||
| endif |
There was a problem hiding this comment.
Why don't you put this in the package Makefile.include ?
There was a problem hiding this comment.
To avoid mis-configuration of this entropy source, we set an invalid default value. Hence, a user must evaluate and configure the module according to the hardware in use and the deployment. This line prevents failures during testing.
|
@aabadie many thanks for your review. I have left some answers inline and will work on the other comments. Regarding the configuration file there seems to be a favor for the RIOT-style option. Lets give it some time anyway, and I will provide an alternative during the next couple of days if no one objects. |
d11ef29 to
f7a7105
Compare
|
@aabadie I have addressed your comments (despite the config header file as described above) |
|
ping @aabadie |
f7a7105 to
a3a2166
Compare
|
Added Kconfig options, enabled config header for doxygen again, tested on native and nrf52840dk, cleaned up minor stuff... @aabadie, mind to give it an other look to remove the change request? |
|
Btw: all tests now pass |
|
@leandrolanzieri is this one ready to go? |
pkg/mbedtls/Makefile.dep
Outdated
| CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_COND=0 | ||
| CFLAGS += -DCONFIG_ENTROPY_SOURCE_ADC_HEALTH_TEST=1 |
There was a problem hiding this comment.
Shouldn't this be better placed in the Makefile.include?
| #if defined(MODULE_MBEDTLS_ENTROPY_SOURCE_ADC) | ||
| entropy_source_adc_init(); | ||
| #endif |
There was a problem hiding this comment.
Would this init be needed for additional sources? Would it make sense to add some interface to initialize the selected sources?
There was a problem hiding this comment.
It depends on the source, but seems likely that most sources have some kind of an init function. Technically, we could add .init function to entropy_source_mbedtls_riot_t but I would like to delay that, since other sources (e.g. PUF) might have very different requirements from ADC sampling or HWRNG readout.
| for (unsigned i = 1; i < NUM_ENTROPY_FUNCS; i++) { | ||
| ret = mbedtls_entropy_add_source(ctx, entropy_funcs[i].func, NULL, | ||
| 1, entropy_funcs[i].strong); | ||
| } |
There was a problem hiding this comment.
I think there's something wrong with the ret usage. For example, if something goes wrong with the first entropy source (ret != 0) but works with the last, the whole function return code will be 0. I don't think this is expected.
Maybe it's better to check the return value in the for loop and return with an error if any of source cannot be added ?
There was a problem hiding this comment.
The loop adds entropy sources to a list which will be polled during entropy gathering. The error case occurs when a maximum number of sources have been added. Hence, the case of failure followed by success in not true. Nevertheless, we can save some redundant calls by returning on the first error.
|
@aabadie , @leandrolanzieri your latest comments should be addressed in this commit aef6196 |
| @@ -0,0 +1,10 @@ | |||
| PKG_NAME=mbedtls | |||
| PKG_URL=https://github.com/ARMmbed/mbedtls.git | |||
| PKG_VERSION=v2.26.0 | |||
There was a problem hiding this comment.
Why not use v3.0.0 here? (or at least v2.27.0)
There was a problem hiding this comment.
v2.26.0 was up to date when I opened this PR. I worked and tested the most with this version. Sticking with it seemed better to me than "blindly" bumping the version.
|
@PeterKietzmann the test fails on |
Yes it is. I ran the test on different platforms, back then. Cannot remember any high-ups. Just wondering: who has tested this PR before merging? Anyway, I will look into this as soon as I can! |
@fjmolinas can you name these boards so I have a starting point? |
Nightlies are failing on |
|
I ran the tests on the PR and on master. Results (ignore failed compilations, there is some kind of raise condition with pkg downloads...): masterpr |
|
Just a quick heads-up:
|
Contribution description
This PR adds mbed TLS as a package to be used for entropy generation, and it is the only module that has been tested in RIOT so far. However, this PR is an enabler for additional mbed TLS features in RIOT.
The PR includes a simple RIOT API to access the entropy generated by mbed TLS. Entropy sources that feed the module are currently ADC Noise (#14324) and the HWRNG. Further information can be found in the package documentation.
Testing procedure
make doc).Issues/PRs references
EXCLUDEnot assigned correctly? #15660