Update the `hashes` domain to use PSA macros in `depends.py` #9634

gabor-mezei-arm · 2024-09-24T18:47:01Z

Description

In depends.py use PSA macros for the hashes domain.

Resolve #9144 #10203
~~Depends on #10145 #9292~~ (merged)

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.

changelog not required because: test related changes
development PR provided this is
framework PR not required
3.6 PR not required because: 4.0 related changes
2.28 PR not required because: 4.0 related changes
tests provided

minosgalanakis

Looks good. Some minor non blocking comments.

minosgalanakis · 2025-01-17T09:55:31Z

tests/scripts/depends.py

+                alter_names = {'!' + symbol for symbol in config_settings.keys()}
+                job = Job(description, config_settings, commands, alter_names)
+                self.jobs.append(job)
+                valid_symbols -= config_settings.keys()


Question.

Updating the valid_symbols while iterating through the mutual_exlusion implies that that a single valid_symbol can only pair with a single group. So we cannot have two groups that depend on the same valid_symbol

Is that intended as per design?

minosgalanakis · 2025-01-17T10:00:05Z

tests/scripts/depends.py

            # across various modules, but it depends on either SHA256 or SHA512.
            # As a consequence an "exclusive" test of anything other than SHA256
            # or SHA512 with MBEDTLS_ENTROPY_C enabled is not possible.
+            # Note for update: when MBEDTLS_SHA3_C is removed the mutual_exclusion


We need a common operator to indicate things that need to be done in the documentation. Until that is decided shoud we use TODO: which is global?

gabor-mezei-arm · 2025-01-29T11:18:20Z

Rebased

ronald-cron-arm

First, I see the grouping mechanism (or mutual_exclusion) as a improvement but not really as something necessary.

In terms of complementary domain jobs, we should have !PSA_WANT_ALG_SHA3_224 !PSA_WANT_ALG_SHA3_256 !PSA_WANT_ALG_SHA3_384 !PSA_WANT_ALG_SHA3_512. I know in all of them MBEDTLS_SHA3_C is enabled but they are not completely equivalent, we have some code guarded by only PSA_WANT_ALG_SHA3_224/256 ... In all these jobs, MBEDTLS_SHA3_C is indeed not disabled but we have the same kind of situation with !PSA_WANT_ALG_SHA_224/384 and !PSA_WANT_ALG_SHA_256/512 (sha256/512.c "enabled").

I agree that it would be nice to keep the case where MBEDTLS_SHA3_C is disabled and have on top of the above jobs, the job !(PSA_WANT_ALG_SHA3_224 PSA_WANT_ALG_SHA3_256 PSA_WANT_ALG_SHA3_384 PSA_WANT_ALG_SHA3_512) as we have currently.

Thus we could add grouping support for complementary and exclusive domains. It should then probably be a "groups" parameter not a "mutual_exclusion" as the latter does not fit well with exclusive domains I'd say. We would have then for hashes, three groups:

PSA_WANT_ALG_SHA3_224 + PSA_WANT_ALG_SHA3_256 + PSA_WANT_ALG_SHA3_384 + PSA_WANT_ALG_SHA3_512
PSA_WANT_ALG_SHA_224 + PSA_WANT_ALG_SHA_256
PSA_WANT_ALG_SHA_384 + PSA_WANT_ALG_SHA_512

I propose to move the grouping support in another PR and here to do only the changes to reach the point where for hashes we have the following jobs:
!PSA_WANT_ALG_SHA3_224 !PSA_WANT_ALG_SHA3_256 !PSA_WANT_ALG_SHA3_384 !PSA_WANT_ALG_SHA3_512 !PSA_WANT_ALG_MD5 !PSA_WANT_ALG_RIPEMD160 !PSA_WANT_ALG_SHA_1 !PSA_WANT_ALG_SHA_224 !PSA_WANT_ALG_SHA_256 !PSA_WANT_ALG_SHA_384 !PSA_WANT_ALG_SHA_512 PSA_WANT_ALG_SHA_256 PSA_WANT_ALG_SHA_512

See my comment below about disabling MBEDTLS_SHA3_C as a reverse dependency.

ronald-cron-arm · 2025-03-31T08:36:35Z

tests/scripts/depends.py

+    'PSA_WANT_ALG_SHA3_224': ['MBEDTLS_SHA3_C'],
+    'PSA_WANT_ALG_SHA3_256': ['MBEDTLS_SHA3_C'],
+    'PSA_WANT_ALG_SHA3_384': ['MBEDTLS_SHA3_C'],
+    'PSA_WANT_ALG_SHA3_512': ['MBEDTLS_SHA3_C'],


For !PSA_WANT_ALG_SHA3_224 !PSA_WANT_ALG_SHA3_256 !PSA_WANT_ALG_SHA3_384 !PSA_WANT_ALG_SHA3_512 (see my general comment on the PR) this is problematic. We are close to remove the MBEDTLS_SHA3_C config option, thus I would say let's just remove that now.

mpg · 2025-04-25T08:18:02Z

I propose to move the grouping support in another PR

Yes, I think we should really prioritize the minimal changes that are needed in order to unblock other work, and leave any improvements for a follow-up.

mpg · 2025-05-07T11:31:06Z

