Define (private) "light" subset of ECP #7410

valeriosetti · 2023-04-05T16:33:39Z

The goal of this PR is to create a new symbol named MBEDTLS_ECP_LIGHT which can be used in conjunction with MBEDTLS_ECP_C to exclude some code from the ecp module at build time.
It basically works like this:

MBEDTLS_ECP_LIGHT only --> ecp module is built without its internal arithmetic functions
MBEDTLS_ECP_LIGHT + MBEDTLS_ECP_C --> ecp module is built entirely as it has been until now

This PR mimics the idea recently introduced by the MBEDTLS_MD_LIGHT symbol.

~~Depends on #7393~~
Resolves #7390

Gatekeeper checklist

changelog not required - we're keeping the new macro private, see Driver-only ECC: goals and strategy #6839 (comment) and Define (private) "light" subset of ECP #7410 (comment)
backport not required: it's an enhancement
tests not required: it uses the already existing ones

mpg

Looking pretty good to me, just one nit about a comment and a question about dependencies & parity.

include/mbedtls/build_info.h

library/pk_wrap.c

tests/suites/test_suite_psa_crypto.data

valeriosetti · 2023-04-07T10:57:46Z

~~Adding the need-work label because as @mpg correctly pointed out analyze_outcomes.py script is failing~~
Solved!
But I noticed that the list of conflicts in the merge is increased. I will rebase and fix them as soon as preceding PRs are merged.

mpg · 2023-04-11T09:22:25Z

changelog question: even thought the new symbol is internal and it's not present in mbedtls_config.h, the final user can still take advantage of it when compiling from the command line, so in my opinion this should be mentioned with a Changelog. Wdyt?

Related: #6839 (comment) - initially I was thinking fully internal as it would be a temporary thing until we complete the 2c EPIC, but now I see reasons for keeping it beyond that.

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

Changes in test_suite_psa_crypto are to enforce the dependency on ECP_C which is mandatory for some key's derivation. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

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

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

Only some test cases are skipped for which ECP_C is mandatory, but the other ones are included. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg · 2023-04-11T10:58:13Z

We should just mention that "part of the ECP module might be automatically re-enabled when some features are required by the user" and then list these cases.

But that's not really the case now, is it? The only case where ECP_LIGHT is auto-enabled in build_info.h right now is when ECP_C is enabled. If you want ECP_LIGHT without the full ECP_C right now the only way is -DMBEDTLS_ECP_LIGHT on the compiler's command line.

For MBEDTLS_PK_PARSE_EC_EXTENDED we could have it auto-enable ECP_LIGHT. But for compressed points and ECC key derivation, there is no feature macro we can use. So I think it's easier to go the other way round: let the user explicitly enable ECP_LIGHT is they want support for reading compressed points and/or deriving ECC keys on Short Weierstrass curves.

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

mpg

I'm afraid there's some confusion about limitations and what the current step is. I also have a number of suggestions about wording.

include/mbedtls/build_info.h

ChangeLog.d/ecp-light.txt

include/mbedtls/mbedtls_config.h

tests/scripts/all.sh

include/mbedtls/mbedtls_config.h

ChangeLog.d/ecp-light.txt

mpg

LGTM, thanks!

AndrzejKurek

Some questions left.

AndrzejKurek · 2023-04-14T09:20:42Z

library/ecp.c

+#if defined(MBEDTLS_ECP_C)
+static unsigned long add_count, dbl_count;
+#endif /* MBEDTLS_ECP_C */
+static unsigned long mul_count;
 #endif


/* MBEDTLS_ECP_LIGHT */

AndrzejKurek · 2023-04-14T09:36:15Z

tests/scripts/all.sh

-    make CFLAGS="$ASAN_CFLAGS -Werror -I../tests/include -I../tests -I../../tests -DPSA_CRYPTO_DRIVER_TEST -DMBEDTLS_TEST_LIBTESTDRIVER1 $loc_accel_flags" LDFLAGS="-ltestdriver1 $ASAN_CFLAGS"
+    loc_symbols="-DPSA_CRYPTO_DRIVER_TEST \
+                 -DMBEDTLS_TEST_LIBTESTDRIVER1 \
+                 -DMBEDTLS_ECP_LIGHT"


