Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Jul 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
192b3b6
This is an automated cherry-pick of #9234
JaySon-Huang Jul 16, 2024
680cde3
Resolve conflict
JaySon-Huang Jul 17, 2024
ae6a5e1
Resolve conflict
JaySon-Huang Jul 17, 2024
81f6f8d
fix
JaySon-Huang Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 39 additions & 43 deletions dbms/src/Common/TiFlashSecurity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ extern const int INVALID_CONFIG_PARAMETER;
class TiFlashSecurityConfig : public ConfigObject
{
public:
TiFlashSecurityConfig() = default;

explicit TiFlashSecurityConfig(const LoggerPtr & log_)
: log(log_)
{}
Expand All @@ -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);
Expand Down Expand Up @@ -94,31 +86,7 @@ class TiFlashSecurityConfig : public ConfigObject
bool update(Poco::Util::AbstractConfiguration & config)
{
std::unique_lock lock(mu);
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"))
{
if (inited && has_security)
{
Expand All @@ -128,16 +96,41 @@ 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");
parseAllowedCN(verify_cns);
JaySon-Huang marked this conversation as resolved.
Show resolved Hide resolved
}

// 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;
}

void parseAllowedCN(String verify_cns)
static std::set<String> 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<String> common_names;
Poco::StringTokenizer string_tokens(verify_cns, ",");
for (const auto & string_token : string_tokens)
{
Expand All @@ -146,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
Expand Down Expand Up @@ -236,18 +230,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)
{
Expand Down Expand Up @@ -322,8 +316,10 @@ class TiFlashSecurityConfig : public ConfigObject
String key_path;

FilesChangesTracker cert_files;
bool redact_info_log = false;
std::set<String> allowed_common_names;

bool redact_info_log = false;

bool has_tls_config = false;
bool has_security = false;
bool inited = false;
Expand Down
150 changes: 116 additions & 34 deletions dbms/src/Common/tests/gtest_tiflash_security.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <Common/Exception.h>
#include <Common/TiFlashSecurity.h>
#include <Poco/File.h>
#include <Poco/FileStream.h>
#include <TestUtils/ConfigTestUtils.h>
#include <TestUtils/TiFlashTestBasic.h>
#include <gtest/gtest.h>

#include <ext/singleton.h>
Expand All @@ -30,54 +32,134 @@ class TiFlashSecurityTest : public ext::Singleton<TiFlashSecurityTest>

TEST(TiFlashSecurityTest, Config)
{
TiFlashSecurityConfig tiflash_config;
const auto log = Logger::get();
tiflash_config.setLog(log);

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);
{
auto cns = TiFlashSecurityConfig::parseAllowedCN(String("[abc,efg]"));
ASSERT_EQ(cns.count("abc"), 1);
ASSERT_EQ(cns.count("efg"), 1);
}

tiflash_config.allowedCommonNames().clear();
{
auto cns = TiFlashSecurityConfig::parseAllowedCN(String(R"(["abc","efg"])"));
ASSERT_EQ(cns.count("abc"), 1);
ASSERT_EQ(cns.count("efg"), 1);
}

tiflash_config.parseAllowedCN(String("[ abc , efg ]"));
ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("abc"), 1);
ASSERT_EQ((int)tiflash_config.allowedCommonNames().count("efg"), 1);
{
auto cns = TiFlashSecurityConfig::parseAllowedCN(String("[ abc , efg ]"));
ASSERT_EQ(cns.count("abc"), 1);
ASSERT_EQ(cns.count("efg"), 1);
}

tiflash_config.allowedCommonNames().clear();
{
auto cns = TiFlashSecurityConfig::parseAllowedCN(String(R"([ "abc", "efg" ])"));
ASSERT_EQ(cns.count("abc"), 1);
ASSERT_EQ(cns.count("efg"), 1);
}

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);
const auto log = Logger::get();

String test =
R"(
{
auto new_config = loadConfigFromString(R"(
[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);
}
}

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, Update)
{
Expand Down
4 changes: 1 addition & 3 deletions dbms/src/Server/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1393,9 +1393,7 @@ int Server::main(const std::vector<std::string> & /*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
Expand Down