Reland "Add encryption support to pseudo-USS"

This is a reland of 66a392fa

Reland fixes unit tests that accidentally used DCHECKs with side
effects, making tests fail in non-debug builds.

Original change's description:
> Add encryption support to pseudo-USS
>
> This patch extends SyncableServiceBasedBridge (core to pseudo-USS)
> with support for encryption, via a newly introduced interface
> (ModelCryptographer), which needs to be usable in the model thread
> (where the bridge lives).
>
> This should unblock the migration of PASSWORDS to pseudo-USS, because
> password data must be encrypted at all times. This is a first step of
> a proposal described in a dedicated section in the Design Doc:
> https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot
>
> Actual usage of this functionality will be introduced in follow-up
> patches.
>
> Bug: 870624
> Change-Id: I227d429dc952bfe1a3a4fb05cd1ab71cac9ba1c6
> Reviewed-on: https://chromium-review.googlesource.com/c/1288594
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601254}

TBR=treib@chromium.org

Bug: 870624
Change-Id: Ie576b1aebf6bb989bba95d808031ab8433ec8b6a
Reviewed-on: https://chromium-review.googlesource.com/c/1292556
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601479}

components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc

@@ -24,8 +24,11 @@ class LazyBridgeBuilder {
       NonUiSyncableServiceBasedModelTypeController::SyncableServiceProvider
           syncable_service_provider,
       const base::RepeatingClosure& dump_stack,
-      scoped_refptr<base::SequencedTaskRunner> task_runner)
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
+          cryptographer)
       : type_(type),
+        cryptographer_(std::move(cryptographer)),
         store_factory_(std::move(store_factory)),
         syncable_service_provider_(std::move(syncable_service_provider)),
         dump_stack_(dump_stack),
@@ -44,13 +47,15 @@ class LazyBridgeBuilder {
           type_, std::move(store_factory_),
           std::make_unique<ClientTagBasedModelTypeProcessor>(type_,
                                                              dump_stack_),
-          syncable_service.get()));
+          syncable_service.get(), cryptographer_));
     }
     return bridge_->change_processor()->GetControllerDelegate();
   }
 
  private:
   const ModelType type_;
+  const scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
+      cryptographer_;
   OnceModelTypeStoreFactory store_factory_;
   NonUiSyncableServiceBasedModelTypeController::SyncableServiceProvider
       syncable_service_provider_;
@@ -68,7 +73,9 @@ NonUiSyncableServiceBasedModelTypeController::
         OnceModelTypeStoreFactory store_factory,
         SyncableServiceProvider syncable_service_provider,
         const base::RepeatingClosure& dump_stack,
-        scoped_refptr<base::SequencedTaskRunner> task_runner)
+        scoped_refptr<base::SequencedTaskRunner> task_runner,
+        scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
+            cryptographer)
     : ModelTypeController(
           type,
           std::make_unique<ProxyModelTypeControllerDelegate>(
@@ -79,7 +86,8 @@ NonUiSyncableServiceBasedModelTypeController::
                                       std::move(store_factory),
                                       std::move(syncable_service_provider),
                                       dump_stack,
-                                      task_runner)))) {}
+                                      task_runner,
+                                      std::move(cryptographer))))) {}
 
 NonUiSyncableServiceBasedModelTypeController::
     ~NonUiSyncableServiceBasedModelTypeController() {}

components/sync/driver/non_ui_syncable_service_based_model_type_controller.h

@@ -15,6 +15,7 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/model_type_controller.h"
 #include "components/sync/model/model_type_store.h"
+#include "components/sync/model_impl/syncable_service_based_bridge.h"
 
 namespace syncer {
 
@@ -39,7 +40,9 @@ class NonUiSyncableServiceBasedModelTypeController
       OnceModelTypeStoreFactory store_factory,
       SyncableServiceProvider syncable_service_provider,
       const base::RepeatingClosure& dump_stack,
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      scoped_refptr<SyncableServiceBasedBridge::ModelCryptographer>
+          cryptographer = nullptr);
   ~NonUiSyncableServiceBasedModelTypeController() override;
 
  private:

components/sync/model/sync_data.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <ostream>
+#include <utility>
 
 #include "base/json/json_writer.h"
 #include "base/strings/string_number_conversions.h"
@@ -75,14 +76,13 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
 }
 
 // Static.
