system-proxy: Propagate worker url to services

The address of the local proxy is sent via a dbus signal when a local
proxy worker becomes active. The local proxy url is sent to the system
services on Chrome OS via Chrome's proxy resolution service. System
services will connect to the local proxy and the local proxy will setup
the connection through the remote web proxy and perform proxy
authentication on behalf of the CrOS services.

BUG=chromium:1042642
TEST=browser_test ProxyResolutionServiceProviderSystemProxyPolicyTest

Change-Id: I73a1c36bb265cba7e6dce0c59431d010a3f096ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2192655
Commit-Queue: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Reviewed-by: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#769055}

chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc

@@ -11,6 +11,10 @@
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_process_platform_part.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/system_proxy_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/storage_partition.h"
@@ -73,14 +77,38 @@ class ProxyLookupRequest : public network::mojom::ProxyLookupClient {
       result = kProxyInfoOnFailure;
     } else {
       result = proxy_info->ToPacString();
+      if (proxy_info->is_http()) {
+        AppendSystemProxyIfActive(&result);
+      }
     }
-
     receiver_.reset();
     std::move(notify_callback_).Run(error, result);
     delete this;
   }
 
  private:
+  // Appends the System-proxy address, if active, to the list of existing
+  // proxies, which can still be used by system services as a fallback if the
+  // local proxy connection fails. System-proxy itself does proxy resolution
+  // trough the same Chrome proxy resolution service to connect to the
+  // remote proxy server. The availability of this feature is controlled by the
+  // |SystemProxySettings| policy.
+  void AppendSystemProxyIfActive(std::string* pac_proxy_list) {
+    policy::SystemProxyManager* system_proxy_manager =
+        g_browser_process->platform_part()
+            ->browser_policy_connector_chromeos()
+            ->GetSystemProxyManager();
+
+    // |system_proxy_manager| may be missing in tests.
+    if (!system_proxy_manager ||
+        system_proxy_manager->SystemServicesProxyPacString().empty()) {
+      return;
+    }
+    *pac_proxy_list = base::StringPrintf(
+        "%s; %s", system_proxy_manager->SystemServicesProxyPacString().c_str(),
+        pac_proxy_list->c_str());
+  }
+
   mojo::Receiver<network::mojom::ProxyLookupClient> receiver_{this};
   ProxyResolutionServiceProvider::NotifyCallback notify_callback_;
 

chrome/browser/chromeos/dbus/proxy_resolution_service_provider_browsertest.cc

@@ -6,8 +6,11 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/system_proxy_manager.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test.h"
@@ -15,6 +18,18 @@
 
 namespace chromeos {
 
+namespace {
+
+constexpr char kLocalProxyUrl[] = "localhost:3128";
+
+// Encode the PAC script as a data: URL.
+std::string GetPacUrl(const char* pac_data) {
+  std::string b64_encoded;
+  base::Base64Encode(pac_data, &b64_encoded);
+  return "data:application/x-javascript-config;base64," + b64_encoded;
+}
+}  // namespace
+
 // Helper for calling ProxyResolutionServiceProvider's |ResolveProxyInternal()|
 // method. Unlike the unit-tests which mock the network setup, this uses the
 // default dependencies from the running browser.
@@ -121,15 +136,8 @@ class ProxyResolutionServiceProviderPacBrowserTest
     : public ProxyResolutionServiceProviderBaseBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII(switches::kProxyPacUrl, GetPacUrl());
-  }
-
- private:
-  // Encode the PAC script as a data: URL.
-  static std::string GetPacUrl() {
-    std::string b64_encoded;
-    base::Base64Encode(kPacData, &b64_encoded);
-    return "data:application/x-javascript-config;base64," + b64_encoded;
+    command_line->AppendSwitchASCII(switches::kProxyPacUrl,
+                                    GetPacUrl(kPacData));
   }
 };
 
@@ -142,4 +150,59 @@ IN_PROC_BROWSER_TEST_F(ProxyResolutionServiceProviderPacBrowserTest,
             ResolveProxyAndWait("http://www.google.com"));
 }
 
