Support kestore keys migration from prefs to USS storage

Directory implementation stores keystore keys in sync prefs. Since
missing keystore keys from the response are possible and can cause
significant sync issues (such as infinite configuration sync cycles),
it's worth to support their migration.

Packed keystore keys are propagated through the bridge constructor
and passed to SetKeystoreKeys() if following conditions satisfied:
1. There are some keystore keys in preferences.
2. There is no errors during unpacking keystore keys.
3. Bridge didn't yet received any keystore keys (i.e. they weren't
restored from Nigori storage).

Bug: 1030736, 922900
Change-Id: I4419da22dbe010e226dae61fdc4164d3a37b5e20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1950946
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721614}

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

@@ -28,6 +28,7 @@
 #include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/nigori.h"
+#include "content/public/test/test_launcher.h"
 #include "crypto/ec_private_key.h"
 #include "google_apis/gaia/gaia_switches.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -269,6 +270,25 @@ class SingleClientNigoriSyncTestWithNotAwaitQuiescence
   DISALLOW_COPY_AND_ASSIGN(SingleClientNigoriSyncTestWithNotAwaitQuiescence);
 };
 
+class SingleClientKeystoreKeysMigrationSyncTest : public SyncTest {
+ public:
+  SingleClientKeystoreKeysMigrationSyncTest() : SyncTest(SINGLE_CLIENT) {
+    if (content::IsPreTest()) {
+      override_features_.InitAndDisableFeature(switches::kSyncUSSNigori);
+    } else {
+      override_features_.InitWithFeatures(
+          /*enabled_features=*/{switches::kSyncUSSPasswords,
+                                switches::kSyncUSSNigori},
+          /*disabled_features=*/{});
+    }
+  }
+
+  ~SingleClientKeystoreKeysMigrationSyncTest() override = default;
+
+ private:
+  base::test::ScopedFeatureList override_features_;
+};
+
 IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
                        ShouldCommitKeystoreNigoriWhenReceivedDefault) {
   // SetupSync() should make FakeServer send default NigoriSpecifics.
@@ -451,6 +471,33 @@ INSTANTIATE_TEST_SUITE_P(USS,
                          SingleClientNigoriSyncTestWithNotAwaitQuiescence,
                          ::testing::Values(false, true));
 
+// Setups Sync with Directory Nigori, so keystore keys are persisted in prefs.
+IN_PROC_BROWSER_TEST_F(SingleClientKeystoreKeysMigrationSyncTest,
+                       PRE_ShouldMigrateKeystoreKeysFromPrefs) {
+  ASSERT_TRUE(SetupSync());
+}
+
+// Disallows population of keystore keys from the server, so preferences are
+// the only source for keystore keys.
+IN_PROC_BROWSER_TEST_F(SingleClientKeystoreKeysMigrationSyncTest,
+                       ShouldMigrateKeystoreKeysFromPrefs) {
+  GetFakeServer()->DisallowSendingEncryptionKeys();
+  EXPECT_TRUE(SetupClients());
+
+  // Ensure that client can decrypt with keystore keys.
+  const std::vector<std::string>& keystore_keys =
+      GetFakeServer()->GetKeystoreKeys();
+  ASSERT_THAT(keystore_keys, SizeIs(1));
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(keystore_keys.back());
+
+  const autofill::PasswordForm password_form =
+      passwords_helper::CreateTestPasswordForm(0);
+  passwords_helper::InjectEncryptedServerPassword(
+      password_form, kKeystoreKeyParams.password,
+      kKeystoreKeyParams.derivation_params, GetFakeServer());
+  EXPECT_TRUE(PasswordFormsChecker(0, {password_form}).Wait());
+}
+
 class SingleClientNigoriWithWebApiTest : public SyncTest {
  public:
   SingleClientNigoriWithWebApiTest() : SyncTest(SINGLE_CLIENT) {

components/sync/driver/glue/sync_engine_backend.cc

@@ -332,7 +332,8 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
         std::make_unique<NigoriStorageImpl>(
             sync_data_folder_.Append(kNigoriStorageFilename), &encryptor_),
         &encryptor_, base::BindRepeating(&Nigori::GenerateScryptSalt),
-        params.restored_key_for_bootstrapping);
+        params.restored_key_for_bootstrapping,
+        params.restored_keystore_key_for_bootstrapping);
     nigori_handler_proxy_ =
         std::make_unique<syncable::NigoriHandlerProxy>(&user_share_);
     sync_encryption_handler_->AddObserver(nigori_handler_proxy_.get());

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -9,6 +9,7 @@
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/feature_list.h"
+#include "base/json/json_string_value_serializer.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/sync/base/encryptor.h"
@@ -302,6 +303,44 @@ sync_pb::NigoriKey UnpackExplicitPassphraseKey(const Encryptor& encryptor,
   return key;
 }
 
+// Returns Base64 encoded keystore keys or empty vector if errors occur. Should
+// be aligned with Directory implementation (UnpackKeystoreBootstrapToken())
+// unless it is removed.
+std::vector<std::string> UnpackKeystoreKeys(
+    const std::string& packed_keystore_keys,
+    const Encryptor& encryptor) {
+  DCHECK(!packed_keystore_keys.empty());
+
+  std::string base64_decoded_packed_keys;
+  if (!base::Base64Decode(packed_keystore_keys, &base64_decoded_packed_keys)) {
+    return std::vector<std::string>();
+  }
+  std::string decrypted_packed_keys;
+  if (!encryptor.DecryptString(base64_decoded_packed_keys,
+                               &decrypted_packed_keys)) {
+    return std::vector<std::string>();
+  }
+
+  JSONStringValueDeserializer json_deserializer(decrypted_packed_keys);
+  std::unique_ptr<base::Value> deserialized_keys(json_deserializer.Deserialize(
+      /*error_code=*/nullptr, /*error_message=*/nullptr));
+  if (!deserialized_keys) {
+    return std::vector<std::string>();
+  }
+  base::ListValue* list_value = nullptr;
+  if (!deserialized_keys->GetAsList(&list_value)) {
+    return std::vector<std::string>();
+  }
+
+  std::vector<std::string> keystore_keys(list_value->GetSize());
+  for (size_t i = 0; i < keystore_keys.size(); ++i) {
+    if (!list_value->GetString(i, &keystore_keys[i])) {
+      return std::vector<std::string>();
+    }
+  }
+  return keystore_keys;
+}
+
 ModelTypeSet GetEncryptedTypes(bool encrypt_everything) {
   if (encrypt_everything) {
     return EncryptableUserTypes();
@@ -408,7 +447,8 @@ NigoriSyncBridgeImpl::NigoriSyncBridgeImpl(
     std::unique_ptr<NigoriStorage> storage,
     const Encryptor* encryptor,
     const base::RepeatingCallback<std::string()>& random_salt_generator,
-    const std::string& packed_explicit_passphrase_key)
+    const std::string& packed_explicit_passphrase_key,
+    const std::string& packed_keystore_keys)
     : encryptor_(encryptor),
       processor_(std::move(processor)),
       storage_(std::move(storage)),
@@ -437,6 +477,8 @@ NigoriSyncBridgeImpl::NigoriSyncBridgeImpl(
            switches::kSyncSupportTrustedVaultPassphrase))) {
     // We either have no Nigori node stored locally or it was corrupted.
     processor_->ModelReadyToSync(this, NigoriMetadataBatch());
+    // Keystore keys needs migration independently of having local Nigori node.
+    MaybeMigrateKeystoreKeys(packed_keystore_keys);
     return;
   }
 