@gabor-mezei-arm Thanks for updating this. Unfortunately, there are now conflicts, and also the CI has complaints about the code style. Can you fix that and then re-request a review?

As of for the other depends.py PR, we have a DI that depends on this, and we're under (justified IMO) pressure to close off our DI taks sooner rather than later.

gabor-mezei-arm · 2025-05-07T15:46:20Z

The MBEDTLS_SHA3_C macro removal is done in #10145 and this PR depends on it.

mpg · 2025-05-12T07:50:22Z

The MBEDTLS_SHA3_C macro removal is done in #10145 and this PR depends on it.

Ah, I wasn't aware, thanks for the info!

Signed-off-by: Gabor Mezei <[email protected]>

ronald-cron-arm

LGTM, thanks.

mpg · 2025-06-12T09:26:02Z

@minosgalanakis This is ready for re-review (and a prompt re-review would be greatly appreciated).

minosgalanakis

LGTM

gabor-mezei-arm added enhancement needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) component-test Test framework and CI scripts priority-high High priority - will be reviewed soon labels Sep 24, 2024

gabor-mezei-arm self-assigned this Sep 24, 2024

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from 8ab32ac to fcfc93d Compare September 25, 2024 11:53

gabor-mezei-arm mentioned this pull request Oct 7, 2024

Support the crypto_config.h usage in depends.py #9292

Merged

5 tasks

gilles-peskine-arm mentioned this pull request Oct 22, 2024

Always enable MBEDTLS_USE_PSA_CRYPTO in all.sh #9614

Merged

5 tasks

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from fcfc93d to baa11a9 Compare January 13, 2025 15:11

gabor-mezei-arm removed the needs-preceding-pr Requires another PR to be merged first label Jan 13, 2025

minosgalanakis self-requested a review January 13, 2025 15:20

gabor-mezei-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Jan 14, 2025

ronald-cron-arm self-requested a review January 15, 2025 08:43

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 15, 2025

minosgalanakis previously approved these changes Jan 17, 2025

View reviewed changes

gabor-mezei-arm dismissed minosgalanakis’s stale review via c5404b2 January 17, 2025 16:26

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from 0e4ecd0 to 7151645 Compare January 29, 2025 11:17

gabor-mezei-arm requested a review from minosgalanakis January 31, 2025 15:15

ronald-cron-arm requested changes Mar 31, 2025

View reviewed changes

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from 7151645 to 8d62e8f Compare April 24, 2025 14:57

gabor-mezei-arm added needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Apr 24, 2025

mpg removed the needs-preceding-pr Requires another PR to be merged first label Apr 25, 2025

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from 8d62e8f to 48372ed Compare April 28, 2025 10:03

gabor-mezei-arm added the needs-preceding-pr Requires another PR to be merged first label May 7, 2025

ronald-cron-arm mentioned this pull request May 13, 2025

Update the pkalgs domain to use PSA macros in depends.py #9654

Merged

5 tasks

gabor-mezei-arm linked an issue Jun 5, 2025 that may be closed by this pull request

Remove temporary test component #10203

Closed

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from 48372ed to b65099a Compare June 6, 2025 10:33

gabor-mezei-arm added 2 commits June 10, 2025 15:40

Use PSA macros for the hashes domain

ef013a6

Signed-off-by: Gabor Mezei <[email protected]>

Remove temporary component created for SHA3 testing

3795f8a

Signed-off-by: Gabor Mezei <[email protected]>

gabor-mezei-arm force-pushed the 9144_update_depends.py_hashes_domain branch from b65099a to 3795f8a Compare June 10, 2025 14:34

gabor-mezei-arm requested a review from ronald-cron-arm June 10, 2025 14:35

ronald-cron-arm removed the needs-preceding-pr Requires another PR to be merged first label Jun 11, 2025

ronald-cron-arm approved these changes Jun 11, 2025

View reviewed changes

mpg removed the needs-ci Needs to pass CI tests label Jun 12, 2025

minosgalanakis approved these changes Jun 12, 2025

View reviewed changes

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

gabor-mezei-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 Jun 12, 2025

ronald-cron-arm added this pull request to the merge queue Jun 13, 2025

Merged via the queue into Mbed-TLS:development with commit b1d3e2e Jun 13, 2025
6 checks passed

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

Update the hashes domain to use PSA macros in depends.py #9634

Update the hashes domain to use PSA macros in depends.py #9634

Uh oh!

Conversation

gabor-mezei-arm commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

minosgalanakis Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

gabor-mezei-arm commented Jan 29, 2025

Uh oh!

ronald-cron-arm left a comment • 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!

ronald-cron-arm Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

mpg commented Apr 25, 2025

Uh oh!

mpg commented May 7, 2025

Uh oh!

gabor-mezei-arm commented May 7, 2025

Uh oh!

mpg commented May 12, 2025

Uh oh!

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Uh oh!

mpg commented Jun 12, 2025

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Update the `hashes` domain to use PSA macros in `depends.py` #9634

Update the `hashes` domain to use PSA macros in `depends.py` #9634

gabor-mezei-arm commented Sep 24, 2024 •

edited

Loading

ronald-cron-arm left a comment •

edited

Loading