Skip to content

Fix TLS min/max for MBEDTLS 3.6.x #17048

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kkalganov
Copy link

MBEDTLS supports TLS 1.3 since release 3.6.0, adopt the default values for internal variables accordingly.

MBEDTLS supports TLS 1.3 since release 3.6.0, adopt the default values for internal variables accordingly.
@github-actions github-actions bot added the TLS label Apr 14, 2025
@testclutch
Copy link

Analysis of PR #17048 at 42eb0a24:

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-None-None-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-None-ciphers1251-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-None-ciphers1253-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-None-ciphers1254-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-ciphers1355-None-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-ciphers1356-ciphers1256-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test ../../tests/http/test_17_ssl_use.py::TestSSLUse::test_17_07_ssl_ciphers[TLSv1.2-ciphers1358-ciphers1258-True-True] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

There are more failures, but that's enough from Gha.

Generated by Testclutch

@vszakats
Copy link
Member

CI consistently fails on some mbedTLS tests with this PR:
https://github.com/curl/curl/actions/runs/14440733778/job/40534479776?pr=17048

@kkalganov
Copy link
Author

My understanding is that the tests fail as they still expect TLS 1.2, did I get it wrong?

Comment on lines 273 to 282
#else
/* mbedTLS 3.2.0 (2022) introduced new methods for setting TLS version */
#if MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_3;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#else
/* mbedTLS 3.2.0 (2022) introduced new methods for setting TLS version */
#if MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_3;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif
#endif
/* mbedTLS 3.2.0 (2022) introduced new methods for setting TLS version */
#elif MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_3;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif

Flatten the #if tree?

Just wondering, wouldn't ver_min be MBEDTLS_SSL_VERSION_TLS1_2 for 3.6+, assuming that it still supports TLSv1.2?

@vszakats
Copy link
Member

My understanding is that the tests fail as they still expect TLS 1.2, did I get it wrong?

I don't know what the issue/solution is here.

/cc @icing

@kkalganov
Copy link
Author

I would imaging the PR would need to be picked by someone with insights into the automated tests involved. In the meanwhile it may help people struggling to make libcurl+mbedtls combo to work with TLS1.3.

@bagder
Copy link
Member

bagder commented Apr 15, 2025

It's not about the test case. Which is the minimum supported TLS version when using mbedTLS 3.6? I would presume it is 1.2.

Comment on lines +275 to +281
#if MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_3;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_3;
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif
mbedtls_ssl_protocol_version ver_min = MBEDTLS_SSL_VERSION_TLS1_2;
#if MBEDTLS_VERSION_NUMBER < 0x03060000
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_2;
#else
mbedtls_ssl_protocol_version ver_max = MBEDTLS_SSL_VERSION_TLS1_3;
#endif

Copy link
Author

Choose a reason for hiding this comment

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

Well, based on the code for 3.2 I assumed you wanted the most recent as default.
Anyway, I stumbled upon this while having MBEDTLS compiled with support for TLS 1.3 only. Also, I tried with ver_min set to 1.2 as you suggest, but got exactly the same error (curl error buffer: ssl_setup failed - mbedTLS: (-0x5E80) SSL - Invalid value in SSL config). Is it possible that curl tries to use ver_min first?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that curl tries to use ver_min first?

Look at the code in that file. The ver_min and ver_max variables are used to set min and max accepted tls versions to mbedtls. It is then mbedtls that does the TLS negotiation.

If mbedTLS then somehow still does not negotiate the correct version it would to me seem to be to not be a curl issue. TLS 1.3 surely should not be the minimum accepted version by default.

Copy link
Author

Choose a reason for hiding this comment

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

Having support for the whole ver_min/ver_max span as the underlying library has is understandable, and Linux Distros and beyond are likely to ship mbedTLS with support for 1.2-1.3.

However, with such strategy, I wonder if setting explicit min/max is the way to go - why don't let the library to support what is does support? Wouldn't it be a more generic approach?

Copy link
Member

Choose a reason for hiding this comment

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

why don't let the library to support what is does support? Wouldn't it be a more generic approach?

Sure, that should work fine as long as the library is fine with it.

@curl curl deleted a comment Apr 19, 2025
bagder added a commit that referenced this pull request Apr 22, 2025
Reported-by: kkalganov on github
Fixes #17048
bagder added a commit that referenced this pull request Apr 24, 2025
Reported-by: kkalganov on github
Fixes #17048
@bagder bagder closed this in 437c72f Apr 24, 2025
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
Co-authored-by: Viktor Szakats
Reported-by: kkalganov on github
Fixes curl#17048
Closes curl#17137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants