mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Configuration file split proposal

Open ronald-cron-arm opened this issue 1 year ago • 3 comments

Description

Add document describing the configuration file split to discuss and agree on how we want to do it. Fixes #9100. Fixes #9103.

PR checklist

  • [x] changelog not required
  • [x] 3.6 backport not required, development only
  • [x] 2.28 backport not required, development only
  • [x] tests not required, just documentation

ronald-cron-arm avatar Jun 07 '24 12:06 ronald-cron-arm

@ronald-cron-arm Can you add appropriate labels? (So that this PR stops showing when searching for PRs that need triaging.) Thanks!

mpg avatar Jun 13 '24 11:06 mpg

@ronald-cron-arm Can you add appropriate labels? (So that this PR stops showing when searching for PRs that need triaging.) Thanks!

Sorry forgot to do it. It is done now.

ronald-cron-arm avatar Jun 13 '24 11:06 ronald-cron-arm

My main feedback is: please don't mix the file split with redesigning and removing some crypto options.

Yes I agree. In the current version some parts are beyond the configuration file split and rather related to the repository split or even tf_psa_crypto_config.h in TF-PSA-Crypto 1.0. I will rework that. Thanks for the "legacy section" suggestion, that looks a good option.

ronald-cron-arm avatar Jun 19 '24 08:06 ronald-cron-arm

@gilles-peskine-arm I believe I have addressed all your comments, please have another look.

ronald-cron-arm avatar Sep 04 '24 07:09 ronald-cron-arm

@mpg I think I've addressed all your "in scope" comments. Please have another look, thanks.

ronald-cron-arm avatar Sep 09 '24 15:09 ronald-cron-arm

@gilles-peskine-arm A number of my thoughts here are actually 4.0 items

  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749897761 Rename MBEDTLS_CIPHER_NULL_CIPHER to a TLS option (enabling integrity-only ciphersuites).
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749898802 Organise mbedtls_config.h into X.509 / TLS / other sections.
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749907998 Rename MBEDTLS_X509_REMOVE_INFO to avoid negative feature macro.
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749912924 Remove disctinction between "feature support" and "module" macros (and remove the _C at the end of the names or the later).
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749912924 Hide internal macros such as SSL_TLS_C, X509_USE_C and X509_CREATE_C: should be auto-enabled based on any TLS (resp. X.509 parsing, writing) being enabled.
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1749968345 Should MBEDTLS_PK_EC_xxx remain in the PK namespace - will they also affect psa_import_ext()?
  • https://github.com/Mbed-TLS/mbedtls/pull/9236#discussion_r1750008283 MBEDTLS_TIMING_C should be renamed into the SSL namespace (as well as the file) as its only public functions are for use as DTLS callbacks.

I'm not sure what's the best way to track those - and if some of them are already covered by existing issues. Will you take it from there or would you like me to open issues and put them on the 4.0 board?

mpg avatar Sep 10 '24 08:09 mpg

@mpg We already have a bit of a mess of overlapping issues about configuration option names. Thanks for creating this summary, I'll sort through them.

gilles-peskine-arm avatar Sep 10 '24 20:09 gilles-peskine-arm

@mpg @gilles-peskine-arm I believe I have addressed all the comments. Please have another look. The section links do not work in the rich diff display. Not sure if this is a real issue or just an issue with the tool.

ronald-cron-arm avatar Sep 27 '24 14:09 ronald-cron-arm

The section links do not work in the rich diff display. Not sure if this is a real issue or just an issue with the tool.

I tried a couple of section links and they Work For Me™ in the rendered version: https://github.com/ronald-cron-arm/mbedtls/blob/config-file-split/docs/proposed/config-split.md

mpg avatar Sep 30 '24 08:09 mpg