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

valeriosetti · 2023-06-08T13:07:35Z

This PR resolves #7442 (please refer to the original issue because the goals are described pretty well there).

Depends on ~~#7641~~ (merged)
Resolves #7442

PR checklist

changelog deferred, see ChangeLog for driver-only work #7413
backport not required
tests not required

mpg · 2023-06-09T10:29:15Z

I had a quick look, and almost everything looks good to me, except for all.sh components that are not as I expected. Before this PR (but after 7682) we have:

test_psa_crypto_config_accel_all_ec_algs_use_psa with all top-level ECC modules (ECDH, ECDSA, ECJPAKE) accelerated, plus !ECP_C && EPC_LIGHT, and support for compressed points, specifiedECDomain (PK_PARSE_EXTENDED) and ECC key derivation. Builds everything (X.509, TLS) and runs all tests including ssl-opt.sh. So far it uses a bit of a hack (-D in CFLAGS) to enabled ECP_LIGHT without ECP_C.
component_test_psa_crypto_full_accel_all_ec_algs_no_ecp_use_psa with all top-level modules accelerated and no ecp.c at all, not even light. Hence, no support for compressed points, SpecifiedECDomain or ECC key derivation. So far, only builds crypto including PK (since 7682), but no X.509 or TLS.

IMO we need to keep the two: one with ECP_LIGHT for the extra features it gives (to make sure those work without the full ECP_C), and one with no ECP at all but without the associated goodies (to make sure the rest works with no ECP at all).

The goal of this issue is only to remove the "So far it uses a bit of a hack" part of (1), by instead just leaving PK_PARSE_EXTENDED and PK_PARSE_COMPRESSED enabled in this component, so we no longer need to explicitly enable ECP_LIGHT via CFLAGS. (And conversely, we need to explicitly disable them in (2) in order to keep it free of any ECP at all.)

Removing the "So far, only builds crypto" part of (2) is out of scope here: it's the final goal of EPIC 2c :) (While this issue completes EPIC 2b (functionally, we'll still have some cleanup remaining).)

I think we should rename the components as follows for improved clarity:

test_psa_crypto_config_accel_all_ec_algs_use_psa -> test_psa_crypto_config_accel_ecc_ecp_light_only (asserts no mbedtls_ecp_mul_ in ecp.o)
component_test_psa_crypto_full_accel_all_ec_algs_no_ecp_use_psa -> component_test_psa_crypto_config_accel_ecc_no_ecp_at_all (asserts to mbedtls_ecp in ecp.o, meaning it's empty)
(Or something similar.)

Note: looking further into the future, EPIC 2d will add a 3rd one component_test_psa_crypto_config_accel_ecc_no_bignum (asserts bignum.o empty) which should be the same as (2) above except without BIGNUM_C, hence without DHM_C, RSA_C and things that depend on those.

mpg · 2023-06-09T10:57:35Z

Note: I just updated the "Testing" part of #6839 with essentially what I wrote above :)

valeriosetti · 2023-06-09T14:47:06Z

Removing the "So far, only builds crypto" part of (2) is out of scope here

Yes, indeed I was having building problems with server2 and client2 since at least one of the 2 uses ECP functions to get the list of known curves and, of course, it wasn't particularly happy about the ECP_LIGHT removal

valeriosetti · 2023-06-14T09:43:07Z

I moved this PR from draft to normal because after this comment (in #7682) this PR is no more depending on 2 non-cascaded PRs, but it can easily be set on top of #7641.
I will update also the description accordingly

mpg

Looks all good to me except one backwards compatibility issue, for people using their own config file.

mpg · 2023-06-16T10:21:42Z

include/mbedtls/mbedtls_config.h

+ * compressed point format.
+ * Please see MBEDTLS_ECP_PF_COMPRESSED in ecp.h for limitations details.
+ */
+#define MBEDTLS_PK_PARSE_EC_COMPRESSED


Note: in 3.4 this was not a separate option, support was automatically present in PK when ECP_C was defined. So, I think in order to preserve backward compatibility for people using a custom config we need to also auto-enable MBEDTLS_PK_PARSE_EC_COMPRESSED when PK_PARSE_C && ECP_C in build_info.h.

valeriosetti · 2023-06-16T13:46:41Z

@mpg even though the internal CI failed on test_psa_crypto_config_accel_hash_use_psa the Open counterpart did not, so I'm not sure what the problem is. Can you please check for me?

References in analyze_outcomes.py are updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ECP_LIGHT was never set as public symbol so it should not be enabled/disabled using the config.py script. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

This includes also: - auto enabling ECP_LIGHT when MBEDTLS_PK_PARSE_EC_COMPRESSED is defined - replacing ECP_LIGHT guards with PK_PARSE_EC_COMPRESSED in pkparse - disabling PK_PARSE_EC_COMPRESSED in tests with accelarated EC curves (it get disabled also in the reference components because we want to achieve test parity) - remove skipped checks in analyze_outcomes.py Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

The comment is also updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

This helps backward compatibility since compressed points were always supported in previous releases as long as PK_PARSE_C and ECP_C were defined. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

gilles-peskine-arm · 2023-06-16T14:10:50Z

The failure is in one of the ssl-opt test cases:

TLS 1.3 m->m: Session resumption failure, age outside tolerance window, too old.  FAIL
  ! pattern 'sent selected_identity:' MUST NOT be present in the Server output
  ! outputs saved to o-XXX-1587.log

This doesn't look related to this pull request. I have no idea what that test is doing so the logs don't mean anything to me. I've filed an issue about it as an intermittent failure.

mpg

Looks good to me, except for one thing that was missed when rebasing: the following paragraph in component_test_psa_crypto_config_accel_ecc_ecp_light_only should be removed:

    # Temporary hack to enable MBEDTLS_ECP_LIGHT
    # (will soon be auto-enabled in build_info.h)
    echo '#define MBEDTLS_ECP_LIGHT' >> include/mbedtls/mbedtls_config.h

AndrzejKurek

Looks good, but some things seem to be unclear.

AndrzejKurek · 2023-06-19T07:26:09Z

include/mbedtls/build_info.h

+ *   these features are not supported in PSA so the only way to have them is
+ *   to enable the built-in solution.
+ * - PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE because Weierstrass key derivation
+ *   still depends on ECP_LIGHT


Suggested change

* still depends on ECP_LIGHT

* still depends on ECP_LIGHT.

Temporarily? Are there plans to change that?

Good question indeed. TBH I didn't check if there is any task associated with this, but it looked strange to me that Montgomery curves' derivation is supported by PSA while Weierstrass is not. So mine was mostly a guess ':)
I'll check this but perhaps @mpg has a better overview of what is expected in the future.

Yes, the plan to change that is to add driver support for key derivation, see the dedicated EPIC.

While at it, MBEDTLS_PK_PARSE_EC_EXTENDED is temporary too, see #7779 and #7789 in the driver-only 2c EPIC, and MBEDTLS_PK_PARSE_EC_COMPRESSED should be fixed later by adding support for more formats in PSA, but this is at an early stage of investigation and doesn't have an EPIC or issues yet.

(I'll create some user-level documentation explaining the current limitations and plans about them for the future, but later in the EPICs, are things are moving too fast now, so documentation would be out-of-date very quickly. This will be done before the next release in any case.)

AndrzejKurek · 2023-06-19T07:27:15Z

include/mbedtls/build_info.h

+ * - PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE because Weierstrass key derivation
+ *   still depends on ECP_LIGHT
+ * - PK_C + USE_PSA + PSA_WANT_ALG_ECDSA is a temporary dependency which will
+ *   be fixed by #7453


Suggested change

* be fixed by #7453

* be fixed by #7453.

AndrzejKurek · 2023-06-19T07:54:13Z

include/mbedtls/mbedtls_config.h

+ * Enable the support for parsing public keys of type Short Weierstrass
+ * (MBEDTLS_ECP_DP_SECP_XXX and MBEDTLS_ECP_DP_BP_XXX) which are using the
+ * compressed point format.
+ * Please see MBEDTLS_ECP_PF_COMPRESSED in ecp.h for limitations details.


The relation between this define and MBEDTLS_ECP_PF_COMPRESSED is not explained.

I'm not sure what the problem is, but just in case: MBEDTLS_ECP_PF_COMPRESSED is not a feature macro, it's a constant in ecp.h, and happens to be where the limitations for parsing are documented. Can you suggest a better wording?

Oh, I see, so it's not a problem about the description itself as I thought initially, but it's more related to the automatic documentation generation. Am I correct?

I'm not sure what the problem is, but just in case: MBEDTLS_ECP_PF_COMPRESSED is not a feature macro, it's a constant in ecp.h, and happens to be where the limitations for parsing are documented. Can you suggest a better wording?

It's not about wording, it's just that the documentation of this define mentiones another (relation unclear) place to see for more details. If there's another place that goes into more detail, It'd be great to know what's the relation between that place and this one. If there's no relation then why does it specify things important for someone considering to turn MBEDTLS_PK_PARSE_EC_COMPRESSED on?

Ok understood. Does the last commit cover this gap or should it be reworked further?

Yes, it's OK, now you're providing the full info and the source of it, regardless of the relation between the two places.

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

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

mpg

LGTM

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

valeriosetti self-assigned this Jun 8, 2023

valeriosetti added needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first 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 8, 2023

mpg mentioned this pull request Jun 12, 2023

driver-only ECC: ECPf.PK testing #7682

Merged

3 tasks

valeriosetti force-pushed the issue7442 branch from db816a1 to bfc348a Compare June 14, 2023 09:39

valeriosetti marked this pull request as ready for review June 14, 2023 09:40

valeriosetti force-pushed the issue7442 branch 2 times, most recently from 30116a9 to b836a81 Compare June 14, 2023 15:05

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

AndrzejKurek self-requested a review June 16, 2023 08:32

mpg requested changes Jun 16, 2023

View reviewed changes

valeriosetti force-pushed the issue7442 branch from 6be559c to 3471e19 Compare June 16, 2023 10:52

valeriosetti added 8 commits June 16, 2023 16:03

test: renaming component testing accelerated EC

4d25a8d

References in analyze_outcomes.py are updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: remove leftover from past PR development

4348368

ECP_LIGHT was never set as public symbol so it should not be enabled/disabled using the config.py script. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

pk: let PK_PARSE_EC_EXTENDED auto-enable ECP_LIGHT

aecd32c

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

build_info: complete list of symbols that auto-enable ECP_LIGHT

4922ba1

The comment is also updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: fix bug for test without bugnum

22d0a79

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

test: update depends.py with new dependencies

15e7044

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

build_info: auto enable PK_PARSE_EC_COMPRESSED when PK_PARSE_C && ECP_C

fdf15dd

This helps backward compatibility since compressed points were always supported in previous releases as long as PK_PARSE_C and ECP_C were defined. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue7442 branch from 3471e19 to fdf15dd Compare June 16, 2023 14:04

valeriosetti requested a review from mpg June 19, 2023 07:22

mpg requested changes Jun 19, 2023

View reviewed changes

AndrzejKurek previously approved these changes Jun 19, 2023

View reviewed changes

test: remove leftover paragraph in test_psa_crypto_config_accel_ecc_e…

a8dc7fd

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

mpg removed needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review labels Jun 19, 2023

valeriosetti added 2 commits June 19, 2023 11:34

build_info: improved description of ECP_LIGHT auto-enabling symbols

a18385b

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

mbedtls_config: improved description of PK_PARSE_EC_COMPRESSED

3cd4ef7

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

valeriosetti dismissed AndrzejKurek’s stale review via 3cd4ef7 June 19, 2023 09:35

valeriosetti requested review from AndrzejKurek and mpg June 19, 2023 09:35

mpg approved these changes Jun 19, 2023

View reviewed changes

AndrzejKurek approved these changes Jun 19, 2023

View reviewed changes

AndrzejKurek added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests labels Jun 19, 2023

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

mpg merged commit cd70070 into Mbed-TLS:development Jun 19, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valeriosetti commented Jun 8, 2023 •

edited by mpg

Loading

mpg commented Jun 9, 2023

mpg commented Jun 9, 2023 •

edited

Loading

valeriosetti commented Jun 9, 2023

valeriosetti commented Jun 14, 2023

mpg left a comment

mpg Jun 16, 2023

valeriosetti commented Jun 16, 2023

gilles-peskine-arm commented Jun 16, 2023

mpg left a comment

AndrzejKurek left a comment

AndrzejKurek Jun 19, 2023

valeriosetti Jun 19, 2023

mpg Jun 19, 2023

AndrzejKurek Jun 19, 2023

AndrzejKurek Jun 19, 2023

mpg Jun 19, 2023

valeriosetti Jun 19, 2023

AndrzejKurek Jun 19, 2023

valeriosetti Jun 19, 2023

AndrzejKurek Jun 19, 2023

mpg left a comment

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

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

Conversation

valeriosetti commented Jun 8, 2023 • edited by mpg Loading

PR checklist

mpg commented Jun 9, 2023

mpg commented Jun 9, 2023 • edited Loading

valeriosetti commented Jun 9, 2023

valeriosetti commented Jun 14, 2023

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti commented Jun 16, 2023

gilles-peskine-arm commented Jun 16, 2023

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

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Jun 8, 2023 •

edited by mpg

Loading

mpg commented Jun 9, 2023 •

edited

Loading