Added a PolicyCertVerifier that uses the trust anchors from the ONC policies.

chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/policy/policy_cert_verifier.h"
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/run_loop.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/testing_profile_manager.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread.h"
+#include "crypto/nss_util.h"
+#include "net/base/cert_test_util.h"
+#include "net/base/cert_trust_anchor_provider.h"
+#include "net/base/cert_verify_result.h"
+#include "net/base/net_log.h"
+#include "net/base/nss_cert_database.h"
+#include "net/base/test_completion_callback.h"
+#include "net/base/test_data_directory.h"
+#include "net/base/x509_certificate.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Mock;
+using testing::ReturnRef;
+
+namespace policy {
+
+namespace {
+
+class MockCertTrustAnchorProvider : public net::CertTrustAnchorProvider {
+ public:
+  MockCertTrustAnchorProvider() {}
+  virtual ~MockCertTrustAnchorProvider() {}
+
+  MOCK_METHOD0(GetAdditionalTrustAnchors, const net::CertificateList&());
+};
+
+}  // namespace
+
+// This is actually a unit test, but is linked with browser_tests because
+// importing a certificate into the NSS test database persists for the duration
+// of a process; since each browser_test runs in a separate process then this
+// won't affect subsequent tests.
+// This can be moved to the unittests target once the TODO in ~ScopedTestNSSDB
+// is fixed.

[Ryan Sleevi, 2013/03/25 21:09:53]

The TODO has been fixed since NSS 3.13.

Since this only works 3.14.2+, it should be fine. If the code is commented out
in ScopedTestNSSDB, then we should move that to a version check.

+class PolicyCertVerifierTest : public testing::Test {
+ public:
+  PolicyCertVerifierTest()
+      : cert_db_(NULL),
+        ui_thread_(content::BrowserThread::UI, &loop_),
+        io_thread_(content::BrowserThread::IO, &loop_),
+        profile_manager_(TestingBrowserProcess::GetGlobal()),
+        profile_(NULL) {}
+
+  virtual ~PolicyCertVerifierTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(test_nssdb_.is_open());
+    cert_db_ = net::NSSCertDatabase::GetInstance();
+
+    ASSERT_TRUE(profile_manager_.SetUp());
+    profile_ = profile_manager_.CreateTestingProfile("profile");
+
+    cert_verifier_.reset(new PolicyCertVerifier(profile_, &trust_provider_));
+  }
+
+  scoped_refptr<net::X509Certificate> LoadCertificate(const std::string& name,
+                                                      net::CertType type) {
+    scoped_refptr<net::X509Certificate> cert =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), name);
+
+    // No certificate is trusted right after it's loaded.
+    net::NSSCertDatabase::TrustBits trust =
+        cert_db_->GetCertTrust(cert.get(), type);
+    EXPECT_EQ(net::NSSCertDatabase::TRUST_DEFAULT, trust);
+
+    return cert;
+  }
+
+ protected:
+  crypto::ScopedTestNSSDB test_nssdb_;
+  net::NSSCertDatabase* cert_db_;
+  MessageLoop loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread io_thread_;
+  TestingProfileManager profile_manager_;
+  TestingProfile* profile_;
+  MockCertTrustAnchorProvider trust_provider_;
+  scoped_ptr<PolicyCertVerifier> cert_verifier_;
+  const net::CertificateList empty_cert_list_;
+};
+
+TEST_F(PolicyCertVerifierTest, VerifyUntrustedCert) {
+  scoped_refptr<net::X509Certificate> cert =
+      LoadCertificate("ok_cert.pem", net::SERVER_CERT);

[pneubeck (no reviews), 2013/03/26 11:04:50]

again the same question: do you have an idea if we can replace these loads by
fake certs?

[Joao da Silva, 2013/03/31 19:22:14]

We certainly can generate the certs at runtime, but why the additional work and
cost at runtime?

+  ASSERT_TRUE(cert);
+
+  // |cert| is untrusted, so Verify() fails.
+  int error;
+  net::CertVerifyResult verify_result;
+  net::TestCompletionCallback callback;
+  net::CertVerifier::RequestHandle request_handle;
+  EXPECT_CALL(trust_provider_, GetAdditionalTrustAnchors())
+      .WillOnce(ReturnRef(empty_cert_list_));
+  error = cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,

[pneubeck (no reviews), 2013/03/26 11:04:50]

int error = ...

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+                                 &verify_result, callback.callback(),
+                                 &request_handle, net::BoundNetLog());
+  Mock::VerifyAndClearExpectations(&trust_provider_);
+  ASSERT_EQ(net::ERR_IO_PENDING, error);
+  ASSERT_TRUE(request_handle);
+  error = callback.WaitForResult();
+  EXPECT_EQ(net::ERR_CERT_AUTHORITY_INVALID, error);
+
+  // Issuing the same request again hits the cache. This tests the synchronous

[pneubeck (no reviews), 2013/03/26 11:04:50]

The cache is in the implementation of MultiThreadedCertVerifier? Then this tests
the MultiThreadedCertVerifier, is that really necessary?

[Joao da Silva, 2013/03/31 19:22:14]

This relies on that behavior of the MultiThreadedCertVerifier, but tests a
different path in PolicyCertVerifier::Verify. Verify() won't used the wrapped
callback when the inner cert_verifier returns immediately (instead of returning
IO_PENDING).

