Cert Provisioning: Certificate renewal

Start using renewal_period_seconds field from cert provisioning
policies. Track expiration date in CertProvisioningScheduler,
provision a new certificate when necessary (according to policy),
delete renewed certificates.

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

chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.cc

@@ -7,6 +7,7 @@
 #include "base/bind_helpers.h"
 #include "base/notreached.h"
 #include "base/optional.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/common/pref_names.h"
@@ -53,35 +54,47 @@ bool IsFinalState(CertProvisioningWorkerState state) {
 
 //===================== CertProfile ============================================
 
+CertProfile::CertProfile(CertProfileId profile_id,
+                         std::string policy_version,
+                         bool is_va_enabled,
+                         base::TimeDelta renewal_period)
+    : profile_id(profile_id),
+      policy_version(policy_version),
+      is_va_enabled(is_va_enabled),
+      renewal_period(renewal_period) {}
+
 base::Optional<CertProfile> CertProfile::MakeFromValue(
     const base::Value& value) {
-  static_assert(kVersion == 3, "This function should be updated");
+  static_assert(kVersion == 4, "This function should be updated");
 
   const std::string* id = value.FindStringKey(kCertProfileIdKey);
   const std::string* policy_version =
       value.FindStringKey(kCertProfilePolicyVersionKey);
   base::Optional<bool> is_va_enabled =
       value.FindBoolKey(kCertProfileIsVaEnabledKey);
+  base::Optional<int> renewal_period_sec =
+      value.FindIntKey(kCertProfileRenewalPeroidSec);
+
   if (!id || !policy_version) {
     return base::nullopt;
   }
-  if (!is_va_enabled) {
-    is_va_enabled = true;
-  }
 
   CertProfile result;
   result.profile_id = *id;
   result.policy_version = *policy_version;
-  result.is_va_enabled = *is_va_enabled;
+  result.is_va_enabled = is_va_enabled.value_or(true);
+  result.renewal_period =
+      base::TimeDelta::FromSeconds(renewal_period_sec.value_or(0));
 
   return result;
 }
 
 bool CertProfile::operator==(const CertProfile& other) const {
-  static_assert(kVersion == 3, "This function should be updated");
+  static_assert(kVersion == 4, "This function should be updated");
   return ((profile_id == other.profile_id) &&
           (policy_version == other.policy_version) &&
-          (is_va_enabled == other.is_va_enabled));
+          (is_va_enabled == other.is_va_enabled) &&
+          (renewal_period == other.renewal_period));
 }
 
 bool CertProfile::operator!=(const CertProfile& other) const {
@@ -90,10 +103,11 @@ bool CertProfile::operator!=(const CertProfile& other) const {
 
 bool CertProfileComparator::operator()(const CertProfile& a,
                                        const CertProfile& b) const {
-  static_assert(CertProfile::kVersion == 3, "This function should be updated");
+  static_assert(CertProfile::kVersion == 4, "This function should be updated");
   return ((a.profile_id < b.profile_id) ||
           (a.policy_version < b.policy_version) ||
-          (a.is_va_enabled < b.is_va_enabled));
+          (a.is_va_enabled < b.is_va_enabled) ||
+          (a.renewal_period < b.renewal_period));
 }
 
 //==============================================================================
@@ -109,6 +123,15 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
       prefs::kCertificateProvisioningStateForDevice);
 }
 
