Backport 2.28: Report configuration settings in the outcome file #9316

gilles-peskine-arm · 2024-06-26T18:19:28Z

Backport of #9172 and its framework companion Mbed-TLS/mbedtls-framework#28.

I followed the same commit structure for the common content, but several things are different in 2.28.

Minor fixes from the main PR.
Test code and manually written test cases.
Commits from the framework PR to create the Python script at a temporary location.
Move the script to its pre-framework location.
Adapt the script content for 2.28. Maybe dependencies_of_setting needs more 2.28-only stuff but we can do that later based on looking at outcome analysis.
Check in the generated files, since we do that in 2.28.
Add the generated files to check-generated-files. In 2.28, there is no handling of generated files in **/Makefile and **/CMakeLists.txt, and no framework submodule to update.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog no (test only)
backport of Report configuration settings in the outcome file #9172
tests provided

Fix PSA_CRYPTO_CONFIG_H being treated as a configuration setting in include/psa/crypto_config.h. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add a test suite intended to report configuration options in the outcome file: we're only interested in SKIP vs PASS. Add a few test cases for some interesting combinations of options. The selection here is just for illustration purposes, more will be added later. A subsequent commit will automatically generate test cases for single options. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

Generate option-on and option-off cases for test_suite_config, for all boolean options (MBEDTLS_xxx and PSA_WANT_xxx, collected from the mbedtls and PSA config files). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

When option A is only meaningful if option B is enabled, when enumerating single-option test cases, emit A:B and !A:B rather than A and !A. This way the "!A" case is actually meaningful. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

I had accidentally reused a variable name inside the same function. Python copes but mypy doesn't. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

The two were used interchangeably. Align on "setting", which is what config.py uses in its documentation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

"Super settings" were effectively the dependencies of a setting, so align on that terminology. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

* Move to the correct location. * Adjust the package name for auxiliary modules. * Adjust the hack to import a module from scripts. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

ronald-cron-arm

Just to be sure, do we really need that in 2.28? Its end of life is at the end of the year thus what the point to "Check that all configuration options are tested in all.sh" now for it?

gilles-peskine-arm · 2024-06-27T12:13:36Z

I don't think it's worth it to enforce the same level of configuration testing in 2.28. And in that respect, I didn't go digging for options with dependencies in 2.28 (options where A requires B so we want to check A:B and !A:B, not just A and !A), not even to the small level I did for 3.6. But to even evaluate this, I need to get the reports from this pull request. Doing the backport was only about ½h of engineering time, plus the time to review it. I think that's well worth it.

coolleng2525 · 2024-06-28T09:22:37Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2024-06-28T09:23:16Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The PR introduces a new script `generate_config_tests.py` which generates test cases for configuration settings. It is crucial to ensure that this script correctly handles all edge cases, especially since it deals with conditional dependencies and complex configurations.
	Code Complexity: The `config.py` script has been modified to include handling for inclusion guards and other complex parsing logic. This increases the complexity of the script, which could lead to maintenance challenges in the future.
	Generated Data Verification: The PR includes a large amount of automatically generated test data. It is important to verify that this data is correct and covers all necessary scenarios.

paul-elliott-arm

LGTM, just one question.

paul-elliott-arm · 2024-06-28T16:35:20Z

tests/scripts/check-generated-files.sh

@@ -102,4 +102,5 @@ check scripts/generate_visualc_files.pl visualc/VS2010
 check scripts/generate_psa_constants.py programs/psa/psa_constant_names_generated.c
 check tests/scripts/generate_psa_wrappers.py tests/include/test/psa_test_wrappers.h tests/src/psa_test_wrappers.c
 check tests/scripts/generate_bignum_tests.py $(tests/scripts/generate_bignum_tests.py --list)
+check tests/scripts/generate_config_tests.py $(tests/scripts/generate_bignum_tests.py --list)


Was this deliberate or copypasta? I don't think generate_config_tests.py has a --list option, but is the generate_bignum_tests.py list compatible?

Woops, copypasta. Fixed.

All the generate_xxx_tests.py use the same main function from test_data_generation.py and they take the same command line options.

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

ronald-cron-arm

I've checked the backport and reviewed the additional commits. One comment otherwise this looks good to me.

ronald-cron-arm · 2024-07-01T08:55:55Z

tests/scripts/generate_config_tests.py

+        # tests that only run Mbed TLS against itself, which only run in
+        # configurations with both sides enabled.
+        if name.startswith('MBEDTLS_SSL_TLS1_3_'):
+            return 'MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3'


