Skip to content

Commit

Permalink
security: Do not let grpc reload ssh cert if not changed (pingcap#9068)…
Browse files Browse the repository at this point in the history
  • Loading branch information
ti-chi-bot authored and JaySon-Huang committed Jun 27, 2024
1 parent 96815a8 commit 416653e
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 5 deletions.
1 change: 1 addition & 0 deletions .github/licenserc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,6 @@ header:
- '**/OWNERS'
- 'OWNERS_ALIASES'
- '**/*.sql'
- 'dbms/src/Common/tests/tls/'

comment: on-failure
6 changes: 4 additions & 2 deletions dbms/src/Common/TiFlashSecurity.h
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,13 @@ class TiFlashSecurityConfig : public ConfigObject
return allowed_common_names.count(cert.commonName()) > 0;
}

grpc::SslCredentialsOptions readAndCacheSslCredentialOptions()
std::optional<grpc::SslCredentialsOptions> readAndCacheSslCredentialOptions()
{
std::unique_lock lock(mu);
// if ssl_cerd_options_cached = true, it means the same options has already been returned to grpc-core
// don't need to return it again if ssl cert is not actually changed
if (ssl_cerd_options_cached)
return options;
return {};
options.pem_root_certs = readFile(ca_path);
options.pem_cert_chain = readFile(cert_path);
options.pem_private_key = readFile(key_path);
Expand Down
83 changes: 83 additions & 0 deletions dbms/src/Common/tests/gtest_tiflash_security.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.

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

Expand Down Expand Up @@ -188,5 +190,86 @@ redact_info_log=false
ASSERT_TRUE(tiflash_config_2.allowedCommonNames().empty());
ASSERT_FALSE(tiflash_config_2.hasTlsConfig());
}

String createConfigString(String & ca_path, String & cert_path, String & key_path)
{
String ret =
R"(
[security]
ca_path=")";
ret += ca_path;
ret += R"("
cert_path=")";
ret += cert_path;
ret += R"("
key_path=")";
ret += key_path;
ret += R"("
)";
return ret;
}

