use AttestationClient to sign simple challenge for content protection.

We are deprecating attestation methods by CryptohomeClient.

BUG=b:158955123
TEST=unit_tests.

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

chrome/browser/chromeos/attestation/platform_verification_flow.cc

@@ -303,13 +303,14 @@ void PlatformVerificationFlow::OnCertificateReady(
   bool is_expiring_soon = (expiry_status == EXPIRY_STATUS_EXPIRING_SOON);
   std::string key_name = kContentProtectionKeyPrefix + context->data.service_id;
   std::string challenge = context->data.challenge;
-  cryptohome::AsyncMethodCaller::DataCallback cryptohome_callback =
-      base::BindOnce(&PlatformVerificationFlow::OnChallengeReady, this,
-                     std::move(*context).data, account_id, certificate_chain,
-                     is_expiring_soon);
-  async_caller_->TpmAttestationSignSimpleChallenge(
-      KEY_USER, cryptohome::Identification(account_id), std::move(key_name),
-      std::move(challenge), std::move(cryptohome_callback));
+  ::attestation::SignSimpleChallengeRequest request;
+  request.set_username(cryptohome::Identification(account_id).id());
+  request.set_key_label(std::move(key_name));
+  request.set_challenge(std::move(challenge));
+  AttestationClient::Get()->SignSimpleChallenge(
+      request, base::BindOnce(&PlatformVerificationFlow::OnChallengeReady, this,
+                              std::move(*context).data, account_id,
+                              certificate_chain, is_expiring_soon));
 }
 
 void PlatformVerificationFlow::OnCertificateTimeout(
@@ -323,15 +324,16 @@ void PlatformVerificationFlow::OnChallengeReady(
     const AccountId& account_id,
     const std::string& certificate_chain,
     bool is_expiring_soon,
-    bool operation_success,
-    const std::string& response_data) {
-  if (!operation_success) {
-    LOG(ERROR) << "PlatformVerificationFlow: Failed to sign challenge.";
+    const ::attestation::SignSimpleChallengeReply& reply) {
+  if (reply.status() != ::attestation::STATUS_SUCCESS) {
+    LOG(ERROR) << "PlatformVerificationFlow: Failed to sign challenge: "
+               << reply.status();
     ReportError(std::move(context).callback, INTERNAL_ERROR);
     return;
   }
   chromeos::attestation::SignedData signed_data_pb;
-  if (response_data.empty() || !signed_data_pb.ParseFromString(response_data)) {
+  if (reply.challenge_response().empty() ||
+      !signed_data_pb.ParseFromString(reply.challenge_response())) {
     LOG(ERROR) << "PlatformVerificationFlow: Failed to parse response data.";
     ReportError(std::move(context).callback, INTERNAL_ERROR);
     return;

chrome/browser/chromeos/attestation/platform_verification_flow.h

@@ -215,20 +215,18 @@ class PlatformVerificationFlow
       scoped_refptr<base::RefCountedData<ChallengeContext>> context);
 
   // A callback called when a challenge signing request has completed.  The
-  // |certificate_chain| is the platform certificate chain for the key which
-  // signed the |challenge|.  The arguments to ChallengePlatformKey are in
-  // |context|. |account_id| identifies the user for which the certificate was
-  // requested. |is_expiring_soon| will be set iff a certificate in the
-  // |certificate_chain| is expiring soon. |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
-  // |context.callback|.
+  // `certificate_chain` is the platform certificate chain for the key which
+  // signed the `challenge`.  The arguments to ChallengePlatformKey are in
+  // `context`. `account_id` identifies the user for which the certificate was
+  // requested. `is_expiring_soon` will be set iff a certificate in the
+  // `certificate_chain` is expiring soon. `reply` is returned from
+  // `AttestationClient`. Upon success, the method will invoke
+  // `context.callback`.
   void OnChallengeReady(ChallengeContext context,
                         const AccountId& account_id,
                         const std::string& certificate_chain,
                         bool is_expiring_soon,
-                        bool operation_success,
-                        const std::string& response_data);
+                        const ::attestation::SignSimpleChallengeReply& reply);
 
   // Checks whether attestation for content protection is allowed by policy.
   bool IsAttestationAllowedByPolicy();

chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc

@@ -18,7 +18,6 @@
 #include "chrome/browser/profiles/profile_impl.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/attestation/mock_attestation_flow.h"
-#include "chromeos/cryptohome/mock_async_method_caller.h"
 #include "chromeos/dbus/attestation/attestation.pb.h"
 #include "chromeos/dbus/attestation/fake_attestation_client.h"
 #include "chromeos/dbus/attestation/interface.pb.h"
@@ -41,8 +40,6 @@ namespace {
 
 const char kTestID[] = "test_id";
 const char kTestChallenge[] = "test_challenge";
-const char kTestSignedData[] = "test_challenge_with_salt";
-const char kTestSignature[] = "test_signature";
 const char kTestCertificate[] = "test_certificate";
 const char kTestEmail[] = "test_email@chromium.org";
 const char kTestURL[] = "http://mytestdomain/test";
@@ -102,7 +99,6 @@ class PlatformVerificationFlowTest : public ::testing::Test {
   PlatformVerificationFlowTest()
       : certificate_status_(ATTESTATION_SUCCESS),
         fake_certificate_index_(0),
-        sign_challenge_success_(true),
         result_(PlatformVerificationFlow::INTERNAL_ERROR) {
     ::chromeos::AttestationClient::InitializeFake();
   }
@@ -112,10 +108,11 @@ class PlatformVerificationFlowTest : public ::testing::Test {
 
   void SetUp() {
     // Create a verifier for tests to call.
-    // We don't need cryptohome_client in unittests because it is only needed
-    // for real AttestationFlow and in unittests we uses mocked one.
+    // We don't need cryptohome_client and its async caller in unittests because
+    // it is only needed for real AttestationFlow and in unittests we use a
+    // mocked one.
     verifier_ = new PlatformVerificationFlow(
-        &mock_attestation_flow_, &mock_async_caller_,
+        &mock_attestation_flow_, /*async_caller=*/nullptr,
         /*cryptohome_client=*/nullptr, AttestationClient::Get(),
         &fake_delegate_);
 
@@ -142,15 +139,11 @@ class PlatformVerificationFlowTest : public ::testing::Test {
         .WillRepeatedly(WithArgs<5>(
             Invoke(this, &PlatformVerificationFlowTest::FakeGetCertificate)));
 
-    // Configure the mock AsyncMethodCaller to call FakeSignChallenge.
-    std::string expected_key_name = std::string(kContentProtectionKeyPrefix) +
-                                    std::string(kTestID);
-    EXPECT_CALL(mock_async_caller_,
-                TpmAttestationSignSimpleChallenge(
-                    KEY_USER, cryptohome::Identification(account_id),
-                    expected_key_name, kTestChallenge, _))
-        .WillRepeatedly(WithArgs<4>(
-            Invoke(this, &PlatformVerificationFlowTest::FakeSignChallenge)));
+    const std::string expected_key_name =
+        std::string(kContentProtectionKeyPrefix) + std::string(kTestID);
+    AttestationClient::Get()
+        ->GetTestInterface()
+        ->AllowlistSignSimpleChallengeKey(kTestEmail, expected_key_name);
   }
 
   void FakeGetCertificate(AttestationFlow::CertificateCallback callback) {
@@ -163,12 +156,6 @@ class PlatformVerificationFlowTest : public ::testing::Test {
     ++fake_certificate_index_;
   }
 
-  void FakeSignChallenge(cryptohome::AsyncMethodCaller::DataCallback callback) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), sign_challenge_success_,
-                                  CreateFakeResponseProto()));
-  }
-
   void FakeChallengeCallback(PlatformVerificationFlow::Result result,
                              const std::string& salt,
                              const std::string& signature,
@@ -179,19 +166,9 @@ class PlatformVerificationFlowTest : public ::testing::Test {
     certificate_ = certificate;
   }
 
-  std::string CreateFakeResponseProto() {
-    SignedData pb;
-    pb.set_data(kTestSignedData);
-    pb.set_signature(kTestSignature);
-    std::string serial;
-    CHECK(pb.SerializeToString(&serial));
-    return serial;
-  }
-
  protected:
   content::BrowserTaskEnvironment task_environment_;
   StrictMock<MockAttestationFlow> mock_attestation_flow_;
-  cryptohome::MockAsyncMethodCaller mock_async_caller_;
   FakeDelegate fake_delegate_;
   ScopedCrosSettingsTestHelper settings_helper_;
   scoped_refptr<PlatformVerificationFlow> verifier_;
@@ -201,9 +178,6 @@ class PlatformVerificationFlowTest : public ::testing::Test {
   std::vector<std::string> fake_certificate_list_;
   size_t fake_certificate_index_;
 
-  // Controls result of FakeSignChallenge.
-  bool sign_challenge_success_;
-
   // Callback functions and data.
   PlatformVerificationFlow::Result result_;
   std::string challenge_salt_;
@@ -217,9 +191,14 @@ TEST_F(PlatformVerificationFlowTest, Success) {
                                   CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
-  EXPECT_EQ(kTestSignedData, challenge_salt_);
-  EXPECT_EQ(kTestSignature, challenge_signature_);
   EXPECT_EQ(kTestCertificate, certificate_);
+  ::attestation::SignedData challenge_respoonse;
+  challenge_respoonse.set_data(challenge_salt_);
+  challenge_respoonse.set_signature(challenge_signature_);
+  EXPECT_TRUE(
+      AttestationClient::Get()
+          ->GetTestInterface()
+          ->VerifySimpleChallengeResponse(kTestChallenge, challenge_respoonse));
 }
 
 TEST_F(PlatformVerificationFlowTest, NotPermittedByUser) {
@@ -257,7 +236,11 @@ TEST_F(PlatformVerificationFlowTest, NotVerifiedDueToBadRequestFailure) {
 }
 
 TEST_F(PlatformVerificationFlowTest, ChallengeSigningError) {
-  sign_challenge_success_ = false;
+  // Force the signing operation to fail.
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->set_sign_simple_challenge_status(
+          ::attestation::STATUS_UNEXPECTED_DEVICE_ERROR);
   ExpectAttestationFlow();
   verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
                                   CreateChallengeCallback());

chromeos/dbus/attestation/attestation_client.h

@@ -134,6 +134,18 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) AttestationClient {
     virtual ::attestation::GetKeyInfoReply* GetMutableKeyInfoReply(
         const std::string& username,
         const std::string& label) = 0;
+
+    // Verifies if `signed_data` is signed against `challenge`.
+    virtual bool VerifySimpleChallengeResponse(
+        const std::string& challenge,
+        const ::attestation::SignedData& signed_data) = 0;
+
+    // Sets the status code returned by `SignSimpleChallenge()`.
+    virtual void set_sign_simple_challenge_status(
+        ::attestation::AttestationStatus status) = 0;
+    // Allowlists the key used to sign simple challenge.
+    virtual void AllowlistSignSimpleChallengeKey(const std::string& username,
+                                                 const std::string& label) = 0;
   };
 
   // Not copyable or movable.

chromeos/dbus/attestation/fake_attestation_client.cc

@@ -17,6 +17,8 @@ namespace {
 
 constexpr int kCertificateNotAssigned = 0;
 constexpr char kFakeCertPrefix[] = "fake cert";
+constexpr char kResponseSuffix[] = "_response";
+constexpr char kSignatureSuffix[] = "_signature";
 
 // Posts |callback| on the current thread's task runner, passing it the
 // |response| message.
@@ -234,7 +236,20 @@ void FakeAttestationClient::SignEnterpriseChallenge(
 void FakeAttestationClient::SignSimpleChallenge(
     const ::attestation::SignSimpleChallengeRequest& request,
     SignSimpleChallengeCallback callback) {
-  NOTIMPLEMENTED();
+  ::attestation::SignSimpleChallengeReply reply;
+  if (allowlisted_sign_simple_challenge_keys_.count(
+          {request.username(), request.key_label()}) == 0) {
+    reply.set_status(::attestation::STATUS_INVALID_PARAMETER);
+  } else {
+    reply.set_status(sign_simple_challenge_status_);
+  }
+  if (reply.status() == ::attestation::STATUS_SUCCESS) {
+    ::attestation::SignedData signed_data;
+    signed_data.set_data(request.challenge() + kResponseSuffix);
+    signed_data.set_signature(request.challenge() + kSignatureSuffix);
+    reply.set_challenge_response(signed_data.SerializeAsString());
+  }
+  PostProtoResponse(std::move(callback), reply);
 }
 
 void FakeAttestationClient::SetKeyPayload(
@@ -366,6 +381,24 @@ void FakeAttestationClient::set_enrollment_id_dbus_error_count(int count) {
   return &(key_info_database_[request]);
 }
 
+bool FakeAttestationClient::VerifySimpleChallengeResponse(
+    const std::string& challenge,
+    const ::attestation::SignedData& signed_data) {
+  return signed_data.data() == challenge + kResponseSuffix &&
+         signed_data.signature() == challenge + kSignatureSuffix;
+}
+
+void FakeAttestationClient::set_sign_simple_challenge_status(
+    ::attestation::AttestationStatus status) {
+  sign_simple_challenge_status_ = status;
+}
+
+void FakeAttestationClient::AllowlistSignSimpleChallengeKey(
+    const std::string& username,
+    const std::string& label) {
+  allowlisted_sign_simple_challenge_keys_.insert({username, label});
+}
+
 AttestationClient::TestInterface* FakeAttestationClient::GetTestInterface() {
   return this;
 }

chromeos/dbus/attestation/fake_attestation_client.h

@@ -9,7 +9,9 @@
 
 #include <deque>
 #include <map>
+#include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/component_export.h"
@@ -118,6 +120,13 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) FakeAttestationClient
   ::attestation::GetKeyInfoReply* GetMutableKeyInfoReply(
       const std::string& username,
       const std::string& label) override;
+  bool VerifySimpleChallengeResponse(
+      const std::string& challenge,
+      const ::attestation::SignedData& signed_data) override;
+  void set_sign_simple_challenge_status(
+      ::attestation::AttestationStatus status) override;
+  void AllowlistSignSimpleChallengeKey(const std::string& username,
+                                       const std::string& label) override;
 
   AttestationClient::TestInterface* GetTestInterface() override;
 
@@ -166,6 +175,14 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) FakeAttestationClient
            ::attestation::GetKeyInfoReply,
            GetKeyInfoRequestComparator>
       key_info_database_;
+
+  // The status returned by `SignSimpleChallenge()`.
+  ::attestation::AttestationStatus sign_simple_challenge_status_ =
+      ::attestation::STATUS_SUCCESS;
+  // The table of username-label pairs of which keys can perform simple sign
+  // challenge.
+  std::set<std::pair<std::string, std::string>>
+      allowlisted_sign_simple_challenge_keys_;
 };
 
 }  // namespace chromeos