Suggested change

return 'MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3'

return 'MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL'

TLS 1.3 is actually not supported in 2.28 thus we could probably just remove this case.

TLS 1.3 is still experimental and partial, and SSL3 is obsolete, so we don't expect much coverage about them, in particular we don't expect them to be the sole supported version. TLS 1.0 and 1.1 exist and we expect good coverage for them. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

ronald-cron-arm

LGTM, thanks.

paul-elliott-arm

LGTM

gilles-peskine-arm added 14 commits June 26, 2024 19:49

Recognize that a double-inclusion guard is not a config setting

41ad660

Fix PSA_CRYPTO_CONFIG_H being treated as a configuration setting in include/psa/crypto_config.h. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Anchor relative paths

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

ae27ca9

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

Detect sub-options

c79ecea

When option A is only meaningful if option B is enabled, when enumerating single-option test cases, emit A:B and !A:B rather than A and !A. This way the "!A" case is actually meaningful. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Pacify mypy

556249e

I had accidentally reused a variable name inside the same function. Python copes but mypy doesn't. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix missing negation

c08d5bf

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

Explain why we require TLS client and server simultaneously

f75c70b

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

Terminology: consistently use "setting", not "option"

5454a84

The two were used interchangeably. Align on "setting", which is what config.py uses in its documentation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Terminology: use "dependencies" for a list of settings

1afb703

"Super settings" were effectively the dependencies of a setting, so align on that terminology. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Adjust generate_config_tests.py for 2.28

744741b

* Move to the correct location. * Adjust the package name for auxiliary modules. * Adjust the hack to import a module from scripts. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove some settings that don't exist in 2.28

cfba247

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

Add generated config tests

d33c48b

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

Declare the new generated files

Loading
Loading status checks…

b19fa4e

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

gilles-peskine-arm added needs-ci size-s component-test priority-high labels Jun 26, 2024

gilles-peskine-arm mentioned this pull request Jun 26, 2024

Report configuration settings in the outcome file #9172

Merged

5 tasks

gilles-peskine-arm requested review from paul-elliott-arm and ronald-cron-arm June 27, 2024 06:56

gilles-peskine-arm added needs-review and removed needs-ci labels Jun 27, 2024

ronald-cron-arm reviewed Jun 27, 2024

View reviewed changes

paul-elliott-arm reviewed Jun 28, 2024

View reviewed changes

Fix copypasta

Loading
Loading status checks…

93708fe

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

gilles-peskine-arm requested a review from ronald-cron-arm June 28, 2024 17:19

gilles-peskine-arm requested a review from paul-elliott-arm June 28, 2024 17:19

ronald-cron-arm requested changes Jul 1, 2024

View reviewed changes

gilles-peskine-arm requested a review from ronald-cron-arm July 1, 2024 09:34

ronald-cron-arm approved these changes Jul 1, 2024

View reviewed changes

paul-elliott-arm approved these changes Jul 1, 2024

View reviewed changes

paul-elliott-arm added approved needs-ci and removed needs-review labels Jul 1, 2024

ronald-cron-arm added this pull request to the merge queue Jul 3, 2024

Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit 3218ccf Jul 3, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport 2.28: Report configuration settings in the outcome file #9316

Backport 2.28: Report configuration settings in the outcome file #9316

gilles-peskine-arm commented Jun 26, 2024

ronald-cron-arm left a comment

gilles-peskine-arm commented Jun 27, 2024

coolleng2525 commented Jun 28, 2024

CodiumAI-Agent commented Jun 28, 2024

paul-elliott-arm left a comment

paul-elliott-arm Jun 28, 2024

gilles-peskine-arm Jun 28, 2024

ronald-cron-arm left a comment

ronald-cron-arm Jul 1, 2024

ronald-cron-arm left a comment

paul-elliott-arm left a comment

	return 'MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3'
	return 'MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL'

Backport 2.28: Report configuration settings in the outcome file #9316

Backport 2.28: Report configuration settings in the outcome file #9316

Conversation

gilles-peskine-arm commented Jun 26, 2024

PR checklist

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 27, 2024

coolleng2525 commented Jun 28, 2024

CodiumAI-Agent commented Jun 28, 2024

PR Reviewer Guide 🔍

paul-elliott-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm Jun 28, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Jun 28, 2024

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm Jul 1, 2024

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm left a comment

Choose a reason for hiding this comment