Repository: kudu Updated Branches: refs/heads/master d25430e28 -> 848180654
Allow configuring TlsContext with key wrappers Change-Id: Idb82427aea5f1bd29bad7529f2d54638b90811e2 Reviewed-on: http://gerrit.cloudera.org:8080/5845 Tested-by: Todd Lipcon <[email protected]> Reviewed-by: Dan Burkert <[email protected]> Reviewed-by: Alexey Serbin <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/70bbcfc6 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/70bbcfc6 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/70bbcfc6 Branch: refs/heads/master Commit: 70bbcfc6ad7f117c77f8c24ac0bf544ba1f25dc6 Parents: d25430e Author: Todd Lipcon <[email protected]> Authored: Tue Jan 31 14:08:56 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Fri Feb 3 17:32:07 2017 +0000 ---------------------------------------------------------------------- src/kudu/rpc/messenger.cc | 4 +- src/kudu/rpc/negotiation-test.cc | 3 +- src/kudu/security/ca/cert_management-test.cc | 8 +- src/kudu/security/cert.cc | 19 +++++ src/kudu/security/cert.h | 7 ++ src/kudu/security/tls_context.cc | 92 ++++++++++++++++++----- src/kudu/security/tls_context.h | 37 +++++++-- src/kudu/security/tls_handshake-test.cc | 15 ++-- 8 files changed, 143 insertions(+), 42 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc index 38c5eaa..da2f364 100644 --- a/src/kudu/rpc/messenger.cc +++ b/src/kudu/rpc/messenger.cc @@ -306,8 +306,8 @@ Status Messenger::Init() { tls_context_.reset(new security::TlsContext()); RETURN_NOT_OK(tls_context_->Init()); if (server_tls_enabled_) { - RETURN_NOT_OK(tls_context_->LoadCertificate(FLAGS_rpc_ssl_server_certificate)); - RETURN_NOT_OK(tls_context_->LoadPrivateKey(FLAGS_rpc_ssl_private_key)); + RETURN_NOT_OK(tls_context_->LoadCertificateAndKey(FLAGS_rpc_ssl_server_certificate, + FLAGS_rpc_ssl_private_key)); RETURN_NOT_OK(tls_context_->LoadCertificateAuthority(FLAGS_rpc_ssl_certificate_authority)); } for (Reactor* r : reactors_) { http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/rpc/negotiation-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/negotiation-test.cc b/src/kudu/rpc/negotiation-test.cc index c05386f..7efbe31 100644 --- a/src/kudu/rpc/negotiation-test.cc +++ b/src/kudu/rpc/negotiation-test.cc @@ -552,8 +552,7 @@ static void RunTlsGssapiNegotiationServer(unique_ptr<Socket> socket) { security::TlsContext tls_context; ASSERT_OK(tls_context.Init()); - ASSERT_OK(tls_context.LoadCertificate(server_cert_path)); - ASSERT_OK(tls_context.LoadPrivateKey(private_key_path)); + ASSERT_OK(tls_context.LoadCertificateAndKey(server_cert_path, private_key_path)); server_negotiation.EnableTls(&tls_context); server_negotiation.set_server_fqdn("127.0.0.1"); http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/security/ca/cert_management-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/ca/cert_management-test.cc b/src/kudu/security/ca/cert_management-test.cc index 39dfec8..0a9b3a8 100644 --- a/src/kudu/security/ca/cert_management-test.cc +++ b/src/kudu/security/ca/cert_management-test.cc @@ -342,8 +342,7 @@ TEST_F(CertManagementTest, SignerInitWithExpiredCert) { // in a single-threaded fashion. TEST_F(CertManagementTest, SignCert) { const CertRequestGenerator::Config gen_config( - PrepareConfig("904A97F9-545A-4746-86D1-85D433FF3F9C", - {"localhost"}, {"127.0.0.1", "127.0.10.20"})); + PrepareConfig("test-uuid", {"localhost"}, {"127.0.0.1", "127.0.10.20"})); PrivateKey key; ASSERT_OK(GeneratePrivateKey(2048, &key)); CertRequestGenerator gen(gen_config); @@ -354,6 +353,11 @@ TEST_F(CertManagementTest, SignCert) { ASSERT_OK(signer.InitFromFiles(ca_cert_file_, ca_private_key_file_)); Cert cert; ASSERT_OK(signer.Sign(req, &cert)); + + EXPECT_EQ("C = US, ST = CA, O = MyCompany, CN = MyName, emailAddress = [email protected]", + cert.IssuerName()); + EXPECT_EQ("C = US, ST = CA, L = San Francisco, O = ASF, OU = The Kudu Project, " + "CN = test-uuid", cert.SubjectName()); } // Generate X509 CA CSR and sign the result certificate. http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/security/cert.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/cert.cc b/src/kudu/security/cert.cc index b141ad1..f04577f 100644 --- a/src/kudu/security/cert.cc +++ b/src/kudu/security/cert.cc @@ -46,6 +46,16 @@ template<> struct SslTypeTraits<X509_REQ> { static constexpr auto write_der = &i2d_X509_REQ_bio; }; +string X509NameToString(X509_NAME* name) { + CHECK(name); + auto bio = ssl_make_unique(BIO_new(BIO_s_mem())); + OPENSSL_CHECK_OK(X509_NAME_print_ex(bio.get(), name, 0, XN_FLAG_ONELINE)); + + BUF_MEM* membuf; + OPENSSL_CHECK_OK(BIO_get_mem_ptr(bio.get(), &membuf)); + return string(membuf->data, membuf->length); +} + Status Cert::FromString(const std::string& data, DataFormat format) { return ::kudu::security::FromString(data, format, &data_); } @@ -58,6 +68,15 @@ Status Cert::FromFile(const std::string& fpath, DataFormat format) { return ::kudu::security::FromFile(fpath, format, &data_); } +string Cert::SubjectName() const { + return X509NameToString(X509_get_subject_name(data_.get())); +} + +string Cert::IssuerName() const { + return X509NameToString(X509_get_issuer_name(data_.get())); +} + + Status CertSignRequest::FromString(const std::string& data, DataFormat format) { return ::kudu::security::FromString(data, format, &data_); } http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/security/cert.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/cert.h b/src/kudu/security/cert.h index 536d7e9..c21ae0b 100644 --- a/src/kudu/security/cert.h +++ b/src/kudu/security/cert.h @@ -24,6 +24,7 @@ // Forward declarations for the OpenSSL typedefs. typedef struct x509_st X509; typedef struct X509_req_st X509_REQ; +typedef struct X509_name_st X509_NAME; namespace kudu { @@ -31,11 +32,17 @@ class Status; namespace security { +// Convert an X509_NAME object to a human-readable string. +std::string X509NameToString(X509_NAME* name); + class Cert : public RawDataWrapper<X509> { public: Status FromString(const std::string& data, DataFormat format); Status ToString(std::string* data, DataFormat format) const; Status FromFile(const std::string& fpath, DataFormat format); + + std::string SubjectName() const; + std::string IssuerName() const; }; class CertSignRequest : public RawDataWrapper<X509_REQ> { http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index 2eec883..b2c4edb 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -21,8 +21,11 @@ #include <openssl/err.h> #include <openssl/ssl.h> +#include <openssl/x509.h> #include "kudu/gutil/strings/substitute.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/status.h" @@ -39,8 +42,13 @@ template<> struct SslTypeTraits<SSL> { template<> struct SslTypeTraits<SSL_CTX> { static constexpr auto free = &SSL_CTX_free; }; +template<> struct SslTypeTraits<X509_STORE_CTX> { + static constexpr auto free = &X509_STORE_CTX_free; +}; + -TlsContext::TlsContext() { +TlsContext::TlsContext() + : has_cert_(false) { security::InitializeOpenSSL(); } @@ -75,37 +83,81 @@ Status TlsContext::Init() { return Status::OK(); } -Status TlsContext::LoadCertificate(const string& certificate_path) { - ERR_clear_error(); - errno = 0; - if (SSL_CTX_use_certificate_file(ctx_.get(), certificate_path.c_str(), SSL_FILETYPE_PEM) != 1) { - return Status::NotFound(Substitute("failed to load certificate file: '$0'", certificate_path), - GetOpenSSLErrors()); +Status TlsContext::VerifyCertChain(const Cert& cert) { + X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get()); + auto store_ctx = ssl_make_unique<X509_STORE_CTX>(X509_STORE_CTX_new()); + + OPENSSL_RET_NOT_OK(X509_STORE_CTX_init(store_ctx.get(), store, cert.GetRawData(), nullptr), + "could not init X509_STORE_CTX"); + int rc = X509_verify_cert(store_ctx.get()); + if (rc != 1) { + int err = X509_STORE_CTX_get_error(store_ctx.get()); + if (err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) { + // It's OK to provide a self-signed cert. + return Status::OK(); + } + + // Get the cert that failed to verify. + X509* cur_cert = X509_STORE_CTX_get_current_cert(store_ctx.get()); + string cert_details; + if (cur_cert) { + cert_details = Substitute(" (error with cert: subject=$0, issuer=$1)", + X509NameToString(X509_get_subject_name(cur_cert)), + X509NameToString(X509_get_issuer_name(cur_cert))); + } + + return Status::RuntimeError( + Substitute("could not verify cert chain$0", cert_details), + X509_verify_cert_error_string(err)); } return Status::OK(); } -Status TlsContext::LoadPrivateKey(const string& key_path) { +Status TlsContext::UseCertificateAndKey(const Cert& cert, const PrivateKey& key) { ERR_clear_error(); - errno = 0; - if (SSL_CTX_use_PrivateKey_file(ctx_.get(), key_path.c_str(), SSL_FILETYPE_PEM) != 1) { - return Status::NotFound(Substitute("failed to load private key file: '$0'", key_path), - GetOpenSSLErrors()); - } + + // Verify that the cert and key match. + OPENSSL_RET_NOT_OK(X509_check_private_key(cert.GetRawData(), key.GetRawData()), + "cert and private key do not match"); + + // 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)); + + 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); return Status::OK(); } -Status TlsContext::LoadCertificateAuthority(const string& certificate_path) { +Status TlsContext::AddTrustedCertificate(const Cert& cert) { + VLOG(2) << "Trusting certificate " << cert.SubjectName(); + ERR_clear_error(); - errno = 0; - if (SSL_CTX_load_verify_locations(ctx_.get(), certificate_path.c_str(), nullptr) != 1) { - return Status::NotFound(Substitute("failed to load certificate authority file: '$0'", - certificate_path), - GetOpenSSLErrors()); - } + 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"); return Status::OK(); } +Status TlsContext::LoadCertificateAndKey(const string& certificate_path, + const string& key_path) { + Cert c; + RETURN_NOT_OK(c.FromFile(certificate_path, DataFormat::PEM)); + PrivateKey k; + RETURN_NOT_OK(k.FromFile(key_path, DataFormat::PEM)); + return UseCertificateAndKey(c, k); +} + +Status TlsContext::LoadCertificateAuthority(const string& certificate_path) { + Cert c; + RETURN_NOT_OK(c.FromFile(certificate_path, DataFormat::PEM)); + return AddTrustedCertificate(c); +} + Status TlsContext::InitiateHandshake(TlsHandshakeType handshake_type, TlsHandshake* handshake) const { CHECK(ctx_); http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/src/kudu/security/tls_context.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.h b/src/kudu/security/tls_context.h index 5ba3c0e..0dadfe8 100644 --- a/src/kudu/security/tls_context.h +++ b/src/kudu/security/tls_context.h @@ -20,13 +20,16 @@ #include <functional> #include <string> -#include "kudu/security/openssl_util.h" #include "kudu/security/tls_handshake.h" +#include "kudu/util/atomic.h" #include "kudu/util/status.h" namespace kudu { 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. @@ -40,19 +43,41 @@ class TlsContext { Status Init(); - // Load the server certificate. - Status LoadCertificate(const std::string& certificate_path); + // 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(); } + + // 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); + + // Add 'cert' as a trusted certificate for this server/client. + // + // 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. + Status AddTrustedCertificate(const Cert& cert); - // Load the private key for the server certificate. - Status LoadPrivateKey(const std::string& key_path); + // Convenience functions for loading cert/CA/key from file paths. + // ------------------------------------------------------------- - // Load the certificate authority. + // Load the server certificate and key (PEM encoded). + Status LoadCertificateAndKey(const std::string& certificate_path, + const std::string& key_path); + + // Load the certificate authority (PEM encoded). Status LoadCertificateAuthority(const std::string& certificate_path); // Initiates a new TlsHandshake instance. Status InitiateHandshake(TlsHandshakeType handshake_type, TlsHandshake* handshake) const; private: + Status VerifyCertChain(const Cert& cert); + + AtomicBool has_cert_; // Owned SSL context. c_unique_ptr<SSL_CTX> ctx_; http://git-wip-us.apache.org/repos/asf/kudu/blob/70bbcfc6/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 5ee550a..337a7af 100644 --- a/src/kudu/security/tls_handshake-test.cc +++ b/src/kudu/security/tls_handshake-test.cc @@ -95,12 +95,10 @@ class TestTlsHandshake : public KuduTest { TEST_F(TestTlsHandshake, TestSuccessfulHandshake) { // Both client and server have certs and CA. - ASSERT_OK(client_tls_.LoadCertificate(cert_path_)); - ASSERT_OK(client_tls_.LoadPrivateKey(key_path_)); ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); - ASSERT_OK(server_tls_.LoadCertificate(cert_path_)); - ASSERT_OK(server_tls_.LoadPrivateKey(key_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_)); TlsHandshake server; TlsHandshake client; @@ -134,8 +132,7 @@ TEST_F(TestTlsHandshake, TestSuccessfulHandshake) { // Client has no cert. // Server has self-signed cert. TEST_F(TestTlsHandshake, Test_ClientNone_ServerSelfSigned) { - ASSERT_OK(server_tls_.LoadCertificate(cert_path_)); - ASSERT_OK(server_tls_.LoadPrivateKey(key_path_)); + 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. @@ -165,11 +162,9 @@ TEST_F(TestTlsHandshake, Test_ClientNone_ServerSelfSigned) { // 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_.LoadCertificate(cert_path_)); - ASSERT_OK(client_tls_.LoadPrivateKey(key_path_)); + ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, key_path_)); ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_)); - ASSERT_OK(server_tls_.LoadCertificate(cert_path_)); - ASSERT_OK(server_tls_.LoadPrivateKey(key_path_)); + ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, key_path_)); ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_)); // This scenario should succeed in all cases.
