NetworkService: Add HTTPS URL path stripping for PACs and quick check.
Also adds an integration test for path stripping, but not for
quick check.
Bug: 715695
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I45d37089d305c5a6683afbac762ef767b910fb0f
Reviewed-on: https://chromium-review.googlesource.com/1028177
Commit-Queue: Matt Menke <[email protected]>
Reviewed-by: Tom Sepez <[email protected]>
Reviewed-by: Gabriel Charette <[email protected]>
Reviewed-by: Maksim Ivanov <[email protected]>
Reviewed-by: Eric Roman <[email protected]>
Cr-Commit-Position: refs/heads/master@{#555769}diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 29195217..f6918c5e 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -419,14 +419,6 @@
ntlm_v2_enabled_.MoveToThread(io_thread_proxy);
#endif
- quick_check_enabled_.Init(prefs::kQuickCheckEnabled,
- local_state);
- quick_check_enabled_.MoveToThread(io_thread_proxy);
-
- pac_https_url_stripping_enabled_.Init(prefs::kPacHttpsUrlStrippingEnabled,
- local_state);
- pac_https_url_stripping_enabled_.MoveToThread(io_thread_proxy);
-
chrome_browser_net::SetGlobalSTHDistributor(
std::make_unique<certificate_transparency::STHDistributor>());
@@ -624,8 +616,6 @@
registry->RegisterBooleanPref(prefs::kBuiltInDnsClientEnabled, true);
registry->RegisterListPref(prefs::kDnsOverHttpsServers);
registry->RegisterListPref(prefs::kDnsOverHttpsServerMethods);
- registry->RegisterBooleanPref(prefs::kQuickCheckEnabled, true);
- registry->RegisterBooleanPref(prefs::kPacHttpsUrlStrippingEnabled, true);
#if defined(OS_POSIX)
registry->RegisterBooleanPref(prefs::kNtlmV2Enabled, true);
#endif
@@ -754,26 +744,12 @@
chrome_browser_net::GetGlobalSTHDistributor()->UnregisterObserver(observer);
}
-bool IOThread::WpadQuickCheckEnabled() const {
- return quick_check_enabled_.GetValue();
-}
-
-bool IOThread::PacHttpsUrlStrippingEnabled() const {
- return pac_https_url_stripping_enabled_.GetValue();
-}
-
void IOThread::SetUpProxyService(
network::URLRequestContextBuilderMojo* builder) const {
#if defined(OS_CHROMEOS)
builder->SetDhcpFetcherFactory(
std::make_unique<chromeos::DhcpPacFileFetcherFactoryChromeos>());
#endif
-
- builder->set_pac_quick_check_enabled(WpadQuickCheckEnabled());
- builder->set_pac_sanitize_url_policy(
- PacHttpsUrlStrippingEnabled()
- ? net::ProxyResolutionService::SanitizeUrlPolicy::SAFE
- : net::ProxyResolutionService::SanitizeUrlPolicy::UNSAFE);
}
certificate_transparency::TreeStateTracker* IOThread::ct_tree_tracker() const {
diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h
index f9724e8..b4c7a49 100644
--- a/chrome/browser/io_thread.h
+++ b/chrome/browser/io_thread.h
@@ -199,17 +199,6 @@
// Un-registers the |observer|.
void UnregisterSTHObserver(certificate_transparency::STHObserver* observer);
- // Returns true if the indicated proxy resolution features are
- // enabled. These features are controlled through
- // preferences/policy/commandline.
- //
- // For a description of what these features are, and how they are
- // configured, see the comments in pref_names.cc for
- // |kQuickCheckEnabled| and |kPacHttpsUrlStrippingEnabled
- // respectively.
- bool WpadQuickCheckEnabled() const;
- bool PacHttpsUrlStrippingEnabled() const;
-
// Configures |builder|'s ProxyResolutionService based on prefs and policies.
void SetUpProxyService(network::URLRequestContextBuilderMojo* builder) const;
@@ -275,10 +264,6 @@
BooleanPrefMember dns_client_enabled_;
- BooleanPrefMember quick_check_enabled_;
-
- BooleanPrefMember pac_https_url_stripping_enabled_;
-
StringListPrefMember dns_over_https_servers_;
StringListPrefMember dns_over_https_server_methods_;
diff --git a/chrome/browser/net/default_network_context_params.cc b/chrome/browser/net/default_network_context_params.cc
index aec0ef0..332c063 100644
--- a/chrome/browser/net/default_network_context_params.cc
+++ b/chrome/browser/net/default_network_context_params.cc
@@ -15,9 +15,12 @@
#include "chrome/common/channel_info.h"
#include "chrome/common/chrome_content_client.h"
#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
#include "components/policy/core/common/policy_namespace.h"
#include "components/policy/core/common/policy_service.h"
#include "components/policy/policy_constants.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
#include "components/version_info/version_info.h"
#include "content/public/common/content_features.h"
#include "content/public/common/content_switches.h"
@@ -59,6 +62,12 @@
}
}
+ PrefService* local_state = g_browser_process->local_state();
+ network_context_params->pac_quick_check_enabled =
+ local_state->GetBoolean(prefs::kQuickCheckEnabled);
+ network_context_params->dangerously_allow_pac_access_to_secure_urls =
+ !local_state->GetBoolean(prefs::kPacHttpsUrlStrippingEnabled);
+
bool http_09_on_non_default_ports_enabled = false;
const base::Value* value =
g_browser_process->policy_service()
@@ -72,3 +81,8 @@
return network_context_params;
}
+
+void RegisterNetworkContextCreationPrefs(PrefRegistrySimple* registry) {
+ registry->RegisterBooleanPref(prefs::kQuickCheckEnabled, true);
+ registry->RegisterBooleanPref(prefs::kPacHttpsUrlStrippingEnabled, true);
+}
diff --git a/chrome/browser/net/default_network_context_params.h b/chrome/browser/net/default_network_context_params.h
index 09c3028..27b7701 100644
--- a/chrome/browser/net/default_network_context_params.h
+++ b/chrome/browser/net/default_network_context_params.h
@@ -7,7 +7,12 @@
#include "services/network/public/mojom/network_service.mojom.h"
+class PrefRegistrySimple;
+
// Returns default set of parameters for configuring the network service.
network::mojom::NetworkContextParamsPtr CreateDefaultNetworkContextParams();
+// Registers prefs used in creating the default NetworkContextParams.
+void RegisterNetworkContextCreationPrefs(PrefRegistrySimple* registry);
+
#endif // CHROME_BROWSER_NET_DEFAULT_NETWORK_CONTEXT_PARAMS_H_
diff --git a/chrome/browser/net/network_context_configuration_browsertest.cc b/chrome/browser/net/network_context_configuration_browsertest.cc
index 7fc1392b..c7a37262 100644
--- a/chrome/browser/net/network_context_configuration_browsertest.cc
+++ b/chrome/browser/net/network_context_configuration_browsertest.cc
@@ -4,11 +4,13 @@
#include <string>
+#include "base/bind.h"
#include "base/command_line.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/files/scoped_temp_dir.h"
#include "base/guid.h"
+#include "base/location.h"
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
@@ -16,6 +18,7 @@
#include "base/threading/thread_restrictions.h"
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
+#include "chrome/browser/net/default_network_context_params.h"
#include "chrome/browser/net/profile_network_context_service.h"
#include "chrome/browser/net/profile_network_context_service_factory.h"
#include "chrome/browser/net/system_network_context_manager.h"
@@ -30,6 +33,7 @@
#include "components/proxy_config/proxy_config_dictionary.h"
#include "components/proxy_config/proxy_config_pref_names.h"
#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/common/content_switches.h"
#include "content/public/common/url_constants.h"
@@ -40,9 +44,11 @@
#include "net/base/host_port_pair.h"
#include "net/base/load_flags.h"
#include "net/base/net_errors.h"
+#include "net/dns/mock_host_resolver.h"
#include "net/http/http_response_headers.h"
#include "net/test/embedded_test_server/controllable_http_response.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/embedded_test_server_connection_listener.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
#include "net/test/spawned_test_server/spawned_test_server.h"
@@ -125,6 +131,9 @@
}
void SetUpOnMainThread() override {
+ // Used in a bunch of proxy tests. Should not resolve.
+ host_resolver()->AddSimulatedFailure("does.not.resolve.test");
+
controllable_http_response_ =
std::make_unique<net::test_server::ControllableHttpResponse>(
embedded_test_server(), kControllablePath);
@@ -151,7 +160,7 @@
// a proxy.
std::string GetPacScript() const {
return base::StringPrintf(
- "function FindProxyForURL(url, host){ return 'PROXY %s;'; }",
+ "function FindProxyForURL(url, host){ return 'PROXY %s;' }",
net::HostPortPair::FromURL(embedded_test_server()->base_url())
.ToString()
.c_str());
@@ -259,7 +268,7 @@
std::unique_ptr<network::ResourceRequest> request =
std::make_unique<network::ResourceRequest>();
// This URL should be directed to the test server because of the proxy.
- request->url = GURL("http://jabberwocky.test:1872/echo");
+ request->url = GURL("http://does.not.resolve.test:1872/echo");
content::SimpleURLLoaderTestHelper simple_loader_helper;
std::unique_ptr<network::SimpleURLLoader> simple_loader =
@@ -820,7 +829,7 @@
std::unique_ptr<network::ResourceRequest> request =
std::make_unique<network::ResourceRequest>();
// This URL should be directed to the test server because of the proxy.
- request->url = GURL("http://jabberwocky.test:1872/echo");
+ request->url = GURL("http://does.not.resolve.test:1872/echo");
content::SimpleURLLoaderTestHelper simple_loader_helper;
std::unique_ptr<network::SimpleURLLoader> simple_loader =
@@ -834,6 +843,113 @@
EXPECT_EQ(net::ERR_PROXY_CONNECTION_FAILED, simple_loader->NetError());
}
+// Used to test that PAC HTTPS URL stripping works. A different test server is
+// used as the "proxy" based on whether the PAC script sees the full path or
+// not. The servers aren't correctly set up to mimic HTTP proxies that tunnel
+// to an HTTPS test server, so the test fixture just watches for any incoming
+// connection.
+class NetworkContextConfigurationHttpsStrippingPacBrowserTest
+ : public NetworkContextConfigurationBrowserTest {
+ public:
+ NetworkContextConfigurationHttpsStrippingPacBrowserTest() {}
+
+ ~NetworkContextConfigurationHttpsStrippingPacBrowserTest() override {}
+
+ void SetUpCommandLine(base::CommandLine* command_line) override {
+ // Test server HostPortPair, as a string.
+ std::string test_server_host_port_pair =
+ net::HostPortPair::FromURL(embedded_test_server()->base_url())
+ .ToString();
+ // Set up a PAC file that directs to different servers based on the URL it
+ // sees.
+ std::string pac_script = base::StringPrintf(
+ "function FindProxyForURL(url, host) {"
+ // With the test URL stripped of the path, try to use the embedded test
+ // server to establish a an SSL tunnel over an HTTP proxy. The request
+ // will fail with ERR_TUNNEL_CONNECTION_FAILED.
+ " if (url == 'https://does.not.resolve.test:1872/')"
+ " return 'PROXY %s';"
+ // With the full test URL, try to use a domain that does not resolve as
+ // a proxy. Errors connecting to the proxy result in
+ // ERR_PROXY_CONNECTION_FAILED.
+ " if (url == 'https://does.not.resolve.test:1872/foo')"
+ " return 'PROXY does.not.resolve.test';"
+ // Otherwise, use direct. If a connection to "does.not.resolve.test"
+ // tries to use a direction connection, it will fail with
+ // ERR_NAME_NOT_RESOLVED. This path will also
+ // be used by the initial request in NetworkServiceState::kRestarted
+ // tests to make sure the network service process is fully started
+ // before it's crashed and restarted. Using direct in this case avoids
+ // that request failing with an unexpeced error when being directed to a
+ // bogus proxy.
+ " return 'DIRECT';"
+ "}",
+ test_server_host_port_pair.c_str());
+
+ command_line->AppendSwitchASCII(switches::kProxyPacUrl,
+ "data:," + pac_script);
+ }
+};
+
+// Start Chrome and check that PAC HTTPS path stripping is enabled.
+IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
+ PRE_PacHttpsUrlStripping) {
+ ASSERT_FALSE(CreateDefaultNetworkContextParams()
+ ->dangerously_allow_pac_access_to_secure_urls);
+
+ std::unique_ptr<network::ResourceRequest> request =
+ std::make_unique<network::ResourceRequest>();
+ // This URL should be directed to the proxy that fails with
+ // ERR_TUNNEL_CONNECTION_FAILED.
+ request->url = GURL("https://does.not.resolve.test:1872/foo");
+
+ content::SimpleURLLoaderTestHelper simple_loader_helper;
+ std::unique_ptr<network::SimpleURLLoader> simple_loader =
+ network::SimpleURLLoader::Create(std::move(request),
+ TRAFFIC_ANNOTATION_FOR_TESTS);
+
+ simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+ loader_factory(), simple_loader_helper.GetCallback());
+ simple_loader_helper.WaitForCallback();
+ EXPECT_FALSE(simple_loader_helper.response_body());
+ EXPECT_EQ(net::ERR_TUNNEL_CONNECTION_FAILED, simple_loader->NetError());
+
+ // Disable stripping paths from HTTPS PAC URLs for the next test.
+ g_browser_process->local_state()->SetBoolean(
+ prefs::kPacHttpsUrlStrippingEnabled, false);
+ // Check that the changed setting is reflected in the network context params.
+ // The changes aren't applied to existing URLRequestContexts, however, so have
+ // to restart to see the setting change respected.
+ EXPECT_TRUE(CreateDefaultNetworkContextParams()
+ ->dangerously_allow_pac_access_to_secure_urls);
+}
+
+// Restart Chrome and check the case where PAC HTTPS path stripping is disabled.
+// Have to restart Chrome because the setting is only checked on NetworkContext
+// creation.
+IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
+ PacHttpsUrlStripping) {
+ ASSERT_TRUE(CreateDefaultNetworkContextParams()
+ ->dangerously_allow_pac_access_to_secure_urls);
+
+ std::unique_ptr<network::ResourceRequest> request =
+ std::make_unique<network::ResourceRequest>();
+ // This URL should be directed to the proxy that fails with
+ // ERR_PROXY_CONNECTION_FAILED.
+ request->url = GURL("https://does.not.resolve.test:1872/foo");
+
+ content::SimpleURLLoaderTestHelper simple_loader_helper;
+ std::unique_ptr<network::SimpleURLLoader> simple_loader =
+ network::SimpleURLLoader::Create(std::move(request),
+ TRAFFIC_ANNOTATION_FOR_TESTS);
+
+ simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+ loader_factory(), simple_loader_helper.GetCallback());
+ simple_loader_helper.WaitForCallback();
+ EXPECT_FALSE(simple_loader_helper.response_body());
+ EXPECT_EQ(net::ERR_PROXY_CONNECTION_FAILED, simple_loader->NetError());
+}
+
// |NetworkServiceTestHelper| doesn't work on browser_tests on OSX.
#if defined(OS_MACOSX)
// Instiates tests with a prefix indicating which NetworkContext is being
@@ -907,5 +1023,7 @@
NetworkContextConfigurationDataPacBrowserTest);
INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(
NetworkContextConfigurationFtpPacBrowserTest);
+INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(
+ NetworkContextConfigurationHttpsStrippingPacBrowserTest);
} // namespace
diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
index 8973309..63e05dfa 100644
--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -64,6 +64,7 @@
#include "chrome/browser/media/router/media_router_feature.h"
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/media/webrtc/media_stream_devices_controller.h"
+#include "chrome/browser/net/default_network_context_params.h"
#include "chrome/browser/net/prediction_options.h"
#include "chrome/browser/net/url_request_mock_util.h"
#include "chrome/browser/permissions/permission_request_manager.h"
@@ -212,6 +213,7 @@
#include "net/url_request/url_request_interceptor.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/network_switches.h"
+#include "services/network/public/mojom/network_service.mojom.h"
#include "services/network/public/mojom/network_service_test.mojom.h"
#include "services/service_manager/public/cpp/connector.h"
#include "testing/gmock/include/gmock/gmock.h"
@@ -1341,24 +1343,10 @@
namespace {
-// The following helpers retrieve whether https:// URL stripping is
-// enabled for PAC scripts. It needs to run on the IO thread.
-void GetPacHttpsUrlStrippingEnabledOnIOThread(IOThread* io_thread,
- bool* enabled) {
- *enabled = io_thread->PacHttpsUrlStrippingEnabled();
-}
-
bool GetPacHttpsUrlStrippingEnabled() {
- bool enabled;
- base::RunLoop loop;
- BrowserThread::PostTaskAndReply(
- BrowserThread::IO, FROM_HERE,
- base::BindOnce(&GetPacHttpsUrlStrippingEnabledOnIOThread,
- g_browser_process->io_thread(),
- base::Unretained(&enabled)),
- loop.QuitClosure());
- loop.Run();
- return enabled;
+ network::mojom::NetworkContextParamsPtr network_context_params =
+ CreateDefaultNetworkContextParams();
+ return !network_context_params->dangerously_allow_pac_access_to_secure_urls;
}
} // namespace
@@ -1368,6 +1356,8 @@
// default.
IN_PROC_BROWSER_TEST_F(PolicyTest, DisablePacHttpsUrlStripping) {
// Stripping is enabled by default.
+ EXPECT_TRUE(g_browser_process->local_state()->GetBoolean(
+ prefs::kPacHttpsUrlStrippingEnabled));
EXPECT_TRUE(GetPacHttpsUrlStrippingEnabled());
// Disable it via a policy.
@@ -1379,6 +1369,8 @@
content::RunAllPendingInMessageLoop();
// It should now reflect as disabled.
+ EXPECT_FALSE(g_browser_process->local_state()->GetBoolean(
+ prefs::kPacHttpsUrlStrippingEnabled));
EXPECT_FALSE(GetPacHttpsUrlStrippingEnabled());
}
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index d0e4d8e..b7ba0b7 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -30,6 +30,7 @@
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/media/webrtc/media_stream_devices_controller.h"
#include "chrome/browser/metrics/chrome_metrics_service_client.h"
+#include "chrome/browser/net/default_network_context_params.h"
#include "chrome/browser/net/nqe/ui_network_quality_estimator_service.h"
#include "chrome/browser/net/prediction_options.h"
#include "chrome/browser/net/predictor.h"
@@ -353,6 +354,7 @@
ProfileInfoCache::RegisterPrefs(registry);
profiles::RegisterPrefs(registry);
rappor::RapporServiceImpl::RegisterPrefs(registry);
+ RegisterNetworkContextCreationPrefs(registry);
RegisterScreenshotPrefs(registry);
SigninManagerFactory::RegisterPrefs(registry);
SSLConfigServiceManager::RegisterPrefs(registry);
