Skip to content

check_config: add missing check for TLS 1.3 key exchanges#10650

Open
valeriosetti wants to merge 2 commits intoMbed-TLS:developmentfrom
valeriosetti:fix-tls13-guard
Open

check_config: add missing check for TLS 1.3 key exchanges#10650
valeriosetti wants to merge 2 commits intoMbed-TLS:developmentfrom
valeriosetti:fix-tls13-guard

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Mar 23, 2026 •
edited
Loading

Description

Add a check to ensure that at least 1 key exchange is selected when TLS 1.3 is also enabled.

PR checklist

  • changelog not required because: minor, just adding a guard to a function
  • development PR not required because: this one
  • Mbed TLS 4.1 backport TODO!
  • TF-PSA-Crypto PR not required because: no change there
  • framework PR not required
  • 3.6 PR not required because: not backported
  • tests not required because: no code change

@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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Mar 23, 2026
@valeriosetti valeriosetti moved this to Triage in in Community Mar 23, 2026
@valeriosetti valeriosetti mentioned this pull request Mar 23, 2026
Closed
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Mar 23, 2026
bjwtaylor
bjwtaylor previously approved these changes Mar 26, 2026
View reviewed changes
@ronald-cron-arm ronald-cron-arm self-requested a review April 15, 2026 12:56
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Apr 15, 2026
@valeriosetti valeriosetti mentioned this pull request Apr 17, 2026
Closed
7 tasks
ronald-cron-arm
ronald-cron-arm requested changes Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown
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.

This seems to be for a configuration where MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_SRV_C are defined but none of MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED, MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED and MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED.

In that case, I'd say that we rather miss in mbedtls_check_config.h for TLS 1.3 a check similar to the TLS 1.2 one:

mbedtls/library/mbedtls_check_config.h

Lines 135 to 143 in 8426c9b

#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \
!(defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) )
#error "One or more versions of the TLS protocol are enabled " \
"but no key exchange methods defined with MBEDTLS_KEY_EXCHANGE_xxxx"
#endif
.

@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Apr 21, 2026 •
edited
Loading

In that case, I'd say that we rather miss in mbedtls_check_config.h for TLS 1.3 a check similar to the TLS 1.2 one:

I just reshaped the PR to implement what you proposed here ;)
I split changes into 2 commits:

  • the 1st one should not be ported because it changes check_config and that might not be OK for an LTS branch;
  • the 2nd one can be ported IMO because it's simply updating the error message.

Wdyt?

ronald-cron-arm
ronald-cron-arm requested changes Apr 22, 2026
View reviewed changes
Copy link
Copy Markdown
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 apart from the pre-processor error.

Comment thread library/mbedtls_check_config.h Outdated
#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \
!(defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED) || \
defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED) || \
defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED) ) \
Copy link
Copy Markdown
Contributor

@ronald-cron-arm ronald-cron-arm Apr 22, 2026

Choose a reason for hiding this comment

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

Suggested change
defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED) ) \
defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED) )

@valeriosetti
check_config: add check for TLS 1.3 key exchanges
5ea7720 
When MBEDTLS_SSL_PROTO_TLS1_3 is enabled ensure that at least one of the
related key exchanges is also enabled.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
check_config: fix error message for missing TLS 1.2 key exchanges
c3d52b9 
Align the error message to the one used for the same check in TLS 1.3.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti changed the title library: tls13: add guard for ssl_tls13_client_hello_has_exts() check_config: add missing check for TLS 1.3 key exchanges Apr 22, 2026
@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Apr 22, 2026

Since the goal of the PR changed recently, I updated the title to make it more accurate of the content of the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjwtaylor bjwtaylor Awaiting requested review from bjwtaylor

@ronald-cron-arm ronald-cron-arm Awaiting requested review from ronald-cron-arm

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)

Projects

Community
Status: Triage in
Roadmap pull requests (new board)
Status: In Development

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@valeriosetti @bjwtaylor @ronald-cron-arm