From 908e55b5c37b0edc5208739224366cf09557fbf4 Mon Sep 17 00:00:00 2001 From: pneubeck Date: Wed, 30 Sep 2015 09:53:49 -0700 Subject: Ignore a missing issuer certificate for certificate matching. BUG=506185 Review URL: https://codereview.chromium.org/1212433006 Cr-Commit-Position: refs/heads/master@{#351579} --- chromeos/network/client_cert_resolver.cc | 50 ++++++----- chromeos/network/client_cert_resolver_unittest.cc | 103 +++++++++++++++++----- 2 files changed, 110 insertions(+), 43 deletions(-) (limited to 'chromeos/network') diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc index e5e4dd9..a0545c9 100644 --- a/chromeos/network/client_cert_resolver.cc +++ b/chromeos/network/client_cert_resolver.cc @@ -71,6 +71,7 @@ bool HasPrivateKey(const net::X509Certificate& cert) { } // Describes a certificate which is issued by |issuer| (encoded as PEM). +// |issuer| can be empty if no issuer certificate is found in the database. struct CertAndIssuer { CertAndIssuer(const scoped_refptr& certificate, const std::string& issuer) @@ -127,6 +128,33 @@ struct MatchCertWithPattern { const CertificatePattern pattern; }; +// Lookup the issuer certificate of |cert|. If it is available, return the PEM +// encoding of that certificate. Otherwise return the empty string. +std::string GetPEMEncodedIssuer(const net::X509Certificate& cert) { + net::ScopedCERTCertificate issuer_handle( + CERT_FindCertIssuer(cert.os_cert_handle(), PR_Now(), certUsageAnyCA)); + if (!issuer_handle) { + VLOG(1) << "Couldn't find an issuer."; + return std::string(); + } + + scoped_refptr issuer = + net::X509Certificate::CreateFromHandle( + issuer_handle.get(), + net::X509Certificate::OSCertHandles() /* no intermediate certs */); + if (!issuer.get()) { + LOG(ERROR) << "Couldn't create issuer cert."; + return std::string(); + } + std::string pem_encoded_issuer; + if (!net::X509Certificate::GetPEMEncoded(issuer->os_cert_handle(), + &pem_encoded_issuer)) { + LOG(ERROR) << "Couldn't PEM-encode certificate."; + return std::string(); + } + return pem_encoded_issuer; +} + std::vector CreateSortedCertAndIssuerList( const net::CertificateList& certs) { // Filter all client certs and determines each certificate's issuer, which is @@ -140,27 +168,7 @@ std::vector CreateSortedCertAndIssuerList( !CertLoader::IsCertificateHardwareBacked(&cert)) { continue; } - net::ScopedCERTCertificate issuer_handle( - CERT_FindCertIssuer(cert.os_cert_handle(), PR_Now(), certUsageAnyCA)); - if (!issuer_handle) { - LOG(ERROR) << "Couldn't find an issuer."; - continue; - } - scoped_refptr issuer = - net::X509Certificate::CreateFromHandle( - issuer_handle.get(), - net::X509Certificate::OSCertHandles() /* no intermediate certs */); - if (!issuer.get()) { - LOG(ERROR) << "Couldn't create issuer cert."; - continue; - } - std::string pem_encoded_issuer; - if (!net::X509Certificate::GetPEMEncoded(issuer->os_cert_handle(), - &pem_encoded_issuer)) { - LOG(ERROR) << "Couldn't PEM-encode certificate."; - continue; - } - client_certs.push_back(CertAndIssuer(*it, pem_encoded_issuer)); + client_certs.push_back(CertAndIssuer(*it, GetPEMEncodedIssuer(cert))); } std::sort(client_certs.begin(), client_certs.end(), &CompareCertExpiration); diff --git a/chromeos/network/client_cert_resolver_unittest.cc b/chromeos/network/client_cert_resolver_unittest.cc index 5032b63..0109b8c 100644 --- a/chromeos/network/client_cert_resolver_unittest.cc +++ b/chromeos/network/client_cert_resolver_unittest.cc @@ -101,25 +101,27 @@ class ClientCertResolverTest : public testing::Test, } } - // Imports a CA cert (stored as PEM in test_ca_cert_pem_) and a client - // certificate signed by that CA. Its PKCS#11 ID is stored in - // |test_cert_id_|. - void SetupTestCerts() { - // Import a CA cert. - net::CertificateList ca_cert_list = - net::CreateCertificateListFromFile(net::GetTestCertsDirectory(), - "client_1_ca.pem", - net::X509Certificate::FORMAT_AUTO); + // Imports a client certificate. Its PKCS#11 ID is stored in |test_cert_id_|. + // If |import_issuer| is true, also imports the CA cert (stored as PEM in + // test_ca_cert_pem_) that issued the client certificate. + void SetupTestCerts(bool import_issuer) { + // Load a CA cert. + net::CertificateList ca_cert_list = net::CreateCertificateListFromFile( + net::GetTestCertsDirectory(), "client_1_ca.pem", + net::X509Certificate::FORMAT_AUTO); ASSERT_TRUE(!ca_cert_list.empty()); - net::NSSCertDatabase::ImportCertFailureList failures; - EXPECT_TRUE(test_nsscertdb_->ImportCACerts( - ca_cert_list, net::NSSCertDatabase::TRUST_DEFAULT, &failures)); - ASSERT_TRUE(failures.empty()) << net::ErrorToString(failures[0].net_error); - net::X509Certificate::GetPEMEncoded(ca_cert_list[0]->os_cert_handle(), &test_ca_cert_pem_); ASSERT_TRUE(!test_ca_cert_pem_.empty()); + if (import_issuer) { + net::NSSCertDatabase::ImportCertFailureList failures; + EXPECT_TRUE(test_nsscertdb_->ImportCACerts( + ca_cert_list, net::NSSCertDatabase::TRUST_DEFAULT, &failures)); + ASSERT_TRUE(failures.empty()) + << net::ErrorToString(failures[0].net_error); + } + // Import a client cert signed by that CA. test_client_cert_ = net::ImportClientCertAndKeyFromFile(net::GetTestCertsDirectory(), @@ -172,10 +174,45 @@ class ClientCertResolverTest : public testing::Test, ->AddManagerService(kWifiStub, true); } - // Setup a policy with a certificate pattern that matches any client cert that - // is signed by the test CA cert (stored in |test_ca_cert_pem_|). In + // Sets up a policy with a certificate pattern that matches any client cert + // with a certain Issuer CN. It will match the test client cert. + void SetupPolicyMatchingIssuerCN() { + const char* kTestPolicy = + "[ { \"GUID\": \"wifi_stub\"," + " \"Name\": \"wifi_stub\"," + " \"Type\": \"WiFi\"," + " \"WiFi\": {" + " \"Security\": \"WPA-EAP\"," + " \"SSID\": \"wifi_ssid\"," + " \"EAP\": {" + " \"Outer\": \"EAP-TLS\"," + " \"ClientCertType\": \"Pattern\"," + " \"ClientCertPattern\": {" + " \"Issuer\": {" + " \"CommonName\": \"B CA\"" + " }" + " }" + " }" + " }" + "} ]"; + + std::string error; + scoped_ptr policy_value = base::JSONReader::ReadAndReturnError( + kTestPolicy, base::JSON_ALLOW_TRAILING_COMMAS, NULL, &error); + ASSERT_TRUE(policy_value) << error; + + base::ListValue* policy = NULL; + ASSERT_TRUE(policy_value->GetAsList(&policy)); + + managed_config_handler_->SetPolicy( + onc::ONC_SOURCE_USER_POLICY, kUserHash, *policy, + base::DictionaryValue() /* no global network config */); + } + + // Sets up a policy with a certificate pattern that matches any client cert + // that is signed by the test CA cert (stored in |test_ca_cert_pem_|). In // particular it will match the test client cert. - void SetupPolicy() { + void SetupPolicyMatchingIssuerPEM() { const char* kTestPolicyTemplate = "[ { \"GUID\": \"wifi_stub\"," " \"Name\": \"wifi_stub\"," @@ -248,12 +285,13 @@ class ClientCertResolverTest : public testing::Test, }; TEST_F(ClientCertResolverTest, NoMatchingCertificates) { + SetupTestCerts(false /* do not import the issuer */); StartCertLoader(); SetupWifi(); base::RunLoop().RunUntilIdle(); network_properties_changed_count_ = 0; SetupNetworkHandlers(); - SetupPolicy(); + SetupPolicyMatchingIssuerPEM(); base::RunLoop().RunUntilIdle(); // Verify that no client certificate was configured. @@ -264,13 +302,34 @@ TEST_F(ClientCertResolverTest, NoMatchingCertificates) { EXPECT_FALSE(client_cert_resolver_->IsAnyResolveTaskRunning()); } +TEST_F(ClientCertResolverTest, MatchIssuerCNWithoutIssuerInstalled) { + SetupTestCerts(false /* do not import the issuer */); + SetupWifi(); + base::RunLoop().RunUntilIdle(); + + SetupNetworkHandlers(); + SetupPolicyMatchingIssuerCN(); + base::RunLoop().RunUntilIdle(); + + network_properties_changed_count_ = 0; + StartCertLoader(); + base::RunLoop().RunUntilIdle(); + + // Verify that the resolver positively matched the pattern in the policy with + // the test client cert and configured the network. + std::string pkcs11_id; + GetClientCertProperties(&pkcs11_id); + EXPECT_EQ(test_cert_id_, pkcs11_id); + EXPECT_EQ(1, network_properties_changed_count_); +} + TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) { - SetupTestCerts(); + SetupTestCerts(true /* import issuer */); SetupWifi(); base::RunLoop().RunUntilIdle(); SetupNetworkHandlers(); - SetupPolicy(); + SetupPolicyMatchingIssuerPEM(); base::RunLoop().RunUntilIdle(); network_properties_changed_count_ = 0; @@ -286,7 +345,7 @@ TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) { } TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) { - SetupTestCerts(); + SetupTestCerts(true /* import issuer */); SetupWifi(); base::RunLoop().RunUntilIdle(); StartCertLoader(); @@ -295,7 +354,7 @@ TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) { // Policy application will trigger the ClientCertResolver. network_properties_changed_count_ = 0; - SetupPolicy(); + SetupPolicyMatchingIssuerPEM(); base::RunLoop().RunUntilIdle(); // Verify that the resolver positively matched the pattern in the policy with -- cgit v1.1