Extend sync's Javascript API with key versions

For debugging and future feature development, the web should be allowed
to pass a key version in addition to key material. If multiple keys are
involved, only the version of the newest is relevant, so the API is kept
as simple as possible by plumbing a single integer.

Bug: 1032485
Change-Id: I0e53c54176e92c1dfa6ffcb496611ae53c8c113f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991432Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730078}

chrome/browser/sync/sync_encryption_keys_tab_helper.cc

@@ -53,14 +53,15 @@ class SyncEncryptionKeysTabHelper::EncryptionKeyApi
 
   // chrome::mojom::SyncEncryptionKeysExtension:
   void SetEncryptionKeys(
-      const std::vector<std::vector<uint8_t>>& encryption_keys,
       const std::string& gaia_id,
+      const std::vector<std::vector<uint8_t>>& encryption_keys,
+      int last_key_version,
       SetEncryptionKeysCallback callback) override {
     CHECK_EQ(receivers_.GetCurrentTargetFrame()->GetLastCommittedOrigin(),
              GetAllowedOrigin());
 
-    sync_service_->AddTrustedVaultDecryptionKeysFromWeb(gaia_id,
-                                                        encryption_keys);
+    sync_service_->AddTrustedVaultDecryptionKeysFromWeb(
+        gaia_id, encryption_keys, last_key_version);
     std::move(callback).Run();
   }
 

chrome/browser/sync/trusted_vault_client_android.cc

@@ -93,7 +93,8 @@ void TrustedVaultClientAndroid::FetchKeys(
 
 void TrustedVaultClientAndroid::StoreKeys(
     const std::string& gaia_id,
-    const std::vector<std::vector<uint8_t>>& keys) {
+    const std::vector<std::vector<uint8_t>>& keys,
+    int last_key_version) {
   // Not supported on Android, where keys are fetched outside the browser.
   NOTREACHED();
 }

chrome/browser/sync/trusted_vault_client_android.h

@@ -49,7 +49,8 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb)
       override;
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::vector<uint8_t>>& keys) override;
+                 const std::vector<std::vector<uint8_t>>& keys,
+                 int last_key_version) override;
   void MarkKeysAsStale(const std::string& gaia_id,
                        base::OnceCallback<void(bool)> cb) override;
 

chrome/common/sync_encryption_keys_extension.mojom

@@ -11,5 +11,6 @@ interface SyncEncryptionKeysExtension {
   // Provides sync encryption keys to the browser process. Returns true if no
   // further encryption keys are needed (which includes the case, among others,
   // of encryption keys not being needed even prior to this call).
-  SetEncryptionKeys(array<array<uint8>> encryption_keys, string gaia_id) => ();
+  SetEncryptionKeys(string gaia_id, array<array<uint8>> encryption_keys,
+                    int32 last_key_version) => ();
 };

chrome/renderer/sync_encryption_keys_extension.cc

