KUDU-2265 CA-signed server certs for non-leader masters This changelist addresses KUDU-2265. Prior to this fix, a non-leader master had a self-signed server TLS certificates if it hasn't ever become a leader.
An integration test to verify the new behavior is added as well. Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710 Reviewed-on: http://gerrit.cloudera.org:8080/9076 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Reviewed-by: Dan Burkert <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c497c69b Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c497c69b Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c497c69b Branch: refs/heads/master Commit: c497c69bb2128a8a62738b9b6bb97a5b83da0323 Parents: e0a743d Author: Alexey Serbin <[email protected]> Authored: Thu Jan 18 23:52:03 2018 -0800 Committer: Alexey Serbin <[email protected]> Committed: Wed Jan 24 19:24:32 2018 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/CMakeLists.txt | 1 + .../security-master-certificates-itest.cc | 100 +++++++++++++++++++ src/kudu/master/catalog_manager.cc | 46 +++++++-- src/kudu/master/catalog_manager.h | 30 +++++- src/kudu/security/tls_context.cc | 10 +- src/kudu/util/rw_mutex.cc | 6 ++ src/kudu/util/rw_mutex.h | 2 + 7 files changed, 177 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/integration-tests/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/CMakeLists.txt b/src/kudu/integration-tests/CMakeLists.txt index 1a10a7d..a6a6938 100644 --- a/src/kudu/integration-tests/CMakeLists.txt +++ b/src/kudu/integration-tests/CMakeLists.txt @@ -94,6 +94,7 @@ ADD_KUDU_TEST(raft_consensus-itest RUN_SERIAL true) ADD_KUDU_TEST(registration-test RESOURCE_LOCK "master-web-port") ADD_KUDU_TEST(security-faults-itest) ADD_KUDU_TEST(security-itest) +ADD_KUDU_TEST(security-master-certificates-itest RESOURCE_LOCK "master-rpc-ports") ADD_KUDU_TEST(security-unknown-tsk-itest) ADD_KUDU_TEST(stop_tablet-itest) ADD_KUDU_TEST(table_locations-itest) http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/integration-tests/security-master-certificates-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/security-master-certificates-itest.cc b/src/kudu/integration-tests/security-master-certificates-itest.cc new file mode 100644 index 0000000..b521d65 --- /dev/null +++ b/src/kudu/integration-tests/security-master-certificates-itest.cc @@ -0,0 +1,100 @@ +// 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 <memory> +#include <vector> + +#include <gflags/gflags_declare.h> +#include <gtest/gtest.h> + +#include "kudu/consensus/raft_consensus.h" +#include "kudu/gutil/ref_counted.h" +#include "kudu/master/catalog_manager.h" +#include "kudu/master/master.h" +#include "kudu/master/mini_master.h" +#include "kudu/master/sys_catalog.h" +#include "kudu/mini-cluster/internal_mini_cluster.h" +#include "kudu/security/tls_context.h" +#include "kudu/tablet/tablet_replica.h" +#include "kudu/util/status.h" +#include "kudu/util/test_macros.h" +#include "kudu/util/test_util.h" + +DECLARE_bool(enable_leader_failure_detection); + +using kudu::cluster::InternalMiniCluster; +using kudu::cluster::InternalMiniClusterOptions; +using kudu::consensus::RaftConsensus; + +using std::unique_ptr; + + +namespace kudu { + +class SecurityMasterCertsTest : public KuduTest { + public: + void SetUp() override { + KuduTest::SetUp(); + + // This test requires manual system catalog leader election. + FLAGS_enable_leader_failure_detection = false; + + InternalMiniClusterOptions opts; + opts.master_rpc_ports = { 11010, 11011, 11012, 11013, 11014, }; + opts.num_masters = opts.master_rpc_ports.size(); + opts.num_tablet_servers = 0; + cluster_.reset(new InternalMiniCluster(env_, opts)); + ASSERT_OK(cluster_->Start()); + } + + void TearDown() override { + cluster_->Shutdown(); + } + + protected: + unique_ptr<InternalMiniCluster> cluster_; +}; + +// This scenario verifies that follower masters get CA-signed certificates +// even if they haven't run in the leader role yet. In this particular scenario, +// only one of the masters has ever become a leader and the rest have always +// been followers. This is a test to cover regressions of KUDU-2265, if any. +TEST_F(SecurityMasterCertsTest, FollowerCertificates) { + for (auto i = 0; i < cluster_->num_masters(); ++i) { + const auto& tls = cluster_->mini_master(i)->master()->tls_context(); + // Initially, all master servers have self-signed certs, + // but none has CA-signed cert. + ASSERT_FALSE(tls.has_signed_cert()); + ASSERT_TRUE(tls.has_cert()); + } + + auto* consensus = cluster_->mini_master(0)->master()->catalog_manager()-> + sys_catalog()->tablet_replica()->consensus(); + ASSERT_OK(consensus->StartElection( + RaftConsensus::ELECT_EVEN_IF_LEADER_IS_ALIVE, + RaftConsensus::EXTERNAL_REQUEST)); + + // After some time, all master servers should have CA-signed certs. + ASSERT_EVENTUALLY([&] { + for (auto i = 0; i < cluster_->num_masters(); ++i) { + const auto& tls = cluster_->mini_master(i)->master()->tls_context(); + ASSERT_TRUE(tls.has_signed_cert()); + } + }); +} + +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 1248144..33fd194 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -536,6 +536,14 @@ void CatalogManagerBgTasks::Run() { LOG(FATAL) << err_msg; } } + } else if (catalog_manager_->NeedToPrepareFollower() && l.owns_lock()) { + // This is the case of a non-leader catalog manager that has some work + // to do in a preparation to run in its current role. + Status s = catalog_manager_->PrepareFollower(); + if (!s.ok()) { + LOG(WARNING) << s.ToString() + << ": failed to prepare follower catalog manager, will retry"; + } } } // Wait for a notification or a timeout expiration. @@ -788,17 +796,16 @@ Status CatalogManager::InitCertAuthority() { // private key and certificate. Status CatalogManager::InitCertAuthorityWith( unique_ptr<PrivateKey> key, unique_ptr<Cert> cert) { - leader_lock_.AssertAcquiredForWriting(); + leader_lock_.AssertAcquired(); + auto* ca = master_->cert_authority(); RETURN_NOT_OK_PREPEND(ca->Init(std::move(key), std::move(cert)), "could not init master CA"); - auto* tls = master_->mutable_tls_context(); RETURN_NOT_OK_PREPEND(tls->AddTrustedCertificate(ca->ca_cert()), "could not trust master CA cert"); // If we haven't signed our own server cert yet, do so. - boost::optional<security::CertSignRequest> csr = - tls->GetCsrIfNecessary(); + boost::optional<security::CertSignRequest> csr = tls->GetCsrIfNecessary(); if (csr) { Cert cert; RETURN_NOT_OK_PREPEND(ca->SignServerCSR(*csr, &cert), @@ -811,7 +818,7 @@ Status CatalogManager::InitCertAuthorityWith( Status CatalogManager::LoadCertAuthorityInfo(unique_ptr<PrivateKey>* key, unique_ptr<Cert>* cert) { - leader_lock_.AssertAcquiredForWriting(); + leader_lock_.AssertAcquired(); SysCertAuthorityEntryPB info; RETURN_NOT_OK(sys_catalog_->GetCertAuthorityEntry(&info)); @@ -975,6 +982,29 @@ void CatalogManager::PrepareForLeadershipTask() { leader_ready_term_ = term; } +bool CatalogManager::NeedToPrepareFollower() { + return !master_->tls_context().has_signed_cert(); +} + +Status CatalogManager::PrepareFollower() { + static const char* const kDescription = + "acquiring CA information for follower catalog manager"; + + leader_lock_.AssertAcquiredForReading(); + + unique_ptr<PrivateKey> key; + unique_ptr<Cert> cert; + Status s = LoadCertAuthorityInfo(&key, &cert).AndThen([&] { + return InitCertAuthorityWith(std::move(key), std::move(cert)); + }); + if (s.ok()) { + LOG_WITH_PREFIX(INFO) << kDescription << ": success"; + } else { + LOG_WITH_PREFIX(WARNING) << kDescription << ": " << s.ToString(); + } + return s; +} + Status CatalogManager::VisitTablesAndTabletsUnlocked() { leader_lock_.AssertAcquiredForWriting(); @@ -1013,10 +1043,8 @@ Status CatalogManager::VisitTablesAndTablets() { Status CatalogManager::InitSysCatalogAsync(bool is_first_run) { std::lock_guard<LockType> l(lock_); - unique_ptr<SysCatalogTable> new_catalog( - new SysCatalogTable(master_, - Bind(&CatalogManager::ElectedAsLeaderCb, - Unretained(this)))); + unique_ptr<SysCatalogTable> new_catalog(new SysCatalogTable( + master_, Bind(&CatalogManager::ElectedAsLeaderCb, Unretained(this)))); if (is_first_run) { RETURN_NOT_OK(new_catalog->CreateNew(master_->fs_manager())); } else { http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/master/catalog_manager.h ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h index 9261cf0..720e6ea 100644 --- a/src/kudu/master/catalog_manager.h +++ b/src/kudu/master/catalog_manager.h @@ -412,11 +412,18 @@ class CatalogManager : public tserver::TabletReplicaLookupIf { // class ScopedLeaderSharedLock { public: - // Creates a new shared lock, acquiring the catalog manager's leader_lock_ - // for reading in the process. The lock is released when this object is - // destroyed. + // Creates a new shared lock, trying to acquire the catalog manager's + // leader_lock_ for reading in the process. If acquired, the lock is + // released when this object is destroyed. // - // 'catalog' must outlive this object. + // In most common use cases, where write lock semantics is assumed, call + // CheckIsInitializedAndIsLeaderOrRespond() to verify that the leader_lock_ + // has been acquired (as shown in the class-wide comment above). In rare + // cases, where both read and write semantics are applicable, use the + // combination of CheckIsInitializedOrRespond() and owns_lock() methods + // to verify that the leader_lock_ is acquired. + // + // The object pointed by the 'catalog' parameter must outlive this object. explicit ScopedLeaderSharedLock(CatalogManager* catalog); // General status of the catalog manager. If not OK (e.g. the catalog @@ -440,6 +447,11 @@ class CatalogManager : public tserver::TabletReplicaLookupIf { return leader_status_; } + // Whether the underlying leader lock of the system catalog is acquired. + bool owns_lock() const { + return leader_shared_lock_.owns_lock(); + } + // Check whether the consensus configuration term has changed from the term // captured at object construction (initial_term_). // Requires: leader_status() returns OK(). @@ -660,6 +672,16 @@ class CatalogManager : public tserver::TabletReplicaLookupIf { // internal state of this object upon becoming the leader. void PrepareForLeadershipTask(); + // Whether this catalog manager needs to prepare for running in the follower + // role. + bool NeedToPrepareFollower(); + + // Perform necessary work to prepare for running in the follower role. + // Currently, this includes reading the CA information from the system table, + // creating TLS server certificate request, signing it with the CA key, + // and installing the certificate TLS server certificates. + Status PrepareFollower(); + // Clears out the existing metadata ('table_names_map_', 'table_ids_map_', // and 'tablet_map_'), loads tables metadata into memory and if successful // loads the tablets metadata. http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index e43522c..8e687f3 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -394,16 +394,16 @@ Status TlsContext::AdoptSignedCert(const Cert& cert) { SCOPED_OPENSSL_NO_PENDING_ERRORS; unique_lock<RWMutex> lock(lock_); - // Verify that the appropriate CA certs have been loaded into the context - // before we adopt a cert. Otherwise, client connections without the CA cert - // available would fail. - RETURN_NOT_OK(VerifyCertChainUnlocked(cert)); - if (!csr_) { // A signed cert has already been adopted. return Status::OK(); } + // Verify that the appropriate CA certs have been loaded into the context + // before we adopt a cert. Otherwise, client connections without the CA cert + // available would fail. + RETURN_NOT_OK(VerifyCertChainUnlocked(cert)); + PublicKey csr_key; RETURN_NOT_OK(csr_->GetPublicKey(&csr_key)); PublicKey cert_key; http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/util/rw_mutex.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/rw_mutex.cc b/src/kudu/util/rw_mutex.cc index 59139af..75fc1e7 100644 --- a/src/kudu/util/rw_mutex.cc +++ b/src/kudu/util/rw_mutex.cc @@ -140,6 +140,12 @@ bool RWMutex::TryWriteLock() { #ifndef NDEBUG +void RWMutex::AssertAcquired() const { + lock_guard<simple_spinlock> l(tid_lock_); + CHECK(ContainsKey(reader_tids_, Env::Default()->gettid()) || + Env::Default()->gettid() == writer_tid_); +} + void RWMutex::AssertAcquiredForReading() const { lock_guard<simple_spinlock> l(tid_lock_); CHECK(ContainsKey(reader_tids_, Env::Default()->gettid())); http://git-wip-us.apache.org/repos/asf/kudu/blob/c497c69b/src/kudu/util/rw_mutex.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/rw_mutex.h b/src/kudu/util/rw_mutex.h index 84f77d2..bb6168c 100644 --- a/src/kudu/util/rw_mutex.h +++ b/src/kudu/util/rw_mutex.h @@ -65,9 +65,11 @@ class RWMutex { bool TryWriteLock(); #ifndef NDEBUG + void AssertAcquired() const; void AssertAcquiredForReading() const; void AssertAcquiredForWriting() const; #else + void AssertAcquired() const {} void AssertAcquiredForReading() const {} void AssertAcquiredForWriting() const {} #endif