TEST(TiFlashSecurityTest, readAndCacheSslCredentialOptions)
{
const auto & test_info = testing::UnitTest::GetInstance()->current_test_info();
assert(test_info);
String file_name = test_info->file();
auto pos = file_name.find_last_of('/');
auto file_path = file_name.substr(0, pos);
// these certs are copied from https://github.com/grpc/grpc/tree/v1.64.0/src/python/grpcio_tests/tests/unit/credentials
auto ca_path = file_path + "/tls/ca.crt";
auto cert_path = file_path + "/tls/cert.crt";
auto key_path = file_path + "/tls/key.pem";
auto test = createConfigString(ca_path, cert_path, key_path);
auto config = loadConfigFromString(test);
const auto log = Logger::get();
TiFlashSecurityConfig tiflash_config(log);
tiflash_config.init(*config);
// first read will return a valid options
auto options = tiflash_config.readAndCacheSslCredentialOptions();
ASSERT_TRUE(options.has_value());
// not return valid options if cert is not changed
auto new_options = tiflash_config.readAndCacheSslCredentialOptions();
ASSERT_FALSE(new_options.has_value());
ca_path = file_path + "/tls/ca.crt.new";
cert_path = file_path + "/tls/cert.crt.new";
key_path = file_path + "/tls/key.pem.new";
{
Poco::FileOutputStream ca_fos(ca_path);
ca_fos << options.value().pem_root_certs;
Poco::FileOutputStream cert_fos(cert_path);
cert_fos << options.value().pem_cert_chain;
Poco::FileOutputStream key_fos(key_path);
key_fos << options.value().pem_private_key;
}
test = createConfigString(ca_path, cert_path, key_path);
config = loadConfigFromString(test);
ASSERT_TRUE(tiflash_config.update(*config));
// return a valid options if cert is changed
options = tiflash_config.readAndCacheSslCredentialOptions();
ASSERT_TRUE(options.has_value());
new_options = tiflash_config.readAndCacheSslCredentialOptions();
ASSERT_FALSE(new_options.has_value());
using namespace std::chrono_literals;
std::this_thread::sleep_for(1s);
{
// change the modified time
Poco::FileOutputStream fos(ca_path);
fos << options.value().pem_root_certs;
}
new_options = tiflash_config.readAndCacheSslCredentialOptions();
// no aware of the change since `update` is not called
ASSERT_FALSE(new_options.has_value());
ASSERT_TRUE(tiflash_config.update(*config));
// aware of the change and return a new options
new_options = tiflash_config.readAndCacheSslCredentialOptions();
ASSERT_TRUE(new_options.has_value());
Poco::File ca_file(ca_path);
ca_file.remove(false);
Poco::File cert_file(cert_path);
cert_file.remove(false);
Poco::File key_file(key_path);
key_file.remove(false);
}
} // namespace tests
} // namespace DB
20 changes: 20 additions & 0 deletions dbms/src/Common/tests/tls/ca.crt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDWjCCAkKgAwIBAgIUWrP0VvHcy+LP6UuYNtiL9gBhD5owDQYJKoZIhvcNAQEL
BQAwVjELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM
GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDEPMA0GA1UEAwwGdGVzdGNhMB4XDTIw
MDMxNzE4NTk1MVoXDTMwMDMxNTE4NTk1MVowVjELMAkGA1UEBhMCQVUxEzARBgNV
BAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0
ZDEPMA0GA1UEAwwGdGVzdGNhMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC
AQEAsGL0oXflF0LzoM+Bh+qUU9yhqzw2w8OOX5mu/iNCyUOBrqaHi7mGHx73GD01
diNzCzvlcQqdNIH6NQSL7DTpBjca66jYT9u73vZe2MDrr1nVbuLvfu9850cdxiUO
Inv5xf8+sTHG0C+a+VAvMhsLiRjsq+lXKRJyk5zkbbsETybqpxoJ+K7CoSy3yc/k
QIY3TipwEtwkKP4hzyo6KiGd/DPexie4nBUInN3bS1BUeNZ5zeaIC2eg3bkeeW7c
qT55b+Yen6CxY0TEkzBK6AKt/WUialKMgT0wbTxRZO7kUCH3Sq6e/wXeFdJ+HvdV
LPlAg5TnMaNpRdQih/8nRFpsdwIDAQABoyAwHjAMBgNVHRMEBTADAQH/MA4GA1Ud
DwEB/wQEAwICBDANBgkqhkiG9w0BAQsFAAOCAQEAkTrKZjBrJXHps/HrjNCFPb5a
THuGPCSsepe1wkKdSp1h4HGRpLoCgcLysCJ5hZhRpHkRihhef+rFHEe60UePQO3S
CVTtdJB4CYWpcNyXOdqefrbJW5QNljxgi6Fhvs7JJkBqdXIkWXtFk2eRgOIP2Eo9
/OHQHlYnwZFrk6sp4wPyR+A95S0toZBcyDVz7u+hOW0pGK3wviOe9lvRgj/H3Pwt
bewb0l+MhRig0/DVHamyVxrDRbqInU1/GTNCwcZkXKYFWSf92U+kIcTth24Q1gcw
eZiLl5FfrWokUNytFElXob0V0a5/kbhiLc3yWmvWqHTpqCALbVyF+rKJo2f5Kw==
-----END CERTIFICATE-----
22 changes: 22 additions & 0 deletions dbms/src/Common/tests/tls/cert.crt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
-----BEGIN CERTIFICATE-----
MIIDtDCCApygAwIBAgIUbJfTREJ6k6/+oInWhV1O1j3ZT0IwDQYJKoZIhvcNAQEL
BQAwVjELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM
GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDEPMA0GA1UEAwwGdGVzdGNhMB4XDTIw
MDMxODAzMTA0MloXDTMwMDMxNjAzMTA0MlowZTELMAkGA1UEBhMCVVMxETAPBgNV
BAgMCElsbGlub2lzMRAwDgYDVQQHDAdDaGljYWdvMRUwEwYDVQQKDAxFeGFtcGxl
LCBDby4xGjAYBgNVBAMMESoudGVzdC5nb29nbGUuY29tMIIBIjANBgkqhkiG9w0B
AQEFAAOCAQ8AMIIBCgKCAQEA5xOONxJJ8b8Qauvob5/7dPYZfIcd+uhAWL2ZlTPz
Qvu4oF0QI4iYgP5iGgry9zEtCM+YQS8UhiAlPlqa6ANxgiBSEyMHH/xE8lo/+caY
GeACqy640Jpl/JocFGo3xd1L8DCawjlaj6eu7T7T/tpAV2qq13b5710eNRbCAfFe
8yALiGQemx0IYhlZXNbIGWLBNhBhvVjJh7UvOqpADk4xtl8o5j0xgMIRg6WJGK6c
6ffSIg4eP1XmovNYZ9LLEJG68tF0Q/yIN43B4dt1oq4jzSdCbG4F1EiykT2TmwPV
YDi8tml6DfOCDGnit8svnMEmBv/fcPd31GSbXjF8M+KGGQIDAQABo2swaTAJBgNV
HRMEAjAAMAsGA1UdDwQEAwIF4DBPBgNVHREESDBGghAqLnRlc3QuZ29vZ2xlLmZy
ghh3YXRlcnpvb2kudGVzdC5nb29nbGUuYmWCEioudGVzdC55b3V0dWJlLmNvbYcE
wKgBAzANBgkqhkiG9w0BAQsFAAOCAQEAS8hDQA8PSgipgAml7Q3/djwQ644ghWQv
C2Kb+r30RCY1EyKNhnQnIIh/OUbBZvh0M0iYsy6xqXgfDhCB93AA6j0i5cS8fkhH
Jl4RK0tSkGQ3YNY4NzXwQP/vmUgfkw8VBAZ4Y4GKxppdATjffIW+srbAmdDruIRM
wPeikgOoRrXf0LA1fi4TqxARzeRwenQpayNfGHTvVF9aJkl8HoaMunTAdG5pIVcr
9GKi/gEMpXUJbbVv3U5frX1Wo4CFo+rZWJ/LyCMeb0jciNLxSdMwj/E/ZuExlyeZ
gc9ctPjSMvgSyXEKv6Vwobleeg88V2ZgzenziORoWj4KszG/lbQZvg==
-----END CERTIFICATE-----
28 changes: 28 additions & 0 deletions dbms/src/Common/tests/tls/key.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDnE443EknxvxBq
6+hvn/t09hl8hx366EBYvZmVM/NC+7igXRAjiJiA/mIaCvL3MS0Iz5hBLxSGICU+
WproA3GCIFITIwcf/ETyWj/5xpgZ4AKrLrjQmmX8mhwUajfF3UvwMJrCOVqPp67t
PtP+2kBXaqrXdvnvXR41FsIB8V7zIAuIZB6bHQhiGVlc1sgZYsE2EGG9WMmHtS86
qkAOTjG2XyjmPTGAwhGDpYkYrpzp99IiDh4/Veai81hn0ssQkbry0XRD/Ig3jcHh
23WiriPNJ0JsbgXUSLKRPZObA9VgOLy2aXoN84IMaeK3yy+cwSYG/99w93fUZJte
MXwz4oYZAgMBAAECggEBAIVn2Ncai+4xbH0OLWckabwgyJ4IM9rDc0LIU368O1kU
koais8qP9dujAWgfoh3sGh/YGgKn96VnsZjKHlyMgF+r4TaDJn3k2rlAOWcurGlj
1qaVlsV4HiEzp7pxiDmHhWvp4672Bb6iBG+bsjCUOEk/n9o9KhZzIBluRhtxCmw5
nw4Do7z00PTvN81260uPWSc04IrytvZUiAIx/5qxD72bij2xJ8t/I9GI8g4FtoVB
8pB6S/hJX1PZhh9VlU6Yk+TOfOVnbebG4W5138LkB835eqk3Zz0qsbc2euoi8Hxi
y1VGwQEmMQ63jXz4c6g+X55ifvUK9Jpn5E8pq+pMd7ECgYEA93lYq+Cr54K4ey5t
sWMa+ye5RqxjzgXj2Kqr55jb54VWG7wp2iGbg8FMlkQwzTJwebzDyCSatguEZLuB
gRGroRnsUOy9vBvhKPOch9bfKIl6qOgzMJB267fBVWx5ybnRbWN/I7RvMQf3k+9y
biCIVnxDLEEYyx7z85/5qxsXg/MCgYEA7wmWKtCTn032Hy9P8OL49T0X6Z8FlkDC
Rk42ygrc/MUbugq9RGUxcCxoImOG9JXUpEtUe31YDm2j+/nbvrjl6/bP2qWs0V7l
dTJl6dABP51pCw8+l4cWgBBX08Lkeen812AAFNrjmDCjX6rHjWHLJcpS18fnRRkP
V1d/AHWX7MMCgYEA6Gsw2guhp0Zf2GCcaNK5DlQab8OL4Hwrpttzo4kuTlwtqNKp
Q9H4al9qfF4Cr1TFya98+EVYf8yFRM3NLNjZpe3gwYf2EerlJj7VLcahw0KKzoN1
QBENfwgPLRk5sDkx9VhSmcfl/diLroZdpAwtv3vo4nEoxeuGFbKTGx3Qkf0CgYEA
xyR+dcb05Ygm3w4klHQTowQ10s1H80iaUcZBgQuR1ghEtDbUPZHsoR5t1xCB02ys
DgAwLv1bChIvxvH/L6KM8ovZ2LekBX4AviWxoBxJnfz/EVau98B0b1auRN6eSC83
FRuGldlSOW1z/nSh8ViizSYE5H5HX1qkXEippvFRE88CgYB3Bfu3YQY60ITWIShv
nNkdcbTT9eoP9suaRJjw92Ln+7ZpALYlQMKUZmJ/5uBmLs4RFwUTQruLOPL4yLTH
awADWUzs3IRr1fwn9E+zM8JVyKCnUEM3w4N5UZskGO2klashAd30hWO+knRv/y0r
uGIYs9Ek7YXlXIRVrzMwcsrt1w==
-----END PRIVATE KEY-----
12 changes: 9 additions & 3 deletions dbms/src/Server/FlashGrpcServerHolder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,15 @@ static grpc_ssl_certificate_config_reload_status sslServerCertificateConfigCallb
}
auto * context = static_cast<Context *>(arg);
auto options = context->getSecurityConfig()->readAndCacheSslCredentialOptions();
grpc_ssl_pem_key_cert_pair pem_key_cert_pair = {options.pem_private_key.c_str(), options.pem_cert_chain.c_str()};
*config = grpc_ssl_server_certificate_config_create(options.pem_root_certs.c_str(), &pem_key_cert_pair, 1);
return GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW;
if (options.has_value())
{
grpc_ssl_pem_key_cert_pair pem_key_cert_pair
= {options.value().pem_private_key.c_str(), options.value().pem_cert_chain.c_str()};
*config
= grpc_ssl_server_certificate_config_create(options.value().pem_root_certs.c_str(), &pem_key_cert_pair, 1);
return GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW;
}
return GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;
}

grpc_server_credentials * grpcSslServerCredentialsCreateWithFetcher(
Expand Down

0 comments on commit 416653e

Please sign in to comment.