@@ -114,13 +114,16 @@ void SyncEncryptionKeysExtension::SetSyncEncryptionKeys(gin::Arguments* args) {
   DCHECK(render_frame());
 
   // This function as exposed to the web has the following signature:
-  //   setSyncEncryptionKeys(callback, gaia_id, encryption_keys)
+  //   setSyncEncryptionKeys(callback, gaia_id, encryption_keys,
+  //                         last_key_version)
   //
   // Where:
   //   callback: Allows caller to get notified upon completion.
   //   gaia_id: String representing the user's server-provided ID.
   //   encryption_keys: Array where each element is an ArrayBuffer representing
   //                    an encryption key (binary blob).
+  //   last_key_version: Key version corresponding to the last key in
+  //                     |encryption_keys|.
 
   v8::HandleScope handle_scope(args->isolate());
 
@@ -151,6 +154,14 @@ void SyncEncryptionKeysExtension::SetSyncEncryptionKeys(gin::Arguments* args) {
     return;
   }
 
+  int last_key_version = 0;
+  if (!args->GetNext(&last_key_version)) {
+    DLOG(ERROR) << "No version provided";
+    // TODO(crbug.com/1032485): Be more strict here and issue an error if the
+    // version is not provided.
+    last_key_version = static_cast<int>(encryption_keys.size()) - 1;
+  }
+
   auto global_callback =
       std::make_unique<v8::Global<v8::Function>>(args->isolate(), callback);
 
@@ -159,7 +170,7 @@ void SyncEncryptionKeysExtension::SetSyncEncryptionKeys(gin::Arguments* args) {
   }
 
   remote_->SetEncryptionKeys(
-      EncryptionKeysAsBytes(encryption_keys), gaia_id,
+      gaia_id, EncryptionKeysAsBytes(encryption_keys), last_key_version,
       base::BindOnce(&SyncEncryptionKeysExtension::RunCompletionCallback,
                      weak_ptr_factory_.GetWeakPtr(),
                      std::move(global_callback)));

chrome/test/data/sync/encryption_keys_retrieval.html

@@ -19,7 +19,8 @@ window.onload = function() {
   } else {
     chrome.setSyncEncryptionKeys(() => { document.title = "OK"; },
                                  location.search.substring(1),
-                                 [asciiToArrayBuffer(location.hash.substring(1))]);
+                                 [asciiToArrayBuffer(location.hash.substring(1))],
+                                 /*last_key_version=*/7);
   }
 }
 </script>

components/sync/driver/fake_sync_service.cc

@@ -165,7 +165,8 @@ void FakeSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {}
 
 void FakeSyncService::AddTrustedVaultDecryptionKeysFromWeb(
     const std::string& gaia_id,
-    const std::vector<std::vector<uint8_t>>& keys) {}
+    const std::vector<std::vector<uint8_t>>& keys,
+    int last_key_version) {}
 
 UserDemographicsResult FakeSyncService::GetUserNoisedBirthYearAndGender(
     base::Time now) {

components/sync/driver/fake_sync_service.h

@@ -70,7 +70,8 @@ class FakeSyncService : public SyncService {
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
   void AddTrustedVaultDecryptionKeysFromWeb(
       const std::string& gaia_id,
-      const std::vector<std::vector<uint8_t>>& keys) override;
+      const std::vector<std::vector<uint8_t>>& keys,
+      int last_key_version) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
 

components/sync/driver/file_based_trusted_vault_client.cc

@@ -77,7 +77,8 @@ class FileBasedTrustedVaultClient::Backend
   }
 
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::vector<uint8_t>>& keys) {
+                 const std::vector<std::vector<uint8_t>>& keys,
+                 int last_key_version) {
     // Find or create user for |gaid_id|.
     sync_pb::LocalTrustedVaultPerUser* per_user_vault = FindUserVault(gaia_id);
     if (!per_user_vault) {
@@ -86,6 +87,7 @@ class FileBasedTrustedVaultClient::Backend
     }
 
     // Replace all keys.
+    per_user_vault->set_last_key_version(last_key_version);
     per_user_vault->clear_key();
     for (const std::vector<uint8_t>& key : keys) {
       per_user_vault->add_key()->set_key_material(key.data(), key.size());
@@ -141,10 +143,12 @@ void FileBasedTrustedVaultClient::FetchKeys(
 
 void FileBasedTrustedVaultClient::StoreKeys(
     const std::string& gaia_id,
-    const std::vector<std::vector<uint8_t>>& keys) {
+    const std::vector<std::vector<uint8_t>>& keys,
+    int last_key_version) {
   TriggerLazyInitializationIfNeeded();
   backend_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&Backend::StoreKeys, backend_, gaia_id, keys));
+      FROM_HERE, base::BindOnce(&Backend::StoreKeys, backend_, gaia_id, keys,
+                                last_key_version));
   observer_list_.Notify();
 }
 

components/sync/driver/file_based_trusted_vault_client.h

