Conversion function from ecp group to PSA curve #8664

valeriosetti · 2023-12-28T16:51:02Z

Resolves #7764

Depends on #8668

PR checklist

changelog provided
backport not required
tests provided

mpg

Looking pretty good. Apart from minor things, the only non-trivial point is about removal of bit_size_is_sloppy.

library/psa_crypto_ecp.c

library/psa_util.c

tests/suites/test_suite_psa_crypto.data

tests/suites/test_suite_psa_crypto.function

mpg · 2023-12-29T08:47:09Z

Also, the CI has quite a lot of things to say. I didn't check, but since I saw a test case failing in the first component, I assume we'll have a lot of occurrences of the same failure:

[2023-12-28T19:38:55.711Z]          PSA import ECC_KEY_PAIR(SECP_K1) 224-bit curve not supported ...... FAILED
[2023-12-28T19:38:55.711Z]            psa_import_key(&attributes, key_material->x, key_material->len, &key_id) == PSA_ERROR_NOT_SUPPORTED
[2023-12-28T19:38:55.711Z]            at line 26, C:/Windows/workspace/mbed-tls-pr-head_PR-8664-head/worktrees/tmpixgncy05/tests/suites/test_suite_psa_crypto_not_supported.function
[2023-12-28T19:38:55.711Z]            lhs = 0xffffffffffffff79 = -135
[2023-12-28T19:38:55.711Z]            rhs = 0xffffffffffffff7a = -134

I suggest we fix this one first, then check if there were others hidden in the results.

valeriosetti · 2023-12-29T08:52:13Z

Also, the CI has quite a lot of things to say. I didn't check, but since I saw a test case failing in the first component, I assume we'll have a lot of occurrences of the same failure:

Unfortunately yes. This is due to the fact that I tried to improve mbedtls_psa_ecp_load_representation() in order to return PSA_ERROR_INVALID_ARGUMENT and PSA_ERROR_INVALID_ARGUMENT when appropriate, instead of returning always the latter. I will try to check if there is a way to overcome this limitation.

Side note: those tests are supposed to be skipped (according to their description), but they are executed because SECP224K1 is commented out in crypto_config.h. Perhaps also this thing might be fixed sooner or later in this automatic generated test.

mpg

LGTM, thanks!

mpg · 2024-01-02T09:11:58Z

I had a look at the stability checks results, and found them OK:

A function is renamed - but that function was marked as internal in the previous release.
Some tests are marked as having disappeared but they just changed due to fixed the value for the secp224k1 private key.

scripts/mbedtls_dev/asymmetric_key_data.py

gilles-peskine-arm

This mostly looks good, and fulfils the requirements for these functions in #8657 as it currently stands. I'm requesting some minor improvements to the documentation and to the tests. Also an optional suggestion for a minor simplification in the implementation.

include/mbedtls/psa_util.h

library/psa_crypto_ecp.c

tests/suites/test_suite_psa_crypto.data

docs/psa-transition.md

ChangeLog.d/7764.txt

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

…_functions_fail() Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

The new name better reflects the fact that the 1st parameter is just the EC family and not the curve. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti · 2024-01-09T12:48:15Z

Rebase done. Almost all commits were kept unchanged a part from:

commits concerning aymmetric_key_data.py (which were reverted in the end anyway) due to conflicts. This part was already fixed in Fix incorrect test data for SECP_R1 in automatically generated tests #8668
last commit (500b92b) which removes support for secp224k1 in order to save few more bytes in the code

mpg

Just o note that I've checked the rebase and additional commit and I'm happy with those. Will do a final (hopefully!) round of review when the CI is green.

valeriosetti · 2024-01-10T13:26:35Z

Will do a final (hopefully!) round of review when the CI is green.

I suspect that the OpenCI encountered several errors in the last run as there are many yellow icons there. Can you please restart it?

gilles-peskine-arm · 2024-01-10T13:31:07Z

I've restarted the CI.

Since this curve is not supported in PSA (and it will not ever be in the future), we save a few bytes. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti · 2024-01-11T06:08:05Z

I forced pushed the last commit because the OpenCI failed again in "Pre-test checks".

mpg

LGTM, thanks!

gilles-peskine-arm

Looks good to me.

The ABI differences reported by the CI are about mbedtls_ecc_group_of_psa, which was previously documented as an experimental function, so not actually part of the public ABI, thus a false positive.

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 Dec 28, 2023

valeriosetti requested a review from mpg December 28, 2023 16:51

valeriosetti self-assigned this Dec 28, 2023

valeriosetti added this to To Do in Roadmap Board for Mbed TLS via automation Dec 28, 2023

mpg requested changes Dec 29, 2023

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from To Do to In Development Dec 29, 2023