+// PAC script that returns a proxy for all url except for a whitelisted domain.
+const char kPacDataWithWhitelistedDomain[] =
+    "function FindProxyForURL(url, host) {\n"
+    "  if (dnsDomainIs(host, '.direct.com'))\n"
+    "    return 'DIRECT';\n"
+    "  return 'PROXY foo1';\n"
+    "}\n";
+
+// Fixture that launches the browser with --proxy-pac-url="data:..." and
+// System-proxy enabled. With System-proxy enabled and configured, all system
+// service connections going trough an http web proxy will be connected through
+// a local proxy that will perform the proxy authentication and connection
+// setup.
+class ProxyResolutionServiceProviderSystemProxyPolicyTest
+    : public ProxyResolutionServiceProviderBaseBrowserTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(switches::kProxyPacUrl,
+                                    GetPacUrl(kPacDataWithWhitelistedDomain));
+  }
+
+ protected:
+  void SetLocalProxyAddress(const std::string& local_proxy_url) {
+    g_browser_process->platform_part()
+        ->browser_policy_connector_chromeos()
+        ->GetSystemProxyManager()
+        ->SetSystemServicesProxyUrlForTest(local_proxy_url);
+  }
+};
+
+// Tests that the proxy resolver returns the address of the local proxy when
+// set.
+IN_PROC_BROWSER_TEST_F(ProxyResolutionServiceProviderSystemProxyPolicyTest,
+                       ResolveProxyLocalProxySet) {
+  SetLocalProxyAddress(kLocalProxyUrl);
+  EXPECT_EQ("PROXY localhost:3128; PROXY foo1:80",
+            ResolveProxyAndWait("http://www.google.com"));
+}
+
+// Tests that the proxy list semicolon separator is not appended if the local
+// proxy is not set.
+IN_PROC_BROWSER_TEST_F(ProxyResolutionServiceProviderSystemProxyPolicyTest,
+                       ResolveProxyNoSeparator) {
+  SetLocalProxyAddress(/* local_proxy_url= */ std::string());
+  EXPECT_EQ("PROXY foo1:80", ResolveProxyAndWait("http://www.google.com"));
+}
+
+// Tests that the proxy resolver doesn't return the local proxy address for
+// DIRECT connections.
+IN_PROC_BROWSER_TEST_F(ProxyResolutionServiceProviderSystemProxyPolicyTest,
+                       ResolveProxyDirect) {
+  SetLocalProxyAddress(kLocalProxyUrl);
+  EXPECT_EQ("DIRECT", ResolveProxyAndWait("http://www.test.direct.com"));
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc

@@ -7,7 +7,9 @@
 #include <memory>
 
 #include "base/test/task_environment.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/dbus/services/service_provider_test_helper.h"
+#include "chromeos/tpm/stub_install_attributes.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -79,6 +81,10 @@ class MockNetworkContext : public network::TestNetworkContext {
 
   LookupProxyForURLMockResult lookup_proxy_result_;
 
+  chromeos::ScopedStubInstallAttributes test_install_attributes_{
+      chromeos::StubInstallAttributes::CreateCloudManaged("fake-domain",
+                                                          "fake-id")};
+
   DISALLOW_COPY_AND_ASSIGN(MockNetworkContext);
 };
 

chrome/browser/chromeos/policy/system_proxy_manager.cc

@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/values.h"
 #include "chromeos/dbus/system_proxy/system_proxy_client.h"
-#include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
@@ -25,12 +24,23 @@ SystemProxyManager::SystemProxyManager(chromeos::CrosSettings* cros_settings)
           base::BindRepeating(
               &SystemProxyManager::OnSystemProxySettingsPolicyChanged,
               base::Unretained(this)))) {
+  // Connect to a signal that indicates when a worker process is active.
+  chromeos::SystemProxyClient::Get()->ConnectToWorkerActiveSignal(
+      base::BindRepeating(&SystemProxyManager::OnWorkerActive,
+                          weak_factory_.GetWeakPtr()));
+
   // Fire it once so we're sure we get an invocation on startup.
   OnSystemProxySettingsPolicyChanged();
 }
 
 SystemProxyManager::~SystemProxyManager() {}
 
