driver-only ECC: ECPf.PK testing #7682

valeriosetti · 2023-06-05T10:31:38Z

This PR re-enables the PK module while keeping the ECP one disabled in the following test components:

component_test_psa_crypto_full_accel_all_ec_algs_no_ecp_use_psa
component_test_psa_crypto_full_reference_all_ec_algs_no_ecp_use_psa

Depends on #7717
Resolve #7453

PR checklist

changelog will be provided in driver-only ECC: ECPf wrap-up #7452
backport not required because it's a new feature
tests not required

valeriosetti · 2023-06-05T10:32:42Z

Note for reviewers: this PR starts at de15ba2

mpg

Looking pretty good, just a few suggestions for improvements.

include/mbedtls/pk.h

library/pk_wrap.c

library/pk.c

library/pkparse.c

tests/suites/test_suite_pk.function

tests/scripts/analyze_outcomes.py

library/pkparse.c

mpg

Looks almost entirely good to me! Just one last point - upon closer inspection, I changed my mind about a suggestion I made, sorry.

library/pkparse.c

mpg

LGTM, thanks!

mpg

LGTM. Thanks for noticing the missing free() call that I misses. (Or was it caught by the CI?)

valeriosetti · 2023-06-09T14:28:12Z

Or was it caught by the CI?

It was caught by the CI indeed. At first I didn't realize that pk_convert_compressed_ec() was allocating the mbedtls_ecp_keypair directly on its own stack so of course this has to be cleaned on exit ':)

mpg · 2023-06-12T15:45:53Z

tests/scripts/all.sh

@@ -2452,7 +2429,6 @@ component_test_psa_crypto_full_accel_all_ec_algs_no_ecp_use_psa () {
    not grep mbedtls_ecjpake_ library/ecjpake.o
    # Also ensure that ECP or RSA modules were not re-enabled


RSA should no longer be mentioned here

This still stands

mpg · 2023-06-12T16:22:28Z

Note: there a few remaining occurrences of ECP_LIGHT that should be changed to MBEDTLS_PK_HAVE_ECC_KEYS. In particular, those that guard the #include "pkwrite.h" in pkparse.c and pkwrite.c. After this PR, trying to build and run tests in a configuration similar to config_psa_crypto_full_all_ec_algs_no_ecp_use_psa but without RSA would fail (even after the check in check_config.h has been fixed).

I'm not sure if we should consider fixing this in scope for this PR, or leave it for a follow-up, but it will need to be fixed at some point.

Also, I'm starting to wonder if have #7717 depend on this one is really the right way, or if we should have this one depending on 7717 instead. Possible reasons for reversing the dependency:

7717 belongs to an earlier EPIC than this one, so it's more logical for later things to depend on ealiers ones,
with 7717 done, I think here we should be able to achieve a state where ECP_LIGHT is never referenced anymore in any pk*.c or pk*.h file - which is easier to review as having to go over all uses and think if they're OK or not.

Wdyt?

valeriosetti · 2023-06-13T12:43:03Z

I'm not sure if we should consider fixing this in scope for this PR, or leave it for a follow-up, but it will need to be fixed at some point.

I'm more for a follow up PR if this is not strictly mandatory to make the CI happy for this one. This helps in keeping the PRs smaller and easy to review.

with 7717 done, I think here we should be able to achieve a state where ECP_LIGHT is never referenced anymore in any pk*.c or pk*.h file - which is easier to review as having to go over all uses and think if they're OK or not.

Ok, this sounds to me like a major benefit of reversing the dependency, so I'll work on this for both PRs

mpg · 2023-06-14T07:55:08Z

I'm more for a follow up PR if this is not strictly mandatory to make the CI happy for this one. This helps in keeping the PRs smaller and easy to review.

Agreed.

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

Note: both MBEDTLS_PK_USE_PSA_EC_DATA and MBEDTLS_PK_HAVE_ECC_KEYS has been move on top of the pk.h file because we need these symbols when crypto.h is evaluated otherwise functions like mbedtls_ecc_group_of_psa() won't be available. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

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

…group_of_psa() This allows also to: - removing the dependency on ECP_C for these functions and only rely on PSA symbols - removing extra header inclusing from crypto_extra.h - return MBEDTLS_PK_USE_PSA_EC_DATA and MBEDTLS_PK_HAVE_ECC_KEYS to their original position in pk.h Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

…ECP at all Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

tests/scripts/analyze_outcomes.py

AndrzejKurek · 2023-06-20T10:42:30Z

tests/suites/test_suite_pkparse.data

 pk_parse_public_keyfile_ec:"data_files/ec_bp512_pub.pem":0

 Parse Public EC Key #9a (RFC 5480, brainpoolP512r1, compressed)
 depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_PK_PARSE_EC_COMPRESSED:MBEDTLS_ECP_DP_BP512R1_ENABLED
 pk_parse_public_keyfile_ec:"data_files/ec_bp512_pub.comp.pem":0

 Parse Public EC Key #10 (RFC 8410, DER, X25519)
-depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_LIGHT:MBEDTLS_ECP_DP_CURVE25519_ENABLED
+depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED


Almost all of these tests have duplicated "depends_on" field, MBEDTLS_PK_HAVE_ECC_KEYS, with the place in the function file,

/* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_PK_HAVE_ECC_KEYS */ void pk_parse_public_keyfile_ec(char *key_file, int result)

If all test cases that call this function need this define, then there's no use of adding it again in the .data file.
If only selected test cases need this define, please remove it from the .function file.

Correct! Thanks for noticing this ;)

AndrzejKurek

Only one remark about the tests left.

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

mpg

LGTM

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 Jun 5, 2023

valeriosetti requested a review from mpg June 5, 2023 10:31

