Fix SetCTPolicy not getting called on network service restart.

Bug: 890104
Change-Id: Id2f726cf4a2052080c1f984cb5844f1d084bf1b0
Reviewed-on: https://chromium-review.googlesource.com/1249732
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595230}

chrome/browser/net/profile_network_context_service.cc

@@ -20,6 +20,7 @@
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/pref_names.h"
+#include "components/certificate_transparency/pref_names.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
@@ -38,6 +39,19 @@
 #include "extensions/common/constants.h"
 #endif
 
+namespace {
+
+std::vector<std::string> TranslateStringArray(const base::ListValue* list) {
+  std::vector<std::string> strings;
+  for (const base::Value& value : *list) {
+    DCHECK(value.is_string());
+    strings.push_back(value.GetString());
+  }
+  return strings;
+}
+
+}  // namespace
+
 ProfileNetworkContextService::ProfileNetworkContextService(Profile* profile)
     : profile_(profile), proxy_config_monitor_(profile) {
   PrefService* profile_prefs = profile->GetPrefs();
@@ -62,6 +76,27 @@ ProfileNetworkContextService::ProfileNetworkContextService(Profile* profile)
 
   // Observe content settings so they can be synced to the network service.
   HostContentSettingsMapFactory::GetForProfile(profile_)->AddObserver(this);
+
+  pref_change_registrar_.Init(profile_prefs);
+
+  // When any of the following CT preferences change, we schedule an update
+  // to aggregate the actual update using a |ct_policy_update_timer_|.
+  pref_change_registrar_.Add(
+      certificate_transparency::prefs::kCTRequiredHosts,
+      base::BindRepeating(&ProfileNetworkContextService::ScheduleUpdateCTPolicy,
+                          base::Unretained(this)));
+  pref_change_registrar_.Add(
+      certificate_transparency::prefs::kCTExcludedHosts,
+      base::BindRepeating(&ProfileNetworkContextService::ScheduleUpdateCTPolicy,
+                          base::Unretained(this)));
+  pref_change_registrar_.Add(
+      certificate_transparency::prefs::kCTExcludedSPKIs,
+      base::BindRepeating(&ProfileNetworkContextService::ScheduleUpdateCTPolicy,
+                          base::Unretained(this)));
+  pref_change_registrar_.Add(
+      certificate_transparency::prefs::kCTExcludedLegacySPKIs,
+      base::BindRepeating(&ProfileNetworkContextService::ScheduleUpdateCTPolicy,
+                          base::Unretained(this)));
 }
 
 ProfileNetworkContextService::~ProfileNetworkContextService() {}
@@ -73,7 +108,11 @@ ProfileNetworkContextService::CreateNetworkContext(
   network::mojom::NetworkContextPtr network_context;
   PartitionInfo partition_info(in_memory, relative_partition_path);
 
-  if (!base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+    content::GetNetworkService()->CreateNetworkContext(
+        MakeRequest(&network_context),
+        CreateNetworkContextParams(in_memory, relative_partition_path));
+  } else {
     // The corresponding |profile_io_data_network_contexts_| may already be
     // initialized if SetUpProfileIODataNetworkContext was called first.
     auto iter = profile_io_data_network_contexts_.find(partition_info);
@@ -91,12 +130,11 @@ ProfileNetworkContextService::CreateNetworkContext(
       // and NetworkContexts can't be destroyed without destroying the profile.
       profile_io_data_network_contexts_.erase(iter);
     }
-    return network_context;
   }
 
-  content::GetNetworkService()->CreateNetworkContext(
-      MakeRequest(&network_context),
-      CreateNetworkContextParams(in_memory, relative_partition_path));
+  std::vector<network::mojom::NetworkContext*> contexts{network_context.get()};
+  UpdateCTPolicyForContexts(contexts);
+
   return network_context;
 }
 
@@ -193,6 +231,51 @@ void ProfileNetworkContextService::UpdateReferrersEnabled() {
           enable_referrers_.GetValue()));
 }
 
