Closed
Conversation
Since 9d8998c, the setopt code changes input DEFAULT to an actual more specific TLS version (1.2) for the backends to use and check for. This means that the default value (0L) cannot and should not actually be used when the TLS backends run. This change adds asserts to verify that and removes code that accepts the DEFAULT value as a valid version with the TLS version functions' logic. Applications can still set a specific lower version if they want (1, 1.0 or 1.1).
There was a problem hiding this comment.
Pull request overview
This pull request removes checks for CURL_SSLVERSION_DEFAULT from TLS backend implementations, following a change where the setopt code now converts DEFAULT (0) to TLSv1.2 before the backends process it. The PR adds defensive assertions to verify that DEFAULT is never passed to backends and removes now-unreachable code branches.
Changes:
- Added
DEBUGASSERTstatements in all TLS backends to verify thatCURL_SSLVERSION_DEFAULTis not passed to backend functions - Removed
case CURL_SSLVERSION_DEFAULT:statements from switch blocks in TLS backend version handling code - Simplified condition in gtls.c from explicit check for DEFAULT/TLSv1 to a less-than-or-equal comparison
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/vtls/wolfssl.c | Added DEBUGASSERT and removed DEFAULT case from ssl_version switch |
| lib/vtls/schannel.c | Added DEBUGASSERT and removed DEFAULT case from version switch |
| lib/vtls/rustls.c | Added DEBUGASSERT and removed DEFAULT case from init_config_builder switch |
| lib/vtls/openssl.c | Added DEBUGASSERT in two functions and removed DEFAULT cases from two switch statements |
| lib/vtls/mbedtls.c | Added DEBUGASSERT and removed DEFAULT case from mbed_set_ssl_version_min_max switch |
| lib/vtls/gtls.c | Added DEBUGASSERT and changed condition from explicit DEFAULT/TLSv1 check to <= TLSv1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since 9d8998c, the setopt code changes input DEFAULT to an actual more specific TLS version (1.2) for the backends to use and check for.
This means that the default value (0L) cannot and should not actually be used when the TLS backends run. This change adds asserts to verify that and removes code that accepts the DEFAULT value as a valid version within the TLS version functions' logic.
Applications can still set a specific lower version if they want (1, 1.0 or 1.1).