@@ -450,6 +492,10 @@ NigoriSyncBridgeImpl::NigoriSyncBridgeImpl(
   metadata_batch.entity_metadata = deserialized_data->entity_metadata();
   processor_->ModelReadyToSync(this, std::move(metadata_batch));
 
+  // Attempt migration of keystore keys after deserialization to not overwrite
+  // newer keys.
+  MaybeMigrateKeystoreKeys(packed_keystore_keys);
+
   if (state_.passphrase_type == NigoriSpecifics::UNKNOWN) {
     // Commit with keystore initialization wasn't successfully completed before
     // the restart, so trigger it again here.
@@ -1156,6 +1202,28 @@ void NigoriSyncBridgeImpl::MaybeTriggerKeystoreKeyRotation() {
   }
 }
 
+void NigoriSyncBridgeImpl::MaybeMigrateKeystoreKeys(
+    const std::string& packed_keystore_keys) {
+  if (!state_.keystore_keys_cryptographer->IsEmpty() ||
+      packed_keystore_keys.empty()) {
+    return;
+  }
+  std::vector<std::string> keystore_keys =
+      UnpackKeystoreKeys(packed_keystore_keys, *encryptor_);
+  if (keystore_keys.empty()) {
+    // Error occurred during unpacking.
+    return;
+  }
+
+  state_.keystore_keys_cryptographer =
+      KeystoreKeysCryptographer::FromKeystoreKeys(keystore_keys);
+  if (!state_.keystore_keys_cryptographer) {
+    // Crypto error occurred during cryptographer creation.
+    state_.keystore_keys_cryptographer =
+        KeystoreKeysCryptographer::CreateEmpty();
+  }
+}
+
 void NigoriSyncBridgeImpl::QueuePendingLocalCommit(
     std::unique_ptr<PendingLocalNigoriCommit> local_commit) {
   DCHECK(processor_->IsTrackingMetadata());

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -54,7 +54,8 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
       std::unique_ptr<NigoriStorage> storage,
       const Encryptor* encryptor,
       const base::RepeatingCallback<std::string()>& random_salt_generator,
-      const std::string& packed_explicit_passphrase_key);
+      const std::string& packed_explicit_passphrase_key,
+      const std::string& packed_keystore_keys);
   ~NigoriSyncBridgeImpl() override;
 
   // SyncEncryptionHandler implementation.
