Skip to content

Remove MBEDTLS_SHA3_C config option #10145

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

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

gabor-mezei-arm
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm commented Apr 23, 2025

Description

Replace uses of MBEDTLS_SHA3_C config option with PSA_WANT macros.

Resolve #10203

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

@gabor-mezei-arm gabor-mezei-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Apr 23, 2025
@gabor-mezei-arm gabor-mezei-arm self-assigned this Apr 23, 2025
@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch 3 times, most recently from f2c4433 to 3413305 Compare April 28, 2025 16:14
@gabor-mezei-arm gabor-mezei-arm removed the needs-ci Needs to pass CI tests label May 7, 2025
@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch 2 times, most recently from fec00a0 to 820ed0d Compare May 7, 2025 15:41
@bjwtaylor bjwtaylor self-requested a review May 8, 2025 14:09
bjwtaylor
bjwtaylor previously approved these changes May 8, 2025
@gabor-mezei-arm gabor-mezei-arm moved this from In Development to In Review in Roadmap pull requests (new board) May 9, 2025
Copy link
Contributor

@felixc-arm felixc-arm left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I see there's a CI failure saying Error: Test case not executed: test_suite_shax;SHA-3 invalid param. It also seems to be present in the TF-PSA-Crypto patch so I'm not sure if the fix would be here or (more likely) in the TF-PSA-Crypto patch. Otherwise LGTM 🙂

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label May 12, 2025
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels May 12, 2025
@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch from 820ed0d to 56e107c Compare May 12, 2025 12:37
@mpg mpg self-requested a review May 13, 2025 10:12
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.

Looks good to me, thanks!

As discussed on the crypto PR, I think we can first merge this without updating the crypto pointer, then merge the crypto PR, and be done with it (no mutual dependency, no follow-up). Can you try removing the commit that updates the crypto pointer? Then if we get a green CI I'll formally approve.

Note: I've compared the error.c files generated before and after this PR, and all the changes look good to me. You've even fixed a minor indentation inconsistency in #if statements, thanks :)

@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch from 56e107c to 4de2732 Compare May 14, 2025 15:33
@mpg
Copy link
Contributor

mpg commented May 15, 2025

As discussed on the crypto PR, I think we can first merge this without updating the crypto pointer

I see you're still updating the crypto pointer here. Do you disagree with the proposed approach? If so, can you explain what you think I'm missing?

Otherwise, can you push a version of this PR with just the last commit removed (ie keeping all changes except for the submodule update)? If it passes CI that way, we can merge it without waiting for the crypto PR, making things much simpler.

@gabor-mezei-arm
Copy link
Contributor Author

gabor-mezei-arm commented May 15, 2025

I see you're still updating the crypto pointer here. Do you disagree with the proposed approach? If so, can you explain what you think I'm missing?

I only want to test the changes of the crypto repo.

@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch from 4de2732 to 1ecf015 Compare May 15, 2025 11:42
@mpg
Copy link
Contributor

mpg commented May 16, 2025

I only want to test the changes of the crypto repo.

Ok, makes sense. Note that this can be done by starting a job manually with all the branches you want as parameters: https://mbedtls.trustedfirmware.org/job/mbed-tls-restricted-pr-test-parametrized/build?delay=0sec

Well looks like the CI fully disagrees with my idea that we could get the Mbed TLS PR merged first, without updating the submodule. So, I officially withdraw that suggestion, sorry.

Let's go back to Gille's plan: let's try to get the crypto PR merged first (ie, with a clean CI on its own, not through the companion mbedtls CI). IIRC there was a point where the only CI issue there was check_names.py and we thought we could just pacify it with a dummy #define to be removed later.

Adapt the `generate_errors.pl` to handle `PSA_WANT` macros and
update to handle SHA3 macros.

Signed-off-by: Gabor Mezei <[email protected]>
The markers for the generated code need to indented due to the code style check.
During the replacement remove the spaces along with the markers.

Signed-off-by: Gabor Mezei <[email protected]>
@gabor-mezei-arm gabor-mezei-arm force-pushed the remove_sha3_config_option branch from 1ecf015 to 04b18e2 Compare June 3, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement needs-work priority-high High priority - will be reviewed soon
Projects
Development

Successfully merging this pull request may close these issues.

Remove temporary test component
5 participants