Why is this passed as a CFLAGS option, not in the config file?

Because we want to keep it private (mostly because it doesn't have well-defined semantics), so it can't appear in the config file.

Passing it in the CFLAGS is a temporary situation, though: in the future it will be auto-enabled based on options that need it, but we're going step by step.

AndrzejKurek · 2023-04-14T09:40:09Z

tests/scripts/analyze_outcomes.py

@@ -211,6 +210,44 @@ def do_analyze_driver_vs_reference(outcome_file, args):
                'test_suite_random': [
                    'PSA classic wrapper: ECDSA signature (SECP256R1)',
                ],
+                # In the accelerated test ECP_C is not set (only ECP_LIGHT is)


Why can't we also only enable ECP_LIGHT in component_test_psa_crypto_config_reference_all_ec_algs_use_psa instead?

For the same reason we're not disabling ECDSA_C in the reference component: we want the feature set of ECC-using modules (PK, X.509, TLS) with ECP_LIGHT && !ECP_C to be as close as possible to the feature set with ECP_C. That's why we're keeping ECP_C enabled in the component, and have an ignore list for test_suite_ecp. This is similar to how we have ECDSA_C in the reference but ignore the whole test_suite_ecdsa - except here we don't ignore the whole test_suite_ecp, just part of it.

AndrzejKurek · 2023-04-14T09:40:38Z

tests/ssl-opt.sh

@@ -5360,7 +5360,6 @@ run_test    "Authentication: server goodcert, client required, no trusted CA" \
 # occasion (to be fixed). If that bug's fixed, the test needs to be altered to use a
 # different means to have the server ignoring the client's supported curve list.



Shouldn't we require ECP_LIGHT here and below instead?

Good question. I don't think we should require ECP_LIGHT here: we already auto-detect based on using server5 that ECDSA support is required. Now perhaps we need to reflect somewhere that PK only has support for ECDSA when ECP_LIGHT is present. Let me check.

Ok, I checked and I think there are multiple sides to this:

Definition of MBEDTLS_PK_CAN_ECDSA_xxx macros in pk.h.

Definition of MBEDTLS_PK_HAVE_ECDSA helper macro in check_config.h.

Definition of requires_pk_alg() in ssl-opt.sh.

None of them are fully correct right now - that is, neither in this PR nor in development. Before this PR, in the USE_PSA case, we should require ECP_C in addition to PSA_WANT_ALG_ECDSA because without it, PK doesn't support ECC keys; after this PR it should be ECP_LIGHT.

Though since we don't want to make ECP_LIGHT public, again what we should do it not require it, but instead auto-enable it when needed, so for example PK_C && USE_PSA && PSA_WANT_ALG_ECDSA would auto-enable ECP_LIGHT and then I think all three of the above definitions would become correct.

AndrzejKurek

Approved based on follow-up work.

valeriosetti self-assigned this Apr 5, 2023

valeriosetti requested a review from mpg April 5, 2023 16:34

valeriosetti force-pushed the issue7390 branch from c51da00 to 4ee04a6 Compare April 5, 2023 17:06

mpg requested changes Apr 6, 2023

View reviewed changes

include/mbedtls/build_info.h Outdated Show resolved Hide resolved

library/pk_wrap.c Outdated Show resolved Hide resolved

tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved

mpg mentioned this pull request Apr 7, 2023

Investigate ECP light #7357

Closed

7 tasks

valeriosetti added the needs-work label Apr 7, 2023

valeriosetti force-pushed the issue7390 branch from 4ee04a6 to 2fcaa1b Compare April 7, 2023 13:55

valeriosetti removed the needs-work label Apr 7, 2023

mpg removed the needs-ci Needs to pass CI tests label Apr 11, 2023

valeriosetti force-pushed the issue7390 branch from 2fcaa1b to f75c822 Compare April 11, 2023 08:36

valeriosetti added 10 commits April 11, 2023 11:33

ecp: introduce new ECP_LIGHT symbol

fd122f4

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

pk: adapt to new ECP_LIGHT symbol

0d2980f

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

library: add remaining changes for the new ECP_LIGHT symbol

d4a5d46

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

test: let test_psa_crypto_config_accel_all_ec_algs_use_psa use ECP_LIGHT

29b395c

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

test: fix remaining failures in test due to the ECP_LIGHT symbol

d49cbc1

Changes in test_suite_psa_crypto are to enforce the dependency on ECP_C which is mandatory for some key's derivation. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

build_info: fixed comment

151bdf9

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

pk_wrap: fix guards in eckey_check_pair to only include 1 option at b…

3f8d23e

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

psa: fix ECP guards for key derivation

5278986

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

solve disparities for ECP_LIGHT between ref/accel

6c496a1

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

test: include also test_suite_ecp for the coverage analysis

0c477d3

Only some test cases are skipped for which ECP_C is mandatory, but the other ones are included. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed mpg’s stale review via a9aafd4 April 11, 2023 10:30

valeriosetti requested a review from mpg April 11, 2023 10:31

test: resolve remaining disparities in test_suite_ssl

9cea093

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

valeriosetti force-pushed the issue7390 branch 2 times, most recently from 32c56e5 to 4e1d32e Compare April 12, 2023 07:58

mpg requested changes Apr 12, 2023

View reviewed changes

valeriosetti force-pushed the issue7390 branch from 4e1d32e to a9c612e Compare April 12, 2023 09:32

valeriosetti requested a review from mpg April 12, 2023 09:41

valeriosetti force-pushed the issue7390 branch from a9c612e to f0434d8 Compare April 12, 2023 11:43

valeriosetti mentioned this pull request Apr 12, 2023

driver-only: ECP.PSA starter #7431

Merged

3 tasks

gilles-peskine-arm reviewed Apr 13, 2023

View reviewed changes

include/mbedtls/mbedtls_config.h Outdated Show resolved Hide resolved

gilles-peskine-arm reviewed Apr 13, 2023

View reviewed changes

ChangeLog.d/ecp-light.txt Outdated Show resolved Hide resolved

valeriosetti force-pushed the issue7390 branch from f0434d8 to 9cea093 Compare April 13, 2023 10:37

mpg mentioned this pull request Apr 14, 2023

Avoid parse/unparse public ECC keys in PK with USE_PSA #7202

Closed

3 tasks

mpg approved these changes Apr 14, 2023

View reviewed changes

AndrzejKurek self-requested a review April 14, 2023 08:26

AndrzejKurek reviewed Apr 14, 2023

View reviewed changes

mpg mentioned this pull request Apr 14, 2023

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

Closed

AndrzejKurek approved these changes Apr 14, 2023

View reviewed changes

AndrzejKurek 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 Apr 14, 2023

mpg mentioned this pull request Apr 14, 2023

Read and write RFC8410 keys #6838

Merged

3 tasks

mpg merged commit 6942cc3 into Mbed-TLS:development Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define (private) "light" subset of ECP #7410

Define (private) "light" subset of ECP #7410

valeriosetti commented Apr 5, 2023 •

edited by mpg

Loading

mpg left a comment

valeriosetti commented Apr 7, 2023 •

edited

Loading

mpg commented Apr 11, 2023

mpg commented Apr 11, 2023

mpg left a comment

mpg left a comment

AndrzejKurek left a comment

AndrzejKurek Apr 14, 2023

AndrzejKurek Apr 14, 2023

mpg Apr 14, 2023

mpg Apr 14, 2023

AndrzejKurek Apr 14, 2023

mpg Apr 14, 2023

AndrzejKurek Apr 14, 2023

mpg Apr 14, 2023

mpg Apr 14, 2023

mpg Apr 14, 2023

AndrzejKurek left a comment

		@@ -5360,7 +5360,6 @@ run_test "Authentication: server goodcert, client required, no trusted CA" \
		# occasion (to be fixed). If that bug's fixed, the test needs to be altered to use a
		# different means to have the server ignoring the client's supported curve list.

Define (private) "light" subset of ECP #7410

Define (private) "light" subset of ECP #7410

Conversation

valeriosetti commented Apr 5, 2023 • edited by mpg Loading

Gatekeeper checklist

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Apr 7, 2023 • edited Loading

mpg commented Apr 11, 2023

mpg commented Apr 11, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

valeriosetti commented Apr 5, 2023 •

edited by mpg

Loading

valeriosetti commented Apr 7, 2023 •

edited

Loading