Enable CertVerifierService for ChromeOS

Adds |additional_untrusted_authorities| to CertVerifier::Config.
This will currently only be used on ChromeOS to expose
additional temporary certificates to NSS when policy requires
it. These certificates unfortunately round-trip through the
NetworkService: when the policy-provided certificates are
updated, ProfileNetworkContextService calls
NetworkContext::UpdateAdditionalCertificates(). This in turn
will call CertVerifierService::SetConfig() with the
policy-provided certificates.

This CL also instantiates the CertVerifierService on the IO
thread (instead of UI) in the browser process on ChromeOS. This
is because CertVerifierBuiltin for ChromeOS uses user slots,
which are initialized on the IO thread.

This CL also parameterizes the browsertests in
//chrome/browser/chromeos/policy/policy_certs_browsertest.cc
to run both with and without the CertVerifierService enabled,
to test the changes in this CL.

Bug: 1085379, 1015134
Change-Id: Ibb623d9e53a46819ea5152e72d4f343ae52e8d60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2243867
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782259}

chrome/browser/chromeos/policy/policy_certs_browsertest.cc

@@ -75,6 +75,8 @@
 #include "net/test/test_data_directory.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/features.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 namespace em = enterprise_management;
@@ -431,7 +433,9 @@ bool IsCertInCertificateList(
 
 // Allows testing if user policy provided trust roots take effect, without
 // having device policy.
-class PolicyProvidedCertsRegularUserTest : public InProcessBrowserTest {
+class PolicyProvidedCertsRegularUserTest
+    : public InProcessBrowserTest,
+      public ::testing::WithParamInterface<bool> {
  protected:
   PolicyProvidedCertsRegularUserTest() {
     // Use the same testing slot as private and public slot for testing.
@@ -443,24 +447,34 @@ class PolicyProvidedCertsRegularUserTest : public InProcessBrowserTest {
   }
 
   void SetUpInProcessBrowserTestFixture() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          network::features::kCertVerifierService);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          network::features::kCertVerifierService);
+    }
+
     InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
     user_policy_certs_helper_.SetUpInProcessBrowserTestFixture();
   }
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   UserPolicyCertsHelper user_policy_certs_helper_;
 
   crypto::ScopedTestNSSDB test_nssdb_;
   std::unique_ptr<net::NSSCertDatabase> test_nss_cert_db_;
 };
 
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsRegularUserTest, TrustAnchorApplied) {
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsRegularUserTest, TrustAnchorApplied) {
   user_policy_certs_helper_.SetRootCertONCUserPolicy(browser()->profile());
   EXPECT_EQ(net::OK,
             VerifyTestServerCert(browser()->profile(),
                                  user_policy_certs_helper_.server_cert()));
 }
 
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsRegularUserTest,
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsRegularUserTest,
                        UntrustedIntermediateAuthorityApplied) {
   // Sanity check: Apply ONC policy which does not mention the intermediate
   // authority.
@@ -480,7 +494,7 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsRegularUserTest,
                 user_policy_certs_helper_.server_cert_by_intermediate()));
 }
 
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsRegularUserTest,
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsRegularUserTest,
                        AuthorityAvailableThroughNetworkCertLoader) {
   // Set |NetworkCertLoader| to use a test NSS database - otherwise, it is not
   // properly initialized because |UserSessionManager| only sets the primary
@@ -505,10 +519,15 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsRegularUserTest,
       chromeos::NetworkCertLoader::Get()->authority_certs()));
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         PolicyProvidedCertsRegularUserTest,
+                         ::testing::Bool());
+
 // Base class for testing policy-provided trust roots with device-local
 // accounts. Needs device policy.
 class PolicyProvidedCertsDeviceLocalAccountTest
-    : public DevicePolicyCrosBrowserTest {
+    : public DevicePolicyCrosBrowserTest,
+      public ::testing::WithParamInterface<bool> {
  public:
   PolicyProvidedCertsDeviceLocalAccountTest() {
     // Use the same testing slot as private and public slot for testing.
@@ -523,6 +542,14 @@ class PolicyProvidedCertsDeviceLocalAccountTest
   virtual void SetupDevicePolicy() = 0;
 
   void SetUpInProcessBrowserTestFixture() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          network::features::kCertVerifierService);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          network::features::kCertVerifierService);
+    }
+
     DevicePolicyCrosBrowserTest::SetUpInProcessBrowserTestFixture();
 
     user_policy_certs_helper_.SetUpInProcessBrowserTestFixture();
@@ -542,6 +569,8 @@ class PolicyProvidedCertsDeviceLocalAccountTest
     command_line->AppendSwitch(chromeos::switches::kOobeSkipPostLogin);
   }
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   chromeos::LocalPolicyTestServerMixin local_policy_mixin_{&mixin_host_};
 
   const AccountId device_local_account_id_ =
@@ -596,7 +625,7 @@ class PolicyProvidedCertsPublicSessionTest
 
 // TODO(https://crbug.com/874831): Re-enable this after the source of the
 // flakiness has been identified.
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsPublicSessionTest,
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsPublicSessionTest,
                        DISABLED_AllowedInPublicSession) {
   StartLogin();
   chromeos::test::WaitForPrimaryUserSessionStart();
@@ -612,10 +641,28 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsPublicSessionTest,
                                  user_policy_certs_helper_.server_cert()));
 }
 
-class PolicyProvidedCertsOnUserSessionInitTest : public LoginPolicyTestBase {
+INSTANTIATE_TEST_SUITE_P(All,
+                         PolicyProvidedCertsPublicSessionTest,
+                         ::testing::Bool());
+
+class PolicyProvidedCertsOnUserSessionInitTest
+    : public LoginPolicyTestBase,
+      public ::testing::WithParamInterface<bool> {
  protected:
   PolicyProvidedCertsOnUserSessionInitTest() {}
 
+  void SetUpInProcessBrowserTestFixture() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          network::features::kCertVerifierService);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          network::features::kCertVerifierService);
+    }
+
+    LoginPolicyTestBase::SetUpInProcessBrowserTestFixture();
+  }
+
   void GetMandatoryPoliciesValue(base::DictionaryValue* policy) const override {
     std::string user_policy_blob = GetTestCertsFileContents(kRootCaCertOnc);
     policy->SetKey(key::kOpenNetworkConfiguration,
@@ -638,12 +685,13 @@ class PolicyProvidedCertsOnUserSessionInitTest : public LoginPolicyTestBase {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
   DISALLOW_COPY_AND_ASSIGN(PolicyProvidedCertsOnUserSessionInitTest);
 };
 
 // Verifies that the policy-provided trust root is active as soon as the user
 // session starts.
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsOnUserSessionInitTest,
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsOnUserSessionInitTest,
                        TrustAnchorsAvailableImmediatelyAfterSessionStart) {
   // Load the certificate which is only OK if the policy-provided authority is
   // actually trusted.
@@ -662,13 +710,27 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsOnUserSessionInitTest,
   EXPECT_EQ(net::OK, VerifyTestServerCert(active_user_profile(), server_cert));
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         PolicyProvidedCertsOnUserSessionInitTest,
+                         ::testing::Bool());
+
 // Testing policy-provided client cert import.
-class PolicyProvidedClientCertsTest : public DevicePolicyCrosBrowserTest {
+class PolicyProvidedClientCertsTest
+    : public DevicePolicyCrosBrowserTest,
+      public ::testing::WithParamInterface<bool> {
  protected:
   PolicyProvidedClientCertsTest() {}
   ~PolicyProvidedClientCertsTest() override {}
 
   void SetUpInProcessBrowserTestFixture() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          network::features::kCertVerifierService);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          network::features::kCertVerifierService);
+    }
+
     // Set up the mock policy provider.
     EXPECT_CALL(provider_, IsInitializationComplete(testing::_))
         .WillRepeatedly(testing::Return(true));
@@ -697,10 +759,11 @@ class PolicyProvidedClientCertsTest : public DevicePolicyCrosBrowserTest {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
   MockConfigurationPolicyProvider provider_;
 };
 
-IN_PROC_BROWSER_TEST_F(PolicyProvidedClientCertsTest, ClientCertsImported) {
+IN_PROC_BROWSER_TEST_P(PolicyProvidedClientCertsTest, ClientCertsImported) {
   // Sanity check: we don't expect the client certificate to be present before
   // setting the user ONC policy.
   EXPECT_FALSE(
@@ -711,21 +774,32 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedClientCertsTest, ClientCertsImported) {
       IsCertInNSSDatabase(browser()->profile(), kClientCertSubjectCommonName));
 }
 
+INSTANTIATE_TEST_SUITE_P(All, PolicyProvidedClientCertsTest, ::testing::Bool());
+
 // TODO(https://crbug.com/874937): Add a test case for a kiosk session.
 
 // Class for testing policy-provided extensions in the sign-in profile.
 // Sets a device policy which applies the |kRootCaCert| for
 // |kSigninScreenExtension1|. Force-installs |kSigninScreenExtension1| and
 // |kSigninScreenExtension2| into the sign-in profile.
 class PolicyProvidedCertsForSigninExtensionTest
-    : public SigninProfileExtensionsPolicyTestBase {
+    : public SigninProfileExtensionsPolicyTestBase,
+      public ::testing::WithParamInterface<bool> {
  protected:
   // Use DEV channel as sign-in screen extensions are currently usable there.
   PolicyProvidedCertsForSigninExtensionTest()
       : SigninProfileExtensionsPolicyTestBase(version_info::Channel::DEV) {}
   ~PolicyProvidedCertsForSigninExtensionTest() override = default;
 
   void SetUpInProcessBrowserTestFixture() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          network::features::kCertVerifierService);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          network::features::kCertVerifierService);
+    }
+
     // Apply |kRootCaCert| for |kSigninScreenExtension1| in Device ONC policy.
     base::FilePath test_certs_path = GetTestCertsPath();
     std::string x509_contents;
@@ -821,6 +895,8 @@ class PolicyProvidedCertsForSigninExtensionTest
     return onc_dict;
   }
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(PolicyProvidedCertsForSigninExtensionTest);
 };  // namespace policy
 
@@ -834,7 +910,7 @@ class PolicyProvidedCertsForSigninExtensionTest
 // Verification of all these aspects has been intentionally put into one test,
 // so if the verification result leaks (e.g. due to accidentally reusing
 // caches), the test is able to catch that.
-IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsForSigninExtensionTest,
+IN_PROC_BROWSER_TEST_P(PolicyProvidedCertsForSigninExtensionTest,
                        ActiveOnlyInSelectedExtension) {
   chromeos::OobeScreenWaiter(chromeos::GaiaView::kScreenId).Wait();
   content::StoragePartition* signin_profile_default_partition =
@@ -874,4 +950,8 @@ IN_PROC_BROWSER_TEST_F(PolicyProvidedCertsForSigninExtensionTest,
                                                    server_cert_));
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         PolicyProvidedCertsForSigninExtensionTest,
+                         ::testing::Bool());
+
 }  // namespace policy

chrome/browser/net/cert_verify_proc_browsertest.cc

@@ -91,14 +91,8 @@ class CertVerifyProcNetLogBrowserTest
  public:
   void SetUpInProcessBrowserTestFixture() override {
     if (GetParam()) {
-#if defined(OS_CHROMEOS)
-      // TODO(crbug.com/1085379): remove this GTEST_SKIP().
-      GTEST_SKIP() << "Skipping test, CertVerifierService feature not yet "
-                      "available on ChromeOS.";
-#else
       scoped_feature_list_.InitAndEnableFeature(
           network::features::kCertVerifierService);
-#endif
     } else {
       scoped_feature_list_.InitAndDisableFeature(
           network::features::kCertVerifierService);

chrome/browser/net/profile_network_context_service_browsertest.cc

@@ -475,16 +475,10 @@ class ProfileNetworkContextServiceCertVerifierBuiltinFeaturePolicyTest
       disabled_features.push_back(net::features::kCertVerifierBuiltinFeature);
     }
     if (enable_cert_verifier_service()) {
-#if defined(OS_CHROMEOS)
-      // TODO(crbug.com/1085379): remove this GTEST_SKIP().
-      GTEST_SKIP() << "Skipping test, CertVerifierService feature not yet "
-                      "available on ChromeOS.";
-#else
       enabled_features.push_back(network::features::kCertVerifierService);
       test_cert_verifier_service_factory_.emplace();
       content::SetCertVerifierServiceFactoryForTesting(
           &test_cert_verifier_service_factory_.value());
-#endif
     } else {
       disabled_features.push_back(network::features::kCertVerifierService);
     }

chrome/browser/net/system_network_context_manager_browsertest.cc

@@ -483,16 +483,10 @@ class SystemNetworkContextServiceCertVerifierBuiltinFeaturePolicyTest
       disabled_features.push_back(net::features::kCertVerifierBuiltinFeature);
     }
     if (enable_cert_verification_service_) {
-#if defined(OS_CHROMEOS)
-      // TODO(crbug.com/1085379): remove this GTEST_SKIP().
-      GTEST_SKIP() << "Skipping test, CertVerifierService feature not yet "
-                      "available on ChromeOS.";
-#else
       enabled_features.push_back(network::features::kCertVerifierService);
       test_cert_verifier_service_factory_.emplace();
       content::SetCertVerifierServiceFactoryForTesting(
           &test_cert_verifier_service_factory_.value());
-#endif
     } else {
       disabled_features.push_back(network::features::kCertVerifierService);
     }

