You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2017/06/23 21:22:09 UTC
kudu git commit: KUDU-1955 refuse to use world-readable keytabs
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 <da...@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 <sa...@cloudera.com>
Authored: Tue Jun 20 17:32:55 2017 -0700
Committer: Dan Burkert <da...@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 {