+const char* GetPrefNameForCertProfiles(CertScope scope) {
+  switch (scope) {
+    case CertScope::kUser:
+      return prefs::kRequiredClientCertificateForUser;
+    case CertScope::kDevice:
+      return prefs::kRequiredClientCertificateForDevice;
+  }
+}
+
 const char* GetPrefNameForSerialization(CertScope scope) {
   switch (scope) {
     case CertScope::kUser:

chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/callback_forward.h"
 #include "base/optional.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chromeos/dbus/constants/attestation_constants.h"
@@ -66,20 +67,32 @@ using CertProfileId = std::string;
 // with definitions of RequiredClientCertificateForDevice and
 // RequiredClientCertificateForUser policies in policy_templates.json file.
 const char kCertProfileIdKey[] = "cert_profile_id";
+const char kCertProfileRenewalPeroidSec[] = "renewal_period_seconds";
 const char kCertProfilePolicyVersionKey[] = "policy_version";
 const char kCertProfileIsVaEnabledKey[] = "enable_remote_attestation_check";
 
 struct CertProfile {
+  static base::Optional<CertProfile> MakeFromValue(const base::Value& value);
+
+  CertProfile() = default;
+  // For tests.
+  CertProfile(CertProfileId profile_id,
+              std::string policy_version,
+              bool is_va_enabled,
+              base::TimeDelta renewal_period);
+
   CertProfileId profile_id;
   std::string policy_version;
   bool is_va_enabled = true;
+  // Default renewal period 0 means that a certificate will be renewed only
+  // after the previous one has expired (0 seconds before it is expires).
+  base::TimeDelta renewal_period = base::TimeDelta::FromSeconds(0);
 
   // IMPORTANT:
   // Increment this when you add/change any member in CertProfile (and update
   // all functions that fail to compile because of it).
-  static constexpr int kVersion = 3;
+  static constexpr int kVersion = 4;
 
-  static base::Optional<CertProfile> MakeFromValue(const base::Value& value);
   bool operator==(const CertProfile& other) const;
   bool operator!=(const CertProfile& other) const;
 };
@@ -90,6 +103,7 @@ struct CertProfileComparator {
 
 void RegisterProfilePrefs(PrefRegistrySimple* registry);
 void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
+const char* GetPrefNameForCertProfiles(CertScope scope);
 const char* GetPrefNameForSerialization(CertScope scope);
 
 // Returns the nickname (CKA_LABEL) for keys created for the |profile_id|.

chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.cc

@@ -17,49 +17,49 @@
 namespace chromeos {
 namespace cert_provisioning {
 
-// ========= CertProvisioningCertsWithIdsGetter ================================
+// ========= CertIterator ======================================================
 
-CertProvisioningCertsWithIdsGetter::CertProvisioningCertsWithIdsGetter() =
-    default;
-
-CertProvisioningCertsWithIdsGetter::~CertProvisioningCertsWithIdsGetter() =
-    default;
-
-bool CertProvisioningCertsWithIdsGetter::IsRunning() const {
-  return !callback_.is_null();
-}
-
-void CertProvisioningCertsWithIdsGetter::GetCertsWithIds(
+CertIterator::CertIterator(
     CertScope cert_scope,
-    platform_keys::PlatformKeysService* platform_keys_service,
-    GetCertsWithIdsCallback callback) {
+    platform_keys::PlatformKeysService* platform_keys_service)
+    : cert_scope_(cert_scope), platform_keys_service_(platform_keys_service) {}
+CertIterator::~CertIterator() = default;
+
+void CertIterator::IterateAll(
+    CertIteratorForEachCallback for_each_callback,
+    CertIteratorOnFinishedCallback on_finished_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(platform_keys_service);
-  DCHECK(callback_.is_null());
 
-  cert_scope_ = cert_scope;
-  platform_keys_service_ = platform_keys_service;
-  callback_ = std::move(callback);
+  Cancel();
+  for_each_callback_ = std::move(for_each_callback);
+  on_finished_callback_ = std::move(on_finished_callback);
 
   platform_keys_service_->GetCertificates(
       GetPlatformKeysTokenId(cert_scope_),
-      base::BindRepeating(
-          &CertProvisioningCertsWithIdsGetter::OnGetCertificatesDone,
+      base::BindRepeating(&CertIterator::OnGetCertificatesDone,
                           weak_factory_.GetWeakPtr()));
 }
 
-void CertProvisioningCertsWithIdsGetter::OnGetCertificatesDone(
+void CertIterator::Cancel() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  weak_factory_.InvalidateWeakPtrs();
+  for_each_callback_.Reset();
+  on_finished_callback_.Reset();
+}
+
+void CertIterator::OnGetCertificatesDone(
     std::unique_ptr<net::CertificateList> existing_certs,
     const std::string& error_message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (!error_message.empty()) {
-    std::move(callback_).Run(/*existing_cert_ids=*/{}, error_message);
+    StopIteration(error_message);
     return;
   }
 
+  // No work to do, return empty error message.
   if (!existing_certs || existing_certs->empty()) {
-    std::move(callback_).Run(/*existing_cert_ids=*/{}, /*error_message=*/"");
+    StopIteration(error_message);
     return;
   }
 
@@ -70,136 +70,238 @@ void CertProvisioningCertsWithIdsGetter::OnGetCertificatesDone(
     platform_keys_service_->GetAttributeForKey(
         GetPlatformKeysTokenId(cert_scope_), public_key,
         platform_keys::KeyAttributeType::CertificateProvisioningId,
-        base::BindOnce(&CertProvisioningCertsWithIdsGetter::CollectOneResult,
+        base::BindOnce(&CertIterator::OnGetAttributeForKeyDone,
                        weak_factory_.GetWeakPtr(), cert));
   }
 }
 
-void CertProvisioningCertsWithIdsGetter::CollectOneResult(
+void CertIterator::OnGetAttributeForKeyDone(
     scoped_refptr<net::X509Certificate> cert,
-    const base::Optional<CertProfileId>& cert_id,
+    const base::Optional<std::string>& attr_value,
     const std::string& error_message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(wait_counter_ > 0);
 
-  // |callback_| could have been already used in case of an error in a parallel
-  // task.
-  if (callback_.is_null()) {
-    return;
-  }
-
   // TODO(crbug.com/1073512): Currently if GetAttributeForKey fails to get the
   // attribute (because it was not set or any other reason), it will return
-  // nullopt for cert_id and empty error message. When PlatformKeysService
-  // switches to error codes, a code for such situation should not be returned
-  // via callback and cert collection can be continued.
+  // nullopt for cert_profile_id and empty error message. When
+  // PlatformKeysService switches to error codes, a code for such situation
+  // should not be returned via callback and cert collection can be continued.
   if (!error_message.empty()) {
-    std::move(callback_).Run(/*existing_cert_ids=*/{}, error_message);
+    StopIteration(error_message);
     return;
   }
 
-  if (cert_id) {
-    certs_with_ids_[cert_id.value()] = cert;
+  if (attr_value) {
+    for_each_callback_.Run(cert, attr_value.value(), /*error_message=*/"");
   }
 
   --wait_counter_;
-  if (wait_counter_ != 0) {
+  if (wait_counter_ == 0) {
+    StopIteration(/*error_message=*/"");
+  }
+}
+
+void CertIterator::StopIteration(const std::string& error_message) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!on_finished_callback_.is_null());
+
+  weak_factory_.InvalidateWeakPtrs();
+  std::move(on_finished_callback_).Run(error_message);
+}
+
+// ========= LatestCertsWithIdsGetter ==========================================
+
+LatestCertsWithIdsGetter::LatestCertsWithIdsGetter(
+    CertScope cert_scope,
+    platform_keys::PlatformKeysService* platform_keys_service)
+    : iterator_(cert_scope, platform_keys_service) {}
+
+LatestCertsWithIdsGetter::~LatestCertsWithIdsGetter() = default;
+
+void LatestCertsWithIdsGetter::GetCertsWithIds(
+    LatestCertsWithIdsGetterCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  Cancel();
+  callback_ = std::move(callback);
+
+  iterator_.IterateAll(
+      base::BindRepeating(&LatestCertsWithIdsGetter::ProcessOneCert,
+                          weak_factory_.GetWeakPtr()),
+      base::BindOnce(&LatestCertsWithIdsGetter::OnIterationFinished,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void LatestCertsWithIdsGetter::Cancel() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  weak_factory_.InvalidateWeakPtrs();
+  callback_.Reset();
+}
+
+bool LatestCertsWithIdsGetter::IsRunning() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return !callback_.is_null();
+}
+
+void LatestCertsWithIdsGetter::ProcessOneCert(
+    scoped_refptr<net::X509Certificate> new_cert,
+    const CertProfileId& cert_profile_id,
+    const std::string& error_message) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!error_message.empty()) {
+    OnIterationFinished(error_message);
+    return;
+  }
+
+  auto cert_iter = certs_with_ids_.find(cert_profile_id);
+  if (cert_iter == certs_with_ids_.end()) {
+    certs_with_ids_[cert_profile_id] = new_cert;
     return;
   }
 
-  // Move the result into the callback argument to allow deleting this object
-  // before using the result.
-  std::move(callback_).Run(std::move(certs_with_ids_), /*error_message=*/"");
+  const auto& existing_cert = cert_iter->second;
+  if (existing_cert->valid_expiry() < new_cert->valid_expiry()) {
+    cert_iter->second = new_cert;
+    return;
+  }
 }
 
-// ========= CertProvisioningCertDeleter =======================================
+void LatestCertsWithIdsGetter::OnIterationFinished(
+    const std::string& error_message) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!callback_.is_null());
 
-CertProvisioningCertDeleter::CertProvisioningCertDeleter() = default;
-CertProvisioningCertDeleter::~CertProvisioningCertDeleter() = default;
+  weak_factory_.InvalidateWeakPtrs();
 
-void CertProvisioningCertDeleter::DeleteCerts(
+  if (!error_message.empty()) {
+    certs_with_ids_ = {};
+  }
+
+  std::move(callback_).Run(std::move(certs_with_ids_), error_message);
+}
+
+// ========= CertDeleter =======================================================
+
+CertDeleter::CertDeleter(
     CertScope cert_scope,
-    platform_keys::PlatformKeysService* platform_keys_service,
+    platform_keys::PlatformKeysService* platform_keys_service)
+    : cert_scope_(cert_scope),
+      platform_keys_service_(platform_keys_service),
+      iterator_(cert_scope, platform_keys_service) {}
+
+CertDeleter::~CertDeleter() = default;
+
+void CertDeleter::DeleteCerts(
     base::flat_set<CertProfileId> cert_profile_ids_to_keep,
-    DeleteCertsCallback callback) {
+    CertDeleterCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(platform_keys_service);
-  DCHECK(callback_.is_null());
-
-  cert_scope_ = cert_scope;
-  platform_keys_service_ = platform_keys_service;
+  Cancel();
   callback_ = std::move(callback);
   cert_profile_ids_to_keep_ = std::move(cert_profile_ids_to_keep);
 
-  cert_getter_ = std::make_unique<CertProvisioningCertsWithIdsGetter>();
-  cert_getter_->GetCertsWithIds(
-      cert_scope, platform_keys_service,
-      base::BindOnce(&CertProvisioningCertDeleter::OnGetCertsWithIdsDone,
+  iterator_.IterateAll(base::BindRepeating(&CertDeleter::ProcessOneCert,
+                                           weak_factory_.GetWeakPtr()),
+                       base::BindOnce(&CertDeleter::OnIterationFinished,
                                       weak_factory_.GetWeakPtr()));
 }
 
-void CertProvisioningCertDeleter::OnGetCertsWithIdsDone(
-    base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
-        certs_with_ids,
+void CertDeleter::Cancel() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  weak_factory_.InvalidateWeakPtrs();
+  iteration_finished_ = false;
+  pending_delete_tasks_counter_ = 0;
+  callback_.Reset();
+  certs_with_ids_.clear();
+}
+
+void CertDeleter::ProcessOneCert(scoped_refptr<net::X509Certificate> cert,
+                                 const CertProfileId& cert_profile_id,
                                  const std::string& error_message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (!error_message.empty()) {
-    std::move(callback_).Run(error_message);
+    ReturnStatus(error_message);
     return;
   }
 
-  if (certs_with_ids.empty()) {
-    std::move(callback_).Run(/*error_message=*/"");
+  RememberOrDelete(cert, cert_profile_id);
+}
+
+void CertDeleter::RememberOrDelete(scoped_refptr<net::X509Certificate> new_cert,
+                                   const CertProfileId& cert_profile_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if ((!base::Contains(cert_profile_ids_to_keep_, cert_profile_id)) ||
+      (base::Time::Now() > new_cert->valid_expiry())) {
+    DeleteCert(new_cert);
     return;
   }
 
-  wait_counter_ = certs_with_ids.size();
+  auto cert_iter = certs_with_ids_.find(cert_profile_id);
+  if (cert_iter == certs_with_ids_.end()) {
+    certs_with_ids_[cert_profile_id] = new_cert;
+    return;
+  }
 
-  for (const auto& kv : certs_with_ids) {
-    const CertProfileId& cert_id = kv.first;
-    if (base::Contains(cert_profile_ids_to_keep_, cert_id)) {
-      AccountOneResult();
-      continue;
+  // Keep only the newest certificate.
+  const auto& existing_cert = cert_iter->second;
+  if (existing_cert->valid_expiry() < new_cert->valid_expiry()) {
+    DeleteCert(existing_cert);
+    cert_iter->second = new_cert;
+    return;
+  } else {
+    DeleteCert(new_cert);
+    return;
   }
+}
+
+void CertDeleter::DeleteCert(scoped_refptr<net::X509Certificate> cert) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-    const scoped_refptr<net::X509Certificate>& cert = kv.second;
+  ++pending_delete_tasks_counter_;
   platform_keys_service_->RemoveCertificate(
       GetPlatformKeysTokenId(cert_scope_), cert,
-        base::BindRepeating(
-            &CertProvisioningCertDeleter::OnRemoveCertificateDone,
+      base::BindRepeating(&CertDeleter::OnDeleteCertDone,
                           weak_factory_.GetWeakPtr()));
-  }
 }
 
-void CertProvisioningCertDeleter::OnRemoveCertificateDone(
-    const std::string& error_message) {
+void CertDeleter::OnDeleteCertDone(const std::string& error_message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(pending_delete_tasks_counter_ > 0);
 
   if (!error_message.empty()) {
-    std::move(callback_).Run(error_message);
+    ReturnStatus(error_message);
     return;
   }
 
-  AccountOneResult();
+  --pending_delete_tasks_counter_;
+  CheckStateAndMaybeFinish();
 }
 
-void CertProvisioningCertDeleter::AccountOneResult() {
+void CertDeleter::OnIterationFinished(const std::string& error_message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(wait_counter_ > 0);
 
-  // |callback_| could have been already used in case of an error in a parallel
-  // task.
-  if (callback_.is_null()) {
-    return;
-  }
+  iteration_finished_ = true;
+  CheckStateAndMaybeFinish();
+}
 
-  --wait_counter_;
-  if (wait_counter_ != 0) {
+void CertDeleter::CheckStateAndMaybeFinish() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!iteration_finished_ || (pending_delete_tasks_counter_ > 0)) {
     return;
   }
 
-  std::move(callback_).Run(/*error_message=*/"");
+  ReturnStatus(/*error_message=*/"");
+}
+
+void CertDeleter::ReturnStatus(const std::string& error_message) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!callback_.is_null());
+
+  weak_factory_.InvalidateWeakPtrs();
+  std::move(callback_).Run(error_message);
 }
 
 }  // namespace cert_provisioning

chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.h

@@ -22,96 +22,150 @@ class PlatformKeysService;
 namespace chromeos {
 namespace cert_provisioning {
 
-// ========= CertProvisioningCertsWithIdsGetter ================================
+// ========= CertIterator ======================================================
 
-using GetCertsWithIdsCallback = base::OnceCallback<void(
-    base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
-        certs_with_ids,
+using CertIteratorForEachCallback =
+    base::RepeatingCallback<void(scoped_refptr<net::X509Certificate> cert,
+                                 const CertProfileId& cert_profile_id,
                                  const std::string& error_message)>;
+using CertIteratorOnFinishedCallback =
+    base::OnceCallback<void(const std::string& error_message)>;
 
-// Helper class that retrieves list of all certificates in a given scope with
-// their certificate profile ids. Certificates without the id are ignored.
-class CertProvisioningCertsWithIdsGetter {
+// Iterates over all existing certificates of a given |cert_scope| and combines
+// them with their certificate provisioning ids when possible. Runs |callback|
+// on every (cert, cert_profile_id) pair that had a present and non-empty
+// |cert_profile_id|. If |error_message| is not empty, then the pair is not
+// valid.
+class CertIterator {
  public:
-  CertProvisioningCertsWithIdsGetter();
-  CertProvisioningCertsWithIdsGetter(
-      const CertProvisioningCertsWithIdsGetter&) = delete;
-  CertProvisioningCertsWithIdsGetter& operator=(
-      const CertProvisioningCertsWithIdsGetter&) = delete;
-  ~CertProvisioningCertsWithIdsGetter();
-
-  bool IsRunning() const;
-
-  void GetCertsWithIds(
-      CertScope cert_scope,
-      platform_keys::PlatformKeysService* platform_keys_service,
-      GetCertsWithIdsCallback callback);
+  CertIterator(CertScope cert_scope,
+               platform_keys::PlatformKeysService* platform_keys_service);
+  CertIterator(const CertIterator&) = delete;
+  CertIterator& operator=(const CertIterator&) = delete;
+  ~CertIterator();
+
+  // Can be called more than once. If previous iteration is not finished, it
+  // will be canceled.
+  void IterateAll(CertIteratorForEachCallback for_each_callback,
+                  CertIteratorOnFinishedCallback on_finished_callback);
+  void Cancel();
 
  private:
   void OnGetCertificatesDone(
       std::unique_ptr<net::CertificateList> existing_certs,
       const std::string& error_message);
-
-  void CollectOneResult(scoped_refptr<net::X509Certificate> cert,
-                        const base::Optional<CertProfileId>& cert_id,
+  void OnGetAttributeForKeyDone(scoped_refptr<net::X509Certificate> cert,
+                                const base::Optional<std::string>& attr_value,
                                 const std::string& error_message);
+  void StopIteration(const std::string& error_message);
 
-  CertScope cert_scope_ = CertScope::kDevice;
-  platform_keys::PlatformKeysService* platform_keys_service_ = nullptr;
+  const CertScope cert_scope_ = CertScope::kDevice;
+  platform_keys::PlatformKeysService* const platform_keys_service_ = nullptr;
 
   size_t wait_counter_ = 0;
-  base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
-      certs_with_ids_;
-  GetCertsWithIdsCallback callback_;
+  CertIteratorForEachCallback for_each_callback_;
+  CertIteratorOnFinishedCallback on_finished_callback_;
 
   SEQUENCE_CHECKER(sequence_checker_);
-  base::WeakPtrFactory<CertProvisioningCertsWithIdsGetter> weak_factory_{this};
+  base::WeakPtrFactory<CertIterator> weak_factory_{this};
 };
 
-// ========= CertProvisioningCertDeleter =======================================
+// ========= LatestCertsWithIdsGetter ==========================================
 
-using DeleteCertsCallback =
-    base::OnceCallback<void(const std::string& error_message)>;
+using LatestCertsWithIdsGetterCallback = base::OnceCallback<void(
+    base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
+        certs_with_ids,
+    const std::string& error_message)>;
 
-// Helper class that deletes all certificates in a given scope with certificate
-// profile ids that are not specified to be kept. Certificates without the id
-// are ignored.
-class CertProvisioningCertDeleter {
+// Collects map of certificates with their certificate provisioning ids and
+// returns it via |callback|. If there are several certificates for the same id,
+// only the newest one will be stored in the map. Only one call to
+// GetCertsWithIds() for one instance is allowed.
+class LatestCertsWithIdsGetter {
  public:
-  CertProvisioningCertDeleter();
-  CertProvisioningCertDeleter(const CertProvisioningCertDeleter&) = delete;
-  CertProvisioningCertDeleter& operator=(const CertProvisioningCertDeleter&) =
-      delete;
-  ~CertProvisioningCertDeleter();
-
-  void DeleteCerts(CertScope cert_scope,
-                   platform_keys::PlatformKeysService* platform_keys_service,
-                   base::flat_set<CertProfileId> cert_profile_ids_to_keep,
-                   DeleteCertsCallback callback);
+  LatestCertsWithIdsGetter(
+      CertScope cert_scope,
+      platform_keys::PlatformKeysService* platform_keys_service);
+  LatestCertsWithIdsGetter(const LatestCertsWithIdsGetter&) = delete;
+  LatestCertsWithIdsGetter& operator=(const LatestCertsWithIdsGetter&) = delete;
+  ~LatestCertsWithIdsGetter();
+
+  // Can be called more than once. If previous task is not finished, it will be
+  // canceled.
+  void GetCertsWithIds(LatestCertsWithIdsGetterCallback callback);
+  bool IsRunning() const;
+  void Cancel();
 
  private:
-  void OnGetCertsWithIdsDone(
-      base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
-          certs_with_ids,
+  void ProcessOneCert(scoped_refptr<net::X509Certificate> new_cert,
+                      const CertProfileId& cert_profile_id,
                       const std::string& error_message);
+  void OnIterationFinished(const std::string& error_message);
 
-  void OnRemoveCertificateDone(const std::string& error_message);
+  CertIterator iterator_;
+
+  // Accumulates results that will be returned at the end via |callback_|.
+  base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
+      certs_with_ids_;
+  LatestCertsWithIdsGetterCallback callback_;
 
-  // Keeps track of how many certificates are already processed. Calls the
-  // |callback_| when all work is done.
-  void AccountOneResult();
+  SEQUENCE_CHECKER(sequence_checker_);
+  base::WeakPtrFactory<LatestCertsWithIdsGetter> weak_factory_{this};
+};
 
-  CertScope cert_scope_ = CertScope::kDevice;
-  platform_keys::PlatformKeysService* platform_keys_service_ = nullptr;
+// ========= CertDeleter =======================================================
 
-  size_t wait_counter_ = 0;
+using CertDeleterCallback =
+    base::OnceCallback<void(const std::string& error_message)>;
+
+// Finds and deletes certificates that 1) have ids that are not in
+// |cert_profile_ids_to_keep| set or 2) have another certificate for the same
+// id with later expiration date. Only one call to DeleteCerts() for one
+// instance is allowed.
+class CertDeleter {
+ public:
+  CertDeleter(CertScope cert_scope,
+              platform_keys::PlatformKeysService* platform_keys_service);
+  CertDeleter(const CertDeleter&) = delete;
+  CertDeleter& operator=(const CertDeleter&) = delete;
+  ~CertDeleter();
+  void Cancel();
+
+  // Can be called more than once. If previous task is not finished, it will be
+  // canceled.
+  void DeleteCerts(base::flat_set<CertProfileId> cert_profile_ids_to_keep,
+                   CertDeleterCallback callback);
+
+ private:
+  void ProcessOneCert(scoped_refptr<net::X509Certificate> cert,
+                      const CertProfileId& cert_profile_id,
+                      const std::string& error_message);
+  void RememberOrDelete(scoped_refptr<net::X509Certificate> new_cert,
+                        const CertProfileId& cert_profile_id);
+  void DeleteCert(scoped_refptr<net::X509Certificate> cert);
+  void OnDeleteCertDone(const std::string& error_message);
+  void OnIterationFinished(const std::string& error_message);
+  void CheckStateAndMaybeFinish();
+  void ReturnStatus(const std::string& error_message);
+
+  const CertScope cert_scope_ = CertScope::kDevice;
+  platform_keys::PlatformKeysService* const platform_keys_service_ = nullptr;
+
+  CertIterator iterator_;
+  bool iteration_finished_ = false;
+  size_t pending_delete_tasks_counter_ = 0;
+  CertDeleterCallback callback_;
+
+  // Contains list of currently existing certificate profile ids. Certificates
+  // with ids outside of this set can be deleted.
   base::flat_set<CertProfileId> cert_profile_ids_to_keep_;
-  DeleteCertsCallback callback_;
 
-  std::unique_ptr<CertProvisioningCertsWithIdsGetter> cert_getter_;
+  // Stores previously seen certificates that allows to find duplicates.
+  base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>
+      certs_with_ids_;
 
   SEQUENCE_CHECKER(sequence_checker_);
-  base::WeakPtrFactory<CertProvisioningCertDeleter> weak_factory_{this};
+  base::WeakPtrFactory<CertDeleter> weak_factory_{this};
 };
 
 }  // namespace cert_provisioning

chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers_unittest.cc

@@ -5,10 +5,14 @@
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.h"
 
 #include <memory>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/containers/flat_map.h"
+#include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
+#include "base/test/gmock_callback_support.h"
+#include "base/time/time.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"
@@ -17,30 +21,164 @@
 #include "net/cert/x509_certificate.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::test::RunOnceCallback;
+using ::testing::_;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
+using ::testing::Eq;
+using ::testing::Invoke;
 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);
+class PlatformKeysHelpersTest : public ::testing::Test {
+ public:
+  PlatformKeysHelpersTest() : certificate_helper_(&platform_keys_service_) {}
+  PlatformKeysHelpersTest(const PlatformKeysHelpersTest&) = delete;
+  PlatformKeysHelpersTest& operator=(const PlatformKeysHelpersTest&) = delete;
+  ~PlatformKeysHelpersTest() override = default;
+
+  void RunUntilIdle() { task_environment_.RunUntilIdle(); }
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  ProfileHelperForTesting profile_helper_;
+  platform_keys::MockPlatformKeysService platform_keys_service_;
+  CertificateHelperForTesting certificate_helper_;
+};
+
+//================= CertProvisioningCertIteratorTest ===========================
+
+class CertProvisioningCertIteratorTest : public PlatformKeysHelpersTest {};
+
+class IteratorCallbackObserver {
+ public:
+  CertIteratorForEachCallback GetForEachCallback() {
+    return base::BindRepeating(&IteratorCallbackObserver::ForEachCallback,
+                               base::Unretained(this));
+  }
+
+  CertIteratorOnFinishedCallback GetOnFinishedCallback() {
+    return base::BindOnce(&IteratorCallbackObserver::OnFinishedCallback,
+                          base::Unretained(this));
   }
-  return keys;
+
+  MOCK_METHOD(void,
+              ForEachCallback,
+              (scoped_refptr<net::X509Certificate> cert,
+               const CertProfileId& cert_id,
+               const std::string& error_message));
+
+  MOCK_METHOD(void, OnFinishedCallback, (const std::string& error_message));
+};
+
+TEST_F(CertProvisioningCertIteratorTest, NoCertificates) {
+  const CertScope kCertScope = CertScope::kDevice;
+
+  base::RunLoop run_loop;
+  IteratorCallbackObserver callback_observer;
+
+  EXPECT_CALL(callback_observer, OnFinishedCallback(/*error_message=*/""))
+      .Times(1)
+      .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
+
+  CertIterator cert_iterator(kCertScope, &platform_keys_service_);
+  cert_iterator.IterateAll(callback_observer.GetForEachCallback(),
+                           callback_observer.GetOnFinishedCallback());
+  run_loop.Run();
 }
 
-class CallbackObserver {
- public:
-  using CertMap =
+TEST_F(CertProvisioningCertIteratorTest, OneCertificate) {
+  const CertScope kCertScope = CertScope::kDevice;
+  const char kCertProfileId[] = "cert_profile_id_1";
+  auto cert = certificate_helper_.AddCert(kCertScope, kCertProfileId);
+
+  base::RunLoop run_loop;
+  IteratorCallbackObserver callback_observer;
+
+  {
+    testing::InSequence seq;
+    EXPECT_CALL(callback_observer, ForEachCallback(/*cert=*/cert,
+                                                   /*cert_id=*/kCertProfileId,
+                                                   /*error_message=*/""))
+        .Times(1);
+    EXPECT_CALL(callback_observer, OnFinishedCallback(/*error_message=*/""))
+        .Times(1)
+        .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
+  }
+
+  CertIterator cert_iterator(kCertScope, &platform_keys_service_);
+  cert_iterator.IterateAll(callback_observer.GetForEachCallback(),
+                           callback_observer.GetOnFinishedCallback());
+  run_loop.Run();
+}
+
+TEST_F(CertProvisioningCertIteratorTest, ManyCertificates) {
+  const CertScope kCertScope = CertScope::kDevice;
+  std::vector<std::string> ids = {"id1, ids2, id3, id4"};
+
+  base::RunLoop run_loop;
+  IteratorCallbackObserver callback_observer;
+
+  testing::ExpectationSet expect_set;
+  for (const auto& id : ids) {
+    auto cert = certificate_helper_.AddCert(kCertScope, id);
+    expect_set +=
+        EXPECT_CALL(callback_observer,
+                    ForEachCallback(/*cert=*/cert,
+                                    /*cert_id=*/id, /*error_message=*/""))
+            .Times(1);
+  }
+
+  EXPECT_CALL(callback_observer, OnFinishedCallback(/*error_message=*/""))
+      .Times(1)
+      .After(expect_set)
+      .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
+
+  CertIterator cert_iterator(kCertScope, &platform_keys_service_);
+  cert_iterator.IterateAll(callback_observer.GetForEachCallback(),
+                           callback_observer.GetOnFinishedCallback());
+  run_loop.Run();
+}
+
+TEST_F(CertProvisioningCertIteratorTest, CertificateWithError) {
+  const CertScope kCertScope = CertScope::kDevice;
+  const char kError[] = "test error";
+
+  certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/"id1");
+  certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/"id2");
+  certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/base::nullopt,
+                              /*error_message=*/kError);
+  certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/"id3");
+  certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/"id4");
+
+  base::RunLoop run_loop;
+  IteratorCallbackObserver callback_observer;
+  EXPECT_CALL(callback_observer, OnFinishedCallback(kError))
+      .Times(1)
+      .WillOnce(Invoke(&run_loop, &base::RunLoop::Quit));
+
+  CertIterator cert_iterator(kCertScope, &platform_keys_service_);
+  cert_iterator.IterateAll(callback_observer.GetForEachCallback(),
+                           callback_observer.GetOnFinishedCallback());
+  run_loop.Run();
+}
+
+//================= CertPrivisioningCertGetter =================================
+
+class CertPrivisioningCertGetter : public PlatformKeysHelpersTest {};
+
+using CertMap =
     base::flat_map<CertProfileId, scoped_refptr<net::X509Certificate>>;
 
-  GetCertsWithIdsCallback GetCallback() {
-    return base::BindOnce(&CallbackObserver::Callback, base::Unretained(this));
+class GetterCallbackObserver {
+ public:
+  LatestCertsWithIdsGetterCallback GetCallback() {
+    return base::BindOnce(&GetterCallbackObserver::Callback,
+                          base::Unretained(this));
   }
 
   const CertMap& GetMap() { return cert_map_; }
@@ -60,138 +198,205 @@ class CallbackObserver {
   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;
+TEST_F(CertPrivisioningCertGetter, NoCertificates) {
+  const CertScope kCertScope = CertScope::kDevice;
 
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
-                              callback_observer.GetCallback());
+  GetterCallbackObserver callback_observer;
+  LatestCertsWithIdsGetter cert_getter(kCertScope, &platform_keys_service_);
+  cert_getter.GetCertsWithIds(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;
+TEST_F(CertPrivisioningCertGetter, SingleCertificateWithId) {
+  const CertScope kCertScope = CertScope::kDevice;
   const char kCertProfileId[] = "cert_profile_id_1";
+  CertMap cert_map;
 
-  certificate_helper_.AddCert(cert_scope, kCertProfileId);
-
-  CertProvisioningCertsWithIdsGetter cert_getter;
+  cert_map[kCertProfileId] =
+      certificate_helper_.AddCert(kCertScope, kCertProfileId);
 
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
-                              callback_observer.GetCallback());
+  GetterCallbackObserver callback_observer;
+  LatestCertsWithIdsGetter cert_getter(kCertScope, &platform_keys_service_);
+  cert_getter.GetCertsWithIds(callback_observer.GetCallback());
   callback_observer.WaitForCallback();
 
-  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAre(kCertProfileId));
+  EXPECT_EQ(callback_observer.GetMap(), cert_map);
   EXPECT_TRUE(callback_observer.GetError().empty());
 }
 
-TEST_F(CertProvisioningCertsWithIdsGetterTest, ManyCertificatesWithId) {
-  CertScope cert_scope = CertScope::kDevice;
+TEST_F(CertPrivisioningCertGetter, ManyCertificatesWithId) {
+  const CertScope kCertScope = CertScope::kDevice;
   std::vector<std::string> ids{"cert_profile_id_0", "cert_profile_id_1",
                                "cert_profile_id_2"};
+  CertMap cert_map;
 
   for (const auto& id : ids) {
-    certificate_helper_.AddCert(cert_scope, id);
+    cert_map[id] = certificate_helper_.AddCert(kCertScope, id);
   }
 
-  CertProvisioningCertsWithIdsGetter cert_getter;
-
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
-                              callback_observer.GetCallback());
+  GetterCallbackObserver callback_observer;
+  LatestCertsWithIdsGetter cert_getter(kCertScope, &platform_keys_service_);
+  cert_getter.GetCertsWithIds(callback_observer.GetCallback());
   callback_observer.WaitForCallback();
 
-  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAreArray(ids));
+  EXPECT_EQ(callback_observer.GetMap(), cert_map);
   EXPECT_TRUE(callback_observer.GetError().empty());
 }
 
-TEST_F(CertProvisioningCertsWithIdsGetterTest, ManyCertificatesWithoutId) {
-  CertScope cert_scope = CertScope::kDevice;
+TEST_F(CertPrivisioningCertGetter, ManyCertificatesWithoutId) {
+  const CertScope kCertScope = CertScope::kDevice;
   size_t cert_count = 4;
   for (size_t i = 0; i < cert_count; ++i) {
-    certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/base::nullopt);
+    certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/base::nullopt);
   }
 
-  CertProvisioningCertsWithIdsGetter cert_getter;
-
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
-                              callback_observer.GetCallback());
+  GetterCallbackObserver callback_observer;
+  LatestCertsWithIdsGetter cert_getter(kCertScope, &platform_keys_service_);
+  cert_getter.GetCertsWithIds(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;
+TEST_F(CertPrivisioningCertGetter, CertificatesWithAndWithoutIds) {
+  const CertScope kCertScope = CertScope::kDevice;
+  CertMap cert_map;
 
   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=*/base::nullopt);
+    certificate_helper_.AddCert(kCertScope, /*cert_profile_id=*/base::nullopt);
   }
 
   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);
+    cert_map[id] = certificate_helper_.AddCert(kCertScope, id);
+  }
+
+  GetterCallbackObserver callback_observer;
+  LatestCertsWithIdsGetter cert_getter(kCertScope, &platform_keys_service_);
+  cert_getter.GetCertsWithIds(callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_EQ(callback_observer.GetMap(), cert_map);
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+//================= CertProvisioningCertDeleterTest ============================
+
+class CertProvisioningCertDeleterTest : public PlatformKeysHelpersTest {};
+
+class DeleterCallbackObserver {
+ public:
+  CertDeleterCallback GetCallback() {
+    return base::BindOnce(&DeleterCallbackObserver::Callback,
+                          base::Unretained(this));
+  }
+
+  const std::string GetError() { return error_message_; }
+  void WaitForCallback() { loop_.Run(); }
+
+ protected:
+  void Callback(const std::string& error_message) {
+    error_message_ = error_message;
+    loop_.Quit();
   }
 
-  CertProvisioningCertsWithIdsGetter cert_getter;
+  base::RunLoop loop_;
+  std::string error_message_;
+};
+
+TEST_F(CertProvisioningCertDeleterTest, NoCertificates) {
+  const CertScope kCertScope = CertScope::kDevice;
+  base::flat_set<CertProfileId> cert_ids_to_keep;
 
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+  EXPECT_CALL(platform_keys_service_, RemoveCertificate).Times(0);
+
+  DeleterCallbackObserver callback_observer;
+  CertDeleter cert_deleter(kCertScope, &platform_keys_service_);
+  cert_deleter.DeleteCerts(std::move(cert_ids_to_keep),
                            callback_observer.GetCallback());
   callback_observer.WaitForCallback();
 
-  EXPECT_THAT(GetKeys(callback_observer.GetMap()), ElementsAreArray(ids));
   EXPECT_TRUE(callback_observer.GetError().empty());
 }
 
-TEST_F(CertProvisioningCertsWithIdsGetterTest, CertificateWithError) {
-  CertScope cert_scope = CertScope::kDevice;
-  const char kError[] = "test error";
+TEST_F(CertProvisioningCertDeleterTest, SomeCertsWithoutPolicy) {
+  const CertScope kCertScope = CertScope::kDevice;
+  std::vector<std::string> cert_ids_to_delete{"id1", "id2", "id3"};
+  base::flat_set<CertProfileId> cert_ids_to_keep{"id4", "id5", "id6"};
+
+  for (const auto& id : cert_ids_to_delete) {
+    auto cert = certificate_helper_.AddCert(kCertScope, id);
+    EXPECT_CALL(platform_keys_service_,
+                RemoveCertificate(GetPlatformKeysTokenId(kCertScope), cert,
+                                  /*callback=*/_))
+        .Times(1)
+        .WillOnce(RunOnceCallback<2>(/*error_message=*/""));
+  }
 
-  certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"id1");
-  certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"id2");
-  certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/base::nullopt,
-                              /*error_message=*/kError);
-  certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"id3");
-  certificate_helper_.AddCert(cert_scope, /*cert_profile_id=*/"id4");
+  for (const auto& id : cert_ids_to_keep) {
+    certificate_helper_.AddCert(kCertScope, id);
+  }
+
+  DeleterCallbackObserver callback_observer;
+  CertDeleter cert_deleter(kCertScope, &platform_keys_service_);
+  cert_deleter.DeleteCerts(std::move(cert_ids_to_keep),
+                           callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
 
-  CertProvisioningCertsWithIdsGetter cert_getter;
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
 
-  CallbackObserver callback_observer;
-  cert_getter.GetCertsWithIds(cert_scope, &platform_keys_service_,
+TEST_F(CertProvisioningCertDeleterTest, CertWasRenewed) {
+  const CertScope kCertScope = CertScope::kDevice;
+  const char kRenewedCertId[] = "id1";
+  const char kCertId2[] = "id2";
+
+  base::Time t1 = base::Time::Now();
+  base::Time t2 = t1 + base::TimeDelta::FromDays(30);
+  base::Time t3 = t2 + base::TimeDelta::FromDays(30);
+
+  auto cert = certificate_helper_.AddCert(kCertScope, kRenewedCertId,
+                                          /*error_message=*/"", t1, t2);
+  EXPECT_CALL(platform_keys_service_,
+              RemoveCertificate(GetPlatformKeysTokenId(kCertScope), cert,
+                                /*callback=*/_))
+      .Times(1)
+      .WillOnce(RunOnceCallback<2>(/*error_message=*/""));
+
+  certificate_helper_.AddCert(kCertScope, kRenewedCertId, /*error_message=*/"",
+                              t2, t3);
+  certificate_helper_.AddCert(kCertScope, kCertId2);
+
+  DeleterCallbackObserver callback_observer;
+  CertDeleter cert_deleter(kCertScope, &platform_keys_service_);
+  cert_deleter.DeleteCerts(/*cert_ids_to_keep=*/{kRenewedCertId, kCertId2},
                            callback_observer.GetCallback());
   callback_observer.WaitForCallback();
 
-  EXPECT_TRUE(callback_observer.GetMap().empty());
-  EXPECT_EQ(callback_observer.GetError(), kError);
+  EXPECT_TRUE(callback_observer.GetError().empty());
+}
+
+TEST_F(CertProvisioningCertDeleterTest, PropogateError) {
+  const CertScope kCertScope = CertScope::kDevice;
+  const char kErrorMsg[] = "error 123";
+
+  certificate_helper_.AddCert(kCertScope, "id1");
+  EXPECT_CALL(platform_keys_service_, RemoveCertificate)
+      .WillOnce(RunOnceCallback<2>(kErrorMsg));
+
+  DeleterCallbackObserver callback_observer;
+  CertDeleter cert_deleter(kCertScope, &platform_keys_service_);
+  cert_deleter.DeleteCerts(/*cert_ids_to_keep=*/{},  // Delete all certs.
+                           callback_observer.GetCallback());
+  callback_observer.WaitForCallback();
+
+  EXPECT_EQ(callback_observer.GetError(), kErrorMsg);
 }
 
 }  // namespace

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.cc

@@ -15,6 +15,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/optional.h"
+#include "base/stl_util.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
@@ -103,6 +104,8 @@ CertProvisioningScheduler::CreateUserCertProvisioningScheduler(
   PrefService* pref_service = profile->GetPrefs();
   policy::CloudPolicyClient* cloud_policy_client =
       GetCloudPolicyClientForUser(profile);
+  platform_keys::PlatformKeysService* platform_keys_service =
+      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
   NetworkStateHandler* network_state_handler = GetNetworkStateHandler();
 
   if (!profile || !pref_service || !cloud_policy_client ||
@@ -112,9 +115,8 @@ CertProvisioningScheduler::CreateUserCertProvisioningScheduler(
   }
 
   return std::make_unique<CertProvisioningScheduler>(
-      CertScope::kUser, profile, pref_service,
-      prefs::kRequiredClientCertificateForUser, cloud_policy_client,
-      network_state_handler,
+      CertScope::kUser, profile, pref_service, cloud_policy_client,
+      platform_keys_service, network_state_handler,
       std::make_unique<CertProvisioningUserInvalidatorFactory>(profile));
 }
 
@@ -127,18 +129,19 @@ CertProvisioningScheduler::CreateDeviceCertProvisioningScheduler(
   PrefService* pref_service = g_browser_process->local_state();
   policy::CloudPolicyClient* cloud_policy_client =
       GetCloudPolicyClientForDevice();
+  platform_keys::PlatformKeysService* platform_keys_service =
+      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
   NetworkStateHandler* network_state_handler = GetNetworkStateHandler();
 
   if (!profile || !pref_service || !cloud_policy_client ||
-      !network_state_handler) {
+      !network_state_handler || !platform_keys_service) {
     LOG(ERROR) << "Failed to create device certificate provisioning scheduler";
     return nullptr;
   }
 
   return std::make_unique<CertProvisioningScheduler>(
-      CertScope::kDevice, profile, pref_service,
-      prefs::kRequiredClientCertificateForDevice, cloud_policy_client,
-      network_state_handler,
+      CertScope::kDevice, profile, pref_service, cloud_policy_client,
+      platform_keys_service, network_state_handler,
       std::make_unique<CertProvisioningDeviceInvalidatorFactory>(
           invalidation_service_provider));
 }
@@ -147,26 +150,28 @@ CertProvisioningScheduler::CertProvisioningScheduler(
     CertScope cert_scope,
     Profile* profile,
     PrefService* pref_service,
-    const char* pref_name,
     policy::CloudPolicyClient* cloud_policy_client,
+    platform_keys::PlatformKeysService* platform_keys_service,
     NetworkStateHandler* network_state_handler,
     std::unique_ptr<CertProvisioningInvalidatorFactory> invalidator_factory)
     : cert_scope_(cert_scope),
       profile_(profile),
       pref_service_(pref_service),
-      pref_name_(pref_name),
       cloud_policy_client_(cloud_policy_client),
+      platform_keys_service_(platform_keys_service),
       network_state_handler_(network_state_handler),
+      certs_with_ids_getter_(cert_scope, platform_keys_service),
+      cert_deleter_(cert_scope, platform_keys_service),
       invalidator_factory_(std::move(invalidator_factory)) {
+  CHECK(profile);
   CHECK(pref_service_);
-  CHECK(pref_name_);
   CHECK(cloud_policy_client_);
-  CHECK(profile);
+  CHECK(platform_keys_service_);
+  CHECK(network_state_handler);
   CHECK(invalidator_factory_);
 
-  platform_keys_service_ =
-      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
-  CHECK(platform_keys_service_);
+  pref_name_ = GetPrefNameForCertProfiles(cert_scope);
+  CHECK(pref_name_);
 
   network_state_handler_->AddObserver(this, FROM_HERE);
 
@@ -198,16 +203,31 @@ void CertProvisioningScheduler::ScheduleDailyUpdate() {
       base::TimeDelta::FromDays(1));
 }
 
-void CertProvisioningScheduler::ScheduleRetry(const CertProfile& profile) {
+void CertProvisioningScheduler::ScheduleRetry(const CertProfileId& profile_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&CertProvisioningScheduler::UpdateOneCertImpl,
-                 weak_factory_.GetWeakPtr(), profile.profile_id),
+                 weak_factory_.GetWeakPtr(), profile_id),
       kInconsistentDataErrorRetryDelay);
 }
 
+void CertProvisioningScheduler::ScheduleRenewal(const CertProfileId& profile_id,
+                                                base::TimeDelta delay) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (base::Contains(scheduled_renewals_, profile_id)) {
+    return;
+  }
+
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CertProvisioningScheduler::InitiateRenewal,
+                 weak_factory_.GetWeakPtr(), profile_id),
+      delay);
+}
+
 void CertProvisioningScheduler::InitialUpdateCerts() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -227,9 +247,8 @@ void CertProvisioningScheduler::DeleteCertsWithoutPolicy() {
     cert_profile_ids_to_keep = base::flat_set<CertProfileId>(std::move(ids));
   }
 
-  cert_deleter_ = std::make_unique<CertProvisioningCertDeleter>();
-  cert_deleter_->DeleteCerts(
-      cert_scope_, platform_keys_service_, cert_profile_ids_to_keep,
+  cert_deleter_.DeleteCerts(
+      cert_profile_ids_to_keep,
       base::BindOnce(&CertProvisioningScheduler::OnDeleteCertsWithoutPolicyDone,
                      weak_factory_.GetWeakPtr()));
 }
@@ -238,15 +257,12 @@ void CertProvisioningScheduler::OnDeleteCertsWithoutPolicyDone(
     const std::string& error_message) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  cert_deleter_.reset();
-
   if (!error_message.empty()) {
     LOG(ERROR) << "Failed to delete certificates without policies: "
                << error_message;
   }
 
   DeserializeWorkers();
-
   CleanVaKeysIfIdle();
 }
 
@@ -326,6 +342,12 @@ void CertProvisioningScheduler::OnPrefsChange() {
   UpdateAllCerts();
 }
 
+void CertProvisioningScheduler::InitiateRenewal(
+    const CertProfileId& cert_profile_id) {
+  scheduled_renewals_.erase(cert_profile_id);
+  UpdateOneCertImpl(cert_profile_id);
+}
+
 void CertProvisioningScheduler::UpdateOneCert(
     const CertProfileId& cert_profile_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -336,6 +358,8 @@ void CertProvisioningScheduler::UpdateOneCert(
 
 void CertProvisioningScheduler::UpdateOneCertImpl(
     const CertProfileId& cert_profile_id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   EraseByKey(failed_cert_profiles_, cert_profile_id);
 
   base::Optional<CertProfile> cert_profile = GetOneCertProfile(cert_profile_id);
@@ -351,6 +375,11 @@ void CertProvisioningScheduler::UpdateAllCerts() {
 
   std::vector<CertProfile> profiles = GetCertProfiles();
   CancelWorkersWithoutPolicy(profiles);
+
+  if (profiles.empty()) {
+    return;
+  }
+
   UpdateCertList(std::move(profiles));
 }
 
@@ -362,17 +391,13 @@ void CertProvisioningScheduler::UpdateCertList(
     return;
   }
 
-  if (certs_with_ids_getter_ && certs_with_ids_getter_->IsRunning()) {
+  if (certs_with_ids_getter_.IsRunning()) {
     queued_profiles_to_update_.insert(std::make_move_iterator(profiles.begin()),
                                       std::make_move_iterator(profiles.end()));
     return;
   }
 
-  certs_with_ids_getter_ =
-      std::make_unique<CertProvisioningCertsWithIdsGetter>();
-  certs_with_ids_getter_->GetCertsWithIds(
-      cert_scope_, platform_keys_service_,
-      base::BindOnce(
+  certs_with_ids_getter_.GetCertsWithIds(base::BindOnce(
       &CertProvisioningScheduler::UpdateCertListWithExistingCerts,
       weak_factory_.GetWeakPtr(), std::move(profiles)));
 }
@@ -384,20 +409,38 @@ void CertProvisioningScheduler::UpdateCertListWithExistingCerts(
     const std::string& error_message) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  certs_with_ids_getter_.reset();
-
   if (!error_message.empty()) {
     LOG(ERROR) << "Failed to get existing cert ids: " << error_message;
     return;
   }
 
   for (const auto& profile : profiles) {
-    if (base::Contains(existing_certs_with_ids, profile.profile_id) ||
-        base::Contains(failed_cert_profiles_, profile.profile_id)) {
+    if (base::Contains(failed_cert_profiles_, profile.profile_id)) {
+      continue;
+    }
+
+    auto cert_iter = existing_certs_with_ids.find(profile.profile_id);
+    if (cert_iter == existing_certs_with_ids.end()) {
+      // The certificate does not exists and should be provisioned.
+      ProcessProfile(profile);
       continue;
     }
 
+    const auto& cert = cert_iter->second;
+    base::Time now = base::Time::Now();
+    if ((now + profile.renewal_period) >= cert->valid_expiry()) {
+      // The certificate should be renewed immediately.
       ProcessProfile(profile);
+      continue;
+    }
+
+    if ((now + base::TimeDelta::FromDays(1) + profile.renewal_period) >=
+        cert->valid_expiry()) {
+      // The certificate should be renewed within 1 day.
+      base::Time target_time = cert->valid_expiry() - profile.renewal_period;
+      ScheduleRenewal(profile.profile_id, /*delay=*/target_time - now);
+      continue;
+    }
   }
 
   if (!queued_profiles_to_update_.empty()) {
@@ -470,7 +513,7 @@ void CertProvisioningScheduler::OnProfileFinished(
     case CertProvisioningWorkerState::kInconsistentDataError:
       LOG(WARNING) << "Inconsistent data error for certificate profile: "
                    << profile.profile_id;
-      ScheduleRetry(profile);
+      ScheduleRetry(profile.profile_id);
       break;
     case CertProvisioningWorkerState::kCanceled:
       break;

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.h

@@ -75,8 +75,8 @@ class CertProvisioningScheduler : public NetworkStateHandlerObserver {
       CertScope cert_scope,
       Profile* profile,
       PrefService* pref_service,
-      const char* pref_name,
       policy::CloudPolicyClient* cloud_policy_client,
+      platform_keys::PlatformKeysService* platform_keys_service,
       NetworkStateHandler* network_state_handler,
       std::unique_ptr<CertProvisioningInvalidatorFactory> invalidator_factory);
   ~CertProvisioningScheduler() override;
@@ -99,8 +99,9 @@ class CertProvisioningScheduler : public NetworkStateHandlerObserver {
  private:
   void ScheduleInitialUpdate();
   void ScheduleDailyUpdate();
-  // Posts delayed task to call ProcessProfile.
-  void ScheduleRetry(const CertProfile& profile);
+  // Posts delayed task to call UpdateOneCertImpl.
+  void ScheduleRetry(const CertProfileId& profile_id);
+  void ScheduleRenewal(const CertProfileId& profile_id, base::TimeDelta delay);
 
   void InitialUpdateCerts();
   void DeleteCertsWithoutPolicy();
@@ -110,6 +111,7 @@ class CertProvisioningScheduler : public NetworkStateHandlerObserver {
   void OnCleanVaKeysIfIdleDone(base::Optional<bool> delete_result);
   void RegisterForPrefsChanges();
 
+  void InitiateRenewal(const CertProfileId& cert_profile_id);
   void UpdateOneCertImpl(const CertProfileId& cert_profile_id);
   void UpdateCertList(std::vector<CertProfile> profiles);
   void UpdateCertListWithExistingCerts(
@@ -150,10 +152,14 @@ class CertProvisioningScheduler : public NetworkStateHandlerObserver {
   PrefService* pref_service_ = nullptr;
   const char* pref_name_ = nullptr;
   policy::CloudPolicyClient* cloud_policy_client_ = nullptr;
-  NetworkStateHandler* network_state_handler_ = nullptr;
   platform_keys::PlatformKeysService* platform_keys_service_ = nullptr;
+  NetworkStateHandler* network_state_handler_ = nullptr;
   PrefChangeRegistrar pref_change_registrar_;
   WorkerMap workers_;
+  // Contains cert profile ids that will be renewed before next daily update.
+  // Helps to prevent creation of more than one delayed task for renewal. When
+  // the renewal starts for a profile id, it is removed from the set.
+  base::flat_set<CertProfileId> scheduled_renewals_;
   // Collection of cert profile ids that failed recently. They will not be
   // retried until next |DailyUpdateCerts|. FailedWorkerInfo contains some extra
   // information about the failure. Profiles that failed with
@@ -167,8 +173,8 @@ class CertProvisioningScheduler : public NetworkStateHandlerObserver {
   // run, because an update for them was triggered during the current run.
   CertProfileSet queued_profiles_to_update_;
 
-  std::unique_ptr<CertProvisioningCertsWithIdsGetter> certs_with_ids_getter_;
-  std::unique_ptr<CertProvisioningCertDeleter> cert_deleter_;
+  LatestCertsWithIdsGetter certs_with_ids_getter_;
+  CertDeleter cert_deleter_;
   std::unique_ptr<CertProvisioningInvalidatorFactory> invalidator_factory_;
 
   base::WeakPtrFactory<CertProvisioningScheduler> weak_factory_{this};

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler_unittest.cc

@@ -6,7 +6,6 @@
 
 #include "base/bind.h"
 #include "base/json/json_reader.h"
-#include "base/test/gmock_callback_support.h"
 #include "base/test/values_test_util.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
@@ -23,9 +22,9 @@
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::Time;
 using base::TimeDelta;
 using base::test::ParseJson;
-using base::test::RunOnceCallback;
 using testing::_;
 using testing::AtLeast;
 using testing::ByMove;
@@ -40,62 +39,10 @@ namespace chromeos {
 namespace cert_provisioning {
 namespace {
 
-const char kWifiServiceGuid[] = "wifi_guid";
-
-//================ CertificateTestHelper =======================================
-
-// 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-----)";
-
-struct CertificateTestHelper {
- public:
-  CertificateTestHelper() {
-    cert = CreateSingleCertificateFromBytes(kFakeCertificate,
-                                            sizeof(kFakeCertificate));
-    DCHECK(cert);
-  }
-
-  void GetCertificates(chromeos::platform_keys::TokenId 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 AddCert() {
-    DCHECK(cert_list.empty())
-        << "Current implementation supports only one certificate";
-    cert_list.push_back(cert);
-  }
-
-  scoped_refptr<net::X509Certificate> GetCert() const { return cert; }
-
-  std::string GetPublicKeyForCert() const {
-    return platform_keys::GetSubjectPublicKeyInfo(cert);
-  }
-
- private:
-  scoped_refptr<net::X509Certificate> cert;
-  net::CertificateList cert_list;
-};
+constexpr char kWifiServiceGuid[] = "wifi_guid";
+constexpr char kCertProfileId[] = "cert_profile_id_1";
+constexpr char kCertProfileVersion[] = "cert_profile_version_1";
+constexpr TimeDelta kCertProfileRenewalPeriod = TimeDelta::FromSeconds(0);
 
 //================ CertProvisioningSchedulerTest ===============================
 
@@ -115,28 +62,18 @@ class CertProvisioningSchedulerTest : public testing::Test {
     RegisterProfilePrefs(pref_service_.registry());
     RegisterLocalStatePrefs(pref_service_.registry());
 
-    platform_keys_service_ =
-        static_cast<platform_keys::MockPlatformKeysService*>(
-            platform_keys::PlatformKeysServiceFactory::GetInstance()
-                ->SetTestingFactoryAndUse(
-                    GetProfile(),
-                    base::BindRepeating(
-                        &platform_keys::BuildMockPlatformKeysService)));
-    ASSERT_TRUE(platform_keys_service_);
+    certificate_helper_ =
+        std::make_unique<CertificateHelperForTesting>(&platform_keys_service_);
 
     AddOnlineWifiNetwork();
   }
 
-  void FastForwardBy(base::TimeDelta delta) {
+  void FastForwardBy(TimeDelta delta) {
     task_environment_.FastForwardBy(delta);
   }
 
   void SetUp() override {
     CertProvisioningWorkerFactory::SetFactoryForTesting(&mock_factory_);
-
-    EXPECT_CALL(*platform_keys_service_, GetCertificates)
-        .WillRepeatedly(Invoke(&certificate_helper_,
-                               &CertificateTestHelper::GetCertificates));
   }
 
   void TearDown() override {
@@ -179,19 +116,19 @@ class CertProvisioningSchedulerTest : public testing::Test {
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   ProfileHelperForTesting profile_helper_for_testing_;
-  platform_keys::MockPlatformKeysService* platform_keys_service_ = nullptr;
+  platform_keys::MockPlatformKeysService platform_keys_service_;
+  std::unique_ptr<CertificateHelperForTesting> certificate_helper_;
   StrictMock<SpyingFakeCryptohomeClient> fake_cryptohome_client_;
   TestingPrefServiceSimple pref_service_;
   policy::MockCloudPolicyClient cloud_policy_client_;
   // Only expected creations are allowed.
   StrictMock<MockCertProvisioningWorkerFactory> mock_factory_;
-  CertificateTestHelper certificate_helper_;
   NetworkStateTestHelper network_state_test_helper_;
   std::string wifi_network_service_path_;
 };
 
 TEST_F(CertProvisioningSchedulerTest, Success) {
-  CertScope cert_scope = CertScope::kUser;
+  const CertScope kCertScope = CertScope::kUser;
 
   auto mock_invalidation_factory_obj =
       std::make_unique<MockCertProvisioningInvalidatorFactory>();
@@ -199,8 +136,8 @@ TEST_F(CertProvisioningSchedulerTest, Success) {
       mock_invalidation_factory_obj.get();
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForUser, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       std::move(mock_invalidation_factory_obj));
 
@@ -211,7 +148,7 @@ TEST_F(CertProvisioningSchedulerTest, Success) {
       .Times(1);
 
   // The policy is empty, so no workers should be created yet.
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
 
   EXPECT_CALL(*mock_invalidation_factory, Create)
@@ -220,23 +157,21 @@ TEST_F(CertProvisioningSchedulerTest, Success) {
           Return(ByMove(nullptr)));  // nullptr is good enough for mock worker.
 
   // One worker will be created on prefs update.
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1),
                           /*is_waiting=*/false, cert_profile);
 
-  // Add 1 certificate profile to the policy ("cert_profile_id" is the same as
-  // above).
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForUser, config);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
 
@@ -248,31 +183,22 @@ TEST_F(CertProvisioningSchedulerTest, Success) {
   EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
   EXPECT_TRUE(scheduler.GetFailedCertProfileIds().empty());
 
-  certificate_helper_.AddCert();
-
-  EXPECT_CALL(
-      *platform_keys_service_,
-      GetAttributeForKey(
-          GetPlatformKeysTokenId(cert_scope),
-          certificate_helper_.GetPublicKeyForCert(),
-          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId, ""));
+  certificate_helper_->AddCert(kCertScope, kCertProfileId);
 
   // Check one more time that scheduler doesn't create new workers for
   // finished certificate profiles (the factory will fail on an attempt to
   // do so).
   scheduler.UpdateAllCerts();
 
-  FastForwardBy(base::TimeDelta::FromSeconds(100));
+  FastForwardBy(TimeDelta::FromSeconds(100));
 }
 
 TEST_F(CertProvisioningSchedulerTest, WorkerFailed) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -283,27 +209,25 @@ TEST_F(CertProvisioningSchedulerTest, WorkerFailed) {
       .Times(1);
 
   // The policy is empty, so no workers should be created yet.
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
 
   // One worker will be created on prefs update.
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1),
                           /*is_waiting=*/false, cert_profile);
 
-  // Add 1 certificate profile to the policy ("cert_profile_id" is the same as
-  // above).
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   // Now 1 worker should be created.
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
@@ -318,16 +242,7 @@ TEST_F(CertProvisioningSchedulerTest, WorkerFailed) {
   EXPECT_TRUE(
       base::Contains(scheduler.GetFailedCertProfileIds(), kCertProfileId));
 
-  certificate_helper_.AddCert();
-
-  EXPECT_CALL(
-      *platform_keys_service_,
-      GetAttributeForKey(
-          GetPlatformKeysTokenId(cert_scope),
-          certificate_helper_.GetPublicKeyForCert(),
-          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId, ""));
+  certificate_helper_->AddCert(kCertScope, kCertProfileId);
 
   // Check one more time that scheduler doesn't create new workers for failed
   // certificate profiles (the factory will fail on an attempt to do so).
@@ -335,24 +250,23 @@ TEST_F(CertProvisioningSchedulerTest, WorkerFailed) {
 }
 
 TEST_F(CertProvisioningSchedulerTest, InitialAndDailyUpdates) {
-  CertScope cert_scope = CertScope::kUser;
+  const CertScope kCertScope = CertScope::kUser;
+
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
-  // Add 1 certificate profile to the policy.
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForUser, config);
-  // Same as in the policy.
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForUser, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -364,46 +278,47 @@ TEST_F(CertProvisioningSchedulerTest, InitialAndDailyUpdates) {
 
   // Now one worker should be created.
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile);
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
 
   // Emulate callback from the worker.
-  CertProfile profile{kCertProfileId, kCertProfileVersion};
-  scheduler.OnProfileFinished(profile, CertProvisioningWorkerState::kFailed);
+  scheduler.OnProfileFinished(cert_profile,
+                              CertProvisioningWorkerState::kFailed);
 
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
   EXPECT_TRUE(
       base::Contains(scheduler.GetFailedCertProfileIds(), kCertProfileId));
 
   // No workers should be created yet.
-  FastForwardBy(base::TimeDelta::FromHours(20));
+  FastForwardBy(TimeDelta::FromHours(20));
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
 
   // Now list of failed profiles should be cleared that will cause a new attempt
   // to provision certificate.
   MockCertProvisioningWorker* worker2 =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker2->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                            cert_profile);
-  FastForwardBy(base::TimeDelta::FromHours(5));
+  FastForwardBy(TimeDelta::FromHours(5));
   ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
 
   // Emulate callback from the worker.
-  scheduler.OnProfileFinished(profile, CertProvisioningWorkerState::kSucceeded);
+  scheduler.OnProfileFinished(cert_profile,
+                              CertProvisioningWorkerState::kSucceeded);
 
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
   EXPECT_TRUE(scheduler.GetFailedCertProfileIds().empty());
 }
 
 TEST_F(CertProvisioningSchedulerTest, MultipleWorkers) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForUser, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -414,57 +329,57 @@ TEST_F(CertProvisioningSchedulerTest, MultipleWorkers) {
       .Times(1);
 
   // The policy is empty, so no workers should be created yet.
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
 
   // New workers will be created on prefs update.
   const char kCertProfileId0[] = "cert_profile_id_0";
   const char kCertProfileVersion0[] = "cert_profile_version_0";
-  CertProfile cert_profile0{kCertProfileId0, kCertProfileVersion0};
+  CertProfile cert_profile0(kCertProfileId0, kCertProfileVersion0,
+                            /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   const char kCertProfileId1[] = "cert_profile_id_1";
   const char kCertProfileVersion1[] = "cert_profile_version_1";
-  CertProfile cert_profile1{kCertProfileId1, kCertProfileVersion1};
+  CertProfile cert_profile1(kCertProfileId1, kCertProfileVersion1,
+                            /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   const char kCertProfileId2[] = "cert_profile_id_2";
   const char kCertProfileVersion2[] = "cert_profile_version_2";
-  CertProfile cert_profile2{kCertProfileId2, kCertProfileVersion2};
+  CertProfile cert_profile2(kCertProfileId2, kCertProfileVersion2,
+                            /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   MockCertProvisioningWorker* worker0 =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile0);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile0);
   worker0->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                            cert_profile0);
   MockCertProvisioningWorker* worker1 =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile1);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile1);
   worker1->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                            cert_profile1);
   MockCertProvisioningWorker* worker2 =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile2);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile2);
   worker2->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                            cert_profile2);
 
-  // Add 3 certificate profiles to the policy ("cert_profile_id"s are the same
-  // as above).
+  // Add 3 certificate profiles to the policy (the values are the same as
+  // in |cert_profile|-s)
   base::Value config = ParseJson(
       R"([{
            "name": "Certificate Profile 0",
            "cert_profile_id":"cert_profile_id_0",
            "policy_version":"cert_profile_version_0",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000
+           "key_algorithm":"rsa"
           },
           {
            "name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000
+           "key_algorithm":"rsa"
           },
           {
            "name": "Certificate Profile 2",
            "cert_profile_id":"cert_profile_id_2",
            "policy_version":"cert_profile_version_2",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000
+           "key_algorithm":"rsa"
           }])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForUser, config);
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   // Now one worker for each profile should be created.
   ASSERT_EQ(scheduler.GetWorkers().size(), 3U);
@@ -485,16 +400,7 @@ TEST_F(CertProvisioningSchedulerTest, MultipleWorkers) {
   EXPECT_TRUE(
       base::Contains(scheduler.GetFailedCertProfileIds(), kCertProfileId2));
 
-  certificate_helper_.AddCert();
-
-  EXPECT_CALL(
-      *platform_keys_service_,
-      GetAttributeForKey(
-          GetPlatformKeysTokenId(cert_scope),
-          certificate_helper_.GetPublicKeyForCert(),
-          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId0, ""));
+  certificate_helper_->AddCert(kCertScope, kCertProfileId0);
 
   // Make scheduler check workers state.
   scheduler.UpdateAllCerts();
@@ -503,15 +409,6 @@ TEST_F(CertProvisioningSchedulerTest, MultipleWorkers) {
   EXPECT_TRUE(
       base::Contains(scheduler.GetFailedCertProfileIds(), kCertProfileId2));
 
-  EXPECT_CALL(
-      *platform_keys_service_,
-      GetAttributeForKey(
-          GetPlatformKeysTokenId(cert_scope),
-          certificate_helper_.GetPublicKeyForCert(),
-          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId0, ""));
-
   // Check one more time that scheduler doesn't create new workers for failed
   // certificate profiles (the factory will fail on an attempt to do so).
   scheduler.UpdateAllCerts();
@@ -519,50 +416,40 @@ TEST_F(CertProvisioningSchedulerTest, MultipleWorkers) {
 }
 
 TEST_F(CertProvisioningSchedulerTest, RemoveCertWithoutPolicy) {
-  CertScope cert_scope = CertScope::kDevice;
-  const char kCertProfileId[] = "cert_profile_id_1";
-
-  certificate_helper_.AddCert();
+  const CertScope kCertScope = CertScope::kDevice;
 
-  EXPECT_CALL(*platform_keys_service_,
-              GetAttributeForKey(
-                  GetPlatformKeysTokenId(cert_scope),
-                  certificate_helper_.GetPublicKeyForCert(),
-                  platform_keys::KeyAttributeType::CertificateProvisioningId,
-                  /*callback=*/_))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId, ""));
+  certificate_helper_->AddCert(kCertScope, kCertProfileId);
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
-  EXPECT_CALL(*platform_keys_service_,
-              RemoveCertificate(GetPlatformKeysTokenId(cert_scope),
-                                /*certificate=*/certificate_helper_.GetCert(),
+  EXPECT_CALL(
+      platform_keys_service_,
+      RemoveCertificate(GetPlatformKeysTokenId(kCertScope),
+                        /*certificate=*/certificate_helper_->GetCerts().back(),
                         /*callback=*/_))
       .Times(1);
 
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
 }
 
 TEST_F(CertProvisioningSchedulerTest, DeserializeWorkers) {
-  CertScope cert_scope = CertScope::kUser;
+  const CertScope kCertScope = CertScope::kUser;
 
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
-  // Add 1 certificate profile to the policy.
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value cert_profiles = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version": "cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForUser, cert_profiles);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), cert_profiles);
   // Add 1 serialized worker for the profile.
   base::Value saved_worker = ParseJson(
       R"({
@@ -578,37 +465,35 @@ TEST_F(CertProvisioningSchedulerTest, DeserializeWorkers) {
   base::Value all_saved_workers(base::Value::Type::DICTIONARY);
   all_saved_workers.SetKey("cert_profile_1", saved_worker.Clone());
 
-  pref_service_.Set(prefs::kCertificateProvisioningStateForUser,
-                    all_saved_workers);
+  pref_service_.Set(GetPrefNameForSerialization(kCertScope), all_saved_workers);
 
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectDeserializeReturnMock(cert_scope, saved_worker);
+      mock_factory_.ExpectDeserializeReturnMock(kCertScope, saved_worker);
   // is_waiting==true should be set by Serializer so Scheduler knows that the
   // worker has to be continued manually.
   worker->SetExpectations(/*do_step_times=*/AtLeast(1),
                           /*is_waiting=*/true, cert_profile);
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForUser, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
   // Now one worker should be created.
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
 }
 
 TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
 
-  const char kCertProfileId[] = "cert_profile_id_1";
   const char kCertProfileVersion1[] = "cert_profile_version_1";
   const char kCertProfileVersion2[] = "cert_profile_version_2";
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -619,23 +504,26 @@ TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
       .Times(1);
 
   // The policy is empty, so no workers should be created yet.
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
 
-  CertProfile cert_profile_v1{kCertProfileId, kCertProfileVersion1};
+  CertProfile cert_profile_v1(kCertProfileId, kCertProfileVersion1,
+                              /*is_va_enabled=*/true,
+                              kCertProfileRenewalPeriod);
+
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile_v1);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile_v1);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile_v1);
 
-  // Add 1 certificate profile to the policy.
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile_v1|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   // Now 1 worker should be created.
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
@@ -650,12 +538,12 @@ TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
   EXPECT_TRUE(scheduler.GetFailedCertProfileIds().empty());
 
   // Add a new worker to the factory.
-  worker = mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile_v1);
+  worker = mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile_v1);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile_v1);
 
   // After some delay a new worker should be created to try again.
-  FastForwardBy(base::TimeDelta::FromSeconds(31));
+  FastForwardBy(TimeDelta::FromSeconds(31));
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
 
   // Emulate callback from the worker.
@@ -668,8 +556,10 @@ TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
   EXPECT_TRUE(scheduler.GetFailedCertProfileIds().empty());
 
   // Add a new worker to the factory.
-  CertProfile cert_profile_v2{kCertProfileId, kCertProfileVersion2};
-  worker = mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile_v2);
+  CertProfile cert_profile_v2(kCertProfileId, kCertProfileVersion2,
+                              /*is_va_enabled=*/true,
+                              kCertProfileRenewalPeriod);
+  worker = mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile_v2);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile_v2);
 
@@ -678,9 +568,8 @@ TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_2",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
 
   // If another update happens, workers with matching policy versions should not
@@ -694,44 +583,40 @@ TEST_F(CertProvisioningSchedulerTest, InconsistentDataErrorHandling) {
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_3",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
+           "key_algorithm":"rsa"}])");
 
   // On policy change scheduler should detect mismatch in policy versions and
   // stop the worker.
   EXPECT_CALL(*worker,
               Stop(CertProvisioningWorkerState::kInconsistentDataError));
 
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
-  // EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   // Emulate that after some time the worker reports back to scheduler.
-  FastForwardBy(base::TimeDelta::FromSeconds(10));
+  FastForwardBy(TimeDelta::FromSeconds(10));
   scheduler.OnProfileFinished(
       cert_profile_v1, CertProvisioningWorkerState::kInconsistentDataError);
   EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
 }
 
 TEST_F(CertProvisioningSchedulerTest, RetryAfterNoInternetConnection) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
   SetWifiNetworkState(shill::kStateIdle);
 