@@ -141,6 +142,16 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   // Queues keystore rotation if current state assume it should happen.
   void MaybeTriggerKeystoreKeyRotation();
 
+  // Prior to USS keystore keys were stored in preferences. To avoid redundant
+  // requests to the server and make USS implementation more robust against
+  // failing such requests, the value restored from preferences should be
+  // populated to current |state_|. Performs unpacking of
+  // |packed_keystore_keys| and populates them to
+  // |keystore_keys_cryptographer|. Has no effect if |packed_keystore_keys| is
+  // empty, errors occur during deserealization or
+  // |keystore_keys_cryptographer| already has keys.
+  void MaybeMigrateKeystoreKeys(const std::string& packed_keystore_keys);
+
   // Serializes state of the bridge and sync metadata into the proto.
   sync_pb::NigoriLocalData SerializeAsNigoriLocalData() const;
 

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -355,7 +355,8 @@ class NigoriSyncBridgeImplTest : public testing::Test {
     bridge_ = std::make_unique<NigoriSyncBridgeImpl>(
         std::move(processor), std::move(storage), &encryptor_,
         base::BindRepeating(&Nigori::GenerateScryptSalt),
-        /*packed_explicit_passphrase_key=*/std::string());
+        /*packed_explicit_passphrase_key=*/std::string(),
+        /*packed_keystore_keys=*/std::string());
     bridge_->AddObserver(&observer_);
   }
 
@@ -1111,7 +1112,8 @@ TEST(NigoriSyncBridgeImplTestWithPackedExplicitPassphrase,
       std::move(processor),
       std::make_unique<testing::NiceMock<MockNigoriStorage>>(), &encryptor,
       base::BindRepeating(&Nigori::GenerateScryptSalt),
-      PackKeyAsExplicitPassphrase(kKeyParams, encryptor));
+      PackKeyAsExplicitPassphrase(kKeyParams, encryptor),
+      /*packed_keystore_keys=*/std::string());
   testing::NiceMock<MockObserver> observer;
   bridge->AddObserver(&observer);
 
@@ -1155,7 +1157,8 @@ TEST(NigoriSyncBridgeImplPersistenceTest, ShouldRestoreKeystoreNigori) {
   auto bridge1 = std::make_unique<NigoriSyncBridgeImpl>(
       std::move(processor1), std::move(storage1), &kEncryptor,
       base::BindRepeating(&Nigori::GenerateScryptSalt),
-      /*packed_explicit_passphrase_key=*/std::string());
+      /*packed_explicit_passphrase_key=*/std::string(),
+      /*packed_keystore_keys=*/std::string());
 
   // Perform initial sync with simple keystore Nigori.
   const std::string kRawKeystoreKey = "raw_keystore_key";
