rpc: add a pre-flight check for server GSSAPI setup This adds a pre-flight check that runs when adding an acceptor pool to a messenger (which indicates that the messenger will act as a server). The pre-flight check runs through a partial SASL GSSAPI authentication flow in order to verify that the plugin is set up and a keytab is available.
Tested this manually as well as with a new unit test. Change-Id: Ia26482003144504dee77ffb2e9de7ebe3c1e2e3d Reviewed-on: http://gerrit.cloudera.org:8080/4909 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/d5d9afbf Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d5d9afbf Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d5d9afbf Branch: refs/heads/master Commit: d5d9afbfc7a7aef375494f2f2971d30312b82a20 Parents: e6cb2ef Author: Todd Lipcon <[email protected]> Authored: Tue Nov 1 17:04:35 2016 -0700 Committer: Todd Lipcon <[email protected]> Committed: Wed Nov 2 20:31:20 2016 +0000 ---------------------------------------------------------------------- src/kudu/rpc/connection.cc | 11 +---------- src/kudu/rpc/messenger.cc | 16 ++++++++++++++++ src/kudu/rpc/messenger.h | 4 ++++ src/kudu/rpc/sasl_rpc-test.cc | 31 +++++++++++++++++++++++++++++++ src/kudu/rpc/sasl_server.cc | 38 ++++++++++++++++++++++++++++++++++++++ src/kudu/rpc/sasl_server.h | 4 ++++ 6 files changed, 94 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/connection.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/connection.cc b/src/kudu/rpc/connection.cc index f134d7c..e074ec0 100644 --- a/src/kudu/rpc/connection.cc +++ b/src/kudu/rpc/connection.cc @@ -53,13 +53,7 @@ using std::shared_ptr; using std::vector; using strings::Substitute; -DEFINE_bool(server_require_kerberos, false, - "Whether to force all inbound RPC connections to authenticate " - "with Kerberos"); -// TODO(todd): this flag is too coarse-grained, since secure servers still -// need to allow non-kerberized connections authenticated by tokens. But -// it's a useful stop-gap. -TAG_FLAG(server_require_kerberos, experimental); +DECLARE_bool(server_require_kerberos); namespace kudu { namespace rpc { @@ -659,9 +653,6 @@ Status Connection::InitSaslClient() { Status Connection::InitSaslServer() { if (FLAGS_server_require_kerberos) { - // TODO(todd): should we use krb5 APIs directly to verify that we have a valid - // keytab when we start up, rather than starting up and then rejecting - // connections? RETURN_NOT_OK(sasl_server().EnableGSSAPI()); } else { RETURN_NOT_OK(sasl_server().EnablePlain()); http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc index 501191d..4f1f0e3 100644 --- a/src/kudu/rpc/messenger.cc +++ b/src/kudu/rpc/messenger.cc @@ -60,6 +60,14 @@ DEFINE_int32(rpc_default_keepalive_time_ms, 65000, "will disconnect the client."); TAG_FLAG(rpc_default_keepalive_time_ms, advanced); +DEFINE_bool(server_require_kerberos, false, + "Whether to force all inbound RPC connections to authenticate " + "with Kerberos"); +// TODO(todd): this flag is too coarse-grained, since secure servers still +// need to allow non-kerberized connections authenticated by tokens. But +// it's a useful stop-gap. +TAG_FLAG(server_require_kerberos, experimental); + namespace kudu { namespace rpc { @@ -158,6 +166,14 @@ void Messenger::Shutdown() { Status Messenger::AddAcceptorPool(const Sockaddr &accept_addr, shared_ptr<AcceptorPool>* pool) { + // Before listening, if we expect to require Kerberos, we want to verify + // 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_server_require_kerberos) { + RETURN_NOT_OK_PREPEND(SaslServer::PreflightCheckGSSAPI(kSaslAppName), + "GSSAPI/Kerberos not properly configured"); + } + Socket sock; RETURN_NOT_OK(sock.Init(0)); RETURN_NOT_OK(sock.SetReuseAddr(true)); http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/messenger.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.h b/src/kudu/rpc/messenger.h index 1424913..a7ac1ae 100644 --- a/src/kudu/rpc/messenger.h +++ b/src/kudu/rpc/messenger.h @@ -139,6 +139,10 @@ class Messenger { // // NOTE: the returned pool is not initially started. You must call // pool->Start(...) to begin accepting connections. + // + // If Kerberos is enabled, this also runs a pre-flight check that makes + // sure the environment is appropriately configured to authenticate + // clients via Kerberos. If not, this returns a RuntimeError. Status AddAcceptorPool(const Sockaddr &accept_addr, std::shared_ptr<AcceptorPool>* pool); http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/sasl_rpc-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_rpc-test.cc b/src/kudu/rpc/sasl_rpc-test.cc index 5458345..2786d89 100644 --- a/src/kudu/rpc/sasl_rpc-test.cc +++ b/src/kudu/rpc/sasl_rpc-test.cc @@ -18,6 +18,7 @@ #include "kudu/rpc/rpc-test-base.h" #include <stdlib.h> +#include <sys/stat.h> #include <functional> #include <memory> @@ -339,6 +340,36 @@ TEST_F(TestSaslRpc, TestGSSAPINegotiation) { } +// Test that the pre-flight check for servers requiring Kerberos provides +// nice error messages for missing or bad keytabs. +TEST_F(TestSaslRpc, TestPreflight) { + // Try pre-flight with no keytab. + Status s = SaslServer::PreflightCheckGSSAPI(kSaslAppName); + ASSERT_STR_MATCHES(s.ToString(), "Key table file.*not found"); + + // Try with a valid krb5 environment and keytab. + MiniKdc kdc; + ASSERT_OK(kdc.Start()); + ASSERT_OK(kdc.SetKrb5Environment()); + string kt_path; + ASSERT_OK(kdc.CreateServiceKeytab("kudu/127.0.0.1", &kt_path)); + CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/)); + + ASSERT_OK(SaslServer::PreflightCheckGSSAPI(kSaslAppName)); + + // Try with an inaccessible keytab. + CHECK_ERR(chmod(kt_path.c_str(), 0000)); + s = SaslServer::PreflightCheckGSSAPI(kSaslAppName); + ASSERT_STR_MATCHES(s.ToString(), "error accessing keytab: Permission denied"); + CHECK_ERR(unlink(kt_path.c_str())); + + // 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 = SaslServer::PreflightCheckGSSAPI(kSaslAppName); + ASSERT_STR_MATCHES(s.ToString(), "No key table entry found matching kudu/.*"); +} + //////////////////////////////////////////////////////////////////////////////// static void RunTimeoutExpectingServer(Socket* conn) { http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/sasl_server.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.cc b/src/kudu/rpc/sasl_server.cc index c8e5fc0..9a6523e 100644 --- a/src/kudu/rpc/sasl_server.cc +++ b/src/kudu/rpc/sasl_server.cc @@ -470,5 +470,43 @@ int SaslServer::PlainAuthCb(sasl_conn_t * /*conn*/, const char * /*user*/, const return SASL_OK; } +Status SaslServer::PreflightCheckGSSAPI(const string& app_name) { + // Initialize a SaslServer with a bogus socket fd, and enable + // only GSSAPI. + // + // We aren't going to actually send/receive any messages, but + // this makes it easier to reuse the initialization code. + SaslServer server(app_name, -1); + Status s = server.EnableGSSAPI(); + if (!s.ok()) { + return Status::RuntimeError(s.message()); + } + + RETURN_NOT_OK(server.Init(app_name)); + + // Start the SASL server as if we were accepting a connection. + const char* server_out = nullptr; // ignored + uint32_t server_out_len = 0; + s = WrapSaslCall(server.sasl_conn_.get(), [&]() { + return sasl_server_start( + server.sasl_conn_.get(), + kSaslMechGSSAPI, + "", 0, // Pass a 0-length token. + &server_out, &server_out_len); + }); + + // We expect 'Incomplete' status to indicate that the first step of negotiation + // was correct. + if (s.IsIncomplete()) return Status::OK(); + + string err_msg = s.message().ToString(); + if (err_msg == "Permission denied") { + // For bad keytab permissions, we get a rather vague message. So, + // we make it more specific for better usability. + err_msg = "error accessing keytab: " + err_msg; + } + return Status::RuntimeError(err_msg); +} + } // namespace rpc } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/d5d9afbf/src/kudu/rpc/sasl_server.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.h b/src/kudu/rpc/sasl_server.h index 0f5946f..a05725c 100644 --- a/src/kudu/rpc/sasl_server.h +++ b/src/kudu/rpc/sasl_server.h @@ -112,6 +112,10 @@ class SaslServer { int PlainAuthCb(sasl_conn_t* conn, const char* user, const char* pass, unsigned passlen, struct propctx* propctx); + // Perform a "pre-flight check" that everything required to act as a Kerberos + // server is properly set up. + static Status PreflightCheckGSSAPI(const std::string& app_name); + private: // Parse and validate connection header. Status ValidateConnectionHeader(faststring* recv_buf);
