AttestationFlow use preparation check consistent with chrome os.

Previously we only check if attestation service is prepared with default
ACA. We change to the check against either default or test ACA because
this is how cryptohome implements TpmIsAttestationPrepared.

Also, we explicitly check the status code returned by attestation
service to make the problem flow more robust for future-proof reason.

Note that this change doesn't introduce any effective behavioral change;
this is because attesataion service always prepares enrollment with both
ACA types, and the enrollment preparation table is always empty if the
returned status is bad.

Admittedly it will become a waste CL right after we switch to platform
side attestation flow, which we are aiming at in the near future, but
it might be still worth doing it for better history reading in the
future because only checking default ACA is confusing.

BUG=b:158955123
TEST=build ok.

Change-Id: I341c8fc68b476805d700b97cd2fb6afadd30ffba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466003
Commit-Queue: Leo Lai <cylai@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818889}

chromeos/attestation/attestation_flow.cc

@@ -34,9 +34,6 @@ constexpr uint16_t kReadyTimeoutInSeconds = 60;
 // attestation.
 constexpr uint16_t kRetryDelayInMilliseconds = 300;
 
-constexpr ::attestation::ACAType kDefaultAcaType =
-    ::attestation::ACAType::DEFAULT_ACA;
-
 void DBusCertificateMethodCallback(
     AttestationFlow::CertificateCallback callback,
     base::Optional<CryptohomeClient::TpmAttestationDataResult> result) {
@@ -52,16 +49,6 @@ void DBusCertificateMethodCallback(
   }
 }
 
