bruschetta: Enable security key forwarding on lacros

Currently messages for gnubbyd from the VM flow like this: VM -> cicerone -> ash::VmSKForwardingServiceProvider -> ash::guest_os::VmSKForwardingNativeMessageHost -> forwarding extension -> gnubbyd With this CL the flow is now: VM -> cicerone -> ash::VmSKForwardingServiceProvider -> guest_os::GuestOsSkForwarder -> crosapi mojo link to lacros -> guest_os::VmSkForwardingService -> guest_os::VmSKForwardingNativeMessageHost -> forwarding extension -> gnubbyd Additionally, some plumbing is required to establish the mojo connection. This flows in the opposite direction: guest_os::VmSkForwardingService -> crosapi::GuestOsSkForwarderFactoryAsh -> guest_os::GuestOsSkForwarder To support both lacros-only and lacros-disabled, guest_os::GuestOsSkForwarder will pass the message either to the old code path or the new, depending on the lacros enabled state. This unfortunately means the code in VmSKForwardingNativeMessageHost is duplicated temporarily, but the copy in ash can be removed once running without lacros is no longer supported. Bug: b:295083119 Change-Id: I3a0137af150e9fe787f4bce17b0a4a622bf6371e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4944758 Reviewed-by: Stefan Kuhne <skuhne@chromium.org> Reviewed-by: Nic Hollingum <hollingum@google.com> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Reviewed-by: Emil Mikulic <easy@google.com> Commit-Queue: Fergus Dall <sidereal@google.com> Cr-Commit-Position: refs/heads/main@{#1212501}
chromium · Oct 20, 2023 · 0277cfa · 0277cfa
1 parent 79fcc6c
commit 0277cfa
Show file tree

Hide file tree

Showing 29 changed files with 721 additions and 9 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -5584,6 +5584,10 @@ static_library("browser") {
       "lacros/fullscreen_controller_client_lacros.h",
       "lacros/geolocation/system_geolocation_source_lacros.cc",
       "lacros/geolocation/system_geolocation_source_lacros.h",
+      "lacros/guest_os/vm_sk_forwarding_native_message_host.cc",
+      "lacros/guest_os/vm_sk_forwarding_native_message_host.h",
+      "lacros/guest_os/vm_sk_forwarding_service.cc",
+      "lacros/guest_os/vm_sk_forwarding_service.h",
       "lacros/identity_manager_lacros.cc",
       "lacros/identity_manager_lacros.h",
       "lacros/lacros_apps_publisher.cc",

diff --git a/chrome/browser/ash/BUILD.gn b/chrome/browser/ash/BUILD.gn
@@ -1424,6 +1424,8 @@ source_set("ash") {
     "guest_os/public/guest_os_service.h",
     "guest_os/public/guest_os_service_factory.cc",
     "guest_os/public/guest_os_service_factory.h",
+    "guest_os/public/guest_os_sk_forwarder.cc",
+    "guest_os/public/guest_os_sk_forwarder.h",
     "guest_os/public/guest_os_terminal_provider.cc",
     "guest_os/public/guest_os_terminal_provider.h",
     "guest_os/public/guest_os_terminal_provider_registry.cc",

diff --git a/chrome/browser/ash/crosapi/BUILD.gn b/chrome/browser/ash/crosapi/BUILD.gn
@@ -142,6 +142,8 @@ source_set("crosapi") {
     "fullscreen_controller_ash.h",
     "geolocation_service_ash.cc",
     "geolocation_service_ash.h",
+    "guest_os_sk_forwarder_factory_ash.cc",
+    "guest_os_sk_forwarder_factory_ash.h",
     "hosted_app_util.cc",
     "hosted_app_util.h",
     "identity_manager_ash.cc",

diff --git a/chrome/browser/ash/crosapi/crosapi_ash.cc b/chrome/browser/ash/crosapi/crosapi_ash.cc
@@ -61,6 +61,7 @@
 #include "chrome/browser/ash/crosapi/force_installed_tracker_ash.h"
 #include "chrome/browser/ash/crosapi/fullscreen_controller_ash.h"
 #include "chrome/browser/ash/crosapi/geolocation_service_ash.h"
+#include "chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.h"
 #include "chrome/browser/ash/crosapi/identity_manager_ash.h"
 #include "chrome/browser/ash/crosapi/idle_service_ash.h"
 #include "chrome/browser/ash/crosapi/image_writer_ash.h"
@@ -201,6 +202,8 @@ CrosapiAsh::CrosapiAsh(CrosapiDependencyRegistry* registry)
       browser_service_host_ash_(std::make_unique<BrowserServiceHostAsh>()),
       browser_version_service_ash_(std::make_unique<BrowserVersionServiceAsh>(
           g_browser_process->component_updater())),
+      guest_os_sk_forwarder_factory_ash_(
+          std::make_unique<GuestOsSkForwarderFactoryAsh>()),
       cert_database_ash_(std::make_unique<CertDatabaseAsh>()),
       cert_provisioning_ash_(std::make_unique<CertProvisioningAsh>()),
       chrome_app_kiosk_service_ash_(
@@ -1002,6 +1005,11 @@ void CrosapiAsh::BindWebPageInfoFactory(
   web_page_info_factory_ash_->BindReceiver(std::move(receiver));
 }
 
+void CrosapiAsh::BindGuestOsSkForwarderFactory(
+    mojo::PendingReceiver<mojom::GuestOsSkForwarderFactory> receiver) {
+  guest_os_sk_forwarder_factory_ash_->BindReceiver(std::move(receiver));
+}
+
 void CrosapiAsh::BindWebAppPublisher(
     mojo::PendingReceiver<mojom::AppPublisher> receiver) {
   Profile* profile = ProfileManager::GetPrimaryUserProfile();

diff --git a/chrome/browser/ash/crosapi/crosapi_ash.h b/chrome/browser/ash/crosapi/crosapi_ash.h
@@ -46,6 +46,7 @@ class AuthenticationAsh;
 class AutomationAsh;
 class BrowserServiceHostAsh;
 class BrowserVersionServiceAsh;
+class GuestOsSkForwarderFactoryAsh;
 class CertDatabaseAsh;
 class CertProvisioningAsh;
 class ChromeAppKioskServiceAsh;
@@ -390,6 +391,9 @@ class CrosapiAsh : public mojom::Crosapi {
       mojo::PendingReceiver<mojom::WebKioskService> receiver) override;
   void BindWebPageInfoFactory(
       mojo::PendingReceiver<mojom::WebPageInfoFactory> receiver) override;
+  void BindGuestOsSkForwarderFactory(
+      mojo::PendingReceiver<mojom::GuestOsSkForwarderFactory> receiver)
+      override;
   void OnBrowserStartup(mojom::BrowserInfoPtr browser_info) override;
   void REMOVED_29(
       mojo::PendingReceiver<mojom::SystemDisplayDeprecated> receiver) override;
@@ -578,6 +582,8 @@ class CrosapiAsh : public mojom::Crosapi {
   std::unique_ptr<AutomationAsh> automation_ash_;
   std::unique_ptr<BrowserServiceHostAsh> browser_service_host_ash_;
   std::unique_ptr<BrowserVersionServiceAsh> browser_version_service_ash_;
+  std::unique_ptr<GuestOsSkForwarderFactoryAsh>
+      guest_os_sk_forwarder_factory_ash_;
   std::unique_ptr<CertDatabaseAsh> cert_database_ash_;
   std::unique_ptr<CertProvisioningAsh> cert_provisioning_ash_;
   std::unique_ptr<ChromeAppKioskServiceAsh> chrome_app_kiosk_service_ash_;

diff --git a/chrome/browser/ash/crosapi/crosapi_util.cc b/chrome/browser/ash/crosapi/crosapi_util.cc
@@ -90,6 +90,7 @@
 #include "chromeos/crosapi/mojom/force_installed_tracker.mojom.h"
 #include "chromeos/crosapi/mojom/fullscreen_controller.mojom.h"
 #include "chromeos/crosapi/mojom/geolocation.mojom.h"
+#include "chromeos/crosapi/mojom/guest_os_sk_forwarder.mojom.h"
 #include "chromeos/crosapi/mojom/holding_space_service.mojom.h"
 #include "chromeos/crosapi/mojom/identity_manager.mojom.h"
 #include "chromeos/crosapi/mojom/image_writer.mojom.h"
@@ -296,7 +297,7 @@ constexpr InterfaceVersionEntry MakeInterfaceVersionEntry() {
   return {T::Uuid_, T::Version_};
 }
 
-static_assert(crosapi::mojom::Crosapi::Version_ == 120,
+static_assert(crosapi::mojom::Crosapi::Version_ == 121,
               "If you add a new crosapi, please add it to "
               "kInterfaceVersionEntries below.");
 
@@ -429,6 +430,7 @@ constexpr InterfaceVersionEntry kInterfaceVersionEntries[] = {
         crosapi::mojom::EmbeddedAccessibilityHelperClient>(),
     MakeInterfaceVersionEntry<
         crosapi::mojom::EmbeddedAccessibilityHelperClientFactory>(),
+    MakeInterfaceVersionEntry<crosapi::mojom::GuestOsSkForwarderFactory>(),
 };
 
 constexpr bool HasDuplicatedUuid() {

diff --git a/chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.cc b/chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.cc
@@ -0,0 +1,34 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.h"
+
+#include "chrome/browser/ash/guest_os/public/guest_os_service.h"
+#include "chrome/browser/profiles/profile_manager.h"
+
+namespace crosapi {
+
+GuestOsSkForwarderFactoryAsh::GuestOsSkForwarderFactoryAsh()
+    : receiver_(this) {}
+
+GuestOsSkForwarderFactoryAsh::~GuestOsSkForwarderFactoryAsh() = default;
+
+void GuestOsSkForwarderFactoryAsh::BindReceiver(
+    mojo::PendingReceiver<mojom::GuestOsSkForwarderFactory> receiver) {
+  receiver_.reset();
+  receiver_.Bind(std::move(receiver));
+}
+
+void GuestOsSkForwarderFactoryAsh::BindGuestOsSkForwarder(
+    mojo::PendingRemote<mojom::GuestOsSkForwarder> remote) {
+  Profile* profile = ProfileManager::GetPrimaryUserProfile();
+
+  auto* service = guest_os::GuestOsService::GetForProfile(profile);
+
+  if (service) {
+    service->SkForwarder()->BindCrosapiRemote(std::move(remote));
+  }
+}
+
+}  // namespace crosapi
diff --git a/chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.h b/chrome/browser/ash/crosapi/guest_os_sk_forwarder_factory_ash.h
@@ -0,0 +1,32 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_ASH_CROSAPI_GUEST_OS_SK_FORWARDER_FACTORY_ASH_H_
+#define CHROME_BROWSER_ASH_CROSAPI_GUEST_OS_SK_FORWARDER_FACTORY_ASH_H_
+
+#include "chromeos/crosapi/mojom/guest_os_sk_forwarder.mojom.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+
+namespace crosapi {
+class GuestOsSkForwarderFactoryAsh : public mojom::GuestOsSkForwarderFactory {
+ public:
+  GuestOsSkForwarderFactoryAsh();
+  GuestOsSkForwarderFactoryAsh(const GuestOsSkForwarderFactoryAsh&) = delete;
+  GuestOsSkForwarderFactoryAsh& operator=(const GuestOsSkForwarderFactoryAsh&) =
+      delete;
+  ~GuestOsSkForwarderFactoryAsh() override;
+
+  void BindReceiver(
+      mojo::PendingReceiver<mojom::GuestOsSkForwarderFactory> receiver);
+
+  void BindGuestOsSkForwarder(
+      mojo::PendingRemote<mojom::GuestOsSkForwarder> remote) override;
+
+ private:
+  mojo::Receiver<mojom::GuestOsSkForwarderFactory> receiver_;
+};
+}  // namespace crosapi
+
+#endif
diff --git a/chrome/browser/ash/dbus/vm/vm_sk_forwarding_service_provider.cc b/chrome/browser/ash/dbus/vm/vm_sk_forwarding_service_provider.cc
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "chrome/browser/ash/crostini/crostini_features.h"
 #include "chrome/browser/ash/crostini/crostini_util.h"
+#include "chrome/browser/ash/guest_os/public/guest_os_service.h"
 #include "chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chromeos/ash/components/dbus/vm_sk_forwarding/sk_forwarding.pb.h"
@@ -70,12 +71,20 @@ void VmSKForwardingServiceProvider::ForwardSecurityKeyMessage(
     return;
   }
 
-  guest_os::VmSKForwardingNativeMessageHost::
-      DeliverMessageToSKForwardingExtension(
-          profile, request.message(),
-          base::BindOnce(&VmSKForwardingServiceProvider::OnResponse,
-                         weak_ptr_factory_.GetWeakPtr(), method_call,
-                         std::move(response_sender)));
+  auto* service = ::guest_os::GuestOsService::GetForProfile(profile);
+  if (!service) {
+    constexpr char error_message[] = "GuestOsService does not exist";
+    std::move(response_sender)
+        .Run(dbus::ErrorResponse::FromMethodCall(method_call, DBUS_ERROR_FAILED,
+                                                 error_message));
+    return;
+  }
+
+  service->SkForwarder()->DeliverMessageToSKForwardingExtension(
+      profile, request.message(),
+      base::BindOnce(&VmSKForwardingServiceProvider::OnResponse,
+                     weak_ptr_factory_.GetWeakPtr(), method_call,
+                     std::move(response_sender)));
 }
 
 void VmSKForwardingServiceProvider::OnResponse(

diff --git a/chrome/browser/ash/guest_os/public/guest_os_service.cc b/chrome/browser/ash/guest_os/public/guest_os_service.cc
@@ -34,4 +34,8 @@ GuestOsWaylandServer* GuestOsService::WaylandServer() {
   return wayland_server_.get();
 }
 
+GuestOsSkForwarder* GuestOsService::SkForwarder() {
+  return &guest_os_sk_forwarder_;
+}
+
 }  // namespace guest_os
diff --git a/chrome/browser/ash/guest_os/public/guest_os_service.h b/chrome/browser/ash/guest_os/public/guest_os_service.h
@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "chrome/browser/ash/guest_os/public/guest_os_mount_provider_registry.h"
+#include "chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.h"
 #include "chrome/browser/ash/guest_os/public/guest_os_terminal_provider_registry.h"
 #include "components/keyed_service/core/keyed_service.h"
 
@@ -36,10 +37,13 @@ class GuestOsService : public KeyedService {
 
   GuestOsWaylandServer* WaylandServer();
 
+  GuestOsSkForwarder* SkForwarder();
+
  private:
   GuestOsMountProviderRegistry mount_provider_registry_;
   GuestOsTerminalProviderRegistry terminal_provider_registry_;
   std::unique_ptr<GuestOsWaylandServer> wayland_server_;
+  GuestOsSkForwarder guest_os_sk_forwarder_;
 };
 
 }  // namespace guest_os

diff --git a/chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.cc b/chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.cc
@@ -0,0 +1,43 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.h"
+
+#include "chrome/browser/ash/crosapi/browser_util.h"
+#include "chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
+
+namespace guest_os {
+
+GuestOsSkForwarder::GuestOsSkForwarder() = default;
+GuestOsSkForwarder::~GuestOsSkForwarder() = default;
+
+void GuestOsSkForwarder::BindCrosapiRemote(
+    mojo::PendingRemote<crosapi::mojom::GuestOsSkForwarder> remote) {
+  remote_.reset();
+  remote_.Bind(std::move(remote));
+}
+
+void GuestOsSkForwarder::DeliverMessageToSKForwardingExtension(
+    Profile* profile,
+    const std::string& json_message,
+    crosapi::mojom::GuestOsSkForwarder::ForwardRequestCallback callback) {
+  // Signal errors or non-response with an empty string.
+  callback =
+      mojo::WrapCallbackWithDefaultInvokeIfNotRun(std::move(callback), "");
+
+  if (crosapi::browser_util::IsLacrosEnabled()) {
+    if (remote_.is_bound() && remote_.is_connected()) {
+      remote_->ForwardRequest(json_message, std::move(callback));
+    }
+  } else {
+    // TODO(b/306296365) Once we require lacros, remove this branch and the ash
+    // copy of VmSKForwardingNativeMessageHost
+    ash::guest_os::VmSKForwardingNativeMessageHost::
+        DeliverMessageToSKForwardingExtension(profile, json_message,
+                                              std::move(callback));
+  }
+}
+
+}  // namespace guest_os
diff --git a/chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.h b/chrome/browser/ash/guest_os/public/guest_os_sk_forwarder.h
@@ -0,0 +1,40 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_GUEST_OS_SK_FORWARDER_H_
+#define CHROME_BROWSER_ASH_GUEST_OS_PUBLIC_GUEST_OS_SK_FORWARDER_H_
+
+#include <string>
+
+#include "chromeos/crosapi/mojom/guest_os_sk_forwarder.mojom.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/remote.h"
+
+class Profile;
+
+namespace guest_os {
+
+class GuestOsSkForwarder {
+ public:
+  GuestOsSkForwarder();
+  ~GuestOsSkForwarder();
+
+  GuestOsSkForwarder(const GuestOsSkForwarder&) = delete;
+  GuestOsSkForwarder& operator=(const GuestOsSkForwarder&) = delete;
+
+  void DeliverMessageToSKForwardingExtension(
+      Profile* profile,
+      const std::string& json_message,
+      crosapi::mojom::GuestOsSkForwarder::ForwardRequestCallback);
+
+  void BindCrosapiRemote(
+      mojo::PendingRemote<crosapi::mojom::GuestOsSkForwarder> remote);
+
+ private:
+  mojo::Remote<crosapi::mojom::GuestOsSkForwarder> remote_;
+};
+
+}  // namespace guest_os
+
+#endif
diff --git a/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.cc b/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// This file has been duplicated for lacros in
+// //chrome/browser/lacros/guest_os/vm_sk_forwarding_native_message_host.cc and
+// should eventually be removed.
+
 #include "chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h"
 
 #include <utility>

diff --git a/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h b/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h
@@ -5,6 +5,10 @@
 #ifndef CHROME_BROWSER_ASH_GUEST_OS_VM_SK_FORWARDING_NATIVE_MESSAGE_HOST_H_
 #define CHROME_BROWSER_ASH_GUEST_OS_VM_SK_FORWARDING_NATIVE_MESSAGE_HOST_H_
 
+// This file has been duplicated for lacros in
+// //chrome/browser/lacros/guest_os/vm_sk_forwarding_native_message_host.h and
+// should eventually be removed.
+
 #include <memory>
 #include <string>
 

diff --git a/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host_unittest.cc b/chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host_unittest.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// This file has been duplicated for lacros in
+// //chrome/browser/lacros/guest_os/vm_sk_forwarding_native_message_host_unittest.cc
+// and should eventually be removed.
+
 #include "chrome/browser/ash/guest_os/vm_sk_forwarding_native_message_host.h"
 
 #include "base/check.h"

diff --git a/chrome/browser/extensions/api/messaging/native_message_host_lacros.cc b/chrome/browser/extensions/api/messaging/native_message_host_lacros.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/extensions/api/messaging/native_message_built_in_host.h"
 #include "chrome/browser/extensions/api/messaging/native_message_echo_host.h"
 #include "chrome/browser/lacros/drivefs_native_message_host_lacros.h"
+#include "chrome/browser/lacros/guest_os/vm_sk_forwarding_native_message_host.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -37,6 +38,10 @@ const NativeMessageBuiltInHost kBuiltInHosts[] = {
      drive::kDriveFsNativeMessageHostOrigins.data(),
      drive::kDriveFsNativeMessageHostOrigins.size(),
      &drive::CreateDriveFsNativeMessageHostLacros},
+    {guest_os::VmSKForwardingNativeMessageHost::kHostName,
+     guest_os::VmSKForwardingNativeMessageHost::kOrigins,
+     guest_os::VmSKForwardingNativeMessageHost::kOriginCount,
+     &guest_os::VmSKForwardingNativeMessageHost::CreateFromExtension},
 };
 
 const size_t kBuiltInHostsCount = std::size(kBuiltInHosts);

diff --git a/chrome/browser/lacros/chrome_browser_main_extra_parts_lacros.cc b/chrome/browser/lacros/chrome_browser_main_extra_parts_lacros.cc
@@ -33,6 +33,7 @@
 #include "chrome/browser/lacros/force_installed_tracker_lacros.h"
 #include "chrome/browser/lacros/fullscreen_controller_client_lacros.h"
 #include "chrome/browser/lacros/geolocation/system_geolocation_source_lacros.h"
+#include "chrome/browser/lacros/guest_os/vm_sk_forwarding_service.h"
 #include "chrome/browser/lacros/lacros_apps_publisher.h"
 #include "chrome/browser/lacros/lacros_extension_apps_controller.h"
 #include "chrome/browser/lacros/lacros_extension_apps_publisher.h"
@@ -184,6 +185,8 @@ void ChromeBrowserMainExtraPartsLacros::PostBrowserStart() {
   if (chromeos::features::IsClipboardHistoryRefreshEnabled()) {
     clipboard_history_lacros_ = CreateClipboardHistoryLacros();
   }
+  vm_sk_forwarding_service_ =
+      std::make_unique<guest_os::VmSkForwardingService>();
 
   memory_pressure::MultiSourceMemoryPressureMonitor* monitor =
       static_cast<memory_pressure::MultiSourceMemoryPressureMonitor*>(