+void ProfileNetworkContextService::UpdateCTPolicyForContexts(
+    const std::vector<network::mojom::NetworkContext*>& contexts) {
+  auto* prefs = profile_->GetPrefs();
+  const base::ListValue* ct_required =
+      prefs->GetList(certificate_transparency::prefs::kCTRequiredHosts);
+  const base::ListValue* ct_excluded =
+      prefs->GetList(certificate_transparency::prefs::kCTExcludedHosts);
+  const base::ListValue* ct_excluded_spkis =
+      prefs->GetList(certificate_transparency::prefs::kCTExcludedSPKIs);
+  const base::ListValue* ct_excluded_legacy_spkis =
+      prefs->GetList(certificate_transparency::prefs::kCTExcludedLegacySPKIs);
+
+  std::vector<std::string> required(TranslateStringArray(ct_required));
+  std::vector<std::string> excluded(TranslateStringArray(ct_excluded));
+  std::vector<std::string> excluded_spkis(
+      TranslateStringArray(ct_excluded_spkis));
+  std::vector<std::string> excluded_legacy_spkis(
+      TranslateStringArray(ct_excluded_legacy_spkis));
+
+  for (auto* context : contexts) {
+    context->SetCTPolicy(required, excluded, excluded_spkis,
+                         excluded_legacy_spkis);
+  }
+}
+
+void ProfileNetworkContextService::UpdateCTPolicy() {
+  std::vector<network::mojom::NetworkContext*> contexts;
+  content::BrowserContext::ForEachStoragePartition(
+      profile_,
+      base::BindRepeating(
+          [](std::vector<network::mojom::NetworkContext*>* contexts_ptr,
+             content::StoragePartition* storage_partition) {
+            contexts_ptr->push_back(storage_partition->GetNetworkContext());
+          },
+          &contexts));
+
+  UpdateCTPolicyForContexts(contexts);
+}
+
+void ProfileNetworkContextService::ScheduleUpdateCTPolicy() {
+  ct_policy_update_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(0),
+                                this,
+                                &ProfileNetworkContextService::UpdateCTPolicy);
+}
+
 void ProfileNetworkContextService::FlushProxyConfigMonitorForTesting() {
   proxy_config_monitor_.FlushForTesting();
 }

chrome/browser/net/profile_network_context_service.h

@@ -9,9 +9,11 @@
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/net/proxy_config_monitor.h"
 #include "components/content_settings/core/browser/content_settings_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_member.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
@@ -86,6 +88,15 @@ class ProfileNetworkContextService : public KeyedService,
 
   void UpdateReferrersEnabled();
 
+  // Update the CTPolicy for the given NetworkContexts.
+  void UpdateCTPolicyForContexts(
+      const std::vector<network::mojom::NetworkContext*>& contexts);
+
+  // Update the CTPolicy for the all of profiles_'s NetworkContexts.
+  void UpdateCTPolicy();
+
+  void ScheduleUpdateCTPolicy();
+
   // Creates parameters for the NetworkContext. Use |in_memory| instead of
   // |profile_->IsOffTheRecord()| because sometimes normal profiles want off the
   // record partitions (e.g. for webview tag).
@@ -123,8 +134,11 @@ class ProfileNetworkContextService : public KeyedService,
   BooleanPrefMember quic_allowed_;
   StringPrefMember pref_accept_language_;
   BooleanPrefMember block_third_party_cookies_;
-
   BooleanPrefMember enable_referrers_;
+  PrefChangeRegistrar pref_change_registrar_;
+
+  // Used to post schedule CT policy updates
+  base::OneShotTimer ct_policy_update_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileNetworkContextService);
 };

chrome/browser/policy/policy_browsertest.cc

@@ -188,6 +188,7 @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_constants.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_paths.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/result_codes.h"
@@ -4580,6 +4581,28 @@ IN_PROC_BROWSER_TEST_P(SSLPolicyTestCommittedInterstitials,
   UpdateProviderPolicy(policies);
   FlushBlacklistPolicy();
 
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_ok.GetURL("/simple.html"));
+
+  // There should be no interstitial after the page loads.
+  EXPECT_FALSE(IsShowingInterstitial(tab));
+  EXPECT_EQ(base::UTF8ToUTF16("OK"),
+            browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
+
+  // Now ensure that this setting still works after a network process crash.
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService) ||
+      base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSingleProcess) ||
+      base::FeatureList::IsEnabled(features::kNetworkServiceInProcess)) {
+    return;
+  }
+
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_ok.GetURL("/title1.html"));
+
+  SimulateNetworkServiceCrash();
+  SetShouldRequireCTForTesting(&required);
+
   ui_test_utils::NavigateToURL(browser(),
                                https_server_ok.GetURL("/simple.html"));
 

chrome/browser/profiles/profile_impl.cc

