Mechanism to error out on removed configuration options #164

gilles-peskine-arm · 2025-04-25T16:39:11Z

Set up a mechanism for erroring out if users try to use removed options in their configuration file. Resolves Mbed-TLS/mbedtls#10147. Specification in #184.

In this PR:

config_checks_generator.py: classes for options of a subproject.
Support static assert in config checks, not just preprocessor errors.
Scripts to save and load historical lists of config macros and derived macros. save 3.6, near-1.0 and near-4.0.

Status: ready for review. If the consuming brnaches require extra work in the framework, I'll do that in new commits.

PR checklist

TF-PSA-Crypto PR Mechanism to error out on removed configuration options TF-PSA-Crypto#269
development PR TODO
3.6 PR not required because: new code for use in 1.0/4.0 only

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Run a C compiler, not just the preprocessor. This allows capturing static assertions (`MBEDTLS_STATIC_ASSERT`). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use the latest patch release at this time, namely 3.6.4. This is the last release made before the first non-beta release of the next major version. ``` scripts/save_config_history.sh mbedtls-3.6.4 3.6 ``` Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use today's `development` head and the crypto submodule there. We may update this again before the release. Once the release is out, we should update to the release tags. ``` scripts/save_config_history.sh 06bae1e110ce71b44c3f4d17974d24feea4d2a92 1.0 scripts/save_config_history.sh 07912c9e3693d7ae5d62bfcbb8aef4daa9e3cafc 4.0 ``` Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mpg

Looks pretty good to me, the only blockers are the two issues with option handling in the shell script.

I have generated the files locally using the instructions in the commit messages and verified with sha256sum that I'm getting identical outputs.

scripts/mbedtls_framework/config_checks_generator.py

mpg · 2025-04-28T08:56:58Z

scripts/mbedtls_framework/config_checks_generator.py

+    before_generator.write(os.path.join(root,
+                                        'include',
+                                        branch_data.header_directory,
+                                        'config_check_before.h'))


In general, I don't think "before" without saying before what is a great name. Suggesting before_user here.

This used to be before/after [the user config] and changed to before/user/final. I agree that the names aren't ideal, but I don't think before_user/user/final is better. It should be before_user/after_user/final. Or it could be just before_user/after_user, since final is currently unused. But then we could go back to before/after since there's only one thing to be before/after and it's a pretty natural one (we're doing config checks, so before/after refers to reading the config.)

I'm not very happy about the “user” part, whether on its own or in after_user, because of the ambiguity between “the whole configuration provided by the user” and “MBEDTLS_USER_CONFIG_FILE (or crypto equivalent) specifically”. Any ideas for a better name?

Note that we'd better commit to names now, because once Mbed-TLS/TF-PSA-Crypto#269 is merged, changing the names will require back-and-forth between the framework and the consuming branches,

I've handled (or replied to) all your comments except this name change. A name change will require coordinated action over the three PR so I'd rather get a consensus first.

I don't think before_user/user/final is better. It should be before_user/after_user/final.

Yes, that's how I meant it, but didn't express it clearly.

I'm not very happy about the “user” part, whether on its own or in after_user, because of the ambiguity between “the whole configuration provided by the user” and “MBEDTLS_USER_CONFIG_FILE (or crypto equivalent) specifically”.

Good point. I'm not sure what would be a better name. Perhaps "input"? Or "editable" (sounds a bit weird, but perhaps less ambiguous)? Or config_file? Ah, perhaps editable_config_files - a bit long, but probably clearer: the plural suggests it includes the main config.h and MBEDTLS_USER_CONFIG_FILE if it exists.

For "final", I'm wondering about "after_adjust"? But I'm happy with "final" too.

A name change will require coordinated action over the three PR so I'd rather get a consensus first.

Sure. Also, if we can't converge on better names quickly, I wouldn't block this series of PRs over this. Since this is internal and unlikely to be edited every other day, we can probably just compensate sub-optimal naming with comments.

Given that the main users of this script would be us, I think we can get away with it with a comment, just giving some nomeclature on the names, rather than effectively changing the code.

If as we polish it we come up with better ones, we can do it later.

Since we're pressed for time and we don't have a consensus on better names, I'm going ahead and merging with the current names.

scripts/mbedtls_framework/test_config_checks_generator.py

scripts/mbedtls_framework/config_checks_generator.py

scripts/save_config_history.sh

history/config-adjust-mbedtls-4.0.txt

There's only one subproject and that's unlikely to change, so being able to specify a subproject at runtime is overkill. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2025-09-23T14:33:34Z

scripts/mbedtls_framework/config_checks_generator.py

    """Checker for an internal-only option."""


+class SubprojectInternal(Checker):


The final file, introduced in a previous pull request, is currently unused. It was strongly needed in an earlier incarnation (back when the config checks were public headers, included every time build_info.h was included, we needed to clean up macros defined by the previous files). Now there's no immediate need for it, but it might become useful in the future. For example, I think it would be needed if we wanted to reject user config files that try to #undef something (which is not a feature, currently, because I'm guessing that nobody does that).

At this point, removing it is more work than leaving it in, which is why I've kept it in.

