kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: KUDU-1955 refuse to use world-readable keytabs
Date Fri, 23 Jun 2017 21:22:09 GMT
Repository: kudu
Updated Branches:
  refs/heads/master f9e51ca63 -> 5121df65e


KUDU-1955 refuse to use world-readable keytabs

Allowing users to supply keytab files and TLS private keys
with world-readable permissions lessens a cluster's security.
During Kerberos/TLS initialization, servers now check the
permissions of these files and exit with bad statuses if they
have world-readable permissions. Additionally, if users wish
to override this safeguard, they may set the flag
'--allow_world_readable_credentials' to true. However, this
flag is tagged as unsafe.

Change-Id: Ic2ee84e71023304f0743ba0ad37ebb1eef24abc6
Reviewed-on: http://gerrit.cloudera.org:8080/7249
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <danburkert@apache.org>


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

Branch: refs/heads/master
Commit: 5121df65e1b0edceb361f3d8cace240520da6949
Parents: f9e51ca
Author: Sam Okrent <samuel.okrent@cloudera.com>
Authored: Tue Jun 20 17:32:55 2017 -0700
Committer: Dan Burkert <danburkert@apache.org>
Committed: Fri Jun 23 21:21:46 2017 +0000

----------------------------------------------------------------------
 src/kudu/integration-tests/security-itest.cc | 63 ++++++++++++++++++++++-
 src/kudu/rpc/messenger.cc                    | 20 +++++++
 src/kudu/security/init.cc                    | 33 ++++++++++--
 src/kudu/util/env.h                          |  5 ++
 src/kudu/util/env_posix.cc                   | 12 +++++
 5 files changed, 128 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/5121df65/src/kudu/integration-tests/security-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc
index 9688afc..bb0fd1f 100644
--- a/src/kudu/integration-tests/security-itest.cc
+++ b/src/kudu/integration-tests/security-itest.cc
@@ -15,15 +15,25 @@
 // specific language governing permissions and limitations
 // under the License.
 
+#include <sys/stat.h>
+#include <sys/types.h>
+
 #include <memory>
+#include <string>
+#include <vector>
 
 #include "kudu/client/client.h"
 #include "kudu/client/client-test-util.h"
-#include "kudu/master/master.proxy.h"
+#include "kudu/gutil/strings/substitute.h"
 #include "kudu/integration-tests/external_mini_cluster.h"
-#include "kudu/tablet/key_value_test_schema.h"
+#include "kudu/master/master.proxy.h"
 #include "kudu/rpc/messenger.h"
 #include "kudu/server/server_base.proxy.h"
+#include "kudu/tablet/key_value_test_schema.h"
+#include "kudu/util/env.h"
+#include "kudu/util/path_util.h"
+#include "kudu/util/subprocess.h"
+#include "kudu/util/test_macros.h"
 #include "kudu/util/test_util.h"
 
 using kudu::client::KuduClient;
@@ -34,6 +44,13 @@ using kudu::client::KuduTable;
 using kudu::client::KuduTableCreator;
 using kudu::rpc::Messenger;
 using std::unique_ptr;
