system-proxy: Listen for AuthRequired signal

When the System-proxy service in CrOS sends an AuthenticationRequired
signal for a specific protection space (proxy url, scheme, realm), the
SystemProxyManager inside the Browser looks for the credentials added by
the user.

If found, it will send the credentials and protection space to
System-proxy via dbus. If no credentials are found or no user has
signed in yet, it will send the protection space with empty credentials
to signal that the request was processed.

BUG=1042642
TEST=unittest, tested on DUT

Change-Id: Id427d962d22171a41586f51d2936a73bfe8694e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246163Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Commit-Queue: Andreea-Elena Costinas <acostinas@google.com>
Cr-Commit-Position: refs/heads/master@{#786355}

chrome/browser/chromeos/policy/system_proxy_manager.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/chromeos/policy/system_proxy_manager.h"
 
 #include "base/bind.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -18,10 +20,11 @@
 #include "components/prefs/pref_service.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
+#include "net/http/http_auth_scheme.h"
 
 namespace {
 const char kSystemProxyService[] = "system-proxy-service";
-}
+}  // namespace
 
 namespace policy {
 
@@ -33,10 +36,14 @@ 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(
+  // Connect to System-proxy signals.
+  chromeos::SystemProxyClient::Get()->SetWorkerActiveSignalCallback(
       base::BindRepeating(&SystemProxyManager::OnWorkerActive,
                           weak_factory_.GetWeakPtr()));
+  chromeos::SystemProxyClient::Get()->SetAuthenticationRequiredSignalCallback(
+      base::BindRepeating(&SystemProxyManager::OnAuthenticationRequired,
+                          weak_factory_.GetWeakPtr()));
+  chromeos::SystemProxyClient::Get()->ConnectToWorkerSignals();
   local_state_ = local_state;
 
   // Listen to pref changes.
@@ -61,6 +68,7 @@ std::string SystemProxyManager::SystemServicesProxyPacString() const {
 
 void SystemProxyManager::StartObservingPrimaryProfilePrefs(Profile* profile) {
   primary_profile_ = profile;
+
   // Listen to pref changes.
   profile_pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
   profile_pref_change_registrar_->Init(primary_profile_->GetPrefs());
@@ -108,6 +116,8 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
     return;
   }
 
+  system_proxy::SetAuthenticationDetailsRequest request;
+  system_proxy::Credentials credentials;
   const std::string* username = proxy_settings->FindStringKey(
       chromeos::kSystemProxySettingsKeySystemServicesUsername);
 
@@ -115,18 +125,15 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
       chromeos::kSystemProxySettingsKeySystemServicesPassword);
 
   if (!username || username->empty() || !password || password->empty()) {
-    NET_LOG(ERROR) << "Proxy credentials for system traffic not set: "
+    NET_LOG(DEBUG) << "Proxy credentials for system traffic not set: "
                    << kSystemProxyService;
-    return;
+  } else {
+    credentials.set_username(*username);
+    credentials.set_password(*password);
+    *request.mutable_credentials() = credentials;
   }
 
-  system_proxy::Credentials credentials;
-  credentials.set_username(*username);
-  credentials.set_password(*password);
-
-  system_proxy::SetAuthenticationDetailsRequest request;
   request.set_traffic_type(system_proxy::TrafficOrigin::SYSTEM);
-  *request.mutable_credentials() = credentials;
 
   chromeos::SystemProxyClient::Get()->SetAuthenticationDetails(
       request, base::BindOnce(&SystemProxyManager::OnSetAuthenticationDetails,
@@ -194,4 +201,45 @@ void SystemProxyManager::OnWorkerActive(
   }
 }
 
+void SystemProxyManager::OnAuthenticationRequired(
+    const system_proxy::AuthenticationRequiredDetails& details) {
+  system_proxy::ProtectionSpace protection_space =
+      details.proxy_protection_space();
+
+  // TODO(acostinas, crbug.com/1098216): Get credentials from the network
+  // service.
+  LookupProxyAuthCredentialsCallback(protection_space,
+                                     /* credentials = */ base::nullopt);
+}
+
+void SystemProxyManager::LookupProxyAuthCredentialsCallback(
+    const system_proxy::ProtectionSpace& protection_space,
+    const base::Optional<net::AuthCredentials>& credentials) {
+  // System-proxy is started via d-bus activation, meaning the first d-bus call
+  // will start the daemon. Check that System-proxy was not disabled by policy
+  // while looking for credentials so we don't accidentally restart it.
+  if (!system_proxy_enabled_) {
+    return;
+  }
+  std::string username;
+  std::string password;
+  if (credentials) {
+    username = base::UTF16ToUTF8(credentials->username());
+    password = base::UTF16ToUTF8(credentials->password());
+  }
+
+  system_proxy::Credentials user_credentials;
+  user_credentials.set_username(username);
+  user_credentials.set_password(password);
+
+  system_proxy::SetAuthenticationDetailsRequest request;
+  request.set_traffic_type(system_proxy::TrafficOrigin::SYSTEM);
+  *request.mutable_credentials() = user_credentials;
+  *request.mutable_protection_space() = protection_space;
+
+  chromeos::SystemProxyClient::Get()->SetAuthenticationDetails(
+      request, base::BindOnce(&SystemProxyManager::OnSetAuthenticationDetails,
+                              weak_factory_.GetWeakPtr()));
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/system_proxy_manager.h

@@ -6,10 +6,14 @@
 #define CHROME_BROWSER_CHROMEOS_POLICY_SYSTEM_PROXY_MANAGER_H_
 
 #include <memory>
+#include <string>
 
+#include "base/callback_forward.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
+#include "net/base/auth.h"
 
 namespace system_proxy {
 class SetAuthenticationDetailsResponse;
@@ -66,6 +70,18 @@ class SystemProxyManager {
   // This function is called when the |WorkerActive| dbus signal is received.
   void OnWorkerActive(const system_proxy::WorkerActiveSignalDetails& details);
 
+  // This function is called when the |AuthenticationRequired| dbus signal is
+  // received.
+  void OnAuthenticationRequired(
+      const system_proxy::AuthenticationRequiredDetails& details);
+
+  // Forwards the user credentials to System-proxy. |credentials| may be empty
+  // indicating the credentials for the specified |protection_space| are not
+  // available.
+  void LookupProxyAuthCredentialsCallback(
+      const system_proxy::ProtectionSpace& protection_space,
+      const base::Optional<net::AuthCredentials>& credentials);
+
   chromeos::CrosSettings* cros_settings_;
   std::unique_ptr<chromeos::CrosSettings::ObserverSubscription>
       system_proxy_subscription_;

chrome/browser/chromeos/policy/system_proxy_manager_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/policy/system_proxy_manager.h"
 
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
 #include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
@@ -17,12 +18,20 @@
 #include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/test/browser_task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::Invoke;
+using testing::WithArg;
+
 namespace {
 constexpr char kSystemServicesUsername[] = "test_username";
 constexpr char kSystemServicesPassword[] = "test_password";
 constexpr char kKerberosActivePrincipalName[] = "kerberos_princ_name";
+constexpr char kProxyAuthUrl[] = "http://example.com:3128";
+constexpr char kRealm[] = "My proxy";
+constexpr char kScheme[] = "BaSiC";
 }  // namespace
 
 namespace policy {
@@ -34,7 +43,6 @@ class SystemProxyManagerTest : public testing::Test {
   // testing::Test
   void SetUp() override {
     testing::Test::SetUp();
-
     profile_ = std::make_unique<TestingProfile>();
     chromeos::SystemProxyClient::InitializeFake();
   }
@@ -78,12 +86,12 @@ TEST_F(SystemProxyManagerTest, SetAuthenticationDetails) {
             "" /* system_services_password */);
   task_environment_.RunUntilIdle();
   // Don't send empty credentials.
-  EXPECT_EQ(0, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+  EXPECT_EQ(1, client_test_interface()->GetSetAuthenticationDetailsCallCount());
 
   SetPolicy(true /* system_proxy_enabled */, kSystemServicesUsername,
             kSystemServicesPassword);
   task_environment_.RunUntilIdle();
-  EXPECT_EQ(1, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+  EXPECT_EQ(2, client_test_interface()->GetSetAuthenticationDetailsCallCount());
 
   system_proxy::SetAuthenticationDetailsRequest request =
       client_test_interface()->GetLastAuthenticationDetailsRequest();
@@ -118,11 +126,11 @@ TEST_F(SystemProxyManagerTest, KerberosConfig) {
             "" /* system_services_password */);
   task_environment_.RunUntilIdle();
   local_state_.Get()->SetBoolean(prefs::kKerberosEnabled, true);
-  EXPECT_EQ(1, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+  EXPECT_EQ(2, client_test_interface()->GetSetAuthenticationDetailsCallCount());
 
   // Listen for pref changes for the primary profile.
   system_proxy_manager.StartObservingPrimaryProfilePrefs(profile_.get());
-  EXPECT_EQ(2, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+  EXPECT_EQ(3, client_test_interface()->GetSetAuthenticationDetailsCallCount());
   system_proxy::SetAuthenticationDetailsRequest request =
       client_test_interface()->GetLastAuthenticationDetailsRequest();
   EXPECT_FALSE(request.has_credentials());
@@ -131,7 +139,7 @@ TEST_F(SystemProxyManagerTest, KerberosConfig) {
   // Set an active principal name.
   profile_->GetPrefs()->SetString(prefs::kKerberosActivePrincipalName,
                                   kKerberosActivePrincipalName);
-  EXPECT_EQ(3, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+  EXPECT_EQ(4, client_test_interface()->GetSetAuthenticationDetailsCallCount());
   request = client_test_interface()->GetLastAuthenticationDetailsRequest();
   EXPECT_EQ(kKerberosActivePrincipalName, request.active_principal_name());
 
@@ -149,4 +157,37 @@ TEST_F(SystemProxyManagerTest, KerberosConfig) {
   system_proxy_manager.StopObservingPrimaryProfilePrefs();
 }
 
+// Tests that when no user is signed in, credential requests are resolved to a
+// d-bus call which sends back to System-proxy empty credentials for the
+// specified protection space.
+TEST_F(SystemProxyManagerTest, UserCredentialsRequiredNoUser) {
+  SystemProxyManager system_proxy_manager(chromeos::CrosSettings::Get(),
+                                          local_state_.Get());
+  SetPolicy(true /* system_proxy_enabled */, "" /* system_services_username */,
+            "" /* system_services_password */);
+
+  system_proxy::ProtectionSpace protection_space;
+  protection_space.set_origin(kProxyAuthUrl);
+  protection_space.set_scheme(kScheme);
+  protection_space.set_realm(kRealm);
+
+  system_proxy::AuthenticationRequiredDetails details;
+  *details.mutable_proxy_protection_space() = protection_space;
+
+  client_test_interface()->SendAuthenticationRequiredSignal(details);
+  task_environment_.RunUntilIdle();
+
+  EXPECT_EQ(2, client_test_interface()->GetSetAuthenticationDetailsCallCount());
+
+  system_proxy::SetAuthenticationDetailsRequest request =
+      client_test_interface()->GetLastAuthenticationDetailsRequest();
+
+  ASSERT_TRUE(request.has_protection_space());
+  ASSERT_EQ(protection_space.SerializeAsString(),
+            request.protection_space().SerializeAsString());
+
+  ASSERT_TRUE(request.has_credentials());
+  EXPECT_EQ("", request.credentials().username());
+  EXPECT_EQ("", request.credentials().password());
+}
 }  // namespace policy

chromeos/dbus/system_proxy/fake_system_proxy_client.cc

@@ -5,7 +5,7 @@
 #include "chromeos/dbus/system_proxy/fake_system_proxy_client.h"
 
 #include "base/bind.h"
-#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
 
 namespace chromeos {
@@ -20,22 +20,28 @@ void FakeSystemProxyClient::SetAuthenticationDetails(
   ++set_credentials_call_count_;
   last_set_auth_details_request_ = request;
   system_proxy::SetAuthenticationDetailsResponse response;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), response));
 }
 
 void FakeSystemProxyClient::ShutDownDaemon(ShutDownDaemonCallback callback) {
   ++shut_down_call_count_;
   system_proxy::ShutDownResponse response;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), response));
 }
 
-void FakeSystemProxyClient::ConnectToWorkerActiveSignal(
+void FakeSystemProxyClient::SetWorkerActiveSignalCallback(
     WorkerActiveCallback callback) {
-  system_proxy::WorkerActiveSignalDetails details;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), details));
+  worker_active_callback_ = callback;
+}
+void FakeSystemProxyClient::SetAuthenticationRequiredSignalCallback(
+    AuthenticationRequiredCallback callback) {
+  auth_required_callback_ = callback;
+}
+
+void FakeSystemProxyClient::ConnectToWorkerSignals() {
+  connect_to_worker_signals_called_ = true;
 }
 
 SystemProxyClient::TestInterface* FakeSystemProxyClient::GetTestInterface() {
@@ -55,4 +61,13 @@ FakeSystemProxyClient::GetLastAuthenticationDetailsRequest() const {
   return last_set_auth_details_request_;
 }
 
+void FakeSystemProxyClient::SendAuthenticationRequiredSignal(
+    const system_proxy::AuthenticationRequiredDetails& details) {
+  if (!connect_to_worker_signals_called_) {
+    return;
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(auth_required_callback_, details));
+}
+
 }  // namespace chromeos

chromeos/dbus/system_proxy/fake_system_proxy_client.h

@@ -25,7 +25,10 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSystemProxyClient
       const system_proxy::SetAuthenticationDetailsRequest& request,
       SetAuthenticationDetailsCallback callback) override;
   void ShutDownDaemon(ShutDownDaemonCallback callback) override;
-  void ConnectToWorkerActiveSignal(WorkerActiveCallback callback) override;
+  void SetWorkerActiveSignalCallback(WorkerActiveCallback callback) override;
+  void SetAuthenticationRequiredSignalCallback(
+      AuthenticationRequiredCallback callback) override;
+  void ConnectToWorkerSignals() override;
 
   SystemProxyClient::TestInterface* GetTestInterface() override;
 
@@ -34,11 +37,17 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSystemProxyClient
   int GetShutDownCallCount() const override;
   system_proxy::SetAuthenticationDetailsRequest
   GetLastAuthenticationDetailsRequest() const override;
+  void SendAuthenticationRequiredSignal(
+      const system_proxy::AuthenticationRequiredDetails& details) override;
 
  private:
   system_proxy::SetAuthenticationDetailsRequest last_set_auth_details_request_;
   int set_credentials_call_count_ = 0;
   int shut_down_call_count_ = 0;
+  bool connect_to_worker_signals_called_ = false;
+  // Signal callbacks.
+  SystemProxyClient::WorkerActiveCallback worker_active_callback_;
+  SystemProxyClient::AuthenticationRequiredCallback auth_required_callback_;
 };
 
 }  // namespace chromeos

chromeos/dbus/system_proxy/system_proxy_client.cc

@@ -71,11 +71,20 @@ class SystemProxyClientImpl : public SystemProxyClient {
     CallProtoMethod(system_proxy::kShutDownMethod, std::move(callback));
   }
 
-  void ConnectToWorkerActiveSignal(WorkerActiveCallback callback) override {
+  void SetWorkerActiveSignalCallback(WorkerActiveCallback callback) override {
     DCHECK(callback);
     DCHECK(!worker_active_callback_);
     worker_active_callback_ = callback;
+  }
+
+  void SetAuthenticationRequiredSignalCallback(
+      AuthenticationRequiredCallback callback) override {
+    DCHECK(callback);
+    DCHECK(!auth_required_callback_);
+    auth_required_callback_ = callback;
+  }
 
+  void ConnectToWorkerSignals() override {
     proxy_->WaitForServiceToBeAvailable(
         base::BindOnce(&SystemProxyClientImpl::OnSystemProxyServiceAvailable,
                        weak_factory_.GetWeakPtr()));
@@ -155,10 +164,32 @@ class SystemProxyClientImpl : public SystemProxyClient {
                     "worker process.";
       return;
     }
-    DCHECK(worker_active_callback_);
+    if (!worker_active_callback_) {
+      LOG(WARNING) << "WorkerActive signal is ignored.";
+      return;
+    }
     worker_active_callback_.Run(details);
   }
 
+  void OnAuthenticationRequired(dbus::Signal* signal) {
+    DCHECK_EQ(signal->GetInterface(), system_proxy::kSystemProxyInterface);
+    DCHECK_EQ(signal->GetMember(), system_proxy::kAuthenticationRequiredSignal);
+
+    dbus::MessageReader signal_reader(signal);
+    system_proxy::AuthenticationRequiredDetails details;
+    if (!signal_reader.PopArrayOfBytesAsProto(&details)) {
+      LOG(ERROR)
+          << "Failed to read required authentication details from signal.";
+      return;
+    }
+    if (!auth_required_callback_) {
+      LOG(WARNING) << "AuthenticationRequired signal is ignored.";
+      return;
+    }
+
+    auth_required_callback_.Run(details);
+  }
+
   void OnSystemProxyServiceAvailable(bool is_available) {
     if (!is_available) {
       LOG(ERROR) << "System-proxy service not available";
@@ -170,10 +201,18 @@ class SystemProxyClientImpl : public SystemProxyClient {
         base::BindRepeating(&SystemProxyClientImpl::OnWorkerActive,
                             weak_factory_.GetWeakPtr()),
         base::BindOnce(&OnSignalConnected));
+
+    proxy_->ConnectToSignal(
+        system_proxy::kSystemProxyInterface,
+        system_proxy::kAuthenticationRequiredSignal,
+        base::BindRepeating(&SystemProxyClientImpl::OnAuthenticationRequired,
+                            weak_factory_.GetWeakPtr()),
+        base::BindOnce(&OnSignalConnected));
   }
 
-  // Signal callback.
+  // Signal callbacks.
   WorkerActiveCallback worker_active_callback_;
+  AuthenticationRequiredCallback auth_required_callback_;
 
   // D-Bus proxy for the SystemProxy daemon, not owned.
   dbus::ObjectProxy* proxy_ = nullptr;

chromeos/dbus/system_proxy/system_proxy_client.h

@@ -27,6 +27,8 @@ class COMPONENT_EXPORT(SYSTEM_PROXY) SystemProxyClient {
       base::OnceCallback<void(const system_proxy::ShutDownResponse& response)>;
   using WorkerActiveCallback = base::RepeatingCallback<void(
       const system_proxy::WorkerActiveSignalDetails& details)>;
+  using AuthenticationRequiredCallback = base::RepeatingCallback<void(
+      const system_proxy::AuthenticationRequiredDetails& details)>;
 
   // Interface with testing functionality. Accessed through GetTestInterface(),
   // only implemented in the fake implementation.
@@ -40,6 +42,12 @@ class COMPONENT_EXPORT(SYSTEM_PROXY) SystemProxyClient {
     // to set authentication details.
     virtual system_proxy::SetAuthenticationDetailsRequest
     GetLastAuthenticationDetailsRequest() const = 0;
+    // Simulates the |AuthenticationRequired| signal by calling the callback set
+    // by |SetAuthenticationRequiredSignalCallback|. The callback is called only
+    // if |FakeSystemProxyClient| was set up to listen for signals by calling
+    // |ConnectToWorkerSignals|.
+    virtual void SendAuthenticationRequiredSignal(
+        const system_proxy::AuthenticationRequiredDetails& details) = 0;
 
    protected:
     virtual ~TestInterface() {}
@@ -74,9 +82,20 @@ class COMPONENT_EXPORT(SYSTEM_PROXY) SystemProxyClient {
   // Returns an interface for testing (fake only), or returns nullptr.
   virtual TestInterface* GetTestInterface() = 0;
 
+  // Sets the callback to be called when System-proxy emits the
+  // |WorkerActiveSignal| signal.
+  virtual void SetWorkerActiveSignalCallback(WorkerActiveCallback callback) = 0;
+
+  // Sets the callback to be called when System-proxy emits the
+  // |AuthenticationRequired| signal.
+  virtual void SetAuthenticationRequiredSignalCallback(
+      AuthenticationRequiredCallback callback) = 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;
+  // to the signals for which a callback has been set with
+  // |SetWorkerActiveSignalCallback| and
+  // |SetAuthenticationRequiredSignalCallback|.
+  virtual void ConnectToWorkerSignals() = 0;
 
  protected:
   // Initialize/Shutdown should be used instead.