Allow the SASL protocol service name to be configurable Previously the SASL service name was always set to a constant "kudu" which was tracked by kSaslProtoName in rpc/constants.h.
However, for applications that use the KRPC library that would prefer to do their own SASL initialization, they would requre to set their own SASL service name to be passed into sasl_server_new()/sasl_client_new(). This patch allows for this configuration by adding a configurable parameter to the MessengerBuilder which is ultimately passed down to the negotiation layer. Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Reviewed-on: http://gerrit.cloudera.org:8080/8218 Reviewed-by: Dan Burkert <[email protected]> Tested-by: Dan Burkert <[email protected]> Reviewed-on: http://gerrit.cloudera.org:8080/8230 Reviewed-by: Sailesh Mukil <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/e06268e8 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/e06268e8 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/e06268e8 Branch: refs/heads/master Commit: e06268e829350d4bc390a54b7685457bba6706ba Parents: 048ce2a Author: Sailesh Mukil <[email protected]> Authored: Thu Oct 5 09:46:24 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Oct 19 01:23:32 2017 +0000 ---------------------------------------------------------------------- be/src/kudu/rpc/client_negotiation.cc | 6 ++++-- be/src/kudu/rpc/client_negotiation.h | 6 +++++- be/src/kudu/rpc/constants.cc | 1 - be/src/kudu/rpc/constants.h | 3 --- be/src/kudu/rpc/messenger.cc | 9 ++++++++- be/src/kudu/rpc/messenger.h | 11 +++++++++++ be/src/kudu/rpc/negotiation-test.cc | 26 ++++++++++++++------------ be/src/kudu/rpc/negotiation.cc | 6 ++++-- be/src/kudu/rpc/server_negotiation.cc | 11 +++++++---- be/src/kudu/rpc/server_negotiation.h | 8 ++++++-- 10 files changed, 59 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/client_negotiation.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/client_negotiation.cc b/be/src/kudu/rpc/client_negotiation.cc index 8ce5190..24ef46f 100644 --- a/be/src/kudu/rpc/client_negotiation.cc +++ b/be/src/kudu/rpc/client_negotiation.cc @@ -109,7 +109,8 @@ static Status StatusFromRpcError(const ErrorStatusPB& error) { ClientNegotiation::ClientNegotiation(unique_ptr<Socket> socket, const security::TlsContext* tls_context, const boost::optional<security::SignedTokenPB>& authn_token, - RpcEncryption encryption) + RpcEncryption encryption, + std::string sasl_proto_name) : socket_(std::move(socket)), helper_(SaslHelper::CLIENT), tls_context_(tls_context), @@ -119,6 +120,7 @@ ClientNegotiation::ClientNegotiation(unique_ptr<Socket> socket, psecret_(nullptr, std::free), negotiated_authn_(AuthenticationType::INVALID), negotiated_mech_(SaslMechanism::INVALID), + sasl_proto_name_(std::move(sasl_proto_name)), deadline_(MonoTime::Max()) { callbacks_.push_back(SaslBuildCallback(SASL_CB_GETOPT, reinterpret_cast<int (*)()>(&ClientNegotiationGetoptCb), this)); @@ -286,7 +288,7 @@ Status ClientNegotiation::InitSaslClient() { sasl_conn_t* sasl_conn = nullptr; RETURN_NOT_OK_PREPEND(WrapSaslCall(nullptr /* no conn */, [&]() { return sasl_client_new( - kSaslProtoName, // Registered name of the service using SASL. Required. + sasl_proto_name_.c_str(), // Registered name of the service using SASL. Required. helper_.server_fqdn(), // The fully qualified domain name of the remote server. nullptr, // Local and remote IP address strings. (we don't use nullptr, // any mechanisms which require this info.) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/client_negotiation.h ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/client_negotiation.h b/be/src/kudu/rpc/client_negotiation.h index aa8c6d9..6650ccd 100644 --- a/be/src/kudu/rpc/client_negotiation.h +++ b/be/src/kudu/rpc/client_negotiation.h @@ -61,7 +61,8 @@ class ClientNegotiation { ClientNegotiation(std::unique_ptr<Socket> socket, const security::TlsContext* tls_context, const boost::optional<security::SignedTokenPB>& authn_token, - RpcEncryption encryption); + RpcEncryption encryption, + std::string sasl_proto_name); // Enable PLAIN authentication. // Must be called before Negotiate(). @@ -244,6 +245,9 @@ class ClientNegotiation { // The SASL mechanism used by the connection. Filled in during negotiation. SaslMechanism::Type negotiated_mech_; + // The SASL protocol name that is used for the SASL negotiation. + const std::string sasl_proto_name_; + // Negotiation timeout deadline. MonoTime deadline_; }; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/constants.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/constants.cc b/be/src/kudu/rpc/constants.cc index bcf9985..a4e024c 100644 --- a/be/src/kudu/rpc/constants.cc +++ b/be/src/kudu/rpc/constants.cc @@ -24,7 +24,6 @@ namespace rpc { const char* const kMagicNumber = "hrpc"; const char* const kSaslAppName = "kudu"; -const char* const kSaslProtoName = "kudu"; // NOTE: the TLS flag is dynamically added based on the local encryption // configuration. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/constants.h ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/constants.h b/be/src/kudu/rpc/constants.h index e179ddc..a3c7c67 100644 --- a/be/src/kudu/rpc/constants.h +++ b/be/src/kudu/rpc/constants.h @@ -32,9 +32,6 @@ extern const char* const kMagicNumber; // App name for SASL library init extern const char* const kSaslAppName; -// Network protocol name for SASL library init -extern const char* const kSaslProtoName; - // Current version of the RPC protocol. static const uint32_t kCurrentRpcVersion = 9; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/messenger.cc b/be/src/kudu/rpc/messenger.cc index ad2ba67..91ccc15 100644 --- a/be/src/kudu/rpc/messenger.cc +++ b/be/src/kudu/rpc/messenger.cc @@ -208,6 +208,7 @@ MessengerBuilder::MessengerBuilder(std::string name) min_negotiation_threads_(0), max_negotiation_threads_(4), coarse_timer_granularity_(MonoDelta::FromMilliseconds(100)), + sasl_proto_name_("kudu"), enable_inbound_tls_(false) { } @@ -242,6 +243,11 @@ MessengerBuilder &MessengerBuilder::set_metric_entity( return *this; } +MessengerBuilder &MessengerBuilder::set_sasl_proto_name(std::string sasl_proto_name) { + sasl_proto_name_ = std::move(sasl_proto_name); + return *this; +} + MessengerBuilder& MessengerBuilder::enable_inbound_tls() { enable_inbound_tls_ = true; return *this; @@ -348,7 +354,7 @@ Status Messenger::AddAcceptorPool(const Sockaddr &accept_addr, // that everything is set up correctly. This way we'll generate errors on // startup rather than later on when we first receive a client connection. if (!FLAGS_keytab_file.empty()) { - RETURN_NOT_OK_PREPEND(ServerNegotiation::PreflightCheckGSSAPI(), + RETURN_NOT_OK_PREPEND(ServerNegotiation::PreflightCheckGSSAPI(sasl_proto_name()), "GSSAPI/Kerberos not properly configured"); } @@ -437,6 +443,7 @@ Messenger::Messenger(const MessengerBuilder &bld) token_verifier_(new security::TokenVerifier()), rpcz_store_(new RpczStore()), metric_entity_(bld.metric_entity_), + sasl_proto_name_(bld.sasl_proto_name_), retain_self_(this) { for (int i = 0; i < bld.num_reactors_; i++) { reactors_.push_back(new Reactor(retain_self_, i, bld)); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/messenger.h ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/messenger.h b/be/src/kudu/rpc/messenger.h index d860f90..80d8a2f 100644 --- a/be/src/kudu/rpc/messenger.h +++ b/be/src/kudu/rpc/messenger.h @@ -118,6 +118,9 @@ class MessengerBuilder { // Set metric entity for use by RPC systems. MessengerBuilder &set_metric_entity(const scoped_refptr<MetricEntity>& metric_entity); + // Set the SASL protocol name that is used for the SASL negotiation. + MessengerBuilder &set_sasl_proto_name(std::string sasl_proto_name); + // Configure the messenger to enable TLS encryption on inbound connections. MessengerBuilder& enable_inbound_tls(); @@ -131,6 +134,7 @@ class MessengerBuilder { int max_negotiation_threads_; MonoDelta coarse_timer_granularity_; scoped_refptr<MetricEntity> metric_entity_; + std::string sasl_proto_name_; bool enable_inbound_tls_; }; @@ -248,6 +252,10 @@ class Messenger { scoped_refptr<MetricEntity> metric_entity() const { return metric_entity_.get(); } + const std::string& sasl_proto_name() const { + return sasl_proto_name_; + } + const scoped_refptr<RpcService> rpc_service(const std::string& service_name) const; private: @@ -309,6 +317,9 @@ class Messenger { scoped_refptr<MetricEntity> metric_entity_; + // The SASL protocol name that is used for the SASL negotiation. + const std::string sasl_proto_name_; + // The ownership of the Messenger object is somewhat subtle. The pointer graph // looks like this: // http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/negotiation-test.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/negotiation-test.cc b/be/src/kudu/rpc/negotiation-test.cc index 43b6e60..e4e6c26 100644 --- a/be/src/kudu/rpc/negotiation-test.cc +++ b/be/src/kudu/rpc/negotiation-test.cc @@ -228,11 +228,13 @@ TEST_P(TestNegotiation, TestNegotiation) { ClientNegotiation client_negotiation(std::move(client_socket), &client_tls_context, authn_token, - desc.client.encryption); + desc.client.encryption, + "kudu"); ServerNegotiation server_negotiation(std::move(server_socket), &server_tls_context, &token_verifier, - desc.server.encryption); + desc.server.encryption, + "kudu"); // Set client and server SASL mechanisms. MiniKdc kdc; @@ -1011,7 +1013,7 @@ static void RunGSSAPINegotiationServer(unique_ptr<Socket> socket, CHECK_OK(tls_context.Init()); TokenVerifier token_verifier; ServerNegotiation server_negotiation(std::move(socket), &tls_context, - &token_verifier, RpcEncryption::OPTIONAL); + &token_verifier, RpcEncryption::OPTIONAL, "kudu"); server_negotiation.set_server_fqdn("127.0.0.1"); CHECK_OK(server_negotiation.EnableGSSAPI()); post_check(server_negotiation.Negotiate(), server_negotiation); @@ -1024,7 +1026,7 @@ static void RunGSSAPINegotiationClient(unique_ptr<Socket> conn, TlsContext tls_context; CHECK_OK(tls_context.Init()); ClientNegotiation client_negotiation(std::move(conn), &tls_context, - boost::none, RpcEncryption::OPTIONAL); + boost::none, RpcEncryption::OPTIONAL, "kudu"); client_negotiation.set_server_fqdn("127.0.0.1"); CHECK_OK(client_negotiation.EnableGSSAPI()); post_check(client_negotiation.Negotiate(), client_negotiation); @@ -1124,7 +1126,7 @@ TEST_F(TestNegotiation, TestGSSAPIInvalidNegotiation) { // the preflight check passes a 0-length token. TEST_F(TestNegotiation, TestPreflight) { // Try pre-flight with no keytab. - Status s = ServerNegotiation::PreflightCheckGSSAPI(); + Status s = ServerNegotiation::PreflightCheckGSSAPI("kudu"); ASSERT_FALSE(s.ok()); #ifndef KRB5_VERSION_LE_1_10 ASSERT_STR_MATCHES(s.ToString(), "Key table file.*not found"); @@ -1137,11 +1139,11 @@ TEST_F(TestNegotiation, TestPreflight) { ASSERT_OK(kdc.CreateServiceKeytab("kudu/127.0.0.1", &kt_path)); CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/)); - ASSERT_OK(ServerNegotiation::PreflightCheckGSSAPI()); + ASSERT_OK(ServerNegotiation::PreflightCheckGSSAPI("kudu")); // Try with an inaccessible keytab. CHECK_ERR(chmod(kt_path.c_str(), 0000)); - s = ServerNegotiation::PreflightCheckGSSAPI(); + s = ServerNegotiation::PreflightCheckGSSAPI("kudu"); ASSERT_FALSE(s.ok()); #ifndef KRB5_VERSION_LE_1_10 ASSERT_STR_MATCHES(s.ToString(), "error accessing keytab: Permission denied"); @@ -1151,7 +1153,7 @@ TEST_F(TestNegotiation, TestPreflight) { // Try with a keytab that has the wrong credentials. ASSERT_OK(kdc.CreateServiceKeytab("wrong-service/127.0.0.1", &kt_path)); CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/)); - s = ServerNegotiation::PreflightCheckGSSAPI(); + s = ServerNegotiation::PreflightCheckGSSAPI("kudu"); ASSERT_FALSE(s.ok()); #ifndef KRB5_VERSION_LE_1_10 ASSERT_STR_MATCHES(s.ToString(), "No key table entry found matching kudu/.*"); @@ -1166,7 +1168,7 @@ static void RunTimeoutExpectingServer(unique_ptr<Socket> socket) { CHECK_OK(tls_context.Init()); TokenVerifier token_verifier; ServerNegotiation server_negotiation(std::move(socket), &tls_context, - &token_verifier, RpcEncryption::OPTIONAL); + &token_verifier, RpcEncryption::OPTIONAL, "kudu"); CHECK_OK(server_negotiation.EnablePlain()); Status s = server_negotiation.Negotiate(); ASSERT_TRUE(s.IsNetworkError()) << "Expected client to time out and close the connection. Got: " @@ -1177,7 +1179,7 @@ static void RunTimeoutNegotiationClient(unique_ptr<Socket> sock) { TlsContext tls_context; CHECK_OK(tls_context.Init()); ClientNegotiation client_negotiation(std::move(sock), &tls_context, - boost::none, RpcEncryption::OPTIONAL); + boost::none, RpcEncryption::OPTIONAL, "kudu"); CHECK_OK(client_negotiation.EnablePlain("test", "test")); MonoTime deadline = MonoTime::Now() - MonoDelta::FromMilliseconds(100L); client_negotiation.set_deadline(deadline); @@ -1198,7 +1200,7 @@ static void RunTimeoutNegotiationServer(unique_ptr<Socket> socket) { CHECK_OK(tls_context.Init()); TokenVerifier token_verifier; ServerNegotiation server_negotiation(std::move(socket), &tls_context, - &token_verifier, RpcEncryption::OPTIONAL); + &token_verifier, RpcEncryption::OPTIONAL, "kudu"); CHECK_OK(server_negotiation.EnablePlain()); MonoTime deadline = MonoTime::Now() - MonoDelta::FromMilliseconds(100L); server_negotiation.set_deadline(deadline); @@ -1211,7 +1213,7 @@ static void RunTimeoutExpectingClient(unique_ptr<Socket> socket) { TlsContext tls_context; CHECK_OK(tls_context.Init()); ClientNegotiation client_negotiation(std::move(socket), &tls_context, - boost::none, RpcEncryption::OPTIONAL); + boost::none, RpcEncryption::OPTIONAL, "kudu"); CHECK_OK(client_negotiation.EnablePlain("test", "test")); Status s = client_negotiation.Negotiate(); ASSERT_TRUE(s.IsNetworkError()) << "Expected server to time out and close the connection. Got: " http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/negotiation.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/negotiation.cc b/be/src/kudu/rpc/negotiation.cc index f2ee4d7..f0280c6 100644 --- a/be/src/kudu/rpc/negotiation.cc +++ b/be/src/kudu/rpc/negotiation.cc @@ -167,7 +167,8 @@ static Status DoClientNegotiation(Connection* conn, ClientNegotiation client_negotiation(conn->release_socket(), &messenger->tls_context(), authn_token, - encryption); + encryption, + messenger->sasl_proto_name()); client_negotiation.set_server_fqdn(conn->outbound_connection_id().hostname()); @@ -245,7 +246,8 @@ static Status DoServerNegotiation(Connection* conn, ServerNegotiation server_negotiation(conn->release_socket(), &messenger->tls_context(), &messenger->token_verifier(), - encryption); + encryption, + messenger->sasl_proto_name()); if (authentication != RpcAuthentication::DISABLED && !FLAGS_keytab_file.empty()) { RETURN_NOT_OK(server_negotiation.EnableGSSAPI()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/server_negotiation.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/server_negotiation.cc b/be/src/kudu/rpc/server_negotiation.cc index 9a4bd91..9878dd9 100644 --- a/be/src/kudu/rpc/server_negotiation.cc +++ b/be/src/kudu/rpc/server_negotiation.cc @@ -126,7 +126,8 @@ static int ServerNegotiationPlainAuthCb(sasl_conn_t* conn, ServerNegotiation::ServerNegotiation(unique_ptr<Socket> socket, const security::TlsContext* tls_context, const security::TokenVerifier* token_verifier, - RpcEncryption encryption) + RpcEncryption encryption, + std::string sasl_proto_name) : socket_(std::move(socket)), helper_(SaslHelper::SERVER), tls_context_(tls_context), @@ -135,6 +136,7 @@ ServerNegotiation::ServerNegotiation(unique_ptr<Socket> socket, token_verifier_(token_verifier), negotiated_authn_(AuthenticationType::INVALID), negotiated_mech_(SaslMechanism::INVALID), + sasl_proto_name_(std::move(sasl_proto_name)), deadline_(MonoTime::Max()) { callbacks_.push_back(SaslBuildCallback(SASL_CB_GETOPT, reinterpret_cast<int (*)()>(&ServerNegotiationGetoptCb), this)); @@ -256,7 +258,7 @@ Status ServerNegotiation::Negotiate() { return Status::OK(); } -Status ServerNegotiation::PreflightCheckGSSAPI() { +Status ServerNegotiation::PreflightCheckGSSAPI(const std::string& sasl_proto_name) { // TODO(todd): the error messages that come from this function on el6 // are relatively useless due to the following krb5 bug: // http://krbdev.mit.edu/rt/Ticket/Display.html?id=6973 @@ -268,7 +270,8 @@ Status ServerNegotiation::PreflightCheckGSSAPI() { // // We aren't going to actually send/receive any messages, but // this makes it easier to reuse the initialization code. - ServerNegotiation server(nullptr, nullptr, nullptr, RpcEncryption::OPTIONAL); + ServerNegotiation server( + nullptr, nullptr, nullptr, RpcEncryption::OPTIONAL, sasl_proto_name); Status s = server.EnableGSSAPI(); if (!s.ok()) { return Status::RuntimeError(s.message()); @@ -375,7 +378,7 @@ Status ServerNegotiation::InitSaslServer() { RETURN_NOT_OK_PREPEND(WrapSaslCall(nullptr /* no conn */, [&]() { return sasl_server_new( // Registered name of the service using SASL. Required. - kSaslProtoName, + sasl_proto_name_.c_str(), // The fully qualified domain name of this server. helper_.server_fqdn(), // Permits multiple user realms on server. NULL == use default. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e06268e8/be/src/kudu/rpc/server_negotiation.h ---------------------------------------------------------------------- diff --git a/be/src/kudu/rpc/server_negotiation.h b/be/src/kudu/rpc/server_negotiation.h index e9e945a..4cafeab 100644 --- a/be/src/kudu/rpc/server_negotiation.h +++ b/be/src/kudu/rpc/server_negotiation.h @@ -58,7 +58,8 @@ class ServerNegotiation { ServerNegotiation(std::unique_ptr<Socket> socket, const security::TlsContext* tls_context, const security::TokenVerifier* token_verifier, - RpcEncryption encryption); + RpcEncryption encryption, + std::string sasl_proto_name); // Enable PLAIN authentication. // Despite PLAIN authentication taking a username and password, we disregard @@ -140,7 +141,7 @@ class ServerNegotiation { // Perform a "pre-flight check" that everything required to act as a Kerberos // server is properly set up. - static Status PreflightCheckGSSAPI() WARN_UNUSED_RESULT; + static Status PreflightCheckGSSAPI(const std::string& sasl_proto_name) WARN_UNUSED_RESULT; private: @@ -240,6 +241,9 @@ class ServerNegotiation { // authentication type is SASL. SaslMechanism::Type negotiated_mech_; + // The SASL protocol name that is used for the SASL negotiation. + const std::string sasl_proto_name_; + // Negotiation timeout deadline. MonoTime deadline_; };
