From 03d37ae3ffa1eb937bfaddfa07e51ef2e84aa7d2 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 23 Sep 2024 14:37:29 -0400 Subject: cmFileCommand: Clarify names and logic using optional --- CTestCustom.cmake.in | 2 +- Source/cmFileCommand.cxx | 72 ++++++++++++++++++++++++------------------------ 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/CTestCustom.cmake.in b/CTestCustom.cmake.in index ae55715..bc88271 100644 --- a/CTestCustom.cmake.in +++ b/CTestCustom.cmake.in @@ -86,7 +86,7 @@ list(APPEND CTEST_CUSTOM_WARNING_EXCEPTION "[0-9]+ Warning\\(s\\) detected" # SunPro # Ignore false positive on `cm::optional` usage from GCC - "cmFileCommand.cxx:[0-9]*:[0-9]*: warning: '\\*\\(\\(void\\*\\)& tls_verify \\+2\\)' may be used uninitialized in this function \\[-Wmaybe-uninitialized\\]" + "cmFileCommand.cxx:[0-9]*:[0-9]*: warning: '\\*\\(\\(void\\*\\)& tlsVerifyOpt \\+2\\)' may be used uninitialized in this function \\[-Wmaybe-uninitialized\\]" "cmGlobalNinjaGenerator.cxx:[0-9]*:[0-9]*: warning: '.*cm::optional::_mem\\)\\)' may be used uninitialized \\[-Wmaybe-uninitialized\\]" "cmGlobalNinjaGenerator.cxx:[0-9]*:[0-9]*: note: '.*cm::optional::_mem\\)\\)' was declared here" "cmGlobalNinjaGenerator.cxx:[0-9]*:[0-9]*: warning: '\\*\\(\\(void\\*\\)& modmap_fmt \\+4\\)' may be used uninitialized in this function \\[-Wmaybe-uninitialized\\]" diff --git a/Source/cmFileCommand.cxx b/Source/cmFileCommand.cxx index 48ea01d..31b0271 100644 --- a/Source/cmFileCommand.cxx +++ b/Source/cmFileCommand.cxx @@ -1932,8 +1932,8 @@ bool HandleDownloadCommand(std::vector const& args, long inactivity_timeout = 0; std::string logVar; std::string statusVar; - cm::optional tls_version; - cm::optional tls_verify; + cm::optional tlsVersionOpt; + cm::optional tlsVerifyOpt; cmValue cainfo = status.GetMakefile().GetDefinition("CMAKE_TLS_CAINFO"); std::string netrc_level = status.GetMakefile().GetSafeDefinition("CMAKE_NETRC"); @@ -1982,7 +1982,7 @@ bool HandleDownloadCommand(std::vector const& args, } else if (*i == "TLS_VERSION") { ++i; if (i != args.end()) { - tls_version = *i; + tlsVersionOpt = *i; } else { status.SetError("DOWNLOAD missing value for TLS_VERSION."); return false; @@ -1990,7 +1990,7 @@ bool HandleDownloadCommand(std::vector const& args, } else if (*i == "TLS_VERIFY") { ++i; if (i != args.end()) { - tls_verify = cmIsOn(*i); + tlsVerifyOpt = cmIsOn(*i); } else { status.SetError("DOWNLOAD missing bool value for TLS_VERIFY."); return false; @@ -2098,27 +2098,27 @@ bool HandleDownloadCommand(std::vector const& args, ++i; } - if (!tls_verify) { + if (!tlsVerifyOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERIFY")) { - tls_verify = v.IsOn(); + tlsVerifyOpt = v.IsOn(); } } - if (!tls_verify) { + if (!tlsVerifyOpt.has_value()) { if (cm::optional v = cmSystemTools::GetEnvVar("CMAKE_TLS_VERIFY")) { - tls_verify = cmIsOn(*v); + tlsVerifyOpt = cmIsOn(*v); } } - if (!tls_version) { + if (!tlsVersionOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERSION")) { - tls_version = *v; + tlsVersionOpt = *v; } } - if (!tls_version) { + if (!tlsVersionOpt.has_value()) { if (cm::optional v = cmSystemTools::GetEnvVar("CMAKE_TLS_VERSION")) { - tls_version = std::move(v); + tlsVersionOpt = std::move(v); } } @@ -2202,21 +2202,21 @@ bool HandleDownloadCommand(std::vector const& args, cmFileCommandCurlDebugCallback); check_curl_result(res, "DOWNLOAD cannot set debug function: "); - if (tls_version) { - if (cm::optional v = cmCurlParseTLSVersion(*tls_version)) { + if (tlsVersionOpt.has_value()) { + if (cm::optional v = cmCurlParseTLSVersion(*tlsVersionOpt)) { res = ::curl_easy_setopt(curl, CURLOPT_SSLVERSION, *v); - check_curl_result( - res, - cmStrCat("DOWNLOAD cannot set TLS/SSL version ", *tls_version, ": ")); + check_curl_result(res, + cmStrCat("DOWNLOAD cannot set TLS/SSL version ", + *tlsVersionOpt, ": ")); } else { status.SetError( - cmStrCat("DOWNLOAD given unknown TLS/SSL version ", *tls_version)); + cmStrCat("DOWNLOAD given unknown TLS/SSL version ", *tlsVersionOpt)); return false; } } // check to see if TLS verification is requested - if (tls_verify && *tls_verify) { + if (tlsVerifyOpt.has_value() && tlsVerifyOpt.value()) { res = ::curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1); check_curl_result(res, "DOWNLOAD cannot set TLS/SSL Verify on: "); } else { @@ -2404,8 +2404,8 @@ bool HandleUploadCommand(std::vector const& args, std::string logVar; std::string statusVar; bool showProgress = false; - cm::optional tls_version; - cm::optional tls_verify; + cm::optional tlsVersionOpt; + cm::optional tlsVerifyOpt; cmValue cainfo = status.GetMakefile().GetDefinition("CMAKE_TLS_CAINFO"); std::string userpwd; std::string netrc_level = @@ -2451,7 +2451,7 @@ bool HandleUploadCommand(std::vector const& args, } else if (*i == "TLS_VERSION") { ++i; if (i != args.end()) { - tls_version = *i; + tlsVersionOpt = *i; } else { status.SetError("UPLOAD missing value for TLS_VERSION."); return false; @@ -2459,7 +2459,7 @@ bool HandleUploadCommand(std::vector const& args, } else if (*i == "TLS_VERIFY") { ++i; if (i != args.end()) { - tls_verify = cmIsOn(*i); + tlsVerifyOpt = cmIsOn(*i); } else { status.SetError("UPLOAD missing bool value for TLS_VERIFY."); return false; @@ -2511,27 +2511,27 @@ bool HandleUploadCommand(std::vector const& args, ++i; } - if (!tls_verify) { + if (!tlsVerifyOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERIFY")) { - tls_verify = v.IsOn(); + tlsVerifyOpt = v.IsOn(); } } - if (!tls_verify) { + if (!tlsVerifyOpt.has_value()) { if (cm::optional v = cmSystemTools::GetEnvVar("CMAKE_TLS_VERIFY")) { - tls_verify = cmIsOn(*v); + tlsVerifyOpt = cmIsOn(*v); } } - if (!tls_version) { + if (!tlsVersionOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERSION")) { - tls_version = *v; + tlsVersionOpt = *v; } } - if (!tls_version) { + if (!tlsVersionOpt.has_value()) { if (cm::optional v = cmSystemTools::GetEnvVar("CMAKE_TLS_VERSION")) { - tls_version = std::move(v); + tlsVersionOpt = std::move(v); } } @@ -2580,21 +2580,21 @@ bool HandleUploadCommand(std::vector const& args, cmFileCommandCurlDebugCallback); check_curl_result(res, "UPLOAD cannot set debug function: "); - if (tls_version) { - if (cm::optional v = cmCurlParseTLSVersion(*tls_version)) { + if (tlsVersionOpt.has_value()) { + if (cm::optional v = cmCurlParseTLSVersion(*tlsVersionOpt)) { res = ::curl_easy_setopt(curl, CURLOPT_SSLVERSION, *v); check_curl_result( res, - cmStrCat("UPLOAD cannot set TLS/SSL version ", *tls_version, ": ")); + cmStrCat("UPLOAD cannot set TLS/SSL version ", *tlsVersionOpt, ": ")); } else { status.SetError( - cmStrCat("UPLOAD given unknown TLS/SSL version ", *tls_version)); + cmStrCat("UPLOAD given unknown TLS/SSL version ", *tlsVersionOpt)); return false; } } // check to see if TLS verification is requested - if (tls_verify && *tls_verify) { + if (tlsVerifyOpt.has_value() && tlsVerifyOpt.value()) { res = ::curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1); check_curl_result(res, "UPLOAD cannot set TLS/SSL Verify on: "); } else { -- cgit v0.12 From dcaea54898cccb9ac1f675a395ac1793d1abfcfc Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 23 Sep 2024 14:39:58 -0400 Subject: cmCTestCurl: Clarify names and logic using optional --- Source/CTest/cmCTestCurl.cxx | 4 ++-- Source/CTest/cmCTestSubmitHandler.cxx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Source/CTest/cmCTestCurl.cxx b/Source/CTest/cmCTestCurl.cxx index 7137e63..6f887f3 100644 --- a/Source/CTest/cmCTestCurl.cxx +++ b/Source/CTest/cmCTestCurl.cxx @@ -84,11 +84,11 @@ bool cmCTestCurl::InitCurl() return false; } cmCurlSetCAInfo(this->Curl); - if (this->CurlOpts.TLSVersionOpt) { + if (this->CurlOpts.TLSVersionOpt.has_value()) { curl_easy_setopt(this->Curl, CURLOPT_SSLVERSION, *this->CurlOpts.TLSVersionOpt); } - if (this->CurlOpts.TLSVerifyOpt) { + if (this->CurlOpts.TLSVerifyOpt.has_value()) { curl_easy_setopt(this->Curl, CURLOPT_SSL_VERIFYPEER, *this->CurlOpts.TLSVerifyOpt ? 1 : 0); } diff --git a/Source/CTest/cmCTestSubmitHandler.cxx b/Source/CTest/cmCTestSubmitHandler.cxx index f05b874..91dea55 100644 --- a/Source/CTest/cmCTestSubmitHandler.cxx +++ b/Source/CTest/cmCTestSubmitHandler.cxx @@ -181,7 +181,7 @@ bool cmCTestSubmitHandler::SubmitUsingHTTP( curl = cm_curl_easy_init(); if (curl) { cmCurlSetCAInfo(curl); - if (curlOpts.TLSVersionOpt) { + if (curlOpts.TLSVersionOpt.has_value()) { cm::optional tlsVersionStr = cmCurlPrintTLSVersion(*curlOpts.TLSVersionOpt); cmCTestOptionalLog( @@ -191,7 +191,7 @@ bool cmCTestSubmitHandler::SubmitUsingHTTP( this->Quiet); curl_easy_setopt(curl, CURLOPT_SSLVERSION, *curlOpts.TLSVersionOpt); } - if (curlOpts.TLSVerifyOpt) { + if (curlOpts.TLSVerifyOpt.has_value()) { cmCTestOptionalLog(this->CTest, HANDLER_VERBOSE_OUTPUT, " Set CURLOPT_SSL_VERIFYPEER to " << (*curlOpts.TLSVerifyOpt ? "on" : "off") -- cgit v0.12 From 8e92ee34f6ba4058ca7dc793009f98f2096d8f38 Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 20 Sep 2024 16:55:26 -0400 Subject: file(DOWNLOAD/UPLOAD): Verify TLS server certificate by default If the connection fails in a way that might be a certificate error, and verification was enabled by the new default, mention environment variable `CMAKE_TLS_VERIFY` in the diagnostic to help users that were relying on the old behavior turn off server certificate verification in their environment. Fixes: #23608 --- Help/command/file.rst | 15 ++++++--- Help/release/dev/curl-tls-verify.rst | 10 ++++++ Help/variable/CMAKE_TLS_VERIFY.rst | 7 ++++- Source/cmFileCommand.cxx | 36 +++++++++++++++++++--- .../file-DOWNLOAD/TLS_VERIFY-bad-stdout.txt | 2 +- Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad.cmake | 4 +-- 6 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 Help/release/dev/curl-tls-verify.rst diff --git a/Help/command/file.rst b/Help/command/file.rst index 315762e..40689c9 100644 --- a/Help/command/file.rst +++ b/Help/command/file.rst @@ -813,8 +813,15 @@ Transfer ``TLS_VERIFY `` Specify whether to verify the server certificate for ``https://`` URLs. - The default is to *not* verify. If this option is not specified, the - value of the :variable:`CMAKE_TLS_VERIFY` variable will be used instead. + If this option is not specified, the value of the + :variable:`CMAKE_TLS_VERIFY` variable or :envvar:`CMAKE_TLS_VERIFY` + environment variable will be used instead. + If neither is set, the default is *on*. + + .. versionchanged:: 3.31 + The default is on. Previously, the default was off. + Users may set the :envvar:`CMAKE_TLS_VERIFY` environment + variable to ``0`` to restore the old default. .. versionadded:: 3.18 Added support to ``file(UPLOAD)``. @@ -827,9 +834,7 @@ Transfer .. versionadded:: 3.18 Added support to ``file(UPLOAD)``. - For ``https://`` URLs CMake must be built with OpenSSL support. ``TLS/SSL`` - certificates are not checked by default. Set ``TLS_VERIFY`` to ``ON`` to - check certificates. + For ``https://`` URLs CMake must be built with SSL/TLS support. Additional options to ``DOWNLOAD`` are: diff --git a/Help/release/dev/curl-tls-verify.rst b/Help/release/dev/curl-tls-verify.rst new file mode 100644 index 0000000..73e1837 --- /dev/null +++ b/Help/release/dev/curl-tls-verify.rst @@ -0,0 +1,10 @@ +curl-tls-verify +--------------- + +* The :command:`file(DOWNLOAD)` and :command:`file(UPLOAD)` commands now + verify TLS server certificates for connections to ``https://`` URLs by + default. See the :variable:`CMAKE_TLS_VERIFY` variable for details. + This change was made without a policy so that users are protected + even when building projects that have not been updated. + Users may set the :envvar:`CMAKE_TLS_VERIFY` environment + variable to ``0`` to restore the old default. diff --git a/Help/variable/CMAKE_TLS_VERIFY.rst b/Help/variable/CMAKE_TLS_VERIFY.rst index 5871ac7..0ecb701 100644 --- a/Help/variable/CMAKE_TLS_VERIFY.rst +++ b/Help/variable/CMAKE_TLS_VERIFY.rst @@ -5,7 +5,12 @@ Specify the default value for the :command:`file(DOWNLOAD)` and :command:`file(UPLOAD)` commands' ``TLS_VERIFY`` options. If this variable is not set, the commands check the :envvar:`CMAKE_TLS_VERIFY` environment variable. -If neither is set, the default is *off*. +If neither is set, the default is *on*. + +.. versionchanged:: 3.31 + The default is on. Previously, the default was off. + Users may set the :envvar:`CMAKE_TLS_VERIFY` environment + variable to ``0`` to restore the old default. This variable is also used by the :module:`ExternalProject` and :module:`FetchContent` modules for internal calls to :command:`file(DOWNLOAD)`. diff --git a/Source/cmFileCommand.cxx b/Source/cmFileCommand.cxx index 31b0271..30d92ca 100644 --- a/Source/cmFileCommand.cxx +++ b/Source/cmFileCommand.cxx @@ -1740,6 +1740,8 @@ bool HandleNativePathCommand(std::vector const& args, #if !defined(CMAKE_BOOTSTRAP) +const bool TLS_VERIFY_DEFAULT = true; + // Stuff for curl download/upload using cmFileCommandVectorOfChar = std::vector; @@ -2109,6 +2111,11 @@ bool HandleDownloadCommand(std::vector const& args, tlsVerifyOpt = cmIsOn(*v); } } + bool tlsVerifyDefaulted = false; + if (!tlsVerifyOpt.has_value()) { + tlsVerifyOpt = TLS_VERIFY_DEFAULT; + tlsVerifyDefaulted = true; + } if (!tlsVersionOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERSION")) { @@ -2317,9 +2324,17 @@ bool HandleDownloadCommand(std::vector const& args, ::curl_easy_cleanup(curl); if (!statusVar.empty()) { + std::string m = curl_easy_strerror(res); + if ((res == CURLE_SSL_CONNECT_ERROR || + res == CURLE_PEER_FAILED_VERIFICATION) && + tlsVerifyDefaulted) { + m = cmStrCat( + std::move(m), + ". If this is due to https certificate verification failure, one may " + "set environment variable CMAKE_TLS_VERIFY=0 to suppress it."); + } status.GetMakefile().AddDefinition( - statusVar, - cmStrCat(static_cast(res), ";\"", ::curl_easy_strerror(res), "\"")); + statusVar, cmStrCat(static_cast(res), ";\"", std::move(m), "\"")); } ::curl_global_cleanup(); @@ -2522,6 +2537,11 @@ bool HandleUploadCommand(std::vector const& args, tlsVerifyOpt = cmIsOn(*v); } } + bool tlsVerifyDefaulted = false; + if (!tlsVerifyOpt.has_value()) { + tlsVerifyOpt = TLS_VERIFY_DEFAULT; + tlsVerifyDefaulted = true; + } if (!tlsVersionOpt.has_value()) { if (cmValue v = status.GetMakefile().GetDefinition("CMAKE_TLS_VERSION")) { @@ -2697,9 +2717,17 @@ bool HandleUploadCommand(std::vector const& args, ::curl_easy_cleanup(curl); if (!statusVar.empty()) { + std::string m = curl_easy_strerror(res); + if ((res == CURLE_SSL_CONNECT_ERROR || + res == CURLE_PEER_FAILED_VERIFICATION) && + tlsVerifyDefaulted) { + m = cmStrCat( + std::move(m), + ". If this is due to https certificate verification failure, one may " + "set environment variable CMAKE_TLS_VERIFY=0 to suppress it."); + } status.GetMakefile().AddDefinition( - statusVar, - cmStrCat(static_cast(res), ";\"", ::curl_easy_strerror(res), "\"")); + statusVar, cmStrCat(static_cast(res), ";\"", std::move(m), "\"")); } ::curl_global_cleanup(); diff --git a/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad-stdout.txt b/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad-stdout.txt index fbff3b9..d32292f 100644 --- a/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad-stdout.txt +++ b/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad-stdout.txt @@ -1,4 +1,4 @@ --- def-0: 0;"No error" +-- def-1: (60;"SSL peer certificate or SSH remote key was not OK|35;"SSL connect error)\. If this is due to https certificate verification failure, one may set environment variable CMAKE_TLS_VERIFY=0 to suppress it\." -- env-0: 0;"No error" -- env-1: (60;"SSL peer certificate or SSH remote key was not OK"|35;"SSL connect error") -- var-0: 0;"No error" diff --git a/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad.cmake b/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad.cmake index 7d50ece..44fcfae 100644 --- a/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad.cmake +++ b/Tests/RunCMake/file-DOWNLOAD/TLS_VERIFY-bad.cmake @@ -7,10 +7,10 @@ function(download case) endif() endfunction() -# The default is OFF. +# The default is ON. unset(ENV{CMAKE_TLS_VERIFY}) unset(CMAKE_TLS_VERIFY) -download(def-0) +download(def-1) # The environment variable overrides the default. set(ENV{CMAKE_TLS_VERIFY} 0) -- cgit v0.12 From 4e62bc943c74cbc564209a42bb84605f0771bca7 Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 20 Sep 2024 17:40:02 -0400 Subject: ctest: Verify TLS server certificate by default Issue: #23608 --- Help/manual/ctest.1.rst | 5 +++++ Help/release/dev/curl-tls-verify.rst | 4 ++++ Help/variable/CTEST_TLS_VERIFY.rst | 6 ++++++ Source/CTest/cmCTestCurl.cxx | 7 +++++++ 4 files changed, 22 insertions(+) diff --git a/Help/manual/ctest.1.rst b/Help/manual/ctest.1.rst index c9ab31e..4793ef5 100644 --- a/Help/manual/ctest.1.rst +++ b/Help/manual/ctest.1.rst @@ -1569,6 +1569,11 @@ Configuration settings include: * `CTest Script`_ variable: :variable:`CTEST_TLS_VERIFY` * :module:`CTest` module variable: ``CTEST_TLS_VERIFY`` + .. versionchanged:: 3.31 + The default is on. Previously, the default was off. + Users may set the :envvar:`CMAKE_TLS_VERIFY` environment + variable to ``0`` to restore the old default. + ``TriggerSite`` Legacy option. Not used. diff --git a/Help/release/dev/curl-tls-verify.rst b/Help/release/dev/curl-tls-verify.rst index 73e1837..96ee421 100644 --- a/Help/release/dev/curl-tls-verify.rst +++ b/Help/release/dev/curl-tls-verify.rst @@ -8,3 +8,7 @@ curl-tls-verify even when building projects that have not been updated. Users may set the :envvar:`CMAKE_TLS_VERIFY` environment variable to ``0`` to restore the old default. + +* The :command:`ctest_submit` command and :option:`ctest -T Submit ` + step now verify TLS server certificates for connections to ``https://`` URLs + by default. See the :variable:`CTEST_TLS_VERIFY` variable for details. diff --git a/Help/variable/CTEST_TLS_VERIFY.rst b/Help/variable/CTEST_TLS_VERIFY.rst index 9b3d96c..b283842 100644 --- a/Help/variable/CTEST_TLS_VERIFY.rst +++ b/Help/variable/CTEST_TLS_VERIFY.rst @@ -11,3 +11,9 @@ to a dashboard via ``https://`` URLs. If ``CTEST_TLS_VERIFY`` is not set, the :variable:`CMAKE_TLS_VERIFY` variable or :envvar:`CMAKE_TLS_VERIFY` environment variable is used instead. +If neither is set, the default is *on*. + +.. versionchanged:: 3.31 + The default is on. Previously, the default was off. + Users may set the :envvar:`CMAKE_TLS_VERIFY` environment + variable to ``0`` to restore the old default. diff --git a/Source/CTest/cmCTestCurl.cxx b/Source/CTest/cmCTestCurl.cxx index 6f887f3..d9dc3b2 100644 --- a/Source/CTest/cmCTestCurl.cxx +++ b/Source/CTest/cmCTestCurl.cxx @@ -14,6 +14,10 @@ #include "cmSystemTools.h" #include "cmValue.h" +namespace { +const bool TLS_VERIFY_DEFAULT = true; +} + cmCTestCurl::cmCTestCurl(cmCTest* ctest) : CTest(ctest) , CurlOpts(ctest) @@ -76,6 +80,9 @@ cmCTestCurlOpts::cmCTestCurlOpts(cmCTest* ctest) } } } + if (!this->TLSVerifyOpt.has_value()) { + this->TLSVerifyOpt = TLS_VERIFY_DEFAULT; + } } bool cmCTestCurl::InitCurl() -- cgit v0.12