Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate DHE test cases to ECDHE #9916

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

valeriosetti
Copy link
Contributor

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 not required because: it's this one
  • framework PR not required
  • 3.6 PR provided # : tests that are moved from DHE-RSA to ECDHE-RSA are likely to require a backport.
  • 2.28 PR not required because:
  • tests not required because: this PR is not introducing new features

@valeriosetti 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 valeriosetti self-assigned this Jan 20, 2025
… alternative

Tests removed here already have an ECDHE-RSA alternative in
test_suite_ssl.data, so they can be removed.

Signed-off-by: Valerio Setti <[email protected]>
Use ECDHE-RSA instead of DHE-RSA.

Signed-off-by: Valerio Setti <[email protected]>
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 <[email protected]>
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Jan 22, 2025
Harry-Ramsey
Harry-Ramsey previously approved these changes Jan 22, 2025
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

valeriosetti commented Jan 22, 2025

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)

Adapted tests do not already have an ECDHE-RSA test available.

Signed-off-by: Valerio Setti <[email protected]>
For these ones there is no ECDHE alternative as they are testing
specific features of DHE.

Signed-off-by: Valerio Setti <[email protected]>
Remove tests which are forcing DHE-RSA, but for which an ECDHE-RSA
alternative already exists.

Signed-off-by: Valerio Setti <[email protected]>
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants