Merge topic 'curl-tls-verify'

4e62bc943c ctest: Verify TLS server certificate by default
8e92ee34f6 file(DOWNLOAD/UPLOAD): Verify TLS server certificate by default
dcaea54898 cmCTestCurl: Clarify names and logic using optional<bool>
03d37ae3ff cmFileCommand: Clarify names and logic using optional<bool>

Acked-by: Kitware Robot <[email protected]>
Merge-request: !9843

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<CxxModuleMapFormat>::_mem\\)\\)' may be used uninitialized \\[-Wmaybe-uninitialized\\]"
   "cmGlobalNinjaGenerator.cxx:[0-9]*:[0-9]*: note: '.*cm::optional<CxxModuleMapFormat>::_mem\\)\\)' was declared here"
   "cmGlobalNinjaGenerator.cxx:[0-9]*:[0-9]*: warning: '\\*\\(\\(void\\*\\)& modmap_fmt \\+4\\)' may be used uninitialized in this function \\[-Wmaybe-uninitialized\\]"

Help/command/file.rst

@@ -813,8 +813,15 @@ Transfer
 
     ``TLS_VERIFY <ON|OFF>``
       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:
 

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.
 

Help/release/dev/curl-tls-verify.rst

@@ -0,0 +1,14 @@
+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.
+
+* The :command:`ctest_submit` command and :option:`ctest -T Submit <ctest -T>`
+  step now verify TLS server certificates for connections to ``https://`` URLs
+  by default.  See the :variable:`CTEST_TLS_VERIFY` variable for details.

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)`.

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.

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()
@@ -84,11 +91,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);
   }

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<std::string> 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")

Source/cmFileCommand.cxx

@@ -1740,6 +1740,8 @@ bool HandleNativePathCommand(std::vector<std::string> const& args,
 
 #if !defined(CMAKE_BOOTSTRAP)
 
+const bool TLS_VERIFY_DEFAULT = true;
+
 // Stuff for curl download/upload
 using cmFileCommandVectorOfChar = std::vector<char>;
 
@@ -1932,8 +1934,8 @@ bool HandleDownloadCommand(std::vector<std::string> const& args,
   long inactivity_timeout = 0;
   std::string logVar;
   std::string statusVar;
-  cm::optional<std::string> tls_version;
-  cm::optional<bool> tls_verify;
+  cm::optional<std::string> tlsVersionOpt;
+  cm::optional<bool> tlsVerifyOpt;
   cmValue cainfo = status.GetMakefile().GetDefinition("CMAKE_TLS_CAINFO");
   std::string netrc_level =
     status.GetMakefile().GetSafeDefinition("CMAKE_NETRC");
@@ -1982,7 +1984,7 @@ bool HandleDownloadCommand(std::vector<std::string> 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 +1992,7 @@ bool HandleDownloadCommand(std::vector<std::string> 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 +2100,32 @@ bool HandleDownloadCommand(std::vector<std::string> 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<std::string> v =
           cmSystemTools::GetEnvVar("CMAKE_TLS_VERIFY")) {
-      tls_verify = cmIsOn(*v);
+      tlsVerifyOpt = cmIsOn(*v);
     }
   }
+  bool tlsVerifyDefaulted = false;
+  if (!tlsVerifyOpt.has_value()) {
+    tlsVerifyOpt = TLS_VERIFY_DEFAULT;
+    tlsVerifyDefaulted = true;
+  }
 
-  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<std::string> v =
           cmSystemTools::GetEnvVar("CMAKE_TLS_VERSION")) {
-      tls_version = std::move(v);
+      tlsVersionOpt = std::move(v);
     }
   }
 
@@ -2202,21 +2209,21 @@ bool HandleDownloadCommand(std::vector<std::string> const& args,
                            cmFileCommandCurlDebugCallback);
   check_curl_result(res, "DOWNLOAD cannot set debug function: ");
 
-  if (tls_version) {
-    if (cm::optional<int> v = cmCurlParseTLSVersion(*tls_version)) {
+  if (tlsVersionOpt.has_value()) {
+    if (cm::optional<int> 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 {
@@ -2317,9 +2324,17 @@ bool HandleDownloadCommand(std::vector<std::string> 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<int>(res), ";\"", ::curl_easy_strerror(res), "\""));
+      statusVar, cmStrCat(static_cast<int>(res), ";\"", std::move(m), "\""));
   }
 
   ::curl_global_cleanup();
@@ -2404,8 +2419,8 @@ bool HandleUploadCommand(std::vector<std::string> const& args,
   std::string logVar;
   std::string statusVar;
   bool showProgress = false;
-  cm::optional<std::string> tls_version;
-  cm::optional<bool> tls_verify;
+  cm::optional<std::string> tlsVersionOpt;
+  cm::optional<bool> tlsVerifyOpt;
   cmValue cainfo = status.GetMakefile().GetDefinition("CMAKE_TLS_CAINFO");
   std::string userpwd;
   std::string netrc_level =
@@ -2451,7 +2466,7 @@ bool HandleUploadCommand(std::vector<std::string> 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 +2474,7 @@ bool HandleUploadCommand(std::vector<std::string> 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 +2526,32 @@ bool HandleUploadCommand(std::vector<std::string> 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<std::string> v =
           cmSystemTools::GetEnvVar("CMAKE_TLS_VERIFY")) {
-      tls_verify = cmIsOn(*v);
+      tlsVerifyOpt = cmIsOn(*v);
     }
   }
+  bool tlsVerifyDefaulted = false;
+  if (!tlsVerifyOpt.has_value()) {
+    tlsVerifyOpt = TLS_VERIFY_DEFAULT;
+    tlsVerifyDefaulted = true;
+  }
 
-  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<std::string> v =
           cmSystemTools::GetEnvVar("CMAKE_TLS_VERSION")) {
-      tls_version = std::move(v);
+      tlsVersionOpt = std::move(v);
     }
   }
 
@@ -2580,21 +2600,21 @@ bool HandleUploadCommand(std::vector<std::string> const& args,
                            cmFileCommandCurlDebugCallback);
   check_curl_result(res, "UPLOAD cannot set debug function: ");
 
-  if (tls_version) {
-    if (cm::optional<int> v = cmCurlParseTLSVersion(*tls_version)) {
+  if (tlsVersionOpt.has_value()) {
+    if (cm::optional<int> 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 {
@@ -2697,9 +2717,17 @@ bool HandleUploadCommand(std::vector<std::string> 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<int>(res), ";\"", ::curl_easy_strerror(res), "\""));
+      statusVar, cmStrCat(static_cast<int>(res), ";\"", std::move(m), "\""));
   }
 
   ::curl_global_cleanup();

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"

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)