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
The following commit(s) were added to refs/heads/master by this push:
new 61d229678 KUDU-3448 Add support for encrypting existing keys
61d229678 is described below
commit 61d2296783a8392763f088d65a03376eb3e6463b
Author: Attila Bukor <abu...@apache.org>
AuthorDate: Mon Jun 12 18:57:18 2023 +0200
KUDU-3448 Add support for encrypting existing keys
On an existing cluster before KUDU-3448, the IPKI and TSK private keys
were stored in clear text. With KUDU-3448, it is now possible to encrypt
these keys, but without this commit, it's not possible to use this
feature in an existing cluster.
This commit introduces a fallback when trying to decrypt the stored
private keys, so that if that fails, it tries to read it without
decrypting it.
If it succeeds to read the IPKI private key, it encrypts it using the
password, and rewrites it in the sys-catalog table. It does no such
thing with the TSK, as they will be rolled out soon anyway, but it
encrypts the new keys, so it's still not possible to go back from
encrypted TSKs after a new key has been generated.
This commit doesn't make it possible to rotate the IPKI key.
Change-Id: Ide6ec4fb86325897f2b011aee9643d276044279d
Reviewed-on: http://gerrit.cloudera.org:8080/20050
Reviewed-by: Alexey Serbin <ale...@apache.org>
Tested-by: Alexey Serbin <ale...@apache.org>
---
src/kudu/integration-tests/security-itest.cc | 58 ++++++++++++++++++++++++++++
src/kudu/master/catalog_manager.cc | 50 ++++++++++++++++++++++++
src/kudu/master/catalog_manager.h | 6 +++
src/kudu/master/sys_catalog.cc | 16 +++++++-
src/kudu/master/sys_catalog.h | 12 ++++++
src/kudu/security/token_signing_key.cc | 15 ++++++-
6 files changed, 153 insertions(+), 4 deletions(-)
diff --git a/src/kudu/integration-tests/security-itest.cc
b/src/kudu/integration-tests/security-itest.cc
index 7cb2bde4a..adea89025 100644
--- a/src/kudu/integration-tests/security-itest.cc
+++ b/src/kudu/integration-tests/security-itest.cc
@@ -1157,6 +1157,64 @@ TEST_F(SecurityITest, IPKICACertWebServerEndPoints) {
ASSERT_EQ(ca_cert_str_1, ca_cert_str_2);
}
+TEST_F(SecurityITest, TestEnableAndDisableEncryptedIPKICert) {
+ SKIP_IF_SLOW_NOT_ALLOWED();
+ ASSERT_OK(StartCluster());
+
+ shared_ptr<KuduClient> client;
+ KuduClientBuilder b;
+ ASSERT_OK(cluster_->CreateClient(&b, &client));
+ NO_FATALS(SmokeTestCluster(client, /*transactional=*/false));
+
+ for (auto i = 0; i < cluster_->num_masters(); ++i) {
+
cluster_->master(i)->mutable_flags()->emplace_back("--ipki_private_key_password_cmd=echo
foo");
+ }
+ cluster_->Shutdown();
+ ASSERT_OK(cluster_->Restart());
+
+ NO_FATALS(SmokeTestCluster(client, /*transactional=*/false));
+
+ for (auto i = 0; i < cluster_->num_masters(); ++i) {
+ cluster_->master(i)->mutable_flags()->pop_back();
+ }
+ cluster_->Shutdown();
+ // the master should fail to start, which means the tablet servers won't be
+ // able to connect to it and the mini cluster will time out waiting for the
+ // cluster to start up.
+ ASSERT_TRUE(cluster_->Restart().IsTimedOut());
+}
+
+TEST_F(SecurityITest, TestEnableAndDisableEncryptedTSK) {
+ SKIP_IF_SLOW_NOT_ALLOWED();
+
cluster_opts_.extra_master_flags.emplace_back("--authn_token_validity_seconds=20");
+
cluster_opts_.extra_master_flags.emplace_back("--authz_token_validity_seconds=20");
+ cluster_opts_.extra_master_flags.emplace_back("--tsk_rotation_seconds=20");
+ ASSERT_OK(StartCluster());
+
+ shared_ptr<KuduClient> client;
+ KuduClientBuilder b;
+ ASSERT_OK(cluster_->CreateClient(&b, &client));
+ NO_FATALS(SmokeTestCluster(client, /*transactional=*/false));
+
+ for (auto i = 0; i < cluster_->num_masters(); ++i) {
+
cluster_->master(i)->mutable_flags()->emplace_back("--tsk_private_key_password_cmd=echo
foo");
+ }
+ cluster_->Shutdown();
+ SleepFor(MonoDelta::FromSeconds(20));
+ ASSERT_OK(cluster_->Restart());
+ NO_FATALS(SmokeTestCluster(client, /*transactional=*/false));
+
+ for (auto i = 0; i < cluster_->num_masters(); ++i) {
+ cluster_->master(i)->mutable_flags()->pop_back();
+ }
+ cluster_->Shutdown();
+ SleepFor(MonoDelta::FromSeconds(20));
+ // the master should fail to start, which means the tablet servers won't be
+ // able to connect to it and the mini cluster will time out waiting for the
+ // cluster to start up.
+ ASSERT_TRUE(cluster_->Restart().IsTimedOut());
+}
+
class EncryptionPolicyTest :
public SecurityITest,
public ::testing::WithParamInterface<tuple<
diff --git a/src/kudu/master/catalog_manager.cc
b/src/kudu/master/catalog_manager.cc
index ef56e428d..cf138364b 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -1261,6 +1261,15 @@ Status CatalogManager::InitCertAuthority() {
// should not run without a CA certificate.
return InitCertAuthorityWith(std::move(key), std::move(cert));
}
+ // If a cluster existed before KUDU-3448, or the
+ // --ipki_private_key_password_cmd was not set on a cluster, the IPKI private
+ // key is stored in cleartext. In this case, if the administrator adds this
+ // flag to the existing cluster, we need to read the cleartext private key,
+ // encrypt it, and overwrite the cleartext key with the encrypted version.
+ if (s.IsRuntimeError() && !ipki_private_key_password_.empty()) {
+ RETURN_NOT_OK(LoadAndEncryptCertAuthorityInfo(&key, &cert));
+ return InitCertAuthorityWith(std::move(key), std::move(cert));
+ }
return s;
}
@@ -1336,6 +1345,47 @@ Status
CatalogManager::LoadCertAuthorityInfo(unique_ptr<PrivateKey>* key,
return Status::OK();
}
+Status CatalogManager::LoadAndEncryptCertAuthorityInfo(unique_ptr<PrivateKey>*
key,
+ unique_ptr<Cert>* cert)
{
+ leader_lock_.AssertAcquiredForWriting();
+
+
MAYBE_INJECT_RANDOM_LATENCY(FLAGS_catalog_manager_inject_latency_load_ca_info_ms);
+
+ SysCertAuthorityEntryPB info;
+ RETURN_NOT_OK(sys_catalog_->GetCertAuthorityEntry(&info));
+
+ unique_ptr<PrivateKey> ca_private_key(new PrivateKey);
+ unique_ptr<Cert> ca_cert(new Cert);
+
+ RETURN_NOT_OK_PREPEND(ca_private_key->FromString(info.private_key(),
DataFormat::DER),
+ "could not load IPKI private key");
+
+ LOG(INFO) << "IPKI private key was stored in cleartext, attempting to
encrypt it.";
+
+ RETURN_NOT_OK_PREPEND(ca_private_key->ToEncryptedString(
+ info.mutable_private_key(), DataFormat::DER,
+ [&](string* password){ *password = ipki_private_key_password_;
+ return Status::OK();
+ }),
+ "private key was stored in cleartext, reencrypting with the provided "
+ "password failed.");
+
+ LOG(INFO) << "IPKI private key successfully encrypted.";
+
+ RETURN_NOT_OK_PREPEND(sys_catalog_->UpdateCertAuthorityEntry(info),
+ "IPKI certificate couldn't be written to syscatalog");
+
+ RETURN_NOT_OK(ca_cert->FromString(
+ info.certificate(), DataFormat::DER));
+ // Extra sanity check.
+ RETURN_NOT_OK(ca_cert->CheckKeyMatch(*ca_private_key));
+
+ key->swap(ca_private_key);
+ cert->swap(ca_cert);
+
+ return Status::OK();
+}
+
// Store cluster ID into the system table.
Status CatalogManager::StoreClusterId(const string& cluster_id) {
leader_lock_.AssertAcquiredForWriting();
diff --git a/src/kudu/master/catalog_manager.h
b/src/kudu/master/catalog_manager.h
index bf819de9c..159a7a243 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -1100,6 +1100,12 @@ class CatalogManager : public
tserver::TabletReplicaLookupIf {
Status LoadCertAuthorityInfo(std::unique_ptr<security::PrivateKey>* key,
std::unique_ptr<security::Cert>* cert);
+ // Same as above, but it doesn't try to decrypt it even if
+ // ipki_private_key_password_ is set, and if it's able to load it, then it
+ // encrypts it using the above password and rewrites the system table entry.
+ Status
LoadAndEncryptCertAuthorityInfo(std::unique_ptr<security::PrivateKey>* key,
+ std::unique_ptr<security::Cert>*
cert);
+
// Store cluster ID into the system table.
Status StoreClusterId(const std::string& cluster_id);
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index 1c346cacf..545dfad0b 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -25,6 +25,7 @@
#include <optional>
#include <ostream>
#include <string>
+#include <type_traits>
#include <utility>
#include <vector>
@@ -50,6 +51,7 @@
#include "kudu/consensus/consensus_peers.h"
#include "kudu/consensus/log.h"
#include "kudu/consensus/log_anchor_registry.h"
+#include "kudu/consensus/opid.pb.h"
#include "kudu/consensus/opid_util.h"
#include "kudu/consensus/quorum_util.h"
#include "kudu/consensus/raft_consensus.h"
@@ -183,7 +185,6 @@ bool ArePBsEqual(const google::protobuf::Message& prev_pb,
} // anonymous namespace
-
SysCatalogTable::SysCatalogTable(Master* master,
ElectedLeaderCallback leader_cb)
: metric_registry_(master->metric_registry()),
@@ -910,6 +911,17 @@ Status SysCatalogTable::AddClusterIdEntry(
Status SysCatalogTable::AddCertAuthorityEntry(
const SysCertAuthorityEntryPB& entry) {
+ return AddOrUpdateCertAuthorityEntry(entry, RowOperationsPB::INSERT);
+}
+
+Status SysCatalogTable::UpdateCertAuthorityEntry(
+ const SysCertAuthorityEntryPB& entry) {
+ return AddOrUpdateCertAuthorityEntry(entry, RowOperationsPB::UPDATE);
+}
+
+Status SysCatalogTable::AddOrUpdateCertAuthorityEntry(
+ const SysCertAuthorityEntryPB& entry,
+ RowOperationsPB::Type op) {
WriteRequestPB req;
req.set_tablet_id(kSysCatalogTabletId);
RETURN_NOT_OK(SchemaToPB(schema_, req.mutable_schema()));
@@ -922,7 +934,7 @@ Status SysCatalogTable::AddCertAuthorityEntry(
CHECK_OK(row.SetStringNoCopy(kSysCatalogTableColId,
kSysCertAuthorityEntryId));
CHECK_OK(row.SetStringNoCopy(kSysCatalogTableColMetadata, metadata_buf));
RowOperationsPBEncoder enc(req.mutable_row_operations());
- enc.Add(RowOperationsPB::INSERT, row);
+ enc.Add(op, row);
return SyncWrite(req);
}
diff --git a/src/kudu/master/sys_catalog.h b/src/kudu/master/sys_catalog.h
index b4d6dd11e..79a697afc 100644
--- a/src/kudu/master/sys_catalog.h
+++ b/src/kudu/master/sys_catalog.h
@@ -26,6 +26,7 @@
#include <gtest/gtest_prod.h>
+#include "kudu/common/row_operations.pb.h"
#include "kudu/common/schema.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/gutil/macros.h"
@@ -267,6 +268,12 @@ class SysCatalogTable {
// There should be no more than one CA entry in the system table.
Status AddCertAuthorityEntry(const SysCertAuthorityEntryPB& entry);
+ // Update an existing root CA entry (private key and certificate) in the
+ // system table.
+ //
+ // There should be no more than one CA entry in the system table.
+ Status UpdateCertAuthorityEntry(const SysCertAuthorityEntryPB& entry);
+
// Add TSK (Token Signing Key) entry into the system table.
Status AddTskEntry(const SysTskEntryPB& entry);
@@ -393,6 +400,11 @@ class SysCatalogTable {
// Overwrite (upsert) the latest event ID in the table with the provided ID.
void ReqSetNotificationLogEventId(tserver::WriteRequestPB* req, int64_t
event_id);
+ // Add or update root CA entry (private key and certificate) into the system
table.
+ // There should be no more than one CA entry in the system table.
+ Status AddOrUpdateCertAuthorityEntry(const SysCertAuthorityEntryPB& entry,
+ RowOperationsPB::Type op);
+
static std::string TskSeqNumberToEntryId(int64_t seq_number);
static int64_t TskEntryIdToSeqNumber(const std::string& entry_id);
diff --git a/src/kudu/security/token_signing_key.cc
b/src/kudu/security/token_signing_key.cc
index 20667ce30..a3cda42e4 100644
--- a/src/kudu/security/token_signing_key.cc
+++ b/src/kudu/security/token_signing_key.cc
@@ -65,12 +65,23 @@ TokenSigningPrivateKey::TokenSigningPrivateKey(
if (password.empty()) {
CHECK_OK(key_->FromString(pb.rsa_key_der(), DataFormat::DER));
} else {
- CHECK_OK(key_->FromEncryptedString(pb.rsa_key_der(), DataFormat::DER,
+ Status s = key_->FromEncryptedString(pb.rsa_key_der(), DataFormat::DER,
[&](string* p) {
*p = password;
return Status::OK();
}
- ));
+ );
+
+ // It's possible that we have old TSKs from before
+ // --tsk_private_key_password_cmd has been added to the master and they're
+ // still stored in cleartext. In this case, we retry to load the TSK
without
+ // password. There's no need to re-encrypt the existing private keys, as
+ // they'll be rolled out soon anyway and the new ones will be encrypted.
+ if (s.IsRuntimeError()) {
+ CHECK_OK(key_->FromString(pb.rsa_key_der(), DataFormat::DER));
+ } else {
+ CHECK_OK(s);
+ }
}
private_key_der_ = pb.rsa_key_der();
key_seq_num_ = pb.key_seq_num();