@@ -1191,7 +1194,8 @@ TEST(NigoriSyncBridgeImplPersistenceTest, ShouldRestoreKeystoreNigori) {
   auto bridge2 = std::make_unique<NigoriSyncBridgeImpl>(
       std::move(processor2), std::move(storage2), &kEncryptor,
       base::BindRepeating(&Nigori::GenerateScryptSalt),
-      /*packed_explicit_passphrase_key=*/std::string());
+      /*packed_explicit_passphrase_key=*/std::string(),
+      /*packed_keystore_keys=*/std::string());
 
   // Verify that we restored Cryptographer state.
   const Cryptographer& cryptographer = bridge2->GetCryptographerForTesting();
@@ -1230,7 +1234,8 @@ TEST(NigoriSyncBridgeImplPersistenceTest,
   auto bridge = std::make_unique<NigoriSyncBridgeImpl>(
       std::move(processor), std::move(storage), &kEncryptor,
       base::BindRepeating(&Nigori::GenerateScryptSalt),
-      /*packed_explicit_passphrase_key=*/std::string());
+      /*packed_explicit_passphrase_key=*/std::string(),
+      /*packed_keystore_keys=*/std::string());
   EXPECT_THAT(bridge->GetData(), HasKeystoreNigori());
 
   // Emulate commit completeness.

components/sync/test/fake_server/fake_server.cc

@@ -40,7 +40,8 @@ FakeServer::FakeServer()
     : commit_error_type_(sync_pb::SyncEnums::SUCCESS),
       error_type_(sync_pb::SyncEnums::SUCCESS),
       alternate_triggered_errors_(false),
-      request_counter_(0) {
+      request_counter_(0),
+      disallow_sending_encryption_keys_(false) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   loopback_server_storage_ = std::make_unique<base::ScopedTempDir>();
   if (!loopback_server_storage_->CreateUniqueTempDir()) {
@@ -55,7 +56,8 @@ FakeServer::FakeServer(const base::FilePath& user_data_dir)
     : commit_error_type_(sync_pb::SyncEnums::SUCCESS),
       error_type_(sync_pb::SyncEnums::SUCCESS),
       alternate_triggered_errors_(false),
-      request_counter_(0) {
+      request_counter_(0),
+      disallow_sending_encryption_keys_(false) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   base::FilePath loopback_server_path =
       user_data_dir.AppendASCII("FakeSyncServer");
@@ -291,6 +293,10 @@ net::HttpStatusCode FakeServer::HandleParsedCommand(
   net::HttpStatusCode http_status_code =
       SendToLoopbackServer(message_without_wallet, response);
 
+  if (response->has_get_updates() && disallow_sending_encryption_keys_) {
+    response->mutable_get_updates()->clear_encryption_keys();
+  }
+
   if (wallet_marker != nullptr && http_status_code == net::HTTP_OK &&
       message.message_contents() ==
           sync_pb::ClientToServerMessage::GET_UPDATES) {
@@ -526,6 +532,10 @@ bool FakeServer::EnableAlternatingTriggeredErrors() {
   return true;
 }
 
+void FakeServer::DisallowSendingEncryptionKeys() {
+  disallow_sending_encryption_keys_ = true;
+}
+
 bool FakeServer::ShouldSendTriggeredError() const {
   if (!alternate_triggered_errors_)
     return true;

components/sync/test/fake_server/fake_server.h

@@ -184,6 +184,10 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   // triggered error alternating was successful.
   bool EnableAlternatingTriggeredErrors();
 
+  // If called, all subsequent GetUpdatesResponses won't contain
+  // encryption_keys.
+  void DisallowSendingEncryptionKeys();
+
   // Adds |observer| to FakeServer's observer list. This should be called
   // before the Profile associated with |observer| is connected to the server.
   void AddObserver(Observer* observer);
@@ -273,6 +277,10 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
   bool alternate_triggered_errors_;
   int request_counter_;
 
+  // If set to true all |this| will clear |encryption_keys| in all
+  // GetUpdateResponse's.
+  bool disallow_sending_encryption_keys_;
+
   // Client command to be included in every response.
   sync_pb::ClientCommand client_command_;