Skip to content

Commit

Permalink
security: Allow empty security config for disabling tls (#9234)
Browse files Browse the repository at this point in the history 
close #9235

security: allow empty `security.ca_path`/`security.cert_path`/`security.key_path`
  • Loading branch information
@JaySon-Huang
JaySon-Huang committed Jul 16, 2024
1 parent 1ee7da7 commit 951e010
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 59 deletions.
75 changes: 34 additions & 41 deletions dbms/src/Common/TiFlashSecurity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ extern const int INVALID_CONFIG_PARAMETER;
class TiFlashSecurityConfig : public ConfigObject
{
public:
TiFlashSecurityConfig() = default;

explicit TiFlashSecurityConfig(const LoggerPtr & log_)
: log(log_)
{}
Expand All @@ -56,12 +54,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 @@ -96,31 +88,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");
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;
}
else
if (!config.has("security"))
{
if (inited && has_security)
{
Expand All @@ -130,8 +98,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;
}

static RedactMode parseRedactLog(const String & config_str)
Expand Down Expand Up @@ -272,18 +263,18 @@ class TiFlashSecurityConfig : public ConfigObject
String new_key_path;
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 @@ -358,8 +349,10 @@ class TiFlashSecurityConfig : public ConfigObject
String key_path;

FilesChangesTracker cert_files;
RedactMode redact_info_log = RedactMode::Disable;
std::set<String> allowed_common_names;

RedactMode redact_info_log = RedactMode::Disable;

bool has_tls_config = false;
bool has_security = false;
bool inited = false;
Expand Down
110 changes: 95 additions & 15 deletions dbms/src/Common/tests/gtest_tiflash_security.cpp
View file
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 Down Expand Up @@ -49,32 +51,110 @@ TEST(TiFlashSecurityTest, Config)
ASSERT_EQ(cns.count("efg"), 1);
}

TiFlashSecurityConfig tiflash_config;
const auto log = Logger::get();
tiflash_config.setLog(log);

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, RedactLogConfig)
{
Expand Down
4 changes: 1 addition & 3 deletions dbms/src/Server/Server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1404,9 +1404,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

0 comments on commit 951e010

Please sign in to comment.