Driver-only ECDH starter #7142

mpg · 2023-02-21T13:01:49Z

Description

This creates / updates the infrastructure for testing work on driver-only ECDH.

Context: see #6839. This is about the TL-ecdh intermediate goal - not doing the work itself, just putting the test components in place. TL-ecdh will be achieved when the all.sh component introduced here is complete (full config without undue exclusions, running ssl-opt.sh too) and has testing parity with its reference component (that is, no undue "ignore"s in analyze_outcome.py).

Follow-ups: tasks being created:

Gatekeeper checklist

changelog not required - test only
backport not required - new feature
tests provided

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

With temporary exclusions to be lifted as follow-ups. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

valeriosetti

LGTM ;)

AndrzejKurek · 2023-03-02T15:50:22Z

tests/scripts/all.sh

+        # Disable the module that's accelerated
+        scripts/config.py unset MBEDTLS_ECDH_C
+    fi
+    # Disable things that depend on it


Please either remove this comment, or unset kex defines conditionally

I'll give my 2cents here. Albeit it is not explicitly written, I think that @mpg's plan is to start with these key exchanges disabled and then remove these lines as long as the ECDH task moves forward. We did the same for accelerated ECDSA.

Sure, it's just good to have a comment / follow-up issue linked to explain why these are disabled even if MBEDTLS_ECDH_C isn't :)

Ah, I'll add a comment, as this is not intuitive indeed.

AndrzejKurek · 2023-03-02T16:08:27Z

tests/scripts/all.sh

    not grep mbedtls_ecdh_ library/ecdh.o

+    # Run the tests
+    # -------------
+
    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH"


Suggested change

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH"

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"

AndrzejKurek · 2023-03-02T16:12:30Z

tests/scripts/all.sh

+# Keep in sync with component_test_psa_crypto_config_accel_ecdh_use_psa.
+# Used by tests/scripts/analyze_outcomes.py for comparison purposes.
+component_test_psa_crypto_config_reference_ecdh_use_psa () {
+    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"


Suggested change

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG reference ECDH + USE_PSA"

AndrzejKurek · 2023-03-02T16:12:40Z

tests/scripts/all.sh

+
+    make
+
+    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"


Suggested change

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"

msg "test: MBEDTLS_PSA_CRYPTO_CONFIG reference ECDH + USE_PSA"

AndrzejKurek · 2023-03-02T16:15:18Z

tests/scripts/all.sh

+    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED
+    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED
+
+    # Build the library


Suggested change

# Build the library

# Build the main library

AndrzejKurek · 2023-03-02T16:15:39Z

tests/scripts/all.sh

+    # Use the same config as reference, only without built-in ECDH
+    config_psa_crypto_config_ecdh_use_psa 1
+
+    # Build the library


Suggested change

# Build the library

# Build the main library

tests/scripts/all.sh

AndrzejKurek

Some comments left.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2023-03-06T12:42:06Z

@AndrzejKurek Thanks for your review! I think I've addressed your comments, so this should be ready to review again.

valeriosetti

I only found one mistake (which I guess might be due to some investigation about issue #7148)

valeriosetti · 2023-03-06T13:21:19Z

tests/scripts/all.sh

+    #make test
+    tests/ssl-opt.sh


This looks like an error to me: I think that in this PR we should run make test, but not ssl-opt.sh as the latter is supposed to be solved in #7148 . Isn't it?

Good catch :)

Ah, thanks for catching this. Indeed you guessed right, this was supposed to be a local experiment that I never intended to commit.

This was never meant to be committed here. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg added 4 commits February 21, 2023 12:19

Add comments to accel_ecdh component

e3095e7

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add component accel_ecdh_use_psa

59a2b8f

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add comparison of accel_ecdh_use_psa against ref

e91bcf3

With temporary exclusions to be lifted as follow-ups. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix some dependencies in test_suite_psa_crypto

d1c001a

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg added enhancement component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d) labels Feb 21, 2023

mpg self-assigned this Feb 21, 2023

mpg requested a review from valeriosetti February 21, 2023 13:02

mpg added 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 Feb 22, 2023

mpg mentioned this pull request Feb 22, 2023

driver-only ECDH: enable ssl-opt.sh with parity #7148

Closed

valeriosetti previously approved these changes Feb 24, 2023

View reviewed changes

AndrzejKurek self-requested a review March 2, 2023 10:15

AndrzejKurek reviewed Mar 2, 2023

View reviewed changes

tests/scripts/all.sh Show resolved Hide resolved

AndrzejKurek suggested changes Mar 2, 2023

View reviewed changes

mpg added 2 commits March 6, 2023 13:35

Add comment about shared config function

0d1f5be

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix some message strings and comments in all.sh

07d9262

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Mar 6, 2023

mpg dismissed valeriosetti’s stale review via 07d9262 March 6, 2023 12:40

mpg requested review from AndrzejKurek and valeriosetti March 6, 2023 12:42

AndrzejKurek previously approved these changes Mar 6, 2023

View reviewed changes

valeriosetti requested changes Mar 6, 2023

View reviewed changes

Revert local experiment.

86393db

This was never meant to be committed here. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg dismissed AndrzejKurek’s stale review via 86393db March 6, 2023 15:20

AndrzejKurek approved these changes Mar 6, 2023

View reviewed changes

valeriosetti approved these changes Mar 6, 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 and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 6, 2023

mpg removed the needs-ci Needs to pass CI tests label Mar 7, 2023

mpg merged commit a5ffa93 into Mbed-TLS:development Mar 7, 2023

This was referenced Mar 9, 2023

driver-only ECDH: enable ssl-opt.sh with parity #7243

Merged

driver-only ECDH: enable ECDH-based TLS 1.2 key exchanges #7253

Closed

driver-only ECDH: enable TLS 1.3 key exchanges #7254

Closed

Driver-only EC JPAKE: starter #7275

Merged

mpg mentioned this pull request Mar 28, 2023

Driver-only curves: parity starter #7344

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Driver-only ECDH starter #7142

Driver-only ECDH starter #7142

mpg commented Feb 21, 2023 •

edited by valeriosetti

Loading

valeriosetti left a comment

AndrzejKurek Mar 2, 2023

valeriosetti Mar 2, 2023

AndrzejKurek Mar 2, 2023

mpg Mar 6, 2023

AndrzejKurek Mar 2, 2023

AndrzejKurek Mar 2, 2023

AndrzejKurek Mar 2, 2023

AndrzejKurek Mar 2, 2023

AndrzejKurek Mar 2, 2023

AndrzejKurek left a comment

mpg commented Mar 6, 2023

valeriosetti left a comment

valeriosetti Mar 6, 2023 •

edited

Loading

AndrzejKurek Mar 6, 2023

mpg Mar 6, 2023

	msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH"
	msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"

	msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"
	msg "test: MBEDTLS_PSA_CRYPTO_CONFIG reference ECDH + USE_PSA"


		make

		msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDH + USE_PSA"

Driver-only ECDH starter #7142

Driver-only ECDH starter #7142

Conversation

mpg commented Feb 21, 2023 • edited by valeriosetti Loading

Description

Gatekeeper checklist

valeriosetti 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

AndrzejKurek left a comment

Choose a reason for hiding this comment

mpg commented Mar 6, 2023

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg commented Feb 21, 2023 •

edited by valeriosetti

Loading

valeriosetti Mar 6, 2023 •

edited

Loading