Remove OpenSSL #955

bettio · 2023-11-20T21:58:08Z

Do not depend both on Mbed-TLS and OpenSSL at the same time, just use Mbed-TLS.
Also now the same code base can be both used on generic unix and MCUs.

Closes #950

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

src/libAtomVM/otp_crypto.c

pguyot · 2023-11-27T06:36:42Z

src/platforms/generic_unix/lib/sys.c

+
+#ifdef ATOMVM_HAS_MBEDTLS
+#ifndef AVM_NO_SMP
+    Mutex *entropy_mutex;


I understand these mutexes are only used for initialization. We don't even need to acquire them if the boolean is true.

More simply, why don't we initialize the mbed tls contexts in sys_init_platform?

I gather the info also here: mutex are used for locking when mbedtls is built without threading support.

We also use them for avoding race condition when doing async init.

pguyot · 2023-11-27T07:27:31Z

src/platforms/generic_unix/lib/sys.c

+#ifndef AVM_NO_SMP
+    Mutex *entropy_mutex;
+#endif
+    mbedtls_entropy_context entropy_ctx;


We probably want the mbed tls contexts to be in GlobalContext and be available in all platforms unless ATOMVM_HAS_MBEDTLS is undefined.

I'd like to do some further decoupling while iterating over, so I don't like turning GlobalContext into a kitchen sink.

So the plan is, as mentioned in #930, shipping some features using common components and libraries, and then features such as mbedtls are picked up from platforms supporting it (+ implementing the missing pieces).

src/platforms/generic_unix/lib/sys.c

Minimal implementation of crypto:strong_rand_bytes/1 using mbedtls. Signed-off-by: Davide Bettio <davide@uninstall.it>

Add test for atomvm:random/0 so implementation can be replaced with a different implementation. Signed-off-by: Davide Bettio <davide@uninstall.it>

Use Mbed-TLS based crypto:strong_rand_bytes/1 for generating random numbers rather than OpenSSL. Signed-off-by: Davide Bettio <davide@uninstall.it>

pguyot · 2023-11-29T21:24:16Z

src/platforms/rp2040/src/lib/sys.c

+        mbedtls_ctr_drbg_init(&platform->random_ctx);
+
+        mbedtls_entropy_context *entropy_ctx = sys_mbedtls_get_entropy_context_lock(global);
+        // Safe to unlock it now, sys_mbedtls_entropy_func will lock it again later


I do not understand this and the locking pattern here.

Isn't locking only meant when MBEDTLS_THREADING_C is not defined?

Are we just fetching entropy context here without locking it? If so, could we simplify the logic?

We do locking as part of async initialization and because we don't know what the caller will do afterwards.
Since here we just interact with entropy through the entropy function we can just release the lock as the first thing.

CHANGELOG.md

pguyot · 2023-11-29T21:30:45Z

src/platforms/esp32/components/avm_sys/sys.c