-SyncData SyncData::CreateRemoteData(
-    int64_t id,
-    const sync_pb::EntitySpecifics& specifics,
-    const base::Time& modification_time,
-    const std::string& client_tag_hash) {
+SyncData SyncData::CreateRemoteData(int64_t id,
+                                    sync_pb::EntitySpecifics specifics,
+                                    base::Time modification_time,
+                                    std::string client_tag_hash) {
   sync_pb::SyncEntity entity;
-  entity.mutable_specifics()->CopyFrom(specifics);
-  entity.set_client_defined_unique_tag(client_tag_hash);
+  *entity.mutable_specifics() = std::move(specifics);
+  entity.set_client_defined_unique_tag(std::move(client_tag_hash));
   return SyncData(/*is_local=*/false, id, &entity, modification_time);
 }
 

components/sync/model/sync_data.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <iosfwd>
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -58,11 +59,10 @@ class SyncData {
                                   const sync_pb::EntitySpecifics& specifics);
 
   // Helper method for creating SyncData objects originating from the syncer.
-  static SyncData CreateRemoteData(
-      int64_t id,
-      const sync_pb::EntitySpecifics& specifics,
-      const base::Time& last_modified_time,
-      const std::string& client_tag_hash = std::string());
+  static SyncData CreateRemoteData(int64_t id,
+                                   sync_pb::EntitySpecifics specifics,
+                                   base::Time last_modified_time,
+                                   std::string client_tag_hash = std::string());
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
   // without valid data is to use the default constructor.

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -372,7 +372,13 @@ void ClientTagBasedModelTypeProcessor::Put(
   DCHECK(data);
   DCHECK(!data->is_deleted());
   DCHECK(!data->non_unique_name.empty());
-  DCHECK_EQ(type_, GetModelTypeFromSpecifics(data->specifics));
+
+  // Only the pseudo-USS bridge for PASSWORDS populates encrypted data.
+  // TODO(crbug.com/856941): Remove when PASSWORDS are migrated to USS and
+  // replace instead with a DCHECK that verifies the input is not encrypted.
+  if (!data->specifics.has_encrypted()) {
+    DCHECK_EQ(type_, GetModelTypeFromSpecifics(data->specifics));
+  }
 
   if (!model_type_state_.initial_sync_done()) {
     // Ignore changes before the initial sync is done.

components/sync/model_impl/syncable_service_based_bridge.h

@@ -11,6 +11,7 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "components/sync/model/model_error.h"
@@ -18,6 +19,10 @@
 #include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/model/sync_change_processor.h"
 
+namespace sync_pb {
+class EntitySpecifics;
+}  // namespace sync_pb
+
 namespace syncer {
 
 class MetadataBatch;
@@ -31,12 +36,35 @@ class SyncableService;
 // considered an implementation detail.
 class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
  public:
+  // Used for passwords only.
+  // TODO(crbug.com/856941): Remove when PASSWORDS are migrated to USS, which
+  // will likely make this API unnecessary.
+  class ModelCryptographer
+      : public base::RefCountedThreadSafe<ModelCryptographer> {
+   public:
+    ModelCryptographer();
+
+    virtual base::Optional<ModelError> Decrypt(
+        sync_pb::EntitySpecifics* specifics) = 0;
+    virtual base::Optional<ModelError> Encrypt(
+        sync_pb::EntitySpecifics* specifics) = 0;
+
+   protected:
+    virtual ~ModelCryptographer();
+
+   private:
+    friend class base::RefCountedThreadSafe<ModelCryptographer>;
+
+    DISALLOW_COPY_AND_ASSIGN(ModelCryptographer);
+  };
+
   // Pointers must not be null and |syncable_service| must outlive this object.
   SyncableServiceBasedBridge(
       ModelType type,
       OnceModelTypeStoreFactory store_factory,
       std::unique_ptr<ModelTypeChangeProcessor> change_processor,
-      SyncableService* syncable_service);
+      SyncableService* syncable_service,
+      scoped_refptr<ModelCryptographer> cryptographer = nullptr);
   ~SyncableServiceBasedBridge() override;
 
   // ModelTypeSyncBridge implementation.
@@ -70,9 +98,10 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
   void OnReadAllMetadataForInit(const base::Optional<ModelError>& error,
                                 std::unique_ptr<MetadataBatch> metadata_batch);
   void MaybeStartSyncableService();
-  SyncChangeList StoreAndConvertRemoteChanges(
+  base::Optional<ModelError> StoreAndConvertRemoteChanges(
       std::unique_ptr<MetadataChangeList> metadata_change_list,
-      EntityChangeList entity_change_list);
+      EntityChangeList input_entity_change_list,
+      SyncChangeList* output_sync_change_list);
   void OnReadDataForProcessor(
       DataCallback callback,
       const base::Optional<ModelError>& error,
@@ -86,6 +115,7 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
 
   const ModelType type_;
   SyncableService* const syncable_service_;
+  const scoped_refptr<ModelCryptographer> cryptographer_;
   OnceModelTypeStoreFactory store_factory_;
 
   std::unique_ptr<ModelTypeStore> store_;

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -38,6 +38,8 @@ using testing::Return;
 using testing::SaveArg;
 using testing::_;
 
+using ModelCryptographer = SyncableServiceBasedBridge::ModelCryptographer;
+
 const ModelType kModelType = PREFERENCES;
 
 sync_pb::EntitySpecifics GetTestSpecifics(const std::string& name = "name") {
@@ -62,6 +64,10 @@ MATCHER_P(HasName, name, "") {
   return arg && arg->specifics.preference().name() == name;
 }
 
+MATCHER(IsEncrypted, "") {
+  return arg && arg->specifics.encrypted().has_blob();
+}
+
 class MockSyncableService : public SyncableService {
  public:
   MOCK_METHOD4(
@@ -77,6 +83,44 @@ class MockSyncableService : public SyncableService {
   MOCK_CONST_METHOD1(GetAllSyncData, SyncDataList(ModelType type));
 };
 
+// Simple ModelCryptographer implementation that serializes the input proto to a
+// string and puts it in proto field EntitySpecifics.encrypted.blob.
+class TestModelCryptographer : public ModelCryptographer {
+ public:
+  TestModelCryptographer() {}
+
+  base::Optional<ModelError> Decrypt(
+      sync_pb::EntitySpecifics* specifics) override {
+    if (!specifics->has_encrypted()) {
+      return ModelError(FROM_HERE, "Not encrypted");
+    }
+    sync_pb::EntitySpecifics unencrypted_specifics;
+    if (!unencrypted_specifics.ParseFromString(specifics->encrypted().blob())) {
+      return ModelError(FROM_HERE, "Cannot parse blob");
+    }
+    *specifics = unencrypted_specifics;
+    return base::nullopt;
+  }
+
+  base::Optional<ModelError> Encrypt(
+      sync_pb::EntitySpecifics* specifics) override {
+    if (specifics->has_encrypted()) {
+      return ModelError(FROM_HERE, "Already encrypted");
+    }
+    sync_pb::EntitySpecifics encrypted_specifics;
+    encrypted_specifics.mutable_encrypted()->set_blob(
+        specifics->SerializeAsString());
+    *specifics = encrypted_specifics;
+    return base::nullopt;
+  }
+
+ protected:
+  ~TestModelCryptographer() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestModelCryptographer);
+};
+
 class SyncableServiceBasedBridgeTest : public ::testing::Test {
  protected:
   SyncableServiceBasedBridgeTest()
@@ -93,7 +137,8 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
 
   ~SyncableServiceBasedBridgeTest() override {}
 
-  void InitializeBridge() {
+  void InitializeBridge(
+      scoped_refptr<ModelCryptographer> cryptographer = nullptr) {
     real_processor_ =
         std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
             kModelType, /*dump_stack=*/base::DoNothing(),
@@ -102,7 +147,8 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
     bridge_ = std::make_unique<SyncableServiceBasedBridge>(
         kModelType,
         ModelTypeStoreTestUtil::FactoryForForwardingStore(store_.get()),
-        mock_processor_.CreateForwardingProcessor(), &syncable_service_);
+        mock_processor_.CreateForwardingProcessor(), &syncable_service_,
+        cryptographer);
   }
 
   void ShutdownBridge() {
@@ -450,5 +496,111 @@ TEST_F(SyncableServiceBasedBridgeTest, ShouldPropagateRemoteDeletion) {
   EXPECT_THAT(GetAllData(), IsEmpty());
 }
 
+TEST_F(SyncableServiceBasedBridgeTest, ShouldDecryptInitialRemoteData) {
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  InitializeBridge(cryptographer);
+  StartSyncing();
+
+  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
+  // Once the initial encrypted data is fetched from the server,
+  // MergeDataAndStartSyncing() should be exercised, which should receive
+  // decrypted specifics.
+  EXPECT_CALL(syncable_service_,
+              MergeDataAndStartSyncing(
+                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
+                  NotNull(), NotNull()));
+  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
+  ASSERT_FALSE(cryptographer->Encrypt(&specifics));
+  worker_->UpdateFromServer(kClientTagHash, specifics);
+  // The bridge's store should contain encrypted content.
+  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
+}
+
+TEST_F(SyncableServiceBasedBridgeTest,
+       ShouldHandleDecryptionErrorForInitialRemoteData) {
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  InitializeBridge(cryptographer);
+  StartSyncing();
+
+  EXPECT_CALL(mock_error_handler_, Run(_));
+  EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _)).Times(0);
+
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_encrypted()->set_blob("corrupt-encrypted-blob");
+  ASSERT_TRUE(cryptographer->Decrypt(&specifics));
+  worker_->UpdateFromServer(kClientTagHash, specifics);
+  EXPECT_THAT(GetAllData(), IsEmpty());
+}
+
+TEST_F(SyncableServiceBasedBridgeTest, ShouldDecryptForRemoteDeletion) {
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  InitializeBridge(cryptographer);
+  StartSyncing();
+
+  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
+  ASSERT_FALSE(cryptographer->Encrypt(&specifics));
+  worker_->UpdateFromServer(kClientTagHash, specifics);
+  ASSERT_THAT(start_syncing_sync_processor_, NotNull());
+  ASSERT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
+
+  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
+  EXPECT_CALL(syncable_service_,
+              ProcessSyncChanges(_, ElementsAre(SyncChangeMatches(
+                                        SyncChange::ACTION_DELETE, "name1"))));
+  worker_->TombstoneFromServer(kClientTagHash);
+  EXPECT_THAT(GetAllData(), IsEmpty());
+}
+
+TEST_F(SyncableServiceBasedBridgeTest,
+       ShouldDecryptPreviousDirectoryDataAfterRestart) {
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  InitializeBridge(cryptographer);
+  StartSyncing();
+
+  sync_pb::EntitySpecifics specifics = GetTestSpecifics("name1");
+  ASSERT_FALSE(cryptographer->Encrypt(&specifics));
+  worker_->UpdateFromServer(kClientTagHash, specifics);
+  // The bridge's store should contain encrypted content.
+  ASSERT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
+
+  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
+  // Mimic restart, which shouldn't start syncing until OnSyncStarting() is
+  // received (exercised in StartSyncing()).
+  ShutdownBridge();
+  InitializeBridge(cryptographer);
+
+  EXPECT_CALL(syncable_service_,
+              MergeDataAndStartSyncing(
+                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
+                  NotNull(), NotNull()));
+  StartSyncing();
+
+  // The bridge's store should still contain encrypted content.
+  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
+}
+
+TEST_F(SyncableServiceBasedBridgeTest, ShouldEncryptLocalCreation) {
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  InitializeBridge(cryptographer);
+  StartSyncing();
+  worker_->UpdateFromServer();
+
+  ASSERT_THAT(start_syncing_sync_processor_, NotNull());
+  ASSERT_THAT(GetAllData(), IsEmpty());
+  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
+
+  // The processor should receive encrypted data.
+  EXPECT_CALL(mock_processor_, Put(kClientTagHash, IsEncrypted(), NotNull()));
+
+  SyncChangeList change_list;
+  change_list.emplace_back(
+      FROM_HERE, SyncChange::ACTION_ADD,
+      SyncData::CreateLocalData(kClientTag, "title", GetTestSpecifics()));
+  const SyncError error =
+      start_syncing_sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
+  EXPECT_FALSE(error.IsSet());
+  EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
+}
+
 }  // namespace
 }  // namespace syncer

components/sync/protocol/password_specifics.proto

@@ -120,7 +120,8 @@ message PasswordSpecifics {
   // message.
   optional EncryptedData encrypted = 1;
   // An unsynced field for use internally on the client. This field should
-  // never be set in any network-based communications.
+  // never be set in any network-based communications because it contains
+  // unencrypted material.
   optional PasswordSpecificsData client_only_encrypted_data = 2;
   // Password related metadata, which is sent to the server side. The field
   // should never be set for full encryption users. If encryption is enabled,

components/sync/test/engine/mock_model_type_worker.cc

@@ -160,7 +160,9 @@ UpdateResponseData MockModelTypeWorker::GenerateUpdateData(
   data.creation_time = base::Time::UnixEpoch() + base::TimeDelta::FromDays(1);
   data.modification_time =
       data.creation_time + base::TimeDelta::FromSeconds(version);
-  data.non_unique_name = data.specifics.preference().name();
+  data.non_unique_name = data.specifics.has_encrypted()
+                             ? "encrypted"
+                             : data.specifics.preference().name();
 
   UpdateResponseData response_data;
   response_data.entity = data.PassToPtr();