Skip to content

Commit

Permalink
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Browse files Browse the repository at this point in the history 
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.

* Add a flag --ssl_cipher_list which controls cipher selection for both
  thrift servers and clients. Default is blank, which means use all
  available cipher suites.
* Add ThriftServerBuilder to simplify construction of
  ThriftServers (whose constructors were otherwise getting very long).

Testing: new tests added to thrift-server-test. Test cases added follow:

* A client cannot connect to a server which does not have any ciphers in
  common with it.
* If ciphers are identical on clients and servers, that ssl connections
  can be made.
* Bad cipher strings lead to errors on both client and server.

Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Reviewed-on: http://gerrit.cloudera.org:8080/7524
Reviewed-by: Henry Robinson <henry@cloudera.com>
Tested-by: Impala Public Jenkins
  • Loading branch information
Henry Robinson authored and Impala Public Jenkins committed Aug 8, 2017
1 parent d61065d commit 68df21b
Show file tree
Hide file tree
Showing 13 changed files with 440 additions and 143 deletions.
6 changes: 4 additions & 2 deletions be/src/benchmarks/network-perf-benchmark.cc
View file
Expand Up @@ -221,8 +221,10 @@ int main(int argc, char** argv) {
boost::shared_ptr<ThreadFactory> thread_factory(
new ThriftThreadFactory("test", "test"));
boost::shared_ptr<TProcessor> processor(new NetworkTestServiceProcessor(handler));
ThriftServer* server = new ThriftServer("Network Test Server", processor,
FLAGS_port, NULL, NULL, 100, ThriftServer::ThreadPool);
ThriftServer* server;
ABORT_IF_ERROR(ThriftServerBuilder("Network Test Server", processor, FLAGS_port)
.thread_pool(100)
.Build(&server));
thread* server_thread = new thread(&TestServer::Server, handler.get(), server);

string input;
Expand Down
12 changes: 8 additions & 4 deletions be/src/catalog/catalogd-main.cc
View file
Expand Up @@ -47,6 +47,7 @@ DECLARE_int32(state_store_subscriber_port);
DECLARE_string(ssl_server_certificate);
DECLARE_string(ssl_private_key);
DECLARE_string(ssl_private_key_password_cmd);
DECLARE_string(ssl_cipher_list);

#include "common/names.h"

Expand Down Expand Up @@ -89,13 +90,16 @@ int CatalogdMain(int argc, char** argv) {
new RpcEventHandler("catalog-server", metrics.get()));
processor->setEventHandler(event_handler);

ThriftServer* server = new ThriftServer("CatalogService", processor,
FLAGS_catalog_service_port, NULL, metrics.get(), 5);
ThriftServer* server;
ThriftServerBuilder builder("CatalogService", processor, FLAGS_catalog_service_port);

if (EnableInternalSslConnections()) {
LOG(INFO) << "Enabling SSL for CatalogService";
ABORT_IF_ERROR(server->EnableSsl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key,
FLAGS_ssl_private_key_password_cmd));
builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
.pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
.cipher_list(FLAGS_ssl_cipher_list);
}
ABORT_IF_ERROR(builder.metrics(metrics.get()).Build(&server));
ABORT_IF_ERROR(server->Start());
LOG(INFO) << "CatalogService started on port: " << FLAGS_catalog_service_port;
server->Join();
Expand Down
2 changes: 2 additions & 0 deletions be/src/rpc/thrift-client.cc
View file
Expand Up @@ -32,6 +32,7 @@ using namespace apache::thrift;
using namespace strings;

DECLARE_string(ssl_client_ca_certificate);
DECLARE_string(ssl_cipher_list);

namespace impala {

Expand Down Expand Up @@ -100,6 +101,7 @@ Status ThriftClientImpl::CreateSocket() {
socket_.reset(new TSocket(address_.hostname, address_.port));
} else {
try {
if (!FLAGS_ssl_cipher_list.empty()) ssl_factory_->ciphers(FLAGS_ssl_cipher_list);
ssl_factory_->loadTrustedCertificates(FLAGS_ssl_client_ca_certificate.c_str());
socket_ = ssl_factory_->createSocket(address_.hostname, address_.port);
} catch (const TException& e) {
Expand Down
217 changes: 172 additions & 45 deletions be/src/rpc/thrift-server-test.cc
View file
Expand Up @@ -17,12 +17,13 @@

#include <string>

#include "testutil/gtest-util.h"
#include "gen-cpp/StatestoreService.h"
#include "gutil/strings/substitute.h"
#include "rpc/thrift-client.h"
#include "service/fe-support.h"
#include "service/impala-server.h"
#include "gen-cpp/StatestoreService.h"
#include "gutil/strings/substitute.h"
#include "testutil/gtest-util.h"
#include "testutil/scoped-flag-setter.h"

#include "common/names.h"

Expand All @@ -31,6 +32,7 @@ using namespace strings;
using namespace apache::thrift;

DECLARE_string(ssl_client_ca_certificate);
DECLARE_string(ssl_cipher_list);

DECLARE_int32(state_store_port);

Expand Down Expand Up @@ -71,12 +73,12 @@ int GetServerPort() {

TEST(ThriftServer, Connectivity) {
int port = GetServerPort();
ThriftClient<StatestoreServiceClientWrapper> wrong_port_client("localhost",
port, "", NULL, false);
ThriftClient<StatestoreServiceClientWrapper> wrong_port_client(
"localhost", port, "", nullptr, false);
ASSERT_FALSE(wrong_port_client.Open().ok());

ThriftServer* server =
new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 5);
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port).Build(&server));
ASSERT_OK(server->Start());

// Test that client recovers from failure to connect.
Expand All @@ -89,14 +91,15 @@ TEST(SslTest, Connectivity) {
// client cannot.
// Here and elsewhere - allocate ThriftServers on the heap to avoid race during
// destruction. See IMPALA-2283.
ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL,
NULL, 5);
ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password"));
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PRIVATE_KEY)
.Build(&server));
ASSERT_OK(server->Start());