valeriosetti self-assigned this Jun 5, 2023

valeriosetti force-pushed the issue7453 branch 2 times, most recently from dfda836 to 80fc0b5 Compare June 6, 2023 10:43

valeriosetti removed the needs-ci Needs to pass CI tests label Jun 6, 2023

mpg requested changes Jun 7, 2023

View reviewed changes

mpg reviewed Jun 7, 2023

View reviewed changes

library/pkparse.c Outdated Show resolved Hide resolved

valeriosetti requested a review from mpg June 7, 2023 15:54

mpg requested changes Jun 8, 2023

View reviewed changes

library/pkparse.c Outdated Show resolved Hide resolved

valeriosetti force-pushed the issue7453 branch from 5cd529b to 40df03f Compare June 8, 2023 08:56

valeriosetti requested a review from mpg June 8, 2023 08:57

mpg previously approved these changes Jun 8, 2023

View reviewed changes

valeriosetti mentioned this pull request Jun 8, 2023

Driver-only ECC: auto-enable ECP_LIGHT when needed #7717

Merged

3 tasks

valeriosetti dismissed mpg’s stale review via 5c87f7c June 8, 2023 13:25

valeriosetti force-pushed the issue7453 branch from 40df03f to 5c87f7c Compare June 8, 2023 13:25

valeriosetti requested a review from mpg June 8, 2023 13:25

mpg previously approved these changes Jun 9, 2023

View reviewed changes

mpg mentioned this pull request Jun 9, 2023

Driver-only ECC: goals and strategy #6839

Closed

mpg reviewed Jun 12, 2023

View reviewed changes

mpg changed the title ~~driver-only ECC: ECP.PK starter~~ driver-only ECC: ECPf.PK testing Jun 13, 2023

valeriosetti added 9 commits June 19, 2023 19:23

test: re-enable PK and RSA in component without ECP/ECP_LIGHT

1814bb7

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

library: replace occurencies of ECP_LIGHT with PK_HAVE_ECC_KEYS

81d7512

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

test: replace occurencies of ECP_LIGHT with PK_HAVE_ECC_KEYS

545a0d6

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

pk: remove useless internal function

30fdc03

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

build_info: do not enable ECP_LIGHT when PSA_WANT_ALG_ECDSA

f54ca35

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

psa: move mbedtls_ecc_group_to_psa() from inline function to standard…

bc2b1d3

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

pkwrite: fix wrong guard position for pk_get_opaque_ec_family()

e165136

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

valeriosetti force-pushed the issue7453 branch from 71a2118 to b0a3ae2 Compare June 19, 2023 17:33

test: ignore compressed points' tests when checking coverage without …

5bd2523

…ECP at all Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue7453 branch from b0a3ae2 to 5bd2523 Compare June 20, 2023 07:06

mpg reviewed Jun 20, 2023

View reviewed changes

tests/scripts/analyze_outcomes.py Show resolved Hide resolved

mpg previously approved these changes Jun 20, 2023

View reviewed changes

mpg mentioned this pull request Jun 20, 2023

Split psa_util.h between internal and public #7716

Merged

3 tasks

AndrzejKurek reviewed Jun 20, 2023

View reviewed changes

AndrzejKurek suggested changes Jun 20, 2023

View reviewed changes

test: remove duplicate PK_HAVE_ECC_KEYS dependency in pkparse suite

4b3c02b

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

valeriosetti dismissed mpg’s stale review via 4b3c02b June 21, 2023 09:23

valeriosetti requested review from AndrzejKurek and mpg June 21, 2023 09:23

mpg approved these changes Jun 21, 2023

View reviewed changes

AndrzejKurek approved these changes Jun 21, 2023

View reviewed changes

AndrzejKurek added needs-ci Needs to pass CI tests 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 Jun 21, 2023

mpg removed the needs-ci Needs to pass CI tests label Jun 22, 2023

mpg merged commit 2fb9d00 into Mbed-TLS:development Jun 22, 2023

valeriosetti deleted the issue7453 branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver-only ECC: ECPf.PK testing #7682

driver-only ECC: ECPf.PK testing #7682

valeriosetti commented Jun 5, 2023 •

edited by mpg

Loading

valeriosetti commented Jun 5, 2023

mpg left a comment

mpg left a comment

mpg left a comment

mpg left a comment

valeriosetti commented Jun 9, 2023

mpg Jun 12, 2023

AndrzejKurek Jun 20, 2023

mpg commented Jun 12, 2023

valeriosetti commented Jun 13, 2023

mpg commented Jun 14, 2023

AndrzejKurek Jun 20, 2023

valeriosetti Jun 21, 2023

AndrzejKurek left a comment

mpg left a comment

		@@ -2452,7 +2429,6 @@ component_test_psa_crypto_full_accel_all_ec_algs_no_ecp_use_psa () {
		not grep mbedtls_ecjpake_ library/ecjpake.o
		# Also ensure that ECP or RSA modules were not re-enabled

driver-only ECC: ECPf.PK testing #7682

driver-only ECC: ECPf.PK testing #7682

Conversation

valeriosetti commented Jun 5, 2023 • edited by mpg Loading

PR checklist

valeriosetti commented Jun 5, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Jun 9, 2023

mpg Jun 12, 2023

Choose a reason for hiding this comment

AndrzejKurek Jun 20, 2023

Choose a reason for hiding this comment

mpg commented Jun 12, 2023

valeriosetti commented Jun 13, 2023

mpg commented Jun 14, 2023

AndrzejKurek Jun 20, 2023

Choose a reason for hiding this comment

valeriosetti Jun 21, 2023

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Jun 5, 2023 •

edited by mpg

Loading