Skip to content

Remove component uses of MBEDTLS_ECDSA_DETERMINISTIC #10367

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
Merged
merged 3 commits into from
Sep 2, 2025
Merged

Remove component uses of MBEDTLS_ECDSA_DETERMINISTIC #10367

merged 3 commits into from
Sep 2, 2025

Conversation

@davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Aug 20, 2025 •
edited
Loading

Fixes #10328

Remove all references to MBEDTLS_ECDSA_DETERMINISTIC from components-configuration-crypto.sh. Replace them with PSA_WANT_ALG_DETERMINISTIC_ECDSA.

This is safe because:

  • MBEDTLS_ECDSA_DETERMINISTIC is only ever unset in components in order to avoid errors from disabling its dependency MBEDTLS_HMAC_DRBG_C.
  • MBEDTLS_ECDSA_DETERMINISTIC is only ever defined in config_adjust_legacy_from_psa.h, and only if PSA_WANT_ALG_DETERMINISTIC_ECDSA is defined.

Therefore PSA_WANT_ALG_DETERMINISTIC_ECDSA's dependencies are a superset of MBEDTLS_ECDSA_DETERMINISTIC's dependencies and must include MBEDTLS_HMAC_DRBG_C, so disabling PSA_WANT_ALG_DETERMINISTIC_ECDSA is a sufficient substitute for disabling MBEDTLS_ECDSA_DETERMINISTIC.

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: Internal changes only
  • development PR here
  • TF-PSA-Crypto PR not required because: Mbed TLS testing change only
  • framework PR not required
  • 3.6 PR not required because: Removal of legacy options is 4.0-only
  • tests not required because: Changes to testing itself

@davidhorstmann-arm
Remove component uses of MBEDTLS_ECDSA_DETERMINISTIC
3492807 
Remove all references to MBEDTLS_ECDSA_DETERMINISTIC from
components-configuration-crypto.sh. Replace them with
PSA_WANT_ALG_DETERMINISTIC_ECDSA.

This is safe because:
* MBEDTLS_ECDSA_DETERMINISTIC is only ever unset in components in order
  to avoid errors from disabling its dependency MBEDTLS_HMAC_DRBG_C.
* MBEDTLS_ECDSA_DETERMINISTIC is only ever defined in
  config_adjust_legacy_from_psa.h, and only if
  PSA_WANT_ALG_DETERMINISTIC_ECDSA is defined.

Therefore PSA_WANT_ALG_DETERMINISTIC_ECDSA's dependencies are a superset
of MBEDTLS_ECDSA_DETERMINISTIC's dependencies and must include
MBEDTLS_HMAC_DRBG_C, so disabling PSA_WANT_ALG_DETERMINISTIC_ECDSA is a
sufficient substitute for disabling MBEDTLS_ECDSA_DETERMINISTIC.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm added needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) 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 labels Aug 20, 2025
@davidhorstmann-arm davidhorstmann-arm moved this from In Development to In Review in Roadmap pull requests (new board) Aug 20, 2025
@davidhorstmann-arm davidhorstmann-arm changed the title Remove component uses of MBEDTLS_ECDSA_DETERMINISTIC Remove component uses of MBEDTLS_ECDSA_DETERMINISTIC Aug 21, 2025
@davidhorstmann-arm davidhorstmann-arm removed the needs-ci Needs to pass CI tests label Aug 21, 2025
ronald-cron-arm
ronald-cron-arm requested changes Aug 25, 2025
View reviewed changes
tests/scripts/components-configuration-crypto.sh Outdated
scripts/config.py full
scripts/config.py unset MBEDTLS_HMAC_DRBG_C
scripts/config.py unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG
scripts/config.py -c $CRYPTO_CONFIG_H unset PSA_WANT_ALG_DETERMINISTIC_ECDSA # requires HMAC_DRBG
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm Aug 22, 2025

Choose a reason for hiding this comment

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