+  // path.
+  EXPECT_CALL(trust_provider_, GetAdditionalTrustAnchors())
+      .WillOnce(ReturnRef(empty_cert_list_));
+  error = cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
+                                 &verify_result, callback.callback(),
+                                 &request_handle, net::BoundNetLog());
+  Mock::VerifyAndClearExpectations(&trust_provider_);
+  EXPECT_EQ(net::ERR_CERT_AUTHORITY_INVALID, error);
+
+  // The profile is not tainted.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile_->GetPrefs()->GetBoolean(prefs::kUsedPolicyCertificatesOnce));
+}
+
+TEST_F(PolicyCertVerifierTest, VerifyTrustedCert) {
+  // |ca_cert| is the issuer of |cert|.
+  scoped_refptr<net::X509Certificate> ca_cert =
+      LoadCertificate("root_ca_cert.crt", net::CA_CERT);
+  ASSERT_TRUE(ca_cert);
+  scoped_refptr<net::X509Certificate> cert =
+      LoadCertificate("ok_cert.pem", net::SERVER_CERT);
+  ASSERT_TRUE(cert);
+
+  // Make the database trust |ca_cert|.
+  net::CertificateList import_list;
+  import_list.push_back(ca_cert);
+  net::NSSCertDatabase::ImportCertFailureList failure_list;
+  ASSERT_TRUE(cert_db_->ImportCACerts(
+      import_list, net::NSSCertDatabase::TRUSTED_SSL, &failure_list));
+  ASSERT_TRUE(failure_list.empty());
+
+  // Verify that it is now trusted.
+  net::NSSCertDatabase::TrustBits trust =

[pneubeck (no reviews), 2013/03/26 11:04:50]

This verifies that ImportCACerts works? Isn't that already tested elsewhere? Or
do you only to ensure that this particular cert can be imported, but then
|failure_list| shouldn't be empty, should it?

[Joao da Silva, 2013/03/31 19:22:14]

This test is meant to verify that manual imports don't taint the Profile, so
that users that import certificates don't see the scary warning.

Even though this seems trivial now, a regression in the future could flag the
Profile by mistake; this also checks that the test setup used in the other 2
tests works as expected (that is, they aren't passing because some other
mechanism is making the verification pass). I usually write both positive and
negative tests, to make sure the test isn't passing due to another knob that I'm
not aware of.

+      cert_db_->GetCertTrust(ca_cert.get(), net::CA_CERT);
+  EXPECT_EQ(net::NSSCertDatabase::TRUSTED_SSL, trust);
+
+  // Verify() successfully verifies |cert| now.

[pneubeck (no reviews), 2013/03/26 11:04:50]

remove "now" as it seems to refer to the previous test.

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+  int error;
+  net::CertVerifyResult verify_result;
+  net::TestCompletionCallback callback;
+  net::CertVerifier::RequestHandle request_handle;
+  EXPECT_CALL(trust_provider_, GetAdditionalTrustAnchors())
+      .WillOnce(ReturnRef(empty_cert_list_));
+  error = cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,

[pneubeck (no reviews), 2013/03/26 11:04:50]

int error =...

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+                                 &verify_result, callback.callback(),
+                                 &request_handle, net::BoundNetLog());
+  Mock::VerifyAndClearExpectations(&trust_provider_);
+  ASSERT_EQ(net::ERR_IO_PENDING, error);
+  ASSERT_TRUE(request_handle);
+  error = callback.WaitForResult();
+  EXPECT_EQ(net::OK, error);
+
+  // The profile is not tainted, since the certificate is trusted from the
+  // database.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile_->GetPrefs()->GetBoolean(prefs::kUsedPolicyCertificatesOnce));
+}
+
+TEST_F(PolicyCertVerifierTest, VerifyUsingAdditionalTrustAnchor) {
+  // |ca_cert| is the issuer of |cert|.
+  scoped_refptr<net::X509Certificate> ca_cert =
+      LoadCertificate("root_ca_cert.crt", net::CA_CERT);
+  ASSERT_TRUE(ca_cert);
+  scoped_refptr<net::X509Certificate> cert =
+      LoadCertificate("ok_cert.pem", net::SERVER_CERT);
+  ASSERT_TRUE(cert);
+
+  net::CertificateList additional_trust_anchors;
+  additional_trust_anchors.push_back(ca_cert);
+
+  // Verify() successfully verifies |cert|, using |ca_cert| from the list of
+  // |additional_trust_anchors|.
+  int error;
+  net::CertVerifyResult verify_result;
+  net::TestCompletionCallback callback;
+  net::CertVerifier::RequestHandle request_handle;
+  EXPECT_CALL(trust_provider_, GetAdditionalTrustAnchors())
+      .WillOnce(ReturnRef(additional_trust_anchors));
+  error = cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,

[pneubeck (no reviews), 2013/03/26 11:04:50]

int error = ...

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+                                 &verify_result, callback.callback(),
+                                 &request_handle, net::BoundNetLog());
+  Mock::VerifyAndClearExpectations(&trust_provider_);
+  ASSERT_EQ(net::ERR_IO_PENDING, error);
+  ASSERT_TRUE(request_handle);
+  error = callback.WaitForResult();
+  EXPECT_EQ(net::OK, error);
+
+  // The profile becomes tainted after using the trust anchors that came from
+  // the policy configuration.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(
+      profile_->GetPrefs()->GetBoolean(prefs::kUsedPolicyCertificatesOnce));
+}
+
+}  // namespace policy