-  // Add 1 certificate profile to the policy.
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
-  // Same as in the policy.
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -741,12 +626,12 @@ TEST_F(CertProvisioningSchedulerTest, RetryAfterNoInternetConnection) {
                   attestation::AttestationKeyType::KEY_DEVICE, kKeyNamePrefix))
       .Times(1);
 
-  FastForwardBy(base::TimeDelta::FromHours(72));
+  FastForwardBy(TimeDelta::FromHours(72));
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
 
   // Add a new worker to the factory.
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile);
 
@@ -756,23 +641,21 @@ TEST_F(CertProvisioningSchedulerTest, RetryAfterNoInternetConnection) {
 }
 
 TEST_F(CertProvisioningSchedulerTest, DeleteWorkerWithoutPolicy) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
 
-  // Add 1 certificate profile to the policy.
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  // Same as in the policy.
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+           "key_algorithm":"rsa"}])");
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
@@ -784,24 +667,24 @@ TEST_F(CertProvisioningSchedulerTest, DeleteWorkerWithoutPolicy) {
 
   // Add a new worker to the factory.
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
                           cert_profile);
 
   // Prefs update will be ignored because initialization task has not finished
   // yet.
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
   ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
 
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
 
   EXPECT_CALL(*worker, Stop(CertProvisioningWorkerState::kCanceled));
 
   config = ParseJson("[]");
