Add reencryption support to pseudo-USS PASSWORDS

Regular USS datatypes (bridges) don't need to care about encryption,
because the processor and worker take care and the original model
doesn't need to be encrypted.

PASSWORDS on pseudo-USS are special because we need that the "directory"
copy (the one in SyncableServiceBasedBridge) is stored encrypted on
disk too, similarly to how the legacy Directory does it.

In certain cases like when setting up a custom passphrase, the
encryption key changes and the processor&worker react to that by
recommitting all entities with the new encryption requirements. For
pseudo-USS PASSWORDS, it should also reencrypt the local "directory".

In order to do this:
1. A new method is introduced in ModelTypeSyncBridge, for the bridge
   to realize there's an ongoing reencryption.

2. SyncableServiceBasedBridge takes care of using the cryptographer
   (available for PASSWORDS only) to reencrypt all data.

Implementation-wise, the simplest way to achieve that is to modify
the bridge such that in_memory_store_ keeps more information, namely
the whole sync_pb::PersistedEntityData proto.

Bug: 870624
Change-Id: I1e0d7c972580377618c05b9d1f79c6d72f58022f
Reviewed-on: https://chromium-review.googlesource.com/c/1288629
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601863}

chrome/browser/sync/test/integration/single_client_passwords_sync_test.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "chrome/browser/sync/test/integration/encryption_helper.h"
 #include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
