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.

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

Move MbedTLS Macros #9915

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Move MbedTLS Macros #9915

merged 3 commits into from
Feb 4, 2025
+8 −2

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Jan 20, 2025 •
edited
Loading

Move SSL macros from Mbed TLS to TF-PSA-Crypto.

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: macro move.
  • development PR provided.
  • framework PR provided Mbed-TLS/mbedtls-framework: #117
  • 3.6 PR not required because: 4.0
  • 2.28 PR not required because: 4.0
  • TF-PSA-Crypto provided: #143
  • tests provided | not required because:

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

Sorry, something went wrong.

@Harry-Ramsey Harry-Ramsey 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 size-s Estimated task size: small (~2d) labels Jan 20, 2025
@Harry-Ramsey Harry-Ramsey self-assigned this Jan 20, 2025
This was referenced Jan 20, 2025
Merged
Merged
@valeriosetti valeriosetti added priority-high High priority - will be reviewed soon and removed needs-reviewer This PR needs someone to pick it up for review labels Jan 20, 2025
@Harry-Ramsey Harry-Ramsey changed the title Move SSL Macros Move MbedTLS Macros Jan 22, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch 7 times, most recently from eb8e380 to 378cdec Compare January 27, 2025 11:33
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch 6 times, most recently from 3805371 to f24e4d4 Compare January 30, 2025 21:55
valeriosetti
valeriosetti reviewed Jan 31, 2025
View reviewed changes
library/ssl_misc.h Outdated
@@ -20,6 +20,9 @@

#include "psa/crypto.h"
#include "psa_util_internal.h"
#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that MBEDTLS_USE_PSA_CRYPTO was recently removed from the development branch as it's assumed to be always set (#9923). Therefore I think we can just remove the guard here.

include/mbedtls/mbedtls_config.h Outdated
@@ -1159,6 +1159,7 @@
* Module: library/x509_create.c
*
* Requires: MBEDTLS_BIGNUM_C, MBEDTLS_OID_C, MBEDTLS_PK_PARSE_C,
* MBEDTLS_MD_C
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the commit message that introduced this change should be updated because it's not related to what the commit actually implements.

valeriosetti
valeriosetti requested changes Jan 31, 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.

Sorry, I forgot to select the "request for change" box before submitting the review.

ronald-cron-arm
ronald-cron-arm requested changes Jan 31, 2025
View reviewed changes
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch 3 times, most recently from 2e396c4 to c20fb07 Compare January 31, 2025 12:45
ronald-cron-arm
ronald-cron-arm requested changes Jan 31, 2025
View reviewed changes
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from c20fb07 to f51bda4 Compare January 31, 2025 13:55
@Harry-Ramsey
Move SSL macro checks from TF-PSA-Crypto to Mbed TLS
2547ae9 
This commit moves macro checks specifically for Mbed TLS from
TF-PSA-Crypto to Mbed TLS where they more approriately belong.

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from f51bda4 to c86cc62 Compare January 31, 2025 13:58
@ronald-cron-arm
Copy link
Contributor

Given that MBEDTLS_X509_USE_C now depends on MBEDTLS_MD_C, in crypto-config-suite-b.h we should now enable
MBEDTLS_MD_C as MBEDTLS_X509_USE_C is enabled in config-suite-b.h.

@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Jan 31, 2025 •
edited
Loading

The issue with test_psa_crypto_config_accel_hmac is more problematic. We will need to discuss that one with @mpg probably.

@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from c86cc62 to 8f5963d Compare January 31, 2025 16:05
ronald-cron-arm
ronald-cron-arm requested changes Feb 3, 2025
View reviewed changes
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from 8f5963d to 991c34e Compare February 3, 2025 09:06
@ronald-cron-arm
Copy link
Contributor

The issue with test_psa_crypto_config_accel_hmac is more problematic. We will need to discuss that one with @mpg probably.

Discussing this with @mpg, actually x509.c (MBEDTLS_X509_USE_C) and x509_create.c (MBEDTLS_X509_CREATE_C) do not depend on MD. In x509 library PKCS7 still depends on MD but not the x509*.c modules. Thus we should just remove the commits adding these dependencies.

@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from 991c34e to f467dbf Compare February 3, 2025 09:46
ronald-cron-arm
ronald-cron-arm previously approved these changes Feb 3, 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

@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch 3 times, most recently from 1a7e446 to 29c68a8 Compare February 3, 2025 15:21
valeriosetti
valeriosetti previously approved these changes Feb 3, 2025
View reviewed changes
ronald-cron-arm added a commit to Mbed-TLS/TF-PSA-Crypto that referenced this pull request Feb 3, 2025
@ronald-cron-arm
Merge pull request #143 from Harry-Ramsey/check-names
d6031a2 
Approved by Mbed-TLS/mbedtls#9915 CI, merging.
@Harry-Ramsey
Update TF-PSA-Crypto pointer
285722a 
This commit updates the TF-PSA-Crypto pointer to include the moved
config files.

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
@Harry-Ramsey
Update framework pointer
93a496e 
This commit updates the framework pointer to include changes to enable
check_names.py to run independently for TF-PSA-Crypto and Mbed TLS.

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
@Harry-Ramsey Harry-Ramsey force-pushed the move-ssl-macros-development branch from 29c68a8 to 93a496e Compare February 4, 2025 08:13
ronald-cron-arm
ronald-cron-arm approved these changes Feb 4, 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

@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Feb 4, 2025
Merged via the queue into Mbed-TLS:development with commit 2a992bf Feb 4, 2025
6 checks passed
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

@valeriosetti valeriosetti

Assignees

@Harry-Ramsey Harry-Ramsey

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)
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Harry-Ramsey @ronald-cron-arm @valeriosetti