security: Allow empty security config for disabling tls (#9234) (#9347)

close #9235 security: allow empty `security.ca_path`/`security.cert_path`/`security.key_path` Signed-off-by: ti-chi-bot <[email protected]> Signed-off-by: JaySon-Huang <[email protected]> Co-authored-by: JaySon <[email protected]> Co-authored-by: JaySon-Huang <[email protected]>
pingcap · Aug 26, 2024 · 7048a4f · 7048a4f
1 parent 683aca0
commit 7048a4f
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 14 deletions.
diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h
@@ -56,18 +56,18 @@ struct TiFlashSecurityConfig
             bool miss_key_path = true;
             if (config.has("security.ca_path"))
             {
-                ca_path = config.getString("security.ca_path");
-                miss_ca_path = false;
+                ca_path = Poco::trim(config.getString("security.ca_path"));
+                miss_ca_path = ca_path.empty();
             }
             if (config.has("security.cert_path"))
             {
-                cert_path = config.getString("security.cert_path");
-                miss_cert_path = false;
+                cert_path = Poco::trim(config.getString("security.cert_path"));
+                miss_cert_path = cert_path.empty();
             }
             if (config.has("security.key_path"))
             {
-                key_path = config.getString("security.key_path");
-                miss_key_path = false;
+                key_path = Poco::trim(config.getString("security.key_path"));
+                miss_key_path = key_path.empty();
             }
             if (miss_ca_path && miss_cert_path && miss_key_path)
             {

diff --git a/dbms/src/Common/tests/gtest_tiflash_security.cpp b/dbms/src/Common/tests/gtest_tiflash_security.cpp
@@ -13,17 +13,14 @@
 // limitations under the License.
 
 #include <Common/TiFlashSecurity.h>
+#include <TestUtils/ConfigTestUtils.h>
+#include <TestUtils/TiFlashTestBasic.h>
 #include <gtest/gtest.h>
 
 #include <ext/singleton.h>
 
-namespace DB
+namespace DB::tests
 {
-namespace tests
-{
-class TestTiFlashSecurity : public ext::Singleton<TestTiFlashSecurity>
-{
-};
 
 TEST(TestTiFlashSecurity, Config)
 {
@@ -50,5 +47,77 @@ TEST(TestTiFlashSecurity, Config)
     ASSERT_EQ((int)config.allowed_common_names.count("abc"), 1);
     ASSERT_EQ((int)config.allowed_common_names.count("efg"), 1);
 }
-} // namespace tests
-} // namespace DB
+
+TEST(TiFlashSecurityTest, EmptyConfig)
+try
+{
+    const auto log = Logger::get();
+
+    for (const auto & c : Strings{
+             // empty strings
+             R"([security]
+ca_path=""
+cert_path=""
+key_path="")",
+             // non-empty strings with space only
+             R"([security]
+ca_path="  "
+cert_path=""
+key_path="")",
+         })
+    {
+        SCOPED_TRACE(fmt::format("case: {}", c));
+        auto new_config = loadConfigFromString(c);
+        TiFlashSecurityConfig tiflash_config(*new_config, log);
+        ASSERT_FALSE(tiflash_config.has_tls_config);
+    }
+}
+CATCH
+
+TEST(TiFlashSecurityTest, InvalidConfig)
+try
+{
+    const auto log = Logger::get();
+
+    for (const auto & c : Strings{
+             // only a part of ssl path is set
+             R"([security]
+ca_path="security/ca.pem"
+cert_path=""
+key_path="")",
+             R"([security]
+ca_path=""
+cert_path="security/cert.pem"
+key_path="")",
+             R"([security]
+ca_path=""
+cert_path=""
+key_path="security/key.pem")",
+             R"([security]
+ca_path=""
+cert_path="security/cert.pem"
+key_path="security/key.pem")",
+             // comment out
+             R"([security]
+ca_path="security/ca.pem"
+#cert_path="security/cert.pem"
+key_path="security/key.pem")",
+         })
+    {
+        SCOPED_TRACE(fmt::format("case: {}", c));
+        auto new_config = loadConfigFromString(c);
+        try
+        {
+            TiFlashSecurityConfig tiflash_config(*new_config, log);
+            ASSERT_FALSE(true) << "should raise exception";
+        }
+        catch (Exception & e)
+        {
+            // the error code must be INVALID_CONFIG_PARAMETER
+            ASSERT_EQ(e.code(), ErrorCodes::INVALID_CONFIG_PARAMETER);
+        }
+    }
+}
+CATCH
+
+} // namespace DB::tests