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

Fix dependencies of mbedtls_pk_ec_ro and mbedtls_pk_ec_rw #8320

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Fix dependencies of mbedtls_pk_ec_ro and mbedtls_pk_ec_rw #8320

merged 4 commits into from
Oct 18, 2023

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Oct 6, 2023 •
edited by minosgalanakis
Loading

Description

This PR resolves #8263 by using !MBEDTLS_PK_USE_PSA_EC_DATA as guard for mbedtls_pk_ec_[ro/rw] and fixing related issues.

PR checklist

  • changelog not required as nothing changes for the end user
  • backport not required
  • tests not required as no new code is introduced and this part is already tested by existing ones

@valeriosetti
pk_internal: change guards for mbedtls_pk_ec_[ro/rw]
d3925d2 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
ssl: fix getting group id in ssl_check_key_curve()
e7cefae 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti added 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-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Oct 6, 2023
@valeriosetti valeriosetti requested a review from mpg October 6, 2023 11:24
@valeriosetti valeriosetti self-assigned this Oct 6, 2023
@valeriosetti
ssl: reorganize guards surrounding ssl_get_ecdh_params_from_cert()
4d0e846 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Oct 12, 2023
mpg
mpg reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM, but I suggested a small improvement.

library/ssl_tls12_server.c Outdated
@@ -676,7 +676,11 @@ static int ssl_check_key_curve(mbedtls_pk_context *pk,
uint16_t *curves_tls_id)
{
uint16_t *curr_tls_id = curves_tls_id;
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use mbedtls_pk_get_group_id().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I added that function but I totally forgot about it :D

@valeriosetti
ssl: improve ssl_check_key_curve()
74cb404 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti requested a review from mpg October 16, 2023 11:52
mpg
mpg approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@mpg mpg 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 self-requested a review October 17, 2023 10:23
gabor-mezei-arm
gabor-mezei-arm approved these changes Oct 18, 2023
View reviewed changes
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 Oct 18, 2023
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 18, 2023
Merged via the queue into Mbed-TLS:development with commit f6f4695 Oct 18, 2023
1 check passed
@valeriosetti valeriosetti deleted the issue8263 branch December 6, 2024 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabor-mezei-arm gabor-mezei-arm gabor-mezei-arm approved these changes

@mpg mpg mpg approved these changes

Assignees

@valeriosetti valeriosetti

Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix dependencies of mbedtls_pk_ec_ro and mbedtls_pk_ec_rw
4 participants
@valeriosetti @mpg @gabor-mezei-arm @gilles-peskine-arm