From 4493930ac7a3715e19961e00aa0eb808dc8473ab Mon Sep 17 00:00:00 2001
From: Jared Casey <jared.casey@couchbase.com>
Date: Thu, 3 Oct 2024 17:42:34 -0400
Subject: [PATCH] CXXCBC-599: Update allowed connection string options

Changes
=======
* Enforce allowed connection string options for Columnar
* Enforce golang duration syntax for timeout options (e.g. don't default
  to millis)
* Update connstr tests
---
 core/utils/connection_string.cxx     | 180 +++++++++++++++++++--------
 test/test_unit_connection_string.cxx | 128 +++++++++++++++----
 2 files changed, 234 insertions(+), 74 deletions(-)

diff --git a/core/utils/connection_string.cxx b/core/utils/connection_string.cxx
index ac8173bbc..799256fe4 100644
--- a/core/utils/connection_string.cxx
+++ b/core/utils/connection_string.cxx
@@ -34,7 +34,7 @@ using namespace tao::pegtl;
 
 struct bucket_name : seq<uri::segment_nz> {
 };
-using param_key = star<sor<abnf::ALPHA, abnf::DIGIT, one<'_'>>>;
+using param_key = star<sor<abnf::ALPHA, abnf::DIGIT, one<'_', '.'>>>;
 using param_value = star<sor<minus<uri::pchar, one<'=', '&', '?'>>, one<'/'>>>;
 struct param : seq<param_key, one<'='>, param_value> {
 };
@@ -237,38 +237,69 @@ parse_option(bool& receiver,
 }
 
 void
