kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject [2/2] kudu git commit: security: simplify CertSigner interface
Date Fri, 03 Feb 2017 17:35:07 GMT
security: simplify CertSigner interface

This makes CertSigner a "one-shot" type of instance, which no longer
holds long-lived references to CA certs or keys. This also removes the
'InitFromFiles()' path in CertSigner, since that's now doable directly
using the wrapper objects, and the code ended up only used by tests.

With these changes, a bunch of other things got simplified:
- no need for shared_ptr<PrivateKey> in a few places where it had leaked
- no need for multi-threading tests in cert_management-test

Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
Reviewed-on: http://gerrit.cloudera.org:8080/5846
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <danburkert@apache.org>
Reviewed-by: Todd Lipcon <todd@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/84818065
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/84818065
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/84818065

Branch: refs/heads/master
Commit: 848180654d5954255b1ac4fac7a19b0a7ad669bd
Parents: 70bbcfc
Author: Todd Lipcon <todd@apache.org>
Authored: Tue Jan 31 16:55:35 2017 -0800
Committer: Todd Lipcon <todd@apache.org>
Committed: Fri Feb 3 17:32:33 2017 +0000

----------------------------------------------------------------------
 src/kudu/master/master_cert_authority.cc     |  23 +-
 src/kudu/master/master_cert_authority.h      |   5 +-
 src/kudu/security/ca/cert_management-test.cc | 252 ++++++----------------
 src/kudu/security/ca/cert_management.cc      |  96 ++-------
 src/kudu/security/ca/cert_management.h       |  54 ++---
 src/kudu/security/crypto.cc                  |   2 +-
 src/kudu/security/crypto.h                   |   2 +-
 src/kudu/security/security-test-util.cc      |  14 +-
 src/kudu/security/security-test-util.h       |   4 +-
 9 files changed, 123 insertions(+), 329 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/master/master_cert_authority.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/master_cert_authority.cc b/src/kudu/master/master_cert_authority.cc
index 45190a1..fd24f28 100644
--- a/src/kudu/master/master_cert_authority.cc
+++ b/src/kudu/master/master_cert_authority.cc
@@ -27,9 +27,8 @@
 #include "kudu/security/openssl_util.h"
 #include "kudu/util/flag_tags.h"
 
-using std::make_shared;
-using std::shared_ptr;
 using std::string;
+using std::unique_ptr;
 
 using kudu::security::Cert;
 using kudu::security::CertSignRequest;
