[security] use Cert::CheckKeyMatch() for sanity checks Use the Cert::CheckKeyMatch() method in appropriate places.
Change-Id: I47d9134377ad51af153accb00ce3ac4cf864cda1 Reviewed-on: http://gerrit.cloudera.org:8080/5938 Reviewed-by: Dan Burkert <[email protected]> Tested-by: Kudu Jenkins Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/dffbf6f3 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/dffbf6f3 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/dffbf6f3 Branch: refs/heads/master Commit: dffbf6f395498cd37d044c4ca859f8571d5bd951 Parents: f369fcf Author: Alexey Serbin <[email protected]> Authored: Tue Feb 7 23:57:45 2017 -0800 Committer: Alexey Serbin <[email protected]> Committed: Wed Feb 8 20:10:20 2017 +0000 ---------------------------------------------------------------------- .../integration-tests/master_cert_authority-itest.cc | 2 +- src/kudu/master/catalog_manager.cc | 2 ++ src/kudu/security/ca/cert_management-test.cc | 11 +++++++++-- src/kudu/security/ca/cert_management.cc | 8 +++----- src/kudu/security/cert-test.cc | 3 +++ src/kudu/security/server_cert_manager.cc | 4 +++- src/kudu/security/tls_context.cc | 3 +-- 7 files changed, 22 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/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 9e4dbfe..8a17373 100644 --- a/src/kudu/integration-tests/master_cert_authority-itest.cc +++ b/src/kudu/integration-tests/master_cert_authority-itest.cc @@ -216,7 +216,7 @@ TEST_F(MasterCertAuthorityTest, CertAuthorityOnLeaderRoleSwitch) { } // Test that every master accepts heartbeats, but only the leader master -// responds with with signed certificate if a heartbeat contains the CSR field. +// responds with signed certificate if a heartbeat contains the CSR field. TEST_F(MasterCertAuthorityTest, MasterLeaderSignsCSR) { shared_ptr<rpc::Messenger> messenger; rpc::MessengerBuilder bld("Client"); http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 02acb38..a2a77e2 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -700,6 +700,8 @@ Status CatalogManager::CheckInitCertAuthority() { info.private_key(), DataFormat::DER)); RETURN_NOT_OK(ca_cert->FromString( info.certificate(), DataFormat::DER)); + // Extra sanity check. + RETURN_NOT_OK(ca_cert->CheckKeyMatch(*ca_private_key)); } else { // Generate new private key and corresponding CA certificate. RETURN_NOT_OK(ca->Generate(ca_private_key.get(), ca_cert.get())); http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/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 d296633..6a9c7a9 100644 --- a/src/kudu/security/ca/cert_management-test.cc +++ b/src/kudu/security/ca/cert_management-test.cc @@ -48,6 +48,9 @@ class CertManagementTest : public KuduTest { 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)); + // Sanity checks. + ASSERT_OK(ca_cert_.CheckKeyMatch(ca_private_key_)); + ASSERT_OK(ca_exp_cert_.CheckKeyMatch(ca_exp_private_key_)); } protected: @@ -233,7 +236,7 @@ TEST_F(CertManagementTest, SignerInitWithMismatchedCertAndKey) { 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"); + ASSERT_STR_CONTAINS(err_msg, "certificate does not match private key"); } { Cert cert; @@ -241,7 +244,7 @@ TEST_F(CertManagementTest, SignerInitWithMismatchedCertAndKey) { .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"); + ASSERT_STR_CONTAINS(err_msg, "certificate does not match private key"); } } @@ -256,6 +259,7 @@ TEST_F(CertManagementTest, SignerInitWithExpiredCert) { // Signer works fine even with expired CA certificate. Cert cert; ASSERT_OK(CertSigner(&ca_exp_cert_, &ca_exp_private_key_).Sign(req, &cert)); + ASSERT_OK(cert.CheckKeyMatch(key)); } // Generate X509 CSR and issues corresponding certificate. @@ -266,6 +270,7 @@ TEST_F(CertManagementTest, SignCert) { const auto& csr = PrepareTestCSR(gen_config, &key); Cert cert; ASSERT_OK(CertSigner(&ca_cert_, &ca_private_key_).Sign(csr, &cert)); + ASSERT_OK(cert.CheckKeyMatch(key)); EXPECT_EQ("C = US, ST = CA, O = MyCompany, CN = MyName, emailAddress = [email protected]", cert.IssuerName()); @@ -281,6 +286,7 @@ TEST_F(CertManagementTest, SignCaCert) { const auto& csr = PrepareTestCSR<CaCertRequestGenerator>(gen_config, &key); Cert cert; ASSERT_OK(CertSigner(&ca_cert_, &ca_private_key_).Sign(csr, &cert)); + ASSERT_OK(cert.CheckKeyMatch(key)); } // Test the creation and use of a CA which uses a self-signed CA cert @@ -300,6 +306,7 @@ TEST_F(CertManagementTest, TestSelfSignedCA) { // Sign it using the self-signed CA. Cert ts_cert; ASSERT_OK(CertSigner(&ca_cert, &ca_key).Sign(ts_csr, &ts_cert)); + ASSERT_OK(ts_cert.CheckKeyMatch(ts_key)); } // Check the transformation chains for X509 CSRs: http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/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 9f0dde2..9a4d9e0 100644 --- a/src/kudu/security/ca/cert_management.cc +++ b/src/kudu/security/ca/cert_management.cc @@ -292,9 +292,9 @@ CertSigner::CertSigner(const Cert* ca_cert, : ca_cert_(ca_cert), ca_private_key_(ca_private_key) { // Private key is required. - CHECK(ca_private_key && ca_private_key->GetRawData()); + CHECK(ca_private_key_ && ca_private_key_->GetRawData()); // The cert is optional, but if we have it, it should be initialized. - CHECK(!ca_cert || ca_cert->GetRawData()); + CHECK(!ca_cert_ || ca_cert_->GetRawData()); } Status CertSigner::Sign(const CertSignRequest& req, Cert* ret) const { @@ -306,9 +306,7 @@ Status CertSigner::Sign(const CertSignRequest& req, Cert* ret) const { // 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"); + RETURN_NOT_OK(ca_cert_->CheckKeyMatch(*ca_private_key_)); } auto x509 = ssl_make_unique(X509_new()); RETURN_NOT_OK(FillCertTemplateFromRequest(req.GetRawData(), x509.get())); http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/src/kudu/security/cert-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/cert-test.cc b/src/kudu/security/cert-test.cc index ca36095..f0d88bb 100644 --- a/src/kudu/security/cert-test.cc +++ b/src/kudu/security/cert-test.cc @@ -42,6 +42,9 @@ class CertTest : public KuduTest { ASSERT_OK(ca_exp_cert_.FromString(kCaExpiredCert, DataFormat::PEM)); ASSERT_OK(ca_exp_private_key_.FromString(kCaExpiredPrivateKey, DataFormat::PEM)); + // Sanity checks. + ASSERT_OK(ca_cert_.CheckKeyMatch(ca_private_key_)); + ASSERT_OK(ca_exp_cert_.CheckKeyMatch(ca_exp_private_key_)); } protected: http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/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 index e682aa9..61da08d 100644 --- a/src/kudu/security/server_cert_manager.cc +++ b/src/kudu/security/server_cert_manager.cc @@ -92,7 +92,9 @@ Status ServerCertManager::AdoptSignedCert(const string& cert_der) { 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. http://git-wip-us.apache.org/repos/asf/kudu/blob/dffbf6f3/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index b0f6dec..8e46deb 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -118,8 +118,7 @@ Status TlsContext::UseCertificateAndKey(const Cert& cert, const PrivateKey& key) ERR_clear_error(); // 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"); + 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