+using std::vector;
+using strings::Substitute;
+
+DECLARE_string(keytab_file);
+DECLARE_string(rpc_private_key_file);
+DECLARE_string(rpc_certificate_file);
+DECLARE_string(rpc_ca_certificate_file);
 
 namespace kudu {
 
@@ -219,4 +236,46 @@ TEST_F(SecurityITest, TestDisableAuthenticationEncryption) {
   NO_FATALS(SmokeTestCluster());
 }
 
+void CreateWorldReadableFile(const string& name) {
+  unique_ptr<RWFile> file;
+  ASSERT_OK(Env::Default()->NewRWFile(name, &file));
+  ASSERT_EQ(chmod(name.c_str(), 0444), 0);
+}
+
+void GetFullBinaryPath(string* binary) {
+  string exe;
+  ASSERT_OK(Env::Default()->GetExecutablePath(&exe));
+  (*binary) = JoinPathSegments(DirName(exe), *binary);
+}
+
+TEST_F(SecurityITest, TestWorldReadableKeytab) {
+  const string credentials_name = GetTestPath("insecure.keytab");
+  NO_FATALS(CreateWorldReadableFile(credentials_name));
+  string binary = "kudu-master";
+  NO_FATALS(GetFullBinaryPath(&binary));
+  const vector<string> argv = { binary, Substitute("--keytab_file=$0", credentials_name)
};
+  string stderr;
+  Status s = Subprocess::Call(argv, "", nullptr, &stderr);
+  ASSERT_STR_CONTAINS(stderr, Substitute(
+      "cannot use keytab file with world-readable permissions: $0",
+      credentials_name));
+}
+
+TEST_F(SecurityITest, TestWorldReadablePrivateKey) {
+  const string credentials_name = GetTestPath("insecure.key");
+  NO_FATALS(CreateWorldReadableFile(credentials_name));
+  string binary = "kudu-master";
+  NO_FATALS(GetFullBinaryPath(&binary));
+  const vector<string> argv = { binary,
+                                "--unlock_experimental_flags",
+                                Substitute("--rpc_private_key_file=$0", credentials_name),
+                                "--rpc_certificate_file=fake_file",
+                                "--rpc_ca_certificate_file=fake_file" };
+  string stderr;
+  Status s = Subprocess::Call(argv, "", nullptr, &stderr);
+  ASSERT_STR_CONTAINS(stderr, Substitute(
+      "cannot use private key file with world-readable permissions: $0",
+      credentials_name));
+}
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/5121df65/src/kudu/rpc/messenger.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc
index b5b8dfb..86ea439 100644
--- a/src/kudu/rpc/messenger.cc
+++ b/src/kudu/rpc/messenger.cc
@@ -47,6 +47,7 @@
 #include "kudu/rpc/transfer.h"
 #include "kudu/security/tls_context.h"
 #include "kudu/security/token_verifier.h"
+#include "kudu/util/env.h"
 #include "kudu/util/errno.h"
 #include "kudu/util/flag_tags.h"
 #include "kudu/util/flag_validators.h"
@@ -107,6 +108,7 @@ DEFINE_int32(rpc_default_keepalive_time_ms, 65000,
 TAG_FLAG(rpc_default_keepalive_time_ms, advanced);
 
 DECLARE_string(keytab_file);
+DECLARE_bool(allow_world_readable_credentials);
 
 namespace kudu {
 namespace rpc {
@@ -190,6 +192,23 @@ static bool ValidateExternalPkiFlags() {
     return false;
   }
 
+  if (has_key && !FLAGS_allow_world_readable_credentials) {
+    bool world_readable_private_key;
+    Status s = Env::Default()->IsFileWorldReadable(FLAGS_rpc_private_key_file,
+                                                   &world_readable_private_key);
+    if (!s.ok()) {
+      LOG(ERROR) << Substitute("$0: could not verify private key file does not have
"
+                               "world-readable permissions: $1",
+                               FLAGS_rpc_private_key_file, s.ToString());
+      return false;
+    }
+    if (world_readable_private_key) {
+      LOG(ERROR) << "cannot use private key file with world-readable permissions: "
+                 << FLAGS_rpc_private_key_file;
+      return false;
+    }
+  }
+
   return true;
 }
 GROUP_FLAG_VALIDATOR(external_pki_flags, ValidateExternalPkiFlags);
@@ -265,6 +284,7 @@ Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) {
     if (!FLAGS_rpc_certificate_file.empty()) {
       CHECK(!FLAGS_rpc_private_key_file.empty());
       CHECK(!FLAGS_rpc_ca_certificate_file.empty());
+
       // TODO(KUDU-1920): should we try and enforce that the server
       // is in the subject or alt names of the cert?
       RETURN_NOT_OK(tls_context->LoadCertificateAuthority(FLAGS_rpc_ca_certificate_file));

http://git-wip-us.apache.org/repos/asf/kudu/blob/5121df65/src/kudu/security/init.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/init.cc b/src/kudu/security/init.cc
index dfdb5cd..cb1569c 100644
--- a/src/kudu/security/init.cc
+++ b/src/kudu/security/init.cc
@@ -25,13 +25,15 @@
 #include <mutex>
 #include <random>
 #include <string>
-#include <vector>
 
 #include <boost/optional.hpp>
 
+#include "kudu/gutil/strings/substitute.h"
 #include "kudu/gutil/strings/util.h"
-#include "kudu/util/flags.h"
+#include "kudu/util/env.h"
 #include "kudu/util/flag_tags.h"
+#include "kudu/util/flag_validators.h"
+#include "kudu/util/flags.h"
 #include "kudu/util/net/net_util.h"
 #include "kudu/util/rw_mutex.h"
 #include "kudu/util/scoped_cleanup.h"
@@ -53,12 +55,17 @@ TAG_FLAG(principal, experimental);
 // See KUDU-1884.
 TAG_FLAG(principal, unsafe);
 
+DEFINE_bool(allow_world_readable_credentials, false,
+            "Enable the use of keytab files and TLS private keys with "
+            "world-readable permissions.");
+TAG_FLAG(allow_world_readable_credentials, unsafe);
+
 using std::mt19937;
 using std::random_device;
 using std::string;
 using std::uniform_int_distribution;
 using std::uniform_real_distribution;
-using std::vector;
+using strings::Substitute;
 
 namespace kudu {
 namespace security {
@@ -67,6 +74,26 @@ namespace {
 
 class KinitContext;
 
+bool ValidateKeytabPermissions() {
+  if (!FLAGS_keytab_file.empty() && !FLAGS_allow_world_readable_credentials) {
+    bool world_readable_keytab;
+    Status s = Env::Default()->IsFileWorldReadable(FLAGS_keytab_file, &world_readable_keytab);
+    if (!s.ok()) {
+      LOG(ERROR) << Substitute("$0: could not verify keytab file does not have world-readable
"
+                               "permissions: $1", FLAGS_keytab_file, s.ToString());
+      return false;
+    }
+    if (world_readable_keytab) {
+      LOG(ERROR) << "cannot use keytab file with world-readable permissions: "
+                 << FLAGS_keytab_file;
+      return false;
+    }
+  }
+
+  return true;
+}
+GROUP_FLAG_VALIDATOR(keytab_permissions, &ValidateKeytabPermissions);
+
 // Global context for usage of the Krb5 library.
 krb5_context g_krb5_ctx;
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/5121df65/src/kudu/util/env.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/env.h b/src/kudu/util/env.h
index 7f06c4e..443e295 100644
--- a/src/kudu/util/env.h
+++ b/src/kudu/util/env.h
@@ -328,6 +328,11 @@ class Env {
   // be changed.
   virtual Status EnsureFileModeAdheresToUmask(const std::string& path) = 0;
 
+  // Checks whether the given path has world-readable permissions.
+  //
+  // On success, 'result' contains the answer. On failure, 'result' is unset.
+  virtual Status IsFileWorldReadable(const std::string& path, bool* result) = 0;
+
   // Special string injected into file-growing operations' random failures
   // (if enabled).
   //

http://git-wip-us.apache.org/repos/asf/kudu/blob/5121df65/src/kudu/util/env_posix.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/env_posix.cc b/src/kudu/util/env_posix.cc
index 9146454..966e80e 100644
--- a/src/kudu/util/env_posix.cc
+++ b/src/kudu/util/env_posix.cc
@@ -1561,6 +1561,18 @@ class PosixEnv : public Env {
     return Status::OK();
   }
 
+  Status IsFileWorldReadable(const string& path, bool* result) override {
+    ThreadRestrictions::AssertIOAllowed();
+    TRACE_EVENT1("io", "PosixEnv::IsFileWorldReadable", "path", path);
+    MAYBE_RETURN_EIO(path, IOError(Env::kInjectedFailureStatusMsg, EIO));
+    struct stat s;
+    if (stat(path.c_str(), &s) != 0) {
+      return IOError("stat", errno);
+    }
+    *result = (s.st_mode & S_IROTH) != 0;
+    return Status::OK();
+  }
+
  private:
   // unique_ptr Deleter implementation for fts_close
   struct FtsCloser {


Mime
View raw message