Cert Provisioning: Ignore certificates without attributes

Ignore certificates that return error when asked for
certificate provisioning specific attribute. The error
most likely just means that the certificate doesn't
have a value for the attribute. This will be changed
later when there is a way to distinguish unset
attribute case and other errors.

Bug: 1045895
Test: CertProvisioning*
Change-Id: Ib4960916f53e50ed6d374085cbd165b8fdcd4c68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2275879
Commit-Queue: Michael Ershov <miersh@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784422}

chrome/browser/chromeos/BUILD.gn

@@ -2983,6 +2983,7 @@ source_set("unit_tests") {
     "base/file_flusher_unittest.cc",
     "bluetooth/debug_logs_manager_unittest.cc",
     "cert_provisioning/cert_provisioning_invalidator_unittest.cc",
+    "cert_provisioning/cert_provisioning_platform_keys_helpers_unittest.cc",
     "cert_provisioning/cert_provisioning_scheduler_unittest.cc",
     "cert_provisioning/cert_provisioning_test_helpers.cc",
     "cert_provisioning/cert_provisioning_test_helpers.h",

chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.cc

@@ -87,13 +87,12 @@ void CertProvisioningCertsWithIdsGetter::CollectOneResult(
     return;
   }
 
-  if (!error_message.empty()) {
-    std::move(callback_).Run(/*existing_cert_ids=*/{}, error_message);
-    return;
+  if (error_message.empty()) {
+    // TODO(crbug.com/1101103): Currently results with errors are just ignored.
+    // Fix when PlatformKeysService API is changed.
+    certs_with_ids_[cert_id] = cert;
   }
 
-  certs_with_ids_[cert_id] = cert;
-
   --wait_counter_;
   if (wait_counter_ != 0) {
     return;

chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/containers/flat_map.h"
+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
+#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.h"
+#include "chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h"
+#include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
+#include "content/public/test/browser_task_environment.h"
+#include "net/cert/x509_certificate.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
+using ::testing::Key;
+
+namespace chromeos {
+namespace cert_provisioning {
+namespace {
+
+template <typename MapType>
+base::flat_set<typename MapType::key_type> GetKeys(const MapType& map) {
+  base::flat_set<typename MapType::key_type> keys;
+  for (const auto& pair : map) {
+    keys.insert(pair.first);
+  }
+  return keys;
+}
+
+class CallbackObserver {
+ public:
+  using CertMap =
+      base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>;
+
+  GetCertsWithIdsCallback GetCallback() {
+    return base::BindOnce(&CallbackObserver::Callback, base::Unretained(this));
+  }
+
+  const CertMap& GetMap() { return cert_map_; }
+  const std::string GetError() { return error_message_; }
+
+  void WaitForCallback() { loop_.Run(); }
+
+ protected:
+  void Callback(CertMap certs_with_ids, const std::string& error_message) {
+    cert_map_ = std::move(certs_with_ids);
+    error_message_ = error_message;
+    loop_.Quit();
+  }
+
+  base::RunLoop loop_;
+  CertMap cert_map_;
+  std::string error_message_;
+};
+
+class CertProvisioningCertsWithIdsGetterTest : public ::testing::Test {
+ public:
+  CertProvisioningCertsWithIdsGetterTest()
+      : certificate_helper_(&platform_keys_service_) {}
+  CertProvisioningCertsWithIdsGetterTest(
+      const CertProvisioningCertsWithIdsGetterTest&) = delete;
+  CertProvisioningCertsWithIdsGetterTest& operator=(
+      const CertProvisioningCertsWithIdsGetterTest&) = delete;
+  ~CertProvisioningCertsWithIdsGetterTest() override = default;
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  ProfileHelperForTesting profile_helper_;
+  platform_keys::MockPlatformKeysService platform_keys_service_;
+  CertificateHelperForTesting certificate_helper_;
+};
+
+TEST_F(CertProvisioningCertsWithIdsGetterTest, NoCertificates) {
+  CertScope cert_scope = CertScope::kDevice;
+
+  CertProvisioningCertsWithIdsGetter cert_getter;
+
+  CallbackObserver callback_observer;
+  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+                              callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_TRUE(callback_observer.GetMap().empty());
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+TEST_F(CertProvisioningCertsWithIdsGetterTest, SingleCertificateWithId) {
+  CertScope cert_scope = CertScope::kDevice;
+  const char kCertProfileId[] = "cert_profile_id_1";
+
+  certificate_helper_.AddCert(cert_scope, kCertProfileId);
+
+  CertProvisioningCertsWithIdsGetter cert_getter;
+
+  CallbackObserver callback_observer;
+  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+                              callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAre(kCertProfileId));
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+TEST_F(CertProvisioningCertsWithIdsGetterTest, ManyCertificatesWithId) {
+  CertScope cert_scope = CertScope::kDevice;
+  std::vector<std::string> ids{"cert_profile_id_0", "cert_profile_id_1",
+                               "cert_profile_id_2"};
+
+  for (const auto& id : ids) {
+    certificate_helper_.AddCert(cert_scope, id);
+  }
+
+  CertProvisioningCertsWithIdsGetter cert_getter;
+
+  CallbackObserver callback_observer;
+  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+                              callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAreArray(ids));
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+TEST_F(CertProvisioningCertsWithIdsGetterTest, ManyCertificatesWithoutId) {
+  CertScope cert_scope = CertScope::kDevice;
+  size_t cert_count = 4;
+  std::vector<std::string> ids{"cert_profile_id_0", "cert_profile_id_1",
+                               "cert_profile_id_2"};
+  for (size_t i = 0; i < cert_count; ++i) {
+    certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"",
+                                /*error_message=*/"no id");
+  }
+
+  CertProvisioningCertsWithIdsGetter cert_getter;
+
+  CallbackObserver callback_observer;
+  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+                              callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_TRUE(callback_observer.GetMap().empty());
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+TEST_F(CertProvisioningCertsWithIdsGetterTest, CertificatesWithAndWithoutIds) {
+  CertScope cert_scope = CertScope::kDevice;
+
+  size_t cert_without_id_count = 4;
+  for (size_t i = 0; i < cert_without_id_count; ++i) {
+    certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"",
+                                /*error_message=*/"no id");
+  }
+
+  std::vector<std::string> ids{"cert_profile_id_0", "cert_profile_id_1",
+                               "cert_profile_id_2"};
+  for (const auto& id : ids) {
+    certificate_helper_.AddCert(cert_scope, id);
+  }
+
+  CertProvisioningCertsWithIdsGetter cert_getter;
+
+  CallbackObserver callback_observer;
+  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+                              callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAreArray(ids));
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+}  // namespace
+}  // namespace cert_provisioning
+}  // namespace chromeos

chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.cc

@@ -4,13 +4,95 @@
 
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.h"
 
+#include "base/test/gmock_callback_support.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "net/test/cert_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::test::RunOnceCallback;
+using testing::_;
+using testing::Invoke;
+
 namespace chromeos {
 namespace cert_provisioning {
 
+//================ CertificateHelperForTesting =================================
+
+// Generated by chrome/test/data/policy/test_certs/create_test_certs.sh
+const char kFakeCertificate[] = R"(-----BEGIN CERTIFICATE-----
+MIIDJzCCAg+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAXMRUwEwYDVQQDDAxyb290
+X2NhX2NlcnQwHhcNMjAwMjI1MTUyNTU2WhcNMzAwMjIyMTUyNTU2WjAUMRIwEAYD
+VQQDDAkxMjcuMC4wLjEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDW
+druvpaJovmyWzIcjtsSk/lp319+zNPSYGLzJzTeEmnFoDf/b89ft6xR1NIahmvVd
+UHGOMlzgDKnNkqWw+pgpn6U8dk+leWnwlUefzDz7OY8qXfX29Vh0m/kATQc64lnp
+rX19fEi2DOgH6heCQDSaHI/KAnAXccwl8kdGuTEnvdzbdHqQq8pPGpEqzC/NOjk7
+kDNkUt0J74ZVMm4+jhVOgZ35mFLtC+xjfycBgbnt8yfPOzmOMwXTjYDPNaIy32AZ
+t66oIToteoW5Ilg+j5Mto3unBDHrw8rml3+W/nwHuOPEIgBqLQFfWtXpuX8CbcS6
+SFNK4hxCJOvlzUbgTpsrAgMBAAGjgYAwfjAMBgNVHRMBAf8EAjAAMB0GA1UdDgQW
+BBRDEl1/2pL5LtKnpIly+XCj3N6MwDAfBgNVHSMEGDAWgBQrwVEnUQZlX850A2N+
+URfS8BxoyzAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwDwYDVR0RBAgw
+BocEfwAAATANBgkqhkiG9w0BAQsFAAOCAQEAXZd+Ul7GUFZPLSiTZ618hUI2UdO0
+7rtPwBw3TephWuyEeHht+WhzA3sRL3nprEiJqIg5w/Tlfz4dsObpSU3vKmDhLzAx
+HJrN5vKdbEj9wyuhYSRJwvJka1ZOgPzhQcDQOp1SqonNxLx/sSMDR2UIDMBGzrkQ
+sDkn58N5eWm+hZADOAKROHR47j85VcsmYGK7z2x479YzsyWyOm0dbACXv7/HvFkz
+56KvgxRaPZQzQUg5yuXa21IjQz07wyWSYnHpm2duAbYFl6CTR9Rlj5vpRkKsQP1W
+mMhGDBfgEskdbM+0agsZrJupoQMBUbD5gflcJlW3kwlboi3dTtiGixfYWw==
+-----END CERTIFICATE-----)";
+
+CertificateHelperForTesting::CertificateHelperForTesting(
+    platform_keys::MockPlatformKeysService* platform_keys_service)
+    : platform_keys_service_(platform_keys_service) {
+  template_cert_ = CreateSingleCertificateFromBytes(kFakeCertificate,
+                                                    sizeof(kFakeCertificate));
+  DCHECK(template_cert_);
+  EXPECT_CALL(*platform_keys_service_, GetCertificates)
+      .WillRepeatedly(
+          Invoke(this, &CertificateHelperForTesting::GetCertificates));
+}
+
+CertificateHelperForTesting::~CertificateHelperForTesting() = default;
+
+void CertificateHelperForTesting::GetCertificates(
+    const std::string& token_id,
+    const platform_keys::GetCertificatesCallback& callback) {
+  auto result = std::make_unique<net::CertificateList>();
+  *result = cert_list_;
+  std::move(callback).Run(std::move(result), "");
+}
+
+void CertificateHelperForTesting::AddCert(
+    CertScope cert_scope,
+    const CertProfileId& cert_profile_id) {
+  AddCert(cert_scope, cert_profile_id, /*error_message=*/"");
+}
+
+void CertificateHelperForTesting::AddCert(CertScope cert_scope,
+                                          const CertProfileId& cert_profile_id,
+                                          const std::string& error_message) {
+  net::CertBuilder cert_builder(template_cert_->cert_buffer(),
+                                /*issuer=*/nullptr);
+  auto cert = cert_builder.GetX509Certificate();
+
+  EXPECT_CALL(
+      *platform_keys_service_,
+      GetAttributeForKey(
+          GetPlatformKeysTokenId(cert_scope),
+          platform_keys::GetSubjectPublicKeyInfo(cert),
+          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
+      .WillRepeatedly(RunOnceCallback<3>(cert_profile_id, error_message));
+
+  cert_list_.push_back(cert);
+}
+
+void CertificateHelperForTesting::ClearCerts() {
+  cert_list_.clear();
+}
+
+const net::CertificateList& CertificateHelperForTesting::GetCerts() const {
+  return cert_list_;
+}
+
 //================ ProfileHelperForTesting =====================================
 
 namespace {

chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_CHROMEOS_CERT_PROVISIONING_CERT_PROVISIONING_TEST_HELPERS_H_
 #define CHROME_BROWSER_CHROMEOS_CERT_PROVISIONING_CERT_PROVISIONING_TEST_HELPERS_H_
 
+#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
+#include "chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/dbus/cryptohome/fake_cryptohome_client.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -13,6 +15,32 @@
 namespace chromeos {
 namespace cert_provisioning {
 
+//================ CertificateHelperForTesting =================================
+
+// Redirects PlatformKeysService::GetCertificate calls to itself. Allows to add
+// certificate to a fake storage with assigned CertProfileId-s.
+struct CertificateHelperForTesting {
+ public:
+  explicit CertificateHelperForTesting(
+      platform_keys::MockPlatformKeysService* platform_keys_service);
+  ~CertificateHelperForTesting();
+
+  void AddCert(CertScope cert_scope, const CertProfileId& cert_profile_id);
+  void AddCert(CertScope cert_scope,
+               const CertProfileId& cert_profile_id,
+               const std::string& error_message);
+  void ClearCerts();
+  const net::CertificateList& GetCerts() const;
+
+ private:
+  void GetCertificates(const std::string& token_id,
+                       const platform_keys::GetCertificatesCallback& callback);
+
+  platform_keys::MockPlatformKeysService* platform_keys_service_ = nullptr;
+  scoped_refptr<net::X509Certificate> template_cert_;
+  net::CertificateList cert_list_;
+};
+
 //================ ProfileHelperForTesting =====================================
 
 class ProfileHelperForTesting {