@@ -36,7 +36,8 @@ class FileBasedTrustedVaultClient : public TrustedVaultClient {
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb)
       override;
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::vector<uint8_t>>& keys) override;
+                 const std::vector<std::vector<uint8_t>>& keys,
+                 int last_key_version) override;
   void MarkKeysAsStale(const std::string& gaia_id,
                        base::OnceCallback<void(bool)> cb) override;
 

components/sync/driver/file_based_trusted_vault_client_unittest.cc

@@ -133,10 +133,10 @@ TEST_F(FileBasedTrustedVaultClientTest, ShouldStoreKeys) {
   const std::vector<uint8_t> kKey3 = {2, 3, 4};
   const std::vector<uint8_t> kKey4 = {3, 4};
 
-  client_.StoreKeys(kGaiaId1, {kKey1});
-  client_.StoreKeys(kGaiaId2, {kKey2});
+  client_.StoreKeys(kGaiaId1, {kKey1}, /*last_key_version=*/7);
+  client_.StoreKeys(kGaiaId2, {kKey2}, /*last_key_version=*/8);
   // Keys for |kGaiaId2| overriden, so |kKey2| should be lost.
-  client_.StoreKeys(kGaiaId2, {kKey3, kKey4});
+  client_.StoreKeys(kGaiaId2, {kKey3, kKey4}, /*last_key_version=*/9);
 
   // Wait until the last write completes.
   WaitForFlush();
@@ -151,8 +151,10 @@ TEST_F(FileBasedTrustedVaultClientTest, ShouldStoreKeys) {
   EXPECT_TRUE(proto.ParseFromString(decrypted_content));
   ASSERT_THAT(proto.user_size(), Eq(2));
   EXPECT_THAT(proto.user(0).key(), ElementsAre(KeyMaterialEq(kKey1)));
+  EXPECT_THAT(proto.user(0).last_key_version(), Eq(7));
   EXPECT_THAT(proto.user(1).key(),
               ElementsAre(KeyMaterialEq(kKey3), KeyMaterialEq(kKey4)));
+  EXPECT_THAT(proto.user(1).last_key_version(), Eq(9));
 }
 
 TEST_F(FileBasedTrustedVaultClientTest, ShouldFetchPreviouslyStoredKeys) {
@@ -162,8 +164,8 @@ TEST_F(FileBasedTrustedVaultClientTest, ShouldFetchPreviouslyStoredKeys) {
   const std::vector<uint8_t> kKey2 = {1, 2, 3, 4};
   const std::vector<uint8_t> kKey3 = {2, 3, 4};
 
-  client_.StoreKeys(kGaiaId1, {kKey1});
-  client_.StoreKeys(kGaiaId2, {kKey2, kKey3});
+  client_.StoreKeys(kGaiaId1, {kKey1}, /*last_key_version=*/0);
+  client_.StoreKeys(kGaiaId2, {kKey2, kKey3}, /*last_key_version=*/1);
 
   // Wait until the last write completes.
   WaitForFlush();

components/sync/driver/mock_sync_service.h

@@ -57,9 +57,10 @@ class MockSyncService : public SyncService {
   MOCK_METHOD1(TriggerRefresh, void(const ModelTypeSet& types));
   MOCK_METHOD1(DataTypePreconditionChanged, void(syncer::ModelType type));
   MOCK_METHOD1(SetInvalidationsForSessionsEnabled, void(bool enabled));
-  MOCK_METHOD2(AddTrustedVaultDecryptionKeysFromWeb,
+  MOCK_METHOD3(AddTrustedVaultDecryptionKeysFromWeb,
                void(const std::string& gaia_id,
-                    const std::vector<std::vector<uint8_t>>& keys));
+                    const std::vector<std::vector<uint8_t>>& keys,
+                    int last_key_version));
   MOCK_METHOD1(GetUserNoisedBirthYearAndGender,
                UserDemographicsResult(base::Time now));
 

components/sync/driver/profile_sync_service.cc

@@ -1789,8 +1789,10 @@ void ProfileSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {
 
 void ProfileSyncService::AddTrustedVaultDecryptionKeysFromWeb(
     const std::string& gaia_id,
-    const std::vector<std::vector<uint8_t>>& keys) {
-  sync_client_->GetTrustedVaultClient()->StoreKeys(gaia_id, keys);
+    const std::vector<std::vector<uint8_t>>& keys,
+    int last_key_version) {
+  sync_client_->GetTrustedVaultClient()->StoreKeys(gaia_id, keys,
+                                                   last_key_version);
 }
 
 UserDemographicsResult ProfileSyncService::GetUserNoisedBirthYearAndGender(

components/sync/driver/profile_sync_service.h

@@ -139,7 +139,8 @@ class ProfileSyncService : public SyncService,
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
   void AddTrustedVaultDecryptionKeysFromWeb(
       const std::string& gaia_id,
-      const std::vector<std::vector<uint8_t>>& keys) override;
+      const std::vector<std::vector<uint8_t>>& keys,
+      int last_key_version) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
   void AddObserver(SyncServiceObserver* observer) override;

components/sync/driver/sync_service.h

@@ -371,9 +371,12 @@ class SyncService : public KeyedService {
 
   // Processes trusted vault encryption keys retrieved from the web. Unused and
   // ignored on platforms where keys are retrieved by other means.
+  // |last_key_version| represents the key version of the last element in
+  // |keys| (unused if empty).
   virtual void AddTrustedVaultDecryptionKeysFromWeb(
       const std::string& gaia_id,
-      const std::vector<std::vector<uint8_t>>& keys) = 0;
+      const std::vector<std::vector<uint8_t>>& keys,
+      int last_key_version) = 0;
 
   //////////////////////////////////////////////////////////////////////////////
   // USER DEMOGRAPHICS

components/sync/driver/sync_service_crypto.cc

@@ -44,7 +44,8 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
   }
 
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::vector<uint8_t>>& keys) override {
+                 const std::vector<std::vector<uint8_t>>& keys,
+                 int last_key_version) override {
     // Never invoked by SyncServiceCrypto.
     NOTREACHED();
   }

components/sync/driver/sync_service_crypto_unittest.cc

@@ -101,7 +101,8 @@ class TestTrustedVaultClient : public TrustedVaultClient {
   }
 
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::vector<uint8_t>>& keys) override {
+                 const std::vector<std::vector<uint8_t>>& keys,
+                 int last_key_version) override {
     gaia_id_to_keys_[gaia_id] = keys;
     observer_list_.Notify();
   }
@@ -194,7 +195,8 @@ TEST_F(SyncServiceCryptoTest,
   // engine (i.e. before SetSyncEngine()).
   crypto_.OnTrustedVaultKeyRequired();
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys,
+                                  /*last_key_version=*/0);
 
   // Trusted vault keys should be fetched only after the engine initialization
   // is completed.
@@ -233,7 +235,8 @@ TEST_F(SyncServiceCryptoTest,
   EXPECT_CALL(reconfigure_cb_, Run(_)).Times(0);
   ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys,
+                                  /*last_key_version=*/0);
 
   // Mimic the engine determining that trusted vault keys are required.
   crypto_.SetSyncEngine(kSyncingAccount, &engine_);
@@ -298,7 +301,8 @@ TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
 
   ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys,
+                                  /*last_key_version=*/0);
 
   // Mimic the engine determining that trusted vault keys are required.
   crypto_.SetSyncEngine(kSyncingAccount, &engine_);
