This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit abe2a73cdbe6438e34f825594d66aec53a329840
Author: Attila Bukor <abu...@apache.org>
AuthorDate: Thu May 26 17:17:37 2022 +0200
KUDU-3373 Key provider interface
Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.
Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Reviewed-on: http://gerrit.cloudera.org:8080/18568
Reviewed-by: Alexey Serbin <ale...@apache.org>
Tested-by: Attila Bukor <abu...@apache.org>
Reviewed-by: Zoltan Chovan <zcho...@cloudera.com>
---
src/kudu/fs/fs_manager.cc | 26 ++++++++++---
src/kudu/fs/fs_manager.h | 8 ++++
src/kudu/mini-cluster/external_mini_cluster.cc | 11 ++++--
src/kudu/mini-cluster/external_mini_cluster.h | 6 +++
src/kudu/server/CMakeLists.txt | 3 ++
src/kudu/server/default_key_provider-test.cc | 48 +++++++++++++++++++++++
src/kudu/server/default_key_provider.h | 53 ++++++++++++++++++++++++++
src/kudu/server/key_provider.h | 41 ++++++++++++++++++++
src/kudu/tools/tool_action_common.cc | 4 +-
src/kudu/util/env.h | 3 +-
src/kudu/util/env_posix.cc | 2 +-
src/kudu/util/test_util.cc | 2 +-
12 files changed, 193 insertions(+), 14 deletions(-)
diff --git a/src/kudu/fs/fs_manager.cc b/src/kudu/fs/fs_manager.cc
index 56315592d..425fb428a 100644
--- a/src/kudu/fs/fs_manager.cc
+++ b/src/kudu/fs/fs_manager.cc
@@ -49,6 +49,8 @@
#include "kudu/gutil/strings/substitute.h"
#include "kudu/gutil/strings/util.h"
#include "kudu/gutil/walltime.h"
+#include "kudu/server/default_key_provider.h"
+#include "kudu/server/key_provider.h"
#include "kudu/util/env_util.h"
#include "kudu/util/flag_tags.h"
#include "kudu/util/metrics.h"
@@ -144,6 +146,7 @@ using kudu::fs::ReadableBlock;
using kudu::fs::UpdateInstanceBehavior;
using kudu::fs::WritableBlock;
using kudu::pb_util::SecureDebugString;
+using kudu::security::DefaultKeyProvider;
using std::ostream;
using std::string;
using std::unique_ptr;
@@ -193,6 +196,9 @@ FsManager::FsManager(Env* env, FsManagerOpts opts)
meta_on_xfs_(false) {
DCHECK(opts_.update_instances == UpdateInstanceBehavior::DONT_UPDATE ||
!opts_.read_only) << "FsManager can only be for updated if not in
read-only mode";
+ if (FLAGS_encrypt_data_at_rest) {
+ key_provider_.reset(new DefaultKeyProvider());
+ }
}
FsManager::~FsManager() {}
@@ -457,10 +463,14 @@ Status FsManager::Open(FsReport* report, Timer*
read_instance_metadata_files,
read_instance_metadata_files->Stop();
}
- if (!server_key().empty()) {
- env_->SetEncryptionKey(server_key().length() * 4,
- reinterpret_cast<const uint8_t*>(
- strings::a2b_hex(server_key()).c_str()));
+ if (!server_key().empty() && key_provider_) {
+ string server_key;
+ RETURN_NOT_OK(key_provider_->DecryptServerKey(this->server_key(),
&server_key));
+ // 'server_key' is a hexadecimal string and SetEncryptionKey expects bits
+ // (hex / 2 = bytes * 8 = bits).
+ env_->SetEncryptionKey(reinterpret_cast<const uint8_t*>(
+ strings::a2b_hex(server_key).c_str()),
+ server_key.length() * 4);
}
// Open the directory manager if it has not been opened already.
@@ -672,14 +682,18 @@ Status
FsManager::CreateInstanceMetadata(boost::optional<string> uuid,
metadata->set_uuid(oid_generator_.Next());
}
if (server_key) {
- metadata->set_server_key(server_key.get());
+ RETURN_NOT_OK(key_provider_->EncryptServerKey(server_key.get(),
+
metadata->mutable_server_key()));
} else if (FLAGS_encrypt_data_at_rest) {
uint8_t key_bytes[32];
int num_bytes = FLAGS_encryption_key_length / 8;
DCHECK(num_bytes <= sizeof(key_bytes));
OPENSSL_RET_NOT_OK(RAND_bytes(key_bytes, num_bytes),
"Failed to generate random key");
- strings::b2a_hex(key_bytes, metadata->mutable_server_key(), num_bytes);
+ string plain_server_key;
+ strings::b2a_hex(key_bytes, &plain_server_key, num_bytes);
+ RETURN_NOT_OK(key_provider_->EncryptServerKey(plain_server_key,
+
metadata->mutable_server_key()));
}
string time_str;
diff --git a/src/kudu/fs/fs_manager.h b/src/kudu/fs/fs_manager.h
index 9c26ca1df..d3700f056 100644
--- a/src/kudu/fs/fs_manager.h
+++ b/src/kudu/fs/fs_manager.h
@@ -39,6 +39,12 @@
#include "kudu/util/path_util.h"
#include "kudu/util/status.h"
+namespace kudu {
+namespace security {
+class KeyProvider;
+} // namespace security
+} // namespace kudu
+
DECLARE_bool(enable_data_block_fsync);
namespace kudu {
@@ -417,6 +423,8 @@ class FsManager {
std::unique_ptr<fs::DataDirManager> dd_manager_;
std::unique_ptr<fs::BlockManager> block_manager_;
+ std::unique_ptr<security::KeyProvider> key_provider_;
+
ObjectIdGenerator oid_generator_;
bool initted_;
diff --git a/src/kudu/mini-cluster/external_mini_cluster.cc
b/src/kudu/mini-cluster/external_mini_cluster.cc
index d2895210a..73335f336 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.cc
+++ b/src/kudu/mini-cluster/external_mini_cluster.cc
@@ -36,6 +36,7 @@
#include "kudu/client/master_rpc.h"
#include "kudu/fs/fs.pb.h"
#include "kudu/rpc/rpc_header.pb.h"
+#include "kudu/server/key_provider.h"
#if !defined(NO_CHRONY)
#include "kudu/clock/test/mini_chronyd.h"
#endif
@@ -56,6 +57,7 @@
#include "kudu/rpc/sasl_common.h"
#include "kudu/rpc/user_credentials.h"
#include "kudu/security/test/mini_kdc.h"
+#include "kudu/server/default_key_provider.h"
#include "kudu/server/server_base.pb.h"
#include "kudu/server/server_base.proxy.h"
#include "kudu/tablet/metadata.pb.h"
@@ -87,6 +89,7 @@ using kudu::master::MasterServiceProxy;
using kudu::pb_util::SecureDebugString;
using kudu::pb_util::SecureShortDebugString;
using kudu::rpc::RpcController;
+using kudu::security::DefaultKeyProvider;
using kudu::server::ServerStatusPB;
using kudu::tserver::ListTabletsRequestPB;
using kudu::tserver::ListTabletsResponsePB;
@@ -1090,6 +1093,7 @@ Status ExternalMiniCluster::RemoveMaster(const HostPort&
hp) {
ExternalDaemon::ExternalDaemon(ExternalDaemonOptions opts)
: opts_(std::move(opts)),
+ key_provider_(new DefaultKeyProvider()),
parent_tid_(std::this_thread::get_id()) {
CHECK(rpc_bind_address().Initialized());
}
@@ -1278,10 +1282,11 @@ Status ExternalDaemon::SetServerKey() {
LOG(INFO) << "Reading " << path;
InstanceMetadataPB instance;
RETURN_NOT_OK(pb_util::ReadPBContainerFromPath(env(), path, &instance,
pb_util::NOT_SENSITIVE));
- if (string key = instance.server_key();
- !key.empty()) {
+ if (!instance.server_key().empty()) {
+ string key;
+ RETURN_NOT_OK(key_provider_->DecryptServerKey(instance.server_key(),
&key));
LOG(INFO) << "Setting key " << key;
- env()->SetEncryptionKey(key.size() * 4, reinterpret_cast<const
uint8_t*>(a2b_hex(key).c_str()));
+ env()->SetEncryptionKey(reinterpret_cast<const
uint8_t*>(a2b_hex(key).c_str()), key.size() * 4);
}
return Status::OK();
}
diff --git a/src/kudu/mini-cluster/external_mini_cluster.h
b/src/kudu/mini-cluster/external_mini_cluster.h
index 1c005371e..fdc71f528 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.h
+++ b/src/kudu/mini-cluster/external_mini_cluster.h
@@ -48,6 +48,9 @@ class Env;
class NodeInstancePB;
class Sockaddr;
class Subprocess;
+namespace security {
+class KeyProvider;
+} // namespace security
namespace client {
class KuduClient;
@@ -548,6 +551,7 @@ class ExternalMiniCluster : public MiniCluster {
std::unique_ptr<MiniKdc> kdc_;
std::unique_ptr<hms::MiniHms> hms_;
std::unique_ptr<ranger::MiniRanger> ranger_;
+ std::unique_ptr<security::KeyProvider> key_provider_;
std::shared_ptr<rpc::Messenger> messenger_;
@@ -739,6 +743,8 @@ class ExternalDaemon : public
RefCountedThreadSafe<ExternalDaemon> {
std::unique_ptr<server::ServerStatusPB> status_;
+ std::unique_ptr<security::KeyProvider> key_provider_;
+
// These capture the daemons parameters and running ports and
// are used to Restart() the daemon with the same parameters.
HostPort bound_rpc_;
diff --git a/src/kudu/server/CMakeLists.txt b/src/kudu/server/CMakeLists.txt
index 78ff96515..a61b2d141 100644
--- a/src/kudu/server/CMakeLists.txt
+++ b/src/kudu/server/CMakeLists.txt
@@ -85,3 +85,6 @@ SET_KUDU_TEST_LINK_LIBS(
security_test_util)
ADD_KUDU_TEST(rpc_server-test)
ADD_KUDU_TEST(webserver-test)
+
+SET_KUDU_TEST_LINK_LIBS(server_process)
+ADD_KUDU_TEST(default_key_provider-test)
diff --git a/src/kudu/server/default_key_provider-test.cc
b/src/kudu/server/default_key_provider-test.cc
new file mode 100644
index 000000000..22d0b7f07
--- /dev/null
+++ b/src/kudu/server/default_key_provider-test.cc
@@ -0,0 +1,48 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/server/default_key_provider.h"
+
+#include <string>
+
+#include <gtest/gtest.h>
+
+#include "kudu/util/test_macros.h"
+#include "kudu/util/test_util.h"
+
+using std::string;
+
+namespace kudu {
+namespace security {
+
+class DefaultKeyProviderTest : public KuduTest {
+ protected:
+ DefaultKeyProvider key_provider_;
+};
+
+TEST_F(DefaultKeyProviderTest, TestEncryptAndDecrypt) {
+ string key = "foo";
+ string encrypted_key;
+ string decrypted_key;
+ ASSERT_OK(key_provider_.EncryptServerKey(key, &encrypted_key));
+ ASSERT_OK(key_provider_.DecryptServerKey(encrypted_key, &decrypted_key));
+ ASSERT_NE(key, encrypted_key);
+ ASSERT_EQ(key, decrypted_key);
+}
+
+} // namespace security
+} // namespace kudu
diff --git a/src/kudu/server/default_key_provider.h
b/src/kudu/server/default_key_provider.h
new file mode 100644
index 000000000..33bab8098
--- /dev/null
+++ b/src/kudu/server/default_key_provider.h
@@ -0,0 +1,53 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <string>
+
+#include "kudu/server/key_provider.h"
+
+namespace kudu {
+namespace security {
+
+class DefaultKeyProvider : public KeyProvider {
+public:
+ ~DefaultKeyProvider() override {}
+ Status DecryptServerKey(const std::string& encrypted_server_key,
+ std::string* server_key) override {
+ return EncryptServerKey(encrypted_server_key, server_key);
+ }
+
+ Status EncryptServerKey(const std::string& server_key,
+ std::string* encrypted_server_key) override {
+ *encrypted_server_key = server_key;
+#ifdef __linux__
+ memfrob(encrypted_server_key->data(), server_key.length());
+#else
+ // On Linux, memfrob() bitwise XORs the data with the magic number that is
+ // the answer to the ultimate question of life, the universe, and
+ // everything. On Mac, we do this manually.
+ const uint8_t kMagic = 42;
+ for (auto i = 0; i < server_key.length(); ++i) {
+ encrypted_server_key->data()[i] ^= kMagic;
+ }
+#endif
+ return Status::OK();
+ }
+};
+} // namespace security
+} // namespace kudu
diff --git a/src/kudu/server/key_provider.h b/src/kudu/server/key_provider.h
new file mode 100644
index 000000000..0c52282f7
--- /dev/null
+++ b/src/kudu/server/key_provider.h
@@ -0,0 +1,41 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <string>
+
+#include "kudu/util/status.h"
+
+namespace kudu {
+namespace security {
+
+// An interface for encrypting and decrypting Kudu's server keys.
+class KeyProvider {
+ public:
+ virtual ~KeyProvider() = default;
+
+ // Decrypts the server key.
+ virtual Status DecryptServerKey(const std::string& encrypted_server_key,
+ std::string* server_key) = 0;
+
+ // Encrypts the server key.
+ virtual Status EncryptServerKey(const std::string& server_key,
+ std::string* encrypted_server_key) = 0;
+};
+} // namespace security
+} // namespace kudu
diff --git a/src/kudu/tools/tool_action_common.cc
b/src/kudu/tools/tool_action_common.cc
index 8dc52d002..c2aceb824 100644
--- a/src/kudu/tools/tool_action_common.cc
+++ b/src/kudu/tools/tool_action_common.cc
@@ -911,8 +911,8 @@ Status SetServerKey() {
if (string key = instance.server_key();
!key.empty()) {
- Env::Default()->SetEncryptionKey(key.length() * 4,
- reinterpret_cast<const
uint8_t*>(a2b_hex(key).c_str()));
+ Env::Default()->SetEncryptionKey(reinterpret_cast<const
uint8_t*>(a2b_hex(key).c_str()),
+ key.length() * 4);
}
return Status::OK();
diff --git a/src/kudu/util/env.h b/src/kudu/util/env.h
index 6fc3ee894..b4674b34b 100644
--- a/src/kudu/util/env.h
+++ b/src/kudu/util/env.h
@@ -390,7 +390,8 @@ class Env {
virtual bool IsEncryptionEnabled() const = 0;
- virtual void SetEncryptionKey(int key_size, const uint8_t* key) = 0;
+ // Set the raw server encryption key. The key size is in bits.
+ virtual void SetEncryptionKey(const uint8_t* key, size_t key_size) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(Env);
diff --git a/src/kudu/util/env_posix.cc b/src/kudu/util/env_posix.cc
index a119ffe17..7957db5d9 100644
--- a/src/kudu/util/env_posix.cc
+++ b/src/kudu/util/env_posix.cc
@@ -2273,7 +2273,7 @@ class PosixEnv : public Env {
bool IsEncryptionEnabled() const override { return
FLAGS_encrypt_data_at_rest; }
- void SetEncryptionKey(int key_size, const uint8_t* server_key) override {
+ void SetEncryptionKey(const uint8_t* server_key, size_t key_size) override {
EncryptionHeader eh;
switch (key_size) {
case 128:
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 2e7fad16c..ba5f16967 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -201,7 +201,7 @@ void KuduTest::OverrideKrb5Environment() {
void KuduTest::SetEncryptionFlags(bool enable_encryption) {
FLAGS_encrypt_data_at_rest = enable_encryption;
if (enable_encryption) {
- Env::Default()->SetEncryptionKey(kEncryptionKeySize * 8, kEncryptionKey);
+ Env::Default()->SetEncryptionKey(kEncryptionKey, kEncryptionKeySize * 8);
}
}