+std::string SystemProxyManager::SystemServicesProxyPacString() const {
+  return system_proxy_enabled_ && !system_services_address_.empty()
+             ? "PROXY " + system_services_address_
+             : std::string();
+}
+
 void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
   chromeos::CrosSettingsProvider::TrustedStatus status =
       cros_settings_->PrepareTrustedValues(base::BindOnce(
@@ -45,11 +55,11 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
   if (!proxy_settings)
     return;
 
-  bool enabled =
+  system_proxy_enabled_ =
       proxy_settings->FindBoolKey(chromeos::kSystemProxySettingsKeyEnabled)
           .value_or(false);
   // System-proxy is inactive by default.
-  if (!enabled) {
+  if (!system_proxy_enabled_) {
     // Send a shut-down command to the daemon. Since System-proxy is started via
     // dbus activation, if the daemon is inactive, this command will start the
     // daemon and tell it to exit.
@@ -57,6 +67,7 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
     // System-proxy is inactive.
     chromeos::SystemProxyClient::Get()->ShutDownDaemon(base::BindOnce(
         &SystemProxyManager::OnDaemonShutDown, weak_factory_.GetWeakPtr()));
+    system_services_address_.clear();
     return;
   }
 
@@ -82,6 +93,12 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
                      weak_factory_.GetWeakPtr()));
 }
 
+void SystemProxyManager::SetSystemServicesProxyUrlForTest(
+    const std::string& local_proxy_url) {
+  system_proxy_enabled_ = true;
+  system_services_address_ = local_proxy_url;
+}
+
 void SystemProxyManager::OnSetSystemTrafficCredentials(
     const system_proxy::SetSystemTrafficCredentialsResponse& response) {
   if (response.has_error_message()) {
@@ -99,4 +116,11 @@ void SystemProxyManager::OnDaemonShutDown(
   }
 }
 
+void SystemProxyManager::OnWorkerActive(
+    const system_proxy::WorkerActiveSignalDetails& details) {
+  if (details.traffic_origin() == system_proxy::TrafficOrigin::SYSTEM) {
+    system_services_address_ = details.local_proxy_url();
+  }
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/system_proxy_manager.h

@@ -9,6 +9,7 @@
 
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
 
 namespace system_proxy {
 class SetSystemTrafficCredentialsResponse;
@@ -32,6 +33,14 @@ class SystemProxyManager {
 
   ~SystemProxyManager();
 
+  // If System-proxy is enabled by policy, it returns the URL of the local proxy
+  // instance that authenticates system services, in PAC format, e.g.
+  //     PROXY localhost:3128
+  // otherwise it returns an empty string.
+  std::string SystemServicesProxyPacString() const;
+
+  void SetSystemServicesProxyUrlForTest(const std::string& local_proxy_url);
+
  private:
   void OnSetSystemTrafficCredentials(
       const system_proxy::SetSystemTrafficCredentialsResponse& response);
@@ -42,10 +51,18 @@ class SystemProxyManager {
   // necessary, to configure the web proxy credentials for system services.
   void OnSystemProxySettingsPolicyChanged();
 
+  // This function is called when the |WorkerActive| dbus signal is received.
+  void OnWorkerActive(const system_proxy::WorkerActiveSignalDetails& details);
+
   chromeos::CrosSettings* cros_settings_;
   std::unique_ptr<chromeos::CrosSettings::ObserverSubscription>
       system_proxy_subscription_;
 
+  bool system_proxy_enabled_ = false;
+  // The authority URI in the format host:port of the local proxy worker for
+  // system services.
+  std::string system_services_address_;
+
   base::WeakPtrFactory<SystemProxyManager> weak_factory_{this};
 };
 

chromeos/dbus/system_proxy/fake_system_proxy_client.cc

@@ -30,6 +30,13 @@ void FakeSystemProxyClient::ShutDownDaemon(ShutDownDaemonCallback callback) {
       FROM_HERE, base::BindOnce(std::move(callback), response));
 }
 
+void FakeSystemProxyClient::ConnectToWorkerActiveSignal(
+    WorkerActiveCallback callback) {
+  system_proxy::WorkerActiveSignalDetails details;
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), details));
+}
+
 SystemProxyClient::TestInterface* FakeSystemProxyClient::GetTestInterface() {
   return this;
 }

chromeos/dbus/system_proxy/fake_system_proxy_client.h

@@ -25,6 +25,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSystemProxyClient
       const system_proxy::SetSystemTrafficCredentialsRequest& request,
       SetSystemTrafficCredentialsCallback callback) override;
   void ShutDownDaemon(ShutDownDaemonCallback callback) override;
+  void ConnectToWorkerActiveSignal(WorkerActiveCallback callback) override;
+
   SystemProxyClient::TestInterface* GetTestInterface() override;
 
   // SystemProxyClient::TestInterface implementation.