-  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
 
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   // Emulate callback from the worker.
   scheduler.OnProfileFinished(cert_profile,
                               CertProvisioningWorkerState::kCanceled);
@@ -810,12 +693,12 @@ TEST_F(CertProvisioningSchedulerTest, DeleteWorkerWithoutPolicy) {
 }
 
 TEST_F(CertProvisioningSchedulerTest, DeleteVaKeysOnIdle) {
-  CertScope cert_scope = CertScope::kDevice;
+  const CertScope kCertScope = CertScope::kDevice;
 
   {
     CertProvisioningScheduler scheduler(
-        cert_scope, GetProfile(), &pref_service_,
-        prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+        kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+        &platform_keys_service_,
         network_state_test_helper_.network_state_handler(),
         MakeFakeInvalidationFactory());
 
@@ -826,15 +709,15 @@ TEST_F(CertProvisioningSchedulerTest, DeleteVaKeysOnIdle) {
             attestation::AttestationKeyType::KEY_DEVICE, kKeyNamePrefix))
         .Times(1);
 
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
   }
 
   {
-    const char kCertProfileId[] = "cert_profile_id_1";
-    const char kCertProfileVersion[] = "cert_profile_version_1";
-    CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+    CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                             /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
-    // Add 1 serialized worker for the profile.
+    // Add 1 serialized worker for the profile (the values are the same as
+    // in |cert_profile|).
     base::Value saved_worker = ParseJson(
         R"({
           "cert_profile": {
@@ -849,64 +732,64 @@ TEST_F(CertProvisioningSchedulerTest, DeleteVaKeysOnIdle) {
     base::Value all_saved_workers(base::Value::Type::DICTIONARY);
     all_saved_workers.SetKey("cert_profile_1", saved_worker.Clone());
 
-    pref_service_.Set(prefs::kCertificateProvisioningStateForDevice,
+    pref_service_.Set(GetPrefNameForSerialization(kCertScope),
                       all_saved_workers);
 
     MockCertProvisioningWorker* worker =
-        mock_factory_.ExpectDeserializeReturnMock(cert_scope, saved_worker);
+        mock_factory_.ExpectDeserializeReturnMock(kCertScope, saved_worker);
     // This worker should be deleted approximately right after creation, hence
     // no calls for DoStep.
     worker->SetExpectations(/*do_step_times=*/Exactly(0),
                             /*is_waiting=*/true, cert_profile);
 
     CertProvisioningScheduler scheduler(
-        cert_scope, GetProfile(), &pref_service_,
-        prefs::kRequiredClientCertificateForDevice, &cloud_policy_client_,
+        kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+        &platform_keys_service_,
         network_state_test_helper_.network_state_handler(),
         MakeFakeInvalidationFactory());
 
     EXPECT_CALL(fake_cryptohome_client_, OnTpmAttestationDeleteKeysByPrefix)
         .Times(0);
 
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
   }
 }
 
 TEST_F(CertProvisioningSchedulerTest, UpdateOneCert) {
-  CertScope cert_scope = CertScope::kUser;
+  const CertScope kCertScope = CertScope::kUser;
 
   CertProvisioningScheduler scheduler(
-      cert_scope, GetProfile(), &pref_service_,
-      prefs::kRequiredClientCertificateForUser, &cloud_policy_client_,
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
       network_state_test_helper_.network_state_handler(),
       MakeFakeInvalidationFactory());
 
-  const char kCertProfileId[] = "cert_profile_id_1";
-  const char kCertProfileVersion[] = "cert_profile_version_1";
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   // From CertProvisioningScheduler::CleanVaKeysIfIdle.
   EXPECT_CALL(fake_cryptohome_client_, OnTpmAttestationDeleteKeysByPrefix);
+  FastForwardBy(TimeDelta::FromSeconds(1));
 
   // There is no policies yet, |kCertProfileId| will not be found.
   scheduler.UpdateOneCert(kCertProfileId);
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   ASSERT_TRUE(scheduler.GetWorkers().empty());
 
   MockCertProvisioningWorker* worker =
-      mock_factory_.ExpectCreateReturnMock(cert_scope, cert_profile);
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
   worker->SetExpectations(/*do_step_times=*/Exactly(1),
                           /*is_waiting=*/false, cert_profile);
 
-  // Add 1 certificate profile to the policy ("cert_profile_id" is the same as
-  // above). That will trigger creation of a worker.
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
   base::Value config = ParseJson(
       R"([{"name": "Certificate Profile 1",
            "cert_profile_id":"cert_profile_id_1",
            "policy_version":"cert_profile_version_1",
-           "key_algorithm":"rsa",
-           "renewal_period_seconds": 365000}])");
-  pref_service_.Set(prefs::kRequiredClientCertificateForUser, config);
+           "key_algorithm":"rsa"}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
+  FastForwardBy(TimeDelta::FromSeconds(1));
 
   // If worker is waiting, it should be continued.
   {
@@ -914,7 +797,7 @@ TEST_F(CertProvisioningSchedulerTest, UpdateOneCert) {
                             /*is_waiting=*/true, cert_profile);
 
     scheduler.UpdateOneCert(kCertProfileId);
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
     ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
   }
 
@@ -924,7 +807,7 @@ TEST_F(CertProvisioningSchedulerTest, UpdateOneCert) {
                             /*is_waiting=*/false, cert_profile);
 
     scheduler.UpdateOneCert(kCertProfileId);
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
     ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
   }
 
