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.

Sign up for GitHub

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

Jump to bottom

config.py: don't rely on section names #9604

Merged
merged 5 commits into from
Sep 26, 2024
Merged

config.py: don't rely on section names #9604

merged 5 commits into from
Sep 26, 2024

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Sep 19, 2024 •
edited
Loading

In config.py, stop relying on section names for config.py full, config.py realfull, etc. This way we can move configuration settings around as described in #9236 without having to worry about messing up the tooling.

Specifically:

  • full and friends no longer rely on the section to determine which settings are booleans that should be activated.
  • realfull is now really full. The goal is to document everything, and it turns out that uncommenting everything does work.

Testing

The script tests/scripts/test_config_script.py runs some tests on config.py and places the output in the given directory. Run it on the version of config.py from two commits to compare its effects.

$ git checkout --recurse-submodules upstream/pull/9604~4
$ tests/scripts/test_config_script.py -d 0
$ git checkout --recurse-submodules upstream/pull/9604~3
$ tests/scripts/test_config_script.py -d 1
$ git checkout --recurse-submodules upstream/pull/9604~2
$ tests/scripts/test_config_script.py -d 2
$ git checkout --recurse-submodules upstream/pull/9604~1
$ tests/scripts/test_config_script.py -d 3
$ git checkout --recurse-submodules upstream/pull/9604
$ tests/scripts/test_config_script.py -d 4
$ diff -ru 0 1
$ diff -ru 1 2
$ diff -ru 2 3 | less
$ diff -ru 3 4

2→3 has an expected difference in the behavior of realfull.

PR checklist

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Sep 19, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 19, 2024
Merged
4 tasks
mpg
mpg reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the general approach, but there's a few things I need clarified before I can approve.

include/mbedtls/mbedtls_config.h Outdated Show resolved Hide resolved
scripts/config.py Outdated Show resolved Hide resolved
scripts/config.py Show resolved Hide resolved
scripts/config.py Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-reviewer This PR needs someone to pick it up for review labels Sep 20, 2024
minosgalanakis added a commit to minosgalanakis/mbedtls that referenced this pull request Sep 23, 2024
@minosgalanakis
crypto_config.h: Temporary disabled sectios from config.
b6e6246 
Please refer to Mbed-TLS#9604

This commit will be ammended when a consensus is reached.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
minosgalanakis added a commit to minosgalanakis/mbedtls that referenced this pull request Sep 23, 2024
@minosgalanakis
crypto_config.h: Temporary disabled sectios from config.
de354f3 
Please refer to Mbed-TLS#9604

This commit will be ammended when a consensus is reached.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
@gilles-peskine-arm
Pass the setting's value to adapters
702d75a 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Change "full" to affect boolean settings rather than use sections
e4c6955 
To get rid on the reliance on sections, change "full" and friends to enable
settings based on whether the setting is boolean, rather than based on the
section it contains.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Change "realfull" to activate everything
36571d6 
Change "realfull" to activate everything. After investigation, it seems that
having "realfull" not activate everything was a historical oddity due to
proximity with "full", not a goal in itself.

Mbed-TLS#520 (comment)
https://github.com/Mbed-TLS/mbedtls/pull/965/files#r523409092

This changes the output of `scripts/config.py realfull`: now all non-boolean
options are uncommented.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 23, 2024
Merged
@gilles-peskine-arm
Don't pass the section name to adapters
00b9144 
We have finished removing the reliance of named configuration on section
names.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
minosgalanakis added a commit to minosgalanakis/mbedtls that referenced this pull request Sep 23, 2024
@minosgalanakis
crypto_config.h: Temporary disabled sections from config.
0ff3245 
Please refer to Mbed-TLS#9604

This commit will be ammended when a consensus is reached.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

I have rebased twice:

  1. Once from https://github.com/gilles-peskine-arm/mbedtls/tree/config-full-booleans-only-1 to https://github.com/gilles-peskine-arm/mbedtls/tree/config-full-booleans-only-2 to cancel the changes related to preserving the not-actually-full behavior of realfull, following the discussion with Manuel here and in Slack.
  2. Once because now that Move config.py functionalities to the framework #9470 is merged, part of the changes are now in the framework. (With an extra force-push that just changes the commit date, because I forgot to push the framework branch before triggering the CI.)

mpg
mpg reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the rebases but I don't think my previous feedback has been addressed.

mpg
mpg previously approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I had marked some of my comments as resolved by the realfull rework, but I for some reason I failed to notice that was also the case for the others, sorry.

ronald-cron-arm
ronald-cron-arm previously approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-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 25, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 25, 2024
Merged
5 tasks
@mpg
Copy link
Contributor

mpg commented Sep 26, 2024

I've merged the framework PR, please update to point to the merge commit.

@gilles-peskine-arm
Update framework to the main branch
3c16e99 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports labels Sep 26, 2024
mpg
mpg approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ronald-cron-arm
ronald-cron-arm approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpg mpg 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, needs-backports Backports are missing or are pending review and approval. labels Sep 26, 2024
@mpg mpg enabled auto-merge September 26, 2024 09:03
@mpg mpg added this pull request to the merge queue Sep 26, 2024
Merged via the queue into Mbed-TLS:development with commit 5602651 Sep 26, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilles-peskine-arm @mpg @ronald-cron-arm