From bcf0861a0a69e4d025673f0e67653b3955ecbc29 Mon Sep 17 00:00:00 2001 From: Junhyun Shim Date: Thu, 1 Sep 2022 15:37:24 +0200 Subject: [PATCH 1/2] Make fdbcli check for TLSConfig and coordinator address mismatch At least one of the coordinator addresses in cluster file must contain ":tls" suffix if fdbcli's resolved TLS client configuration holds any of the TLS elements (key, cert, or CA) Conversely, if none of the TLS elements are configured, at least one of coordinator addresses must be without ":tls" suffix --- fdbcli/fdbcli.actor.cpp | 68 ++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/fdbcli/fdbcli.actor.cpp b/fdbcli/fdbcli.actor.cpp index b10ed32a20..12106dd9d4 100644 --- a/fdbcli/fdbcli.actor.cpp +++ b/fdbcli/fdbcli.actor.cpp @@ -1050,7 +1050,7 @@ Future stopNetworkAfter(Future what) { } } -ACTOR Future cli(CLIOptions opt, LineNoise* plinenoise) { +ACTOR Future cli(CLIOptions opt, LineNoise* plinenoise, Reference ccf) { state LineNoise& linenoise = *plinenoise; state bool intrans = false; @@ -1075,20 +1075,6 @@ ACTOR Future cli(CLIOptions opt, LineNoise* plinenoise) { state FdbOptions* options = &globalOptions; - state Reference ccf; - - state std::pair resolvedClusterFile = - ClusterConnectionFile::lookupClusterFileName(opt.clusterFile); - try { - ccf = makeReference(resolvedClusterFile.first); - } catch (Error& e) { - if (e.code() == error_code_operation_cancelled) { - throw; - } - fprintf(stderr, "%s\n", ClusterConnectionFile::getErrorString(resolvedClusterFile, e).c_str()); - return 1; - } - // Ordinarily, this is done when the network is run. However, network thread should be set before TraceEvents are // logged. This thread will eventually run the network, so call it now. TraceEvent::setNetworkThread(); @@ -1987,7 +1973,7 @@ ACTOR Future cli(CLIOptions opt, LineNoise* plinenoise) { } } -ACTOR Future runCli(CLIOptions opt) { +ACTOR Future runCli(CLIOptions opt, Reference ccf) { state LineNoise linenoise( [](std::string const& line, std::vector& completions) { fdbcliCompCmd(line, completions); }, [enabled = opt.cliHints](std::string const& line) -> LineNoise::Hint { @@ -2051,7 +2037,7 @@ ACTOR Future runCli(CLIOptions opt) { .GetLastError(); } - state int result = wait(cli(opt, &linenoise)); + state int result = wait(cli(opt, &linenoise, ccf)); if (!historyFilename.empty()) { try { @@ -2073,6 +2059,33 @@ ACTOR Future timeExit(double duration) { return Void(); } +const char* checkTlsConfigAgainstCoordAddrs(const ClusterConnectionString& ccs) { + // Resolve TLS config and inspect whether any of the certificate, key, ca bytes has been set + extern TLSConfig tlsConfig; + auto const loaded = tlsConfig.loadSync(); + const bool tlsConfigured = + !loaded.getCertificateBytes().empty() || !loaded.getKeyBytes().empty() || !loaded.getCABytes().empty(); + int tlsAddrs = 0; + int totalAddrs = 0; + for (const auto& addr : ccs.coords) { + if (addr.isTLS()) + tlsAddrs++; + totalAddrs++; + } + for (const auto& host : ccs.hostnames) { + if (host.isTLS) + tlsAddrs++; + totalAddrs++; + } + if (tlsConfigured && tlsAddrs == 0) { + return "fdbcli is configured with TLS, but none of the coordinators have TLS addresses."; + } else if (!tlsConfigured && tlsAddrs == totalAddrs) { + return "fdbcli is not configured with TLS, but all of the coordinators have TLS addresses."; + } else { + return nullptr; + } +} + int main(int argc, char** argv) { platformInit(); Error::init(); @@ -2177,6 +2190,25 @@ int main(int argc, char** argv) { return 0; } + Reference ccf; + std::pair resolvedClusterFile = ClusterConnectionFile::lookupClusterFileName(opt.clusterFile); + + try { + ccf = makeReference(resolvedClusterFile.first); + } catch (Error& e) { + if (e.code() == error_code_operation_cancelled) { + throw; + } + fprintf(stderr, "%s\n", ClusterConnectionFile::getErrorString(resolvedClusterFile, e).c_str()); + return 1; + } + + // Make sure that TLS configuration lines up with ":tls" prefix on coordinator addresses + if (auto errorMsg = checkTlsConfigAgainstCoordAddrs(ccf->getConnectionString())) { + fprintf(stderr, "ERROR: %s\n", errorMsg); + return 1; + } + try { API->selectApiVersion(opt.apiVersion); if (opt.useFutureProtocolVersion) { @@ -2188,7 +2220,7 @@ int main(int argc, char** argv) { return opt.exit_code; } Future memoryUsageMonitor = startMemoryUsageMonitor(opt.memLimit); - Future cliFuture = runCli(opt); + Future cliFuture = runCli(opt, ccf); Future timeoutFuture = opt.exit_timeout ? timeExit(opt.exit_timeout) : Never(); auto f = stopNetworkAfter(success(cliFuture) || timeoutFuture); API->runNetwork(); From 738a101a5887dcf93a2f707f7f5702b03b4b2b06 Mon Sep 17 00:00:00 2001 From: Junhyun Shim Date: Mon, 5 Sep 2022 19:27:22 +0200 Subject: [PATCH 2/2] Add test for fdbcli's coordinator TLS suffix check --- bindings/python/tests/fdbcli_tests.py | 64 +++++++++++++++++++++++++++ flow/MkCertCli.cpp | 13 ++++-- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/bindings/python/tests/fdbcli_tests.py b/bindings/python/tests/fdbcli_tests.py index 552bba8f49..0fbb0adfa4 100755 --- a/bindings/python/tests/fdbcli_tests.py +++ b/bindings/python/tests/fdbcli_tests.py @@ -7,6 +7,7 @@ import subprocess import logging import functools import json +import tempfile import time import random from argparse import ArgumentParser, RawDescriptionHelpFormatter @@ -770,6 +771,68 @@ def integer_options(): assert lines[1].startswith('Committed') assert error_output == b'' +def tls_address_suffix(): + # fdbcli shall prevent a non-TLS fdbcli run from connecting to an all-TLS cluster, and vice versa + preamble = 'eNW1yf1M:eNW1yf1M@' + def make_addr(port: int, tls: bool = False): + return "127.0.0.1:{}{}".format(port, ":tls" if tls else "") + testcases = [ + # IsServerTLS, NumServerAddrs + (True, 1), + (False, 1), + (True, 3), + (False, 3), + ] + err_output_server_no_tls = "ERROR: fdbcli is configured with TLS, but none of the coordinators have TLS addresses." + err_output_server_tls = "ERROR: fdbcli is not configured with TLS, but all of the coordinators have TLS addresses." + + # technically the contents of the certs and key files are not evaluated + # before tls-suffix check against tls configuration takes place, + # but we generate the certs and keys anyway to avoid + # imposing nuanced TLSConfig evaluation ordering requirement on the testcase + with tempfile.TemporaryDirectory() as tmpdir: + cert_file = tmpdir + "/client-cert.pem" + key_file = tmpdir + "/client-key.pem" + ca_file = tmpdir + "/server-ca.pem" + mkcert_process = subprocess.run([ + args.build_dir + "/bin/mkcert", + "--server-chain-length", "1", + "--client-chain-length", "1", + "--server-cert-file", tmpdir + "/server-cert.pem", + "--client-cert-file", tmpdir + "/client-cert.pem", + "--server-key-file", tmpdir + "/server-key.pem", + "--client-key-file", tmpdir + "/client-key.pem", + "--server-ca-file", tmpdir + "/server-ca.pem", + "--client-ca-file", tmpdir + "/client-ca.pem", + ], + capture_output=True) + if mkcert_process.returncode != 0: + print("mkcert returned with code {}".format(mkcert_process.returncode)) + print("Output:\n{}{}\n".format( + mkcert_process.stdout.decode("utf8").strip(), + mkcert_process.stderr.decode("utf8").strip())) + assert False + cluster_fn = tmpdir + "/fdb.cluster" + for testcase in testcases: + is_server_tls, num_server_addrs = testcase + with open(cluster_fn, "w") as fp: + fp.write(preamble + ",".join( + [make_addr(port=4000 + addr_idx, tls=is_server_tls) for addr_idx in range(num_server_addrs)])) + fp.close() + tls_args = ["--tls-certificate-file", + cert_file, + "--tls-key-file", + key_file, + "--tls-ca-file", + ca_file] if not is_server_tls else [] + fdbcli_process = subprocess.run(command_template[:2] + [cluster_fn] + tls_args, capture_output=True) + assert fdbcli_process.returncode != 0 + err_out = fdbcli_process.stderr.decode("utf8").strip() + if is_server_tls: + assert err_out == err_output_server_tls, f"unexpected output: {err_out}" + else: + assert err_out == err_output_server_no_tls, f"unexpected output: {err_out}" + if __name__ == '__main__': parser = ArgumentParser(formatter_class=RawDescriptionHelpFormatter, description=""" @@ -816,6 +879,7 @@ if __name__ == '__main__': tenants() versionepoch() integer_options() + tls_address_suffix() else: assert args.process_number > 1, "Process number should be positive" coordinators() diff --git a/flow/MkCertCli.cpp b/flow/MkCertCli.cpp index 7517e2a203..c809bf8eb5 100644 --- a/flow/MkCertCli.cpp +++ b/flow/MkCertCli.cpp @@ -49,6 +49,7 @@ enum EMkCertOpt : int { OPT_PRINT_SERVER_CERT, OPT_PRINT_CLIENT_CERT, OPT_PRINT_ARGUMENTS, + OPT_ENABLE_TRACE, }; CSimpleOpt::SOption gOptions[] = { { OPT_HELP, "--help", SO_NONE }, @@ -68,6 +69,7 @@ CSimpleOpt::SOption gOptions[] = { { OPT_HELP, "--help", SO_NONE }, { OPT_PRINT_SERVER_CERT, "--print-server-cert", SO_NONE }, { OPT_PRINT_CLIENT_CERT, "--print-client-cert", SO_NONE }, { OPT_PRINT_ARGUMENTS, "--print-args", SO_NONE }, + { OPT_ENABLE_TRACE, "--trace", SO_NONE }, SO_END_OF_OPTIONS }; template @@ -191,6 +193,7 @@ int main(int argc, char** argv) { auto printServerCert = false; auto printClientCert = false; auto printArguments = false; + auto enableTrace = false; auto args = CSimpleOpt(argc, argv, gOptions, SO_O_EXACT | SO_O_HYPHEN_TO_UNDERSCORE); while (args.Next()) { if (auto err = args.LastError()) { @@ -266,6 +269,8 @@ int main(int argc, char** argv) { case OPT_PRINT_ARGUMENTS: printArguments = true; break; + case OPT_ENABLE_TRACE: + enableTrace = true; default: fmt::print(stderr, "ERROR: Unknown option {}\n", args.OptionText()); return FDB_EXIT_ERROR; @@ -277,12 +282,14 @@ int main(int argc, char** argv) { platformInit(); Error::init(); g_network = newNet2(TLSConfig()); - openTraceFile(NetworkAddress(), 10 << 20, 10 << 20, ".", "mkcert"); + if (enableTrace) + openTraceFile(NetworkAddress(), 10 << 20, 10 << 20, ".", "mkcert"); auto thread = std::thread([]() { g_network->run(); }); - auto cleanUpGuard = ScopeExit([&thread]() { + auto cleanUpGuard = ScopeExit([&thread, enableTrace]() { g_network->stop(); thread.join(); - closeTraceFile(); + if (enableTrace) + closeTraceFile(); }); serverArgs.transformPathToAbs();