kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [1/2] kudu git commit: [security] security-flags
Date Wed, 22 Feb 2017 17:59:27 GMT
Repository: kudu
Updated Branches:
  refs/heads/master dc454dcd0 -> dc1e45a9a


[security]  security-flags

This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:

--rpc_authentication
    This is a new flag which will apply to Kudu servers and the kudu
    command line tool which will allow operators to configure a policy
    for authentication of RPC connections. The possible values are
    'enabled', 'disabled', and 'required'. Three states are necessary
    (as opposed to just 'disabled' and 'required') in order to provide a
    graceful upgrade path for clusters from unsecured to secured.
    'enabled' is the default. A follow up commit will hook this flag
    into the RPC system to ensure that authentication is enforced as
    necessary.

--rpc_encryption
    This is a new flag which will apply to Kudu servers and the 'kudu'
    command line tool which allows operators to configure a policy for
    encryption on RPC connections. This is a tristate flag for the same
    reasons as outlined in --rpc_authentication. A follow up commit will
    hook this flag into the RPC system to ensure that encryption is
    enforced as necessary.

--server_require_kerberos
    This flag has been removed, and in it's place the --keytab and
    --rpc_authentication=required flags are provided. --keytab is used
    to enable Kerberos authentication on a server, and
    --rpc_authentication=required is used to ensure that all RPCs use
    authentication.

--rpc_certificate_file
--rpc_ssl_server_certificate
    --rpc_certificate_file is replacing --rpc_ssl_server_certificate.
    The latter has a few issues. 1) It's not strictly a server flag, it
    also applies to the kudu CLI tool. 3) It's not consistent with the
    similar --webserver_certificate_file flag.

--rpc_private_key_file
--rpc_ssl_private_key
    --rpc_private_key_file is replacing --rpc_ssl_private_key. Same
    reasons as --rpc_cert.

--rpc_ca_certificate_file
--rpc_ssl_certificate_authority
    --rpc_ca_certificate_file is replacing
    --rpc_ssl_certificate_authority. Same reasons as --rpc_cert.

Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Reviewed-on: http://gerrit.cloudera.org:8080/6052
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <todd@apache.org>
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/70ea7fb1
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/70ea7fb1
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/70ea7fb1

Branch: refs/heads/master
Commit: 70ea7fb19cbd4474a0c4bc09e84f2f6276a1e917
Parents: dc454dc
Author: Dan Burkert <danburkert@apache.org>
Authored: Thu Feb 16 16:39:35 2017 -0800
Committer: Todd Lipcon <todd@apache.org>
Committed: Wed Feb 22 17:09:28 2017 +0000

----------------------------------------------------------------------
 .../org/apache/kudu/client/MiniKuduCluster.java |   4 +-
 .../integration-tests/external_mini_cluster.cc  |   2 +-
 src/kudu/rpc/messenger.cc                       | 111 ++++++++++++++-----
 src/kudu/rpc/messenger.h                        |  21 ++++
 src/kudu/rpc/negotiation.cc                     |  10 +-
 src/kudu/rpc/rpc-test-base.h                    |  16 +--
 src/kudu/rpc/sasl_common.cc                     |   7 +-
 7 files changed, 122 insertions(+), 49 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java b/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
