You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/11/02 21:39:46 UTC
[4/6] kudu git commit: rpc: add a pre-flight check for server GSSAPI
setup
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 <to...@apache.org>
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 <to...@apache.org>
Authored: Tue Nov 1 17:04:35 2016 -0700
Committer: Todd Lipcon <to...@apache.org>
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);