From c1838001167c8ba83af759085a71ad61e6c2a5af Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Wed, 22 May 2024 16:55:17 +0800 Subject: [PATCH] security: Do not let grpc reload ssh cert if not changed (#9068) (#9071) ref pingcap/tiflash#8535 --- .github/licenserc.yml | 1 + dbms/src/Common/TiFlashSecurity.h | 6 +- .../Common/tests/gtest_tiflash_security.cpp | 83 +++++++++++++++++++ dbms/src/Common/tests/tls/ca.crt | 20 +++++ dbms/src/Common/tests/tls/cert.crt | 22 +++++ dbms/src/Common/tests/tls/key.pem | 28 +++++++ dbms/src/Server/FlashGrpcServerHolder.cpp | 12 ++- 7 files changed, 167 insertions(+), 5 deletions(-) create mode 100644 dbms/src/Common/tests/tls/ca.crt create mode 100644 dbms/src/Common/tests/tls/cert.crt create mode 100644 dbms/src/Common/tests/tls/key.pem diff --git a/.github/licenserc.yml b/.github/licenserc.yml index 69460282f4a..2eebf892ae0 100644 --- a/.github/licenserc.yml +++ b/.github/licenserc.yml @@ -43,5 +43,6 @@ header: - '**/OWNERS' - 'OWNERS_ALIASES' - '**/*.sql' + - 'dbms/src/Common/tests/tls/' comment: on-failure diff --git a/dbms/src/Common/TiFlashSecurity.h b/dbms/src/Common/TiFlashSecurity.h index 3a5c7d85075..a02afc9d04e 100644 --- a/dbms/src/Common/TiFlashSecurity.h +++ b/dbms/src/Common/TiFlashSecurity.h @@ -177,11 +177,13 @@ class TiFlashSecurityConfig : public ConfigObject return allowed_common_names.count(cert.commonName()) > 0; } - grpc::SslCredentialsOptions readAndCacheSslCredentialOptions() + std::optional 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); diff --git a/dbms/src/Common/tests/gtest_tiflash_security.cpp b/dbms/src/Common/tests/gtest_tiflash_security.cpp index eb4e17ed945..7319744ffe1 100644 --- a/dbms/src/Common/tests/gtest_tiflash_security.cpp +++ b/dbms/src/Common/tests/gtest_tiflash_security.cpp @@ -13,6 +13,8 @@ // limitations under the License. #include +#include +#include #include #include @@ -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 diff --git a/dbms/src/Common/tests/tls/ca.crt b/dbms/src/Common/tests/tls/ca.crt new file mode 100644 index 00000000000..49d39cd8ed5 --- /dev/null +++ b/dbms/src/Common/tests/tls/ca.crt @@ -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----- diff --git a/dbms/src/Common/tests/tls/cert.crt b/dbms/src/Common/tests/tls/cert.crt new file mode 100644 index 00000000000..88244f856c6 --- /dev/null +++ b/dbms/src/Common/tests/tls/cert.crt @@ -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----- diff --git a/dbms/src/Common/tests/tls/key.pem b/dbms/src/Common/tests/tls/key.pem new file mode 100644 index 00000000000..086462992cf --- /dev/null +++ b/dbms/src/Common/tests/tls/key.pem @@ -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----- diff --git a/dbms/src/Server/FlashGrpcServerHolder.cpp b/dbms/src/Server/FlashGrpcServerHolder.cpp index bbbcd63b05c..95fd21ac9aa 100644 --- a/dbms/src/Server/FlashGrpcServerHolder.cpp +++ b/dbms/src/Server/FlashGrpcServerHolder.cpp @@ -96,9 +96,15 @@ static grpc_ssl_certificate_config_reload_status sslServerCertificateConfigCallb } auto * context = static_cast(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(