Revert of Do a better job at faking simple challenge signatures. (patchset #7...

Revert of Do a better job at faking simple challenge signatures. (patchset #7 id:120001 of https://codereview.chromium.org/2297193006/ )

Reason for revert:
crbug.com/645052

This is causing build_package failures on chromeos.

Original issue's description:
> Do a better job at faking simple challenge signatures.
>
> By returning a signed simple challenge that can actually be parsed as SignedData, we allow callers of the FakeCryptohomeClient to extract the original data back and process it, making for better fake behavior and simpler tests.
>
> Note that the signature is purposedly not verifiable in the FakeCryptohomeClient.
>
> BUG=643245
> TEST=chromeos_unittests; unit_tests and components_unittests also pass
>
> Committed: https://crrev.com/911199cb30f608636c79d47e50dca84735e3a08f
> Cr-Commit-Position: refs/heads/master@{#417118}

TBR=dkrahn@chromium.org,xiyuan@chromium.org,drcrash@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=643245

Review-Url: https://codereview.chromium.org/2326473002
Cr-Commit-Position: refs/heads/master@{#417242}

chrome/browser/chromeos/BUILD.gn

@@ -49,7 +49,6 @@ source_set("chromeos") {
     "//chrome/common/safe_browsing:proto",
     "//chrome/installer/util:with_no_strings",
     "//chromeos",
-    "//chromeos:attestation_proto",
     "//chromeos:cryptohome_proto",
     "//chromeos:cryptohome_signkey_proto",
     "//chromeos:power_manager_proto",
@@ -1597,5 +1596,6 @@ proto_library("device_policy_proto") {
 proto_library("attestation_proto") {
   sources = [
     "attestation/attestation_key_payload.proto",
+    "attestation/attestation_signed_data.proto",
   ]
 }

chrome/browser/chromeos/attestation/platform_verification_flow.cc

@@ -14,11 +14,11 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/attestation/attestation_ca_client.h"
+#include "chrome/browser/chromeos/attestation/attestation_signed_data.pb.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/permissions/permission_manager.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chromeos/attestation/attestation.pb.h"
 #include "chromeos/attestation/attestation_flow.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/cryptohome/async_method_caller.h"
@@ -335,7 +335,7 @@ void PlatformVerificationFlow::OnChallengeReady(
     ReportError(context.callback, INTERNAL_ERROR);
     return;
   }
-  chromeos::attestation::SignedData signed_data_pb;
+  SignedData signed_data_pb;
   if (response_data.empty() || !signed_data_pb.ParseFromString(response_data)) {
     LOG(ERROR) << "PlatformVerificationFlow: Failed to parse response data.";
     ReportError(context.callback, INTERNAL_ERROR);

chromeos/BUILD.gn

@@ -24,7 +24,6 @@ component("chromeos") {
     "//dbus",
   ]
   deps = [
-    ":attestation_proto",
     ":cryptohome_proto",
     ":power_manager_proto",
     "//base",
@@ -62,7 +61,6 @@ static_library("test_support") {
   testonly = true
   configs += [ "//build/config/linux/dbus" ]
   public_deps = [
-    ":attestation_proto",
     ":chromeos",
     ":cryptohome_proto",
     ":power_manager_proto",
@@ -126,7 +124,6 @@ static_library("test_support_without_gmock") {
   testonly = true
   configs += [ "//build/config/linux/dbus" ]
   deps = [
-    ":attestation_proto",
     ":chromeos",
     ":cryptohome_proto",
     ":power_manager_proto",
@@ -150,7 +147,6 @@ test("chromeos_unittests") {
     "//third_party/nss:system_nss_no_ssl_config",
   ]
   deps = [
-    ":attestation_proto",
     ":cryptohome_proto",
     ":power_manager_proto",
     ":test_support",
@@ -205,14 +201,6 @@ proto_library("cryptohome_proto") {
   proto_out_dir = "chromeos/dbus/cryptohome"
 }
 
-proto_library("attestation_proto") {
-  sources = [
-    "dbus/proto/attestation.proto",
-  ]
-
-  proto_out_dir = "chromeos/attestation"
-}
-
 proto_library("cryptohome_signkey_proto") {
   sources = [
     "//third_party/cros_system_api/dbus/cryptohome/signed_secret.proto",

chromeos/chromeos.gyp

@@ -410,7 +410,6 @@
       'dbus/cras_audio_client_unittest.cc',
       'dbus/cros_disks_client_unittest.cc',
       'dbus/dbus_client_bundle_unittest.cc',
-      'dbus/fake_cryptohome_client_unittest.cc',
       'dbus/fake_easy_unlock_client_unittest.cc',
       'dbus/fake_power_manager_client_unittest.cc',
       'dbus/gsm_sms_client_unittest.cc',
@@ -496,7 +495,6 @@
         '../third_party/libxml/libxml.gyp:libxml',
         '../third_party/protobuf/protobuf.gyp:protobuf_lite',
         '../url/url.gyp:url_lib',
-        'attestation_proto',
         'cryptohome_proto',
         'power_manager_proto',
       ],
@@ -681,20 +679,6 @@
       },
       'includes': ['../build/protoc.gypi'],
     },
-    {
-      # GN version: //chromeos:cryptohome_attestation
-      # Protobuf compiler/generator for attestation related protocol buffers.
-      'target_name': 'attestation_proto',
-      'type': 'static_library',
-      'sources': [
-        'dbus/proto/attestation.proto',
-      ],
-      'variables': {
-        'proto_in_dir': 'dbus/proto',
-        'proto_out_dir': 'chromeos/cryptohome',
-      },
-      'includes': ['../build/protoc.gypi'],
-    },
     {
       # GN version: //chromeos:cryptohome_signkey_proto
       # Protobuf compiler/generator for cryptohome key signing protocol buffer.

chromeos/dbus/fake_cryptohome_client.cc

@@ -15,7 +15,6 @@
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/chromeos_paths.h"
-#include "chromeos/attestation/attestation.pb.h"
 #include "chromeos/dbus/cryptohome/key.pb.h"
 #include "chromeos/dbus/cryptohome/rpc.pb.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
@@ -25,13 +24,6 @@
 
 namespace chromeos {
 
-namespace {
-// Signature nonces are twenty bytes. This matches the attestation code.
-constexpr char kTwentyBytesNonce[] = "+addtwentybytesnonce";
-// A symbolic signature.
-constexpr char kSignature[] = "signed";
-}  // namespace
-
 FakeCryptohomeClient::FakeCryptohomeClient()
     : service_is_available_(true),
       async_call_id_(1),
@@ -88,7 +80,7 @@ void FakeCryptohomeClient::AsyncCheckKey(
     const cryptohome::Identification& cryptohome_id,
     const std::string& key,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncMigrateKey(
@@ -96,13 +88,13 @@ void FakeCryptohomeClient::AsyncMigrateKey(
     const std::string& from_key,
     const std::string& to_key,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncRemove(
     const cryptohome::Identification& cryptohome_id,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::RenameCryptohome(
@@ -151,7 +143,7 @@ void FakeCryptohomeClient::AsyncMount(
     const std::string& key,
     int flags,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncAddKey(
@@ -159,19 +151,19 @@ void FakeCryptohomeClient::AsyncAddKey(
     const std::string& key,
     const std::string& new_key,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncMountGuest(
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncMountPublic(
     const cryptohome::Identification& public_mount_id,
     int flags,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::TpmIsReady(
@@ -374,14 +366,14 @@ void FakeCryptohomeClient::TpmAttestationIsEnrolled(
 void FakeCryptohomeClient::AsyncTpmAttestationCreateEnrollRequest(
     chromeos::attestation::PrivacyCAType pca_type,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodData(callback, std::string());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::AsyncTpmAttestationEnroll(
     chromeos::attestation::PrivacyCAType pca_type,
     const std::string& pca_response,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodResult(callback);
+  ReturnAsyncMethodResult(callback, false);
 }
 
 void FakeCryptohomeClient::AsyncTpmAttestationCreateCertRequest(
@@ -390,7 +382,7 @@ void FakeCryptohomeClient::AsyncTpmAttestationCreateCertRequest(
     const cryptohome::Identification& cryptohome_id,
     const std::string& request_origin,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodData(callback, std::string());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::AsyncTpmAttestationFinishCertRequest(
@@ -399,7 +391,7 @@ void FakeCryptohomeClient::AsyncTpmAttestationFinishCertRequest(
     const cryptohome::Identification& cryptohome_id,
     const std::string& key_name,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodData(callback, std::string());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::TpmAttestationDoesKeyExist(
@@ -436,7 +428,7 @@ void FakeCryptohomeClient::TpmAttestationRegisterKey(
     const cryptohome::Identification& cryptohome_id,
     const std::string& key_name,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodData(callback, std::string());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::TpmAttestationSignEnterpriseChallenge(
@@ -448,7 +440,7 @@ void FakeCryptohomeClient::TpmAttestationSignEnterpriseChallenge(
     attestation::AttestationChallengeOptions options,
     const std::string& challenge,
     const AsyncMethodCallback& callback) {
-  ReturnAsyncMethodData(callback, std::string());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::TpmAttestationSignSimpleChallenge(
@@ -457,10 +449,7 @@ void FakeCryptohomeClient::TpmAttestationSignSimpleChallenge(
     const std::string& key_name,
     const std::string& challenge,
     const AsyncMethodCallback& callback) {
-  chromeos::attestation::SignedData signed_data;
-  signed_data.set_data(challenge + kTwentyBytesNonce);
-  signed_data.set_signature(kSignature);
-  ReturnAsyncMethodData(callback, signed_data.SerializeAsString());
+  ReturnAsyncMethodResult(callback, true);
 }
 
 void FakeCryptohomeClient::TpmAttestationGetKeyPayload(
@@ -599,41 +588,26 @@ void FakeCryptohomeClient::ReturnProtobufMethodCallback(
 }
 
 void FakeCryptohomeClient::ReturnAsyncMethodResult(
-    const AsyncMethodCallback& callback) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(&FakeCryptohomeClient::ReturnAsyncMethodResultInternal,
-                 weak_ptr_factory_.GetWeakPtr(), callback));
-}
-
-void FakeCryptohomeClient::ReturnAsyncMethodData(
     const AsyncMethodCallback& callback,
-    const std::string& data) {
+    bool returns_data) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(&FakeCryptohomeClient::ReturnAsyncMethodDataInternal,
-                 weak_ptr_factory_.GetWeakPtr(), callback, data));
+      base::Bind(&FakeCryptohomeClient::ReturnAsyncMethodResultInternal,
+                 weak_ptr_factory_.GetWeakPtr(), callback, returns_data));
 }
 
 void FakeCryptohomeClient::ReturnAsyncMethodResultInternal(
-    const AsyncMethodCallback& callback) {
+    const AsyncMethodCallback& callback,
+    bool returns_data) {
   callback.Run(async_call_id_);
-  if (!async_call_status_handler_.is_null()) {
+  if (!returns_data && !async_call_status_handler_.is_null()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(async_call_status_handler_, async_call_id_, true,
                               cryptohome::MOUNT_ERROR_NONE));
-  }
-  ++async_call_id_;
-}
-
-void FakeCryptohomeClient::ReturnAsyncMethodDataInternal(
-    const AsyncMethodCallback& callback,
-    const std::string& data) {
-  callback.Run(async_call_id_);
-  if (!async_call_status_data_handler_.is_null()) {
+  } else if (returns_data && !async_call_status_data_handler_.is_null()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(async_call_status_data_handler_, async_call_id_,
-                              true, data));
+                              true, std::string()));
   }
   ++async_call_id_;
 }

chromeos/dbus/fake_cryptohome_client.h

@@ -220,18 +220,12 @@ class CHROMEOS_EXPORT FakeCryptohomeClient : public CryptohomeClient {
       const ProtobufMethodCallback& callback);
 
   // Posts tasks which return fake results to the UI thread.
-  void ReturnAsyncMethodResult(const AsyncMethodCallback& callback);
+  void ReturnAsyncMethodResult(const AsyncMethodCallback& callback,
+                               bool returns_data);
 
-  // Posts tasks which return fake data to the UI thread.
-  void ReturnAsyncMethodData(const AsyncMethodCallback& callback,
-                             const std::string& data);
-
-  // This method is used to implement ReturnAsyncMethodResult without data.
-  void ReturnAsyncMethodResultInternal(const AsyncMethodCallback& callback);
-
-  // This method is used to implement ReturnAsyncMethodResult with data.
-  void ReturnAsyncMethodDataInternal(const AsyncMethodCallback& callback,
-                                     const std::string& data);
+  // This method is used to implement ReturnAsyncMethodResult.
+  void ReturnAsyncMethodResultInternal(const AsyncMethodCallback& callback,
+                                       bool returns_data);
 
   bool service_is_available_;
   int async_call_id_;

chromeos/dbus/fake_cryptohome_client_unittest.cc

-// Copyright 2016 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.
-
-#include "chromeos/dbus/fake_cryptohome_client.h"
-
-#include <string>
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/run_loop.h"
-#include "chromeos/attestation/attestation.pb.h"
-#include "chromeos/cryptohome/cryptohome_parameters.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-using ::testing::_;
-using ::testing::SaveArg;
-
-namespace chromeos {
-
-class FakeCryptohomeClientTest : public ::testing::Test {
- public:
-  FakeCryptohomeClientTest() : weak_ptr_factory_(this) {
-    async_method_callback_ =
-        base::Bind(&FakeCryptohomeClientTest::MockHandleAsyncMethodCallback,
-                   weak_ptr_factory_.GetWeakPtr());
-    fake_cryptohome_client_.SetAsyncCallStatusHandlers(
-        base::Bind(
-            &FakeCryptohomeClientTest::MockHandleAsyncMethodResultResponse,
-            weak_ptr_factory_.GetWeakPtr()),
-        base::Bind(&FakeCryptohomeClientTest::MockHandleAsyncMethodDataResponse,
-                   weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  MOCK_METHOD1(MockHandleAsyncMethodCallback, void(int));
-  MOCK_METHOD3(MockHandleAsyncMethodResultResponse, void(int, bool, int));
-  MOCK_METHOD3(MockHandleAsyncMethodDataResponse,
-               void(int, bool, const std::string&));
-
- protected:
-  base::MessageLoop message_loop_;
-
-  FakeCryptohomeClient fake_cryptohome_client_;
-  CryptohomeClient::AsyncMethodCallback async_method_callback_;
-
-  base::WeakPtrFactory<FakeCryptohomeClientTest> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeCryptohomeClientTest);
-};
-
-TEST_F(FakeCryptohomeClientTest, SignSimpleChallenge) {
-  const std::string challenge{"challenge"};
-
-  EXPECT_CALL(*this, MockHandleAsyncMethodCallback(_));
-
-  std::string return_data;
-  EXPECT_CALL(*this, MockHandleAsyncMethodDataResponse(_, true, _))
-      .WillOnce(SaveArg<2>(&return_data));
-
-  cryptohome::Identification cryptohome_id;
-  fake_cryptohome_client_.TpmAttestationSignSimpleChallenge(
-      attestation::AttestationKeyType::KEY_DEVICE, cryptohome_id, "key_name",
-      challenge, async_method_callback_);
-
-  base::RunLoop().RunUntilIdle();
-
-  chromeos::attestation::SignedData signed_data;
-  ASSERT_TRUE(signed_data.ParseFromString(return_data));
-  ASSERT_EQ(static_cast<size_t>(20),
-            signed_data.data().size() - challenge.size());
-  ASSERT_EQ(challenge,
-            signed_data.data().substr(0, signed_data.data().size() - 20));
-}
-
-}  // namespace cryptohome