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

modules: mbedtls: remove default-enabling of hash algorithms #72078

Merged
merged 3 commits into from
May 3, 2024

Conversation

tomi-font
Copy link
Collaborator

@tomi-font tomi-font commented Apr 29, 2024

Do not enable hash algorithms except SHA-256 by default.
This unnecessarily inflates the final code size even if not all the enabled hash algorithms are actually used.

SHA-256 is (for now) kept enabled by default because many configurations across the code base assume that there is some hash algorithm available without needing to enable it.

Relates to (and will help) #71947.

@ceolin
Copy link
Member

ceolin commented Apr 29, 2024

There are some tests failing in CI due some tangled build options, but I think that is the right direction. Features should be disabled by default and explicitly requested.

@zephyrbot zephyrbot added area: Samples Samples area: TF-M ARM Trusted Firmware-M (TF-M) labels Apr 30, 2024
@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 30, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

west.yml Outdated Show resolved Hide resolved
@tomi-font tomi-font marked this pull request as draft April 30, 2024 10:38
@ceolin
Copy link
Member

ceolin commented May 1, 2024

Changes on Zephyr looks fine for me. Lets get modules in and update it.

@tomi-font tomi-font force-pushed the mbedtls_config_default branch 3 times, most recently from 039169f to 08602f8 Compare May 2, 2024 09:09
Do not enable hash algorithms except SHA-256 by default.
This unnecessarily inflates the final code size even if not all the
enabled hash algorithms are actually used.

SHA-256 is (for now) kept enabled by default because many configurations
across the code base assume that there is some hash algorithm
available without needing to enable it.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
Explicitly enable SHA-512 now that it is not enabled by default
anymore.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
@tomi-font
Copy link
Collaborator Author

This is ready for review.

@ceolin ceolin merged commit 55c8a16 into zephyrproject-rtos:main May 3, 2024
22 of 28 checks passed
@fabiobaltieri
Copy link
Member

@tomi-font looks like some TLS tests started failing after this got merged https://github.com/zephyrproject-rtos/zephyr/runs/24575825044, could you look into it?

tomi-font added a commit to tomi-font/zephyr that referenced this pull request May 6, 2024
Fix the failures introduced in zephyrproject-rtos#72078 by manually enabling all the
hash algorithms as they used to be.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
@tomi-font
Copy link
Collaborator Author

@fabiobaltieri Thanks for the heads-up. I opened a fix PR: #72334

@aescolar
Copy link
Member

aescolar commented May 6, 2024

@tomi-font would this PR warrant a line in the migration guidelines?

@tomi-font
Copy link
Collaborator Author

@tomi-font would this PR warrant a line in the migration guidelines?

Yeah, actually I started thinking about this afterwards. It does seem like a good idea. There will be other related PRs and changes made to the configuration; I'll address this point in a follow-up PR.

aescolar pushed a commit that referenced this pull request May 6, 2024
Fix the failures introduced in #72078 by manually enabling all the
hash algorithms as they used to be.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
tomi-font added a commit to tomi-font/zephyr that referenced this pull request May 8, 2024
For changes brought by PRs zephyrproject-rtos#71118 and zephyrproject-rtos#72078.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
tomi-font added a commit to tomi-font/zephyr that referenced this pull request May 8, 2024
For changes brought by PRs zephyrproject-rtos#71118 and zephyrproject-rtos#72078.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
tomi-font added a commit to tomi-font/zephyr that referenced this pull request May 8, 2024
For changes brought by PRs zephyrproject-rtos#71118 and zephyrproject-rtos#72078.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
nashif pushed a commit that referenced this pull request May 12, 2024
For changes brought by PRs #71118 and #72078.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
mariopaja pushed a commit to mariopaja/zephyr that referenced this pull request May 26, 2024
Fix the failures introduced in zephyrproject-rtos#72078 by manually enabling all the
hash algorithms as they used to be.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
mariopaja pushed a commit to mariopaja/zephyr that referenced this pull request May 26, 2024
For changes brought by PRs zephyrproject-rtos#71118 and zephyrproject-rtos#72078.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Crypto / RNG area: Samples Samples area: TF-M ARM Trusted Firmware-M (TF-M)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants