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

chromeos/attestation/attestation_flow.cc

Index: chromeos/attestation/attestation_flow.cc
diff --git a/chromeos/attestation/attestation_flow.cc b/chromeos/attestation/attestation_flow.cc
index c82cbe554008bf6f81cde6267570743de5e8c3b5..d8d8145d14b6b87406ddf9ae16ea68add1103ec2 100644
--- a/chromeos/attestation/attestation_flow.cc
+++ b/chromeos/attestation/attestation_flow.cc
@@ -7,6 +7,8 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/ptr_util.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"
@@ -17,6 +19,33 @@ namespace attestation {
 
 namespace {
 
+// Default backoff policy.
+const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
+    // Number of initial errors (in sequence) to ignore before applying
+    // exponential back-off rules.
+    0,
+
+    // Initial delay for exponential back-off in ms.
+    2000,
+
+    // Factor by which the waiting time will be multiplied.
+    2,
+
+    // Fuzzing percentage. ex: 10% will spread requests randomly
+    // between 90%-100% of the calculated time.
+    0,
+
+    // Maximum amount of time we are willing to delay our request in ms.
+    -1,
+
+    // Time to keep an entry from being discarded even when it
+    // has no significant state, -1 to never discard.
+    -1,
+
+    // Don't use initial delay unless the last request was an error.
+    false,
+};
+
 // Redirects to one of three callbacks based on a boolean value and dbus call
 // status.
 //
@@ -29,10 +58,11 @@ namespace {
 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;
@@ -95,6 +125,8 @@ AttestationFlow::AttestationFlow(cryptohome::AsyncMethodCaller* async_caller,
     : async_caller_(async_caller),
       cryptohome_client_(cryptohome_client),
       server_proxy_(std::move(server_proxy)),
+      retry_timer_(new base::OneShotTimer()),
+      retry_backoff_(&kDefaultBackoffPolicy),
       weak_factory_(this) {}
 
 AttestationFlow::~AttestationFlow() {
@@ -106,25 +138,71 @@ void AttestationFlow::GetCertificate(
     const std::string& request_origin,
     bool force_new_key,
     const CertificateCallback& callback) {
+  // Fail requests if we are backing off right now.
+  if (retry_backoff_.ShouldRejectRequest()) {

[The one and only Dr. Crash, 2016/12/02 05:57:49]

I actually think that the behavior I want is to stack the waiting callbacks.
There is no reason to go try ensure the first certificate request will succeed
by retrying, but then when we back off simply drop the certificate requests on
the floor. Also I am not sure that ShouldRejectRequest() will return true after
a first failure only (that doesn't seem right), so a second call to
GetCertificate() while a call is pending would mean the first call would never
have its callback called.

So instead I am thinking of managing a stack (vector) of the callbacks given to
GetCertificate(), push callbacks onto that stack and then process them in order
once we decide what to do.

What do you think?

+    if (!callback.is_null())
+      callback.Run(false, "");
+    return;
+  }
   // 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 =
+      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::SetRetryTimerForTest(
+    std::unique_ptr<base::OneShotTimer> retry_timer) {
+  retry_timer_ = std::move(retry_timer);
+}
+
+void AttestationFlow::InitiateEnroll(const base::Closure& on_failure,
+                                     const base::Closure& next_task) {
+  base::Closure do_enroll =
+      base::Bind(&AttestationFlow::StartEnroll, weak_factory_.GetWeakPtr(),
+                 on_failure, next_task);
+  base::Closure retry_initiate_enroll =
+      base::Bind(&AttestationFlow::RetryInitiateEnroll,
+                 weak_factory_.GetWeakPtr(), on_failure, next_task);
+  cryptohome_client_->TpmAttestationIsPrepared(
+      base::Bind(&DBusBoolRedirectCallback, do_enroll, retry_initiate_enroll,
+                 on_failure, "check for attestation readiness"));
+}
+
+void AttestationFlow::RetryInitiateEnroll(const base::Closure& on_failure,
+                                          const base::Closure& next_task) {
+  if (initiate_enroll_retries_ == 0) {
+    LOG(ERROR) << "Attestation: not ready for attestation."
+               << " Giving up on retrying.";
+    if (!on_failure.is_null())
+      on_failure.Run();
+  } else {
+    if (initiate_enroll_retries_ > 0)
+      --initiate_enroll_retries_;
+    retry_backoff_.InformOfRequest(false);
+    auto retry_delay = retry_backoff_.GetTimeUntilRelease().InMilliseconds();
+    LOG(WARNING) << "Attestation: not ready for attestation yet."
+                 << " Retrying in " << retry_delay << " ms.";
+    retry_timer_->Start(
+        FROM_HERE, retry_backoff_.GetTimeUntilRelease(),
+        base::Bind(&AttestationFlow::InitiateEnroll, weak_factory_.GetWeakPtr(),
+                   on_failure, next_task));
+  }
 }
 
 void AttestationFlow::StartEnroll(const base::Closure& on_failure,
                                   const base::Closure& next_task) {
+  // We are done retrying the enrollment initiation (i.e. TPM is ready).
+  retry_backoff_.Reset();
   // Get the attestation service to create a Privacy CA enrollment request.
   async_caller_->AsyncTpmAttestationCreateEnrollRequest(
       server_proxy_->GetType(),
@@ -222,7 +300,8 @@ void AttestationFlow::StartCertificateRequest(
     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"));
   }
 }