Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll.

chromeos/attestation/attestation_flow.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/attestation/attestation_flow.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/time/tick_clock.h"
+#include "base/timer/timer.h"
 #include "chromeos/cryptohome/async_method_caller.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "components/signin/core/account_id/account_id.h"
 
 namespace chromeos {
 namespace attestation {
 
 namespace {
 
+// Delay before checking whether attestation has been prepared again.
+constexpr base::TimeDelta kRetryDelay = base::TimeDelta::FromSeconds(7);
+
 // Redirects to one of three callbacks based on a boolean value and dbus call
 // status.
 //
 // Parameters
 //   on_true - Called when status=succes and value=true.
 //   on_false - Called when status=success and value=false.
 //   on_fail - Called when status=failure.
 //   status - The D-Bus operation status.
 //   value - The value returned by the D-Bus operation.
 void DBusBoolRedirectCallback(const base::Closure& on_true,
                               const base::Closure& on_false,
                               const base::Closure& on_fail,
+                              const std::string& on_fail_message,
                               DBusMethodCallStatus status,
                               bool value) {
   if (status != DBUS_METHOD_CALL_SUCCESS) {
-    LOG(ERROR) << "Attestation: Failed to query enrollment state.";
+    LOG(ERROR) << "Attestation: Failed to " << on_fail_message << ".";
     if (!on_fail.is_null())
       on_fail.Run();
     return;
   }
   const base::Closure& task = value ? on_true : on_false;
   if (!task.is_null())
     task.Run();
 }
 
 void DBusDataMethodCallback(
     const AttestationFlow::CertificateCallback& callback,
     DBusMethodCallStatus status,
     bool result,
     const std::string& data) {
   if (status != DBUS_METHOD_CALL_SUCCESS) {
     LOG(ERROR) << "Attestation: DBus data operation failed.";
     if (!callback.is_null())
       callback.Run(false, "");
     return;
   }
   if (!callback.is_null())
     callback.Run(result, data);
 }
 
 }  // namespace
 
+struct AttestationFlow::RetryData {
+  RetryData(base::TimeDelta timeout, base::TickClock* tick_clock)
+      : timeout(timeout),
+        tick_clock(tick_clock),
+        retry_timer(tick_clock),
+        begin(tick_clock->NowTicks()) {}
+  base::TimeDelta timeout;
+  base::TickClock* tick_clock;
+  base::OneShotTimer retry_timer;
+  base::TimeTicks begin;
+};
+
 AttestationKeyType AttestationFlow::GetKeyTypeForProfile(
     AttestationCertificateProfile certificate_profile) {
   switch (certificate_profile) {
     case PROFILE_ENTERPRISE_MACHINE_CERTIFICATE:
     case PROFILE_ENTERPRISE_ENROLLMENT_CERTIFICATE:
       return KEY_DEVICE;
     case PROFILE_ENTERPRISE_USER_CERTIFICATE:
     case PROFILE_CONTENT_PROTECTION_CERTIFICATE:
       return KEY_USER;
   }
@@ skipping 12 matching lines @@
       return kEnterpriseUserKey;
     case PROFILE_CONTENT_PROTECTION_CERTIFICATE:
       return std::string(kContentProtectionKeyPrefix) + request_origin;
   }
   NOTREACHED();
   return "";
 }
 
 AttestationFlow::AttestationFlow(cryptohome::AsyncMethodCaller* async_caller,
                                  CryptohomeClient* cryptohome_client,
-                                 std::unique_ptr<ServerProxy> server_proxy)
+                                 std::unique_ptr<ServerProxy> server_proxy,
+                                 base::TimeDelta preparedness_timeout)
     : async_caller_(async_caller),
       cryptohome_client_(cryptohome_client),
       server_proxy_(std::move(server_proxy)),
+      preparedness_timeout_(preparedness_timeout),
       weak_factory_(this) {}
 
 AttestationFlow::~AttestationFlow() {
 }
 
 void AttestationFlow::GetCertificate(
     AttestationCertificateProfile certificate_profile,
     const AccountId& account_id,
     const std::string& request_origin,
     bool force_new_key,
     const CertificateCallback& callback) {
   // If this device has not enrolled with the Privacy CA, we need to do that
   // first.  Once enrolled we can proceed with the certificate request.
   base::Closure do_cert_request = base::Bind(
       &AttestationFlow::StartCertificateRequest, weak_factory_.GetWeakPtr(),
       certificate_profile, account_id, request_origin, force_new_key, callback);
   base::Closure on_enroll_failure = base::Bind(callback, false, "");
-  base::Closure do_enroll = base::Bind(&AttestationFlow::StartEnroll,
-                                       weak_factory_.GetWeakPtr(),
-                                       on_enroll_failure,
-                                       do_cert_request);
-  cryptohome_client_->TpmAttestationIsEnrolled(base::Bind(
-      &DBusBoolRedirectCallback,
-      do_cert_request,  // If enrolled, proceed with cert request.
-      do_enroll,        // If not enrolled, initiate enrollment.
-      on_enroll_failure));
+  base::Closure initiate_enroll =

[achuithb, 2016/12/05 19:53:59]

const.

on_enroll_failure too

+      base::Bind(&AttestationFlow::InitiateEnroll, weak_factory_.GetWeakPtr(),
+                 on_enroll_failure, do_cert_request);
+  cryptohome_client_->TpmAttestationIsEnrolled(
+      base::Bind(&DBusBoolRedirectCallback,
+                 do_cert_request,  // If enrolled, proceed with cert request.
+                 initiate_enroll,  // If not enrolled, initiate enrollment.
+                 on_enroll_failure, "check enrollment state"));
+}
+
+void AttestationFlow::SetTickClockForTest(base::TickClock* tick_clock) {
+  tick_clock_ = tick_clock;
+}
+
+void AttestationFlow::InitiateEnroll(const base::Closure& on_failure,
+                                     const base::Closure& next_task) {
+  TryInitiateEnroll(new RetryData(preparedness_timeout_, tick_clock_),

[achuithb, 2016/12/05 19:53:59]

YOu should pass ownership with a unique_ptr.

Should retry_data be a data member of this class instead?

+                    on_failure, next_task);
+}
+
+void AttestationFlow::TryInitiateEnroll(RetryData* retry_data,
+                                        const base::Closure& on_failure,
+                                        const base::Closure& next_task) {
+  base::Closure do_enroll = base::Bind(

[achuithb, 2016/12/05 19:53:59]

const here and below

+      &AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(), retry_data,
+      base::Bind(&AttestationFlow::StartEnroll, weak_factory_.GetWeakPtr(),
+                 on_failure, next_task));
+  base::Closure retry_initiate_enroll = base::Bind(
+      &AttestationFlow::StillRetrying, weak_factory_.GetWeakPtr(), retry_data,
+      base::Bind(&AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(),
+                 retry_data, on_failure),
+      base::Bind(&AttestationFlow::TryInitiateEnroll,
+                 weak_factory_.GetWeakPtr(), retry_data, on_failure,
+                 next_task));
+  base::Closure do_fail =
+      base::Bind(&AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(),
+                 retry_data, on_failure);
+  cryptohome_client_->TpmAttestationIsPrepared(
+      base::Bind(&DBusBoolRedirectCallback, do_enroll, retry_initiate_enroll,
+                 do_fail, "check for attestation readiness"));
 }
 
 void AttestationFlow::StartEnroll(const base::Closure& on_failure,
                                   const base::Closure& next_task) {
   // Get the attestation service to create a Privacy CA enrollment request.
   async_caller_->AsyncTpmAttestationCreateEnrollRequest(
       server_proxy_->GetType(),
       base::Bind(&AttestationFlow::SendEnrollRequestToPCA,
                  weak_factory_.GetWeakPtr(),
                  on_failure,
@@ skipping 81 matching lines @@
         &AttestationFlow::GetExistingCertificate, weak_factory_.GetWeakPtr(),
         key_type, account_id, key_name, callback);
     // If the key does not exist, call this method back with |generate_new_key|
     // set to true.
     base::Closure on_key_not_exists = base::Bind(
         &AttestationFlow::StartCertificateRequest, weak_factory_.GetWeakPtr(),
         certificate_profile, account_id, request_origin, true, callback);
     cryptohome_client_->TpmAttestationDoesKeyExist(
         key_type, cryptohome::Identification(account_id), key_name,
         base::Bind(&DBusBoolRedirectCallback, on_key_exists, on_key_not_exists,
-                   base::Bind(callback, false, "")));
+                   base::Bind(callback, false, ""),
+                   "check for existence of attestation key"));
   }
 }
 
 void AttestationFlow::SendCertificateRequestToPCA(
     AttestationKeyType key_type,
     const AccountId& account_id,
     const std::string& key_name,
     const CertificateCallback& callback,
     bool success,
     const std::string& data) {
@@ skipping 34 matching lines @@
 void AttestationFlow::GetExistingCertificate(
     AttestationKeyType key_type,
     const AccountId& account_id,
     const std::string& key_name,
     const CertificateCallback& callback) {
   cryptohome_client_->TpmAttestationGetCertificate(
       key_type, cryptohome::Identification(account_id), key_name,
       base::Bind(&DBusDataMethodCallback, callback));
 }
 
+void AttestationFlow::StillRetrying(RetryData* retry_data,
+                                    const base::Closure& on_giving_up,
+                                    const base::Closure& on_retrying) {
+  base::TimeTicks now = retry_data->tick_clock->NowTicks();

[achuithb, 2016/12/05 19:53:59]

const

+  base::TimeDelta elapsed = now - retry_data->begin;

[achuithb, 2016/12/05 19:53:59]

const

+  if (elapsed + kRetryDelay > retry_data->timeout) {
+    LOG(ERROR) << "Attestation: Not prepared."
+               << " Giving up on retrying after " << elapsed << ".";
+    if (!on_giving_up.is_null())
+      on_giving_up.Run();
+  } else {
+    LOG(WARNING) << "Attestation: Not prepared yet."

[achuithb, 2016/12/05 19:53:59]

Should this be a LOG(WARNING)? Maybe VLOG(1)?

+                 << " Retrying in " << kRetryDelay << ".";
+    retry_data->retry_timer.Start(FROM_HERE, kRetryDelay, on_retrying);
+  }
+}
+
+void AttestationFlow::DoneRetrying(RetryData* retry_data,

[achuithb, 2016/12/05 19:53:59]

We don's pass naked pointers around like this with implicit ownership

+                                   const base::Closure& continuation) {
+  retry_data->retry_timer.Stop();
+  delete retry_data;

[apronin1, 2016/12/05 19:25:50]

Not a big fan of deleting using the pointer passed from above. Especially
without the ability to set the variable to NULL in the calling layer. 

And not sure if it's ok with the styleguide.
go/cppguide#Ownership_and_Smart_Pointers

Can it be somehow converted to using unique_ptr, for example, and moving it
around as necessary?

+  if (!continuation.is_null())
+    continuation.Run();
+}
+
 ServerProxy::~ServerProxy() {}
 
 PrivacyCAType ServerProxy::GetType() {
   return DEFAULT_PCA;
 }
 
 }  // namespace attestation
 }  // namespace chromeos