+
+int sys_mbedtls_entropy_func(void *entropy, unsigned char *buf, size_t size)
+{
+#ifndef MBEDTLS_THREADING_C


In fact, if we are building for multicore, we should define MBEDTLS_THREADING_C on esp32 & pico for a finer-grained locking directly managed by mbedtls (they know when to lock and unlock).
On esp32, it's disabled by default but it's part of the sdk config.
On Pico, we provide our own mbedtls-config.h, so we can just add it there.

Then we need to define MBEDTLS_THREADING_ALT and call mbedtls_threading_set_alt.
On esp32, this is defined by default.
On Pico, we need to add it to mbedtls-config.h as well.

On generic unix, we don't control how mbedtls was compiled, so it makes sense to keep this logic and handle the locking ourselves if MBEDTLS_THREADING_C is undefined.

I did a quick check and they don't look much more fine grained than this implementation.
On ESP32 I didn't want to alter the default configuration and I wanted to support both scenarios, since supporting them didn't cost a large effort I just implemented that wrapper function and handled both configurations.
Right now we customize sdkconfig just for partitioning and that's it.

Right.

Still we probably want to use mbedtls native threading on Pico as we're providing our own config file.

I would add a TODO, since this requires extra work for providing rp2040 specific synchronization helpers to mbedtls.

pguyot

Do we want ssl to use this shared entropy as part of this PR?

I would keep the scope of this PR limited to this.

pguyot · 2023-12-03T15:46:56Z

src/platforms/esp32/components/avm_sys/sys.c

+    if (!platform->random_is_initialized) {
+        mbedtls_ctr_drbg_init(&platform->random_ctx);
+
+        mbedtls_entropy_context *entropy_ctx = sys_mbedtls_get_entropy_context_lock(global);


I am still not a fan of this.

If would deadlock if first calling code first gets entropy and then ctr_drbg.

yes, but even removing this doesn't completely fix the problem: it might be still hidden in implicit use of entropy from ctr_drbg.
My only valid suggestion here is telling very clearly that ctr_drbg shouldn't be used at all when owning entropy lock.

pguyot · 2023-12-03T15:50:07Z

src/platforms/esp32/components/avm_sys/sys.c

+
+int sys_mbedtls_entropy_func(void *entropy, unsigned char *buf, size_t size)
+{
+#ifndef MBEDTLS_THREADING_C


Right.

Still we probably want to use mbedtls native threading on Pico as we're providing our own config file.

fadushin

I vote we merge and leave TODOs in the relevant parts of the code.

Define also sys_mbedtls.h header and sys_mbedtls_get_entropy_context_lock and sys_mbedtls_get_ctr_drbg_context_lock functions. Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio force-pushed the remove-openssl branch from 9e979e5 to 5c9be88 Compare November 20, 2023 22:12

pguyot reviewed Nov 20, 2023

View reviewed changes

src/libAtomVM/otp_crypto.c Outdated Show resolved Hide resolved

bettio marked this pull request as draft November 20, 2023 22:51

bettio force-pushed the remove-openssl branch 4 times, most recently from c69faf6 to 7510cf5 Compare November 26, 2023 12:33

bettio marked this pull request as ready for review November 26, 2023 16:37

pguyot reviewed Nov 27, 2023

View reviewed changes

bettio force-pushed the remove-openssl branch 2 times, most recently from db622ed to 57a12a6 Compare November 28, 2023 12:23

bettio added 3 commits November 28, 2023 14:05

Initial crypto:strong_rand_bytes/1

fb4845c

Minimal implementation of crypto:strong_rand_bytes/1 using mbedtls. Signed-off-by: Davide Bettio <davide@uninstall.it>

tests: add test for atomvm:random/0

c1faa55

Add test for atomvm:random/0 so implementation can be replaced with a different implementation. Signed-off-by: Davide Bettio <davide@uninstall.it>

generic_unix: remove OpenSSL

67874ce

Use Mbed-TLS based crypto:strong_rand_bytes/1 for generating random numbers rather than OpenSSL. Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio force-pushed the remove-openssl branch 4 times, most recently from 9581fea to 10a3195 Compare November 29, 2023 00:08

pguyot reviewed Nov 29, 2023

View reviewed changes

bettio force-pushed the remove-openssl branch from 10a3195 to 77ff4b8 Compare November 29, 2023 23:00

pguyot reviewed Dec 3, 2023

View reviewed changes

bettio force-pushed the remove-openssl branch from 77ff4b8 to 255f18c Compare December 3, 2023 19:23

fadushin approved these changes Dec 5, 2023

View reviewed changes

Move mbedtls entropy and random ctx init to platform specific code

2052e3d

Define also sys_mbedtls.h header and sys_mbedtls_get_entropy_context_lock and sys_mbedtls_get_ctr_drbg_context_lock functions. Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio force-pushed the remove-openssl branch from 255f18c to 2052e3d Compare December 5, 2023 22:20

bettio merged commit dfcfc96 into atomvm:master Dec 5, 2023
84 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove OpenSSL #955

Remove OpenSSL #955

bettio commented Nov 20, 2023 •

edited

Loading

pguyot Nov 27, 2023

bettio Nov 27, 2023

pguyot Nov 27, 2023

bettio Nov 27, 2023

pguyot Nov 29, 2023

bettio Nov 29, 2023

pguyot Nov 29, 2023 •

edited

Loading

bettio Nov 29, 2023

pguyot Dec 3, 2023

bettio Dec 3, 2023 •

edited

Loading

pguyot left a comment •

edited by bettio

Loading

pguyot Dec 3, 2023

bettio Dec 3, 2023

pguyot Dec 3, 2023

fadushin left a comment

Remove OpenSSL #955

Remove OpenSSL #955

Conversation

bettio commented Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguyot Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

pguyot left a comment • edited by bettio Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadushin left a comment

Choose a reason for hiding this comment

bettio commented Nov 20, 2023 •

edited

Loading

pguyot Nov 29, 2023 •

edited

Loading

bettio Dec 3, 2023 •

edited

Loading

pguyot left a comment •

edited by bettio

Loading