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

chromeos/attestation/attestation_flow.h

Index: chromeos/attestation/attestation_flow.h
diff --git a/chromeos/attestation/attestation_flow.h b/chromeos/attestation/attestation_flow.h
index 26a7292d60a9d4eab6901f21e268e31b6a799846..256914673d913fbe5749cf6ff279a28a3e4aa0ef 100644
--- a/chromeos/attestation/attestation_flow.h
+++ b/chromeos/attestation/attestation_flow.h
@@ -5,12 +5,18 @@
 #ifndef CHROMEOS_ATTESTATION_ATTESTATION_FLOW_H_
 #define CHROMEOS_ATTESTATION_ATTESTATION_FLOW_H_
 
+#include <map>
 #include <memory>
 #include <string>
 
 #include "base/callback_forward.h"
+#include "base/gtest_prod_util.h"
+#include "base/gtest_prod_util.h"

[Darren Krahn, 2016/12/06 17:31:25]

included twice

 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "chromeos/attestation/attestation_constants.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
@@ -46,11 +52,14 @@ class CHROMEOS_EXPORT ServerProxy {
 // Implements the message flow for Chrome OS attestation tasks.  Generally this
 // consists of coordinating messages between the Chrome OS attestation service
 // and the Chrome OS Privacy CA server.  Sample usage:
+//
 //    AttestationFlow flow(AsyncMethodCaller::GetInstance(),
 //                         DBusThreadManager::Get().GetCryptohomeClient(),
 //                         std::move(my_server_proxy));
 //    AttestationFlow::CertificateCallback callback = base::Bind(&MyCallback);
 //    flow.GetCertificate(ENTERPRISE_USER_CERTIFICATE, false, callback);
+//
+// This class is not thread safe.
 class CHROMEOS_EXPORT AttestationFlow {
  public:
   typedef base::Callback<void(bool success,
@@ -77,11 +86,22 @@ class CHROMEOS_EXPORT AttestationFlow {
       AttestationCertificateProfile certificate_profile,
       const std::string& request_origin);
 
+  // Creates an attestation flow which will retry for attestation
+  // preparedness for a reasonable amount of time.

[Darren Krahn, 2016/12/06 17:31:25]

nit: It does a lot of other things too. I don't think this one sub-feature needs
to be described in the constructor. If anything, the arguments should be
documented.

   AttestationFlow(cryptohome::AsyncMethodCaller* async_caller,
                   CryptohomeClient* cryptohome_client,
                   std::unique_ptr<ServerProxy> server_proxy);
   virtual ~AttestationFlow();
 
+  // Sets the timeout for attestation preparedness.
+  void SetPreparednessTimeout(base::TimeDelta preparedness_timeout) {

[Darren Krahn, 2016/12/06 17:31:25]

nit: use set_timeout() and timeout() style.

optional: I think "preparedness" is awkward. I'd prefer "ready".

+    preparedness_timeout_ = preparedness_timeout;
+  }
+  // Gets the timeout for attestation preparedness.
+  base::TimeDelta GetPreparednessTimeout() const {
+    return preparedness_timeout_;
+  }
+
   // Gets an attestation certificate for a hardware-protected key.  If a key for
   // the given profile does not exist, it will be generated and a certificate
   // request will be made to the Chrome OS Privacy CA to issue a certificate for
@@ -108,8 +128,52 @@ class CHROMEOS_EXPORT AttestationFlow {
                               bool force_new_key,
                               const CertificateCallback& callback);
 
+  // Sets the tick clock for tests.
+  void SetTickClockForTest(base::TickClock* tick_clock);
+
  private:
+  FRIEND_TEST_ALL_PREFIXES(AttestationFlowTest, CreateRetryData);
+
+  using RetryId = int16_t;
+  constexpr static RetryId kInvalidRetryId = -1;
+
+  struct RetryData;
+
+  // Creates (retrieves) retry data. These data are then identified by a
+  // retry identifier.
+  RetryId CreateRetryData();
+  RetryData& GetRetryData(RetryId retry_id);
+
   // Asynchronously initiates the attestation enrollment flow.
+  // If attestation is not ready yet, retry as needed.
+  //
+  // Parameters
+  //   on_failure - Called if any failure occurs.
+  //   next_task - Called on successful enrollment.
+  void InitiateEnroll(const base::Closure& on_failure,

[Darren Krahn, 2016/12/06 17:31:25]

Better naming please. The semantic difference between "InitiateEnroll" and
"StartEnroll" are not clear. Maybe WaitForAttestationServiceReady and
StartEnroll?

+                      const base::Closure& next_task);
+
+  // Asynchronously tries to initiate the attestation enrollment flow.
+  // If attestation is not ready yet, retry as needed.

[Darren Krahn, 2016/12/06 17:31:25]

This is identical to InitiateEnroll above, they do the same thing?

+  //
+  // Parameters
+  //   retry_data - Data to manage retries.
+  //   on_failure - Called if any failure occurs.
+  //   next_task - Called on successful enrollment.
+  void TryInitiateEnroll(RetryId retry_id,
+                         const base::Closure& on_failure,
+                         const base::Closure& next_task);
+
+  // Called when atestation is not prepared yet, to re-initiate enrollment

[Darren Krahn, 2016/12/06 17:31:25]

sp:atestation->attestation

+  // after a delay.
+  //
+  // Parameters
+  //   on_failure - Called if any failure occurs.
+  //   next_task - Called on successful enrollment.
+  void RetryInitiateEnroll(const base::Closure& on_failure,
+                           const base::Closure& next_task);
+
+  // Called when attestation is prepared, to start the actual enrollment flow.
   //
   // Parameters
   //   on_failure - Called if any failure occurs.
@@ -223,10 +287,26 @@ class CHROMEOS_EXPORT AttestationFlow {
                               const std::string& key_name,
                               const CertificateCallback& callback);
 
+  void StartRetrying(const base::Closure& on_giving_up,
+                     const base::Closure& on_retrying);
+  // Handles retries. If |retry_data| indicates that we are done retrying,
+  // runs |on_giving_up|, otherwise runs |on_retrying| after a delay.
+  void StillRetrying(RetryId retry_id,
+                     const base::Closure& on_giving_up,
+                     const base::Closure& on_retrying);
+  // Handles the end of retries. Deletes |retry_data| and runs |continuation|.
+  void DoneRetrying(RetryId retry_id, const base::Closure& continuation);

[Darren Krahn, 2016/12/06 17:31:25]

6 methods and a data structure to implement retry, this really seems to add
complexity to this class. How is this a bigger problem than
AttestationPolicyObserver::Reschedule?

+
   cryptohome::AsyncMethodCaller* async_caller_;
   CryptohomeClient* cryptohome_client_;
   std::unique_ptr<ServerProxy> server_proxy_;
 
+  base::TimeDelta preparedness_timeout_;
+  base::TickClock* tick_clock_ = nullptr;
+
+  std::map<RetryId, std::unique_ptr<RetryData>> retries_;
+  RetryId next_retry_id_ = 0;
+
   base::WeakPtrFactory<AttestationFlow> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AttestationFlow);