Repository: kudu Updated Branches: refs/heads/master f7d5c73dd -> 098a1e581
[security] generate self-signed certs on server startup Masters and tablet servers now generate a keypair and self-signed cert at startup for accepting TLS connections. Additionally, the tablet server will now request and receive a CA-signed cert from the Master during heartbeat. As a result, connections between servers and between servers and TLS-capable clients will now use TLS. The responsibilies of ServerCertManager have been rolled into TlsContext, since they overlapped a great deal. This resulted in an overall simpler flow for acquiring, storing, and using TLS certs. This also changes a bit of code to generate proper self-signed certs. Self-signed certs need to have the 'keyCertSign' attribute set, or else OpenSSL won't properly recognize the self-signature. Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622 Reviewed-on: http://gerrit.cloudera.org:8080/5955 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/098a1e58 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/098a1e58 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/098a1e58 Branch: refs/heads/master Commit: 098a1e58192cbd012828d9436e4c61fd79c53d38 Parents: f7d5c73 Author: Dan Burkert <[email protected]> Authored: Thu Feb 9 00:52:56 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Tue Feb 14 05:55:51 2017 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/registration-test.cc | 4 +- src/kudu/master/master.proto | 11 + src/kudu/master/master_service.cc | 1 + src/kudu/rpc/client_negotiation.cc | 7 +- src/kudu/rpc/messenger.cc | 44 +-- src/kudu/rpc/messenger.h | 7 + src/kudu/security/CMakeLists.txt | 1 - src/kudu/security/ca/cert_management.cc | 42 ++- src/kudu/security/ca/cert_management.h | 21 +- src/kudu/security/cert.cc | 23 ++ src/kudu/security/cert.h | 10 + src/kudu/security/crypto.cc | 20 +- src/kudu/security/crypto.h | 3 + src/kudu/security/openssl_util.h | 3 + src/kudu/security/server_cert_manager.cc | 119 -------- src/kudu/security/server_cert_manager.h | 94 ------- src/kudu/security/tls_context.cc | 136 +++++++++- src/kudu/security/tls_context.h | 109 ++++++-- src/kudu/security/tls_handshake-test.cc | 271 +++++++++++++++---- src/kudu/server/server_base.cc | 16 +- src/kudu/server/server_base.h | 11 +- src/kudu/tserver/heartbeater.cc | 26 +- 22 files changed, 619 insertions(+), 360 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/integration-tests/registration-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/registration-test.cc b/src/kudu/integration-tests/registration-test.cc index 52a0272..6603245 100644 --- a/src/kudu/integration-tests/registration-test.cc +++ b/src/kudu/integration-tests/registration-test.cc @@ -30,7 +30,7 @@ #include "kudu/master/master.pb.h" #include "kudu/master/mini_master.h" #include "kudu/master/ts_descriptor.h" -#include "kudu/security/server_cert_manager.h" +#include "kudu/security/tls_context.h" #include "kudu/tserver/mini_tablet_server.h" #include "kudu/tserver/tablet_server.h" #include "kudu/util/curl_util.h" @@ -204,7 +204,7 @@ TEST_F(RegistrationTest, TestTabletReports) { TEST_F(RegistrationTest, TestTSGetsSignedX509Certificate) { MiniTabletServer* ts = cluster_->mini_tablet_server(0); AssertEventually([&](){ - ASSERT_TRUE(ts->server()->cert_manager()->has_signed_cert()); + ASSERT_TRUE(ts->server()->tls_context().has_signed_cert()); }, MonoDelta::FromSeconds(10)); } http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/master/master.proto ---------------------------------------------------------------------- diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index 14b7771..9527ab5 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -296,6 +296,17 @@ message TSHeartbeatResponsePB { // If the heartbeat request had a CSR, then the successfully // signed certificate will be returned in DER format. optional bytes signed_cert_der = 7; + + // Any CA certs used by the cluster, currently only included when + // 'signed_cert_der' is also sent (however the tablet server will always check + // for all heartbeat responses). Currently the master only uses one cert, but + // we may support rolling this cert in the future, so tablet servers should + // add all returned certs to their trusted CA list. + // + // NOTE: this is not necessarily a "certificate chain" but rather a set of + // independent certs to be trusted. They may or may not have any signing + // relationship between them. + repeated bytes ca_cert_der = 8; } ////////////////////////////// http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/master/master_service.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/master_service.cc b/src/kudu/master/master_service.cc index b57a986..2ea29db 100644 --- a/src/kudu/master/master_service.cc +++ b/src/kudu/master/master_service.cc @@ -161,6 +161,7 @@ void MasterServiceImpl::TSHeartbeat(const TSHeartbeatRequestPB* req, } LOG(INFO) << "Signed X509 certificate for tserver " << rpc->requestor_string(); resp->mutable_signed_cert_der()->swap(cert); + resp->add_ca_cert_der(server_->cert_authority()->ca_cert_der()); } // TODO(aserbin): 7. Send any active CA certs which the TS doesn't have. http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/rpc/client_negotiation.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index 9c48332..4319183 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -169,10 +169,15 @@ Status ClientNegotiation::Negotiate() { RETURN_NOT_OK(tls_context_->InitiateHandshake(security::TlsHandshakeType::CLIENT, &tls_handshake_)); - if (negotiated_mech_ == SaslMechanism::GSSAPI) { + if (negotiated_mech_ == SaslMechanism::GSSAPI || + negotiated_mech_ == SaslMechanism::PLAIN) { // When using GSSAPI, we don't verify the server's certificate. Instead, // we rely on Kerberos authentication, and use channel binding to tie the // SASL authentication to the TLS channel. + // + // When using 'PLAIN' authentication, this implies that strong authentication + // is not enabled. So, we are just using TLS for encryption and don't need to + // validate a cert. tls_handshake_.set_verification_mode(security::TlsVerificationMode::VERIFY_NONE); } http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc index 2dd21fe..81116de 100644 --- a/src/kudu/rpc/messenger.cc +++ b/src/kudu/rpc/messenger.cc @@ -49,6 +49,7 @@ #include "kudu/util/metrics.h" #include "kudu/util/monotime.h" #include "kudu/util/net/socket.h" +#include "kudu/util/scoped_cleanup.h" #include "kudu/util/status.h" #include "kudu/util/threadpool.h" #include "kudu/util/trace.h" @@ -129,19 +130,39 @@ MessengerBuilder &MessengerBuilder::set_metric_entity( return *this; } +MessengerBuilder& MessengerBuilder::enable_inbound_tls(std::string server_uuid) { + enable_inbound_tls_server_uuid_ = server_uuid; + return *this; +} + Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) { RETURN_NOT_OK(SaslInit()); // Initialize SASL library before we start making requests + Messenger* new_msgr(new Messenger(*this)); - Status build_status = new_msgr->Init(); - if (!build_status.ok()) { - // 'new_msgr' will be freed when 'retain_self_' is reset, so no need to explicitly free it. - new_msgr->AllExternalReferencesDropped(); - return build_status; + + auto cleanup = MakeScopedCleanup([&] () { + new_msgr->AllExternalReferencesDropped(); + }); + + RETURN_NOT_OK(new_msgr->Init()); + if (enable_inbound_tls_server_uuid_) { + auto* tls_context = new_msgr->mutable_tls_context(); + if (!FLAGS_rpc_ssl_server_certificate.empty() || + !FLAGS_rpc_ssl_private_key.empty() || + !FLAGS_rpc_ssl_certificate_authority.empty()) { + // TODO(PKI): should we try and enforce that the server UUID and/or + // hostname is in the subject or alt names of the cert? + RETURN_NOT_OK(tls_context->LoadCertificateAuthority(FLAGS_rpc_ssl_certificate_authority)); + RETURN_NOT_OK(tls_context->LoadCertificateAndKey(FLAGS_rpc_ssl_server_certificate, + FLAGS_rpc_ssl_private_key)); + } else { + RETURN_NOT_OK(tls_context->GenerateSelfSignedCertAndKey(*enable_inbound_tls_server_uuid_)); + } } // See docs on Messenger::retain_self_ for info about this odd hack. - *msgr = shared_ptr<Messenger>( - new_msgr, std::mem_fun(&Messenger::AllExternalReferencesDropped)); + cleanup.cancel(); + *msgr = shared_ptr<Messenger>(new_msgr, std::mem_fun(&Messenger::AllExternalReferencesDropped)); return Status::OK(); } @@ -298,16 +319,7 @@ Reactor* Messenger::RemoteToReactor(const Sockaddr &remote) { } Status Messenger::Init() { - Status status; - RETURN_NOT_OK(tls_context_->Init()); - if (!FLAGS_rpc_ssl_server_certificate.empty() || - !FLAGS_rpc_ssl_private_key.empty() || - !FLAGS_rpc_ssl_certificate_authority.empty()) { - RETURN_NOT_OK(tls_context_->LoadCertificateAuthority(FLAGS_rpc_ssl_certificate_authority)); - RETURN_NOT_OK(tls_context_->LoadCertificateAndKey(FLAGS_rpc_ssl_server_certificate, - FLAGS_rpc_ssl_private_key)); - } for (Reactor* r : reactors_) { RETURN_NOT_OK(r->Init()); } http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/rpc/messenger.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.h b/src/kudu/rpc/messenger.h index 8303361..c9fda69 100644 --- a/src/kudu/rpc/messenger.h +++ b/src/kudu/rpc/messenger.h @@ -25,6 +25,7 @@ #include <unordered_map> #include <vector> +#include <boost/optional.hpp> #include <gtest/gtest_prod.h> #include "kudu/gutil/gscoped_ptr.h" @@ -100,6 +101,11 @@ class MessengerBuilder { // Set metric entity for use by RPC systems. MessengerBuilder &set_metric_entity(const scoped_refptr<MetricEntity>& metric_entity); + // Configure the messenger to enable TLS encryption on inbound connections. + // The 'server_uuid' will be used as the subject name for the server's + // certificate. + MessengerBuilder& enable_inbound_tls(std::string server_uuid); + Status Build(std::shared_ptr<Messenger> *msgr); private: @@ -110,6 +116,7 @@ class MessengerBuilder { int max_negotiation_threads_; MonoDelta coarse_timer_granularity_; scoped_refptr<MetricEntity> metric_entity_; + boost::optional<string> enable_inbound_tls_server_uuid_; }; // A Messenger is a container for the reactor threads which run event loops http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/src/kudu/security/CMakeLists.txt b/src/kudu/security/CMakeLists.txt index d539e25..9126f4b 100644 --- a/src/kudu/security/CMakeLists.txt +++ b/src/kudu/security/CMakeLists.txt @@ -61,7 +61,6 @@ set(SECURITY_SRCS init.cc openssl_util.cc ${PORTED_X509_CHECK_HOST_CC} - server_cert_manager.cc tls_context.cc tls_handshake.cc tls_socket.cc http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/ca/cert_management.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/ca/cert_management.cc b/src/kudu/security/ca/cert_management.cc index 9a4d9e0..bd690af 100644 --- a/src/kudu/security/ca/cert_management.cc +++ b/src/kudu/security/ca/cert_management.cc @@ -56,9 +56,6 @@ template<> struct SslTypeTraits<ASN1_INTEGER> { template<> struct SslTypeTraits<BIGNUM> { static constexpr auto free = &BN_free; }; -template<> struct SslTypeTraits<EVP_PKEY> { - static constexpr auto free = &EVP_PKEY_free; -}; namespace ca { @@ -111,9 +108,9 @@ Status CertRequestGeneratorBase::GenerateRequest(const PrivateKey& key, } Status CertRequestGeneratorBase::PushExtension(stack_st_X509_EXTENSION* st, - int32_t nid, const char* value) { + int32_t nid, StringPiece value) { auto ex = ssl_make_unique( - X509V3_EXT_conf_nid(nullptr, nullptr, nid, const_cast<char*>(value))); + X509V3_EXT_conf_nid(nullptr, nullptr, nid, const_cast<char*>(value.data()))); if (!ex) { return Status::RuntimeError("error configuring extension"); } @@ -123,15 +120,12 @@ Status CertRequestGeneratorBase::PushExtension(stack_st_X509_EXTENSION* st, } CertRequestGenerator::CertRequestGenerator(Config config) - : CertRequestGeneratorBase(config), - extensions_(nullptr), - is_initialized_(false) { + : CertRequestGeneratorBase(config) { } Status CertRequestGenerator::Init() { InitializeOpenSSL(); - lock_guard<simple_spinlock> guard(lock_); CHECK(!is_initialized_); if (config_.uuid.empty()) { return Status::InvalidArgument("missing end-entity UUID/name"); @@ -151,14 +145,21 @@ Status CertRequestGenerator::Init() { // The generated certificates are for using as TLS certificates for // both client and server. - RETURN_NOT_OK(PushExtension(extensions_, NID_key_usage, - "critical,digitalSignature,keyEncipherment")); + string usage = "critical,digitalSignature,keyEncipherment"; + if (for_self_signing_) { + // If we are generating a CSR for self-signing, then we need to + // add this keyUsage attribute. See https://s.apache.org/BFHk + usage += ",keyCertSign"; + } + + RETURN_NOT_OK(PushExtension(extensions_, NID_key_usage, usage)); // The generated certificates should be good for authentication // of a server to a client and vice versa: the intended users of the // certificates are tablet servers which are going to talk to master // and other tablet servers via TLS channels. RETURN_NOT_OK(PushExtension(extensions_, NID_ext_key_usage, "critical,serverAuth,clientAuth")); + // The generated certificates are not intended to be used as CA certificates // (i.e. they cannot be used to sign/issue certificates). RETURN_NOT_OK(PushExtension(extensions_, NID_basic_constraints, @@ -206,7 +207,6 @@ Status CertRequestGenerator::Init() { } bool CertRequestGenerator::Initialized() const { - lock_guard<simple_spinlock> guard(lock_); return is_initialized_; } @@ -287,6 +287,24 @@ Status CertSigner::SelfSignCA(const PrivateKey& key, return Status::OK(); } +Status CertSigner::SelfSignCert(const PrivateKey& key, + CertRequestGenerator::Config config, + Cert* cert) { + // Generate a CSR. + CertSignRequest csr; + { + CertRequestGenerator gen(std::move(config)); + gen.enable_self_signing(); + RETURN_NOT_OK(gen.Init()); + RETURN_NOT_OK(gen.GenerateRequest(key, &csr)); + } + + // Self-sign the CSR with the key. + RETURN_NOT_OK(CertSigner(nullptr, &key).Sign(csr, cert)); + return Status::OK(); +} + + CertSigner::CertSigner(const Cert* ca_cert, const PrivateKey* ca_private_key) : ca_cert_(ca_cert), http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/ca/cert_management.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/ca/cert_management.h b/src/kudu/security/ca/cert_management.h index 203a810..29558a1 100644 --- a/src/kudu/security/ca/cert_management.h +++ b/src/kudu/security/ca/cert_management.h @@ -25,6 +25,7 @@ #include <vector> #include "kudu/gutil/macros.h" +#include "kudu/gutil/strings/stringpiece.h" #include "kudu/security/crypto.h" #include "kudu/security/openssl_util.h" #include "kudu/util/locks.h" @@ -78,7 +79,7 @@ class CertRequestGeneratorBase { protected: // Push the specified extension into the stack provided. static Status PushExtension(stack_st_X509_EXTENSION* st, int32_t nid, - const char* value); + StringPiece value); // Set the certificate-specific extensions into the specified request. virtual Status SetExtensions(X509_REQ* req) const = 0; @@ -103,13 +104,19 @@ class CertRequestGenerator : public CertRequestGeneratorBase { Status Init() override; bool Initialized() const override; + CertRequestGenerator& enable_self_signing() { + CHECK(!is_initialized_); + for_self_signing_ = true; + return *this; + } + protected: Status SetExtensions(X509_REQ* req) const override; private: - stack_st_X509_EXTENSION* extensions_; - mutable simple_spinlock lock_; - bool is_initialized_; // protected by lock_ + stack_st_X509_EXTENSION* extensions_ = nullptr; + bool is_initialized_ = false; + bool for_self_signing_ = false; }; // An utility class that facilitates issuing of root CA self-signed certificate @@ -148,6 +155,12 @@ class CertSigner { CaCertRequestGenerator::Config config, Cert* cert); + // Generate a self-signed certificate using the given key and CSR + // configuration. + static Status SelfSignCert(const PrivateKey& key, + CertRequestGenerator::Config config, + Cert* cert); + // Create a CertSigner. // // The given cert and key must stay valid for the lifetime of the http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/cert.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/cert.cc b/src/kudu/security/cert.cc index a75d33a..3efb8f8 100644 --- a/src/kudu/security/cert.cc +++ b/src/kudu/security/cert.cc @@ -135,6 +135,13 @@ void Cert::AdoptAndAddRefRawData(X509* data) { AdoptRawData(data); } +Status Cert::GetPublicKey(PublicKey* key) const { + EVP_PKEY* raw_key = X509_get_pubkey(data_.get()); + OPENSSL_RET_IF_NULL(raw_key, "unable to get certificate public key"); + key->AdoptRawData(raw_key); + return Status::OK(); +} + Status CertSignRequest::FromString(const std::string& data, DataFormat format) { return ::kudu::security::FromString(data, format, &data_); } @@ -147,5 +154,21 @@ Status CertSignRequest::FromFile(const std::string& fpath, DataFormat format) { return ::kudu::security::FromFile(fpath, format, &data_); } +CertSignRequest CertSignRequest::Clone() const { + CHECK_GT(CRYPTO_add(&data_->references, 1, CRYPTO_LOCK_X509_REQ), 1) + << "X509_REQ use-after-free detected"; + + CertSignRequest clone; + clone.AdoptRawData(GetRawData()); + return clone; +} + +Status CertSignRequest::GetPublicKey(PublicKey* key) const { + EVP_PKEY* raw_key = X509_REQ_get_pubkey(data_.get()); + OPENSSL_RET_IF_NULL(raw_key, "unable to get CSR public key"); + key->AdoptRawData(raw_key); + return Status::OK(); +} + } // namespace security } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/cert.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/cert.h b/src/kudu/security/cert.h index fc2464b..b460c31 100644 --- a/src/kudu/security/cert.h +++ b/src/kudu/security/cert.h @@ -28,6 +28,7 @@ class Status; namespace security { class PrivateKey; +class PublicKey; // Convert an X509_NAME object to a human-readable string. std::string X509NameToString(X509_NAME* name); @@ -51,6 +52,9 @@ class Cert : public RawDataWrapper<X509> { // Adopts the provided X509 certificate, and increments the reference count. void AdoptAndAddRefRawData(X509* data); + + // Returns the certificate's public key. + Status GetPublicKey(PublicKey* key) const; }; class CertSignRequest : public RawDataWrapper<X509_REQ> { @@ -58,6 +62,12 @@ class CertSignRequest : public RawDataWrapper<X509_REQ> { Status FromString(const std::string& data, DataFormat format); Status ToString(std::string* data, DataFormat format) const; Status FromFile(const std::string& fpath, DataFormat format); + + // Returns a shallow clone of the CSR (only a reference count is incremented). + CertSignRequest Clone() const; + + // Returns the CSR's public key. + Status GetPublicKey(PublicKey* key) const; }; } // namespace security http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/crypto.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/crypto.cc b/src/kudu/security/crypto.cc index dda9da3..4488384 100644 --- a/src/kudu/security/crypto.cc +++ b/src/kudu/security/crypto.cc @@ -63,9 +63,6 @@ int DerWritePublicKey(BIO* bio, EVP_PKEY* key) { template<> struct SslTypeTraits<BIGNUM> { static constexpr auto free = &BN_free; }; -template<> struct SslTypeTraits<EVP_PKEY> { - static constexpr auto free = &EVP_PKEY_free; -}; struct RsaPrivateKeyTraits : public SslTypeTraits<EVP_PKEY> { static constexpr auto read_pem = &PEM_read_bio_PrivateKey; static constexpr auto read_der = &d2i_PrivateKey_bio; @@ -151,6 +148,23 @@ Status PublicKey::VerifySignature(DigestType digest, return Status::OK(); } +Status PublicKey::Equals(const PublicKey& other, bool* equals) const { + int cmp = EVP_PKEY_cmp(data_.get(), other.data_.get()); + switch (cmp) { + case -2: + return Status::NotSupported("failed to compare public keys"); + case -1: // Key types are different; treat this as not equal + case 0: // Keys are not equal + *equals = false; + return Status::OK(); + case 1: + *equals = true; + return Status::OK(); + default: + return Status::RuntimeError("unexpected public key comparison result", std::to_string(cmp)); + } +} + Status PrivateKey::FromString(const std::string& data, DataFormat format) { return ::kudu::security::FromString<RawDataType, RsaPrivateKeyTraits>( data, format, &data_); http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/crypto.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/crypto.h b/src/kudu/security/crypto.h index ce951b9..c9dd380 100644 --- a/src/kudu/security/crypto.h +++ b/src/kudu/security/crypto.h @@ -55,6 +55,9 @@ class PublicKey : public RawDataWrapper<EVP_PKEY> { Status VerifySignature(DigestType digest, const std::string& data, const std::string& signature) const; + + // Sets 'equals' to true if the other public key equals this. + Status Equals(const PublicKey& other, bool* equals) const; }; // A class with generic private key interface, but actually it represents http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/openssl_util.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/openssl_util.h b/src/kudu/security/openssl_util.h index 35ff8d6..c1a4ecf 100644 --- a/src/kudu/security/openssl_util.h +++ b/src/kudu/security/openssl_util.h @@ -100,6 +100,9 @@ template<> struct SslTypeTraits<X509_REQ> { static constexpr auto write_pem = &PEM_write_bio_X509_REQ; static constexpr auto write_der = &i2d_X509_REQ_bio; }; +template<> struct SslTypeTraits<EVP_PKEY> { + static constexpr auto free = &EVP_PKEY_free; +}; template<typename SSL_TYPE, typename Traits = SslTypeTraits<SSL_TYPE>> c_unique_ptr<SSL_TYPE> ssl_make_unique(SSL_TYPE* d) { http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/server_cert_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/server_cert_manager.cc b/src/kudu/security/server_cert_manager.cc deleted file mode 100644 index 61da08d..0000000 --- a/src/kudu/security/server_cert_manager.cc +++ /dev/null @@ -1,119 +0,0 @@ -// 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/security/server_cert_manager.h" - -#include <memory> -#include <string> - -#include <boost/optional/optional.hpp> -#include <gflags/gflags.h> - -#include "kudu/util/flag_tags.h" -#include "kudu/security/ca/cert_management.h" -#include "kudu/security/cert.h" -#include "kudu/security/openssl_util.h" - -using std::string; -using std::unique_ptr; - -DEFINE_int32(server_rsa_key_length_bits, 2048, - "The number of bits to use for the server's private RSA key. This is used " - "for TLS connections to and from clients and other servers."); -TAG_FLAG(server_rsa_key_length_bits, experimental); - -namespace kudu { -namespace security { - -using ca::CertRequestGenerator; - -ServerCertManager::ServerCertManager(string server_uuid) - : server_uuid_(std::move(server_uuid)) { -} - -ServerCertManager::~ServerCertManager() { -} - -Status ServerCertManager::Init() { - MutexLock lock(lock_); - CHECK(!key_); - - unique_ptr<PrivateKey> key(new PrivateKey()); - RETURN_NOT_OK_PREPEND(GeneratePrivateKey(FLAGS_server_rsa_key_length_bits, - key.get()), - "could not generate private key"); - - // TODO(aserbin): do these fields actually have to be set? - const CertRequestGenerator::Config config = { - "US", // country - "CA", // state - "San Francisco", // locality - "ASF", // org - "The Kudu Project", // unit - server_uuid_, // uuid - "", // comment - {"localhost"}, // hostnames TODO(PKI): use real hostnames - {"127.0.0.1"}, // ips - }; - - CertRequestGenerator gen(config); - CertSignRequest csr; - RETURN_NOT_OK_PREPEND(gen.Init(), "could not initialize CSR generator"); - RETURN_NOT_OK_PREPEND(gen.GenerateRequest(*key, &csr), "could not generate CSR"); - RETURN_NOT_OK_PREPEND(csr.ToString(&csr_der_, DataFormat::DER), - "unable to output DER format CSR"); - key_ = std::move(key); - return Status::OK(); -} - - -Status ServerCertManager::AdoptSignedCert(const string& cert_der) { - MutexLock lock(lock_); - // If we already have a cert, then no need to adopt a new one. - // We heartbeat to multiple masters in parallel, and so we might - // get multiple valid signed certs from different masters. - if (signed_cert_) { - return Status::OK(); - } - unique_ptr<Cert> new_cert(new Cert()); - RETURN_NOT_OK_PREPEND(new_cert->FromString(cert_der, DataFormat::DER), - "could not parse DER data"); - RETURN_NOT_OK_PREPEND(new_cert->CheckKeyMatch(*key_), - "signed certificate does not match the private key " - "of the original CSR"); - LOG(INFO) << "Adopting new signed X509 certificate"; - signed_cert_ = std::move(new_cert); - // No longer need to get a signed cert, so we can forget our CSR. - csr_der_.clear(); - return Status::OK(); -} - -// Return a new CSR, if this tablet server's cert has not yet been -// signed. If the cert is already signed, returns null. -boost::optional<string> ServerCertManager::GetCSRIfNecessary() const { - MutexLock lock(lock_); - if (signed_cert_) return boost::none; - return csr_der_; -} - -bool ServerCertManager::has_signed_cert() const { - MutexLock lock(lock_); - return signed_cert_ != nullptr; -} - -} // namespace security -} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/server_cert_manager.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/server_cert_manager.h b/src/kudu/security/server_cert_manager.h deleted file mode 100644 index 0a6a26c..0000000 --- a/src/kudu/security/server_cert_manager.h +++ /dev/null @@ -1,94 +0,0 @@ -// 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 <memory> -#include <string> - -#include <boost/optional/optional_fwd.hpp> - -#include "kudu/gutil/macros.h" -#include "kudu/util/mutex.h" -#include "kudu/util/status.h" - -namespace kudu { -namespace security { - -class Cert; -class CertSignRequest; -class PrivateKey; - -// Manages the X509 certificate used by a server when the built-in -// PKI infrastructure is enabled. -// -// This generates an X509 certificate for the server, provides a CSR which can be -// transferred to the master, and then stores the signed certificate once provided. -// -// Note that the master, in addition to acting as a CA, also needs its own cert -// (signed by the CA cert) which it uses for its RPC server. This handles the latter. -// -// This class is thread-safe after initialization. -class ServerCertManager { - public: - // Construct a ServertCertManager. - explicit ServerCertManager(std::string server_uuid); - ~ServerCertManager(); - - // Generate a private key and CSR. - // - // This must be called exactly once before any methods below. - Status Init(); - - // Return a new CSR in DER format, if this server's cert has not yet been - // signed. If the cert is already signed, returns boost::none; - boost::optional<std::string> GetCSRIfNecessary() const; - - // Adopt the given signed cert (provided in DER format) as the cert for - // this server. - // - // This has no effect if the instance already has a signed cert. - Status AdoptSignedCert(const std::string& cert_der); - - // TODO(PKI): some code to register a callback when the cert is adopted, - // so that it can set it on the SSLFactory? - - // Return true if we currently have a signed certificate. - // TODO(PKI): should this check validity? - bool has_signed_cert() const; - - private: - const std::string server_uuid_; - - // Protects the below variables. - mutable Mutex lock_; - - // The CSR for this server. If the cert has already been - // signed, set to empty. - std::string csr_der_; - - // The signed cert for this server. Only set once the cert - // has been signed. - std::unique_ptr<Cert> signed_cert_; - - // The keypair associated with this server's cert. - std::unique_ptr<PrivateKey> key_; - - DISALLOW_COPY_AND_ASSIGN(ServerCertManager); -}; - -} // namespace security -} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index 8e46deb..cae2b38 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -19,23 +19,34 @@ #include <string> +#include <gflags/gflags.h> #include <openssl/err.h> #include <openssl/ssl.h> #include <openssl/x509.h> +#include <openssl/x509v3.h> #include "kudu/gutil/strings/substitute.h" +#include "kudu/security/ca/cert_management.h" #include "kudu/security/cert.h" #include "kudu/security/crypto.h" #include "kudu/security/openssl_util.h" #include "kudu/security/tls_handshake.h" +#include "kudu/util/flag_tags.h" #include "kudu/util/status.h" using strings::Substitute; using std::string; +DEFINE_int32(server_rsa_key_length_bits, 2048, + "The number of bits to use for the server's private RSA key. This is used " + "for TLS connections to and from clients and other servers."); +TAG_FLAG(server_rsa_key_length_bits, experimental); + namespace kudu { namespace security { +using ca::CertRequestGenerator; + template<> struct SslTypeTraits<SSL> { static constexpr auto free = &SSL_free; }; @@ -46,10 +57,9 @@ template<> struct SslTypeTraits<X509_STORE_CTX> { static constexpr auto free = &X509_STORE_CTX_free; }; - TlsContext::TlsContext() - : has_cert_(false), - trusted_cert_count_(0) { + : trusted_cert_count_(0), + has_cert_(false) { security::InitializeOpenSSL(); } @@ -108,28 +118,29 @@ Status TlsContext::VerifyCertChain(const Cert& cert) { } return Status::RuntimeError( - Substitute("could not verify cert chain$0", cert_details), + Substitute("could not verify certificate chain$0", cert_details), X509_verify_cert_error_string(err)); } return Status::OK(); } Status TlsContext::UseCertificateAndKey(const Cert& cert, const PrivateKey& key) { - ERR_clear_error(); - - // Verify that the cert and key match. - RETURN_NOT_OK(cert.CheckKeyMatch(key)); - // 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(VerifyCertChain(cert)); + // Verify that the cert and key match. + RETURN_NOT_OK(cert.CheckKeyMatch(key)); + + MutexLock lock(lock_); + CHECK(!has_cert_); + OPENSSL_RET_NOT_OK(SSL_CTX_use_PrivateKey(ctx_.get(), key.GetRawData()), "failed to use private key"); OPENSSL_RET_NOT_OK(SSL_CTX_use_certificate(ctx_.get(), cert.GetRawData()), "failed to use certificate"); - has_cert_.Store(true); + has_cert_ = true; return Status::OK(); } @@ -146,11 +157,112 @@ Status TlsContext::AddTrustedCertificate(const Cert& cert) { if (ERR_GET_LIB(err) == ERR_LIB_X509 && ERR_GET_REASON(err) == X509_R_CERT_ALREADY_IN_HASH_TABLE) { ERR_clear_error(); - return Status::AlreadyPresent("cert already trusted"); + return Status::AlreadyPresent("certificate already trusted"); } OPENSSL_RET_NOT_OK(rc, "failed to add trusted certificate"); } - trusted_cert_count_.Increment(); + MutexLock lock(lock_); + trusted_cert_count_ += 1; + return Status::OK(); +} + +Status TlsContext::GenerateSelfSignedCertAndKey(const std::string& server_uuid) { + // Step 1: generate the private key to be self signed. + PrivateKey key; + RETURN_NOT_OK_PREPEND(GeneratePrivateKey(FLAGS_server_rsa_key_length_bits, + &key), + "failed to generate private key"); + + // Step 2: generate a CSR so that the self-signed cert can eventually be + // replaced with a CA-signed cert. + // TODO(aserbin): do these fields actually have to be set? + const CertRequestGenerator::Config config = { + "US", // country + "CA", // state + "San Francisco", // locality + "ASF", // org + "The Kudu Project", // unit + server_uuid, // uuid + "", // comment + {"localhost"}, // hostnames TODO(PKI): use real hostnames + {"127.0.0.1"}, // ips + }; + + CertRequestGenerator gen(config); + CertSignRequest csr; + RETURN_NOT_OK_PREPEND(gen.Init(), "could not initialize CSR generator"); + RETURN_NOT_OK_PREPEND(gen.GenerateRequest(key, &csr), "could not generate CSR"); + + // Step 3: generate a self-signed cert that we can use for terminating TLS + // connections until we get the CA-signed cert. + Cert cert; + RETURN_NOT_OK_PREPEND(ca::CertSigner::SelfSignCert(key, config, &cert), + "failed to self-sign cert"); + + // Workaround for an OpenSSL memory leak caused by a race in x509v3_cache_extensions. + // Upon first use of each certificate, this function gets called to parse various + // fields of the certificate. However, it's racey, so if multiple "first calls" + // happen concurrently, one call overwrites the cached data from another, causing + // a leak. Calling this nonsense X509_check_ca() forces the X509 extensions to + // get cached, so we don't hit the race later. 'VerifyCertChain' also has the + // effect of triggering the racy codepath. + ignore_result(X509_check_ca(cert.GetRawData())); + + // Step 4: Adopt the new key and cert. + MutexLock lock(lock_); + CHECK(!has_cert_); + OPENSSL_RET_NOT_OK(SSL_CTX_use_PrivateKey(ctx_.get(), key.GetRawData()), + "failed to use private key"); + OPENSSL_RET_NOT_OK(SSL_CTX_use_certificate(ctx_.get(), cert.GetRawData()), + "failed to use certificate"); + has_cert_ = true; + csr_ = std::move(csr); + return Status::OK(); +} + +boost::optional<CertSignRequest> TlsContext::GetCsrIfNecessary() const { + MutexLock lock(lock_); + if (csr_) { + return csr_->Clone(); + } + return boost::none; +} + +Status TlsContext::AdoptSignedCert(const Cert& cert) { + // 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(VerifyCertChain(cert)); + + MutexLock lock(lock_); + + if (!csr_) { + // A signed cert has already been adopted. + return Status::OK(); + } + + PublicKey csr_key; + RETURN_NOT_OK(csr_->GetPublicKey(&csr_key)); + PublicKey cert_key; + RETURN_NOT_OK(cert.GetPublicKey(&cert_key)); + bool equals; + RETURN_NOT_OK(csr_key.Equals(cert_key, &equals)); + if (!equals) { + return Status::RuntimeError("certificate public key does not match the CSR public key"); + } + + OPENSSL_RET_NOT_OK(SSL_CTX_use_certificate(ctx_.get(), cert.GetRawData()), + "failed to use certificate"); + + // This should never fail since we already compared the cert's public key + // against the CSR, but better safe than sorry. If this *does* fail, it + // appears to remove the private key from the SSL_CTX, so we are left in a bad + // state. + OPENSSL_CHECK_OK(SSL_CTX_check_private_key(ctx_.get())) + << "certificate does not match the private key"; + + csr_ = boost::none; + return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/tls_context.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.h b/src/kudu/security/tls_context.h index dfc994f..e1bf1a9 100644 --- a/src/kudu/security/tls_context.h +++ b/src/kudu/security/tls_context.h @@ -20,8 +20,12 @@ #include <functional> #include <string> +#include <boost/optional.hpp> + +#include "kudu/security/cert.h" #include "kudu/security/tls_handshake.h" #include "kudu/util/atomic.h" +#include "kudu/util/mutex.h" #include "kudu/util/status.h" namespace kudu { @@ -30,9 +34,33 @@ namespace security { class Cert; class PrivateKey; -// TlsContext wraps data required by the OpenSSL library for creating TLS -// protected channels. A single TlsContext instance should be used per server or -// client instance. +// TlsContext wraps data required by the OpenSSL library for creating and +// accepting TLS protected channels. A single TlsContext instance should be used +// per server or client instance. +// +// Internally, a 'TlsContext' manages a single keypair which it uses for +// terminating TLS connections. It also manages a collection of trusted root CA +// certificates (a trust store), as well as a signed certificate for the +// keypair. +// +// When used on a server, the TlsContext can generate a keypair and a +// self-signed certificate, and provide a CSR for transititioning to a CA-signed +// certificate. This allows Kudu servers to start with a self-signed +// certificate, and later adopt a CA-signed certificate as it becomes available. +// See GenerateSelfSignedCertAndKey(), GetCsrIfNecessary(), and +// AdoptSignedCert() for details on how to generate the keypair and self-signed +// cert, access the CSR, and transtition to a CA-signed cert, repectively. +// +// When used in a client or a server, the TlsContext can immediately adopt a +// private key and CA-signed cert using UseCertificateAndKey(). A TlsContext +// only manages a single keypair, so if UseCertificateAndKey() is called, +// GenerateSelfSignedCertAndKey() must not be called, and vice versa. +// +// TlsContext may be used with or without a keypair and cert to initiate TLS +// connections, when mutual TLS authentication is not needed (for example, for +// token or Kerberos authenticated connections). +// +// This class is thread-safe after initialization. class TlsContext { public: @@ -43,27 +71,61 @@ class TlsContext { Status Init(); - // Return true if this TlsContext has been configured with a cert and key to - // accept TLS connections. - bool has_cert() const { return has_cert_.Load(); } + // Returns true if this TlsContext has been configured with a cert and key for + // use with TLS-encrypted connections. + bool has_cert() const { + MutexLock lock(lock_); + return has_cert_; + } - // Use 'cert' and 'key' as the cert/key for this server/client. - // - // If the cert is not self-signed, checks that the CA that issued - // the signature on 'cert' is already trusted by this context - // (e.g. by AddTrustedCertificate). - Status UseCertificateAndKey(const Cert& cert, const PrivateKey& key); + // Returns true if this TlsContext has been configured with a CA-signed TLS + // cert and key for use with TLS-encrypted connections. + bool has_signed_cert() const { + MutexLock lock(lock_); + return has_cert_ && !csr_; + } - // Add 'cert' as a trusted certificate for this server/client. + // Adds 'cert' as a trusted root CA certificate. // - // This determines whether other peers are trusted. It also must - // be called for any CA certificates that are part of the certificate - // chain for the cert passed in 'UseCertificate' above. + // This determines whether other peers are trusted. It also must be called for + // any CA certificates that are part of the certificate chain for the cert + // passed in to 'UseCertificateAndKey()' or 'AdoptSignedCert()'. // // Returns AlreadyPresent if the cert is already marked as trusted. Other // OpenSSL errors will be RuntimeError. Status AddTrustedCertificate(const Cert& cert); + // Uses 'cert' and 'key' as the cert and key for use with TLS connections. + // + // Checks that the CA that issued the signature on 'cert' is already trusted + // by this context (e.g. by AddTrustedCertificate()). + Status UseCertificateAndKey(const Cert& cert, const PrivateKey& key); + + // Generates a self-signed cert and key for use with TLS connections. + // + // This method should only be used on the server. Once this method is called, + // 'GetCsrIfNecessary' can be used to retrieve a CSR for generating a + // CA-signed cert for the generated private key, and 'AdoptSignedCert' can be + // used to transition to using the CA-signed cert with subsequent TLS + // connections. + Status GenerateSelfSignedCertAndKey(const std::string& server_uuid); + + // Returns a new certificate signing request (CSR) in DER format, if this + // context's cert is self-signed. If the cert is already signed, returns + // boost::none. + boost::optional<CertSignRequest> GetCsrIfNecessary() const; + + // Adopts the provided CA-signed certificate for this TLS context. + // + // The certificate must correspond to a CSR previously returned by + // 'GetCsrIfNecessary()'. + // + // Checks that the CA that issued the signature on 'cert' is already trusted + // by this context (e.g. by AddTrustedCertificate()). + // + // This has no effect if the instance already has a CA-signed cert. + Status AdoptSignedCert(const Cert& cert); + // Convenience functions for loading cert/CA/key from file paths. // ------------------------------------------------------------- @@ -80,18 +142,23 @@ class TlsContext { // Return the number of certs that have been marked as trusted. // Used by tests. int trusted_cert_count_for_tests() const { - return trusted_cert_count_.Load(); + MutexLock lock(lock_); + return trusted_cert_count_; } private: - Status VerifyCertChain(const Cert& cert); - - AtomicBool has_cert_; - AtomicInt<int32> trusted_cert_count_; + Status VerifyCertChain(const Cert& cert); // Owned SSL context. c_unique_ptr<SSL_CTX> ctx_; + + // Protexts trusted_cert_count_, has_cert_ and csr_, as well as ctx_ when it + // needs to be updated transactionally with has_cert_ and csr_. + mutable Mutex lock_; + int32_t trusted_cert_count_; + bool has_cert_; + boost::optional<CertSignRequest> csr_; }; } // namespace security http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/security/tls_handshake-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_handshake-test.cc b/src/kudu/security/tls_handshake-test.cc index 337a7af..0d8b9a8 100644 --- a/src/kudu/security/tls_handshake-test.cc +++ b/src/kudu/security/tls_handshake-test.cc @@ -18,28 +18,79 @@ #include "kudu/security/tls_handshake.h" #include <functional> +#include <iostream> #include <string> +#include <boost/optional.hpp> +#include <gflags/gflags.h> #include <gtest/gtest.h> +#include "kudu/security/ca/cert_management.h" +#include "kudu/security/crypto.h" #include "kudu/security/security-test-util.h" #include "kudu/security/tls_context.h" #include "kudu/util/test_util.h" using std::string; +DECLARE_int32(server_rsa_key_length_bits); + namespace kudu { namespace security { -class TestTlsHandshake : public KuduTest { +using ca::CertSigner; + +enum class CertType { + NONE, + SELF_SIGNED, + SIGNED, +}; + +struct Case { + CertType client_cert; + TlsVerificationMode client_verification; + CertType server_cert; + TlsVerificationMode server_verification; + Status expected_status; +}; + +// Beautifies CLI test output. +std::ostream& operator<<(std::ostream& o, Case c) { + + auto cert_type_name = [] (const CertType& cert_type) { + switch (cert_type) { + case CertType::NONE: return "NONE"; + case CertType::SELF_SIGNED: return "SELF_SIGNED"; + case CertType::SIGNED: return "SIGNED"; + } + }; + + auto verification_mode_name = [] (const TlsVerificationMode& verification_mode) { + switch (verification_mode) { + case TlsVerificationMode::VERIFY_NONE: return "NONE"; + case TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST: return "REMOTE_CERT_AND_HOST"; + } + }; + + o << "{client-cert: " << cert_type_name(c.client_cert) << ", " + << "client-verification: " << verification_mode_name(c.client_verification) << ", " + << "server-cert: " << cert_type_name(c.client_cert) << ", " + << "server-verification: " << verification_mode_name(c.client_verification) << ", " + << "expected-status: " << c.expected_status.ToString() << "}"; + + return o; +} + +class TestTlsHandshake : public KuduTest, + public ::testing::WithParamInterface<Case> { public: void SetUp() override { KuduTest::SetUp(); - key_path_ = GetTestPath("key.pem"); - cert_path_ = GetTestPath("cert.pem"); - ASSERT_OK(CreateSSLServerCert(cert_path_)); - ASSERT_OK(CreateSSLPrivateKey(key_path_)); + // Tune down the RSA key length in order to speed up tests. We would tune it + // smaller, but at 512 bits OpenSSL returns a "digest too big for rsa key" + // error during negotiation. + FLAGS_server_rsa_key_length_bits = 1024; ASSERT_OK(client_tls_.Init()); ASSERT_OK(server_tls_.Init()); @@ -93,12 +144,38 @@ class TestTlsHandshake : public KuduTest { string key_path_; }; -TEST_F(TestTlsHandshake, TestSuccessfulHandshake) { +namespace { +Status InitTlsContextCert(const PrivateKey& ca_key, + const Cert& ca_cert, + TlsContext* tls_context, + CertType cert_type) { + RETURN_NOT_OK(tls_context->AddTrustedCertificate(ca_cert)); + switch (cert_type) { + case CertType::SIGNED: { + Cert cert; + RETURN_NOT_OK(tls_context->GenerateSelfSignedCertAndKey("test-uuid")); + RETURN_NOT_OK(CertSigner(&ca_cert, &ca_key).Sign(*tls_context->GetCsrIfNecessary(), &cert)); + RETURN_NOT_OK(tls_context->AdoptSignedCert(cert)); + break; + } + case CertType::SELF_SIGNED: + RETURN_NOT_OK(tls_context->GenerateSelfSignedCertAndKey("test-uuid")); + break; + case CertType::NONE: + break; + } + return Status::OK(); +} +} // anonymous namespace + +TEST_F(TestTlsHandshake, TestHandshakeSequence) { + PrivateKey ca_key; + Cert ca_cert; + ASSERT_OK(GenerateSelfSignedCAForTests(&ca_key, &ca_cert)); + // Both client and server have certs and CA. - ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); - ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, key_path_)); - ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_)); - ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); + ASSERT_OK(InitTlsContextCert(ca_key, ca_cert, &client_tls_, CertType::SIGNED)); + ASSERT_OK(InitTlsContextCert(ca_key, ca_cert, &server_tls_, CertType::SIGNED)); TlsHandshake server; TlsHandshake client; @@ -129,59 +206,147 @@ TEST_F(TestTlsHandshake, TestSuccessfulHandshake) { ASSERT_EQ(buf2.size(), 0); } -// Client has no cert. -// Server has self-signed cert. -TEST_F(TestTlsHandshake, Test_ClientNone_ServerSelfSigned) { - ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); - - // If the client wants to verify the server, it should fail because - // the server cert is self-signed. - Status s = RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, - TlsVerificationMode::VERIFY_NONE); - ASSERT_STR_MATCHES(s.ToString(), "client error:.*certificate verify failed"); - - // If the client doesn't care, it should succeed against the self-signed - // server. - ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, - TlsVerificationMode::VERIFY_NONE)); - - // If the client loads the cert as trusted, then it should succeed - // with verification enabled. - ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); +// Tests that the TlsContext can transition from self signed cert to signed +// cert, and that it rejects invalid certs along the way. We are testing this +// here instead of in a dedicated TlsContext test because it requires completing +// handshakes to fully validate. +TEST_F(TestTlsHandshake, TestTlsContextCertTransition) { + ASSERT_FALSE(server_tls_.has_cert()); + ASSERT_FALSE(server_tls_.has_signed_cert()); + ASSERT_EQ(boost::none, server_tls_.GetCsrIfNecessary()); + + ASSERT_OK(server_tls_.GenerateSelfSignedCertAndKey("test-uuid")); + ASSERT_TRUE(server_tls_.has_cert()); + ASSERT_FALSE(server_tls_.has_signed_cert()); + ASSERT_NE(boost::none, server_tls_.GetCsrIfNecessary()); + ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, TlsVerificationMode::VERIFY_NONE)); + ASSERT_STR_MATCHES(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + TlsVerificationMode::VERIFY_NONE).ToString(), + "client error:.*certificate verify failed"); + + PrivateKey ca_key; + Cert ca_cert; + ASSERT_OK(GenerateSelfSignedCAForTests(&ca_key, &ca_cert)); + + Cert cert; + ASSERT_OK(CertSigner(&ca_cert, &ca_key).Sign(*server_tls_.GetCsrIfNecessary(), &cert)); + + // Try to adopt the cert without first trusting the CA. + ASSERT_STR_MATCHES(server_tls_.AdoptSignedCert(cert).ToString(), + "could not verify certificate chain"); + + // Check that we can still do (unverified) handshakes. + ASSERT_TRUE(server_tls_.has_cert()); + ASSERT_FALSE(server_tls_.has_signed_cert()); + ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, TlsVerificationMode::VERIFY_NONE)); + + // Trust the root cert. + ASSERT_OK(server_tls_.AddTrustedCertificate(ca_cert)); + + // Generate a bogus cert and attempt to adopt it. + Cert bogus_cert; + { + TlsContext bogus_tls; + ASSERT_OK(bogus_tls.Init()); + ASSERT_OK(bogus_tls.GenerateSelfSignedCertAndKey("test-uuid")); + ASSERT_OK(CertSigner(&ca_cert, &ca_key).Sign(*bogus_tls.GetCsrIfNecessary(), &bogus_cert)); + } + ASSERT_STR_MATCHES(server_tls_.AdoptSignedCert(bogus_cert).ToString(), + "certificate public key does not match the CSR public key"); + + // Check that we can still do (unverified) handshakes. + ASSERT_TRUE(server_tls_.has_cert()); + ASSERT_FALSE(server_tls_.has_signed_cert()); + ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, TlsVerificationMode::VERIFY_NONE)); + + // Adopt the legitimate signed cert. + ASSERT_OK(server_tls_.AdoptSignedCert(cert)); + + // Check that we can do verified handshakes. + ASSERT_TRUE(server_tls_.has_cert()); + ASSERT_TRUE(server_tls_.has_signed_cert()); + ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, TlsVerificationMode::VERIFY_NONE)); + ASSERT_OK(client_tls_.AddTrustedCertificate(ca_cert)); ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, TlsVerificationMode::VERIFY_NONE)); - - // If the server requires authentication of the client, the handshake should fail. - s = RunHandshake(TlsVerificationMode::VERIFY_NONE, - TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST); - ASSERT_TRUE(s.IsRuntimeError()); - ASSERT_STR_MATCHES(s.ToString(), "server error:.*peer did not return a certificate"); } -// TODO(PKI): this test has both the client and server using the same cert, -// which isn't very realistic. We should have this generate self-signed certs -// on the fly. -TEST_F(TestTlsHandshake, Test_ClientSelfSigned_ServerSelfSigned) { - ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, key_path_)); - ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); - ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); - ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_)); +TEST_P(TestTlsHandshake, TestHandshake) { + Case test_case = GetParam(); - // This scenario should succeed in all cases. - ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, - TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST)); + PrivateKey ca_key; + Cert ca_cert; + ASSERT_OK(GenerateSelfSignedCAForTests(&ca_key, &ca_cert)); - ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, - TlsVerificationMode::VERIFY_NONE)); + ASSERT_OK(InitTlsContextCert(ca_key, ca_cert, &client_tls_, test_case.client_cert)); + ASSERT_OK(InitTlsContextCert(ca_key, ca_cert, &server_tls_, test_case.server_cert)); - ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, - TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST)); + Status s = RunHandshake(test_case.client_verification, test_case.server_verification); - ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE, - TlsVerificationMode::VERIFY_NONE)); + EXPECT_EQ(test_case.expected_status.CodeAsString(), s.CodeAsString()); + ASSERT_STR_MATCHES(s.ToString(), test_case.expected_status.message().ToString()); } -// TODO(PKI): add test coverage for mismatched common-name in the cert. +INSTANTIATE_TEST_CASE_P(CertCombinations, + TestTlsHandshake, + ::testing::Values( + + // We don't test any cases where the server has no cert or the client + // has a self-signed cert, since we don't expect those to occur in + // practice. + + Case { CertType::NONE, TlsVerificationMode::VERIFY_NONE, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::RuntimeError("client error:.*certificate verify failed") }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_NONE, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::RuntimeError("server error:.*peer did not return a certificate") }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::RuntimeError("client error:.*certificate verify failed") }, + + Case { CertType::NONE, TlsVerificationMode::VERIFY_NONE, + CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_NONE, + CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::RuntimeError("server error:.*peer did not return a certificate") }, + Case { CertType::NONE, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::RuntimeError("server error:.*peer did not return a certificate") }, + + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::RuntimeError("client error:.*certificate verify failed") }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::OK() }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SELF_SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::RuntimeError("client error:.*certificate verify failed") }, + + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + Status::OK() }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_NONE, + CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::OK() }, + Case { CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + CertType::SIGNED, TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST, + Status::OK() } +)); } // namespace security } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/server/server_base.cc ---------------------------------------------------------------------- diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc index 9125fa1..e8f9578 100644 --- a/src/kudu/server/server_base.cc +++ b/src/kudu/server/server_base.cc @@ -32,7 +32,6 @@ #include "kudu/gutil/walltime.h" #include "kudu/rpc/messenger.h" #include "kudu/security/init.h" -#include "kudu/security/server_cert_manager.h" #include "kudu/server/default-path-handlers.h" #include "kudu/server/generic_service.h" #include "kudu/server/glog_metrics.h" @@ -188,18 +187,15 @@ Status ServerBase::Init() { } RETURN_NOT_OK_PREPEND(s, "Failed to load FS layout"); - // Initialize the cert manager. - // TODO(PKI): make built-in PKI enabled/disabled based on a flag. - cert_manager_.reset(new security::ServerCertManager(fs_manager_->uuid())); - RETURN_NOT_OK(cert_manager_->Init()); - // Create the Messenger. rpc::MessengerBuilder builder(name_); - builder.set_num_reactors(FLAGS_num_reactor_threads); - builder.set_min_negotiation_threads(FLAGS_min_negotiation_threads); - builder.set_max_negotiation_threads(FLAGS_max_negotiation_threads); - builder.set_metric_entity(metric_entity()); + builder.set_num_reactors(FLAGS_num_reactor_threads) + .set_min_negotiation_threads(FLAGS_min_negotiation_threads) + .set_max_negotiation_threads(FLAGS_max_negotiation_threads) + .set_metric_entity(metric_entity()) + // TODO(PKI): make built-in PKI enabled/disabled based on a flag. + .enable_inbound_tls(fs_manager_->uuid()); RETURN_NOT_OK(builder.Build(&messenger_)); RETURN_NOT_OK(rpc_server_->Init(messenger_)); http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/server/server_base.h ---------------------------------------------------------------------- diff --git a/src/kudu/server/server_base.h b/src/kudu/server/server_base.h index 4bd39b5..6f17985 100644 --- a/src/kudu/server/server_base.h +++ b/src/kudu/server/server_base.h @@ -23,6 +23,7 @@ #include "kudu/gutil/gscoped_ptr.h" #include "kudu/gutil/macros.h" #include "kudu/gutil/ref_counted.h" +#include "kudu/rpc/messenger.h" #include "kudu/rpc/service_if.h" #include "kudu/server/server_base_options.h" #include "kudu/util/status.h" @@ -43,12 +44,11 @@ class Thread; class Webserver; namespace rpc { -class Messenger; class ServiceIf; } // namespace rpc namespace security { -class ServerCertManager; +class TlsContext; } // namespace security namespace server { @@ -76,7 +76,8 @@ class ServerBase { FsManager* fs_manager() { return fs_manager_.get(); } - security::ServerCertManager* cert_manager() { return cert_manager_.get(); } + const security::TlsContext& tls_context() { return messenger_->tls_context(); } + security::TlsContext* mutable_tls_context() { return messenger_->mutable_tls_context(); } // Return the instance identifier of this server. // This may not be called until after the server is Started. @@ -116,10 +117,6 @@ class ServerBase { gscoped_ptr<RpcServer> rpc_server_; gscoped_ptr<Webserver> web_server_; - // Manager for the SSL certificate associated with this server, if built-in - // PKI is enabled. - std::unique_ptr<security::ServerCertManager> cert_manager_; - std::shared_ptr<rpc::Messenger> messenger_; scoped_refptr<rpc::ResultTracker> result_tracker_; bool is_first_run_; http://git-wip-us.apache.org/repos/asf/kudu/blob/098a1e58/src/kudu/tserver/heartbeater.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/heartbeater.cc b/src/kudu/tserver/heartbeater.cc index fbae7e8..d95c836 100644 --- a/src/kudu/tserver/heartbeater.cc +++ b/src/kudu/tserver/heartbeater.cc @@ -30,7 +30,8 @@ #include "kudu/gutil/ref_counted.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/master.proxy.h" -#include "kudu/security/server_cert_manager.h" +#include "kudu/security/cert.h" +#include "kudu/security/tls_context.h" #include "kudu/server/webserver.h" #include "kudu/tserver/tablet_server.h" #include "kudu/tserver/tablet_server_options.h" @@ -360,9 +361,10 @@ Status Heartbeater::Thread::DoHeartbeat() { // Check with the TS cert manager if it has a cert that needs signing. // if so, send the CSR in the heartbeat for the master to sign. - boost::optional<string> csr = server_->cert_manager()->GetCSRIfNecessary(); + boost::optional<security::CertSignRequest> csr = + server_->mutable_tls_context()->GetCsrIfNecessary(); if (csr != boost::none) { - req.mutable_csr_der()->swap(*csr); + RETURN_NOT_OK(csr->ToString(req.mutable_csr_der(), security::DataFormat::DER)); VLOG(1) << "Sending a CSR to the master in the next heartbeat"; } @@ -413,11 +415,25 @@ Status Heartbeater::Thread::DoHeartbeat() { last_hb_response_.Swap(&resp); + for (const auto& ca_cert_der : last_hb_response_.ca_cert_der()) { + security::Cert ca_cert; + RETURN_NOT_OK_PREPEND( + ca_cert.FromString(ca_cert_der, security::DataFormat::DER), + "failed to parse CA certificate from master"); + RETURN_NOT_OK_PREPEND( + server_->mutable_tls_context()->AddTrustedCertificate(ca_cert), + "failed to adopt master-signed X509 cert"); + } + // If we have a new signed certificate from the master, adopt it. if (last_hb_response_.has_signed_cert_der()) { + security::Cert cert; + RETURN_NOT_OK_PREPEND( + cert.FromString(last_hb_response_.signed_cert_der(), security::DataFormat::DER), + "failed to parse signed certificate from master"); RETURN_NOT_OK_PREPEND( - server_->cert_manager()->AdoptSignedCert(last_hb_response_.signed_cert_der()), - "could not adopt master-signed X509 cert"); + server_->mutable_tls_context()->AdoptSignedCert(cert), + "failed to adopt master-signed X509 cert"); } MarkTabletReportAcknowledged(req.tablet_report());