@@ -339,7 +343,8 @@ TEST_F(SyncServiceCryptoTest, ShouldRefetchTrustedVaultKeysWhenChangeObserved) {
   const std::vector<std::vector<uint8_t>> kNewKeys = {{0, 1, 2, 3, 4},
                                                       {2, 3, 4, 5}};
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys,
+                                  /*last_key_version=*/0);
 
   // The engine replies with OnTrustedVaultKeyAccepted() only if |kNewKeys| are
   // provided.
@@ -364,7 +369,8 @@ TEST_F(SyncServiceCryptoTest, ShouldRefetchTrustedVaultKeysWhenChangeObserved) {
 
   // Mimic keys being added to the vault, which triggers a notification to
   // observers (namely |crypto_|), leading to a second fetch.
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys,
+                                  /*last_key_version=*/1);
   EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
   EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
   EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
@@ -382,7 +388,8 @@ TEST_F(SyncServiceCryptoTest,
   const std::vector<std::vector<uint8_t>> kNewKeys = {{0, 1, 2, 3, 4},
                                                       {2, 3, 4, 5}};
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys,
+                                  /*last_key_version=*/0);
 
   // The engine replies with OnTrustedVaultKeyAccepted() only if |kNewKeys| are
   // provided.
@@ -404,7 +411,8 @@ TEST_F(SyncServiceCryptoTest,
 
   // While there is an ongoing fetch, mimic keys being added to the vault, which
   // triggers a notification to observers (namely |crypto_|).
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys,
+                                  /*last_key_version=*/1);
 
   // Because there's already an ongoing fetch, a second one should not have been
   // triggered yet and should be deferred instead.