@@ -76,25 +75,19 @@ Status MasterCertAuthority::Init() {
   CHECK(!ca_private_key_);
 
   // Create a key and cert for the self-signed CA.
-  auto key = make_shared<PrivateKey>();
-  auto ca_cert = make_shared<Cert>();
-  RETURN_NOT_OK(GeneratePrivateKey(FLAGS_master_ca_rsa_key_length_bits,
-                                   key.get()));
-
-  RETURN_NOT_OK(CertSigner::SelfSignCA(key, PrepareCaConfig(server_uuid_), ca_cert.get()));
+  unique_ptr<PrivateKey> key(new PrivateKey());
+  unique_ptr<Cert> ca_cert(new Cert());
+  RETURN_NOT_OK(GeneratePrivateKey(FLAGS_master_ca_rsa_key_length_bits, key.get()));
+  RETURN_NOT_OK(CertSigner::SelfSignCA(*key, PrepareCaConfig(server_uuid_), ca_cert.get()));
 
   // Initialize our signer with the new CA.
-  auto signer = make_shared<CertSigner>();
-  RETURN_NOT_OK(signer->Init(ca_cert, key));
-
-  cert_signer_ = std::move(signer);
   ca_cert_ = std::move(ca_cert);
   ca_private_key_ = std::move(key);
   return Status::OK();
 }
 
 Status MasterCertAuthority::SignServerCSR(const string& csr_der, string* cert_der) {
-  CHECK(cert_signer_) << "not initialized";
+  CHECK(ca_cert_ && ca_private_key_) << "not initialized";
 
   // TODO(PKI): before signing, should we somehow verify the CSR's
   // hostname/server_uuid matches what we think is the hostname? can the signer
@@ -104,8 +97,10 @@ Status MasterCertAuthority::SignServerCSR(const string& csr_der, string*
cert_de
   CertSignRequest csr;
   RETURN_NOT_OK_PREPEND(csr.FromString(csr_der, security::DataFormat::DER),
                         "could not parse CSR");
+  // TODO(PKI): need to set expiration interval on the signed CA cert!
   Cert cert;
-  RETURN_NOT_OK_PREPEND(cert_signer_->Sign(csr, &cert),
+  RETURN_NOT_OK_PREPEND(CertSigner(ca_cert_.get(), ca_private_key_.get())
+                        .Sign(csr, &cert),
                         "failed to sign cert");
 
   RETURN_NOT_OK_PREPEND(cert.ToString(cert_der, security::DataFormat::DER),

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/master/master_cert_authority.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/master_cert_authority.h b/src/kudu/master/master_cert_authority.h
index 9b8e499..3ffed73 100644
--- a/src/kudu/master/master_cert_authority.h
+++ b/src/kudu/master/master_cert_authority.h
@@ -66,9 +66,8 @@ class MasterCertAuthority {
   // The UUID of the master. This is used as a field in the certificate.
   const std::string server_uuid_;
 
-  std::shared_ptr<security::ca::CertSigner> cert_signer_;
-  std::shared_ptr<security::PrivateKey> ca_private_key_;
-  std::shared_ptr<security::Cert> ca_cert_;
+  std::unique_ptr<security::PrivateKey> ca_private_key_;
+  std::unique_ptr<security::Cert> ca_cert_;
 
   DISALLOW_COPY_AND_ASSIGN(MasterCertAuthority);
 };

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/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 0a9b3a8..d296633 100644
--- a/src/kudu/security/ca/cert_management-test.cc
+++ b/src/kudu/security/ca/cert_management-test.cc
@@ -18,7 +18,6 @@
 #include "kudu/security/ca/cert_management.h"
 
 #include <memory>
-#include <thread>
 #include <utility>
 #include <vector>
 
@@ -29,16 +28,12 @@
 #include "kudu/security/openssl_util.h"
 #include "kudu/security/security-test-util.h"
 #include "kudu/security/test/test_certs.h"
-#include "kudu/util/env.h"
-#include "kudu/util/path_util.h"
 #include "kudu/util/status.h"
 #include "kudu/util/test_macros.h"
 #include "kudu/util/test_util.h"
 
-using std::shared_ptr;
 using std::string;
 using std::vector;
-using std::thread;
 using strings::Substitute;
 
 namespace kudu {
@@ -47,38 +42,15 @@ namespace ca {
 
 class CertManagementTest : public KuduTest {
  public:
-  CertManagementTest() :
-      pem_dir_(GetTestPath("pem")),
-      ca_cert_file_(JoinPathSegments(pem_dir_, "ca.cert.pem")),
-      ca_private_key_file_(JoinPathSegments(pem_dir_, "ca.pkey.pem")),
-      ca_public_key_file_(JoinPathSegments(pem_dir_, "ca.pubkey.pem")),
-      ca_exp_cert_file_(JoinPathSegments(pem_dir_, "ca.exp.cert.pem")),
-      ca_exp_private_key_file_(JoinPathSegments(pem_dir_, "ca.exp.pkey.pem")) {
-  }
-
   void SetUp() override {
-    ASSERT_OK(env_->CreateDir(pem_dir_));
-    ASSERT_OK(WriteStringToFile(env_, kCaCert, ca_cert_file_));
-    ASSERT_OK(WriteStringToFile(env_, kCaPrivateKey, ca_private_key_file_));
-    ASSERT_OK(WriteStringToFile(env_, kCaPublicKey, ca_public_key_file_));
-    ASSERT_OK(WriteStringToFile(env_, kCaExpiredCert, ca_exp_cert_file_));
-    ASSERT_OK(WriteStringToFile(env_, kCaExpiredPrivateKey,
-        ca_exp_private_key_file_));
+    ASSERT_OK(ca_cert_.FromString(kCaCert, DataFormat::PEM));
+    ASSERT_OK(ca_private_key_.FromString(kCaPrivateKey, DataFormat::PEM));
+    ASSERT_OK(ca_public_key_.FromString(kCaPublicKey, DataFormat::PEM));
+    ASSERT_OK(ca_exp_cert_.FromString(kCaExpiredCert, DataFormat::PEM));
+    ASSERT_OK(ca_exp_private_key_.FromString(kCaExpiredPrivateKey, DataFormat::PEM));
   }
 
  protected:
-  // Different sharing scenarios for request generator and signer.
-  enum SharingType {
-    DEDICATED,
-    SHARED
-  };
-
-  // Different init patterns for request generator and signer.
-  enum InitType {
-    SINGLE_INIT,
-    MULTIPLE_INIT
-  };
-
   CertRequestGenerator::Config PrepareConfig(
       const string& uuid,
       const vector<string>& hostnames = {},
@@ -100,76 +72,30 @@ class CertManagementTest : public KuduTest {
     };
   }
 
-  // Run multiple threads which do certificate signing request generation
-  // and signing those in parallel.  The 'is_shared' and 'multi_init' parameters
-  // are to specify whether the threads use shared
-  // CertRequestGenerator/CertSigner instances and whether every thread
-  // initializes the shared instance it's using.
-  void SignMultiThread(size_t num_threads, size_t iter_num,
-                       SharingType sharing_type, InitType init_type) {
-    const CertRequestGenerator::Config gen_config(
-        PrepareConfig("757F3158-DCB5-4D6C-8054-5348BB4AEA07",
-                      {"localhost"}, {"127.0.0.1"}));
-
-    CertRequestGenerator gen_shared(gen_config);
-    if (SINGLE_INIT == init_type) {
-      ASSERT_OK(gen_shared.Init());
-    }
-    CertSigner signer_shared;
-    if (SINGLE_INIT == init_type) {
-      ASSERT_OK(signer_shared.InitFromFiles(ca_cert_file_, ca_private_key_file_));
-    }
-
-    vector<thread> threads;
-    threads.reserve(num_threads);
-    for (size_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-      // 'thread_idx' is captured by value to avoid data races
-      threads.emplace_back([&, thread_idx]() {
-        for (size_t i = 0; i < iter_num; ++i) {
-          CertRequestGenerator gen_local(gen_config);
-          CertSigner signer_local;
-
-          CertRequestGenerator& gen = (SHARED == sharing_type) ? gen_shared
-                                                               : gen_local;
-          CertSigner& signer = (SHARED == sharing_type) ? signer_shared
-                                                        : signer_local;
-
-          if (DEDICATED == sharing_type) {
-            CHECK_OK(gen.Init());
-          }
-          const size_t sel = i % 4;
-          const size_t key_bits = (sel + 1) * 512;
-          PrivateKey key;
-          CHECK_OK(GeneratePrivateKey(key_bits, &key));
-          CertSignRequest req;
-          CHECK_OK(gen.GenerateRequest(key, &req));
-          if (DEDICATED == sharing_type) {
-            CHECK_OK(signer.InitFromFiles(ca_cert_file_, ca_private_key_file_));
-          }
-          Cert cert;
-          CHECK_OK(signer.Sign(req, &cert));
-        }
-      });
-    }
-    for (auto& e : threads) {
-      e.join();
-    }
+  // Create a new private key in 'key' and return a CSR associated with that
+  // key.
+  template<class CSRGen = CertRequestGenerator>
+  CertSignRequest PrepareTestCSR(CertRequestGenerator::Config config,
+                                 PrivateKey* key) {
+    CHECK_OK(GeneratePrivateKey(512, key));
+    CSRGen gen(std::move(config));
+    CHECK_OK(gen.Init());
+    CertSignRequest req;
+    CHECK_OK(gen.GenerateRequest(*key, &req));
+    return req;
   }
 
-  const string pem_dir_;
-
-  const string ca_cert_file_;
-  const string ca_private_key_file_;
-  const string ca_public_key_file_;
+  Cert ca_cert_;
+  PrivateKey ca_private_key_;
+  PublicKey ca_public_key_;
 
-  const string ca_exp_cert_file_;
-  const string ca_exp_private_key_file_;
+  Cert ca_exp_cert_;
+  PrivateKey ca_exp_private_key_;
 };
 
 // Check input/output of RSA private keys in PEM format.
 TEST_F(CertManagementTest, RsaPrivateKeyInputOutputPEM) {
-  PrivateKey key;
-  ASSERT_OK(key.FromFile(ca_private_key_file_, DataFormat::PEM));
+  const auto& key = ca_private_key_;
   string key_str;
   key.ToString(&key_str, DataFormat::PEM);
   RemoveExtraWhitespace(&key_str);
@@ -181,8 +107,7 @@ TEST_F(CertManagementTest, RsaPrivateKeyInputOutputPEM) {
 
 // Check input/output of RSA public keys in PEM format.
 TEST_F(CertManagementTest, RsaPublicKeyInputOutputPEM) {
-  PublicKey key;
-  ASSERT_OK(key.FromFile(ca_public_key_file_, DataFormat::PEM));
+  const auto& key = ca_public_key_;
   string str_key;
   key.ToString(&str_key, DataFormat::PEM);
   RemoveExtraWhitespace(&str_key);
@@ -195,8 +120,7 @@ TEST_F(CertManagementTest, RsaPublicKeyInputOutputPEM) {
 // Check extraction of the public part out from RSA private keys par.
 TEST_F(CertManagementTest, RsaExtractPublicPartFromPrivateKey) {
   // Load the reference RSA private key.
-  PrivateKey private_key;
-  ASSERT_OK(private_key.FromString(kCaPrivateKey, DataFormat::PEM));
+  const PrivateKey& private_key = ca_private_key_;
 
   PublicKey public_key;
   ASSERT_OK(private_key.GetPublicKey(&public_key));
@@ -211,8 +135,7 @@ TEST_F(CertManagementTest, RsaExtractPublicPartFromPrivateKey) {
 
 // Check input/output of the X509 certificates in PEM format.
 TEST_F(CertManagementTest, CertInputOutputPEM) {
-  Cert cert;
-  ASSERT_OK(cert.FromFile(ca_cert_file_, DataFormat::PEM));
+  const Cert& cert = ca_cert_;
   string cert_str;
   cert.ToString(&cert_str, DataFormat::PEM);
   RemoveExtraWhitespace(&cert_str);
@@ -222,6 +145,15 @@ TEST_F(CertManagementTest, CertInputOutputPEM) {
   EXPECT_EQ(ca_input_cert, cert_str);
 }
 
+// Check that Cert behaves in a predictable way if given invalid PEM data.
+TEST_F(CertManagementTest, CertInvalidInput) {
+  // Providing files which guaranteed to exists, but do not contain valid data.
+  // This is to make sure the init handles that situation correctly and
+  // does not choke on the wrong input data.
+  Cert c;
+  ASSERT_FALSE(c.FromFile("/bin/sh", DataFormat::PEM).ok());
+}
+
 // Check for basic SAN-related constraints while initializing
 // CertRequestGenerator objects.
 TEST_F(CertManagementTest, RequestGeneratorSanConstraints) {
@@ -280,7 +212,6 @@ TEST_F(CertManagementTest, RequestGeneratorBasics) {
 
   PrivateKey key;
   ASSERT_OK(GeneratePrivateKey(1024, &key));
-  ASSERT_OK(GeneratePrivateKey(2048, &key));
   CertRequestGenerator gen(gen_config);
   ASSERT_OK(gen.Init());
   string key_str;
@@ -290,29 +221,24 @@ TEST_F(CertManagementTest, RequestGeneratorBasics) {
   ASSERT_TRUE(s.IsRuntimeError());
 }
 
-// Check that CertSigner behaves in a predictable way if given non-expected
-// content for the CA private key/certificate.
-TEST_F(CertManagementTest, SignerInitWithWrongFiles) {
-  // Providing files which guaranteed to exists, but do not contain valid data.
-  // This is to make sure the init handles that situation correctly and
-  // does not choke on the wrong input data.
-  CertSigner signer;
-  ASSERT_FALSE(signer.InitFromFiles("/bin/sh", "/bin/cat").ok());
-}
-
 // Check that CertSigner behaves in a predictable way if given non-matching
 // CA private key and certificate.
 TEST_F(CertManagementTest, SignerInitWithMismatchedCertAndKey) {
+  PrivateKey key;
+  const auto& csr = PrepareTestCSR(PrepareConfig("test-uuid", {"localhost"}), &key);
   {
-    CertSigner signer;
-    Status s = signer.InitFromFiles(ca_cert_file_, ca_exp_private_key_file_);
+    Cert cert;
+    Status s = CertSigner(&ca_cert_, &ca_exp_private_key_)
+        .Sign(csr, &cert);
+
     const string err_msg = s.ToString();
     ASSERT_TRUE(s.IsRuntimeError()) << err_msg;
     ASSERT_STR_CONTAINS(err_msg, "CA certificate and private key do not match");
   }
   {
-    CertSigner signer;
-    Status s = signer.InitFromFiles(ca_exp_cert_file_, ca_private_key_file_);
+    Cert cert;
+    Status s = CertSigner(&ca_exp_cert_, &ca_private_key_)
+        .Sign(csr, &cert);
     const string err_msg = s.ToString();
     ASSERT_TRUE(s.IsRuntimeError()) << err_msg;
     ASSERT_STR_CONTAINS(err_msg, "CA certificate and private key do not match");
@@ -325,34 +251,21 @@ TEST_F(CertManagementTest, SignerInitWithExpiredCert) {
   const CertRequestGenerator::Config gen_config(
       PrepareConfig("F4466090-BBF8-4042-B72F-BB257500C45A", {"localhost"}));
   PrivateKey key;
-  ASSERT_OK(GeneratePrivateKey(2048, &key));
-  CertRequestGenerator gen(gen_config);
-  ASSERT_OK(gen.Init());
-  CertSignRequest req;
-  ASSERT_OK(gen.GenerateRequest(key, &req));
-  CertSigner signer;
-  // Even if the certificate is expired, the signer should initialize OK.
-  ASSERT_OK(signer.InitFromFiles(ca_exp_cert_file_, ca_exp_private_key_file_));
-  Cert cert;
+  CertSignRequest req = PrepareTestCSR(gen_config, &key);
+
   // Signer works fine even with expired CA certificate.
-  ASSERT_OK(signer.Sign(req, &cert));
+  Cert cert;
+  ASSERT_OK(CertSigner(&ca_exp_cert_, &ca_exp_private_key_).Sign(req, &cert));
 }
 
-// Generate X509 CSR and issues corresponding certificate: everything is done
-// in a single-threaded fashion.
+// Generate X509 CSR and issues corresponding certificate.
 TEST_F(CertManagementTest, SignCert) {
   const CertRequestGenerator::Config gen_config(
       PrepareConfig("test-uuid", {"localhost"}, {"127.0.0.1", "127.0.10.20"}));
   PrivateKey key;
-  ASSERT_OK(GeneratePrivateKey(2048, &key));
-  CertRequestGenerator gen(gen_config);
-  ASSERT_OK(gen.Init());
-  CertSignRequest req;
-  ASSERT_OK(gen.GenerateRequest(key, &req));
-  CertSigner signer;
-  ASSERT_OK(signer.InitFromFiles(ca_cert_file_, ca_private_key_file_));
+  const auto& csr = PrepareTestCSR(gen_config, &key);
   Cert cert;
-  ASSERT_OK(signer.Sign(req, &cert));
+  ASSERT_OK(CertSigner(&ca_cert_, &ca_private_key_).Sign(csr, &cert));
 
   EXPECT_EQ("C = US, ST = CA, O = MyCompany, CN = MyName, emailAddress = my@email.com",
             cert.IssuerName());
@@ -365,45 +278,28 @@ TEST_F(CertManagementTest, SignCaCert) {
   const CertRequestGenerator::Config gen_config(
       PrepareConfig("8C084CF6-A30B-4F5B-9673-A73E62E29A9D"));
   PrivateKey key;
-  ASSERT_OK(GeneratePrivateKey(2048, &key));
-  CaCertRequestGenerator gen(gen_config);
-  ASSERT_OK(gen.Init());
-  CertSignRequest req;
-  ASSERT_OK(gen.GenerateRequest(key, &req));
-  CertSigner signer;
-  ASSERT_OK(signer.InitFromFiles(ca_cert_file_, ca_private_key_file_));
+  const auto& csr = PrepareTestCSR<CaCertRequestGenerator>(gen_config, &key);
   Cert cert;
-  ASSERT_OK(signer.Sign(req, &cert));
+  ASSERT_OK(CertSigner(&ca_cert_, &ca_private_key_).Sign(csr, &cert));
 }
 
 // Test the creation and use of a CA which uses a self-signed CA cert
 // generated on the fly.
 TEST_F(CertManagementTest, TestSelfSignedCA) {
-  shared_ptr<PrivateKey> ca_key;
-  shared_ptr<Cert> ca_cert;
+  PrivateKey ca_key;
+  Cert ca_cert;
   ASSERT_OK(GenerateSelfSignedCAForTests(&ca_key, &ca_cert));
 
-  // Create a key for the tablet server.
-  auto ts_key = std::make_shared<PrivateKey>();
-  ASSERT_OK(GeneratePrivateKey(2048, ts_key.get()));
-
-  // Prepare a CSR for a tablet server that wants signing.
-  CertSignRequest ts_csr;
-  {
-    CertRequestGenerator gen(PrepareConfig(
-        "some-tablet-server",
-        {"localhost"}, {"127.0.0.1", "127.0.10.20"}));
-    ASSERT_OK(gen.Init());
-    ASSERT_OK(gen.GenerateRequest(*ts_key, &ts_csr));
-  }
+  // Create a key and CSR for the tablet server.
+  const auto& config = PrepareConfig(
+      "some-tablet-server",
+      {"localhost"}, {"127.0.0.1", "127.0.10.20"});
+  PrivateKey ts_key;
+  CertSignRequest ts_csr = PrepareTestCSR(config, &ts_key);
 
   // Sign it using the self-signed CA.
   Cert ts_cert;
-  {
-    CertSigner signer;
-    ASSERT_OK(signer.Init(ca_cert, ca_key));
-    ASSERT_OK(signer.Sign(ts_csr, &ts_cert));
-  }
+  ASSERT_OK(CertSigner(&ca_cert, &ca_key).Sign(ts_csr, &ts_cert));
 }
 
 // Check the transformation chains for X509 CSRs:
@@ -446,10 +342,9 @@ TEST_F(CertManagementTest, X509FromAndToString) {
   CertSignRequest req;
   ASSERT_OK(gen.GenerateRequest(key, &req));
 
-  CertSigner signer;
-  ASSERT_OK(signer.InitFromFiles(ca_cert_file_, ca_private_key_file_));
   Cert cert_ref;
-  ASSERT_OK(signer.Sign(req, &cert_ref));
+  ASSERT_OK(CertSigner(&ca_cert_, &ca_private_key_)
+            .Sign(req, &cert_ref));
 
   for (auto format : kFormats) {
     SCOPED_TRACE(Substitute("X509 format: $0", DataFormatToString(format)));
@@ -463,27 +358,6 @@ TEST_F(CertManagementTest, X509FromAndToString) {
   }
 }
 
-// Generate CSR and issue corresponding certificate in a multi-threaded fashion:
-// every thread uses its own instances of CertRequestGenerator and CertSigner,
-// which were initialized earlier (i.e. those threads do not call Init()).
-TEST_F(CertManagementTest, SignMultiThreadExclusiveSingleInit) {
-  ASSERT_NO_FATAL_FAILURE(SignMultiThread(32, 16, DEDICATED, SINGLE_INIT));
-}
-
-// Generate CSR and issue corresponding certificate in a multi-threaded fashion:
-// every thread uses its own instances of CertRequestGenerator and CertSigner,
-// and every thread initializes those shared objects by itself.
-TEST_F(CertManagementTest, SignMultiThreadExclusiveMultiInit) {
-  ASSERT_NO_FATAL_FAILURE(SignMultiThread(16, 32, DEDICATED, MULTIPLE_INIT));
-}
-
-// Generate CSR and issue corresponding certificate in a multi-thread fashion:
-// all threads use shared instances of CertRequestGenerator and CertSigner,
-// which were initialized earlier (i.e. those threads do not call Init()).
-TEST_F(CertManagementTest, SignMultiThreadSharedSingleInit) {
-  ASSERT_NO_FATAL_FAILURE(SignMultiThread(32, 16, SHARED, SINGLE_INIT));
-}
-
 } // namespace ca
 } // namespace security
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/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 710b23e..a871b75 100644
--- a/src/kudu/security/ca/cert_management.cc
+++ b/src/kudu/security/ca/cert_management.cc
@@ -44,7 +44,6 @@
 using std::lock_guard;
 using std::move;
 using std::ostringstream;
-using std::shared_ptr;
 using std::string;
 using strings::Substitute;
 
@@ -281,7 +280,7 @@ Status CaCertRequestGenerator::SetExtensions(X509_REQ* req) const {
   return Status::OK();
 }
 
-Status CertSigner::SelfSignCA(const shared_ptr<PrivateKey>& key,
+Status CertSigner::SelfSignCA(const PrivateKey& key,
                               CaCertRequestGenerator::Config config,
                               Cert* cert) {
   // Generate a CSR for the CA.
@@ -289,90 +288,37 @@ Status CertSigner::SelfSignCA(const shared_ptr<PrivateKey>&
key,
   {
     CaCertRequestGenerator gen(std::move(config));
     RETURN_NOT_OK(gen.Init());
-    RETURN_NOT_OK(gen.GenerateRequest(*key, &ca_csr));
+    RETURN_NOT_OK(gen.GenerateRequest(key, &ca_csr));
   }
 
   // Self-sign the CA's CSR.
-  {
-    CertSigner ca_signer;
-    RETURN_NOT_OK(ca_signer.InitForSelfSigning(key));
-    RETURN_NOT_OK(ca_signer.Sign(ca_csr, cert));
-  }
+  RETURN_NOT_OK(CertSigner(nullptr, &key).Sign(ca_csr, cert));
   return Status::OK();
 }
 
-CertSigner::CertSigner()
-    : is_initialized_(false) {
-}
-
-Status CertSigner::Init(shared_ptr<Cert> ca_cert,
-                        shared_ptr<PrivateKey> ca_private_key) {
-  CHECK(ca_cert && ca_cert->GetRawData());
+CertSigner::CertSigner(const Cert* ca_cert,
+                       const PrivateKey* ca_private_key)
+    : ca_cert_(ca_cert),
+      ca_private_key_(ca_private_key) {
+  // Private key is required.
   CHECK(ca_private_key && ca_private_key->GetRawData());
-  InitializeOpenSSL();
-
-  std::lock_guard<simple_spinlock> guard(lock_);
-  CHECK(!is_initialized_);
-
-  ca_cert_ = std::move(ca_cert);
-  ca_private_key_ = std::move(ca_private_key);
-  is_initialized_ = true;
-  return Status::OK();
-}
-
-Status CertSigner::InitForSelfSigning(shared_ptr<PrivateKey> private_key) {
-  CHECK(private_key);
-  InitializeOpenSSL();
-
-  std::lock_guard<simple_spinlock> guard(lock_);
-  CHECK(!is_initialized_);
-
-  ca_private_key_ = std::move(private_key);
-  is_initialized_ = true;
-  return Status::OK();
-}
-
-Status CertSigner::InitFromFiles(const string& ca_cert_path,
-                                 const string& ca_private_key_path) {
-  InitializeOpenSSL();
-
-  std::lock_guard<simple_spinlock> guard(lock_);
-  CHECK(!is_initialized_);
-  auto cert = std::make_shared<Cert>();
-  std::shared_ptr<PrivateKey> key = std::make_shared<PrivateKey>();
-  RETURN_NOT_OK(cert->FromFile(ca_cert_path, DataFormat::PEM));
-  RETURN_NOT_OK(key->FromFile(ca_private_key_path,
-                              DataFormat::PEM));
-  OPENSSL_RET_NOT_OK(
-      X509_check_private_key(cert->GetRawData(), key->GetRawData()),
-      Substitute("$0, $1: CA certificate and private key do not match",
-          ca_cert_path, ca_private_key_path));
-  ca_cert_ = std::move(cert);
-  ca_private_key_ = std::move(key);
-  is_initialized_ = true;
-  return Status::OK();
-}
-
-bool CertSigner::Initialized() const {
-  lock_guard<simple_spinlock> guard(lock_);
-  return is_initialized_;
-}
-
-const Cert& CertSigner::ca_cert() const {
-  lock_guard<simple_spinlock> guard(lock_);
-  DCHECK(is_initialized_);
-  return *CHECK_NOTNULL(ca_cert_.get());
-}
-
-const PrivateKey& CertSigner::ca_private_key() const {
-  lock_guard<simple_spinlock> guard(lock_);
-  DCHECK(is_initialized_);
-  return *ca_private_key_;
+  // The cert is optional, but if we have it, it should be initialized.
+  CHECK(!ca_cert || ca_cert->GetRawData());
 }
 
 Status CertSigner::Sign(const CertSignRequest& req, Cert* ret) const {
+  InitializeOpenSSL();
   CHECK(ret);
-  CHECK(Initialized());
+
+  // If we are not self-signing, then make sure that the provided CA
+  // cert and key match each other. Technically this would be programmer
+  // error since we're always using internally-generated CA certs, but
+  // this isn't a hot path so we'll keep the extra safety.
+  if (ca_cert_) {
+    OPENSSL_RET_NOT_OK(
+        X509_check_private_key(ca_cert_->GetRawData(), ca_private_key_->GetRawData()),
+        "CA certificate and private key do not match");
+  }
   auto x509 = ssl_make_unique(X509_new());
   RETURN_NOT_OK(FillCertTemplateFromRequest(req.GetRawData(), x509.get()));
   RETURN_NOT_OK(DoSign(EVP_sha256(), exp_interval_sec_, x509.get()));

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/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 7a2887a..203a810 100644
--- a/src/kudu/security/ca/cert_management.h
+++ b/src/kudu/security/ca/cert_management.h
@@ -132,69 +132,59 @@ class CaCertRequestGenerator : public CertRequestGeneratorBase {
 };
 
 // An utility class for issuing and signing certificates.
+//
+// This is used in "fluent" style. For example:
+//
+//    CHECK_OK(CertSigner(&my_ca_cert, &my_ca_key)
+//      .set_expiration_interval(MonoDelta::FromSeconds(3600))
+//      .Sign(csr, &cert));
+//
+// As such, this class is not guaranteed thread-safe.
 class CertSigner {
  public:
   // Generate a self-signed certificate authority using the given key
   // and CSR configuration.
-  static Status SelfSignCA(const std::shared_ptr<PrivateKey>& key,
+  static Status SelfSignCA(const PrivateKey& key,
                            CaCertRequestGenerator::Config config,
                            Cert* cert);
+
   // Create a CertSigner.
-  // Exactly one of the following Init*() methods must be called
-  // exactly once before the instance may be used.
-  CertSigner();
+  //
+  // The given cert and key must stay valid for the lifetime of the
+  // cert signer. See class documentation above for recommended usage.
+  //
+  // 'ca_cert' may be nullptr in order to perform self-signing (though
+  // the SelfSignCA() static method above is recommended).
+  CertSigner(const Cert* ca_cert, const PrivateKey* ca_private_key);
   ~CertSigner() = default;
 
-  // Initialize the signer from existing Cert/Key objects.
-  // The passed objects must be initialized.
-  Status Init(std::shared_ptr<Cert> ca_cert,
-              std::shared_ptr<PrivateKey> ca_private_key);
-
-  // Initialize the signer from a CA cert and private key stored
-  // on disk.
-  Status InitFromFiles(const std::string& ca_cert_path,
-                       const std::string& ca_private_key_path);
-
   // Set the expiration interval for certs signed by this signer.
   // This may be changed at any point.
-  void set_expiration_interval(MonoDelta expiration) {
-    std::lock_guard<simple_spinlock> l(lock_);
+  CertSigner& set_expiration_interval(MonoDelta expiration) {
     exp_interval_sec_ = expiration.ToSeconds();
+    return *this;
   }
 
-  bool Initialized() const;
-
-  const Cert& ca_cert() const;
-  const PrivateKey& ca_private_key() const;
-
   Status Sign(const CertSignRequest& req, Cert* ret) const;
 
  private:
+
   static Status CopyExtensions(X509_REQ* req, X509* x);
   static Status FillCertTemplateFromRequest(X509_REQ* req, X509* tmpl);
   static Status DigestSign(const EVP_MD* md, EVP_PKEY* pkey, X509* x);
   static Status GenerateSerial(c_unique_ptr<ASN1_INTEGER>* ret);
 
-  // Initialize the signer for self-signing using the given private key.
-  //
-  // Any certificates signed by this CertSigner will have the 'issuer' equal
-  // to the signed cert's subject.
-  Status InitForSelfSigning(std::shared_ptr<PrivateKey> private_key);
-
   Status DoSign(const EVP_MD* digest, int32_t exp_seconds, X509 *ret) const;
 
-  mutable simple_spinlock lock_;
-  bool is_initialized_; // protected by lock_
-
   // The expiration interval of certs signed by this signer.
   int32_t exp_interval_sec_ = 24 * 60 * 60;
 
   // The CA cert. null if this CertSigner is configured for self-signing.
-  std::shared_ptr<Cert> ca_cert_;
+  const Cert* const ca_cert_;
 
   // The CA private key. If configured for self-signing, this is the
   // private key associated with the target cert.
-  std::shared_ptr<PrivateKey> ca_private_key_;
+  const PrivateKey* const ca_private_key_;
 
   DISALLOW_COPY_AND_ASSIGN(CertSigner);
 };

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/security/crypto.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/crypto.cc b/src/kudu/security/crypto.cc
index c5d5c8c..dda9da3 100644
--- a/src/kudu/security/crypto.cc
+++ b/src/kudu/security/crypto.cc
@@ -169,7 +169,7 @@ Status PrivateKey::FromFile(const std::string& fpath, DataFormat format)
{
 // The code is modeled after $OPENSSL_ROOT/apps/rsa.c code: there is
 // corresponding functionality to read public part from RSA private/public
 // keypair.
-Status PrivateKey::GetPublicKey(PublicKey* public_key) {
+Status PrivateKey::GetPublicKey(PublicKey* public_key) const {
   CHECK(public_key);
   auto rsa = ssl_make_unique(EVP_PKEY_get1_RSA(CHECK_NOTNULL(data_.get())));
   if (PREDICT_FALSE(!rsa)) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/security/crypto.h
----------------------------------------------------------------------
diff --git a/src/kudu/security/crypto.h b/src/kudu/security/crypto.h
index 6063f37..ce951b9 100644
--- a/src/kudu/security/crypto.h
+++ b/src/kudu/security/crypto.h
@@ -69,7 +69,7 @@ class PrivateKey : public RawDataWrapper<EVP_PKEY> {
   Status FromFile(const std::string& fpath, DataFormat format);
 
   // Output the public part of the keypair into the specified placeholder.
-  Status GetPublicKey(PublicKey* public_key);
+  Status GetPublicKey(PublicKey* public_key) const;
 
   // Using the key, generate data signature using the specified
   // message digest algorithm. The result signature is in raw format

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/security/security-test-util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/security-test-util.cc b/src/kudu/security/security-test-util.cc
index 2fa8b1f..c01f562 100644
--- a/src/kudu/security/security-test-util.cc
+++ b/src/kudu/security/security-test-util.cc
@@ -17,8 +17,6 @@
 
 #include "kudu/security/security-test-util.h"
 
-#include <memory>
-
 #include <glog/logging.h>
 
 #include "kudu/security/ca/cert_management.h"
@@ -31,19 +29,13 @@ namespace security {
 using ca::CaCertRequestGenerator;
 using ca::CertSigner;
 
-Status GenerateSelfSignedCAForTests(std::shared_ptr<PrivateKey>* ca_key,
-                                    std::shared_ptr<Cert>* ca_cert) {
+Status GenerateSelfSignedCAForTests(PrivateKey* ca_key, Cert* ca_cert) {
   // Create a key for the self-signed CA.
-  auto ret_ca_key = std::make_shared<PrivateKey>();
-  auto ret_ca_cert = std::make_shared<Cert>();
-  RETURN_NOT_OK(GeneratePrivateKey(512, ret_ca_key.get()));
+  RETURN_NOT_OK(GeneratePrivateKey(512, ca_key));
 
   CaCertRequestGenerator::Config config;
   config.uuid = "test-ca-uuid";
-  RETURN_NOT_OK(CertSigner::SelfSignCA(ret_ca_key, config, ret_ca_cert.get()));
-
-  *ca_key = std::move(ret_ca_key);
-  *ca_cert = std::move(ret_ca_cert);
+  RETURN_NOT_OK(CertSigner::SelfSignCA(*ca_key, config, ca_cert));
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/84818065/src/kudu/security/security-test-util.h
----------------------------------------------------------------------
diff --git a/src/kudu/security/security-test-util.h b/src/kudu/security/security-test-util.h
index 17c5375..df4730f 100644
--- a/src/kudu/security/security-test-util.h
+++ b/src/kudu/security/security-test-util.h
@@ -99,9 +99,7 @@ dc+JVPKL8Fe4a8fmsI6ndcZQ9qpOdZM5WOD0ldKRc+SsrYKkTmOOJQ==
   return Status::OK();
 }
 
-// TODO(todd): change these from shared_ptrs to unique_ptrs
-Status GenerateSelfSignedCAForTests(std::shared_ptr<PrivateKey>* ca_key,
-                                    std::shared_ptr<Cert>* ca_cert);
+Status GenerateSelfSignedCAForTests(PrivateKey* ca_key, Cert* ca_cert);
 
 } // namespace security
 } // namespace kudu


Mime
View raw message