Skip to content

[tf-psa-crypto] Remove DHM module#175

Merged
ronald-cron-arm merged 6 commits intoMbed-TLS:developmentfrom
valeriosetti:issue9956-tfpsacrypto
Mar 6, 2025
Merged

[tf-psa-crypto] Remove DHM module#175
ronald-cron-arm merged 6 commits intoMbed-TLS:developmentfrom
valeriosetti:issue9956-tfpsacrypto

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Feb 12, 2025

Description

Helps resolving Mbed-TLS/mbedtls#9956

This PR introduces changes which are incompatible if tested with development branch of the main Mbed-TLS repo. Please use CI of Mbed-TLS/mbedtls#9972 to check if changes here are OK or not.

PR checklist

  • changelog not needed
  • framework PR not needed
  • mbedtls PR provided [development] Remove DHM module mbedtls#9972
  • tests not required because: we're removing support in this PR, so we're mostly removing tests here instead of adding them.

@valeriosetti valeriosetti self-assigned this Feb 12, 2025
@valeriosetti valeriosetti added 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 priority-high High priority - will be reviewed soon needs-ci Needs to pass CI tests labels Feb 12, 2025
Harry-Ramsey
Harry-Ramsey previously approved these changes Feb 13, 2025
Harry-Ramsey
Harry-Ramsey previously approved these changes Feb 16, 2025
@valeriosetti valeriosetti removed the needs-reviewer This PR needs someone to pick it up for review label Feb 24, 2025
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Feb 25, 2025
@valeriosetti
Copy link
Copy Markdown
Contributor Author

I'm removing the need-ci label because the CI of Mbed-TLS/mbedtls#9972 is green and that's the only one that takes into account the overall changes introduced by this and 9972.

Harry-Ramsey
Harry-Ramsey previously approved these changes Feb 27, 2025
Copy link
Copy Markdown
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.

This looks almost good to me, only some minor comments, thanks.

MBEDTLS_DHM_C is being removed so related test suites can be removed
as well.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti force-pushed the issue9956-tfpsacrypto branch from 34dfe6d to c2ebd20 Compare March 5, 2025 09:06
Harry-Ramsey
Harry-Ramsey previously approved these changes Mar 5, 2025
Copy link
Copy Markdown
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.

This looks almost good to me, just one thing why not merging psa_crypto_ffdh_params.h into psa_crypto_ffdh.h? I do not see why we would need two files.

@valeriosetti
Copy link
Copy Markdown
Contributor Author

This looks almost good to me, just one thing why not merging psa_crypto_ffdh_params.h into psa_crypto_ffdh.h? I do not see why we would need two files.

psa_crypto_ffdh_params.h is basically a long list of defines and adding it to psa_crypto_ffdh.h might make the latter not really readable IMO. I know it's mostly a matter of preference, but if you really don't like it this way, I can merge the two.

Copy link
Copy Markdown
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.

One last thing otherwise it looks good to me, thanks.

valeriosetti and others added 5 commits March 6, 2025 15:57
Standardized DH groups which were defined in dhm.h have been moved to
psa_crypto_ffdh_params.h. These are used by the psa_crypto_ffdh.c file
internally only.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
This commit also removes references to DHM module in the documentation
of other symbols in crypto_config.h.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Copy link
Copy Markdown
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.

@github-project-automation github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Mar 6, 2025
@ronald-cron-arm
Copy link
Copy Markdown
Contributor

PR validated by the CI of #9972. Ready to be merged.

@ronald-cron-arm ronald-cron-arm disabled auto-merge March 6, 2025 19:15
@ronald-cron-arm ronald-cron-arm merged commit 7d60bf1 into Mbed-TLS:development Mar 6, 2025
2 of 5 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review Every commit must be reviewed by at least two team members priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Development

Successfully merging this pull request may close these issues.

3 participants