@@ -58,7 +59,6 @@ IN_PROC_BROWSER_TEST_P(SingleClientPasswordsSyncTest,
   ASSERT_EQ(1, GetVerifierPasswordCount());
   AddLogin(GetPasswordStore(0), form);
   ASSERT_EQ(1, GetPasswordCount(0));
-
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
   const std::vector<sync_pb::SyncEntity> entities =
@@ -85,7 +85,54 @@ IN_PROC_BROWSER_TEST_P(SingleClientPasswordsSyncTest,
   ASSERT_EQ(1, GetVerifierPasswordCount());
   AddLogin(GetPasswordStore(0), form);
   ASSERT_EQ(1, GetPasswordCount(0));
+  ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
+
+  const std::vector<sync_pb::SyncEntity> entities =
+      fake_server_->GetSyncEntitiesByModelType(syncer::PASSWORDS);
+  ASSERT_EQ(1U, entities.size());
+  EXPECT_EQ("", entities[0].non_unique_name());
+  EXPECT_TRUE(entities[0].specifics().password().has_encrypted());
+  EXPECT_FALSE(
+      entities[0].specifics().password().has_client_only_encrypted_data());
+  EXPECT_FALSE(entities[0].specifics().password().has_unencrypted_metadata());
+}
+
+// Tests the scenario when a syncing user enables a custom passphrase. PASSWORDS
+// should be recommitted with the new encryption key.
+IN_PROC_BROWSER_TEST_P(SingleClientPasswordsSyncTest,
+                       ReencryptsDataWhenPassphraseIsSet) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(ServerNigoriChecker(GetSyncService(0), fake_server_.get(),
+                                  syncer::PassphraseType::KEYSTORE_PASSPHRASE)
+                  .Wait());
+
+  PasswordForm form = CreateTestPasswordForm(0);
+  AddLogin(GetVerifierPasswordStore(), form);
+  ASSERT_EQ(1, GetVerifierPasswordCount());
+  AddLogin(GetPasswordStore(0), form);
+  ASSERT_EQ(1, GetPasswordCount(0));
+  ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
+  std::string prior_encryption_key_name;
+  {
+    const std::vector<sync_pb::SyncEntity> entities =
+        fake_server_->GetSyncEntitiesByModelType(syncer::PASSWORDS);
+    ASSERT_EQ(1U, entities.size());
+    ASSERT_EQ("", entities[0].non_unique_name());
+    ASSERT_TRUE(entities[0].specifics().password().has_encrypted());
+    ASSERT_FALSE(
+        entities[0].specifics().password().has_client_only_encrypted_data());
+    ASSERT_TRUE(entities[0].specifics().password().has_unencrypted_metadata());
+    prior_encryption_key_name =
+        entities[0].specifics().password().encrypted().key_name();
+  }
+
+  ASSERT_FALSE(prior_encryption_key_name.empty());
+
+  GetSyncService(0)->SetEncryptionPassphrase("hunter2");
+  ASSERT_TRUE(ServerNigoriChecker(GetSyncService(0), fake_server_.get(),
+                                  syncer::PassphraseType::CUSTOM_PASSPHRASE)
+                  .Wait());
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
   const std::vector<sync_pb::SyncEntity> entities =
@@ -96,6 +143,11 @@ IN_PROC_BROWSER_TEST_P(SingleClientPasswordsSyncTest,
   EXPECT_FALSE(
       entities[0].specifics().password().has_client_only_encrypted_data());
   EXPECT_FALSE(entities[0].specifics().password().has_unencrypted_metadata());
+
+  const std::string new_encryption_key_name =
+      entities[0].specifics().password().encrypted().key_name();
+  EXPECT_FALSE(new_encryption_key_name.empty());
+  EXPECT_NE(new_encryption_key_name, prior_encryption_key_name);
 }
 
 INSTANTIATE_TEST_CASE_P(USS,

components/sync/model/model_type_sync_bridge.cc

@@ -64,4 +64,12 @@ ModelTypeChangeProcessor* ModelTypeSyncBridge::change_processor() {
   return change_processor_.get();
 }
 
+base::Optional<ModelError>
+ModelTypeSyncBridge::ApplySyncChangesWithNewEncryptionRequirements(
+    std::unique_ptr<MetadataChangeList> metadata_change_list,
+    EntityChangeList entity_changes) {
+  return ApplySyncChanges(std::move(metadata_change_list),
+                          std::move(entity_changes));
+}
+
 }  // namespace syncer

components/sync/model/model_type_sync_bridge.h

@@ -179,6 +179,15 @@ class ModelTypeSyncBridge {
   // before or atomically with the model changes.
   ModelTypeChangeProcessor* change_processor();
 
+  // Similar to ApplySyncChanges(), but notifies the bridge that the processor
+  // is about to recommit all data due to encryption changes.
+  // TODO(crbug.com/856941): Remove when PASSWORDS are migrated to USS, which
+  // will likely make this API unnecessary.
+  virtual base::Optional<ModelError>
+  ApplySyncChangesWithNewEncryptionRequirements(
+      std::unique_ptr<MetadataChangeList> metadata_change_list,
+      EntityChangeList entity_changes);
+
  private:
   std::unique_ptr<ModelTypeChangeProcessor> change_processor_;
 };

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -849,7 +849,7 @@ ConflictResolution::Type ClientTagBasedModelTypeProcessor::ResolveConflict(
 }
 
 void ClientTagBasedModelTypeProcessor::RecommitAllForEncryption(
-    std::unordered_set<std::string> already_updated,
+    const std::unordered_set<std::string>& already_updated,
     MetadataChangeList* metadata_changes) {
   ModelTypeSyncBridge::StorageKeyList entities_needing_data;
 
@@ -1026,11 +1026,12 @@ ClientTagBasedModelTypeProcessor::OnIncrementalUpdateReceived(
     // recommit only the ones whose encryption key doesn't match the one in
     // DataTypeState. Work is tracked in http://crbug.com/727874.
     RecommitAllForEncryption(already_updated, metadata_changes.get());
+    return bridge_->ApplySyncChangesWithNewEncryptionRequirements(
+        std::move(metadata_changes), std::move(entity_changes));
   }
   // Inform the bridge of the new or updated data.
-  base::Optional<ModelError> error =
-      bridge_->ApplySyncChanges(std::move(metadata_changes), entity_changes);
-  return error;
+  return bridge_->ApplySyncChanges(std::move(metadata_changes),
+                                   std::move(entity_changes));
 }
 
 void ClientTagBasedModelTypeProcessor::OnPendingDataLoaded(

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -131,8 +131,9 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                                            EntityChangeList* changes);
 
   // Recommit all entities for encryption except those in |already_updated|.
-  void RecommitAllForEncryption(std::unordered_set<std::string> already_updated,
-                                MetadataChangeList* metadata_changes);
+  void RecommitAllForEncryption(
+      const std::unordered_set<std::string>& already_updated,
+      MetadataChangeList* metadata_changes);
 
   // Validates the update specified by the input parameters and returns whether
   // it should get further processed. If the update is incorrect, this function

components/sync/model_impl/syncable_service_based_bridge.h

@@ -21,6 +21,7 @@
 
 namespace sync_pb {
 class EntitySpecifics;
+class PersistedEntityData;
 }  // namespace sync_pb
 
 namespace syncer {
@@ -88,6 +89,9 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
   StopSyncResponse ApplyStopSyncChanges(
       std::unique_ptr<MetadataChangeList> delete_metadata_change_list) override;
   size_t EstimateSyncOverheadMemoryUsage() const override;
+  base::Optional<ModelError> ApplySyncChangesWithNewEncryptionRequirements(
+      std::unique_ptr<MetadataChangeList> metadata_change_list,
+      EntityChangeList entity_changes) override;
 
  private:
   void OnStoreCreated(const base::Optional<ModelError>& error,
@@ -99,19 +103,16 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
                                 std::unique_ptr<MetadataBatch> metadata_batch);
   void MaybeStartSyncableService();
   base::Optional<ModelError> StoreAndConvertRemoteChanges(
-      std::unique_ptr<MetadataChangeList> metadata_change_list,
+      std::unique_ptr<ModelTypeStore::WriteBatch> batch,
       EntityChangeList input_entity_change_list,
       SyncChangeList* output_sync_change_list);
-  void OnReadDataForProcessor(
-      DataCallback callback,
-      const base::Optional<ModelError>& error,
-      std::unique_ptr<ModelTypeStore::RecordList> record_list,
-      std::unique_ptr<ModelTypeStore::IdList> missing_id_list);
-  void OnReadAllDataForProcessor(
-      DataCallback callback,
-      const base::Optional<ModelError>& error,
-      std::unique_ptr<ModelTypeStore::RecordList> record_list);
   void ReportErrorIfSet(const base::Optional<ModelError>& error);
+  base::Optional<ModelError> ReencryptEverything(
+      ModelTypeStore::WriteBatch* batch);
+  base::Optional<ModelError> ApplySyncChangesWithBatch(
+      std::unique_ptr<MetadataChangeList> metadata_change_list,
+      EntityChangeList entity_change_list,
+      std::unique_ptr<ModelTypeStore::WriteBatch> batch);
 
   const ModelType type_;
   SyncableService* const syncable_service_;
@@ -123,7 +124,7 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
 
   // In-memory copy of |store_|, needed for remote deletions, because we need to
   // provide specifics of the deleted entity to the SyncableService.
-  std::map<std::string, sync_pb::EntitySpecifics> in_memory_store_;
+  std::map<std::string, sync_pb::PersistedEntityData> in_memory_store_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/sync/model_impl/syncable_service_based_bridge.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -68,6 +69,10 @@ MATCHER(IsEncrypted, "") {
   return arg && arg->specifics.encrypted().has_blob();
 }
 
+MATCHER_P(IsEncryptedWithKey, key, "") {
+  return arg && arg->specifics.encrypted().key_name() == key;
+}
+
 class MockSyncableService : public SyncableService {
  public:
   MOCK_METHOD4(
@@ -84,18 +89,30 @@ class MockSyncableService : public SyncableService {
 };
 
 // Simple ModelCryptographer implementation that serializes the input proto to a
-// string and puts it in proto field EntitySpecifics.encrypted.blob.
+// string, prepends a prefix (representing the encryption key) and puts the
+// result in proto field EntitySpecifics.encrypted.blob.
 class TestModelCryptographer : public ModelCryptographer {
  public:
-  TestModelCryptographer() {}
+  TestModelCryptographer() { AddKey("DefaultKey"); }
+
+  void AddKey(const std::string& key) { keys_.push_back(key); }
 
   base::Optional<ModelError> Decrypt(
       sync_pb::EntitySpecifics* specifics) override {
     if (!specifics->has_encrypted()) {
       return ModelError(FROM_HERE, "Not encrypted");
     }
+    const std::string& actual_key = specifics->encrypted().key_name();
+    if (std::find(keys_.begin(), keys_.end(), actual_key) == keys_.end()) {
+      return ModelError(FROM_HERE, "Unknown key");
+    }
+    if (specifics->encrypted().blob().find(actual_key) != 0) {
+      return ModelError(FROM_HERE, "Corrupt blob");
+    }
     sync_pb::EntitySpecifics unencrypted_specifics;
-    if (!unencrypted_specifics.ParseFromString(specifics->encrypted().blob())) {
+    const std::string blob_without_key =
+        specifics->encrypted().blob().substr(actual_key.size());
+    if (!unencrypted_specifics.ParseFromString(blob_without_key)) {
       return ModelError(FROM_HERE, "Cannot parse blob");
     }
     *specifics = unencrypted_specifics;
@@ -108,8 +125,9 @@ class TestModelCryptographer : public ModelCryptographer {
       return ModelError(FROM_HERE, "Already encrypted");
     }
     sync_pb::EntitySpecifics encrypted_specifics;
+    encrypted_specifics.mutable_encrypted()->set_key_name(keys_.back());
     encrypted_specifics.mutable_encrypted()->set_blob(
-        specifics->SerializeAsString());
+        keys_.back() + specifics->SerializeAsString());
     *specifics = encrypted_specifics;
     return base::nullopt;
   }
@@ -118,6 +136,8 @@ class TestModelCryptographer : public ModelCryptographer {
   ~TestModelCryptographer() override {}
 
  private:
+  std::vector<std::string> keys_;
+
   DISALLOW_COPY_AND_ASSIGN(TestModelCryptographer);
 };
 
@@ -529,7 +549,6 @@ TEST_F(SyncableServiceBasedBridgeTest,
   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) {
@@ -602,5 +621,48 @@ TEST_F(SyncableServiceBasedBridgeTest, ShouldEncryptLocalCreation) {
   EXPECT_THAT(GetAllData(), ElementsAre(Pair(kClientTagHash, IsEncrypted())));
 }
 
+TEST_F(SyncableServiceBasedBridgeTest, ShouldReencryptDataUponKeyChange) {
+  const std::string kEncryptionKey1 = "EncryptionKey1";
+  const std::string kEncryptionKey2 = "EncryptionKey2";
+
+  auto cryptographer = base::MakeRefCounted<TestModelCryptographer>();
+  cryptographer->AddKey(kEncryptionKey1);
+  InitializeBridge(cryptographer);
+  StartSyncing();
+
+  EXPECT_CALL(mock_error_handler_, Run(_)).Times(0);
+
+  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, IsEncryptedWithKey(kEncryptionKey1))));
+
+  // Mimic reencryption.
+  EXPECT_CALL(mock_processor_, Put(_, _, _)).Times(0);
+  cryptographer->AddKey(kEncryptionKey2);
+  worker_->UpdateWithEncryptionKey(kEncryptionKey2);
+
+  // The bridge's store should still contain encrypted content.
+  EXPECT_THAT(
+      GetAllData(),
+      ElementsAre(Pair(kClientTagHash, IsEncryptedWithKey(kEncryptionKey2))));
+
+  // Mimic restart, which shouldn't start syncing until OnSyncStarting() is
+  // received (exercised in StartSyncing()). Having reencrypted the data should
+  // not be noticeable by the SyncableService, which should receive the data
+  // unencrypted.
+  ShutdownBridge();
+  InitializeBridge(cryptographer);
+
+  EXPECT_CALL(syncable_service_,
+              MergeDataAndStartSyncing(
+                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
+                  NotNull(), NotNull()));
+  StartSyncing();
+}
+
 }  // namespace
 }  // namespace syncer