-bool IsPreparedWith(const ::attestation::GetEnrollmentPreparationsReply& reply,
-                    ::attestation::ACAType aca_type) {
-  for (const auto& preparation : reply.enrollment_preparations()) {
-    if (preparation.first == aca_type) {
-      return preparation.second;
-    }
-  }
-  return false;
-}
-
 }  // namespace
 
 AttestationKeyType AttestationFlow::GetKeyTypeForProfile(
@@ -137,7 +124,6 @@ void AttestationFlow::WaitForAttestationPrepared(
     base::TimeTicks end_time,
     base::OnceCallback<void(bool)> callback) {
   ::attestation::GetEnrollmentPreparationsRequest request;
-  request.set_aca_type(kDefaultAcaType);
   attestation_client_->GetEnrollmentPreparations(
       request, base::BindOnce(&AttestationFlow::OnPreparedCheckComplete,
                               weak_factory_.GetWeakPtr(), end_time,
@@ -148,8 +134,7 @@ void AttestationFlow::OnPreparedCheckComplete(
     base::TimeTicks end_time,
     base::OnceCallback<void(bool)> callback,
     const ::attestation::GetEnrollmentPreparationsReply& reply) {
-  if (reply.status() == ::attestation::STATUS_SUCCESS &&
-      IsPreparedWith(reply, kDefaultAcaType)) {
+  if (AttestationClient::IsAttestationPrepared(reply)) {
     // Get the attestation service to create a Privacy CA enrollment request.
     async_caller_->AsyncTpmAttestationCreateEnrollRequest(
         server_proxy_->GetType(),

chromeos/attestation/attestation_flow_unittest.cc

@@ -269,6 +269,42 @@ TEST_F(AttestationFlowTest, GetCertificate_Attestation_Never_Prepared) {
   Run();
 }
 
+TEST_F(AttestationFlowTest, GetCertificate_Attestation_Never_Confirm_Prepared) {
+  StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
+  async_caller.SetUp(false, cryptohome::MOUNT_ERROR_NONE);
+
+  chromeos::FakeCryptohomeClient client;
+  client.set_tpm_attestation_is_enrolled(false);
+  chromeos::AttestationClient::Get()
+      ->GetTestInterface()
+      ->ConfigureEnrollmentPreparationsStatus(
+          ::attestation::STATUS_NOT_AVAILABLE);
+
+  // We're not expecting any server calls in this case; StrictMock will verify.
+  std::unique_ptr<MockServerProxy> proxy(new StrictMock<MockServerProxy>());
+  EXPECT_CALL(*proxy, GetType()).WillRepeatedly(DoDefault());
+
+  StrictMock<MockObserver> observer;
+  EXPECT_CALL(observer,
+              MockCertificateCallback(ATTESTATION_UNSPECIFIED_FAILURE, ""))
+      .Times(1);
+  AttestationFlow::CertificateCallback callback =
+      base::BindOnce(&AttestationFlowTest::QuitRunLoopCertificateCallback,
+                     base::Unretained(this),
+                     base::Bind(&MockObserver::MockCertificateCallback,
+                                base::Unretained(&observer)));
+
+  std::unique_ptr<ServerProxy> proxy_interface(proxy.release());
+  AttestationFlow flow(&async_caller, &client, std::move(proxy_interface));
+  flow.set_ready_timeout(base::TimeDelta::FromMilliseconds(20));
+  flow.set_retry_delay(base::TimeDelta::FromMilliseconds(6));
+  flow.GetCertificate(PROFILE_ENTERPRISE_USER_CERTIFICATE, EmptyAccountId(),
+                      "fake_origin", true, std::string() /* key_name */,
+                      std::move(callback));
+
+  Run();
+}
+
 TEST_F(AttestationFlowTest, GetCertificate_NoEK) {
   StrictMock<cryptohome::MockAsyncMethodCaller> async_caller;
   async_caller.SetUp(false, cryptohome::MOUNT_ERROR_NONE);

chromeos/dbus/attestation/attestation_client.cc

@@ -9,6 +9,7 @@
 #include <google/protobuf/message_lite.h>
 
 #include "base/bind.h"
+#include "base/check_op.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
@@ -311,4 +312,18 @@ AttestationClient* AttestationClient::Get() {
   return g_instance;
 }
 
+// static
+bool AttestationClient::IsAttestationPrepared(
+    const ::attestation::GetEnrollmentPreparationsReply& reply) {
+  if (reply.status() != ::attestation::STATUS_SUCCESS) {
+    return false;
+  }
+  for (const auto& preparation : reply.enrollment_preparations()) {
+    if (preparation.second) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace chromeos

chromeos/dbus/attestation/attestation_client.h

@@ -84,6 +84,12 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) AttestationClient {
     // the |sequence| one-by-one until all the elements are consumed.
     virtual void ConfigureEnrollmentPreparationsSequence(
         std::deque<bool> sequence) = 0;
+    // Injects a bad status to `GetEnrollmentPreparations()` calls. By design,
+    // this only accepts bad status so |STATUS_SUCCESS| is seen as an illegal
+    // input and abort the program. To recover the fake behavior to successful
+    // calls, call ConfigureEnrollmentPreparations(Sequence)?.
+    virtual void ConfigureEnrollmentPreparationsStatus(
+        ::attestation::AttestationStatus status) = 0;
 
     // Allowlists |request| so the certificate requests that comes in afterwards
     // will get a fake certificate. if any alias of |request| has been
@@ -110,6 +116,11 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) AttestationClient {
   // Returns the global instance which may be null if not initialized.
   static AttestationClient* Get();
 
+  // Checks if |reply| indicates the attestation service is prepared with any
+  // ACA.
+  static bool IsAttestationPrepared(
+      const ::attestation::GetEnrollmentPreparationsReply& reply);
+
   // Attestation daemon D-Bus method calls. See org.chromium.Attestation.xml and
   // the corresponding protobuf definitions in Chromium OS code for the
   // documentation of the methods and request/ messages.

chromeos/dbus/attestation/fake_attestation_client.cc

@@ -99,17 +99,31 @@ void FakeAttestationClient::RegisterKeyWithChapsToken(
 void FakeAttestationClient::GetEnrollmentPreparations(
     const ::attestation::GetEnrollmentPreparationsRequest& request,
     GetEnrollmentPreparationsCallback callback) {
-  bool is_prepared = is_prepared_;
-  // Override the state if there is a customized sequence.
-  if (!preparation_sequences_.empty()) {
-    is_prepared = preparation_sequences_.front();
-    preparation_sequences_.pop_front();
-  }
-
   ::attestation::GetEnrollmentPreparationsReply reply;
-  if (is_prepared) {
-    (*reply.mutable_enrollment_preparations())[request.aca_type()] = true;
+  reply.set_status(preparations_status_);
+
+  if (reply.status() == ::attestation::STATUS_SUCCESS) {
+    bool is_prepared = is_prepared_;
+    // Override the state if there is a customized sequence.
+    if (!preparation_sequences_.empty()) {
+      is_prepared = preparation_sequences_.front();
+      preparation_sequences_.pop_front();
+    }
+    if (is_prepared) {
+      std::vector<::attestation::ACAType> prepared_types;
+      // As we do in the attestation service, if the ACA type is not specified,
+      // returns the statuses with all the possible ACA types.
+      if (request.has_aca_type()) {
+        prepared_types = {request.aca_type()};
+      } else {
+        prepared_types = {::attestation::DEFAULT_ACA, ::attestation::TEST_ACA};
+      }
+      for (const auto& type : prepared_types) {
+        (*reply.mutable_enrollment_preparations())[type] = true;
+      }
+    }
   }
+
   PostProtoResponse(std::move(callback), reply);
 }
 
@@ -226,14 +240,22 @@ void FakeAttestationClient::GetCertifiedNvIndex(
 }
 
 void FakeAttestationClient::ConfigureEnrollmentPreparations(bool is_prepared) {
+  preparations_status_ = ::attestation::STATUS_SUCCESS;
   is_prepared_ = is_prepared;
 }
 
 void FakeAttestationClient::ConfigureEnrollmentPreparationsSequence(
     std::deque<bool> sequence) {
+  preparations_status_ = ::attestation::STATUS_SUCCESS;
   preparation_sequences_ = std::move(sequence);
 }
 
+void FakeAttestationClient::ConfigureEnrollmentPreparationsStatus(
+    ::attestation::AttestationStatus status) {
+  CHECK_NE(status, ::attestation::STATUS_SUCCESS);
+  preparations_status_ = status;
+}
+
 void FakeAttestationClient::AllowlistCertificateRequest(
     const ::attestation::GetCertificateRequest& request) {
   for (const auto& req : allowlisted_requests_) {

chromeos/dbus/attestation/fake_attestation_client.h

@@ -95,12 +95,16 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) FakeAttestationClient
   void ConfigureEnrollmentPreparations(bool is_prepared) override;
   void ConfigureEnrollmentPreparationsSequence(
       std::deque<bool> sequence) override;
+  void ConfigureEnrollmentPreparationsStatus(
+      ::attestation::AttestationStatus status) override;
   void AllowlistCertificateRequest(
       const ::attestation::GetCertificateRequest& request) override;
 
   AttestationClient::TestInterface* GetTestInterface() override;
 
  private:
+  ::attestation::AttestationStatus preparations_status_ =
+      ::attestation::STATUS_SUCCESS;
   bool is_prepared_ = true;
   std::deque<bool> preparation_sequences_;