valeriosetti force-pushed the issue7764 branch from 93e8fb9 to 5c9fbfc Compare December 29, 2023 12:36

mpg previously approved these changes Jan 2, 2024

View reviewed changes

mpg removed the needs-ci Needs to pass CI tests label Jan 2, 2024

valeriosetti mentioned this pull request Jan 2, 2024

Export the mbedtls_md_psa_alg_from_type function. #8340

Closed

gilles-peskine-arm self-requested a review January 3, 2024 10:24

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 3, 2024

mpg mentioned this pull request Jan 3, 2024

Test case generation framework: PSA key-type-not-supported #4076

Merged

mpg reviewed Jan 3, 2024

View reviewed changes

scripts/mbedtls_dev/asymmetric_key_data.py Outdated Show resolved Hide resolved

valeriosetti dismissed mpg’s stale review via 23db2d6 January 3, 2024 14:23

gilles-peskine-arm requested changes Jan 3, 2024

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 3, 2024

gilles-peskine-arm mentioned this pull request Jan 3, 2024

Fix incorrect test data for SECP_R1 in automatically generated tests #8668

Merged

3 tasks

valeriosetti requested review from mpg and gilles-peskine-arm January 4, 2024 12:18

valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jan 4, 2024

valeriosetti added 7 commits January 9, 2024 13:41

psa_util: update documentation of EC conversion functions

eca0714

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test_suite_psa_crypto: test also MBEDTLS_ECP_DP_MAX in ecc_conversion…

db6e029

…_functions_fail() Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psa_crypto_ecp: fix comment for secp224k1 in check_ecc_parameters

0d438fa

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

changelog: improve wording

2622b1d

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

changelog: improve wording

3afdd02

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psa_util: update documentation for PSA conversion functions

d0aa9c1

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

psa_util: rename parameter of mbedtls_ecc_group_from_psa

39faa9c

The new name better reflects the fact that the 1st parameter is just the EC family and not the curve. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue7764 branch from 0fa34db to 500b92b Compare January 9, 2024 12:46

mpg reviewed Jan 10, 2024

View reviewed changes

valeriosetti removed the needs-work label Jan 11, 2024

psa_crypto_ecp: remove support for secp224k1

19ec9e4

Since this curve is not supported in PSA (and it will not ever be in the future), we save a few bytes. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue7764 branch from 500b92b to 19ec9e4 Compare January 11, 2024 06:07

valeriosetti requested review from mpg and gilles-peskine-arm January 11, 2024 06:08

mpg added the needs-review Every commit must be reviewed by at least two team members, label Jan 11, 2024

mpg approved these changes Jan 11, 2024

View reviewed changes

gilles-peskine-arm approved these changes Jan 15, 2024

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from In Development to Has Approval Jan 15, 2024

gilles-peskine-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, labels Jan 15, 2024

gilles-peskine-arm mentioned this pull request Jan 17, 2024

Update PSA transition guide for the new PK-PSA bridge functions in 3.6 #8714

Closed

gilles-peskine-arm added this pull request to the merge queue Jan 18, 2024

Merged via the queue into Mbed-TLS:development with commit c9077cc Jan 18, 2024
4 of 6 checks passed

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Jan 18, 2024

gilles-peskine-arm mentioned this pull request Feb 6, 2024

Implement mbedtls_pk_get_psa_attributes #8738

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conversion function from ecp group to PSA curve #8664

Conversion function from ecp group to PSA curve #8664

valeriosetti commented Dec 28, 2023 •

edited

Loading

mpg left a comment

mpg commented Dec 29, 2023

valeriosetti commented Dec 29, 2023

mpg left a comment

mpg commented Jan 2, 2024

gilles-peskine-arm left a comment

valeriosetti commented Jan 9, 2024

mpg left a comment •

edited

Loading

valeriosetti commented Jan 10, 2024

gilles-peskine-arm commented Jan 10, 2024

valeriosetti commented Jan 11, 2024

mpg left a comment

gilles-peskine-arm left a comment

Conversion function from ecp group to PSA curve #8664

Conversion function from ecp group to PSA curve #8664

Conversation

valeriosetti commented Dec 28, 2023 • edited Loading

PR checklist

mpg left a comment

Choose a reason for hiding this comment

mpg commented Dec 29, 2023

valeriosetti commented Dec 29, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg commented Jan 2, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 9, 2024

mpg left a comment • edited Loading

Choose a reason for hiding this comment

valeriosetti commented Jan 10, 2024

gilles-peskine-arm commented Jan 10, 2024

valeriosetti commented Jan 11, 2024

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

valeriosetti commented Dec 28, 2023 •

edited

Loading

mpg left a comment •

edited

Loading