Repository: kudu Updated Branches: refs/heads/master 93df70337 -> c4c770a45
client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Reviewed-on: http://gerrit.cloudera.org:8080/5899 Tested-by: Kudu Jenkins 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/c4c770a4 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c4c770a4 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c4c770a4 Branch: refs/heads/master Commit: c4c770a45c19d564b1df8ce2ccd21f5e10254b2c Parents: 93df703 Author: Todd Lipcon <[email protected]> Authored: Fri Feb 3 13:54:52 2017 -0800 Committer: Dan Burkert <[email protected]> Committed: Wed Feb 8 20:01:12 2017 +0000 ---------------------------------------------------------------------- src/kudu/client/client-internal.cc | 33 ++++++++++++++++++++++++++++++++- src/kudu/client/client-internal.h | 6 ++++++ src/kudu/client/client-test.cc | 9 +++++++++ src/kudu/client/client.h | 1 + src/kudu/master/master.proto | 4 ++++ src/kudu/rpc/messenger.h | 1 + src/kudu/security/tls_context.cc | 18 +++++++++++++++--- src/kudu/security/tls_context.h | 11 +++++++++++ 8 files changed, 79 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/client/client-internal.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-internal.cc b/src/kudu/client/client-internal.cc index 6cdd335..b328bf5 100644 --- a/src/kudu/client/client-internal.cc +++ b/src/kudu/client/client-internal.cc @@ -37,10 +37,13 @@ #include "kudu/master/master.h" #include "kudu/master/master.pb.h" #include "kudu/master/master.proxy.h" +#include "kudu/rpc/messenger.h" #include "kudu/rpc/request_tracker.h" #include "kudu/rpc/rpc.h" #include "kudu/rpc/rpc_controller.h" #include "kudu/rpc/rpc_header.pb.h" +#include "kudu/security/cert.h" +#include "kudu/security/tls_context.h" #include "kudu/util/logging.h" #include "kudu/util/net/dns_resolver.h" #include "kudu/util/net/net_util.h" @@ -603,12 +606,40 @@ Status KuduClient::Data::GetTableSchema(KuduClient* client, void KuduClient::Data::ConnectedToClusterCb( const Status& status, const Sockaddr& leader_addr, - const master::ConnectToMasterResponsePB& /*connect_response*/) { + const master::ConnectToMasterResponsePB& connect_response) { + + // Ensure that all of the CAs reported by the master are trusted + // in our local TLS configuration. + if (status.ok()) { + for (const string& cert_der : connect_response.ca_cert_der()) { + security::Cert cert; + Status s = cert.FromString(cert_der, security::DataFormat::DER); + if (!s.ok()) { + KLOG_EVERY_N_SECS(WARNING, 5) << "Master " << leader_addr.ToString() + << " provided an unparseable CA cert: " + << s.ToString(); + continue; + } + s = messenger_->mutable_tls_context()->AddTrustedCertificate(cert); + // In the case that we are just re-connecting, then the attempt to trust the cert + // will return AlreadyPresent, since we already have the cert trusted. + if (!s.ok() && !s.IsAlreadyPresent()) { + KLOG_EVERY_N_SECS(WARNING, 5) << "Master " << leader_addr.ToString() + << " provided a cert that could not be trusted: " + << s.ToString(); + continue; + } + } + } + vector<StatusCallback> cbs; { std::lock_guard<simple_spinlock> l(leader_master_lock_); cbs.swap(leader_master_callbacks_); leader_master_rpc_.reset(); + if (connect_response.has_authn_token()) { + authn_token_ = connect_response.authn_token(); + } if (status.ok()) { leader_master_hostport_ = HostPort(leader_addr); http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/client/client-internal.h ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-internal.h b/src/kudu/client/client-internal.h index 6594945..146a886 100644 --- a/src/kudu/client/client-internal.h +++ b/src/kudu/client/client-internal.h @@ -25,8 +25,10 @@ #include <vector> #include <boost/function.hpp> +#include <boost/optional.hpp> #include "kudu/client/client.h" +#include "kudu/security/token.pb.h" #include "kudu/util/atomic.h" #include "kudu/util/locks.h" #include "kudu/util/monotime.h" @@ -231,6 +233,10 @@ class KuduClient::Data { scoped_refptr<internal::ConnectToClusterRpc> leader_master_rpc_; std::vector<StatusCallback> leader_master_callbacks_; + // The latest authentication token that this client should use to talk + // to the cluster. + boost::optional<security::SignedTokenPB> authn_token_; + // Protects 'leader_master_rpc_', 'leader_master_hostport_', // and master_proxy_ // http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/client/client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index ed91910..b7bffc7 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -54,6 +54,7 @@ #include "kudu/master/mini_master.h" #include "kudu/master/ts_descriptor.h" #include "kudu/rpc/messenger.h" +#include "kudu/security/tls_context.h" #include "kudu/server/hybrid_clock.h" #include "kudu/tablet/tablet_peer.h" #include "kudu/tablet/transactions/write_transaction.h" @@ -4650,5 +4651,13 @@ TEST_F(ClientTest, TestConnectToClusterCompatibility) { ASSERT_EQ(initial_val + 1, final_val); } +// Test that, when the client connects to a cluster, that it gets the relevant +// certificate authority and authentication token from the master. +TEST_F(ClientTest, TestGetSecurityInfoFromMaster) { + // Client is already connected when the test starts. + ASSERT_TRUE(client_->data_->authn_token_ != boost::none); + ASSERT_EQ(1, client_->data_->messenger_->tls_context().trusted_cert_count_for_tests()); +} + } // namespace client } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/client/client.h ---------------------------------------------------------------------- diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h index 16cea6a..2f51d3d 100644 --- a/src/kudu/client/client.h +++ b/src/kudu/client/client.h @@ -473,6 +473,7 @@ class KUDU_EXPORT KuduClient : public sp::enable_shared_from_this<KuduClient> { friend class KuduTableCreator; FRIEND_TEST(kudu::ClientStressTest, TestUniqueClientIds); + FRIEND_TEST(ClientTest, TestGetSecurityInfoFromMaster); FRIEND_TEST(ClientTest, TestGetTabletServerBlacklist); FRIEND_TEST(ClientTest, TestMasterDown); FRIEND_TEST(ClientTest, TestMasterLookupPermits); http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/master/master.proto ---------------------------------------------------------------------- diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index e5f5d7e..14b7771 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -574,6 +574,10 @@ message ConnectToMasterResponsePB { // Any CA certs used by the cluster. Currently the master only uses // one cert, but we may support rolling this cert in the future, so // clients 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 = 3; // If the client requested an authentication token, and security is http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/rpc/messenger.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.h b/src/kudu/rpc/messenger.h index 1d2623f..0bf4d07 100644 --- a/src/kudu/rpc/messenger.h +++ b/src/kudu/rpc/messenger.h @@ -186,6 +186,7 @@ class Messenger { MonoDelta when); const security::TlsContext& tls_context() const { return *tls_context_; } + security::TlsContext* mutable_tls_context() { return tls_context_.get(); } ThreadPool* negotiation_pool() const { return negotiation_pool_.get(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index b2c4edb..b0f6dec 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -48,7 +48,8 @@ template<> struct SslTypeTraits<X509_STORE_CTX> { TlsContext::TlsContext() - : has_cert_(false) { + : has_cert_(false), + trusted_cert_count_(0) { security::InitializeOpenSSL(); } @@ -138,8 +139,19 @@ Status TlsContext::AddTrustedCertificate(const Cert& cert) { ERR_clear_error(); auto* cert_store = SSL_CTX_get_cert_store(ctx_.get()); - OPENSSL_RET_NOT_OK(X509_STORE_add_cert(cert_store, cert.GetRawData()), - "failed to add trusted certificate"); + int rc = X509_STORE_add_cert(cert_store, cert.GetRawData()); + if (rc <= 0) { + // Translate the common case of re-adding a cert that is already in the + // trust store into an AlreadyPresent status. + auto err = ERR_peek_error(); + 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"); + } + OPENSSL_RET_NOT_OK(rc, "failed to add trusted certificate"); + } + trusted_cert_count_.Increment(); return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/c4c770a4/src/kudu/security/tls_context.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.h b/src/kudu/security/tls_context.h index 0dadfe8..dfc994f 100644 --- a/src/kudu/security/tls_context.h +++ b/src/kudu/security/tls_context.h @@ -59,6 +59,9 @@ class TlsContext { // 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. + // + // Returns AlreadyPresent if the cert is already marked as trusted. Other + // OpenSSL errors will be RuntimeError. Status AddTrustedCertificate(const Cert& cert); // Convenience functions for loading cert/CA/key from file paths. @@ -74,11 +77,19 @@ class TlsContext { // Initiates a new TlsHandshake instance. Status InitiateHandshake(TlsHandshakeType handshake_type, TlsHandshake* handshake) const; + // 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(); + } + private: Status VerifyCertChain(const Cert& cert); AtomicBool has_cert_; + AtomicInt<int32> trusted_cert_count_; + // Owned SSL context. c_unique_ptr<SSL_CTX> ctx_; };