@@ -937,38 +820,86 @@ TEST_F(CertProvisioningSchedulerTest, UpdateOneCert) {
                             /*is_waiting=*/true, cert_profile);
 
     scheduler.UpdateOneCert(kCertProfileId);
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
     ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
 
     worker->SetExpectations(/*do_step_times=*/Exactly(1),
                             /*is_waiting=*/true, cert_profile);
+
     SetWifiNetworkState(shill::kStateOnline);
   }
 
   // Emulate callback from the worker.
   scheduler.OnProfileFinished(cert_profile,
                               CertProvisioningWorkerState::kSucceeded);
-  FastForwardBy(base::TimeDelta::FromSeconds(1));
+  FastForwardBy(TimeDelta::FromSeconds(1));
   ASSERT_TRUE(scheduler.GetWorkers().empty());
 
-  certificate_helper_.AddCert();
-  EXPECT_CALL(
-      *platform_keys_service_,
-      GetAttributeForKey(
-          GetPlatformKeysTokenId(cert_scope),
-          certificate_helper_.GetPublicKeyForCert(),
-          platform_keys::KeyAttributeType::CertificateProvisioningId, _))
-      .Times(1)
-      .WillOnce(base::test::RunOnceCallback<3>(kCertProfileId, ""));
+  certificate_helper_->AddCert(kCertScope, kCertProfileId);
 
   {
     // If a certificate already exists, a new worker should not be created.
     scheduler.UpdateOneCert(kCertProfileId);
-    FastForwardBy(base::TimeDelta::FromSeconds(1));
+    FastForwardBy(TimeDelta::FromSeconds(1));
     ASSERT_TRUE(scheduler.GetWorkers().empty());
   }
 }
 
+TEST_F(CertProvisioningSchedulerTest, CertRenewal) {
+  const CertScope kCertScope = CertScope::kUser;
+  // 1 day == 86400 seconds.
+  const TimeDelta kRenewalPeriod = TimeDelta::FromDays(1);
+
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kRenewalPeriod);
+
+  const Time t1 = Time::Now() - TimeDelta::FromDays(1);
+  const Time t2 = Time::Now() + TimeDelta::FromDays(7);
+  certificate_helper_->AddCert(kCertScope, kCertProfileId, /*error_message=*/"",
+                               /*nat_valid_before=*/t1, /*not_valid_after=*/t2);
+
+  // Add 1 certificate profile to the policy (the values are the same as
+  // in |cert_profile|).
+  base::Value config = ParseJson(
+      R"([{"name": "Certificate Profile 1",
+           "cert_profile_id":"cert_profile_id_1",
+           "policy_version":"cert_profile_version_1",
+           "key_algorithm":"rsa",
+           "renewal_period_seconds": 86400}])");
+  pref_service_.Set(GetPrefNameForCertProfiles(kCertScope), config);
+
+  CertProvisioningScheduler scheduler(
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
+      network_state_test_helper_.network_state_handler(),
+      MakeFakeInvalidationFactory());
+
+  // From CertProvisioningScheduler::CleanVaKeysIfIdle.
+  EXPECT_CALL(fake_cryptohome_client_,
+              OnTpmAttestationDeleteKeysByPrefix(
+                  attestation::AttestationKeyType::KEY_USER, kKeyNamePrefix))
+      .Times(1);
+
+  // The certificate already exists, nothing should happen on scheduler
+  // creation.
+  FastForwardBy(TimeDelta::FromSeconds(1));
+  ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
+
+  // Also nothing should happen in the next ~6 days.
+  FastForwardBy(TimeDelta::FromDays(5) + TimeDelta::FromHours(23));
+  ASSERT_EQ(scheduler.GetWorkers().size(), 0U);
+
+  MockCertProvisioningWorker* worker =
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
+  worker->SetExpectations(/*do_step_times=*/AtLeast(1),
+                          /*is_waiting=*/false, cert_profile);
+
+  // One day (according to the policy) before the certificate expires, scheduler
+  // should create a new worker to provision a replacement.
+  FastForwardBy(TimeDelta::FromHours(1));
+  ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
+}
+
 }  // namespace
 }  // namespace cert_provisioning
 }  // namespace chromeos

chrome/browser/chromeos/cert_provisioning/cert_provisioning_serializer.cc

@@ -7,6 +7,7 @@
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/optional.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -16,15 +17,16 @@ namespace cert_provisioning {
 
 namespace {
 
-const char kKeyNameCertScope[] = "cert_scope";
-const char kKeyNameCertProfile[] = "cert_profile";
-const char kKeyNameState[] = "state";
-const char kKeyNamePublicKey[] = "public_key";
-const char kKeyNameInvalidationTopic[] = "invalidation_topic";
+constexpr char kKeyNameCertScope[] = "cert_scope";
+constexpr char kKeyNameCertProfile[] = "cert_profile";
+constexpr char kKeyNameState[] = "state";
+constexpr char kKeyNamePublicKey[] = "public_key";
+constexpr char kKeyNameInvalidationTopic[] = "invalidation_topic";
 
-const char kKeyNameCertProfileId[] = "profile_id";
-const char kKeyNameCertProfileVersion[] = "policy_version";
-const char kKeyNameCertProfileVaEnabled[] = "va_enabled";
+constexpr char kKeyNameCertProfileId[] = "profile_id";
+constexpr char kKeyNameCertProfileVersion[] = "policy_version";
+constexpr char kKeyNameCertProfileVaEnabled[] = "va_enabled";
+constexpr char kKeyNameCertProfileRenewalPeriod[] = "renewal_period";
 
 template <typename T>
 bool ConvertToEnum(int value, T* dst) {
@@ -71,21 +73,34 @@ bool DeserializeBoolValue(const base::Value& parent_value,
   return true;
 }
 
+bool DeserializeRenewalPeriod(const base::Value& parent_value,
+                              const char* value_name,
+                              base::TimeDelta* dst) {
+  base::Optional<int> serialized_time = parent_value.FindIntKey(value_name);
+  *dst = base::TimeDelta::FromSeconds(serialized_time.value_or(0));
+  return true;
+}
+
 base::Value SerializeCertProfile(const CertProfile& profile) {
-  static_assert(CertProfile::kVersion == 3, "This function should be updated");
+  static_assert(CertProfile::kVersion == 4, "This function should be updated");
 
   base::Value result(base::Value::Type::DICTIONARY);
   result.SetStringKey(kKeyNameCertProfileId, profile.profile_id);
   result.SetStringKey(kKeyNameCertProfileVersion, profile.policy_version);
   result.SetBoolKey(kKeyNameCertProfileVaEnabled, profile.is_va_enabled);
 
+  if (!profile.renewal_period.is_zero()) {
+    result.SetIntKey(kKeyNameCertProfileRenewalPeriod,
+                     profile.renewal_period.InSeconds());
+  }
+
   return result;
 }
 
 bool DeserializeCertProfile(const base::Value& parent_value,
                             const char* value_name,
                             CertProfile* dst) {
-  static_assert(CertProfile::kVersion == 3, "This function should be updated");
+  static_assert(CertProfile::kVersion == 4, "This function should be updated");
 
   const base::Value* serialized_profile =
       parent_value.FindKeyOfType(value_name, base::Value::Type::DICTIONARY);
@@ -104,6 +119,9 @@ bool DeserializeCertProfile(const base::Value& parent_value,
   is_ok = is_ok && DeserializeBoolValue(*serialized_profile,
                                         kKeyNameCertProfileVaEnabled,
                                         &(dst->is_va_enabled));
+  is_ok = is_ok && DeserializeRenewalPeriod(*serialized_profile,
+                                            kKeyNameCertProfileRenewalPeriod,
+                                            &(dst->renewal_period));
   return is_ok;
 }
 

chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.cc

@@ -6,7 +6,7 @@
 
 #include "base/optional.h"
 #include "base/test/gmock_callback_support.h"
-#include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "net/test/cert_builder.h"
@@ -63,18 +63,15 @@ void CertificateHelperForTesting::GetCertificates(
   std::move(callback).Run(std::move(result), "");
 }
 
-void CertificateHelperForTesting::AddCert(
-    CertScope cert_scope,
-    const base::Optional<CertProfileId>& cert_profile_id) {
-  AddCert(cert_scope, cert_profile_id, /*error_message=*/"");
-}
-
-void CertificateHelperForTesting::AddCert(
+scoped_refptr<net::X509Certificate> CertificateHelperForTesting::AddCert(
     CertScope cert_scope,
     const base::Optional<CertProfileId>& cert_profile_id,
-    const std::string& error_message) {
+    const std::string& error_message,
+    base::Time not_valid_before,
+    base::Time not_valid_after) {
   net::CertBuilder cert_builder(template_cert_->cert_buffer(),
                                 /*issuer=*/nullptr);
+  cert_builder.SetValidity(not_valid_before, not_valid_after);
   auto cert = cert_builder.GetX509Certificate();
 
   EXPECT_CALL(
@@ -86,6 +83,30 @@ void CertificateHelperForTesting::AddCert(
       .WillRepeatedly(RunOnceCallback<3>(cert_profile_id, error_message));
 
   cert_list_.push_back(cert);
+  return cert;
+}
+
+scoped_refptr<net::X509Certificate> CertificateHelperForTesting::AddCert(
+    CertScope cert_scope,
+    const base::Optional<CertProfileId>& cert_profile_id) {
+  base::Time not_valid_before =
+      base::Time::Now() - base::TimeDelta::FromDays(1);
+  base::Time not_valid_after =
+      base::Time::Now() + base::TimeDelta::FromDays(365);
+  return AddCert(cert_scope, cert_profile_id, /*error_message=*/"",
+                 not_valid_before, not_valid_after);
+}
+
+scoped_refptr<net::X509Certificate> CertificateHelperForTesting::AddCert(
+    CertScope cert_scope,
+    const base::Optional<CertProfileId>& cert_profile_id,
+    const std::string& error_message) {
+  base::Time not_valid_before =
+      base::Time::Now() - base::TimeDelta::FromDays(1);
+  base::Time not_valid_after =
+      base::Time::Now() + base::TimeDelta::FromDays(365);
+  return AddCert(cert_scope, cert_profile_id, error_message, not_valid_before,
+                 not_valid_after);
 }
 
 void CertificateHelperForTesting::ClearCerts() {

chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.h

@@ -19,19 +19,41 @@ namespace cert_provisioning {
 
 //================ CertificateHelperForTesting =================================
 
-// Redirects PlatformKeysService::GetCertificate calls to itself. Allows to add
-// certificate to a fake storage with assigned CertProfileId-s.
+// Allows to add certificate to a fake storage with assigned CertProfileId-s.
+// Redirects PlatformKeysService::GetCertificate calls to itself and return all
+// stored certificates as a result.
 struct CertificateHelperForTesting {
  public:
   explicit CertificateHelperForTesting(
       platform_keys::MockPlatformKeysService* platform_keys_service);
   ~CertificateHelperForTesting();
 
-  void AddCert(CertScope cert_scope,
+  // Generates and adds a certificate to internal fake certificate storage.
+  // Returns refpointer to the generated certificate. If |error_message| is not
+  // empty, an attempt to retrieve |cert_profile_id| via
+  // PlatformKeysService::GetAttributeForKey() will fail with |error_message|.
+  // |not_valid_before|, |not_valid_after| configure validity period of the
+  // certificate.
+  scoped_refptr<net::X509Certificate> AddCert(
+      CertScope cert_scope,
+      const base::Optional<CertProfileId>& cert_profile_id,
+      const std::string& error_message,
+      base::Time not_valid_before,
+      base::Time not_valid_after);
+
+  // Simplified version of AddCert(). The certificate is not expired and has
+  // |cert_profile_id|.
+  scoped_refptr<net::X509Certificate> AddCert(
+      CertScope cert_scope,
       const base::Optional<CertProfileId>& cert_profile_id);
-  void AddCert(CertScope cert_scope,
+
+  // Simplified version of AddCert(). The certificate is not expired, but fails
+  // to retrieve |cert_profile_id|.
+  scoped_refptr<net::X509Certificate> AddCert(
+      CertScope cert_scope,
       const base::Optional<CertProfileId>& cert_profile_id,
       const std::string& error_message);
+
   void ClearCerts();
   const net::CertificateList& GetCerts() const;
 

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.cc

@@ -109,19 +109,6 @@ int GetStateOrderedIndex(CertProvisioningWorkerState state) {
   return res;
 }
 
-bool CheckPublicKeyInCertificate(
-    const scoped_refptr<net::X509Certificate>& cert,
-    const std::string& public_key) {
-  base::StringPiece spki_from_cert;
-  if (!net::asn1::ExtractSPKIFromDERCert(
-          net::x509_util::CryptoBufferAsStringPiece(cert->cert_buffer()),
-          &spki_from_cert)) {
-    return false;
-  }
-
-  return (public_key == spki_from_cert);
-}
-
 }  // namespace
 
 // ============= CertProvisioningWorkerFactory =================================
@@ -633,7 +620,9 @@ void CertProvisioningWorkerImpl::ImportCert(
     return;
   }
 
-  if (!CheckPublicKeyInCertificate(cert, public_key_)) {
+  std::string public_key_from_cert =
+      platform_keys::GetSubjectPublicKeyInfo(cert);
+  if (public_key_from_cert != public_key_) {
     LOG(ERROR) << "Downloaded certificate does not match the expected key pair";
     UpdateState(CertProvisioningWorkerState::kFailed);
     return;

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker_unittest.cc

@@ -79,6 +79,8 @@ constexpr char kPublicKeyBase64[] =
 
 constexpr char kCertProfileId[] = "cert_profile_1";
 constexpr char kCertProfileVersion[] = "cert_profile_version_1";
+constexpr base::TimeDelta kCertProfileRenewalPeriod =
+    base::TimeDelta::FromSeconds(0);
 // Prefix + certificate profile name.
 constexpr char kCertScopeStrUser[] = "google/chromeos/user";
 constexpr char kCertScopeStrDevice[] = "google/chromeos/device";
@@ -404,7 +406,8 @@ class CertProvisioningWorkerTest : public ::testing::Test {
 TEST_F(CertProvisioningWorkerTest, Success) {
   base::HistogramTester histogram_tester;
 
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   MockCertProvisioningInvalidator* mock_invalidator = nullptr;
@@ -480,8 +483,8 @@ TEST_F(CertProvisioningWorkerTest, Success) {
 // Checks that the worker makes all necessary requests to other modules during
 // success scenario when VA challenge is not received.
 TEST_F(CertProvisioningWorkerTest, NoVaSuccess) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion,
-                           /*is_va_enabled=*/false};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/false, kCertProfileRenewalPeriod);
 
   CertProvisioningWorkerImpl worker(
       CertScope::kUser, GetProfile(), &testing_pref_service_, cert_profile,
@@ -531,7 +534,8 @@ TEST_F(CertProvisioningWorkerTest, NoVaSuccess) {
 // Checks that when the server returns try_again_later field, the worker will
 // retry a request when it asked to continue the provisioning.
 TEST_F(CertProvisioningWorkerTest, TryLaterManualRetry) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   CertProvisioningWorkerImpl worker(
@@ -633,7 +637,8 @@ TEST_F(CertProvisioningWorkerTest, TryLaterManualRetry) {
 // Checks that when the server returns try_again_later field, the worker will
 // automatically retry a request after some time.
 TEST_F(CertProvisioningWorkerTest, TryLaterWait) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   CertProvisioningWorkerImpl worker(
@@ -743,7 +748,8 @@ TEST_F(CertProvisioningWorkerTest, TryLaterWait) {
 // Checks that when the server returns error status, the worker will enter an
 // error state and stop the provisioning.
 TEST_F(CertProvisioningWorkerTest, StatusErrorHandling) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   CertProvisioningWorkerImpl worker(
@@ -785,7 +791,8 @@ TEST_F(CertProvisioningWorkerTest, StatusErrorHandling) {
 TEST_F(CertProvisioningWorkerTest, ResponseErrorHandling) {
   base::HistogramTester histogram_tester;
 
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   auto worker = CertProvisioningWorkerFactory::Get()->Create(
@@ -828,7 +835,8 @@ TEST_F(CertProvisioningWorkerTest, ResponseErrorHandling) {
 }
 
 TEST_F(CertProvisioningWorkerTest, InconsistentDataErrorHandling) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   auto worker = CertProvisioningWorkerFactory::Get()->Create(
@@ -867,7 +875,8 @@ TEST_F(CertProvisioningWorkerTest, InconsistentDataErrorHandling) {
 // Checks that when the server returns TEMPORARY_UNAVAILABLE status code, the
 // worker will automatically retry a request using exponential backoff strategy.
 TEST_F(CertProvisioningWorkerTest, BackoffStrategy) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   CertProvisioningWorkerImpl worker(
@@ -930,7 +939,8 @@ TEST_F(CertProvisioningWorkerTest, BackoffStrategy) {
 TEST_F(CertProvisioningWorkerTest, RemoveRegisteredKey) {
   base::HistogramTester histogram_tester;
 
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   MockCertProvisioningInvalidator* mock_invalidator = nullptr;
   CertProvisioningWorkerImpl worker(
@@ -1017,8 +1027,10 @@ class PrefServiceObserver {
 };
 
 TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
-  CertScope cert_scope = CertScope::kUser;
+  const base::TimeDelta kRenewalPeriod = base::TimeDelta::FromSeconds(1200300);
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kRenewalPeriod);
+  const CertScope kCertScope = CertScope::kUser;
 
   std::unique_ptr<MockCertProvisioningInvalidator> mock_invalidator_obj;
   MockCertProvisioningInvalidator* mock_invalidator = nullptr;
@@ -1026,7 +1038,7 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   std::unique_ptr<CertProvisioningWorker> worker =
       CertProvisioningWorkerFactory::Get()->Create(
-          cert_scope, GetProfile(), &testing_pref_service_, cert_profile,
+          kCertScope, GetProfile(), &testing_pref_service_, cert_profile,
           &cloud_policy_client_, MakeInvalidator(), GetCallback());
 
   StrictMock<PrefServiceObserver> pref_observer(
@@ -1051,7 +1063,8 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
             "cert_profile": {
               "policy_version": "cert_profile_version_1",
               "profile_id": "cert_profile_1",
-              "va_enabled": true
+              "va_enabled": true,
+              "renewal_period": 1200300
             },
             "cert_scope": 0,
             "invalidation_topic": "",
@@ -1083,7 +1096,7 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
         .Times(1);
 
     worker = CertProvisioningWorkerFactory::Get()->Deserialize(
-        cert_scope, GetProfile(), &testing_pref_service_,
+        kCertScope, GetProfile(), &testing_pref_service_,
         *pref_val.FindKeyOfType(kCertProfileId, base::Value::Type::DICTIONARY),
         &cloud_policy_client_, MakeInvalidator(&mock_invalidator),
         GetCallback());
@@ -1127,7 +1140,8 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
             "cert_profile": {
               "policy_version": "cert_profile_version_1",
               "profile_id": "cert_profile_1",
-              "va_enabled": true
+              "va_enabled": true,
+              "renewal_period": 1200300
             },
             "cert_scope": 0,
             "invalidation_topic": "fake_invalidation_topic_1",
@@ -1161,7 +1175,7 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
         .Times(1);
 
     worker = CertProvisioningWorkerFactory::Get()->Deserialize(
-        cert_scope, GetProfile(), &testing_pref_service_,
+        kCertScope, GetProfile(), &testing_pref_service_,
         *pref_val.FindKeyOfType(kCertProfileId, base::Value::Type::DICTIONARY),
         &cloud_policy_client_, std::move(mock_invalidator_obj), GetCallback());
   }
@@ -1190,7 +1204,8 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
 }
 
 TEST_F(CertProvisioningWorkerTest, SerializationOnFailure) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   auto worker = CertProvisioningWorkerFactory::Get()->Create(
@@ -1250,7 +1265,8 @@ TEST_F(CertProvisioningWorkerTest, SerializationOnFailure) {
 }
 
 TEST_F(CertProvisioningWorkerTest, InformationalGetters) {
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   CertProvisioningWorkerImpl worker(
@@ -1303,12 +1319,13 @@ TEST_F(CertProvisioningWorkerTest, InformationalGetters) {
 TEST_F(CertProvisioningWorkerTest, CancelDeviceWorker) {
   base::HistogramTester histogram_tester;
 
-  CertScope cert_scope = CertScope::kDevice;
-  CertProfile cert_profile{kCertProfileId, kCertProfileVersion};
+  const CertScope kCertScope = CertScope::kDevice;
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
 
   MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
   auto worker = CertProvisioningWorkerFactory::Get()->Create(
-      cert_scope, GetProfile(), &testing_pref_service_, cert_profile,
+      kCertScope, GetProfile(), &testing_pref_service_, cert_profile,
       &cloud_policy_client_, MakeInvalidator(), GetCallback());
 
   EXPECT_CALL(callback_observer_, Callback).Times(0);