@@ -437,7 +445,8 @@ TEST_F(
   const std::vector<std::vector<uint8_t>> kLatestKeys = {
       {0, 1, 2, 3, 4}, {2, 3, 4, 5}, {3, 4}};
 
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys,
+                                  /*last_key_version=*/0);
 
   // The engine replies with OnTrustedVaultKeyAccepted() only if |kLatestKeys|
   // are provided.
@@ -461,12 +470,14 @@ TEST_F(
 
   // Mimic keys being added to the vault, which triggers a notification to
   // observers (namely |crypto_|), leading to a second fetch.
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kIntermediateKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kIntermediateKeys,
+                                  /*last_key_version=*/1);
   EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
 
   // While the second fetch is ongoing, mimic more keys being added to the
   // vault, which triggers a notification to observers (namely |crypto_|).
-  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kLatestKeys);
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kLatestKeys,
+                                  /*last_key_version=*/2);
 
   // Because there's already an ongoing fetch, a third one should not have been
   // triggered yet and should be deferred instead.

components/sync/driver/test_sync_service.cc

@@ -306,7 +306,8 @@ void TestSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {}
 
 void TestSyncService::AddTrustedVaultDecryptionKeysFromWeb(
     const std::string& gaia_id,
-    const std::vector<std::vector<uint8_t>>& keys) {}
+    const std::vector<std::vector<uint8_t>>& keys,
+    int last_key_version) {}
 
 UserDemographicsResult TestSyncService::GetUserNoisedBirthYearAndGender(
     base::Time now) {

components/sync/driver/test_sync_service.h

@@ -108,7 +108,8 @@ class TestSyncService : public SyncService {
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
   void AddTrustedVaultDecryptionKeysFromWeb(
       const std::string& gaia_id,
-      const std::vector<std::vector<uint8_t>>& keys) override;
+      const std::vector<std::vector<uint8_t>>& keys,
+      int last_key_version) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
 

components/sync/driver/trusted_vault_client.h

@@ -56,7 +56,8 @@ class TrustedVaultClient {
   // keys, so callers may not assume that later calls to FetchKeys() would
   // necessarily return the keys passed here.
   virtual void StoreKeys(const std::string& gaia_id,
-                         const std::vector<std::vector<uint8_t>>& keys) = 0;
+                         const std::vector<std::vector<uint8_t>>& keys,
+                         int last_key_version) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TrustedVaultClient);

components/sync/protocol/local_trusted_vault.proto

@@ -19,6 +19,9 @@ message LocalTrustedVaultPerUser {
 
   // All keys known for a user.
   repeated LocalTrustedVaultKey key = 2;
+
+  // The version corresponding to the last element in |key|.
+  optional int32 last_key_version = 3;
 }
 
 message LocalTrustedVault {