@@ -101,7 +101,6 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "components/bookmarks/browser/bookmark_model.h"
-#include "components/certificate_transparency/pref_names.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/pref_names.h"
@@ -616,21 +615,6 @@ void ProfileImpl::DoFinalInit() {
       base::Bind(&ProfileImpl::UpdateIsEphemeralInStorage,
                  base::Unretained(this)));
 
-  // When any of the following CT preferences change, we schedule an update
-  // to aggregate the actual update using a |ct_policy_update_timer_|.
-  pref_change_registrar_.Add(
-      certificate_transparency::prefs::kCTRequiredHosts,
-      base::Bind(&ProfileImpl::ScheduleUpdateCTPolicy, base::Unretained(this)));
-  pref_change_registrar_.Add(
-      certificate_transparency::prefs::kCTExcludedHosts,
-      base::Bind(&ProfileImpl::ScheduleUpdateCTPolicy, base::Unretained(this)));
-  pref_change_registrar_.Add(
-      certificate_transparency::prefs::kCTExcludedSPKIs,
-      base::Bind(&ProfileImpl::ScheduleUpdateCTPolicy, base::Unretained(this)));
-  pref_change_registrar_.Add(
-      certificate_transparency::prefs::kCTExcludedLegacySPKIs,
-      base::Bind(&ProfileImpl::ScheduleUpdateCTPolicy, base::Unretained(this)));
-
   media_device_id_salt_ = new MediaDeviceIDSalt(prefs_.get());
 
   // It would be nice to use PathService for fetching this directory, but
@@ -744,8 +728,6 @@ void ProfileImpl::DoFinalInit() {
 
   content::URLDataSource::Add(this,
                               std::make_unique<PrefsInternalsSource>(this));
-
-  ScheduleUpdateCTPolicy();
 }
 
 base::FilePath ProfileImpl::last_selected_directory() {
@@ -1459,36 +1441,6 @@ void ProfileImpl::UpdateIsEphemeralInStorage() {
   }
 }
 
-std::vector<std::string> TranslateStringArray(const base::ListValue* list) {
-  std::vector<std::string> strings;
-  for (const base::Value& value : *list) {
-    DCHECK(value.is_string());
-    strings.push_back(value.GetString());
-  }
-  return strings;
-}
-
-void ProfileImpl::ScheduleUpdateCTPolicy() {
-  ct_policy_update_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(0),
-                                this, &ProfileImpl::UpdateCTPolicy);
-}
-
-void ProfileImpl::UpdateCTPolicy() {
-  const base::ListValue* ct_required =
-      prefs_->GetList(certificate_transparency::prefs::kCTRequiredHosts);
-  const base::ListValue* ct_excluded =
-      prefs_->GetList(certificate_transparency::prefs::kCTExcludedHosts);
-  const base::ListValue* ct_excluded_spkis =
-      prefs_->GetList(certificate_transparency::prefs::kCTExcludedSPKIs);
-  const base::ListValue* ct_excluded_legacy_spkis =
-      prefs_->GetList(certificate_transparency::prefs::kCTExcludedLegacySPKIs);
-
-  GetDefaultStoragePartition(this)->GetNetworkContext()->SetCTPolicy(
-      TranslateStringArray(ct_required), TranslateStringArray(ct_excluded),
-      TranslateStringArray(ct_excluded_spkis),
-      TranslateStringArray(ct_excluded_legacy_spkis));
-}
-
 // Gets the media cache parameters from the command line. |cache_path| will be
 // set to the user provided path, or will not be touched if there is not an
 // argument. |max_size| will be the user provided value or zero by default.

chrome/browser/profiles/profile_impl.h

@@ -14,7 +14,6 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "chrome/browser/net/reporting_permissions_checker.h"
 #include "chrome/browser/profiles/profile.h"
@@ -191,9 +190,6 @@ class ProfileImpl : public Profile {
   void UpdateNameInStorage();
   void UpdateAvatarInStorage();
   void UpdateIsEphemeralInStorage();
-  void UpdateCTPolicy();
-
-  void ScheduleUpdateCTPolicy();
 
   void GetMediaCacheParameters(base::FilePath* cache_path, int* max_size);
 
@@ -290,9 +286,6 @@ class ProfileImpl : public Profile {
 
   ReportingPermissionsCheckerFactory reporting_permissions_checker_factory_;
 
-  // Used to post schedule CT policy updates
-  base::OneShotTimer ct_policy_update_timer_;
-
   DISALLOW_COPY_AND_ASSIGN(ProfileImpl);
 };