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.

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

Replace MBEDTLS_PK_HAVE_ECC_KEYS with PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY #34

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

eleuzi01
Copy link
Contributor

@eleuzi01 eleuzi01 commented Jul 5, 2024

Part of #9492

@eleuzi01 eleuzi01 added enhancement New feature or request needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) and removed needs-ci Needs to pass CI tests labels Jul 5, 2024
@gabor-mezei-arm gabor-mezei-arm self-requested a review July 9, 2024 10:17
@eleuzi01 eleuzi01 added the priority-high High priority - will be reviewed soon label Jul 10, 2024
@eleuzi01 eleuzi01 added needs-work and removed 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 Jul 30, 2024
@@ -87,7 +87,7 @@ def dependencies_of_setting(cfg: config.Config,
if name.startswith('MBEDTLS_CIPHER_PADDING_'):
return 'MBEDTLS_CIPHER_C:MBEDTLS_CIPHER_MODE_CBC'
if name.startswith('MBEDTLS_PK_PARSE_EC_'):
return 'MBEDTLS_PK_C:MBEDTLS_PK_HAVE_ECC_KEYS'
return 'MBEDTLS_PK_C:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY'
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this change for development (to be able to merge #9492) and to keep MBEDTLS_PK_HAVE_ECC_KEYS for 3.6. There has been some discussion how to do that (see #51) but to unblock #9492 let's go with a local ad-hoc solution here based on build_tree.is_mbedtls_3_6(). Something like (not tested):

if build_tree.is_mbedtls_3_6():
    return 'MBEDTLS_PK_C:MBEDTLS_PK_HAVE_ECC_KEYS'
else:
    return 'MBEDTLS_PK_C:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY'

with a comment indicating that it is temporary and pointing to #51.

Otherwise, let's do the same in #31 and eventually update #9492 with both #31 and #34 changes in the framework.

@eleuzi01 eleuzi01 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-work labels Sep 25, 2024
@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Sep 26, 2024

@eleuzi01 This looks good to me and #31 as well, but you probably need to merge the two PRs in one (let's say this one) then you can update in #9492 the Mbed TLS framework pointer to the HEAD of this PR. Then we will also need an 3.6 PR that just update the framework to the head of that PR to validate the changes in 3.6 through Mbed TLS CI.

@eleuzi01
Copy link
Contributor Author

@eleuzi01 This looks good to me and #31 as well, but you probably need to merge the two PRs in one (let's say this one) then you can update in #9492 the Mbed TLS framework pointer to the HEAD of this PR. Then we will also need an 3.6 PR that just update the framework to the head of that PR to validate the changes in 3.6 through Mbed TLS CI.

Thanks for the help! I'm not used to working with submodules but I think I managed to clean this up.

And add temporary solution so it works with 3.6 and 4.0

Temporary solution to be resolved in Mbed-TLS#51.

Signed-off-by: Elena Uziunaite <[email protected]>
Signed-off-by: Elena Uziunaite <[email protected]>
@ronald-cron-arm
Copy link
Contributor

@eleuzi01 This looks good to me and #31 as well, but you probably need to merge the two PRs in one (let's say this one) then you can update in #9492 the Mbed TLS framework pointer to the HEAD of this PR. Then we will also need an 3.6 PR that just update the framework to the head of that PR to validate the changes in 3.6 through Mbed TLS CI.

Thanks for the help! I'm not used to working with submodules but I think I managed to clean this up.

That's fine no worries. Our workflow without CI for mbedtls-framework is also quite complicated. It looks all good now, well done. This is not the end of the story though. When this one is fully approved, we will merge it. Then you will need to update your last commit in #9492 and #9640 with the commit id of the merge of this PR into main. I will ping you at that time.

Copy link
Contributor

@gabor-mezei-arm gabor-mezei-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

@gabor-mezei-arm 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, needs-reviewer This PR needs someone to pick it up for review labels Sep 30, 2024
@ronald-cron-arm ronald-cron-arm merged commit 33ac133 into Mbed-TLS:main Sep 30, 2024
1 check passed
@ronald-cron-arm
Copy link
Contributor

Validated for dev by the CI of Mbed-TLS/mbedtls#9492, for 3.6 by the CI of Mbed-TLS/mbedtls#9640.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports enhancement New feature or request priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants