Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a place for user-specific configuration and files #9869

Open
irwir opened this issue Dec 23, 2024 · 5 comments
Open

Provide a place for user-specific configuration and files #9869

irwir opened this issue Dec 23, 2024 · 5 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version enhancement needs-design-approval

Comments

@irwir
Copy link
Contributor

irwir commented Dec 23, 2024

Suggested enhancement

Add a directory (at the library root) for user-specific settings and files - for example, custom.
The directory contains a number of empty files as placeholders. There should be:
custom/mbedtls_user_config.h,
custom/crypto_user_config.h,
custom/threading_alt.h
... all other files corresponding to '_ALT' macros.

Justification

Users should not edit mbedtls_config.h; all settings go to custom/mbedtls_user_config.h as #define / #undef directives.
The same applies to crypto_config.h
Of course, both custom/*_user_config.h have to be included unconditionally (could be the last line of the corresponding *_config.h).

Mbed TLS needs this because
library code could be treated as "read-only", user-alterable files have a separate and predefined location.
"Include" paths in projects need no modifications; just put #include directive into the provided placeholder files.
This should simplify merging, updates and maintenance.
Currently,

  • MBEDTLS_USER_CONFIG_FILE is commented out (points to null device), therefore users have to alter mbedtls_config.h;
  • "*_alt.h" have no specific path, hence either project has to be edited, or library and user files would make a mess in the same folders.
@mpg mpg added enhancement needs-design-approval api-break This issue/PR breaks the API and must wait for a new major version labels Dec 24, 2024
@mpg
Copy link
Contributor

mpg commented Dec 24, 2024

Thanks for the suggestion.

We had plans about splitting the configuration file in a somewhat similar way. Our goal was to get a better separation between "user config" and "list of all tunable options for reference", because the confusion between the two causes issues in some of our test scripts. But the end result would be similar: better separation between a "read-only" part and an "editable" part.

This kind of change can only be done in a major version, because it's a breaking API change. Unfortunately, at this point it seems unlikely we'll have time to do it in the upcoming one (Mbed TLS 4.0 + TF-PSA-Crypto 1.0).

Cc @gilles-peskine-arm

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Dec 24, 2024

A related proposal: #9664

(To clarify, this doesn't solve the same problem. It's related in that it's another proposal for reorganizing the configuration files, but that one is for maintainers' convenience and would not, in itself, change things for application developers.)

@irwir
Copy link
Contributor Author

irwir commented Dec 24, 2024

This kind of change can only be done in a major version, because it's a breaking API change.

Where is the break?
User still could ignore the news and continue editing library files the old way.
File changes are limited to 2 #defines in *_config.h files - uncomment *_USER_CONFIG_FILE and give it a standard value.
The default header files are empty and thus cannot break anything even when included unconditionally.

Also, @gilles-peskine-arm suggested to backport to 3.6 branch in his issue; this hints that the changes are minor.
No real need to wait for 5.0 release.

Additionally, OpenSSL could be mentioned here - before building it, user had to run a script that created a file with the desired settings. This file does roughly the same as should do *_user_config.h files.

@gilles-peskine-arm
Copy link
Contributor

Users should not edit mbedtls_config.h; all settings go to custom/mbedtls_user_config.h as #define / #undef directives.

I'm not sure I understand. How does that differ from defining MBEDTLS_CONFIG_FILE to "custom/mbedtls_user_config.h"?

Of course, both custom/*_user_config.h have to be included unconditionally (could be the last line of the corresponding *_config.h).

I don't understand how that could work. What we distribute has to work for different kinds of users, including:

  • Users who want to build the library unmodified. They don't create or edit any file, and they don't define any particular identifier.
  • Users who don't want to edit the source tree at all, but want to fully customize the configuration, and have no control over the include path order. They need the ability to set MBEDTLS_CONFIG_FILE to a different name.
  • Users who want to follow the default configuration with tweaks. They define MBEDTLS_USER_CONFIG_FILE and place their tweaks there.
  • Users who edit the configuration through an IDE that toggles boolean options on and off by (un)commenting #define lines. They edit mbedtls/mbedtls_config.h through that IDE.

(These are not theoretical concerns: all of these scenarios have come up in the past.)

Providing empty files as placeholders would break some of these scenarios. We'd have to provide a way to override the names of these files. But then what's the advantage over the current organization?

@gilles-peskine-arm
Copy link
Contributor

This kind of change can only be done in a major version, because it's a breaking API change.

Indeed, if we stop honoring MBEDTLS_CONFIG_FILE and MBEDTLS_USER_CONFIG_FILE, it's a breaking change.

If we define MBEDTLS_USER_CONFIG_FILE with a default name, let's say custom/mbedtls_user_config.h, and we provide an empty file of that name, it's only a breaking change if there are users who are already using that name, and whose custom/mbedtls_user_config.h happens to be later on the include path than ours. We could avoid this break by picking a name that nobody has used, but that would have to mean that the name is unintuitive, by construction. (A randomly generated name?)

Also, @gilles-peskine-arm suggested to backport to 3.6 branch in his issue; this hints that the changes are minor.

The change I proposed in #9664 would result in releases with identical mbedtls_config.h. It would only affect direct use of unreleased branches from Git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version enhancement needs-design-approval
Projects
None yet
Development

No branches or pull requests

3 participants