Send placeholder keys to keymaster before installing them on ARC

Inform arc-keymasterd of Chaps keys that will be installed in ARC.
Needed so arc-keymasterd can associate placeholders to the original
Chaps keys appropriately.

  out_/Default/unit_tests --gtest_filter="ArcCertInstaller*" && \
  out_/Default/browser_tests --gtest_filter="CertStoreServiceTest*"

Bug: b:172907736
Test: autoninja -C out_/Default unit_tests browser_tests && \
Change-Id: I85b2d234c5f12706093428228bcf690de45ddd9e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613965
Commit-Queue: Edman Anjos <edman@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Polina Bondarenko <pbond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843205}

chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer.cc

@@ -24,21 +24,15 @@
 
 namespace arc {
 
-namespace {
+CertDescription::CertDescription(crypto::RSAPrivateKey* placeholder_key,
+                                 CERTCertificate* nss_cert)
+    : placeholder_key(placeholder_key), nss_cert(nss_cert) {}
 
-// Exports subject public key info and encodes it in base64.
-std::string exportSpki(crypto::RSAPrivateKey* rsa) {
-  std::vector<uint8_t> spki;
-  if (!rsa->ExportPublicKey(&spki)) {
-    LOG(ERROR) << "Key export has failed.";
-    return "";
-  }
-  std::string encoded_spki;
-  base::Base64Encode(std::string(spki.begin(), spki.end()), &encoded_spki);
-  return encoded_spki;
-}
+CertDescription::CertDescription(CertDescription&& other) = default;
+
+CertDescription& CertDescription::operator=(CertDescription&& other) = default;
 
-}  // namespace
+CertDescription::~CertDescription() = default;
 
 ArcCertInstaller::ArcCertInstaller(content::BrowserContext* context)
     : ArcCertInstaller(Profile::FromBrowserContext(context),
@@ -60,7 +54,7 @@ ArcCertInstaller::~ArcCertInstaller() {
 }
 
 std::map<std::string, std::string> ArcCertInstaller::InstallArcCerts(
-    const std::vector<net::ScopedCERTCertificate>& certificates,
+    std::vector<CertDescription> certificates,
     InstallArcCertsCallback callback) {
   VLOG(1) << "ArcCertInstaller::InstallArcCerts";
 
@@ -75,7 +69,8 @@ std::map<std::string, std::string> ArcCertInstaller::InstallArcCerts(
   std::map<std::string, std::string> required_cert_names;
   callback_ = std::move(callback);
 
-  for (const auto& nss_cert : certificates) {
+  for (const auto& certificate : certificates) {
+    const net::ScopedCERTCertificate& nss_cert = certificate.nss_cert;
     if (!nss_cert) {
       LOG(ERROR)
           << "An invalid certificate has been passed to ArcCertInstaller";
@@ -84,7 +79,7 @@ std::map<std::string, std::string> ArcCertInstaller::InstallArcCerts(
 
     std::string cert_name =
         x509_certificate_model::GetCertNameOrNickname(nss_cert.get());
-    required_cert_names[cert_name] = InstallArcCert(cert_name, nss_cert);
+    required_cert_names[cert_name] = InstallArcCert(cert_name, certificate);
   }
 
   // Cleanup |known_cert_names_| according to |required_cert_names|.
@@ -103,7 +98,7 @@ std::map<std::string, std::string> ArcCertInstaller::InstallArcCerts(
 
 std::string ArcCertInstaller::InstallArcCert(
     const std::string& name,
-    const net::ScopedCERTCertificate& nss_cert) {
+    const CertDescription& certificate) {
   VLOG(1) << "ArcCertInstaller::InstallArcCert " << name;
 
   // Do not install certificate if it is already installed or pending.
@@ -113,7 +108,7 @@ std::string ArcCertInstaller::InstallArcCert(
   }
 
   std::string der_cert;
-  if (!net::x509_util::GetDEREncoded(nss_cert.get(), &der_cert)) {
+  if (!net::x509_util::GetDEREncoded(certificate.nss_cert.get(), &der_cert)) {
     LOG(ERROR) << "Certificate encoding error: " << name;
     return "";
   }
@@ -132,8 +127,7 @@ std::string ArcCertInstaller::InstallArcCert(
   std::string der_cert64;
   base::Base64Encode(der_cert, &der_cert64);
 
-  std::unique_ptr<crypto::RSAPrivateKey> rsa =
-      crypto::RSAPrivateKey::Create(2048);
+  crypto::RSAPrivateKey* rsa = certificate.placeholder_key.get();
   std::string pkcs12 = CreatePkcs12ForKey(name, rsa->key());
   command_proto.set_payload(
       base::StringPrintf("{\"type\":\"INSTALL_KEY_PAIR\","
@@ -153,7 +147,7 @@ std::string ArcCertInstaller::InstallArcCert(
     pending_commands_[next_id_++] = name;
     queue_->AddJob(std::move(job));
 
-    return exportSpki(rsa.get());
+    return ExportSpki(rsa);
   }
 }
 

chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer.h

@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/policy/core/common/remote_commands/remote_command_job.h"
 #include "components/policy/core/common/remote_commands/remote_commands_queue.h"
+#include "crypto/rsa_private_key.h"
 #include "net/cert/scoped_nss_types.h"
 
 class Profile;
@@ -26,6 +27,19 @@ class BrowserContext;
 
 namespace arc {
 
+struct CertDescription {
+  CertDescription(crypto::RSAPrivateKey* placeholder_key,
+                  CERTCertificate* nss_cert);
+  CertDescription(CertDescription&& other);
+  CertDescription(const CertDescription& other) = delete;
+  CertDescription& operator=(CertDescription&& other);
+  CertDescription& operator=(const CertDescription& other) = delete;
+  ~CertDescription();
+
+  std::unique_ptr<crypto::RSAPrivateKey> placeholder_key;
+  net::ScopedCERTCertificate nss_cert;
+};
+
 // This class manages the certificates, available to ARC.
 // It keeps track of the certificates and installs missing ones via
 // ARC remote commands.
@@ -49,7 +63,7 @@ class ArcCertInstaller : public policy::RemoteCommandsQueue::Observer {
   // Return false via |callback| in case of any error, and true otherwise.
   // Made virtual for override in test.
   virtual std::map<std::string, std::string> InstallArcCerts(
-      const std::vector<net::ScopedCERTCertificate>& certs,
+      std::vector<CertDescription> certificates,
       InstallArcCertsCallback callback);
 
  private:
@@ -58,7 +72,7 @@ class ArcCertInstaller : public policy::RemoteCommandsQueue::Observer {
   // or an empty string if the key is not installed during this call
   // (either error or already installed key pair).
   std::string InstallArcCert(const std::string& name,
-                             const net::ScopedCERTCertificate& nss_cert);
+                             const CertDescription& certificate);
 
   // RemoteCommandsQueue::Observer overrides:
   void OnJobStarted(policy::RemoteCommandJob* command) override {}

chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer_unittest.cc

@@ -64,8 +64,7 @@ class MockPolicyInstance : public FakePolicyInstance {
                     OnCommandReceivedCallback callback));
 };
 
-void AddCert(const std::string& cn,
-             std::vector<net::ScopedCERTCertificate>* certs) {
+void AddCert(const std::string& cn, std::vector<CertDescription>* certs) {
   std::string der_cert;
   net::ScopedCERTCertificate cert;
   std::unique_ptr<crypto::RSAPrivateKey> key(
@@ -77,7 +76,7 @@ void AddCert(const std::string& cn,
   cert = net::x509_util::CreateCERTCertificateFromBytes(
       reinterpret_cast<const uint8_t*>(der_cert.data()), der_cert.size());
   ASSERT_TRUE(cert);
-  certs->push_back(std::move(cert));
+  certs->push_back(CertDescription(key.release(), cert.release()));
 }
 
 }  // namespace
@@ -150,14 +149,14 @@ class ArcCertInstallerTest : public testing::Test {
 // Tests that installation of an empty cert list completes successfully.
 TEST_F(ArcCertInstallerTest, NoCertsTest) {
   installer()->InstallArcCerts(
-      std::vector<net::ScopedCERTCertificate>(),
+      std::vector<CertDescription>(),
       base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
 }
 
 // Tests that installing certs completes successfully if there are two certs
 // available.
 TEST_F(ArcCertInstallerTest, BasicCertTest) {
-  std::vector<net::ScopedCERTCertificate> certs;
+  std::vector<CertDescription> certs;
 
   AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
   AddCert(base::StringPrintf(kCNFormat, kFakeName2), &certs);
@@ -194,7 +193,7 @@ TEST_F(ArcCertInstallerTest, ConsequentInstallTest) {
                                policy::RemoteCommandJob::Status::SUCCEEDED)))
       .Times(3);
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
     AddCert(base::StringPrintf(kCNFormat, kFakeName2), &certs);
     base::RunLoop run_loop;
@@ -210,7 +209,7 @@ TEST_F(ArcCertInstallerTest, ConsequentInstallTest) {
 
   ExpectArcCommandForName(kFakeName3, mojom::CommandResultType::SUCCESS);
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
     AddCert(base::StringPrintf(kCNFormat, kFakeName3), &certs);
 
@@ -236,7 +235,7 @@ TEST_F(ArcCertInstallerTest, FailureIncompleteInstallationTest) {
                                policy::RemoteCommandJob::Status::SUCCEEDED)));
 
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
 
     installer()->InstallArcCerts(std::move(certs),
@@ -248,7 +247,7 @@ TEST_F(ArcCertInstallerTest, FailureIncompleteInstallationTest) {
   }
 
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
 
     base::RunLoop run_loop;
@@ -271,7 +270,7 @@ TEST_F(ArcCertInstallerTest, FailedRequiredSmartCardTest) {
   EXPECT_CALL(*observer(), OnJobFinished(IsCommandWithStatus(
                                policy::RemoteCommandJob::Status::FAILED)));
 
-  std::vector<net::ScopedCERTCertificate> certs;
+  std::vector<CertDescription> certs;
   AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
 
   base::RunLoop run_loop;
@@ -292,7 +291,7 @@ TEST_F(ArcCertInstallerTest, FailiedNotRequiredSmartCardTest) {
                                policy::RemoteCommandJob::Status::RUNNING)))
       .Times(2);
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName1), &certs);
 
     installer()->InstallArcCerts(std::move(certs),
@@ -311,7 +310,7 @@ TEST_F(ArcCertInstallerTest, FailiedNotRequiredSmartCardTest) {
                                policy::RemoteCommandJob::Status::FAILED)));
 
   {
-    std::vector<net::ScopedCERTCertificate> certs;
+    std::vector<CertDescription> certs;
     AddCert(base::StringPrintf(kCNFormat, kFakeName2), &certs);
 
     base::RunLoop run_loop;

chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer_utils.cc

@@ -7,6 +7,7 @@
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
+#include "crypto/rsa_private_key.h"
 #include "third_party/boringssl/src/include/openssl/evp.h"
 #include "third_party/boringssl/src/include/openssl/mem.h"
 #include "third_party/boringssl/src/include/openssl/pkcs8.h"
@@ -41,4 +42,16 @@ std::string CreatePkcs12ForKey(const std::string& name, EVP_PKEY* key) {
   return encoded_pkcs12_key;
 }
 
+std::string ExportSpki(crypto::RSAPrivateKey* rsa) {
+  DCHECK(rsa);
+  std::vector<uint8_t> spki;
+  if (!rsa->ExportPublicKey(&spki)) {
+    LOG(ERROR) << "Key export has failed.";
+    return "";
+  }
+  std::string encoded_spki;
+  base::Base64Encode(std::string(spki.begin(), spki.end()), &encoded_spki);
+  return encoded_spki;
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer_utils.h

@@ -9,12 +9,21 @@
 
 #include "third_party/boringssl/src/include/openssl/base.h"
 
+namespace crypto {
+
+class RSAPrivateKey;
+
+}  // namespace crypto
+
 namespace arc {
 
 // Creates a PKCS12 container named |name| with private key |key|.
 // Returns empty string in case of any error.
 std::string CreatePkcs12ForKey(const std::string& name, EVP_PKEY* key);
 
+// Exports the subject public key info of the given |rsa| key encoded in base64.
+std::string ExportSpki(crypto::RSAPrivateKey* rsa);
+
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ENTERPRISE_CERT_STORE_ARC_CERT_INSTALLER_UTILS_H_

chrome/browser/chromeos/arc/enterprise/cert_store/cert_store_service.cc

@@ -16,6 +16,8 @@
 #include "base/memory/singleton.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer_utils.h"
+#include "chrome/browser/chromeos/arc/keymaster/arc_keymaster_bridge.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_bridge.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
 #include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_service_factory.h"
@@ -26,6 +28,7 @@
 #include "chrome/browser/net/nss_context.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/net/x509_certificate_model_nss.h"
+#include "chrome/services/keymaster/public/mojom/cert_store.mojom.h"
 #include "components/arc/arc_browser_context_keyed_service_factory_base.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/policy_namespace.h"
@@ -123,6 +126,60 @@ void IsCertificateAllowed(IsCertificateAllowedCallback callback,
                          public_key_spki_der, context));
 }
 
+std::vector<CertDescription> PrepareCertDescriptions(
+    net::ScopedCERTCertificateList nss_certs) {
+  std::vector<CertDescription> certificates;
+  for (auto& nss_cert : nss_certs) {
+    if (!nss_cert)
+      continue;
+    // Generate the placeholder RSA key that will be installed in ARC.
+    auto placeholder_key = crypto::RSAPrivateKey::Create(2048);
+    DCHECK(placeholder_key);
+
+    certificates.emplace_back(placeholder_key.release(), nss_cert.release());
+  }
+  return certificates;
+}
+
+std::vector<keymaster::mojom::ChromeOsKeyPtr> PrepareChromeOsKeys(
+    const std::vector<CertDescription>& certificates) {
+  std::vector<keymaster::mojom::ChromeOsKeyPtr> chrome_os_keys;
+  for (const auto& certificate : certificates) {
+    CERTCertificate* nss_cert = certificate.nss_cert.get();
+    DCHECK(nss_cert);
+
+    // Fetch PKCS#11 CKA_LABEL.
+    SECKEYPrivateKey* priv_key =
+        PK11_FindKeyByAnyCert(nss_cert, nullptr /* wincx */);
+    if (!priv_key)
+      continue;
+    crypto::ScopedSECKEYPrivateKey priv_key_destroyer(priv_key);
+
+    char* nickname = PK11_GetPrivateKeyNickname(priv_key);
+    if (!nickname)
+      continue;
+    std::string pkcs11_label(nickname);
+
+    // Fetch PKCS#11 CKA_ID.
+    SECItem* id_item = PK11_GetLowLevelKeyIDForPrivateKey(priv_key);
+    if (!id_item)
+      continue;
+    crypto::ScopedSECItem sec_item_destroyer(id_item);
+    std::string pkcs11_id(id_item->data, id_item->data + id_item->len);
+
+    // Build a mojo ChromeOsKey and store it in the output vector.
+    keymaster::mojom::ChapsKeyDataPtr key_data =
+        keymaster::mojom::ChapsKeyData::New(pkcs11_label, pkcs11_id);
+    keymaster::mojom::ChromeOsKeyPtr key = keymaster::mojom::ChromeOsKey::New(
+        ExportSpki(certificate.placeholder_key.get()),
+        keymaster::mojom::KeyData::NewChapsKeyData(std::move(key_data)));
+
+    chrome_os_keys.push_back(std::move(key));
+  }
+
+  return chrome_os_keys;
+}
+
 }  // namespace
 
 // static
@@ -239,13 +296,39 @@ void CertStoreService::OnCertificatesListed(
 
 void CertStoreService::OnFilteredAllowedCertificates(
     net::ScopedCERTCertificateList allowed_certs) {
+  ArcKeymasterBridge* const keymaster_bridge =
+      ArcKeymasterBridge::GetForBrowserContext(context_);
+  if (!keymaster_bridge) {
+    LOG(ERROR) << "Missing instance of ArcKeymasterBridge.";
+    return;
+  }
+
+  std::vector<CertDescription> certificates =
+      PrepareCertDescriptions(std::move(allowed_certs));
+  std::vector<keymaster::mojom::ChromeOsKeyPtr> keys =
+      PrepareChromeOsKeys(certificates);
+
+  keymaster_bridge->UpdatePlaceholderKeys(
+      std::move(keys),
+      base::BindOnce(&CertStoreService::OnUpdatedKeymasterKeys,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(certificates)));
+}
+
+void CertStoreService::OnUpdatedKeymasterKeys(
+    std::vector<CertDescription> certificate_descriptions,
+    bool success) {
+  if (!success) {
+    LOG(WARNING) << "Could not update placeholder keys with keymaster.";
+    return;
+  }
+
   certificate_cache_.clear_need_policy_update();
-  auto certificates = certificate_cache_.Update(std::move(allowed_certs));
+  certificate_cache_.Update(certificate_descriptions);
 
   // Maps cert name to dummy SPKI.
   std::map<std::string, std::string> installed_keys =
       installer_->InstallArcCerts(
-          std::move(certificates),
+          std::move(certificate_descriptions),
           base::BindOnce(&CertStoreService::OnArcCertsInstalled,
                          weak_ptr_factory_.GetWeakPtr()));
 
@@ -255,34 +338,35 @@ void CertStoreService::OnFilteredAllowedCertificates(
 CertStoreService::CertificateCache::CertificateCache() = default;
 CertStoreService::CertificateCache::~CertificateCache() = default;
 
-net::ScopedCERTCertificateList CertStoreService::CertificateCache::Update(
-    net::ScopedCERTCertificateList allowed_certs) {
-  net::ScopedCERTCertificateList certs;
+void CertStoreService::CertificateCache::Update(
+    const std::vector<CertDescription>& certificates) {
   // Map cert name to real SPKI.
   key_info_by_name_cache_.clear();
   std::set<std::string> new_required_cert_names;
-  for (auto& cert : allowed_certs) {
-    if (!cert)
-      continue;
+  for (const auto& certificate : certificates) {
+    CERTCertificate* nss_cert = certificate.nss_cert.get();
+    DCHECK(nss_cert);
+
+    // Fetch certificate name.
     std::string cert_name =
-        x509_certificate_model::GetCertNameOrNickname(cert.get());
+        x509_certificate_model::GetCertNameOrNickname(nss_cert);
+
+    // Fetch PKCS#11 CKA_ID.
     SECKEYPrivateKey* priv_key =
-        PK11_FindKeyByAnyCert(cert.get(), nullptr /* wincx */);
+        PK11_FindKeyByAnyCert(nss_cert, nullptr /* wincx */);
     if (!priv_key)
       continue;
-    // Get the CKA_ID attribute for a key.
+    crypto::ScopedSECKEYPrivateKey priv_key_destroyer(priv_key);
+
     SECItem* sec_item = PK11_GetLowLevelKeyIDForPrivateKey(priv_key);
     std::string pkcs11_id;
     if (sec_item) {
-      pkcs11_id = base::HexEncode(sec_item->data, sec_item->len);
+      pkcs11_id = std::string(sec_item->data, sec_item->data + sec_item->len);
       SECITEM_FreeItem(sec_item, PR_TRUE);
     }
-    SECKEY_DestroyPrivateKey(priv_key);
 
     key_info_by_name_cache_[cert_name] = {cert_name, pkcs11_id};
     new_required_cert_names.insert(cert_name);
-
-    certs.push_back(std::move(cert));
   }
   need_policy_update_ = (required_cert_names_ != new_required_cert_names);
   for (auto cert_name : required_cert_names_) {
@@ -292,7 +376,6 @@ net::ScopedCERTCertificateList CertStoreService::CertificateCache::Update(
     }
   }
   required_cert_names_ = new_required_cert_names;
-  return certs;
 }
 
 void CertStoreService::CertificateCache::Update(

chrome/browser/chromeos/arc/enterprise/cert_store/cert_store_service.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_CHROMEOS_ARC_ENTERPRISE_CERT_STORE_CERT_STORE_SERVICE_H_
 #define CHROME_BROWSER_CHROMEOS_ARC_ENTERPRISE_CERT_STORE_CERT_STORE_SERVICE_H_
 
+#include <map>
 #include <memory>
 #include <set>
 #include <string>
@@ -14,6 +15,7 @@
 #include "base/containers/queue.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/arc/enterprise/cert_store/arc_cert_installer.h"
+#include "chrome/services/keymaster/public/mojom/cert_store.mojom.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/browser_context.h"
@@ -72,13 +74,15 @@ class CertStoreService : public KeyedService,
   using FilterAllowedCertificatesCallback =
       base::OnceCallback<void(net::ScopedCERTCertificateList allowed_certs)>;
 
+  // TODO(b/177051802) Some of certificate cache is obsolete. Clean up.
   class CertificateCache {
    public:
     CertificateCache();
+    CertificateCache(const CertificateCache& other) = delete;
+    CertificateCache& operator=(const CertificateCache&) = delete;
     ~CertificateCache();
 
-    net::ScopedCERTCertificateList Update(
-        net::ScopedCERTCertificateList allowed_certs);
+    void Update(const std::vector<CertDescription>& certificates);
     void Update(std::map<std::string, std::string> dummy_spki_by_name);
 
     base::Optional<KeyInfo> GetKeyInfoForDummySpki(
@@ -121,6 +125,8 @@ class CertStoreService : public KeyedService,
   void OnCertificatesListed(net::ScopedCERTCertificateList cert_list);
   void OnFilteredAllowedCertificates(
       net::ScopedCERTCertificateList allowed_certs);
+  void OnUpdatedKeymasterKeys(std::vector<CertDescription> certificates,
+                              bool success);
   void OnArcCertsInstalled(bool success);
 
   content::BrowserContext* const context_;

chrome/browser/chromeos/arc/enterprise/cert_store/cert_store_service_browsertest.cc

@@ -12,6 +12,7 @@
 #include "base/command_line.h"
 #include "base/run_loop.h"
 #include "chrome/browser/chromeos/arc/enterprise/cert_store/cert_store_service.h"
+#include "chrome/browser/chromeos/arc/keymaster/arc_keymaster_bridge.h"
 #include "chrome/browser/chromeos/arc/session/arc_service_launcher.h"
 #include "chrome/browser/chromeos/login/test/local_policy_test_server_mixin.h"
 #include "chrome/browser/chromeos/platform_keys/key_permissions/extension_key_permissions_service.h"
@@ -26,6 +27,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/net/x509_certificate_model_nss.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/services/keymaster/public/mojom/cert_store.mojom.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/constants/chromeos_switches.h"
@@ -70,12 +72,12 @@ class FakeArcCertInstaller : public ArcCertInstaller {
 
   // Returns map from nicknames to real der cert64 to identify certificates.
   std::map<std::string, std::string> InstallArcCerts(
-      const std::vector<net::ScopedCERTCertificate>& certs,
+      std::vector<CertDescription> certs,
       InstallArcCertsCallback callback) override {
     certs_.clear();
     for (const auto& cert : certs) {
-      certs_[x509_certificate_model::GetCertNameOrNickname(cert.get())] =
-          GetDerCert64(cert.get());
+      certs_[x509_certificate_model::GetCertNameOrNickname(
+          cert.nss_cert.get())] = GetDerCert64(cert.nss_cert.get());
     }
 
     callback_ = std::move(callback);
@@ -94,7 +96,7 @@ class FakeArcCertInstaller : public ArcCertInstaller {
 
   void Stop() {
     if (run_loop_)
-      run_loop_->Quit();
+      run_loop_->QuitWhenIdle();
   }
 
   std::map<std::string, std::string> certs() const { return certs_; }
@@ -105,6 +107,33 @@ class FakeArcCertInstaller : public ArcCertInstaller {
   InstallArcCertsCallback callback_;
 };
 
+class FakeArcKeymasterBridge : public ArcKeymasterBridge {
+ public:
+  explicit FakeArcKeymasterBridge(content::BrowserContext* context)
+      : ArcKeymasterBridge(context, nullptr) {}
+  FakeArcKeymasterBridge(const FakeArcKeymasterBridge& other) = delete;
+  FakeArcKeymasterBridge& operator=(const FakeArcKeymasterBridge&) = delete;
+
+  void UpdatePlaceholderKeys(std::vector<keymaster::mojom::ChromeOsKeyPtr> keys,
+                             UpdatePlaceholderKeysCallback callback) override {
+    keys_ = std::move(keys);
+    std::move(callback).Run(/*success=*/true);
+  }
+
+  const std::vector<keymaster::mojom::ChromeOsKeyPtr>& placeholder_keys()
+      const {
+    return keys_;
+  }
+
+ private:
+  std::vector<keymaster::mojom::ChromeOsKeyPtr> keys_;
+};
+
+std::unique_ptr<KeyedService> BuildFakeArcKeymasterBridge(
+    content::BrowserContext* profile) {
+  return std::make_unique<FakeArcKeymasterBridge>(profile);
+}
+
 std::unique_ptr<KeyedService> BuildCertStoreService(
     std::unique_ptr<FakeArcCertInstaller> installer,
     content::BrowserContext* profile) {
@@ -166,6 +195,13 @@ class CertStoreServiceTest : public MixinBasedInProcessBrowserTest {
     browser()->profile()->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted,
                                                  true);
 
+    ArcKeymasterBridge::GetFactory()->SetTestingFactoryAndUse(
+        browser()->profile(),
+        base::BindRepeating(&BuildFakeArcKeymasterBridge));
+    auto* keymaster_bridge =
+        ArcKeymasterBridge::GetForBrowserContext(browser()->profile());
+    keymaster_bridge_ = static_cast<FakeArcKeymasterBridge*>(keymaster_bridge);
+
     auto installer = std::make_unique<FakeArcCertInstaller>(
         browser()->profile(), std::make_unique<policy::RemoteCommandsQueue>());
     installer_ = installer.get();
@@ -254,11 +290,23 @@ class CertStoreServiceTest : public MixinBasedInProcessBrowserTest {
     loop.Run();
   }
 
+  bool PlaceholdersContainId(const std::string& id) {
+    for (const auto& key : keymaster_bridge()->placeholder_keys()) {
+      if (key->key_data->is_chaps_key_data() &&
+          key->key_data->get_chaps_key_data()->id == id) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   void CheckInstalledCerts(size_t installed_cert_num,
                            CertStoreService* service) {
     EXPECT_EQ(installed_cert_num, client_certs_.size());
     EXPECT_EQ(installed_cert_num, installer()->certs().size());
     EXPECT_EQ(installed_cert_num, service->get_required_cert_names().size());
+    EXPECT_EQ(installed_cert_num,
+              keymaster_bridge()->placeholder_keys().size());
 
     for (const auto& cert_name : service->get_required_cert_names()) {
       bool found = false;
@@ -279,9 +327,12 @@ class CertStoreServiceTest : public MixinBasedInProcessBrowserTest {
           EXPECT_EQ(key_info.value().nickname, cert_name);
           int slot_id;
           // Check CKA_ID.
-          EXPECT_EQ(key_info.value().id,
+          std::string hex_encoded_id = base::HexEncode(
+              key_info.value().id.data(), key_info.value().id.size());
+          EXPECT_EQ(hex_encoded_id,
                     chromeos::NetworkCertLoader::GetPkcs11IdAndSlotForCert(
                         cert.get(), &slot_id));
+          EXPECT_TRUE(PlaceholdersContainId(key_info.value().id));
           break;
         }
       }
@@ -292,6 +343,8 @@ class CertStoreServiceTest : public MixinBasedInProcessBrowserTest {
 
   FakeArcCertInstaller* installer() { return installer_; }
 
+  FakeArcKeymasterBridge* keymaster_bridge() { return keymaster_bridge_; }
+
   net::ScopedCERTCertificateList client_certs_;
 
  private:
@@ -370,6 +423,7 @@ class CertStoreServiceTest : public MixinBasedInProcessBrowserTest {
 
   // Owned by service.
   FakeArcCertInstaller* installer_;
+  FakeArcKeymasterBridge* keymaster_bridge_;
 };
 
 // Test no corporate usage keys.
@@ -388,6 +442,7 @@ IN_PROC_BROWSER_TEST_F(CertStoreServiceTest, Basic) {
   // No corporate usage keys installed.
   EXPECT_TRUE(installer()->certs().empty());
   EXPECT_TRUE(service->get_required_cert_names().empty());
+  EXPECT_TRUE(keymaster_bridge()->placeholder_keys().empty());
 }
 
 // Test installation of 2 corporate usage keys.
@@ -420,6 +475,10 @@ IN_PROC_BROWSER_TEST_F(CertStoreServiceTest, UninstalledCorporateUsageKeys) {
             Profile::FromBrowserContext(browser()->profile()));
   ASSERT_TRUE(service);
 
+  installer()->Wait();
+  installer()->RunCompletionCallback(true /* success */);
+
+  CheckInstalledCerts(0, service);
   ASSERT_NO_FATAL_FAILURE(
       SetUpCerts({kCertFiles}, true /* is_corporate_usage_key */));
   installer()->Wait();

chrome/browser/chromeos/arc/keymaster/arc_keymaster_bridge.cc

@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/process/process_handle.h"
+#include "chrome/services/keymaster/public/mojom/cert_store.mojom.h"
 #include "chromeos/dbus/arc/arc_keymaster_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_browser_context_keyed_service_factory_base.h"
@@ -42,6 +43,11 @@ class ArcKeymasterBridgeFactory
 
 }  // namespace
 
+// static
+BrowserContextKeyedServiceFactory* ArcKeymasterBridge::GetFactory() {
+  return ArcKeymasterBridgeFactory::GetInstance();
+}
+
 // static
 ArcKeymasterBridge* ArcKeymasterBridge::GetForBrowserContext(
     content::BrowserContext* context) {
@@ -53,35 +59,73 @@ ArcKeymasterBridge::ArcKeymasterBridge(content::BrowserContext* context,
     : arc_bridge_service_(bridge_service),
       cert_store_bridge_(std::make_unique<keymaster::CertStoreBridge>(context)),
       weak_factory_(this) {
-  arc_bridge_service_->keymaster()->SetHost(this);
+  if (arc_bridge_service_)
+    arc_bridge_service_->keymaster()->SetHost(this);
 }
 
 ArcKeymasterBridge::~ArcKeymasterBridge() {
-  arc_bridge_service_->keymaster()->SetHost(nullptr);
+  if (arc_bridge_service_)
+    arc_bridge_service_->keymaster()->SetHost(nullptr);
+}
+
+void ArcKeymasterBridge::UpdatePlaceholderKeys(
+    std::vector<keymaster::mojom::ChromeOsKeyPtr> keys,
+    UpdatePlaceholderKeysCallback callback) {
+  if (cert_store_bridge_->is_proxy_bound()) {
+    cert_store_bridge_->UpdatePlaceholderKeysInKeymaster(std::move(keys),
+                                                         std::move(callback));
+  } else {
+    BootstrapMojoConnection(base::BindOnce(
+        &ArcKeymasterBridge::UpdatePlaceholderKeysAfterBootstrap,
+        weak_factory_.GetWeakPtr(), std::move(keys), std::move(callback)));
+  }
+}
+
+void ArcKeymasterBridge::UpdatePlaceholderKeysAfterBootstrap(
+    std::vector<keymaster::mojom::ChromeOsKeyPtr> keys,
+    UpdatePlaceholderKeysCallback callback,
+    bool bootstrapResult) {
+  if (bootstrapResult) {
+    cert_store_bridge_->UpdatePlaceholderKeysInKeymaster(std::move(keys),
+                                                         std::move(callback));
+  } else {
+    std::move(callback).Run(/*success=*/false);
+  }
 }
 
 void ArcKeymasterBridge::GetServer(GetServerCallback callback) {
-  if (!keymaster_server_proxy_.is_bound()) {
-    BootstrapMojoConnection(std::move(callback));
-    return;
+  if (keymaster_server_proxy_.is_bound()) {
+    std::move(callback).Run(keymaster_server_proxy_.Unbind());
+  } else {
+    BootstrapMojoConnection(
+        base::BindOnce(&ArcKeymasterBridge::GetServerAfterBootstrap,
+                       weak_factory_.GetWeakPtr(), std::move(callback)));
   }
-  std::move(callback).Run(keymaster_server_proxy_.Unbind());
 }
 
-void ArcKeymasterBridge::OnBootstrapMojoConnection(GetServerCallback callback,
-                                                   bool result) {
+void ArcKeymasterBridge::GetServerAfterBootstrap(GetServerCallback callback,
+                                                 bool bootstrapResult) {
+  if (bootstrapResult)
+    std::move(callback).Run(keymaster_server_proxy_.Unbind());
+  else
+    std::move(callback).Run(mojo::NullRemote());
+}
+
+void ArcKeymasterBridge::OnBootstrapMojoConnection(
+    BootstrapMojoConnectionCallback callback,
+    bool result) {
   cert_store_bridge_->OnBootstrapMojoConnection(result);
-  if (!result) {
+  if (result) {
+    DVLOG(1) << "Success bootstrapping Mojo in arc-keymasterd.";
+  } else {
     LOG(ERROR) << "Error bootstrapping Mojo in arc-keymasterd.";
     keymaster_server_proxy_.reset();
-    std::move(callback).Run(mojo::NullRemote());
-    return;
   }
-  DVLOG(1) << "Success bootstrapping Mojo in arc-keymasterd.";
-  std::move(callback).Run(keymaster_server_proxy_.Unbind());
+  std::move(callback).Run(result);
 }
 
-void ArcKeymasterBridge::BootstrapMojoConnection(GetServerCallback callback) {
+void ArcKeymasterBridge::BootstrapMojoConnection(
+    BootstrapMojoConnectionCallback callback) {
   DVLOG(1) << "Bootstrapping arc-keymasterd Mojo connection via D-Bus.";
 
   mojo::OutgoingInvitation invitation;

chrome/browser/chromeos/arc/keymaster/arc_keymaster_bridge.h

@@ -5,9 +5,12 @@
 #ifndef CHROME_BROWSER_CHROMEOS_ARC_KEYMASTER_ARC_KEYMASTER_BRIDGE_H_
 #define CHROME_BROWSER_CHROMEOS_ARC_KEYMASTER_ARC_KEYMASTER_BRIDGE_H_
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/arc/keymaster/cert_store_bridge.h"
+#include "chrome/services/keymaster/public/mojom/cert_store.mojom.h"
+#include "components/arc/arc_browser_context_keyed_service_factory_base.h"
 #include "components/arc/mojom/keymaster.mojom.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -27,6 +30,9 @@ class ArcBridgeService;
 // to the KeymasterInstance in ARC.
 class ArcKeymasterBridge : public KeyedService, public mojom::KeymasterHost {
  public:
+  using mojom::KeymasterHost::GetServerCallback;
+  using UpdatePlaceholderKeysCallback = base::OnceCallback<void(bool)>;
+
   // Returns singleton instance for the given BrowserContext, or nullptr if the
   // browser |context| is not allowed to use ARC.
   static ArcKeymasterBridge* GetForBrowserContext(
@@ -36,20 +42,39 @@ class ArcKeymasterBridge : public KeyedService, public mojom::KeymasterHost {
                      ArcBridgeService* bridge_service);
   ~ArcKeymasterBridge() override;
 
+  // Return the factory instance for this class.
+  static BrowserContextKeyedServiceFactory* GetFactory();
+
+  // Update the list of placeholder keys to be instlaled in arc-keymasterd.
+  //
+  // Made virtual for override in tests.
+  virtual void UpdatePlaceholderKeys(
+      std::vector<keymaster::mojom::ChromeOsKeyPtr> keys,
+      UpdatePlaceholderKeysCallback callback);
+
   // KeymasterHost mojo interface.
-  using mojom::KeymasterHost::GetServerCallback;
   void GetServer(GetServerCallback callback) override;
 
  private:
-  void BootstrapMojoConnection(GetServerCallback callback);
-  void OnBootstrapMojoConnection(GetServerCallback callback, bool result);
+  using BootstrapMojoConnectionCallback = base::OnceCallback<void(bool)>;
+
+  void BootstrapMojoConnection(BootstrapMojoConnectionCallback callback);
+  void OnBootstrapMojoConnection(BootstrapMojoConnectionCallback callback,
+                                 bool bootstrapResult);
+  void UpdatePlaceholderKeysAfterBootstrap(
+      std::vector<keymaster::mojom::ChromeOsKeyPtr> keys,
+      UpdatePlaceholderKeysCallback callback,
+      bool bootstrapResult);
+  void GetServerAfterBootstrap(GetServerCallback callback,
+                               bool bootstrapResult);
 
   ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager.
 
   // Points to a proxy bound to the implementation in arc-keymasterd.
   mojo::Remote<mojom::KeymasterServer> keymaster_server_proxy_;
 
-  // Points to the host implementation in Chrome, used to interact with arc-keymasterd.
+  // Points to the host implementation in Chrome, used to interact with the
+  // arc-keymasterd daemon.
   std::unique_ptr<keymaster::CertStoreBridge> cert_store_bridge_;
 
   // WeakPtrFactory to use for callbacks.

chrome/browser/chromeos/arc/keymaster/cert_store_bridge.cc

@@ -24,6 +24,19 @@ CertStoreBridge::~CertStoreBridge() {
   VLOG(2) << "CertStoreBridge::~CertStoreBridge";
 }
 
+void CertStoreBridge::UpdatePlaceholderKeysInKeymaster(
+    std::vector<mojom::ChromeOsKeyPtr> keys,
+    mojom::CertStoreInstance::UpdatePlaceholderKeysCallback callback) {
+  VLOG(2) << "CertStoreBridge::UpdatePlaceholderKeysInKeymaster";
+  if (cert_store_proxy_.is_bound()) {
+    cert_store_proxy_->UpdatePlaceholderKeys(std::move(keys),
+                                             std::move(callback));
+  } else {
+    LOG(ERROR) << "Tried to update placeholders but cert store is not bound";
+    std::move(callback).Run(/*success=*/false);
+  }
+}
+
 void CertStoreBridge::GetSecurityTokenOperation(
     mojo::PendingReceiver<mojom::SecurityTokenOperation> operation_receiver,
     GetSecurityTokenOperationCallback callback) {

chrome/browser/chromeos/arc/keymaster/cert_store_bridge.h

@@ -35,6 +35,13 @@ class CertStoreBridge : public mojom::CertStoreHost {
   void BindToInvitation(mojo::OutgoingInvitation* invitation);
   void OnBootstrapMojoConnection(bool result);
 
+  bool is_proxy_bound() const { return cert_store_proxy_.is_bound(); }
+
+  // Send the latest information about Chrome OS keys to arc-keymasterd.
+  void UpdatePlaceholderKeysInKeymaster(
+      std::vector<mojom::ChromeOsKeyPtr> keys,
+      mojom::CertStoreInstance::UpdatePlaceholderKeysCallback callback);
+
   // CertStoreHost overrides.
   void GetSecurityTokenOperation(
       mojo::PendingReceiver<mojom::SecurityTokenOperation> operation_receiver,

chrome/services/keymaster/public/mojom/cert_store.mojom

@@ -28,11 +28,29 @@ enum Digest {
 [Extensible]
 enum SignatureResult {
   kOk,
-  // Failed with error.
+  // Failed with net or internal error on chrome side.
   kFailed,
   kUnsupportedAlgorithm,
 };
 
+// Metadata to uniquely identify a Chaps key.
+struct ChapsKeyData {
+  string label;
+  string id;
+};
+
+// Union of Chrome OS keys from different sources.
+union KeyData {
+  ChapsKeyData chaps_key_data;
+};
+
+// Describes a placeholder for a Chrome OS key along with metadata about the
+// original key.
+struct ChromeOsKey {
+  string base64_subject_public_key_info;
+  KeyData key_data;
+};
+
 // Interface exposed by Chrome.
 // Next method ID: 1
 interface CertStoreHost {
@@ -42,10 +60,13 @@ interface CertStoreHost {
 };
 
 // Interface exposed by arc-keymaster daemon.
-// Next method ID: 1
+// Next method ID: 2
 interface CertStoreInstance {
   // Establishes full-duplex communication with the host.
   Init@0(pending_remote<CertStoreHost> host_remote) => ();
+
+  // Updates info about the latest set of keys owned by Chrome OS.
+  UpdatePlaceholderKeys@1(array<ChromeOsKey> keys) => (bool success);
 };
 
 // Implemented in Chrome.