use AttestationClient to get key info for tpm challenge key.

We are deprecating attestation methods by CryptohomeClient.

This CL also includes a simplied flow by consolidating
TpmAttestationDoesKeyExist and TpmAttestationGetCertificate, of which
results come from the same methods of attestation service anyway.

BUG=b:158955123
TEST=unit_tests.

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

chrome/browser/chromeos/attestation/tpm_challenge_key_subtle.cc

@@ -26,6 +26,8 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/cryptohome/async_method_caller.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
+#include "chromeos/dbus/attestation/attestation_client.h"
+#include "chromeos/dbus/attestation/interface.pb.h"
 #include "chromeos/dbus/constants/attestation_constants.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/tpm/install_attributes.h"
@@ -402,13 +404,14 @@ void TpmChallengeKeySubtleImpl::GetEnrollmentPreparationsCallback(
     return;
   }
 
-  // Attestation is available, see if the key we need already exists.
-  CryptohomeClient::Get()->TpmAttestationDoesKeyExist(
-      key_type_,
-      cryptohome::CreateAccountIdentifierFromAccountId(GetAccountId()),
-      key_name_,
-      base::BindOnce(&TpmChallengeKeySubtleImpl::DoesKeyExistCallback,
-                     weak_factory_.GetWeakPtr()));
+  ::attestation::GetKeyInfoRequest request;
+  request.set_username(
+      cryptohome::CreateAccountIdentifierFromAccountId(GetAccountId())
+          .account_id());
+  request.set_key_label(key_name_);
+  AttestationClient::Get()->GetKeyInfo(
+      request, base::BindOnce(&TpmChallengeKeySubtleImpl::DoesKeyExistCallback,
+                              weak_factory_.GetWeakPtr()));
 }
 
 void TpmChallengeKeySubtleImpl::PrepareKeyErrorHandlerCallback(
@@ -430,17 +433,21 @@ void TpmChallengeKeySubtleImpl::PrepareKeyErrorHandlerCallback(
 }
 
 void TpmChallengeKeySubtleImpl::DoesKeyExistCallback(
-    base::Optional<bool> result) {
+    const ::attestation::GetKeyInfoReply& reply) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!result.has_value()) {
-    std::move(callback_).Run(Result::MakeError(ResultCode::kDbusError));
+  if (reply.status() != ::attestation::STATUS_SUCCESS &&
+      reply.status() != ::attestation::STATUS_INVALID_PARAMETER) {
+    std::move(callback_).Run(
+        Result::MakeError(reply.status() == ::attestation::STATUS_DBUS_ERROR
+                              ? ResultCode::kDbusError
+                              : ResultCode::kAttestationServiceInternalError));
     return;
   }
 
-  if (result.value()) {
+  if (reply.status() == ::attestation::STATUS_SUCCESS) {
     // The key exists. Do nothing more.
-    GetPublicKey();
+    PrepareKeyFinished(reply);
     return;
   }
 
@@ -501,30 +508,31 @@ void TpmChallengeKeySubtleImpl::GetCertificateCallback(
 void TpmChallengeKeySubtleImpl::GetPublicKey() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  CryptohomeClient::Get()->TpmAttestationGetPublicKey(
-      key_type_,
-      cryptohome::CreateAccountIdentifierFromAccountId(GetAccountId()),
-      key_name_,
-      base::BindOnce(&TpmChallengeKeySubtleImpl::PrepareKeyFinished,
-                     weak_factory_.GetWeakPtr()));
+  ::attestation::GetKeyInfoRequest request;
+  request.set_username(
+      cryptohome::CreateAccountIdentifierFromAccountId(GetAccountId())
+          .account_id());
+  request.set_key_label(key_name_);
+  AttestationClient::Get()->GetKeyInfo(
+      request, base::BindOnce(&TpmChallengeKeySubtleImpl::PrepareKeyFinished,
+                              weak_factory_.GetWeakPtr()));
 }
 
 void TpmChallengeKeySubtleImpl::PrepareKeyFinished(
-    base::Optional<CryptohomeClient::TpmAttestationDataResult>
-        prepare_key_result) {
+    const ::attestation::GetKeyInfoReply& reply) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!prepare_key_result.has_value() || !prepare_key_result->success) {
+  if (reply.status() != ::attestation::STATUS_SUCCESS) {
     std::move(callback_).Run(
         Result::MakeError(ResultCode::kGetPublicKeyFailedError));
     return;
   }
 
   if (profile_ && will_register_key_) {
-    public_key_ = prepare_key_result->data;
+    public_key_ = reply.public_key();
   }
 
-  std::move(callback_).Run(Result::MakePublicKey(prepare_key_result->data));
+  std::move(callback_).Run(Result::MakePublicKey(reply.public_key()));
 }
 
 void TpmChallengeKeySubtleImpl::StartSignChallengeStep(

chrome/browser/chromeos/attestation/tpm_challenge_key_subtle.h

@@ -177,13 +177,11 @@ class TpmChallengeKeySubtleImpl final : public TpmChallengeKeySubtle {
   // EmptyAccountId() if GetUser() returns nullptr.
   AccountId GetAccountId() const;
 
-  // Actually prepares a key after all checks are passed and if |can_continue|
+  // Actually prepares a key after all checks are passed and if `can_continue`
   // is true.
   void PrepareKey(bool can_continue);
-  // Returns a public key (or an error) via |prepare_key_callback_|.
-  void PrepareKeyFinished(
-      base::Optional<CryptohomeClient::TpmAttestationDataResult>
-          prepare_key_result);
+  // Returns a public key (or an error) via `callback_`.
+  void PrepareKeyFinished(const ::attestation::GetKeyInfoReply& reply);
 
   void SignChallengeCallback(
       const ::attestation::SignEnterpriseChallengeReply& reply);
@@ -201,7 +199,7 @@ class TpmChallengeKeySubtleImpl final : public TpmChallengeKeySubtle {
   void GetEnrollmentPreparationsCallback(
       const ::attestation::GetEnrollmentPreparationsReply& reply);
   void PrepareKeyErrorHandlerCallback(base::Optional<bool> is_tpm_enabled);
-  void DoesKeyExistCallback(base::Optional<bool> result);
+  void DoesKeyExistCallback(const ::attestation::GetKeyInfoReply& reply);
   void AskForUserConsent(base::OnceCallback<void(bool)> callback) const;
   void AskForUserConsentCallback(bool result);
   void GetCertificateCallback(AttestationStatus status,

chrome/browser/chromeos/attestation/tpm_challenge_key_subtle_unittest.cc

@@ -37,6 +37,7 @@
 
 using base::test::RunOnceCallback;
 using testing::_;
+using testing::Invoke;
 using testing::StrictMock;
 
 namespace chromeos {
@@ -48,6 +49,7 @@ constexpr char kTestUserDomain[] = "google.com";
 constexpr char kTestUserGaiaId[] = "test_gaia_id";
 constexpr char kEmptyKeyName[] = "";
 constexpr char kNonDefaultKeyName[] = "key_name_123";
+constexpr char kFakeCertificate[] = "fake_cert";
 
 const char* GetDefaultKeyName(AttestationKeyType type) {
   switch (type) {
@@ -112,6 +114,42 @@ struct CallbackHolder {
   T callback;
 };
 
+//================= MockableFakeAttestationFlow ================================
+
+class MockableFakeAttestationFlow : public MockAttestationFlow {
+ public:
+  MockableFakeAttestationFlow() {
+    ON_CALL(*this, GetCertificate(_, _, _, _, _, _))
+        .WillByDefault(
+            Invoke(this, &MockableFakeAttestationFlow::GetCertificateInternal));
+  }
+  ~MockableFakeAttestationFlow() override = default;
+  void set_status(AttestationStatus status) { status_ = status; }
+
+ private:
+  void GetCertificateInternal(
+      AttestationCertificateProfile /*certificate_profile*/,
+      const AccountId& account_id,
+      const std::string& /*request_origin*/,
+      bool /*force_new_key*/,
+      const std::string& key_name,
+      CertificateCallback callback) {
+    std::string certificate;
+    if (status_ == ATTESTATION_SUCCESS) {
+      certificate = certificate_;
+      AttestationClient::Get()
+          ->GetTestInterface()
+          ->GetMutableKeyInfoReply(cryptohome::Identification(account_id).id(),
+                                   key_name)
+          ->set_public_key(public_key_);
+    }
+    std::move(callback).Run(status_, certificate);
+  }
+  AttestationStatus status_ = ATTESTATION_SUCCESS;
+  const std::string certificate_ = kFakeCertificate;
+  const std::string public_key_ = GetPublicKey();
+};
+
 //================== TpmChallengeKeySubtleTest =================================
 
 class TpmChallengeKeySubtleTest : public ::testing::Test {
@@ -154,7 +192,7 @@ class TpmChallengeKeySubtleTest : public ::testing::Test {
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
-  StrictMock<chromeos::attestation::MockAttestationFlow> mock_attestation_flow_;
+  StrictMock<MockableFakeAttestationFlow> mock_attestation_flow_;
   cryptohome::MockAsyncMethodCaller* mock_async_method_caller_ = nullptr;
   chromeos::FakeCryptohomeClient cryptohome_client_;
   std::unique_ptr<platform_keys::MockKeyPermissionsManager>
@@ -185,10 +223,6 @@ TpmChallengeKeySubtleTest::TpmChallengeKeySubtleTest()
   challenge_key_subtle_ = std::make_unique<TpmChallengeKeySubtleImpl>(
       &mock_attestation_flow_, &mock_cert_uploader_);
 
-  cryptohome_client_.set_tpm_attestation_public_key(
-      CryptohomeClient::TpmAttestationDataResult{/*success=*/true,
-                                                 GetPublicKey()});
-
   // By default make it reply that the certificate is already uploaded.
   ON_CALL(mock_cert_uploader_, WaitForUploadComplete)
       .WillByDefault(RunOnceCallback<0>(true));
@@ -395,7 +429,10 @@ TEST_F(TpmChallengeKeySubtleTest, UserKeyDeviceAttestationDisabled) {
 TEST_F(TpmChallengeKeySubtleTest, DoesKeyExistDbusFailed) {
   InitSigninProfile();
 
-  cryptohome_client_.set_tpm_attestation_does_key_exist_should_succeed(false);
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->GetMutableKeyInfoReply(/*username=*/"", kEnterpriseMachineKey)
+      ->set_status(::attestation::STATUS_DBUS_ERROR);
 
   RunOneStepAndExpect(
       KEY_DEVICE, /*will_register_key=*/false, kEmptyKeyName,
@@ -406,11 +443,8 @@ TEST_F(TpmChallengeKeySubtleTest, GetCertificateFailed) {
   InitSigninProfile();
   const AttestationKeyType key_type = KEY_DEVICE;
 
-  EXPECT_CALL(mock_attestation_flow_,
-              GetCertificate(_, _, _, _, GetDefaultKeyName(key_type), _))
-      .WillOnce(RunOnceCallback<5>(
-          chromeos::attestation::ATTESTATION_UNSPECIFIED_FAILURE,
-          /*pem_certificate_chain=*/""));
+  mock_attestation_flow_.set_status(ATTESTATION_UNSPECIFIED_FAILURE);
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, _, _));
 
   RunOneStepAndExpect(
       key_type, /*will_register_key=*/false, kEmptyKeyName,
@@ -422,12 +456,10 @@ TEST_F(TpmChallengeKeySubtleTest, KeyExists) {
   InitSigninProfile();
   const AttestationKeyType key_type = KEY_DEVICE;
 
-  cryptohome_client_.SetTpmAttestationDeviceCertificate("attest-ent-machine",
-                                                        std::string());
-  // GetCertificate must not be called if the key exists.
-  EXPECT_CALL(mock_attestation_flow_,
-              GetCertificate(_, _, _, _, GetDefaultKeyName(key_type), _))
-      .Times(0);
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->GetMutableKeyInfoReply(/*username=*/"", kEnterpriseMachineKey)
+      ->set_public_key(GetPublicKey());
 
   RunOneStepAndExpect(key_type, /*will_register_key=*/false, kEmptyKeyName,
                       TpmChallengeKeyResult::MakePublicKey(GetPublicKey()));
@@ -490,10 +522,7 @@ TEST_F(TpmChallengeKeySubtleTest, DeviceKeyNotRegisteredSuccess) {
   const AttestationKeyType key_type = KEY_DEVICE;
   const char* const key_name = GetDefaultKeyName(key_type);
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   ::attestation::SignEnterpriseChallengeRequest expected_request;
   expected_request.set_key_label(key_name);
@@ -513,10 +542,7 @@ TEST_F(TpmChallengeKeySubtleTest, DeviceKeyRegisteredSuccess) {
   const AttestationKeyType key_type = KEY_DEVICE;
   const char* const key_name = kNonDefaultKeyName;
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   ::attestation::SignEnterpriseChallengeRequest expected_request;
   expected_request.set_key_label(GetDefaultKeyName(key_type));
@@ -546,10 +572,7 @@ TEST_F(TpmChallengeKeySubtleTest, UserKeyNotRegisteredSuccess) {
   const AttestationKeyType key_type = KEY_USER;
   const char* const key_name = GetDefaultKeyName(key_type);
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   ::attestation::SignEnterpriseChallengeRequest expected_request;
   expected_request.set_username(kTestUserEmail);
@@ -571,10 +594,7 @@ TEST_F(TpmChallengeKeySubtleTest, UserKeyRegisteredSuccess) {
   const AttestationKeyType key_type = KEY_USER;
   const char* const key_name = kNonDefaultKeyName;
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   ::attestation::SignEnterpriseChallengeRequest expected_request;
   expected_request.set_username(kTestUserEmail);
@@ -603,10 +623,7 @@ TEST_F(TpmChallengeKeySubtleTest, SignChallengeFailed) {
   const AttestationKeyType key_type = KEY_DEVICE;
 
   EXPECT_CALL(mock_attestation_flow_,
-              GetCertificate(_, _, _, _, GetDefaultKeyName(key_type), _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+              GetCertificate(_, _, _, _, GetDefaultKeyName(key_type), _));
 
   // The signing operations fails because we don't allowlist any key.
   RunTwoStepsAndExpect(
@@ -696,12 +713,14 @@ TEST_F(TpmChallengeKeySubtleTest, GetPublicKeyFailed) {
   InitAffiliatedProfile();
   const char* const key_name = kNonDefaultKeyName;
 
-  cryptohome_client_.set_tpm_attestation_public_key(base::nullopt);
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  // Force the attestation client to report absence even after successful
+  // attestation flow.
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->GetMutableKeyInfoReply(kTestUserEmail, key_name)
+      ->set_status(::attestation::STATUS_INVALID_PARAMETER);
 
   RunOneStepAndExpect(KEY_DEVICE, /*will_register_key=*/true, key_name,
                       TpmChallengeKeyResult::MakeError(
@@ -719,10 +738,7 @@ TEST_F(TpmChallengeKeySubtleTest, WaitForCertificateUploaded) {
       .WillOnce(
           testing::Invoke(&callback_holder, &CallbackHolderT::SaveCallback));
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   CallbackObserver callback_observer;
   challenge_key_subtle_->StartPrepareKeyStep(
@@ -751,10 +767,7 @@ TEST_F(TpmChallengeKeySubtleTest, NoCertificateUploaderSuccess) {
   challenge_key_subtle_ = std::make_unique<TpmChallengeKeySubtleImpl>(
       &mock_attestation_flow_, /*machine_certificate_uploader=*/nullptr);
 
-  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _))
-      .WillOnce(
-          RunOnceCallback<5>(chromeos::attestation::ATTESTATION_SUCCESS,
-                             /*pem_certificate_chain=*/"fake_certificate"));
+  EXPECT_CALL(mock_attestation_flow_, GetCertificate(_, _, _, _, key_name, _));
 
   RunOneStepAndExpect(KEY_USER, /*will_register_key=*/true, key_name,
                       TpmChallengeKeyResult::MakePublicKey(GetPublicKey()));

chrome/browser/chromeos/login/saml/saml_browsertest.cc

@@ -1874,6 +1874,13 @@ INSTANTIATE_TEST_SUITE_P(All, SAMLPasswordAttributesTest, testing::Bool());
 
 void FakeGetCertificateCallbackTrue(
     attestation::AttestationFlow::CertificateCallback callback) {
+  // In reality, attestation service holds the certificate after a successful
+  // attestation flow.
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->GetMutableKeyInfoReply(/*username=*/"",
+                               attestation::kEnterpriseMachineKey)
+      ->set_certificate("certificate");
   std::move(callback).Run(attestation::ATTESTATION_SUCCESS, "certificate");
 }