Migrate DHE test cases to ECDHE #9916

valeriosetti · 2025-01-20T14:39:19Z

Description

Resoves #9688

PR checklist

changelog not required because: it will be done as part of Remove the DHE-RSA key exchange #9685
development PR here
framework PR not required
3.6 PR provided [Backport 3.6] Migrate DHE test cases to ECDHE #9937
2.28 PR not required because:
tests not required because: this PR is not introducing new features

Harry-Ramsey

LGTM

I did notice however that these lines in test_suite_ssl.data are present althought I am not sure if they should be removed.

Test configuration of groups for DHE through mbedtls_ssl_conf_curves()
conf_curve:
Test configuration of groups for DHE through mbedtls_ssl_conf_groups()
conf_group:

valeriosetti · 2025-01-22T09:59:41Z

I did notice however that these lines exist in test_suite_ssl.data are present althought I am not sure if they should be removed.

Oh, good catch! IMO those tests seems to be DHE specific, so they can just be removed. I'll update the commits.
It's implemented here. All the following commits are untouched (as you can see by clicking on the "compare" button)

Harry-Ramsey

LGTM

ronald-cron-arm

I've done a full review. I've some questions.

tests/suites/test_suite_ssl.data

tests/ssl-opt.sh

Harry-Ramsey

LGTM

valeriosetti · 2025-01-27T04:39:49Z

@Harry-Ramsey sorry to dismiss your review, but I had to update 61f4549 because I forgot to update analyze_outcomes.py when I changed the name of the test and that was causing a CI failure. Nothing else is changed though in the commit history as you can see from the "Compare" button

ronald-cron-arm

This looks almost good to me. Two last comments.

tests/suites/test_suite_ssl.data

ronald-cron-arm

LGTM, thanks. Do you plan to backport the new ECDHE-RSA tests to 3.6?

valeriosetti · 2025-01-27T11:09:07Z

LGTM, thanks. Do you plan to backport the new ECDHE-RSA tests to 3.6?

According to the requirement from #9688, i.e.

This applies to development only. But we may want to backport the new test cases as additional tests in 3.6 for a minor but very cheap coverage improvement.

