From 192b3b60cb10a48560d79fa084373a5cfd56a07d Mon Sep 17 00:00:00 2001 From: JaySon Date: Tue, 16 Jul 2024 17:59:59 +0800 Subject: [PATCH 1/4] This is an automated cherry-pick of #9234 Signed-off-by: ti-chi-bot --- dbms/src/Common/TiFlashSecurity.h | 55 ++++-- .../Common/tests/gtest_tiflash_security.cpp | 165 ++++++++++++++++-- dbms/src/Server/Server.cpp | 4 +- 3 files changed, 194 insertions(+), 30 deletions(-) diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h index a02afc9d04e..5c2c0f622b2 100644 --- a/dbms/src/Common/TiFlashSecurity.h +++ b/dbms/src/Common/TiFlashSecurity.h @@ -39,8 +39,6 @@ extern const int INVALID_CONFIG_PARAMETER; class TiFlashSecurityConfig : public ConfigObject { public: - TiFlashSecurityConfig() = default; - explicit TiFlashSecurityConfig(const LoggerPtr & log_) : log(log_) {} @@ -54,12 +52,6 @@ class TiFlashSecurityConfig : public ConfigObject } } - void setLog(const LoggerPtr & log_) - { - std::unique_lock lock(mu); - log = log_; - } - bool hasTlsConfig() { std::unique_lock lock(mu); @@ -94,6 +86,7 @@ class TiFlashSecurityConfig : public ConfigObject bool update(Poco::Util::AbstractConfiguration & config) { std::unique_lock lock(mu); +<<<<<<< HEAD if (config.has("security")) { if (inited && !has_security) @@ -119,6 +112,9 @@ class TiFlashSecurityConfig : public ConfigObject return cert_file_updated; } else +======= + if (!config.has("security")) +>>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) { if (inited && has_security) { @@ -128,8 +124,31 @@ class TiFlashSecurityConfig : public ConfigObject { LOG_INFO(log, "security config is not set"); } + return false; } - return false; + + assert(config.has("security")); + if (inited && !has_security) + { + LOG_WARNING(log, "Can't add security config online"); + return false; + } + has_security = true; + + bool cert_file_updated = updateCertPath(config); + + if (config.has("security.cert_allowed_cn") && has_tls_config) + { + String verify_cns = config.getString("security.cert_allowed_cn"); + allowed_common_names = parseAllowedCN(verify_cns); + } + + // Mostly options name are combined with "_", keep this style + if (config.has("security.redact_info_log")) + { + redact_info_log = parseRedactLog(config.getString("security.redact_info_log")); + } + return cert_file_updated; } void parseAllowedCN(String verify_cns) @@ -236,18 +255,18 @@ class TiFlashSecurityConfig : public ConfigObject bool updated = false; if (config.has("security.ca_path")) { - new_ca_path = config.getString("security.ca_path"); - miss_ca_path = false; + new_ca_path = Poco::trim(config.getString("security.ca_path")); + miss_ca_path = new_ca_path.empty(); } if (config.has("security.cert_path")) { - new_cert_path = config.getString("security.cert_path"); - miss_cert_path = false; + new_cert_path = Poco::trim(config.getString("security.cert_path")); + miss_cert_path = new_cert_path.empty(); } if (config.has("security.key_path")) { - new_key_path = config.getString("security.key_path"); - miss_key_path = false; + new_key_path = Poco::trim(config.getString("security.key_path")); + miss_key_path = new_key_path.empty(); } if (miss_ca_path && miss_cert_path && miss_key_path) { @@ -322,8 +341,14 @@ class TiFlashSecurityConfig : public ConfigObject String key_path; FilesChangesTracker cert_files; +<<<<<<< HEAD bool redact_info_log = false; +======= +>>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) std::set allowed_common_names; + + RedactMode redact_info_log = RedactMode::Disable; + bool has_tls_config = false; bool has_security = false; bool inited = false; diff --git a/dbms/src/Common/tests/gtest_tiflash_security.cpp b/dbms/src/Common/tests/gtest_tiflash_security.cpp index 7319744ffe1..b76b04c7ef7 100644 --- a/dbms/src/Common/tests/gtest_tiflash_security.cpp +++ b/dbms/src/Common/tests/gtest_tiflash_security.cpp @@ -12,10 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include #include +#include #include #include @@ -30,10 +32,37 @@ class TiFlashSecurityTest : public ext::Singleton TEST(TiFlashSecurityTest, Config) { +<<<<<<< HEAD TiFlashSecurityConfig tiflash_config; +======= + { + auto cns = TiFlashSecurityConfig::parseAllowedCN(String("[abc,efg]")); + ASSERT_EQ(cns.count("abc"), 1); + ASSERT_EQ(cns.count("efg"), 1); + } + + { + auto cns = TiFlashSecurityConfig::parseAllowedCN(String(R"(["abc","efg"])")); + ASSERT_EQ(cns.count("abc"), 1); + ASSERT_EQ(cns.count("efg"), 1); + } + + { + auto cns = TiFlashSecurityConfig::parseAllowedCN(String("[ abc , efg ]")); + ASSERT_EQ(cns.count("abc"), 1); + ASSERT_EQ(cns.count("efg"), 1); + } + + { + auto cns = TiFlashSecurityConfig::parseAllowedCN(String(R"([ "abc", "efg" ])")); + ASSERT_EQ(cns.count("abc"), 1); + ASSERT_EQ(cns.count("efg"), 1); + } + +>>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) const auto log = Logger::get(); - tiflash_config.setLog(log); +<<<<<<< HEAD tiflash_config.parseAllowedCN(String("[abc,efg]")); ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1); ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1); @@ -58,27 +87,139 @@ TEST(TiFlashSecurityTest, Config) String test = R"( +======= + { + auto new_config = loadConfigFromString(R"( +>>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) [security] ca_path="security/ca.pem" cert_path="security/cert.pem" key_path="security/key.pem" cert_allowed_cn="tidb" - )"; - auto new_config = loadConfigFromString(test); - tiflash_config.update(*new_config); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("tidb"), 1); + )"); + TiFlashSecurityConfig tiflash_config(log); + tiflash_config.init(*new_config); + ASSERT_TRUE(tiflash_config.hasTlsConfig()); + ASSERT_EQ(tiflash_config.allowedCommonNames().count("tidb"), 1); + } - test = - R"( + { + auto new_config = loadConfigFromString(R"( [security] cert_allowed_cn="tidb" - )"; - new_config = loadConfigFromString(test); - auto new_tiflash_config = TiFlashSecurityConfig(log); - new_tiflash_config.init(*new_config); - ASSERT_EQ((int)new_tiflash_config.allowedCommonNames().count("tidb"), 0); + )"); + auto new_tiflash_config = TiFlashSecurityConfig(log); + new_tiflash_config.init(*new_config); + ASSERT_FALSE(new_tiflash_config.hasTlsConfig()); + // allowed common names is ignored when tls is not enabled + ASSERT_EQ(new_tiflash_config.allowedCommonNames().count("tidb"), 0); + } +} + +<<<<<<< HEAD +======= +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)); + TiFlashSecurityConfig tiflash_config(log); + auto new_config = loadConfigFromString(c); + tiflash_config.init(*new_config); + ASSERT_FALSE(tiflash_config.hasTlsConfig()); + } +} +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)); + TiFlashSecurityConfig tiflash_config(log); + auto new_config = loadConfigFromString(c); + try + { + tiflash_config.init(*new_config); + ASSERT_FALSE(true) << "should raise exception"; + } + catch (Exception & e) + { + // has_tls remains false when an exception raise + ASSERT_FALSE(tiflash_config.hasTlsConfig()); + // the error code must be INVALID_CONFIG_PARAMETER + ASSERT_EQ(e.code(), ErrorCodes::INVALID_CONFIG_PARAMETER); + } + } +} +CATCH + +TEST(TiFlashSecurityTest, RedactLogConfig) +{ + for (const auto & [input, expect] : std::vector>{ + {"marker", RedactMode::Marker}, + {"Marker", RedactMode::Marker}, + {"MARKER", RedactMode::Marker}, + {"true", RedactMode::Enable}, + {"True", RedactMode::Enable}, + {"TRUE", RedactMode::Enable}, + {"yes", RedactMode::Enable}, + {"on", RedactMode::Enable}, + {"1", RedactMode::Enable}, + {"2", RedactMode::Enable}, + {"false", RedactMode::Disable}, + {"False", RedactMode::Disable}, + {"FALSE", RedactMode::Disable}, + {"no", RedactMode::Disable}, + {"off", RedactMode::Disable}, + {"0", RedactMode::Disable}, + }) + { + EXPECT_EQ(TiFlashSecurityConfig::parseRedactLog(input), expect); + } } +>>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) TEST(TiFlashSecurityTest, Update) { String test = diff --git a/dbms/src/Server/Server.cpp b/dbms/src/Server/Server.cpp index 3da05ad9ba3..0799f00984d 100644 --- a/dbms/src/Server/Server.cpp +++ b/dbms/src/Server/Server.cpp @@ -1393,9 +1393,7 @@ int Server::main(const std::vector & /*args*/) } { // update TiFlashSecurity and related config in client for ssl certificate reload. - bool updated - = global_context->getSecurityConfig()->update(*config); // Whether the cert path or file is updated. - if (updated) + if (bool updated = global_context->getSecurityConfig()->update(*config); updated) { auto raft_config = TiFlashRaftConfig::parseSettings(*config, log); auto cluster_config From 680cde3b19b6d28770a56f4dd0a4b6c1a0534177 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 17 Jul 2024 16:26:03 +0800 Subject: [PATCH 2/4] Resolve conflict --- dbms/src/Common/TiFlashSecurity.h | 39 +++--------- .../Common/tests/gtest_tiflash_security.cpp | 59 ------------------- 2 files changed, 7 insertions(+), 91 deletions(-) diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h index 5c2c0f622b2..69614375b66 100644 --- a/dbms/src/Common/TiFlashSecurity.h +++ b/dbms/src/Common/TiFlashSecurity.h @@ -86,35 +86,7 @@ class TiFlashSecurityConfig : public ConfigObject bool update(Poco::Util::AbstractConfiguration & config) { std::unique_lock lock(mu); -<<<<<<< HEAD - if (config.has("security")) - { - if (inited && !has_security) - { - LOG_WARNING(log, "Can't add security config online"); - return false; - } - has_security = true; - - bool cert_file_updated = updateCertPath(config); - - if (config.has("security.cert_allowed_cn") && has_tls_config) - { - String verify_cns = config.getString("security.cert_allowed_cn"); - parseAllowedCN(verify_cns); - } - - // Mostly options name are combined with "_", keep this style - if (config.has("security.redact_info_log")) - { - redact_info_log = config.getBool("security.redact_info_log"); - } - return cert_file_updated; - } - else -======= if (!config.has("security")) ->>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) { if (inited && has_security) { @@ -140,23 +112,25 @@ class TiFlashSecurityConfig : public ConfigObject if (config.has("security.cert_allowed_cn") && has_tls_config) { String verify_cns = config.getString("security.cert_allowed_cn"); - allowed_common_names = parseAllowedCN(verify_cns); + parseAllowedCN(verify_cns); } // Mostly options name are combined with "_", keep this style if (config.has("security.redact_info_log")) { - redact_info_log = parseRedactLog(config.getString("security.redact_info_log")); + redact_info_log = config.getBool("security.redact_info_log"); } return cert_file_updated; } - void parseAllowedCN(String verify_cns) + static std::set parseAllowedCN(String verify_cns) { if (verify_cns.size() > 2 && verify_cns[0] == '[' && verify_cns[verify_cns.size() - 1] == ']') { verify_cns = verify_cns.substr(1, verify_cns.size() - 2); } + + std::set common_names; Poco::StringTokenizer string_tokens(verify_cns, ","); for (const auto & string_token : string_tokens) { @@ -165,8 +139,9 @@ class TiFlashSecurityConfig : public ConfigObject { cn = cn.substr(1, cn.size() - 2); } - allowed_common_names.insert(std::move(cn)); + common_names.insert(std::move(cn)); } + return common_names; } bool checkGrpcContext(const grpc::ServerContext * grpc_context) const diff --git a/dbms/src/Common/tests/gtest_tiflash_security.cpp b/dbms/src/Common/tests/gtest_tiflash_security.cpp index b76b04c7ef7..194d78b1d70 100644 --- a/dbms/src/Common/tests/gtest_tiflash_security.cpp +++ b/dbms/src/Common/tests/gtest_tiflash_security.cpp @@ -32,9 +32,6 @@ class TiFlashSecurityTest : public ext::Singleton TEST(TiFlashSecurityTest, Config) { -<<<<<<< HEAD - TiFlashSecurityConfig tiflash_config; -======= { auto cns = TiFlashSecurityConfig::parseAllowedCN(String("[abc,efg]")); ASSERT_EQ(cns.count("abc"), 1); @@ -59,38 +56,10 @@ TEST(TiFlashSecurityTest, Config) ASSERT_EQ(cns.count("efg"), 1); } ->>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) const auto log = Logger::get(); -<<<<<<< HEAD - tiflash_config.parseAllowedCN(String("[abc,efg]")); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1); - - tiflash_config.allowedCommonNames().clear(); - - tiflash_config.parseAllowedCN(String(R"(["abc","efg"])")); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1); - - tiflash_config.allowedCommonNames().clear(); - - tiflash_config.parseAllowedCN(String("[ abc , efg ]")); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1); - - tiflash_config.allowedCommonNames().clear(); - - tiflash_config.parseAllowedCN(String(R"([ "abc", "efg" ])")); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1); - ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1); - - String test = - R"( -======= { auto new_config = loadConfigFromString(R"( ->>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) [security] ca_path="security/ca.pem" cert_path="security/cert.pem" @@ -116,8 +85,6 @@ cert_allowed_cn="tidb" } } -<<<<<<< HEAD -======= TEST(TiFlashSecurityTest, EmptyConfig) try { @@ -194,32 +161,6 @@ key_path="security/key.pem")", } CATCH -TEST(TiFlashSecurityTest, RedactLogConfig) -{ - for (const auto & [input, expect] : std::vector>{ - {"marker", RedactMode::Marker}, - {"Marker", RedactMode::Marker}, - {"MARKER", RedactMode::Marker}, - {"true", RedactMode::Enable}, - {"True", RedactMode::Enable}, - {"TRUE", RedactMode::Enable}, - {"yes", RedactMode::Enable}, - {"on", RedactMode::Enable}, - {"1", RedactMode::Enable}, - {"2", RedactMode::Enable}, - {"false", RedactMode::Disable}, - {"False", RedactMode::Disable}, - {"FALSE", RedactMode::Disable}, - {"no", RedactMode::Disable}, - {"off", RedactMode::Disable}, - {"0", RedactMode::Disable}, - }) - { - EXPECT_EQ(TiFlashSecurityConfig::parseRedactLog(input), expect); - } -} - ->>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) TEST(TiFlashSecurityTest, Update) { String test = From ae6a5e1607b5cd973baabd80c3736a6ceec9fb0a Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 17 Jul 2024 16:26:21 +0800 Subject: [PATCH 3/4] Resolve conflict --- dbms/src/Common/TiFlashSecurity.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h index 69614375b66..9d46dbde6d3 100644 --- a/dbms/src/Common/TiFlashSecurity.h +++ b/dbms/src/Common/TiFlashSecurity.h @@ -316,13 +316,9 @@ class TiFlashSecurityConfig : public ConfigObject String key_path; FilesChangesTracker cert_files; -<<<<<<< HEAD - bool redact_info_log = false; -======= ->>>>>>> 951e010ab8 (security: Allow empty security config for disabling tls (#9234)) std::set allowed_common_names; - RedactMode redact_info_log = RedactMode::Disable; + bool redact_info_log = false; bool has_tls_config = false; bool has_security = false; From 81f6f8d4408f07fce505bc913f7beb269d4039ab Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 18 Jul 2024 09:22:41 +0800 Subject: [PATCH 4/4] fix --- dbms/src/Common/TiFlashSecurity.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h index 9d46dbde6d3..67daaa40590 100644 --- a/dbms/src/Common/TiFlashSecurity.h +++ b/dbms/src/Common/TiFlashSecurity.h @@ -112,7 +112,7 @@ class TiFlashSecurityConfig : public ConfigObject if (config.has("security.cert_allowed_cn") && has_tls_config) { String verify_cns = config.getString("security.cert_allowed_cn"); - parseAllowedCN(verify_cns); + allowed_common_names = parseAllowedCN(verify_cns); } // Mostly options name are combined with "_", keep this style