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.

Sign up for GitHub

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

Jump to bottom

Remove DHE-PSK key exchange #9910

Open
wants to merge 12 commits into from
Open

Remove DHE-PSK key exchange #9910

wants to merge 12 commits into from
+18 −575

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Jan 15, 2025 •
edited
Loading

Description

Resolves #9684

Depends on Mbed-TLS/TF-PSA-Crypto#143 as per this comment

PR checklist

  • changelog provided
  • development PR provided
  • framework PR not required
  • 3.6 PR provided # | not required because:
  • 2.28 PR provided # | not required because:
  • tests provided | not required because:

@valeriosetti valeriosetti self-assigned this Jan 15, 2025
@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 15, 2025
@valeriosetti valeriosetti mentioned this pull request Jan 15, 2025
Closed
3 tasks
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Jan 16, 2025 •
edited
Loading

Note for reviewers: the failure on check_names test is expected and should be solved by Mbed-TLS/TF-PSA-Crypto#143. Therefore I'm removing the need-ci label

@valeriosetti valeriosetti added needs-preceding-pr Requires another PR to be merged first and removed needs-ci Needs to pass CI tests labels Jan 16, 2025
@ronald-cron-arm ronald-cron-arm self-requested a review January 17, 2025 10:43
ronald-cron-arm
ronald-cron-arm requested changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks almost good to me but one thing. Otherwise still some references to DHE-PSK to remove I think:
tests/scripts/analyze_outcomes.py: re.compile(r'PSK callback:.\bdhe-psk\b.'),
tests/scripts/components-configuration-crypto.sh: scripts/config.py unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
tests/scripts/components-configuration-crypto.sh: scripts/config.py unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
tests/scripts/components-configuration-crypto.sh: scripts/config.py unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
tests/scripts/components-configuration-crypto.sh: scripts/config.py unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED

tests/opt-testcases/tls13-kex-modes.sh Outdated Show resolved Hide resolved
ronald-cron-arm
ronald-cron-arm previously approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for quickly addressing my comments.

@gabor-mezei-arm gabor-mezei-arm self-requested a review January 20, 2025 14:56
@valeriosetti valeriosetti mentioned this pull request Jan 22, 2025
Open
5 tasks
@valeriosetti valeriosetti mentioned this pull request Jan 23, 2025
Merged
3 tasks
@valeriosetti valeriosetti removed the needs-preceding-pr Requires another PR to be merged first label Jan 23, 2025
@valeriosetti
ssl-opt.sh: remove DHE-PSK only test cases
5c730c1 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
compat.sh: do not use DHE-PSK key exchange in gnutls tests
9a9c9a5 
DHE-PSK is being removed from Mbed TLS so we cannot use this key
exchange with gnutls testing.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
compat.sh: remove usage of DHE-PSK
64d264d 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
ssl_tls: remove usage of DHE-PSK
48659a1 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
ssl_ciphersuites: remove references/usages of DHE-PSK
6348b46 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
analyze_outcomes.py: remove exceptions related to DHE-PSK
70cc4e6 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
components-configuration-crypto.sh: remove references to DHE_PSK kex
6e892cb 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
check_config: remove checks for DHE-PSK
a073452 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
mbedtls_config: remove MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
6ba324d 
This commit also removes its disabling from config_adjust_ssl.h

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

valeriosetti commented Jan 24, 2025 •
edited
Loading

I just rebased this PR as well because there were failures in analyze-outcomes.py which look similar to the ones I had yesterday in Mbed-TLS/TF-PSA-Crypto#165 and that were related to some recent framework change. Hopefully the rebase will fix this CI as well, let's see.

@ronald-cron-arm ronald-cron-arm self-requested a review January 24, 2025 11:12
ronald-cron-arm
ronald-cron-arm previously approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

The rebase and TF-PSA-Crypto pointer updates look good to me.

gabor-mezei-arm
gabor-mezei-arm previously approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me. All occurrences of the DHE-PSK macros are removed.

@valeriosetti
tf-psa-crypto: update reference
094fd49 
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
Copy link
Contributor Author

Since Mbed-TLS/TF-PSA-Crypto#165 was merged, I updated the last commit (tf-psa-crypto reference) so the PR is ready for reviews again

@valeriosetti valeriosetti removed the needs-reviewer This PR needs someone to pick it up for review label Jan 27, 2025
ronald-cron-arm
ronald-cron-arm approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM

gabor-mezei-arm
gabor-mezei-arm approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriosetti valeriosetti added the approved Design and code approved - may be waiting for CI or backports label Jan 27, 2025
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabor-mezei-arm gabor-mezei-arm gabor-mezei-arm approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

Assignees

@valeriosetti valeriosetti

Labels
approved Design and code approved - may be waiting for CI or backports needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap pull requests (new board)
Status: Has Approval
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove DHE-PSK key exchange
3 participants
@valeriosetti @ronald-cron-arm @gabor-mezei-arm