I think this is required indeed. Now that I have some approval I can start working on it (I was waiting to get to a decent point with this PR, so that I don't need to refresh the backport continuously).

valeriosetti · 2025-01-27T11:10:01Z

But I just saw that there are also conflicts in this PR with development so I might address those ones before starting the backport.

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

Use ECDHE-RSA instead of DHE-RSA. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

These tests were specific for DHE-RSA (which is being removed on development branch) and also for each of them there was already the ECDHE-RSA counterpart available. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

These tests are about EC curves/groups, not DH ones, so the description should be updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Adapted tests do not already have an ECDHE-RSA test available. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

For these ones there is no ECDHE alternative as they are testing specific features of DHE. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Remove tests which are forcing DHE-RSA, but for which an ECDHE-RSA alternative already exists. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

PR Mbed-TLS#9916 adapt some DHE-RSA tests to use ECDHE-RSA instead. However, since DHE-RSA is not deprecated in mbedtls-3.6 branch, this commit adds these new tests alongside DHE-RSA ones intead of replacing them in order to increase test coverage. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti · 2025-01-27T14:58:47Z

LGTM, thanks. Do you plan to backport the new ECDHE-RSA tests to 3.6?

Done here #9937

Harry-Ramsey

LGTM

ronald-cron-arm

The rebase looks good to me. Thanks.

PR Mbed-TLS#9916 adapt some DHE-RSA tests to use ECDHE-RSA instead. However, since DHE-RSA is not deprecated in mbedtls-3.6 branch, this commit adds these new tests alongside DHE-RSA ones intead of replacing them in order to increase test coverage. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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 Jan 20, 2025

valeriosetti self-assigned this Jan 20, 2025

valeriosetti force-pushed the issue9688 branch from 8c93c9b to a7ba35c Compare January 21, 2025 15:39

valeriosetti removed the needs-ci Needs to pass CI tests label Jan 22, 2025

Harry-Ramsey previously approved these changes Jan 22, 2025

View reviewed changes

valeriosetti dismissed Harry-Ramsey’s stale review via 4c1a996 January 22, 2025 10:04

valeriosetti force-pushed the issue9688 branch from a7ba35c to 4c1a996 Compare January 22, 2025 10:04

Harry-Ramsey previously approved these changes Jan 22, 2025

View reviewed changes

valeriosetti mentioned this pull request Jan 22, 2025

[development] Remove the DHE-RSA key exchange #9917

Merged

5 tasks

ronald-cron-arm self-requested a review January 23, 2025 10:36

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 23, 2025

ronald-cron-arm requested changes Jan 23, 2025

View reviewed changes

valeriosetti dismissed Harry-Ramsey’s stale review via ddf47c5 January 24, 2025 15:34

valeriosetti force-pushed the issue9688 branch from 4c1a996 to ddf47c5 Compare January 24, 2025 15:34

valeriosetti requested review from ronald-cron-arm and Harry-Ramsey January 24, 2025 15:34

Harry-Ramsey previously approved these changes Jan 24, 2025

View reviewed changes

valeriosetti dismissed Harry-Ramsey’s stale review via 489ed07 January 27, 2025 04:37

valeriosetti force-pushed the issue9688 branch from ddf47c5 to 489ed07 Compare January 27, 2025 04:37

valeriosetti requested a review from Harry-Ramsey January 27, 2025 04:39

ronald-cron-arm requested changes Jan 27, 2025

View reviewed changes

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

tests/suites/test_suite_ssl.data Show resolved Hide resolved

valeriosetti force-pushed the issue9688 branch from 489ed07 to 4fd532f Compare January 27, 2025 09:06

valeriosetti requested a review from ronald-cron-arm January 27, 2025 09:07

ronald-cron-arm previously approved these changes Jan 27, 2025

View reviewed changes

valeriosetti added 7 commits January 27, 2025 12:38

test_suite_ssl: adapt DHE-RSA tests to ECDHE-RSA

5b7bfd8

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

test_suite_ssl: adapt handshake_fragmentation() to use ECDHE-RSA

b8ef2a4

Use ECDHE-RSA instead of DHE-RSA. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test_suite_ssl: remove tests specific for DHE-RSA

8638603

These tests were specific for DHE-RSA (which is being removed on development branch) and also for each of them there was already the ECDHE-RSA counterpart available. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test_suite_ssl: update description for conf_curve and conf_gruop tests

592f682

These tests are about EC curves/groups, not DH ones, so the description should be updated accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ssl-opt.sh: adapt tests from DHE-RSA to ECDHE-RSA

309a7ec

Adapted tests do not already have an ECDHE-RSA test available. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ssl-opt.sh: remove tests which are specific for DHE-RSA

3b412e2

For these ones there is no ECDHE alternative as they are testing specific features of DHE. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

ssl-opt.sh: remove tests forcing DHE-RSA for which have alternatives

0ebd6de

Remove tests which are forcing DHE-RSA, but for which an ECDHE-RSA alternative already exists. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed ronald-cron-arm’s stale review via 0ebd6de January 27, 2025 11:47

valeriosetti force-pushed the issue9688 branch from 4fd532f to 0ebd6de Compare January 27, 2025 11:47

valeriosetti mentioned this pull request Jan 27, 2025

[Backport 3.6] Migrate DHE test cases to ECDHE #9937

Merged

5 tasks

valeriosetti added the needs-backports Backports are missing or are pending review and approval. label Jan 27, 2025

Harry-Ramsey approved these changes Jan 27, 2025

View reviewed changes

ronald-cron-arm approved these changes Jan 27, 2025

View reviewed changes

valeriosetti added the approved Design and code approved - may be waiting for CI or backports label Jan 27, 2025

ronald-cron-arm removed the needs-review Every commit must be reviewed by at least two team members, label Jan 29, 2025

ronald-cron-arm added this pull request to the merge queue Jan 29, 2025

Merged via the queue into Mbed-TLS:development with commit ed44508 Jan 29, 2025
5 of 6 checks passed

ronald-cron-arm mentioned this pull request Jan 30, 2025

Migrate DHE test cases to ECDHE #9688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate DHE test cases to ECDHE #9916

Migrate DHE test cases to ECDHE #9916

valeriosetti commented Jan 20, 2025 •

edited

Loading

Harry-Ramsey left a comment •

edited

Loading

valeriosetti commented Jan 22, 2025 •

edited

Loading

Harry-Ramsey left a comment

ronald-cron-arm left a comment

Harry-Ramsey left a comment

valeriosetti commented Jan 27, 2025

ronald-cron-arm left a comment

ronald-cron-arm left a comment

valeriosetti commented Jan 27, 2025

valeriosetti commented Jan 27, 2025

valeriosetti commented Jan 27, 2025

Harry-Ramsey left a comment

ronald-cron-arm left a comment

Migrate DHE test cases to ECDHE #9916

Migrate DHE test cases to ECDHE #9916

Conversation

valeriosetti commented Jan 20, 2025 • edited Loading

Description

PR checklist

Harry-Ramsey left a comment • edited Loading

Choose a reason for hiding this comment

valeriosetti commented Jan 22, 2025 • edited Loading

Harry-Ramsey left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Harry-Ramsey left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 27, 2025

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 27, 2025

valeriosetti commented Jan 27, 2025

valeriosetti commented Jan 27, 2025

Harry-Ramsey left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 20, 2025 •

edited

Loading

Harry-Ramsey left a comment •

edited

Loading

valeriosetti commented Jan 22, 2025 •

edited

Loading