Added a timeout for platform verification key generation.

chrome/browser/chromeos/attestation/platform_verification_flow.h

 // Copyright 2013 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_ATTESTATION_PLATFORM_VERIFICATION_FLOW_H_
 #define CHROME_BROWSER_CHROMEOS_ATTESTATION_PLATFORM_VERIFICATION_FLOW_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "url/gurl.h"
 
 class PrefService;
 
 namespace content {
 class WebContents;
 }
 
 namespace cryptohome {
 class AsyncMethodCaller;
 }
 
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
 
 namespace chromeos {
 
 class CryptohomeClient;
 class UserManager;
 class User;
 
 namespace attestation {
 
 class AttestationFlow;
 
 // This class allows platform verification for the content protection use case.
 // All methods must only be called on the UI thread.  Example:
-//   PlatformVerificationFlow verifier;
+//   scoped_refptr<PlatformVerificationFlow> verifier =
+//       new PlatformVerificationFlow();
 //   PlatformVerificationFlow::Callback callback = base::Bind(&MyCallback);
-//   verifier.ChallengePlatformKey(my_web_contents, "my_id", "some_challenge",
-//                                 callback);
-class PlatformVerificationFlow {
+//   verifier->ChallengePlatformKey(my_web_contents, "my_id", "some_challenge",
+//                                  callback);
+//
+// This class is RefCountedThreadSafe because it may need to outlive its caller.
+// The attestation flow that needs to happen to establish a certified platform
+// key may take minutes on some hardware.  This class will timeout after a much
+// shorter time so the caller can proceed without platform verification but it
+// is important that the pending operation be allowed to finish.  If the
+// attestation flow is aborted at any stage, it will need to start over.  If we
+// use weak pointers, the attestation flow will stop when the next callback is
+// run.  So we need the instance to stay alive until the platform key is fully
+// certified so the next time ChallegePlatformKey() is invoked it will be quick.
+class PlatformVerificationFlow
+    : public base::RefCountedThreadSafe<PlatformVerificationFlow> {
  public:
   enum Result {
     SUCCESS,                // The operation succeeded.
     INTERNAL_ERROR,         // The operation failed unexpectedly.
     PLATFORM_NOT_VERIFIED,  // The platform cannot be verified.  For example:
                             // - It is not a Chrome device.
                             // - It is not running a verified OS image.
     USER_REJECTED,          // The user explicitly rejected the operation.
     POLICY_REJECTED,        // The operation is not allowed by policy/settings.
+    TIMEOUT,                // The operation timed out.
   };
 
   enum ConsentType {
     CONSENT_TYPE_NONE,         // No consent necessary.
     CONSENT_TYPE_ATTESTATION,  // Consent to use attestation.
     CONSENT_TYPE_ALWAYS,       // Consent because 'Always Ask' was requested.
   };
 
   enum ConsentResponse {
     CONSENT_RESPONSE_NONE,
@@ skipping 38 matching lines @@
   PlatformVerificationFlow();
 
   // An alternate constructor which specifies dependent objects explicitly.
   // This is useful in testing.  The caller retains ownership of all pointers.
   PlatformVerificationFlow(AttestationFlow* attestation_flow,
                            cryptohome::AsyncMethodCaller* async_caller,
                            CryptohomeClient* cryptohome_client,
                            UserManager* user_manager,
                            Delegate* delegate);
 
-  virtual ~PlatformVerificationFlow();
-
   // Invokes an asynchronous operation to challenge a platform key.  Any user
   // interaction will be associated with |web_contents|.  The |service_id| is an
   // arbitrary value but it should uniquely identify the origin of the request
   // and should not be determined by that origin; its purpose is to prevent
   // collusion between multiple services.  The |challenge| is also an arbitrary
   // value but it should be time sensitive or associated to some kind of session
   // because its purpose is to prevent certificate replay.  The |callback| will
   // be called when the operation completes.  The duration of the operation can
   // vary depending on system state, hardware capabilities, and interaction with
   // the user.
   void ChallengePlatformKey(content::WebContents* web_contents,
                             const std::string& service_id,
                             const std::string& challenge,
                             const ChallengeCallback& callback);
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* prefs);
 
   void set_testing_prefs(PrefService* testing_prefs) {
     testing_prefs_ = testing_prefs;
   }
 
   void set_testing_url(const GURL& testing_url) {
     testing_url_ = testing_url;
   }
 
+  void set_timeout_delay(int timeout_delay) {
+    timeout_delay_ = timeout_delay;
+  }
+
  private:
+  // Tracks timeout status for an asynchronous operation.
+  struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> {

[DaleCurtis, 2013/10/30 18:06:00]

Why all this instead of base::DelayTimer from base/timer/timer.h? It's self
invalidating so you don't need any additional RefCounted or WeakPtr business all
over.

[Darren Krahn, 2013/10/30 19:14:30]

AFAICT, DelayTimer offers no real benefit over PostDelayedTask which I'm using. 
This ref-counted status exists because the class is designed to support multiple
ChallengePlatformKey calls in flight simultaneously so I want status tracked
with the call, not global to the class instance.  When either of the callbacks
(timer/actual) return, they need to be aware of whether the other callback has
already run, so they both hold a ref to the status.

Maybe I'm missing something?

[DaleCurtis, 2013/10/30 20:11:52]

Ah I forgot there can be multiple calls outstanding.  I still think it'd be
cleaner in terms of less code and better tested (timer already has extensive
tests) than manually managing this TimeoutStatus structure.  Up to you though.

Also, err, sorry, I meant OneShotTimer.  If you instead make ChallengeContext a
ref counted structure (which would save on copies anyways), you can stick the
OneShotTimer in there.  Then you can use Timer::IsRunning() to determine if it
has fired or not and Timer::Stop() to prevent it from firing.

[Darren Krahn, 2013/10/30 22:24:53]

Good suggestion -- updated to use base::Timer and base::Passed.

+    bool finished;
+    bool timed_out;
+
+    TimeoutStatus() : finished(false), timed_out(false) {}
+
+   private:
+    friend class base::RefCountedThreadSafe<TimeoutStatus>;
+    ~TimeoutStatus() {}
+  };
+
+  // Holds the arguments of a ChallengePlatformKey call.  This is convenient for
+  // use with base::Bind so we don't get too many arguments.
+  struct ChallengeContext {
+    content::WebContents* web_contents;
+    std::string service_id;
+    std::string challenge;
+    ChallengeCallback callback;
+  };
+
+  friend class base::RefCountedThreadSafe<PlatformVerificationFlow>;
+  ~PlatformVerificationFlow();
+
   // Checks whether we need to prompt the user for consent before proceeding and
-  // invokes the consent UI if so.  All parameters are the same as in
-  // ChallengePlatformKey except for the additional |attestation_enrolled| which
-  // specifies whether attestation has been enrolled for this device.
-  void CheckConsent(content::WebContents* web_contents,
-                    const std::string& service_id,
-                    const std::string& challenge,
-                    const ChallengeCallback& callback,
+  // invokes the consent UI if so.  The arguments to ChallengePlatformKey are
+  // in |context| and |attestation_enrolled| specifies whether attestation has
+  // been enrolled for this device.
+  void CheckConsent(const ChallengeContext& context,
                     bool attestation_enrolled);
 
-  // A callback called when the user has given their consent response.  All
-  // parameters are the same as in ChallengePlatformKey except for the
-  // additional |consent_type| and |consent_response| which indicate the consent
-  // type and user response, respectively.  If the response indicates that the
-  // operation should proceed, this method invokes a certificate request.
-  void OnConsentResponse(content::WebContents* web_contents,
-                         const std::string& service_id,
-                         const std::string& challenge,
-                         const ChallengeCallback& callback,
+  // A callback called when the user has given their consent response.  The
+  // arguments to ChallengePlatformKey are in |context|.  |consent_type| and
+  // |consent_response| indicate the consent type and user response,
+  // respectively.  If the response indicates that the operation should proceed,
+  // this method invokes a certificate request.
+  void OnConsentResponse(const ChallengeContext& context,
                          ConsentType consent_type,
                          ConsentResponse consent_response);
 
   // A callback called when an attestation certificate request operation
-  // completes.  |service_id|, |challenge|, and |callback| are the same as in
-  // ChallengePlatformKey.  |user_id| identifies the user for which the
-  // certificate was requested.  |operation_success| is true iff the certificate
-  // request operation succeeded.  |certificate| holds the certificate for the
-  // platform key on success.  If the certificate request was successful, this
-  // method invokes a request to sign the challenge.
-  void OnCertificateReady(const std::string& user_id,
-                          const std::string& service_id,
-                          const std::string& challenge,
-                          const ChallengeCallback& callback,
+  // completes.  The arguments to ChallengePlatformKey are in |context|.
+  // |user_id| identifies the user for which the certificate was requested.
+  // |operation_success| is true iff the certificate request operation
+  // succeeded.  |certificate| holds the certificate for the platform key on
+  // success.  If the certificate request was successful, this method invokes a
+  // request to sign the challenge.  If the operation timed out prior to this
+  // method being called, this method does nothing - notably, the callback is
+  // not invoked.
+  void OnCertificateReady(const ChallengeContext& context,
+                          const std::string& user_id,
+                          TimeoutStatus* timeout_status,
                           bool operation_success,
                           const std::string& certificate);
 
+  // A callback run after a constant delay to handle timeouts for lengthy
+  // certificate requests.  |context.callback| will be invoked with a TIMEOUT
+  // result if the certificate request has not already finished.
+  void OnCertificateTimeout(const ChallengeContext& context,
+                            TimeoutStatus* timeout_status);
+
   // A callback called when a challenge signing request has completed.  The
   // |certificate| is the platform certificate for the key which signed the
-  // |challenge|.  |callback| is the same as in ChallengePlatformKey.
+  // |challenge|.  The arguments to ChallengePlatformKey are in |context|.
   // |operation_success| is true iff the challenge signing operation was
   // successful.  If it was successful, |response_data| holds the challenge
-  // response and the method will invoke |callback|.
-  void OnChallengeReady(const std::string& certificate,
-                        const std::string& challenge,
-                        const ChallengeCallback& callback,
+  // response and the method will invoke |context.callback|.
+  void OnChallengeReady(const ChallengeContext& context,
+                        const std::string& certificate,
                         bool operation_success,
                         const std::string& response_data);
 
   // Gets prefs associated with the given |web_contents|.  If prefs have been
   // set explicitly using set_testing_prefs(), then these are always returned.
   // If no prefs are associated with |web_contents| then NULL is returned.
   PrefService* GetPrefs(content::WebContents* web_contents);
 
   // Gets the URL associated with the given |web_contents|.  If a URL as been
   // set explicitly using set_testing_url(), then this value is always returned.
@@ skipping 38 matching lines @@
 
   AttestationFlow* attestation_flow_;
   scoped_ptr<AttestationFlow> default_attestation_flow_;
   cryptohome::AsyncMethodCaller* async_caller_;
   CryptohomeClient* cryptohome_client_;
   UserManager* user_manager_;
   Delegate* delegate_;
   scoped_ptr<Delegate> default_delegate_;
   PrefService* testing_prefs_;
   GURL testing_url_;
-
-  // Note: This should remain the last member so it'll be destroyed and
-  // invalidate the weak pointers before any other members are destroyed.
-  base::WeakPtrFactory<PlatformVerificationFlow> weak_factory_;
+  int timeout_delay_;

[DaleCurtis, 2013/10/30 20:11:52]

base::TimeDelta

[Darren Krahn, 2013/10/30 22:24:53]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(PlatformVerificationFlow);
 };
 
 }  // namespace attestation
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_ATTESTATION_PLATFORM_VERIFICATION_FLOW_H_