Browse Source

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
Brad King 1 year ago
parent
commit
8e92ee34f6

+ 10 - 5
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:
 

+ 10 - 0
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.

+ 6 - 1
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)`.

+ 32 - 4
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>;
 
@@ -2109,6 +2111,11 @@ bool HandleDownloadCommand(std::vector<std::string> 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<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();
@@ -2522,6 +2537,11 @@ bool HandleUploadCommand(std::vector<std::string> 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<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();

+ 1 - 1
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"

+ 2 - 2
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)