FLAGS_ssl_client_ca_certificate = SERVER_CERT;
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", NULL, true);
"localhost", port, "", nullptr, true);
ASSERT_OK(ssl_client.Open());
TRegisterSubscriberResponse resp;
bool send_done = false;
Expand All @@ -106,7 +109,7 @@ TEST(SslTest, Connectivity) {

// Disable SSL for this client.
ThriftClient<StatestoreServiceClientWrapper> non_ssl_client(
"localhost", port, "", NULL, false);
"localhost", port, "", nullptr, false);
ASSERT_OK(non_ssl_client.Open());
send_done = false;
EXPECT_THROW(non_ssl_client.iface()->RegisterSubscriber(
Expand All @@ -116,13 +119,14 @@ TEST(SslTest, Connectivity) {
TEST(SslTest, BadCertificate) {
FLAGS_ssl_client_ca_certificate = "unknown";
int port = GetServerPort();
ThriftClient<StatestoreServiceClientWrapper>
ssl_client("localhost", port, "", NULL, true);
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);
ASSERT_FALSE(ssl_client.Open().ok());

ThriftServer* server =
new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 5);
ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password"));
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PRIVATE_KEY)
.Build(&server));
ASSERT_OK(server->Start());

// Check that client does not recover from failure to create socket.
Expand All @@ -133,14 +137,16 @@ TEST(PasswordProtectedPemFile, CorrectOperation) {
// Require the server to execute a shell command to read the password to the private key
// file.
int port = GetServerPort();
ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL,
NULL, 5);
ASSERT_OK(server->EnableSsl(
SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo password"));
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("echo password")
.Build(&server));
ASSERT_OK(server->Start());
FLAGS_ssl_client_ca_certificate = SERVER_CERT;
ThriftClient<StatestoreServiceClientWrapper>
ssl_client("localhost", port, "", NULL, true);

auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, SERVER_CERT);
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);
ASSERT_OK(ssl_client.Open());
TRegisterSubscriberResponse resp;
bool send_done = false;
Expand All @@ -150,28 +156,38 @@ TEST(PasswordProtectedPemFile, CorrectOperation) {

TEST(PasswordProtectedPemFile, BadPassword) {
// Test failure when password to private key is wrong.
ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), NULL, NULL, 5);
ASSERT_OK(server.EnableSsl(
SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo wrongpassword"));
EXPECT_FALSE(server.Start().ok());
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), GetServerPort())
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("echo wrongpassword")
.Build(&server));
EXPECT_FALSE(server->Start().ok());
}

TEST(PasswordProtectedPemFile, BadCommand) {
// Test failure when password command is badly formed.
ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), NULL, NULL, 5);
EXPECT_FALSE(server.EnableSsl(
SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "cmd-no-exist").ok());
ThriftServer* server;

// Keep clang-tdy happy - NOLINT (which here is due to deliberately leaked 'server')
// does not get pushed into EXPECT_ERROR.
Status s = ThriftServerBuilder("DummyStatestore", MakeProcessor(), GetServerPort()) // NOLINT
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("cmd-no-exist")
.Build(&server);
EXPECT_ERROR(s, TErrorCode::SSL_PASSWORD_CMD_FAILED);
}

TEST(SslTest, ClientBeforeServer) {
// Instantiate a thrift client before a thrift server and test if it works (IMPALA-2747)
FLAGS_ssl_client_ca_certificate = SERVER_CERT;
auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, SERVER_CERT);
int port = GetServerPort();
ThriftClient<StatestoreServiceClientWrapper>
ssl_client("localhost", port, "", NULL, true);
ThriftServer* server =
new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 5);
ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY));
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);

ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PRIVATE_KEY)
.Build(&server));
ASSERT_OK(server->Start());

