Repository: kudu Updated Branches: refs/heads/master 3cd434e51 -> 914ed3b98
IPKI: remove unused fields from cert code This removes all of the X509 attributes that we weren't currently setting. It's better to not have them than have invalid values. Change-Id: Ie3e6ff00fa33fe0156a04ead6f72db3432775cdb Reviewed-on: http://gerrit.cloudera.org:8080/6115 Reviewed-by: Dan Burkert <[email protected]> Reviewed-by: Alexey Serbin <[email protected]> Tested-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/914ed3b9 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/914ed3b9 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/914ed3b9 Branch: refs/heads/master Commit: 914ed3b98533f18513a0ea9a6c47162c8b464100 Parents: 3cd434e Author: Todd Lipcon <[email protected]> Authored: Wed Feb 22 10:48:35 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Thu Feb 23 01:01:24 2017 +0000 ---------------------------------------------------------------------- .../master_cert_authority-itest.cc | 3 +- src/kudu/master/master_cert_authority.cc | 23 +---- src/kudu/security/ca/cert_management-test.cc | 89 ++++---------------- src/kudu/security/ca/cert_management.cc | 61 ++------------ src/kudu/security/ca/cert_management.h | 17 +--- src/kudu/security/security-test-util.cc | 3 +- src/kudu/security/tls_context.cc | 13 +-- 7 files changed, 28 insertions(+), 181 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/src/kudu/integration-tests/master_cert_authority-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/master_cert_authority-itest.cc b/src/kudu/integration-tests/master_cert_authority-itest.cc index 8a17373..227c4cd 100644 --- a/src/kudu/integration-tests/master_cert_authority-itest.cc +++ b/src/kudu/integration-tests/master_cert_authority-itest.cc @@ -226,8 +226,7 @@ TEST_F(MasterCertAuthorityTest, MasterLeaderSignsCSR) { string csr_str; { CertRequestGenerator::Config gen_config; - gen_config.uuid = kFakeTsUUID; - gen_config.hostnames = {"localhost"}; + gen_config.cn = "ts.foo.com"; PrivateKey key; ASSERT_OK(security::GeneratePrivateKey(512, &key)); CertRequestGenerator gen(gen_config); http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/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 75d8b73..f5490f9 100644 --- a/src/kudu/master/master_cert_authority.cc +++ b/src/kudu/master/master_cert_authority.cc @@ -58,26 +58,6 @@ TAG_FLAG(ipki_server_cert_expiration_seconds, experimental); namespace kudu { namespace master { -namespace { - -CaCertRequestGenerator::Config PrepareCaConfig(const string& server_uuid) { - // TODO(aserbin): do we actually have to set all these fields given we - // aren't using a web browser to connect? - return { - "US", // country - "CA", // state - "San Francisco", // locality - "ASF", // org - "The Kudu Project", // unit - server_uuid, // uuid - "Kudu IPKI self-signed root CA certificate",// comment - {}, // hostnames - {}, // ips - }; -} - -} // anonymous namespace - MasterCertAuthority::MasterCertAuthority(string server_uuid) : server_uuid_(std::move(server_uuid)) { } @@ -91,9 +71,10 @@ Status MasterCertAuthority::Generate(security::PrivateKey* key, CHECK(key); CHECK(cert); // Create a key and cert for the self-signed CA. + CaCertRequestGenerator::Config config = { "kudu-ipki-ca" }; RETURN_NOT_OK(GeneratePrivateKey(FLAGS_ipki_ca_key_size, key)); return CertSigner::SelfSignCA(*key, - PrepareCaConfig(server_uuid_), + config, FLAGS_ipki_ca_cert_expiration_seconds, cert); } http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/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 e2b2552..b3ec45b 100644 --- a/src/kudu/security/ca/cert_management-test.cc +++ b/src/kudu/security/ca/cert_management-test.cc @@ -54,24 +54,8 @@ class CertManagementTest : public KuduTest { protected: CertRequestGenerator::Config PrepareConfig( - const string& uuid, - const vector<string>& hostnames = {}, - const vector<string>& ips = {}) const { - const ::testing::TestInfo* const test_info = - ::testing::UnitTest::GetInstance()->current_test_info(); - const string comment = string(test_info->test_case_name()) + "." + - test_info->name(); - return { - "US", // country - "CA", // state - "San Francisco", // locality - "ASF", // org - "The Kudu Project", // unit - uuid, // uuid - comment, // comment - hostnames, // hostnames - ips, // ips - }; + const string& common_name) { + return { common_name }; } // Create a new private key in 'key' and return a CSR associated with that @@ -95,52 +79,17 @@ class CertManagementTest : public KuduTest { PrivateKey ca_exp_private_key_; }; -// Check for basic SAN-related constraints while initializing +// Check for basic constraints while initializing // CertRequestGenerator objects. -TEST_F(CertManagementTest, RequestGeneratorSanConstraints) { - const string kEntityUUID = "D94FBF10-6F40-4F9F-BC82-F96A1C4F2CFB"; - - // No hostnames, nor IP addresses are given to populate X509v3 SAN extension. - { - const CertRequestGenerator::Config gen_config = PrepareConfig(kEntityUUID); - CertRequestGenerator gen(gen_config); - const Status s = gen.Init(); - const string err_msg = s.ToString(); - ASSERT_TRUE(s.IsInvalidArgument()) << err_msg; - ASSERT_STR_CONTAINS(err_msg, "SAN: missing DNS names and IP addresses"); - } - - // An empty hostname - { - const CertRequestGenerator::Config gen_config = - PrepareConfig(kEntityUUID, {"localhost", ""}); - CertRequestGenerator gen(gen_config); - const Status s = gen.Init(); - const string err_msg = s.ToString(); - ASSERT_TRUE(s.IsInvalidArgument()) << err_msg; - ASSERT_STR_CONTAINS(err_msg, "SAN: an empty hostname"); - } - - // An empty IP address - { - const CertRequestGenerator::Config gen_config = - PrepareConfig(kEntityUUID, {}, {"127.0.0.1", ""}); - CertRequestGenerator gen(gen_config); - const Status s = gen.Init(); - const string err_msg = s.ToString(); - ASSERT_TRUE(s.IsInvalidArgument()) << err_msg; - ASSERT_STR_CONTAINS(err_msg, "SAN: an empty IP address"); - } - - // Missing UUID +TEST_F(CertManagementTest, RequestGeneratorConstraints) { + // Missing CN { - const CertRequestGenerator::Config gen_config = - PrepareConfig("", {"localhost"}); + const CertRequestGenerator::Config gen_config = PrepareConfig(""); CertRequestGenerator gen(gen_config); const Status s = gen.Init(); const string err_msg = s.ToString(); ASSERT_TRUE(s.IsInvalidArgument()) << err_msg; - ASSERT_STR_CONTAINS(err_msg, "missing end-entity UUID/name"); + ASSERT_STR_CONTAINS(err_msg, "missing end-entity CN"); } } @@ -148,8 +97,7 @@ TEST_F(CertManagementTest, RequestGeneratorSanConstraints) { // check it's able to generate keys of expected number of bits and that it // reports an error if trying to generate a key of unsupported number of bits. TEST_F(CertManagementTest, RequestGeneratorBasics) { - const CertRequestGenerator::Config gen_config = - PrepareConfig("702C1C5E-CF02-4EDC-8883-07ECDEC8CE97", {"localhost"}); + const CertRequestGenerator::Config gen_config = PrepareConfig("my-cn"); PrivateKey key; ASSERT_OK(GeneratePrivateKey(1024, &key)); @@ -166,7 +114,7 @@ TEST_F(CertManagementTest, RequestGeneratorBasics) { // CA private key and certificate. TEST_F(CertManagementTest, SignerInitWithMismatchedCertAndKey) { PrivateKey key; - const auto& csr = PrepareTestCSR(PrepareConfig("test-uuid", {"localhost"}), &key); + const auto& csr = PrepareTestCSR(PrepareConfig("test-cn"), &key); { Cert cert; Status s = CertSigner(&ca_cert_, &ca_exp_private_key_) @@ -189,8 +137,7 @@ TEST_F(CertManagementTest, SignerInitWithMismatchedCertAndKey) { // Check how CertSigner behaves if given expired CA certificate // and corresponding private key. TEST_F(CertManagementTest, SignerInitWithExpiredCert) { - const CertRequestGenerator::Config gen_config( - PrepareConfig("F4466090-BBF8-4042-B72F-BB257500C45A", {"localhost"})); + const CertRequestGenerator::Config gen_config = PrepareConfig("test-cn"); PrivateKey key; CertSignRequest req = PrepareTestCSR(gen_config, &key); @@ -202,8 +149,7 @@ TEST_F(CertManagementTest, SignerInitWithExpiredCert) { // 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"})); + const CertRequestGenerator::Config gen_config = PrepareConfig("test-cn"); PrivateKey key; const auto& csr = PrepareTestCSR(gen_config, &key); Cert cert; @@ -212,8 +158,7 @@ TEST_F(CertManagementTest, SignCert) { 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()); + EXPECT_EQ("CN = test-cn", cert.SubjectName()); } // Generate X509 CA CSR and sign the result certificate. @@ -235,9 +180,7 @@ TEST_F(CertManagementTest, TestSelfSignedCA) { ASSERT_OK(GenerateSelfSignedCAForTests(&ca_key, &ca_cert)); // 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"}); + const auto& config = PrepareConfig("some-tablet-server"); PrivateKey ts_key; CertSignRequest ts_csr = PrepareTestCSR(config, &ts_key); @@ -255,8 +198,7 @@ TEST_F(CertManagementTest, X509CsrFromAndToString) { PrivateKey key; ASSERT_OK(GeneratePrivateKey(1024, &key)); - CertRequestGenerator gen(PrepareConfig( - "4C931ADC-3945-4E05-8DB2-447327BF8F62", {"localhost"})); + CertRequestGenerator gen(PrepareConfig("test-cn")); ASSERT_OK(gen.Init()); CertSignRequest req_ref; ASSERT_OK(gen.GenerateRequest(key, &req_ref)); @@ -281,8 +223,7 @@ TEST_F(CertManagementTest, X509FromAndToString) { PrivateKey key; ASSERT_OK(GeneratePrivateKey(1024, &key)); - CertRequestGenerator gen(PrepareConfig( - "86F676E9-4E77-4DDC-B15C-596E74B03D90", {"localhost"})); + CertRequestGenerator gen(PrepareConfig("test-cn")); ASSERT_OK(gen.Init()); CertSignRequest req; ASSERT_OK(gen.GenerateRequest(key, &req)); http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/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 5585556..88bb3fb 100644 --- a/src/kudu/security/ca/cert_management.cc +++ b/src/kudu/security/ca/cert_management.cc @@ -88,12 +88,7 @@ Status CertRequestGeneratorBase::GenerateRequest(const PrivateKey& key, } \ } while (false) - CERT_SET_SUBJ_FIELD(config_.country, "C", "country"); - CERT_SET_SUBJ_FIELD(config_.state, "ST", "state"); - CERT_SET_SUBJ_FIELD(config_.locality, "L", "locality/city"); - CERT_SET_SUBJ_FIELD(config_.org, "O", "organization"); - CERT_SET_SUBJ_FIELD(config_.unit, "OU", "organizational unit"); - CERT_SET_SUBJ_FIELD(config_.uuid, "CN", "common name"); + CERT_SET_SUBJ_FIELD(config_.cn, "CN", "common name"); #undef CERT_SET_SUBJ_FIELD // Set necessary extensions into the request. @@ -127,13 +122,8 @@ Status CertRequestGenerator::Init() { InitializeOpenSSL(); CHECK(!is_initialized_); - if (config_.uuid.empty()) { - return Status::InvalidArgument("missing end-entity UUID/name"); - } - // Check that the config contain at least one entity (DNS name/IP address) - // to bind the generated certificate. - if (config_.hostnames.empty() && config_.ips.empty()) { - return Status::InvalidArgument("SAN: missing DNS names and IP addresses"); + if (config_.cn.empty()) { + return Status::InvalidArgument("missing end-entity CN"); } extensions_ = sk_X509_EXTENSION_new_null(); @@ -164,43 +154,7 @@ Status CertRequestGenerator::Init() { // (i.e. they cannot be used to sign/issue certificates). RETURN_NOT_OK(PushExtension(extensions_, NID_basic_constraints, "critical,CA:FALSE")); - ostringstream san_hosts; - for (size_t i = 0; i < config_.hostnames.size(); ++i) { - const string& hostname = config_.hostnames[i]; - if (hostname.empty()) { - // Basic validation: check for emptyness. Probably, more advanced - // validation is needed here. - return Status::InvalidArgument("SAN: an empty hostname"); - } - if (i != 0) { - san_hosts << ","; - } - san_hosts << "DNS." << i << ":" << hostname; - } - ostringstream san_ips; - for (size_t i = 0; i < config_.ips.size(); ++i) { - const string& ip = config_.ips[i]; - if (ip.empty()) { - // Basic validation: check for emptyness. Probably, more advanced - // validation is needed here. - return Status::InvalidArgument("SAN: an empty IP address"); - } - if (i != 0) { - san_ips << ","; - } - san_ips << "IP." << i << ":" << ip; - } - // Encode hostname and IP address into the subjectAlternativeName attribute. - const string alt_name = san_hosts.str() + - ((!san_hosts.str().empty() && !san_ips.str().empty()) ? "," : "") + - san_ips.str(); - RETURN_NOT_OK(PushExtension(extensions_, NID_subject_alt_name, - alt_name.c_str())); - if (!config_.comment.empty()) { - // Add the comment if it's not empty. - RETURN_NOT_OK(PushExtension(extensions_, NID_netscape_comment, - config_.comment.c_str())); - } + is_initialized_ = true; return Status::OK(); @@ -233,7 +187,7 @@ Status CaCertRequestGenerator::Init() { if (is_initialized_) { return Status::OK(); } - if (config_.uuid.empty()) { + if (config_.cn.empty()) { return Status::InvalidArgument("missing CA service UUID/name"); } @@ -250,11 +204,6 @@ Status CaCertRequestGenerator::Init() { // The generated certificates are for the private CA service. RETURN_NOT_OK(PushExtension(extensions_, NID_basic_constraints, "critical,CA:TRUE")); - if (!config_.comment.empty()) { - // Add the comment if it's not empty. - RETURN_NOT_OK(PushExtension(extensions_, NID_netscape_comment, - config_.comment.c_str())); - } is_initialized_ = true; return Status::OK(); http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/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 682989c..f0fd342 100644 --- a/src/kudu/security/ca/cert_management.h +++ b/src/kudu/security/ca/cert_management.h @@ -55,15 +55,7 @@ class CertRequestGeneratorBase { // Properties for the generated X509 CSR. Using server UUID for the common // name field. struct Config { - std::string country; // subject field: C - std::string state; // subject field: ST - std::string locality; // subject field: L - std::string org; // subject field: O - std::string unit; // subject field: OU - std::string uuid; // subject field: CN - std::string comment; // custom extension: Netscape Comment - std::vector<std::string> hostnames; // subjectAltName extension (DNS:) - std::vector<std::string> ips; // subjectAltName extension (IP:) + std::string cn; // subject field: CN }; explicit CertRequestGeneratorBase(Config config); @@ -93,11 +85,8 @@ class CertRequestGeneratorBase { // (a.k.a. X509 CSRs). class CertRequestGenerator : public CertRequestGeneratorBase { public: - // The CertRequestGenerator object is bound to the server UUID, hostnames - // and IP addresses specified by the 'config' parameter. The hostnames and - // IP addresses are put into the X509v3 SAN extension (subject alternative - // name, a.k.a. subjectAltName). The SAN can be used while verifying the - // generated certificates during TLS handshake. + // 'config' contains the properties to fill into the X509 attributes of the + // CSR. explicit CertRequestGenerator(Config config); ~CertRequestGenerator(); http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/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 bfafb39..dfa9c4c 100644 --- a/src/kudu/security/security-test-util.cc +++ b/src/kudu/security/security-test-util.cc @@ -35,8 +35,7 @@ Status GenerateSelfSignedCAForTests(PrivateKey* ca_key, Cert* ca_cert) { // Create a key for the self-signed CA. RETURN_NOT_OK(GeneratePrivateKey(512, ca_key)); - CaCertRequestGenerator::Config config; - config.uuid = "test-ca-uuid"; + CaCertRequestGenerator::Config config = { "test-ca-cn" }; RETURN_NOT_OK(CertSigner::SelfSignCA(*ca_key, config, kRootCaCertExpirationSeconds, http://git-wip-us.apache.org/repos/asf/kudu/blob/914ed3b9/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index 2c248ca..0844a70 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -242,18 +242,7 @@ Status TlsContext::GenerateSelfSignedCertAndKey(const std::string& server_uuid) // 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 - }; + const CertRequestGenerator::Config config = { server_uuid }; CertRequestGenerator gen(config); CertSignRequest csr;