content/browser/network_service_instance_impl.cc

@@ -497,6 +497,14 @@ GetNewCertVerifierServiceRemote(
   return cert_verifier_remote;
 }
 
+void CreateInProcessCertVerifierServiceOnThread(
+    mojo::PendingReceiver<cert_verifier::mojom::CertVerifierServiceFactory>
+        receiver) {
+  // Except in tests, our CertVerifierServiceFactoryImpl is a singleton.
+  static base::NoDestructor<cert_verifier::CertVerifierServiceFactoryImpl>
+      cv_service_factory(std::move(receiver));
+}
+
 // Owns the CertVerifierServiceFactory used by the browser.
 // Lives on the UI thread.
 class CertVerifierServiceFactoryOwner {
@@ -528,10 +536,18 @@ class CertVerifierServiceFactoryOwner {
   cert_verifier::mojom::CertVerifierServiceFactory*
   GetCertVerifierServiceFactory() {
     if (!service_factory_) {
-      // Except in tests, our CertVerifierServiceFactoryImpl is a singleton.
-      static base::NoDestructor<cert_verifier::CertVerifierServiceFactoryImpl>
-          cv_service_factory(
-              service_factory_remote_.BindNewPipeAndPassReceiver());
+#if defined(OS_CHROMEOS)
+      // ChromeOS's in-process CertVerifierService should run on the IO thread
+      // because it interacts with IO-bound NSS and ChromeOS user slots.
+      // See for example InitializeNSSForChromeOSUser().
+      GetIOThreadTaskRunner({})->PostTask(
+          FROM_HERE,
+          base::BindOnce(&CreateInProcessCertVerifierServiceOnThread,
+                         service_factory_remote_.BindNewPipeAndPassReceiver()));
+#else
+      CreateInProcessCertVerifierServiceOnThread(
+          service_factory_remote_.BindNewPipeAndPassReceiver());
+#endif
       service_factory_ = service_factory_remote_.get();
     }
     return service_factory_;

net/cert/cert_verifier.cc

@@ -120,11 +120,13 @@ bool operator==(const CertVerifier::Config& lhs,
   return std::tie(
              lhs.enable_rev_checking, lhs.require_rev_checking_local_anchors,
              lhs.enable_sha1_local_anchors, lhs.disable_symantec_enforcement,
-             lhs.crl_set, lhs.additional_trust_anchors) ==
+             lhs.crl_set, lhs.additional_trust_anchors,
+             lhs.additional_untrusted_authorities) ==
          std::tie(
              rhs.enable_rev_checking, rhs.require_rev_checking_local_anchors,
              rhs.enable_sha1_local_anchors, rhs.disable_symantec_enforcement,
-             rhs.crl_set, rhs.additional_trust_anchors);
+             rhs.crl_set, rhs.additional_trust_anchors,
+             rhs.additional_untrusted_authorities);
 }
 
 bool operator!=(const CertVerifier::Config& lhs,

net/cert/cert_verifier.h

@@ -65,6 +65,12 @@ class NET_EXPORT CertVerifier {
     // system store. This is implementation-specific plumbing for passing
     // additional anchors through.
     CertificateList additional_trust_anchors;
+
+    // Additional temporary certs to consider as intermediates during path
+    // validation. Ordinarily, implementations of CertVerifier use intermediate
+    // certs from the configured system store. This is implementation-specific
+    // plumbing for passing additional intermediates through.
+    CertificateList additional_untrusted_authorities;
   };
 
   class Request {