Conversation
|
Analysis of PR #22475 at 47a1d320: Test 2118 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 80 different CI jobs (the link just goes to one of them). Generated by Testclutch |
| easy_setopt(curl, CURLOPT_IPRESOLVE, (long)CURL_IPRESOLVE_V4); | ||
| easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L); | ||
| easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L); |
There was a problem hiding this comment.
| easy_setopt(curl, CURLOPT_IPRESOLVE, (long)CURL_IPRESOLVE_V4); | |
| easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L); | |
| easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L); | |
| easy_setopt(curl, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); | |
| easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L); | |
| easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 1L); |
Let's drop a redundant cast, and use the modern value for VERIFYHOST.
There was a problem hiding this comment.
Done, applied as suggested.
Those two backends do not enforce the name check when peer verification is disabled, so exclude them like test 313 does. Also drop a redundant cast.
|
The 2118 CI failures were all wolfSSL and rustls builds. There the mismatching certificate is accepted with |
There was a problem hiding this comment.
Pull request overview
Fixes mbedTLS hostname verification when peer verification is disabled.
Changes:
- Preserves hostname-mismatch flags independently of peer verification.
- Adds regression test 2118 and registers it in test manifests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
lib/vtls/mbedtls.c |
Corrects verification-flag filtering. |
tests/libtest/lib2118.c |
Implements the regression test client. |
tests/data/test2118 |
Defines the test scenario and expected failure. |
tests/libtest/Makefile.inc |
Registers the test client. |
tests/data/Makefile.am |
Registers the test definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Repro:
CURLOPT_SSL_VERIFYPEER0 together withCURLOPT_SSL_VERIFYHOST2, against a server holding a certificate issued for a different name. ExpectedCURLE_PEER_FAILED_VERIFICATION, gotCURLE_OK.Cause:
mbed_verify_cb()clears the whole flag bitmask when peer verification is off, soMBEDTLS_X509_BADCERT_CN_MISMATCHgoes with it. Nothing else in the backend looks at the name, soverifyhosthas no effect on that path. The comment abovembedtls_ssl_conf_authmode()already says only the flags of a disabled check should be cleared.Fix: clear the flags of the disabled check only, so a name mismatch survives a disabled peer verification. OpenSSL, GnuTLS, wolfSSL and Schannel all keep the two options independent already, Schannel with an explicit branch for exactly this combination.
Added libtest 2118, which reuses the
localhost.nncertificate from test 312. It fails on mbedTLS without the change and passes with it; unaffected on the other backends.