index fa4fea4..c008b9a 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
@@ -182,7 +182,7 @@ public class MiniKuduCluster implements AutoCloseable {
       if (miniKdc != null) {
         commandLine.add("--keytab=" + keytab);
         commandLine.add("--kerberos_principal=kudu/" + bindHost);
-        commandLine.add("--server_require_kerberos");
+        commandLine.add("--rpc_authentication=required");
       }
 
       commandLine.addAll(extraTserverFlags);
@@ -263,7 +263,7 @@ public class MiniKuduCluster implements AutoCloseable {
       if (miniKdc != null) {
         commandLine.add("--keytab=" + keytab);
         commandLine.add("--kerberos_principal=kudu/" + bindHost);
-        commandLine.add("--server_require_kerberos");
+        commandLine.add("--rpc_authentication=required");
       }
 
       commandLine.addAll(extraMasterFlags);

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/integration-tests/external_mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/external_mini_cluster.cc b/src/kudu/integration-tests/external_mini_cluster.cc
index 4c6dfb1..d6c3d97 100644
--- a/src/kudu/integration-tests/external_mini_cluster.cc
+++ b/src/kudu/integration-tests/external_mini_cluster.cc
@@ -606,7 +606,7 @@ Status ExternalDaemon::EnableKerberos(MiniKdc* kdc, const string&
bind_host) {
   extra_env_ = kdc->GetEnvVars();
   extra_flags_.push_back(Substitute("--keytab=$0", ktpath));
   extra_flags_.push_back(Substitute("--kerberos_principal=$0", spn));
-  extra_flags_.push_back("--server_require_kerberos");
+  extra_flags_.push_back("--rpc_authentication=required");
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/rpc/messenger.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc
index 591e7d0..5ba0141 100644
--- a/src/kudu/rpc/messenger.cc
+++ b/src/kudu/rpc/messenger.cc
@@ -22,6 +22,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include <boost/algorithm/string/predicate.hpp>
 #include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <list>
@@ -59,31 +60,50 @@ using std::string;
 using std::shared_ptr;
 using strings::Substitute;
 
-DEFINE_string(rpc_ssl_server_certificate, "", "Path to the SSL certificate to be used for
the RPC "
-    "layer.");
-DEFINE_string(rpc_ssl_private_key, "",
-    "Path to the private key to be used to complement the public key present in "
-    "--ssl_server_certificate");
-DEFINE_string(rpc_ssl_certificate_authority, "",
-    "Path to the certificate authority to be used by the client side of the connection to
verify "
-    "the validity of the certificate presented by the server.");
+DEFINE_string(rpc_authentication, "optional",
+              "Whether to require RPC connections to authenticate. Must be one "
+              "of 'disabled', 'optional', or 'required'. If 'optional', "
+              "authentication will be used when the remote end supports it. If "
+              "'required', connections which are not able to authenticate "
+              "(because the remote end lacks support) are rejected. Secure "
+              "clusters should use 'required'.");
+DEFINE_string(rpc_encryption, "optional",
+              "Whether to require RPC connections to be encrypted. Must be one "
+              "of 'disabled', 'optional', or 'required'. If 'optional', "
+              "encryption will be used when the remote end supports it. If "
+              "'required', connections which are not able to use encryption "
+              "(because the remote end lacks support) are rejected. If 'disabled', "
+              "encryption will not be used, and RPC authentication "
+              "(--rpc_authentication) must also be disabled as well. "
+              "Secure clusters should use 'required'.");
+TAG_FLAG(rpc_authentication, stable);
+TAG_FLAG(rpc_encryption, stable);
+
+DEFINE_string(rpc_certificate_file, "",
+              "Path to a PEM encoded X509 certificate to use for securing RPC "
+              "connections with SSL/TLS. If set, '--rpc_private_key_file' and "
+              "'--rpc_ca_certificate_file' must be set as well.");
+DEFINE_string(rpc_private_key_file, "",
+              "Path to a PEM encoded private key paired with the certificate "
+              "from '--rpc_certificate_file'");
+DEFINE_string(rpc_ca_certificate_file, "",
+              "Path to the PEM encoded X509 certificate of the trusted external "
+              "certificate authority. The provided certificate should be the root "
+              "issuer of the certificate passed in '--rpc_certificate_file'.");
+
+// Setting TLS certs and keys via CLI flags is only necessary for external
+// PKI-based security, which is not yet production ready. Instead, see
+// internal PKI (ipki) and Kerberos-based authentication.
+TAG_FLAG(rpc_certificate_file, experimental);
+TAG_FLAG(rpc_private_key_file, experimental);
+TAG_FLAG(rpc_ca_certificate_file, experimental);
 
 DEFINE_int32(rpc_default_keepalive_time_ms, 65000,
              "If an RPC connection from a client is idle for this amount of time, the server
"
              "will disconnect the client.");
-
-TAG_FLAG(rpc_ssl_server_certificate, experimental);
-TAG_FLAG(rpc_ssl_private_key, experimental);
-TAG_FLAG(rpc_ssl_certificate_authority, experimental);
 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);
+DECLARE_string(keytab);
 
 namespace kudu {
 namespace rpc {
@@ -145,17 +165,52 @@ Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) {
       new_msgr->AllExternalReferencesDropped();
   });
 
+  if (boost::iequals(FLAGS_rpc_authentication, "required")) {
+    new_msgr->authentication_ = RpcAuthentication::REQUIRED;
+  } else if (boost::iequals(FLAGS_rpc_authentication, "optional")) {
+    new_msgr->authentication_ = RpcAuthentication::OPTIONAL;
+  } else if (boost::iequals(FLAGS_rpc_authentication, "disabled")) {
+    new_msgr->authentication_ = RpcAuthentication::DISABLED;
+  } else {
+    return Status::InvalidArgument(
+        "--rpc_authentication flag must be one of 'required', 'optional', or 'disabled'");
+  }
+
+  if (boost::iequals(FLAGS_rpc_encryption, "required")) {
+    new_msgr->encryption_ = RpcEncryption::REQUIRED;
+  } else if (boost::iequals(FLAGS_rpc_encryption, "optional")) {
+    new_msgr->encryption_ = RpcEncryption::OPTIONAL;
+  } else if (boost::iequals(FLAGS_rpc_encryption, "disabled")) {
+    new_msgr->encryption_ = RpcEncryption::DISABLED;
+  } else {
+    return Status::InvalidArgument(
+        "--rpc_encryption flag must be one of 'required', 'optional', or 'disabled'");
+  }
+
+  if (new_msgr->encryption_ == RpcEncryption::DISABLED &&
+      new_msgr->authentication_ != RpcAuthentication::DISABLED) {
+    return Status::InvalidArgument("RPC authentication (--rpc_authentication) must be disabled
"
+                                   "if RPC encryption (--rpc_encryption) is disabled");
+  }
+
   RETURN_NOT_OK(new_msgr->Init());
-  if (enable_inbound_tls_server_uuid_) {
+  if (new_msgr->encryption_ != RpcEncryption::DISABLED && enable_inbound_tls_server_uuid_)
{
     auto* tls_context = new_msgr->mutable_tls_context();
-    if (!FLAGS_rpc_ssl_server_certificate.empty() ||
-        !FLAGS_rpc_ssl_private_key.empty() ||
-        !FLAGS_rpc_ssl_certificate_authority.empty()) {
+
+    if (!FLAGS_rpc_certificate_file.empty() &&
+        !FLAGS_rpc_private_key_file.empty() &&
+        !FLAGS_rpc_ca_certificate_file.empty()) {
+
       // TODO(PKI): should we try and enforce that the server UUID and/or
       // hostname is in the subject or alt names of the cert?
-      RETURN_NOT_OK(tls_context->LoadCertificateAuthority(FLAGS_rpc_ssl_certificate_authority));
-      RETURN_NOT_OK(tls_context->LoadCertificateAndKey(FLAGS_rpc_ssl_server_certificate,
-                                                       FLAGS_rpc_ssl_private_key));
+      RETURN_NOT_OK(tls_context->LoadCertificateAuthority(FLAGS_rpc_ca_certificate_file));
+      RETURN_NOT_OK(tls_context->LoadCertificateAndKey(FLAGS_rpc_certificate_file,
+                                                       FLAGS_rpc_private_key_file));
+    } else if (!FLAGS_rpc_certificate_file.empty() ||
+               !FLAGS_rpc_private_key_file.empty() ||
+               !FLAGS_rpc_ca_certificate_file.empty()) {
+      return Status::InvalidArgument("--rpc_certificate_file, --rpc_private_key_file, and
"
+                                     "--rpc_ca_certificate_file flags must be set as a group");
     } else {
       RETURN_NOT_OK(tls_context->GenerateSelfSignedCertAndKey(*enable_inbound_tls_server_uuid_));
     }
@@ -213,7 +268,7 @@ Status Messenger::AddAcceptorPool(const Sockaddr &accept_addr,
   // 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) {
+  if (!FLAGS_keytab.empty()) {
     RETURN_NOT_OK_PREPEND(ServerNegotiation::PreflightCheckGSSAPI(),
                           "GSSAPI/Kerberos not properly configured");
   }
@@ -292,6 +347,8 @@ void Messenger::RegisterInboundSocket(Socket *new_socket, const Sockaddr
&remote
 Messenger::Messenger(const MessengerBuilder &bld)
   : name_(bld.name_),
     closing_(false),
+    authentication_(RpcAuthentication::REQUIRED),
+    encryption_(RpcEncryption::REQUIRED),
     tls_context_(new security::TlsContext()),
     token_verifier_(new security::TokenVerifier()),
     rpcz_store_(new RpczStore()),

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/rpc/messenger.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/messenger.h b/src/kudu/rpc/messenger.h
index c232bf5..89dd933 100644
--- a/src/kudu/rpc/messenger.h
+++ b/src/kudu/rpc/messenger.h
@@ -74,6 +74,20 @@ struct AcceptorPoolInfo {
   Sockaddr bind_address_;
 };
 
+// Authentication configuration for RPC connections.
+enum class RpcAuthentication {
+  DISABLED,
+  OPTIONAL,
+  REQUIRED,
+};
+
+// Encryption configuration for RPC connections.
+enum class RpcEncryption {
+  DISABLED,
+  OPTIONAL,
+  REQUIRED,
+};
+
 // Used to construct a Messenger.
 class MessengerBuilder {
  public:
@@ -252,6 +266,13 @@ class Messenger {
 
   bool closing_;
 
+  // Whether to require authentication and encryption on the connections managed
+  // by this messenger.
+  // TODO(PKI): scope these to individual proxies, so that messengers can be
+  // reused by different clients.
+  RpcAuthentication authentication_;
+  RpcEncryption encryption_;
+
   // Pools which are listening on behalf of this messenger.
   // Note that the user may have called Shutdown() on one of these
   // pools, so even though we retain the reference, it may no longer

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/rpc/negotiation.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/negotiation.cc b/src/kudu/rpc/negotiation.cc
index f303179..e044c98 100644
--- a/src/kudu/rpc/negotiation.cc
+++ b/src/kudu/rpc/negotiation.cc
@@ -53,7 +53,7 @@ DEFINE_int32(rpc_negotiation_inject_delay_ms, 0,
              "the RPC negotiation process on the server side.");
 TAG_FLAG(rpc_negotiation_inject_delay_ms, unsafe);
 
-DECLARE_bool(server_require_kerberos);
+DECLARE_string(keytab);
 
 DEFINE_bool(rpc_encrypt_loopback_connections, false,
             "Whether to encrypt data transfer on RPC connections that stay within "
@@ -208,10 +208,12 @@ static Status DoServerNegotiation(Connection* conn, const MonoTime&
deadline) {
                                        &messenger->tls_context(),
                                        &messenger->token_verifier());
 
-  if (FLAGS_server_require_kerberos) {
-    RETURN_NOT_OK(server_negotiation.EnableGSSAPI());
-  } else {
+  // TODO(PKI): this should be enabling PLAIN if authn < required, and GSSAPI if
+  // there is a keytab and authn > disabled. Same with client version.
+  if (FLAGS_keytab.empty()) {
     RETURN_NOT_OK(server_negotiation.EnablePlain());
+  } else {
+    RETURN_NOT_OK(server_negotiation.EnableGSSAPI());
   }
   server_negotiation.set_deadline(deadline);
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/rpc/rpc-test-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc-test-base.h b/src/kudu/rpc/rpc-test-base.h
index 5bdacee..47f209f 100644
--- a/src/kudu/rpc/rpc-test-base.h
+++ b/src/kudu/rpc/rpc-test-base.h
@@ -50,10 +50,6 @@
 #include "kudu/util/test_util.h"
 #include "kudu/util/trace.h"
 
-DECLARE_string(rpc_ssl_server_certificate);
-DECLARE_string(rpc_ssl_private_key);
-DECLARE_string(rpc_ssl_certificate_authority);
-
 namespace kudu { namespace rpc {
 
 using kudu::rpc_test::AddRequestPB;
@@ -367,16 +363,12 @@ class RpcTestBase : public KuduTest {
   std::shared_ptr<Messenger> CreateMessenger(const string &name,
                                              int n_reactors = 1,
                                              bool enable_ssl = false) {
+    MessengerBuilder bld(name);
+
     if (enable_ssl) {
-      std::string server_cert_path = GetTestPath("server-cert.pem");
-      std::string private_key_path = GetTestPath("server-key.pem");
-      CHECK_OK(security::CreateSSLServerCert(server_cert_path));
-      CHECK_OK(security::CreateSSLPrivateKey(private_key_path));
-      FLAGS_rpc_ssl_server_certificate = server_cert_path;
-      FLAGS_rpc_ssl_private_key = private_key_path;
-      FLAGS_rpc_ssl_certificate_authority = server_cert_path;
+      bld.enable_inbound_tls(name);
     }
-    MessengerBuilder bld(name);
+
     bld.set_num_reactors(n_reactors);
     bld.set_connection_keepalive_time(
       MonoDelta::FromMilliseconds(keepalive_time_ms_));

http://git-wip-us.apache.org/repos/asf/kudu/blob/70ea7fb1/src/kudu/rpc/sasl_common.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/sasl_common.cc b/src/kudu/rpc/sasl_common.cc
index a0ae7e1..c852675 100644
--- a/src/kudu/rpc/sasl_common.cc
+++ b/src/kudu/rpc/sasl_common.cc
@@ -42,7 +42,7 @@
 
 using std::set;
 
-DECLARE_bool(server_require_kerberos);
+DECLARE_string(keytab);
 
 namespace kudu {
 namespace rpc {
@@ -318,9 +318,10 @@ Status WrapSaslCall(sasl_conn_t* conn, const std::function<int()>&
call) {
   g_auth_failure_capture = &err;
 
   // Take the 'kerberos_reinit_lock' here to avoid a possible race with ticket renewal.
-  if (FLAGS_server_require_kerberos) kudu::security::KerberosReinitLock()->ReadLock();
+  bool kerberos_supported = !FLAGS_keytab.empty();
+  if (kerberos_supported) kudu::security::KerberosReinitLock()->ReadLock();
   int rc = call();
-  if (FLAGS_server_require_kerberos) kudu::security::KerberosReinitLock()->ReadUnlock();
+  if (kerberos_supported) kudu::security::KerberosReinitLock()->ReadUnlock();
   g_auth_failure_capture = nullptr;
 
   switch (rc) {


Mime
View raw message