chromeos/dbus/system_proxy/system_proxy_client.cc

@@ -40,6 +40,15 @@ const char* DeserializeProto(dbus::Response* response,
   return nullptr;
 }
 
+void OnSignalConnected(const std::string& interface_name,
+                       const std::string& signal_name,
+                       bool success) {
+  DCHECK_EQ(interface_name, system_proxy::kSystemProxyInterface);
+  if (!success) {
+    LOG(ERROR) << "Failed to connect to the System-proxy d-bus interface.";
+  }
+}
+
 // "Real" implementation of SystemProxyClient talking to the SystemProxy daemon
 // on the Chrome OS side.
 class SystemProxyClientImpl : public SystemProxyClient {
@@ -61,6 +70,16 @@ class SystemProxyClientImpl : public SystemProxyClient {
     CallProtoMethod(system_proxy::kShutDownMethod, std::move(callback));
   }
 
+  void ConnectToWorkerActiveSignal(WorkerActiveCallback callback) override {
+    DCHECK(callback);
+    DCHECK(!worker_active_callback_);
+    worker_active_callback_ = callback;
+
+    proxy_->WaitForServiceToBeAvailable(
+        base::BindOnce(&SystemProxyClientImpl::OnSystemProxyServiceAvailable,
+                       weak_factory_.GetWeakPtr()));
+  }
+
   void Init(dbus::Bus* bus) {
     proxy_ = bus->GetObjectProxy(
         system_proxy::kSystemProxyServiceName,
@@ -124,6 +143,37 @@ class SystemProxyClientImpl : public SystemProxyClient {
     std::move(callback).Run(response_proto);
   }
 
+  void OnWorkerActive(dbus::Signal* signal) {
+    DCHECK_EQ(signal->GetInterface(), system_proxy::kSystemProxyInterface);
+    DCHECK_EQ(signal->GetMember(), system_proxy::kWorkerActiveSignal);
+
+    dbus::MessageReader signal_reader(signal);
+    system_proxy::WorkerActiveSignalDetails details;
+    if (!signal_reader.PopArrayOfBytesAsProto(&details)) {
+      LOG(ERROR) << "Failed to read connection details for active proxy "
+                    "worker process.";
+      return;
+    }
+    DCHECK(worker_active_callback_);
+    worker_active_callback_.Run(details);
+  }
+
+  void OnSystemProxyServiceAvailable(bool is_available) {
+    if (!is_available) {
+      LOG(ERROR) << "System-proxy service not available";
+      return;
+    }
+
+    proxy_->ConnectToSignal(
+        system_proxy::kSystemProxyInterface, system_proxy::kWorkerActiveSignal,
+        base::BindRepeating(&SystemProxyClientImpl::OnWorkerActive,
+                            weak_factory_.GetWeakPtr()),
+        base::BindOnce(&OnSignalConnected));
+  }
+
+  // Signal callback.
+  WorkerActiveCallback worker_active_callback_;
+
   // D-Bus proxy for the SystemProxy daemon, not owned.
   dbus::ObjectProxy* proxy_ = nullptr;
 

chromeos/dbus/system_proxy/system_proxy_client.h

@@ -25,6 +25,8 @@ class COMPONENT_EXPORT(SYSTEM_PROXY) SystemProxyClient {
       const system_proxy::SetSystemTrafficCredentialsResponse& response)>;
   using ShutDownDaemonCallback =
       base::OnceCallback<void(const system_proxy::ShutDownResponse& response)>;
+  using WorkerActiveCallback = base::RepeatingCallback<void(
+      const system_proxy::WorkerActiveSignalDetails& details)>;
 
   // Interface with testing functionality. Accessed through GetTestInterface(),
   // only implemented in the fake implementation.
@@ -68,6 +70,10 @@ class COMPONENT_EXPORT(SYSTEM_PROXY) SystemProxyClient {
   // Returns an interface for testing (fake only), or returns nullptr.
   virtual TestInterface* GetTestInterface() = 0;
 
+  // Waits for the System-proxy d-bus service to be available and then connects
+  // to the WorkerActvie signal.
+  virtual void ConnectToWorkerActiveSignal(WorkerActiveCallback callback) = 0;
+
  protected:
   // Initialize/Shutdown should be used instead.
   SystemProxyClient();