-parse_option(tls_verify_mode& receiver,
+parse_option(io::ip_protocol& receiver,
              const std::string& name,
              const std::string& value,
              std::vector<std::string>& warnings)
 {
-  if (value == "none") {
-    receiver = tls_verify_mode::none;
-  } else if (value == "peer") {
-    receiver = tls_verify_mode::peer;
+  if (value == "any") {
+    receiver = io::ip_protocol::any;
+  } else if (value == "force_ipv4") {
+    receiver = io::ip_protocol::force_ipv4;
+  } else if (value == "force_ipv6") {
+    receiver = io::ip_protocol::force_ipv6;
   } else {
     warnings.push_back(fmt::format(
-      R"(unable to parse "{}" parameter in connection string (value "{}" is not a valid TLS verification mode))",
+      R"(unable to parse "{}" parameter in connection string (value "{}" is not a valid IP protocol preference))",
       name,
       value));
   }
 }
 
+#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
 void
-parse_option(io::ip_protocol& receiver,
+parse_option(std::chrono::milliseconds& receiver,
              const std::string& name,
              const std::string& value,
              std::vector<std::string>& warnings)
 {
-  if (value == "any") {
-    receiver = io::ip_protocol::any;
-  } else if (value == "force_ipv4") {
-    receiver = io::ip_protocol::force_ipv4;
-  } else if (value == "force_ipv6") {
-    receiver = io::ip_protocol::force_ipv6;
+  try {
+    receiver = std::chrono::duration_cast<std::chrono::milliseconds>(
+      parse_duration(string_codec::url_decode(value)));
+  } catch (const duration_parse_error& dpe) {
+    warnings.push_back(
+      fmt::format(R"(unable to parse "{}" parameter in connection string (value: "{}"): {})",
+                  name,
+                  value,
+                  dpe.what()));
+  } catch (const std::invalid_argument& ex1) {
+    warnings.push_back(fmt::format(
+      R"(unable to parse "{}" parameter in connection string (value "{}" is not a number): {})",
+      name,
+      value,
+      ex1.what()));
+  } catch (const std::out_of_range& ex2) {
+    warnings.push_back(fmt::format(
+      R"(unable to parse "{}" parameter in connection string (value "{}" is out of range): {})",
+      name,
+      value,
+      ex2.what()));
+  }
+}
+#else
+void
+parse_option(tls_verify_mode& receiver,
+             const std::string& name,
+             const std::string& value,
+             std::vector<std::string>& warnings)
+{
+  if (value == "none") {
+    receiver = tls_verify_mode::none;
+  } else if (value == "peer") {
+    receiver = tls_verify_mode::peer;
   } else {
     warnings.push_back(fmt::format(
-      R"(unable to parse "{}" parameter in connection string (value "{}" is not a valid IP protocol preference))",
+      R"(unable to parse "{}" parameter in connection string (value "{}" is not a valid TLS verification mode))",
       name,
       value));
   }
@@ -323,6 +354,7 @@ parse_option(std::chrono::milliseconds& receiver,
     }
   }
 }
+#endif
 
 void
 extract_options(connection_string& connstr)
@@ -333,6 +365,54 @@ extract_options(connection_string& connstr)
     connstr.options.enable_dns_srv = false;
   }
   for (const auto& [name, value] : connstr.params) {
+#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
+    if (name == "security.trust_only_pem_file") {
+      /**
+       * Set the trust cert path
+       */
+      parse_option(connstr.options.trust_certificate, name, value, connstr.warnings);
+    } else if (name == "security.disable_server_certificate_verification") {
+      /**
+       * Disable TLS server cert verification if set to true.
+       */
+      if (value == "true" || value == "yes" || value == "on" || value == "1") {
+        connstr.options.tls_verify = tls_verify_mode::none;
+      } else if (value == "false" || value == "no" || value == "off" || value == "0") {
+        connstr.options.tls_verify = tls_verify_mode::peer;
+      } else {
+        connstr.warnings.push_back(fmt::format(
+          R"(unable to parse "{}" parameter in connection string (value "{}" cannot be interpreted as a boolean))",
+          name,
+          value));
+      }
+
+    } else if (name == "timeout.connect_timeout") {
+      /**
+       * The period of time allocated to complete bootstrap
+       */
+      parse_option(connstr.options.bootstrap_timeout, name, value, connstr.warnings);
+    } else if (name == "timeout.dispatch_timeout") {
+      /**
+       * Number of seconds to wait before timing out a Query or N1QL request by the client.
+       */
+      parse_option(connstr.options.dispatch_timeout, name, value, connstr.warnings);
+    } else if (name == "timeout.query_timeout") {
+      /**
+       * Number of seconds to wait before timing out a Query or N1QL request by the client.
+       */
+      parse_option(connstr.options.query_timeout, name, value, connstr.warnings);
+    } else if (name == "timeout.resolve_timeout") {
+      /**
+       * The period of time to resolve DNS name of the node to IP address
+       */
+      parse_option(connstr.options.resolve_timeout, name, value, connstr.warnings);
+    } else if (name == "timeout.socket_connect_timeout") {
+      /**
+       * Number of seconds the client should wait while attempting to connect to a node’s KV service
+       * via a socket. Initial connection, reconnecting, node added, etc.
+       */
+      parse_option(connstr.options.connect_timeout, name, value, connstr.warnings);
+#else
     if (name == "kv_connect_timeout") {
       /**
        * Number of seconds the client should wait while attempting to connect to a node’s KV service
@@ -404,6 +484,7 @@ extract_options(connection_string& connstr)
       if (force_ipv4) {
         connstr.options.use_ip_protocol = io::ip_protocol::force_ipv4;
       }
+#endif
     } else if (name == "ip_protocol") {
       /**
        * Controls preference of IP protocol for name resolution
@@ -413,6 +494,37 @@ extract_options(connection_string& connstr)
       parse_option(connstr.options.config_poll_interval, name, value, connstr.warnings);
     } else if (name == "config_poll_floor") {
       parse_option(connstr.options.config_poll_floor, name, value, connstr.warnings);
+    } else if (name == "enable_dns_srv") {
+      if (connstr.bootstrap_nodes.size() == 1) {
+        parse_option(connstr.options.enable_dns_srv, name, value, connstr.warnings);
+      } else {
+        connstr.warnings.push_back(fmt::format(
+          R"(parameter "{}" requires single entry in bootstrap nodes list of the connection string, ignoring (value "{}"))",
+          name,
+          value));
+      }
+    } else if (name == "network") {
+      connstr.options.network =
+        value; /* current known values are "auto", "default" and "external" */
+    } else if (name == "user_agent_extra") {
+      /**
+       * string, that will be appended to identification fields of the server protocols (key in HELO
+       * packet for MCBP, "user-agent" header for HTTP)
+       */
+      parse_option(connstr.options.user_agent_extra, name, value, connstr.warnings);
+    } else if (name == "dump_configuration") {
+      /**
+       * Whether to dump every new configuration on TRACE level
+       */
+      parse_option(connstr.options.dump_configuration, name, value, connstr.warnings);
+    } else if (name == "enable_clustermap_notification") {
+      /**
+       * Allow the server to push configuration updates asynchronously.
+       */
+      parse_option(connstr.options.enable_clustermap_notification, name, value, connstr.warnings);
+    } else if (name == "disable_mozilla_ca_certificates") {
+      parse_option(connstr.options.disable_mozilla_ca_certificates, name, value, connstr.warnings);
+#ifndef COUCHBASE_CXX_CLIENT_COLUMNAR
     } else if (name == "max_http_connections") {
       /**
        * The maximum number of HTTP connections allowed on a per-host and per-port basis.  0
@@ -429,40 +541,16 @@ extract_options(connection_string& connstr)
        * The period of time allocated to complete bootstrap
        */
       parse_option(connstr.options.bootstrap_timeout, name, value, connstr.warnings);
-#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
-    } else if (name == "dispatch_timeout") {
-      /**
-       * The period of time allocated to complete HTTP session bootstrap
-       */
-      parse_option(connstr.options.dispatch_timeout, name, value, connstr.warnings);
-#endif
     } else if (name == "resolve_timeout") {
       /**
        * The period of time to resolve DNS name of the node to IP address
        */
       parse_option(connstr.options.resolve_timeout, name, value, connstr.warnings);
-    } else if (name == "enable_dns_srv") {
-      if (connstr.bootstrap_nodes.size() == 1) {
-        parse_option(connstr.options.enable_dns_srv, name, value, connstr.warnings);
-      } else {
-        connstr.warnings.push_back(fmt::format(
-          R"(parameter "{}" requires single entry in bootstrap nodes list of the connection string, ignoring (value "{}"))",
-          name,
-          value));
-      }
-    } else if (name == "network") {
-      connstr.options.network =
-        value; /* current known values are "auto", "default" and "external" */
     } else if (name == "show_queries") {
       /**
        * Whether to display N1QL, Analytics, Search queries on info level (default false)
        */
       parse_option(connstr.options.show_queries, name, value, connstr.warnings);
-    } else if (name == "enable_clustermap_notification") {
-      /**
-       * Allow the server to push configuration updates asynchronously.
-       */
-      parse_option(connstr.options.enable_clustermap_notification, name, value, connstr.warnings);
     } else if (name == "enable_unordered_execution") {
       /**
        * Allow the server to reorder commands
@@ -487,25 +575,13 @@ extract_options(connection_string& connstr)
       parse_option(connstr.options.enable_metrics, name, value, connstr.warnings);
     } else if (name == "tls_verify") {
       parse_option(connstr.options.tls_verify, name, value, connstr.warnings);
-    } else if (name == "disable_mozilla_ca_certificates") {
-      parse_option(connstr.options.disable_mozilla_ca_certificates, name, value, connstr.warnings);
     } else if (name == "tls_disable_deprecated_protocols") {
       parse_option(connstr.options.tls_disable_deprecated_protocols, name, value, connstr.warnings);
     } else if (name == "tls_disable_v1_2") {
       parse_option(connstr.options.tls_disable_v1_2, name, value, connstr.warnings);
-    } else if (name == "user_agent_extra") {
-      /**
-       * string, that will be appended to identification fields of the server protocols (key in HELO
-       * packet for MCBP, "user-agent" header for HTTP)
-       */
-      parse_option(connstr.options.user_agent_extra, name, value, connstr.warnings);
-    } else if (name == "dump_configuration") {
-      /**
-       * Whether to dump every new configuration on TRACE level
-       */
-      parse_option(connstr.options.dump_configuration, name, value, connstr.warnings);
     } else if (name == "server_group") {
       parse_option(connstr.options.server_group, name, value, connstr.warnings);
+#endif
     } else {
       connstr.warnings.push_back(
         fmt::format(R"(unknown parameter "{}" in connection string (value "{}"))", name, value));
diff --git a/test/test_unit_connection_string.cxx b/test/test_unit_connection_string.cxx
index 83c4a872b..aaf48ab72 100644
--- a/test/test_unit_connection_string.cxx
+++ b/test/test_unit_connection_string.cxx
@@ -19,18 +19,20 @@
 
 #include "core/utils/connection_string.hxx"
 
+#include <couchbase/build_config.hxx>
+
 TEST_CASE("unit: connection string", "[unit]")
 {
   SECTION("full example")
   {
     auto spec = couchbase::core::utils::parse_connection_string(
-      "couchbase://localhost:8091=http;127.0.0.1=mcd/default?enable_tracing=false");
+      "couchbase://localhost:8091=http;127.0.0.1=mcd/default?dump_configuration=true");
     CHECK(spec.scheme == "couchbase");
     CHECK(spec.default_port == 11210);
     CHECK(spec.default_mode == couchbase::core::utils::connection_string::bootstrap_mode::gcccp);
     CHECK(spec.tls == false);
     CHECK(spec.params == std::map<std::string, std::string>{
-                           { "enable_tracing", "false" },
+                           { "dump_configuration", "true" },
                          });
     CHECK(spec.bootstrap_nodes ==
           std::vector<couchbase::core::utils::connection_string::node>{
@@ -43,7 +45,7 @@ TEST_CASE("unit: connection string", "[unit]")
               couchbase::core::utils::connection_string::address_type::ipv4,
               couchbase::core::utils::connection_string::bootstrap_mode::gcccp },
           });
-    CHECK(spec.options.enable_tracing == false);
+    CHECK(spec.options.dump_configuration == true);
     CHECK(spec.default_bucket_name == "default");
   }
 
@@ -406,6 +408,87 @@ TEST_CASE("unit: connection string", "[unit]")
     }
   }
 
+#ifdef COUCHBASE_CXX_CLIENT_COLUMNAR
+  SECTION("options")
+  {
+    CHECK(couchbase::core::utils::parse_connection_string("couchbase://127.0.0.1")
+            .options.trust_certificate.empty());
+    CHECK(couchbase::core::utils::parse_connection_string(
+            "couchbase://127.0.0.1?security.trust_only_pem_file=/etc/tls/example.cert")
+            .options.trust_certificate == "/etc/tls/example.cert");
+    auto spec = couchbase::core::utils::parse_connection_string(
+      "couchbase://127.0.0.1?timeout.connect_timeout=42ms&timeout.query_timeout=123ms");
+    CHECK(spec.options.bootstrap_timeout == std::chrono::milliseconds(42));
+    CHECK(spec.options.query_timeout == std::chrono::milliseconds(123));
+
+    SECTION("parameters")
+    {
+      CHECK(spec.params == std::map<std::string, std::string>{
+                             { "timeout.connect_timeout", "42ms" },
+                             { "timeout.query_timeout", "123ms" },
+                           });
+
+      spec = couchbase::core::utils::parse_connection_string(
+        "couchbase://127.0.0.1?timeout.connect_timeout=42ms&foo=bar");
+      CHECK(spec.params == std::map<std::string, std::string>{
+                             { "timeout.connect_timeout", "42ms" },
+                             { "foo", "bar" },
+                           });
+      CHECK(spec.options.bootstrap_timeout == std::chrono::milliseconds(42));
+
+      spec = couchbase::core::utils::parse_connection_string(
+        "couchbase://127.0.0.1?timeout.resolve_timeout=4s2ms");
+      CHECK(spec.params == std::map<std::string, std::string>{
+                             { "timeout.resolve_timeout", "4s2ms" },
+                           });
+      CHECK(spec.options.resolve_timeout == std::chrono::milliseconds(4002));
+
+      spec = couchbase::core::utils::parse_connection_string(
+        "couchbase://"
+        "127.0.0.1?user_agent_extra=couchnode%2F4.1.1%20(node%2F12.11."
+        "1%3B%20v8%2F7.7.299.11-node.12%3B%20ssl%2F1.1.1c)");
+      CHECK(spec.options.user_agent_extra ==
+            "couchnode/4.1.1 (node/12.11.1; v8/7.7.299.11-node.12; ssl/1.1.1c)");
+    }
+  }
+
+  SECTION("parsing warnings")
+  {
+    auto spec = couchbase::core::utils::parse_connection_string(
+      "couchbase://127.0.0.1?timeout.connect_timeout=42ms&foo=bar");
+    CHECK(spec.warnings == std::vector<std::string>{
+                             R"(unknown parameter "foo" in connection string (value "bar"))",
+                           });
+
+    spec = couchbase::core::utils::parse_connection_string(
+      "couchbase://127.0.0.1?enable_dns_srv=maybe&ip_protocol=yes");
+    CHECK(
+      spec.warnings ==
+      std::vector<std::string>{
+        R"(unable to parse "enable_dns_srv" parameter in connection string (value "maybe" cannot be interpreted as a boolean))",
+        R"(unable to parse "ip_protocol" parameter in connection string (value "yes" is not a valid IP protocol preference))",
+      });
+
+    spec = couchbase::core::utils::parse_connection_string(
+      "couchbase://localhost:8091=http;127.0.0.1=mcd/default?enable_dns_srv=true");
+    CHECK(
+      spec.warnings ==
+      std::vector<std::string>{
+        R"(parameter "enable_dns_srv" requires single entry in bootstrap nodes list of the connection string, ignoring (value "true"))",
+      });
+
+    spec = couchbase::core::utils::parse_connection_string(
+      "couchbase://"
+      "localhost?timeout.query_timeout=10000ms&timeout.dispatch_timeout=true&timeout.resolve_"
+      "timeout=11000ms");
+    std::string warning_prefix =
+      R"(unable to parse "timeout.dispatch_timeout" parameter in connection string (value: "true"): invalid duration: true)";
+    CHECK(spec.warnings.at(0).substr(0, warning_prefix.size()) == warning_prefix);
+    CHECK(spec.options.query_timeout == std::chrono::milliseconds(10000));
+    CHECK(spec.options.resolve_timeout == std::chrono::milliseconds(11000));
+  }
+
+#else
   SECTION("options")
   {
     CHECK(couchbase::core::utils::parse_connection_string("couchbase://127.0.0.1")
@@ -449,25 +532,6 @@ TEST_CASE("unit: connection string", "[unit]")
     }
   }
 
-  SECTION("parsing errors")
-  {
-    CHECK(couchbase::core::utils::parse_connection_string("").error ==
-          "failed to parse connection string: empty input");
-    CHECK(
-      couchbase::core::utils::parse_connection_string("couchbase://127.0.0.1/bucket/foo").error ==
-      R"(failed to parse connection string (column: 29, trailer: "/foo"))");
-    CHECK(couchbase::core::utils::parse_connection_string("couchbase://[:13.15.49.232]").error ==
-          R"(failed to parse connection string (column: 14, trailer: ":13.15.49.232]"))");
-    CHECK(couchbase::core::utils::parse_connection_string(
-            "couchbase://[2001:1:db8:85a3:8d3:1319:8a2e:370:7348]")
-            .error == R"(failed to parse connection string (column: 47, trailer: ":7348]"))");
-    CHECK(
-      couchbase::core::utils::parse_connection_string(
-        "couchbase://2001:db8:85a3:8d3:1319:8a2e:370:7348")
-        .error.value() ==
-      R"(failed to parse connection string (column: 18, trailer: "db8:85a3:8d3:1319:8a2e:370:7348"))");
-  }
-
   SECTION("parsing warnings")
   {
     auto spec = couchbase::core::utils::parse_connection_string(
@@ -501,4 +565,24 @@ TEST_CASE("unit: connection string", "[unit]")
     CHECK(spec.options.query_timeout == std::chrono::milliseconds(10000));
     CHECK(spec.options.management_timeout == std::chrono::milliseconds(11000));
   }
+#endif
+
+  SECTION("parsing errors")
+  {
+    CHECK(couchbase::core::utils::parse_connection_string("").error ==
+          "failed to parse connection string: empty input");
+    CHECK(
+      couchbase::core::utils::parse_connection_string("couchbase://127.0.0.1/bucket/foo").error ==
+      R"(failed to parse connection string (column: 29, trailer: "/foo"))");
+    CHECK(couchbase::core::utils::parse_connection_string("couchbase://[:13.15.49.232]").error ==
+          R"(failed to parse connection string (column: 14, trailer: ":13.15.49.232]"))");
+    CHECK(couchbase::core::utils::parse_connection_string(
+            "couchbase://[2001:1:db8:85a3:8d3:1319:8a2e:370:7348]")
+            .error == R"(failed to parse connection string (column: 47, trailer: ":7348]"))");
+    CHECK(
+      couchbase::core::utils::parse_connection_string(
+        "couchbase://2001:db8:85a3:8d3:1319:8a2e:370:7348")
+        .error.value() ==
+      R"(failed to parse connection string (column: 18, trailer: "db8:85a3:8d3:1319:8a2e:370:7348"))");
+  }
 }