mpg

LGTM, thanks!

I don't consider the remaining naming issue to be a blocker: I'm happy to improve naming if we quickly find a scheme we can all agree on, otherwise getting this merged in time for the release seems more important than getting internal names right.

minosgalanakis

Looks good, just one note on portability of the newly introduced code.

minosgalanakis · 2025-09-24T09:31:26Z

scripts/save_config_history.sh

+    for header in "$@"; do
+        git -C "$project_root" show "$commit:$header" >>"$temp_file"
+    done
+    sed -n 's![/ ]*# *define  *\([A-Z_a-z][0-9A-Z_a-z]*\).*!\1!p' <"$temp_file" |


Given that we just introduced config_checks_generator.py it would make sense to do this whole functionality in python, and not rely on external tools like sed and temporary files?

This is even more important if we consider that sed is not using the same syntax across GNU and BSD/MacOS

It would only need to pull regex, subprocess and argparse

The equivalent would be.
parsing_rex = re.compile(r'^[\s/*]*#\s*define\s+([A-Za-z_]\w*)\b', re.MULTILINE)

I wont be blocking the PR for this

I agree that for maintainability this script should be probably in Python. But I had started to write it in sh, and we needed the result urgently, so I polished and commited the sh code that I had rather than rewrite the thing in Python.

This script won't run on the CI and we'll probably only ever run it manually a couple of times each time we make a new major version. I almost left it in a commit message, but the adjust part was getting a bit too complicated for that.

AFAICT I haven't used any non-POSIX sed feature.

gilles-peskine-arm added needs-work size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Apr 25, 2025

gilles-peskine-arm added this to Roadmap pull requests (new board) Apr 25, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Apr 25, 2025

gilles-peskine-arm mentioned this pull request Apr 25, 2025

Mechanism to error out on removed configuration options Mbed-TLS/TF-PSA-Crypto#269

Open

7 tasks

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from 1940e16 to c00ddf3 Compare July 10, 2025 12:12

This was referenced Jul 14, 2025

Framework to generate config checks: specification #184

Open

Make check_names.py repo-specific #186

Open

Cleanup after mechanism for marking a compilation option as private Mbed-TLS/mbedtls#10305

Open

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from c00ddf3 to dffa814 Compare July 16, 2025 20:46

gilles-peskine-arm mentioned this pull request Jul 16, 2025

Mechanism to error out on removed configuration options Mbed-TLS/mbedtls#10306

Open

5 tasks

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from 2aa51b4 to 28e29ba Compare July 18, 2025 13:00

gilles-peskine-arm mentioned this pull request Jul 22, 2025

check_config.h: unit tests #192

Merged

3 tasks

gilles-peskine-arm mentioned this pull request Jul 31, 2025

Introduce config_checks_generator.py #196

Merged

3 tasks

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from 28e29ba to c2d9805 Compare August 1, 2025 21:21

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from c2d9805 to 11f8e95 Compare September 15, 2025 15:57

gilles-peskine-arm added 8 commits September 19, 2025 21:12

Add checker classes for options used in the wrong project

f7efa14

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Support non-ASCII compiler messages (assume UTF-8)

103cdbc

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Run cc instead of cpp

172e29b

Run a C compiler, not just the preprocessor. This allows capturing static assertions (`MBEDTLS_STATIC_ASSERT`). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add function to get the framework root

08b8ce3

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

New module to load historical config options information

2a68ccf

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

New script to save historical config options information

83e8568

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the config-error-on-removed-options-framework branch from 11f8e95 to 8943efa Compare September 19, 2025 20:19

gilles-peskine-arm added needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Sep 19, 2025

gilles-peskine-arm requested review from mpg and minosgalanakis September 20, 2025 07:19

gilles-peskine-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Sep 22, 2025

mpg requested changes Sep 23, 2025

View reviewed changes

mpg added the needs-work label Sep 23, 2025

gilles-peskine-arm added 5 commits September 23, 2025 16:14

Remove subproject as a constructor argument

fcfe192

There's only one subproject and that's unlikely to change, so being able to specify a subproject at runtime is overkill. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove redundant assignment

4402172

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix save_config_history.sh when options are passed

00dddd2

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use -C as the option to convey a directory to run git in

48a1cbc

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Improve documentation

0acab5c

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm requested a review from mpg September 23, 2025 14:26

gilles-peskine-arm removed the needs-work label Sep 23, 2025

gilles-peskine-arm commented Sep 23, 2025

View reviewed changes

mpg approved these changes Sep 24, 2025

View reviewed changes

minosgalanakis approved these changes Sep 24, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Sep 24, 2025

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 24, 2025

gilles-peskine-arm merged commit 0bfaf0e into Mbed-TLS:main Sep 24, 2025
4 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Sep 24, 2025

		"""Checker for an internal-only option."""


		class SubprojectInternal(Checker):

Mechanism to error out on removed configuration options #164

Mechanism to error out on removed configuration options #164

Uh oh!

Conversation

gilles-peskine-arm commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Apr 25, 2025 •

edited

Loading

gilles-peskine-arm Sep 23, 2025 •

edited

Loading