ASSERT_OK(ssl_client.Open());
Expand All @@ -180,6 +196,113 @@ TEST(SslTest, ClientBeforeServer) {
ssl_client.iface()->RegisterSubscriber(resp, TRegisterSubscriberRequest(), &send_done);
}

TEST(SslTest, BadCiphers) {
int port = GetServerPort();
{
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PRIVATE_KEY)
.cipher_list("this_is_not_a_cipher")
.Build(&server));
EXPECT_FALSE(server->Start().ok());
}

{
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PRIVATE_KEY)
.Build(&server));
EXPECT_OK(server->Start());

auto s1 =
ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, "this_is_not_a_cipher");
auto s2 =
ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, SERVER_CERT);

ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);
EXPECT_FALSE(ssl_client.Open().ok());
}
}

TEST(SslTest, MismatchedCiphers) {
int port = GetServerPort();
FLAGS_ssl_client_ca_certificate = SERVER_CERT;

ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("echo password")
.cipher_list("AES256-SHA256")
.Build(&server));
EXPECT_OK(server->Start());

auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, "RC4-SHA");
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);

// Failure to negotiate a cipher will show up when data is sent, not when socket is
// opened.
EXPECT_OK(ssl_client.Open());

bool send_done = false;
TRegisterSubscriberResponse resp;
EXPECT_THROW(ssl_client.iface()->RegisterSubscriber(
resp, TRegisterSubscriberRequest(), &send_done),
TTransportException);
}

TEST(SslTest, MatchedCiphers) {
int port = GetServerPort();
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("echo password")
.cipher_list("AES256-SHA256")
.Build(&server));
EXPECT_OK(server->Start());

FLAGS_ssl_client_ca_certificate = SERVER_CERT;
auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, "AES256-SHA256");
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);

EXPECT_OK(ssl_client.Open());

bool send_done = false;
TRegisterSubscriberResponse resp;
EXPECT_NO_THROW({
ssl_client.iface()->RegisterSubscriber(
resp, TRegisterSubscriberRequest(), &send_done);
});
}

TEST(SslTest, OverlappingMatchedCiphers) {
int port = GetServerPort();
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
.pem_password_cmd("echo password")
.cipher_list("RC4-SHA,AES256-SHA256")
.Build(&server));
EXPECT_OK(server->Start());

FLAGS_ssl_client_ca_certificate = SERVER_CERT;
auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list,
"AES256-SHA256,not-a-cipher");
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", port, "", nullptr, true);

EXPECT_OK(ssl_client.Open());

bool send_done = false;
TRegisterSubscriberResponse resp;
EXPECT_NO_THROW({
ssl_client.iface()->RegisterSubscriber(
resp, TRegisterSubscriberRequest(), &send_done);
});
}

/// Test disabled because requires a high ulimit -n on build machines. Since the test does
/// not always fail, we don't lose much coverage by disabling it until we fix the build
/// infra issue.
Expand All @@ -189,13 +312,14 @@ TEST(ConcurrencyTest, DISABLED_ManyConcurrentConnections) {
// Note that without the fix for IMPALA-4135, this test won't always fail, depending on
// the hardware that it is run on.
int port = GetServerPort();
ThriftServer* server = new ThriftServer("DummyServer", MakeProcessor(), port);
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyServer", MakeProcessor(), port).Build(&server));
ASSERT_OK(server->Start());

ThreadPool<int64_t> pool(
"group", "test", 256, 10000, [port](int tid, const int64_t& item) {
using Client = ThriftClient<ImpalaInternalServiceClient>;
Client* client = new Client("127.0.0.1", port, "", NULL, false);
Client* client = new Client("127.0.0.1", port, "", nullptr, false);
Status status = client->Open();
ASSERT_OK(status);
});
Expand All @@ -204,13 +328,16 @@ TEST(ConcurrencyTest, DISABLED_ManyConcurrentConnections) {
}

TEST(NoPasswordPemFile, BadServerCertificate) {
ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(),
FLAGS_state_store_port + 5, NULL, NULL, 5);
EXPECT_OK(server->EnableSsl(BAD_SERVER_CERT, BAD_PRIVATE_KEY));
EXPECT_OK(server->Start());
FLAGS_ssl_client_ca_certificate = SERVER_CERT;
int port = GetServerPort();
ThriftServer* server;
EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
.ssl(BAD_SERVER_CERT, BAD_PRIVATE_KEY)
.Build(&server));
ASSERT_OK(server->Start());

auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, SERVER_CERT);
ThriftClient<StatestoreServiceClientWrapper> ssl_client(
"localhost", FLAGS_state_store_port + 5, "", NULL, true);
"localhost", port, "", nullptr, true);
EXPECT_OK(ssl_client.Open());
TRegisterSubscriberResponse resp;
bool send_done = false;
Expand Down

0 comments on commit 68df21b

Please sign in to comment.