Suggested change
scripts/config.py -c $CRYPTO_CONFIG_H unset PSA_WANT_ALG_DETERMINISTIC_ECDSA # requires HMAC_DRBG
scripts/config.py unset PSA_WANT_ALG_DETERMINISTIC_ECDSA # requires HMAC_DRBG

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm Aug 22, 2025

Choose a reason for hiding this comment

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

"-c $CRYPTO_CONFIG_H was necessary at some point, but hasn't been for a while now. I had been planning to clean it up eventually. I know this isn’t the main focus of this PR, but would you mind cleaning it up across all the components-*.sh scripts?"

@github-project-automation github-project-automation bot moved this from In Review to In Development in Roadmap pull requests (new board) Aug 25, 2025
@davidhorstmann-arm
Remove uses of the -c $CRYPTO_CONFIG_H idiom
7cbeedc 
This is no longer needed as config.py knows where the crypto config file
is these days.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
ronald-cron-arm
ronald-cron-arm requested changes Aug 27, 2025
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.

There are some "scripts/config.py -f "$CRYPTO_CONFIG_H" instances as well and please do the change in the other components-*.sh as well.

@davidhorstmann-arm
Remove other cases of explicit crypto config file
b907dbc 
Remove unnecessary passing of the crypto config filename either with the
'-f' or '-c' switch, throughout all of the all.sh component files.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm
Copy link
Contributor Author

davidhorstmann-arm commented Aug 27, 2025

There are some "scripts/config.py -f "$CRYPTO_CONFIG_H" instances as well and please do the change in the other components-*.sh as well.

Sorry, I missed those, that should be done now.

ronald-cron-arm
ronald-cron-arm approved these changes Aug 27, 2025
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, thanks.

@ronald-cron-arm ronald-cron-arm moved this from In Development to In Review in Roadmap pull requests (new board) Aug 28, 2025
@valeriosetti valeriosetti self-requested a review September 1, 2025 15:07
@valeriosetti valeriosetti removed the needs-reviewer This PR needs someone to pick it up for review label Sep 1, 2025
valeriosetti
valeriosetti reviewed Sep 1, 2025
View reviewed changes
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

The PR looks OK to me. I have a couple of questions though:

  1. now that we removed MBEDTLS_ECDSA_DETERMINISTIC from the mbedtls repo, shouldn't we also comment this line out?
  2. I just realized that there are still a couple of usages of MBEDTLS_ECDSA_DETERMINISTIC in tf-psa-crypto (here and here). Do you think it's worth creating a new issue for the future to clean these up as well?

@davidhorstmann-arm
Copy link
Contributor Author

davidhorstmann-arm commented Sep 1, 2025 •
edited
Loading

I have a couple of questions though:

  1. now that we removed MBEDTLS_ECDSA_DETERMINISTIC from the mbedtls repo, shouldn't we also comment this line out?

  2. I just realized that there are still a couple of usages of MBEDTLS_ECDSA_DETERMINISTIC in tf-psa-crypto (here and here). Do you think it's worth creating a new issue for the future to clean these up as well?

I think the answer is yes to both, but the script is about converting testcases from legacy to PSA dependencies, so I think both of these can be done in one task.

@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Sep 1, 2025
Open
@davidhorstmann-arm
Copy link
Contributor Author

davidhorstmann-arm commented Sep 1, 2025

I've created #10380 as a followup.

@github-project-automation github-project-automation bot moved this from In Review to Has Approval in Roadmap pull requests (new board) Sep 1, 2025
@valeriosetti valeriosetti 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 1, 2025
@davidhorstmann-arm davidhorstmann-arm added this pull request to the merge queue Sep 2, 2025
Merged via the queue into Mbed-TLS:development with commit f790fb8 Sep 2, 2025
8 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Sep 2, 2025
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

@valeriosetti valeriosetti valeriosetti approved these changes

Assignees

No one assigned

Labels

approved Design and code approved - may be waiting for CI or backports size-s Estimated task size: small (~2d)

Projects

Roadmap pull requests (new board)
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix MBEDTLS_ECDSA_DETERMINISTIC in components-configuration-crypto.sh

3 participants

@davidhorstmann-arm @ronald-cron-arm @valeriosetti