driver-only ECDSA: add ssl-opt.sh testing with testing parity #7082

valeriosetti · 2023-02-10T13:09:09Z

Description

The goal of this PR is to re-enable ssl-opt.sh tests when running accelerated ECDSA coverage analysis.
Resolves #6861
~~Depends on #7064~~

Gatekeeper checklist

changelog not required
backport not required
tests not required

valeriosetti · 2023-02-13T13:39:18Z

rebased after driver-only ECDSA: get testing parity in SSL #7064 was updated
fixed disparities in analyze_outcomes.py
run outcome-analysis.sh; attaching its output
outcome-analysis-results.txt

valeriosetti · 2023-02-13T14:39:54Z

Last rebase is because #6997 was merged and consequently also #7064 was rebased

mpg · 2023-02-14T08:37:49Z

(Changed the base twice just to force-refresh github's list of commits. Now it no longer lists the commits from 7064 as being added by this PR.)

mpg

Looks pretty good generally, mostly just one point about sometimes using a stronger requirement than necessary.

tests/ssl-opt.sh

library/ssl_misc.h

programs/ssl/ssl_server2.c

tests/ssl-opt.sh

valeriosetti · 2023-02-22T10:03:21Z

Here is the result of the outcome-analysis.sh script
outcome-analysis-results.txt

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

mprse

Thanks for addressing my comments. It seems that there is no correct way to skip tests other than ECDSA in ssl-opt, so we need to keep all enabled.

LGTM.

gilles-peskine-arm · 2023-03-03T15:32:22Z

I think Manuel and Przemysław are both reviewing, hence removing the needs-reviewer label.

mpg

(I'm running out of time for today, just releasing the one comment I have so far, will continue my review tomorrow.)

mpg · 2023-03-06T11:52:07Z

tests/opt-testcases/tls13-kex-modes.sh

@@ -2833,6 +2833,7 @@ requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
 requires_config_enabled MBEDTLS_DEBUG_C
 requires_config_enabled MBEDTLS_SSL_CLI_C
 requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
+requires_pk_alg ECDSA


Question: I don't understand why this case needs this but the next one apparently doesn't? The only differences I can see between the command lines is that the next one has sig_algs=ecdsa_secp256r1_sha256 and this one doesn't, but that would suggest the next once needs ECDSA (too).

Can you clarify the reasoning that led to adding this here?

You're absolutely right, this is a redundant requirement (which probably belongs to the initial steps for this PR) that now is automatically satisfied by the changes in detect_required_features().
I'll fix this and all other redundancies.

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

mpg

Looking pretty good to me. I now think all the dependencies are correct, but that (1) some of them are redundant with what detect_required_feature does thanks to your extension, and (2) some would become redundant with a simple extra extension (detect server7 too).

library/ssl_misc.h

library/ssl_tls.c

mpg · 2023-03-07T08:23:03Z

tests/scripts/all.sh

@@ -2158,7 +2158,8 @@ component_test_psa_crypto_config_accel_ecdsa_use_psa () {
    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECDSA + USE_PSA"
    make test

-    # TODO: ssl-opt.sh (currently doesn't pass) - #6861
+    msg "test: ssl-opt.sh"


I understand the concern for reducing the load, but until we have a solid mechanism for selecting cases, I'd really prefer to run the whole thing. Otherwise it's very hard to be confident we didn't miss anything.

tests/ssl-opt.sh

mpg · 2023-03-07T08:38:27Z

tests/ssl-opt.sh

@@ -3691,6 +3717,7 @@ run_test    "Session resume using tickets: session copy" \
            -c "a session has been resumed"

 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2
+requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_ECDSA_CERT


Ah right, I missed the things implied by $O_SRV, right. Thanks for clarifying.

Though, isn't that automatically handled by your extension to detect_required_features? (Same question for the bunch with $G_SRV below.)

mpg · 2023-03-07T08:49:59Z

tests/ssl-opt.sh

@@ -5535,6 +5595,7 @@ run_test    "Authentication: openssl client no cert, server optional" \

 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2
 requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT
+requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_ECDSA_CERT


The line above becomes redundant when adding this one. (Also, it looks to me like this would be auto-detected too.)

tests/ssl-opt.sh

mpg · 2023-03-07T08:53:54Z

tests/ssl-opt.sh

@@ -9448,7 +9521,8 @@ run_test    "DTLS reassembly: fragmentation, nbio (openssl server)" \

 requires_config_enabled MBEDTLS_SSL_PROTO_DTLS
 requires_config_enabled MBEDTLS_RSA_C
-requires_config_enabled MBEDTLS_ECDSA_C
+requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT
+requires_pk_alg "ECDSA"


This one and the next few look like they could be auto-detected based on loading a server7 certificate.

tests/ssl-opt.sh

mpg · 2023-03-07T09:00:27Z

tests/ssl-opt.sh

@@ -11473,6 +11557,7 @@ requires_config_enabled MBEDTLS_DEBUG_C
 requires_config_enabled MBEDTLS_SSL_CLI_C
 requires_all_configs_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE \
                             MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
+requires_pk_alg "ECDSA"


I think for this case, and probably all the other TLS 1.3 cases until the end of the files, detect_require_features already does the job so don't need this explicit declaration.

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

This was a leftover from some debug activity that unfortunately ended up in previous commits. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

There were some dependencies that are now automatically satisfied by the detect_required_features() function. After this check there should be no redundant requirement for: - requires_pk_alg "ECDSA" - requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_ECDSA_CERT - requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti · 2023-03-08T09:53:20Z

I checked ssl-opt.sh for all occurencies of:

requires_pk_alg "ECDSA"
requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_ECDSA_CERT
requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT

and removed all redundancies which are automatically satisfied by detect_required_features() (which has also been extended to include server7 certificate).
Here is also the outcome-analysis result:
outcome-analysis.txt

mpg

Thanks for addressing my feedback. Looks all good to me now as long as the CI agrees :)

mprse

Delta Review ok. Only one typo found.

mprse · 2023-03-08T10:42:07Z

tests/ssl-opt.sh

+#       detect_required_features() function), it does NOT guarantee that the
+#       result is accurate. It does not check other conditions, such as:
+#       - MBEDTLS_SSL_PROTO_TLS1_x can be disabled to selectively remove
+#         TLS 1.2/1.3 suppport


typo: support

Ops, sorry for this. In case this PR does not require any change, I will fix this in the following one (#7117)

gilles-peskine-arm · 2023-11-28T15:35:09Z

tests/ssl-opt.sh

+# Note: the function "detect_required_features()" is not able to detect more than
+#       one "force_ciphersuite" per client/server and it only picks the 2nd one.
+#       Therefore the 1st one is added explicitly here


I don't understand this comment: detect_required_features runs independently on the client command line and on the server command line. If they have different force_ciphersuite options, this should lead to skipping the test case unless both TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256 and TLS-ECDHE-RSA-WITH-AES-128-CBC-SHA256 are enabled. So the explicit requirement on MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED should not be necessary. What am I missing?

I just had a look and I don't think you're missing anything. @valeriosetti wdyt?

It's been some time since I did this change, so I'm trying to remember the exact reason for it. I looked at the code and I think the problem with these tests:

SSL async private: cancel after start then fall back to transparent key

SSL async private: sign, error in resume then fall back to transparent key
is that they define 2 different ciphersuites for the client (because the client is run twice).

In the scenario you described (which is the most used case) we have 1 ciphersuite for the server and 1 for the client, so detect_required_features works well. Here the problem is that we are running the client twice, with different ciphersuites in each case, but detect_required_features() only picks the 2nd one (ignoring ECDHE-ECDSA). That's why I enforced that in those tests.
I agree that a better approach would have been to extend detect_required_features() in order to support multiple ciphersuites, but it was only a matter of 2 tests, so that's why I didn't try to do so.

Oh duh, yes, I see, it's because we run the client twice. I agree with not making the detection code more complex just for two test cases. Thanks for the explanation.

valeriosetti requested a review from mpg February 10, 2023 13:09

valeriosetti self-assigned this Feb 10, 2023

valeriosetti added enhancement needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first size-m Estimated task size: medium (~1w) labels Feb 10, 2023

valeriosetti force-pushed the issue6861 branch from 43280d7 to 882759c Compare February 10, 2023 15:15

valeriosetti mentioned this pull request Feb 13, 2023

driver-only ECDSA: get testing parity in SSL #7064

Merged

3 tasks

valeriosetti force-pushed the issue6861 branch from 882759c to 6d64a42 Compare February 13, 2023 13:36

valeriosetti force-pushed the issue6861 branch 2 times, most recently from a2f2469 to 8a15c4c Compare February 13, 2023 14:39

mpg changed the base branch from development to features/new-code-style/development February 14, 2023 08:36

mpg changed the base branch from features/new-code-style/development to development February 14, 2023 08:36

valeriosetti 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 and removed needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Feb 15, 2023

valeriosetti mentioned this pull request Feb 16, 2023

driver-only ECDSA: enable ECDSA-based TLS 1.2 key exchanges #7117

Merged

3 tasks

mpg requested changes Feb 20, 2023

View reviewed changes

valeriosetti force-pushed the issue6861 branch from 8a15c4c to 133f9b0 Compare February 22, 2023 08:55

valeriosetti requested a review from mpg February 22, 2023 08:56

valeriosetti added the needs-ci Needs to pass CI tests label Feb 22, 2023

fix typos

194e2bd

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

valeriosetti requested a review from mprse March 2, 2023 16:18

mprse previously approved these changes Mar 3, 2023

View reviewed changes

gilles-peskine-arm removed needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Mar 3, 2023

mpg reviewed Mar 6, 2023

View reviewed changes

ssl-opt: remove redundant ECDSA dependencies in TLS1.3 tests

23e50b9

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

valeriosetti dismissed mprse’s stale review via 23e50b9 March 6, 2023 13:48

mpg requested changes Mar 7, 2023

View reviewed changes

valeriosetti added 4 commits March 7, 2023 18:14

ssl: fix included pk header file

2f1d967

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

ssl-opt: enhance comment for get_tls_version() function

213c4ea

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

ssl-opt: return to previous debug level in test

3b2c028

This was a leftover from some debug activity that unfortunately ended up in previous commits. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti requested review from mpg and mprse March 8, 2023 09:53

mpg approved these changes Mar 8, 2023

View reviewed changes

mprse reviewed Mar 8, 2023

View reviewed changes

mprse approved these changes Mar 8, 2023

View reviewed changes

mprse 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 Mar 8, 2023

mpg merged commit 289e5ba into Mbed-TLS:development Mar 8, 2023

gilles-peskine-arm reviewed Nov 28, 2023

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver-only ECDSA: add ssl-opt.sh testing with testing parity #7082

driver-only ECDSA: add ssl-opt.sh testing with testing parity #7082

valeriosetti commented Feb 10, 2023 •

edited by paul-elliott-arm

Loading

valeriosetti commented Feb 13, 2023

valeriosetti commented Feb 13, 2023

mpg commented Feb 14, 2023

mpg left a comment

valeriosetti commented Feb 22, 2023

mprse left a comment

gilles-peskine-arm commented Mar 3, 2023

mpg left a comment

mpg Mar 6, 2023

valeriosetti Mar 6, 2023

mpg left a comment

mpg Mar 7, 2023

mpg Mar 7, 2023

mpg Mar 7, 2023

mpg Mar 7, 2023

mpg Mar 7, 2023

valeriosetti commented Mar 8, 2023

mpg left a comment

mprse left a comment

mprse Mar 8, 2023

valeriosetti Mar 8, 2023

gilles-peskine-arm Nov 28, 2023

mpg Nov 29, 2023

valeriosetti Nov 29, 2023

gilles-peskine-arm Nov 29, 2023

driver-only ECDSA: add ssl-opt.sh testing with testing parity #7082

driver-only ECDSA: add ssl-opt.sh testing with testing parity #7082

Conversation

valeriosetti commented Feb 10, 2023 • edited by paul-elliott-arm Loading

Description

Gatekeeper checklist

valeriosetti commented Feb 13, 2023

valeriosetti commented Feb 13, 2023

mpg commented Feb 14, 2023

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Feb 22, 2023

mprse left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Mar 3, 2023

mpg left a 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

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

valeriosetti commented Mar 8, 2023

mpg left a comment

Choose a reason for hiding this comment

mprse 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

valeriosetti commented Feb 10, 2023 •

edited by paul-elliott-arm

Loading