Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.54k stars 2.6k forks source link

Move back error.c to mbedtls #9623

Closed ronald-cron-arm closed 3 weeks ago

ronald-cron-arm commented 1 month ago

Suggested enhancement

As decided as part of the review of https://github.com/Mbed-TLS/mbedtls/pull/9236 we will not have error.c in TF-PSA-Crypto. Thus revert to the generation of error.c in /library by both make and CMake build system.

error.h cannot be moved back to include/mbedtls as it is included by crypto modules. It is better for the functions defined in error.c not to be declared in crypto modules though.

Thus split out of error.h into error_common.h (other name possible) all of error.h but the declaration of the *_strerr* functions and include it instead of error.h in all files in tf-psa-crypto.

Last point, the declaration of the function pointer mbedtls_test_hook_error_add() has to be moved out of error.c as we will need it in tf-psa-crypto. Not sure where though.

Justification

Mbed TLS needs this for the repo split.

gilles-peskine-arm commented 1 month ago

Last point, the declaration of the function pointer mbedtls_test_hook_error_add() has to be moved out of error.c as we will need it in tf-psa-crypto. Not sure where though.

Addition of error codes is going away (no issue for that yet, but: general plan of which we'll only do a little; DI of removing addition).

Hopefully uses of MBEDTLS_ERR_ERROR_xxx can change to PSA_ERROR_xxx, but I need to do more DI on that.

So all that remains in error.h is the error-code-to-error-message functionality, which I think we can live without in TF-PSA-Crypto 1.0, and I'd like to replace by what's now in psa_constant_names. In Mbed TLS, I think keeping user-friendly error messages is more justified, but not critical for 4.0.

ronald-cron-arm commented 1 month ago

Last point, the declaration of the function pointer mbedtls_test_hook_error_add() has to be moved out of error.c as we will need it in tf-psa-crypto. Not sure where though.

Addition of error codes is going away (no issue for that yet, but: general plan of which we'll only do a little; DI of removing addition).

Hopefully uses of MBEDTLS_ERR_ERROR_xxx can change to PSA_ERROR_xxx, but I need to do more DI on that.

So all that remains in error.h is the error-code-to-error-message functionality, which I think we can live without in TF-PSA-Crypto 1.0, and I'd like to replace by what's now in psa_constant_names. In Mbed TLS, I think keeping user-friendly error messages is more justified, but not critical for 4.0.

yes I am aware of these changes but it is likely (as the work above is not going to be done) that in Q4 for the repo split we will need to move the declaration of mbedtls_test_hook_error_add() somewhere in tf-psa-crypto, right?

Harry-Ramsey commented 1 month ago

I have fixed some of the build errors with my patch. However, I have noticed some programs are reliant upon error.c but are not dependent on Mbed TLS. There are two options going forward here:

What would be the appropriate action? The programs are all related to the pk suite.

Furthermore, should error.c be included in src_x509 or src_tls in the CMakeList.txt file for building?

ronald-cron-arm commented 1 month ago

I have fixed some of the build errors with my patch. However, I have noticed some programs are reliant upon error.c but are not dependent on Mbed TLS. There are two options going forward here:

* Make them dependent upon Mbed TLS.

* Remove any code that is dependent upon `error.c`

What would be the appropriate action? The programs are all related to the pk suite.

Furthermore, should error.c be included in src_x509 or src_tls in the CMakeList.txt file for building?

Like version*.c, please include error.c in the TLS library thus src_tls.

Otherwise, regarding pkey programs, I'd say just remove the code on error.c rather than link the TLS library to the programs just for that. For the pkey programs that we will keep in 4.x and move to TF-PSA-Crypto we will add back if we feel the need some error logs when we have the replacement for the mbedtls_.sterr functions.

Harry-Ramsey commented 1 month ago

It seems that the x509 programs also use strerror functions found inside error_common.h. Should those also be removed or should error.c be included in both x509 and SSL sources?

gilles-peskine-arm commented 1 month ago

We should put error.c in libmbedx509 so that it can be used by both x509 and tls.

ronald-cron-arm commented 1 month ago

We should put error.c in libmbedx509 so that it can be used by both x509 and tls.

Fine by me if we need it for x509 as well.